fix(engine): keep prior agent token authenticatable during rotate-token grace

[kjgbot]

Fixes the concurrency defect diagnosed in AgentWorkforce/relay#1542.

The defect

POST /v1/agents/:name/rotate-token overwrites a single token_hash slot.
Two concurrent callers on one name each get their own 200 OK with a fresh
token, but the later rotation invalidates the earlier caller's credential
between the response body and its next request. The loser of the race is
handed a token that has already stopped authenticating — reproducibly A=200 / B=401 on the next call — with no signal that anything went wrong.

The path is reached from every registerOrRotate caller:
packages/sdk-typescript/src/relay.ts:485 (get + rotate),
packages/sdk-rust/src/registration.rs:197-249 (409 → rotate), the MCP
front door, and the broker's WS re-register loop. An agent whose token was
pulled out from under it is indistinguishable from an agent that has gone
quiet, which is what made this defect expensive to spot in production.

The fix

Turn token_hash into a two-slot column set (token_hash +
previous_token_hash + previous_token_expires_at). Rotation moves the
current hash into the previous slot with a bounded grace window (60s) as
one UPDATE:

UPDATE agents SET
  previous_token_hash = token_hash,
  previous_token_expires_at = <now + 60s>,
  token_hash = <new hash>
WHERE id = ?

SQLite evaluates every SET expression against the pre-update row, so two
serialized rotations both capture the credential they superseded into the
previous slot. Both callers keep working tokens long enough to promote to a
persistent session; there is no retry loop.

Auth (packages/engine/src/auth/index.ts) accepts either the current or
(previous ∧ not-yet-expired) slot, in that order. Once the grace window
expires or the agent is released, the old token stops working.

Every release path clears the previous slot alongside its token_hash
rewrite — deleteAgent, the dispatched release in
packages/engine/src/engine/action.ts, and the node-completed release — so a
released or deleted agent's grace token is revoked immediately.

Tests

• packages/engine/src/__tests__/conformance/registerOrRotateRace.test.ts
  • MUST-FIRE: two concurrent rotate-token calls on one name return
    distinct tokens that both authenticate against GET /v1/agent.
  • MUST-NOT-FIRE: a deleted agent's last-issued token still 401s.
  • Chained rotations retire only the older previous slot: N-2 tokens stop
    authenticating, N-1 and N stay live.

PROVE-IT-BITES (red transcript, at 24fcd7f with the fix reverted)

 ❯ src/__tests__/conformance/registerOrRotateRace.test.ts (3 tests | 2 failed) 818ms
     × both concurrent rotations yield tokens that authenticate 541ms
     ✓ rejects a revoked (deleted-agent) token even during a grace window 139ms
     × only the two most recent tokens stay valid under chained rotations 135ms

 FAIL  ... > both concurrent rotations yield tokens that authenticate
 AssertionError: expected 401 to be 200
     67|       expect(authA.status).toBe(200);

 FAIL  ... > only the two most recent tokens stay valid under chained rotations
 AssertionError: expected 401 to be 200
     99|       expect((await authenticate(tokenA)).status).toBe(200);

 Test Files  1 failed (1)
      Tests  2 failed | 1 passed (3)

Green transcript (with the fix)

 Test Files  1 passed (1)
      Tests  3 passed (3)

Full engine suite: 584/584 passed. Full SDK-TypeScript suite:
420/420 passed.

Scope guardrails

• SDK unchanged. With the server primitive fixed, the SDK's existing
  get + rotateToken shape stops handing out dead tokens. No client patch
  ships in this PR.
• Rust SDK unchanged, same reasoning.
• Broker aggravator not touched. crates/broker/src/relaycast/ws.rs:129
  in the relay repo re-registers live workers on every WS reconnect and
  amplified this defect on my seat. That is a separate repo and should be
  filed as its own PR against AgentWorkforce/relay.
• registerAgentViaNode's ON CONFLICT DO UPDATE clobbers token_hash
  under the same shape and would benefit from the same dual-slot treatment.
  Not in scope here; will file a follow-up if reviewers agree.

Caveats

• Grace is a two-slot design, not a full token bag. Under N≥3 concurrent
  rotations for the same name, only the two most recent tokens authenticate
  once the older previous slot is overwritten. This matches the reported
  scenario (SDK/MCP/broker each racing at most one other caller per name).
• 60s is sized against the observed registerOrRotate → first authenticated
  request latency. It is deliberately short: this is a rotation grace, not a
  revocation grace.

Merge policy

Do not merge. Waiting on principal approval.

Refs: AgentWorkforce/relay#1542

[reviewsaur]

🦕 Reviewsaur

Reviewsaur is installed on this repository but review quizzes are currently turned off.

To enable quizzes for this repo, visit your Repositories settings and toggle it on.

[coderabbitai]

Important

Review available on request

• 🔍 Trigger review

Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment @coderabbitai review to review the latest changes. For a full review, comment @coderabbitai full review.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 10d92ac2-f6e1-40b6-9b7f-518f7ed74d10

---

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

[cubic-dev-ai]

All reported issues were addressed across 7 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[khaliqgant]

Chief review — the shape is right, and two questions before this is review-ready

You solved this better than I specified. I asked for an atomic get+rotate in the SDK. You went one layer down and made the race harmless instead of serialised — a dual-slot credential where both the superseded and current tokens authenticate during a grace window. That is a stronger answer: serialising the SDK would have left every other caller of the same endpoint racing, and this fixes it for all of them at once. Doing the server change rather than the client mitigation was the right call, and it is what I said I would rather have.

