Skip to content

fix: delete throwaway sessions only after the turn settles (FOREIGN KEY race on aborted turns) - #90

Open
Futuri-Risk wants to merge 2 commits into
KochC:mainfrom
Futuri-Risk:fix/safe-session-teardown
Open

fix: delete throwaway sessions only after the turn settles (FOREIGN KEY race on aborted turns)#90
Futuri-Risk wants to merge 2 commits into
KochC:mainfrom
Futuri-Risk:fix/safe-session-teardown

Conversation

@Futuri-Risk

Copy link
Copy Markdown

Problem

Deleting throwaway sessions from catch/finally paths races the OpenCode server's final persist of the aborted turn: the DELETE /session/:id commits while the aborted turn's last insert into "message" is still in flight, so the insert fails against the deleted parent row:

EffectDrizzleQueryError: Failed query: insert into "message" (...)
cause: SQLiteError: FOREIGN KEY constraint failed

Any client that cancels a slow request mid-generation triggers it. Bulk clients make it a storm: a RAG pipeline (litellm, 54s timeout, retry loop) driving document ingest through the proxy produced 3,813 constraint errors across 3,546 distinct throwaway sessions in a single day — every error an aborted turn whose cleanup deleted its own session out from under the server's final write.

Repro (against a live proxy on the z.ai coding plan): 60 concurrent /v1/chat/completions calls, 4s client timeout, long-generation prompt → ~30 FK errors (~40% of cancels hit the race). After this patch: identical load → 0.

Root cause

executePrompt and runAgentTurn both delete the session in finally / catch paths — including when session.prompt() rejected on client abort. At that moment the server is still finalizing the aborted turn (persisting the partial assistant message); the delete wins the race and the pending insert fails with the FK constraint. The failure is pure noise pollution (the sessions are throwaways), but it is loud, log-flooding, and indistinguishable at a glance from real DB trouble.

Fix

