Skip to content

fix(router): provisional cursor on BatchFlusher poison skip - #86

Merged
olucasandrade merged 5 commits into
fix/review-findings-20260924from
fix/router-poison-skip-batchflusher-cursor
Sep 28, 2026
Merged

olucasandrade merged 5 commits into
fix/review-findings-20260924from
fix/router-poison-skip-batchflusher-cursor

Conversation

@olucasandrade

Copy link
Copy Markdown
Owner

Source fix plan

.claude/fix-plans/router-poison-skip-batchflusher-cursor-20260924-212132.md

Problem

In applyDispatchCursors, the RTR-07 poison-skip branch called advanceCursorLocked, which advanced cursorByPartition and queued SaveCursor immediately. For BatchFlusher consumers, Deliver only buffers — the durable cursor must move only after FlushBatch succeeds via provisional promotion.

Trace: durable cursor at 1; poison at seq 3 → skip persisted cursor to 4 while seqs 1–2 were only buffered; flush failure discarded provisional → next read started at 4 → seqs 1–2 lost. Real queue sinks drop their pending buffer on flush failure, so this is an at-least-once violation.

Introduced by

36c9733

Implementation

  • In applyDispatchCursors, BatchFlusher consumers record a poison skip on provisionalByPartition (same promotion path as a successful buffered Deliver).
  • Non-flusher consumers keep immediate advanceCursorLocked.
  • Skip-set still suppresses re-delivery of the poisoned seq after a failed flush re-reads the partition.
  • CHK-01 and non-flusher cursor semantics unchanged.

Validation

  • CGO_ENABLED=0 go test ./internal/router -count=1 → pass (18.5s)
  • go test -race -count=1 -timeout 180s ./internal/router → pass (22.9s)
  • New tests: mid-window poison (seq 3) + next flush failure re-delivers seqs 1–2; no cursor past unflushed buffered seqs while flush keeps failing; existing poison-at-start + successful flush tests remain green.

Residual risk

  • Incorrect provisional bookkeeping could still re-deliver a poisoned seq or stall a key if skip-set and provisional promotion get out of sync under unusual concurrent partition patterns; covered for the mid-window + flush-failure path but not every multi-consumer floor interaction.

Poison skips were advancing the durable cursor immediately, so a mid-window
poison could persist past unflushed buffered seqs and lose them after a
failed FlushBatch. Route BatchFlusher skips through provisionalByPartition
like successful Deliver; keep immediate advance for non-flushers.

Introduced-by: 36c9733
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2f94571e-9a85-4898-830a-5309da0be6f4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Deploying kaptanto with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6fa1f97
Status: ✅  Deploy successful!
Preview URL: https://da555aa2.kaptanto.pages.dev
Branch Preview URL: https://fix-router-poison-skip-batch.kaptanto.pages.dev

View logs

@olucasandrade
olucasandrade merged commit 06c246e into fix/review-findings-20260924 Sep 28, 2026
15 checks passed
@olucasandrade
olucasandrade deleted the fix/router-poison-skip-batchflusher-cursor branch September 28, 2026 12:25
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