Skip to content

fix(api): prevent duplicate run/approval processing on client retries - #89754

Open
RoySRose wants to merge 1 commit into
NousResearch:mainfrom
RoySRose:fix/runs-approval-admission-idempotency
Open

RoySRose wants to merge 1 commit into
NousResearch:mainfrom
RoySRose:fix/runs-approval-admission-idempotency

Conversation

@RoySRose

Copy link
Copy Markdown

POST /v1/runs has a window before any run state exists where a client
retry (e.g. after a timeout) can be admitted twice, starting the same
agent run concurrently. This isn't just a duplicate HTTP response: it
means the LLM call and any side effects it triggers (tool calls,
external actions) execute twice for a single logical request. Approval
resolution over the run's event stream has the same class of bug on
retry — a retried resolve call can process the same approval decision
twice.

This adds bounded, TTL-capped keyed admission on both endpoints:

  • Clients may send an Idempotency-Key header. The first request with
    a given key reserves an admission record before any async-visible
    state is created; a fingerprint of the semantically relevant inputs
    is stored alongside it.
  • A retry with the same key and matching fingerprint replays the
    cached outcome instead of reprocessing. A retry with the same key
    but a different fingerprint is rejected with 409, since reusing that
    key for a different request is unsafe.
  • A new GET /v1/runs/meta endpoint exposes a per-process server
    generation UUID, so clients can detect a server restart (which
    invalidates prior admission bookkeeping) via an optional
    X-Hermes-Expected-Generation header instead of assuming stale
    state is still valid.
  • Both admission tables are capped and swept on TTL, so retrying
    clients add bounded memory, not unbounded growth.

/v1/runs/meta is registered before the existing /v1/runs/{run_id}
route. aiohttp's UrlDispatcher resolves routes in registration
order, so the dynamic route would otherwise swallow the literal
meta path segment.

Testing

Added TestStartRunIdempotency and two new cases in TestRunEvents
covering: duplicate-run prevention on retry, 409 on key reuse with a
changed body, 400 on an empty Idempotency-Key header, the new
/v1/runs/meta endpoint, generation-mismatch handling, and the
equivalent retry/409 behavior for approval resolution.

  • tests/gateway/test_api_server_runs.py: 29/29 passing (22
    pre-existing + 7 new).
  • Full tests/gateway/ suite: no new failures relative to baseline.
  • All 7 new tests verified to fail against pre-fix code and pass
    against the fix.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 19, 2026
POST /v1/runs has a window before any run state exists where a
client retry (e.g. after a timeout) can be admitted twice, starting
the same agent run concurrently. This isn't just a duplicate HTTP
response: it means the LLM call and any side effects it triggers
(tool calls, external actions) execute twice for a single logical
request. Approval resolution over the run's event stream has the
same class of bug on retry.

Add bounded, TTL-capped keyed admission on both endpoints:

- Clients may send an Idempotency-Key header. The first request with
  a given key reserves an admission record before any async-visible
  state is created; a fingerprint of the semantically relevant
  inputs is stored alongside it.
- A retry with the same key and same fingerprint replays the cached
  outcome instead of reprocessing. A retry with the same key but a
  different fingerprint is rejected with 409, since that key is no
  longer safe to reuse for a different request.
- A new GET /v1/runs/meta endpoint exposes a per-process server
  generation UUID, so clients can detect a server restart (which
  invalidates any prior admission bookkeeping) via an optional
  X-Hermes-Expected-Generation header rather than assuming stale
  state is still valid.
- Both admission tables are capped and swept on a TTL so retried
  clients are bounded memory, not unbounded growth.

/v1/runs/meta is registered before the existing /v1/runs/{run_id}
route in the route table; aiohttp's UrlDispatcher resolves routes in
registration order, so the dynamic route would otherwise swallow the
literal "meta" path segment.

The concurrency cap check (_concurrency_limited_response) is moved
past the idempotency lookup so a replayed retry is served from cache
even if capacity has since filled; only genuinely new admissions are
subject to it.

