amadan
fix: decks review round 1 — settings row and document commit together
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01563XGCgxcSZToEXwuTBsKE
cmd/slides/main.go+5 −1internal/slides/app.go+13 −5internal/slides/decks.go+32 −19internal/slides/decks_test.go+79 −0internal/slides/handlers.go+19 −0internal/slides/sample.go+1 −1internal/slidestest/browser_test.go+2 −1internal/slidestest/harness_test.go+4 −1internal/slidestest/signin_test.go+4 −2
Intent
<task-notification> <task-id>aa9b6322b4f4b353d</task-id> <tool-use-id>toolu_01RhQxzjcHAt8NmS9nkTgZ9M</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/aa9b6322b4f4b353d.output</output-file> <status>completed</status> <summary>Agent "Implement Task 3: decks" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>**Status:** Done. `make ci` green, nothing pushed. **Commit:** `cf0b7d5` — feat: decks with persistence, list, settings and a sample deck **Tests:** 5 new slidestest cases + 3 in `internal/slides/deck` + 2 assertions added to `auth_test.go`; all pass, whole suite green (vet, gofmt, tests, `migration check`). **Concerns:** - The deck list opens one wasm module per deck to count slides — deliberate (no denormalised count column per the brief's model), fine at small deck counts, and task 6's hub is the fix. - No snapshots yet: `openDeck` replays the whole log; trigger noted in its doc comment. - `saveDeckSettings` writes the document then the row in two transactions — a crash between leaves the list stale (the better half of the trade, but a window). - Deviations from the brief, all documented in code: `openDeck`/`applyOps` take the `Deck` (and `applyOps` the open handle) because the Y.Doc guid is the sqid; the sample is eight slides with `blank` riding on the two canvas slides (seven templates plus two canvas would be nine); `deck.Deck` gained `Meta`/`Fonts`/`Asset` from the core's view, which is the contract; migration is named `0002_create_decks.sql` by the generator. - A parallel agent committed to `core/` in this same worktree while I worked; I staged my files by name, nothing of theirs is in my commit. **Report:** `/home/paulca/amadan.net/oficina/slides/.claude/worktrees/slice-1/.superpowers/sdd/2026-09-07-slice-1-live/task-3-report.md`</result> <usage><subagent_tokens>253995</subagent_tokens><tool_uses>120</tool_uses><duration_ms>1214459</duration_ms></usage> </task-notification>
<task-notification> <task-id>a37efb2cc0c49bb94</task-id> <tool-use-id>toolu_018mZvTgwoo6fhwrNWkEdpTF</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina/2420c44b-a27a-4e0d-bbe7-8b3ca0464c21/tasks/a37efb2cc0c49bb94.output</output-file> <status>completed</status> <summary>Agent "Review Task 3 (spec + quality)" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>### Verdict: **Approved** Task 3 delivers decks persistence, list/create/settings/delete screens, and the sample deck, all matching the brief's interfaces and the controller decisions given for this review (sqid: 10 chars over the given alphabet, crypto/rand, 5 retries; slide/block ids: 12 chars over the given base32 alphabet; Y.Doc guid = sqid; server client id 1; one `*core.Runtime` on `app`; `deck.Deck` gaining `Meta`/`Fonts`/`Asset` correctly mirrors `core/src/view.rs`, which I read directly and confirmed field-for-field). Tests exercise real behaviour, not just status codes. ### Spec Compliance - §2 model: `deck.Deck`/`Slide`/`Block`/`Text`/`Run`/`Box`/`PhoneOverride` match `core/src/view.rs`'s emitted JSON exactly (`internal/slides/deck/deck.go:1-179`, verified against `core/src/view.rs:34-96`). The op vocabulary used in `decks.go`/`sample.go` (`set_meta`, `add_slide`, `add_block`, `set_notes`, `set_slide`, `set_phone`) matches `core/src/ops.rs`'s `Op` enum field-for-field, including `markdown` on `AddBlock`. - §5 rungs: every deck starts `private` (`decks.go:1050` region, `createDeckRow`); `visibleDecks` (`decks.go` "the visibility seam") ORs owner, team, and grant — `granted` is honoured even though unreachable via UI, as the brief allows. - §6.1/6.2: `sessions.Current(r).Subject` (email) mapped via `currentUser`→`ensureUser`, never `UserID`; every write's actor is `person:<id>` via `actorFor` (`decks.go`). - Global seam constraint: not-found and not-yours both 404 via `deckFor`/`ownedDeckFor` sharing `a.notFound`; writes require owner (`ownedDeckFor`), confirmed by `TestForeignPrivateDeckIs404` and the settings/delete "stranger→404" assertions in `TestTeamDeckVisibleToOthers`/`TestRenameAndDelete`. - csrf/session wiring: `csrf.Protect(a.cfg.Origin)` wraps the whole router (`app.go:39`); every new mutating route is POST inside the `RequireSession` group (`app.go:57-72`). - Transaction correctness (checked against rastrillo §3, the named risk): `applyOps` and `deleteDeck` both use the callback's `tx`, never `a.db.G`/`d.G`, inside `Transaction(...)` (`decks.go`). `h.JSON` was correctly moved outside the write transaction (self-reported fix, confirmed present in the diff). - `class="rst-mono"` in `deck.html` is **not** a violation: tokens.css explicitly documents utilities (`rst-mono` among them) as "keep the class spelling and are not paired" — the bare-attribute rule applies to component kinds, not utilities (`internal/slides/static/tokens.css:43-45`). - Deviations (openDeck/applyOps taking `Deck`/`*Deck`+handle instead of `deckID`; eight-slide sample with `blank` on both canvas slides; `deck.Deck` gaining `Meta`/`Fonts`/`Asset`; migration filename) are all reasonable and correctly justified against the brief and the "core is the contract" rule — none conflict with a hard constraint. ### Strengths - The visibility seam is genuinely single-sourced and defended with a comment explaining why the OR clause must be parenthesised — a real bug class avoided. - `editDeck`'s retry-on-`errDeckBusy` correctly reopens a fresh handle rather than retrying on the stale one, avoiding double-application of ops after a rolled-back transaction — I traced this and it's correct. - `Block.UnmarshalJSON`'s fractional-`z` tolerance is a well-reasoned, narrowly-scoped repair with a clear justification (CRDT peers can insert fractional z), and it's exactly the one place the package says it repairs anything. - Test coverage exercises cross-cutting behaviour that matters: rename reaching both row and document, delete cascading to updates/grants, empty-list state, and 404-not-403 semantics for both reads and writes. - Self-review section (h.JSON out of transaction, per-row degrade on `slideCount`, `rst-box-head` nesting fix) shows real verification happened before commit, not just claimed. ### Issues #### Critical (Must Fix) None. #### Important (Should Fix) - **`saveDeckSettings` writes the document then the row in two non-atomic steps** (`decks.go`, `saveDeckSettings`): a crash or error between `editDeck` and the row `Updates` call leaves the list showing a stale title while the document (and any concurrent viewer of `GET /d/{sqid}`) shows the new one — the opposite of the "list promises a name the deck doesn't have" case the author optimized against, but still a real, unhandled divergence with no repair path. Acknowledged by the author as "a window"; worth a follow-up ticket rather than blocking this task, since fixing it properly needs either a single source of truth for title/theme or a reconciliation step outside this task's scope. #### Minor (Nice to Have) - `decks.go` is 1007 lines covering ids, the seam, document ops, and six screens in one file; the header comment justifies the grouping, but a future split (e.g. screens vs. document/seam plumbing) would ease navigation as slice 2+ grows it further. - The process never closes `core.Runtime` (`app.go`), and each test's `newApp` builds a fresh one — acknowledged by the author as harmless today; worth a `Close` seam before this pattern multiplies across more test files. - `slides.deck.theme.invalid` is unreachable through the browser (native `<select>` can't produce a bad value) — correctly defended as "the handler must not trust the body," not a real issue, just noting it's untested by an integration test (only reachable via a hand-crafted POST). ### Assessment **Task quality:** Approved **Reasoning:** Every hard constraint in the task brief and the global rules (seam, 404 semantics, actor recording, transaction scoping to `tx`, CSRF/session placement, catalog strings, rst-* grammar, sqid/id schemes, one `*core.Runtime`) is met and verifiable in the diff. The one real gap — the two-phase, non-atomic settings write — is a narrow, acknowledged edge case with a defensible ordering choice, not a correctness or security defect, and doesn't block this task; it's a good candidate for the reviewer's punch-list on the next task rather than a blocker here.</result> <usage><subagent_tokens>128256</subagent_tokens><tool_uses>31</tool_uses><duration_ms>215973</duration_ms></usage> </task-notification>
Attributed to this commit: 14.0 g CO₂e (1782293 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on worktree-slice-1, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/cmd/slides/main.go b/cmd/slides/main.go| index 1e6db24..09f4dfe 100644 |
| --- a/cmd/slides/main.go |
| +++ b/cmd/slides/main.go |
| @@ -222,7 +222,7 @@ func main() { |
| // Every empty value below is a startup refusal inside slides.New |
| // unless env is "development" — see config.go's package comment. |
| - mux, err := slides.New(d, slides.Config{ |
| + mux, a, err := slides.New(d, slides.Config{ |
| Origin: origin, |
| Env: env, |
| InstanceKey: os.Getenv("SLIDES_INSTANCE_KEY"), |
| @@ -238,6 +238,10 @@ func main() { |
| logger.Error("build app", "err", err) |
| os.Exit(1) |
| } |
| + // Releases the compiled deck core's wazero runtime on the way out — |
| + // after rastrillo.Serve returns (a signal, or a fatal error), same |
| + // as d.Close() above. |
| + defer a.Close() |
| opts.Mux = mux |
| opts.DBPath = "" |
diff --git a/internal/slides/app.go b/internal/slides/app.go| index 2efcc5e..c86a7db 100644 |
| --- a/internal/slides/app.go |
| +++ b/internal/slides/app.go |
| @@ -28,10 +28,17 @@ import ( |
| // The database handle is the caller's (cmd/slides/main.go's db.Open), |
| // not one this opens: SKILL.md §1's Resolve/Serve split depends on |
| // exactly one connection to the file. |
| -func New(d *db.DB, cfg Config) (*http.ServeMux, error) { |
| +// |
| +// The second return value is the built *app, unexported so callers |
| +// outside this package can only ever hold one (via :=) and call its |
| +// exported methods — today, just Close, which releases the deck core's |
| +// runtime. Returning it is what lets cmd/slides/main.go and the test |
| +// harness release that resource on shutdown without this package |
| +// exposing anything more of the app than it already did. |
| +func New(d *db.DB, cfg Config) (*http.ServeMux, *app, error) { |
| a, authPlugin, err := newApp(d, cfg) |
| if err != nil { |
| - return nil, err |
| + return nil, nil, err |
| } |
| r := chi.NewRouter() |
| @@ -82,7 +89,7 @@ func New(d *db.DB, cfg Config) (*http.ServeMux, error) { |
| // so the sign-in page can load its stylesheet signed out. |
| mux.Handle("GET /static/", assets.Handler()) |
| mux.Handle("/", r) |
| - return mux, nil |
| + return mux, a, nil |
| } |
| // newApp is everything New does before the router: resolve the config |
| @@ -91,8 +98,9 @@ func New(d *db.DB, cfg Config) (*http.ServeMux, error) { |
| // Separate from New so this package's own tests can assert on the |
| // resolved *app — that a.cfg carries the origin the app is really |
| // protecting rather than the empty string a development caller passed |
| -// in (config_test.go). New itself hands back only a mux, which cannot |
| -// answer that question. |
| +// in (config_test.go). New's own *app return is unexported, so a |
| +// caller outside this package can hold one and Close it but never |
| +// spell its type or read a.cfg; only a package-internal test can. |
| func newApp(d *db.DB, cfg Config) (*app, *rauth.Auth, error) { |
| // Fail closed, loudly, and before anything opens: Origin, |
| // InstanceKey, Mailer and AllowedAddresses each decide whether |
diff --git a/internal/slides/decks.go b/internal/slides/decks.go| index e0831cb..392f36e 100644 |
| --- a/internal/slides/decks.go |
| +++ b/internal/slides/decks.go |
| @@ -242,7 +242,18 @@ var errDeckBusy = errors.New("slides: the deck moved while this update was being |
| // |
| // d.Seq is updated in place on success so a caller applying two batches |
| // in a row does not have to re-read the row. |
| -func (a *app) applyOps(ctx context.Context, h *core.Handle, d *Deck, actor string, ops []byte) (*deck.Deck, error) { |
| +// |
| +// rowUpdates is the row's own columns (title, theme, rung — the ones a |
| +// list or a link needs) to change alongside the document, in the SAME |
| +// transaction as the update-log insert. Nil for every caller that only |
| +// touches the document. Merged into the same `WHERE id = ? AND seq = ?` |
| +// Updates call the seq bump already makes: that call is the optimistic |
| +// lock, so folding the row's other columns into it costs nothing extra |
| +// and means the row and the log commit or roll back together — a |
| +// failure applying rowUpdates (a rejected rung, a constraint) now rolls |
| +// back h.Ops' own DeckUpdate row too, rather than leaving the row |
| +// diverged from the document's meta. |
| +func (a *app) applyOps(ctx context.Context, h *core.Handle, d *Deck, actor string, ops []byte, rowUpdates map[string]any) (*deck.Deck, error) { |
| err := a.db.G.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
| update, err := h.Ops(ctx, ops) |
| if err != nil { |
| @@ -260,8 +271,16 @@ func (a *app) applyOps(ctx context.Context, h *core.Handle, d *Deck, actor strin |
| return fmt.Errorf("slides: record deck %s update %d: %w", d.Sqid, next, err) |
| } |
| + // Named columns off a map, never a bound struct (SKILL.md §4): |
| + // "seq" is always present so the WHERE ... AND seq = ? clause |
| + // above stays the sole optimistic lock, whether or not this |
| + // call is also renaming the deck. |
| + fields := map[string]any{"seq": next} |
| + for k, v := range rowUpdates { |
| + fields[k] = v |
| + } |
| res := tx.Model(&Deck{}).Where("id = ? AND seq = ?", d.ID, d.Seq). |
| - Updates(map[string]any{"seq": next}) |
| + Updates(fields) |
| if res.Error != nil { |
| return fmt.Errorf("slides: bump deck %s seq: %w", d.Sqid, res.Error) |
| } |
| @@ -302,14 +321,14 @@ func (a *app) applyOps(ctx context.Context, h *core.Handle, d *Deck, actor strin |
| // document — the batch applied nothing, so replaying it against the |
| // state that beat us is the correct thing to do, and a second conflict |
| // means real contention rather than a race worth spinning on. |
| -func (a *app) editDeck(ctx context.Context, d *Deck, actor string, ops []byte) error { |
| +func (a *app) editDeck(ctx context.Context, d *Deck, actor string, ops []byte, rowUpdates map[string]any) error { |
| var lastErr error |
| for attempt := 0; attempt < 2; attempt++ { |
| h, _, err := a.openDeck(ctx, *d) |
| if err != nil { |
| return err |
| } |
| - _, err = a.applyOps(ctx, h, d, actor, ops) |
| + _, err = a.applyOps(ctx, h, d, actor, ops, rowUpdates) |
| h.Close(ctx) |
| if err == nil { |
| return nil |
| @@ -713,7 +732,7 @@ func (a *app) createDeck(w http.ResponseWriter, r *http.Request) { |
| a.fail(w, r, "build the opening ops", err) |
| return |
| } |
| - if err := a.editDeck(r.Context(), &d, actorFor(u), ops); err != nil { |
| + if err := a.editDeck(r.Context(), &d, actorFor(u), ops, nil); err != nil { |
| a.fail(w, r, "write the opening ops", err) |
| return |
| } |
| @@ -881,10 +900,12 @@ func (a *app) deckSettings(w http.ResponseWriter, r *http.Request) { |
| // saveDeckSettings serves POST /d/{sqid}/settings. |
| // |
| // Title and theme live in two places — the row the list reads and the |
| -// document everyone edits — so both are written. The document goes |
| -// first: it is what the deck IS, and a crash between the two writes |
| -// should leave the list stale rather than leave the list promising a |
| -// name the deck does not have. |
| +// document everyone edits — so both are written, and both go through |
| +// editDeck's rowUpdates so they land in the SAME transaction as the |
| +// update-log insert applyOps makes. A rejected row write (a rung a |
| +// constraint refuses, say) now rolls the document change back too, |
| +// rather than leaving the row's title/theme diverged from what the |
| +// document's meta says — the earlier two-transaction shape's window. |
| func (a *app) saveDeckSettings(w http.ResponseWriter, r *http.Request) { |
| u, ok := a.viewer(w, r) |
| if !ok { |
| @@ -920,19 +941,11 @@ func (a *app) saveDeckSettings(w http.ResponseWriter, r *http.Request) { |
| a.fail(w, r, "build the settings ops", err) |
| return |
| } |
| - if err := a.editDeck(r.Context(), &d, actorFor(u), ops); err != nil { |
| + rowUpdates := map[string]any{"title": title, "theme": theme, "rung": rung} |
| + if err := a.editDeck(r.Context(), &d, actorFor(u), ops, rowUpdates); err != nil { |
| a.fail(w, r, "write the settings ops", err) |
| return |
| } |
| - // Named columns, never a bound struct (SKILL.md §4), and scoped to |
| - // this owner's deck so the write carries the same condition the |
| - // read did. |
| - if err := a.db.G.WithContext(r.Context()).Model(&Deck{}). |
| - Where("id = ? AND owner_id = ?", d.ID, u.ID). |
| - Updates(map[string]any{"title": title, "theme": theme, "rung": rung}).Error; err != nil { |
| - a.fail(w, r, "save deck settings", err) |
| - return |
| - } |
| http.Redirect(w, r, "/d/"+d.Sqid, http.StatusSeeOther) |
| } |
diff --git a/internal/slides/decks_test.go b/internal/slides/decks_test.go| new file mode 100644 |
| index 0000000..e6046d1 |
| --- /dev/null |
| +++ b/internal/slides/decks_test.go |
| @@ -0,0 +1,79 @@ |
| +package slides |
| + |
| +import ( |
| + "context" |
| + "testing" |
| +) |
| + |
| +// TestSettingsRowAndDocumentChangeTogether is the regression test for a |
| +// real bug: saveDeckSettings used to write the document via editDeck |
| +// and then the decks row via a second, separate Updates call — two |
| +// transactions, so a failure between them left the row's title/theme |
| +// diverged from what the document's own meta said. |
| +// |
| +// The fix folds the row write into applyOps' one transaction via |
| +// editDeck's rowUpdates: it lands in the same Updates call the seq |
| +// bump already makes, under the same `WHERE id = ? AND seq = ?` lock, |
| +// so it commits or rolls back with the update-log insert. This proves |
| +// the "or rolls back" half by making the row write fail (an unknown |
| +// column — the particular reason doesn't matter, only that it is a |
| +// failure applyOps' transaction sees) and checking that BOTH the row |
| +// and the document come out unchanged, seq included. |
| +func TestSettingsRowAndDocumentChangeTogether(t *testing.T) { |
| + d := testDB(t) |
| + a, _, err := newApp(d, Config{Env: EnvDevelopment, Logger: quietLogger()}) |
| + if err != nil { |
| + t.Fatalf("newApp: %v", err) |
| + } |
| + t.Cleanup(func() { a.Close() }) |
| + ctx := context.Background() |
| + |
| + owner, _, err := ensureUser(ctx, d.G, "paul@example.test") |
| + if err != nil { |
| + t.Fatalf("ensureUser: %v", err) |
| + } |
| + |
| + row, err := a.createDeckRow(ctx, owner, "Original title", defaultTheme) |
| + if err != nil { |
| + t.Fatalf("createDeckRow: %v", err) |
| + } |
| + ops, err := openingOps("Original title", defaultTheme) |
| + if err != nil { |
| + t.Fatalf("openingOps: %v", err) |
| + } |
| + if err := a.editDeck(ctx, &row, actorFor(owner), ops, nil); err != nil { |
| + t.Fatalf("seed editDeck: %v", err) |
| + } |
| + seqBefore := row.Seq |
| + |
| + renameOps, err := opsJSON([]op{{"op": "set_meta", "title": "New title", "theme": defaultTheme}}) |
| + if err != nil { |
| + t.Fatalf("opsJSON: %v", err) |
| + } |
| + // No column named this, so the Updates call inside applyOps' own |
| + // transaction fails — the row write, not h.Ops, is what breaks. |
| + badRowUpdates := map[string]any{"no_such_column": "boom"} |
| + if err := a.editDeck(ctx, &row, actorFor(owner), renameOps, badRowUpdates); err == nil { |
| + t.Fatal("editDeck with an invalid row update returned no error") |
| + } |
| + |
| + var reloaded Deck |
| + if err := d.G.First(&reloaded, row.ID).Error; err != nil { |
| + t.Fatalf("reload deck row: %v", err) |
| + } |
| + if reloaded.Title != "Original title" { |
| + t.Errorf("deck row title = %q after a failed settings write, want %q (unchanged)", reloaded.Title, "Original title") |
| + } |
| + if reloaded.Seq != seqBefore { |
| + t.Errorf("deck row seq = %d after a failed settings write, want %d (unchanged)", reloaded.Seq, seqBefore) |
| + } |
| + |
| + h, view, err := a.openDeck(ctx, reloaded) |
| + if err != nil { |
| + t.Fatalf("openDeck: %v", err) |
| + } |
| + defer h.Close(ctx) |
| + if view.Meta.Title != "Original title" { |
| + t.Errorf("document meta title = %q after a failed settings write, want %q (unchanged)", view.Meta.Title, "Original title") |
| + } |
| +} |
diff --git a/internal/slides/handlers.go b/internal/slides/handlers.go| index e051dd4..ee9ca5d 100644 |
| --- a/internal/slides/handlers.go |
| +++ b/internal/slides/handlers.go |
| @@ -1,6 +1,7 @@ |
| package slides |
| import ( |
| + "context" |
| "log/slog" |
| "amadan.net/rastrillo/rastrillo/db" |
| @@ -32,3 +33,21 @@ type app struct { |
| // broadcasts to open browsers; every caller is nil-safe. |
| onUpdate func(deckID, seq int64) |
| } |
| + |
| +// Close releases the compiled deck core's wazero runtime — the one |
| +// resource New builds for itself rather than taking from its caller |
| +// (newApp's doc comment: the database handle is the caller's, closed |
| +// there). It does not close the database: New never opened it, so it |
| +// is never this method's to release. |
| +// |
| +// New hands back the *app this closes alongside the mux it also |
| +// returns; a caller that never stops (a long-running server whose |
| +// process exit reclaims everything anyway) is free to ignore it, but |
| +// cmd/slides/main.go's shutdown path and every test harness call it so |
| +// a compiled module is not leaked per app built. |
| +func (a *app) Close() error { |
| + if a == nil || a.core == nil { |
| + return nil |
| + } |
| + return a.core.Close(context.Background()) |
| +} |
diff --git a/internal/slides/sample.go b/internal/slides/sample.go| index 4607898..9fc6f83 100644 |
| --- a/internal/slides/sample.go |
| +++ b/internal/slides/sample.go |
| @@ -45,7 +45,7 @@ func (a *app) seedSampleDeck(ctx context.Context, u User) error { |
| if err != nil { |
| return err |
| } |
| - return a.editDeck(ctx, &d, actorFor(u), encoded) |
| + return a.editDeck(ctx, &d, actorFor(u), encoded, nil) |
| } |
| // slideIDs mints the ids one slide's ops need: the slide's own and n |
diff --git a/internal/slidestest/browser_test.go b/internal/slidestest/browser_test.go| index a085bb6..c101a9c 100644 |
| --- a/internal/slidestest/browser_test.go |
| +++ b/internal/slidestest/browser_test.go |
| @@ -43,7 +43,7 @@ func TestBrowserWalk(t *testing.T) { |
| t.Fatalf("db.Open: %v", err) |
| } |
| t.Cleanup(func() { d.Close() }) |
| - mux, err := slides.New(d, slides.Config{ |
| + mux, a, err := slides.New(d, slides.Config{ |
| Origin: origin, |
| InstanceKey: testInstanceKey, |
| Mailer: &fakeMailer{}, |
| @@ -53,6 +53,7 @@ func TestBrowserWalk(t *testing.T) { |
| if err != nil { |
| t.Fatalf("slides.New: %v", err) |
| } |
| + t.Cleanup(func() { a.Close() }) |
| return localised(t, mux) |
| }) |
| rig.Run(chromedp.Navigate(rig.Origin + "/signin")) |
diff --git a/internal/slidestest/harness_test.go b/internal/slidestest/harness_test.go| index b74a492..2217c61 100644 |
| --- a/internal/slidestest/harness_test.go |
| +++ b/internal/slidestest/harness_test.go |
| @@ -114,10 +114,13 @@ func newApp(t *testing.T) *testApp { |
| func newAppWith(t *testing.T, cfg slides.Config, mailer *fakeMailer) *testApp { |
| t.Helper() |
| d := openTestDB(t, cfg) |
| - mux, err := slides.New(d, cfg) |
| + mux, a, err := slides.New(d, cfg) |
| if err != nil { |
| t.Fatalf("slides.New: %v", err) |
| } |
| + // Releases the compiled deck core's wazero runtime; every test app |
| + // built over slides.New leaks one without this (task 3 fix round 1). |
| + t.Cleanup(func() { a.Close() }) |
| return &testApp{Handler: localised(t, mux), mailer: mailer, db: d} |
| } |
diff --git a/internal/slidestest/signin_test.go b/internal/slidestest/signin_test.go| index dbe5ccb..84e4f5e 100644 |
| --- a/internal/slidestest/signin_test.go |
| +++ b/internal/slidestest/signin_test.go |
| @@ -255,7 +255,7 @@ func TestDevelopmentDefaultsRefusedInProduction(t *testing.T) { |
| cfg, _ := newConfig(t) |
| cfg.Env = "production" |
| tc.bend(&cfg) |
| - if _, err := slides.New(openTestDB(t, cfg), cfg); err == nil { |
| + if _, _, err := slides.New(openTestDB(t, cfg), cfg); err == nil { |
| t.Fatalf("slides.New with an empty %s and Env %q returned no error; want a startup refusal", tc.name, cfg.Env) |
| } |
| }) |
| @@ -272,7 +272,9 @@ func TestDevelopmentDefaultsAllowedInDevelopment(t *testing.T) { |
| cfg.Origin = "" |
| cfg.Mailer = nil |
| cfg.AllowedAddresses = nil |
| - if _, err := slides.New(openTestDB(t, cfg), cfg); err != nil { |
| + _, a, err := slides.New(openTestDB(t, cfg), cfg) |
| + if err != nil { |
| t.Fatalf("slides.New with an empty config and Env %q: %v", cfg.Env, err) |
| } |
| + t.Cleanup(func() { a.Close() }) |
| } |