Skip to content

fix(api): make run starts idempotent - #84468

Open
Jags3Alpha wants to merge 3 commits into
NousResearch:mainfrom
Jags3Alpha:agent/codex/runs-idempotency-20260812
Open

Jags3Alpha wants to merge 3 commits into
NousResearch:mainfrom
Jags3Alpha:agent/codex/runs-idempotency-20260812

Conversation

@Jags3Alpha

@Jags3Alpha Jags3Alpha commented Aug 12, 2026 •

Copy link
Copy Markdown

Problem

POST /v1/runs accepted Idempotency-Key but did not use it to recover a lost 202 response. A retry could start duplicate work, accept a changed payload, or return 429 instead of recovering the original run at concurrency capacity.

Fix

  • profile-scope each key and fingerprint the JSON body plus authenticated session key
  • replay the original 202/run ID before the concurrency ceiling
  • reject changed-payload reuse with 409 idempotency_conflict
  • retain current main's pre-body-parse concurrency guard for genuinely new work
  • bound keys to 255 characters and fail closed at 1,000 live entries
  • expire entries with their pollable run status

No-key behaviour is unchanged. Recovery is deliberately process-local; crash-persistent recovery is not claimed.

Exact verification

  • Current exact head: 8258982f1403e3914d85d577409a1d5a77c692e8
  • Merged upstream main at branch repair: 7626105380b78bd360a20fe3ea3752046efc4a45
  • Repository runner, Runs suite: 23/23 pass
  • Ruff: pass
  • Diff check: pass
  • Exact-head CI 31671074262 and Docker 31671073897 are action_required; maintainer workflow approval and review remain required.

Latest disposable composition of upstream main c7a1bfea07762b82ef47e0ba31042963dc359490 -> this functional series -> #84931 functional series produced 8a24991f80d469d293f80f70f0fb56177bd16e19 (tree 93d06996facbfc61e1aa39e18d2d27cbe0f46a8b): 136/136 focused Runs, reconnect and MCP tests pass; Ruff and diff checks pass.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades needs-decision Awaiting maintainer decision before any implementation labels Aug 12, 2026
@Jags3Alpha
Jags3Alpha marked this pull request as ready for review August 12, 2026 10:33
@Jags3Alpha

Copy link
Copy Markdown
Author

Current-upstream composition evidence (source only; no merge/deployment authority):

Both merges were conflict-free. scripts/run_tests.sh passed 132/132 tests across test_api_server_runs.py, test_mcp_tool.py and test_mcp_tool_session_expired.py; Ruff 0.15.10 and git diff --check passed. The disposable worktree was removed and nothing synthetic was pushed or deployed.

This clears current-source drift only. Maintainer review, upstream checks/merges, exact hosted deployment and injected lost-response proof remain required.

@Jags3Alpha

Copy link
Copy Markdown
Author

Exact current-main composition verification (read-only/disposable; no production action):

Command used the repository-mandated scripts/run_tests.sh; gateway tests used the declared dev + messaging extras. The disposable worktree was removed afterwards. This is source-level evidence only; it does not prove hosted runtime or iPhone behaviour.

@Jags3Alpha

Copy link
Copy Markdown
Author

Exact-head maintainer handoff for 8258982f1403e3914d85d577409a1d5a77c692e8:

  • rebased/merged current upstream main 7626105380b78bd360a20fe3ea3752046efc4a45
  • repository runner: 23/23 focused Runs tests pass
  • Ruff and diff check pass
  • exact current-main composition with fix(mcp): preserve tool attempt identity across retries #84931 exact 9d629a8592c695f991b905a180afd751373ae983: 30201ee1530f3066b44d7db3e8124d79c4b366e5 / tree e9cdce9d67b8aa03d33b74b30c67a4d84710df54; 136/136 focused Runs/reconnect/MCP tests pass

Please approve fork workflows for this exact head and review; no merge is requested by this comment.

@Jags3Alpha

Jags3Alpha commented Aug 13, 2026 •

Copy link
Copy Markdown
Author

Fresh disposable composition after upstream main advanced again:\n\n- upstream base: 9460cc1\n- PR #84468 head: 8258982\n- PR #84931 head: 6414315\n- composition commit: e144552ba91205efe4814b5cec658d0d83f96d1c\n- composition tree: 5a424861f2537615151ecffe9262b0dd6f492de2\n- merge: clean\n- focused Runs/reconnect/MCP suite: 136/136 pass\n- Ruff: pass\n- git diff --check: pass\n\nThe intervening upstream changes do not touch gateway/platforms/api_server.py or tools/mcp_tool.py. This refresh supersedes the prior composition evidence. It is read-only source-compatibility evidence only and does not claim maintainer review, workflow approval, merge, deployment or live Hermes proof.

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(api): make run starts idempotent

Reviewed gateway/platforms/api_server.py and the new tests. The fingerprint-based conflict detection, capacity bounds, and replay-bypasses-concurrency behavior are all well tested.

A few observations:

  1. Idempotency window is bound by the run-status TTL (~1 hour). An Idempotency-Key replay whose run status was swept by _sweep_orphaned_runs_once (after _RUN_STATUS_TTL = 3600s) is treated as brand-new work and starts a second run — duplicate work after ~1h. That's a deliberate memory bound, but it means the at-least-once guarantee only holds for about an hour; a client that retries later (long network timeouts, deferred sync) can double-start. Worth documenting the window or decoupling the two lifecycles.

  2. Replay always returns "status": "started" with 202, even when the original run has already completed. A client that replays after completion gets a misleading status instead of the run's actual terminal state. Consider echoing the current entry from _run_statuses when present.

  3. Minor: a fresh idempotency key on new work passes through the pre-parse concurrency check and the post-parse check (both see prior_idempotent_run is None) — redundant but harmless; if the intent is "limit only genuinely new work", the double-check could be clarified.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants