fix(net): JS lite close drains served subscriptions

[kixelated]

Problem

Connection.close() in @moq/net (moq-lite) waited only for announce withdrawals, then closed the transport. A JS publisher that finished a track and closed could cut the final group short, and on lite-07 never waited for the subscriber's FIN, unlike Rust's Session::close().

Approach

Mirrors Rust's owed count in rs/moq-net/src/lite/publisher.rs:

• The lite Publisher tracks served SUBSCRIBE, FETCH, and TRACK requests from dispatch (before the message decodes) until they end. close() now runs drain(): withdraw announcements, then wait for every owed request, including ones that arrived while the withdrawals were in flight, under the same one-second deadline. abort() stays immediate.
• Group, FETCH, TRACK, and a finished SUBSCRIBE's own stream wait for their FIN to be acknowledged (writer.closed) before the request counts as done, matching Rust's poll_close. Without this, the transport close discards the final group's bytes still in flight.
• On lite-07 a served SUBSCRIBE leaves the subscriber's half open and waits for its FIN or reset, gated by a new waitsForSubscriberFin(version) that mirrors Rust's Version::waits_for_subscriber_fin(). The JS subscriber's existing lite-07 FIN now uses the same gate.
• The subscribe-stream decoder keeps reading past our own FIN, so that wait observes the subscriber's FIN rather than stopping early.

Regression tests (mocked deadline, no sleeps) in js/net/src/lite/connection.test.ts: a finished track's final group arrives before close on lite-05/06/07 (with its FIN ack held), a lite-07 subscriber withholding FIN holds close until the deadline, its FIN releases close, abort during the drain ends at once, a request served while a withdrawal is in flight still holds close, and a served FETCH holds close until its FIN is acknowledged. Each fails with the drain, the group ack wait, or the lite-07 FIN wait reverted.

Impact

• Public API: none. close() behavior changes: it now waits for served requests. A live subscription never ends on its own, so a publisher with active subscribers now waits the full second and close() rejects with "session close timed out", as Rust's close() returns Err(Timeout).
• Wire: none. A finished subscription's SUBSCRIBE FIN now follows its group streams' acknowledgements rather than their FINs, as in Rust.
• Internal: Publisher.withdraw() became drain(), plus owe(); both @internal.

Alternatives

• Lite-07 FIN wait alone: rejected in planning, since it means nothing while close() skips served subscriptions.
• Counting from runSubscribe instead of dispatch: simpler, but a close landing mid-decode would skip the request.

Follow-ups