Adds a regression test class covering: duplicate-run prevention on
retry, 409 on key reuse with a changed body, 400 on an empty
Idempotency-Key header, the new /v1/runs/meta endpoint, generation
mismatch handling, and the equivalent retry/409 behavior for
approval resolution.
@RoySRose
RoySRose force-pushed the fix/runs-approval-admission-idempotency branch from 82ca347 to ca0fc70 Compare August 19, 2026 05:38

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head ca0fc705fdede51ae61e5c91c5ac0e5ae252677c against PR base 4903993cdd9747a8a0e11d4022d88d1e20b33eb4 and current main 5d3c15aaa776cfcb4c88fb6e4f0431a95387eeab. I inspected the full two-file diff, the actual /v1/runs execution inputs, multiplex profile boundary, focused tests, exact-head Actions state, prior run-idempotency work, and the parallel run-stream ordering PR.

The placement is right: reserving before run-visible state/task creation closes the duplicate-side-effect window, replay-before-concurrency is the correct ordering, approval resolution needs its own receipt, and the process-generation token makes the intentionally process-local contract explicit. I found two blockers in the identity/fingerprint contract.

1. The run fingerprint omits inputs that actually change the run

_run_admission_fingerprint() hashes input, instructions, previous_response_id, conversation_history, session_id, model, and the gateway session key. But this same handler parses additional execution-semantic inputs through _request_agent_overrides() and passes them into _create_agent():

  • top-level provider → requested_provider
  • model_options → runtime options (reasoning/service-tier/etc.)

Those fields are not in the admission fingerprint. A client can therefore reuse one key with the same model/prompt but a different provider or different model options and receive the original 202/run_id instead of the required 409, even though the second request asked Hermes to execute a materially different run.

Concrete witness:

Idempotency-Key: same
{"input":"x","model":"m","provider":"openai","model_options":{"reasoning_effort":"low"}}

Idempotency-Key: same
{"input":"x","model":"m","provider":"anthropic","model_options":{"reasoning_effort":"high"}}

The current digest is identical for those bodies after selecting its semantic subset; _create_agent() is not.

Please fingerprint the normalized effective admission inputs rather than a partial raw-body subset — at minimum the normalized requested provider/model/options plus the existing conversation/session inputs — and add changed-provider and changed-model_options conflict tests.

2. Raw Idempotency-Key is process-global across multiplex profiles

The listener is multiplex-aware: authentication resolves against _api_request_profile, and the background run captures that profile and re-enters its runtime scope before agent creation. The new admission registry, however, is Dict[str, ...] keyed only by the raw Idempotency-Key, and the fingerprint contains no request-profile identity.

So two independently authenticated profiles can collide:

POST /p/alpha/v1/runs   Idempotency-Key: retry-1   {"input":"hello"}
POST /p/beta/v1/runs    Idempotency-Key: retry-1   {"input":"hello"}

The second request can replay alpha's run_id instead of starting beta's run. This crosses an existing ownership boundary: the first run executes under alpha's captured profile, while the replay decision is made from a shared process table before checking any receipt owner.

This is also a regression relative to the earlier open implementation in #84468 by @Jags3Alpha, which explicitly keys run idempotency by (request_profile, idempotency_key) and fingerprints the complete body plus authenticated session key. #89754 is broader — it adds approval idempotency and generation recovery — so it can supersede that implementation, but it should preserve the profile-scope invariant that prior work already identified.

Please namespace run admissions by canonical request profile/auth scope and bind that scope into the receipt/fingerprint. The approval registry should use the same namespace: its run_id fingerprint prevents a wrong replay today, but a raw-key collision in another profile still produces an avoidable cross-profile 409/DoS. Add a two-profile same-key witness proving independent admissions.

Interlocks / merge order

  • #84468 / @Jags3Alpha: earlier, narrower /v1/runs idempotency implementation. #89754 is a legitimate broader superseder, but should retain its profile-scoped key ownership rather than regress it.
  • #61960 / @Chavezrene: complementary durable idempotency for persisted session-chat. Different endpoint/lifetime contract, but its (scope, authenticated principal, session, key) receipt identity is the same ownership lesson.
  • #89748 / @RoySRose: complementary run-event ordering repair on the exact same api_server.py / test_api_server_runs.py surface. Neither supersedes the other; whichever lands second needs a rebase/composed run-suite pass because both modify _make_run_event_callback and the runs tests.

CI / current-main state

The exact-head CI, Nix, and Docker workflow runs currently conclude action_required, and the CI run exposes no executed jobs, so there is no exact-head repository matrix to credit yet. Current main has also advanced/diverged since this head.

Re-review gate: close the effective-input fingerprint gap, scope both admission registries to the multiplex ownership boundary, add the adversarial provider/options + cross-profile tests, rebase onto current main (including any #89748 composition that lands first), and attach an executed exact-head matrix.

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

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants