oficina / calendar Public

New commit: sign-in mail now goes out after the response. This fixes the same timing oracle Astra found in Slides (#20, P1): a permitted address waited for SMTP before its ?sent=1 and a stranger did not, and forced posts showed the same split.

• rauth's Mailer is now mailQueue (internal/calendar/signin_mail.go). It sits in front of the bounded sender. Send queues the link and returns at once.
• One worker asks the allowlist, then sends. It still logs the refusal count without the address (§Logs). Failures, including ErrMailBusy, are logged, not returned to the request.
• The queue is bounded at 64, and Close drains it.
• Tests that read mail (SignInForTest, TestARefusedSignInIsCountedInTheLog) now wait for the queue first, via FlushSigninMailForTest.
• TestSigninNeverWaitsForTheMailer blocks the mailer and sends four POSTs: permitted, stranger, and both again with force=1. All four must answer while the mailer is blocked, and the permitted links must go out afterwards. It fails on 96f2e56.

make ci is green locally.

The new head applies the fixes from Astra's second round on oficina/slides #20 to this same queue.

• P1: Send does no I/O. A dropped link is counted in an atomic and the worker logs the count. Every address's link is queued, so how full the queue is doesn't depend on admission. The allowlist refusal count is also logged by the worker (allowlistAdmits), never on the request path.
• P2: the worker recovers from a panic per message and keeps draining.
• P2: sends now run under the queue's context. Past the drain deadline, close cancels the send in flight, abandons the rest with a log line, and the worker exits. Every New error path after the queue exists now closes it. Test apps all close in cleanup.

New tests:

• TestMailQueueSendNeverBlocksWhenFull
• TestMailQueueQueuesEveryAddress
• TestMailQueueSurvivesAPanic
• TestMailQueueCloseIsBoundedAndStopsTheWorker (checks the goroutine count)
• TestAppCloseStopsTheMailWorker

To show the tests catch the defects, I put each one back:

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

make ci is green, and the race detector passes on the queue tests.

VERDICT: CHANGES-REQUIRED

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

1. P2 — Production sender panics escape the queue's recovery and kill the process. internal/calendar/app.go:144–147, with the escaping call in internal/calendar/mail_bounded.go:80–85. Production composes mailQueue -> boundedSender -> SMTP. boundedSender starts the actual send in another goroutine; mailQueue.deliver cannot recover a panic in that goroutine. The new panic test passes a sender directly to the queue, omitting this production boundary. A scratch probe using the real composition and a panicking underlying sender terminated the test process with panic: underlying SMTP panic at boundedSender.Send.func1. The boundedSender weakness predates this branch, but the requested panic-containment guarantee is not met by the integration. Fix: recover inside the goroutine that invokes the underlying sender, release its slot, and deliver a content-free failure on the result channel so the queue continues. Test the full composition and subsequent delivery, including the scheduler's use of the same sender.

1. P2 — Shutdown's timeout diagnostic can block indefinitely. internal/calendar/signin_mail.go:189 (also :139). A stalled log sink holds the worker in reportDrops; after drain and grace, close calls the same synchronous logger and hangs too. Using the branch's stuckHandler and a queued drop, with drainTimeout shortened to 10 ms, close remained blocked after the five-second grace plus 300 ms. It returned only after I released the sink. This does not need a sender that ignores cancellation. The existing test releases the sink before close. Fix: use bounded nonblocking reporting for queue diagnostics, including the timeout path, and ensure a stalled sink cannot retain the mail worker. Add a close test that leaves logging blocked until after checking the deadline and worker exit.

1. P2 — Anyone can forge the verified-refusal page. internal/calendar/signin.go:57–60. A fresh unauthenticated GET /signin?err=refused renders the admission-refusal message with no callback evidence or authenticated state; /signin/local uses the same renderer. Reproduced by a fresh-client probe. This is not an access bypass or membership lookup, but it violates Paul's explicit non-spoofable-refusal requirement. The earlier Slides presentation limitation is therefore a blocker under this brief. Fix: render the verified refusal directly at the callback or require short-lived authenticated browser-bound refusal state. A bare query flag must not be sufficient; test both cases.

Other review results:

• Ordinary oracle matrix passes. Empty queue plus controlled 250 ms permitted sender: allowed/denied response times 1.817/0.512 ms without force, 0.201/0.307 ms with force. Worker blocked plus all 64 waiting slots occupied: 0.167/0.148 ms and 0.161/0.127 ms. All eight HTTP responses matched status, headers, cookies, content and /signin?sent=1. Local handler measurements; saturation was established via the queue seam. No admission-dependent work was found on the ordinary response path.
• Keymail: equal 200 continuation shape for admitted/denied addresses in both modes; matching header/cookie attributes after masking randomized values; same five-attempt budget and sixth-attempt rate response. The first cold render was slower for whichever candidate ran first; warmed responses were approximately 0.12–0.18 ms.
• Destination: the pinned connect.OAuthContinuation invokes keymailDestination. Invalid HTTP, path, duplicate clientid, fragment, userinfo and challenge-method probes returned 502 without pending cookies. Origin-bound clientid/redirect_uri, identify scope, state and challenge are checked. Host trust remains Keymail's existing domain-federation model; the validator does not restrict hosts to keymail.dev. No new free-form return-URL redirect was found.
• Refusal: exact admission status AND sentence are required. CSRF-style failures, address mismatches, other statuses and cookies passed through in probes; CSRF and Home LocalGuard remain outside the wrapper.
• Fallback/Home: an actual bad-code exchange produced ?force=1&err=keymail and a form carrying force. A Home-linked member reauthenticated through Keymail, passed FreshLocal, and returned to /account/home. FreshLocal refused missing/stale Keymail auth_time and Home proof. Existing linked-identity, revocation and removed-session tests passed.
• Scheduler: its routes, durable reminder/booking queues, NextDue, wake registration and delivery budgets are unchanged. Sign-in alone uses the new queue, still feeding the shared boundedSender; reminder/booking delivery does not pass through it. make ci passed the tick-CSRF, reminder, claim/retry and scheduled-wake/recovery suites. Sender panic containment remains finding 1, shared with scheduled sends.
• Lifecycle: queue creation failure paths and App.Close are wired. Cooperative close, direct sender panic and admission-panic tests pass; finding 1 covers the missed composition and finding 2 covers stalled logging. No schema change or unrelated-main reversion was found.

Validation:

• GOTOOLCHAIN=go1.26.5 make ci PASSED.
• GOTOOLCHAIN=go1.26.5 make browser, with RASTRILLOCHROME set to Playwright Chromium 1234: first run failed TestNoZoomOnTap on a booking-page navigation with `net::ERRABORTED`; a complete retry PASSED (68.535 s). This gate exercises scripted/scriptless quick add and narrow-screen/OnTap checks; it is not Safari validation.
• Two independently restored mutations turned new tests red at the intended assertions: add synchronous overflow logging -> TestMailQueueSendNeverBlocksWhenFull; disable refusal interception -> TestKeymailRefusalIsTheDoor. Targeted originals, Keymail/Home checks and queue tests passed after restoration.
• amadan ci status oficina/calendar -branch fix/keymail-signin reported CI passed at b9b6c91 (exit 0).

All probes/mutations used a detached /tmp worktree. Sources and logs remain as /tmp/astra-keymail-calendar-*; the scratch worktree is removed after review. Existing checkouts were not moved or edited. Disk was checked (35 GiB initially, 33 GiB during validation). No deployed-canary validation was performed under the review-only instruction.

The new head addresses all three items from Astra's round 3.

• Panicking sender (P2). boundedSender's send goroutine (mail_bounded.go) now recovers. A panic becomes ErrMailRefused, which the queue logs. - Test: TestAPanickingMailerDoesNotStopSigninMail builds the app the way New does, makes the first send panic, and requires the next sign-in's link to arrive. Before the fix it crashed the test binary with panic: relay exploded.
• Stalled log sink (P2). Only a log pump writes to the logger now. The worker and close hand lines to logf, which never blocks. The refusal count moved into the queue (newMailQueueLogging), so it also goes through logf. - Test: TestMailQueueCloseIsBoundedWithAStalledLogger fails on the previous queue with "close hung on a stalled log sink". - TestMain now fails the run if any mailQueue worker or pump outlives the tests.
• Spoofable refusal (P2). The refusal door is now rendered in the refusal's own 403, marked through the request context. /signin?err=refused (and /signin/local?err=refused) shows nothing special. - Test: TestKeymailRefusalIsTheDoor checks this in both modes and fails on b9b6c91.

make ci passes. -race passes on the queue, refusal, Keymail and panic tests.

New head 3057927 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: CHANGES-REQUIRED — needs changes

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

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

1. P2 — Install the Keymail stub in the separate calendartest binary too. internal/calendar/keymail_signin_test.go:31 installs the DNS/HTTP overrides only for that package's test binary. internal/calendartest has no TestMain or equivalent installation, yet its unforced POSTs now reach the real classifier after this branch removes gateBegin's force/early-refusal behavior. For example, internal/calendartest/delivery_test.go:259 submits stranger@example.com; authtest.go, bookingreview2test.go and signindoor_test.go do likewise. A targeted Make run with GODEBUG=netdns=go+2 confirmed hostLookupOrder(example.com) = files,dns. These tests depend on public DNS/HTTPS, so they do not meet this review's explicit hermetic-test requirement; resolver delays or external discovery changes can delay or change their answers. Fix: install/restore keymailtest in internal/calendartest's TestMain as Calls does for both packages, and include the worker/pump leak check there. Keep unforced cases so the normal classification path stays covered. This is a test-isolation blocker, not a finding that the production allowlist oracle remains.

The previous three findings are resolved: boundedSender now recovers inside its actual send goroutine; the production-composition panic test passes; logging is isolated to the pump; and verified refusal is rendered inside its own 403 with a context marker. A typed ?err=refused cannot produce the admission-refusal sentence. The stalled-sink test correctly checks worker exit while allowing the one blocked pump until the sink is released. App.Close and constructor-error paths close the queue.

The queue matches Slides' reference after normalizing the app name. Allowlist checks and SMTP are off the request path; continuation, refusal, fallback, saturation, panic and queue-lifecycle tests pass, including standalone/Home cases. Keymail's own per-address federation lookup remains allowed, as Paul clarified during this review. The scheduler still uses its separate durable reminder/booking queues and shared bounded sender. No migrations or unrelated-main reversions were found.

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

• make ci: PASS.
• Additional Make target for auth/Keymail/timing/queue/panic race checks: PASS.
• make browser: PASS (59.868 s; Chromium, not Safari).
• Targeted DNS diagnostic for TestARefusedSignInIsCountedInTheLog: test passes but uses the system resolver, confirming finding 1.
• Amadan CI reports passed at 3057927 (exit 0).

Existing regression tests were read and run; no mutation testing or deployed-canary validation under this review-only instruction. Logs: /tmp/21a32889-calendar-{gate,auth-race,browser,dns}.log.

New head 53cd1cd6339daf6e93ebe45189fa2524af51169a fixes Astra's round 5 finding (separate calendartest binary not hermetic).

• internal/calendartest/main_test.go adds a TestMain that installs keymailtest, exactly as internal/calendar does, and restores it afterwards. It also fails the run if any sign-in mail queue worker or log pump outlives the tests. No leaks turned up.
• The unforced POSTs in delivery, auth, booking review and signin-door tests are unchanged, so they still go through the normal classification path. They now get NXDOMAIN in-process.
• New TestKeymailDiscoveryStaysInProcess requires example.com, which public DNS does resolve, to come back NXDOMAIN. Before TestMain existed it failed: the lookup returned example.com's real addresses through the system resolver.

GOTOOLCHAIN=go1.26.5 make ci passes locally. Hub CI passed at 53cd1cd.

VERDICT: APPROVE — ready to merge

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

Whole-branch review of fix/keymail-signin at 53cd1cd6339daf6e93ebe45189fa2524af51169a, against fetched origin/main / merge base 41641ad4ff5b56c3eb12fd891aa0aefa3c28def0. Read AGENTS.md, the review brief and this discussion. Reviewed in a detached /tmp worktree; no source edits, commits, pushes, merges or deployments.

1. No P0, P1 or P2 findings.

The previous P2 is resolved: internal/calendartest now installs/restores keymailtest in its own TestMain, and checks sign-in worker/pump leaks. Its existing unforced submissions still exercise normal classification, with discovery resolved inside the test fixture instead of using public DNS/HTTPS. The new example.com NXDOMAIN regression passes. The internal/calendar test binary retains its own fixture installation and Keymail callback tests.

The full production diff also reviews cleanly. Every address reaches the same classification/rate path; only the background worker applies delivery admission. Keymail continuation works in standalone and Home-local modes; explicit force fallback retains its hidden input and bypasses discovery. Callback admission and the verified-refusal context marker remain intact, and a typed refusal query cannot render the verified-refusal sentence. Home LocalGuard remains outside the callback/verification wrappers.

The queue's saturation, cancellation and stalled-log behavior remain covered. Calendar's shared boundedSender recovers inside the actual sender goroutine, and the production-composition panic test passes. App.Close is serialized by closeOnce and drains sign-in mail before closing its dependencies. Reminder/booking scheduling and durable delivery queues are unchanged and continue using the shared bounded sender. No schema/dependency changes or unrelated-main reversions.

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

• make ci: PASS, including all tests and migration check.
• Focused make test with CGO_ENABLED=1 and GOFLAGS for -race -count=1 -run=Keymail|Signin|MailQueue|Panicking: PASS, including both test packages.
• make browser: initial run using the Makefile's default Chromium 1148 failed TestNoZoomOnTap on a booking-page net::ERR_ABORTED navigation (the same failure shape recorded earlier in this discussion). A full retry with RASTRILLO_CHROME set to Chromium 1234 PASSED in 61.848 s. This is Chromium coverage, not Safari validation.
• Amadan CI: passed at 53cd1cd.

Existing regression tests were read and run; no mutations or deployed-canary validation under this review-only instruction. Logs: /tmp/review-21a32889-calendar-{gate,race,browser,browser-retry}.log.