Skip to content

fix(api): preserve legacy client run IDs with durable admission - #104892

Open
Joeywrz wants to merge 1 commit into
NousResearch:mainfrom
Joeywrz:fix/legacy-client-run-id-followup
Open

Joeywrz wants to merge 1 commit into
NousResearch:mainfrom
Joeywrz:fix/legacy-client-run-id-followup

Conversation

@Joeywrz

@Joeywrz Joeywrz commented Sep 7, 2026 •

Copy link
Copy Markdown

What does this PR do?

Preserve UUID-shaped client run_id values on POST /v1/runs using the existing durable, profile/credential-scoped idempotency store.

Legacy thin gateways retry the body ID after a lost 202 without sending Idempotency-Key. On base c4a5deeffa959f8ebc891e02c5bcf767aff1dd2a, that ID is ignored and a new server ID is generated. This change adds only the compatibility adapter; it does not introduce another retry store or an exactly-once execution guarantee.

Related Issue

No separate open issue is claimed as fixed. Related predecessor and overlapping work:

The client-ID use case was previously proposed in #34399 by @lepapillonterrible; that PR was closed unmerged. This candidate keeps the narrower UUID-shaped syntax from Joey's locally carried compatibility work and corrects a transaction-close defect in that implementation.

#83253, #89754 and #84139 cover adjacent header-based admission/correlation work. Live duplicate searches found no equivalent open body-ID adapter; this is not a claim of exhaustive search. If another admission change lands first, rebase and rerun against its authoritative store rather than introduce parallel machinery.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/platforms/api_server_runs.py: client-ID validation, reuse of scoped durable admission, and replay after concurrent history loading.

  • gateway/platforms/api_server_run_idempotency.py: reject cross-key/scope run-ID collisions and close the transaction on the conflict path.

  • tests/gateway/test_api_server_client_run_id.py: HTTP, durable ownership, follow-on admission and synchronized simultaneous-retry regressions.

  • website/docs/user-guide/features/api-server.md: accepted ID syntax, header precedence, conflicts and retention limits.

  • Accept body IDs matching run_[0-9a-f]{32} and reject malformed IDs before allocating run state.

  • Without a nonblank header, reserve legacy-run:<run_id> through the existing request fingerprint and authenticated scope.

  • With both fields present, the header selects the idempotency key and the body selects the run ID. Mismatched bodies/session keys or an ID already owned by another key/scope fail closed.

  • Reject durable run-ID collisions inside the insertion transaction and commit before returning conflict, keeping the next lookup/admission usable.

  • Document the precedence and retention boundary. No schema, dependency, config, or model-tool changes.

How to Test

  1. On the unpatched base, POST a UUID-shaped body run_id to /v1/runs; the server returns a different generated ID. Use the regression cases below for an isolated localhost reproduction without a real provider.
  2. Run the canonical affected-area command below on this branch. The cases use real authenticated HTTP handlers and temporary SQLite; only the external agent is substituted.
  3. Verify preserved IDs, replay without duplicate dispatch, collision isolation and a usable store after conflict. The synchronized pair must return two 202s and launch once.

Regression coverage

Three invariant tests (six parameterized cases):

  • Real localhost HTTP with two authenticated adapters and separate connections to a shared temporary SQLite DB: preserved ID, retry without redispatch (including at capacity), header/body/session-key conflicts, durable collision, ownership hiding, unchanged persisted original record, successful follow-on admission, malformed-ID rejection.
  • Store collision across profiles or keys: original ownership retained, transaction closed, subsequent lookup and reservation succeed.
  • Simultaneous identical HTTP requests synchronized during session-history loading: both return 202, one replay header, one agent execution; explicit-header and body-only variants.

The second adapter has no live run-owner cache, so the HTTP collision exercises durable reservation rather than only the in-memory guard. The external LLM agent is replaced; no live provider or deployed gateway is contacted.

Local verification

  • RED on the pinned upstream: four cases failed (generated ID instead of requested ID; SQLite UNIQUE collision).
  • RED on the unchanged carried compatibility delta: four cases failed; the next HTTP admission returned 500 because the conflict left its transaction open.
  • A separate review exposed a false 409 for simultaneous identical body-ID requests. The deterministic regression failed in both variants before the scoped replay recheck and passed afterward.
  • Focused GREEN and broader canonical runner: 209 passed, 0 failed across eight files, retries disabled; final head cb794c42ff417e7da608681bc96ac1da84f1c3b0 received an independent delta review and a fresh six-case regression pass.
  • Ruff on the changed Python files and git diff --check: pass.
HERMES_TEST_FILE_RETRIES=0 \
  scripts/run_tests.sh \
  tests/gateway/test_api_server_client_run_id.py \
  tests/gateway/test_api_server_runs.py \
  tests/gateway/test_api_server_runs_extraction.py \
  tests/gateway/test_api_server_run_idempotency.py \
  tests/gateway/test_api_server.py \
  tests/gateway/test_api_server_profile_prefix_misdelivery.py \
  tests/gateway/test_api_server_multiplex_secret_scope.py \
  tests/gateway/test_api_server_room_grants.py -q

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate; adjacent work is listed above
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — not claimed. Local affected-area tests used the canonical scripts/run_tests.sh; the full local repository suite was not run. Hosted CI is reported separately below.
  • I've added tests for my changes
  • I've tested on my platform: macOS / Apple Silicon (arm64), Python 3.11; automated real-handler integration paths described above, not a live-provider/desktop end-user smoke test

Documentation & Housekeeping

  • I've updated relevant documentation — website/docs/user-guide/features/api-server.md updated
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys changed
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A, neither changed
  • I've considered cross-platform impact — no new OS-specific production primitives; scripts/check-windows-footguns.py --diff c4a5deeffa959f8ebc891e02c5bcf767aff1dd2a passed. Local execution was macOS; no local Windows/Linux execution claimed.
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A, no model-tool schema or description changed; the HTTP API contract is documented above

Screenshots / Logs

No UI change; no screenshot required. Local logs recorded 209 passing tests across eight files. No live-provider or real gateway crash/restart test was performed; no full local suite or docs build is claimed.

Hosted checks at exact head cb794c42ff417e7da608681bc96ac1da84f1c3b0 completed successfully:

CI success and local review are not maintainer approval or a merge claim.

@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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 7, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Summary (Non-blocking)

Lets legacy clients supply run_id in the body with durable, profile-scoped admission (legacy-run:<id> fallback key, cross-profile conflict on reuse). The in-transaction uniqueness check plus transaction-closing on conflict looks correct, and the tests (retry, cross-profile collision, simultaneous admission, invalid IDs) are thorough.

Notes

  • gateway/platforms/api_server_runs.py:45 — when a client sends both an explicit Idempotency-Key and a body run_id, the header selects the key. A later retry that omits the header derives a different key (legacy-run:...) with the same run_id and will get 409 rather than a replay. Docs (api-server.md) say retries must keep the same body and session key — worth adding "and the same header presence" to save legacy clients a confusing 409.
  • gateway/platforms/api_server_run_idempotency.py:11 — good that the run_id uniqueness check runs inside the same write transaction as the insert; otherwise two profiles reserving the same client ID concurrently could alias. The commit() before returning "conflict" (keeping the store usable, asserted in tests) is a nice touch.
  • gateway/platforms/api_server_runs.py:56 — the in-memory run_id in self._run_owners fast path races if two coroutines pass the check before either inserts; the durable reserve later is the real backstop, and the simultaneous-admission test covers the history-loading yield. No change needed; just confirming the durable path (not the dict) is treated as authoritative.
  • Validation (api-server.md, api_server_runs.py:38): uppercase hex rejected (run_AAAA... → 400, covered in tests). Clients generating IDs with uuid4().hex.upper() or hyphenated UUIDs will 400 — intended, but the error message could hint at the expected run_<32 lowercase hex> shape.

Adapt the client-ID compatibility change from local commit 6117359a604ee145592ce37dc79d73e10e7dc145 onto current upstream. Close the collision transaction before returning, so a rejected identity cannot poison later admission. Cover HTTP retry, conflicting identity, and follow-on admission with real SQLite storage.
@Joeywrz
Joeywrz force-pushed the fix/legacy-client-run-id-followup branch from cb794c4 to af49398 Compare September 18, 2026 19:04

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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants