fix(idempotency): a failed async run stops replaying its receipt (#3245)

[AndriiPasternak31]

Fixes #3245

What was wrong

An async /task (and a sync /task or pulled /chat that timed out) stores a dispatch receipt (async_mode: true, status accepted / queued / queued_timeout) as its completed idempotency row at dispatch time. Nothing revisited that row, so for 24 h an identical request got the dead run's receipt back even after the run failed. A caller retrying word for word, as the delegation contract tells it to, was stuck. The MCP client also dropped X-Idempotent-Replay, so an MCP caller could not tell a replay from a fresh receipt.

What changes

• Replay seam (backend). dispatch_admission_service._reclaim_ended_receipt runs right after idempotency_service.begin(...) in both admit_chat_request (/chat) and begin_task_idempotency (/task). When the stored snapshot is a dispatch receipt, it asks idempotency_service.execution_liveness(...) whether that run can still succeed. Only ended (failed / cancelled / skipped) and gone (no row) reclaim: the row is compare-and-deleted (db.idempotency_discard_completed_if_execution, only while it still names the dead execution id) and the key is claimed again once. live, succeeded, maybe_alive and indeterminate replay as before. The check is read-time only: no terminal writer gains a side effect (bug: recovery paths write execution terminals without closing the paired activity — agent shows as running for up to 2h, then logs a fabricated 120-min failure #1804), and rows written before this fix heal on their next retry.
• lease_expired hold. A FAILED that the slot reaper wrote (lease_expired: error prefix) may still report success, so it replays until completed_at + agent timeout + 300 s, then a retry starts fresh.
• MCP. task() and chat() mark a replayed 200 body, and the 409 in-flight receipt, with idempotent_replay: true.
• Delegation contract (every agent's prompt, both byte-identical copies): one new line, Confirmed failed or cancelled: re-send word for word to retry; same execution_id back: set_reminder, end your turn. The contract length pin moves 1650 → 1730 (a deliberate raise). The chat_with_agent lead drops " (the primary way to use sub-agents)" so its published description stays under Claude Code's 2,048-char cap (2,030).

Behaviour change

• A verbatim re-send after the original run ended failed, cancelled or skipped, or whose execution row is gone, now starts a new run. A re-send while the run is queued, running or pending retry, or after it succeeded, still replays the original.
• A lease_expired failure keeps replaying through its window (the agent's timeout + 5 min after the failure), then a re-send starts a new run.
• sched: keys (the scheduler's own) are never reclaimed. A gated skill request is still answered by its gate record first.
• Backend and MCP ship independently: an old MCP server against a new backend just lacks the marker; a new MCP server against an old backend marks replays that still carry the old behaviour.

Coverage notes

• The replay seam also covers feat(pull): /chat on the durable queue for pull pilots (#3127) #3145's /chat queued_timeout receipt (pull pilots): there is a test that writes that receipt through the real run_pulled_chat_turn timeout branch, fails the run, and admits the identical retry fresh.
• Intent-scoped dedupe, tracked privately: the replay marker field is idempotent_replay (not replayed), and execution_liveness is a reusable "can this run still succeed?" helper for it.
• idempotent_replay is positive evidence only. Its absence does not prove a fresh dispatch: the client's own timeout-recovery receipts carry none.

Risks left (documented in docs/memory/architecture/execution.md §Idempotency)

• Phantom FAILED (bug: Execution shows Failed then Success after cleanup (race condition) #378). FAILED is not final, and it is not proof the agent stopped. SUCCESS can still overwrite a FAILED row that kept its async marker (the Phase-1 stale sweep's, for example), so a retry admitted after it can run beside an original that later lands SUCCESS, the same leftover as bug(mcp): the sync /task route has no queued_timeout receipt — the MCP gateway kills the held fetch and the caller sees 'fetch failed' while the target keeps running (#914, second route) #2661's sync release. And when the backend's own dispatch timeout cannot reach the agent to stop it, the original may keep working (its late result is rejected) while the retry runs.
• The lease_expired horizon is a heuristic, not a bound. It is anchored at the failure time (completed_at), because the reaper fires only after the slot TTL from acquire has already run out. Residuals: (1) the agent's restart sweep re-posts persisted results with no age limit, so a restart after the window can still land SUCCESS beside a fresh run; (2) only the two slot-reaper writers carry the tag (AST-pinned); the untagged sweeps go fresh at once; (3) the tag does not mean "async": sync push rows are tagged too, and the hold is conservative protection for every tagged row; (4) the horizon uses the agent's current timeout, and nothing wakes the caller when the window ends (the contract line tells it to set a reminder); (5) a late SUCCESS landing in the milliseconds between the liveness read and the delete can start a fresh run beside it.
• Effects can repeat across attempts. Effect dedupe is per execution (effect:{execution_id}), and a new attempt has a new id.
• Retry pile-up. The stale replay no longer brakes a caller that re-sends after every failure; bounded by the caller, the dispatch breaker and the event fire budget.
• Not fixed here: the in-flight wedge. A claim still in_flight when its run died keeps answering 409 for 24 h. A safe read-time reclaim needs owner tokens on claims first: bug: an /chat or /task idempotency claim left in_flight by a dead process answers 409 for 24 h #3298.

Tests

• tests/unit/test_3245_async_replay_after_failure.py (new): the liveness verdict matrix; the lease_expired time axis at the boundary (T+899 s replays, T+900 s and T+901 s fresh) on a fixed clock; the compare-and-delete (a slow retry cannot erase a faster retry's fresh record, both interleavings, plus a teeth case where an unconditional delete starts two runs); both seams; the pulled-/chat receipt; sched: and gated-skill carve-outs; AC2 end to end over the real execute_parallel_task (dispatch, fail, identical re-send gets a new execution_id with no replay header, a third send while running replays the new id); AST guards for seam wiring, the tag writers and "nothing writes a claim after its receipt".
• tests/unit/test_ent568_delegation_contract.py: parity, budget and classification with the new line.
• src/mcp-server/src/task-receipt.test.ts, delegation-contract.test.ts: the replay marker on task() / chat() / 409, absent without the header, and kept through runAgentChat.
• The replay-side AC1 cases (live / queued / pending_retry / success still replay) are guards: they are green on dev too.
• The idempotency DB tests run on SQLite only (CI has no PostgreSQL pytest), so there is no PostgreSQL proof of the new DELETE / SELECT. Both are plain SQLAlchemy expressions.

Local proof

Targeted only, per this PR's verification policy; CI runs the full suites. Local interpreter is Python 3.14 (CI is 3.13).

• Backend, new + touched-module tests (87 files: everything under tests/unit that exercises the admission seam, the idempotency service/DB layer, the platform prompt or the contract, plus the registry guard), pytest-randomly seeds 1 / 12345 / 99999: 2885 passed / 0 failed, 2884 / 1, 2885 / 0 (5 skipped each). Every failure and error is identical on a clean dev worktree with the same seeds: 20 setup errors in test_3210_room_oversized_reply.py (a conftest import-order issue, 35/35 alone) and one DNS-dependent test_736_a2a_outbound_call.py case on seed 12345.
• Teeth: the branch's test files run against clean dev fail as expected: backend 71 failed / 61 passed (132 passed on the branch); MCP marker tests 4 failed / 36 passed (40 passed on the branch). Reverting only the two seam calls turns 24 tests red on their assertions; dropping the CAS's execution_id predicate turns 5 red, its status predicate 1.
• MCP: npm test 729 / 729, npm run build exit 0.
• tests/lint_sys_modules.py, tests/lint_root_test_placement.py: exit 0.
• Not run: /verify-local, a live stack, PostgreSQL.

Follow-ups

• bug: an /chat or /task idempotency claim left in_flight by a dead process answers 409 for 24 h #3298: the in-flight wedge (claim owner tokens + a lease on agent-scope in_flight rows).

[AndriiPasternak31]

@webmixgamer: this PR changes the delegation contract from #3240, so I'd like your review of that part.

• New bullet, byte-identical in platform_prompt_service.DELEGATION_CONTRACT and src/mcp-server/src/delegation_contract.ts, after the "Read the result with get_execution_result…" bullet:
  - Confirmed failedorcancelled: re-send word for word to retry; same execution_idback:set_reminder, end your turn.
• Budget raise. The contract becomes 1,720 chars, so MAX_CONTRACT_CHARS in tests/unit/test_ent568_delegation_contract.py goes from 1650 to 1730. This is a deliberate raise.
• Lead trim. chat_with_agent's lead drops " (the primary way to use sub-agents)", so its published description stays under Claude Code's 2,048-char cap (2,030 published; 2,066 without the trim).
• Why "same execution_id back → set_reminder". A lease_expired failure (the slot reaper's) keeps replaying the original receipt for the agent's timeout + 5 min after the failure, because the run may still report success. An immediate re-send can therefore get the same id back. The bullet tells the caller to park and come back rather than loop.

[dolho]

/review Report

Branch: AndriiPasternak31/issue-3245 → dev (merge-base a72271d9c)
Files Changed: 27 (+1431/-43)
Scope: CLEAN
Plan Completion: 3 done / 0 partial / 0 not done / 0 changed / 0 unverifiable (issue #3245 acceptance criteria)

Intent: after an async run ends failed/cancelled, an identical request should dispatch a new execution (while a live or succeeded run still replays), and an MCP caller should be able to tell a replay from a fresh receipt.
Delivered: a read-time reclaim at both admission seams (begin_task_idempotency for /task, admit_chat_request for /chat) that compare-and-deletes a stored dispatch receipt whose execution ended or is gone, a lease_expired hold window, an idempotent_replay: true marker in the MCP client, one new delegation-contract line, and matching docs. The /chat queued_timeout coverage (pull pilots) is a natural extension of the same seam, not drift.

Acceptance criterion	Status	Evidence
Ended run → identical request dispatches fresh; live/succeeded still replays	DONE	dispatch_admission_service.py _reclaim_ended_receipt + both call sites; idempotency_service.execution_liveness
Test drives dispatch → terminal failure → identical re-send → new execution_id	DONE	TestAC2EndToEnd::test_dispatch_fail_resend_new_id_then_replay_the_new_run (over the real execute_parallel_task)
MCP caller can tell a replay from a fresh receipt	DONE	client.ts markIdempotentReplay on chat()/task() 200s and the 409 inFlightReplayReceipt

Execution coverage (Step 2.5)

changed symbol / test file	executed by	live consumer	verdict
_reclaim_ended_receipt	TestTaskSeamAC1, TestChatSeam, TestAC2EndToEnd	dispatch_admission_service.py (admit_chat_request, begin_task_idempotency)	✅ executed
execution_liveness / lease_expired_horizon	liveness matrix + fixed-clock time-axis tests	_reclaim_ended_receipt	✅ executed
discard_completed_if_execution / discard_replay_of	CAS interleaving tests on real SQLite	_reclaim_ended_receipt	✅ executed
get_execution_failure_stamp	liveness tests (real SQLite store)	execution_liveness	✅ executed
markIdempotentReplay	task-receipt.test.ts	TrinityClient.chat() / task()	✅ executed
AST guards (seam wiring, lease_expired tag writers, no claim written after a receipt)	test_3245_async_replay_after_failure.py	the seam / cleanup_service.fail_stale_slot_execution callers	🛡 guard (the behaviour is also executed above)
Delegation contract line (Python + TS copies)	test_ent568_delegation_contract.py, delegation-contract.test.ts	platform prompt + chat_with_agent description	✅ executed / 🛡 parity guard

Fix mutation (run locally in a scratch worktree, then restored):

• Both _reclaim_ended_receipt(...) seam calls replaced with no-ops → 24 tests red in test_3245_async_replay_after_failure.py, including the AC2 end-to-end case.
• execution_id == execution_id predicate dropped from the compare-and-delete → 5 tests red (the slow-retry/fast-retry interleavings).

Local runs: test_3245_async_replay_after_failure.py + test_ent568_delegation_contract.py 132 passed; with the neighbouring idempotency/admission/prompt suites (test_idempotency, test_chat_admission, test_2661_task_timeout_receipt, test_946_task_idempotency_on_deny, test_1422_webhook_idempotency, test_platform_prompt_*) under pytest-randomly: 265 passed, 4 skipped. MCP server: npm test 729/729, tsc --noEmit clean.

Critical Findings (block merge)

None.

Informational Findings (review required)

[I1] Test gaps: no PostgreSQL proof of the new DELETE/SELECT (Confidence: 6/10)
File: src/backend/db/idempotency.py (discard_completed_if_execution), src/backend/db/schedules/executions.py (get_execution_failure_stamp)
Issue: both run only against SQLite in CI. They are plain SQLAlchemy Core expressions with no dialect-specific constructs, and result.rowcount on a single-statement DELETE is reliable on psycopg, so the risk is low. The PR description already states this.
Suggestion: none required for merge; worth folding into whatever PostgreSQL pytest coverage lands next for the idempotency layer.

Clean Categories

• SQL & data safety: the new delete is parameterized and conditioned on scope, key, status = completed and execution_id; no unconditional form exists. The read is three columns by primary key.
• Race conditions: two retries racing on the same dead receipt start exactly one run (the loser's CAS matches nothing, its begin sees the winner's in_flight claim → 409 replay). Verified by mutation. The remaining late-SUCCESS window is documented in the PR and execution.md.
• Terminal writers (bug: recovery paths write execution terminals without closing the paired activity — agent shows as running for up to 2h, then logs a fabricated 120-min failure #1804): the check is read-time only; no terminal writer gains a side effect.
• lease_expired tag: confirmed the only execution-row writers carrying the tag are the two db.fail_stale_slot_execution calls in cleanup_service.py; the other tagged strings there are activity closes and event payloads. Tag spelling is parity-pinned against TaskExecutionErrorCode.LEASE_EXPIRED.
• Carve-outs: sched: keys are skipped; the skill gate runs before begin on /chat, so a gated request is still answered by its gate record.
• Fail-safe direction: every doubtful liveness read (lookup error, foreign owner, manual id, status flip between reads, missing timestamp) resolves to indeterminate, which replays; a delete error replays the original.
• Auth boundaries: no new endpoints or dependencies.
• Credential exposure: logs carry agent name and execution ids only.
• Enterprise disclosure (4.5): the enterprise-docs-guard pattern finds no hits in the added docs/ lines.
• Documentation staleness: idempotency-keys.md, execution.md, mcp-orchestration.md, the requirements area file and the user docs are updated with the behaviour change.
• MCP published description: the chat_with_agent lead was shortened so the published description stays under the 2,048-char cap; pinned by delegation-contract.test.ts.

Low confidence (appendix)

• (5/10) The new contract line ("Confirmed failed or cancelled: re-send word for word to retry…") sits beside bullets that also cover fan_out. /api/agents/{name}/fan-out stores its full result as the idempotency snapshot and is not covered by this reclaim, so a word-for-word fan_out re-send after failed children would still replay. The line does say execution_id, not fan_out_id, so it most likely reads as scoped to single delegations; mentioning it only in case a clarifying word is wanted.

Summary

• Critical: 0 — none found
• Informational: 1 — review recommended
• Scope: clean

🤖 Generated with Claude Code

[dolho]

Approved — /review found no blocking findings; see review comment above.

[vybe]

merge-train (2026-10-07): merged as part of train #3329 (green). Before the squash, dev was merged into this branch because an earlier sibling had appended to the same files (tests/registry.json and/or docs/memory/feature-flows*.md). Both sides were kept, nothing else was changed.