fix(explore): merge the catalog artifact in its own mbid encoding (#258) #259

2026-09-25T14:23:18Z

yonlu commented

2026-09-25 14:23:18 +00:00

The prebuilt catalog never merged on a fresh install: mergeArtifactRows
positioned its batch walk with a Go string cursor against the artifact's
BLOB mbid column, so every batch merged nothing and the walk never
advanced — an unbounded loop at 100% CPU behind a progress bar reading
0 of 1,077,893 rows. Closes #258.

commit	issue	what it does
`d4ea14ca`	#258	types the walk's cursor to the artifact's own encoding, guards a bound that does not advance, covers the walk on both encodings
`1e3a490c`	#258	refuses a merge that lands fewer rows than the artifact declares
`5d9c677c`	#258	imports the exported artifact as a client does, before it is published

Verification

The bug, reproduced. TestImportCoreArtifactBatchWalkCoversAllRowsCompact
crosses several batch boundaries over a compact artifact. Against the pre-fix
comparison behaviour it goes red in ~3 seconds, at
artifact walk did not advance past 0bea635cc6ec422182fb8eaaa46fbf9f — the new
guard turning the day-long spin into a named failure. Covered on both
encodings, because the walk was only ever tested against the text one.

TestImportCoreArtifactRefusesAMergeThatLosesRows reaches the row-count
assertion honestly: a row with an empty mbid is excluded by mbid > ? in both
encodings, so an artifact carrying one used to import as a success with a row
missing.

The real artifact, end to end. TestImportPublishedArtifact takes the
published file and runs the client's own path over it — checksum, decompress,
merge — asserting 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 MBID
conversions. Measured against the current artifact (64.8 MB compressed →
135 MB, 1,077,893 rows): 39 seconds, and it fails in ~3 seconds on the
pre-fix behaviour. The workflow step that runs it was extracted from the YAML
and run for real, both on the success and the missing-file paths.

In the app. make sandbox caleb re-fetched, merged 1,077,893 rows in ~50 s,
removed the staging file, and the index now reports 50,115 artists / 330,839
release groups / 787,346 recordings with popularity on 50,000 / 274,293 /
777,087, explore_index_fts in sync at 1,168,350, and core_artifact_version +
dump_import_done + champion_built stamped. The run before it sat at 111,482
rows for a day.

Gates. make lint — 0 issues, all three build configurations; go test
on the explore package in the default and indexbuild tag sets; go vet,
gofmt, codegen-check, bindings-check; pre-push ran the full go test ./....

Two environment notes. make lint needs GOTOOLCHAIN=go1.26.0 locally: the
local toolchain is 1.27.0 and honnef.co/go/tools@v0.7.0 panics in buildir on
it, which reproduces on pristine main — CI pins GO_VERSION: 1.26.0, where it
was checked. And the local pre-commit hook inherits that, so the commits here
were made with the pinned toolchain rather than by skipping the hook.

Closes #258

The prebuilt catalog never merged on a fresh install: `mergeArtifactRows` positioned its batch walk with a Go `string` cursor against the artifact's `BLOB` `mbid` column, so every batch merged nothing and the walk never advanced — an unbounded loop at 100% CPU behind a progress bar reading `0 of 1,077,893 rows`. Closes #258. | commit | issue | what it does | | --- | --- | --- | | `d4ea14ca` | #258 | types the walk's cursor to the artifact's own encoding, guards a bound that does not advance, covers the walk on both encodings | | `1e3a490c` | #258 | refuses a merge that lands fewer rows than the artifact declares | | `5d9c677c` | #258 | imports the exported artifact as a client does, before it is published | ## Verification **The bug, reproduced.** `TestImportCoreArtifactBatchWalkCoversAllRowsCompact` crosses several batch boundaries over a compact artifact. Against the pre-fix comparison behaviour it goes red in ~3 seconds, at `artifact walk did not advance past 0bea635cc6ec422182fb8eaaa46fbf9f` — the new guard turning the day-long spin into a named failure. Covered on both encodings, because the walk was only ever tested against the text one. `TestImportCoreArtifactRefusesAMergeThatLosesRows` reaches the row-count assertion honestly: a row with an empty mbid is excluded by `mbid > ?` in both encodings, so an artifact carrying one used to import as a success with a row missing. **The real artifact, end to end.** `TestImportPublishedArtifact` takes the published file and runs the client's own path over it — checksum, decompress, merge — asserting 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 MBID conversions. Measured against the current artifact (**64.8 MB compressed** → 135 MB, 1,077,893 rows): **39 seconds**, and it fails in ~3 seconds on the pre-fix behaviour. The workflow step that runs it was extracted from the YAML and run for real, both on the success and the missing-file paths. **In the app.** `make sandbox caleb` re-fetched, merged 1,077,893 rows in ~50 s, removed the staging file, and the index now reports 50,115 artists / 330,839 release groups / 787,346 recordings with popularity on 50,000 / 274,293 / 777,087, `explore_index_fts` in sync at 1,168,350, and `core_artifact_version` + `dump_import_done` + `champion_built` stamped. The run before it sat at 111,482 rows for a day. **Gates.** `make lint` — 0 issues, all three build configurations; `go test` on the explore package in the default and `indexbuild` tag sets; `go vet`, `gofmt`, `codegen-check`, `bindings-check`; pre-push ran the full `go test ./...`. Two environment notes. `make lint` needs `GOTOOLCHAIN=go1.26.0` locally: the local toolchain is 1.27.0 and `honnef.co/go/tools@v0.7.0` panics in `buildir` on it, which reproduces on pristine `main` — CI pins `GO_VERSION: 1.26.0`, where it was checked. And the local pre-commit hook inherits that, so the commits here were made with the pinned toolchain rather than by skipping the hook. Closes #258

yonlu added 4 commits 2026-09-25 14:23:18 +00:00

ci(skill-check): find a make target inside a hard-wrapped span e5dc54d0ec

`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

ci: lint every commit a PR would merge, not just its tip e67462ab53

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

test(e2e): give the job specs their own state back 62c1a95ead

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

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

Details

CI / e2e (pull_request) Successful in 14m14s

Details

d4ea14ca5c

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

yonlu added 2 commits 2026-09-25 15:06:04 +00:00

fix(explore): refuse a catalog merge that does not land every row 1e3a490c12

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

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

Details

CI / e2e (pull_request) Successful in 12m51s

Details

5d9c677cf7

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

All checks were successful

CI / check (push) Skipped

Required

CI / e2e (push) Skipped

Required

CI / check (pull_request) Successful in 3m59s

Required

Details

CI / e2e (pull_request) Successful in 12m51s

Required

Details

You are not authorized to merge this pull request.

This pull request can be merged automatically.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/258-artifact-blob-cursor:fix/258-artifact-blob-cursor

git checkout fix/258-artifact-blob-cursor

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#259