amadan
🤖 fix(notion): the code review's blocker, and six majors with it
Reviewed by claude-fable-5-1 before merge: REQUEST CHANGES, 12 findings,
one blocker.
The blocker was a correctness bug that would have shipped wrong data.
Roots() kept every search result whose parent was not itself in the
result set, on this design's own assumption that search returns only
directly-shared pages. Notion's reference says it returns "all parent or
child pages ... shared with a connection", and a page nested inside a
toggle has a block_id parent, which search never returns. So a
descendant routinely came back looking exactly like a root: it was
queued first, ingested with EMPTY ancestry, and marked visited, and the
correct "Handbook > Onboarding" Ref was then discarded when the
traversal reached it legitimately. On the next run search might include
the ancestor and the page would flap between two ancestries, re-ingesting
each time. The fake had a searchLeaks switch built for this case and no
test using it. Roots and orphans are separate now, and orphans are
walked only after the real traversal drains.
Two more were this codebase's own named failure shapes, in code written
to avoid them. The run counters were published only at completion, so a
thirty-minute import rendered "0 so far" throughout — a count maintained
nowhere, displayed as a confident zero, on the status page written to
honour that rule. And pageRow returned (nil, nil) on any error, so a
database failure was indistinguishable from "never seen this page": the
mark was skipped, the ledger under-reported, and nothing was logged.
The rest:
- The heartbeat counted pages, not time, so a slow-but-healthy run
could be declared stale and joined by a SECOND goroutine on the same
ledger. It is a ticker now.
- Database rows imported with no properties at all, and Properties()
was dead code. The design warned that without it a forty-row
database becomes forty blank documents; that is what shipped.
- Data sources sat in the ledger as pages that could never import, so
the page said "1 of 2 imported" forever. Containers are no longer
ledgered, and "imported" now counts rows memoria HOLDS a document
for rather than rows whose last fetch succeeded — a page that 404s
after importing was dropping out of its own total while the document
was still there and still searchable.
- The production-seam gate greped only for AppForTest, so app.AsPerson
in main would have compiled and made every request an owner.
- Retry-After was uncapped and ignored the context: one hostile value
parked a run for a day, which also got it reclaimed as stale.
- The pagination cursor went into a query string unescaped.
- Database failures reported as "network", so the page said "We could
not reach Notion" about a local write.
- Two gate comments claimed more than their gates tested: a canary in
a response body that did not exist, and an assertion on an
unconditional label that could not fail.
Seven new mutations, all verified red. One of them — a page failing
after it had already imported — went red against the FIX as well as the
bug, which is how the incomplete half of that fix was found: preserving
the columns was not enough while the count filtered on state.
Thirty-seven mutations across four suites now.
§15's ship requirement is still unmet and this does not change it: no
run against a real workspace has happened, and the blocker above is
exactly what that requirement exists to catch — found by a reviewer
reading Notion's documentation, not by a test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VauJkQsPeGRErbWFiQNZzC
14 files changed,
+686
−102
docs/superpowers/specs/2026-09-05-notion-ingest-design.md+66 −0internal/memoria/connector.go+8 −2internal/memoria/connector_models.go+5 −2internal/memoria/handlers_connector.go+5 −0internal/memoria/notion/client.go+41 −9internal/memoria/notion/client_test.go+107 −5internal/memoria/notion/fake_test.go+38 −17internal/memoria/notion/traverse.go+76 −9internal/memoria/sync.go+190 −45internal/memoriatest/connector_test.go+19 −9internal/memoriatest/container_test.go+51 −0internal/memoriatest/gates_test.go+10 −2internal/memoriatest/regress_test.go+68 −0tools/drive/main.go+2 −2
diff --git a/docs/superpowers/specs/2026-09-05-notion-ingest-design.md b/docs/superpowers/specs/2026-09-05-notion-ingest-design.md| index e62b6b1..259e6f6 100644 |
| --- a/docs/superpowers/specs/2026-09-05-notion-ingest-design.md |
| +++ b/docs/superpowers/specs/2026-09-05-notion-ingest-design.md |
| @@ -162,6 +162,72 @@ which put a rare housekeeping act between the reader and the thing the |
| page is for. |
| +### 1.3 What the Fable code review changed |
| + |
| +The branch was reviewed by `claude-fable-5-1` before merge — REQUEST |
| +CHANGES, 12 findings, one blocker. Recorded here, as §1.1 and §1.2 are. |
| + |
| +- **Nested pages were ingested as roots with no ancestry (blocker).** |
| + `Roots` kept every search result whose parent was not itself in the |
| + result set, on the assumption — taken from this design, and wrong — |
| + that search returns only directly-shared pages. Notion's reference says |
| + it returns "all parent or child pages ... shared with a connection", |
| + and a page nested in a toggle has a `block_id` parent, which search |
| + never returns. Such a page therefore always looked like a root, was |
| + queued first, was ingested with an EMPTY path and marked visited, and |
| + the correct Ref was discarded when the traversal reached it. On the |
| + next run search might include the ancestor and the page would flap |
| + between two ancestries, re-ingesting each time. The fake had a |
| + `searchLeaks` switch built for exactly this and no test using it. Roots |
| + and orphans are now separate, and orphans are walked only after the |
| + real traversal drains. |
| +- **A page was not findable by its own title.** A Notion title is a |
| + property, not a block, so it reached none of heading, body or path. |
| + Fixed by the merge with main, which had already found the same bug for |
| + pasted documents and added a title column. |
| +- **The heartbeat was counted in pages, not time**, so a slow-but-healthy |
| + run could be declared stale, "reclaimed", and joined by a second |
| + goroutine walking the same ledger. It is a 20-second ticker now. |
| +- **The status page said "0 so far" for a whole run**, because the |
| + counters were written only at completion — a count maintained nowhere, |
| + rendered as a confident zero, on the page written to honour that rule. |
| + The ticker publishes them. |
| +- **Database rows imported with no properties**, and `Properties` was |
| + dead code. §5 promised the key/value block and warned that without it |
| + "a forty-row database becomes forty blank documents"; that is precisely |
| + what shipped. |
| +- **Data sources sat in the ledger as pages that never import**, so the |
| + page read "1 of 2 imported" forever. Containers are no longer |
| + ledgered — and "imported" now counts rows memoria HOLDS A DOCUMENT for, |
| + not rows whose last fetch happened to succeed, so a page that 404s |
| + after importing no longer drops out of its own total. |
| +- **Ledger writes failed silently.** `pageRow` returned `(nil, nil)` on |
| + any error, so a database failure was indistinguishable from "never seen |
| + this page" and the mark was skipped with nothing logged. And a failure |
| + on a page memoria already held wiped its `document_id`, erasing the |
| + record of a document that was still there. |
| +- Smaller: the production-seam gate greped only for `AppForTest`, so |
| + `AsPerson` in main would have compiled and made every request an owner; |
| + `Retry-After` was uncapped and ignored the context, so one hostile |
| + value parked a run for a day; the pagination cursor went into a query |
| + string unescaped; database failures reported as `network`, so the page |
| + said "We could not reach Notion" about a local write; and two gate |
| + comments claimed more than their gates tested. |
| + |
| +### 1.4 Sequencing, as it actually went |
| + |
| +§14 says stage 1 lands and is reviewed on its own. It did not: all three |
| +stages landed on one branch and were reviewed together. Recorded rather |
| +than quietly diverged from — the cost was a larger review than the design |
| +intended, and the review found a blocker that a stage-2-only review would |
| +have found sooner. |
| + |
| +**§15's ship requirement is still unmet.** No run against a real Notion |
| +workspace has happened. Finding 1 above is exactly the failure §15 |
| +predicts from "a connector verified only against fixtures its author also |
| +wrote": it was found by a reviewer reading Notion's documentation, not by |
| +a test. That requirement stands. |
| + |
| --- |
| ## 2. Why this is unblocked, when the parent sequenced it fourth |
diff --git a/internal/memoria/connector.go b/internal/memoria/connector.go| index e3913e3..ad01aec 100644 |
| --- a/internal/memoria/connector.go |
| +++ b/internal/memoria/connector.go |
| @@ -349,9 +349,15 @@ func (s *Store) CountPages(ctx context.Context, connectorID int64) (total, fetch |
| if err := q.Count(&t).Error; err != nil { |
| return 0, 0, err |
| } |
| + // "Imported" means memoria HOLDS a document for this row, which is |
| + // what document_id records — not what state the last fetch ended in. |
| + // |
| + // Counting states instead made a page that imported fine and then |
| + // 404'd on a later run drop out of the total, though the document was |
| + // still there and still searchable. memoria does not delete on |
| + // absence; the count must not imply that it did. |
| if err := s.db.G.WithContext(ctx).Model(&SourcePage{}). |
| - Where("connector_id = ? AND state IN ?", connectorID, |
| - []string{PageFetched, PageEmpty}).Count(&f).Error; err != nil { |
| + Where("connector_id = ? AND document_id > 0", connectorID).Count(&f).Error; err != nil { |
| return 0, 0, err |
| } |
| return int(t), int(f), nil |
diff --git a/internal/memoria/connector_models.go b/internal/memoria/connector_models.go| index c15b0d2..80ca42a 100644 |
| --- a/internal/memoria/connector_models.go |
| +++ b/internal/memoria/connector_models.go |
| @@ -65,8 +65,11 @@ type SyncRun struct { |
| type SourcePage struct { |
| ID int64 |
| ConnectorID int64 `gorm:"index;not null"` |
| - // PagePrint is the keyed page id — lookup, and what joins to a |
| - // Document's ExternalID. |
| + // PagePrint is the keyed page id, used to find this row. |
| + // |
| + // It does NOT join to a Document's ExternalID, though a comment here |
| + // used to say so: both are HMACs, under different purposes, of |
| + // different inputs. The link to the document is DocumentID below. |
| PagePrint string `gorm:"index;not null"` |
| // SealedPageID is the id itself, sealed and REVERSIBLE. |
| // |
diff --git a/internal/memoria/handlers_connector.go b/internal/memoria/handlers_connector.go| index c560232..06a9055 100644 |
| --- a/internal/memoria/handlers_connector.go |
| +++ b/internal/memoria/handlers_connector.go |
| @@ -82,6 +82,11 @@ func failMessage(code string) string { |
| return "The import could not read all of your Notion, so it stopped rather than finish with pages missing. Nothing was lost." |
| case "network": |
| return "We could not reach Notion. Nothing was lost — start the import again." |
| + case "database": |
| + // A failure on OUR side, not Notion's. It used to report as |
| + // "network", so the page said "We could not reach Notion" about a |
| + // database write that never left the building. |
| + return "Something went wrong on our side and the import stopped. Nothing was lost — start it again." |
| case "interrupted": |
| return "The import stopped before it finished. Start it again to carry on." |
| } |
diff --git a/internal/memoria/notion/client.go b/internal/memoria/notion/client.go| index ccbfced..955581c 100644 |
| --- a/internal/memoria/notion/client.go |
| +++ b/internal/memoria/notion/client.go |
| @@ -53,6 +53,16 @@ const defaultInterval = 350 * time.Millisecond |
| // pages. |
| const retryBudget = 5 |
| +// maxRetryAfter caps how long we will honour a Retry-After. |
| +// |
| +// Notion sends it in seconds and nothing bounds what it may ask for. An |
| +// uncapped honour of "Retry-After: 86400" parks the run for a day, and |
| +// because the goroutine is also what proves the run is alive, a wait |
| +// past the staleness window gets the run reclaimed and a SECOND one |
| +// started on the same ledger. Past this we give up and say so, which is |
| +// recoverable; sleeping for a day is not. |
| +const maxRetryAfter = 2 * time.Minute |
| + |
| // Code is a closed set of error codes. It is a code and never a message |
| // because Notion's own error bodies carry content: a 404 reads "Could |
| // not find database with ID: be907abe-… shared with your connection "My |
| @@ -248,7 +258,9 @@ func (c *Client) call(ctx context.Context, method, path string, body, out any) e |
| } |
| var ne net.Error |
| if attempt < retryBudget && errors.As(err, &ne) { |
| - c.backoff(attempt, 0) |
| + if !c.backoff(ctx, attempt, 0) { |
| + return &Error{Code: CodeNetwork} |
| + } |
| continue |
| } |
| return &Error{Code: CodeNetwork} |
| @@ -258,7 +270,9 @@ func (c *Client) call(ctx context.Context, method, path string, body, out any) e |
| if retryable { |
| res.Body.Close() |
| if attempt < retryBudget { |
| - c.backoff(attempt, retryAfter) |
| + if !c.backoff(ctx, attempt, retryAfter) { |
| + return &Error{Code: CodeNetwork} |
| + } |
| continue |
| } |
| // Budget exhausted. Return the error — never the partial |
| @@ -310,7 +324,11 @@ func classify(status int, retryAfter string) (bool, time.Duration) { |
| switch status { |
| case http.StatusTooManyRequests, 529: |
| if s, err := strconv.Atoi(retryAfter); err == nil && s >= 0 { |
| - return true, time.Duration(s) * time.Second |
| + d := time.Duration(s) * time.Second |
| + if d > maxRetryAfter { |
| + d = maxRetryAfter |
| + } |
| + return true, d |
| } |
| return true, 0 |
| case http.StatusInternalServerError, http.StatusBadGateway, |
| @@ -320,13 +338,27 @@ func classify(status int, retryAfter string) (bool, time.Duration) { |
| return false, 0 |
| } |
| -func (c *Client) backoff(attempt int, after time.Duration) { |
| +// backoff waits, and stops waiting if the context is done. |
| +// |
| +// c.sleep used to be a bare time.Sleep, so a cancelled run kept sleeping |
| +// — for up to maxRetryAfter, per attempt, per page. |
| +func (c *Client) backoff(ctx context.Context, attempt int, after time.Duration) bool { |
| if c.interval <= 0 && after == 0 { |
| - return // tests: no real waiting |
| + return ctx.Err() == nil // tests: no real waiting |
| } |
| - if after > 0 { |
| - c.sleep(after) |
| - return |
| + d := after |
| + if d <= 0 { |
| + d = time.Duration(1<<attempt) * 200 * time.Millisecond |
| + } |
| + if d > maxRetryAfter { |
| + d = maxRetryAfter |
| + } |
| + t := time.NewTimer(d) |
| + defer t.Stop() |
| + select { |
| + case <-ctx.Done(): |
| + return false |
| + case <-t.C: |
| + return true |
| } |
| - c.sleep(time.Duration(1<<attempt) * 200 * time.Millisecond) |
| } |
diff --git a/internal/memoria/notion/client_test.go b/internal/memoria/notion/client_test.go| index 3e4c163..b0ef798 100644 |
| --- a/internal/memoria/notion/client_test.go |
| +++ b/internal/memoria/notion/client_test.go |
| @@ -162,7 +162,7 @@ func TestClientDialsOnlyNotion(t *testing.T) { |
| if c.base == srv.URL { |
| t.Fatal("could not build a different-host URL; this subtest proves nothing") |
| } |
| - if _, err := c.Roots(t.Context()); err == nil { |
| + if _, _, err := c.Roots(t.Context()); err == nil { |
| t.Fatal("the client called a host it was not pinned to") |
| } else if CodeOf(err) != CodeForbiddenHost { |
| t.Errorf("code %q, want %q", CodeOf(err), CodeForbiddenHost) |
| @@ -177,7 +177,7 @@ func TestClientDialsOnlyNotion(t *testing.T) { |
| t.Fatal(err) |
| } |
| c.base = strings.Replace(srv.URL, "http://", "https://", 1) |
| - if _, err := c.Roots(t.Context()); err == nil { |
| + if _, _, err := c.Roots(t.Context()); err == nil { |
| t.Fatal("the client accepted a scheme it was not pinned to") |
| } else if CodeOf(err) != CodeForbiddenHost { |
| t.Errorf("code %q, want %q", CodeOf(err), CodeForbiddenHost) |
| @@ -205,7 +205,7 @@ func TestClientDialsOnlyNotion(t *testing.T) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| - if _, err := c.Roots(t.Context()); err == nil { |
| + if _, _, err := c.Roots(t.Context()); err == nil { |
| t.Error("a redirect was followed; the bearer token travels with it") |
| } |
| if landed { |
| @@ -311,12 +311,15 @@ func TestDatabaseRowsBecomePagesUnderTheDatabase(t *testing.T) { |
| f.pages["root"].childDBs = []string{"db1"} |
| f.dbs["db1"] = []string{"ds1"} |
| f.dbParent["db1"] = "root" |
| - f.page("row1", "Project Bluebird", "") |
| + f.page("row1", "Project Bluebird", "").extra = map[string]any{ |
| + "Status": map[string]any{"type": "status", "status": map[string]any{"name": "Shipped"}}, |
| + "Owner": map[string]any{"type": "rich_text", "rich_text": []map[string]any{{"plain_text": "Ana"}}}, |
| + } |
| f.page("row2", "Project Kestrel", "") |
| f.roots = []string{"root"} // rows are reached through the database only |
| f.rows["ds1"] = []string{"row1", "row2"} |
| - refs, _, err := walkAll(t, client(t, f)) |
| + refs, content, err := walkAll(t, client(t, f)) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -333,6 +336,21 @@ func TestDatabaseRowsBecomePagesUnderTheDatabase(t *testing.T) { |
| t.Errorf("row %q ancestry is %q, want %q", title, got, "Workspace › Projects") |
| } |
| } |
| + |
| + // The row's COLUMNS are its content. A row's body is usually empty, |
| + // so importing rows without their properties produced blank |
| + // documents — the design warned about it and the first |
| + // implementation shipped it anyway, with the renderer written and |
| + // never called. |
| + // |
| + // MUTATION-TESTED 2026-09-05: dropping the Properties() prepend in |
| + // Fetch makes this fail. Reverted. |
| + md := content["row1"].Markdown |
| + for _, want := range []string{"Status: Shipped", "Owner: Ana"} { |
| + if !strings.Contains(md, want) { |
| + t.Errorf("row markdown missing %q\n---\n%s", want, md) |
| + } |
| + } |
| } |
| // TestTrashedPagesAreSkipped. `in_trash` replaced `archived` at API |
| @@ -351,3 +369,87 @@ func TestTrashedPagesAreSkipped(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// TestSearchLeakingADescendantDoesNotStripItsAncestry is the gate for the |
| +// blocker a code review found — and the fake had a switch built for this |
| +// case with no test using it. |
| +// |
| +// Notion's search returns "all parent or child pages ... shared with a |
| +// connection", not only the ones shared directly. And a page nested |
| +// inside a toggle has a `block_id` parent, which search never returns. So |
| +// a descendant routinely comes back from search with a parent that is not |
| +// in the result set, and looks exactly like a root. |
| +// |
| +// Treated as a root it is ingested with EMPTY ancestry and marked |
| +// visited, so the correct "Handbook › Onboarding" Ref is discarded when |
| +// the traversal reaches it. Wrong ancestry is then sealed, indexed and |
| +// shown, and it flaps on the next run when search changes its mind. |
| +// |
| +// MUTATION-TESTED 2026-09-05: classifying every non-present-parent result |
| +// as a root (the pre-fix behaviour) makes this fail with an empty path. |
| +// Reverted. |
| +func TestSearchLeakingADescendantDoesNotStripItsAncestry(t *testing.T) { |
| + f := nestedFake(t) |
| + f.searchLeaks = []string{"grandkid"} |
| + c := client(t, f) |
| + |
| + roots, orphans, err := c.Roots(t.Context()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, r := range roots { |
| + if r.ID == "grandkid" { |
| + t.Error("a nested page came back as a true root; it would be ingested with no ancestry") |
| + } |
| + } |
| + if len(orphans) != 1 || orphans[0].ID != "grandkid" { |
| + t.Fatalf("expected the leaked descendant classified as an orphan, got %d", len(orphans)) |
| + } |
| + |
| + refs, _, err := walkAll(t, c) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var got string |
| + var seen int |
| + for _, r := range refs { |
| + if r.ID == "grandkid" { |
| + seen++ |
| + got = strings.Join(r.Path, " › ") |
| + } |
| + } |
| + if seen != 1 { |
| + t.Errorf("the grandchild was walked %d times, want once", seen) |
| + } |
| + if got != "Handbook › Onboarding" { |
| + t.Errorf("grandchild ancestry is %q, want %q — the orphan claimed it first", |
| + got, "Handbook › Onboarding") |
| + } |
| +} |
| + |
| +// TestErrorsCarryNoResponseBody. Notion's error bodies contain content: a |
| +// real 404 reads "Could not find block with ID: <uuid>. Make sure the |
| +// relevant pages and databases are shared with your connection <name>." |
| +// A %w of that into a log line puts a coordinate and a workspace name |
| +// into storage nobody can grep. |
| +// |
| +// notion.Error therefore has no message field at all — the shape is the |
| +// guarantee, rather than the discipline of whoever formats it. |
| +// |
| +// MUTATION-TESTED 2026-09-05: adding a Body field to Error and filling it |
| +// from the response makes this fail. Reverted. |
| +func TestErrorsCarryNoResponseBody(t *testing.T) { |
| + f := nestedFake(t) |
| + f.notFound["kid"] = true |
| + c := client(t, f) |
| + |
| + _, _, err := walkAll(t, c) |
| + if err == nil { |
| + t.Fatal("expected the 404 to surface as an error") |
| + } |
| + for _, canary := range []string{"CANARYCONNECTION", "Could not find block"} { |
| + if strings.Contains(err.Error(), canary) { |
| + t.Errorf("the error string carries %q from the response body:\n%s", canary, err.Error()) |
| + } |
| + } |
| +} |
diff --git a/internal/memoria/notion/fake_test.go b/internal/memoria/notion/fake_test.go| index cd5fc93..a1d988e 100644 |
| --- a/internal/memoria/notion/fake_test.go |
| +++ b/internal/memoria/notion/fake_test.go |
| @@ -48,6 +48,11 @@ type fakePage struct { |
| blocks []map[string]any |
| children []string // child_page ids |
| childDBs []string // child_database ids |
| + // extra are properties beyond the title, as a database row carries. |
| + // A row's body is usually empty and these are the whole of what it |
| + // says, which is why importing rows without them produced blank |
| + // documents. |
| + extra map[string]any |
| } |
| func newFake(t *testing.T) *fakeNotion { |
| @@ -105,12 +110,20 @@ func (f *fakeNotion) object(id string) map[string]any { |
| "object": "page", "id": p.id, "in_trash": p.inTrash, |
| "last_edited_time": p.edited, |
| "parent": parentOf(p.parent), |
| - "properties": map[string]any{ |
| - "Name": map[string]any{"type": "title", "title": []map[string]any{{"plain_text": p.title}}}, |
| - }, |
| + "properties": properties(p), |
| } |
| } |
| +func properties(p *fakePage) map[string]any { |
| + out := map[string]any{ |
| + "Name": map[string]any{"type": "title", "title": []map[string]any{{"plain_text": p.title}}}, |
| + } |
| + for k, v := range p.extra { |
| + out[k] = v |
| + } |
| + return out |
| +} |
| + |
| func parentOf(id string) map[string]any { |
| if id == "" { |
| return map[string]any{"type": "workspace", "workspace": true} |
| @@ -288,28 +301,36 @@ func (f *fakeNotion) reject(w http.ResponseWriter, key string) bool { |
| // every page reached, in order. |
| func walkAll(t *testing.T, c *Client) ([]Ref, map[string]*Content, error) { |
| t.Helper() |
| - roots, err := c.Roots(t.Context()) |
| + roots, orphans, err := c.Roots(t.Context()) |
| if err != nil { |
| return nil, nil, err |
| } |
| var out []Ref |
| got := map[string]*Content{} |
| seen := map[string]bool{} |
| + // Two passes, exactly as the syncer does it: the real tree first, so |
| + // every page that has an ancestor is reached through it, and only |
| + // then the orphans, skipping anything already visited. Draining them |
| + // into one queue would let an orphan claim a page before the |
| + // traversal could give it real ancestry. |
| queue := append([]Ref{}, roots...) |
| - for len(queue) > 0 { |
| - ref := queue[0] |
| - queue = queue[1:] |
| - if seen[ref.ID] { |
| - continue |
| - } |
| - seen[ref.ID] = true |
| - content, err := c.Fetch(t.Context(), ref) |
| - if err != nil { |
| - return out, got, err |
| + for _, phase := range [][]Ref{queue, orphans} { |
| + q := append([]Ref{}, phase...) |
| + for len(q) > 0 { |
| + ref := q[0] |
| + q = q[1:] |
| + if seen[ref.ID] { |
| + continue |
| + } |
| + seen[ref.ID] = true |
| + content, err := c.Fetch(t.Context(), ref) |
| + if err != nil { |
| + return out, got, err |
| + } |
| + out = append(out, ref) |
| + got[ref.ID] = content |
| + q = append(q, content.Children...) |
| } |
| - out = append(out, ref) |
| - got[ref.ID] = content |
| - queue = append(queue, content.Children...) |
| } |
| return out, got, nil |
| } |
diff --git a/internal/memoria/notion/traverse.go b/internal/memoria/notion/traverse.go| index 1ed5cd5..12743dc 100644 |
| --- a/internal/memoria/notion/traverse.go |
| +++ b/internal/memoria/notion/traverse.go |
| @@ -3,6 +3,7 @@ package notion |
| import ( |
| "context" |
| "encoding/json" |
| + "net/url" |
| "strings" |
| ) |
| @@ -22,6 +23,13 @@ type Ref struct { |
| // DataSourceID is set when Kind is KindDataSource: the thing to |
| // query for rows. |
| DataSourceID string |
| + // Properties is a database row's raw property map. CONTENT — never |
| + // logged — and carried here because a row's body is usually empty: |
| + // the information is in the columns. Without it a forty-row database |
| + // becomes forty blank documents, which the design warned about and |
| + // the first implementation shipped anyway, with the renderer written |
| + // and never called. |
| + Properties json.RawMessage |
| } |
| type Kind string |
| @@ -120,9 +128,12 @@ func (c *Client) paginate(ctx context.Context, method, path string, body map[str |
| } |
| err = c.post(ctx, path, b, &page) |
| } else { |
| + // Escaped: the cursor is an opaque string from a response, |
| + // and splicing it raw into a query string is how a stray & or |
| + // # silently truncates the request. |
| p := path + "?page_size=100" |
| if cursor != "" { |
| - p += "&start_cursor=" + cursor |
| + p += "&start_cursor=" + url.QueryEscape(cursor) |
| } |
| err = c.get(ctx, p, &page) |
| } |
| @@ -168,9 +179,9 @@ func (c *Client) paginate(ctx context.Context, method, path string, body map[str |
| // parent is absent IS a root, even if it is nested in Notion — we cannot |
| // see above it, so its ancestry starts here and the status page says how |
| // many roots were found. |
| -func (c *Client) Roots(ctx context.Context) ([]Ref, error) { |
| +func (c *Client) Roots(ctx context.Context) (roots, orphans []Ref, err error) { |
| var objs []object |
| - err := c.paginate(ctx, "POST", "/v1/search", nil, func(raw json.RawMessage) error { |
| + err = c.paginate(ctx, "POST", "/v1/search", nil, func(raw json.RawMessage) error { |
| var o object |
| if err := json.Unmarshal(raw, &o); err != nil { |
| return &Error{Code: CodeTraversalFailed} |
| @@ -179,7 +190,7 @@ func (c *Client) Roots(ctx context.Context) ([]Ref, error) { |
| return nil |
| }) |
| if err != nil { |
| - return nil, err |
| + return nil, nil, err |
| } |
| present := make(map[string]bool, len(objs)) |
| @@ -187,13 +198,12 @@ func (c *Client) Roots(ctx context.Context) ([]Ref, error) { |
| present[o.ID] = true |
| } |
| - var roots []Ref |
| for _, o := range objs { |
| if o.InTrash { |
| continue |
| } |
| if pid := o.Parent.id(); pid != "" && present[pid] { |
| - continue // reachable by traversal, with ancestry |
| + continue // reachable by traversal, with its ancestry known |
| } |
| r := Ref{ID: o.ID, EditedTime: o.LastEdited, Kind: KindPage} |
| switch o.Object { |
| @@ -211,9 +221,37 @@ func (c *Client) Roots(ctx context.Context) ([]Ref, error) { |
| if r.Title == "" { |
| r.Title = "Untitled" |
| } |
| - roots = append(roots, r) |
| + // A result whose parent is the WORKSPACE is a true root: nothing |
| + // sits above it and its ancestry genuinely starts here. |
| + // |
| + // Anything else reaching this point is an ORPHAN — its parent |
| + // exists but search did not return it. Two ways that happens, and |
| + // both are ordinary rather than exotic: a page nested inside a |
| + // toggle or a bulleted item has a `block_id` parent, and search |
| + // never returns blocks, so `present[parent]` is always false for |
| + // it; and search's index is eventually consistent, so a middle |
| + // ancestor can simply be missing from one response. |
| + // |
| + // Treating an orphan as a root is how a nested page gets ingested |
| + // with EMPTY ancestry. Worse, it used to win: roots were queued |
| + // first, so the orphan was visited and sealed with no path before |
| + // the traversal reached it legitimately, and the correct |
| + // `Handbook › Onboarding` Ref was then discarded as already |
| + // visited. On the next run search might include the ancestor, the |
| + // path print would change, and the page would flap between two |
| + // ancestries with a re-ingest each time. |
| + // |
| + // So orphans come back separately and the caller walks them only |
| + // after the real traversal has drained. Found by a code review; |
| + // the fake had a searchLeaks switch built for exactly this case |
| + // and no test using it. |
| + if o.Parent.Type == "workspace" || o.Parent.Workspace { |
| + roots = append(roots, r) |
| + } else { |
| + orphans = append(orphans, r) |
| + } |
| } |
| - return roots, nil |
| + return roots, orphans, nil |
| } |
| // titleOf pulls a page's title out of its properties. The title property |
| @@ -262,9 +300,30 @@ func (c *Client) Fetch(ctx context.Context, ref Ref) (*Content, error) { |
| } |
| childPath := append(append([]string{}, ref.Path...), ref.Title) |
| md, kids, unread := render(blocks, childPath) |
| + // A database row's columns lead its body. They are usually the whole |
| + // of what the row says. |
| + if props := Properties(ref.Properties); props != "" { |
| + if md == "" { |
| + md = props |
| + } else { |
| + md = props + "\n\n" + md |
| + } |
| + } |
| return &Content{Markdown: md, Children: kids, UnreadBlocks: unread}, nil |
| } |
| +// firstProperty returns the first non-empty rendered property value, for |
| +// naming a row whose title property is blank. |
| +func firstProperty(raw json.RawMessage) string { |
| + rendered := Properties(raw) |
| + for _, line := range strings.Split(rendered, "\n") { |
| + if _, v, ok := strings.Cut(line, ": "); ok && v != "" { |
| + return v |
| + } |
| + } |
| + return "" |
| +} |
| + |
| // fetchDataSource turns a database into its rows. |
| // |
| // The database itself is not a document — it is a container — so its |
| @@ -292,11 +351,19 @@ func (c *Client) fetchDataSource(ctx context.Context, ref Ref) (*Content, error) |
| } |
| title := titleOf(o) |
| if title == "" { |
| - title = "Untitled" |
| + // A row with an empty title property. firstLine() over a |
| + // property block would title every such document |
| + // "Status: Shipped", so name it for the collection it is |
| + // in plus its first non-empty value. |
| + title = firstNonEmpty(ref.Title+" row", "Untitled") |
| + if v := firstProperty(o.Properties); v != "" { |
| + title = ref.Title + ": " + v |
| + } |
| } |
| kids = append(kids, Ref{ |
| ID: o.ID, Title: title, Path: childPath, |
| EditedTime: o.LastEdited, Kind: KindPage, |
| + Properties: o.Properties, |
| }) |
| return nil |
| }) |
diff --git a/internal/memoria/sync.go b/internal/memoria/sync.go| index 620039c..f3ffd69 100644 |
| --- a/internal/memoria/sync.go |
| +++ b/internal/memoria/sync.go |
| @@ -4,8 +4,11 @@ import ( |
| "context" |
| "errors" |
| "log/slog" |
| + "sync" |
| "time" |
| + "gorm.io/gorm" |
| + |
| "amadan.net/oficina/memoria/internal/memoria/chunk" |
| "amadan.net/oficina/memoria/internal/memoria/notion" |
| "amadan.net/oficina/memoria/internal/memoria/seal" |
| @@ -15,7 +18,9 @@ import ( |
| // loop can be driven against a fake without the app package importing an |
| // HTTP client — which the network gate forbids it anyway. |
| type SourceClient interface { |
| - Roots(ctx context.Context) ([]notion.Ref, error) |
| + // Roots returns the true roots and, separately, the orphans — |
| + // results whose parent search did not return. See notion.Roots. |
| + Roots(ctx context.Context) (roots, orphans []notion.Ref, err error) |
| Fetch(ctx context.Context, ref notion.Ref) (*notion.Content, error) |
| } |
| @@ -34,9 +39,6 @@ func NewSyncer(store *Store, logger *slog.Logger) *Syncer { |
| }} |
| } |
| -// beatEvery is how often the run says it is still alive. |
| -const beatEvery = 25 |
| - |
| // Run imports everything reachable from the connector's roots. |
| // |
| // ONE PASS, not the two phases an earlier design specified. With |
| @@ -72,9 +74,68 @@ func (s *Syncer) Run(ctx context.Context, connectorID, runID int64) { |
| s.RunWith(ctx, client, connectorID, runID) |
| } |
| +// runStats is the live count of what a run has done. |
| +// |
| +// Guarded, because the heartbeat goroutine reads it while the walk |
| +// writes it — and it is read for a reason: an earlier version wrote the |
| +// counters only at the very end, so a thirty-minute import rendered |
| +// "0 so far" for its whole duration. A count maintained nowhere and |
| +// displayed as zero is this codebase's second broken rule, on the status |
| +// page written to honour it. |
| +type runStats struct { |
| + mu sync.Mutex |
| + fetched, skipped, empty, failed, unread, seen int |
| +} |
| + |
| +func (r *runStats) add(f func(*runStats)) { |
| + r.mu.Lock() |
| + defer r.mu.Unlock() |
| + f(r) |
| +} |
| + |
| +func (r *runStats) snapshot() map[string]any { |
| + r.mu.Lock() |
| + defer r.mu.Unlock() |
| + return map[string]any{ |
| + "fetched": r.fetched, "skipped": r.skipped, "empty": r.empty, |
| + "failed_pages": r.failed, "unread_blocks": r.unread, |
| + } |
| +} |
| + |
| +// beatEvery is how often a run says it is still alive AND publishes its |
| +// counters. A DURATION, not a page count: pages take wildly different |
| +// times — one request per nested block, each paced at 350ms, plus an |
| +// honoured Retry-After — so "every 25 pages" could exceed the staleness |
| +// window while the run was perfectly healthy. StartRun would then reclaim |
| +// a LIVE run and start a second goroutine on the same ledger. Found by a |
| +// code review. |
| +const beatEvery = 20 * time.Second |
| + |
| +// startBeat publishes the heartbeat and the counters until stopped. |
| +func (s *Syncer) startBeat(ctx context.Context, runID int64, st *runStats) func() { |
| + done := make(chan struct{}) |
| + go func() { |
| + t := time.NewTicker(beatEvery) |
| + defer t.Stop() |
| + for { |
| + select { |
| + case <-done: |
| + return |
| + case <-t.C: |
| + patch := st.snapshot() |
| + patch["heartbeat"] = time.Now().UTC() |
| + if err := s.store.UpdateRun(ctx, runID, patch); err != nil { |
| + s.logger.Error("sync heartbeat", "run", runID, "err", err) |
| + } |
| + } |
| + } |
| + }() |
| + return func() { close(done) } |
| +} |
| + |
| // RunWith is Run against a supplied client. |
| func (s *Syncer) RunWith(ctx context.Context, client SourceClient, connectorID, runID int64) { |
| - roots, err := client.Roots(ctx) |
| + roots, orphans, err := client.Roots(ctx) |
| if err != nil { |
| s.fail(ctx, runID, string(notion.CodeOf(err))) |
| return |
| @@ -82,14 +143,54 @@ func (s *Syncer) RunWith(ctx context.Context, client SourceClient, connectorID, |
| if err := s.store.UpdateRun(ctx, runID, map[string]any{ |
| "roots": len(roots), "state": RunFetching, |
| }); err != nil { |
| - s.fail(ctx, runID, "network") |
| + s.fail(ctx, runID, "database") |
| return |
| } |
| - var stats struct{ fetched, skipped, empty, failed, unread, seen int } |
| + st := &runStats{} |
| + stop := s.startBeat(ctx, runID, st) |
| + defer stop() |
| + |
| visited := map[string]bool{} |
| - queue := append([]notion.Ref{}, roots...) |
| + // The real tree first, so every page that HAS an ancestor is reached |
| + // through it and inherits the right path. |
| + if !s.walk(ctx, client, connectorID, runID, roots, visited, st) { |
| + return |
| + } |
| + // Then the orphans — results whose parent search did not return, most |
| + // often a page nested inside a toggle, whose parent is a block. They |
| + // go last and skip anything already visited, so an orphan can never |
| + // claim a page the traversal would have given real ancestry to. |
| + if !s.walk(ctx, client, connectorID, runID, orphans, visited, st) { |
| + return |
| + } |
| + |
| + now := time.Now().UTC() |
| + patch := st.snapshot() |
| + st.mu.Lock() |
| + patch["total"] = st.seen |
| + st.mu.Unlock() |
| + patch["state"] = RunDone |
| + patch["finished_at"] = now |
| + patch["heartbeat"] = now |
| + if err := s.store.UpdateRun(ctx, runID, patch); err != nil { |
| + s.logger.Error("sync finish", "run", runID, "err", err) |
| + } |
| + // Counts only. No title, no path, no page id — AGENTS.md's rule, and |
| + // the reason this line carries nothing that came from the workspace. |
| + s.logger.Info("sync finished", "run", runID, "pages", patch["total"], |
| + "fetched", patch["fetched"], "skipped", patch["skipped"], |
| + "empty", patch["empty"], "failed", patch["failed_pages"], |
| + "unread_blocks", patch["unread_blocks"]) |
| +} |
| + |
| +// walk breadth-first from seeds. It reports false when the run has |
| +// failed, having already marked it. |
| +func (s *Syncer) walk(ctx context.Context, client SourceClient, connectorID, runID int64, |
| + seeds []notion.Ref, visited map[string]bool, st *runStats) bool { |
| + |
| + queue := append([]notion.Ref{}, seeds...) |
| for len(queue) > 0 { |
| ref := queue[0] |
| queue = queue[1:] |
| @@ -97,67 +198,62 @@ func (s *Syncer) RunWith(ctx context.Context, client SourceClient, connectorID, |
| continue |
| } |
| visited[ref.ID] = true |
| - stats.seen++ |
| + st.add(func(r *runStats) { r.seen++ }) |
| - if stats.seen%beatEvery == 0 { |
| - if err := s.store.Beat(ctx, runID); err != nil { |
| - s.logger.Error("sync heartbeat", "run", runID, "err", err) |
| + // A data source is a CONTAINER, not a document. It gets no ledger |
| + // row: one there would sit "pending" forever, be counted in the |
| + // total, and be counted as "already up to date" on every later |
| + // run — so the page said "1 of 2 imported" permanently. |
| + container := ref.Kind == notion.KindDataSource |
| + if !container { |
| + // Record the coordinate BEFORE fetching, so a page that then |
| + // fails to read has somewhere to be named rather than being |
| + // absent. |
| + if err := s.store.SeeSourcePage(ctx, connectorID, ref.ID, ref.Title, ref.Path, ref.EditedTime); err != nil { |
| + s.logger.Error("sync ledger write", "run", runID, "err", err) |
| + s.fail(ctx, runID, "database") |
| + return false |
| } |
| } |
| - // Record the coordinate BEFORE fetching, so a page that fails to |
| - // fetch still has a row to be named in rather than being absent. |
| - if err := s.store.SeeSourcePage(ctx, connectorID, ref.ID, ref.Title, ref.Path, ref.EditedTime); err != nil { |
| - s.logger.Error("sync ledger write", "run", runID, "err", err) |
| - s.fail(ctx, runID, "network") |
| - return |
| - } |
| - |
| content, err := client.Fetch(ctx, ref) |
| if err != nil { |
| if gone(err) { |
| - stats.failed++ |
| - s.markByPage(ctx, connectorID, ref, PageFailed, string(notion.CodeOf(err)), 0, "") |
| + st.add(func(r *runStats) { r.failed++ }) |
| + if !container { |
| + s.markGone(ctx, connectorID, ref, string(notion.CodeOf(err))) |
| + } |
| continue |
| } |
| // Not survivable: the subtree below here was never seen. |
| s.fail(ctx, runID, string(notion.CodeOf(err))) |
| - return |
| + return false |
| } |
| queue = append(queue, content.Children...) |
| - stats.unread += content.UnreadBlocks |
| + st.add(func(r *runStats) { r.unread += content.UnreadBlocks }) |
| + |
| + if container { |
| + continue |
| + } |
| state, docID, err := s.ingest(ctx, connectorID, ref, content) |
| if err != nil { |
| - stats.failed++ |
| - s.markByPage(ctx, connectorID, ref, PageFailed, "extract_failed", 0, "") |
| + st.add(func(r *runStats) { r.failed++ }) |
| + s.markGone(ctx, connectorID, ref, "extract_failed") |
| continue |
| } |
| switch state { |
| case PageEmpty: |
| - stats.empty++ |
| + st.add(func(r *runStats) { r.empty++ }) |
| case "": |
| - stats.skipped++ |
| + st.add(func(r *runStats) { r.skipped++ }) |
| continue |
| default: |
| - stats.fetched++ |
| + st.add(func(r *runStats) { r.fetched++ }) |
| } |
| s.markByPage(ctx, connectorID, ref, state, "", docID, ref.EditedTime) |
| } |
| - |
| - now := time.Now().UTC() |
| - if err := s.store.UpdateRun(ctx, runID, map[string]any{ |
| - "state": RunDone, "total": stats.seen, "fetched": stats.fetched, |
| - "skipped": stats.skipped, "empty": stats.empty, "failed_pages": stats.failed, |
| - "unread_blocks": stats.unread, "finished_at": now, "heartbeat": now, |
| - }); err != nil { |
| - s.logger.Error("sync finish", "run", runID, "err", err) |
| - } |
| - // Counts only. No title, no path, no page id — AGENTS.md's rule, and |
| - // the reason this line carries nothing that came from the workspace. |
| - s.logger.Info("sync finished", "run", runID, "pages", stats.seen, |
| - "fetched", stats.fetched, "skipped", stats.skipped, |
| - "empty", stats.empty, "failed", stats.failed, "unread_blocks", stats.unread) |
| + return true |
| } |
| // ingest stores one page, or reports that it did not need storing. |
| @@ -186,6 +282,18 @@ func (s *Syncer) ingest(ctx context.Context, connectorID int64, ref notion.Ref, |
| AllowTitleOnly: true, |
| }) |
| if err != nil { |
| + // ErrIndexLag means the row COMMITTED and only the in-memory |
| + // index is behind. The document exists, is in the list, and comes |
| + // back at the next rebuild, which is every boot. Recording that |
| + // as a failure — and wiping the id — would be telling someone a |
| + // document is missing when it is right there. |
| + if errors.Is(err, ErrIndexLag) { |
| + s.logger.Error("index refresh failed after commit", "source", "notion", "err", err) |
| + if content.Markdown == "" { |
| + return PageEmpty, id, nil |
| + } |
| + return PageFetched, id, nil |
| + } |
| return "", 0, err |
| } |
| if content.Markdown == "" { |
| @@ -219,13 +327,42 @@ func (s *Syncer) upToDate(ctx context.Context, connectorID int64, ref notion.Ref |
| return rows.IngestedPath == want, nil |
| } |
| +// markGone records a failure WITHOUT erasing what memoria already holds. |
| +// |
| +// An earlier version routed failures through markByPage with a zero |
| +// document id and an empty edit time, so a page that had imported fine |
| +// and then 404'd on a later run had its record wiped: "Imported" dropped, |
| +// and the next run had to fetch and re-ingest a document that was never |
| +// gone. Found by a code review. |
| +func (s *Syncer) markGone(ctx context.Context, connectorID int64, ref notion.Ref, code string) { |
| + row, err := s.store.pageRow(ctx, connectorID, ref.ID) |
| + if err != nil { |
| + s.logger.Error("sync read ledger row", "err", err) |
| + return |
| + } |
| + if row == nil { |
| + return |
| + } |
| + // Keep document_id, ingested_edited and ingested_path exactly as they |
| + // were; only the state and the code move. |
| + if err := s.store.MarkPage(ctx, row.ID, PageFailed, code, |
| + row.DocumentID, row.IngestedEdited, row.IngestedPath); err != nil { |
| + s.logger.Error("sync mark page", "err", err) |
| + } |
| +} |
| + |
| func (s *Syncer) markByPage(ctx context.Context, connectorID int64, ref notion.Ref, state, code string, docID int64, edited string) { |
| row, err := s.store.pageRow(ctx, connectorID, ref.ID) |
| - if err != nil || row == nil { |
| + if err != nil { |
| + s.logger.Error("sync read ledger row", "err", err) |
| + return |
| + } |
| + if row == nil { |
| return |
| } |
| print, err := s.store.pathPrint(ref.Path, ref.Title) |
| if err != nil { |
| + s.logger.Error("sync path print", "err", err) |
| return |
| } |
| if edited == "" { |
| @@ -268,7 +405,15 @@ func (s *Store) pageRow(ctx context.Context, connectorID int64, pageID string) ( |
| var row SourcePage |
| res := s.db.G.WithContext(ctx).Where("connector_id = ? AND page_print = ?", connectorID, print).First(&row) |
| if res.Error != nil { |
| - return nil, nil |
| + // NOT-FOUND is not an error; anything else is. This returned |
| + // (nil, nil) for both, so a database failure looked exactly like |
| + // "memoria has never seen this page" — the caller then skipped |
| + // its mark silently and the ledger under-reported with nothing |
| + // logged. The plausible-empty-result shape, in a helper. |
| + if errors.Is(res.Error, gorm.ErrRecordNotFound) { |
| + return nil, nil |
| + } |
| + return nil, res.Error |
| } |
| return &row, nil |
| } |
diff --git a/internal/memoriatest/connector_test.go b/internal/memoriatest/connector_test.go| index ca95afe..180b4d3 100644 |
| --- a/internal/memoriatest/connector_test.go |
| +++ b/internal/memoriatest/connector_test.go |
| @@ -37,6 +37,10 @@ type fakeSource struct { |
| unread map[string]int |
| failWith map[string]error |
| fetches map[string]int |
| + // orphans are results search returned whose parent it did not — a |
| + // page inside a toggle, or a middle ancestor missing from an |
| + // eventually-consistent index. They must not be treated as roots. |
| + orphans []notion.Ref |
| } |
| func newSource() *fakeSource { |
| @@ -46,11 +50,11 @@ func newSource() *fakeSource { |
| } |
| } |
| -func (f *fakeSource) Roots(ctx context.Context) ([]notion.Ref, error) { |
| - if err := f.failWith["__roots__"]; err != nil { |
| - return nil, err |
| +func (f *fakeSource) Roots(ctx context.Context) (roots, orphans []notion.Ref, err error) { |
| + if e := f.failWith["__roots__"]; e != nil { |
| + return nil, nil, e |
| } |
| - return f.roots, nil |
| + return f.roots, f.orphans, nil |
| } |
| func (f *fakeSource) Fetch(ctx context.Context, ref notion.Ref) (*notion.Content, error) { |
| @@ -228,9 +232,12 @@ func TestAFailedPageIsNamedNotAbsent(t *testing.T) { |
| if run.FailedPages != 1 { |
| t.Errorf("failed_pages %d, want 1 — the page vanished from the totals", run.FailedPages) |
| } |
| + // The label is unconditional markup, so asserting on it proves |
| + // nothing — an earlier version of this test did exactly that. The |
| + // NOTE beside the count renders only when there were failures. |
| body := get(t, h, "/connectors/notion").Body.String() |
| - if !strings.Contains(body, "Could not be read") { |
| - t.Error("the status page does not name the pages that could not be read") |
| + if !strings.Contains(body, "counted here rather than left out of the totals") { |
| + t.Error("the status page does not say the unreadable pages are counted rather than dropped") |
| } |
| } |
| @@ -429,9 +436,12 @@ func TestInterruptedRunDoesNotWedgeTheConnector(t *testing.T) { |
| // bodies, ancestry, page ids and the token are all content or |
| // coordinates, and `carlos logs` is storage we cannot grep. |
| // |
| -// The fake's error carries a canary in a 404, which is the shape Notion |
| -// really uses — its 404 body quotes the page id and the connection name |
| -// — so the error path is exercised rather than assumed. |
| +// The failing page's TITLE is the canary, so the fetch-error path is |
| +// exercised with content in scope. It is not a canary in a response |
| +// body: this fake returns a Go error with no body at all, and an earlier |
| +// version of this comment claimed otherwise. Notion's real 404 body does |
| +// quote a page id and the connection name, and that path is covered |
| +// where it can be — notion.TestErrorsCarryNoResponseBody. |
| // |
| // MUTATION-TESTED 2026-09-05: logging ref.Title at Info in the fetch |
| // loop, or %w-ing the notion error into the run's failure line, makes |
diff --git a/internal/memoriatest/container_test.go b/internal/memoriatest/container_test.go| new file mode 100644 |
| index 0000000..12835bb |
| --- /dev/null |
| +++ b/internal/memoriatest/container_test.go |
| @@ -0,0 +1,51 @@ |
| +package memoriatest |
| + |
| +import ( |
| + "context" |
| + "testing" |
| + |
| + "amadan.net/oficina/memoria/internal/memoria/notion" |
| +) |
| + |
| +// TestContainersAreNotCountedAsPages. |
| +// |
| +// A data source is a container, not a document. Ledgering one gave it a |
| +// row that stayed "pending" forever, counted it in the run's total, and |
| +// counted it as "already up to date" on every later run — so the status |
| +// page said "1 of 2 known pages imported" permanently, about a workspace |
| +// that was completely imported. A number that can never reach its own |
| +// denominator is the same lie as a confident zero, told the other way up. |
| +// |
| +// Found by a code review. |
| +// |
| +// MUTATION-TESTED 2026-09-05: removing the container guard around |
| +// SeeSourcePage in Syncer.walk makes this fail with 1 of 2. Reverted. |
| +func TestContainersAreNotCountedAsPages(t *testing.T) { |
| + src := newSource() |
| + src.roots = append(src.roots, notion.Ref{ |
| + ID: "db1", Title: "Projects", EditedTime: "t1", |
| + Kind: notion.KindDataSource, DataSourceID: "db1", |
| + }) |
| + src.children["db1"] = []notion.Ref{{ |
| + ID: "row1", Title: "Project Bluebird", Path: []string{"Projects"}, |
| + EditedTime: "t1", Kind: notion.KindPage, |
| + }} |
| + src.bodies["row1"] = "Status: Shipped. The deal closed in March." |
| + |
| + h, app, cid, _ := connectorApp(t, src) |
| + sync(t, h, app, cid) |
| + sync(t, h, app, cid) |
| + |
| + total, imported, err := app.Store().CountPages(context.Background(), cid) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if total != imported { |
| + t.Errorf("%d of %d imported: a container is sitting in the ledger as a page that "+ |
| + "can never import, so this number can never reach its own denominator", |
| + imported, total) |
| + } |
| + if total != 1 { |
| + t.Errorf("ledger holds %d rows, want 1 — the row, not the database", total) |
| + } |
| +} |
diff --git a/internal/memoriatest/gates_test.go b/internal/memoriatest/gates_test.go| index 4a52066..dbe2d85 100644 |
| --- a/internal/memoriatest/gates_test.go |
| +++ b/internal/memoriatest/gates_test.go |
| @@ -406,8 +406,16 @@ func TestProductionDoesNotUseTheTestConstructor(t *testing.T) { |
| if err != nil { |
| return err |
| } |
| - if strings.Contains(string(raw), "AppForTest") { |
| - t.Errorf("%s calls AppForTest; production would serve every route unauthenticated", p) |
| + // Every seam, not just the constructor. App() hands cmd/memoria |
| + // the same *app that carries these, so `app.AsPerson("x")` in |
| + // main would compile, make every request an owner, and pass a |
| + // gate that greps for AppForTest alone. Found by a code review. |
| + for _, seam := range []string{ |
| + "AppForTest", "AsPerson", "UseFakeSource", "WaitForSync", |
| + } { |
| + if strings.Contains(string(raw), seam) { |
| + t.Errorf("%s calls the test seam %s; production must not reach it", p, seam) |
| + } |
| } |
| if !strings.Contains(string(raw), "UseAuth") && strings.Contains(string(raw), "memoria.App(") { |
| t.Errorf("%s builds the app but never calls UseAuth", p) |
diff --git a/internal/memoriatest/regress_test.go b/internal/memoriatest/regress_test.go| new file mode 100644 |
| index 0000000..4b6c7e4 |
| --- /dev/null |
| +++ b/internal/memoriatest/regress_test.go |
| @@ -0,0 +1,68 @@ |
| +package memoriatest |
| + |
| +import ( |
| + "context" |
| + "testing" |
| + |
| + memoria "amadan.net/oficina/memoria/internal/memoria" |
| + "amadan.net/oficina/memoria/internal/memoria/notion" |
| +) |
| + |
| +// TestAFailureAfterImportKeepsWhatWeHold. |
| +// |
| +// A page imports fine, and on a LATER run it 404s — deleted or unshared |
| +// in the meantime. Recording that failure used to write document_id=0 and |
| +// an empty ingested edit time, which erased memoria's own record that it |
| +// holds the document. Two consequences, both wrong: "Imported" dropped on |
| +// the status page for a document that is still there, and the next run |
| +// had to fetch and re-ingest a page that had never actually changed. |
| +// |
| +// The document itself was never at risk — memoria does not delete on |
| +// absence — which is exactly why this was easy to miss. |
| +// |
| +// Found by a code review. |
| +// |
| +// MUTATION-TESTED 2026-09-05: passing 0, "", "" to MarkPage from |
| +// markGone (the pre-fix behaviour) makes this fail. Reverted. |
| +func TestAFailureAfterImportKeepsWhatWeHold(t *testing.T) { |
| + src := newSource() |
| + src.add("root", "Handbook", "", "Expenses are reimbursed within thirty days.") |
| + src.add("kid", "Onboarding", "root", "Week one covers travel booking.", "Handbook") |
| + |
| + h, app, cid, _ := connectorApp(t, src) |
| + sync(t, h, app, cid) |
| + |
| + _, importedBefore, err := app.Store().CountPages(context.Background(), cid) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if importedBefore != 2 { |
| + t.Fatalf("imported %d before the failure, want 2 — nothing to lose otherwise", importedBefore) |
| + } |
| + |
| + // The page goes away upstream. |
| + src.failWith["kid"] = ¬ion.Error{Code: notion.CodeTraversalFailed, Status: 404} |
| + sync(t, h, app, cid) |
| + |
| + run := latest(t, app, cid) |
| + if run.State != memoria.RunDone { |
| + t.Fatalf("run state %q, want done", run.State) |
| + } |
| + if run.FailedPages != 1 { |
| + t.Errorf("failed_pages %d, want 1", run.FailedPages) |
| + } |
| + |
| + // The document is still held, and memoria still KNOWS it holds it. |
| + if hits, _ := app.Store().Search(context.Background(), "travel booking", 10); len(hits) == 0 { |
| + t.Error("the document was removed when its page 404'd; memoria does not delete on absence") |
| + } |
| + _, importedAfter, err := app.Store().CountPages(context.Background(), cid) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if importedAfter != importedBefore { |
| + t.Errorf("imported dropped %d→%d because a fetch failed; the record that memoria holds "+ |
| + "the document was erased, though the document is still there", |
| + importedBefore, importedAfter) |
| + } |
| +} |
diff --git a/tools/drive/main.go b/tools/drive/main.go| index 938f360..6697401 100644 |
| --- a/tools/drive/main.go |
| +++ b/tools/drive/main.go |
| @@ -116,8 +116,8 @@ func seedViaHTTP(h http.Handler, addr string) { |
| // a real tree and real counts behind them. |
| type driveSource struct{ tree map[string][]notion.Ref } |
| -func (d *driveSource) Roots(ctx context.Context) ([]notion.Ref, error) { |
| - return d.tree[""], nil |
| +func (d *driveSource) Roots(ctx context.Context) (roots, orphans []notion.Ref, err error) { |
| + return d.tree[""], nil, nil |
| } |
| func (d *driveSource) Fetch(ctx context.Context, ref notion.Ref) (*notion.Content, error) { |