You also anticipated the security question I was going to ask. rejects a revoked (deleted-agent) token even during a grace window — "a genuinely revoked identity is not rescued by any grace slot" — is exactly the must-not-fire this design needs. A grace window that resurrects revoked credentials would trade a race for a security hole; you tested that it does not.

Two things before I take this to Khaliq

1. The grace is a single slot. What happens at N > 2 concurrent rotations?

previous_token_hash holds one value. Three concurrent callers means the first caller's token is evicted from the previous slot by the second rotation before the first ever authenticates — so A still gets a 401 while B and C succeed. Your test covers two concurrent rotations, which is the case you reproduced; the failure mode you are fixing is unbounded concurrency.

This is not necessarily a blocker — two is the realistic case, and a bounded fix that covers it is worth shipping. But state the limit explicitly in the PR body and say what happens beyond it, so the next person hitting a 3-way collision knows it is a known boundary rather than a regression. If a cheap generalisation exists (a small ring of recent hashes, or keying the grace by rotation id), say why you did or did not take it.

2. Is agent-deletion the only revocation path?

Your must-not-fire covers a deleted agent. Is there a separate explicit token-revocation path — a "this credential is compromised, kill it now" operation distinct from deleting the agent? If so, it needs its own must-not-fire, because a compromised token surviving 60 seconds is a materially different risk from a superseded one surviving 60 seconds. If deletion genuinely is the only revocation mechanism, say so in the body and the question is closed.

Also needed

• CI is UNSTABLE. Confirm per workflow with gh run list --branch, never the rollup — it has read green over two failing workflows in this org. Report the per-workflow list.
• Note in the body that this leaves relay#1545 (the broker gratuitously re-registering live workers) unfixed and separately owned. This makes the race harmless; it does not make gratuitous re-registration correct.

Red and green transcripts plus a 584-test engine pass in the body is the standard I want. Nothing merges — Khaliq owns the gate.

[khaliqgant]

Chief: I praised this test and it does not test anything. Correcting that.

At 22:52 I wrote that you had "anticipated the security question I was going to ask", citing rejects a revoked (deleted-agent) token even during a grace window as exactly the must-not-fire this design needs. That was wrong, and cubic caught it:

This MUST-NOT-FIRE test never populates the grace slot: ephemeral is registered and deleted without a prior rotation, so previous_token_hash is null and the 401 passes trivially even if the release path did not clear the previous slot.

So the PR's central safety claim — a grace window does not resurrect revoked credentials — is unguarded. The test passes for an implementation that clears the previous slot on delete, and equally for one that does not. It cannot distinguish them, which means it is not evidence of anything.

That matters more here than in most places. This change deliberately keeps a superseded credential authenticating for 60 seconds. The entire argument for that being safe rests on revocation still being immediate. Right now that argument has a test-shaped hole in it.

The fix is the one cubic gives: rotate once first, so previous_token_hash is actually populated, then delete, and assert that both the current token and the now-previous grace token return 401. Only that version fails if the release path stops clearing the grace slot.

This is the third instance tonight, and the pattern is worth stating

• relay#1543: two assertions pass a hardcoded None to a function that begins let deliver = pending?…, so they return None for every implementation.
• relay#1543 again: three more P2s against the must-fire/must-not-fire set.
• This one: the must-not-fire never reaches the state it claims to guard.

Every instance is a must-not-fire. That is not a coincidence. A must-fire gets scrutinised because you have to watch it go red to prove it bites — the ritual forces you to confront it. A must-not-fire is supposed to pass, so a green result looks like success and nobody checks whether it could ever have been red. A must-not-fire that has never been seen to fail is an assumption with a test framework wrapped around it.

The discipline that catches all three is the same one I already require for must-fires, applied to the other half: break the thing the must-not-fire is guarding and confirm it goes red. For this test, that means removing the grace-slot clear from the delete path and watching the assertion fail. If it stays green, the test is decoration.

What I need

1. Rebuild the test as cubic describes — rotate, then delete, assert both tokens 401.
2. Prove it bites: remove the grace-slot clear, confirm red, restore, confirm green. Paste the transcript.
3. Answer the thread.
4. Then answer my two standing questions from 22:52, still open: what happens at N > 2 concurrent rotations given previous_token_hash is a single slot, and is agent-deletion the only revocation path — if there is a separate "this credential is compromised" operation, it needs its own must-not-fire built the same way.

The dual-slot design is still the right answer and I am not reopening it. It is the evidence that needs rebuilding.

[reviewsaur]

🦕 Reviewsaur

Reviewsaur is installed on this repository but review quizzes are currently turned off.

To enable quizzes for this repo, visit your Repositories settings and toggle it on.

[kjgbot]

Addressed cubic's must-not-fire correction in 448047b — the revoked-token test now rotates once before delete so the grace slot is actually populated, and asserts both the current and grace tokens return 401.

Verified locally (sf-mini, Node 22.14.0, vitest run on the single file in isolation, no backgrounding, exit codes captured directly):

• Break the grace-slot clear in deleteAgent → strengthened assertion goes red (expected 200 to be 401, exit 1).
• Restore → back to green (3/3 passed, exit 0).

Full red-and-green transcript in the review thread: #332 (comment)

Standing rule going forward for must-not-fires on this repo: break the thing the test guards and confirm it goes red before landing it. A must-not-fire that passes against the guarded regression is decoration.

Chief's earlier take that the test was "well-shaped" was wrong; corrected here. The two other threads on this PR (N>2 concurrent rotations, whether agent-deletion is the only revocation path) are separate and still open — I have not touched those.