| 1 | // A deleted document must not survive as a citation. |
| 2 | // |
| 3 | // These tests live in package memoria for the same reason lag_test.go |
| 4 | // does: reaching the state they are about means breaking the in-memory |
| 5 | // index under a running app, and the only way to do that from outside |
| 6 | // would be to grow a production method whose only caller is a test. |
| 7 | package memoria |
| 8 | |
| 9 | import ( |
| 10 | "context" |
| 11 | "net/http" |
| 12 | "net/http/httptest" |
| 13 | "strconv" |
| 14 | "strings" |
| 15 | "testing" |
| 16 | |
| 17 | "amadan.net/oficina/memoria/internal/memoria/index" |
| 18 | ) |
| 19 | |
| 20 | // storeOneDocument puts a document in and proves it is findable, so every |
| 21 | // test below starts from a document that is genuinely there. |
| 22 | func storeOneDocument(t *testing.T, a *app, term string) int64 { |
| 23 | t.Helper() |
| 24 | ctx := context.Background() |
| 25 | id, err := a.store.Ingest(ctx, IngestSpec{ |
| 26 | Source: "upload", Key: "regional-notes.txt", Title: "regional notes", |
| 27 | Body: "The " + term + " terms for the departing regional lead were agreed in March.", |
| 28 | }) |
| 29 | if err != nil { |
| 30 | t.Fatalf("Ingest: %v", err) |
| 31 | } |
| 32 | hits, err := searchAsReader(a.store, ctx, term, 10) |
| 33 | if err != nil { |
| 34 | t.Fatalf("Search: %v", err) |
| 35 | } |
| 36 | if len(hits) != 1 { |
| 37 | t.Fatalf("%d hits for the freshly stored document, want 1. Nothing below is about a "+ |
| 38 | "stale citation if the document was never findable in the first place", len(hits)) |
| 39 | } |
| 40 | return id |
| 41 | } |
| 42 | |
| 43 | // postDelete presses the delete button the way the page does. |
| 44 | func postDelete(t *testing.T, h http.Handler, id int64) *httptest.ResponseRecorder { |
| 45 | t.Helper() |
| 46 | req := httptest.NewRequest(http.MethodPost, "/documents/"+strconv.FormatInt(id, 10)+"/delete", nil) |
| 47 | req.Header.Set("Origin", lagOrigin) |
| 48 | rec := httptest.NewRecorder() |
| 49 | h.ServeHTTP(rec, req) |
| 50 | return rec |
| 51 | } |
| 52 | |
| 53 | // getWithFlash follows a redirect carrying whatever cookie came back, so |
| 54 | // the sentence a scriptless browser is shown is read off the page it is |
| 55 | // shown on rather than out of a header. |
| 56 | func getWithFlash(t *testing.T, h http.Handler, path string, from *httptest.ResponseRecorder) string { |
| 57 | t.Helper() |
| 58 | req := httptest.NewRequest(http.MethodGet, path, nil) |
| 59 | for _, c := range from.Result().Cookies() { |
| 60 | req.AddCookie(c) |
| 61 | } |
| 62 | rec := httptest.NewRecorder() |
| 63 | h.ServeHTTP(rec, req) |
| 64 | if rec.Code != http.StatusOK { |
| 65 | t.Fatalf("GET %s answered %d, want 200", path, rec.Code) |
| 66 | } |
| 67 | return rec.Body.String() |
| 68 | } |
| 69 | |
| 70 | // A search never cites a document the database does not have. |
| 71 | // |
| 72 | // The failure this replaces: Store.Delete removed the rows in a |
| 73 | // transaction and then removed the index entries, and a failure of that |
| 74 | // second step was returned as a plain error and swallowed by the handler. |
| 75 | // The document was gone and its passages stayed in the index, so Search |
| 76 | // returned a hit for it, the handler resolved the title out of the |
| 77 | // database and found nothing, and the reader got an empty-titled result |
| 78 | // linking to a page that 404s — a citation to text that is not there, |
| 79 | // which is the failure §4.1 exists to prevent. |
| 80 | // |
| 81 | // The divergence is produced by running the real Delete with the store's |
| 82 | // index swapped for a scratch one, so the removal does not reach the |
| 83 | // index Search reads. That is exactly the state a failed RemoveDocument |
| 84 | // leaves — rows gone, entries still there — and it is reached this way |
| 85 | // rather than by deleting rows in the test because Delete is the code |
| 86 | // that produces it. There is no way to make the index's DELETE fail while |
| 87 | // its SELECT still works: closing it breaks both. |
| 88 | // |
| 89 | // This test stays honest after the ordering fix in Delete, and that is |
| 90 | // the point of it. Removing the index entries first stops Delete creating |
| 91 | // this state; it does not stop it existing — an ingest racing a delete on |
| 92 | // the same document can still re-index it between the two steps, and a |
| 93 | // partial rebuild can leave it at boot. Whatever the index says, the |
| 94 | // database decides whether a passage may be cited. |
| 95 | // |
| 96 | // MUTATION-TESTED 2026-09-04, each reverted, each observed red: |
| 97 | // |
| 98 | // (a) the `if !livePassage[h.ChunkID] { continue }` guard deleted from |
| 99 | // Store.Search — the code as it stood before this review → "the |
| 100 | // search cited 1 document(s) that are not in the database ... its |
| 101 | // title came back empty ("")", and the page assertion under it |
| 102 | // (b) `livePassage` filled from the HITS instead of from the chunk rows |
| 103 | // the database returned, which is how someone shortens this later |
| 104 | // → the same two failures. The guard is only a guard because it |
| 105 | // asks the database. |
| 106 | // |
| 107 | // A third was tried and came back GREEN, which is why the code no longer |
| 108 | // has the thing it mutated: an earlier version of the guard also asked |
| 109 | // whether the DOCUMENT was still in the database, and filling that map |
| 110 | // from the hits changed nothing. A chunk row cannot outlive its document |
| 111 | // row — one transaction, and the schema cascades — so the document half |
| 112 | // could not fail for anything the passage half does not already catch, |
| 113 | // and a guard nobody can see fail is the kind this file exists to stop. |
| 114 | func TestASearchNeverCitesADocumentThatIsNotInTheDatabase(t *testing.T) { |
| 115 | h, a := appWithAWorkingIndex(t) |
| 116 | ctx := context.Background() |
| 117 | const term = "severance" |
| 118 | id := storeOneDocument(t, a, term) |
| 119 | |
| 120 | // Delete for real, with an index that does not hear about it. |
| 121 | scratch, err := index.Open() |
| 122 | if err != nil { |
| 123 | t.Fatalf("index.Open: %v", err) |
| 124 | } |
| 125 | t.Cleanup(func() { scratch.Close() }) |
| 126 | live := a.store.ix |
| 127 | a.store.ix = scratch |
| 128 | if err := a.store.Delete(ctx, id); err != nil { |
| 129 | t.Fatalf("Delete: %v", err) |
| 130 | } |
| 131 | a.store.ix = live |
| 132 | |
| 133 | // Two vacuity guards, because either failing would make every |
| 134 | // assertion below pass for the wrong reason. |
| 135 | docs, err := a.store.Summaries(ctx) |
| 136 | if err != nil { |
| 137 | t.Fatalf("listing documents: %v", err) |
| 138 | } |
| 139 | if len(docs) != 0 { |
| 140 | t.Fatalf("%d documents are still in the database, want 0; the delete did not happen, so "+ |
| 141 | "this is not a test about a stale index", len(docs)) |
| 142 | } |
| 143 | if n, err := a.store.Count(); err != nil { |
| 144 | t.Fatalf("counting passages: %v", err) |
| 145 | } else if n == 0 { |
| 146 | t.Fatalf("the index holds no passages, so it is not stale and a search finding nothing " + |
| 147 | "here would prove nothing at all") |
| 148 | } |
| 149 | |
| 150 | hits, err := searchAsReader(a.store, ctx, term, 10) |
| 151 | if err != nil { |
| 152 | t.Fatalf("Search: %v", err) |
| 153 | } |
| 154 | if len(hits) != 0 { |
| 155 | t.Errorf("the search cited %d document(s) that are not in the database: %v. The row is "+ |
| 156 | "gone, so its title came back empty (%q) and the link goes to a page that 404s — "+ |
| 157 | "a citation to text that is not there.", len(hits), hits, hits[0].Title) |
| 158 | } |
| 159 | |
| 160 | // And the same thing where a reader would see it. A result rendered |
| 161 | // from a hit like that is a blank link on the page. |
| 162 | body := getWithFlash(t, h, "/?q="+term, httptest.NewRecorder()) |
| 163 | if strings.Contains(body, `href="/documents/`+strconv.FormatInt(id, 10)+`#p`) { |
| 164 | t.Errorf("the results page links to document %d, which is deleted:\n%s", id, body) |
| 165 | } |
| 166 | } |
| 167 | |
| 168 | // A search never cites a passage the document no longer contains. |
| 169 | // |
| 170 | // This is the same bug in the other function, and it is the one the |
| 171 | // document check above does not catch. Ingest replaces a document's |
| 172 | // chunks inside its transaction and drops the old index entries AFTER the |
| 173 | // commit; when that drop fails it returns ErrIndexLag, and the document |
| 174 | // is still there, so the stale entries pass every check that asks only |
| 175 | // whether the document exists. What the reader gets is a snippet of text |
| 176 | // the author deleted in the last revision, under a live title, linking to |
| 177 | // an ordinal that now holds something else. |
| 178 | // |
| 179 | // It also makes the sentence the app says about ErrIndexLag wrong. |
| 180 | // "Stored but will not be found by a search until this instance restarts" |
| 181 | // is said about a document whose previous revision is still answering |
| 182 | // searches. |
| 183 | // |
| 184 | // Built the same way as the test above, and with the same limitation: the |
| 185 | // old entries are left behind by re-ingesting with the store's index |
| 186 | // swapped for a scratch one, because there is no way to make the index's |
| 187 | // DELETE fail while its SELECT still works. |
| 188 | // |
| 189 | // MUTATION-TESTED 2026-09-04, each reverted, each observed red: |
| 190 | // |
| 191 | // (a) `!livePassage[h.ChunkID]` weakened to the check this guard was |
| 192 | // first written as — whether the DOCUMENT is still among the rows |
| 193 | // the database returned — → "the search cited 1 passage(s) that are |
| 194 | // no longer in the document ... quoted under the document's live |
| 195 | // title". That is the mutation that matters here: the document IS |
| 196 | // still there, so a check that looks at documents sees nothing |
| 197 | // wrong, and the test above passes throughout. |
| 198 | // (b) `livePassage` filled from the hits rather than from the chunk |
| 199 | // rows the database returned → the same failure |
| 200 | // (c) the guard deleted outright → the same failure, plus the deleted- |
| 201 | // document test above |
| 202 | func TestASearchNeverCitesAPassageTheDocumentNoLongerHas(t *testing.T) { |
| 203 | _, a := appWithAWorkingIndex(t) |
| 204 | ctx := context.Background() |
| 205 | const gone = "reorganisation" |
| 206 | id := storeOneDocument(t, a, gone) |
| 207 | |
| 208 | // Re-ingest under the same key with the word taken out, with an index |
| 209 | // that does not hear about the old passages. |
| 210 | scratch, err := index.Open() |
| 211 | if err != nil { |
| 212 | t.Fatalf("index.Open: %v", err) |
| 213 | } |
| 214 | t.Cleanup(func() { scratch.Close() }) |
| 215 | live := a.store.ix |
| 216 | a.store.ix = scratch |
| 217 | again, err := a.store.Ingest(ctx, IngestSpec{ |
| 218 | Source: "upload", Key: "regional-notes.txt", Title: "regional notes", |
| 219 | Body: "The terms for the departing regional lead were agreed in March.", |
| 220 | }) |
| 221 | if err != nil { |
| 222 | t.Fatalf("re-Ingest: %v", err) |
| 223 | } |
| 224 | a.store.ix = live |
| 225 | if again != id { |
| 226 | t.Fatalf("the re-ingest made document %d instead of updating %d, so this is two documents "+ |
| 227 | "and not a revision at all", again, id) |
| 228 | } |
| 229 | |
| 230 | // Vacuity: the document is still there, the word is genuinely out of |
| 231 | // it, and the index still holds the passage that had it. |
| 232 | body := passageText(t, a, id) |
| 233 | if strings.Contains(body, gone) { |
| 234 | t.Fatalf("the new revision still contains %q, so there is no stale passage here", gone) |
| 235 | } |
| 236 | if n, err := a.store.Count(); err != nil { |
| 237 | t.Fatalf("counting passages: %v", err) |
| 238 | } else if n == 0 { |
| 239 | t.Fatalf("the index holds no passages, so nothing stale survived and a search finding " + |
| 240 | "nothing here would prove nothing") |
| 241 | } |
| 242 | |
| 243 | hits, err := searchAsReader(a.store, ctx, gone, 10) |
| 244 | if err != nil { |
| 245 | t.Fatalf("Search: %v", err) |
| 246 | } |
| 247 | if len(hits) != 0 { |
| 248 | t.Errorf("the search cited %d passage(s) that are no longer in the document: %v. The "+ |
| 249 | "snippet is text the last revision removed, and it is quoted under the document's "+ |
| 250 | "live title — a citation to text that is not there.", len(hits), hits) |
| 251 | } |
| 252 | } |
| 253 | |
| 254 | // passageText joins a document's stored passages, so a test can say what |
| 255 | // the document now contains rather than what it was given. |
| 256 | func passageText(t *testing.T, a *app, id int64) string { |
| 257 | t.Helper() |
| 258 | ps, err := a.store.Passages(context.Background(), id) |
| 259 | if err != nil { |
| 260 | t.Fatalf("Passages: %v", err) |
| 261 | } |
| 262 | var b strings.Builder |
| 263 | for _, p := range ps { |
| 264 | b.WriteString(p.Text) |
| 265 | b.WriteString("\n") |
| 266 | } |
| 267 | return b.String() |
| 268 | } |
| 269 | |
| 270 | // A delete the index refused is not a delete. |
| 271 | // |
| 272 | // Delete removes the index entries BEFORE the transaction, so a failure |
| 273 | // there leaves the document whole and findable rather than deleted and |
| 274 | // findable. The old order left the second of those: the row gone, the |
| 275 | // passages still in the index, and — since deleteDocument swallowed the |
| 276 | // error and redirected — nobody told. The passage someone deleted may be |
| 277 | // exactly the passage that must not be findable, so the delete that |
| 278 | // cannot be completed is refused outright. |
| 279 | // |
| 280 | // MUTATION-TESTED 2026-09-04, each reverted, each observed red: |
| 281 | // |
| 282 | // (a) Store.Delete's index removal moved back after the transaction — |
| 283 | // the code as it stood before this review → "the document was |
| 284 | // deleted from the database even though the index still holds its |
| 285 | // passages" |
| 286 | // (b) the notice deleted from deleteDocument's error branch → "the |
| 287 | // page says nothing about the delete that did not happen" |
| 288 | // |
| 289 | // One more was tried and came back GREEN, recorded because a mutation |
| 290 | // nobody can see fail is worth knowing about: deleting the early `return` |
| 291 | // from that branch so it falls through to the redirect below. It is |
| 292 | // behaviour-preserving — the flash is already set and one redirect still |
| 293 | // goes out — so it is a style edit, not a defect, and no test should be |
| 294 | // contorted to catch it. |
| 295 | func TestADeleteTheIndexRefusesLeavesTheDocumentAlone(t *testing.T) { |
| 296 | h, a := appWithAWorkingIndex(t) |
| 297 | ctx := context.Background() |
| 298 | id := storeOneDocument(t, a, "redundancy") |
| 299 | |
| 300 | // From here the index refuses everything, removals included. |
| 301 | if err := a.store.ix.Close(); err != nil { |
| 302 | t.Fatalf("closing the index: %v", err) |
| 303 | } |
| 304 | |
| 305 | rec := postDelete(t, h, id) |
| 306 | if rec.Code != http.StatusSeeOther { |
| 307 | t.Fatalf("delete answered %d, want 303:\n%s", rec.Code, rec.Body.String()) |
| 308 | } |
| 309 | |
| 310 | docs, err := a.store.Summaries(ctx) |
| 311 | if err != nil { |
| 312 | t.Fatalf("listing documents: %v", err) |
| 313 | } |
| 314 | if len(docs) != 1 { |
| 315 | t.Errorf("%d documents are in the database, want 1: the document was deleted from the "+ |
| 316 | "database even though the index still holds its passages, which leaves a passage "+ |
| 317 | "that can be found and cited for a document that no longer exists.", len(docs)) |
| 318 | } |
| 319 | |
| 320 | c := newDropZone() |
| 321 | body := getWithFlash(t, h, "/", rec) |
| 322 | if !strings.Contains(body, c.DeleteFailed) { |
| 323 | t.Errorf("the page says nothing about the delete that did not happen:\n want %q in the "+ |
| 324 | "page after the redirect", c.DeleteFailed) |
| 325 | } |
| 326 | } |
| 327 | |
| 328 | // A failed delete's notice survives a visit to /add. |
| 329 | // |
| 330 | // It is the index's notice, and /add has nowhere to show it. When both |
| 331 | // pages shared one cookie, whichever loaded first spent it, so opening |
| 332 | // Add content in another tab was enough to lose the only word that a |
| 333 | // delete had not happened. |
| 334 | // |
| 335 | // MUTATION-TESTED 2026-09-29, reverted: addContent takes indexNotice |
| 336 | // instead of addNotice → "/add showed the failed delete's notice, which |
| 337 | // belongs on the index". |
| 338 | func TestAFailedDeleteNoticeWaitsForTheIndex(t *testing.T) { |
| 339 | h, a := appWithAWorkingIndex(t) |
| 340 | id := storeOneDocument(t, a, "redundancy") |
| 341 | if err := a.store.ix.Close(); err != nil { |
| 342 | t.Fatalf("closing the index: %v", err) |
| 343 | } |
| 344 | |
| 345 | b := newCookieBrowser(t, h) |
| 346 | b.keep(postDelete(t, h, id)) |
| 347 | visit := b.visit |
| 348 | |
| 349 | failed := newDropZone().DeleteFailed |
| 350 | if strings.Contains(visit("/add"), failed) { |
| 351 | t.Error("/add showed the failed delete's notice, which belongs on the index") |
| 352 | } |
| 353 | if !strings.Contains(visit("/"), failed) { |
| 354 | t.Error("after a visit to /add, the index no longer says the delete failed") |
| 355 | } |
| 356 | } |
| 357 | |
| 358 | // The ordinary delete still deletes, said here because the test above |
| 359 | // only proves the refusal. A guard that made every delete fail would pass |
| 360 | // it. |
| 361 | // |
| 362 | // MUTATION-TESTED 2026-09-04, reverted: the transaction removed from |
| 363 | // Store.Delete, leaving only the index removal → "1 documents are in the |
| 364 | // database after a delete that reported no error". |
| 365 | func TestAnOrdinaryDeleteStillDeletes(t *testing.T) { |
| 366 | _, a := appWithAWorkingIndex(t) |
| 367 | ctx := context.Background() |
| 368 | const term = "restructure" |
| 369 | id := storeOneDocument(t, a, term) |
| 370 | |
| 371 | if err := a.store.Delete(ctx, id); err != nil { |
| 372 | t.Fatalf("Delete: %v", err) |
| 373 | } |
| 374 | docs, err := a.store.Summaries(ctx) |
| 375 | if err != nil { |
| 376 | t.Fatalf("listing documents: %v", err) |
| 377 | } |
| 378 | if len(docs) != 0 { |
| 379 | t.Errorf("%d documents are in the database after a delete that reported no error", len(docs)) |
| 380 | } |
| 381 | if n, err := a.store.Count(); err != nil { |
| 382 | t.Fatalf("counting passages: %v", err) |
| 383 | } else if n != 0 { |
| 384 | t.Errorf("%d passages are still indexed after a delete that reported no error", n) |
| 385 | } |
| 386 | hits, err := searchAsReader(a.store, ctx, term, 10) |
| 387 | if err != nil { |
| 388 | t.Fatalf("Search: %v", err) |
| 389 | } |
| 390 | if len(hits) != 0 { |
| 391 | t.Errorf("a deleted document is still searchable: %v", hits) |
| 392 | } |
| 393 | } |
| 394 | |
amadan