From 30c6b665f1a85aed461267edb43e53ccdfac54d2 Mon Sep 17 00:00:00 2001 From: Logan Date: Fri, 21 Aug 2026 17:23:23 -0400 Subject: [PATCH] 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)