fix(cache): keep an aborted invalidation bump queued - #1748
Conversation
|
@codex review |
![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: 5821706ae3
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28ecf6ada
ℹ️ 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".
_flush_pending_bumps clears each pending marker before awaiting its write — deliberately, so a request_bump() arriving mid-write re-queues rather than being coalesced into the version already being written. But it only restored the marker when bump() returned False. A write that was cancelled or raised left the namespace neither written nor pending. stop() cancels the polling task by design, so this silently dropped whatever namespace the poll loop was mid-write on at shutdown: a mutation that had already committed never reached peer replicas, leaving their caches stale until an unrelated later bump. It affects every namespace on the bus. Found while investigating #1354; independent of that fix. Co-Authored-By: Darafei Praliaskouski <me@komzpa.net> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
b81fe02 to
d451c09
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d451c09195
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0da465250c
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e33ee2945
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 787c8f50a0
ℹ️ 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".
The invalidation-bus spec already requires coalesced namespaces to remain pending until a bump succeeds; an aborted write silently dropping one violated that. Record the clarification rather than inventing a new shutdown-delivery contract — 'a lost bump still converges within the fallback TTL' is the documented bound and stays so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
787c8f5 to
2bf3e44
Compare
Soju06
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bf3e44b86
ℹ️ 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".
A cancellation can reach the bump write after the database accepted its commit. The restore is unconditional there by design: a redundant bump only re-runs peers' idempotent invalidation callbacks, while dropping an unconfirmed write leaves them stale until the fallback TTL. The bus already tolerates extra increments — a request_bump arriving mid-flush produces one deliberately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d3e9593e
ℹ️ 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".
…abort The aborted-write scenario asserted no version is written, which the ambiguous-abort scenario immediately below it contradicts. Scope it to a cancellation arriving before the database accepts the commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d953c568de
ℹ️ 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".
The qualifier only covered cancellation, but a driver can raise after the server accepted COMMIT too. Both scenarios now split on whether the database accepted the commit rather than on how the write aborted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79c47e84bd
ℹ️ 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".
The unit-level tests asserted the marker is restored; the product behavior is that the background poller then writes the version. Cover it end to end: first write raises, the running poller retries, the version lands. Also moves the ambiguous-abort rationale out of the normative requirement into the proposal, per the spec/context split, and trims the code comment to what is true now that shutdown delivery is out of scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16a4cd69fe
ℹ️ 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".
My previous edit left the original clause fragment in place, so the rationale I said I had moved was still there, duplicated. The requirement now states only the testable outcome; the reasoning lives in the proposal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48e32f2126
ℹ️ 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".
…tring The restore matters where a later cycle exists to retry; at process stop no cycle remains either way — shutdown delivery is explicitly out of scope and the restore there only keeps the pending set honest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4095d19e78
ℹ️ 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".
bump() reports normal failure by returning False, so a raise is abnormal — but re-raising aborted the sorted flush loop, and a persistently raising namespace that sorts first would starve every namespace after it on every cycle. Cancellation still restores and re-raises (teardown must abort); an abnormal raise now restores, logs at warning, and continues. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4249001368
ℹ️ 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".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1677023e4
ℹ️ 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".
The committed row can become visible while bump() is still in its shielded session cleanup; stopping the poller at that instant cancels the retry mid-flight. Gate the stop on an event set only after bump() fully returns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Found while investigating #1354 (see #1747); independent of that fix and affects every namespace on the invalidation bus.
The bug
The invalidation-bus spec already requires that "coalesced (
request_bump) namespaces MUST remain pending and be retried on subsequent poll cycles until a bump succeeds". The implementation violated that for one case._flush_pending_bumpsclears each namespace's pending marker before awaiting its write. That is deliberate and documented — arequest_bump()arriving mid-write must re-queue rather than be coalesced into the version already being written. But the marker was restored only whenbump()returnedFalse:A write that is cancelled or raises left the namespace neither written nor pending — nothing logged, no retry holding it.
_runswallows poll exceptions and keeps cycling, so during ordinary operation a raising write silently lost its namespace until an unrelated later bump.The fix
BaseExceptionrather thanExceptionis the point —CancelledErroris the case that matters.Scope
Process shutdown is deliberately not covered.
stop()cancels the polling task, so a bump queued at that instant has no cycle left to drain it — and that is already the documented contract: "a lost bump still converges within the fallback TTL".An earlier revision of this PR added a bounded flush in
stop()to close that window. Each round of review surfaced a further ordering or task-ownership corner in it (unbounded poll-task cancellation,wait_forawaiting the cancelled write's unwind, restore-vs-abandon ordering, cancellation ofstop()itself) — the same pattern that sank #1716. Guaranteeing delivery against an unresponsive database at shutdown is a real design problem and deserves its own change, not an appendix to a two-line bug fix. Those commits are dropped.Tests
OpenSpec
openspec/changes/keep-aborted-invalidation-bumps-pending/— MODIFIED delta making "remains pending" explicitly cover an aborted write, not only a failed one. Validation passes.Gates green locally: ruff, ruff format, ty, 22 invalidation-bus tests.
Co-authored with @Komzpa, whose bump-persistence work on the now-closed #1716 is what led here.
🤖 Generated with Claude Code