fix(session-sqlx): let failed executions re-run - #1531
Closed
0xAlcibiades wants to merge 1 commit into
Closed
Conversation
The unique idempotency index on session_executions covered every row, so a failed or cancelled execution permanently blocked a fresh execution that carried the same (thread_key, idempotency_key). For triggers with a stable key per commit, a turn killed by a control-plane roll became un-rerunnable: the insert hit the index and the caller received the dead row with created = false. The index now covers only queued, running, and completed rows. In-flight and completed work still dedupes; failed and cancelled work falls out of the index, so the next insert creates a fresh execution. Migration 0053 swaps the index in place and the upsert conflict clause is narrowed to match it.
Collaborator
|
Thanks for looking into this. I don’t think we should merge this. It weakens the session API’s global idempotency contract: failed executions may already have produced external side effects, so allowing the same request key to create another execution can duplicate work. I opened #1603 with a GitHub-specific fix instead. It keys explicit review requests by GitHub delivery ID, so a fresh request can run again while redeliveries remain idempotent. I’m going to close this PR in favor of that one. |
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.
Closes #1507
Context
The unique idempotency index on
session_executionscovered every row, so afailed or cancelled execution permanently held its
(thread_key, idempotency_key)pair. For any trigger with a stable key the work then becameun-rerunnable: the insert hit the index and the caller got the dead row back
with
created = false.A GitHub review is keyed per commit, so this is reachable in normal operation.
A turn killed mid-flight — a control-plane roll, say — leaves a
failedrowthat no identical re-request can ever get past. The only recovery is editing the
row by hand or pushing a new commit to move the key.
Change
Migration
0053swaps the index in place so it covers only the statuses wherededupe is still meaningful:
In-flight and completed work still dedupes, so a retried webhook cannot start a
second live turn or resurrect a finished answer. Failed and cancelled rows drop
out of the index, and the next insert with that key creates a fresh execution.
The upsert conflict clause is narrowed to match the new index.
Testing
cargo fmt --all --checkandcargo clippy -p centaur-session-sqlx --all-targets: clean.cargo test -p centaur-session-sqlx: passes.One caveat worth stating plainly: the new test,
failed_execution_does_not_block_rerun_on_same_idempotency_key, isdatabase-backed and returns early when
SESSION_RUNTIME_TEST_DATABASE_URLisunset. I had no Postgres available, so it reported
okwithout executing, andI have not exercised the migration or the new index locally. CI has the
database and will actually run it. The test asserts that a failed execution is
followed by a fresh
created = trueexecution on the same key.0053is the next free number on top of0052_session_execution_request.sqlatthe time of writing; it will need renumbering if another migration lands first.