amadan

oficina / docs Public

Seed a person's space on creation, not the instance on boot

The app shipped and the document list was empty. Six sample documents
existed and were correct; they were unreachable on any deployed instance.
Seed was reachable only through -seed, gated on -env development, which
the platform never passes — that gate is correct and unchanged. And
documents are owner-scoped, so a boot-time seed has no person to own
anything. Ownership is per person, so seeding has to be too.

A person's space is now seeded when their row is first created, opted in
per instance by $DOCS_SAMPLE_DOCUMENTS, default off. Tied to creation
rather than to an empty list so deleting every document cannot resurrect
them; default off so a real team is never handed six invented documents
about an allotment committee.

Seeding runs after the person INSERT commits, outside any transaction,
and every failure is logged and swallowed: a person who cannot sign in
because sample content failed to generate is worse than one with an
empty list. $DOCS_SAMPLE_DOCUMENTS is deliberately not fail-closed,
unlike Origin, InstanceKey and the sign-in allowlist — those are security
controls, this is cosmetic content, and a typo parses to off. The
asymmetry is documented where a reader will expect a refusal.

Verified against the real binary with no -seed flag and only the env var,
the exact shape the platform runs: six documents, four recency groups.
Testing the wiring is what produced an empty instance in the first place.

Also fixes a vacuous test found by review, the fourth of its shape here:
TestEnsurePersonSurvivesSeedingFailure asserted EnsurePerson succeeded
and List did not error, both true whether or not seeding failed. It now
asserts fewer than len(seedDocs) documents survived, verified by swapping
the closed core for a working one and watching it fail.

Known: accounts created before the setting is enabled never get samples.
The found-branch of EnsurePerson skips seeding by design.

Discussion: https://amadan.net/oficina/docs/discussions/8
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011F5CivVm4CmL9ByfSSAaXA
Paul Campbell pushed by paul@keymail.dev 25930b1f9fbfffe1fa52bd47a2318f6a78ae5435 parent 43ca2e2
7 files changed, +596 −21
  • cmd/docs/main.go +26 −5
  • internal/docs/app.go +55 −1
  • internal/docs/sample_documents_internal_test.go +219 −0
  • internal/docs/sample_documents_test.go +189 −0
  • internal/docs/seed.go +18 −9
  • internal/docs/seed_test.go +3 −3
  • internal/docs/store.go +86 −3

CI passed — run details

