oficina / sheets Public

New commit: sign-in mail is now sent after the response. This is the same timing oracle Astra found in Slides (#20, P1). Before, an admitted address waited for SMTP before getting ?sent=1 and a stranger did not, and forced posts did the same.

• The change: - auth's Mailer is now mailQueue (internal/sheets/mailqueue.go). Send queues the link and returns immediately. - One worker asks App.Admits and only then calls SMTP. Failures are logged, never answered. - The queue holds 64 links. When it is full, the link is dropped and a line is logged. - main drains the queue after Serve returns (DrainSigninMail). - admissionMailer is gone.
• The test: TestSigninNeverWaitsForTheMailer blocks the mailer and sends four POSTs: an admitted address, a stranger, and both again with force=1. Every one must answer 303 ?sent=1 while the mailer is still blocked. After release, the two admitted links must go out. It fails on 2dfacac.
• Test recorder: it now waits for the queue before anything reads it.

All six gate steps pass locally (vet, fmt, build, migrations, go test, race).

I've applied Astra's second-round findings on oficina/slides #20 to the identical queue here. The commit is the new head.

• P1: Send does no I/O at all, not even the "queue full" log line. - Dropped links are counted in an atomic, and the worker logs the count. - Every address's link is queued, admitted or not, so how full the queue gets does not depend on admission.
• P2: the worker recovers from a panic on each message and keeps draining.
• P2: sends now run under the queue's context. Past the drain deadline, close cancels the send in flight, abandons whatever is still queued with a log line, and the worker exits.
• Leaks: each plugin's queue is registered under that plugin. CloseSigninMail(plugin) stops it, and all four tests that build a plugin now call it in cleanup.

New tests:

• TestMailQueueSendNeverBlocksWhenFull (stalled log sink, twice the queue depth)
• TestMailQueueQueuesEveryAddress
• TestMailQueueSurvivesAPanic
• TestMailQueueCloseIsBoundedAndStopsTheWorker (checks the goroutine count)
• TestCloseSigninMailStopsTheWorker

To show each failing first, I put the defect back for each one:

• A log line in Send makes it block.
• Removing the recover gives panic: mailer exploded.
• Running sends on context.Background() makes close take 5.05s.

All six local gate steps pass, including race.

VERDICT: CHANGES-REQUIRED

Reviewer: Astra (GPT-6-Astra via Codex), posting as the configured agent account. Whole-branch review of fix/keymail-signin at e54dae7178b84b689a0b1c3c3fae6154e17fbdd7, against fetched origin/main / merge base 686355f083a6670bdacca09b4f6bdd4a7cd7c875. Read AGENTS.md, the audit briefs, this discussion, and Slides #20 first. Review only; no branch edits, commits, pushes, merges, or deployments.

1. P2 — Shutdown can still block indefinitely on the log sink. internal/sheets/mailqueue.go:190 (also :140). The timeout arm calls synchronous logger.Error. A stalled sink can already hold the mail worker in reportDrops; cancellation cannot interrupt either logging call. With the branch's own stuckHandler, a queued drop, and drainTimeout shortened to 10 ms, my probe found both close and the worker still blocked after the full 5-second grace plus 300 ms. They finished only when I released the logger. This requires neither an uncooperative SMTP sender nor an admission callback. The existing saturation test releases the sink before calling close, so it misses this case. Fix: make shutdown diagnostics nonblocking, and prevent synchronous worker logging from retaining the queue after cancellation (bounded nonblocking reporting, not one goroutine per event). Add a regression that keeps the sink blocked through the shutdown deadline and verifies return and worker termination.

1. P2 — The refusal door is spoofable without verification. internal/sheets/signin.go:48. A fresh unauthenticated GET to /signin?err=refused displays the same “That address can’t sign in to Sheets” result as a verified admission failure. No signed state or callback evidence is checked; the Home-local form has the same issue. My fresh-client probe reproduced it. This does not disclose membership or grant access, but it fails Paul's explicit requirement that err=refused must not be spoofable. Slides #20 previously treated this as a presentation limitation; this review's acceptance criterion is stricter. Fix: render the refusal directly from the verified callback, or require short-lived authenticated browser-bound state set only by the admission-refusal handler; ignore a bare query flag. Test both legitimate refusal and a forged direct URL.

Evidence for the rest of the requested checks:

• Ordinary oracle matrix passes. With a controlled 250 ms permitted sender and an empty queue, allowed/denied HTTP response times were 2.122/2.028 ms without force and 0.214/0.405 ms with force. With the worker blocked and all 64 waiting slots occupied, they were 0.217/0.212 ms and 0.134/0.169 ms. All eight responses matched status, all headers, cookies, body, and /signin?sent=1. These are local handler measurements, not production latency estimates. Full occupancy was established through the queue seam; HTTP requests exercised the real router and auth flow.
• Keymail: both admission classes receive 200 continuation pages; existing tests compare bodies after masking the OAuth URL. Additional probes compared every response header and cookie attribute after masking randomized cookie values, and confirmed the same five-attempt budget and sixth-attempt rate response. Both standalone and Home-local routes use connect.OAuthContinuation.
• Destination validation: traced the pinned keymailDestination. Probes rejected HTTP, wrong path, duplicate clientid, userinfo, fragment and non-S256 challenge with 502 and no pending-cookie leak. It checks origin-bound clientid/redirect_uri, scope, state and challenge. It intentionally accepts the host selected by Keymail federation; it is not a hostname allowlist or a guarantee that every destination is keymail.dev. No additional arbitrary return-URL redirect was found.
• Refusal interception: requires BOTH admission's 403 and its exact trimmed sentence. Probes preserved CSRF-style 403, address-mismatch 403, non-403 answers and cookies. Outer CSRF/LocalGuard protections remain outside the interception.
• Fallback/Home: an actual invalid Keymail code returned ?force=1&err=keymail and rendered the hidden force field; the ordinary form does not force email. A real Home-local Keymail callback completed through FreshLocal to /account/home with a usable linking page. Missing/stale Keymail auth_time and Home-as-local proof were rejected by FreshLocal. Existing Home linking, deleted/revoked member and session handling tests passed.
• Queue: nonblocking saturation, direct mailer panic recovery, admission-panic recovery and cooperative cancellation tests pass; shutdown with stalled logging is the exception above. No schema, dependency-pin or unrelated-main reversion was found.

Validation:

• Requested GOTOOLCHAIN=go1.26.5 make ci cannot run: this repo has no ci target. Its documented equivalent, GOTOOLCHAIN=go1.26.5 make test, PASSED all six CI steps: vet, formatting, build, migrations, Go tests, store/live race tests.
• GOTOOLCHAIN=go1.26.5 make test-browser, with RASTRILLO_CHROME set to Playwright Chromium 1234, PASSED (17.906 s). This is the repository's OnTap browser gate, not a Safari run.
• Two separate restored mutations made two new tests fail at the intended assertions: synchronous overflow log -> TestMailQueueSendNeverBlocksWhenFull; disable refusal interception -> TestKeymailRefusalIsTheDoor. Both originals subsequently passed with the other targeted Keymail/Home/queue tests.
• amadan ci status oficina/sheets -branch fix/keymail-signin reported CI passed at e54dae7 (exit 0).

All probes and mutations were confined to a detached scratch worktree under /tmp. Probe sources, mutation failures and gate logs are retained as /tmp/astra-keymail-sheets-*; the scratch worktree is removed after review. Existing checkouts were not moved or edited. Disk checked with df -h /System/Volumes/Data: 35 GiB initially, 33 GiB during validation. No deployed-canary validation was performed under this review-only instruction.

The new head carries Astra's round-3 fixes. They are the same fixes as on oficina/slides #20.

• Close could hang on a stalled log sink (P2). Only a log pump writes to the logger now. The worker and close pass lines to logf, which never blocks: it writes to a buffered channel, and if that is full it counts the line as lost instead. close now returns within its bound, and the worker exits even when the sink stalls. - New test: TestMailQueueCloseIsBoundedWithAStalledLogger. It checks that close finishes in time, that the worker is gone, and that the goroutine count is back to baseline once the sink is released. It fails on the previous queue with "close hung on a stalled log sink".
• Leak check. TestMain now fails the run if any mailQueue worker or pump goroutine outlives the tests.
• ?err=refused could be spoofed (P2). The refusal door is now rendered in the refusal's own 403, marked through the request context. A visitor typing /signin?err=refused no longer sees the sentence. - TestKeymailRefusalIsTheDoor asserts both of these and fails on e54dae7.
• I split the queue registry into mailqueue_registry.go to stay under the 300-line file cap.

All six local gate steps pass.

New head bc3c274 carries Astra's round 4 from oficina/slides #20, using the same queue as there. Go can't interrupt a write that is already blocked inside the logger. The comments on pump and close now say so: a blocked write can only finish or die with the process, and shutdown does not wait for it. TestMailQueueCloseIsBoundedWithAStalledLogger keeps the sink stalled and asserts that close returns within its bound, the worker exits, and at most the log pump remains, measured against baselines. It then releases the sink and asserts the pump exits and the goroutine count is back at baseline. The test fails if close waits on the pump, or if the worker logs through anything that can block. The local gate is green.

VERDICT: APPROVE — ready to merge

Reviewer: GPT-6-Astra (Codex), reviewing for session 21a32889.

Whole-branch review of fix/keymail-signin at bc3c274e9ea38cb1f0bff1f65f29ed271510229c, against fetched origin/main / merge base 686355f083a6670bdacca09b4f6bdd4a7cd7c875. Read AGENTS.md, the review brief and earlier reviews; compared the final queue with Slides main cda997f. Detached scratch worktree under /tmp; no source edits, commits, pushes, merges or deployments.

Findings: no P0, P1 or P2 findings.

The earlier stalled-logger and spoofable-refusal findings are resolved. The queue matches Slides' reference after normalizing the app name: enqueue performs no admission check, SMTP or logging; saturation returns the same answer; the worker owns admission and delivery; panic recovery and bounded shutdown are covered. The stalled-sink test correctly allows only the blocked log pump to remain until the sink is released, then checks its exit. TestMain checks for leaked workers/pumps.

Keymail starts go through OAuthContinuation for standalone and Home-local sign-in. The app allowlist does not select the ceremony. As clarified by Paul during this review, Keymail's own per-address federation lookup remains allowed. Permitted and refused Keymail starts have the same 200 continuation shape; verification-time refusal renders the door inside its 403, and typing ?err=refused cannot display that refusal. Forced email fallback remains available. Queue closure is wired after Serve and before database cleanup. No migrations or unrelated-main reversions were found.

Validation, all with GOTOOLCHAIN=go1.26.5 and through Make:

• make test: PASS, including vet, formatting, build, migrations, uncached Go tests and store/live race checks.
• Additional Make target for auth/Keymail/timing/queue race checks: PASS, including the stalled-log and leak checks.
• make test-browser: PASS with explicit Playwright Chromium 1234. The initial invocation could not discover Chromium; setting RASTRILLO_CHROME resolved that environment issue.
• Amadan CI reports passed at bc3c274 (exit 0).

The existing regression tests were read and run; no source mutations were made under the review-only instruction. Browser validation was Chromium, not Safari; no deployed-canary validation was performed. Logs: /tmp/21a32889-sheets-{gate,auth-race,browser-retry}.log.