[kixelated]

Problem

closing the loop keeps the tokens until the session drains (js/net/src/connection/reload.test.ts) failed under multi-file load: fin.closed was already true after a 999 ms clock advance. It was quarantined in moq-dev/moq#7764 / moq-dev/moq#7884.

The cause is a production race in the lite-07 publisher (js/net/src/lite/publisher.ts), not the fixture:

• A lite-07 subscriber FINs its subscribe stream once it read the finished track's tail.
• That FIN can land while a reply write is still pending. The write's bytes already reached the peer, but Writer.write is still yielding on the shared write budget. The reply can be SUBSCRIBE_FINISH, or a SUBSCRIBE_DROP for a superseded group.
• SubscriptionControls.response() then reports the peer as gone, and the serving loop returns.
• Only the drained() exit owed the subscription's opened group streams to the session's drain. The other exits owed nothing, so close()'s drain finished at once and aborted the transport while the final group's FIN was still in flight.

The shared fin.hold/fin.closed hypothesis from the quest's Plan is refuted: the captured failure had one dial, and the client closed from close()'s own .finally(abort).

Approach

• The serving loop's finally now owes the opened group streams on every lite-07 exit, replacing the one-branch special case. Groups still queued for a slot are cancelled as before. A reader that wants none of them resets them.
• Added a deterministic lite-07 test to lite/connection.test.ts. The subscribe stream's writes deliver their bytes, then resume a host task later, as a budget yield does. The subscriber's FIN wins the race, and the close must still wait for the final group's held FIN.
• Restored the quarantined reload test unchanged, apart from removing test.skip.
• Recorded the cause in the epic and deleted the quest.

Impact

• Public API: none. Wire: none.
• Behavior: on lite-07, a draining close now waits for a subscription's opened group streams however the subscription ended, bounded by the existing one-second close deadline.

Validation

