Skip to content

fix(auth): CLI session revocation closes open sockets in the running server - #490

Merged
lukemaj merged 5 commits into
mainfrom
fix/325-cli-revocation-sockets
Oct 19, 2026
Merged

lukemaj merged 5 commits into
mainfrom
fix/325-cli-revocation-sockets

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

What: t3 auth session revoke now 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.ts builds a separate EnvironmentAuth runtime, so SessionStore.revoke publishes clientRemoved on a PubSub only the CLI process sees. In the server, ws.ts races each live socket against SessionStore.awaitInvalidation, which waits for that PubSub or the credential deadline (up to 30 days). New requests were already rejected because verify rereads revoked_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_BUSY there blocks the single writer for busy_timeout per socket and then closes healthy sockets. Replaced with one shared watcher in a fork-owned module, apps/server/src/auth/forkCrossProcessRevocation.ts, hooked into SessionStore.make:

  • One fiber per server. It idles on the store's watched set (a SubscriptionRef) until at least one session is watched, then every 2 s reads PRAGMA data_version through NodeSqliteClient.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 made TestClock jumps of years replay millions of ticks and time out three peer tests in CI.
  • Only when the version changed does it snapshot the watched session ids and run one SELECT over them, also on the reader. The watched set belongs to SessionStore: each awaitInvalidation registers 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).
  • Any read error, including lock errors, is logged and retried on the next tick. The version is only marked checked after a successful read, so nothing is missed and nothing is closed because of an error.
  • Same-process revocation and expiry stay on the existing PubSub and deadline paths, unchanged.

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.

  • If that read comes after R, S sees revoked_at and closes itself.
  • If it comes before R, S registered before R. R comes before the version read that notices R, and the snapshot is taken after that read, so S is in the snapshot.

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_version on the reader, so a busy server runs at most one indexed SELECT per tick. The writer is not touched and ordinary requests do not read SQLite. With no reader worker (in-memory databases), readOnly falls 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/t3code main, Issues and PRs. Correcting the earlier record, which said related PRs covered only same-process revocation:

  • #14355 did implement cross-process revocation. It used a per-socket getById re-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.
  • #8418 and #13844 closed unmerged. The /ws race against awaitInvalidation that main has today came from upstream #17370, which covers same-process revocation and expiry.
  • Upstream main still has the single-read watcher. There is no merged fix to port.

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/main 69a5b9e5f7. Head is 6d1d1c4e92f7168a8897b4b46727ce9a4970e819. Tests and typecheck ran on the same tree one rebase earlier; the three new main commits touch neither apps/server/src/auth nor 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.
    • "invalidates only sessions another process revoked, once per commit": an unchanged version runs no SELECT; an unrelated commit runs one and closes nothing; a revocation commit closes only the revoked id.
    • SessionStore "closes a socket that starts watching after a tick already passed its revocation", Luna round 3's exact order: the late socket passes verify at 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 on 7c45bb2d93, 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.
    • "stays idle with nothing watched and starts checking once a session is": TestClock.adjust("3653 days") with nothing watched makes no reads, and the first registration starts checks.
    • SessionStore "covers a watcher whose session read precedes a cross-process revocation": exactly Luna's interleaving. A watcher reads a valid row, then the CLI store commits the revocation, then a tick runs while that session has no connection bookkeeping. On the previous head it fails with Test timed out in 20000ms; it passes now.
    • "checks a socket that registers while the commit version is being read": the fake version read registers a revoked late socket mid-tick. It is invalidated on that tick. Against the previous ordering (snapshot, then version) the same test fails: expected ["late-session"], received [].
    • "keeps sessions open through a locked read and retries on the next tick": the SELECT fails with database is locked for two ticks and nothing closes. The next tick succeeds with no newer commit and closes the revoked id.
    • SessionStore "closes a real socket when another process revokes its session": two store runtimes on one disk SQLite database with the reader worker enabled, standing in for the server and the CLI. A real HTTP server mirrors the /ws route (verify at upgrade, markConnected, race the socket against awaitInvalidation). 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 fails verify with SessionTokenRevokedError. With the hook disabled, the test fails with Test timed out in 20000ms.
    • Existing same-process revoke, revokeAllExcept, replace and expiry tests still pass.
  • Peer regressions from CI run 39190215009, each run alone:
    • src/peer/PeerLinks.test.ts -t "tells the agent to link again": timed out after 120 s on 7c45bb2d93 locally, 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 needs env -u T3_SERVICE_LAUNCHER_CONTEXT, because the agent shell runs inside a launched T3 server and otherwise fails with ServiceLauncherClientError: ... different t3 version; CI has no such variable.
    • I reproduced only the PeerLinks failure on the old head. The other two match the CI timeouts by mechanism but were not rerun on 7c45bb2d93.
  • tsc-rs --noEmit in apps/server: no errors. vp lint on the four auth files: exit 0. scripts/fork-check.sh: OK. The new files are registered under paired-session-renewal in docs/fork-features.md.
  • Limit: the socket test uses a route that copies the /ws race rather than the full ws.ts RPC route, and the "CLI" is a second runtime in the same test process, not a spawned t3 binary. Both runtimes use separate SQLite connections, which is what data_version depends 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.revoke and awaitInvalidation, the ws.ts race, nodeSqliteClient reader routing and busy_timeout, and upstream main's SessionStore.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

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS labels Oct 18, 2026
@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Grok roast at e08bfc4: BLOCKING

