Compare commits

...
6 Commits
Author SHA1 Message Date
yonlu 5d9c677cf7 ci(index-artifact): import the exported artifact before publishing it
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m59s
CI / e2e (pull_request) In progress
Nothing should be published until the code that imports it on a user's
machine has imported it here. 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. 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, and no install could finish a first index
build.

`TestImportPublishedArtifact` 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: the row
count, the rows carrying a listen count, an FTS index in step with the
table, and one row read back through the app's own MBID conversions. It
skips without `YJ_CORE_INDEX_ARTIFACT`, so an ordinary run pays nothing.

It needs no Wails, which is why the step runs it under the indexbuild
tag: that container has no GTK. Measured on the current artifact, 64.8 MB
compressed: 39 seconds including the decompress.

Verified against the pre-fix comparison behaviour, the step goes red in
about 3 seconds — the strictly-advancing guard fails the import with a
named reason rather than the day-long spin it used to produce.

Refs #258
2026-09-25 11:03:52 -04:00
yonlu 1e3a490c12 fix(explore): refuse a catalog merge that does not land every row
The walk's predicates partition the artifact's key space, so a merge that
ends with fewer rows than the artifact declares does not mean the artifact
was smaller than it said — it means a predicate filtered rows out, and
the catalog is quietly partial while reporting complete.

Equality rather than a lower bound: 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, so an artifact
carrying one imports as a success with a row missing — which is the shape
#258 had, one cause over. The test covers exactly that artifact.

Refs #258
2026-09-25 11:03:33 -04:00
yonlu d4ea14ca5c fix(explore): merge the catalog artifact in its own mbid encoding
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 4m4s
CI / e2e (pull_request) Successful in 14m14s
The prebuilt catalog never merged. `mergeArtifactRows` positions itself
with `WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?` against the
attached artifact, and it bound that cursor as a Go `string` while
`cmd/indexexport` publishes `explore_index.mbid` as 16 raw bytes — the
storage change that took the table from 677 MB to 389 MB.

SQLite does not coerce between TEXT and BLOB and orders every blob after
every text value, so against a byte column the predicate was not wrong
but unconditional: `mbid > <text>` matched the whole artifact, so the
bound the walk looked up was the same row every time and the cursor
never advanced, and `mbid <= <text>` matched nothing, so no batch
merged. No error, no rows, no state change — a fresh install sat at "0
of 1,077,893 rows" burning a core indefinitely, which is what it did
here for a day, while Explore showed only the rows the library scan and
the lazy artist enrichment had produced and popularity for none of the
catalog.

The cursor is now an `artifactKey`, typed to the encoding
`artifactStoresText` reports for the file it is attached to, so the
comparison is made in the same type as the column it is made against.
Two things guard the class rather than the instance: a nil key binds as
an empty value instead of SQL NULL, because `mbid > NULL` agrees with
nothing and would import nothing just as silently; and the walk returns
an error when its bound does not strictly advance, because the failure
here is silence and the next one should be a failed job with a reason.

It was never caught because the fixture that guards the walk writes the
old text encoding, and the only compact fixture is a single row — below
`artifactMergeBatch`, so the bound query never ran at all. The walk is
now covered on both encodings, across several batch boundaries.

Closes #258
2026-09-25 10:18:51 -04:00
yonlu 62c1a95ead test(e2e): give the job specs their own state back
Both specs stage a job through `/__test/emit` and neither cleared it.
Nothing resets those stores, so the spec that staged it is the one that
should put it back, and `JobsChanged` with `[]` is the whole cleanup --
`JobStore` replaces its list from every snapshot, so `testctl` needs no
special case.

**The leak as reported did not reproduce, and that is worth recording
rather than quietly fixing.**  Measured with a temporary probe: a
positive control confirmed a staged job really does move the shell at a
phone width (`job-band` renders a row, `.main-panel`'s top goes 0 to
55), and the very next page had no job at all.  The reason is that
every test gets a fresh page and `JobStore.init()` refetches `GetJobs()`
from a backend registry `/__test/emit` never writes to -- it calls
`events.Deliver`, which touches frontends and no state.  So the state
cannot cross a spec boundary as described, and the 55px offset the
draft assertion saw in that suite run has another cause that is not in
evidence.

The cleanup stays, because it costs a line and the leak would need only
one spec that keeps a page alive, and the comments say what was
measured rather than asserting the mechanism.

The durable half is the rule, now in the harness reference: measure
against the element next to you, not an absolute coordinate.  An
absolute number in a shell measurement is also a claim about everything
above it -- `contentTop === 0` asserts "and no background job is
running", which that spec could not arrange.

Closes #168
2026-09-23 07:51:07 -04:00
yonlu e67462ab53 ci: lint every commit a PR would merge, not just its tip
Gitea leaves `github.event.before` empty on a `pull_request`, so the
Commit messages 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, so a green
PR stopped being true after the merge, and it happened twice: PR #245
merged a 75-char subject its own CI never saw.

The PR's base is the stand-in.  `base.sha..head` lints the PR's own
commits because base advances on main, so the commits the branch shares
with it stay reachable from it and drop out of the range.

Both payload fields 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 `github.event` is the webhook
body unmarshalled into a map, so `pull_request.base.sha` comes from
Gitea's own `PRBranchInfo.Sha`.  The shell then falls back to today's
behaviour for a dispatch run, an all-zeros push, or a base commit the
clone does not have -- so the worst case is the fix not taking effect
rather than a broken job.

Verified locally against the report's own evidence: at 68e7edb8 the old
invocation passes ("HEAD is well-formed") while the range catches
a3b5b437 at 75 chars, which is what main's post-merge run did.  All
four event shapes were exercised against the new snippet.  The
end-to-end proof is the next PR with an over-length commit that is not
its tip.

Closes #254
2026-09-23 07:50:59 -04:00
yonlu e5dc54d0ec ci(skill-check): find a make target inside a hard-wrapped span
`scripts/skill-check.sh` matched one regex against one line, so a
mention the file hard-wraps -- `` `make `` at the end of one line and
the target at the start of the next -- was invisible to it.  These docs
are mostly hard-wrapped prose, so the wrap is what the author does not
think about, and `CONTRIBUTING.md:80` is already that shape.

Lines are now joined while the inline span is still open, which an odd
number of backticks means.  The fence and line-start halves are
untouched: a fenced command is already whole, and joining inside one
would break the rule that made this awk rather than a grep.  Joining is
bounded three ways -- a fence, a blank line (CommonMark allows no blank
line inside a code span) and a file boundary -- so a stray backtick
costs one paragraph of over-matching rather than the rest of the file.

The reporting loop needed the other half of the same fix: it named the
offending file with `grep -ln "make $t"`, which cannot see a wrapped
mention either, so a target the new parser found 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.  It falls back to the bare
name.

Verified by planting the report's own wrapped `make
no-such-wrapped-target` into `CONTRIBUTING.md`: the old script reports
48 targets and exits 0, the new one names the target and the file and
exits 1.  Plant removed afterwards.

Closes #228
2026-09-23 07:50:51 -04:00
10 changed files with 795 additions and 85 deletions
+21 -4
View File
@@ -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
+44 -1
View File
@@ -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: |
@@ -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.
+16
View File
@@ -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
+88 -11
View File
@@ -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
+250 -62
View File
@@ -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 <= <text>` 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)
}
}
+266
View File
@@ -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=<core-index.db.zst> 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
}
+17
View File
@@ -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,
+16
View File
@@ -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).
*
+62 -7
View File
@@ -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 <t>` 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