docs: describe device-token rotation race as open, not fixed

davidangularme · davidangularme · commit 53849c6c993c · 2026-04-28T12:17:47.000+03:00
Codex review on #71156 correctly flagged that the AGENTS.md was describing a synchronous 'invalidated' flag on GatewayWsClient plus a per-RPC dispatch guard as already-landed guarantees, when they live in the still-open fix/gateway-device-token-rpc-revalidation PR. On main, GatewayWsClient has no 'invalidated' field and message-handler.ts only re-checks usesSharedGatewayAuth. Documenting the proposed fix as a shipped guarantee could let reviewers wave through rotate/revoke code paths that are in fact still racy against pipelined RPCs. Rewrite three references in the two AGENTS.md files: - Reframe the 'Related incidents' list in server-methods/AGENTS.md from 'landed' to 'in-flight' and mark rotate/revoke on main as best-effort rather than race-safe. - Rewrite the 'Shared-auth vs device-token' section in channels/plugins/AGENTS.md to describe current main (no per-RPC check for device-token) and position the 'invalidated' flag as a proposal, not a state. - Rewrite the rotate/revoke 'must never happen' bullet from 'the invalidated flag must be set before respond()' to a mechanism- agnostic invariant ('old-token authority must end before response flush, whichever mechanism closes the race'). No code changes. Keeps the content honest against main until the device-token PR lands (which this doc PR does not depend on).
diff --git a/src/channels/plugins/AGENTS.md b/src/channels/plugins/AGENTS.md
@@ -55,20 +55,25 @@ This asymmetry is subtle and load-bearing:
 
 - **Shared-auth clients** (`usesSharedGatewayAuth: true`) are re-checked
   on every RPC against `sharedGatewaySessionGeneration` at
-  `src/gateway/server/ws-connection/message-handler.ts:1444-1458`. A token
-  rotation forces the next RPC from any stale client to close with
-  `4001 gateway auth changed`.
-- **Device-token clients** were historically not re-checked per RPC. A
-  `device.token.rotate` / `.revoke` scheduled the socket close via
-  `queueMicrotask`, and RPCs already pipelined in the WS buffer landed
-  with the old token. Fixed in `fix/gateway-device-token-rpc-revalidation`
-  by adding a synchronous `invalidated` flag on `GatewayWsClient` set
-  before `respond()` and checked at the top of the per-RPC dispatch.
+  `src/gateway/server/ws-connection/message-handler.ts` in the per-request
+  dispatch block. A token rotation forces the next RPC from any stale
+  client to close with `4001 gateway auth changed`.
+- **Device-token clients are not re-checked per RPC on `main`.** A
+  `device.token.rotate` / `.revoke` schedules the socket close via
+  `queueMicrotask`, so RPCs already pipelined in the WS buffer can land
+  with the old token before the disconnect takes effect. The PR
+  `fix/gateway-device-token-rpc-revalidation` proposes closing this race
+  with a synchronous `invalidated` flag on `GatewayWsClient` set before
+  `respond()` and checked at the top of the per-RPC dispatch — but until
+  that PR lands, **treat `device.token.rotate` / `.revoke` as
+  best-effort rather than race-safe**, and be careful about adding
+  operations whose correctness depends on the rotate response arriving
+  before any further RPC is processed.
 
 If you add a new auth method in this zone, decide explicitly which of
 these two per-RPC check shapes it follows and document it. Don't leave
 the third case "I didn't think about it" — that is exactly how the
-device-token race landed.
+device-token race exists in the first place.
 
 ## What must never happen
 
@@ -78,10 +83,13 @@ device-token race landed.
   `allowFrom` list, or vice versa. If the two stores disagree, a
   previously-paired sender may silently get rejected or a revoked sender
   may silently get accepted.
-- A `device.token.rotate` / `.revoke` response arriving at the admin
-  while the old token is still authenticating RPCs. The `invalidated`
-  flag must be set _before_ `respond()` fires, not in a microtask that
-  races the response flush.
+- A `device.token.rotate` / `.revoke` completing (from the admin's
+  point of view) while pipelined RPCs on the rotated client can still
+  authenticate with the old token. Whichever mechanism closes this race
+  (the in-flight `invalidated` flag proposal, or something equivalent),
+  the invariant is that the old-token authority ends **before** the
+  response flush, not after — a microtask-scheduled disconnect alone is
+  not sufficient.
 - A pairing adapter's `notifyApproval` throwing without a downstream
   catch that either (a) rolls the approval back or (b) logs the
   partial-commit loudly. Silent failure here is the pattern that put
diff --git a/src/gateway/server-methods/AGENTS.md b/src/gateway/server-methods/AGENTS.md
@@ -59,9 +59,9 @@ place to add logic.
 
 ### Related incidents (silent-failure shape)
 
-These three PRs landed on the same class of bug in adjacent files. Same
-shape, different surfaces — cite them as examples of what "regress silently"
-looks like concretely:
+Three in-flight PRs target this same class of bug in adjacent files. At
+time of writing some may still be open against `main`; treat the shape
+description as the invariant, not the merge status:
 
 - `fix/config-restore-false-success` — bare catch on `copyFile` during a
   suspicious-read recovery caused the audit log to report `valid: true`
@@ -72,10 +72,12 @@ looks like concretely:
   rotation that threw on close was swallowed, leaving the client
   authenticated past the point the admin thought they were kicked.
 - `fix/gateway-device-token-rpc-revalidation` — rotate/revoke responded
-  OK before the microtask-scheduled disconnect, and there was no per-RPC
-  re-check for device-token auth (only for shared-auth). Pipelined RPCs
-  landed with the rotated token. Fixed by a synchronous `invalidated`
-  flag plus a dispatcher-level guard.
+  OK before the microtask-scheduled disconnect, and there is no per-RPC
+  re-check for device-token auth on `main` (only for shared-auth).
+  Pipelined RPCs could therefore land with the rotated token. The PR
+  proposes a synchronous `invalidated` flag plus a dispatcher-level
+  guard; until it lands, **treat rotate/revoke as best-effort, not
+  race-safe**.
 
 The common pattern: **a privileged operation claims success before the
 security guarantee is actually established.** Watch for it in reviews of
