Skip to main content
← Back to list
01Issue
BugOpenSwamp CLIPublic
AssigneesNone

Relationships

#2535 serve: server token GC follow-ups (upgrade backlog holds the sync gate, not-found matching, owner lookup, UX)

Opened by stack72 · 9/25/2026

Non-blocking review notes on the server token GC from the swamp-club#2413 verification (PR #2631). None affects correctness or security. The GC fails closed in each case, but each is worth tightening.

1. First sweep after an upgrade holds the sync gate for a long time (MEDIUM)

Until #2413, serve never ran the GC, so on upgrade every revoked or expired server token ever minted is eligible at the first sweep. On a long-running OAuth deployment that could be thousands.

collectToken (src/serve/server_token_gc_deps.ts) takes the exclusive sync gate per token, deletes locally, then calls syncService.pushChanged, which is a remote index round trip. At a few hundred ms per push, a backlog of 5,000 holds the gate almost continuously for tens of minutes:

  • WebSocket mutation handlers queue behind each unit.
  • Pollers skip and escalate.
  • Every HA replica does the same at once.

It does not block /ready, and it finishes.

Options:

  • Push once every N collected tokens, or once per sweep. This means accepting that a poller pull could land between a delete and its push, or holding the gate across a small batch.
  • Cap the tokens collected per sweep.

2. Not-found detection matches message text (LOW)

deleteIgnoringNotFound treats any vault error matching /not found/i as "already deleted".

  • The control-plane provider's delete is idempotent, so this mostly affects the legacy-vault branch.
  • There, an unrelated error ("credentials file not found", "key not found in keyring") would count as success. The records would be removed while the legacy secret stays.

A typed not-found error from vault providers would be tighter.

3. Owner lookup covers only the auto-definitions directory (LOW)

The GC resolves a token's owning definition through a repository rooted at autoDefinitionsDir, while token_auth.ts uses the shared repository, which also searches the primary models directory.

A server-token definition still outside auto-definitions (an older layout that boot migration missed) takes the orphan branch. Its token-main is deleted, but the definition and secret stay and a warning is logged each sweep. This fails safe but leaks.

The fix is to resolve the owner the way auth does, or to use the orphan branch only when neither repository finds the name.

4. A tampered legacy record can choose the vault (LOW)

The legacy branch deletes the canonical server-token-<name> key from the record's vaultName when its secretKey is canonical. Someone who can write the datastore can point vaultName at any delete-capable vault. The key name is fixed, so the blast radius is one key name.

Options:

  • Only do the legacy delete when the key exists in that vault.
  • Only delete from vaults that actually held server-token secrets.

5. UX

  • Daemon help: the serve daemon enable help for --token-gc-grace-period omits "0 deletes at expiry", which serve --help has.
  • Quoting 0: the skill guide (references/serve/guide.md) doesn't mention that 0 must be quoted in serve.yaml. The type error ("expected string, got number") could also suggest quoting duration values.
  • Case of units: the zero check is case-insensitive (0H accepted) while parseDuration is case-sensitive (1H rejected).
  • Comment accuracy: the serve.ts comment "starts after token secret migration" is accurate only for OAuth mode, which is the only mode that runs migrateTokenSecrets.
02Bog Flow
◉OPEN○TRIAGED○IN PROGRESS○SHIPPED

Open

9/25/2026, 4:10:15 PM

No activity in this phase yet.

03Sludge Pulse

Sign in to post a ripple.