Skip to content

fix(net): settle lite-07 tails on stream counts - #4224

Merged
kixelated merged 3 commits into
mainfrom
quest/m1/lite-count-settle
Sep 27, 2026
Merged

kixelated merged 3 commits into
mainfrom
quest/m1/lite-count-settle

Conversation

@kixelated

@kixelated kixelated commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

lite-07 subscribers ignore SUBSCRIBE_END's stream count. A skipped group sequence keeps Rust and JS subscribers waiting for the tail grace after all opened streams have arrived.

Approach

Retain the received stream count and settle when at least that many headers arrive. Keep the grace for streams reset before their headers and preserve lite-05/06 range and DROP accounting. Extend both existing tail suites and document completion behavior.

Impact

  • Public API: unchanged.
  • Wire format: unchanged; subscribers now use lite-07's existing Stream Count. Skipped groups and zero streams no longer add a grace wait.
  • lite-05 and lite-06 behavior stays unchanged; received groups retain their own FIN/reset lifecycle.

Validation

  • Reproduced the Rust skipped-tail delay (900 ms of paused time) and both JS skipped/zero-count waits before the fix.
  • All 1,287 moq-net tests pass; JS tail suite: 22 passed, one DROP-only case intentionally skipped for lite-07.
  • just check passed, including the scoped Rust, JS, documentation, and feature checks.
  • just test interop --all passed all 32 publisher/subscriber lanes. This existing matrix uses the default protocol; the count-specific lite-07 proof below remains outstanding.

Alternatives

Range accounting cannot distinguish skipped sequences from streams still in flight on lite-07. The existing count supplies that distinction without changing the protocol or the grace.

Follow-ups

Keep this draft and retain the quest until its count-specific Rust-JS interop proof is complete. Track-tail interop #4225 currently exposes a separate relay start-floor defect: a newer group arriving first can cause earlier in-flight groups to be discarded. This PR does not modify that harness or the relay publisher.

(Written by GPT-6)

kixelated and others added 2 commits September 25, 2026 18:36
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
@kixelated
kixelated marked this pull request as ready for review September 26, 2026 14:32
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-27T03:30:21.485056Z e6f8e6c New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d89ed15ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if settle
.poll(waiter, |tail| owed.clone().is_some_and(|owed| tail.covers(owed)))
.poll(waiter, |tail| match streams {
Some(streams) => tail.streams() >= *streams,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject under-reported stream counts

When a lite-07 peer declares fewer streams than it actually opened, this threshold becomes true after only the declared number of headers, the subscription mapping is removed, and later streams are silently discarded as belonging to a closed subscription. Stream Count is specified as the exact number opened, so detect an observed count above the declaration and reject later streams for that completed subscription instead of treating malformed input as a clean end; the TypeScript comparison in js/net/src/lite/subscriber.ts has the same issue.

AGENTS.md reference: AGENTS.md:L17-L17

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one. Once the declared count is met the subscription is gone, so a surplus stream after that looks the same as a stream for any closed or locally cancelled subscription, and it is already refused through the unknown-subscription path rather than delivered. The only surplus we could still catch is one that lands before SUBSCRIBE_END. That would mean adding an error path to the tail settle in both languages to catch part of a publisher bug, and it wouldn't stop the track from ending. If the count ever needs to be enforced, reliable reset (quest/m1/quic/reliable-reset.md) is where the count becomes exact and a check could cover every case.

(Written by Claude Opus 5.5)

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: 3b13f3da-827f-4605-8d5d-dcb62ae31d67

📥 Commits

Reviewing files that changed from the base of the PR and between 5d89ed1 and e6f8e6c.

📒 Files selected for processing (3)
  • js/net/src/lite/subscriber.ts
  • js/net/src/lite/tail.test.ts
  • rs/moq-net/src/lite/subscriber.rs

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 lite-07, Rust and JavaScript subscribers now complete a tail when received group-stream headers reach the count in SUBSCRIBE_END. Lite-05 and lite-06 retain range-based accounting. Tests cover skipped sequences, zero streams, resets, and completion across draft versions. The documentation describes these rules and the grace period for streams reset before their headers arrive.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to e6f8e

No actionable merge-blocking issue remains. The count-specific Rust–JavaScript interop check is still pending, but no failure has been established.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to e6f8e

An inaccurate stream count could make a lite-07 subscriber finish before all group streams arrive. The normal publishers wait for stream openings before declaring the count, and the potential loss is scoped to the affected subscription. Count-specific Rust–JavaScript interoperability proof remains outstanding.

Retained concerns

  • Medium · security · inferred: An understated lite-07 SUBSCRIBE_END count can satisfy tail settlement before all group headers arrive, causing late streams to lose their subscription route. This depends on an inaccurate peer response; the examined conforming publisher paths wait for stream openings.
Security review details

Security Blast Radius

  • inferred — The demonstrated failure path affects completeness of the track served by an individual upstream subscription and potentially its downstream consumers. The examined paths do not demonstrate new credential, tenant, or deployment authority.

Security Findings and Attack Paths

  • inferred — A peer that supplies an understated count can satisfy the new completion predicate while other group headers are in flight. Subsequent subscription cleanup can leave those late streams without a route. This is conditional on an inaccurate response, not a verified conforming-peer failure.

Trust Boundaries and Controls

  • observed — The subscribers take the completion count from SUBSCRIBE_END rather than independently verifying how many streams the peer opened. The examined normal publishers wait for pending openings, and settlement retains grace for headers that never arrive.

Resilience and Maintainability Implications

  • inferred — Cancellation and terminal cleanup limit stale subscription state, but cannot recover a group stream that arrives after a prematurely completed subscription has been removed.

Hardening Proposals

  • proposed — Complete count-specific Rust–JavaScript interop checks and exercise understated, overstated, and repeated END counts with late and reset-before-header streams. Establish whether the intended peer trust model requires rejecting inconsistent END responses.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the lite-07 stream-count change, preserved lite-05/06 behavior, validation results, and known follow-up.
Title check ✅ Passed The title clearly and concisely identifies the main change: lite-07 tails now settle based on stream counts.
Docstring Coverage ✅ Passed Docstring coverage is 90.91% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files.
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
✨ Simplify code
  • Commit to this branch
  • Create a new PR

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

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

# Conflicts:
#	js/net/src/lite/tail.test.ts
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged main in to resolve the js/net/src/lite/tail.test.ts conflict. The per-version describe.each refactor now sits alongside main's new bare-FIN and reset tests, which call subscribed(Version.DRAFT_05).

  • The Release JS Packages failure was a cache.nixos.org download error (HTTP 416) during nix shell setup, not a code problem. It passes on the new head.
  • Wire: unchanged. The draft already defines Stream Count and says the subscriber has every Group Stream once it has read that many headers; this PR only makes subscribers honor it.
  • just check and just test interop --all (all lanes) pass locally.
  • I replied to the Codex under-count finding instead of changing the code.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit 8ef17ee into main Sep 27, 2026
6 checks passed
@kixelated
kixelated deleted the quest/m1/lite-count-settle branch September 27, 2026 04:22
This was referenced Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant