diff --git a/backend/database/sql/queries/albums.sql b/backend/database/sql/queries/albums.sql index ec7001f..5922157 100644 --- a/backend/database/sql/queries/albums.sql +++ b/backend/database/sql/queries/albums.sql @@ -40,7 +40,8 @@ WHERE id = ? AND (mbid IS NULL OR mbid = ''); -- name: GetAlbumsWithPendingReleaseMBID :many SELECT id, pending_release_mbid FROM albums WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != '' - AND (mbid IS NULL OR mbid = ''); + AND (mbid IS NULL OR mbid = '') +LIMIT ?; -- name: DeleteAlbum :exec DELETE FROM albums WHERE id = ?; diff --git a/backend/database/sql/sqlcgen/albums.sql.go b/backend/database/sql/sqlcgen/albums.sql.go index 3acb204..5a80196 100644 --- a/backend/database/sql/sqlcgen/albums.sql.go +++ b/backend/database/sql/sqlcgen/albums.sql.go @@ -331,6 +331,7 @@ const getAlbumsWithPendingReleaseMBID = `-- name: GetAlbumsWithPendingReleaseMBI SELECT id, pending_release_mbid FROM albums WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != '' AND (mbid IS NULL OR mbid = '') +LIMIT ? ` type GetAlbumsWithPendingReleaseMBIDRow struct { @@ -338,8 +339,8 @@ type GetAlbumsWithPendingReleaseMBIDRow struct { PendingReleaseMbid sql.NullString } -func (q *Queries) GetAlbumsWithPendingReleaseMBID(ctx context.Context) ([]GetAlbumsWithPendingReleaseMBIDRow, error) { - rows, err := q.db.QueryContext(ctx, getAlbumsWithPendingReleaseMBID) +func (q *Queries) GetAlbumsWithPendingReleaseMBID(ctx context.Context, limit int64) ([]GetAlbumsWithPendingReleaseMBIDRow, error) { + rows, err := q.db.QueryContext(ctx, getAlbumsWithPendingReleaseMBID, limit) if err != nil { return nil, err } diff --git a/backend/explore/explore.go b/backend/explore/explore.go index 6b9304f..501a8e5 100644 --- a/backend/explore/explore.go +++ b/backend/explore/explore.go @@ -2,6 +2,7 @@ package explore import ( "context" + "database/sql" "log/slog" "math" "sort" @@ -12,6 +13,7 @@ import ( "golang.org/x/sync/singleflight" "yellowjacket/backend/database" + "yellowjacket/backend/database/sql/sqlcgen" "yellowjacket/backend/events" "yellowjacket/backend/jobs" ) @@ -279,37 +281,32 @@ func (e *Service) BackfillReleaseGroupMBIDs() { go e.backfillReleaseGroupMBIDs(e.ctx) } -func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) { - rows, err := e.db.QueryContext( - "SELECT id, pending_release_mbid FROM release_groups "+ - "WHERE (mbid IS NULL OR mbid = '') "+ - "AND pending_release_mbid IS NOT NULL AND pending_release_mbid != '' "+ - "LIMIT ?", - releaseGroupMBIDBackfillMaxPerRun, +// pendingReleaseMBIDs is the albums this pass has work to do on. +// +// It is separate from the pass, and returns its error rather than +// logging it, so that a test can assert the statement runs against the +// real schema. That is not a general preference -- it is this +// statement's history: it named `release_groups`, a table plan 013 +// renamed to `albums`, so it failed on every launch since e7748f1 and +// the pass returned quietly having done nothing. A test of the pass +// as a whole cannot see that, because a query error and an empty +// library are the same early return. +func (e *Service) pendingReleaseMBIDs( + ctx context.Context, +) ([]sqlcgen.GetAlbumsWithPendingReleaseMBIDRow, error) { + return e.db.ReadQueries.GetAlbumsWithPendingReleaseMBID( + ctx, releaseGroupMBIDBackfillMaxPerRun, ) +} + +func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) { + pending, err := e.pendingReleaseMBIDs(ctx) if err != nil { e.logger.Warn("release-group mbid backfill: query failed", "error", err) return } - type pendingRow struct { - id int64 - releaseMBID string - } - - var pending []pendingRow - - for rows.Next() { - var p pendingRow - - if err := rows.Scan(&p.id, &p.releaseMBID); err == nil { - pending = append(pending, p) - } - } - - _ = rows.Close() - if len(pending) == 0 { return } @@ -335,7 +332,7 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) { job.progress(i, len(pending)) - release, err := e.mb.LookupRelease(ctx, p.releaseMBID) + release, err := e.mb.LookupRelease(ctx, p.PendingReleaseMbid.String) if err != nil || release.ReleaseGroupMBID == "" { // Left alone rather than cleared: LookupRelease caches its // answer (success or a release with no group) for 7 days, @@ -344,12 +341,19 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) { continue } - _, err = e.db.ExecContext( - "UPDATE release_groups SET mbid = ?, pending_release_mbid = NULL "+ - "WHERE id = ? AND (mbid IS NULL OR mbid = '')", - release.ReleaseGroupMBID, p.id, - ) - if err != nil { + // The writer, not ReadQueries: an UPDATE issued on the + // query-only pool fails at runtime with "attempt to write a + // readonly database". + if err := e.db.Queries.ResolveAlbumPendingReleaseMBID( + ctx, + sqlcgen.ResolveAlbumPendingReleaseMBIDParams{ + Mbid: sql.NullString{ + String: release.ReleaseGroupMBID, + Valid: true, + }, + ID: p.ID, + }, + ); err != nil { e.logger.Warn("release-group mbid backfill: update failed", "error", err) } } diff --git a/backend/explore/pendingreleasembid_test.go b/backend/explore/pendingreleasembid_test.go new file mode 100644 index 0000000..a14f59d --- /dev/null +++ b/backend/explore/pendingreleasembid_test.go @@ -0,0 +1,250 @@ +package explore + +import ( + "database/sql" + "log/slog" + "strconv" + "testing" + + "yellowjacket/backend/database" + "yellowjacket/backend/database/sql/sqlcgen" +) + +// The release-group MBID backfill queried `release_groups`, a table +// plan 013 renamed to `albums`, so it failed on its first statement on +// every launch from e7748f1 until #189 -- and the pass swallowed that, +// because a query error and an empty library are the same early +// return. Nothing noticed for two reasons worth keeping in mind: +// +// - the statement was **raw SQL**, so sqlc never read it. Every other +// statement in the repo was renamed by the same change because sqlc +// reads sql/schemas/ and cannot generate against a table that is not +// declared. The two sqlc queries this now calls were written by 013 +// and left uncalled. +// - it needs no network and no fixture library to reproduce. The +// failure is at prepare time. + +// seedPendingAlbum inserts an album whose files carried a release MBID +// but no release-group MBID, which is what `library.updateMBIDs` +// leaves behind for this pass to resolve. +func seedPendingAlbum( + t *testing.T, + db *database.DB, + name, pendingMBID string, +) int64 { + t.Helper() + + res, err := db.ExecContext( + "INSERT INTO albums (name, artist_credit, pending_release_mbid) "+ + "VALUES (?, ?, ?)", + name, "Test Artist", pendingMBID, + ) + if err != nil { + t.Fatalf("insert albums row: %v", err) + } + + id, err := res.LastInsertId() + if err != nil { + t.Fatalf("last insert id: %v", err) + } + + return id +} + +func newPendingTestService(db *database.DB) *Service { + return &Service{db: db, logger: slog.Default()} +} + +// TestPendingReleaseMBIDsRunsAgainstTheRealSchema is the regression. +// +// It asserts the statement *runs*, which is the whole of what was +// broken: against the old raw SQL this returns +// "no such table: release_groups" rather than a row. +func TestPendingReleaseMBIDsRunsAgainstTheRealSchema(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + e := newPendingTestService(db) + + want := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1") + + pending, err := e.pendingReleaseMBIDs(db.Ctx) + if err != nil { + t.Fatalf("the backfill's query failed: %v", err) + } + + if len(pending) != 1 { + t.Fatalf("got %d pending albums, want 1", len(pending)) + } + + if pending[0].ID != want { + t.Errorf("got album id %d, want %d", pending[0].ID, want) + } + + if got := pending[0].PendingReleaseMbid.String; got != "release-mbid-1" { + t.Errorf("got pending mbid %q, want %q", got, "release-mbid-1") + } +} + +// TestOnlyUnresolvedAlbumsAreReturned pins the two conditions that make +// the pass idempotent, since between them they are what stops it doing +// the same MusicBrainz lookups on every launch forever. +func TestOnlyUnresolvedAlbumsAreReturned(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + e := newPendingTestService(db) + + pendingID := seedPendingAlbum(t, db, "Still Pending", "release-mbid-1") + + // Already resolved: it has a real MBID, so there is nothing to + // look up even though a marker is still sitting on it. + resolved := seedPendingAlbum(t, db, "Already Resolved", "release-mbid-2") + if err := db.Queries.SetAlbumMBID(db.Ctx, sqlcgen.SetAlbumMBIDParams{ + Mbid: sql.NullString{String: "rg-mbid", Valid: true}, + ID: resolved, + }); err != nil { + t.Fatalf("set album mbid: %v", err) + } + + // Never had a release MBID to resolve in the first place, which is + // most of a library. + seedPendingAlbum(t, db, "Nothing Pending", "") + + pending, err := e.pendingReleaseMBIDs(db.Ctx) + if err != nil { + t.Fatalf("the backfill's query failed: %v", err) + } + + if len(pending) != 1 || pending[0].ID != pendingID { + t.Fatalf( + "got %d albums %v, want only the unresolved one (%d)", + len(pending), pending, pendingID, + ) + } +} + +// TestResolvingClearsTheMarker is the other half: once the lookup has +// answered, the album must stop being a candidate, or the pass repeats +// the same live MusicBrainz call on every launch. +func TestResolvingClearsTheMarker(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + e := newPendingTestService(db) + + id := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1") + + // The writer, deliberately: this is an UPDATE, and the read pool + // would refuse it at runtime. + if err := db.Queries.ResolveAlbumPendingReleaseMBID( + db.Ctx, + sqlcgen.ResolveAlbumPendingReleaseMBIDParams{ + Mbid: sql.NullString{String: "resolved-rg-mbid", Valid: true}, + ID: id, + }, + ); err != nil { + t.Fatalf("resolve pending release mbid: %v", err) + } + + pending, err := e.pendingReleaseMBIDs(db.Ctx) + if err != nil { + t.Fatalf("the backfill's query failed: %v", err) + } + + if len(pending) != 0 { + t.Fatalf("a resolved album is still a candidate: %v", pending) + } + + album, err := db.ReadQueries.GetAlbum(db.Ctx, id) + if err != nil { + t.Fatalf("get album: %v", err) + } + + if album.Mbid.String != "resolved-rg-mbid" { + t.Errorf("album mbid = %q, want the resolved one", album.Mbid.String) + } + + if album.PendingReleaseMbid.Valid && + album.PendingReleaseMbid.String != "" { + t.Errorf( + "the pending marker survived as %q", + album.PendingReleaseMbid.String, + ) + } +} + +// TestAResolvedMBIDIsNeverOverwritten covers the guard in the UPDATE. +// +// The pass runs against rows it read earlier, and a rescan can resolve +// an album from its tags in between -- a real MBID from the file must +// win over one this pass inferred from a release. +func TestAResolvedMBIDIsNeverOverwritten(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + + id := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1") + + if err := db.Queries.SetAlbumMBID(db.Ctx, sqlcgen.SetAlbumMBIDParams{ + Mbid: sql.NullString{String: "from-the-tags", Valid: true}, + ID: id, + }); err != nil { + t.Fatalf("set album mbid: %v", err) + } + + if err := db.Queries.ResolveAlbumPendingReleaseMBID( + db.Ctx, + sqlcgen.ResolveAlbumPendingReleaseMBIDParams{ + Mbid: sql.NullString{String: "from-the-backfill", Valid: true}, + ID: id, + }, + ); err != nil { + t.Fatalf("resolve pending release mbid: %v", err) + } + + album, err := db.ReadQueries.GetAlbum(db.Ctx, id) + if err != nil { + t.Fatalf("get album: %v", err) + } + + if album.Mbid.String != "from-the-tags" { + t.Errorf( + "album mbid = %q, want the tagged one to have won", + album.Mbid.String, + ) + } +} + +// TestThePassIsBounded checks the LIMIT. +// +// Each row costs a live MusicBrainz lookup on a 1 req/s limiter shared +// with every page the user can open, so an unbounded read is a run that +// lasts as long as the library is untagged. The sqlc query 013 wrote +// had no LIMIT; the raw statement it was replacing did. +func TestThePassIsBounded(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + e := newPendingTestService(db) + + for i := range releaseGroupMBIDBackfillMaxPerRun + 10 { + seedPendingAlbum( + t, db, + "Album "+string(rune('A'+i%26))+strconv.Itoa(i), + "release-mbid-"+strconv.Itoa(i), + ) + } + + pending, err := e.pendingReleaseMBIDs(db.Ctx) + if err != nil { + t.Fatalf("the backfill's query failed: %v", err) + } + + if len(pending) != releaseGroupMBIDBackfillMaxPerRun { + t.Errorf( + "got %d albums, want the run bounded at %d", + len(pending), releaseGroupMBIDBackfillMaxPerRun, + ) + } +} diff --git a/backend/system/userdata.go b/backend/system/userdata.go index 9f232fe..665c962 100644 --- a/backend/system/userdata.go +++ b/backend/system/userdata.go @@ -52,6 +52,75 @@ func UseHomeOverride(base string) { _ = os.Setenv(envHomeOverride, base) } +// envTempDir is the variable Go's os.TempDir() reads, and through it +// every library in the process that asks for a temporary file. +const envTempDir = "TMPDIR" + +// tempDirName is the subdirectory of the app's own storage that +// becomes that answer. +const tempDirName = "tmp" + +// UseTempDir gives the process a temporary directory that exists. +// +// **Android has no /tmp and hands an app no TMPDIR**, and Go's +// os.TempDir() falls back to "/tmp" when the variable is unset -- so +// every library in the process that wants scratch space is handed a +// path that has never existed. SQLite is the one that noticed: an +// INSERT ... SELECT large enough to spill returned +// SQLITE_IOERR_GETTEMPPATH (disk I/O error 6410), which is how the +// champion search index came to fail its rebuild on every launch while +// the app otherwise looked healthy (#190). +// +// It is the *class* that is fixed here rather than that statement. +// Anything that spills fails the same way on that platform -- large +// sorts, large joins, VACUUM -- so the repair belongs at the process's +// one answer to the question rather than at each caller. The +// alternative considered was PRAGMA temp_store = MEMORY, which is +// cheaper and more local and is a promise that every future spill fits +// in RAM on a phone; the catalog is the largest thing in this app and +// that is not a promise worth making silently. +// +// The rules are UseHomeOverride's, for the same reasons. **An empty +// base is a no-op**, because that is what +// application.Mobile.StoragePath() returns on desktop -- so this needs +// no build tag and changes nothing off mobile, where /tmp is real. And +// **an explicit TMPDIR wins**, so anyone who set one deliberately gets +// what they asked for; nothing sets it on the platform this exists for. +// +// It returns its error rather than swallowing it because a temp +// directory that could not be created is the same silent failure one +// step earlier, and since #160 a log line on that platform is +// something a person can actually read. +func UseTempDir(base string) error { + if base == "" || os.Getenv(envTempDir) != "" { + return nil + } + + dir := filepath.Join(base, tempDirName) + + if err := os.MkdirAll(dir, os.ModePerm); err != nil { + return fmt.Errorf("could not make the temp directory %s: %w", dir, err) + } + + // Writability is checked rather than assumed: the whole failure + // this repairs is a directory that is named and cannot be used, and + // MkdirAll on an existing unwritable directory succeeds. + probe, err := os.CreateTemp(dir, "probe") + if err != nil { + return fmt.Errorf("temp directory %s is not writable: %w", dir, err) + } + + name := probe.Name() + _ = probe.Close() + _ = os.Remove(name) + + if err := os.Setenv(envTempDir, dir); err != nil { + return fmt.Errorf("could not set %s: %w", envTempDir, err) + } + + return nil +} + // getUserDirPath returns and creates the path for a user directory. func getUserDirPath(dt dirType) (string, error) { path, err := resolveUserDirPath(dt) diff --git a/backend/system/userdata_test.go b/backend/system/userdata_test.go index 3829360..42030a1 100644 --- a/backend/system/userdata_test.go +++ b/backend/system/userdata_test.go @@ -87,3 +87,116 @@ func TestUseHomeOverride(t *testing.T) { }) } } + +// UseTempDir carries UseHomeOverride's two rules for the same reasons, +// plus one of its own: the directory it names has to be usable. +// +// **The only tier that can compile the platform this exists for is a +// phone**, so everything decidable off one is decided here -- which is +// androidpayload.go's discipline, and is why the platform call is a +// parameter rather than something this package reaches for. The +// device's half is a single measurement: no /tmp, no TMPDIR (#190). +func TestUseTempDir(t *testing.T) { + t.Run("an empty base is a no-op", func(t *testing.T) { + // This is the desktop case in full: StoragePath() answers "" + // off mobile, where /tmp is real and must be left alone. + t.Setenv(envTempDir, "") + + if err := UseTempDir(""); err != nil { + t.Fatalf("UseTempDir(\"\") = %v, want nil", err) + } + + if got := os.Getenv(envTempDir); got != "" { + t.Errorf("%s = %q, want it untouched", envTempDir, got) + } + }) + + t.Run("an explicit TMPDIR wins", func(t *testing.T) { + const chosen = "/somewhere/deliberate" + + // The base is taken before TMPDIR moves, because t.TempDir() + // reads TMPDIR too -- which is the same fact this function is + // about, met from the other side. + base := t.TempDir() + + t.Setenv(envTempDir, chosen) + + if err := UseTempDir(base); err != nil { + t.Fatalf("UseTempDir = %v, want nil", err) + } + + if got := os.Getenv(envTempDir); got != chosen { + t.Errorf("%s = %q, want the explicit %q", envTempDir, got, chosen) + } + }) + + t.Run("points at a real directory under the base", func(t *testing.T) { + base := t.TempDir() + + t.Setenv(envTempDir, "") + + if err := UseTempDir(base); err != nil { + t.Fatalf("UseTempDir = %v, want nil", err) + } + + got := os.Getenv(envTempDir) + + want := filepath.Join(base, tempDirName) + if got != want { + t.Fatalf("%s = %q, want %q", envTempDir, got, want) + } + + // The whole failure being repaired is a temp directory that is + // named and does not exist, so naming one is not enough. + info, err := os.Stat(got) + if err != nil { + t.Fatalf("the temp directory was named but not created: %v", err) + } + + if !info.IsDir() { + t.Fatalf("%s is not a directory", got) + } + }) + + t.Run("os.TempDir then answers with it", func(t *testing.T) { + // The point of setting the variable at all: this is what every + // library in the process reads, SQLite's driver included. + base := t.TempDir() + + t.Setenv(envTempDir, "") + + if err := UseTempDir(base); err != nil { + t.Fatalf("UseTempDir = %v, want nil", err) + } + + if got := os.TempDir(); got != filepath.Join(base, tempDirName) { + t.Errorf("os.TempDir() = %q, want the directory we made", got) + } + }) + + t.Run("an unwritable directory is an error, not a silent success", func(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root can write anywhere, so there is nothing to refuse") + } + + base := t.TempDir() + + // MkdirAll on an existing directory succeeds whatever its + // mode, so without the write probe this case would set TMPDIR + // to a directory nothing can use -- which is the bug again, + // one directory over. + if err := os.Mkdir(filepath.Join(base, tempDirName), 0o500); err != nil { + t.Fatalf("prepare the unwritable directory: %v", err) + } + + t.Setenv(envTempDir, "") + + if err := UseTempDir(base); err == nil { + t.Fatal("UseTempDir accepted a directory it cannot write to") + } + + if got := os.Getenv(envTempDir); got != "" { + t.Errorf("%s was set to %q despite the failure", envTempDir, got) + } + }) +} diff --git a/main.go b/main.go index b489659..e853b3e 100644 --- a/main.go +++ b/main.go @@ -113,6 +113,19 @@ func main() { slog.SetDefault(sLogger) sLogger.Info("starting yellowjacket", "version", version, "commit", commit) + // Android has no /tmp and gives an app no TMPDIR, so anything in + // this process that spills to a temporary file is handed a path that + // does not exist -- see system.UseTempDir. It runs here rather than + // beside UseHomeOverride above because it has something to say when + // it fails and the logger does not exist up there; what matters is + // that it is before NewYellowJacketApp, which opens the database. + // + // A failure is not fatal: it leaves the platform's own answer in + // place, which is what every release before this one ran with. + if err := system.UseTempDir(application.Mobile.StoragePath()); err != nil { + sLogger.Error("could not set up a temp directory", "err", err.Error()) + } + // Start profiling server (pprof + trace). In production builds this // is a no-op — the compiler eliminates all profiling code. stopProfiler := profiling.Start(sLogger)