test(database): the next destructive repair fails a test, not a volume
The fix for the dropped catalog pins one table in one wrong shape, which is the failure that happened. What cost the rebuild was more general: a destructive repair added at `database.NewDB` -- the chokepoint every binary in this project shares -- without asking which binary it runs in. The next one will have a different name and a different reason. So `TestNoCacheTableIsRetiredHere` asserts the outcome instead: put every `datamap` Cache table into a shape the schema has moved past, open the database the way cmd/indexbuild does, and require all of them to still be there. Driving it from `datamap.ByKind` is what makes it cover tables nobody remembered -- flipping the policy back fails on five, including the two artist-credit tables added the same day, where the existing test fails on one. It asserts the rows survive too, because SQLite does an implicit DELETE before a DROP and a repair that recreated the table would look identical. And it accepts an error from `NewDB`, because that is the documented trade: loud is recoverable, gone is not. `scripts/index-cache-snapshot.sh` covers the half no test can reach. The volume holds the only copy of a catalog that costs hours of someone else's bandwidth to re-derive. `VACUUM INTO` rather than `cp`, since a byte copy of a live SQLite file is a corrupt file of plausible size; the resumable staging directory is skipped; and each snapshot is reopened and asked for its catalog row count before anything is rotated out. A corrupt source and an empty catalog were both exercised: each exits non-zero, removes its own output, and leaves the previous snapshots alone. docs/index-cache.md is the restore, and the reason to bother: a restored snapshot resolves to `refresh` and folds in the listens since, which is minutes against the 3-23h this rebuild has been estimating.
This commit is contained in:
@@ -53,6 +53,10 @@ on:
|
|||||||
|
|
||||||
# Runs share one persistent working directory, so they must not overlap.
|
# Runs share one persistent working directory, so they must not overlap.
|
||||||
# A push landing mid-build waits rather than corrupting the checkpoint.
|
# A push landing mid-build waits rather than corrupting the checkpoint.
|
||||||
|
#
|
||||||
|
# That directory holds the only copy of a catalog nothing can cheaply
|
||||||
|
# re-derive: see docs/index-cache.md for the snapshot it takes and the
|
||||||
|
# restore, which is minutes against the hours a rebuild costs.
|
||||||
concurrency:
|
concurrency:
|
||||||
group: search-index
|
group: search-index
|
||||||
cancel-in-progress: false
|
cancel-in-progress: false
|
||||||
|
|||||||
@@ -3404,3 +3404,44 @@ Three things worth keeping from it:
|
|||||||
rebuilding. That is the right default, and it means the next schema
|
rebuilding. That is the right default, and it means the next schema
|
||||||
change touching `explore_index` needs a deliberate plan for this one
|
change touching `explore_index` needs a deliberate plan for this one
|
||||||
database rather than none.
|
database rather than none.
|
||||||
|
|
||||||
|
## Two guards for the index cache, and what each one is worth (2026-08-17)
|
||||||
|
|
||||||
|
Both come out of the incident above, and they protect different halves
|
||||||
|
of it.
|
||||||
|
|
||||||
|
**`TestNoCacheTableIsRetiredHere` asserts the outcome, not the
|
||||||
|
mechanism.** The test that shipped with the fix pins one table in one
|
||||||
|
wrong shape, which is the failure that happened; what actually cost the
|
||||||
|
rebuild was a destructive repair added at `database.NewDB` — the
|
||||||
|
chokepoint every binary here shares — without asking which binary it was
|
||||||
|
in. The next one will have a different name and a different reason. So
|
||||||
|
this puts *every* `datamap` Cache table into a shape the schema has
|
||||||
|
moved past, opens the database the way `cmd/indexbuild` does, and
|
||||||
|
requires all of them to still be there.
|
||||||
|
|
||||||
|
Three things it got right by being written this way. The table list is
|
||||||
|
`datamap.ByKind(Cache)`, so the two credit tables added the same day
|
||||||
|
were covered without anyone adding them — flipping the policy back fails
|
||||||
|
on **five** tables including `artist_credit_part` and
|
||||||
|
`artist_credit_ref`, where the single-table test fails on one. It
|
||||||
|
asserts rows survive as well as the table, because SQLite does an
|
||||||
|
implicit DELETE before a DROP and a repair that recreated the table
|
||||||
|
would otherwise look identical. And it *accepts* an error from `NewDB`,
|
||||||
|
because that is the trade the fix documents: loud failure instead of a
|
||||||
|
silent day of downloading.
|
||||||
|
|
||||||
|
**`scripts/index-cache-snapshot.sh` covers the half no test can.** The
|
||||||
|
volume held the only copy of a catalog whose rebuild is hours of someone
|
||||||
|
else's bandwidth. `VACUUM INTO` rather than `cp`, because a byte copy of
|
||||||
|
a live SQLite file is a corrupt file of plausible size; the staging
|
||||||
|
directory is deliberately not copied, since a build resumes without it;
|
||||||
|
and the snapshot is reopened and asked for its catalog row count before
|
||||||
|
any rotation happens. Both failure paths were exercised rather than
|
||||||
|
argued: a corrupt source and an empty catalog each exit non-zero, delete
|
||||||
|
their own output, and leave the previous snapshots in place.
|
||||||
|
|
||||||
|
`docs/index-cache.md` is the restore procedure, and the number that
|
||||||
|
makes it worth having: a restored snapshot resolves to `refresh` and
|
||||||
|
folds in the incremental listens since — minutes, against the 3–23 h a
|
||||||
|
rebuild was estimating.
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ import (
|
|||||||
_ "modernc.org/sqlite"
|
_ "modernc.org/sqlite"
|
||||||
|
|
||||||
"yellowjacket/backend/database"
|
"yellowjacket/backend/database"
|
||||||
|
"yellowjacket/backend/datamap"
|
||||||
"yellowjacket/backend/system"
|
"yellowjacket/backend/system"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -214,3 +215,97 @@ func TestTheCatalogSurvivesAStaleShape(t *testing.T) {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestNoCacheTableIsRetiredHere is the general form of the accident
|
||||||
|
// above, and it exists because the specific one is not the risk.
|
||||||
|
//
|
||||||
|
// `TestTheCatalogSurvivesAStaleShape` pins one table in one wrong shape,
|
||||||
|
// which is the failure that happened. What cost the ~205 GB was not that
|
||||||
|
// shape: it was a destructive repair added to `database.NewDB` -- the
|
||||||
|
// one chokepoint every binary in this project shares -- without asking
|
||||||
|
// which binary it was running in. The next such repair will have a
|
||||||
|
// different name and a different reason, and this database still cannot
|
||||||
|
// afford it.
|
||||||
|
//
|
||||||
|
// So the assertion is about the *outcome* rather than the mechanism: put
|
||||||
|
// every Cache table in a shape the schema has certainly moved past, open
|
||||||
|
// the database the way cmd/indexbuild does, and require that all of them
|
||||||
|
// are still there afterwards. Any future repair that drops one fails
|
||||||
|
// here regardless of how it decides to.
|
||||||
|
//
|
||||||
|
// Two things about it are deliberate.
|
||||||
|
//
|
||||||
|
// The table list comes from `datamap.ByKind(Cache)` rather than being
|
||||||
|
// written out, so a Cache table added next year is covered by this test
|
||||||
|
// on the day it is added -- the same reason `TestCatalogCoversSchema`
|
||||||
|
// reads the schema instead of a list.
|
||||||
|
//
|
||||||
|
// And `NewDB` returning an error is *accepted*, because that is the
|
||||||
|
// trade the fix documents: with Cache tables no longer rebuilt here, a
|
||||||
|
// shape the schema moved past now fails this job loudly instead of
|
||||||
|
// silently costing it a day of downloading. Loud is fine. Gone is not.
|
||||||
|
func TestNoCacheTableIsRetiredHere(t *testing.T) {
|
||||||
|
logger := slog.New(slog.DiscardHandler)
|
||||||
|
|
||||||
|
t.Setenv("YJ_HOME", t.TempDir())
|
||||||
|
|
||||||
|
dataDir, err := system.GetUserDataDirPath()
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("resolve data dir: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
dbPath := filepath.Join(dataDir, "yj.db")
|
||||||
|
|
||||||
|
if _, err := database.NewDB(logger); err != nil {
|
||||||
|
t.Fatalf("first open: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// An FTS table is four shadow tables and cannot be given a "wrong
|
||||||
|
// shape" meaningfully; the repair skips them for the same reason and
|
||||||
|
// retires them with their parent, which the parents below cover.
|
||||||
|
var cache []string
|
||||||
|
|
||||||
|
for _, table := range datamap.ByKind(datamap.Cache) {
|
||||||
|
if table.FTS {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
|
cache = append(cache, table.Name)
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(cache) == 0 {
|
||||||
|
t.Fatal("no Cache tables to check: the datamap or this test is wrong")
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, name := range cache {
|
||||||
|
// A shape nothing in the current schema describes. What matters
|
||||||
|
// is only that it disagrees; the real mismatch was one column's
|
||||||
|
// type.
|
||||||
|
exec(t, dbPath, `
|
||||||
|
DROP TABLE IF EXISTS `+name+`;
|
||||||
|
CREATE TABLE `+name+` (id INTEGER PRIMARY KEY, moved_past TEXT);
|
||||||
|
INSERT INTO `+name+` (moved_past) VALUES ('irreplaceable');
|
||||||
|
`)
|
||||||
|
}
|
||||||
|
|
||||||
|
// The error is not the assertion: see the note above.
|
||||||
|
_, _ = database.NewDB(logger)
|
||||||
|
|
||||||
|
for _, name := range cache {
|
||||||
|
rows := count(t, dbPath,
|
||||||
|
`SELECT count(*) FROM sqlite_master WHERE type = 'table' AND name = '`+name+`'`)
|
||||||
|
if rows == 0 {
|
||||||
|
t.Errorf("%s was retired: in this database a Cache table is derived, "+
|
||||||
|
"not downloaded, and dropping one costs the ~205 GB dump stream", name)
|
||||||
|
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
|
// Present but emptied is the same loss wearing a different
|
||||||
|
// shape: SQLite does an implicit DELETE before a DROP, and a
|
||||||
|
// repair that recreated the table would look identical here.
|
||||||
|
if n := count(t, dbPath, `SELECT count(*) FROM `+name); n == 0 {
|
||||||
|
t.Errorf("%s survived but was emptied", name)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -0,0 +1,99 @@
|
|||||||
|
# The index cache, and why it has a snapshot
|
||||||
|
|
||||||
|
`/srv/yellowjacket/index-cache` on the Gitea host is the `YJ_HOME` the
|
||||||
|
search-index job keeps between runs — `.gitea/workflows/index-artifact.yml`
|
||||||
|
mounts it at `/cache`. It holds the catalog every user eventually
|
||||||
|
downloads, and it is the one database in this project that is
|
||||||
|
**derived rather than downloaded**.
|
||||||
|
|
||||||
|
That is the whole reason this document exists. An install with a broken
|
||||||
|
catalog re-fetches the ~0.6 GB artifact and is fine in a minute. This
|
||||||
|
database *is* what that artifact is cut from, so its only route back is
|
||||||
|
re-streaming the MetaBrainz dumps: hours, at a rate that belongs to
|
||||||
|
someone else's server, holding a runner of capacity 1 the entire time.
|
||||||
|
|
||||||
|
## What happened on 2026-08-17
|
||||||
|
|
||||||
|
A schema repair (`fix(database): retire a table whose shape the schema
|
||||||
|
moved past`) dropped every table whose live shape disagreed with the
|
||||||
|
schema, before `applySchema`. Correct for the app. Applied here it
|
||||||
|
deleted the catalog 19 seconds into the first run:
|
||||||
|
|
||||||
|
```
|
||||||
|
retiring a table ... table=explore_index
|
||||||
|
reason="column entity_type is TEXT, schema declares INTEGER"
|
||||||
|
index maintenance mode=build reason="no completed import yet"
|
||||||
|
```
|
||||||
|
|
||||||
|
The mismatch was real and deliberate: this database is kept in the older
|
||||||
|
text encoding, which `artifactStoresText` and `sourceColumns` exist to
|
||||||
|
tolerate. So it would have been judged stale on *every* run.
|
||||||
|
|
||||||
|
Two things came out of it. `retireStaleCache` is now a build tag —
|
||||||
|
false under `indexbuild`, true in the app — and
|
||||||
|
`TestNoCacheTableIsRetiredHere` asserts the outcome rather than the
|
||||||
|
mechanism, so the next destructive repair fails a test instead of a
|
||||||
|
production volume. And the volume got the snapshot it should always have
|
||||||
|
had, below.
|
||||||
|
|
||||||
|
## Taking snapshots
|
||||||
|
|
||||||
|
```sh
|
||||||
|
scripts/index-cache-snapshot.sh [SOURCE_HOME] [DEST_DIR] [KEEP]
|
||||||
|
```
|
||||||
|
|
||||||
|
Defaults: `/srv/yellowjacket/index-cache`, `/srv/yellowjacket/index-snapshots`,
|
||||||
|
keep 2. On the Gitea host, daily and away from the Monday 04:00 build:
|
||||||
|
|
||||||
|
```
|
||||||
|
30 5 * * * /path/to/index-cache-snapshot.sh >> /var/log/yj-index-snapshot.log 2>&1
|
||||||
|
```
|
||||||
|
|
||||||
|
Three properties worth knowing before trusting it:
|
||||||
|
|
||||||
|
- **It uses `VACUUM INTO`, not `cp`.** The database may be open, and a
|
||||||
|
byte copy of a live SQLite file is a corrupt file of plausible size.
|
||||||
|
`VACUUM INTO` takes a read lock and writes a consistent, compacted
|
||||||
|
copy; it is safe to run while a build is in progress.
|
||||||
|
- **It does not copy `data/explore-staging`.** That is a resumable
|
||||||
|
checkpoint of work in flight — large, constantly changing, and a build
|
||||||
|
resumes without it. What cannot be cheaply re-derived is the finished
|
||||||
|
catalog, which is in the database.
|
||||||
|
- **It verifies before it rotates.** Each snapshot is reopened and asked
|
||||||
|
for its catalog row count; a run that produces an unreadable or empty
|
||||||
|
file fails loudly, deletes its own output, and leaves the previous
|
||||||
|
snapshots alone. Both paths are exercised, not assumed.
|
||||||
|
|
||||||
|
## Restoring
|
||||||
|
|
||||||
|
Stop anything that might be using the volume first — the job holds it
|
||||||
|
for the length of a build, and the concurrency group (`search-index`)
|
||||||
|
means a queued run will start the moment one ends.
|
||||||
|
|
||||||
|
```sh
|
||||||
|
cd /srv/yellowjacket
|
||||||
|
mv index-cache/data/yj.db index-cache/data/yj.db.broken # keep it until you are sure
|
||||||
|
cp index-snapshots/yj-index-<stamp>.db index-cache/data/yj.db
|
||||||
|
chown --reference=index-cache/data/yj.db.broken index-cache/data/yj.db
|
||||||
|
```
|
||||||
|
|
||||||
|
Then dispatch the workflow with `mode=auto`. A restored snapshot is
|
||||||
|
older than the dumps, so `indexbuild` resolves to `refresh` and folds in
|
||||||
|
the incremental listens since — which is minutes, not hours.
|
||||||
|
|
||||||
|
Two notes on what a restore does *not* need. The staging directory can
|
||||||
|
be deleted; it will be rebuilt if a build is needed. And the published
|
||||||
|
artifact is untouched by any of this: users keep downloading the last
|
||||||
|
good one until a run reports `complete=true` and `changed=true`
|
||||||
|
republishes.
|
||||||
|
|
||||||
|
## The trade this leaves open
|
||||||
|
|
||||||
|
With Cache tables no longer retired under `indexbuild`, a future
|
||||||
|
`explore_index` column change will fail this job **loudly** — at
|
||||||
|
`applySchema`, or at the first query naming the column — rather than
|
||||||
|
silently rebuilding. That is the right default: loud is recoverable and
|
||||||
|
a silent day of downloading is not. It does mean the next schema change
|
||||||
|
touching `explore_index` needs a deliberate plan for this one database:
|
||||||
|
take a snapshot, apply the change to a copy, or accept a rebuild
|
||||||
|
knowingly.
|
||||||
Executable
+89
@@ -0,0 +1,89 @@
|
|||||||
|
#!/bin/sh
|
||||||
|
# Snapshot the index build's database, which is the only copy of it.
|
||||||
|
#
|
||||||
|
# `/srv/yellowjacket/index-cache` is the `YJ_HOME` the index job keeps
|
||||||
|
# between runs (`.gitea/workflows/index-artifact.yml` mounts it at
|
||||||
|
# `/cache`). Its catalog is *derived*, not downloaded: the only way to
|
||||||
|
# rebuild it is to re-stream the MetaBrainz dumps, which is hours at a
|
||||||
|
# rate that is someone else's to decide. On 2026-08-17 a schema repair
|
||||||
|
# dropped it and cost exactly that.
|
||||||
|
#
|
||||||
|
# So it gets a snapshot, and this is the script a cron on that host runs.
|
||||||
|
# It is deliberately not part of the workflow: a backup that only exists
|
||||||
|
# while the thing it protects is being modified is not a backup.
|
||||||
|
#
|
||||||
|
# Usage (on the Gitea host):
|
||||||
|
#
|
||||||
|
# scripts/index-cache-snapshot.sh [SOURCE_HOME] [DEST_DIR] [KEEP]
|
||||||
|
#
|
||||||
|
# SOURCE_HOME default /srv/yellowjacket/index-cache
|
||||||
|
# DEST_DIR default /srv/yellowjacket/index-snapshots
|
||||||
|
# KEEP how many to retain, default 2
|
||||||
|
#
|
||||||
|
# Suggested cron — daily, and nowhere near the Monday 04:00 build:
|
||||||
|
#
|
||||||
|
# 30 5 * * * /path/to/index-cache-snapshot.sh >> /var/log/yj-index-snapshot.log 2>&1
|
||||||
|
#
|
||||||
|
# Three things about it are load-bearing.
|
||||||
|
#
|
||||||
|
# **`VACUUM INTO`, not `cp`.** The database may be open, and a byte copy
|
||||||
|
# of a live SQLite file is a corrupt file with a plausible size.
|
||||||
|
# `VACUUM INTO` takes a read lock, writes a consistent compacted copy,
|
||||||
|
# and is safe while the index job is running — it costs the snapshot's
|
||||||
|
# own write, not the source's availability.
|
||||||
|
#
|
||||||
|
# **The staging directory is not copied.** `/cache/data/explore-staging`
|
||||||
|
# is a resumable checkpoint of work in flight; it is large, it changes
|
||||||
|
# constantly, and a build resumes without it. What cannot be re-derived
|
||||||
|
# cheaply is the finished catalog, which is in the database.
|
||||||
|
#
|
||||||
|
# **A snapshot that is not verified is a belief.** Each one is opened
|
||||||
|
# and asked for its catalog row count before the old ones are rotated
|
||||||
|
# out, so a run that produced an unreadable file leaves the previous
|
||||||
|
# good snapshot in place and fails loudly.
|
||||||
|
set -eu
|
||||||
|
|
||||||
|
SOURCE_HOME="${1:-/srv/yellowjacket/index-cache}"
|
||||||
|
DEST_DIR="${2:-/srv/yellowjacket/index-snapshots}"
|
||||||
|
KEEP="${3:-2}"
|
||||||
|
|
||||||
|
DB="$SOURCE_HOME/data/yj.db"
|
||||||
|
STAMP=$(date +%Y%m%d-%H%M%S)
|
||||||
|
OUT="$DEST_DIR/yj-index-$STAMP.db"
|
||||||
|
|
||||||
|
die() { echo "index-snapshot: $*" >&2; exit 1; }
|
||||||
|
|
||||||
|
command -v sqlite3 >/dev/null 2>&1 || die "sqlite3 is not installed"
|
||||||
|
[ -f "$DB" ] || die "no database at $DB (is SOURCE_HOME right?)"
|
||||||
|
|
||||||
|
mkdir -p "$DEST_DIR"
|
||||||
|
|
||||||
|
# Headroom: the copy is at most the size of the source, usually less
|
||||||
|
# (VACUUM compacts). Refusing here beats a half-written snapshot.
|
||||||
|
need_kb=$(du -k "$DB" | cut -f1)
|
||||||
|
free_kb=$(df -Pk "$DEST_DIR" | awk 'NR == 2 { print $4 }')
|
||||||
|
[ "$free_kb" -gt "$need_kb" ] || die "not enough space in $DEST_DIR (need ~${need_kb}K, have ${free_kb}K)"
|
||||||
|
|
||||||
|
# A failed snapshot must leave nothing behind. `VACUUM INTO` refuses an
|
||||||
|
# existing file, so a partial one from a disk-full write would block
|
||||||
|
# every later run -- and worse, rotation counts files by name, so it
|
||||||
|
# would eventually be kept *instead of* a good one.
|
||||||
|
cleanup() { [ -n "${KEPT:-}" ] || rm -f "$OUT"; }
|
||||||
|
trap cleanup EXIT
|
||||||
|
|
||||||
|
echo "index-snapshot: $DB -> $OUT"
|
||||||
|
sqlite3 "$DB" "VACUUM INTO '$OUT'" || die "VACUUM INTO failed"
|
||||||
|
|
||||||
|
rows=$(sqlite3 "$OUT" "SELECT count(*) FROM explore_index" 2>/dev/null) \
|
||||||
|
|| die "snapshot is unreadable: keeping the previous ones"
|
||||||
|
[ "${rows:-0}" -gt 0 ] || die "snapshot has an empty catalog: keeping the previous ones"
|
||||||
|
|
||||||
|
KEPT=1
|
||||||
|
|
||||||
|
echo "index-snapshot: ok, $rows catalog rows, $(du -h "$OUT" | cut -f1)"
|
||||||
|
|
||||||
|
# Rotate only after the new one has been verified.
|
||||||
|
ls -1t "$DEST_DIR"/yj-index-*.db 2>/dev/null | tail -n +"$((KEEP + 1))" | while read -r old; do
|
||||||
|
echo "index-snapshot: removing $old"
|
||||||
|
rm -f "$old"
|
||||||
|
done
|
||||||
Reference in New Issue
Block a user