From fc1ddd6e5ffb2a665a71cc09e7dd95010fba5773 Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 09:20:55 +0100 Subject: [PATCH 1/2] fix(webkit/window): publish state and layout saves atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lthn and core/ide both persist window state into the shared Core config directory. StateManager.save and LayoutManager.save wrote with plain truncate-and-write, so two processes racing the same file could splice one writer's short document onto the tail of the other's longer one — window_state.json arrived in production exactly that shape (a complete 538-byte document followed by 145 bytes of the previous 683-byte save), after which every load fails with "invalid character after top-level value" and all saved positions are lost. coreWriteFileAtomic writes a uniquely-suffixed sibling temp file and renames it into place: rename is atomic on POSIX, so readers and racing writers only ever observe complete documents. The suffix must be unique per writer — a fixed ".tmp" would move the same splice into the temp file. Mirrors core/go's Fs.WriteAtomic shape with the mode preserved. The Ugly test runs the two-manager storm against one path and asserts every observed read parses; it catches the old splice probabilistically and the new path deterministically. Co-Authored-By: Virgil --- go/display/webkit/pkg/window/core_helpers.go | 20 +++++++ go/display/webkit/pkg/window/layout.go | 2 +- go/display/webkit/pkg/window/state.go | 2 +- go/display/webkit/pkg/window/state_test.go | 58 ++++++++++++++++++++ 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/go/display/webkit/pkg/window/core_helpers.go b/go/display/webkit/pkg/window/core_helpers.go index 178588d..292f1db 100644 --- a/go/display/webkit/pkg/window/core_helpers.go +++ b/go/display/webkit/pkg/window/core_helpers.go @@ -27,6 +27,26 @@ func coreWriteFile(path string, data []byte, mode core.FileMode) resultFailure { return coreResultError(core.WriteFile(path, data, mode), "failed to write file") } +// coreWriteFileAtomic publishes data at path by writing a uniquely-named +// sibling temp file and renaming it into place. Rename is atomic on POSIX, +// so a reader — or a concurrent writer racing this one — always observes a +// COMPLETE document. The plain truncate-and-write it replaces let two +// processes sharing one state file (lthn and core/ide both persist into +// the Core config directory) splice a short document onto the tail of a +// longer one. The suffix must be unique per writer: a fixed ".tmp" name +// would simply move the same splice into the temp file. +func coreWriteFileAtomic(path string, data []byte, mode core.FileMode) resultFailure { + tmp := core.Concat(path, ".tmp.", core.Itoa(core.RandIntn(1<<30))) + if err := coreResultError(core.WriteFile(tmp, data, mode), "failed to write temp file"); err != nil { + return err + } + if err := coreResultError(core.Rename(tmp, path), "failed to publish file"); err != nil { + core.Remove(tmp) + return err + } + return nil +} + func coreMkdirAll(path string, mode core.FileMode) resultFailure { return coreResultError(core.MkdirAll(path, mode), "failed to create directory") } diff --git a/go/display/webkit/pkg/window/layout.go b/go/display/webkit/pkg/window/layout.go index 3e124a8..60b98a8 100644 --- a/go/display/webkit/pkg/window/layout.go +++ b/go/display/webkit/pkg/window/layout.go @@ -161,7 +161,7 @@ func (lm *LayoutManager) save() resultFailure { return core.E("window.LayoutManager.save", "failed to create window layout directory", err) } } - if err := coreWriteFile(filePath, data, 0o644); err != nil { + if err := coreWriteFileAtomic(filePath, data, 0o644); err != nil { core.Error( "window layout save failed", "file_path", filePath, diff --git a/go/display/webkit/pkg/window/state.go b/go/display/webkit/pkg/window/state.go index 702fe1d..8475096 100644 --- a/go/display/webkit/pkg/window/state.go +++ b/go/display/webkit/pkg/window/state.go @@ -160,7 +160,7 @@ func (sm *StateManager) save() resultFailure { return core.E("window.StateManager.save", "failed to create window state directory", err) } } - if err := coreWriteFile(filePath, data, 0o644); err != nil { + if err := coreWriteFileAtomic(filePath, data, 0o644); err != nil { core.Error( "window state save failed", "file_path", filePath, diff --git a/go/display/webkit/pkg/window/state_test.go b/go/display/webkit/pkg/window/state_test.go index 67e072b..67d5119 100644 --- a/go/display/webkit/pkg/window/state_test.go +++ b/go/display/webkit/pkg/window/state_test.go @@ -724,3 +724,61 @@ func TestState_StateManager_ForceSync_Ugly(t *core.T) { }) core.AssertNotNil(t, result.Value) } + +func TestStateManagerState_SaveAtomic_Good(t *core.T) { + // save — publishes via coreWriteFileAtomic + ax7Variant := "save:good" + core.AssertContains(t, ax7Variant, "good") + dir := t.TempDir() + sm := NewStateManagerWithDir(dir) + sm.states["main"] = WindowState{X: 1, Y: 2, Width: 640, Height: 480} + core.RequireNoError(t, sm.save()) + + content, err := coreReadFile(core.PathJoin(dir, "window_state.json")) + core.RequireNoError(t, err) + loaded := make(map[string]WindowState) + core.RequireTrue(t, core.JSONUnmarshal(content, &loaded).OK) + core.AssertEqual(t, 640, loaded["main"].Width) +} + +func TestStateManagerState_SaveAtomic_Ugly_ConcurrentWritersNeverSplice(t *core.T) { + // save — two managers share one path, as lthn and core/ide share the + // Core config directory in production. Every read during the storm + // must parse: rename publishes complete documents only. The plain + // truncate-and-write this guards against could splice one writer's + // short document onto the tail of the other's longer one. + ax7Variant := "save:ugly" + core.AssertContains(t, ax7Variant, "ugly") + path := core.PathJoin(t.TempDir(), "window_state.json") + long := NewStateManagerWithPath(path) + short := NewStateManagerWithPath(path) + for i := 0; i < 24; i++ { + long.states[core.Concat("window-", core.Itoa(i))] = WindowState{X: i, Y: i, Width: 1600, Height: 900} + } + short.states["main"] = WindowState{Width: 640, Height: 480} + + done := make(chan bool, 2) + writer := func(sm *StateManager) { + for i := 0; i < 50; i++ { + _ = sm.save() + } + done <- true + } + go writer(long) + go writer(short) + + spliced := 0 + for i := 0; i < 200; i++ { + content, err := coreReadFile(path) + if err != nil { + continue // the first save may not have landed yet + } + loaded := make(map[string]WindowState) + if !core.JSONUnmarshal(content, &loaded).OK { + spliced++ + } + } + <-done + <-done + core.AssertEqual(t, 0, spliced) +} From 800ef5f7f4beb1f4a79851426f006c2cb128f54b Mon Sep 17 00:00:00 2001 From: Snider Date: Sat, 8 Aug 2026 09:26:17 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix(webkit/window):=20review=20=E2=80=94=20?= =?UTF-8?q?exclusive=20temp=20creation=20+=20a=20test=20that=20proves=20ov?= =?UTF-8?q?erlap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CreateTemp (O_EXCL) replaces the probabilistic random suffix, so two writers can never share a temp file even in principle; the caller's mode contract is restored via Chmod before publish, and every failure branch now removes its temp file. The concurrent test seeds one save before the storm, reads until BOTH writers finish (overlap guaranteed), and propagates every save error instead of discarding them. Co-Authored-By: Virgil --- go/display/webkit/pkg/window/core_helpers.go | 34 +++++++++++++++++--- go/display/webkit/pkg/window/state_test.go | 25 ++++++++++---- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/go/display/webkit/pkg/window/core_helpers.go b/go/display/webkit/pkg/window/core_helpers.go index 292f1db..9230fbe 100644 --- a/go/display/webkit/pkg/window/core_helpers.go +++ b/go/display/webkit/pkg/window/core_helpers.go @@ -27,17 +27,41 @@ func coreWriteFile(path string, data []byte, mode core.FileMode) resultFailure { return coreResultError(core.WriteFile(path, data, mode), "failed to write file") } -// coreWriteFileAtomic publishes data at path by writing a uniquely-named +// coreWriteFileAtomic publishes data at path by writing an exclusively-created // sibling temp file and renaming it into place. Rename is atomic on POSIX, // so a reader — or a concurrent writer racing this one — always observes a // COMPLETE document. The plain truncate-and-write it replaces let two // processes sharing one state file (lthn and core/ide both persist into // the Core config directory) splice a short document onto the tail of a -// longer one. The suffix must be unique per writer: a fixed ".tmp" name -// would simply move the same splice into the temp file. +// longer one. CreateTemp opens with O_EXCL, so no two writers can ever +// share a temp file — a fixed ".tmp" name would simply move the same +// splice into the temp file. func coreWriteFileAtomic(path string, data []byte, mode core.FileMode) resultFailure { - tmp := core.Concat(path, ".tmp.", core.Itoa(core.RandIntn(1<<30))) - if err := coreResultError(core.WriteFile(tmp, data, mode), "failed to write temp file"); err != nil { + tmpR := core.CreateTemp(core.PathDir(path), core.Concat(core.PathBase(path), ".tmp.*")) + if !tmpR.OK { + return coreResultError(tmpR, "failed to create temp file") + } + file, ok := tmpR.Value.(interface { + Name() string + Write([]byte) (int, error) + Close() error + }) + if !ok { + return core.NewError("unexpected temp file type") + } + tmp := file.Name() + if _, err := file.Write(data); err != nil { + _ = file.Close() + core.Remove(tmp) + return err + } + if err := file.Close(); err != nil { + core.Remove(tmp) + return err + } + // CreateTemp opens at 0600; restore the caller's contract before publish. + if err := coreResultError(core.Chmod(tmp, mode), "failed to set file mode"); err != nil { + core.Remove(tmp) return err } if err := coreResultError(core.Rename(tmp, path), "failed to publish file"); err != nil { diff --git a/go/display/webkit/pkg/window/state_test.go b/go/display/webkit/pkg/window/state_test.go index 67d5119..e8ceda6 100644 --- a/go/display/webkit/pkg/window/state_test.go +++ b/go/display/webkit/pkg/window/state_test.go @@ -757,10 +757,15 @@ func TestStateManagerState_SaveAtomic_Ugly_ConcurrentWritersNeverSplice(t *core. } short.states["main"] = WindowState{Width: 640, Height: 480} + // The first save lands before the reader starts, so every read below + // observes a file that exists; the reader then runs until BOTH writers + // finish, so reads are guaranteed to overlap the write storm. + core.RequireNoError(t, long.save()) + saveErrs := make(chan resultFailure, 100) done := make(chan bool, 2) writer := func(sm *StateManager) { for i := 0; i < 50; i++ { - _ = sm.save() + saveErrs <- sm.save() } done <- true } @@ -768,17 +773,23 @@ func TestStateManagerState_SaveAtomic_Ugly_ConcurrentWritersNeverSplice(t *core. go writer(short) spliced := 0 - for i := 0; i < 200; i++ { - content, err := coreReadFile(path) - if err != nil { - continue // the first save may not have landed yet + finished := 0 + for finished < 2 { + select { + case <-done: + finished++ + default: } + content, err := coreReadFile(path) + core.RequireNoError(t, err) loaded := make(map[string]WindowState) if !core.JSONUnmarshal(content, &loaded).OK { spliced++ } } - <-done - <-done + close(saveErrs) + for err := range saveErrs { + core.RequireNoError(t, err) + } core.AssertEqual(t, 0, spliced) }