diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 78a2763..f9e750b 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -107,15 +107,32 @@ jobs: # Conventional Commits. `.releaserc.yml` has always derived the # version from the commit type; until now nothing checked that the # type was one it recognises, so a malformed subject silently meant - # "no release". BEFORE is the push's previous tip and is absent or - # all-zeros for a new branch, in which case only the tip is linted. + # "no release". + # + # **On a `pull_request` there is no `before`.** Gitea leaves + # `github.event.before` empty for one, so this step fell through to + # bare `make commit-check`, which lints `git log -1` — the tip + # alone. Every other commit the branch would bring was first + # examined by *main's* post-merge run, which is a green PR that + # stops being true after the merge, and which happened twice (#254). + # The PR's base is the stand-in: the range below already excludes + # what the base shares with the branch, because base advances on + # main and those commits stay reachable from it. + # + # Both are handed to the shell rather than chosen in an expression: + # `github.event.issue.number` in unclaim.yml is this repo's proof + # that payload fields resolve, and the shell then falls back to + # today's behaviour for a dispatch run or a missing field instead of + # depending on how `&&`/`||` treat an absent context. - name: Commit messages working-directory: /src env: - BEFORE: ${{ github.event.before }} + PR_BASE: ${{ github.event.pull_request.base.sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | set -eu - if [ -n "${BEFORE:-}" ] && [ "${BEFORE#0000000}" = "$BEFORE" ] \ + BEFORE="${PR_BASE:-${PUSH_BEFORE:-}}" + if [ -n "$BEFORE" ] && [ "${BEFORE#0000000}" = "$BEFORE" ] \ && git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then make commit-check RANGE="$BEFORE..$SHA" else diff --git a/.gitea/workflows/index-artifact.yml b/.gitea/workflows/index-artifact.yml index 4ae894c..b28ccd2 100644 --- a/.gitea/workflows/index-artifact.yml +++ b/.gitea/workflows/index-artifact.yml @@ -68,7 +68,7 @@ jobs: # claim with a test behind it now (cmd/indexbuild/deps_test.go), # because the v3 migration quietly broke it and this job was where # that surfaced. - image: golang:1.25 + image: golang:1.26 # This host path must exist on the runner and be listed verbatim in # act_runner's container.valid_volumes. It holds explore-staging/ # (counts.bin + state.json) and yj.db — the checkpoint that makes @@ -148,6 +148,49 @@ jobs: sha256sum /tmp/core-index.db.zst | tee /tmp/core-index.db.zst.sha256 ls -lh /tmp/core-index.db.zst + # Nothing is published until it has been imported by the code that + # imports it on a user's machine. The exporter and the importer are + # two descriptions of one storage format, and every other tier tests + # the importer against a *fixture* rather than against the file being + # shipped — a second description free to be wrong in the same + # direction as the code reading it. + # + # That is how #258 reached everyone: the importer positioned its batch + # walk with a Go `string` cursor against this file's 16-byte `mbid` + # column, and SQLite neither coerces between TEXT and BLOB nor + # complains about the comparison — so the walk merged no rows and + # never advanced, and no install could finish its first index build. + # The fixture guarding that walk writes the old text encoding, and the + # only compact fixture is one row, below the batch size, so the bound + # query never ran. Both were green throughout. + # + # Running it here is also what keeps the failure cheap: the previous + # artifact stays published while this runs, so a failure costs one + # stale catalog rather than an empty one for every install. + # + # `-tags indexbuild` because this container has no GTK and the default + # tag set links the app through Wails. The `--- PASS` grep is not + # decoration — the test skips without the path, and a skip is + # indistinguishable from a pass in a summary line. + - name: Import the exported artifact as a client does + if: steps.maintain.outputs.complete == 'true' && steps.maintain.outputs.changed == 'true' + working-directory: /src + env: + YJ_CORE_INDEX_ARTIFACT: /tmp/core-index.db.zst + run: | + set -eu + log=/tmp/import-check.log + if ! go test -tags indexbuild -count=1 -timeout 30m -v \ + -run TestImportPublishedArtifact ./backend/explore/ > "$log" 2>&1; + then + tail -60 "$log" + echo "::error::The artifact does not import; not publishing it." + exit 1 + fi + cat "$log" + grep -qF -- 'PASS: TestImportPublishedArtifact' "$log" + echo "::notice::The artifact imports as a client would merge it." + - name: Publish to the Gitea package registry if: steps.maintain.outputs.complete == 'true' && steps.maintain.outputs.changed == 'true' run: | diff --git a/.pi/skills/yellowjacket-dev/references/harness.md b/.pi/skills/yellowjacket-dev/references/harness.md index e9d6d8c..4873ef0 100644 --- a/.pi/skills/yellowjacket-dev/references/harness.md +++ b/.pi/skills/yellowjacket-dev/references/harness.md @@ -57,6 +57,21 @@ behind `YJ_TESTCTL=1`, which `scripts/dev-headless.sh` sets and staging the work that would produce it — job progress, download progress, scan progress. It calls `events.Deliver`, which *errors* when the event reaches nobody, so a `200` means it really arrived. +- **State you stage, you own** (#168). Nothing resets those stores, so + clear yours in `test.afterEach` with the same event that staged it + (`emit('JobsChanged', [])`) — the store replaces its list from every + snapshot, so `testctl` needs no special case. **Measured: this does + not currently cross a spec boundary**, because every test gets a fresh + page and `JobStore.init()` refetches `GetJobs()` from a backend + registry that `/__test/emit` never writes to. Stated anyway, because + it costs one line and the leak needs only one spec that keeps a page + alive — but do not cite #168 for a symptom you have not reproduced. +- **Measure against the thing next to you, not an absolute + coordinate.** An absolute number in a shell measurement is also a + claim about everything above it — `contentTop === 0` quietly asserts + "and no background job is running", which is not what that spec was + about or could arrange, while `contentTop === jobBandBottom` is true + either way. This is the half of #168 that stands on its own. - **`restore` is slow** (~40 s in the suite) because it copies every table. Prefer snapshotting once and restoring only when a spec genuinely mutates state. diff --git a/CLAUDE.md b/CLAUDE.md index 2512365..605b8a8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -845,6 +845,22 @@ that is quietly empty. top-N, exact match, FTS search, popularity batch, the CAA map — and asserts each returns something with a dashed id. A missed conversion site shows up there and essentially nowhere else. +- **A comparison is typed on *both* sides, and a parameter is the half + that gets forgotten.** The paragraph above is about a literal; the + artifact merge positioned its batch walk with a Go `string` cursor + against the artifact's byte column, and SQLite answered rather than + complained: `mbid > ?` with a text key is true of every row, so the + bound the walk looked up was the same every time and the cursor + never advanced, while `mbid <= ?` is false of every row, so no batch + merged at all. The import looped indefinitely at 100% CPU behind a + progress bar reading "0 of 1,077,893 rows", merged nothing and + raised nothing (#258). Nothing caught it because the fixture that + guards the walk writes the old text form and the only compact one is + a single row — below `artifactMergeBatch`, so the bound query never + ran. `artifactKey` types the cursor to the artifact's own encoding + now, and the walk fails loudly when its bound does not strictly + advance, because the failure mode here is silence rather than a + wrong answer. **The artifact is read in either encoding.** A published artifact carries whichever form the exporter that built it used, and there is one diff --git a/backend/explore/artifactimport.go b/backend/explore/artifactimport.go index 49f92ca..c6cfaaa 100644 --- a/backend/explore/artifactimport.go +++ b/backend/explore/artifactimport.go @@ -1,8 +1,10 @@ package explore import ( + "bytes" "context" "database/sql" + "database/sql/driver" "errors" "fmt" "os" @@ -283,6 +285,25 @@ func (si *SearchIndex) importCoreArtifact(ctx context.Context, path string) erro } merged, mergeErr := si.mergeArtifactRows(ctx, info.rows) + + // Every row the artifact declares has to land. The walk partitions + // the artifact's key space, so a total short of info.rows does not + // mean the artifact was smaller than it said -- it means a predicate + // filtered rows out, and the catalog is quietly partial. Equality + // rather than a lower bound because RowsAffected counts an upsert + // that changes nothing, and a row already merged locally is counted + // again here. + // + // One reachable case, so this is not merely a tripwire: a row whose + // mbid is empty is excluded by `mbid > ?` in both encodings, and an + // artifact carrying one would otherwise import as complete. + if mergeErr == nil && merged != info.rows { + mergeErr = fmt.Errorf( + "%w: merged %d of %d rows — a row the artifact holds was not selected", + ErrArtifactUnusable, merged, info.rows, + ) + } + if mergeErr == nil { si.mergeArtifactCredits(ctx) } @@ -348,8 +369,12 @@ func (si *SearchIndex) analyzeIndex() { // is an index range scan and a cancelled import leaves committed work // behind rather than rolling it all back. func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, error) { + // Asked once, because it is a property of the file and it decides + // how the walk's own comparisons are typed. See artifactKey. + storesText := si.artifactStoresText() + selectColumns := artifactSelectColumns( - si.artifactStoresText(), si.artifactHasTotals(), + storesText, si.artifactHasTotals(), ) insertSQL := ` @@ -367,7 +392,7 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e WHERE mbid > ? AND mbid <= ?` + upsertIndexConflictSQL var ( - cursor string + cursor artifactKey merged int ) @@ -376,17 +401,30 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e return merged, err } - upper, hasUpper, err := si.artifactBatchBound(cursor) + upper, hasUpper, err := si.artifactBatchBound(storesText, cursor) if err != nil { return merged, err } + if hasUpper && bytes.Compare(upper, cursor) <= 0 { + // The predicate matched the cursor itself, so the walk can + // never advance. SQLite says nothing when a comparison is + // made between types it will not coerce - the query simply + // answers wrongly - so a mismatch here would otherwise spin + // forever behind an unmoving progress bar. Fail instead. + return merged, fmt.Errorf( + "%w: artifact walk did not advance past %x", + ErrArtifactUnusable, []byte(cursor), + ) + } + var res sql.Result if hasUpper { - res, err = si.db.ExecContext(insertRangeSQL, cursor, upper) + res, err = si.db.ExecContext(insertRangeSQL, + cursor.bind(storesText), upper.bind(storesText)) } else { - res, err = si.db.ExecContext(insertSQL, cursor) + res, err = si.db.ExecContext(insertSQL, cursor.bind(storesText)) } if err != nil { @@ -413,26 +451,65 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e } } +// artifactKey is one MBID as the attached artifact stores it: 16 raw +// bytes in a compact artifact, the dashed 36-character form in one +// published before that storage change. +// +// It is a type with a bind method rather than a string because the +// comparison it feeds is typed, and the wrong type is silent. SQLite +// does not coerce between TEXT and BLOB and orders every blob after +// every text value, so a cursor bound as text against a byte column +// makes `mbid > ?` true of the whole table - the walk rediscovers the +// same batch bound forever, and `mbid <= ?` false of the whole table, +// so no batch merges at all. Nothing errors; the import simply never +// finishes. bind is the one place that knows which form the column is +// in, decided by artifactStoresText, which asks the artifact rather than +// trusting a version number. +type artifactKey []byte + +// bind renders the key as a statement argument in the artifact's own +// encoding. +func (k artifactKey) bind(storesText bool) driver.Value { + if storesText { + return string(k) + } + + // Never nil. database/sql converts a nil []byte to SQL NULL, and + // `mbid > NULL` is NULL for every row - so an unset cursor would + // agree with nothing and import nothing, which is the same silently + // empty merge this type exists to prevent, one type over. + if k == nil { + return []byte{} + } + + return []byte(k) +} + // artifactBatchBound returns the MBID that ends the next batch, and // whether one exists — no bound means the remainder is the last batch. -func (si *SearchIndex) artifactBatchBound(cursor string) (string, bool, error) { - var bound string +// +// The bound is read out of the artifact and handed back as an +// artifactKey, because it becomes the next comparison the walk makes. +func (si *SearchIndex) artifactBatchBound( + storesText bool, cursor artifactKey, +) (artifactKey, bool, error) { + var bound []byte err := si.db.QueryRowWriter( `SELECT mbid FROM core.explore_index WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?`, - cursor, artifactMergeBatch-1, + cursor.bind(storesText), artifactMergeBatch-1, ).Scan(&bound) if errors.Is(err, sql.ErrNoRows) { - return "", false, nil + return nil, false, nil } if err != nil { - return "", false, fmt.Errorf("%w: batch bound: %w", ErrArtifactUnusable, err) + return nil, false, fmt.Errorf("%w: batch bound: %w", ErrArtifactUnusable, err) } - return bound, true, nil + return artifactKey(bound), true, nil } // stampArtifactMeta records what the merge established: the catalog half diff --git a/backend/explore/artifactimport_test.go b/backend/explore/artifactimport_test.go index 6a8184e..2b4a552 100644 --- a/backend/explore/artifactimport_test.go +++ b/backend/explore/artifactimport_test.go @@ -1,6 +1,7 @@ package explore import ( + "bytes" "context" "database/sql" "encoding/hex" @@ -71,13 +72,7 @@ func writeTestArtifact( } } - for k, v := range meta { - if _, err := db.Exec( - `INSERT INTO artifact_meta (key, value) VALUES (?, ?)`, k, v, - ); err != nil { - t.Fatalf("stamp artifact meta: %v", err) - } - } + stampArtifactMeta(t, db, meta) for _, r := range rows { if _, err := db.Exec(` @@ -93,6 +88,101 @@ func writeTestArtifact( return path } +// compactArtifactSchema is the artifact cmd/indexexport publishes: the +// catalog's ids as 16 raw bytes, its entity types as codes, and the +// per-release-group total_tracks the exporter added after the first +// artifact was shipped. +// +// It matters that a fixture carries this encoding and not the older +// text one, because SQLite does not coerce between TEXT and BLOB and +// every comparison the importer makes against an mbid is therefore +// encoding-sensitive. writeTestArtifact above is the *other* fixture: +// it still writes the text form, which is what the first published +// artifact carries and what the importer must keep reading. +var compactArtifactSchema = []string{ + `CREATE TABLE explore_index ( + entity_type INTEGER NOT NULL, + mbid BLOB NOT NULL, + title TEXT NOT NULL, + artist_name TEXT NOT NULL, + artist_mbid BLOB NOT NULL, + aliases TEXT NOT NULL DEFAULT '', + popularity INTEGER NOT NULL DEFAULT 0, + listener_count INTEGER NOT NULL DEFAULT 0, + duration INTEGER NOT NULL DEFAULT 0, + caa_release_mbid BLOB NOT NULL DEFAULT x'', + release_name TEXT NOT NULL DEFAULT '', + primary_type TEXT NOT NULL DEFAULT '', + secondary_types TEXT NOT NULL DEFAULT '', + release_date TEXT NOT NULL DEFAULT '', + total_tracks INTEGER NOT NULL DEFAULT 0, + artist_type TEXT NOT NULL DEFAULT '', + country TEXT NOT NULL DEFAULT '', + disambiguation TEXT NOT NULL DEFAULT '', + sort_name TEXT NOT NULL DEFAULT '', + discog_fetched INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (mbid) + ) WITHOUT ROWID`, + `CREATE TABLE artifact_meta ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + )`, +} + +// writeCompactTestArtifact builds the artifact the exporter publishes +// today, in its own encoding, so the importer is exercised against what +// a client actually downloads rather than against what it was written +// for. +func writeCompactTestArtifact( + t *testing.T, meta map[string]string, rows []artifactRow, +) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "core-index.db") + + db, err := sql.Open("sqlite", "file:"+path) + if err != nil { + t.Fatalf("open artifact: %v", err) + } + + defer func() { _ = db.Close() }() + + for _, stmt := range compactArtifactSchema { + if _, err := db.Exec(stmt); err != nil { + t.Fatalf("create artifact schema: %v", err) + } + } + + stampArtifactMeta(t, db, meta) + + for _, r := range rows { + if _, err := db.Exec(` + INSERT INTO explore_index + (entity_type, mbid, title, artist_name, artist_mbid, popularity) + VALUES (?, ?, ?, ?, ?, ?)`, + entityCode(r.entityType), mbidBytes(r.mbid), r.title, + r.artistName, mbidBytes(r.artistMBID), r.popularity, + ); err != nil { + t.Fatalf("insert artifact row: %v", err) + } + } + + return path +} + +// stampArtifactMeta writes the artifact_meta rows a fixture declares. +func stampArtifactMeta(t *testing.T, db *sql.DB, meta map[string]string) { + t.Helper() + + for k, v := range meta { + if _, err := db.Exec( + `INSERT INTO artifact_meta (key, value) VALUES (?, ?)`, k, v, + ); err != nil { + t.Fatalf("stamp artifact meta: %v", err) + } + } +} + // validMeta is the artifact_meta a well-formed artifact carries. func validMeta() map[string]string { return map[string]string{ @@ -270,6 +360,71 @@ func TestImportCoreArtifactBatchWalkCoversAllRows(t *testing.T) { } } +// TestImportCoreArtifactBatchWalkCoversAllRowsCompact is the batch walk +// on the encoding the exporter actually publishes. +// +// The walk positions itself by comparing the artifact's own mbid column +// against the last id it reached, and that column holds 16 raw bytes. +// SQLite does not coerce between TEXT and BLOB, and a blob sorts after +// every text value, so a cursor bound as text is a predicate that either +// matches every row or none: `mbid > ?` with an empty text key is true +// of the whole table, so +// the 100th row is always the 100th row and the bound never advances, +// while `mbid <= ` is false of the whole table, so no batch ever +// merges. The result is not a wrong import but an unbounded loop that +// merges nothing and never fails. +// +// Both encodings are covered on purpose. The walk was only ever tested +// against the text fixture above, which is why it shipped broken on the +// one the clients download. +func TestImportCoreArtifactBatchWalkCoversAllRowsCompact(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + original := artifactMergeBatch + artifactMergeBatch = 100 + + t.Cleanup(func() { artifactMergeBatch = original }) + + const total = 337 + + rows := make([]artifactRow, 0, total) + for i := range total { + rows = append(rows, artifactRow{ + entityType: EntityRecording, + mbid: syntheticMBID(i), + title: "Song", + artistName: "Artist", + artistMBID: artA, + popularity: i, + }) + } + + path := writeCompactTestArtifact(t, validMeta(), rows) + + if err := si.importCoreArtifact(context.Background(), path); err != nil { + t.Fatalf("importCoreArtifact: %v", err) + } + + var got, top int + + if err := db.QueryRowWriter( + "SELECT COUNT(*), MAX(popularity) FROM explore_index", + ).Scan(&got, &top); err != nil { + t.Fatalf("count rows: %v", err) + } + + if got != total { + t.Errorf("merged %d rows, want %d", got, total) + } + + // A count alone would pass if the walk re-merged the same first + // batch forever, so the far end of the artifact is checked too. + if top != total-1 { + t.Errorf("highest popularity = %d, want %d", top, total-1) + } +} + func TestImportCoreArtifactRejectsBadArtifacts(t *testing.T) { tests := []struct { name string @@ -454,61 +609,9 @@ func TestArtifactColumnsMatchExporter(t *testing.T) { // the importer decides by asking the artifact, not by trusting a // version number, and both must land identically. func TestImportCoreArtifactAcceptsBothEncodings(t *testing.T) { - compact := filepath.Join(t.TempDir(), "core-index.db") - - db, err := sql.Open("sqlite", "file:"+compact) - if err != nil { - t.Fatalf("open artifact: %v", err) - } - - if _, err := db.Exec(`CREATE TABLE explore_index ( - entity_type INTEGER NOT NULL, - mbid BLOB NOT NULL, - title TEXT NOT NULL, - artist_name TEXT NOT NULL, - artist_mbid BLOB NOT NULL, - aliases TEXT NOT NULL DEFAULT '', - popularity INTEGER NOT NULL DEFAULT 0, - listener_count INTEGER NOT NULL DEFAULT 0, - duration INTEGER NOT NULL DEFAULT 0, - caa_release_mbid BLOB NOT NULL DEFAULT x'', - release_name TEXT NOT NULL DEFAULT '', - primary_type TEXT NOT NULL DEFAULT '', - secondary_types TEXT NOT NULL DEFAULT '', - release_date TEXT NOT NULL DEFAULT '', - artist_type TEXT NOT NULL DEFAULT '', - country TEXT NOT NULL DEFAULT '', - disambiguation TEXT NOT NULL DEFAULT '', - sort_name TEXT NOT NULL DEFAULT '', - discog_fetched INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY (mbid) - )`); err != nil { - t.Fatalf("create artifact table: %v", err) - } - - if _, err := db.Exec( - `CREATE TABLE artifact_meta (key TEXT PRIMARY KEY, value TEXT NOT NULL)`, - ); err != nil { - t.Fatalf("create artifact meta: %v", err) - } - - for k, v := range validMeta() { - if _, err := db.Exec( - "INSERT INTO artifact_meta (key, value) VALUES (?, ?)", k, v, - ); err != nil { - t.Fatalf("write artifact meta: %v", err) - } - } - - if _, err := db.Exec(` - INSERT INTO explore_index (entity_type, mbid, title, artist_name, artist_mbid, popularity) - VALUES (1, ?, 'Artist A', 'Artist A', ?, 5000)`, - mbidBytes(artA), mbidBytes(artA), - ); err != nil { - t.Fatalf("write artifact row: %v", err) - } - - _ = db.Close() + compact := writeCompactTestArtifact(t, validMeta(), []artifactRow{ + {EntityArtist, artA, "Artist A", "Artist A", artA, 5000}, + }) live := database.NewTestDB(t) si := NewSearchIndex(live, nil, nil, testLogger()) @@ -748,3 +851,88 @@ func TestImportCoreArtifactWithoutCredits(t *testing.T) { t.Errorf("credit refs = %d, want 0", refs) } } + +// TestImportCoreArtifactRefusesAMergeThatLosesRows is the count guard's +// positive case. +// +// The walk's predicates partition the artifact's key space, so a merge +// that lands fewer rows than the artifact declares means a predicate +// dropped some — and the failure is a catalog that looks populated and +// is missing things nobody can name. An empty mbid is the reachable +// way to get there: `mbid > ?` is false of it in both encodings, so it +// is never selected, and nothing else in the import would notice. +func TestImportCoreArtifactRefusesAMergeThatLosesRows(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + path := writeCompactTestArtifact(t, validMeta(), []artifactRow{ + {EntityArtist, artA, "Artist A", "Artist A", artA, 5000}, + {EntityArtist, "", "Nameless", "Artist A", artA, 4000}, + }) + + err := si.importCoreArtifact(context.Background(), path) + if err == nil { + t.Fatal("a merge that lost a row was reported as a complete import") + } + + if !strings.Contains(err.Error(), "merged 1 of 2 rows") { + t.Errorf("error = %v, want it to name the shortfall", err) + } + + // And the same rule as every other rejection: a failed merge must not + // leave the index claiming it has a catalog, or the real build would + // never run again. + if si.hasMeta(dumpImportDoneKey) { + t.Error("a failed import still stamped dump_import_done") + } +} + +// TestArtifactKeyBindsInTheArtifactsOwnEncoding pins the one place the +// batch walk's comparison type is decided. +// +// Every wrong answer is silent, which is why it is worth pinning all +// four. SQLite does not coerce TEXT to BLOB and orders every blob after +// every text value, so a text key against a byte column makes +// `mbid > ?` true of the whole artifact - the cursor never advances and +// the walk spins forever without merging a row - while a byte key +// against a text column makes it false of the whole artifact, so every +// batch merges nothing and the import "succeeds" empty. An unset cursor +// is the same fault once more: database/sql converts a nil []byte to +// SQL NULL, and `mbid > NULL` matches no row at all. +func TestArtifactKeyBindsInTheArtifactsOwnEncoding(t *testing.T) { + raw := mbidBytes(artA) + + for _, tt := range []struct { + name string + key artifactKey + want []byte + }{ + {"unset", nil, []byte{}}, + {"set", artifactKey(raw), raw}, + } { + t.Run("bytes/"+tt.name, func(t *testing.T) { + got, ok := tt.key.bind(false).([]byte) + if !ok { + t.Fatalf("bind(false) = %T, want []byte", tt.key.bind(false)) + } + + if got == nil { + t.Fatal("bound to SQL NULL, which matches no row") + } + + if !bytes.Equal(got, tt.want) { + t.Errorf("bind(false) = %x, want %x", got, tt.want) + } + }) + } + + // The dashed form is what an artifact published before the storage + // change carries, and it has to compare as text against text. + if got := artifactKey(nil).bind(true); got != "" { + t.Errorf("bind(true) on an unset cursor = %#v, want an empty string", got) + } + + if got := artifactKey(artA).bind(true); got != artA { + t.Errorf("bind(true) = %#v, want %q", got, artA) + } +} diff --git a/backend/explore/publishedartifact_test.go b/backend/explore/publishedartifact_test.go new file mode 100644 index 0000000..3501e75 --- /dev/null +++ b/backend/explore/publishedartifact_test.go @@ -0,0 +1,266 @@ +package explore + +import ( + "context" + "database/sql" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "yellowjacket/backend/database" +) + +// Import of the artifact we actually publish, as a client imports it. +// +// Every other test here builds a fixture, and a fixture is a second +// description of the storage format that can be wrong in the same +// direction as the code reading it. That is how #258 shipped: the +// importer positioned its batch walk with a Go `string` cursor against +// the artifact's 16-byte `mbid` column, and SQLite neither coerces +// between TEXT and BLOB nor complains about the comparison — so the walk +// merged nothing and never advanced, and no install could finish its +// first index build. The fixture that guards the walk writes the old +// text encoding; the only compact fixture is one row, below the batch +// size, so the bound query never ran. Both passed throughout. +// +// So this one takes the published file and runs the client's own path +// over it — checksum, decompress, merge — and asserts that what the +// artifact holds is what the client ends up with. +// +// It skips without the path, so an ordinary test run pays nothing for +// it, and the publish job is where it is meant to run: +// +// YJ_CORE_INDEX_ARTIFACT=/tmp/core-index.db.zst \ +// go test -tags indexbuild -run TestImportPublishedArtifact \ +// ./backend/explore/ +// +// The indexbuild tag is not incidental: that job's container has no GTK, +// and the default tag set links the app through Wails. + +// publishedArtifactEnv points at the published artifact: the compressed +// core-index.db.zst, or the unpacked core-index.db. +const publishedArtifactEnv = "YJ_CORE_INDEX_ARTIFACT" + +// artifactTotals is the pair this test compares across the boundary. +// +// Rows is the whole point — a merge that lands fewer of them than the +// artifact declares is a catalog that looks populated and is missing +// things nobody can name — and popularity is the half whose absence was +// reported when it happened, because it arrives only through the merge. +type artifactTotals struct { + rows int + withListen int +} + +func TestImportPublishedArtifact(t *testing.T) { + published := strings.TrimSpace(os.Getenv(publishedArtifactEnv)) + if published == "" { + t.Skipf("set %s= to import the published artifact", + publishedArtifactEnv) + } + + if _, err := os.Stat(published); err != nil { + t.Fatalf("%s: %v", publishedArtifactEnv, err) + } + + // A file-backed database rather than NewTestDB's in-memory one: the + // artifact is ~135MB and a million rows, which is not a thing to hold + // in RAM inside a test. YJ_HOME is how NewDB is pointed somewhere + // disposable, and going through NewDB means this is the constructor, + // the schema and the read pool the app itself opens. + // + // Nothing closes it, because nothing can: `DB` has no Close and the + // app's handles are process-lifetime by design. The directory is + // unlinked at cleanup and the file goes with it. + t.Setenv("YJ_HOME", t.TempDir()) + + db, err := database.NewDB(testLogger()) + if err != nil { + t.Fatalf("open database: %v", err) + } + + si := NewSearchIndex(db, nil, nil, testLogger()) + + // The checksum the publisher shipped, if it shipped one. Every + // client verifies it and refuses the artifact when it does not + // match, so a wrong one breaks Explore for everyone who has not + // already imported — and nothing else would see it, because the + // comparison is between two files only the publisher has. + if want, ok := publishedChecksum(published); ok { + got, err := fileSHA256(published) + if err != nil { + t.Fatalf("checksum the artifact: %v", err) + } + + if got != want { + t.Errorf("published artifact hashes to %s, but its .sha256 says %s", + got, want) + } + } + + unpacked := unpackPublishedArtifact(t, si, published) + + want, err := artifactTotalsOf(unpacked) + if err != nil { + t.Fatalf("count the artifact's rows: %v", err) + } + + if err := si.importCoreArtifact(context.Background(), unpacked); err != nil { + t.Fatalf("importCoreArtifact: %v", err) + } + + got, err := indexTotalsOf(db) + if err != nil { + t.Fatalf("count the index's rows: %v", err) + } + + if got.rows != want.rows { + t.Errorf("merged %d rows, but the artifact holds %d", + got.rows, want.rows) + } + + if got.withListen != want.withListen { + t.Errorf("%d rows carry a listen count, but the artifact holds %d of them", + got.withListen, want.withListen) + } + + // The FTS index is rebuilt from the table once the merge is done, and + // it is what search actually reads: a merge that lands without it + // leaves Explore silently matching nothing, which is the state #258 + // produced by a different route. + var indexed int + if err := db.QueryRowWriter( + "SELECT COUNT(*) FROM explore_index_fts", + ).Scan(&indexed); err != nil { + t.Fatalf("count the FTS index: %v", err) + } + + if indexed != got.rows { + t.Errorf("FTS index holds %d rows against the table's %d", + indexed, got.rows) + } + + // And one row read back through the app's own path, which is the + // other direction of every conversion the merge makes: a byte MBID + // out of the table, the app's dashed form, and back in as a lookup. + var raw []byte + if err := db.QueryRowWriter(` + SELECT mbid FROM explore_index + WHERE entity_type = 1 /* artist */ AND popularity > 0 + ORDER BY popularity DESC LIMIT 1`).Scan(&raw); err != nil { + t.Fatalf("read a stored mbid: %v", err) + } + + dashed, err := mbidFromBytes(raw) + if err != nil { + t.Fatalf("the stored mbid is not one: %v", err) + } + + artist := si.LookupArtistByMBID(dashed) + if artist == nil { + t.Fatalf("the artifact's most popular artist %s does not look up", dashed) + } + + if artist.Popularity == 0 { + t.Errorf("artist %s came back with no popularity", dashed) + } +} + +// publishedChecksum reads the sha256 the publisher wrote beside the +// artifact, in `sha256sum` output form. A missing file is not a +// failure: it is only there when the artifact came from the publish job. +func publishedChecksum(path string) (string, bool) { + body, err := os.ReadFile(path + ".sha256") + if err != nil { + return "", false + } + + sum := strings.TrimSpace(string(body)) + if i := strings.IndexAny(sum, " \t"); i > 0 { + sum = sum[:i] + } + + if len(sum) != 64 { + return "", false + } + + return strings.ToLower(sum), true +} + +// unpackPublishedArtifact returns a path to the unpacked database, +// going through the client's own decompression when it is handed the +// compressed file that is actually published. +func unpackPublishedArtifact(t *testing.T, si *SearchIndex, path string) string { + t.Helper() + + if strings.HasSuffix(path, ".db") { + return path + } + + // Copied into the test's own directory first: decompress writes + // beside the compressed file, and the publisher's directory is not + // this test's to write in. + staging := t.TempDir() + dst := filepath.Join(staging, coreArtifactFile) + + src, err := os.Open(path) + if err != nil { + t.Fatalf("open the published artifact: %v", err) + } + + defer func() { _ = src.Close() }() + + out, err := os.Create(dst) + if err != nil { + t.Fatalf("create a staging copy: %v", err) + } + + if _, err := io.Copy(out, src); err != nil { + t.Fatalf("copy the published artifact: %v", err) + } + + if err := out.Close(); err != nil { + t.Fatalf("close the staging copy: %v", err) + } + + fetcher := &artifactFetcher{si: si, stagingDir: staging} + + if err := fetcher.decompress(context.Background()); err != nil { + t.Fatalf("decompress the published artifact: %v", err) + } + + return fetcher.unpackedPath() +} + +// artifactTotalsOf counts what an artifact file holds, read directly so +// the numbers do not depend on anything the client does. +func artifactTotalsOf(path string) (artifactTotals, error) { + db, err := sql.Open("sqlite", "file:"+path+"?mode=ro") + if err != nil { + return artifactTotals{}, err + } + + defer func() { _ = db.Close() }() + + var totals artifactTotals + + err = db.QueryRow(`SELECT COUNT(*), COALESCE(SUM(popularity > 0), 0) + FROM explore_index`).Scan(&totals.rows, &totals.withListen) + if err != nil { + return artifactTotals{}, err + } + + return totals, nil +} + +// indexTotalsOf counts what the client ended up with. +func indexTotalsOf(db *database.DB) (artifactTotals, error) { + var totals artifactTotals + + err := db.QueryRowWriter(`SELECT COUNT(*), COALESCE(SUM(popularity > 0), 0) + FROM explore_index`).Scan(&totals.rows, &totals.withListen) + + return totals, err +} diff --git a/e2e/specs/jobs-on-a-phone.spec.ts b/e2e/specs/jobs-on-a-phone.spec.ts index f76b639..1b8ae6c 100644 --- a/e2e/specs/jobs-on-a-phone.spec.ts +++ b/e2e/specs/jobs-on-a-phone.spec.ts @@ -60,6 +60,23 @@ const PHONE = { width: 424, height: 439 }; const DESKTOP = { width: 1100, height: 800 }; test.describe('background jobs on a phone', () => { + /** + * **State a spec stages is the spec's to clear.** `/__test/emit` writes + * to a store nothing resets, so the event that staged a job is the + * event that clears it — `JobStore` replaces its whole list from every + * snapshot, so `testctl` needs no special case. + * + * **Measured on #168: this does not currently outlive the page.** Every + * test gets a fresh page, and `JobStore.init()` refetches `GetJobs()` + * from a backend registry that `/__test/emit` never writes to, so the + * staged job is gone before the next spec starts. Ownership is stated + * rather than a live leak repaired — the leak needs a page that + * survives its own spec, and there is none today. + */ + test.afterEach(async ({ testctl }) => { + await testctl.emit('JobsChanged', []); + }); + test('are shown in the band, without opening anything', async ({ app, testctl, diff --git a/e2e/specs/top-bar-fit.spec.ts b/e2e/specs/top-bar-fit.spec.ts index badd8d0..0955942 100644 --- a/e2e/specs/top-bar-fit.spec.ts +++ b/e2e/specs/top-bar-fit.spec.ts @@ -102,6 +102,22 @@ const collapsed = (page: Page) => })); test.describe('the top bar fits the window', () => { + /** + * **State a spec stages is the spec's to clear** (#168). `/__test/emit` + * writes to a store nothing resets, and this file stages the widest job + * in the app, so it puts it back — with the same event, since the store + * replaces its whole list from every snapshot. + * + * **Measured: it does not currently outlive the page.** Every test gets + * a fresh page and `JobStore.init()` refetches `GetJobs()` from a + * backend registry `/__test/emit` never writes to, so nothing is being + * repaired here; the rule is stated because it costs one line and the + * leak would need only one spec that keeps a page alive. + */ + test.afterEach(async ({ testctl }) => { + await testctl.emit('JobsChanged', []); + }); + /** * The phone's answer, which is not "it fits" (#57). * diff --git a/scripts/skill-check.sh b/scripts/skill-check.sh index 08181bb..af4dbfb 100755 --- a/scripts/skill-check.sh +++ b/scripts/skill-check.sh @@ -82,23 +82,69 @@ targets="$({ make -pqRr 2>/dev/null || true; } | # happened to break there, and a check that fails on reflow gets # disabled rather than fixed. # +# **An inline span may be hard-wrapped, and then the mention is split +# across two lines.** `make` at the end of one line and its target at +# the start of the next is one code span to Markdown and two strings to +# a per-line regex, so the target was invisible — and these docs are +# mostly hard-wrapped prose, so the wrap is what the author does not +# think about. Lines are therefore joined while the span is still open, +# which is what an odd number of backticks means. +# +# Joining re-opens the reflow trap above unless it is bounded, so it is +# bounded three ways: a fence flushes first (a fenced command is already +# whole, and joining inside one would break the line-start rule), a +# blank line flushes (CommonMark does not allow a blank line inside a +# code span, so nothing legitimate is split by one), and so does a file +# boundary. A stray odd backtick in prose therefore costs one paragraph +# of over-matching rather than the rest of the file. +# # AGENTS.md is deliberately not in this list: it is a symlink to # CLAUDE.md, asserted above, so scanning it would report every failure # twice under two names. mentioned="$(printf '%s\n' "$docs" | xargs awk ' - FNR == 1 { fence = 0 } - /^```/ { fence = !fence; next } - { - rest = $0 + function scan(text, rest) { + rest = text while (match(rest, /`make [a-z][a-z0-9-]*/)) { print substr(rest, RSTART + 6, RLENGTH - 6) rest = substr(rest, RSTART + RLENGTH) } - if (fence && match($0, /^make [a-z][a-z0-9-]*/)) { - print substr($0, 6, RLENGTH - 5) + } + + function lineStart(text) { + if (match(text, /^make [a-z][a-z0-9-]*/)) { + print substr(text, 6, RLENGTH - 5) } } + + function ticks(s, n, i) { + n = 0 + for (i = 1; i <= length(s); i++) { + if (substr(s, i, 1) == "`") n++ + } + return n + } + + function flush() { + if (buf == "") return + scan(buf) + if (fence) lineStart(buf) + buf = "" + } + + FNR == 1 { flush(); fence = 0 } + + /^```/ { flush(); fence = !fence; next } + + /^[[:space:]]*$/ { flush(); next } + + { + if (fence) { scan($0); lineStart($0); next } + buf = (buf == "" ? $0 : buf " " $0) + if (ticks(buf) % 2 == 0) flush() + } + + END { flush() } ' | sort -u)" missing="" @@ -113,7 +159,16 @@ if [ -n "$missing" ]; then echo "skill-check: the docs name make targets that do not exist:" >&2 for t in $missing; do echo " make $t" >&2 - printf '%s\n' "$docs" | xargs grep -ln "make $t" | sed 's/^/ /' >&2 + # `make ` on one line first, because that is where a target is + # normally named and it is the precise answer. The bare name is the + # fallback, and it exists because the parser above can now find a + # mention that *this* grep cannot: a wrapped span has `make` and its + # target on different lines. Without it a missing target reported no + # file at all, and `set -o pipefail` turned the empty grep into exit + # 123, before the line telling the author what to do. + hits="$(printf '%s\n' "$docs" | xargs grep -ln "make $t" 2>/dev/null || true)" + [ -n "$hits" ] || hits="$(printf '%s\n' "$docs" | xargs grep -ln -- "$t" 2>/dev/null || true)" + [ -n "$hits" ] && printf '%s\n' "$hits" | sed 's/^/ /' >&2 done echo "Fix the docs, or restore the target." >&2 exit 1