Repository navigation
fix(auth): CLI session revocation closes open sockets in the running server - #490
Conversation
|
Grok roast at e08bfc4: BLOCKING Reviewed 1. Blocking. A lock error stalls the writer and closes healthy socketsTrigger: any open socket, and the shared database returns
Failure: a desktop and a browser are connected on still-valid sessions. A CLI command holds the database across the next recheck. The first socket's SELECT blocks the writer for 5s and then the handler dies, so a session nobody revoked disconnects mid-turn. The other sockets are queued on the same semaphore, so the stall repeats once per socket until the lock clears. A revoked socket also dies here, but because the read threw, not because A retry inside the per-socket loop is not enough. N of those waits still take the writer one after another. 2. Blocking. One shared check keeps the same close guarantee and drops the N-way costThe PR deletes "a shared cross-socket poller" and keeps one primary-key SELECT per open socket per 5s. That is the larger design.
A committed revoke still closes that socket on the next successful read. A locked read closes nothing. Idle cost is one pragma, not N table reads, and the writer is not busy-waited once per socket. A CLI-to-server notify endpoint is still more machinery than this. Watching the 3. Non-blocking. 5s is a poll period, not a security cut-off#325 asks for a prompt close. The #323 design said Cleared: after a committed revoke, the next successful |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
|
Luna review round 1 at e08bfc4: CHANGES REQUESTED Blocking: a transient database read failure can drop a healthy socket ( Non-blocking: upstream-first record needs correction. Upstream PR #14355 explicitly implemented a 30-second cross-process database re-read, with a WebSocket test and CLI live check; it closed unmerged. Current upstream main still has the single-read watcher, so there is no merged fix to port, but the PR statement that #14355 covered only same-process revocation is inaccurate. The new test fails on baseline per the recorded run and asserts the |
e08bfc4 to
d4f2e87
Compare
d3190dd to
44d6d6d
Compare
|
Grok roast round 2 at 44d6d6d: PASS Reviewed Round-1 blocker is fixedThe per-socket writer recheck is gone. AttacksReader. Confirmed for the disk server. If the worker is missing (in-memory, or Natural expiry. Zero sockets. The fiber is scoped to the store and keeps the 2s sleep. An empty open set returns before any SQL and does not advance data_version. On SQLite 3.53.4, WAL: another connection's commit changes the reader's value immediately, before any checkpoint. The same connection's own writes do not. A passive checkpoint does not move a reader that already saw those commits. Latency. The loop sleeps, then reads. An already-open socket is closed on the next successful tick: at most one 2s interval plus the read. A failed read does not consume the version, so the bound stretches to the next success, as the PR says. Upstream. Non-blockingThe version cursor can skip a socket that connects during the tick. The tick snapshots open ids, then reads Checked, not findings
|
44d6d6d to
8f98125
Compare
|
Luna review round 2 at 8f98125: CHANGES REQUESTED
No other concrete findings. The shared WAL-reader watcher avoids writer-side reads; failed reads are retried without invalidating sockets; the real WebSocket test covers cross-process revoke and a healthy socket; the module is wired into |
8f98125 to
7c45bb2
Compare
|
Luna review round 3 at 7c45bb2: CHANGES REQUESTED Blocking: the post-snapshot registration race still loses a revocation ( Confirmed: on |
…server One shared watcher reads PRAGMA data_version on the WAL reader every 2 seconds while sockets are open. When another process, such as `t3 auth session revoke`, commits, it runs one SELECT over the open session ids and publishes clientRemoved for revoked or deleted ones. A failed read closes nothing and is retried on the next tick. Closes #325 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A socket that registered between the open-id snapshot and the data_version read was left out of that tick's SELECT, and the advanced cursor then skipped it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The cross-process check snapshotted connection bookkeeping, which the caller updates. A watcher whose row read preceded a revocation could miss the tick that advanced the cursor past it. awaitInvalidation now registers its session before reading, so the snapshot always covers it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…atched The check slept in a forever loop even with no watchers, so a TestClock jump of years replayed millions of 2 s ticks and timed out the peer revocation tests. It now waits on the watched set until a session is registered. Adds a regression for a socket that starts watching after a tick already passed its revocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7c45bb2 to
6d1d1c4
Compare
|
Luna review round 4 at 6d1d1c4: CHANGES REQUESTED Blocking: the watcher stops polling after its first tick while sessions remain watched ( Cleared in this round: the late registration during |
|
Luna review round 5 at 6d1d1c4: APPROVE The round-4 polling finding is a false positive. At this exact source, The tests do not explicitly perform two separate revocations while a real socket remains open. The watcher test does verify repeated ticks with the watched set unchanged and observes a later version change; the real-socket test covers one revoke. A two-revocation same-socket test would be useful as regression coverage, but is not needed to establish this behavior given the replay contract and loop shown above. |
|
Grok roast round 3 at 6d1d1c4: PASS Reviewed The race is closed by order
Take a watcher S and a revocation commit R.
A read that misses R and a snapshot that also misses S cannot both be true for the version that contains R. The old hole needed both. The cursor can no longer step over S. Idling does not open a windowWith an empty set the fiber is blocked on Then a socket registers. Its A revoke that commits after a registered session's valid read is the steady-state case. The session is already in the set, so the next successful tick snapshots it. The bound stays one 2s interval plus the read. The first wake from idle uses that same sleep-then-tick, so a revoke in that first interval has the same bound. Same-process and permanent peer sessions
DesignStill one fiber, one version cursor, and one select after a commit the cursor has not seen. The idle wait is the piece that had to be added: the tick already returned on an empty set, and the sleep in front of it still ran, which is what Checked, not findingsOn effect 4.0.1, |
…server (#490) * fix(auth): CLI session revocation closes open sockets in the running server One shared watcher reads PRAGMA data_version on the WAL reader every 2 seconds while sockets are open. When another process, such as `t3 auth session revoke`, commits, it runs one SELECT over the open session ids and publishes clientRemoved for revoked or deleted ones. A failed read closes nothing and is retried on the next tick. Closes #325 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(auth): CLI revocation closes a real open socket Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(auth): read the commit version before snapshotting open sockets A socket that registered between the open-id snapshot and the data_version read was left out of that tick's SELECT, and the advanced cursor then skipped it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(auth): cover each invalidation watcher from before its first read The cross-process check snapshotted connection bookkeeping, which the caller updates. A watcher whose row read preceded a revocation could miss the tick that advanced the cursor past it. awaitInvalidation now registers its session before reading, so the snapshot always covers it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(auth): idle the cross-process revocation check while nothing is watched The check slept in a forever loop even with no watchers, so a TestClock jump of years replayed millions of 2 s ticks and timed out the peer revocation tests. It now waits on the watched set until a session is registered. Adds a regression for a socket that starts watching after a tick already passed its revocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
What:
t3 auth session revokenow closes the revoked session's open sockets in the running server within about 2 seconds, and a database lock never closes a healthy socket.Why: The CLI runs its own SessionStore with its own in-memory PubSub, so the server's socket watcher never heard about revocations written to the shared SQLite and slept until credential expiry.
So what: Luna review, Grok roast and CI, then the backlog coordinator merges.
Problem
apps/server/src/cli/auth.tsbuilds a separateEnvironmentAuthruntime, soSessionStore.revokepublishesclientRemovedon a PubSub only the CLI process sees. In the server,ws.tsraces each live socket againstSessionStore.awaitInvalidation, which waits for that PubSub or the credential deadline (up to 30 days). New requests were already rejected becauseverifyrereadsrevoked_at; existing sockets stayed open.Fix (redesigned after the Grok roast)
The first version re-read each socket's row on the writer every 5 s. The roast showed a
SQLITE_BUSYthere blocks the single writer forbusy_timeoutper socket and then closes healthy sockets. Replaced with one shared watcher in a fork-owned module,apps/server/src/auth/forkCrossProcessRevocation.ts, hooked intoSessionStore.make:SubscriptionRef) until at least one session is watched, then every 2 s readsPRAGMA data_versionthroughNodeSqliteClient.readOnly, which is the WAL reader worker that fix(contextmd): keep serving reads off the writer connection #340 uses for serving. With nothing watched no timer runs; an earlier head polled regardless, which madeTestClockjumps of years replay millions of ticks and time out three peer tests in CI.SELECTover them, also on the reader. The watched set belongs toSessionStore: eachawaitInvalidationregisters its session before its first row read and unregisters when it ends, so connection bookkeeping in callers plays no part. With the snapshot taken after the version read, any watcher missing from it registered later, and so read its row after every commit that version covers. Advancing the cursor cannot skip it (round-2 roast and Luna round-2 races).Why no revocation is skipped: take any watcher S and a revocation commit R. S registers in the watched set before its own first row read.
revoked_atand closes itself.Each case has a test below.
Latency bound: at most one 2 s tick (
CROSS_PROCESS_REVOCATION_POLL_INTERVAL) after the CLI commits, plus the reader query. If a read fails, the bound extends to the first tick whose read succeeds.Cost: while sockets are open, one pragma on the reader worker every 2 s. Server commits also bump
data_versionon the reader, so a busy server runs at most one indexedSELECTper tick. The writer is not touched and ordinary requests do not read SQLite. With no reader worker (in-memory databases),readOnlyfalls back to the writer as everywhere else.A CLI-to-server notify endpoint was rejected because it adds an endpoint, its auth and a no-server fallback to get the same result.
Upstream
Searched
pingdotgg/t3codemain, Issues and PRs. Correcting the earlier record, which said related PRs covered only same-process revocation:getByIdre-read every 30 s on the writer and failed closed: a read error ended the connection. Macroscope did not approve it, and its author closed it unmerged on 2026-10-03./wsrace againstawaitInvalidationthat main has today came from upstream #17370, which covers same-process revocation and expiry.Reused from #14355: its proof shape, a real open socket that closes after a revocation the server was never told about. Not reused: its per-socket writer re-read and fail-closed error handling, which are the two defects both reviews of this PR blocked.
Proof
Rebased on
origin/main69a5b9e5f7. Head is6d1d1c4e92f7168a8897b4b46727ce9a4970e819. Tests and typecheck ran on the same tree one rebase earlier; the three new main commits touch neitherapps/server/src/authnor the three peer tests. One test process at a time.vp test run src/auth/forkCrossProcessRevocation.test.ts src/auth/SessionStore.test.ts: 2 files, 35/35 passed.SELECT; an unrelated commit runs one and closes nothing; a revocation commit closes only the revoked id.verifyat t0, the CLI store revokes it at t1, a tick reads the new version and snapshots without it at t2/t3, and it starts watching at t4 with no later commit or tick. It completes. It also passes on7c45bb2d93, so that head did not have this race: the watcher's own first read comes after registration and sees the revocation. No failing-before run exists for it.TestClock.adjust("3653 days")with nothing watched makes no reads, and the first registration starts checks.Test timed out in 20000ms; it passes now.["late-session"], received[].SELECTfails withdatabase is lockedfor two ticks and nothing closes. The next tick succeeds with no newer commit and closes the revoked id./wsroute (verify at upgrade,markConnected, race the socket againstawaitInvalidation). Two real WebSockets connect. The CLI store revokes one; after one tick that socket closes. The other socket stays open and still echoes, and the revoked token failsverifywithSessionTokenRevokedError. With the hook disabled, the test fails withTest timed out in 20000ms.src/peer/PeerLinks.test.ts -t "tells the agent to link again": timed out after 120 s on7c45bb2d93locally, now 1 passed.src/peer/permanentSession.test.ts -t "pairing-code lifetime and immediate revoke": now 2 passed.src/peer/environmentCli.test.ts: now 1 passed. Locally it needsenv -u T3_SERVICE_LAUNCHER_CONTEXT, because the agent shell runs inside a launched T3 server and otherwise fails withServiceLauncherClientError: ... different t3 version; CI has no such variable.7c45bb2d93.tsc-rs --noEmitinapps/server: no errors.vp linton the four auth files: exit 0.scripts/fork-check.sh: OK. The new files are registered underpaired-session-renewalindocs/fork-features.md./wsrace rather than the fullws.tsRPC route, and the "CLI" is a second runtime in the same test process, not a spawnedt3binary. Both runtimes use separate SQLite connections, which is whatdata_versiondepends on.Requirements and who asked: #325 (from #323's review) and the backlog coordinator: CLI revocation must close live sockets in the running server. The Grok roast and the coordinator added that lock errors must never close a socket and that there must be no per-socket writer reads.
Deleted: The per-socket 5 s writer recheck from the first version, a CLI-to-server notify endpoint with its auth and fallback, and a shared expiry timer (per-socket deadline sleeps already cost nothing).
Bottleneck: The server learned of revocation only from an in-process PubSub that the CLI cannot reach, and the server has a single writer connection that any extra read would contend for.
Checked myself: upstream #14355's full diff and close history; CLI runtime,
SessionStore.revokeandawaitInvalidation, thews.tsrace,nodeSqliteClientreader routing andbusy_timeout, and upstream main'sSessionStore.ts. I ran each new test against a broken variant (hook disabled, failure path) and saw it fail.Closes #325
Model and harness: Claude Opus 5.5 (
claude-opus-5-5) in Claude Code via T3 Code.🤖 Generated with Claude Code