• Reproduced before the fix: 6 concurrent full js/net suites with --rerun-each 10 under quest gate, with per-dial and close-stack diagnostics added temporarily. The reload test failed once. The diagnostics showed 1 dial and the client closed at clock 0 by close()'s abort.
• Deterministic reproduction: a scratch preload forcing every Budget.poll() to yield failed the reload test 10 of 10. With the setup group read through FIN, a single forced yield still failed at 1 of 140 positions (position 103: SUBSCRIBE_FINISH's write).
• The other drain cases (teardown by …, a URL change …, a retired GOAWAY predecessor …): no failure under all-yield (40 runs) or at any of 700 single-yield positions, which covers every budget poll they make. No change needed.
• After the fix: the reload test passes 10 of 10 under all-yield and at all 140 single-yield positions. The new test passes 20 of 20 and fails 20 of 20 with the fix reverted.
• Repeated multi-file load after the fix: the same 6 × --rerun-each 10 full js/net load ran about 30,500 tests per run. No reload drain case or new test failed. The other failures, all pre-existing:
  • Five tests fail on every repeat from --rerun-each state leakage, also noted in the quest's earlier attempts: WebSocket preference ×3, the benchmark Chromium launch, and the WebTransport head start.
  • lite-07 answers a bounded window that resolves empty and FINs in reply failed 2 times. It also fails 10 of 10 on base under forced yields (see Follow-ups).
  • moq-lite-05/moq-lite-06: a session close right after a datagram track's end keeps the end failed once each. These versions are outside this change's keepsSubscribeOpen gate.
• just ci origin/main: passed.

Alternatives

• Fix only the fixture (read the setup group through FIN, as moq-dev/moq#7658 did for lite/connection.test.ts): rejected. The forced-yield sweep still fails with a cleanly finished track, so the race is in the publisher.
• Make response() wait out a write that is only yielding: it can't tell a budget yield from a write blocked on flow control by a peer that stopped reading. Resetting the latter is what bounds what a peer can hold.

Choices made without asking

• Kept the reload fixture unchanged, including its unfinished setup group, which ends the track errored. It passes with the fix, and it also covers the SUBSCRIBE_DROP variant of the race.
• Did not keep the per-dial diagnostics: the cause is established and they would add weight to every run.
• Owing stays gated to lite-07 (keepsSubscribeOpen), matching the previous special case. Earlier versions end the subscription with a reset.

Follow-ups

• response() resets a reply whose bytes already went out. When a lite-07 subscriber FINs while Writer.write is still in its post-write budget yield, SubscriptionControls.response() resets the stream with Cancel. The peer then sees remote error: 1 instead of a clean reply FIN. This is the cause of lite-07 answers a bounded window that resolves empty and FINs in reply failing under load, and it reproduces on base. A fix likely moves the budget yield out of the write's completion, for example by yielding before the next write, so a pending write means the transport has not taken it.

Review

Codex, gpt-6.1-sol, medium effort, quest review --base origin/main, session 01a1b297-7d43-7aa0-9bf5-73eff94e10da, round 1, head 2abbf3f1a496268d5f4b635c5daf0efd14c116e8 against 4d51f9c051fdb202594a04fcd4e6208f8463c495: no findings.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 12d67c6d-b857-4411-9aed-d5dc92245967

Commits

Reviewing files that changed from the base of the PR and between 45930a7 and 2abbf3f.

Files selected for processing: 5

• js/net/src/connection/reload.test.ts
• js/net/src/lite/connection.test.ts
• js/net/src/lite/publisher.ts
• quest/m0/test-flakes-2/README.md
• quest/m0/test-flakes-2/reload-close-drain.md

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head: 2abbf3f

No actionable correctness findings in the five-file diff. Moving the lite-07 group-drain obligation into #runTrack's finally is the right direction: it covers exits from response writes and control handling as well as drained(). The remaining group tasks are registered before the already-owed subscription request can settle, so Publisher.drain() cannot observe a gap between those obligations. Queued opens still receive unsubscribe(), opened streams retain their completion tracking, and the existing one-second session-close deadline remains in force.

The new test exercises delivered reply bytes with delayed write completion while holding the final group's FIN acknowledgement; the reload regression is re-enabled without weakening its assertions. I also checked the surrounding request tracking, group completion, priority cleanup, and connection-close paths.

Limits: GitHub-only static review; no Bun tests, browser/transport runs, or load experiments were executed by this review. Check CI was still in progress when inspected. The separately documented response-reset race is not fixed by this change.

[pullfrog]

✅ No new issues found.

Reviewed changes

• Lite-07 group drain on every exit — #serveSubscribe's serve-loop finally now owes the opened group streams to the session drain for lite-07 (keepsSubscribeOpen(this.version) && groups.size > 0), replacing the special case that only ran on the drained() exit. Groups queued for a slot are still cancelled by unsubscribe() when finished is false, and the one-second close deadline still bounds the wait.
• Deterministic regression test — the new lite-07 case in js/net/src/lite/connection.test.ts holds the final group's FIN acknowledgement while a yieldReplies helper makes every reply write deliver its bytes and then yield, so the subscriber's FIN lands mid-write; it asserts close() is still pending with the group FIN unreleased.
• Restored reload test — closing the loop keeps the tokens until the session drains loses only its test.skip, assertions unchanged.
• Quest bookkeeping — cause recorded in the epic README, its ## Required bullet removed, and the child quest file deleted. No references to the deleted file remain.

Verification I ran in this checkout: with the finally owe removed the new test fails at expect(closed).toBe(false), with it the test passes 30/30 repeat runs; bun test src/lite/ is 702 pass, src/connection/reload.test.ts 44 pass, src/lite/publisher.test.ts 132 pass; biome check and tsc --noEmit are clean. The gating also checks out against the version semantics — pre-lite-07 subscribers only FIN after the publisher's FIN, so no reply write can be pending there, and the change brings JS level with rs/moq-net's Drain state. The response() reset race the body lists as a follow-up is pre-existing and out of scope here.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}