From 7fdcb197676a744f7d182b09dc859c225af1d6b7 Mon Sep 17 00:00:00 2001 From: omartelo Date: Tue, 28 Jul 2026 16:30:14 -0300 Subject: [PATCH 1/2] fix: stop a session's directory read from killing the app on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tracking a session's working directory walks the child process's PEB and reads DosPath.Length out of that process's memory. x/sys documents the length as "should always be even"; a 32-bit child, or one exiting mid-walk, hands back whatever happens to sit there. An odd length passed the `== 0` guard, sized the buffer to zero, and panicked on `&buf[0]` — on the bare goroutine watchCwd runs on, so it took the whole process with it. The window survived, still rendering its terminal, with nothing behind it. The crash left no evidence: the runtime prints a panic to stderr, the windowsgui build has no console, and slog never sees it — lich.log simply stopped mid-session. Init now points the runtime's crash output at the same file, so the next one is readable. The length check is a pure helper outside the build-tagged file, following the chromium candidate lists, so the guard is tested on every OS. --- CHANGELOG.md | 9 ++++++++ internal/logging/logging.go | 18 +++++++++++---- internal/logging/logging_test.go | 35 +++++++++++++++++++++++++++++ internal/terminal/cwd.go | 14 ++++++++++++ internal/terminal/cwd_track_test.go | 24 ++++++++++++++++++++ internal/terminal/cwd_windows.go | 5 +++-- 6 files changed, 99 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f2ea65d1..4fc20d0f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **lich no longer vanishes mid-session on Windows.** Tracking a session's + working directory reads it out of the child process's own memory, and a length + that memory reported odd — which a 32-bit child, or one exiting mid-read, does + — crashed the whole app. The window stayed on screen still showing its + terminal, so the only sign was that nothing ever updated again and reloading + found nothing listening. The read now refuses a length it cannot use. A crash + that does happen is also written to `lich.log` now: it used to go to a console + the Windows build does not have, leaving the log to simply stop with no trace + of why. - **A session no longer offers to resume a conversation that is gone.** Reopening a session lich had run Claude Code in asked whether to continue that conversation — and answering yes could drop you into a terminal showing diff --git a/internal/logging/logging.go b/internal/logging/logging.go index bbbe78c27..e118ef2e4 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -12,6 +12,7 @@ import ( "log/slog" "os" "path/filepath" + "runtime/debug" "strings" ) @@ -22,10 +23,11 @@ const maxLogSize = 5 << 20 // Init points slog's default logger at stderr plus /lich.log // (lich-dev.log under LICH_DEV, mirroring the store's database split), with -// the level taken from LICH_LOG_LEVEL (debug|warn|error; default info). The -// returned closer owns the file. When the file cannot be opened, logging -// still works on stderr alone: the closer is nil and the error says why the -// persistent half is missing — the caller reports it and carries on. +// the level taken from LICH_LOG_LEVEL (debug|warn|error; default info), and +// routes the runtime's crash output to the same file. The returned closer owns +// the file. When the file cannot be opened, logging still works on stderr +// alone: the closer is nil and the error says why the persistent half is +// missing — the caller reports it and carries on. func Init(dir string) (io.Closer, error) { file, err := openLogFile(dir) out := io.Writer(os.Stderr) @@ -42,6 +44,14 @@ func Init(dir string) (io.Closer, error) { AddSource: true, Level: parseLevel(os.Getenv("LICH_LOG_LEVEL")), }))) + if file != nil { + // A panic bypasses slog entirely: the runtime prints it to stderr, which + // the windowsgui build does not have. Without this the log simply stops + // mid-session and the crash leaves no trace anywhere. + if err := debug.SetCrashOutput(file, debug.CrashOptions{}); err != nil { + slog.Warn("crash output unavailable — a panic will not reach the log", "err", err) + } + } return closer, err } diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go index 1eb7abc59..7b00ed8fb 100644 --- a/internal/logging/logging_test.go +++ b/internal/logging/logging_test.go @@ -3,6 +3,7 @@ package logging import ( "log/slog" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -146,3 +147,37 @@ func TestInitSurvivesUnwritableDir(t *testing.T) { } slog.Info("must not panic") } + +// crashProbeEnv hands the child half of TestInitRoutesPanicToLogFile the +// directory to log into; empty means this process is the parent. +const crashProbeEnv = "LICH_TEST_CRASH_DIR" + +// TestInitRoutesPanicToLogFile proves an unhandled panic reaches the log file +// rather than only the stderr a windowsgui process does not have. Runs the +// crash in a subprocess, because a panic ends the process it happens in. +func TestInitRoutesPanicToLogFile(t *testing.T) { + if dir := os.Getenv(crashProbeEnv); dir != "" { + if _, err := Init(dir); err != nil { + t.Fatalf("Init: %v", err) + } + panic("crash probe") + } + + dir := t.TempDir() + cmd := exec.Command(os.Args[0], "-test.run=^TestInitRoutesPanicToLogFile$") + cmd.Env = append(os.Environ(), crashProbeEnv+"="+dir, "LICH_DEV=") + if err := cmd.Run(); err == nil { + t.Fatal("child exited cleanly, want the panic to kill it") + } + + content, err := os.ReadFile(filepath.Join(dir, "lich.log")) + if err != nil { + t.Fatalf("read log: %v", err) + } + // The message and a stack: a trace without the panic value names no cause. + for _, want := range []string{"crash probe", "goroutine "} { + if !strings.Contains(string(content), want) { + t.Errorf("panic did not reach the log file, missing %q:\n%s", want, content) + } + } +} diff --git a/internal/terminal/cwd.go b/internal/terminal/cwd.go index 1c9307e3f..3372cdd52 100644 --- a/internal/terminal/cwd.go +++ b/internal/terminal/cwd.go @@ -24,6 +24,20 @@ type cwdEvent struct { // only on change, so a static directory costs nothing to re-render. const cwdPollInterval = 300 * time.Millisecond +// dosPathRunes is how many UTF-16 code units a DosPath of n bytes holds, and 0 +// when n is not a length a UTF-16 buffer can have. n comes out of the child's +// own memory (the Windows processCwd walks its PEB), where x/sys documents it +// as "should always be even" — a 32-bit child, or one exiting mid-walk, hands +// back whatever happens to sit there. An odd n would size the buffer to zero +// and take the read path straight into a panic, on a bare goroutine that ends +// the process. Kept out of the build-tagged file so the guard tests on any OS. +func dosPathRunes(n uint16) int { + if n == 0 || n%2 != 0 { + return 0 + } + return int(n / 2) +} + // watchCwd reports the session child's working directory to the frontend // whenever it changes, until done closes. Each platform brings its own read // (see the cwd_* files); on one without any, and when the PTY reports no diff --git a/internal/terminal/cwd_track_test.go b/internal/terminal/cwd_track_test.go index 279a590ae..f51152442 100644 --- a/internal/terminal/cwd_track_test.go +++ b/internal/terminal/cwd_track_test.go @@ -38,6 +38,30 @@ func waitEmit(t *testing.T, emits <-chan string) string { } } +// TestDosPathRunesRejectsUnusableLengths proves a DosPath byte length that no +// UTF-16 buffer can have is refused outright rather than sized down to an +// empty buffer — the Windows walk reads into &buf[0], so a zero-length buffer +// there panics and, on its bare goroutine, takes the whole app with it. +func TestDosPathRunesRejectsUnusableLengths(t *testing.T) { + cases := []struct { + name string + in uint16 + want int + }{ + {"empty", 0, 0}, + {"odd", 1, 0}, + {"odd garbage", 4097, 0}, + {"one code unit", 2, 1}, + {"C:\\", 6, 3}, + {"max even", 65534, 32767}, + } + for _, tc := range cases { + if got := dosPathRunes(tc.in); got != tc.want { + t.Errorf("%s: dosPathRunes(%d) = %d, want %d", tc.name, tc.in, got, tc.want) + } + } +} + // TestProcessCwdReadsSelf proves the platform read resolves a live process's // working directory — exercised against the test process itself. func TestProcessCwdReadsSelf(t *testing.T) { diff --git a/internal/terminal/cwd_windows.go b/internal/terminal/cwd_windows.go index 9c939a76e..fb9bbaa7f 100644 --- a/internal/terminal/cwd_windows.go +++ b/internal/terminal/cwd_windows.go @@ -52,10 +52,11 @@ func processCwd(pid int) string { } dos := params.CurrentDirectory.DosPath - if dos.Length == 0 || dos.Buffer == nil { + runes := dosPathRunes(dos.Length) + if runes == 0 || dos.Buffer == nil { return "" } - buf := make([]uint16, dos.Length/2) + buf := make([]uint16, runes) if err := readMemory(h, uintptr(unsafe.Pointer(dos.Buffer)), unsafe.Pointer(&buf[0]), uintptr(dos.Length)); err != nil { return "" From 8b61d3781d69c8ebe3a26560ee9ac72206f1c474 Mon Sep 17 00:00:00 2001 From: omartelo Date: Tue, 28 Jul 2026 16:58:00 -0300 Subject: [PATCH 2/2] test: release the crash-output handle before a temp log dir is removed SetCrashOutput keeps a duplicate of the log file's handle for as long as the registration stands. Windows refuses to remove a file another handle still holds, so TempDir's cleanup failed in the two tests that let Init open a file. The clear is registered after TempDir so the LIFO order runs it first. --- internal/logging/logging_test.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go index 7b00ed8fb..4c97e575d 100644 --- a/internal/logging/logging_test.go +++ b/internal/logging/logging_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime/debug" "strings" "testing" ) @@ -17,6 +18,19 @@ func restoreDefault(t *testing.T) { t.Cleanup(func() { slog.SetDefault(prev) }) } +// logDir is a temp dir for a test that lets Init succeed. Init points the +// runtime's crash output at the log file, and that keeps a duplicate of the +// file's handle open for as long as the registration stands — on Windows a +// file another handle still holds cannot be removed, so TempDir's cleanup +// fails unless the registration is dropped first. Registered after TempDir on +// purpose: cleanups run last-in-first-out, so this one runs before the removal. +func logDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + t.Cleanup(func() { _ = debug.SetCrashOutput(nil, debug.CrashOptions{}) }) + return dir +} + // TestParseLevel proves the LICH_LOG_LEVEL values map onto slog levels and // that anything unrecognized falls back to Info. func TestParseLevel(t *testing.T) { @@ -55,7 +69,7 @@ func TestInitWritesRecordWithSource(t *testing.T) { restoreDefault(t) t.Setenv("LICH_DEV", "") t.Setenv("LICH_LOG_LEVEL", "") - dir := t.TempDir() + dir := logDir(t) closer, err := Init(dir) if err != nil { @@ -81,7 +95,7 @@ func TestInitWritesRecordWithSource(t *testing.T) { func TestInitRotatesOversizedLog(t *testing.T) { restoreDefault(t) t.Setenv("LICH_DEV", "") - dir := t.TempDir() + dir := logDir(t) path := filepath.Join(dir, "lich.log") if err := os.WriteFile(path, []byte("old generation\n"), 0o600); err != nil { t.Fatal(err)