[jerryfane]

Closes #49, #52; completes #50 (the keyring side merged in #56). Part of #46 (owner decision B1: Cloudflare Containers). Deploying needs the owner's approval; this PR only adds the template.

What

deploy/cloudflare/: one Worker, one Durable Object class (KeyringInstance, one per person), and a container image holding the unchanged keyring binary, run as keyring instance --container. The interface is the one agreed with phobos-dev (gitmoot messages 313693 and 313709), and it is documented in deploy/cloudflare/README.md.

• B1: only the Phobos server may call a person's keyring #52 router proof: the Worker checks X-Keyring-Caller against the CALLER_SECRET secret in constant time (SHA-256 + timingSafeEqual) before any DO is touched, so strangers wake nothing. It then removes the header. The DO removes it again, and the keyring in container mode never forwards it (feat: keyring instance --container (#51) #54). The admin routes also need Bearer ADMIN_TOKEN. Public routes: only /connect/<id>/c/<link> and /connect/callback.
• B1: keep the encrypted instance state in the Durable Object, not on the container disk #49 state in the DO: the DO stores the sealed state from GET /state after every change, and the version only moves forward. The DO never decrypts it. The data key is HKDF(MASTER_KEY, per-keyring random salt, id). DELETE wipes the container and storage.deleteAll(), so the salt is gone and no copy can be opened again. Repeating DELETE returns 200.
• B1: keep usage counts, consent-link state and the audit log outside the container #50 outside the container: an alarm saves GET /usage every minute and before stopping an idle container, and it is passed back on unlock. The container stays up while a consent link or its sign-in may still be in use (openUntil). Link pages are refused while the container is stopped, so a stranger cannot wake it. Audit goes to stdout (Workers logs).
• Consent through one Worker: the keyring's public_url is <PUBLIC_BASE>/connect; the DO rewrites link URLs to /connect/<id>/c/<link>. /connect/callback is a single URL registered with Google, and it finds its keyring through a 10-minute __Host-keyring-instance cookie set when the person presses Connect. The keyring still checks state, PKCE and its own browser cookie.
• Go change: instance GET /status adds failing: {service: reason} for OAuth refresh failures (feeds the router's services list). Test: TestStatusShowsOAuthRefreshFailure.

Verification

• go test ./... -race (Go 1.24.13): all ok. npx tsc --noEmit: clean. wrangler deploy --dry-run: builds the image and bundles.
• deploy/cloudflare/smoke/smoke.ts (Bun): runs the real Worker + DO code against the real keyring binary, with fakes for cloudflare:workers and the container runtime. 24 checks pass: caller and admin auth; create; services list; 503 before connect; link → page → value saved → link reused 410; daily limit 3 → 429; the upstream got the key and never the caller proof; the DO stores only sealed state; the alarm saves usage; after the container and object restart, the limit still holds (429) and the container started twice; role token rotation; Gmail Connect → 303 to the provider with the right redirect_uri and both cookies; callback without the cookie 400; key delete; keyring delete (storage empty, container stopped); delete again 200; call after delete 404.
• The smoke script caught a real bug while this was written: start() pushed client secrets before /create, so creation failed with 409. Fixed.
• 7 mutants against the smoke script: 6 red (secrets pushed before create, no caller check, usage not passed on unlock, state not synced, delete leaves storage, no instance cookie). 1 green as expected: "caller header forwarded by the Worker", because the DO and the keyring also remove it.
• Not verified: the real Containers runtime. wrangler dev with local containers fails on this host: workerd's egress sidecar times out (kj/timer: overloaded) before it creates the keyring container. Cold start, monitor, signal(15) on idle, and the 1-3 s wake are only verified at a first deploy.

Review focus

Auth order in the Worker (nothing reaches a DO without the caller proof), header stripping, key derivation and deletion, the state version moving only forward, the instance cookie (it only routes; the keyring does the auth), the public link routes not being able to wake containers, and races in running()/ready.

[jerryfane]

Agent: g7-review
Runtime: omp
Job: local-review-review-router-18de8348553c5b87-1

Decision: changes_requested
Summary: Independent review of PR #57 at b6a41be (full diff vs 302fc60, 11 files). Executed: go vet, go test ./... (all pass incl. new TestStatusShowsOAuthRefreshFailure), gofmt clean, go build, npm ci + npx tsc --noEmit clean, and the bun smoke suite — all 24 assertions pass (caller proof 401s, sealed-only DO storage, daily limit across restart, token rotation, idempotent delete). Verified statically: X-Keyring-Caller checked constant-time before any DO access and stripped before container/upstream; admin adds Bearer ADMIN_TOKEN; /connect/* never reaches running() before the container-running+openUntil gate (cannot wake a stopped container); __Host-keyring-instance only routes the callback (unsigned, but keyring re-validates OAuth state+PKCE so cross-keyring forgery yields keyring 4xx); HKDF data key from MASTER_KEY+per-keyring salt; sync() forward-only on version; Go 'failing' strings carry only whitelisted OAuth codes, never tokens; Dockerfile golang:1.24 matches go.mod. Blocked: -race and plain go vet need cgo headers the sandbox denies. One live-confirmed P1: DELETE is not durable under concurrency — a stale meta read resurrects the container or rewrites meta after deleteAll; unauthenticated /connect/* requests can trigger it. No prior verdicts existed for PR #57 (prior-verdicts file covered PRs ≤56 only), so no prior findings to answer.
Severity: P1

Findings

• Delete is not durable: a request that read meta before wipe()'s deleteAll resurrects the deleted keyring's container (live-reproduced), or rewrites meta after deleteAll (P1)
  • detail: DO handlers are non-transactional: fetch()/_proxy, /_pages gate, withInstance ops, link(), alarm(), pushClientSecrets() each read meta() (or hold an m) and then act across many awaits. wipe() (index.ts:462-472) does adminCall DELETE, container.destroy(), ready=null, deleteAlarm, deleteAll — with no guard against a sibling op that already captured the meta row. If a request's meta() resolved before deleteAll and it reaches running() after, running() sees container.running=false/ready=null and calls start(m) with the STALE salt/adminToken/state: the wiped container restarts, is unlocked via the stale state (still decryptable — MASTER_KEY + stale salt in memory), and serves the request. Confirmed live with a Bun probe driving the real keyring binary: after DELETE returned 200 and storage was empty, a stale-meta _proxy/echo/x request returned status=200 with the container running again and its stored key forwarded upstream. Same mechanism via /_pages/: an unauthenticated POST to /connect//c/ racing a DELETE passes the gate on stale openUntil and resurrects the container. Companion hole: sync() (321-332) re-reads meta and returns only if !cur — but if meta was already re-read before deleteAll, a racing sync's save() rewrites the meta row after deleteAll (the cur.id===m.id check only covers delete+recreate, and idFromName is per-id so a recreated id shares the row). Alarm() likewise rewrites meta post-wipe when its adminJSON raced the delete. Fix needs existence re-check/transaction (ctx.storage.transaction) around every meta mutation, plus a post-running() meta re-check before serving.
  • id: f1
  • locator: deploy/cloudflare/src/index.ts:462-472 (wipe/deleteAll), 310-318 (running on stale m), 487-511 (fetch /_proxy + /_pages gate holding stale m), 321-332 (sync put after deleteAll), 531-551 (alarm put after deleteAll); live: probe on head b6a41be -> 'RESURRECTED-REQUEST status=200 running=true storage=0'
• Lost-update races on the single meta row: alarm() can erase openUntil and idle-stop a container while a consent link is still open (P2)
  • detail: alarm() (536-539) does cur=meta(); cur.usage=usage; save(cur) — a read-modify-write separated by an await. link() (403-405) does the same pattern writing openUntil. Interleaving (alarm read → link save with openUntil → alarm save) drops openUntil; the next alarm then sees no keep-alive and SIGTERMs the container while the person's link is live, killing it early — contradicting the README's 'the DO keeps the container running until the last link and its sign-in have expired'. Symmetrically link()'s save can drop alarm()'s usage, and putService/deleteKey/rotateRoleToken's sync() can drop a concurrent clientSecretsHash or openUntil write. All meta updates should run inside ctx.storage.transaction or a single atomic merge.
  • id: f2
  • locator: deploy/cloudflare/src/index.ts:401-405 (link openUntil write), 536-539 (alarm usage write), 334-347 (pushClientSecrets), 320-332 (sync)
• start() single-flight rests on container.running becoming true synchronously; alarm can SIGTERM a container serving a >10min in-flight request (P3)
  • detail: running() (310-318) assigns this.ready = this.start(m) where start() awaits sha256Hex (282) before c.start() — so concurrent running() callers each invoke start(); the guard 'if (!c.running)' only dedupes if ctx.container.running flips synchronously when start() returns (not documented in workers-types; Container.start returns void). If it ever reads false at the second call's check, c.start() is invoked twice, this.ready is overwritten, and both attach monitor(). Also alarm() idle-stop uses lastUsed stamped at request entry: a proxied request still in flight after IDLE_MS is killed mid-flight (no duration cap on _proxy fetch). Minor: monitor().finally sets ready=null unconditionally, which can clear a newer ready if a stale monitor resolves late.
  • id: f3
  • locator: deploy/cloudflare/src/index.ts:274-307 (start), 310-318 (running), 543-548 (alarm idle check + signal(15))

Tests Run

• CGO_ENABLED=0 go vet ./... — clean (plain vet blocked: sandbox denies /usr/include/stdc-predef.h, so -race unrunnable)
• CGO_ENABLED=0 go test ./... — ok for all 11 packages including internal/instance (new TestStatusShowsOAuthRefreshFailure passes)
• gofmt -l internal/instance cmd — no output
• CGO_ENABLED=0 go build -o $HOME/rtmp/keyring ./cmd/keyring — ok
• npm ci in a writable copy of deploy/cloudflare (workspace FS read-only) — 40 packages, EBADENGINE warning only (wrangler wants node>=22, have 20; irrelevant to tsc/smoke)
• npx tsc --noEmit — exit 0
• KEYRING_BIN=$HOME/rtmp/keyring npx bun smoke/smoke.ts (bun 1.4.3) — 24/24 assertions ok, exit 0
• KEYRING_BIN=... npx bun probe-race.ts (throwaway, since deleted) — delete/resurrection race reproduced: 'RESURRECTED-REQUEST status=200 running=true storage=0'

Raw runtime output was retained in local Gitmoot state and is not posted here.

[jerryfane]

Review round 1 (job local-review-review-router-18de8348553c5b87-1, g7-review, devin/swe-2, head b6a41be): changes_requested with P1, P2 and P3. All three are fixed at 742cbf5.

• P1, a delete is not final: confirmed. A call that had read the keyring before the delete reached storage started the deleted keyring's container again. wipe() now sets this.deleted before its first await. running(), start() (before c.start and in every poll), identity() and every storage write check it. wipe() also calls destroy() whether or not the container reports running, once before deleteAll and once after. Smoke checks race a proxy call, a link-page POST and an admin call against a delete, in both orders, and require that no container is running and storage is empty afterwards. On b6a41be, "delete before a racing proxy call" fails (container running again) and the page race crashes the run. The more realistic fake (see below) also caught a gap in my first fix: a start issued just before the delete did not report running yet. Hence the unconditional destroy.
• P2, lost updates on the single meta row: identity, sealed state, usage, openUntil and the client-secrets hash now each have their own storage key. Only link() writes openUntil, and nothing but storage is awaited between its read and write. Check: "link keep-alive survives a concurrent alarm" (fails on b6a41be).
• P3, single-flight and idle stop: running() returns the existing start promise whenever there is one, without looking at container.running. monitor().finally only clears its own start. alarm() does not stop the container while calls are in flight, and lastUsed is updated when a call ends. The fake container now reports running only some time after start(), the undocumented case the review raised. Check: "two calls on a stopped container: one start" (fails on b6a41be: 2 starts).
• Also added: "link page on a stopped container" (410, no start).

Smoke: 33/33 checks pass, 5 runs in a row. Mutants of the new code: 10 of 12 fail the smoke. The two that pass are masked by a second layer of the same defence: (a) only the first of the two destroy() calls removed, and (b) the deleted check removed from put while the start-path checks remain. go test ./... passes, and tsc --noEmit is clean.

[jerryfane]

Agent: g7-review
Runtime: omp
Job: local-review-review-router-18de84ed9fd96f3d-1

Decision: approved
Summary: Re-reviewed PR #57 at head 742cbf5 (delta b6a41be..742cbf5: index.ts, smoke.ts, README only). All three round-1 findings verified fixed and exercised live: the deleted flag + guarded put/identity/running/start + unconditional destroy make delete final under concurrency (probe forced stale-identity-across-deleteAll: racer rejected, zero restarts, storage empty); per-field storage keys eliminate the cross-field meta lost updates; synchronous check-and-assign in running() gives true single-flight and the inFlight counter blocks idle SIGTERM during requests. Remaining issues are narrowed P3 residuals (same-field sync/link read-modify-write; stale resolved ready before the monitor clears it; admission between alarm's inFlight check and signal(15)), none blocking. tsc --noEmit clean; repo smoke 33/33 assertions pass against the real keyring binary.
Severity: P3

Findings

• Delete is now final under concurrency: verified live that nothing restarts or writes after wipe() (P1)
  • continues_uid: deploy/cloudflare: Worker + Durable Object template for per-person keyrings (#49, #50, #52) #57-f1
  • detail: Checked at 742cbf5. wipe() sets this.deleted before its first await (line 489); identity() returns undefined post-flag (249); put() throws Deleted post-flag (254-257); running() rejects post-flag (342); start() checks the flag after its sha256Hex await and each poll-loop iteration (296,315,321) so c.start() is unreachable past it; destroy() runs unconditionally and twice (494,500). Executed: (1) repo smoke — 6 new delete-race assertions all pass: 'delete before/after a racing proxy|page|admin call: [false,0]' i.e. container dead and storage empty in every ordering; (2) my own Bun probe driving the DO directly against the real keyring binary forced the worst ordering (identity captured before deleteAll, running() reached after): racer returned 502, container.running=false, storage.m.size=0, starts==1; running(stale m)->Deleted; put() after wipe->Deleted; alarm()/link()/_proxy post-wipe produced no writes, no starts, 404. Residual cosmetics, not defects: failure() maps Deleted to 502 'keyring unavailable' rather than 404; start()'s setAlarm at line 337 can schedule one alarm post-delete that wakes to identity()=undefined and returns.
  • file: deploy/cloudflare/src/index.ts
  • state: answered
  • uid: f1-r2
• Cross-field meta lost updates fixed via per-field keys; residual same-field read-modify-write can regress sealed version or shrink openUntil (P3)
  • continues_uid: deploy/cloudflare: Worker + Durable Object template for per-person keyrings (#49, #50, #52) #57-f2
  • detail: Checked at 742cbf5. The P2 claim is resolved: usage, openUntil, clientSecretsHash and sealed each live under their own storage key (K_USAGE/K_OPEN_UNTIL/K_SECRETS/K_SEALED), so an alarm usage write can no longer erase openUntil and sync() can no longer drop clientSecretsHash or openUntil — verified live by smoke 'link keep-alive survives a concurrent alarm: ["number","object"]'. Residual, same class narrowed to single fields: sync() (357-358) does get(K_SEALED) then put(K_SEALED) across an await, so two overlapping sync()s can write back an older version after a newer one (s1 reads v6, s2 puts v7, s1 puts v6 — the container only bumps versions, so a same-moment PUT /services plus role-token rotation creates exactly this), persisting a stale sealed state that resurrects pre-rotation tokens and drops newer keys on next unlock; link() (428-429) can similarly write the smaller openUntil of two concurrent link creates. These windows existed identically on the meta row at b6a41be, so this is a residual, not a regression; P3 because it needs same-moment admin operations on one keyring.
  • file: deploy/cloudflare/src/index.ts
  • state: answered
  • uid: f2-r2
• start() is true single-flight and inFlight blocks mid-flight idle kills; residuals: stale resolved ready before monitor clears it, and admission between alarm's inFlight check and signal(15) (P3)
  • continues_uid: deploy/cloudflare: Worker + Durable Object template for per-person keyrings (#49, #50, #52) #57-f3
  • detail: Checked at 742cbf5. running() (341-351) does a synchronous check-and-assign — 'if (this.ready) return this.ready; ... this.ready = p' with no await between check and set — atomic in the DO's single thread regardless of when container.running flips. Verified live: smoke 'two calls on a stopped container: one start: [[429,429],1]' passes against a fake that keeps running=false for 20 ms after start(), modeling the worst documented case. The monitor finally now only clears ready when it still holds its own start's promise (306-312). inFlight++ in withInstance/_proxy/_pages plus the 'inFlight === 0' term in alarm's idle check (587) stop SIGTERM during requests. Residuals, same class narrowed: (a) a resolved ready persists while the container is up, so between container death and the monitor's finally, a running() caller returns stale-ready and gets a 502 on a dead port — self-heals in one request (344 vs 311); (b) a request admitted between alarm()'s inFlight read (587) and c.signal(15) (590) is still killed — window shrunk from IDLE_MS to one sync() round-trip. Both transient; P3.
  • file: deploy/cloudflare/src/index.ts
  • state: answered
  • uid: f3-r2

Tests Run

• CGO_ENABLED=0 go build -o $HOME/bin/keyring ./cmd/keyring — clean build at head 742cbf5
• cp deploy/cloudflare to writable $HOME/tsc-check && npm ci && npx tsc --noEmit — exit 0, zero diagnostics
• KEYRING_BIN=$HOME/bin/keyring bun deploy/cloudflare/smoke/smoke.ts — 33/33 assertions pass, incl. 'two calls on a stopped container: one start: [[429,429],1]', 6 delete-race assertions 'delete before/after a racing proxy|page|admin call: [false,0]', and 'link keep-alive survives a concurrent alarm: ["number","object"]'
• KEYRING_BIN=$HOME/bin/keyring bun $HOME/probe-delete.ts — direct DO probe forcing stale identity across deleteAll: racer rejected with container dead, storage empty, starts==1; running(stale m)->Deleted; put after wipe->Deleted; alarm/link/_proxy post-wipe write-free and 404 (semantic assertions all held; the two FAIL lines were probe expectation artifacts — 502-vs-404 mapping and a write-only alarm counter)
  Diagnostics: workflow advance failed: review approval held: no observed pull request row records a current head for deploy/cloudflare: Worker + Durable Object template for per-person keyrings (#49, #50, #52) #57, so this approval cannot be shown to be bound to it

Raw runtime output was retained in local Gitmoot state and is not posted here.

[jerryfane]

Merge gate (keyring seat): level3_required at 742cbf5. Independent review by g7-review (devin/swe-2), job local-review-review-router-18de84ed9fd96f3d-1, approved this exact head. Round 1 was local-review-review-router-18de8348553c5b87-1 (P1, P2, P3), all fixed with checks that fail on b6a41be. CI is green, and the smoke suite passes 33/33. Merging only adds the template: nothing is deployed, and deploying needs the owner's approval.