diff --git a/cmd/docs/main.go b/cmd/docs/main.go
index 3780808..7656c6a 100644
--- a/cmd/docs/main.go
+++ b/cmd/docs/main.go
@@ -229,6 +229,17 @@ func main() {
// would otherwise silently mean.
allowedAddresses := os.Getenv("DOCS_ALLOWED_ADDRESSES")
+ // $DOCS_SAMPLE_DOCUMENTS opts a deployed instance into
+ // docs.Config.SampleDocuments — see its own doc comment for the
+ // full reasoning (parsed permissively, default off, deliberately
+ // not fail-closed like the settings just above). This is the
+ // platform-reachable fix for the bug this branch exists to close:
+ // -seed below never runs on a deployed instance (gated to -env
+ // development, which activation argv never sets), so without this
+ // env var a real deployment's sample content stays exactly as
+ // unreachable as it was before.
+ sampleDocuments := os.Getenv("DOCS_SAMPLE_DOCUMENTS")
+
// chooseEnv: -env wins; $DOCS_ENV wins next EXCEPT for
// "development" itself, which chooseEnv never honours from an env
// var (fix round 2 — see its own doc comment); the default with
@@ -258,6 +269,7 @@ func main() {
InstanceKey: instanceKey,
Mailer: mailer,
AllowedAddresses: allowedAddresses,
+ SampleDocuments: sampleDocuments,
Logger: logger,
}
app, err := docs.New(cfg)
@@ -268,21 +280,30 @@ func main() {
defer app.Close()
// Only reachable with both -seed and -env development (the check
- // above) — after New, since docs.Seed needs a real *docs.App with
- // its store and migrated database already in place, and after the
- // InstanceKey/Mailer checks above, so a database that cannot even
- // send a real sign-in link never gets seeded either.
+ // above) — after New, since both EnsurePerson and docs.Seed need a
+ // real app.Store() over an already-migrated database, and after
+ // the InstanceKey/Mailer checks above, so a database that cannot
+ // even send a real sign-in link never gets seeded either.
if seedFlag {
seedAddress := strings.TrimSpace(os.Getenv("DOCS_SEED_ADDRESS"))
if seedAddress == "" {
seedAddress = devSeedAddress
}
+ // EnsurePerson here runs with cfg.SampleDocuments as given on
+ // this invocation: if it's also truthy, EnsurePerson's own
+ // per-person seeding (store.go) already seeds seedAddress the
+ // moment its Person row is first created, before this call
+ // even returns — see its doc comment. The explicit docs.Seed
+ // call just below runs unconditionally regardless, exactly as
+ // it always has: -seed's whole point is "seed this address
+ // right now, guaranteed", not "seed it only if some other
+ // setting happens to agree."
owner, err := app.Store().EnsurePerson(context.Background(), seedAddress)
if err != nil {
logger.Error("docs: seed: resolve person", "address", seedAddress, "err", err)
os.Exit(1)
}
- if err := docs.Seed(context.Background(), app, owner); err != nil {
+ if err := docs.Seed(context.Background(), app.Store(), owner); err != nil {
logger.Error("docs: seed", "address", seedAddress, "err", err)
os.Exit(1)
}
diff --git a/internal/docs/app.go b/internal/docs/app.go
index 5be3dfe..eb2b6da 100644
--- a/internal/docs/app.go
+++ b/internal/docs/app.go
@@ -160,10 +160,64 @@ type Config struct {
// alternative of silently admitting nobody.
AllowedAddresses string
+ // SampleDocuments opts this instance into seeding a brand-new
+ // person's very first sign-in with CARLOS Docs' own sample
+ // documents (seed.go's Seed, six of them) — the fix for "Paul
+ // signed in to a live instance and saw an empty list": the sample
+ // documents docs.Seed already wrote were never reachable on a
+ // deployed instance, because -seed is gated to Env ==
+ // "development" (main.go's own doc comment, unchanged by this
+ // field) and a deployed instance never runs with that flag.
+ // Documents are owner-scoped (Document.OwnerID, owned() in
+ // store.go) and a boot-time seed has no person to own anything, so
+ // this switches seeding from a boot-time, instance-wide event to a
+ // per-person one — see Store.EnsurePerson's own doc comment for
+ // exactly where that happens and why there, not "the list is
+ // empty" (a person who deletes every sample document must not get
+ // them back).
+ //
+ // Sourced from $DOCS_SAMPLE_DOCUMENTS (cmd/docs/main.go), the same
+ // "<APP>_..." convention every other setting on this struct uses.
+ // Parsed by parseSampleDocuments below, which accepts a small,
+ // case-insensitive truthy set ("1", "true", "yes") and treats
+ // everything else — unset, empty, or a typo alike — as off.
+ // Default OFF: a real team instance must never get six invented
+ // documents about a reading room and an allotment committee just
+ // because nobody thought to unset something.
+ //
+ // Deliberately NOT fail-closed the way Origin/InstanceKey/
+ // AllowedAddresses above are: those three guard who can sign in
+ // and what seals a session, where a value New cannot make sense of
+ // silently taking the permissive branch would be a real security
+ // regression, so New refuses to start rather than guess. This
+ // setting only decides whether a new person's document list starts
+ // with six sample documents or zero — cosmetic content, not a
+ // security control — so an unparseable value here is read as "off"
+ // and New boots anyway. A platform whose env var block has a typo
+ // in $DOCS_SAMPLE_DOCUMENTS should get an empty list, silently, the
+ // same outcome as never having set it at all, not a refusal to
+ // serve any traffic over a setting that was never load-bearing for
+ // safety in the first place.
+ SampleDocuments string
+
// Logger defaults to slog.Default().
Logger *slog.Logger
}
+// parseSampleDocuments interprets raw the way Config.SampleDocuments'
+// own doc comment describes: a small, case-insensitive truthy set
+// means on, and everything else means off. See that doc comment for
+// why this is deliberately not fail-closed like resolveInstanceKey/
+// resolveAllowlist (auth.go) just below use for their own settings.
+func parseSampleDocuments(raw string) bool {
+ switch strings.ToLower(strings.TrimSpace(raw)) {
+ case "1", "true", "yes":
+ return true
+ default:
+ return false
+ }
+}
+
// DefaultOrigin is Config.Origin's fallback when empty — see its doc
// comment. Exported so a caller that needs to know what origin this
// app is protecting itself against (NewForTest's callers, notably)
@@ -323,7 +377,7 @@ func New(cfg Config) (*App, error) {
d.Close()
return nil, fmt.Errorf("docs: start core runtime: %w", err)
}
- store := NewStore(d.G, rt)
+ store := NewStore(d.G, rt, parseSampleDocuments(cfg.SampleDocuments), logger)
render, err := newRenderer()
if err != nil {
diff --git a/internal/docs/sample_documents_internal_test.go b/internal/docs/sample_documents_internal_test.go
new file mode 100644
index 0000000..e2aa5cb
--- /dev/null
+++ b/internal/docs/sample_documents_internal_test.go
@@ -0,0 +1,219 @@
+package docs
+
+import (
+ "context"
+ "path/filepath"
+ "sync"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/docs/internal/core"
+)
+
+// newSampleDocumentsInternalTestApp is this file's own copy of
+// sample_documents_test.go's newSampleDocumentsTestApp: that one
+// lives in package docs_test and cannot be reused from here, and the
+// tests below need package-internal access (a *Store's own db handle,
+// to soft-delete rows the way a future "delete document" handler
+// would; a second, deliberately broken *core.Runtime) that only a
+// docs-package test can reach.
+func newSampleDocumentsInternalTestApp(t *testing.T, sampleDocuments string) *App {
+ t.Helper()
+ app, err := New(Config{
+ DBPath: filepath.Join(t.TempDir(), "docs.db"),
+ Env: "development",
+ SampleDocuments: sampleDocuments,
+ })
+ if err != nil {
+ t.Fatalf("New: %v", err)
+ }
+ t.Cleanup(func() {
+ if cerr := app.Close(); cerr != nil {
+ t.Errorf("App.Close: %v", cerr)
+ }
+ })
+ return app
+}
+
+// TestPersonWhoDeletesSampleDocumentsGetsNoneBackOnNextSignIn is the
+// property EnsurePerson's own doc comment calls out by name: seeding
+// is tied to "this Person row was just created", never to "this
+// person's list happens to be empty". There is no delete handler yet
+// (spec's later work), so this reaches into store.db directly to
+// soft-delete every one of the seeded rows exactly the way such a
+// handler would (setting deleted_at, the same column List's own
+// "deleted_at IS NULL" filter checks) — then signs the same address
+// in again and asserts nothing comes back.
+func TestPersonWhoDeletesSampleDocumentsGetsNoneBackOnNextSignIn(t *testing.T) {
+ app := newSampleDocumentsInternalTestApp(t, "1")
+ ctx := context.Background()
+ store := app.Store()
+
+ owner, err := store.EnsurePerson(ctx, "deletes-everything@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson (first): %v", err)
+ }
+ first, err := store.List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List (first): %v", err)
+ }
+ if len(first) != 6 {
+ t.Fatalf("got %d seeded documents, want 6", len(first))
+ }
+
+ now := time.Now().UTC()
+ if err := owned(store.db.WithContext(ctx), owner).
+ Model(&Document{}).
+ Update("deleted_at", now).Error; err != nil {
+ t.Fatalf("soft-delete owner's documents: %v", err)
+ }
+
+ afterDelete, err := store.List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List (after delete): %v", err)
+ }
+ if len(afterDelete) != 0 {
+ t.Fatalf("got %d documents after deleting all of them, want 0", len(afterDelete))
+ }
+
+ // The person row already exists, so this hits EnsurePerson's
+ // found branch, not its create branch — seedIfEnabled is never
+ // called, and List must stay at zero.
+ ownerAgain, err := store.EnsurePerson(ctx, "deletes-everything@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson (second): %v", err)
+ }
+ if ownerAgain != owner {
+ t.Fatalf("EnsurePerson(again) = %d, want %d", ownerAgain, owner)
+ }
+
+ after, err := store.List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List (after second sign-in): %v", err)
+ }
+ if len(after) != 0 {
+ t.Fatalf("got %d documents back after signing in again, want 0 — sample documents were resurrected", len(after))
+ }
+}
+
+// TestEnsurePersonSurvivesSeedingFailure is the care point the task
+// calls out explicitly: seeding failure must never break sign-in. To
+// force Seed itself to fail without touching the Person row
+// EnsurePerson creates first (which would defeat the point of the
+// test — the person must exist BEFORE seeding is even attempted),
+// this builds a second, deliberately broken *core.Runtime — closed
+// immediately after it compiles — and hands it to a Store sharing the
+// real, working database underneath app's own Store.
+//
+// Confirmed directly (see the probe this test's design was checked
+// against): calling a wazero module method through a Runtime whose
+// underlying wazero runtime has been Close'd returns a clean error
+// ("runtime closed with exit_code(0)"), not a panic — Store.replay's
+// s.core.NewDoc call, the first thing Seed's loop does for its first
+// document's ApplyUpdate, fails this way every time, deterministically,
+// with no flakiness from timing.
+func TestEnsurePersonSurvivesSeedingFailure(t *testing.T) {
+ app := newSampleDocumentsInternalTestApp(t, "1")
+ ctx := context.Background()
+
+ brokenCore, err := core.New(ctx)
+ if err != nil {
+ t.Fatalf("core.New: %v", err)
+ }
+ if err := brokenCore.Close(ctx); err != nil {
+ t.Fatalf("brokenCore.Close: %v", err)
+ }
+
+ broken := NewStore(app.store.db, brokenCore, true, app.logger)
+
+ id, err := broken.EnsurePerson(ctx, "seed-fails@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson must succeed even when seeding fails, got: %v", err)
+ }
+ if id <= 0 {
+ t.Fatalf("EnsurePerson returned a non-positive id: %d", id)
+ }
+
+ // The person is durably there: a fresh EnsurePerson call for the
+ // same address finds the same row rather than creating (or
+ // re-seeding) it.
+ id2, err := broken.EnsurePerson(ctx, "seed-fails@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson (again): %v", err)
+ }
+ if id2 != id {
+ t.Fatalf("EnsurePerson(again) = %d, want %d (same person, not recreated)", id2, id)
+ }
+
+ // And the same person, looked up through the app's own working
+ // Store, is a real, usable person: List works and returns without
+ // error (whatever partial content Seed managed before it failed —
+ // this asserts "does not break", not "produced zero documents",
+ // since Seed's per-document loop may have left one partial row
+ // behind before its first ApplyUpdate failed).
+ docs, err := app.Store().List(ctx, id)
+ if err != nil {
+ t.Fatalf("List for the person seeding failed for: %v", err)
+ }
+
+ // Assert the seeding actually FAILED. Without this the test is
+ // vacuous: "EnsurePerson succeeded and List did not error" holds
+ // just as well if seeding silently succeeded, or was skipped
+ // because a future refactor stopped calling seedIfEnabled at all
+ // — in which case this test would keep passing while proving
+ // nothing about surviving a failure. A complete seed is
+ // len(seedDocs) documents, so anything short of that is
+ // positive evidence the broken core was reached and the loop died
+ // partway. Deliberately not asserting an exact count: where
+ // precisely Seed dies is an implementation detail, but that it
+ // died at all is the whole property under test.
+ if len(docs) >= len(seedDocs) {
+ t.Fatalf("got %d documents, want fewer than %d — seeding was expected to FAIL against the closed core, so this test is not proving what it claims", len(docs), len(seedDocs))
+ }
+}
+
+// TestConcurrentFirstSignInsProduceOnePersonAndOneSampleSet is the
+// concurrency requirement stated explicitly: two simultaneous first
+// sign-ins for the same brand-new address must not produce two
+// people or two sets of documents. EnsurePerson's own duplicate-key
+// race handling (store.go) already guarantees one Person row; this
+// pins that seeding rides along with it — exactly one winner reaches
+// seedIfEnabled, so the address ends up with exactly six documents,
+// never twelve.
+func TestConcurrentFirstSignInsProduceOnePersonAndOneSampleSet(t *testing.T) {
+ app := newSampleDocumentsInternalTestApp(t, "1")
+ store := app.Store()
+ const address = "concurrent-first-signin@example.com"
+
+ const n = 8
+ ids := make([]int64, n)
+ errs := make([]error, n)
+ var wg sync.WaitGroup
+ wg.Add(n)
+ for i := 0; i < n; i++ {
+ go func(i int) {
+ defer wg.Done()
+ ids[i], errs[i] = store.EnsurePerson(context.Background(), address)
+ }(i)
+ }
+ wg.Wait()
+
+ for i, err := range errs {
+ if err != nil {
+ t.Fatalf("EnsurePerson[%d]: %v", i, err)
+ }
+ }
+ for i := 1; i < n; i++ {
+ if ids[i] != ids[0] {
+ t.Fatalf("EnsurePerson returned different ids across concurrent calls: %d and %d — two people were created for one address", ids[0], ids[i])
+ }
+ }
+
+ list, err := store.List(context.Background(), ids[0])
+ if err != nil {
+ t.Fatalf("List: %v", err)
+ }
+ if len(list) != 6 {
+ t.Fatalf("got %d documents after %d concurrent first sign-ins, want exactly 6 (one seeded set, not %d)", len(list), n, n*6)
+ }
+}
diff --git a/internal/docs/sample_documents_test.go b/internal/docs/sample_documents_test.go
new file mode 100644
index 0000000..72f10fd
--- /dev/null
+++ b/internal/docs/sample_documents_test.go
@@ -0,0 +1,189 @@
+package docs_test
+
+import (
+ "context"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "github.com/carlosframework/docs/internal/docs"
+)
+
+// newSampleDocumentsTestApp builds a development App exactly like
+// docs.NewForTest, except with Config.SampleDocuments set explicitly
+// — the same "build directly via New, package-internal test helper"
+// shape auth_allowlist_test.go's newAllowlistedTestApp uses for the
+// same reason: NewForTest itself never sets this, so any test that
+// wants seeding on (or explicitly off) has to build its own App.
+func newSampleDocumentsTestApp(t *testing.T, sampleDocuments string) *docs.App {
+ t.Helper()
+ app, err := docs.New(docs.Config{
+ DBPath: filepath.Join(t.TempDir(), "docs.db"),
+ Env: "development",
+ SampleDocuments: sampleDocuments,
+ })
+ if err != nil {
+ t.Fatalf("New: %v", err)
+ }
+ t.Cleanup(func() {
+ if cerr := app.Close(); cerr != nil {
+ t.Errorf("App.Close: %v", cerr)
+ }
+ })
+ return app
+}
+
+// TestNewPersonWithSampleDocumentsOnGetsExactlySix pins the design
+// this branch exists to ship: a brand-new person's very first
+// EnsurePerson call, on an instance that opted in via
+// $DOCS_SAMPLE_DOCUMENTS, gets seed.go's own sample set — exactly six
+// documents (len(seedDocs) in seed.go), owned by them, each with real,
+// substantial text read back through the actual CRDT (Store.Text),
+// never a row count standing in for content that was never applied.
+func TestNewPersonWithSampleDocumentsOnGetsExactlySix(t *testing.T) {
+ app := newSampleDocumentsTestApp(t, "1")
+ ctx := context.Background()
+
+ owner, err := app.Store().EnsurePerson(ctx, "brand-new@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson: %v", err)
+ }
+
+ list, err := app.Store().List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List: %v", err)
+ }
+ if len(list) != 6 {
+ t.Fatalf("got %d documents, want exactly 6", len(list))
+ }
+
+ for _, d := range list {
+ text, err := app.Store().Text(ctx, d)
+ if err != nil {
+ t.Fatalf("Text(%q): %v", d.Title, err)
+ }
+ if len(strings.TrimSpace(text)) < 200 {
+ t.Errorf("%q has %d characters of text — want substantial, real content", d.Title, len(text))
+ }
+ if d.OwnerID != owner {
+ t.Errorf("%q has OwnerID %d, want %d", d.Title, d.OwnerID, owner)
+ }
+ }
+}
+
+// TestNewPersonWithSampleDocumentsOffGetsNone is the opt-in's default:
+// with $DOCS_SAMPLE_DOCUMENTS unset (SampleDocuments left "" here,
+// exactly what a real deployment gets if nobody has set the variable
+// at all), a brand-new person's list starts empty — the pre-existing,
+// unchanged behaviour every real team instance must keep.
+func TestNewPersonWithSampleDocumentsOffGetsNone(t *testing.T) {
+ app := newSampleDocumentsTestApp(t, "")
+ ctx := context.Background()
+
+ owner, err := app.Store().EnsurePerson(ctx, "brand-new@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson: %v", err)
+ }
+
+ list, err := app.Store().List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List: %v", err)
+ }
+ if len(list) != 0 {
+ t.Fatalf("got %d documents with SampleDocuments off, want 0", len(list))
+ }
+}
+
+// TestExistingPersonSigningInAgainGetsNoNewDocuments is the seam this
+// whole design hangs off: EnsurePerson's create branch seeds, its
+// found branch (Person row already exists) never does. Calling
+// EnsurePerson a second time for the same address must not grow the
+// document count at all, whether or not sample content already
+// landed on the first call.
+func TestExistingPersonSigningInAgainGetsNoNewDocuments(t *testing.T) {
+ app := newSampleDocumentsTestApp(t, "true")
+ ctx := context.Background()
+
+ owner, err := app.Store().EnsurePerson(ctx, "returning@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson (first): %v", err)
+ }
+ first, err := app.Store().List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List (first): %v", err)
+ }
+ if len(first) != 6 {
+ t.Fatalf("got %d documents after first sign-in, want 6", len(first))
+ }
+
+ ownerAgain, err := app.Store().EnsurePerson(ctx, "returning@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson (second): %v", err)
+ }
+ if ownerAgain != owner {
+ t.Fatalf("EnsurePerson(again) = %d, want the same person id %d", ownerAgain, owner)
+ }
+
+ second, err := app.Store().List(ctx, owner)
+ if err != nil {
+ t.Fatalf("List (second): %v", err)
+ }
+ if len(second) != len(first) {
+ t.Fatalf("document count grew from %d to %d on a repeat sign-in", len(first), len(second))
+ }
+}
+
+// TestTwoPeopleEachGetTheirOwnSampleSet checks seeding does not
+// undermine owned() (store.go): two different brand-new people each
+// get their own full set of six, and List for one never includes a
+// row belonging to the other — the same ownership guarantee
+// TestOwnedScopesByOwner (scope_test.go) pins for hand-created
+// documents, exercised here through the seeding path instead.
+func TestTwoPeopleEachGetTheirOwnSampleSet(t *testing.T) {
+ app := newSampleDocumentsTestApp(t, "yes")
+ ctx := context.Background()
+
+ alice, err := app.Store().EnsurePerson(ctx, "alice-sample@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson(alice): %v", err)
+ }
+ bob, err := app.Store().EnsurePerson(ctx, "bob-sample@example.com")
+ if err != nil {
+ t.Fatalf("EnsurePerson(bob): %v", err)
+ }
+ if alice == bob {
+ t.Fatalf("alice and bob resolved to the same person id %d", alice)
+ }
+
+ aliceDocs, err := app.Store().List(ctx, alice)
+ if err != nil {
+ t.Fatalf("List(alice): %v", err)
+ }
+ bobDocs, err := app.Store().List(ctx, bob)
+ if err != nil {
+ t.Fatalf("List(bob): %v", err)
+ }
+ if len(aliceDocs) != 6 {
+ t.Fatalf("alice has %d documents, want 6", len(aliceDocs))
+ }
+ if len(bobDocs) != 6 {
+ t.Fatalf("bob has %d documents, want 6", len(bobDocs))
+ }
+
+ for _, d := range aliceDocs {
+ if d.OwnerID != alice {
+ t.Errorf("alice's List returned a document owned by %d, not %d", d.OwnerID, alice)
+ }
+ if _, err := app.Store().Get(ctx, bob, d.Sqid); err == nil {
+ t.Errorf("bob can Get alice's document %q — owned() is not isolating seeded documents", d.Sqid)
+ }
+ }
+ for _, d := range bobDocs {
+ if d.OwnerID != bob {
+ t.Errorf("bob's List returned a document owned by %d, not %d", d.OwnerID, bob)
+ }
+ if _, err := app.Store().Get(ctx, alice, d.Sqid); err == nil {
+ t.Errorf("alice can Get bob's document %q — owned() is not isolating seeded documents", d.Sqid)
+ }
+ }
+}
diff --git a/internal/docs/seed.go b/internal/docs/seed.go
index fd608af..7a4dbc4 100644
--- a/internal/docs/seed.go
+++ b/internal/docs/seed.go
@@ -61,15 +61,24 @@ var seedDocs = []seedDoc{
// exactly the route the wire-compatibility gate's own fixtures
// already use (ReadFixture, testing.go).
//
-// It is meant for a fresh, empty database — cmd/docs/main.go's -seed
-// flag, gated to Env == "development", is the only caller — and does
-// not check for or skip existing documents: calling it twice against
-// the same owner seeds the sample set twice over. That is a feature
-// for reasoning about, not a safety net around; the safety belongs at
-// the caller, not here (main.go's own doc comment on why -seed is
-// gated the way it is).
-func Seed(ctx context.Context, app *App, owner int64) error {
- store := app.Store()
+// It is meant for a fresh, empty owner and does not check for or skip
+// existing documents: calling it twice against the same owner seeds
+// the sample set twice over. That is a feature for reasoning about,
+// not a safety net around; the safety belongs at the caller, not
+// here. There are exactly two callers, each with its own safety
+// story: cmd/docs/main.go's -seed flag, gated to Env == "development"
+// (main.go's own doc comment on why); and Store.EnsurePerson's
+// seedIfEnabled (store.go), called at most once per person, ever, on
+// the one event in a person's history that can only happen once —
+// their row not existing yet (seedIfEnabled's own doc comment).
+//
+// Takes a *Store rather than a *App: this is the same store either
+// caller already has (app.Store(), or the store method's own s), and
+// a *Store has no need for anything else the App wraps — the router,
+// the mailer, the auth plugin. Store.EnsurePerson calling this
+// directly, in-package, is what makes per-person seeding possible at
+// all without a Store depending back on the *App that holds it.
+func Seed(ctx context.Context, store *Store, owner int64) error {
base := time.Now().UTC()
for _, sd := range seedDocs {
diff --git a/internal/docs/seed_test.go b/internal/docs/seed_test.go
index 5c84f04..1c11da6 100644
--- a/internal/docs/seed_test.go
+++ b/internal/docs/seed_test.go
@@ -17,7 +17,7 @@ func TestSeedProducesReadableDocuments(t *testing.T) {
ctx := context.Background()
owner := app.TestPerson(t, "seed@example.com")
- if err := docs.Seed(ctx, app, owner); err != nil {
+ if err := docs.Seed(ctx, app.Store(), owner); err != nil {
t.Fatalf("Seed: %v", err)
}
@@ -54,7 +54,7 @@ func TestSeedVariesTimestamps(t *testing.T) {
ctx := context.Background()
owner := app.TestPerson(t, "seed@example.com")
- if err := docs.Seed(ctx, app, owner); err != nil {
+ if err := docs.Seed(ctx, app.Store(), owner); err != nil {
t.Fatalf("Seed: %v", err)
}
@@ -86,7 +86,7 @@ func TestSeedContentMatchesItsSource(t *testing.T) {
ctx := context.Background()
owner := app.TestPerson(t, "source-check@example.com")
- if err := docs.Seed(ctx, app, owner); err != nil {
+ if err := docs.Seed(ctx, app.Store(), owner); err != nil {
t.Fatalf("Seed: %v", err)
}
diff --git a/internal/docs/store.go b/internal/docs/store.go
index fde79d1..a4a3cff 100644
--- a/internal/docs/store.go
+++ b/internal/docs/store.go
@@ -5,6 +5,7 @@ import (
crand "crypto/rand"
"errors"
"fmt"
+ "log/slog"
mrand "math/rand/v2"
"strings"
"time"
@@ -21,6 +22,17 @@ import (
type Store struct {
db *gorm.DB
core *core.Runtime
+
+ // sampleDocuments mirrors Config.SampleDocuments, already parsed —
+ // see EnsurePerson's own doc comment for what it gates and why the
+ // gate lives here rather than at the HTTP layer.
+ sampleDocuments bool
+
+ // logger is where a sample-seeding failure goes — see
+ // seedIfEnabled's own doc comment for why that failure is logged
+ // and swallowed here rather than ever returned to EnsurePerson's
+ // caller.
+ logger *slog.Logger
}
// NewStore builds a Store over an already-migrated *gorm.DB and a
@@ -28,8 +40,20 @@ type Store struct {
// and is reused for every document (core.New's doc comment); each
// call into it below opens its own per-document instance (a
// *core.Handle) and closes it again.
-func NewStore(db *gorm.DB, rt *core.Runtime) *Store {
- return &Store{db: db, core: rt}
+//
+// sampleDocuments and logger back EnsurePerson's own per-person
+// sample seeding (see its doc comment): sampleDocuments is
+// Config.SampleDocuments already parsed by parseSampleDocuments
+// (app.go), and logger is where a seeding failure is reported rather
+// than propagated. A nil logger defaults to slog.Default(), the same
+// fallback New (app.go) applies to Config.Logger — this Store can be
+// built directly in a test (scope_test.go and others already do) with
+// no logger of its own to pass.
+func NewStore(db *gorm.DB, rt *core.Runtime, sampleDocuments bool, logger *slog.Logger) *Store {
+ if logger == nil {
+ logger = slog.Default()
+ }
+ return &Store{db: db, core: rt, sampleDocuments: sampleDocuments, logger: logger}
}
// sqidAlphabet avoids visually ambiguous characters (0/O, 1/l/I) —
@@ -132,6 +156,17 @@ func owned(g *gorm.DB, owner int64) *gorm.DB {
// a test can mint an owner id without going through HTTP at all — the
// two must and do agree, since TestPerson is this method and nothing
// else.
+//
+// The create branch below — and only the create branch — is also
+// where a brand-new person's sample documents get seeded
+// (seedIfEnabled), when this instance has opted in. That is
+// deliberate and is the whole reason this hangs off "a Person row was
+// just created" rather than off "List came back empty": a person who
+// deletes every one of their documents still has a Person row, so
+// their next sign-in takes the found branch just above and returns
+// early — never reaching seedIfEnabled — and gets nothing back.
+// Seeding is a one-time event in a person's history, tied to the one
+// moment that can only ever happen once: their row not existing yet.
func (s *Store) EnsurePerson(ctx context.Context, email string) (int64, error) {
email = strings.TrimSpace(strings.ToLower(email))
if email == "" {
@@ -152,7 +187,11 @@ func (s *Store) EnsurePerson(ctx context.Context, email string) (int64, error) {
// A concurrent first sign-in from the same brand-new address
// can race two inserts against the unique index on email; the
// loser looks up what the winner just created rather than
- // failing the request over a benign collision.
+ // failing the request over a benign collision. The loser never
+ // seeds: only the goroutine whose INSERT actually won reaches
+ // seedIfEnabled below, so two simultaneous first sign-ins for
+ // the same address still produce exactly one set of sample
+ // documents, not two.
if errors.Is(err, gorm.ErrDuplicatedKey) {
if err2 := s.db.WithContext(ctx).Where("email = ?", email).First(&p).Error; err2 == nil {
return p.ID, nil
@@ -160,9 +199,53 @@ func (s *Store) EnsurePerson(ctx context.Context, email string) (int64, error) {
}
return 0, fmt.Errorf("docs: create person %q: %w", email, err)
}
+
+ s.seedIfEnabled(ctx, p.ID, email)
return p.ID, nil
}
+// seedIfEnabled seeds newOwner's document list with CARLOS Docs' own
+// sample content, when this instance has opted in
+// (Config.SampleDocuments, threaded through as Store.sampleDocuments)
+// — called only from EnsurePerson's just-created-the-row branch above,
+// never from its found-an-existing-row branch, and never called twice
+// for the same person: EnsurePerson's own doc comment has the full
+// reasoning for why that hook point is the one that gives "seed once,
+// per person, ever" for free rather than needing its own bookkeeping.
+//
+// This runs after the Create above has already returned nil — not
+// wrapped in a transaction of its own, and deliberately not folded
+// into a transaction around Create either (there isn't one today:
+// that INSERT already autocommits on its own). Seed's own work below
+// is six Store.Create calls, six Store.ApplyUpdate calls — each of
+// which replays a document's update log and applies a fresh one
+// through internal/core's WebAssembly runtime, not a trivial
+// in-transaction operation — and six backdating UPDATEs. Holding all
+// of that, plus the person row's own INSERT, inside one atomic unit
+// would mean a slow seed holds whatever locks SQLite takes for the
+// duration, and would mean a seed failure could take the newly
+// created person row down with it on rollback. Neither is
+// acceptable: a person's own row must be durable the moment Create
+// above succeeds, regardless of what happens next.
+//
+// So the error boundary sits here, not in Seed and not in
+// EnsurePerson's return: a failure is logged, with the address and
+// the new person id for context, and then dropped on the floor.
+// EnsurePerson still returns (p.ID, nil) no matter what happens in
+// here. A person who cannot sign in because their sample content
+// failed to generate is a far worse outcome than a person who signs
+// in to an empty list — the second is this feature not having worked
+// yet; the first is this feature having broken sign-in, which is the
+// exact failure this whole plan exists to fix, not reintroduce.
+func (s *Store) seedIfEnabled(ctx context.Context, newOwner int64, email string) {
+ if !s.sampleDocuments {
+ return
+ }
+ if err := Seed(ctx, s, newOwner); err != nil {
+ s.logger.Error("docs: seed sample documents for new person", "person", newOwner, "address", email, "err", err)
+ }
+}
+
// Create makes a new document owned by owner: a GUID and a client id
// assigned here by the app (never drawn from yrs — see Document's doc
// comment) and stored, an app-generated sqid, and the given title. It