From d034d6e57102a33ccd08fd7293d0351f96fc9db1 Mon Sep 17 00:00:00 2001 From: Logan Date: Fri, 21 Aug 2026 17:23:05 -0400 Subject: [PATCH 1/2] fix(explore): resolve pending release MBIDs against the real table The release-group MBID backfill queried `release_groups`, which plan 013 renamed to `albums`. It failed on its first statement on every launch since e7748f1 and the pass returned quietly having done nothing: W/yellowjacket: msg="release-group mbid backfill: query failed" explore.error="SQL logic error: no such table: release_groups (1)" What it does is resolve a release-level MBID (MUSICBRAINZ_ALBUMID, which many taggers write instead of MUSICBRAINZ_RELEASEGROUPID) into the release-group MBID everything else on the album page is keyed by. A scan cannot afford a live MusicBrainz call, so `library.updateMBIDs` stashes the release MBID in `pending_release_mbid` and defers to this. With this broken the marker was written by every scan and resolved by nothing, so those albums were untagged as far as the catalog is concerned, permanently. **The fix is to call the queries plan 013 already wrote.** `GetAlbumsWithPendingReleaseMBID` and `ResolveAlbumPendingReleaseMBID` have been in sql/queries/albums.sql since that change, generated and never called -- the writer of the marker was repointed at `albums` and the reader was not. So this is not a missed rename so much as a call site left behind, and thirty lines of raw SQL and hand-rolled scanning become three. That is also the durable half. These two were the last raw-SQL references to a schema table in the tree, and being raw is exactly why 013 missed them: sqlc reads sql/schemas/ and cannot generate against a table that is not declared, which is what made every other statement in the repo immune to the same rename. Three smaller things. **The LIMIT came back.** The raw statement bounded a run at releaseGroupMBIDBackfillMaxPerRun and 013's sqlc replacement had no LIMIT at all, so switching over as-written would have swapped a dead pass for an unbounded one -- each row is a live MusicBrainz lookup on a 1 req/s limiter shared with every page the user can open. **The UPDATE goes through the writer.** `ReadQueries` is a query-only pool and an UPDATE issued on it fails at runtime with "attempt to write a readonly database". **The query is its own method so its failure is assertable.** A test of the pass as a whole cannot see this bug, because a query error and an empty library are the same early return -- which is the whole reason it survived. `pendingReleaseMBIDs` returns the error, and the test reproduces the device's exact message against the old statement. Verified on the reference device: the warning is gone from logcat. Closes #189 --- backend/database/sql/queries/albums.sql | 3 +- backend/database/sql/sqlcgen/albums.sql.go | 5 +- backend/explore/explore.go | 66 +++--- backend/explore/pendingreleasembid_test.go | 250 +++++++++++++++++++++ 4 files changed, 290 insertions(+), 34 deletions(-) create mode 100644 backend/explore/pendingreleasembid_test.go 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, + ) + } +} From 30c6b665f1a85aed461267edb43e53ccdfac54d2 Mon Sep 17 00:00:00 2001 From: Logan Date: Fri, 21 Aug 2026 17:23:23 -0400 Subject: [PATCH 2/2] fix(system): give the process a temp directory that exists Android has no /tmp and hands an app no TMPDIR. Go's os.TempDir() falls back to "/tmp" when the variable is unset, so every library in this process that wants scratch space was being handed a path that has never existed. SQLite is the one that noticed, and it said so precisely: W/yellowjacket: msg="champion index rebuild failed" explore.search-index.error="populate champion fts: disk I/O error (6410)" 6410 is not a generic I/O error. `6410 & 0xff` is 10, SQLITE_IOERR, and `6410 >> 8` is 25 -- SQLITE_IOERR_GETTEMPPATH. SQLite could not work out where to put a temporary file. Two measurements on the device say why: `ls -d /tmp` does not exist, and the app process's environment carries no TMPDIR. A shell's does (/data/local/tmp), which is why this is easy to miss from `adb shell`. The cost was a silent performance cliff on the slowest device this app runs on: `championReady` stayed false, so every Explore search took the generic path over the whole 1,079,667-row index instead of the champion subset, and the rebuild was re-attempted on every launch. **The class is fixed rather than the statement.** The trigger is the *size* of the work, not that query -- anything that spills fails the same way there, so large sorts, large joins and VACUUM were all waiting their turn. The repair belongs at the process's one answer to "where do temporary files go". `PRAGMA temp_store = MEMORY` was the alternative: cheaper, more local, and 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. UseTempDir sits beside UseHomeOverride and carries its two rules 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 it; nothing sets it on the platform this exists for. It needs no new Wails API and no Java change: StoragePath() is already what YJ_HOME is pointed at, and the directory goes under it. Two things beyond the rename of a variable. **Writability is probed, not assumed.** MkdirAll on an existing unwritable directory succeeds, so without the probe this could set TMPDIR to a directory nothing can use -- which is the same bug one directory over, and just as quiet. **It returns its error, and main logs it.** A temp directory that could not be created is the same silent failure one step earlier. A failure is not fatal: it leaves the platform's answer in place, which is what every release before this one ran with. That log line is readable on the platform only because of #160. Verified on the reference device, where the same launch that used to print the failure now prints: I/yellowjacket: msg="champion index rebuilt" explore.search-index.elapsed=6.496s Closes #190 --- backend/system/userdata.go | 69 +++++++++++++++++++ backend/system/userdata_test.go | 113 ++++++++++++++++++++++++++++++++ main.go | 13 ++++ 3 files changed, 195 insertions(+) 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)