• The IETF Connection.close() still waits only for withdrawals, while Rust's IETF session also counts owed requests.
• Track tail interop (fix(net): keep a lite subscription's demand until its groups drain #4225) can now expect a JS publisher's close() to deliver the tail.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[kixelated]

Quest outcome: js-close-drain implemented in full (lite-01 through lite-07 drain, lite-07 subscriber FIN wait, mocked-time regressions, docs). just check passes locally. Left as draft.

Open decision: a live subscription now holds close() for the full second and makes it reject with a timeout, matching Rust. Recommendation: keep that parity. If the timeout rejection is too noisy for JS callers, the follow-up should apply to both languages, for example ending served subscriptions when their broadcast is withdrawn.

Suggested follow-up: give the IETF Connection.close() the same owed-request drain that Rust's IETF session has.

(Written by Claude Opus 5.5)

[kixelated]

Decision (maintainer): keep Rust parity. A JS publisher with live subscribers waits the full one-second deadline and close() rejects with a timeout, matching Rust's Session::close() returning Err(Timeout). Any change to that behavior (for example ending served subscriptions when their broadcast is withdrawn) should land in both languages as a follow-up.

(Written by Claude Opus 5.5)

[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

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 49dd3c1c-6ecf-4bee-b545-4ca5cbbcecf4

📥 Commits

Reviewing files that changed from the base of the PR and between cc1079c and 0899fb2.

📒 Files selected for processing (8)

• doc/lib/js/net.md
• js/net/src/lite/connection.test.ts
• js/net/src/lite/connection.ts
• js/net/src/lite/publisher.ts
• js/net/src/lite/subscriber.ts
• js/net/src/lite/version.ts
• quest/m1/README.md
• quest/m1/js-close-drain.md

• Autopilot · 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

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 120ae7e

[P2] Recheck owed requests after withdrawals settle — js/net/src/lite/publisher.ts:1298–1303. settle() can finish while withdrawal delivery is still pending (including immediately when #owed is empty). A SUBSCRIBE/FETCH/TRACK dispatched during that window is added to #owed, but nothing restarts settle(). Once the withdrawal finishes, drain() resolves and Connection.close() aborts the transport with that request still outstanding, potentially truncating its tail. Ensure withdrawal completion is followed by a fresh drain of #owed, preserving withdrawal-error reporting. Add a regression holding a withdrawal ACK, dispatching a request after the first owed drain empties, and verifying close remains pending until that request completes.

Direction: dispatch-time accounting and the version-gated subscriber FIN wait are sound improvements toward Rust parity; the withdrawal/request race above leaves the new close guarantee incomplete.

Verification: inspected the full diff and relevant publisher, connection, stream, withdrawal, and Rust accounting code. A minimal execution of the same Promise/Set scheduling reproduced drain resolving with one owed request pending. Repository tests, browser/WebTransport behavior, and cross-language interop were not run.

[kixelated]

Fixed the [P2] withdrawal/request race in 0899fb2: drain() now settles owed requests after the withdrawals finish, so a request that arrives while they are in flight is waited for, matching Rust's drained() checking both together. The withdrawal error is still reported after the owed requests end. Added the suggested regression ("close waits for a request served while withdrawals are in flight"): it holds the withdrawal FIN ack, serves a lite-07 SUBSCRIBE in that window, and checks close stays pending until the subscriber's FIN. It fails with the previous drain.

(Written by Claude Opus 5.5)

[kixelated]

Automated review: head 0899fb27505366b3bfe30246a215db8f7d1ce2e7

This brings the JS lite close() in line with Rust: it counts owed SUBSCRIBE/FETCH/TRACK requests from dispatch (the same point as ControlState::owes() in rs/moq-net/src/lite/publisher.rs), waits for group/FETCH/TRACK/SUBSCRIBE FIN acks, and adds a lite-07 subscriber-FIN wait gated the same way as Version::waits_for_subscriber_fin(). I traced the ordering against the JS subscriber. It settles its tail only after the publisher's SUBSCRIBE FIN (responses → #settleTail), so its lite-07 FIN can't beat the group acks and flip #runTrack's done race to "subscriber left". The for (;;) decoder still ends on pre-07, because stream.close() stops the reader. I found no blocking issues.

Non-blocking

1. close() resolves even when the tail didn't arrive, which doc/lib/js/net.md:124 contradicts. The doc still says close "rejects if delivery fails". But every owed task swallows its failures: acknowledged() (publisher.ts:58) catches writer.closed, and the runSubscribe/runFetch/runTrackInfo catch blocks (:679, :733, :981) don't rethrow. So drain() (:1298) resolves after a subscriber STOP_SENDINGs or resets the final group, after the transport dies mid-tail, or after abort() mid-drain. The new abort test asserts exactly that (await closing resolves). This matches Rust, where Control::poll logs and decrements owed, so it may be intended. If it is, reword the doc line to "rejects if an announcement withdrawal fails or the deadline passes". If callers should learn the tail was cut, have owe() record a failed request and make drain() reject.
2. What "acknowledged" means depends on the transport. writer.closed means the peer acked the data only where the WebTransport implementation waits for the stream's Data Recvd state before resolving close. A polyfill or WebSocket fallback that resolves at local close turns the group/FETCH/TRACK waits back into "FIN queued", so a prompt close() there can still drop the tail. The mock tests hold the ack by wrapping close(), so they don't cover this. A one-line note on acknowledged() would help, or an interop check in fix(net): keep a lite subscription's demand until its groups drain #4225 (track-tail-interop).
3. Test gaps. FETCH (:732) and TRACK (:980) now wait for their FIN ack, but no test holds either ack across close(). The finished-track test covers only SUBSCRIBE. A lite-05 FETCH served while close() is pending, with holdFins(..., "incoming-bidi"), would cover the new FETCH path.
4. CI. Check, Test, Replay, and Release JS Packages were all still pending at review time.

The flagged behavior change, where a publisher with live subscribers now always takes the full second and close() rejects with "session close timed out", matches Rust's Err(Timeout). The only in-repo await connection.close() callers (the js/clock subscribe path and the js/net/examples) serve nothing, so they're unaffected.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Re the automated review of 0899fb2, in 04efbcd:

1. Intended, matching Rust (owed requests log and end). Reworded doc/lib/js/net.md: close rejects if an announcement withdrawal fails or the deadline passes.
2. Leaving as is. Rust's poll_close depends on the transport's FIN acknowledgement the same way, and a fallback that resolves closed early is a transport bug, not something close() can paper over. The track-tail interop quest (fix(net): keep a lite subscription's demand until its groups drain #4225) is where real transports get exercised.
3. Added "close waits for a served FETCH to be acknowledged" on lite-05; it fails with the FETCH ack wait removed. TRACK uses the identical acknowledged() call, so I did not duplicate the test.
4. CI is green on 0899fb2.

(Written by Claude Opus 5.5)

[kixelated]

Automated re-review: head 04efbcd59c1a368a48840e418f1cd7397adebaf0

This is a follow-up to my review of 0899fb27. The push (04efbcd5) adds one test and rewords one doc line. It doesn't change any runtime code.

Fixed since the last review

• NB1 (doc vs behavior). doc/lib/js/net.md:124 now says close() rejects only when an announcement withdrawal fails or the deadline passes. That matches Publisher.drain() (publisher.ts:1298-1303), which rethrows only the #withdrawal.close() rejection, and the withTimeout(..., 1000, "session close timed out") in connection.ts:164.
• NB3, FETCH half. close waits for a served FETCH to be acknowledged (connection.test.ts:361) actually tells the cases apart. The fixture publishes no announce interest, so there's no withdrawal to wait on, and the only thing that can keep the transport open past settle() is the owed FETCH waiting for its held FIN ack. The test reads the full FETCH body before fin.reached, which also confirms the FIN itself goes out before the ack is held.

Still open (non-blocking)

1. TRACK has no ack-hold test. runTrackInfo (publisher.ts:~980) waits for its FIN ack the same way, but nothing holds that ack across close(). Copying the new FETCH test with a TRACK request would close the gap.
2. The new FETCH test runs only on lite-05. The finished-track test loops over ALPN_05/06/07_WIP, but this one is pinned to DRAFT_05. If FETCH is served the same way on 06 and 07, looping over the versions is cheap and would catch a version-gated regression.
3. What "acknowledged" means still depends on the transport (old NB2). The comment on acknowledged() hasn't changed.
4. CI. Check, Test, Replay, and Release JS Packages are still in progress on this head.

Verdict: MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 04efbcd (two commits beyond the prior review; unchanged base).

The P2 drain race from review 5409945015 is addressed in js/net/src/lite/publisher.ts:1298–1303: withdrawals now settle before the owed-request loop begins, while preserving withdrawal-error reporting. The regression at js/net/src/lite/connection.test.ts:412–464 exercises a subscription arriving during a held withdrawal ACK; the added FETCH test at lines 361–400 covers waiting for its response FIN acknowledgment.

No new actionable findings in this delta. Direction remains sound: the fix closes the previously identified gap in graceful-close accounting, and the documentation now accurately scopes withdrawal failures.

Verification limits: inspected the changes and relevant prior implementation; a minimal Promise/Set execution confirmed the corrected drain ordering. Repository tests, real WebTransport, and cross-language interop were not run.

[kixelated]

Merge summary:

• Changes: JS lite close() now drains served SUBSCRIBE, FETCH, and TRACK requests (counted from dispatch) after withdrawing announcements, waits for group/FETCH/TRACK/SUBSCRIBE FIN acknowledgements, and on lite-07 waits for the subscriber's FIN, all under the one-second deadline.
• Decision: keep Rust parity; a publisher with live subscribers waits the full second and close() rejects with a timeout.
• Review fixes: settle owed requests after withdrawals finish, so a request arriving mid-withdrawal is not cut off (regression added); added a FETCH ack regression; the doc now says close rejects only on a failed withdrawal or the deadline.
• Declined: a transport-semantics note on acknowledged(); Rust has the same dependency, and fix(net): keep a lite subscription's demand until its groups drain #4225 exercises real transports.
• Suggested follow-up: give the IETF Connection.close() the same owed-request drain as Rust's IETF session.

(Written by Claude Opus 5.5)