Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
4e5c6b9f7a |
+4
-21
@@ -107,32 +107,15 @@ jobs:
|
|||||||
# Conventional Commits. `.releaserc.yml` has always derived the
|
# Conventional Commits. `.releaserc.yml` has always derived the
|
||||||
# version from the commit type; until now nothing checked that the
|
# version from the commit type; until now nothing checked that the
|
||||||
# type was one it recognises, so a malformed subject silently meant
|
# type was one it recognises, so a malformed subject silently meant
|
||||||
# "no release".
|
# "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.
|
||||||
# **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
|
- name: Commit messages
|
||||||
working-directory: /src
|
working-directory: /src
|
||||||
env:
|
env:
|
||||||
PR_BASE: ${{ github.event.pull_request.base.sha }}
|
BEFORE: ${{ github.event.before }}
|
||||||
PUSH_BEFORE: ${{ github.event.before }}
|
|
||||||
run: |
|
run: |
|
||||||
set -eu
|
set -eu
|
||||||
BEFORE="${PR_BASE:-${PUSH_BEFORE:-}}"
|
if [ -n "${BEFORE:-}" ] && [ "${BEFORE#0000000}" = "$BEFORE" ] \
|
||||||
if [ -n "$BEFORE" ] && [ "${BEFORE#0000000}" = "$BEFORE" ] \
|
|
||||||
&& git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then
|
&& git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then
|
||||||
make commit-check RANGE="$BEFORE..$SHA"
|
make commit-check RANGE="$BEFORE..$SHA"
|
||||||
else
|
else
|
||||||
|
|||||||
@@ -68,7 +68,7 @@ jobs:
|
|||||||
# claim with a test behind it now (cmd/indexbuild/deps_test.go),
|
# claim with a test behind it now (cmd/indexbuild/deps_test.go),
|
||||||
# because the v3 migration quietly broke it and this job was where
|
# because the v3 migration quietly broke it and this job was where
|
||||||
# that surfaced.
|
# that surfaced.
|
||||||
image: golang:1.26
|
image: golang:1.25
|
||||||
# This host path must exist on the runner and be listed verbatim in
|
# This host path must exist on the runner and be listed verbatim in
|
||||||
# act_runner's container.valid_volumes. It holds explore-staging/
|
# act_runner's container.valid_volumes. It holds explore-staging/
|
||||||
# (counts.bin + state.json) and yj.db — the checkpoint that makes
|
# (counts.bin + state.json) and yj.db — the checkpoint that makes
|
||||||
@@ -148,49 +148,6 @@ jobs:
|
|||||||
sha256sum /tmp/core-index.db.zst | tee /tmp/core-index.db.zst.sha256
|
sha256sum /tmp/core-index.db.zst | tee /tmp/core-index.db.zst.sha256
|
||||||
ls -lh /tmp/core-index.db.zst
|
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
|
- name: Publish to the Gitea package registry
|
||||||
if: steps.maintain.outputs.complete == 'true' && steps.maintain.outputs.changed == 'true'
|
if: steps.maintain.outputs.complete == 'true' && steps.maintain.outputs.changed == 'true'
|
||||||
run: |
|
run: |
|
||||||
|
|||||||
@@ -57,21 +57,6 @@ behind `YJ_TESTCTL=1`, which `scripts/dev-headless.sh` sets and
|
|||||||
staging the work that would produce it — job progress, download
|
staging the work that would produce it — job progress, download
|
||||||
progress, scan progress. It calls `events.Deliver`, which *errors*
|
progress, scan progress. It calls `events.Deliver`, which *errors*
|
||||||
when the event reaches nobody, so a `200` means it really arrived.
|
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
|
- **`restore` is slow** (~40 s in the suite) because it copies every
|
||||||
table. Prefer snapshotting once and restoring only when a spec
|
table. Prefer snapshotting once and restoring only when a spec
|
||||||
genuinely mutates state.
|
genuinely mutates state.
|
||||||
|
|||||||
@@ -5078,22 +5078,3 @@ last rendered card.
|
|||||||
("ask the virtualizer for a larger overscan") is therefore not
|
("ask the virtualizer for a larger overscan") is therefore not
|
||||||
available without patching a private, which is why the request is
|
available without patching a private, which is why the request is
|
||||||
issued ahead of the element instead.
|
issued ahead of the element instead.
|
||||||
|
|
||||||
## Declined, and recorded nowhere else (2026-09-25, #256)
|
|
||||||
|
|
||||||
When CLAUDE.md was cut down to rules, most of its "considered and
|
|
||||||
declined" paragraphs already had a home in a code or config comment
|
|
||||||
beside what they explain. These three did not:
|
|
||||||
|
|
||||||
- **`touch-action: manipulation` was declined** (#54). The 300ms tap
|
|
||||||
delay it is offered for is already absent on a `width=device-width`
|
|
||||||
viewport; what it would actually change is the gesture stack #63 tuned
|
|
||||||
by measurement on the reference device.
|
|
||||||
- **There is no "Go to Genre"** in the phone row menu (#67). That menu
|
|
||||||
replaces name links a phone cannot use, and there has never been a
|
|
||||||
genre link to replace — it would be new navigation, which wants its
|
|
||||||
own issue.
|
|
||||||
- **The overlaid queue has no tap-outside gutter on a phone** (#171).
|
|
||||||
The drawer-style gutter would buy the affordance by taking width off a
|
|
||||||
full-screen surface on a 424px viewport; back and a 44px close button
|
|
||||||
answer it instead.
|
|
||||||
|
|||||||
@@ -779,7 +779,6 @@ func (yj *YellowJacketApp) startJanitor() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
yj.janitor.Register(maintenance.ExpiredHTTPCacheJob(yj.database))
|
yj.janitor.Register(maintenance.ExpiredHTTPCacheJob(yj.database))
|
||||||
yj.janitor.Register(maintenance.StaleArtistMetadataJob(yj.database))
|
|
||||||
yj.janitor.Register(maintenance.StaleSearchClicksJob(yj.database))
|
yj.janitor.Register(maintenance.StaleSearchClicksJob(yj.database))
|
||||||
yj.janitor.Register(maintenance.OrphanedCoverFilesJob(
|
yj.janitor.Register(maintenance.OrphanedCoverFilesJob(
|
||||||
yj.database, coversDir, library.CoverArtFileSet,
|
yj.database, coversDir, library.CoverArtFileSet,
|
||||||
|
|||||||
@@ -5,7 +5,6 @@ import (
|
|||||||
"database/sql"
|
"database/sql"
|
||||||
"fmt"
|
"fmt"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
"strings"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
// Preserving a playlist entry across the loss of its track is two
|
// Preserving a playlist entry across the loss of its track is two
|
||||||
@@ -90,14 +89,12 @@ const (
|
|||||||
// metadata on the entry itself, so the entry survives the rows being
|
// metadata on the entry itself, so the entry survives the rows being
|
||||||
// deleted underneath it.
|
// deleted underneath it.
|
||||||
//
|
//
|
||||||
// Every path that empties `audio_files` must call this (or the scoped
|
// Every path that empties `audio_files` must call this first, inside
|
||||||
// variant below) first, inside the same transaction as the delete.
|
// the same transaction as the delete. There are two such paths and
|
||||||
// These paths have drifted before: the full rescan in backend/library
|
// they had drifted: the full rescan in backend/library did this and the
|
||||||
// did this and the stale-shape retire in this package did not, so the
|
// stale-shape retire in this package did not, so the *documented*
|
||||||
// *documented* repair ("delete and rescan") preserved playlists while
|
// repair ("delete and rescan") preserved playlists while the automatic
|
||||||
// the automatic one that exists to spare the user that work silently
|
// one that exists to spare the user that work silently emptied them.
|
||||||
// emptied them (#183). The incremental scan's orphan cleanup and
|
|
||||||
// RemoveFromLibrary drifted the same way and are #246.
|
|
||||||
//
|
//
|
||||||
// The display half is skipped, with a warning, when `track_metadata`
|
// The display half is skipped, with a warning, when `track_metadata`
|
||||||
// cannot answer -- see the note above. Skipping it costs a phantom
|
// cannot answer -- see the note above. Skipping it costs a phantom
|
||||||
@@ -106,39 +103,13 @@ const (
|
|||||||
func PreservePlaylistPhantoms(
|
func PreservePlaylistPhantoms(
|
||||||
ctx context.Context, tx *sql.Tx, logger *slog.Logger,
|
ctx context.Context, tx *sql.Tx, logger *slog.Logger,
|
||||||
) error {
|
) error {
|
||||||
return preservePlaylistPhantoms(ctx, tx, nil, logger)
|
if _, err := tx.ExecContext(ctx, preservePhantomPathSQL); err != nil {
|
||||||
}
|
|
||||||
|
|
||||||
// PreservePlaylistPhantomsForFiles is PreservePlaylistPhantoms scoped to
|
|
||||||
// the given audio file ids, for the two removal paths that delete a
|
|
||||||
// known subset of the table rather than all of it: the incremental
|
|
||||||
// scan's orphan cleanup and RemoveFromLibrary. A bulk pass there would
|
|
||||||
// rewrite every linked playlist row on every scan for nothing.
|
|
||||||
func PreservePlaylistPhantomsForFiles(
|
|
||||||
ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger,
|
|
||||||
) error {
|
|
||||||
return preservePlaylistPhantoms(ctx, tx, ids, logger)
|
|
||||||
}
|
|
||||||
|
|
||||||
func preservePlaylistPhantoms(
|
|
||||||
ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger,
|
|
||||||
) error {
|
|
||||||
clause, args, skip := phantomIDFilter(ids)
|
|
||||||
if skip {
|
|
||||||
return nil
|
|
||||||
}
|
|
||||||
|
|
||||||
if _, err := tx.ExecContext(
|
|
||||||
ctx, preservePhantomPathSQL+clause, args...,
|
|
||||||
); err != nil {
|
|
||||||
return fmt.Errorf(
|
return fmt.Errorf(
|
||||||
"could not preserve playlist track file paths: %w", err,
|
"could not preserve playlist track file paths: %w", err,
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
if _, err := tx.ExecContext(
|
if _, err := tx.ExecContext(ctx, preservePhantomDisplaySQL); err != nil {
|
||||||
ctx, preservePhantomDisplaySQL+clause, args...,
|
|
||||||
); err != nil {
|
|
||||||
// A failed statement does not roll back a SQLite transaction,
|
// A failed statement does not roll back a SQLite transaction,
|
||||||
// so the path half above stands and the entries remain
|
// so the path half above stands and the entries remain
|
||||||
// re-linkable.
|
// re-linkable.
|
||||||
@@ -152,26 +123,3 @@ func preservePlaylistPhantoms(
|
|||||||
|
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// phantomIDFilter builds the extra WHERE terms and arguments that scope
|
|
||||||
// a preservation pass to a set of audio file ids. A nil ids returns the
|
|
||||||
// empty clause (a bulk run over every linked entry); an empty slice
|
|
||||||
// reports skip, since there is nothing to preserve.
|
|
||||||
func phantomIDFilter(ids []int64) (clause string, args []any, skip bool) {
|
|
||||||
switch {
|
|
||||||
case ids == nil:
|
|
||||||
return "", nil, false
|
|
||||||
case len(ids) == 0:
|
|
||||||
return "", nil, true
|
|
||||||
}
|
|
||||||
|
|
||||||
clause = " AND audio_file_id IN (" +
|
|
||||||
strings.Repeat("?,", len(ids)-1) + "?)"
|
|
||||||
|
|
||||||
args = make([]any, len(ids))
|
|
||||||
for i, id := range ids {
|
|
||||||
args[i] = id
|
|
||||||
}
|
|
||||||
|
|
||||||
return clause, args, false
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -1,94 +0,0 @@
|
|||||||
package database
|
|
||||||
|
|
||||||
import (
|
|
||||||
"context"
|
|
||||||
"database/sql"
|
|
||||||
"testing"
|
|
||||||
)
|
|
||||||
|
|
||||||
// TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs is the
|
|
||||||
// scoping half of the scoped variant: a run over one file's id must
|
|
||||||
// fill that file's playlist entries and leave every other entry alone,
|
|
||||||
// because the incremental scan calls this once per orphan batch and a
|
|
||||||
// pass that rewrote the whole table would touch every playlist row on
|
|
||||||
// every scan.
|
|
||||||
func TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs(t *testing.T) {
|
|
||||||
ctx := context.Background()
|
|
||||||
db := openRaw(t, t.TempDir())
|
|
||||||
|
|
||||||
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
|
|
||||||
t.Fatalf("pragma: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if err := applySchema(ctx, db); err != nil {
|
|
||||||
t.Fatalf("applySchema: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if _, err := db.ExecContext(ctx, `
|
|
||||||
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
|
|
||||||
INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music');
|
|
||||||
INSERT INTO artists (id, name) VALUES (3, 'Aurora Fields');
|
|
||||||
INSERT INTO cover_art (id, file_path, mime_type)
|
|
||||||
VALUES (9, 'covers/7.jpg', 'image/jpeg');
|
|
||||||
INSERT INTO albums (id, name, artist_id, cover_art_id)
|
|
||||||
VALUES (4, 'Tideline', 3, 9);
|
|
||||||
INSERT INTO audio_files
|
|
||||||
(id, file_path, file_type_id, length_milliseconds,
|
|
||||||
title, artist_credit, artist_id, album_id)
|
|
||||||
VALUES
|
|
||||||
(7, '/music/a.flac', 1, 1000,
|
|
||||||
'Slack Water', 'Aurora Fields', 3, 4),
|
|
||||||
(8, '/music/b.flac', 1, 2000,
|
|
||||||
'Second Tide', 'Aurora Fields', 3, 4);
|
|
||||||
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
|
||||||
VALUES (1, 7, 0), (1, 8, 1);
|
|
||||||
`); err != nil {
|
|
||||||
t.Fatalf("seed: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
tx, err := db.BeginTx(ctx, nil)
|
|
||||||
if err != nil {
|
|
||||||
t.Fatalf("begin: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
defer func() { _ = tx.Rollback() }()
|
|
||||||
|
|
||||||
if err := PreservePlaylistPhantomsForFiles(
|
|
||||||
ctx, tx, []int64{7}, testLogger(),
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("preserve: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if err := tx.Commit(); err != nil {
|
|
||||||
t.Fatalf("commit: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
var filled, untouched sql.NullString
|
|
||||||
|
|
||||||
if err := db.QueryRowContext(ctx,
|
|
||||||
"SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 7",
|
|
||||||
).Scan(&filled); err != nil {
|
|
||||||
t.Fatalf("read the requested entry: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if filled.String != "/music/a.flac" {
|
|
||||||
t.Errorf(
|
|
||||||
"requested entry phantom_file_path = %q, want %q",
|
|
||||||
filled.String, "/music/a.flac",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
if err := db.QueryRowContext(ctx,
|
|
||||||
"SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 8",
|
|
||||||
).Scan(&untouched); err != nil {
|
|
||||||
t.Fatalf("read the untouched entry: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if untouched.Valid {
|
|
||||||
t.Errorf(
|
|
||||||
"untouched entry got phantom_file_path = %q, want NULL "+
|
|
||||||
"(a scoped run must not rewrite the whole table)",
|
|
||||||
untouched.String,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
@@ -1,10 +1,8 @@
|
|||||||
package explore
|
package explore
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"bytes"
|
|
||||||
"context"
|
"context"
|
||||||
"database/sql"
|
"database/sql"
|
||||||
"database/sql/driver"
|
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
@@ -285,25 +283,6 @@ func (si *SearchIndex) importCoreArtifact(ctx context.Context, path string) erro
|
|||||||
}
|
}
|
||||||
|
|
||||||
merged, mergeErr := si.mergeArtifactRows(ctx, info.rows)
|
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 {
|
if mergeErr == nil {
|
||||||
si.mergeArtifactCredits(ctx)
|
si.mergeArtifactCredits(ctx)
|
||||||
}
|
}
|
||||||
@@ -369,12 +348,8 @@ func (si *SearchIndex) analyzeIndex() {
|
|||||||
// is an index range scan and a cancelled import leaves committed work
|
// is an index range scan and a cancelled import leaves committed work
|
||||||
// behind rather than rolling it all back.
|
// behind rather than rolling it all back.
|
||||||
func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, error) {
|
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(
|
selectColumns := artifactSelectColumns(
|
||||||
storesText, si.artifactHasTotals(),
|
si.artifactStoresText(), si.artifactHasTotals(),
|
||||||
)
|
)
|
||||||
|
|
||||||
insertSQL := `
|
insertSQL := `
|
||||||
@@ -392,7 +367,7 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e
|
|||||||
WHERE mbid > ? AND mbid <= ?` + upsertIndexConflictSQL
|
WHERE mbid > ? AND mbid <= ?` + upsertIndexConflictSQL
|
||||||
|
|
||||||
var (
|
var (
|
||||||
cursor artifactKey
|
cursor string
|
||||||
merged int
|
merged int
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -401,30 +376,17 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e
|
|||||||
return merged, err
|
return merged, err
|
||||||
}
|
}
|
||||||
|
|
||||||
upper, hasUpper, err := si.artifactBatchBound(storesText, cursor)
|
upper, hasUpper, err := si.artifactBatchBound(cursor)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return merged, err
|
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
|
var res sql.Result
|
||||||
|
|
||||||
if hasUpper {
|
if hasUpper {
|
||||||
res, err = si.db.ExecContext(insertRangeSQL,
|
res, err = si.db.ExecContext(insertRangeSQL, cursor, upper)
|
||||||
cursor.bind(storesText), upper.bind(storesText))
|
|
||||||
} else {
|
} else {
|
||||||
res, err = si.db.ExecContext(insertSQL, cursor.bind(storesText))
|
res, err = si.db.ExecContext(insertSQL, cursor)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -451,65 +413,26 @@ 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
|
// artifactBatchBound returns the MBID that ends the next batch, and
|
||||||
// whether one exists — no bound means the remainder is the last batch.
|
// whether one exists — no bound means the remainder is the last batch.
|
||||||
//
|
func (si *SearchIndex) artifactBatchBound(cursor string) (string, bool, error) {
|
||||||
// The bound is read out of the artifact and handed back as an
|
var bound string
|
||||||
// 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(
|
err := si.db.QueryRowWriter(
|
||||||
`SELECT mbid FROM core.explore_index
|
`SELECT mbid FROM core.explore_index
|
||||||
WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?`,
|
WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?`,
|
||||||
cursor.bind(storesText), artifactMergeBatch-1,
|
cursor, artifactMergeBatch-1,
|
||||||
).Scan(&bound)
|
).Scan(&bound)
|
||||||
|
|
||||||
if errors.Is(err, sql.ErrNoRows) {
|
if errors.Is(err, sql.ErrNoRows) {
|
||||||
return nil, false, nil
|
return "", false, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, false, fmt.Errorf("%w: batch bound: %w", ErrArtifactUnusable, err)
|
return "", false, fmt.Errorf("%w: batch bound: %w", ErrArtifactUnusable, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
return artifactKey(bound), true, nil
|
return bound, true, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// stampArtifactMeta records what the merge established: the catalog half
|
// stampArtifactMeta records what the merge established: the catalog half
|
||||||
|
|||||||
@@ -1,7 +1,6 @@
|
|||||||
package explore
|
package explore
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"bytes"
|
|
||||||
"context"
|
"context"
|
||||||
"database/sql"
|
"database/sql"
|
||||||
"encoding/hex"
|
"encoding/hex"
|
||||||
@@ -72,7 +71,13 @@ func writeTestArtifact(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
stampArtifactMeta(t, db, meta)
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
for _, r := range rows {
|
for _, r := range rows {
|
||||||
if _, err := db.Exec(`
|
if _, err := db.Exec(`
|
||||||
@@ -88,101 +93,6 @@ func writeTestArtifact(
|
|||||||
return path
|
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.
|
// validMeta is the artifact_meta a well-formed artifact carries.
|
||||||
func validMeta() map[string]string {
|
func validMeta() map[string]string {
|
||||||
return map[string]string{
|
return map[string]string{
|
||||||
@@ -360,71 +270,6 @@ 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) {
|
func TestImportCoreArtifactRejectsBadArtifacts(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
@@ -609,9 +454,61 @@ func TestArtifactColumnsMatchExporter(t *testing.T) {
|
|||||||
// the importer decides by asking the artifact, not by trusting a
|
// the importer decides by asking the artifact, not by trusting a
|
||||||
// version number, and both must land identically.
|
// version number, and both must land identically.
|
||||||
func TestImportCoreArtifactAcceptsBothEncodings(t *testing.T) {
|
func TestImportCoreArtifactAcceptsBothEncodings(t *testing.T) {
|
||||||
compact := writeCompactTestArtifact(t, validMeta(), []artifactRow{
|
compact := filepath.Join(t.TempDir(), "core-index.db")
|
||||||
{EntityArtist, artA, "Artist A", "Artist A", artA, 5000},
|
|
||||||
})
|
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()
|
||||||
|
|
||||||
live := database.NewTestDB(t)
|
live := database.NewTestDB(t)
|
||||||
si := NewSearchIndex(live, nil, nil, testLogger())
|
si := NewSearchIndex(live, nil, nil, testLogger())
|
||||||
@@ -851,88 +748,3 @@ func TestImportCoreArtifactWithoutCredits(t *testing.T) {
|
|||||||
t.Errorf("credit refs = %d, want 0", refs)
|
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)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -1,266 +0,0 @@
|
|||||||
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
|
|
||||||
}
|
|
||||||
@@ -3,7 +3,6 @@ package library
|
|||||||
import (
|
import (
|
||||||
"bytes"
|
"bytes"
|
||||||
"crypto/sha256"
|
"crypto/sha256"
|
||||||
"database/sql"
|
|
||||||
"encoding/hex"
|
"encoding/hex"
|
||||||
"fmt"
|
"fmt"
|
||||||
"image"
|
"image"
|
||||||
@@ -70,77 +69,6 @@ func CoverArtFileSet(coverPath string) []string {
|
|||||||
return paths
|
return paths
|
||||||
}
|
}
|
||||||
|
|
||||||
// sweepOrphanedCoverArt deletes the cover_art rows no album references
|
|
||||||
// and returns their file paths, for the caller to remove from disk
|
|
||||||
// after the transaction commits. Cover art is referenced only by
|
|
||||||
// albums.cover_art_id, so an orphan is a cover whose album is gone —
|
|
||||||
// which is every album the caller just swept.
|
|
||||||
//
|
|
||||||
// One implementation because the scan path, RemoveFromLibrary and
|
|
||||||
// RemoveLibrary all reach this state, and the scan side used to skip it
|
|
||||||
// entirely while RemoveLibrary did it inline (#247).
|
|
||||||
func (l *Library) sweepOrphanedCoverArt(tx *sql.Tx) ([]string, error) {
|
|
||||||
const orphanSQL = `
|
|
||||||
SELECT file_path FROM cover_art WHERE id NOT IN (
|
|
||||||
SELECT DISTINCT cover_art_id FROM albums
|
|
||||||
WHERE cover_art_id IS NOT NULL
|
|
||||||
)`
|
|
||||||
|
|
||||||
rows, err := tx.QueryContext(l.ctx, orphanSQL)
|
|
||||||
if err != nil {
|
|
||||||
return nil, fmt.Errorf("could not query orphaned cover art: %w", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
var paths []string
|
|
||||||
|
|
||||||
for rows.Next() {
|
|
||||||
var filePath string
|
|
||||||
|
|
||||||
if err := rows.Scan(&filePath); err != nil {
|
|
||||||
l.logger.Warn("could not scan cover art path", "err", err)
|
|
||||||
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
paths = append(paths, filePath)
|
|
||||||
}
|
|
||||||
|
|
||||||
// Close before the DELETE: the two run on the one writer connection.
|
|
||||||
if err := rows.Close(); err != nil {
|
|
||||||
l.logger.Warn("could not close cover art rows", "err", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if len(paths) > 0 {
|
|
||||||
if _, err := tx.ExecContext(l.ctx, `
|
|
||||||
DELETE FROM cover_art WHERE id NOT IN (
|
|
||||||
SELECT DISTINCT cover_art_id FROM albums
|
|
||||||
WHERE cover_art_id IS NOT NULL
|
|
||||||
)`); err != nil {
|
|
||||||
return nil, fmt.Errorf("could not delete orphaned cover_art: %w", err)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return paths, nil
|
|
||||||
}
|
|
||||||
|
|
||||||
// removeCoverArtFiles removes a cover original and its derived size
|
|
||||||
// variants. Only the original is stored in cover_art.file_path; the
|
|
||||||
// _sm/_md/_lg tiers are derived filenames beside it, so they have to be
|
|
||||||
// removed by name or they accumulate forever.
|
|
||||||
func (l *Library) removeCoverArtFiles(coverPaths []string) {
|
|
||||||
for _, coverPath := range coverPaths {
|
|
||||||
for _, path := range CoverArtFileSet(coverPath) {
|
|
||||||
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
|
|
||||||
l.logger.Warn(
|
|
||||||
"could not remove orphaned cover art file",
|
|
||||||
"path", path,
|
|
||||||
"err", err,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// saveCoverArt saves embedded cover art to the cache directory.
|
// saveCoverArt saves embedded cover art to the cache directory.
|
||||||
// Returns the file path where the art was saved, or empty string
|
// Returns the file path where the art was saved, or empty string
|
||||||
// if no picture data. Timing is recorded in the provided metrics.
|
// if no picture data. Timing is recorded in the provided metrics.
|
||||||
|
|||||||
+50
-8
@@ -320,12 +320,43 @@ func (l *Library) RemoveLibrary(id int64) (*RemovalSummary, error) {
|
|||||||
|
|
||||||
genresRemoved, _ := result.RowsAffected()
|
genresRemoved, _ := result.RowsAffected()
|
||||||
|
|
||||||
// Collect and delete orphaned cover_art rows before the commit. The
|
// 15. Collect orphaned cover_art file paths for post-commit cleanup.
|
||||||
// shared helper is the one place this sweep lives, so the scan path,
|
// SAFETY: Hand-crafted SELECT for orphaned cover art identification.
|
||||||
// RemoveFromLibrary and this removal cannot drift (#247).
|
// Parameterless.
|
||||||
orphanedCoverArtPaths, err := l.sweepOrphanedCoverArt(tx)
|
rows, err := tx.QueryContext(l.ctx,
|
||||||
|
`SELECT file_path FROM cover_art WHERE id NOT IN (
|
||||||
|
SELECT DISTINCT cover_art_id FROM albums
|
||||||
|
WHERE cover_art_id IS NOT NULL
|
||||||
|
)`)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, fmt.Errorf("could not query orphaned cover art: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
var orphanedCoverArtPaths []string
|
||||||
|
|
||||||
|
for rows.Next() {
|
||||||
|
var filePath string
|
||||||
|
if err := rows.Scan(&filePath); err != nil {
|
||||||
|
l.logger.Warn("could not scan cover art path", "err", err)
|
||||||
|
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
|
orphanedCoverArtPaths = append(orphanedCoverArtPaths, filePath)
|
||||||
|
}
|
||||||
|
|
||||||
|
if err := rows.Close(); err != nil {
|
||||||
|
l.logger.Warn("could not close cover art rows", "err", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// 16. Delete orphaned cover_art rows.
|
||||||
|
// SAFETY: Hand-crafted orphan cleanup SQL. Parameterless.
|
||||||
|
if _, err := tx.ExecContext(l.ctx,
|
||||||
|
`DELETE FROM cover_art WHERE id NOT IN (
|
||||||
|
SELECT DISTINCT cover_art_id FROM albums
|
||||||
|
WHERE cover_art_id IS NOT NULL
|
||||||
|
)`); err != nil {
|
||||||
|
return nil, fmt.Errorf("could not delete orphaned cover_art: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// 17. Delete the library's tagging queue. tagging_items holds a
|
// 17. Delete the library's tagging queue. tagging_items holds a
|
||||||
@@ -361,9 +392,20 @@ func (l *Library) RemoveLibrary(id int64) (*RemovalSummary, error) {
|
|||||||
// avoids a costly full re-index of all remaining tracks (~10s for
|
// avoids a costly full re-index of all remaining tracks (~10s for
|
||||||
// 25K tracks).
|
// 25K tracks).
|
||||||
|
|
||||||
// Post-commit: remove the orphaned cover art files and their sized
|
// 21. Post-commit: Delete orphaned cover art files and their sized
|
||||||
// variants.
|
// variants. Only the original is stored in cover_art.file_path; the
|
||||||
l.removeCoverArtFiles(orphanedCoverArtPaths)
|
// _sm/_md/_lg thumbnails are derived filenames beside it, so they
|
||||||
|
// have to be removed by name or they accumulate forever.
|
||||||
|
for _, coverPath := range orphanedCoverArtPaths {
|
||||||
|
for _, path := range CoverArtFileSet(coverPath) {
|
||||||
|
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
|
||||||
|
l.logger.Warn("could not remove orphaned cover art file",
|
||||||
|
"path", path,
|
||||||
|
"err", err,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// 22. Post-commit: Compact queue.
|
// 22. Post-commit: Compact queue.
|
||||||
if l.removalHooks.CompactQueue != nil {
|
if l.removalHooks.CompactQueue != nil {
|
||||||
|
|||||||
+35
-110
@@ -911,96 +911,30 @@ func (l *Library) scanInternal(
|
|||||||
|
|
||||||
orphanStart := time.Now()
|
orphanStart := time.Now()
|
||||||
|
|
||||||
// Snapshot the orphan set first. The playlist-phantom
|
|
||||||
// preservation and the deletes are one transaction (the
|
|
||||||
// preservation has to land before the ON DELETE SET NULL, and
|
|
||||||
// both have to succeed or neither does), and the ids are what
|
|
||||||
// scope that preservation to just these files instead of
|
|
||||||
// rewriting every playlist row on a routine scan.
|
|
||||||
var (
|
|
||||||
orphans []sqlcgen.AudioFile
|
|
||||||
orphanPaths []string
|
|
||||||
)
|
|
||||||
|
|
||||||
existingPaths.Range(func(key, value any) bool {
|
existingPaths.Range(func(key, value any) bool {
|
||||||
orphanPaths = append(orphanPaths, key.(string))
|
path := key.(string)
|
||||||
orphans = append(orphans, value.(sqlcgen.AudioFile))
|
audioFile := value.(sqlcgen.AudioFile)
|
||||||
|
|
||||||
return true
|
l.logger.Debug(
|
||||||
})
|
"removing orphaned database entry",
|
||||||
|
"path", path, "id", audioFile.ID,
|
||||||
|
)
|
||||||
|
|
||||||
deleted := make([]bool, len(orphans))
|
if err := l.db.Queries.DeleteAudioFile(
|
||||||
|
l.ctx, audioFile.ID,
|
||||||
|
); err != nil {
|
||||||
|
l.logger.Warn(
|
||||||
|
"failed to delete orphaned audio file",
|
||||||
|
"path", path,
|
||||||
|
"id", audioFile.ID,
|
||||||
|
"err", err,
|
||||||
|
)
|
||||||
|
|
||||||
if len(orphans) > 0 {
|
metrics.addWarning(path, "orphan", err)
|
||||||
orphanIDs := make([]int64, len(orphans))
|
|
||||||
for i, f := range orphans {
|
return true
|
||||||
orphanIDs[i] = f.ID
|
|
||||||
}
|
}
|
||||||
|
|
||||||
tx, beginErr := l.db.BeginTx()
|
|
||||||
if beginErr != nil {
|
|
||||||
metrics.addWarning("", "orphan", beginErr)
|
|
||||||
} else {
|
|
||||||
defer func() { _ = tx.Rollback() }() // no-op after commit
|
|
||||||
|
|
||||||
if err := database.PreservePlaylistPhantomsForFiles(
|
|
||||||
l.ctx, tx, orphanIDs, l.logger,
|
|
||||||
); err != nil {
|
|
||||||
// Deleting without the phantoms is exactly the
|
|
||||||
// playlist-emptying bug the preservation exists to
|
|
||||||
// prevent, so leave the rows for the next scan
|
|
||||||
// rather than empty the playlists now.
|
|
||||||
l.logger.Error(
|
|
||||||
"skipping orphan deletion: could not preserve "+
|
|
||||||
"playlist entries",
|
|
||||||
"err", err,
|
|
||||||
)
|
|
||||||
|
|
||||||
metrics.addWarning("", "orphan", err)
|
|
||||||
|
|
||||||
_ = tx.Rollback()
|
|
||||||
} else {
|
|
||||||
txq := l.db.Queries.WithTx(tx)
|
|
||||||
|
|
||||||
for i, f := range orphans {
|
|
||||||
if err := txq.DeleteAudioFile(
|
|
||||||
l.ctx, f.ID,
|
|
||||||
); err != nil {
|
|
||||||
l.logger.Warn(
|
|
||||||
"failed to delete orphaned audio file",
|
|
||||||
"path", orphanPaths[i],
|
|
||||||
"id", f.ID,
|
|
||||||
"err", err,
|
|
||||||
)
|
|
||||||
|
|
||||||
metrics.addWarning(orphanPaths[i], "orphan", err)
|
|
||||||
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
deleted[i] = true
|
|
||||||
}
|
|
||||||
|
|
||||||
if err := tx.Commit(); err != nil {
|
|
||||||
l.logger.Error(
|
|
||||||
"could not commit orphan deletion",
|
|
||||||
"err", err,
|
|
||||||
)
|
|
||||||
|
|
||||||
metrics.addWarning("", "orphan", err)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// Post-commit bookkeeping for the files that actually went.
|
|
||||||
for i, f := range orphans {
|
|
||||||
if !deleted[i] {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
path := orphanPaths[i]
|
|
||||||
|
|
||||||
// Keep the file's tagging group in sync: drop the group's
|
// Keep the file's tagging group in sync: drop the group's
|
||||||
// track count and clear it out once empty, mirroring the
|
// track count and clear it out once empty, mirroring the
|
||||||
// bookkeeping maybeRebindTaggingGroup does for a group_key
|
// bookkeeping maybeRebindTaggingGroup does for a group_key
|
||||||
@@ -1008,25 +942,25 @@ func (l *Library) scanInternal(
|
|||||||
// and replaced leaves a stale tagging_items row behind —
|
// and replaced leaves a stale tagging_items row behind —
|
||||||
// its track_count still counts the deleted files, and it
|
// its track_count still counts the deleted files, and it
|
||||||
// never clears from the autotag queue.
|
// never clears from the autotag queue.
|
||||||
if f.GroupKey != "" {
|
if audioFile.GroupKey != "" {
|
||||||
if err := l.db.Queries.DecrementTaggingItemTrackCount(
|
if err := l.db.Queries.DecrementTaggingItemTrackCount(
|
||||||
l.ctx, f.GroupKey,
|
l.ctx, audioFile.GroupKey,
|
||||||
); err != nil {
|
); err != nil {
|
||||||
l.logger.Warn(
|
l.logger.Warn(
|
||||||
"failed to decrement tagging group for orphan",
|
"failed to decrement tagging group for orphan",
|
||||||
"path", path,
|
"path", path,
|
||||||
"group_key", f.GroupKey,
|
"group_key", audioFile.GroupKey,
|
||||||
"err", err,
|
"err", err,
|
||||||
)
|
)
|
||||||
|
|
||||||
metrics.addWarning(path, "orphan", err)
|
metrics.addWarning(path, "orphan", err)
|
||||||
} else if err := l.db.Queries.DeleteTaggingItemIfEmpty(
|
} else if err := l.db.Queries.DeleteTaggingItemIfEmpty(
|
||||||
l.ctx, f.GroupKey,
|
l.ctx, audioFile.GroupKey,
|
||||||
); err != nil {
|
); err != nil {
|
||||||
l.logger.Warn(
|
l.logger.Warn(
|
||||||
"failed to clean up emptied tagging group for orphan",
|
"failed to clean up emptied tagging group for orphan",
|
||||||
"path", path,
|
"path", path,
|
||||||
"group_key", f.GroupKey,
|
"group_key", audioFile.GroupKey,
|
||||||
"err", err,
|
"err", err,
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -1035,20 +969,24 @@ func (l *Library) scanInternal(
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Remove from FTS5 search index and the lyrics index.
|
// Remove from FTS5 search index and the lyrics index.
|
||||||
if err := l.db.DeleteSearchIndex(f.ID); err != nil {
|
if err := l.db.DeleteSearchIndex(
|
||||||
|
audioFile.ID,
|
||||||
|
); err != nil {
|
||||||
l.logger.Warn(
|
l.logger.Warn(
|
||||||
"failed to delete FTS entry for orphan",
|
"failed to delete FTS entry for orphan",
|
||||||
"id", f.ID,
|
"id", audioFile.ID,
|
||||||
"err", err,
|
"err", err,
|
||||||
)
|
)
|
||||||
|
|
||||||
metrics.addWarning(path, "orphan", err)
|
metrics.addWarning(path, "orphan", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := l.db.DeleteLyricsIndex(f.ID); err != nil {
|
if err := l.db.DeleteLyricsIndex(
|
||||||
|
audioFile.ID,
|
||||||
|
); err != nil {
|
||||||
l.logger.Warn(
|
l.logger.Warn(
|
||||||
"failed to delete lyrics index entry for orphan",
|
"failed to delete lyrics index entry for orphan",
|
||||||
"id", f.ID,
|
"id", audioFile.ID,
|
||||||
"err", err,
|
"err", err,
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -1056,7 +994,9 @@ func (l *Library) scanInternal(
|
|||||||
}
|
}
|
||||||
|
|
||||||
removed.Add(1)
|
removed.Add(1)
|
||||||
}
|
|
||||||
|
return true
|
||||||
|
})
|
||||||
|
|
||||||
metrics.OrphanCleanup = time.Since(orphanStart)
|
metrics.OrphanCleanup = time.Since(orphanStart)
|
||||||
|
|
||||||
@@ -1265,32 +1205,17 @@ func (l *Library) pruneEmptyEntities() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Cover art after albums: a cover whose album just went is
|
|
||||||
// unreferenced, and leaving the row behind keeps its files exempt
|
|
||||||
// from the janitor's covers sweep forever (#247).
|
|
||||||
orphanedCovers, err := l.sweepOrphanedCoverArt(tx)
|
|
||||||
if err != nil {
|
|
||||||
l.logger.Warn("could not sweep orphaned cover art", "err", err)
|
|
||||||
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
if err := tx.Commit(); err != nil {
|
if err := tx.Commit(); err != nil {
|
||||||
l.logger.Warn("could not commit entity cleanup", "err", err)
|
l.logger.Warn("could not commit entity cleanup", "err", err)
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
// Post-commit: the rows are gone, so their files can go too.
|
if len(albumIDs) > 0 || len(artistIDs) > 0 || len(genreIDs) > 0 {
|
||||||
l.removeCoverArtFiles(orphanedCovers)
|
|
||||||
|
|
||||||
if len(albumIDs) > 0 || len(artistIDs) > 0 || len(genreIDs) > 0 ||
|
|
||||||
len(orphanedCovers) > 0 {
|
|
||||||
l.logger.Info("pruned empty library entities",
|
l.logger.Info("pruned empty library entities",
|
||||||
"albums", len(albumIDs),
|
"albums", len(albumIDs),
|
||||||
"artists", len(artistIDs),
|
"artists", len(artistIDs),
|
||||||
"genres", len(genreIDs),
|
"genres", len(genreIDs),
|
||||||
"covers", len(orphanedCovers),
|
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -94,57 +94,6 @@ func countRows(
|
|||||||
return n
|
return n
|
||||||
}
|
}
|
||||||
|
|
||||||
// RemoveFromLibrary is the one path that empties a track *deliberately*:
|
|
||||||
// the file stays on disk but is excluded, so nothing re-imports it. The
|
|
||||||
// playlist entry must still survive as a re-linkable phantom rather than
|
|
||||||
// an empty row, because a later full rescan clears the exclusion and is
|
|
||||||
// what re-links the entry then (#246).
|
|
||||||
func TestRemoveFromLibrary_PreservesPlaylistPhantoms(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
lib, db := setupTestLibrary(t)
|
|
||||||
|
|
||||||
seedRemovableLibrary(t, lib, "/nonexistent/cover.jpg")
|
|
||||||
|
|
||||||
if _, err := lib.db.ExecContext(
|
|
||||||
`INSERT INTO playlists (name) VALUES ('keepme')`,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed playlist: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
playlistID := queryInt(
|
|
||||||
t, db, `SELECT id FROM playlists WHERE name = 'keepme'`,
|
|
||||||
)
|
|
||||||
|
|
||||||
if _, err := lib.db.ExecContext(
|
|
||||||
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
|
||||||
SELECT ?, id, 0 FROM audio_files
|
|
||||||
WHERE file_path = '/music/song.mp3'`,
|
|
||||||
playlistID,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed playlist_tracks: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil {
|
|
||||||
t.Fatalf("RemoveFromLibrary: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
phantomPath := queryString(
|
|
||||||
t, db,
|
|
||||||
`SELECT phantom_file_path FROM playlist_tracks
|
|
||||||
WHERE playlist_id = ?`,
|
|
||||||
playlistID,
|
|
||||||
)
|
|
||||||
|
|
||||||
if phantomPath != "/music/song.mp3" {
|
|
||||||
t.Fatalf(
|
|
||||||
"phantom_file_path is %q, want %q -- the entry cannot be "+
|
|
||||||
"re-linked after a later full rescan",
|
|
||||||
phantomPath, "/music/song.mp3",
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// A library with tagging_items must still be removable. tagging_items
|
// A library with tagging_items must still be removable. tagging_items
|
||||||
// FK-references libraries with no ON DELETE clause, so leaving those
|
// FK-references libraries with no ON DELETE clause, so leaving those
|
||||||
// rows behind fails the DELETE and rolls back the entire removal.
|
// rows behind fails the DELETE and rolls back the entire removal.
|
||||||
@@ -249,53 +198,3 @@ func TestCoverArtFileSet(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Removing the last track of an album must take the album's cover art
|
|
||||||
// with it — both the row and every derived file — or the row keeps its
|
|
||||||
// files exempt from the janitor's covers sweep forever (#247).
|
|
||||||
func TestRemoveFromLibrary_DeletesOrphanedCoverArt(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
lib, _ := setupTestLibrary(t)
|
|
||||||
|
|
||||||
dir := t.TempDir()
|
|
||||||
|
|
||||||
// The largest tier is what cover_art.file_path names; write every
|
|
||||||
// variant so the sweep has a real set to remove.
|
|
||||||
for _, tier := range thumbnailTiers {
|
|
||||||
p := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", tier.Suffix))
|
|
||||||
if err := os.WriteFile(p, []byte("img"), 0o600); err != nil {
|
|
||||||
t.Fatalf("write %s: %v", p, err)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
cover := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", "_lg"))
|
|
||||||
|
|
||||||
seedRemovableLibrary(t, lib, cover)
|
|
||||||
|
|
||||||
// Link the album to the cover so it is not orphaned until the track
|
|
||||||
// (and with it the album) goes.
|
|
||||||
if _, err := lib.db.ExecContext(
|
|
||||||
`UPDATE albums SET cover_art_id =
|
|
||||||
(SELECT id FROM cover_art WHERE file_path = ?)
|
|
||||||
WHERE name = 'Test Album'`,
|
|
||||||
cover,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("link cover art: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil {
|
|
||||||
t.Fatalf("RemoveFromLibrary: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if n := countRows(t, lib, "cover_art"); n != 0 {
|
|
||||||
t.Errorf("cover_art has %d rows after removal, want 0", n)
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, tier := range thumbnailTiers {
|
|
||||||
p := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", tier.Suffix))
|
|
||||||
if _, err := os.Stat(p); !os.IsNotExist(err) {
|
|
||||||
t.Errorf("cover art file still present: %s", filepath.Base(p))
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -4,7 +4,6 @@ import (
|
|||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
|
||||||
"yellowjacket/backend/database"
|
|
||||||
"yellowjacket/backend/events"
|
"yellowjacket/backend/events"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -58,25 +57,6 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error)
|
|||||||
|
|
||||||
var result RemovalResult
|
var result RemovalResult
|
||||||
|
|
||||||
// Preserve the playlist entries before the rows go, so they survive
|
|
||||||
// as re-linkable phantoms rather than empty rows. The track is
|
|
||||||
// excluded and will not be re-imported on its own, but a later full
|
|
||||||
// rescan clears the exclusion and this is what lets the entry
|
|
||||||
// re-link then — the same preservation every other path that empties
|
|
||||||
// audio_files performs (#246).
|
|
||||||
rowIDs := make([]int64, len(rows))
|
|
||||||
for i, row := range rows {
|
|
||||||
rowIDs[i] = row.ID
|
|
||||||
}
|
|
||||||
|
|
||||||
if err := database.PreservePlaylistPhantomsForFiles(
|
|
||||||
l.ctx, tx, rowIDs, l.logger,
|
|
||||||
); err != nil {
|
|
||||||
return nil, fmt.Errorf(
|
|
||||||
"could not preserve playlist entries for removal: %w", err,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
// Exclude every path the caller named, including one whose row has
|
// Exclude every path the caller named, including one whose row has
|
||||||
// already gone: the user asked for that file to stay out, and a row
|
// already gone: the user asked for that file to stay out, and a row
|
||||||
// that disappeared between the click and the commit is not a reason
|
// that disappeared between the click and the commit is not a reason
|
||||||
|
|||||||
@@ -231,98 +231,3 @@ func TestScan_MultipleDirectoriesDoNotCrossContaminate(t *testing.T) {
|
|||||||
t.Errorf("Album A and Album B must not share a group_key: %+v", keys)
|
t.Errorf("Album A and Album B must not share a group_key: %+v", keys)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestScan_OrphanCleanupPreservesPlaylistPhantoms guards #246: a file
|
|
||||||
// deleted from the library folder *outside* YellowJacket is discovered
|
|
||||||
// as an orphan by the next scan, and its playlist entry must survive as
|
|
||||||
// a re-linkable phantom — the same preservation the full rescan and
|
|
||||||
// stale-retire paths already perform, scoped here to just the orphaned
|
|
||||||
// file. Before the fix the entry became an empty row (audio_file_id
|
|
||||||
// NULL and no phantom_file_path), which nothing can ever re-link.
|
|
||||||
func TestScan_OrphanCleanupPreservesPlaylistPhantoms(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
lib, db := setupTestLibrary(t)
|
|
||||||
|
|
||||||
root := t.TempDir()
|
|
||||||
track := filepath.Join(root, "gone.mp3")
|
|
||||||
|
|
||||||
writeTestTrack(t, track, 0)
|
|
||||||
|
|
||||||
library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{
|
|
||||||
Name: "orphans",
|
|
||||||
Path: root,
|
|
||||||
})
|
|
||||||
if err != nil {
|
|
||||||
t.Fatalf("create library: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
|
|
||||||
t.Fatal("first scan returned nil metrics")
|
|
||||||
}
|
|
||||||
|
|
||||||
trackID := queryInt(
|
|
||||||
t, db, "SELECT id FROM audio_files WHERE file_path = ?", track,
|
|
||||||
)
|
|
||||||
if trackID == 0 {
|
|
||||||
t.Fatal("first scan did not import the track")
|
|
||||||
}
|
|
||||||
|
|
||||||
if _, err := db.ExecContext(
|
|
||||||
`INSERT INTO playlists (name) VALUES ('keepme')`,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed playlist: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
playlistID := queryInt(
|
|
||||||
t, db, "SELECT id FROM playlists WHERE name = 'keepme'",
|
|
||||||
)
|
|
||||||
|
|
||||||
if _, err := db.ExecContext(
|
|
||||||
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
|
||||||
VALUES (?, ?, 0)`,
|
|
||||||
playlistID, trackID,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed playlist_tracks: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
// The file goes away outside the app.
|
|
||||||
if err := os.Remove(track); err != nil {
|
|
||||||
t.Fatalf("remove track: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
|
|
||||||
t.Fatal("second scan returned nil metrics")
|
|
||||||
}
|
|
||||||
|
|
||||||
if n := queryInt(
|
|
||||||
t, db, "SELECT COUNT(*) FROM audio_files WHERE file_path = ?", track,
|
|
||||||
); n != 0 {
|
|
||||||
t.Fatalf("audio_files still holds the removed path: %d rows", n)
|
|
||||||
}
|
|
||||||
|
|
||||||
if n := queryInt(
|
|
||||||
t, db,
|
|
||||||
"SELECT COUNT(*) FROM playlist_tracks WHERE playlist_id = ? "+
|
|
||||||
"AND audio_file_id IS NULL",
|
|
||||||
playlistID,
|
|
||||||
); n != 1 {
|
|
||||||
t.Fatalf(
|
|
||||||
"playlist entry did not become a phantom: %d null-id rows, want 1",
|
|
||||||
n,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
phantomPath := queryString(
|
|
||||||
t, db,
|
|
||||||
"SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = ?",
|
|
||||||
playlistID,
|
|
||||||
)
|
|
||||||
if phantomPath != track {
|
|
||||||
t.Fatalf(
|
|
||||||
"phantom_file_path = %q, want %q -- the entry cannot be "+
|
|
||||||
"re-linked if the file comes back",
|
|
||||||
phantomPath, track,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -667,74 +667,6 @@ func TestExpiredHTTPCacheJob_TrimsToBudget(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestStaleArtistMetadataJob pins the sweep's two keep rules: an owned
|
|
||||||
// artist's metadata survives, a browsed artist's survives while it still
|
|
||||||
// holds cached artwork, and everything else goes (#248).
|
|
||||||
func TestStaleArtistMetadataJob(t *testing.T) {
|
|
||||||
t.Parallel()
|
|
||||||
|
|
||||||
db := database.NewTestDB(t)
|
|
||||||
|
|
||||||
const (
|
|
||||||
ownedMBID = "11111111-1111-1111-1111-111111111111"
|
|
||||||
browsedMBID = "22222222-2222-2222-2222-222222222222"
|
|
||||||
staleMBID = "33333333-3333-3333-3333-333333333333"
|
|
||||||
)
|
|
||||||
|
|
||||||
// The owned artist is in the library - which means a *file* says
|
|
||||||
// so. An artists row on its own is not ownership.
|
|
||||||
database.InsertTestTrack(t, db, database.TestTrack{
|
|
||||||
FilePath: "/music/owned.mp3",
|
|
||||||
Artist: "Owned",
|
|
||||||
ArtistMBID: ownedMBID,
|
|
||||||
})
|
|
||||||
|
|
||||||
for _, mbid := range []string{ownedMBID, browsedMBID, staleMBID} {
|
|
||||||
if _, err := db.ExecContext(
|
|
||||||
`INSERT INTO artist_metadata (mbid, source, data, fetched_at)
|
|
||||||
VALUES (?, 'wikidata-p18', x'00', CURRENT_TIMESTAMP)`,
|
|
||||||
mbid,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed artist_metadata for %s: %v", mbid, err)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// The browsed artist holds cached artwork, so its metadata is still
|
|
||||||
// referenced and must survive.
|
|
||||||
if _, err := db.ExecContext(
|
|
||||||
`INSERT INTO artist_images
|
|
||||||
(artist_mbid, source, source_url, file_path)
|
|
||||||
VALUES (?, 'test', 'http://x', '/art/primary.jpg')`,
|
|
||||||
browsedMBID,
|
|
||||||
); err != nil {
|
|
||||||
t.Fatalf("seed artist_images: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if _, err := StaleArtistMetadataJob(db).Run(context.Background()); err != nil {
|
|
||||||
t.Fatalf("run job: %v", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
for _, tc := range []struct {
|
|
||||||
mbid string
|
|
||||||
want int
|
|
||||||
}{
|
|
||||||
{ownedMBID, 1},
|
|
||||||
{browsedMBID, 1},
|
|
||||||
{staleMBID, 0},
|
|
||||||
} {
|
|
||||||
var n int
|
|
||||||
if err := db.QueryRowWriter(
|
|
||||||
"SELECT COUNT(*) FROM artist_metadata WHERE mbid = ?", tc.mbid,
|
|
||||||
).Scan(&n); err != nil {
|
|
||||||
t.Fatalf("count %s: %v", tc.mbid, err)
|
|
||||||
}
|
|
||||||
|
|
||||||
if n != tc.want {
|
|
||||||
t.Errorf("artist_metadata rows for %s = %d, want %d", tc.mbid, n, tc.want)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// TestStaleSearchClicksJob deletes only the clicks old enough to have
|
// TestStaleSearchClicksJob deletes only the clicks old enough to have
|
||||||
// left the retention window (#249).
|
// left the retention window (#249).
|
||||||
func TestStaleSearchClicksJob(t *testing.T) {
|
func TestStaleSearchClicksJob(t *testing.T) {
|
||||||
|
|||||||
@@ -629,40 +629,6 @@ func dirSize(dir string) (bytes, files int64) {
|
|||||||
return bytes, files
|
return bytes, files
|
||||||
}
|
}
|
||||||
|
|
||||||
// StaleArtistMetadataJob evicts long-lived artist metadata (bios, wiki
|
|
||||||
// leads, relationships) for artists the user no longer has any reason
|
|
||||||
// to keep around: not owned and holding no cached artwork.
|
|
||||||
//
|
|
||||||
// artist_metadata has no TTL by design — entity data changes rarely and
|
|
||||||
// re-fetching spends someone else's rate limit — so without a sweep it
|
|
||||||
// grows for the life of the install. This is the "swept when the
|
|
||||||
// artist is no longer referenced" contract the datamap always declared
|
|
||||||
// for it and nothing ever performed (#248).
|
|
||||||
func StaleArtistMetadataJob(db *database.DB) Job {
|
|
||||||
return Job{
|
|
||||||
Name: "artist-metadata-sweep",
|
|
||||||
MinInterval: dailyInterval,
|
|
||||||
Run: func(_ context.Context) (Result, error) {
|
|
||||||
res, err := db.ExecContext(
|
|
||||||
`DELETE FROM artist_metadata
|
|
||||||
WHERE mbid NOT IN (` + ownedArtistMBIDs + `)
|
|
||||||
AND mbid NOT IN (
|
|
||||||
SELECT artist_mbid FROM artist_images
|
|
||||||
)`,
|
|
||||||
)
|
|
||||||
if err != nil {
|
|
||||||
return Result{}, fmt.Errorf(
|
|
||||||
"delete stale artist_metadata rows: %w", err,
|
|
||||||
)
|
|
||||||
}
|
|
||||||
|
|
||||||
rows, _ := res.RowsAffected()
|
|
||||||
|
|
||||||
return Result{RowsDeleted: rows}, nil
|
|
||||||
},
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// searchClicksRetention is how long a search-click ranking signal stays
|
// searchClicksRetention is how long a search-click ranking signal stays
|
||||||
// useful. search_clicks is authored behavioural data — nothing that
|
// useful. search_clicks is authored behavioural data — nothing that
|
||||||
// owns a row ever drops it — so age is the ceiling that keeps the table
|
// owns a row ever drops it — so age is the ceiling that keeps the table
|
||||||
|
|||||||
@@ -60,23 +60,6 @@ const PHONE = { width: 424, height: 439 };
|
|||||||
const DESKTOP = { width: 1100, height: 800 };
|
const DESKTOP = { width: 1100, height: 800 };
|
||||||
|
|
||||||
test.describe('background jobs on a phone', () => {
|
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 ({
|
test('are shown in the band, without opening anything', async ({
|
||||||
app,
|
app,
|
||||||
testctl,
|
testctl,
|
||||||
|
|||||||
@@ -171,8 +171,8 @@ test.describe('search on a phone', () => {
|
|||||||
// Attached, not visible: `wa-dialog`'s host is `display: contents`,
|
// Attached, not visible: `wa-dialog`'s host is `display: contents`,
|
||||||
// so the element carrying the testid always reports hidden — what
|
// so the element carrying the testid always reports hidden — what
|
||||||
// is visible is the native `<dialog>` inside it. That awkwardness
|
// is visible is the native `<dialog>` inside it. That awkwardness
|
||||||
// is why the assertion that this is really up is the role query
|
// is written down in CLAUDE.md and is why the assertion that this
|
||||||
// below.
|
// is really up is the role query below.
|
||||||
await expect(dialog).toBeAttached();
|
await expect(dialog).toBeAttached();
|
||||||
|
|
||||||
// Named, which `getByRole` can answer and the a11y snapshot cannot
|
// Named, which `getByRole` can answer and the a11y snapshot cannot
|
||||||
|
|||||||
@@ -102,22 +102,6 @@ const collapsed = (page: Page) =>
|
|||||||
}));
|
}));
|
||||||
|
|
||||||
test.describe('the top bar fits the window', () => {
|
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).
|
* The phone's answer, which is not "it fits" (#57).
|
||||||
*
|
*
|
||||||
|
|||||||
@@ -56,16 +56,6 @@ export function CycleRepeat(): $CancellablePromise<void> {
|
|||||||
return $Call.ByID(3510519482);
|
return $Call.ByID(3510519482);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* DropSourceForPlaylist clears the queue's "Playing from" label when
|
|
||||||
* its source playlist is deleted. A link back to a playlist that no
|
|
||||||
* longer exists is worse than none, and the label otherwise survives
|
|
||||||
* the deletion until the next SetQueue (#249).
|
|
||||||
*/
|
|
||||||
export function DropSourceForPlaylist(playlistID: number): $CancellablePromise<void> {
|
|
||||||
return $Call.ByID(1435106374, playlistID);
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* EmitCurrentState emits the current queue state to the frontend.
|
* EmitCurrentState emits the current queue state to the frontend.
|
||||||
* This is called after the frontend DOM is ready.
|
* This is called after the frontend DOM is ready.
|
||||||
|
|||||||
@@ -172,7 +172,8 @@ describe('<player-progress-line>', () => {
|
|||||||
* The reason this component asks `matchMedia` instead of letting a
|
* The reason this component asks `matchMedia` instead of letting a
|
||||||
* stylesheet hide it: a media query cannot stop a 1 Hz interval
|
* stylesheet hide it: a media query cannot stop a 1 Hz interval
|
||||||
* running for the life of every desktop session. That claim is
|
* running for the life of every desktop session. That claim is
|
||||||
* load-bearing, so it is asserted rather than described — the timer count, because a desktop render is empty
|
* load-bearing in CLAUDE.md, so it is asserted rather than
|
||||||
|
* described — the timer count, because a desktop render is empty
|
||||||
* either way and so cannot tell the two apart.
|
* either way and so cannot tell the two apart.
|
||||||
*/
|
*/
|
||||||
it('runs no interpolation timer above the breakpoint', async () => {
|
it('runs no interpolation timer above the breakpoint', async () => {
|
||||||
|
|||||||
@@ -64,7 +64,8 @@ var frontendDistAssets embed.FS
|
|||||||
// Returning early is not a degraded mode: `nativeInit` has already
|
// Returning early is not a degraded mode: `nativeInit` has already
|
||||||
// re-attached the bridge, so the recreated activity's WebView talks to
|
// re-attached the bridge, so the recreated activity's WebView talks to
|
||||||
// the app that is still running, with its queue and its playback
|
// the app that is still running, with its queue and its playback
|
||||||
// position intact.
|
// position intact. See CLAUDE.md, "An activity is a view onto the
|
||||||
|
// process".
|
||||||
//
|
//
|
||||||
// It is inert off Android, where a process has exactly one main().
|
// It is inert off Android, where a process has exactly one main().
|
||||||
var mainStarted atomic.Bool
|
var mainStarted atomic.Bool
|
||||||
|
|||||||
+7
-62
@@ -82,69 +82,23 @@ targets="$({ make -pqRr 2>/dev/null || true; } |
|
|||||||
# happened to break there, and a check that fails on reflow gets
|
# happened to break there, and a check that fails on reflow gets
|
||||||
# disabled rather than fixed.
|
# 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
|
# AGENTS.md is deliberately not in this list: it is a symlink to
|
||||||
# CLAUDE.md, asserted above, so scanning it would report every failure
|
# CLAUDE.md, asserted above, so scanning it would report every failure
|
||||||
# twice under two names.
|
# twice under two names.
|
||||||
mentioned="$(printf '%s\n' "$docs" |
|
mentioned="$(printf '%s\n' "$docs" |
|
||||||
xargs awk '
|
xargs awk '
|
||||||
function scan(text, rest) {
|
FNR == 1 { fence = 0 }
|
||||||
rest = text
|
/^```/ { fence = !fence; next }
|
||||||
|
{
|
||||||
|
rest = $0
|
||||||
while (match(rest, /`make [a-z][a-z0-9-]*/)) {
|
while (match(rest, /`make [a-z][a-z0-9-]*/)) {
|
||||||
print substr(rest, RSTART + 6, RLENGTH - 6)
|
print substr(rest, RSTART + 6, RLENGTH - 6)
|
||||||
rest = substr(rest, RSTART + RLENGTH)
|
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)"
|
' | sort -u)"
|
||||||
|
|
||||||
missing=""
|
missing=""
|
||||||
@@ -159,16 +113,7 @@ if [ -n "$missing" ]; then
|
|||||||
echo "skill-check: the docs name make targets that do not exist:" >&2
|
echo "skill-check: the docs name make targets that do not exist:" >&2
|
||||||
for t in $missing; do
|
for t in $missing; do
|
||||||
echo " make $t" >&2
|
echo " make $t" >&2
|
||||||
# `make <t>` on one line first, because that is where a target is
|
printf '%s\n' "$docs" | xargs grep -ln "make $t" | sed 's/^/ /' >&2
|
||||||
# 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
|
done
|
||||||
echo "Fix the docs, or restore the target." >&2
|
echo "Fix the docs, or restore the target." >&2
|
||||||
exit 1
|
exit 1
|
||||||
|
|||||||
Reference in New Issue
Block a user