[sprites 2-3] Real session termination via the kill endpoint

[2witstudios]

Summary

Every teardown path called kill('SIGKILL') on the client WebSocket handle — but TTY sessions have max_run_after_disconnect: 0 and keep running server-side after the socket closes. Nothing in the codebase called the documented kill endpoint (POST /v1/sprites/{name}/exec/{session_id}/kill), so "killed" panes could leak live sessions that later reattach paths rediscover and cross-wire.

This wires the real endpoint into both places that need genuine termination — the explicit killAgentTerminal API and the 30-min detached-idle reap — while preserving detach-never-terminates semantics.

Branched from current master (post #2038/#2039/#2041/#2042/#2031); verified against https://sprites.dev/api.

Convergence history: three review passes landed after the initial push — all self-driven, no human review yet requested beyond automated bots. (Commit hashes below reflect the post-rebase history — the branch was rebased onto master after these fixes landed, so earlier hashes cited in review-thread replies predate the rebase but refer to the same changes.)

1. Codex caught a P1: the kill URL was .../exec/sessions/{id}/kill (extra sessions/ segment, 404s against the real API) instead of the documented .../exec/{session_id}/kill. Since 404 was (correctly) treated as idempotent success, every real kill would have silently no-op'd. Fixed in 0be834f, along with draining the endpoint's streaming NDJSON response body (a 200 alone doesn't confirm the signal was delivered). See the review thread.
2. A multi-angle self-review (8 independent finder passes) surfaced three more issues, fixed in 1a3b7cb: (a) no retry on transient/cold-wake failures — added, since a kill is idempotent so blanket retry is safe; (b) withKillSession SDK-wrapping duplicated near-verbatim across apps/web/apps/realtime — hoisted into one shared sprites.ts export; (c) the 'user-kill' trigger name was misleading — its only real caller is a platform-forced teardown (revoked access / insolvent payer), never a literal click — renamed to 'forced-teardown'.
3. Codex caught a second P1: the kill endpoint defaults to SIGTERM when signal is omitted, but the old WS-based kill('SIGKILL') path this replaces always sent SIGKILL. A process that traps/ignores TERM would survive a default-signal call while killSpriteSession still reported success, silently removing row/session bookkeeping out from under a still-live, still-billable session. Fixed in 3ca3b3f by requesting ?signal=SIGKILL explicitly on every kill request. See the review thread.

Requirements

• Given a user-initiated terminal kill, should call the sessions kill endpoint for the exact session id AND close the socket.
  packages/lib/src/services/machines/agent-terminals.ts's killAtLocation now calls handle.killSession(row.streamSessionId) — a direct REST call, replacing the old open-a-stream-then-signal-then-corroborate-via-listStreams dance (that dance existed only because a WS signal is unreliable; the REST endpoint answers authoritatively on its own).

• Given a viewer detach (navigate away), should close/abandon the client socket only — never kill the session.
  sprites-shell.ts's new planTeardown({trigger: 'detach'}) returns {killSession: false, closeSocket: false}. agent-terminal-handler.ts's disconnectConnection main body (immediate detach, not the idle timer) never calls PtyShell.kill() at all — it only calls setViewerAttached(false), which stops the reconnect watchdog without touching the exec session. Verified this can never regress into a kill via planTeardown's pure matrix tests plus shell.kill('detach') integration tests.

• Given the detached-idle reap (30-min timer), should kill the server-side session.
  agent-terminal-handler.ts's disconnectConnection idle timer now calls session.command.kill('idle-reap'), which planTeardown maps to {killSession: true, closeSocket: true} — PtyShell.kill calls sprite.killSession(currentSessionId) (works even though the exec socket is almost certainly already dead by then, since detach stopped reconnecting) plus a best-effort local current.kill('SIGKILL').

• Given a kill against an already-dead session id, should succeed idempotently.
  sprites.ts's killSpriteSession treats HTTP 404/410 as success rather than throwing (the documented response for a missing session — verified against the "Attach to Exec Session"/"List Exec Sessions" endpoints, which share the resource family). Covered end-to-end: killAgentTerminal against a dangling streamSessionId still drops the row; MachineHandle.killSession/SpriteInstanceLike.killSession inherit the idempotency.

Design notes

• Pure core: planTeardown({trigger: 'forced-teardown' | 'detach' | 'idle-reap' | 'shell-exit'}) -> {killSession, closeSocket} in sprites-shell.ts, fully unit-tested (all 4 branches). Only forced-teardown and idle-reap have a real call site today (agent-terminal-handler.ts) — detach/shell-exit complete the tested decision matrix but aren't wired anywhere yet (a plain detach never calls kill() at all; a real shell exit is handled by fatal(), which also never calls kill() since the process already ended). Documented explicitly so this doesn't read as dead code.
• The @fly/sprites SDK (rc37) exposes no session-kill-by-id — only attachSession/createSession/kill() (a per-command WS signal, which no-ops once the socket is closed). killSpriteSession hits POST /v1/sprites/{name}/exec/{session_id}/kill directly using the real SpritesClient's public baseURL/token fields. withKillSession (shared in sprites.ts) bolts the method onto a raw SDK Sprite instance; both app boundaries (apps/web/src/lib/sandbox/sprites-client.ts, apps/realtime/src/terminal/realtime-sprites-client.ts) call it instead of each defining their own copy.
• The endpoint's 200 response is streaming NDJSON progress (signal → exited/killed → complete, or a mid-stream type: 'error' line if the signal couldn't actually be delivered) — the HTTP status alone doesn't confirm the kill succeeded. killSpriteSession drains the body and rejects on any error line (pure killSessionStreamErrorMessage helper).
• killSpriteSession retries up to MAX_EXEC_ATTEMPTS (3, linear backoff — the same schedule withWakeRetry uses) on any failure, not just a structurally-detected pre-open drop like the exec/spawn paths in this file require. A kill is idempotent by construction (see the 404/410 handling), so blanket retry is safe and closes a real reliability gap: both the fire-and-forget kill in PtyShell.kill() and the DB-row-keeping kill in killAtLocation previously gave up after one attempt, even against a transient failure like the cold-Sprite wake-on-request connection drop the exec paths already guard against. Each attempt is bounded by a 10s AbortController timeout, matching this driver's existing pattern for every other network call (filesystem ops via withTimeout, checkpoints via CHECKPOINT_TIMEOUT_MS).
• MachineHandle gained killSession(sessionId), backed by the same REST call via sdk.getSprite(...).killSession(...) in sprite-machine-host.ts. Its stream() method's wake-retry doc used to describe protecting killAgentTerminal — now stale (that caller no longer uses stream()), so the comment was corrected; stream()/listStreams() currently have no production caller at all (kept as the general primitive MachineHandle promises, per the file header).
• Every kill request explicitly asks for ?signal=SIGKILL — the endpoint defaults to SIGTERM when the query param is omitted, which would let a process trapping/ignoring TERM survive a call that still reports success. Both real call sites (killAtLocation's explicit kill and PtyShell.kill's idle-reap) go through attemptKillSpriteSession, so both inherit this for free.

Test evidence

packages/lib (sandbox+machines): 49 test files, 1234 tests passed
apps/realtime (terminal):        12 test files, 439 tests passed
bun run typecheck: 16/16 packages clean (includes web build)
bun run lint: clean (pre-existing unrelated warnings only)

New/changed tests:

• sprites.test.ts: killSpriteSession (correct URL shape + encoding + explicit signal=SIGKILL, NDJSON success/error-line draining, 404/410 idempotency, retry-then-succeed, retry-exhaustion, backoff schedule), killSessionStreamErrorMessage (pure NDJSON-line matrix), withKillSession (bolt-on delegation, other methods untouched)
• sprites-shell.test.ts: planTeardown matrix + shell.kill(trigger) integration (forced-teardown/idle-reap kill by id, detach/shell-exit never touch the session, idempotent double-kill, kill-failure doesn't throw)
• agent-terminal-handler.test.ts: trigger assertions on the two real call sites
• sprite-machine-host.test.ts: MachineHandle.killSession delegation + idempotency
• agent-terminals.test.ts: rewrote killAgentTerminal/killAgentTerminalById suites for the simplified REST-call path (dropped the now-obsolete transient-stream-failure/listing-corroboration tests, added a dangling-session idempotency test)

Out of scope (per leaf spec)

Grace-window length; sprite destroy semantics (6-2). Also considered and deliberately NOT done (see PR discussion for full reasoning): removing the now-callerless MachineHandle.stream()/listStreams() methods (kept as general infrastructure); reusing packages/lib/src/utils/fetch-with-timeout.ts for the kill call's timeout (would require adding fetch-injection to an otherwise-unrelated, currently-unused shared utility); eliminating the extra sdk.getSprite() round trip in killAtLocation's attach-then-kill sequence (architectural consistency with every other MachineHandle operation outweighs the marginal latency win on a non-hot path).

🤖 Generated with Claude Code

https://claude.ai/code/session_013mYrPXsCJFegYupazDEDGe

[coderabbitai]

Warning

Review limit reached

@2witstudios, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 87e934e2-112c-4278-a4c7-b8216d334572

📥 Commits

Reviewing files that changed from the base of the PR and between f031674 and 3ca3b3f.

📒 Files selected for processing (18)

• apps/realtime/src/terminal/__tests__/agent-terminal-handler.test.ts
• apps/realtime/src/terminal/__tests__/sprites-shell.test.ts
• apps/realtime/src/terminal/agent-terminal-handler.ts
• apps/realtime/src/terminal/realtime-sprites-client.ts
• apps/realtime/src/terminal/sprites-shell.ts
• apps/web/src/lib/sandbox/sprites-client.ts
• packages/lib/src/services/machines/__tests__/agent-terminals.test.ts
• packages/lib/src/services/machines/__tests__/machine-branches.test.ts
• packages/lib/src/services/machines/agent-terminals.ts
• packages/lib/src/services/sandbox/__tests__/machine-diff.test.ts
• packages/lib/src/services/sandbox/__tests__/machine-fs.test.ts
• packages/lib/src/services/sandbox/machine-host.ts
• packages/lib/src/services/sandbox/sandbox-client/__tests__/machine-host-adapter.test.ts
• packages/lib/src/services/sandbox/sandbox-client/__tests__/sprite-machine-host.test.ts
• packages/lib/src/services/sandbox/sandbox-client/__tests__/sprites.test.ts
• packages/lib/src/services/sandbox/sandbox-client/__tests__/wake-retry.test.ts
• packages/lib/src/services/sandbox/sandbox-client/sprite-machine-host.ts
• packages/lib/src/services/sandbox/sandbox-client/sprites.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/sprites-2-3-kill-endpoint

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6397823fb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Use the documented session kill URL

The Sprites exec API documents the kill endpoint as POST /sprites/{name}/exec/{session_id}/kill (and the SDK example builds ${client.baseURL}/v1/sprites/${spriteName}/exec/${targetSession.id}/kill), but this helper posts to /exec/sessions/{id}/kill. For every explicit terminal kill or idle reap this wrong route can return 404, which this same helper treats as idempotent success, so callers will remove the terminal row/close the pane while the server-side session keeps running and billing.

Useful? React with 👍 / 👎.

[2witstudios]

Confirmed and fixed in f5fcd19 — thank you, this was a real bug.

I verified against the live docs (https://sprites.dev/api/sprites/exec#kill-exec-session): the documented endpoint is POST /sprites/{name}/exec/{session_id}/kill, not .../exec/sessions/{id}/kill. The extra sessions/ segment was wrong.

Went further than just the URL, since the same root issue (trusting a response too loosely) extends to the success path too: the docs show 200 returns streaming NDJSON progress (signal → exited/killed → complete, or a mid-stream {"type":"error",...} line if the signal couldn't actually be delivered). The old code only checked response.ok, so it would have silently treated a 200 with a mid-stream error as success too. killSpriteSession now:

1. Uses the correct URL (/exec/{session_id}/kill).
2. Drains the response body on 200 and rejects if any NDJSON line reports type: 'error' (new pure helper killSessionStreamErrorMessage, unit-tested).
3. Still treats 404/410 as idempotent success — confirmed via the docs' own "Attach to Exec Session" / "List Exec Sessions" endpoints, which document 404 as the response for a session that doesn't exist (the same resource family, so I'm treating that as representative for kill too, since the kill endpoint's own docs list 200/404/500 as its only documented codes).
4. Added a 10s wall-clock timeout via AbortController around the whole request+drain, matching this driver's existing pattern for every other network call (filesystem ops, checkpoints) — an unbounded raw fetch would've been the one exception.

Full verification re-run after the fix: packages/lib 8025 tests, apps/realtime 825 tests, apps/web machine routes 167 tests, full monorepo typecheck (16/16 packages) — all green.

Leaving this thread open for your verification rather than auto-resolving it, since it represents real work done during this convergence pass, not something that was already fixed before I started.

[2witstudios]

Follow-up: ran a broader multi-angle self-review after this fix and found a few more things worth closing in the same pass (189954a):

1. No retry on transient failure. killSpriteSession now retries up to 3 times with linear backoff on ANY failure (not just a structurally-detected pre-open drop like the exec/spawn paths in this file require) — a kill is idempotent by construction, so blanket retry is safe, and it closes a real gap: both the fire-and-forget kill from PtyShell.kill() and the DB-row-keeping kill from killAtLocation previously gave up after one attempt, even against the exact cold-Sprite wake-on-request drop this codebase already has dedicated retry machinery for elsewhere.
2. Duplicated SDK wrapper. The withKillSession bolt-on was copy-pasted near-verbatim across apps/web and apps/realtime's SDK factories. Hoisted into one shared, exported withKillSession in sprites.ts.
3. Misleading trigger name. teardownAgentTerminalSession's only real trigger was 'user-kill', but that path is the platform force-ending a session (revoked access / insolvent payer) — never an actual user click. Renamed to 'forced-teardown' so an incident review doesn't misattribute cause.
4. Fixed a now-stale comment in sprite-machine-host.ts's stream() doc that still claimed to protect killAgentTerminal's kill path — that caller no longer uses stream().

Re-verified: packages/lib 8029 tests, apps/realtime 825 tests, full monorepo typecheck (16/16) — all green.

[2witstudios]

@coderabbitai review
@codex review

[coderabbitai]

@2witstudios: Sounds good — I'll review the changes now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 189954a326

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve SIGKILL semantics for session termination

In detached/idle-reap and killAgentTerminal paths this REST call is the only signal that can reach the server-side PTY, but the Sprites kill endpoint defaults to SIGTERM when no signal query is provided, whereas the old stream-based path used SIGKILL. For agents or child processes that trap/ignore TERM, the row/session bookkeeping can be removed after this call even though the billable exec session is still alive; please request signal=SIGKILL (or otherwise escalate/fail on non-termination) to keep explicit kills and reaps authoritative.

Useful? React with 👍 / 👎.

[2witstudios]

Confirmed and fixed in 979cc4c — thank you, this was a real gap.

I verified the endpoint contract directly (sprites.dev/api/sprites/exec#kill-exec-session): signal is an optional query param, default: SIGTERM. The old WS-based kill("SIGKILL") path this REST call replaces always sent SIGKILL, so a process that traps/ignores TERM would have survived a default-signal call while killSpriteSession still reported success — exactly the bookkeeping-vs-reality gap you flagged.

Fix: attemptKillSpriteSession now requests ?signal=SIGKILL explicitly on every kill (both the killAgentTerminal REST path and the idle-reap path go through this same function, so both are covered). Updated the doc comment to spell out why the explicit signal matters, and added a dedicated test asserting the signal=SIGKILL query param is present on every kill request, plus updated the two existing URL-shape assertions to include it.

Leaving this thread open per our convergence process (only auto-resolving threads that were already fixed before this loop started) — happy for you to verify on the new commit.