Reviewed e08bfc4f99fd5994144032b054836bfdb1ce4aec read-only. A clean cross-runtime revoke does complete awaitInvalidation within 5s at the SessionStore seam. The recheck that gets there is the expensive one, and a lock error on it drops healthy sockets.

1. Blocking. A lock error stalls the writer and closes healthy sockets

Trigger: any open socket, and the shared database returns SQLITE_BUSY. That already happens on the live server. #323 comment 6180641383 recorded LockTimeoutError: database is locked on a CLI read while the server was running a command. #323 comment 6191637927 recorded the same error on t3 auth session issue during migrations while the server was working.

REVOCATION_RECHECK_INTERVAL (apps/server/src/auth/SessionStore.ts:463) makes every awaitInvalidation call getById again at least every 5s (:1112-1133). getById is not readOnly, so the SELECT runs on the server's only writer connection (packages/shared/src/nodeSqliteClient.ts:750, semaphore of 1). apps/server/src/persistence/Sqlite.ts:20-21 sets PRAGMA busy_timeout = 5000 because the CLI and the server write from separate processes and the code would rather wait than fail. node:sqlite's DatabaseSync is synchronous, so that wait blocks the event loop, permit held. classifySqliteError maps SQLITE_BUSY / SQLITE_LOCKED to LockTimeoutError and this loop does not retry it. Every getById failure becomes SessionCredentialVerificationError (SessionStore.ts:1116). apps/server/src/ws.ts:3291-3299 turns that into EnvironmentInternalError, the race ends, and markDisconnected runs.

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 revoked_at was seen. The new test (SessionStore.test.ts:193) adjusts TestClock on two in-process stores and never fails a read, so this stays green.

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 cost

The PR deletes "a shared cross-socket poller" and keeps one primary-key SELECT per open socket per 5s. That is the larger design. markConnected already knows which sessions have a socket. One fiber should own the wait.

PRAGMA data_version, read outside a transaction, changes when another connection commits. That is the CLI. It does not change for a commit on the connection that reads it, which is fine: same-process revoke already publishes on this store's PubSub and wins the existing race. Any other write also bumps it; the follow-up is one SELECT of the open ids, not one per socket. On LockTimeoutError, back off and try again. Do not complete the waiters. Expiry is not a write, so keep a single sleep until the soonest expires_at.

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 -wal mtime is the same idea if a pragma poll is unwanted.

3. Non-blocking. 5s is a poll period, not a security cut-off

#325 asks for a prompt close. The #323 design said t3 auth session revoke cuts the live socket off at once. Same-process revoke still does. Cross-process revoke keeps the socket's upgrade scopes for up to 5s, plus another 5s when the read that would see revoked_at is inside busy_timeout. A stolen session can keep calling tools through that window. The issue states no numeric SLO, so the number alone is not a merge block. With the data_version wake there is nothing to tune: the bound is the short poll, and idle cost stays one pragma.

Cleared: after a committed revoke, the next successful getById returns, and the other session in the new test stays open. I did not find a path where a committed revoke leaves the socket up until TTL. The hole is the error path and the per-socket poll, not a missed row on the happy path.

@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ The exact PR base did not have a successful artifact. Baseline uses the latest successful main measurement shown below.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB −24 B (−0.5%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB −24 B (−2.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB −41 B (−0.2%) 29.3 KiB ✅
Claude Live turn messages 2 1 −1 (−50.0%) 8 ✅

Baseline: 8f6fef8 · PR result: 6d1d1c4 · Source CI: success

Scenario and decoded snapshot size

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

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj

lukemaj commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Luna review round 1 at e08bfc4: CHANGES REQUESTED

Blocking: a transient database read failure can drop a healthy socket (apps/server/src/auth/SessionStore.ts:1112-1117; apps/server/src/ws.ts:3289-3299). Each 5-second recheck maps any getById failure to SessionCredentialVerificationError; the WebSocket race converts that to EnvironmentInternalError and releases the socket. getById is not marked read-only, so it runs through the server writer connection, whose DatabaseSync call is synchronous. The repository has recorded live LockTimeoutError: database is locked failures during CLI operations (#323, comments 6180641383 and 6191637927). Thus a busy/locked read during a recheck can disconnect a session that was never revoked. The added test does not exercise a failed poll. Please keep transient lookup errors from being treated as proof of invalidation, and cover that path.

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 awaitInvalidation wakeup, unchanged sessions, and revoked credential rejection. It does not satisfy #325's explicit real-open-WebSocket acceptance criterion; that remains unverified. Both changed files are registered in scripts/fork-upstream-edits.txt, and the Fork Stack Model CI check passed. I did not run tests.

@lukemaj
lukemaj force-pushed the fix/325-cli-revocation-sockets branch from e08bfc4 to d4f2e87 Compare October 18, 2026 23:25
@github-actions github-actions Bot added size:M and removed size:XS labels Oct 18, 2026
@lukemaj
lukemaj force-pushed the fix/325-cli-revocation-sockets branch from d3190dd to 44d6d6d Compare October 18, 2026 23:32
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Grok roast round 2 at 44d6d6d: PASS

Reviewed 44d6d6db5071d1238279fef6f71f24c18bc6e52f read-only. Did not re-run the suite. Probed PRAGMA data_version on this machine's node:sqlite (SQLite 3.53.4).

Round-1 blocker is fixed

The per-socket writer recheck is gone. SessionStore.make forks one watchCrossProcessRevocations. Each tick reads PRAGMA data_version and, only after a change, one SELECT of the open ids. Both effects go through NodeSqliteClient.readOnly. On a file database Sqlite.layerFromPath sets readerWorker: true, and readOnly routes to that worker: a separate DatabaseSync opened read-only, busy_timeout 5000, its own semaphore, off the main thread. The writer semaphore is not taken. Effect.catchCause logs and leaves checkedVersion unchanged, so SQLITE_BUSY does not publish clientRemoved. ws.ts still races sessions.awaitInvalidation, which is what that publish completes. Same-process revoke still publishes directly.

Attacks

Reader. Confirmed for the disk server. If the worker is missing (in-memory, or makeReader has set unavailable), readOnly falls back to the writer, the same path as every other read. A lock can then block the event loop for busy_timeout again. The catch still does not close the socket. That is the residual of the stall, and it is the existing reader-outage path.

Natural expiry. awaitInvalidation still re-reads and sleeps until expiresAt. The watcher selects revoked_at IS NULL and leaves expiry to that deadline, so a live socket still closes when the deadline it already armed fires. A cross-process edit of expires_at that leaves revoked_at null waits for that armed deadline.

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 checkedVersion, so a revoke committed while idle is still visible on the next non-empty tick.

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. wal_checkpoint(TRUNCATE) moves it once, which costs one extra SELECT and does not close a live session. A cached prepared statement sees the next commit. Server writes bump the reader because the worker is a different connection, so a busy server does at most one SELECT per tick, as the PR says.

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. #14355 is closed unmerged (mergedAt null, closed 2026-10-03). Its body is a per-socket 30s re-read that ends the connection when the read fails. Macroscope's approvability check said not approvable. #8418 and #13844 are closed unmerged. #17370 is merged. The corrected record matches.

Non-blocking

The version cursor can skip a socket that connects during the tick. The tick snapshots open ids, then reads data_version, then selects that snapshot. markConnected updates the ref and commits last_connected_at between those steps. If another socket is already open, the tick does not bail, the snapshot omits the new id, and that commit (the revoke, or the revoke plus last_connected_at) is stored as checkedVersion. Later ticks with the same version skip the SELECT. verify already passed on a pre-revoke snapshot. The socket stays until a later commit, including a truncate checkpoint, or its own deadline. Sockets that were already open are in the snapshot and still close on that tick. Re-read the open set after the version, and select any id this cursor has not been checked against.

Checked, not findings

forkScoped without startImmediately still schedules the fiber (forkUnsafe queues it when immediate is omitted). The unit test's explicit startImmediately: true is for TestClock. The copied /ws route in the socket test is a stand-in; the production route still races awaitInvalidation.

@lukemaj
lukemaj force-pushed the fix/325-cli-revocation-sockets branch from 44d6d6d to 8f98125 Compare October 19, 2026 02:08
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Luna review round 2 at 8f98125: CHANGES REQUESTED

  • Blocking — missed revocation race — apps/server/src/auth/forkCrossProcessRevocation.ts:37-46. If a WebSocket verifies its session before an external revoke commits, but calls markConnected after this tick snapshots openSessionIds, the query omits that session and then stores the new checkedVersion. Subsequent ticks see the same data_version and skip the query, so the revoked socket remains connected despite successful reads. The path is reachable because ws.ts verifies before markConnected (apps/server/src/ws.ts:3286-3288). The new deterministic test only registers during dataVersion read, before the snapshot, so it does not cover registration after the snapshot. Ensure newly registered sessions are checked against the current database state, and add a deterministic regression for registration after the snapshot but before the version is marked checked.

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 SessionStore.make and registered in fork metadata. Test suites were not run during this review.

@lukemaj
lukemaj force-pushed the fix/325-cli-revocation-sockets branch from 8f98125 to 7c45bb2 Compare October 19, 2026 02:29
@github-actions github-actions Bot added size:L and removed size:M labels Oct 19, 2026
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Luna review round 3 at 7c45bb2: CHANGES REQUESTED

Blocking: the post-snapshot registration race still loses a revocation (apps/server/src/auth/forkCrossProcessRevocation.ts:37-48). Interleaving: S verifies valid at t0; the external revoke commits at t1; the watcher reads the bumped data_version at t2 and snapshots open sessions at t3 without S; S then registers at t4. The watcher queries only its t3 snapshot and advances checkedVersion to the t2 version, so later ticks skip the query and S remains connected. The new deterministic regression registers S during dataVersion, before the watcher takes its second open-session snapshot, so it does not model this interleaving and would not fail on it. Keep newly registered sessions eligible for a check after any snapshot/cursor advance, and add the exact post-snapshot regression.

Confirmed: on dataVersion or listActive read errors the catch leaves checkedVersion unchanged and invalidates no sessions, so a healthy socket is not closed by a failed read. I did not run tests.

lukemaj and others added 5 commits October 19, 2026 05:00
…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>
@lukemaj
lukemaj force-pushed the fix/325-cli-revocation-sockets branch from 7c45bb2 to 6d1d1c4 Compare October 19, 2026 03:00
@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Luna review round 4 at 6d1d1c4: CHANGES REQUESTED

Blocking: the watcher stops polling after its first tick while sessions remain watched (apps/server/src/auth/forkCrossProcessRevocation.ts:57-65). Effect.forever repeats untilWatched, which waits for a change on watchedSessions each time. After the first registration wakes it and one tick runs, the next iteration waits for another registration/unregistration change instead of sleeping and polling. If an open session remains watched, a later CLI revocation will not be observed until some socket changes the watched set, so its existing socket can retain upgrade-time RPC access indefinitely. Keep the idle gate only while empty, then repeat sleep/tick while watched sessions exist; add a deterministic test with a continuously watched session and two successive external commits/revocations, requiring the second to invalidate without any watched-set change.

Cleared in this round: the late registration during dataVersion read test (forkCrossProcessRevocation.test.ts, “checks a socket that registers while the commit version is being read”) exercises the snapshot/cursor race and its ordering would miss the late id under 7c45bb2d93; registration before the first row read covers the connect-time read/revoke/snapshot/register order. Watcher read failures are caught before cursor advancement and do not invalidate sockets. The no-watched initial state performs no DB read and starts with an unchecked cursor on registration. Same-process revocation still publishes through the existing PubSub. The source uses one shared watcher and one active-session query per observed version. Both changed fork files appear in the fork feature registry. Upstream PR #14355 is closed unmerged and did implement a 30-second per-session cross-process reread that failed closed on read errors.

@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Luna review round 5 at 6d1d1c4: APPROVE

The round-4 polling finding is a false positive. At this exact source, SubscriptionRef.make uses PubSub.unbounded({ replay: 1 }) and publishes the initial value; changes is Stream.fromPubSub(self.pubsub), documented to emit the current value before future changes. Stream.fromPubSub subscribes through Channel.fromPubSubArray, whose PubSub subscription takes replay-window values before waiting for new messages. Thus, when the watched map is non-empty, each new untilWatched stream immediately gets the replayed current map, passes the filter, and allows the loop to sleep and tick again. apps/server declares catalog effect, resolved by pnpm-lock.yaml to 4.0.1, matching the vendored 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.

@lukemaj

lukemaj commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

Grok roast round 3 at 6d1d1c4: PASS

Reviewed 6d1d1c4e92f7168a8897b4b46727ce9a4970e819 read-only. Did not re-run the suite. No blocking or non-blocking findings.

The race is closed by order

awaitInvalidation updates watchedSessionsRef and only then calls getById (SessionStore.ts:1137-1153). The tick reads data_version and, only when that value differs from checkedVersion, snapshots the ref and selects (forkCrossProcessRevocation.ts:46-57). checkedVersion is stored after emitRemoved. A failed read leaves it alone.

Take a watcher S and a revocation commit R.

  • If S's getById starts after R is committed, that statement sees revoked_at and returns. The upgrade read in authenticateWebSocketUpgrade can still miss R. The socket does not keep that result: awaitInvalidation reads again after it registers. "closes a socket that starts watching after a tick already passed its revocation" is this order, and it returns with no later commit and no later tick.
  • If S's getById does not see R, that read started before R was visible. Registration finished before the read, so it finished before R was visible. The tick that observes R's data_version runs after R is visible, and the snapshot runs after that read. SubscriptionRef.update assigns and publishes before it returns, and the snapshot is SubscriptionRef.get. S is in the snapshot, listActive sees R, and emitRemoved runs before the cursor moves.

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 window

With an empty set the fiber is blocked on SubscriptionRef.changes (forkCrossProcessRevocation.ts:40-42, effect 4.0.1 PubSub replay 1). It does not sleep, and it does not read data_version, so a revoke committed while idle does not move checkedVersion. The empty return inside the tick (:45) is the same: it returns before the version read.

Then a socket registers. Its getById is a new writer statement (AuthSessions.getById is not readOnly) and runs after the commit, so it sees revoked_at and awaitInvalidation returns on that read. The watcher wakes, sleeps one interval, and ticks. That tick is redundant for this socket. The late-socket test is the harsher form of the same fact: the cursor has already stored the version, and the socket's own read still finishes the wait.

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

revoke still publishes clientRemoved before it returns (SessionStore.ts:1086-1092). The subscription is taken before the first read (:1150). A publish that lands during getById stays on the unbounded pubsub and the race takes it when the read returns. The poll is not on this path. revokeAllExcept still publishes each id the same way.

EnvironmentAuth.revokeSession is sessions.revoke. The permanent-peer test that timed out jumps 31 days and then 3653 days with no awaitInvalidation running, on the in-memory store. The idle wait is a subscription, so TestClock does not replay it. Its "immediate" check is authenticateMcpClient after revokeSession: a fresh read of the revoked row. A connected permanent session is the same awaitInvalidation race as any other socket, so a same-process revoke still completes that race on the publish. While that socket is registered the 2s pragma runs again, which is the watched-set cost, and it does not sit in front of the publish.

Design

Still 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 TestClock.adjust("3653 days") replayed. Waiting on the ref is the smallest way to stop that sleep. A per-socket poll and a CLI notify endpoint remain larger.

Checked, not findings

On effect 4.0.1, Fiber.interrupt while the caught tick is suspended still ends the fiber (catchCause does not run, the exit is interrupted). Scope close awaits that fiber (fiberInterruptAs), and this path completes. A lock during the tick still logs and retries without publishing and without storing the version. Expiry is still the deadline armed by the session's own read. I did not re-run tests.

@lukemaj
lukemaj merged commit 0f20733 into main Oct 19, 2026
27 of 28 checks passed
@lukemaj
lukemaj deleted the fix/325-cli-revocation-sockets branch October 19, 2026 04:23
lukemaj added a commit that referenced this pull request Oct 29, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(auth): CLI revocation must invalidate sockets in the running server

1 participant