Skip to content

quest(m1): plan early SUBSCRIBE_DROPs in Rust and JS publishers - #5303

Merged
kixelated merged 2 commits into
mainfrom
quest/m1/subscribe-drop-early
Oct 13, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest/m1/subscribe-drop-early

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Adds Early drops as a child of the SUBSCRIBE_DROP questline, following the Codex finding on #5301 that the JS publisher never sends SUBSCRIBE_DROP.

The new [L] quest covers lite-05 and lite-06. The Rust and @moq/net publishers send a DROP the moment they give a sequence up (cursor skips, max-delay expiry, and drops the upstream publisher sent), and both keep the end-of-track sweep. A per-sequence dropped mark in both track models lets a relay forward upstream drops. The parent keeps resets before the header, lite-07, and showing drops to readers.

Decisions

How should the drop_unserved port be tracked?

  • ✅ Split a child quest
  • Note it in the existing quest
  • Nothing

Goal: mirror only Rust's end-of-track sweep in JS?

  • Confirmed, end-of-track only
  • ✅ Also drop early

Early drops in Rust too?

  • ✅ Rust and JS together (maintainer: "Make sure Rust drops early too")
  • JS only

What counts as giving a sequence up?

  • ✅ Cursor skips
  • ✅ Max-delay expiry
  • ✅ (maintainer) Explicit gaps only when the original publisher explicitly sent a DROP, like after a cut
  • Explicit gaps inferred from missing sequences

Tests?

  • ✅ Unit tests plus just test interop --tail
  • Unit tests only

Where does the per-sequence dropped mark go?

  • ✅ In this child
  • Leave to the parent

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

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 6 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: c65f26f1-4d29-4bd1-8dc2-b275ebb9c030
📥 Commits

Reviewing files that changed from the base of the PR and between 7a94840 and 8e80d76.

📒 Files selected for processing (3)
  • quest/m1/README.md
  • quest/m1/subscribe-drop-early.md
  • quest/m1/subscribe-drop.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

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 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-10-11T03:33:30.500868Z b5afa63 PR opened
ℹ️ 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.

@kixelated kixelated left a comment

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.

Automated review by review (OpenAI)

Reviewed commit: b5afa63.

The focused Rust/JS parity quest follows the independent #5301 finding. Keeping lite-07, reset-before-header handling, and reader errors in the parent is reasonable.

One actionable acceptance gap in quest/m1/subscribe-drop-early.md:28–33: the child explicitly says max-delay expiry also closes the parent's grace-aging hole, but dropping a buffered group does not cover a never-arrived gap. Rust marks a group served before its stream opens, and Tail::expire silently folds missing gaps. An absent group has no buffered object to expire and, correctly, is not an explicit upstream DROP. Keeping the existing end sweep alone can therefore still omit it.

Make preserving unresolved gaps for the final sweep an explicit acceptance condition, rather than treating them as delivered on the publisher's grace clock. Add a mirrored mock-time regression: serve group 0, leave group 1 absent, queue group 2 behind exhausted stream credit for longer than the publisher's grace, then release credit and finish. Group 1 must be named before FIN without waiting for the receiver's later-started grace. Also retain the live-reordering case: no inferred DROP while group 1 could still arrive.

This PR changes planning documents only; no current API or wire change. GitHub-only static review of all three changed files and relevant Rust/JS publisher, subscriber, model, and tail code; tests not run. Head and reviews rechecked before posting; no existing substantive review to duplicate.

(Written by OpenAI)

@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: b5afa63c85

ℹ️ 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".

Comment thread quest/m1/subscribe-drop-early.md Outdated
Comment thread quest/m1/subscribe-drop-early.md Outdated
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the quest/m1/subscribe-drop-early branch from b5afa63 to 908e617 Compare October 13, 2026 01:33
…never-arrived gaps for the sweep

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Addressing the OpenAI review: agreed that max-delay expiry has nothing to drop for a group that never arrived. The quest now makes it an explicit acceptance condition that such a gap stays unresolved until the end-of-track sweep names it, rather than counting as delivered on the publisher's grace clock. It adds the mirrored mock-time regression you described (group 1 absent, group 2 held behind exhausted credit past the publisher's grace, group 1 named before the FIN) and keeps the live-reordering case with no inferred DROP. The max-delay bullet no longer claims to close the parent's aging hole; the new condition does. Fixed in 8e80d76.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary:

  • Rebased on main; quest check passes.
  • Codex P2 (pre-FIN settlement): this child's tests now assert the DROP is emitted and accounted at the skip and the owed range settles right after the FIN; settling before the FIN, which needs reader disposition, moved to the parent's regression tests.
  • Codex P2 (draft text): the SHOULD-send-early wording moved to the parent's draft update with the lite-07 SUBSCRIBE_DROP restoration; this child lists the draft text as a non-goal.
  • OpenAI: a never-arrived gap stays unresolved until the end-of-track sweep names it, as an explicit acceptance condition with a mirrored mock-time regression and the live-reordering case. The parent's note on the aging hole now points here.

No reviewer posted on 8e80d76 after CI finished, so the earlier reviews cover it per CONTRIBUTING. Enabling auto-merge.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit b512433 Oct 13, 2026
4 checks passed
@kixelated
kixelated deleted the quest/m1/subscribe-drop-early branch October 13, 2026 02:34
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