[sprites 1-3] onConnect: in-memory reattach before sprite resolution

[2witstudios]

Why

Tab-back to an open terminal was slow for a reason that had nothing to do with the platform. onConnect ran the entire fused checkAuth chain — resolve the agent-terminal row → getSprite → write a code-execution audit row → reserve a concurrency slot — and only then looked in sessionMap to discover that a live in-memory session was sitting right there, ready to reattach.

docs.sprites.dev/concepts/lifecycle puts a warm wake at 100–500ms and a cold one at 1–2s. That is the platform floor; everything we stacked above it on a tab-back was our own orchestration overhead.

What changed

checkAuth no longer resolves the Sprite (or reserves a slot) eagerly. It returns the cheap, DB-only access verdict (sessionKey, payerId) plus an uncalled resolveSandbox() thunk. onConnect runs the access check, looks up the live session (the key derives without a Sprite, per 1-1), and a pure planConnect({ accessResult, existingSession }) returns a discriminated deny | reattach | create plan carrying the narrowed payload each branch needs. Only the cold path invokes the thunk.

The audit row moved into resolveSandbox with it — it records a PTY actually being launched, and a reattach launches nothing.

• apps/realtime/src/terminal/agent-terminal-handler.ts — planConnect (pure), split AgentTerminalCheckAuthResult / AgentTerminalSandboxResult, reordered onConnect.
• apps/realtime/src/terminal/agent-terminal-access.ts — buildAgentTerminalCheckAuth returns the lazy thunk, which owns slot reservation + release.

AgentTerminalCheckAuthDeps is unchanged, so the index.ts wiring needed no edits.

🐛 A severe pre-existing bug this surfaced (please read)

Reviewing the reorder turned up a bug that the reorder would otherwise have shipped on top of. acquireCodeExecutionSlot (packages/lib/src/services/sandbox/quota.ts) is a per-user counter — free: 1, pro: 2, founder: 3, business: 5 — and checkAuth reserved a slot on every call, including calls that start no PTY:

1. Reattach was impossible at the limit. A free-tier user with one live session already holds the only slot. The tab-back's access check tried to acquire a second, failed, and was denied concurrency_limit. The fast path this PR exists to build was unreachable for the most common tier.
2. Live sessions were killed after 60s. The re-auth tick also calls checkAuth. At the limit it too failed to acquire a slot — and the tick reads any denial as a revoked authorization, so it called teardownAgentTerminalSession. A free-tier PTY died ~60 seconds after opening.

Both are pre-existing on master; they are masked in production only because CODE_EXECUTION_ENABLED is off. My handler tests missed them because they mock checkAuth — the bug lives in the real composition.

The fix falls out of the same lazy split: a slot bounds how many PTYs a user has running, so it is reserved inside resolveSandbox (the only path that starts one), and releaseSlot is surfaced on the sandbox success result — there is no slot to release unless one was actually reserved. Reattach takes none; re-auth reserves nothing and hands nothing back. Regression tests are at the access layer where the real slot logic lives.

🐛 Second bug: concurrent cold connects (double-mount)

The leaf requires that a double-mount not double-create. The sequential remount was already safe (connect #2 finds the session via getByKey). The genuinely concurrent race was not — and this PR widens its window, because the cold path is exactly where a connect now spends seconds inside resolveSandbox.

My first fix was to let both open a PTY and then discard the loser. An adversarial review pass found that this was destructive, and I replaced it. Recording it here because the reason is subtle and worth a reviewer's attention:

openPtyShell with a non-null sessionId calls sprite.attachSession(id) — it connects to a server-side exec session — and PtyShell.kill() is SIGKILL on the process, not a local socket close. So whenever a streamSessionId is persisted (any warm reattach after a realtime restart), both racers attach to the same remote session, and killing the duplicate SIGKILLs the very process the survivor is attached to. It turned a recoverable orphan into a guaranteed terminal kill, on exactly the path it was meant to protect.

The fix in place: serialize at the key, so a second PTY is never opened at all. A cold connect claims the key before its first await; a concurrent connect for that key awaits the in-flight create and joins its session.

• TerminalSessionMap gains trackCreate / pendingCreate.
• The claim is released on every exit from the cold path — success, denial, or throw — so a failed create cannot wedge the key. The joiner loops, in case another create queued behind a failure.
• No await sits between the join-loop's exit and the claim, so it is atomic w.r.t. the event loop and needs no lock.

This dissolves the class rather than patching it: no duplicate PTY → no kill; no double slot; no double hold; no ambiguous session-id discovery.

🐛 Fourth/fifth/sixth: found by the same adversarial pass

• Concurrency slot leaked when billing.gate throws. The slot was reserved but no session owned it yet, so a rejection escaped onConnect with the slot still held. activeByUser is a process-lifetime counter — one transient billing/DB blip permanently locked a free-tier user (limit 1) out of terminals on that replica until restart. Now released before rethrowing.
• discoverNewSessionId took the FIRST new tty session. One Sprite hosts every agent terminal on its machine, so a sibling terminal launching in the same window is indistinguishable from ours — persisting the sibling's id points this terminal's next cold connect at another terminal's PTY. Now abstains unless exactly one appeared (the rule sprites-shell.ts's newTtySessionId already applies).
• Re-auth checked the CREATOR's userId forever. A session outlives its creator's connection, and any authorized user may reattach. A viewer whose access was revoked mid-session kept receiving output and could keep typing — because the long-gone creator still passed the check. Sessions now carry viewerUserId, set on create and on every reattach, and the tick re-checks whoever is actually driving the PTY.

