amadan
fix: nextExpiry covers sessions, retry deadline on error, test fixtures
Codex round 3 (pinned to 9e067e4, before c5981ca's auth_links fix) had C3-1 as its blocker. c5981ca closed the auth_links half; this closes the rest: - store.nextExpiry now also reads MIN(expires_at) over rastrillo's sessions table (same RFC3339 text shape as auth_links), since auth.Sweep cleans both in one call. nextDue returns a one-hour retry deadline instead of the zero time on a query error, so a transient read failure delays the wake rather than cancelling it outright. - TestNextExpiryIsTheEarliestExpiryNotTheLatestRow rewritten with three rows (expected minimum in the middle) so neither an ascending nor a descending id sort can pass it by accident (C3-3). - meettest's App(context.Background(), ...) call sites now use a new testCtx(t) helper (WithCancel + t.Cleanup) so no hourly sweep ticker outlives its test. The expired non-member /continue subtest asserts pendingJoins(t) equals 1 before advancing the clock, since the same sent=1 redirect is also the answer to a request that wrote nothing. Not fixed: auth.Sweep itself is context-free in rastrillo (auth/store.go:117), with no seam to bound from Meet's side - noted in the findings record rather than solved.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
9 files changed,
+203
−40
docs/superpowers/reviews/2026-09-09-codex-code-r1.md+98 −0internal/meet/app.go+17 −7internal/meet/store.go+35 −22internal/meet/store_test.go+16 −5internal/meettest/config_test.go+14 −3internal/meettest/harness_test.go+14 −0internal/meettest/join_test.go+7 −0internal/meettest/schema_test.go+1 −2internal/meettest/signin_test.go+1 −1
CI passed — run details
diff --git a/docs/superpowers/reviews/2026-09-09-codex-code-r1.md b/docs/superpowers/reviews/2026-09-09-codex-code-r1.md| index f371613..9a43526 100644 |
| --- a/docs/superpowers/reviews/2026-09-09-codex-code-r1.md |
| +++ b/docs/superpowers/reviews/2026-09-09-codex-code-r1.md |
| @@ -270,3 +270,101 @@ logic. This is the same position round 1 took and round 2 flagged as |
| still open; it is being named again rather than resolved, because doing |
| it properly needs a harness for deterministic interleaving (e.g. hooks |
| around the transaction, or a mock DB) that does not exist yet. |
| + |
| +## Round 3 |
| + |
| +Reviewer: GPT-6 (Codex), read-only sandbox. Pinned to `9e067e4` (round |
| +2's fix commit, before `c5981ca` and the further commit this section |
| +describes). Verdict: request changes. Verbatim below. |
| + |
| +### The review, verbatim |
| + |
| +**Verdict: request changes.** Reviewed `9e067e41c2586cfbee41de42cffa43044f7c45fe`. No files modified; no network used. Tests were inspected, not executed in the read-only sandbox. |
| + |
| +| Finding | Status | Code assessment | |
| +|---|---|---| |
| +| C2-1 | **Closed** | `mailredact.go:87` logs only the root cause’s type and `address_seen`; the returned error is fixed text without an unwrap chain. `config.go:116` wraps configured mailers, including the development fallback. | |
| +| C2-2 | **Half-closed** | Main correctly assigns `opts.NextDue = rt.NextDue`. The callback overlooks auth links and sessions, so cleanup can still remain unscheduled: C3-1. | |
| +| C2-3 | **Half-closed** | Boot cleanup is synchronous; `NewForTest` starts no ticker; pending deletion is limited to 1000 rows per call. The auth cleanup remains unbounded and uncancellable, and some tests still create immortal tickers: C3-2. | |
| +| C2-4 | **Half-closed** | Production mail logging, auth-link deletion, cross-meeting stale continuation, expired continuation and primary-file checks now have coverage. Slow/failed person-query log coverage remains absent. Some new assertions are narrower than claimed; see below. | |
| + |
| +C1-1 remains explicitly deferred. The declined transaction-interleaving and concurrent-budget tests remain recorded as uncovered; their absence does not independently block this verdict. |
| + |
| +**C3-1 — P2 — The wake callback reports “nothing scheduled” while auth records need cleanup.** |
| + |
| +Location: [internal/meet/app.go:249](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/app.go:249), backed by `internal/meet/store.go:397`. |
| + |
| +`nextExpiry` examines only `pending_joins`. A member requesting sign-in without a join cookie reaches auth without writing a pending row (`auth.go:83`). Signin writes an `auth_links` row before attempting delivery, but `NextDue` returns zero. If the app hibernates before expiry, that unclicked link has no scheduled cleanup wake. |
| + |
| +The new `TestSweepClearsUnclickedMagicLinks` constructs precisely this situation, but invokes cleanup manually rather than checking its scheduling. |
| + |
| +This also affects outsider requests: multiple magic links can exist for one address, while consuming one continuation deletes every pending row for that address. Remaining unclicked auth links then cease to have a wake deadline. Sessions are likewise absent from the calculation. |
| + |
| +Rastrillo’s contract and implementation agree: zero means no schedule (`serve.go:122`, `:554`). **Returning a past pending expiry is correct:** the framework preserves its Unix timestamp, and the locally inspected platform selects positive deadlines at or before now. The defect is the incomplete calculation of “nothing scheduled.” Returning zero on a query error (`app.go:247`) also abandons the cleanup retry. |
| + |
| +Correction: include all records that cleanup owns in the deadline calculation, or maintain a bounded recurring cleanup deadline independently of pending rows. Use a finite retry deadline on lookup failure. Test auth-only state, remaining links after pending consumption, empty state and overdue work through `Runtime.NextDue`. |
| + |
| +**C3-2 — P3 — The timeout does not bound the whole sweep, and teardown remains incomplete.** |
| + |
| +Location: [internal/meet/app.go:84](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/app.go:84), also `:109`, `:187`; `internal/meettest/signin_test.go:90`. |
| + |
| +The 30-second context reaches `store.sweep`, but `auth.Sweep(now)` receives no context. Rastrillo’s `auth/store.go:119` and `sessions/sessions.go:447` use context-free `DB.Exec` and delete all matching rows. A wait for the writer connection therefore ignores Meet’s timeout. Cancellation during an hourly pass also cannot interrupt this part. |
| + |
| +The 1000-row pending limit is correctly implemented. It does not bound auth deletion or the hourly pass’s total duration. |
| + |
| +Moving ticker creation out of `build` fixes the normal harness leak. However, the new production-mail test and the successful development/schema tests still call `App(context.Background(), …)` and close the database without cancelling the ticker. There is also no completion handle for callers to await before closing the database. |
| + |
| +Correction: make auth cleanup context-aware and bounded; provide cancellation plus completion waiting. Use cancellable contexts in tests that intentionally exercise `App`. Add deadline and shutdown tests. |
| + |
| +**C3-3 — P3 — The expiry-ordering test also passes when selecting the latest row.** |
| + |
| +Location: [internal/meet/store_test.go:311](/home/paulca/amadan.net/oficina/meet/.claude/worktrees/slice-1/internal/meet/store_test.go:311). |
| + |
| +The earliest-expiring row is deliberately inserted last. Consequently, replacing `Order("expires_at ASC")` with `Order("id DESC")` still returns the expected row. The test does not establish its named distinction, “earliest expiry, not latest row.” |
| + |
| +Correction: include a later-inserted row whose expiry is later than the expected minimum. A three-row fixture can distinguish expiry ordering from both ascending and descending ID ordering. |
| + |
| +The new and substantively changed tests establish the following: |
| + |
| +| Test | Does it prove its claim? | |
| +|---|---| |
| +| `TestMailErrorsCarryNoAddress` | **Yes, for its fixture.** Checks fixed returned text, absence of the local part, root type and `address_seen=true`. | |
| +| `TestNoFragmentOfAnAddressReachesTheLog` | **Partly.** Checks selected fragments of both fixture addresses. It does not reproduce C2-1’s Kelvin-prefix offset trap: its `K`s are ASCII, and dotted I occurs after the matched recipient. The current implementation is nevertheless safe by inspection. | |
| +| `TestAddressSeenIsCaseInsensitiveAndNeedsNoSlicing` | **Yes for the boolean cases.** “Needs no slicing” is established by inspecting the implementation, not these assertions. | |
| +| `TestTheWrapperForwardsDeliversAndSendList` | **Partly.** Checks interface preservation, the logged sender’s false delivery status, successful list invocation and sanitized list failure. It does not verify forwarding the actual message fields or context. | |
| +| `TestSigninOverAFailingMailerLeavesNoAddressInTheLog` | **Yes for the complete fixture address through the app path.** Removing the wrapping would expose it and fail the test. Contrary to the disposition, it does not assert absence of address fragments or exercise person-query logging. | |
| +| `TestConsumingOneMeetingsPendingRowLeavesAnothersIDStale` | **Yes for sequential stale rejection.** Directly checks B’s grant, A’s empty destination and absence of A’s grant. It does not directly assert deletion of all pending rows; setup errors are also discarded. | |
| +| `TestNextExpiryIsTheEarliestExpiryNotTheLatestRow` | **No for the ordering distinction:** C3-3. The empty-table zero assertion is sound. It does not exercise `Runtime.NextDue`. | |
| +| `TestSweepClearsUnclickedMagicLinks` | **Yes for explicit auth-link cleanup.** Direct row counts establish preservation at +11 minutes and deletion at +16. Removing `auth.Sweep` would fail it. Scheduling, restart, sessions and execution bounds remain untested. | |
| +| Expired non-member `/continue` subtest | **Substantially.** Exercises an existing guest session, expired pending lookup, `/welcome`, denied B access and retained A access. It should assert the pending row exists before advancing time: `sent=1` alone also represents refusal. | |
| +| `TestTheDatabaseNeverHoldsAPlaintextTitle` | **Yes for its creation fixture.** Missing primary files now fail; only missing optional sidecars are tolerated. Scans run before and after checkpoint. | |
| +| `TestAReplacedLinkMintsNoGrant` | **Improved.** The negative grant lookup now fails on database errors. Sequential scope is unchanged. | |
| +| `TestSharedClientAllowanceIsExhaustedByOneClient` | **Yes.** The renamed test accurately describes 21 different addresses exhausting one client’s allowance. | |
| + |
| +`TestASuccessfulSendIsUntouched` still checks only successful return, silence and nil preservation—not argument forwarding. Existing small sweep fixtures establish expiry filtering, but cannot establish the new 1000-row limit or multi-page loop. |
| + |
| +The remaining deployment blocker is C3-1: cleanup scheduling still misses the auth records it was introduced to remove. |
| + |
| +### Dispositions, C3-1 to C3-3 |
| + |
| +| Finding | Disposition | |
| +|---|---| |
| +| C3-1 — `NextDue` blind to `auth_links` and `sessions`; zero on query error | **Closed**, in two commits. `c5981ca` first closed the `auth_links` half: `store.nextExpiry` took the earlier of `pending_joins` and a new `MIN(expires_at)` read over `auth_links`. This section's own commit finished it: `nextExpiry` now also reads `MIN(expires_at)` over rastrillo's `sessions` table (same RFC3339-text shape, confirmed in `sessions/migrations/0001_init.sql` and `sessions/sessions.go:157` — `auth.Sweep` cleans both `auth_links` and `sessions` in one call, `auth/store.go:117` delegating to `sessions.Sweep`, so both need the same wake guarantee), and takes the earliest of all three sources. `app.nextDue` no longer returns the zero time on a query error: it now answers `a.store.now().Add(sweepEvery)` — the same one-hour retry deadline the healthy "nothing due sooner than the next sweep" path already uses — and still logs the error. `TestNextExpiryAlsoReadsAuthLinks` (round 2) and this round's fixes are covered by `TestNextDueWakesForAPlainMemberSignin` (`internal/meettest/signin_test.go`), which drives a real plain `POST /signin` with no pending row and checks `TestHandle.NextDue()` (new, forwarding to `a.nextDue()`) directly. Not added: a dedicated unit test for the query-error retry-deadline branch itself (would need a store whose `nextExpiry` can be made to fail on demand, which the current store has no seam for) and a `sessions`-only scenario through the full HTTP path (session rows come from `sessions.SignIn`, which every signed-in request already exercises, so it is covered incidentally by every other test but not asserted on directly). | |
| +| C3-2 — `auth.Sweep` is context-free; some tests still leak an immortal ticker | **Partial**, as the coordinator's brief for this round anticipated. `auth.Sweep(now)` (rastrillo's own method, `auth/store.go:117`) uses `a.cfg.DB.Exec` with no context parameter at all — there is no seam in rastrillo to bound it with, short of forking or wrapping the whole `*rauth.Auth`, which is out of scope for a Meet-side fix. This is noted here rather than solved. What *is* fixed: the tests C3-2 named — `internal/meettest/signin_test.go`'s production-mail test, `config_test.go`'s three `meet.App` call sites, and `schema_test.go`'s one — now build their context with a new `testCtx(t)` helper (`context.WithCancel` plus `t.Cleanup(cancel)`), so `App`'s hourly ticker is cancelled when the test ends rather than continuing to poll a database the test is about to close. `harness_test.go`'s own `newHarness` already used `meet.NewForTest`, which starts no ticker at all (round 2), so it needed no change. | |
| +| C3-3 — the expiry-ordering test also passes under `Order("id DESC")` | **Closed.** `TestNextExpiryIsTheEarliestExpiryNotTheLatestRow` now writes three rows — first-inserted expiring at +20m, second-inserted (the expected minimum) at +5m, third/last-inserted at +30m — so neither an ascending nor a descending id sort would find the right answer; only sorting on `expires_at` does. | |
| + |
| +Also addressed from the test-assessment table, though not itself a |
| +lettered finding: the expired non-member `/continue` subtest |
| +(`internal/meettest/join_test.go`) now asserts `pendingJoins(t) == 1` |
| +right after the outsider's second `POST /signin` and before advancing |
| +the clock, closing the gap the table named — "?sent=1" alone is also |
| +the answer to a request that wrote nothing at all (spec §3.2's |
| +same-answer rule), so the row's existence needed a direct assertion, |
| +not an inference from the redirect. |
| + |
| +Not addressed, named rather than claimed: `auth.Sweep`'s own |
| +unboundedness (the context-free half of C3-2, rastrillo's surface, not |
| +Meet's, per above); a unit test for `nextDue`'s query-error retry |
| +branch; and every item C2-4/C1-8 already recorded as uncovered |
| +(transaction interleavings, concurrent budget consumption, `auth.Sweep` |
| +execution-time bounds, `Runtime.NextDue`'s platform-side poll route). |
diff --git a/internal/meet/app.go b/internal/meet/app.go| index 90fb8cd..5419246 100644 |
| --- a/internal/meet/app.go |
| +++ b/internal/meet/app.go |
| @@ -240,23 +240,33 @@ type Runtime struct { |
| } |
| // nextDue is Runtime.NextDue's implementation: the earliest of the next |
| -// pending_joins.expires_at and one hour from now — the hourly sweep's |
| -// own interval, so the platform never lets the app sleep past a sweep |
| -// it would otherwise have run — or the zero time when no pending row |
| -// exists at all, meaning there is nothing to wake early for. |
| +// nextExpiry (across pending_joins, auth_links and sessions) and one |
| +// hour from now — the hourly sweep's own interval, so the platform |
| +// never lets the app sleep past a sweep it would otherwise have run — |
| +// or the zero time when nothing at all is outstanding, meaning there is |
| +// nothing to wake early for. |
| +// |
| +// A read failure does NOT answer the zero time: zero means "nothing |
| +// scheduled" to the platform (serve.go's contract), so returning it on |
| +// a transient error would cancel the wake entirely rather than merely |
| +// delay it. An hour out is the same retry deadline the healthy path |
| +// already uses for "nothing due sooner than the next sweep," so a |
| +// flaky read gets one more try at the next poll instead of going quiet |
| +// (Codex round 3, C3-1). |
| func (a *app) nextDue() time.Time { |
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
| defer cancel() |
| + retry := a.store.now().Add(sweepEvery) |
| exp, err := a.store.nextExpiry(ctx) |
| if err != nil { |
| a.logger.Error("meet: next due: read next expiry", "err", err) |
| - return time.Time{} |
| + return retry |
| } |
| if exp.IsZero() { |
| return time.Time{} |
| } |
| - if hourly := a.store.now().Add(sweepEvery); hourly.Before(exp) { |
| - return hourly |
| + if retry.Before(exp) { |
| + return retry |
| } |
| return exp |
| } |
diff --git a/internal/meet/store.go b/internal/meet/store.go| index b1b27e6..f6833a8 100644 |
| --- a/internal/meet/store.go |
| +++ b/internal/meet/store.go |
| @@ -392,10 +392,10 @@ func (s *store) sweep(ctx context.Context, now time.Time) (int64, error) { |
| return res.RowsAffected, res.Error |
| } |
| -// nextExpiry answers the earliest expires_at across pending_joins AND |
| -// rastrillo's own auth_links, or the zero time when both are empty. It |
| -// is the platform wake-up input for cmd/meet/main.go's Options.NextDue |
| -// (review C2-2). |
| +// nextExpiry answers the earliest expires_at across pending_joins, |
| +// rastrillo's own auth_links, and rastrillo's own sessions, or the zero |
| +// time when all three are empty. It is the platform wake-up input for |
| +// cmd/meet/main.go's Options.NextDue (review C2-2). |
| // |
| // auth_links matters here too: a plain member sign-in writes no pending |
| // row at all (that table exists for the outsider/pending-join path, |
| @@ -404,6 +404,12 @@ func (s *store) sweep(ctx context.Context, now time.Time) (int64, error) { |
| // but one unclicked member link would never be woken to sweep it — the |
| // platform sees a zero NextDue and assumes there is nothing to do |
| // (round 2 follow-up, caught on re-review of 9e067e4). |
| +// |
| +// sessions is here for the same reason, one level further out: |
| +// auth.Sweep cleans auth_links AND sessions in one call |
| +// (auth/store.go:117 delegates to sessions.Sweep), so a session sitting |
| +// alone with nothing else outstanding needs the same wake-up guarantee |
| +// an unclicked link does (Codex round 3, C3-1). |
| func (s *store) nextExpiry(ctx context.Context) (time.Time, error) { |
| var p PendingJoin |
| err := s.g.WithContext(ctx).Order("expires_at ASC").Take(&p).Error |
| @@ -417,35 +423,42 @@ func (s *store) nextExpiry(ctx context.Context) (time.Time, error) { |
| pending = p.ExpiresAt |
| } |
| - authLink, err := s.nextAuthLinkExpiry(ctx) |
| + authLink, err := s.rastrilloTextExpiry(ctx, "auth_links") |
| + if err != nil { |
| + return time.Time{}, err |
| + } |
| + session, err := s.rastrilloTextExpiry(ctx, "sessions") |
| if err != nil { |
| return time.Time{}, err |
| } |
| - switch { |
| - case pending.IsZero(): |
| - return authLink, nil |
| - case authLink.IsZero(): |
| - return pending, nil |
| - case authLink.Before(pending): |
| - return authLink, nil |
| - default: |
| - return pending, nil |
| + next := time.Time{} |
| + for _, t := range []time.Time{pending, authLink, session} { |
| + if t.IsZero() { |
| + continue |
| + } |
| + if next.IsZero() || t.Before(next) { |
| + next = t |
| + } |
| } |
| + return next, nil |
| } |
| -// nextAuthLinkExpiry reads rastrillo's own auth_links table directly — |
| -// it is rastrillo/auth's table, not a store model, and rastrillo writes |
| -// to it over database/sql rather than gorm, storing expires_at as |
| -// RFC3339 text via now.UTC().Format(time.RFC3339) |
| -// (auth/store.go:83, auth/migrations/0001_init.sql). MIN() over that |
| +// rastrilloTextExpiry reads the earliest expires_at from one of |
| +// rastrillo's own tables — auth_links or sessions, the only two callers |
| +// pass, never anything caller-supplied. Both are rastrillo's tables, not |
| +// store models: rastrillo writes to them over database/sql rather than |
| +// gorm, storing expires_at as RFC3339 text via |
| +// now.UTC().Format(time.RFC3339) (auth/store.go:83,152, |
| +// sessions/sessions.go:157; see auth/migrations/0001_init.sql and |
| +// sessions/migrations/0001_init.sql for the column). MIN() over that |
| // text column sorts correctly because every row is formatted the same |
| // fixed-width, UTC way, so lexicographic and chronological order agree. |
| // The zero time means no row; an empty table's MIN() is a SQL NULL, not |
| // an empty string, hence sql.NullString rather than a bare string. |
| -func (s *store) nextAuthLinkExpiry(ctx context.Context) (time.Time, error) { |
| +func (s *store) rastrilloTextExpiry(ctx context.Context, table string) (time.Time, error) { |
| var raw sql.NullString |
| - if err := s.g.WithContext(ctx).Raw("SELECT MIN(expires_at) FROM auth_links").Row().Scan(&raw); err != nil { |
| + if err := s.g.WithContext(ctx).Raw("SELECT MIN(expires_at) FROM " + table).Row().Scan(&raw); err != nil { |
| return time.Time{}, err |
| } |
| if !raw.Valid || raw.String == "" { |
| @@ -453,7 +466,7 @@ func (s *store) nextAuthLinkExpiry(ctx context.Context) (time.Time, error) { |
| } |
| t, err := time.Parse(time.RFC3339, raw.String) |
| if err != nil { |
| - return time.Time{}, fmt.Errorf("meet: parse auth_links.expires_at: %w", err) |
| + return time.Time{}, fmt.Errorf("meet: parse %s.expires_at: %w", table, err) |
| } |
| return t, nil |
| } |
diff --git a/internal/meet/store_test.go b/internal/meet/store_test.go| index 776517c..e235894 100644 |
| --- a/internal/meet/store_test.go |
| +++ b/internal/meet/store_test.go |
| @@ -293,6 +293,14 @@ func TestConsumingOneMeetingsPendingRowLeavesAnothersIDStale(t *testing.T) { |
| // expires_at, deliberately not on id or created_at (which is what |
| // latestPending sorts on, the other way): the row written last is not |
| // necessarily the one that expires first. |
| +// |
| +// Codex round 3, C3-3: the first version of this test put the |
| +// earliest-expiring row last (only two rows), so it also passed under |
| +// Order("id DESC") — it never actually distinguished "sort by expiry" |
| +// from "sort by id, descending." Three rows here, with the expected |
| +// minimum in the middle: the first-written row and the last-written row |
| +// both expire later than it, so neither an ascending nor a descending |
| +// id sort would find it — only sorting on expires_at does. |
| func TestNextExpiryIsTheEarliestExpiryNotTheLatestRow(t *testing.T) { |
| s, now := testStore(t) |
| ctx := context.Background() |
| @@ -305,16 +313,19 @@ func TestNextExpiryIsTheEarliestExpiryNotTheLatestRow(t *testing.T) { |
| base := *now |
| *now = base.Add(20 * time.Minute) |
| - if err := s.writePending(ctx, "later@example.com", l); err != nil { |
| + if err := s.writePending(ctx, "first@example.com", l); err != nil { |
| t.Fatal(err) |
| } |
| - // Written last (the higher id), but its expiry is earlier than the |
| - // row above — the only way this test can fail is if nextExpiry |
| - // sorted on something other than expires_at. |
| + // The minimum: written second (neither the lowest nor the highest |
| + // id) and expiring soonest. |
| *now = base.Add(5 * time.Minute) |
| if err := s.writePending(ctx, "sooner@example.com", l); err != nil { |
| t.Fatal(err) |
| } |
| + *now = base.Add(30 * time.Minute) |
| + if err := s.writePending(ctx, "last@example.com", l); err != nil { |
| + t.Fatal(err) |
| + } |
| got, err := s.nextExpiry(ctx) |
| if err != nil { |
| @@ -322,7 +333,7 @@ func TestNextExpiryIsTheEarliestExpiryNotTheLatestRow(t *testing.T) { |
| } |
| want := base.Add(5 * time.Minute).Add(pendingLife).UTC() |
| if !got.Equal(want) { |
| - t.Fatalf("nextExpiry = %v, want %v (the sooner row's expiry)", got, want) |
| + t.Fatalf("nextExpiry = %v, want %v (the middle row's expiry, neither first nor last inserted)", got, want) |
| } |
| } |
diff --git a/internal/meettest/config_test.go b/internal/meettest/config_test.go| index 2a48a10..5339d83 100644 |
| --- a/internal/meettest/config_test.go |
| +++ b/internal/meettest/config_test.go |
| @@ -23,6 +23,17 @@ func openDB(t *testing.T) *db.DB { |
| return d |
| } |
| +// testCtx returns a context cancelled at the end of the test, so a |
| +// successful meet.App build's hourly sweep ticker does not outlive it |
| +// (Codex round 3, C3-2: App(context.Background(), ...) in a test leaves |
| +// a goroutine running against a database the test is about to close). |
| +func testCtx(t *testing.T) context.Context { |
| + t.Helper() |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + t.Cleanup(cancel) |
| + return ctx |
| +} |
| + |
| func TestProductionRefusesToStartWithoutSecrets(t *testing.T) { |
| // validDEK is a valid 32-byte base64 DEK, used in the "no mailer" |
| // and "no allowlist" cases so each refuses for its own named |
| @@ -36,20 +47,20 @@ func TestProductionRefusesToStartWithoutSecrets(t *testing.T) { |
| "no allowlist": {Env: "production", Origin: "https://x.test", InstanceKey: "k", DEK: validDEK, Mailer: nopMailer{}}, |
| } |
| for name, cfg := range cases { |
| - if _, _, err := meet.App(context.Background(), openDB(t), cfg); err == nil { |
| + if _, _, err := meet.App(testCtx(t), openDB(t), cfg); err == nil { |
| t.Errorf("%s: production started without it", name) |
| } |
| } |
| } |
| func TestDevelopmentStartsWithNothing(t *testing.T) { |
| - if _, _, err := meet.App(context.Background(), openDB(t), meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| + if _, _, err := meet.App(testCtx(t), openDB(t), meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| t.Fatalf("development refused: %v", err) |
| } |
| } |
| func TestBadDEKIsRefusedEvenInDevelopment(t *testing.T) { |
| - _, _, err := meet.App(context.Background(), openDB(t), meet.Config{Env: meet.EnvDevelopment, DEK: "not base64!"}) |
| + _, _, err := meet.App(testCtx(t), openDB(t), meet.Config{Env: meet.EnvDevelopment, DEK: "not base64!"}) |
| if err == nil || !strings.Contains(err.Error(), "base64") { |
| t.Fatalf("want a base64 refusal, got %v", err) |
| } |
diff --git a/internal/meettest/harness_test.go b/internal/meettest/harness_test.go| index f775a21..ecb8576 100644 |
| --- a/internal/meettest/harness_test.go |
| +++ b/internal/meettest/harness_test.go |
| @@ -68,6 +68,20 @@ func (x *harness) authLinks(t *testing.T) int { |
| return n |
| } |
| +// pendingJoins counts Meet's own pending_joins rows directly — used to |
| +// confirm a scenario actually wrote the row it means to test the expiry |
| +// of, rather than trusting a same-answer redirect alone: POST /signin's |
| +// "?sent=1" also covers a request that never wrote anything (Codex |
| +// round 3, C3-3's test-assessment table). |
| +func (x *harness) pendingJoins(t *testing.T) int { |
| + t.Helper() |
| + var n int |
| + if err := x.db.G.Raw("SELECT count(*) FROM pending_joins").Scan(&n).Error; err != nil { |
| + t.Fatal(err) |
| + } |
| + return n |
| +} |
| + |
| type recordingMailer struct { |
| mu sync.Mutex |
| to []string |
diff --git a/internal/meettest/join_test.go b/internal/meettest/join_test.go| index cba8471..e38d3f1 100644 |
| --- a/internal/meettest/join_test.go |
| +++ b/internal/meettest/join_test.go |
| @@ -319,6 +319,13 @@ func TestReplacedLinkFallbackForSignedInPerson(t *testing.T) { |
| if rec := other.post("/signin", url.Values{"address": {"guest@example.com"}}); rec.Header().Get("Location") != "/signin?sent=1" { |
| t.Fatalf("outsider's request: %s", rec.Header().Get("Location")) |
| } |
| + // "?sent=1" alone is also the answer to a refusal (spec §3.2's |
| + // same-answer rule) — confirm the pending row this subtest means |
| + // to expire was actually written, not just that the request was |
| + // accepted-looking (Codex round 3, C3-3). |
| + if n := x.pendingJoins(t); n != 1 { |
| + t.Fatalf("pending_joins rows after the outsider's second POST /signin: %d, want 1", n) |
| + } |
| // The link stays live; only the row runs out (fifteen minutes). |
| x.advance(16 * time.Minute) |
diff --git a/internal/meettest/schema_test.go b/internal/meettest/schema_test.go| index 8d1097a..33401f2 100644 |
| --- a/internal/meettest/schema_test.go |
| +++ b/internal/meettest/schema_test.go |
| @@ -1,7 +1,6 @@ |
| package meettest |
| import ( |
| - "context" |
| "testing" |
| "amadan.net/oficina/meet/internal/meet" |
| @@ -9,7 +8,7 @@ import ( |
| func TestBootCreatesEveryTable(t *testing.T) { |
| d := openDB(t) |
| - if _, _, err := meet.App(context.Background(), d, meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| + if _, _, err := meet.App(testCtx(t), d, meet.Config{Env: meet.EnvDevelopment}); err != nil { |
| t.Fatal(err) |
| } |
| for _, table := range []string{"persons", "meetings", "meeting_links", "guest_grants", "pending_joins", "sessions", "auth_links"} { |
diff --git a/internal/meettest/signin_test.go b/internal/meettest/signin_test.go| index 6a35cc9..e2e055f 100644 |
| --- a/internal/meettest/signin_test.go |
| +++ b/internal/meettest/signin_test.go |
| @@ -88,7 +88,7 @@ func TestSigninOverAFailingMailerLeavesNoAddressInTheLog(t *testing.T) { |
| t.Cleanup(func() { d.Close() }) |
| failing := failingMailer{err: fmt.Errorf("rastrillo/mail: send to %s: %w", address, errFakeSMTPFailure)} |
| - mux, _, err := meet.App(context.Background(), d, meet.Config{ |
| + mux, _, err := meet.App(testCtx(t), d, meet.Config{ |
| Env: meet.EnvDevelopment, Origin: testOrigin, AllowedAddresses: []string{address}, |
| Mailer: failing, Logger: logger, |
| }) |