fix(net): skip the empty namespace for foreign draft-14 and draft-15 peers

[kixelated]

Problem

An unscoped subscriber asks for every namespace with an empty SUBSCRIBE_NAMESPACE. That prefix was illegal before draft-16: the track namespace tuple had a minimum of one field, and a zero-field prefix was a protocol violation. Draft-16 made the empty prefix legal (moq-transport#1393, from moq-transport#1346).

Rust offers moq-00, so a peer that prefers draft-14 (moxygen does) negotiates 14. Moxygen then rejects the request with NAMESPACE_PREFIX_UNKNOWN (code 16) reason empty, and the session continues with no announcements. The same request is what the Cloudflare draft-14 cells sit on. JavaScript hits the same drafts when a peer selects 14.

Approach

On draft-14 and draft-15, do not send SUBSCRIBE_NAMESPACE for an empty prefix when the peer's SETUP declared no MoQ Solicit. Such a peer is foreign: it may enforce the draft, and it announces unasked anyway. A peer that declared Solicit is ours (moq-net, js/net, moq-relay): it accepts the empty prefix and only announces when asked, so it still gets the request. Skipping it there left two of our endpoints discovering nothing.

Rust decides in run_subscribe_namespace, after the peer's SETUP is read. JavaScript passes the peer's Solicit declaration to the subscriber, and on a skip keeps the announce consumer registered until the caller closes it, so an unsolicited PUBLISH_NAMESPACE still lands.

✅ maintainer 2026-10-08: skip only for peers that declared no Solicit.

Impact

• No public API change.
• Wire: on draft-14 and draft-15, an unscoped subscriber no longer sends an empty SUBSCRIBE_NAMESPACE to a peer that declared no Solicit. Peers that declared Solicit, named prefixes, and draft-16+ are unchanged.

Tests

• an_empty_namespace_is_not_asked_of_a_foreign_peer_before_draft_16 (Rust) and the matching JS tests cover: foreign peer skipped, soliciting peer asked, named prefix asked, draft-16 asked.
• Two moq-net peers on draft-14/15 still discover each other: announce_to_serve::remote_ietf_consumer_sees_what_a_local_one_does, broadcast_close::close_ends_a_remote_consumer, and datagram::ietf_delivers_datagrams fail without the Solicit check.

Alternatives

• Skip for every peer on draft-14/15. Breaks discovery between our own endpoints, which declare Solicit.
• Stop declaring Solicit on draft-14/15. The client writes SETUP before the version is negotiated, and it gives up one discovery path per namespace.
• Stop offering moq-00. Drops every peer that only speaks draft-14.

Follow-ups

• A foreign relay that only fans out announcements to a successful SUBSCRIBE_NAMESPACE still has nothing legal to key them on before draft-16. moxygen announces nothing unasked on draft-14, so a d14 link to it discovers no broadcasts; doc/bin/relay/cluster.md now says so.

Completes and deletes quest/m0/ietf-d14-root-prefix.md. Supersedes #5024.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit 0406026dc9c09344e3a425273786d0a25d4bd11c.

No actionable correctness issue found in the changed namespace-subscription paths and tests. Filtering the root prefix before either Rust driver arm and keeping the TypeScript announce consumer alive preserve unsolicited discovery while retaining named prefixes and draft-16 behavior.

Direction: this is the broader draft-14/15 alternative to #5024; choose/reconcile the overlap before landing rather than applying both independently.

Verification: static diff and session/lifecycle review only; no tests or external-stack interop were run. The reported base-branch test-compilation limitation was not independently reproduced.

[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 11 minutes.

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: 104ab0f7-e561-400e-b906-55786557b7da

📥 Commits

Reviewing files that changed from the base of the PR and between 0a6a263 and da29e6b.

📒 Files selected for processing (9)

• doc/bin/relay/cluster.md
• js/net/src/goaway-requests.test.ts
• js/net/src/ietf/connection.ts
• js/net/src/ietf/subscriber.test.ts
• js/net/src/ietf/subscriber.ts
• quest/m0/README.md
• quest/m0/ietf-d14-root-prefix.md
• rs/moq-net/src/ietf/session.rs
• rs/moq-net/src/ietf/subscriber.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 46b169f6-61f2-4557-97be-2cd5f53ca3f5

📥 Commits

Reviewing files that changed from the base of the PR and between d520823 and 0a6a263.

📒 Files selected for processing (8)

• doc/bin/relay/cluster.md
• js/net/src/ietf/connection.ts
• js/net/src/ietf/subscriber.test.ts
• js/net/src/ietf/subscriber.ts
• quest/m0/README.md
• quest/m0/ietf-d14-root-prefix.md
• rs/moq-net/src/ietf/session.rs
• rs/moq-net/src/ietf/subscriber.rs

💤 Files with no reviewable changes (2)

• quest/m0/ietf-d14-root-prefix.md
• quest/m0/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

---

Walkthrough

For draft-14/15 peers that did not declare MoQ Solicit, JavaScript and Rust subscribers now skip empty-prefix namespace requests. The JavaScript subscriber remains available for unsolicited announcements until the consumer or session closes. Named-prefix requests and empty-prefix requests in other covered cases remain supported. Tests and documentation describe these behaviors.

Fixed issue severity: <fixed_issue_severity>Medium</fixed_issue_severity>

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 0a6a2

No actionable merge-blocking issue remains in the reviewed change; proceed with normal checks.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: skipping empty namespace requests for foreign draft-14 and draft-15 peers.
Description check	✅ Passed	The description is directly related to the changeset. It explains the protocol issue, implementation approach, impact, tests, and follow-ups.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 5 files. (1 skipped: 1 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 2

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch fix/ietf-empty-namespace-pre16

✨ Simplify code

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

• 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]

Merged main (picks up #5016, so the moq-net tests compile again). With that, three integration tests fail on this branch and pass on main:

• moq-net::datagram ietf_delivers_datagrams (moq-transport-14 times out)
• moq-net::broadcast_close close_ends_a_remote_consumer (moq-transport-14 announce timeout)
• moq-net::announce_to_serve remote_ietf_consumer_sees_what_a_local_one_does (broadcast is unroutable)

Cause: every moq-net and js/net session declares MoQ Solicit (SOLICIT=1, sent in CLIENT_SETUP before the version is negotiated). A peer that declares it is never told unsolicited, so it must ask. On draft-14/15 this PR stops the unscoped subscriber from asking, so two of our own endpoints (including a relay link) discover nothing. The unsolicited PUBLISH_NAMESPACE this PR relies on only comes from peers that did not declare Solicit, such as moxygen.

Holding the merge until the approach is settled.

(Written by Claude Opus 5.5)

[kixelated]

Automated review: #5018 at c2581668

Full review (first Grok pass). The head adds only a merge of origin/main on top of 0406026d, so the PR's own diff is still the four files. The spec premise holds: draft-14 §2.4 and draft-15 §2.4.1/SUBSCRIBE_NAMESPACE both require 1 to 32 namespace fields, and draft-16 is the first to allow 0 to 32 for the SUBSCRIBE_NAMESPACE prefix. The problem is which peers stop getting asked.

Blocking

1. Unscoped draft-14/15 links between our own endpoints now discover nothing.
Every moq-net and js/net session declares MoQ Solicit unconditionally (rs/moq-net/src/ietf/session.rs:664 calls solicit::into_setup, which writes SOLICIT=1 on every draft; js/net/src/ietf/solicit.ts:46 does the same). A publisher that sees that declaration skips its unsolicited loop: Publisher::run_publish_namespaces returns at publisher.rs:2146 when requires_solicitation(), and js publisher.ts:904 does the same. So when both ends are ours, the only way announcements flow is the SUBSCRIBE_NAMESPACE this PR stops sending. An unscoped client or relay link on moq-transport-14/-15 now gets no announcements at all. The unsolicited PUBLISH_NAMESPACE the PR relies on only comes from peers that declared nothing, such as moxygen. This matches the three integration tests your comment reports failing after the merge (datagram::ietf_delivers_datagrams, broadcast_close::close_ends_a_remote_consumer, announce_to_serve::remote_ietf_consumer_sees_what_a_local_one_does, all on moq-transport-14). The earlier CI run on 0406026d couldn't show it because the tests didn't compile then. CI on c2581668 is still pending.

Suggested fix: gate the skip on what the peer declared, not only on the version. Skip the empty prefix only when the peer's SETUP carried no Solicit option (peer_setup.solicit == None), which means it doesn't know the extension and will announce unasked. In Rust, the sub_ns_run block (session.rs:278) can await subscriber.solicit() once, the way the accept loop does at session.rs:975. The peer's SETUP always arrives first, so that costs no extra round. Then filter there instead of when namespaces is built at session.rs:139. js/net needs the same check: Connection already holds #solicit and can pass it into Subscriber for #runAnnounced. The catch is that two of our own endpoints on d14/15 would still exchange a zero-field prefix the draft forbids. They tolerate it today, so that's no regression, but it's worth a comment. The alternative is to stop declaring Solicit on d14/15 when the subscriber is unscoped, which pushes more change into the publish side.

Non-blocking

2. Doc comments become false. solicit.rs into_setup and solicit.ts solicitIntoSetup justify always declaring Solicit with "we send SUBSCRIBE_NAMESPACE for every prefix we are allowed to discover". After this change that's untrue on d14/15 for an unscoped origin. Fix 1 makes it true again for our own peers. Either way, update the wording.
3. Tests miss the case that broke. an_empty_namespace_is_not_asked_before_draft_16 and the JS test.each only run with peer_declared: None, or with no Solicit at all on the JS side. Add a case where the peer declared Some(true) and assert that the empty prefix is still asked on d14/15 (or that announcements still arrive), and a case for None where it's skipped. That pins down the fix and keeps the integration tests from being the only guard.
4. Overlap with fix(net): skip an empty SUBSCRIBE_NAMESPACE on draft-14 #5024 is gone. fix(net): skip an empty SUBSCRIBE_NAMESPACE on draft-14 #5024 (draft-14 only) is closed, so there's nothing to reconcile there.

Verdict: ITERATE. The draft-14/15 rule is right, but skipping by version alone cuts off discovery between our own Solicit-declaring endpoints. Gate the skip on the peer not declaring Solicit, then add a test for that case.

Reviewed head: c258166833cd6288be00699545de38aa95489b2c

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit c258166833cd6288be00699545de38aa95489b2c against the prior reviewed head 0406026dc9c09344e3a425273786d0a25d4bd11c, accounting for merged main.

[P1] Unscoped draft-14/15 discovery still loses its only announcement path (rs/moq-net/src/ietf/session.rs:139-145; js/net/src/ietf/subscriber.ts:319-321). The legacy handshakes advertise SOLICIT=1 (client.rs:356, server.rs:596, JS connection/connect.ts:307), so a cooperating publisher suppresses unsolicited announcements; the new filter also removes the subscription. Two moq endpoints therefore discover no broadcasts. I verified the source path and concur with the existing finding, without duplicating inline comments. This corrects my earlier no-actionable-issue conclusion; it is an existing PR defect, not a new bug introduced by the merge.

Direction: avoiding the illegal empty prefix is worthwhile, but the fallback and Solicit contract must agree. Coordinate SETUP/receive behavior with the skip and add an end-to-end draft-14/15 test with Solicit-aware peers; the added no-Solicit/mock tests do not exercise this case. Restoring an empty request only for extension-aware peers would preserve current interoperability while retaining the standards exception. The previous overlap concern is resolved: #5024 is closed.

Verification: GitHub-only static review. The three reported integration failures were not rerun; current Check CI is queued, so this is not a test-pass claim.

[kixelated]

Automated review: #5018 follow-up at b8424b3c

Re-review after a push. The new commit b8424b3c ("still ask a soliciting peer for the empty namespace") is the only PR change since c2581668. It replaces the version-only filter in session.rs with a check in Subscriber::run_subscribe_namespace (rs/moq-net/src/ietf/subscriber.rs:853-863), and adds the same check in js Subscriber.#runAnnounced (js/net/src/ietf/subscriber.ts:329-333) via a new solicit option threaded from Connection (connection.ts:165).

Earlier findings

1. Blocking, fixed. The empty prefix is now skipped on d14/15 only when the peer's SETUP carried no Solicit option (declared.solicit.is_none() / this.#solicit === undefined). Our own endpoints always write SOLICIT=1 (solicit.rs into_setup, solicit.ts solicitIntoSetup), so they get asked again. Keying on "declared nothing" instead of Some(true) is also right for Some(false): per solicit.rs:31-32, a peer that writes the option at all is held to our requirement and won't announce unasked, so it has to be asked too. The JS side reads the peer's declaration, not ours (connection.ts:68, :163), matching Rust.
2. Doc comments, effectively resolved. The into_setup / solicitIntoSetup claim that we ask for every prefix is true again for every peer that honors Solicit. The one exception, a foreign d14/15 peer, never reads the option anyway.
3. Tests, fixed. Rust an_empty_namespace_is_not_asked_of_a_foreign_peer_before_draft_16 now covers None (skipped), a named prefix (still asked), and Some(true) (asked). JS adds "%s still asks a soliciting peer for the empty namespace" for d14/15.

New in this push

No blocking issues. I checked the move from the session into run_subscribe_namespace:

• The early return comes after peer_setup.get().await, which the function already awaited for hidden, so it adds no new wait or hang.
• It now runs after the caller has already called Stream::open (session.rs:276, :422). On d14/15 that's a virtual stream (adapter.rs). Dropping it unwritten never registers a request ID, finish() with an empty buffer pushes nothing, and VirtualRecvStream::drop only closes its queue. So nothing reaches the wire, and no request ID or routing entry leaks.
• Returning Ok(()) takes the caller's quiet path, not the "subscribe_namespace failed" warn.

Non-blocking:

• Weak JS assertion. The new JS test only checks that nextStream(pair.client) produced a stream. It doesn't decode the message to confirm it's a SUBSCRIBE_NAMESPACE with a zero-field namespace. Decoding the first message, the way the named-prefix test checks its prefix, would pin it down.
• Spec note for the Solicit draft. Two Solicit-declaring endpoints on d14/15 will exchange a zero-field namespace that d14/15 forbid. That's fine between our own stacks, and the code comment (subscriber.rs:853-856) says so. A third-party implementer of draft-lcurley-moq-solicit on d14/15 would have to tolerate it too. A sentence in the extension draft would make that explicit.
• CI is still pending on b8424b3c. The three moq-transport-14 integration tests from the earlier report (datagram::ietf_delivers_datagrams, broadcast_close::close_ends_a_remote_consumer, announce_to_serve::remote_ietf_consumer_sees_what_a_local_one_does) are the real confirmation for this fix.

Verdict: MERGE once CI is green, especially those three integration tests.

Reviewed head: b8424b3c3699d5452ed47b0136ce2c52abcab93b

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit b8424b3c3699d5452ed47b0136ce2c52abcab93b, comparing the follow-up with c258166833cd6288be00699545de38aa95489b2c and the complete PR delta against its merged-main base.

Prior P1 fixed in source: both subscribers now retain the empty request for peers that declared Solicit, including explicit false/zero. I concur with the independent follow-up on that fix, without duplicating its findings.

One newly identified P2 remains at js/net/src/ietf/subscriber.ts:330-332: the skipped-request reader never ends when its session closes, leaving direct conn.announced() consumers waiting indefinitely (inline details). This was already present in the original skip branch, not introduced by this follow-up.

Direction: the peer-aware skip addresses the discovery regression; tie the foreign-peer announcement reader to session teardown and cover local/remote close as well as unsolicited delivery.

Verification: GitHub-only static review; no tests or external-stack interop run. Check CI for this head is still queued, so the previously reported integration failures have not been independently verified as fixed.

[kixelated]

[P2] End skipped announcement readers when the session closes

For a draft-14/15 peer with no Solicit declaration, this wait has no connection-lifetime signal. No request stream exists to wake it on disconnect; Subscriber.close() only sets #localClose, and Connection.#close() only closes the transport/adapter. Consequently, after conn.abort() or a remote disconnect, a direct conn.announced() reader's pending next() and closed remain unsettled, and the producer stays registered until the caller explicitly closes it. Callers using stream termination for cleanup/reconnection can hang. forwardAnnounced closes its own reader on conn.closed, but that only protects the consume wiring. Race this wait against session/transport closure, or close these producers during all connection teardown paths, and add a local/remote-close regression test.

[kixelated]

Fixed in f066a4f: the skipped-request wait now races announced.closed against the transport's closed, so the reader ends on a local or remote close. Regression tests cover both sides and fail without the fix.

(Written by Claude Opus 5.5)

[kixelated]

Automated review: #5018 follow-up at f066a4f8

Re-review after a push. Since the last review on b8424b3c, the PR changes are 865511b9 (quest/docs only, already checked) and the new f066a4f8 ("end an unasked announcement reader with its session"). The two origin/main merges add nothing of the PR's own.

This push

It fixes a real hang. Before, a d14/15 empty-prefix reader for a peer that declared no Solicit waited only on announced.closed (js/net/src/ietf/subscriber.ts:330-335). Nothing else ended it, so a caller iterating announced() hung forever after the session died, and the consumer stayed in #announcedConsumers. Now it races announced.closed against quic.closed. I checked that quic.closed really does settle on every production path where the session ends:

• When the d14/15 control stream dies, adapter.run() rejects and Connection calls #close(), which closes the transport (connection.ts:149-151, :192-204).
• When the adapter ends cleanly, acceptBi returns undefined, #runBidis exits, and #run's finally closes the transport (:213-225). A graceful close() closes it once its drain finishes.
• Connection always passes quic (:165), so the if (this.#quic) fallback only matters for the unit tests that build a bare Subscriber.
• A GOAWAY drain keeps the transport open, and the reader stays registered. That's right, because the peer can still announce during the drain.

The new test covers a local close and a peer close. The mock propagates close() to the peer's closed (mock.ts:226-271), so the remote case really exercises the cross-side path.

No blocking issues.

Non-blocking:

• A session failure looks like a clean end. quic.closed.catch(() => undefined) plus the caller's finally { announced.close() } means this reader always ends cleanly, even on an abrupt loss or an error close code. A named-prefix reader on the same session ends differently: its stream errors and goes through announced.close(e) (:484-492, which says why a failure has to stay distinguishable). If parity matters, close with the session's error when quic.closed rejects or carries a non-zero code (#closedSession() / closeError already build it). Then assert that in the test instead of toBeUndefined().

Earlier findings

• Weak JS assertion: still open. "%s still asks a soliciting peer for the empty namespace" (subscriber.test.ts:114-122) still only checks that a stream opened. It doesn't decode a zero-field SUBSCRIBE_NAMESPACE.
• Solicit draft note: still open. drafts/draft-lcurley-moq-solicit.md still doesn't say that two Solicit-declaring d14/15 endpoints exchange a zero-field namespace those drafts forbid. 865511b9's cluster.md note covers the opposite case (foreign peers that aren't asked).
• CI: no test workflow has reported on f066a4f8 yet; only Auto-merge and Dependabot have run (both skipped). The three moq-transport-14 integration tests named earlier are still the real confirmation.

Verdict: MERGE once CI is green.

Reviewed head: f066a4f871c0d6e7a016a173d19c78a2b59b8611

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit f066a4f871c0d6e7a016a173d19c78a2b59b8611, including the teardown follow-up and the full PR delta against 8a55152ccf323ebb050c4c99814cabc737d4718c; merged-main changes were accounted for separately.

The previous P2 is fixed in source: js/net/src/ietf/subscriber.ts:330-335 now races the skipped reader against transport closure, and the existing finalizer unregisters/closes its producer. Production Connection supplies that transport; local abort, remote close, and rejected transport closure all release this wait. The new tests at subscriber.test.ts:95-108 cover local/remote mock closes. The earlier Solicit discovery P1 remains fixed in both implementations.

No new actionable correctness issue found. Direction: the peer-aware skip plus session-bound reader lifetime addresses both findings; the documented unscoped moxygen draft-14 discovery limitation remains an intentional scope constraint.

Verification: GitHub-only static code/test review. I did not run tests or external-stack interop, and have not independently confirmed the earlier integration failures are green.

[kixelated]

Ready to land at f066a4f87.

• Decision (maintainer 2026-10-08): skip the empty SUBSCRIBE_NAMESPACE on draft-14/15 only for a peer whose SETUP declared no MoQ Solicit. Our own endpoints still get it, so the three draft-14 integration tests pass again.
• Review fix: the JS reader left waiting after the skip now ends when the session closes, local or remote.
• Deleted quest/m0/ietf-d14-root-prefix.md (this PR completes it) and noted the moxygen draft-14 discovery gap in doc/bin/relay/cluster.md.
• Unrelated: quest check on main fails on quest/m1/js-session-parity.md linking the missing /quest/m0/broadcast-epoch/stats-epoch.md.

(Written by Claude Opus 5.5)

[kixelated]

Merge summary

• Merged main twice. The only conflict was quest/m0/README.md: this PR drops the ietf-d14-root-prefix entry and main had dropped the dial-split-horizon entry on the adjacent line, so both lines are gone. No code conflicts.
• Settled decision: a draft-14/15 SUBSCRIBE_NAMESPACE for the empty prefix is skipped only when the peer declared no MoQ Solicit; a Solicit peer still gets it. The PR deletes quest/m0/ietf-d14-root-prefix.md.
• The automated OpenAI review of f066a4f8 found no actionable issues; only clean main merges follow it.
• Local: just check is green except three moq-uring worker tests that fail on this shared host's RLIMIT_MEMLOCK (unrelated to this PR); the rest of the Rust suite passes (6329/6329). just test interop --all passes 33/33.
• Overlaps with fix(net): honor SUBSCRIBE_NAMESPACE options and fill draft-16+ streams #5032 are textually and semantically independent (this PR changes the subscriber's outgoing request; fix(net): honor SUBSCRIBE_NAMESPACE options and fill draft-16+ streams #5032 changes how the publisher handles incoming options). Whichever lands second will need a trivial quest/m0/README.md re-merge.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit 0a6a263af5521296aff1e3bda8a502440b76a04f against the previous review, separating merged-main changes from the PR delta against d5208239f4ae4c5452f4b81fbd0246806e854860.

No new actionable correctness issue found. The only PR-owned follow-up is test compatibility at rs/moq-net/src/ietf/session.rs:1266,1300: SubscribeOptions::Namespace retains the former 0x01 wire value, and limits: Default::default() supplies main’s new configuration field without changing the test’s discovery assertions. All other PR additions/deletions match the previously reviewed delta.

The Solicit-aware discovery fix and session-bound JS reader lifetime remain intact. Direction is unchanged: avoid the illegal empty request only for foreign draft-14/15 peers, preserving discovery between our own endpoints; the documented moxygen discovery gap remains intentional.

Verification: GitHub-only static review of the diff, relevant implementation, tests and existing discussion. No tests or interop run; this head’s Check/Test jobs are queued. Reported local successes were not independently reproduced.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit b205748afe3dc81e5c08e1dc4778850bb929cce5 against the previous review, separating the merged-main delta against 2f39fde517435794f9ec5af9e1e17f29cc530ca0.

[P2] Adapt the newly merged GOAWAY test to the foreign-peer skip. At js/net/src/goaway-requests.test.ts:257-264, the fixture constructs new Subscriber({ session, goaway }), clears opened, then calls unscoped announced() and waits for a stream. On the first iteration (draft-14), Solicit is undefined, so js/net/src/ietf/subscriber.ts:370-376 intentionally waits without opening any stream. Consequently waitUntil(() => opened) hits its two-second timeout. Give this fixture solicit: true or request a named prefix, retaining its GOAWAY assertion while exercising an allowed request. This is a new merge-integration test regression; the test was absent at the previous reviewed head.

Direction remains sound: the merge keeps both the Solicit declaration and main's GOAWAY signal wired into Subscriber. The previous discovery and reader-lifetime fixes remain intact; no new production correctness issue found.

Verification: GitHub-only static review of the merge, complete PR patch, relevant source/tests and discussion. I did not run tests or interop; the timeout is established by the source path, not an observed test run. This head's Check workflow is queued.

[kixelated]

Re the [P2] on b205748a (GOAWAY announce test): fixed in 8c7be13. The fixture now passes solicit: true, so it models one of our peers and draft-14 still asks for the empty namespace after GOAWAY. Confirmed it timed out before and js/net now passes 1366/1366.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit 0a06f6473ff0dd6da955d4f0349945b6a2876060 against the previous review, separating merged-main changes from the PR delta against 2870c0c536a928c61af93e69611e08172c46b7a0.

The prior P2 is fixed in source: js/net/src/goaway-requests.test.ts:257-265 now supplies solicit: true, so draft-14 reaches openBi() instead of the foreign-peer wait, while retaining the post-GOAWAY assertion. This agrees with the reported fix. All other PR patch hunks match the previously reviewed delta; the additional changes come from main’s #4914.

No new actionable correctness issue found. The earlier Solicit discovery and JS reader-lifetime fixes remain intact. Direction remains sound: skip the illegal empty request only for foreign draft-14/15 peers; the documented moxygen discovery limitation remains intentional.

Verification: GitHub-only static diff/source/test review. No tests or external-stack interop run; the reported 1366/1366 JS pass was not independently reproduced. This head’s Check and Test jobs are in progress.

[kixelated]

Merge summary update at da29e6bf

• Merged main after fix(net): honor SUBSCRIBE_NAMESPACE options and fill draft-16+ streams #5032 and fix(net): give the group-order session test its limits #5048 landed. Conflicts resolved:
  • quest/m0/README.md: dropped both this PR's entry and fix(net): honor SUBSCRIBE_NAMESPACE options and fill draft-16+ streams #5032's.
  • js/net/src/ietf/{connection,subscriber}.ts: Subscriber now takes both this PR's solicit and main's goaway.
• PR-side fixes for main's changes: the Rust empty-namespace test uses SubscribeOptions::Namespace and limits (from fix(net): honor SUBSCRIBE_NAMESPACE options and fill draft-16+ streams #5032 and feat(net)!: bound peer-declared lengths and per-session requests #4820). The draft-14 GOAWAY announce test declares Solicit (the OpenAI review's P2 on b205748a).
• The OpenAI review of 0a06f647 found nothing new; only a clean main merge follows it.
• Local: moq-net 1623/1623, js/net 1368/1368, full Rust suite green apart from the host-limited moq-uring tests, just test interop --all 33/33.

(Written by Claude Opus 5.5)