🐛 Third bug: re-auth stopped noticing a deleted scope (review catch — codex P2)

Making the sandbox resolution lazy also made the (scope, name) existence check lazy — because the fused checkAuth only ever learned "this terminal still exists" as a side effect of resolving its sandbox. The 60s re-auth tick calls only the access half, so after the split it stopped noticing that a terminal's project, branch, or own row had been deleted: the orphaned PTY kept running against a scope that no longer existed.

Restoring the check naively would have re-broken the epic. resolveAgentTerminal fuses two very differently-priced questions:

• cheap — does the row exist? (resolveScopeKey + store.findByName: a couple of indexed reads, no Sprite)
• expensive — where does its Sprite live? (machineSandbox.acquire, which for machine/project scope can reconnect or resume a hibernated Sprite)

Calling it from the access half would wake the Sprite on every re-auth tick and every tab-back. So I split the question, not just the call site:

• New resolveAgentTerminalRow (packages/lib/src/services/machines/agent-terminals.ts) — DB-only. Its tests pass machineSandbox: undefined throughout, which is the proof it cannot wake a Sprite: it is never handed one.
• The access half runs it eagerly, after the read-only access gates — so a user without edit rights still learns nothing about which terminals exist.
• The Sprite wake/read stays lazy, so reattach still performs zero Sprite SDK calls.

Requirements

Requirement	How it is satisfied
Live detached session + allowed user → agent-terminal:ready (with scrollback) after the access check, ZERO sprite SDK calls	Reattach returns before resolveSandbox() is ever called. Test spies on the thunk and every sprite method (listSessions/spawn/createSession/attachSession/updateNetworkPolicy) and asserts none fired — plus that no slot was taken.
No live session → cold path unchanged (resolve sandbox → billing gate → openShell)	Create path calls the thunk, then gates, then opens. All pre-existing handler tests (launch command, cwd, session-id discovery/persistence, billing holds, heartbeat settle, re-auth teardown) pass untouched.
Denied user + live session → refuse, not reattach	planConnect returns { kind: 'deny' } for a denied verdict regardless of existingSession (pure test). In the shell a denied verdict never reaches the lookup. Handler test: an unauthorized socket gets agent-terminal:error, never :ready, and the victim's session stays bound to its original socket.
Two connects for the same key (double-mount) → no double-create	Regression test: the second connect finds the session via getByKey and reattaches — openShell called exactly once. setNew/getByKey semantics preserved, no new locking.

Tests

TDD: planConnect is pure and tested with no mocks (riteway assert({given, should, actual, expected})); the handler is tested with an injected spy sprite client and a real createTerminalSessionMap.

apps/realtime  (all)   19 files / 641 tests pass · branch coverage 98.02% (gate 98%)
packages/lib  (machines) 8 files / 165 tests pass

tsc --noEmit clean, eslint clean. No any.

Notes for reviewers

• A reattach no longer writes a code-execution audit row. Intended ("no policy writes" in the leaf spec) — the row asserts a PTY was launched, and a reattach launches none. The cold path still audits (test-covered). Flag it if compliance expects a row per connect rather than per launch.
• Side benefit of the split: the 60s re-auth tick no longer resolves or wakes the Sprite. It calls checkAuth and never touches the thunk.
• Untouched, as scoped out: the cold-path wake hack (1-4) and frontend keep-alive (4-1).

🤖 Generated with Claude Code

https://claude.ai/code/session_01HnFmynYiN6bmrtibxdzLDJ

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 57 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: 063f0c83-3f72-498d-a03f-645dce7ff1dd

📥 Commits

Reviewing files that changed from the base of the PR and between 9138066 and c50c36f.

📒 Files selected for processing (9)

• apps/realtime/src/index.ts
• apps/realtime/src/terminal/__tests__/agent-terminal-access.test.ts
• apps/realtime/src/terminal/__tests__/agent-terminal-handler.test.ts
• apps/realtime/src/terminal/__tests__/terminal-session-map.test.ts
• apps/realtime/src/terminal/agent-terminal-access.ts
• apps/realtime/src/terminal/agent-terminal-handler.ts
• apps/realtime/src/terminal/terminal-session-map.ts
• packages/lib/src/services/machines/__tests__/agent-terminals.test.ts
• packages/lib/src/services/machines/agent-terminals.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/sprites-1-3-onconnect-reorder

---

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: 4f2163fc8e

ℹ️ 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".

[2witstudios]

@coderabbitai review

[coderabbitai]

✅ 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.