From 8c7ca1ba313e8087213934a55fe6d500985024a4 Mon Sep 17 00:00:00 2001 From: SivanCola <32437197+SivanCola@users.noreply.github.com> Date: Sun, 9 Aug 2026 20:50:29 +0800 Subject: [PATCH 1/5] fix(desktop): make session sidecars durable Problem: Desktop title and display sidecars published temporary files without fsync, and concurrent title read-modify-write operations could overwrite each other. Root cause: The sidecars duplicated a manual temp-write-close-replace sequence instead of the shared durable atomic writer. Unlike display sidecars, the legacy title map also had no cross-process serialization. Fix: Route title, display, and planner-display sidecars through fileutil.AtomicWriteFile, serialize title mutations with a bounded file lock, and cover the durable helper, lock boundary, and concurrent updates. Verification: - go test ./internal/fileutil -count=1 - cd desktop && go test ./... -count=1 - cd desktop && go test -race . -run 'TestSessionSidecarSavesUseDurableAtomicWrite|TestSetSessionTitle|TestRecordSessionDisplaySerializesConcurrentTabs|TestRecordSessionPlannerDisplayConcurrentPreservesEverySession' -count=1 - cd desktop && go vet ./... - git diff --check Co-authored-by: ipanta <8358507+myipanta@users.noreply.github.com> --- desktop/sessions.go | 118 ++++++++++++++++++-------------------- desktop/sessions_test.go | 120 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 64 deletions(-) diff --git a/desktop/sessions.go b/desktop/sessions.go index c089d32b8e..f1181cd44b 100644 --- a/desktop/sessions.go +++ b/desktop/sessions.go @@ -33,6 +33,8 @@ const sessionPlannerDisplayFile = ".planner-display.json" const sessionTrashDir = ".trash" const sessionTrashMetaFile = ".trash-meta.json" +var sessionTitlesLockTimeout = 750 * time.Millisecond + func sessionTitlesPath(dir string) string { return filepath.Join(dir, sessionTitlesFile) } func sessionDisplayPath(dir string) string { return filepath.Join(dir, sessionDisplayFile) } func sessionTrashPath(dir string) string { return filepath.Join(dir, sessionTrashDir) } @@ -73,7 +75,33 @@ func loadSessionTitlesForUpdate(dir string) (map[string]string, error) { return loadStringMapForUpdate(sessionTitlesPath(dir)) } -// saveSessionTitles writes the map atomically (temp file + rename). +func updateSessionTitles(dir string, mutate func(map[string]string) bool) error { + if strings.TrimSpace(dir) == "" { + return errors.New("title directory is empty") + } + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), sessionTitlesLockTimeout) + defer cancel() + release, err := filelock.Acquire(ctx, sessionTitlesPath(dir)+".lock") + if err != nil { + return fmt.Errorf("lock title sidecar: %w", err) + } + defer release() + + m, err := loadSessionTitlesForUpdate(dir) + if err != nil { + return err + } + if !mutate(m) { + return nil + } + return saveSessionTitles(dir, m) +} + +// saveSessionTitles writes the map durably and atomically. Keep this on the +// shared helper so the temporary file is fsynced before it is published. func saveSessionTitles(dir string, m map[string]string) error { b, err := json.MarshalIndent(m, "", " ") if err != nil { @@ -82,21 +110,7 @@ func saveSessionTitles(dir string, m map[string]string) error { if err := os.MkdirAll(dir, 0o755); err != nil { return err } - tmp, err := os.CreateTemp(dir, ".titles.*.tmp") - if err != nil { - return err - } - tmpPath := tmp.Name() - if _, err := tmp.Write(b); err != nil { - tmp.Close() - os.Remove(tmpPath) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpPath) - return err - } - return fileutil.ReplaceFile(tmpPath, sessionTitlesPath(dir)) + return fileutil.AtomicWriteFile(sessionTitlesPath(dir), b, 0o600) } // setSessionTitle sets (or, with an empty title, clears) a session's custom name. @@ -105,17 +119,22 @@ func setSessionTitle(dir, sessionPath, title string) error { if err != nil { return err } - m, err := loadSessionTitlesForUpdate(dir) - if err != nil { - return err - } key := filepath.Base(sessionPath) - if strings.TrimSpace(title) == "" { - delete(m, key) - } else { - m[key] = strings.TrimSpace(title) - } - return saveSessionTitles(dir, m) + return updateSessionTitles(dir, func(m map[string]string) bool { + title = strings.TrimSpace(title) + if title == "" { + if _, ok := m[key]; !ok { + return false + } + delete(m, key) + return true + } + if m[key] == title { + return false + } + m[key] = title + return true + }) } // deleteSessionFile moves a session's .jsonl and file sidecars into the local @@ -598,15 +617,14 @@ func purgeTrashedSessionFile(dir, path string) error { if err := os.RemoveAll(itemDir); err != nil { return err } - m, err := loadSessionTitlesForUpdate(dir) - if err != nil { - return err - } - if _, ok := m[key]; ok { - delete(m, key) - if err := saveSessionTitles(dir, m); err != nil { - return err + if err := updateSessionTitles(dir, func(m map[string]string) bool { + if _, ok := m[key]; !ok { + return false } + delete(m, key) + return true + }); err != nil { + return err } if err := removeSessionDisplayKey(dir, key); err != nil { return err @@ -1018,21 +1036,7 @@ func saveSessionPlannerDisplays(dir string, m sessionPlannerDisplayMap) error { if err := os.MkdirAll(dir, 0o755); err != nil { return err } - tmp, err := os.CreateTemp(dir, ".planner-display.*.tmp") - if err != nil { - return err - } - tmpPath := tmp.Name() - if _, err := tmp.Write(b); err != nil { - tmp.Close() - os.Remove(tmpPath) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpPath) - return err - } - return fileutil.ReplaceFile(tmpPath, sessionPlannerDisplayPath(dir)) + return fileutil.AtomicWriteFile(sessionPlannerDisplayPath(dir), b, 0o600) } func saveOrRemoveSessionPlannerDisplays(dir string, m sessionPlannerDisplayMap) error { @@ -1155,21 +1159,7 @@ func saveSessionDisplays(dir string, m sessionDisplayMap) error { if err := os.MkdirAll(dir, 0o755); err != nil { return err } - tmp, err := os.CreateTemp(dir, ".display.*.tmp") - if err != nil { - return err - } - tmpPath := tmp.Name() - if _, err := tmp.Write(b); err != nil { - tmp.Close() - os.Remove(tmpPath) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpPath) - return err - } - return fileutil.ReplaceFile(tmpPath, sessionDisplayPath(dir)) + return fileutil.AtomicWriteFile(sessionDisplayPath(dir), b, 0o600) } func saveOrRemoveSessionDisplays(dir string, m sessionDisplayMap) error { diff --git a/desktop/sessions_test.go b/desktop/sessions_test.go index d61167b312..0179717658 100644 --- a/desktop/sessions_test.go +++ b/desktop/sessions_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "encoding/json" "errors" "fmt" @@ -13,6 +14,8 @@ import ( "time" "reasonix/internal/agent" + "reasonix/internal/filelock" + "reasonix/internal/fileutil" "reasonix/internal/jobs" "reasonix/internal/store" ) @@ -102,6 +105,69 @@ func TestSaveSessionTitlesRoundTrip(t *testing.T) { } } +func TestSessionSidecarSavesUseDurableAtomicWrite(t *testing.T) { + tests := []struct { + name string + path func(string) string + save func(string) error + }{ + { + name: "titles", + path: sessionTitlesPath, + save: func(dir string) error { + return saveSessionTitles(dir, map[string]string{"session.jsonl": "Durable title"}) + }, + }, + { + name: "displays", + path: sessionDisplayPath, + save: func(dir string) error { + return saveSessionDisplays(dir, sessionDisplayMap{ + "session.jsonl": {messageDisplayKey("expanded prompt"): "visible prompt"}, + }) + }, + }, + { + name: "planner displays", + path: sessionPlannerDisplayPath, + save: func(dir string) error { + return saveSessionPlannerDisplays(dir, sessionPlannerDisplayMap{ + "session.jsonl": {{UserHash: "prompt-digest", Messages: []HistoryMessage{}}}, + }) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + path := tt.path(dir) + previousCrashPoint := fileutil.CrashPoint + t.Cleanup(func() { fileutil.CrashPoint = previousCrashPoint }) + usedDurableWrite := false + fileutil.CrashPoint = func(op, gotPath string) { + if op == "atomic-write" && gotPath == path { + usedDurableWrite = true + } + } + + if err := tt.save(dir); err != nil { + t.Fatalf("save %s sidecar: %v", tt.name, err) + } + if !usedDurableWrite { + t.Fatalf("%s sidecar bypassed fileutil.AtomicWriteFile", tt.name) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s sidecar: %v", tt.name, err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("%s sidecar mode = %o, want 600", tt.name, got) + } + }) + } +} + // setSessionTitle func TestSetSessionTitle(t *testing.T) { @@ -160,6 +226,60 @@ func TestSetSessionTitlePreservesExistingTitlesWhenTimedReadSlotsFull(t *testing } } +func TestSetSessionTitleHonorsSidecarLock(t *testing.T) { + dir := t.TempDir() + release, err := filelock.Acquire(context.Background(), sessionTitlesPath(dir)+".lock") + if err != nil { + t.Fatalf("acquire title sidecar lock: %v", err) + } + previousTimeout := sessionTitlesLockTimeout + sessionTitlesLockTimeout = 40 * time.Millisecond + t.Cleanup(func() { sessionTitlesLockTimeout = previousTimeout }) + + err = setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Locked") + if err == nil { + release() + t.Fatal("setSessionTitle succeeded while the title sidecar lock was held") + } + release() + if err := setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Saved"); err != nil { + t.Fatalf("setSessionTitle after release: %v", err) + } +} + +func TestSetSessionTitleSerializesConcurrentUpdates(t *testing.T) { + dir := t.TempDir() + const sessions = 32 + errs := make(chan error, sessions) + var wg sync.WaitGroup + for i := range sessions { + wg.Add(1) + go func(i int) { + defer wg.Done() + path := filepath.Join(dir, fmt.Sprintf("session-%02d.jsonl", i)) + errs <- setSessionTitle(dir, path, fmt.Sprintf("Title %02d", i)) + }(i) + } + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Fatalf("setSessionTitle: %v", err) + } + } + + titles := loadSessionTitles(dir) + if len(titles) != sessions { + t.Fatalf("title count = %d, want %d: %#v", len(titles), sessions, titles) + } + for i := range sessions { + key := fmt.Sprintf("session-%02d.jsonl", i) + if got := titles[key]; got != fmt.Sprintf("Title %02d", i) { + t.Fatalf("%s title = %q, want retained concurrent value", key, got) + } + } +} + // deleteSessionFile func TestDeleteSessionFile(t *testing.T) { From 5f74632f0324c647e4325bb36a3c646344adcb39 Mon Sep 17 00:00:00 2001 From: SivanCola <32437197+SivanCola@users.noreply.github.com> Date: Sun, 9 Aug 2026 20:54:29 +0800 Subject: [PATCH 2/5] test(desktop): isolate sidecar durability coverage Problem: The new session-sidecar regressions pushed an already large legacy test file further beyond the repository test-file-size budget. Root cause: The focused coverage was added beside older session tests instead of in its own owner-level test file. Fix: Move the new durability and concurrent-title tests unchanged into session_sidecar_durability_test.go. Verification: - go run ./tools/repolint - cd desktop && go test -race . -run 'TestSessionSidecarSavesUseDurableAtomicWrite|TestSetSessionTitle|TestRecordSessionDisplaySerializesConcurrentTabs|TestRecordSessionPlannerDisplayConcurrentPreservesEverySession' -count=1 - git diff --check --- desktop/session_sidecar_durability_test.go | 131 +++++++++++++++++++++ desktop/sessions_test.go | 120 ------------------- 2 files changed, 131 insertions(+), 120 deletions(-) create mode 100644 desktop/session_sidecar_durability_test.go diff --git a/desktop/session_sidecar_durability_test.go b/desktop/session_sidecar_durability_test.go new file mode 100644 index 0000000000..31f44012ef --- /dev/null +++ b/desktop/session_sidecar_durability_test.go @@ -0,0 +1,131 @@ +package main + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "reasonix/internal/filelock" + "reasonix/internal/fileutil" +) + +func TestSessionSidecarSavesUseDurableAtomicWrite(t *testing.T) { + tests := []struct { + name string + path func(string) string + save func(string) error + }{ + { + name: "titles", + path: sessionTitlesPath, + save: func(dir string) error { + return saveSessionTitles(dir, map[string]string{"session.jsonl": "Durable title"}) + }, + }, + { + name: "displays", + path: sessionDisplayPath, + save: func(dir string) error { + return saveSessionDisplays(dir, sessionDisplayMap{ + "session.jsonl": {messageDisplayKey("expanded prompt"): "visible prompt"}, + }) + }, + }, + { + name: "planner displays", + path: sessionPlannerDisplayPath, + save: func(dir string) error { + return saveSessionPlannerDisplays(dir, sessionPlannerDisplayMap{ + "session.jsonl": {{UserHash: "prompt-digest", Messages: []HistoryMessage{}}}, + }) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + path := tt.path(dir) + previousCrashPoint := fileutil.CrashPoint + t.Cleanup(func() { fileutil.CrashPoint = previousCrashPoint }) + usedDurableWrite := false + fileutil.CrashPoint = func(op, gotPath string) { + if op == "atomic-write" && gotPath == path { + usedDurableWrite = true + } + } + + if err := tt.save(dir); err != nil { + t.Fatalf("save %s sidecar: %v", tt.name, err) + } + if !usedDurableWrite { + t.Fatalf("%s sidecar bypassed fileutil.AtomicWriteFile", tt.name) + } + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s sidecar: %v", tt.name, err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("%s sidecar mode = %o, want 600", tt.name, got) + } + }) + } +} + +func TestSetSessionTitleHonorsSidecarLock(t *testing.T) { + dir := t.TempDir() + release, err := filelock.Acquire(context.Background(), sessionTitlesPath(dir)+".lock") + if err != nil { + t.Fatalf("acquire title sidecar lock: %v", err) + } + previousTimeout := sessionTitlesLockTimeout + sessionTitlesLockTimeout = 40 * time.Millisecond + t.Cleanup(func() { sessionTitlesLockTimeout = previousTimeout }) + + err = setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Locked") + if err == nil { + release() + t.Fatal("setSessionTitle succeeded while the title sidecar lock was held") + } + release() + if err := setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Saved"); err != nil { + t.Fatalf("setSessionTitle after release: %v", err) + } +} + +func TestSetSessionTitleSerializesConcurrentUpdates(t *testing.T) { + dir := t.TempDir() + const sessions = 32 + errs := make(chan error, sessions) + var wg sync.WaitGroup + for i := range sessions { + wg.Add(1) + go func(i int) { + defer wg.Done() + path := filepath.Join(dir, fmt.Sprintf("session-%02d.jsonl", i)) + errs <- setSessionTitle(dir, path, fmt.Sprintf("Title %02d", i)) + }(i) + } + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Fatalf("setSessionTitle: %v", err) + } + } + + titles := loadSessionTitles(dir) + if len(titles) != sessions { + t.Fatalf("title count = %d, want %d: %#v", len(titles), sessions, titles) + } + for i := range sessions { + key := fmt.Sprintf("session-%02d.jsonl", i) + if got := titles[key]; got != fmt.Sprintf("Title %02d", i) { + t.Fatalf("%s title = %q, want retained concurrent value", key, got) + } + } +} diff --git a/desktop/sessions_test.go b/desktop/sessions_test.go index 0179717658..d61167b312 100644 --- a/desktop/sessions_test.go +++ b/desktop/sessions_test.go @@ -1,7 +1,6 @@ package main import ( - "context" "encoding/json" "errors" "fmt" @@ -14,8 +13,6 @@ import ( "time" "reasonix/internal/agent" - "reasonix/internal/filelock" - "reasonix/internal/fileutil" "reasonix/internal/jobs" "reasonix/internal/store" ) @@ -105,69 +102,6 @@ func TestSaveSessionTitlesRoundTrip(t *testing.T) { } } -func TestSessionSidecarSavesUseDurableAtomicWrite(t *testing.T) { - tests := []struct { - name string - path func(string) string - save func(string) error - }{ - { - name: "titles", - path: sessionTitlesPath, - save: func(dir string) error { - return saveSessionTitles(dir, map[string]string{"session.jsonl": "Durable title"}) - }, - }, - { - name: "displays", - path: sessionDisplayPath, - save: func(dir string) error { - return saveSessionDisplays(dir, sessionDisplayMap{ - "session.jsonl": {messageDisplayKey("expanded prompt"): "visible prompt"}, - }) - }, - }, - { - name: "planner displays", - path: sessionPlannerDisplayPath, - save: func(dir string) error { - return saveSessionPlannerDisplays(dir, sessionPlannerDisplayMap{ - "session.jsonl": {{UserHash: "prompt-digest", Messages: []HistoryMessage{}}}, - }) - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - dir := t.TempDir() - path := tt.path(dir) - previousCrashPoint := fileutil.CrashPoint - t.Cleanup(func() { fileutil.CrashPoint = previousCrashPoint }) - usedDurableWrite := false - fileutil.CrashPoint = func(op, gotPath string) { - if op == "atomic-write" && gotPath == path { - usedDurableWrite = true - } - } - - if err := tt.save(dir); err != nil { - t.Fatalf("save %s sidecar: %v", tt.name, err) - } - if !usedDurableWrite { - t.Fatalf("%s sidecar bypassed fileutil.AtomicWriteFile", tt.name) - } - info, err := os.Stat(path) - if err != nil { - t.Fatalf("stat %s sidecar: %v", tt.name, err) - } - if got := info.Mode().Perm(); got != 0o600 { - t.Fatalf("%s sidecar mode = %o, want 600", tt.name, got) - } - }) - } -} - // setSessionTitle func TestSetSessionTitle(t *testing.T) { @@ -226,60 +160,6 @@ func TestSetSessionTitlePreservesExistingTitlesWhenTimedReadSlotsFull(t *testing } } -func TestSetSessionTitleHonorsSidecarLock(t *testing.T) { - dir := t.TempDir() - release, err := filelock.Acquire(context.Background(), sessionTitlesPath(dir)+".lock") - if err != nil { - t.Fatalf("acquire title sidecar lock: %v", err) - } - previousTimeout := sessionTitlesLockTimeout - sessionTitlesLockTimeout = 40 * time.Millisecond - t.Cleanup(func() { sessionTitlesLockTimeout = previousTimeout }) - - err = setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Locked") - if err == nil { - release() - t.Fatal("setSessionTitle succeeded while the title sidecar lock was held") - } - release() - if err := setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Saved"); err != nil { - t.Fatalf("setSessionTitle after release: %v", err) - } -} - -func TestSetSessionTitleSerializesConcurrentUpdates(t *testing.T) { - dir := t.TempDir() - const sessions = 32 - errs := make(chan error, sessions) - var wg sync.WaitGroup - for i := range sessions { - wg.Add(1) - go func(i int) { - defer wg.Done() - path := filepath.Join(dir, fmt.Sprintf("session-%02d.jsonl", i)) - errs <- setSessionTitle(dir, path, fmt.Sprintf("Title %02d", i)) - }(i) - } - wg.Wait() - close(errs) - for err := range errs { - if err != nil { - t.Fatalf("setSessionTitle: %v", err) - } - } - - titles := loadSessionTitles(dir) - if len(titles) != sessions { - t.Fatalf("title count = %d, want %d: %#v", len(titles), sessions, titles) - } - for i := range sessions { - key := fmt.Sprintf("session-%02d.jsonl", i) - if got := titles[key]; got != fmt.Sprintf("Title %02d", i) { - t.Fatalf("%s title = %q, want retained concurrent value", key, got) - } - } -} - // deleteSessionFile func TestDeleteSessionFile(t *testing.T) { From 781dd38fb820e55eba1d1bcfd1b32765dbccd864 Mon Sep 17 00:00:00 2001 From: SivanCola <32437197+SivanCola@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:04:59 +0800 Subject: [PATCH 3/5] test(desktop): respect Windows file mode semantics Problem: The sidecar durability regression asserted Unix permission bits on Windows, where Go reports regular files as 0666 and access control is governed by Windows ACLs. Root cause: The test coupled the cross-platform durable-write contract to a Unix-only os.FileMode representation. Fix: Keep the AtomicWriteFile assertion on every platform and restrict the 0600 mode-bit assertion to non-Windows systems. Verification: - cd desktop && go test -race . -run 'TestSessionSidecarSavesUseDurableAtomicWrite|TestSetSessionTitle' -count=1 - GOOS=windows CGO_ENABLED=0 go test -c -o /tmp/reasonix-desktop-windows.test.exe . - go run ./tools/repolint - git diff --check --- desktop/session_sidecar_durability_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/desktop/session_sidecar_durability_test.go b/desktop/session_sidecar_durability_test.go index 31f44012ef..70e15ee910 100644 --- a/desktop/session_sidecar_durability_test.go +++ b/desktop/session_sidecar_durability_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sync" "testing" "time" @@ -65,12 +66,14 @@ func TestSessionSidecarSavesUseDurableAtomicWrite(t *testing.T) { if !usedDurableWrite { t.Fatalf("%s sidecar bypassed fileutil.AtomicWriteFile", tt.name) } - info, err := os.Stat(path) - if err != nil { - t.Fatalf("stat %s sidecar: %v", tt.name, err) - } - if got := info.Mode().Perm(); got != 0o600 { - t.Fatalf("%s sidecar mode = %o, want 600", tt.name, got) + if runtime.GOOS != "windows" { + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s sidecar: %v", tt.name, err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("%s sidecar mode = %o, want 600", tt.name, got) + } } }) } From 9754fc395f6dee44d358382f8ddef42ba8aef985 Mon Sep 17 00:00:00 2001 From: SivanCola <32437197+SivanCola@users.noreply.github.com> Date: Sun, 9 Aug 2026 21:28:23 +0800 Subject: [PATCH 4/5] fix(desktop): budget durable sidecar lock bursts Problem: Windows CI showed that 32 concurrent display-sidecar writers could exhaust the 750ms lock deadline after sidecar publication gained an fsync. Root cause: filelock serializes in-process waiters, but every waiter starts its deadline before entering that queue. Durable Windows writes can therefore consume the full budget of tail writers even without external contention. Fix: Use a shared five-second bounded lock budget for title, display, and planner-display sidecars. This keeps contention bounded while accommodating durable burst writes on slower disks. Verification: - cd desktop && go test -race . -run 'TestSetSessionTitleSerializesConcurrentUpdates|TestRecordSessionDisplaySerializesConcurrentTabs|TestRecordSessionPlannerDisplayConcurrentPreservesEverySession' -count=10 - cd desktop && go vet ./... - cd desktop && GOOS=windows CGO_ENABLED=0 go test -c -o /tmp/reasonix-desktop-windows.test.exe . - go run ./tools/repolint - git diff --check --- desktop/sessions.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/desktop/sessions.go b/desktop/sessions.go index f1181cd44b..9116b8872f 100644 --- a/desktop/sessions.go +++ b/desktop/sessions.go @@ -33,7 +33,11 @@ const sessionPlannerDisplayFile = ".planner-display.json" const sessionTrashDir = ".trash" const sessionTrashMetaFile = ".trash-meta.json" -var sessionTitlesLockTimeout = 750 * time.Millisecond +// Durable sidecar publication includes an fsync while holding the update lock. +// Keep enough budget for a burst of in-process writers on slower Windows disks. +const sessionSidecarLockTimeout = 5 * time.Second + +var sessionTitlesLockTimeout = sessionSidecarLockTimeout func sessionTitlesPath(dir string) string { return filepath.Join(dir, sessionTitlesFile) } func sessionDisplayPath(dir string) string { return filepath.Join(dir, sessionDisplayFile) } @@ -969,7 +973,7 @@ type plannerDisplayTurn struct { } var ( - sessionPlannerDisplayLockTimeout = 750 * time.Millisecond + sessionPlannerDisplayLockTimeout = sessionSidecarLockTimeout errCorruptSessionPlannerDisplay = errors.New("corrupt planner display sidecar") ) @@ -1184,7 +1188,7 @@ func updateSessionDisplays(dir string, mutate func(sessionDisplayMap) bool) erro if err := os.MkdirAll(dir, 0o755); err != nil { return err } - ctx, cancel := context.WithTimeout(context.Background(), 750*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), sessionSidecarLockTimeout) defer cancel() release, err := filelock.Acquire(ctx, sessionDisplayPath(dir)+".lock") if err != nil { From f11321b60d19e764856d75119aad102b52d92395 Mon Sep 17 00:00:00 2001 From: SivanCola <32437197+SivanCola@users.noreply.github.com> Date: Sun, 9 Aug 2026 22:07:12 +0800 Subject: [PATCH 5/5] fix(desktop): bound cross-process sidecar lock waits Problem: the 5-second durability budget also applied to external OS lock retries, so another Reasonix process could stall turn completion before display persistence entered its retry queue. Root cause: filelock.Acquire used one context for both the in-process writer queue and cross-process flock contention. Fix: add a split-budget acquisition API, keep the 5-second local fsync queue, restore the 750ms external lock budget, and cover both the generic lock and real display-sidecar subprocess path. Verification: focused and race tests, both Go module suites, go vet, repolint, Windows cross-compilation, and latest-base merge-tree all pass. --- desktop/session_sidecar_durability_test.go | 66 +++++++++++++++++++++- desktop/sessions.go | 34 ++++++----- desktop/sessions_test.go | 2 +- internal/filelock/filelock.go | 25 +++++++- internal/filelock/filelock_test.go | 28 +++++++++ 5 files changed, 135 insertions(+), 20 deletions(-) diff --git a/desktop/session_sidecar_durability_test.go b/desktop/session_sidecar_durability_test.go index 70e15ee910..a5da85ef61 100644 --- a/desktop/session_sidecar_durability_test.go +++ b/desktop/session_sidecar_durability_test.go @@ -2,10 +2,13 @@ package main import ( "context" + "errors" "fmt" "os" + "os/exec" "path/filepath" "runtime" + "strings" "sync" "testing" "time" @@ -85,9 +88,9 @@ func TestSetSessionTitleHonorsSidecarLock(t *testing.T) { if err != nil { t.Fatalf("acquire title sidecar lock: %v", err) } - previousTimeout := sessionTitlesLockTimeout - sessionTitlesLockTimeout = 40 * time.Millisecond - t.Cleanup(func() { sessionTitlesLockTimeout = previousTimeout }) + previousTimeout := sessionTitlesQueueTimeout + sessionTitlesQueueTimeout = 40 * time.Millisecond + t.Cleanup(func() { sessionTitlesQueueTimeout = previousTimeout }) err = setSessionTitle(dir, filepath.Join(dir, "locked.jsonl"), "Locked") if err == nil { @@ -100,6 +103,63 @@ func TestSetSessionTitleHonorsSidecarLock(t *testing.T) { } } +func TestRecordSessionDisplayExternalLockUsesShortBudget(t *testing.T) { + if os.Getenv("REASONIX_DISPLAY_LOCK_HELPER") != "" { + dir := os.Getenv("REASONIX_DISPLAY_LOCK_DIR") + release, err := filelock.Acquire(context.Background(), sessionDisplayPath(dir)+".lock") + if err != nil { + t.Fatalf("acquire external display lock: %v", err) + } + defer release() + if err := os.WriteFile(filepath.Join(dir, "display-lock.ready"), []byte("ready"), 0o600); err != nil { + t.Fatal(err) + } + if !waitForPlannerDisplayTestFile(filepath.Join(dir, "display-lock.release"), 10*time.Second) { + t.Fatal("timed out waiting to release external display lock") + } + return + } + + dir := t.TempDir() + var output strings.Builder + cmd := exec.Command(os.Args[0], "-test.run=^TestRecordSessionDisplayExternalLockUsesShortBudget$") + cmd.Env = append(os.Environ(), + "REASONIX_DISPLAY_LOCK_HELPER=1", + "REASONIX_DISPLAY_LOCK_DIR="+dir, + ) + cmd.Stdout = &output + cmd.Stderr = &output + if err := cmd.Start(); err != nil { + t.Fatalf("start external display-lock helper: %v", err) + } + releasePath := filepath.Join(dir, "display-lock.release") + releaseHelper := func() { + _ = os.WriteFile(releasePath, []byte("release"), 0o600) + } + if !waitForPlannerDisplayTestFile(filepath.Join(dir, "display-lock.ready"), 5*time.Second) { + releaseHelper() + _ = cmd.Wait() + t.Fatalf("external display-lock helper did not start: %s", output.String()) + } + + previousTimeout := sessionDisplayExternalLockTimeout + sessionDisplayExternalLockTimeout = 100 * time.Millisecond + t.Cleanup(func() { sessionDisplayExternalLockTimeout = previousTimeout }) + started := time.Now() + err := recordSessionDisplay(dir, filepath.Join(dir, "session.jsonl"), "expanded prompt", "visible prompt") + elapsed := time.Since(started) + releaseHelper() + if waitErr := cmd.Wait(); waitErr != nil { + t.Fatalf("external display-lock helper failed: %v\n%s", waitErr, output.String()) + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("record display error = %v, want external lock deadline exceeded", err) + } + if elapsed >= 2*time.Second { + t.Fatalf("record display waited %v, want the short external lock budget", elapsed) + } +} + func TestSetSessionTitleSerializesConcurrentUpdates(t *testing.T) { dir := t.TempDir() const sessions = 32 diff --git a/desktop/sessions.go b/desktop/sessions.go index 9116b8872f..8ff327e337 100644 --- a/desktop/sessions.go +++ b/desktop/sessions.go @@ -33,11 +33,20 @@ const sessionPlannerDisplayFile = ".planner-display.json" const sessionTrashDir = ".trash" const sessionTrashMetaFile = ".trash-meta.json" -// Durable sidecar publication includes an fsync while holding the update lock. -// Keep enough budget for a burst of in-process writers on slower Windows disks. -const sessionSidecarLockTimeout = 5 * time.Second +const ( + // Durable sidecar publication includes an fsync while holding the update + // lock. Keep enough queue budget for a burst of in-process writers on + // slower Windows disks, but fail external contention quickly so turn + // completion and retry-queue handoff do not stall behind another process. + sessionSidecarQueueTimeout = 5 * time.Second + sessionSidecarExternalLockTimeout = 750 * time.Millisecond +) -var sessionTitlesLockTimeout = sessionSidecarLockTimeout +var ( + sessionTitlesQueueTimeout = sessionSidecarQueueTimeout + sessionPlannerDisplayExternalLockTimeout = sessionSidecarExternalLockTimeout + sessionDisplayExternalLockTimeout = sessionSidecarExternalLockTimeout +) func sessionTitlesPath(dir string) string { return filepath.Join(dir, sessionTitlesFile) } func sessionDisplayPath(dir string) string { return filepath.Join(dir, sessionDisplayFile) } @@ -86,9 +95,9 @@ func updateSessionTitles(dir string, mutate func(map[string]string) bool) error if err := os.MkdirAll(dir, 0o755); err != nil { return err } - ctx, cancel := context.WithTimeout(context.Background(), sessionTitlesLockTimeout) + ctx, cancel := context.WithTimeout(context.Background(), sessionTitlesQueueTimeout) defer cancel() - release, err := filelock.Acquire(ctx, sessionTitlesPath(dir)+".lock") + release, err := filelock.AcquireWithExternalTimeout(ctx, sessionTitlesPath(dir)+".lock", sessionSidecarExternalLockTimeout) if err != nil { return fmt.Errorf("lock title sidecar: %w", err) } @@ -972,10 +981,7 @@ type plannerDisplayTurn struct { Messages []HistoryMessage `json:"messages"` } -var ( - sessionPlannerDisplayLockTimeout = sessionSidecarLockTimeout - errCorruptSessionPlannerDisplay = errors.New("corrupt planner display sidecar") -) +var errCorruptSessionPlannerDisplay = errors.New("corrupt planner display sidecar") // sessionPlannerDisplayUpdateAfterLoad is a subprocess-test seam. Production // leaves it nil; tests use it to force two independent processes into the old @@ -1061,9 +1067,9 @@ func updateSessionPlannerDisplays(dir string, recoverCorrupt bool, mutate func(s if err := os.MkdirAll(dir, 0o755); err != nil { return err } - ctx, cancel := context.WithTimeout(context.Background(), sessionPlannerDisplayLockTimeout) + ctx, cancel := context.WithTimeout(context.Background(), sessionSidecarQueueTimeout) defer cancel() - release, err := filelock.Acquire(ctx, sessionPlannerDisplayPath(dir)+".lock") + release, err := filelock.AcquireWithExternalTimeout(ctx, sessionPlannerDisplayPath(dir)+".lock", sessionPlannerDisplayExternalLockTimeout) if err != nil { return fmt.Errorf("lock planner display sidecar: %w", err) } @@ -1188,9 +1194,9 @@ func updateSessionDisplays(dir string, mutate func(sessionDisplayMap) bool) erro if err := os.MkdirAll(dir, 0o755); err != nil { return err } - ctx, cancel := context.WithTimeout(context.Background(), sessionSidecarLockTimeout) + ctx, cancel := context.WithTimeout(context.Background(), sessionSidecarQueueTimeout) defer cancel() - release, err := filelock.Acquire(ctx, sessionDisplayPath(dir)+".lock") + release, err := filelock.AcquireWithExternalTimeout(ctx, sessionDisplayPath(dir)+".lock", sessionDisplayExternalLockTimeout) if err != nil { return fmt.Errorf("lock display sidecar: %w", err) } diff --git a/desktop/sessions_test.go b/desktop/sessions_test.go index d61167b312..ad048805f5 100644 --- a/desktop/sessions_test.go +++ b/desktop/sessions_test.go @@ -1243,7 +1243,7 @@ func TestRecordSessionPlannerDisplayConcurrentPreservesEverySession(t *testing.T func TestRecordSessionPlannerDisplayCrossProcessPreservesEverySession(t *testing.T) { if role := os.Getenv("REASONIX_PLANNER_DISPLAY_HELPER"); role != "" { dir := os.Getenv("REASONIX_PLANNER_DISPLAY_DIR") - sessionPlannerDisplayLockTimeout = 5 * time.Second + sessionPlannerDisplayExternalLockTimeout = 5 * time.Second attempted := filepath.Join(dir, role+".attempted") loaded := filepath.Join(dir, role+".loaded") release := filepath.Join(dir, role+".release") diff --git a/internal/filelock/filelock.go b/internal/filelock/filelock.go index d7b850ff11..c9ff243eda 100644 --- a/internal/filelock/filelock.go +++ b/internal/filelock/filelock.go @@ -36,6 +36,21 @@ var localRegistry = struct { // function is called. It serializes both goroutines in this process and other // Reasonix processes, and never waits past ctx's deadline. func Acquire(ctx context.Context, path string) (func(), error) { + return acquire(ctx, path, 0) +} + +// AcquireWithExternalTimeout obtains an exclusive lock while keeping the +// in-process queue and cross-process file-lock budgets separate. ctx bounds +// only the wait for another goroutine in this process; externalTimeout starts +// after that queue is acquired and bounds retries against other processes. +func AcquireWithExternalTimeout(ctx context.Context, path string, externalTimeout time.Duration) (func(), error) { + if externalTimeout <= 0 { + return nil, errors.New("external file lock timeout must be positive") + } + return acquire(ctx, path, externalTimeout) +} + +func acquire(ctx context.Context, path string, externalTimeout time.Duration) (func(), error) { if ctx == nil { ctx = context.Background() } @@ -48,6 +63,12 @@ func Acquire(ctx context.Context, path string) (func(), error) { return nil, err } _ = local + fileCtx := ctx + cancel := func() {} + if externalTimeout > 0 { + fileCtx, cancel = context.WithTimeout(context.Background(), externalTimeout) + } + defer cancel() for { releaseFile, err := tryLockFile(key) @@ -67,7 +88,7 @@ func Acquire(ctx context.Context, path string) (func(), error) { timer := time.NewTimer(retryInterval) select { case <-timer.C: - case <-ctx.Done(): + case <-fileCtx.Done(): if !timer.Stop() { select { case <-timer.C: @@ -75,7 +96,7 @@ func Acquire(ctx context.Context, path string) (func(), error) { } } releaseLocal() - return nil, fmt.Errorf("acquire file lock: %w", ctx.Err()) + return nil, fmt.Errorf("acquire file lock: %w", fileCtx.Err()) } } } diff --git a/internal/filelock/filelock_test.go b/internal/filelock/filelock_test.go index 3f74ea190c..b743740bd0 100644 --- a/internal/filelock/filelock_test.go +++ b/internal/filelock/filelock_test.go @@ -29,6 +29,34 @@ func TestAcquireHonorsDeadlineAndRecoversAfterRelease(t *testing.T) { secondRelease() } +func TestAcquireWithExternalTimeoutBoundsOnlyFileLockRetries(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.lock") + releaseExternal, err := tryLockFile(path) + if err != nil { + t.Fatalf("hold external file lock: %v", err) + } + defer releaseExternal() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + started := time.Now() + _, err = AcquireWithExternalTimeout(ctx, path, 60*time.Millisecond) + elapsed := time.Since(started) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("external acquire error = %v, want deadline exceeded", err) + } + if elapsed >= time.Second { + t.Fatalf("external acquire waited %v, want the short external budget", elapsed) + } +} + +func TestAcquireWithExternalTimeoutRejectsInvalidBudget(t *testing.T) { + path := filepath.Join(t.TempDir(), "state.lock") + if _, err := AcquireWithExternalTimeout(context.Background(), path, 0); err == nil { + t.Fatal("zero external timeout should be rejected") + } +} + func TestLocalRegistryReclaimsReleasedEntries(t *testing.T) { before := RegistrySizeForTest() path := filepath.Join(t.TempDir(), "ephemeral.lock")