Repository navigation
D-GSO-8 follow-up: refuse a reused event seq - #1366
Conversation
|
Warning Review limit reachedYour organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 67 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (2)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 78095ef3-7085-4a13-8980-cc30ed8bf917) |
The probe's key precondition (each event's seq is globally unique) was stated but never checked: row coalescing hides a same-row reuse from first_tie, and a reuse across seals leaves each batch tie-free. apply now refuses any seq not greater than the last accepted one, and resume_after restores the highest persisted key. The order-binding test swaps values instead of events so its stream stays strictly increasing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgxrCafsJuAahpBs7oC14R
ec13149 to
ca50dda
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 1690fa55-ad91-45f2-9298-0173cd6cd69c) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca50ddaff8
ℹ️ 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".
| let top = s.batch.landings.iter().map(|l| l.stream_position).max(); | ||
| e.last_seq = e.last_seq.max(top); |
There was a problem hiding this comment.
When resuming after a seal, the largest landing position is not necessarily the last accepted event sequence: boundary events never become landings, and internal events whose folds leave a row unchanged are omitted from the batch. For example, after accepting internal seq=5 and boundary seq=6, this restores last_seq as 5, so a resumed engine accepts a reused seq=6, defeating the recovery guarantee this change claims to add. Persist and restore the accepted event high-water mark itself, including the boundary sequence, rather than deriving it from coalesced landings.
Useful? React with 👍 / 👎.
2 participants
Follow-up to #1363 (merged), addressing the Codex review finding there.
What was wrong
The seal-boundary probe assumes every event's
seqis globally unique, but it never checked this. The only check wasfirst_tieover the casts in one batch. Two kinds of reuse got past it:seqon the same row: row coalescing had already reduced the two events to one cast;seqacross two seals: each batch on its own has no tie.Change (
crates/lance-graph-planner/examples/seal_boundary_probe.rs)applynow refuses any event whoseseqis not greater than the last accepted one (SealError::StaleSeq). The check runs before anything folds.resume_afterrestores the highest persisted key, so a resumed engine still refuses a reused one.the_seal_binds_base_content_and_ordernow swaps the two values instead of the two events. Theseqstream therefore stays strictly increasing, and the test still checks that order is bound into the seal.a_reused_seq_is_refused. It covers a reuse on the same row, a reuse across seals, and a reuse after a resume. It also checks that a fresh, largerseqis still accepted.SUPERSESSION-INDEXregenerated, no change.Verification
cargo test --manifest-path crates/lance-graph-planner/Cargo.toml --example seal_boundary_probe: 7 passed.seqcheck failsa_reused_seq_is_refused;last_seqrestore inresume_afterfails it too.cargo runstill prints that replay from the prior seal reproduces the last seal.🤖 Generated with Claude Code
https://claude.ai/code/session_01DgxrCafsJuAahpBs7oC14R
Generated by Claude Code