Mirror the pattern used by long-lived SDK consumers (e.g. chat bots that keep one session per conversation and never delete mid-flight): only delete a session whose turn settled cleanly.

  • Non-streaming path: settled flag — delete only on the success path (prompt resolved, content extracted).
  • Streaming path: the three failure-path deletes (catch, errorMessage with no tool calls, messages-fetch failure) are removed; the final delete is additionally gated on !errorMessage (an errored turn with partial tool calls still lands at the result — it must leak too).
  • Abandoned sessions are intentionally leaked (inert rows) and reaped by sweepStaleProxySessions: at plugin start and every 6h (unref'd interval), Proxy:-titled sessions idle > 24h are deleted — long-idle deletes are race-free.
  • Sweep fails safe everywhere: unknown/missing/seconds-epoch time_updated values are left alone (ms-magnitude guard, so a units mismatch can never mass-delete fresh sessions), list() failure logs a warning, individual delete failures are skipped for the next sweep.
  • OPENCODE_LLM_PROXY_KEEP_SESSIONS=true behavior unchanged (still never deletes; note kept sessions are also swept after 24h idle — documented in README).
  • README gains a "Session lifecycle (safe teardown)" section documenting the leak-and-reap lifecycle, the reserved Proxy: title prefix, the first-run backlog note, and the keepSessions caveat.

Tests (10 new, suite 250/250 green)

  • non-streaming success → deletes
  • non-streaming client abort → does not delete (regression pin; fails on old code)
  • streaming success → still deletes
  • streaming client abort → does not delete (the reported path; fails on old code)
  • streaming session.error → does not delete
  • keepSessions=true → never deletes, even on success
  • sweep: reaps only stale (ms-epoch) Proxy: sessions; fresh / non-Proxy / unknown-age / seconds-epoch are kept
  • sweep: one failing delete doesn't stop the rest
  • sweep: no-op / warn-only when session.list is missing or throws

Notes for review

  • Title-prefix heuristic: sessions are matched by the exact prefix this plugin itself assigns at creation (Proxy: ${model.id}). A user-titled session starting with Proxy: would also be swept after 24h idle — the README now documents the prefix as reserved. Happy to switch to a rarer sentinel (+ one-release dual-prefix sweep) if you prefer; I kept the current title for backwards compatibility with sessions already leaked in the wild.
  • Backlog: the first plugin start after upgrading will sweep any previously leaked backlog in one pass (sequential deletes, error-tolerant).
  • sweepStaleProxySessions is exported for testability (the plugin start path needs Bun.serve, unavailable under node --test) — marked internal in its doc comment.

Fixes the constraint-error storms reported by bulk/retrying clients (cognee + litellm in our case). The deeper ordering question — whether DELETE /session/:id should quiesce a session's in-flight writes before destroying the parent row — belongs upstream in OpenCode itself; we also observed a small pre-plugin baseline (~20-40/day) of the same FK error from non-proxy deletions, which suggests the server-side race exists independently of this plugin.

Futuri-Risk added 2 commits August 20, 2026 07:39
Deleting from catch/finally paths races the OpenCode server final persist
of the aborted turn: the delete commits while the turn last message insert
is still in flight, so the insert fails with FOREIGN KEY constraint failed
against the deleted session row. Bulk clients that cancel slow streams
(RAG pipelines with aggressive timeouts and retry loops) turned this into
thousands of constraint errors per day (3.8k observed on one ingest day,
~40% of cancels hitting the race under a 60-cancel repro).

- non-streaming path: delete gated on a settled flag (success only)
- streaming path: failure-path deletes removed; happy-path deletes kept
- leaked sessions are reaped by sweepStaleProxySessions at plugin start
  (Proxy: titled, idle > 24h; long-idle deletes are race-free)
- OPENCODE_LLM_PROXY_KEEP_SESSIONS=true behavior unchanged

This mirrors how long-lived SDK consumers (chat bots keeping one session
per conversation) avoid the race entirely: never delete a session whose
turn has not settled.
client.session.delete() accepted no abort signal and, on the happy path,
runs inside the request finally BEFORE the concurrency slot releases. A
hung SDK connection there blocks the response forever and permanently
leaks the slot; with the default 8 slots, eight such hangs wedge the
proxy: /v1/models (unmetered GET) keeps answering while every completion
hangs. Observed in production for ~9.5h until a process restart.

- deleteSession now passes the request signal into the SDK call and
  races it with a timer (default 5s, OPENCODE_LLM_PROXY_DELETE_TIMEOUT_MS,
  read per-call; on timeout give up - the stale-session sweep reaps)
- both happy-path call sites pass options.signal
- regression test: a never-resolving session.delete must not block the
  response past the timeout
@Futuri-Risk

Copy link
Copy Markdown
Author

Added a second commit to this branch — same lifecycle concern, discovered in production two days after the first commit:

The wedge: client.session.delete() accepted no abort signal and runs in the request finally BEFORE the concurrency slot releases. One hung SDK connection = one slot leaked forever (the 120s request timeout cannot fire inside a signal-less call — it never errored, it just sat). Eight leaks later the limiter is pinned: /v1/models (unmetered GET) keeps answering while every completion hangs. We ran in that state ~9.5h; external clients saw pure hangs because queued waiters only clean on abort.

The fix (this commit): pass the request signal into the SDK delete and race it with a timer — default 5s, env-tunable OPENCODE_LLM_PROXY_DELETE_TIMEOUT_MS (read per-call, mirroring the other knobs). On timeout, give up: the stale-session sweep from the first commit reaps the session later, so bounded-abandon is safe here specifically because of the leak-and-reap design.

Test: a never-resolving session.delete must not block the response past the timeout (uses the env knob at 250ms). Suite: 251/251.

Happy to split this into its own PR if you prefer reviewing the two commits separately — they are independently revertable.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant