Repository navigation
fix(gateway): keep create_thread idempotent when the insert loses a race - #3800
WillemJiang merged 3 commits into
Conversation
The /api/threads create endpoint checks for an existing row and then inserts, but the two steps are not atomic. When two requests create the same thread_id concurrently, one commits first and the other's INSERT fails on the duplicate primary key. The SQL-backed thread store raises, the handler catches it as a generic failure, and the loser gets a 500 — even though the docstring promises the call is idempotent. Re-read the row after a failed insert: if it is now present (the competing request won), return it instead of surfacing the conflict. A genuine write failure where the row is still absent keeps the 500. Covered by a regression test that simulates the lost race and asserts the endpoint returns the existing thread rather than erroring. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
willem-bd
left a comment
There was a problem hiding this comment.
Review — the race fix is correct and the regression test is well-constructed. Two notes inline: the re-read runs under a broad except Exception (masking/logging concern) and the error path skips the owner reconciliation the happy-path idempotency block performs.
| # and the store then rejects ours on the duplicate primary key. | ||
| # Honour the documented idempotency contract by returning the | ||
| # now-existing record instead of surfacing the conflict as a 500. | ||
| existing_record = await thread_store.get(thread_id, **thread_owner_kwargs) |
There was a problem hiding this comment.
This re-read runs under a broad except Exception, so any non-race failure that coincides with an existing row is silently swallowed as 200 — with no log.
The comment scopes the fix to the duplicate-key IntegrityError, but the except Exception: above was already broad and the new re-read runs for every exception type. Failure scenario: thread_store.create() raises an OperationalError (connection dropped after the INSERT committed), or a different constraint/CHECK violation, while a pre-existing row for that thread_id is present (a prior legitimate create, or a client retry that already got a row). The handler now returns that row as 200 and, unlike before, never logs the exception — logger.exception only runs in the fall-through 500 branch, so genuine failures become invisible to ops.
The codebase's own canonical pattern catches IntegrityError specifically (backend/app/gateway/auth/repositories/sqlite.py:74). Narrowing here (catch IntegrityError only) or at minimum logging the swallowed exception at warning/debug would preserve observability. Minor aside: the PR says this mirrors channel_connections.upsert_connection, but that IntegrityError-as-"someone else committed" handling actually lives in auth/repositories/sqlite.py, and it catches IntegrityError, not Exception.
| # now-existing record instead of surfacing the conflict as a 500. | ||
| existing_record = await thread_store.get(thread_id, **thread_owner_kwargs) | ||
| if existing_record is None and thread_owner_user_id: | ||
| existing_record = await thread_store.get(thread_id, user_id=None) |
There was a problem hiding this comment.
The error-path re-read skips the update_owner reconciliation the happy-path idempotency block performs — so ownership of the same thread diverges by timing.
The initial idempotency block (~lines 267-273) claims an unscoped record for a trusted internal owner via update_owner when unscoped_record.user_id != thread_owner_user_id. This error-path block only does the unscoped get and returns directly — no update_owner.
Failure scenario: a channel adapter (trusted internal owner B) and an ownerless request race to create thread X; the ownerless request commits first, so B's INSERT loses and hits this error path, which returns the row still owned by None. Had B's request arrived slightly earlier and found the row via the happy path, the thread would have been re-assigned to B. The same logical "create X as owner B" therefore yields different ownership (and thus different access isolation) depending on insert ordering. If the omission is intentional it's worth a comment; otherwise the two paths should reconcile ownership identically. (Extracting the duplicated ThreadResponse(...) construction into a _record_to_response(...) helper would keep the two idempotency paths trivially identical and prevent this kind of drift.)
|
@he-yufeng Do you mind taking a look at the review comments? |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the Gateway’s POST /api/threads endpoint so that concurrent creates for the same thread_id remain idempotent (returning the existing thread instead of intermittently 500-ing when an insert loses a uniqueness race).
Changes:
- Add a recovery path on failed thread meta insert to re-read and return the now-existing thread record.
- Add a regression test that simulates “commit happened elsewhere, then our insert raises
IntegrityError” to verify idempotent behavior under races.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
backend/app/gateway/routers/threads.py |
Re-reads thread meta after a failed insert to preserve create-thread idempotency under concurrent inserts. |
backend/tests/test_threads_router.py |
Adds a regression test covering the insert-race/idempotency scenario. |
| except Exception: | ||
| # The idempotency ``get`` above and this insert are not atomic: a | ||
| # concurrent request for the same thread_id can commit in between, | ||
| # and the store then rejects ours on the duplicate primary key. | ||
| # Honour the documented idempotency contract by returning the | ||
| # now-existing record instead of surfacing the conflict as a 500. | ||
| existing_record = await thread_store.get(thread_id, **thread_owner_kwargs) | ||
| if existing_record is None and thread_owner_user_id: | ||
| existing_record = await thread_store.get(thread_id, user_id=None) | ||
| if existing_record is not None: | ||
| return ThreadResponse( | ||
| thread_id=thread_id, | ||
| status=existing_record.get("status", "idle"), | ||
| created_at=coerce_iso(existing_record.get("created_at", "")), | ||
| updated_at=coerce_iso(existing_record.get("updated_at", "")), | ||
| metadata=existing_record.get("metadata", {}), | ||
| ) | ||
| logger.exception("Failed to write thread_meta for %s", sanitize_log_param(thread_id)) | ||
| raise HTTPException(status_code=500, detail="Failed to create thread") |
…nciliation Address review on bytedance#3800: - Scope the insert-race recovery to sqlalchemy IntegrityError instead of a broad `except Exception`, so a non-race failure that coincides with an existing row is no longer silently returned as a 200. Any other error logs and surfaces as a 500. (The memory store overwrites on duplicate rather than raising, so it never reaches this path.) - Route both the fast path and the recovery path through a shared `_resolve_existing_thread` helper so the recovery performs the same trusted- owner reconciliation (claiming a legacy unscoped `user_id=None` row) the fast path does. Thread ownership no longer diverges based on which path resolved the record. Add regression tests for both: the recovery claims an unscoped row for a trusted owner, and a non-IntegrityError failure surfaces as a 500.
|
Thanks @WillemJiang, and thanks to the reviewer for catching both of these; they were real gaps. Pushed 6c01960:
Added two regression tests: one drives the insert-race interleaving so the recovery path (not the fast path) is the one that claims the unscoped row for a trusted owner, and one asserts a non- |
…#4080 bytedance#3800) - bytedance#4072: window LoopDetection per-tool frequency counter (deque + Counter sized to largest hard limit) so long runs no longer false-trip; monotonic _tool_freq replaced. Also clear mirrored _tool_name_counter on evict/reset (upstream leaves it stale). - bytedance#4080: drop orphan ToolMessages (no matching AIMessage tool_call) in DanglingToolCallMiddleware so strict providers don't 400. - bytedance#3800: create_thread idempotent on insert race - catch IntegrityError, re-read existing record instead of 500 (owner-reconciliation half N/A: contextvar owner scoping). - bytedance#4193 skipped: depends on tool-name sanitization infra (bytedance#4119) absent in our fork. - bytedance#4064 deferred: ~500-line run-lifecycle lease rewrite (cancel bool->CancelOutcome), needs dedicated design. Verified: loop_detection 65 + dangling 28 + threads 22 tests pass.
…ace (bytedance#3800) * fix(gateway): keep create_thread idempotent when the insert loses a race The /api/threads create endpoint checks for an existing row and then inserts, but the two steps are not atomic. When two requests create the same thread_id concurrently, one commits first and the other's INSERT fails on the duplicate primary key. The SQL-backed thread store raises, the handler catches it as a generic failure, and the loser gets a 500 — even though the docstring promises the call is idempotent. Re-read the row after a failed insert: if it is now present (the competing request won), return it instead of surfacing the conflict. A genuine write failure where the row is still absent keeps the 500. Covered by a regression test that simulates the lost race and asserts the endpoint returns the existing thread rather than erroring. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * fix(gateway): scope create_thread race recovery and mirror owner reconciliation Address review on bytedance#3800: - Scope the insert-race recovery to sqlalchemy IntegrityError instead of a broad `except Exception`, so a non-race failure that coincides with an existing row is no longer silently returned as a 200. Any other error logs and surfaces as a 500. (The memory store overwrites on duplicate rather than raising, so it never reaches this path.) - Route both the fast path and the recovery path through a shared `_resolve_existing_thread` helper so the recovery performs the same trusted- owner reconciliation (claiming a legacy unscoped `user_id=None` row) the fast path does. Thread ownership no longer diverges based on which path resolved the record. Add regression tests for both: the recovery claims an unscoped row for a trusted owner, and a non-IntegrityError failure surfaces as a 500. --------- Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
…ace (bytedance#3800) * fix(gateway): keep create_thread idempotent when the insert loses a race The /api/threads create endpoint checks for an existing row and then inserts, but the two steps are not atomic. When two requests create the same thread_id concurrently, one commits first and the other's INSERT fails on the duplicate primary key. The SQL-backed thread store raises, the handler catches it as a generic failure, and the loser gets a 500 — even though the docstring promises the call is idempotent. Re-read the row after a failed insert: if it is now present (the competing request won), return it instead of surfacing the conflict. A genuine write failure where the row is still absent keeps the 500. Covered by a regression test that simulates the lost race and asserts the endpoint returns the existing thread rather than erroring. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * fix(gateway): scope create_thread race recovery and mirror owner reconciliation Address review on bytedance#3800: - Scope the insert-race recovery to sqlalchemy IntegrityError instead of a broad `except Exception`, so a non-race failure that coincides with an existing row is no longer silently returned as a 200. Any other error logs and surfaces as a 500. (The memory store overwrites on duplicate rather than raising, so it never reaches this path.) - Route both the fast path and the recovery path through a shared `_resolve_existing_thread` helper so the recovery performs the same trusted- owner reconciliation (claiming a legacy unscoped `user_id=None` row) the fast path does. Thread ownership no longer diverges based on which path resolved the record. Add regression tests for both: the recovery claims an unscoped row for a trusted owner, and a non-IntegrityError failure surfaces as a 500. --------- Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Why
While reading the thread gateway I noticed
POST /api/threadsdoes a check-then-insert: it callsthread_store.get()to honour idempotency, and if the row is absent it callsthread_store.create(). Those two steps aren't atomic. If two requests create the samethread_idat the same time (e.g. a channel adapter or orchestration layer retrying on a timeout with a client-chosen id), one commits first and the second's INSERT hits the duplicate primary key. The SQL-backed store raises, the handler treats it as a generic write failure, and the losing request gets a 500 — even though the endpoint's own docstring promises the call is idempotent ("returns the existing record whenthread_idalready exists").What changed
On a failed insert the handler now re-reads the row. If it's present (the competing request won the race), it returns that record, so concurrent creates of the same thread are idempotent from the caller's side instead of one of them 500-ing. A genuine write failure where the row is still absent keeps returning 500 as before. This mirrors how
channel_connections.upsert_connectionalready treats a concurrentIntegrityErroras "someone else committed it" rather than a hard error.Surface area
POST /api/threadserror path underbackend/app/gatewayScreenshots / Recording
Not applicable — backend only.
Bug fix verification
backend/tests/test_threads_router.py::test_create_thread_returns_existing_when_insert_loses_racemainand green on this branch? yes — onmainthe endpoint returns 500 (theIntegrityErroris caught as a generic failure); on this branch it returns the existing thread (200). The test drives the endpoint with a store whose firstcreate()commits the row and then raises, simulating the lost race.Validation
Run from
backend/:uv run pytest tests/test_threads_router.py— 25 passed (includes the new regression test)uv run ruff check app/gateway/routers/threads.py tests/test_threads_router.py— cleanuv run ruff format --check app/gateway/routers/threads.py tests/test_threads_router.py— cleanAI assistance
Tool(s) used: Claude Code
How you used it: Used it to help locate the check-then-create window and draft the regression test; I reviewed the data flow, confirmed the SQL backend raises on the duplicate key while the memory backend overwrites, and decided on the re-read-on-conflict approach myself.