A receipt can carry an acknowledgement id no worker reported - #747
Merged
Merged
Conversation
The trigger read only the row as it was, so a statement could write an acknowledgement id and leave the row exposed. That puts an id in the receipt that no worker ever reported — the half of "with the acknowledgement or never" that the old row cannot see. Three things now have to hold together: nothing recorded before, the row still waiting to be acknowledged, and this same statement acknowledging it. Ack already did all three, so nothing it does changes.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Editing migration 5 leaves existing version-5 ledgers using the vulnerable trigger.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Strengthens the connector ledger invariant so acknowledgement IDs are written only during the transition to delivered.
Changes:
- Tightens the acknowledgement trigger.
- Adds rejection and successful-acknowledgement tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/connector/ledger.go |
Tightens the acknowledgement trigger. |
internal/connector/dispatch_lifecycle_test.go |
Tests invalid and valid acknowledgement-ID writes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The first version of this edited migration 5, which had already shipped in #736. A ledger already at version 5 would have kept the loose trigger for good: migrate skips what it has applied, so an edit to a shipped migration reaches no existing ledger. The contract at the top of the list says so. Migration 5 is back exactly as it shipped, and migration 6 drops and recreates the trigger. The new test walks the upgrade that actually happens: a ledger built from migrations 1 through 5 alone, opened, and then held to the tighter rule.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A receipt can carry an acknowledgement id that no worker ever reported.
The rule the ledger states is that the id a worker points at is written when it
acknowledges, or never. The trigger meant to hold that rule read only the row as it was,
so a statement could write the id and leave the row exposed — unacknowledged, with an id
sitting in it. Whoever reads the receipt later cannot tell the difference.
Copilot found this on #736 two and a
half minutes after that pull request merged, so it landed on main. Nothing in the
connector does it today:
Ackis the only statement that writes the id, and it alwaysdelivers the row in the same update. This closes the gap in the rule rather than a
defect in a caller.
The trigger now requires all three parts together — nothing recorded before, the row
still waiting, and this same statement acknowledging it.