From 79f9b3752b872cf9b79b60fe0c3cb697465380c8 Mon Sep 17 00:00:00 2001 From: yetone Date: Thu, 8 Oct 2026 19:04:18 +0800 Subject: [PATCH] sessions: a Codex archived copy rewritten at the same size in the clock tick of its last comparison is compared again, not taken from the cache, on Windows too (#1299, wizzy-yang) The copy cache trusted a change time it had seen as proof the file had not changed. NTFS stamps ChangeTime from Windows' clock tick, not the precise clock: on the Windows box, 300 ms of back-to-back same-size writes got a new ChangeTime only every ~0.5 ms (2147 of 2691 writes kept the previous stamp; diffs 0.5/1.0/1.5 ms). A rewrite in the tick of the comparison kept the stamp the cache saw, so the cached relation was reused. That, not write merging, is why TestCodexArchivedSameSizeRewriteInvalidatesDiscovery failed 19 of 300 runs on Windows, alone as well as in the full package. A comparison is now cached only when both files' change times are more than a second older than the moment it started (codexStampSettle); a file changed more recently is compared again next time. A later rewrite then lands in a later tick and changes the stamp. Change times are compared as times (Windows FILETIME converted). TestCodexSameTickRewriteIsNotCached gives every write one change time and a clock 10 ms into that tick. With the settle rule taken out (cacheable = both stamps present), it fails: "a rewrite in the tick of the comparison reused its stale relation". With it, it passes, and the second half checks settled copies are still compared once, not every time. TestCodexArchivedDiscoveryCacheAndAppend moves its clock a minute on so the just-written copies count as settled. The Windows CI step now runs the Codex copy tests (CodexArchived, CodexSameTick, CodexPlainTwin), which it had never run. Run: Windows 11 box (cross-built sessions.test.exe, sandbox USERPROFILE/ APPDATA/TEMP): old binary 19/300 failures of the reported test; new binary 0/1000 for the three Codex copy tests, and the Windows CI regex x5 with testdata passes. macOS: go test -race ./internal/sessions, -race -count=30 of the Codex copy tests, go vet (darwin, GOOS=linux, GOOS=windows), gofmt -l. Not run: Linux with a coarse ctime clock (kernels before 6.13); the rule covers it the same way. --- .github/workflows/test.yml | 5 +- internal/sessions/codex_archive_test.go | 60 +++++++++++++++++++- internal/sessions/codex_discovery.go | 35 +++++++++--- internal/sessions/codex_discovery_windows.go | 5 +- 4 files changed, 92 insertions(+), 13 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 66f30c3a..90e6de91 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -122,10 +122,11 @@ jobs: run: go test -v -count=1 -timeout 2m ./internal/update -run '^(TestInstallBinary|TestMoveAside)' # These tests use temporary paths, without the Unix HOME assumptions - # of the full suite. Run the native link-kind and sharing-error checks. + # of the full suite. Run the native link-kind and sharing-error checks, + # and the Codex copy checks that read NTFS ChangeTime (#1299). - name: Windows session moves if: runner.os == 'Windows' - run: go test -v -count=1 ./internal/sessions -run '^Test(WindowsSession|Move|CopyAll)' + run: go test -v -count=1 ./internal/sessions -run '^Test(WindowsSession|Move|CopyAll|CodexArchived|CodexSameTick|CodexPlainTwin)' # dsh's layered config is read and edited in temporary homes. Run # these checks on Windows too, including Desktop's home patch layer. diff --git a/internal/sessions/codex_archive_test.go b/internal/sessions/codex_archive_test.go index 1470b1ea..4f27bc8e 100644 --- a/internal/sessions/codex_archive_test.go +++ b/internal/sessions/codex_archive_test.go @@ -109,9 +109,14 @@ func TestCodexArchivedDiscoveryCacheAndAppend(t *testing.T) { if err != nil { t.Fatal(err) } - if codexChangeStamp(info) == "" { + if codexChangeTime(info).IsZero() { t.Skip("filesystem exposes no change-time stamp; safe fallback reads content") } + // The copies were written a moment ago; a minute on, their stamps have + // settled and the comparison is cached. + now := codexDiscoveryNow + codexDiscoveryNow = func() time.Time { return time.Now().Add(time.Minute) } + t.Cleanup(func() { codexDiscoveryNow = now }) reads, compare := 0, compareCodexCopies compareCodexCopies = func(a, b string) (int, error) { reads++ @@ -158,6 +163,59 @@ func TestCodexArchivedSameSizeRewriteInvalidatesDiscovery(t *testing.T) { } } +// #1299: NTFS stamps ChangeTime from Windows' clock tick (0.5 ms here, up +// to 15.6 ms), so a rewrite in the tick of the comparison keeps the stamp +// the cache saw. Here every write in the test lands in one 15.6 ms tick and +// gets the same change time, and the rewrite must still be seen. +func TestCodexSameTickRewriteIsNotCached(t *testing.T) { + d := setupCalls(t) + name := "rollout-2026-09-20T10-00-00-0190aaaa-1111-7222-8333-444455556666.jsonl" + live, archived := filepath.Join(d.codex, "sessions", name), filepath.Join(d.codex, "archived_sessions", name) + tick := time.Now().Truncate(time.Second) + stat, now := codexDiscoveryStat, codexDiscoveryNow + codexDiscoveryStat = func(path string) (os.FileInfo, error) { + info, err := os.Stat(path) + if err != nil { + return nil, err + } + return codexDiscoveryInfo{info, tick}, nil + } + codexDiscoveryNow = func() time.Time { return tick.Add(10 * time.Millisecond) } + t.Cleanup(func() { codexDiscoveryStat, codexDiscoveryNow = stat, now }) + writeLines(t, live, `{"x":1}`) + writeLines(t, archived, `{"x":1}`) + info, err := os.Stat(archived) + if err != nil { + t.Fatal(err) + } + if len(codexFiles()) != 1 { + t.Fatal("initial copies were not deduplicated") + } + writeLines(t, archived, `{"x":2}`) + if err := os.Chtimes(archived, info.ModTime(), info.ModTime()); err != nil { + t.Fatal(err) + } + if fs := codexFiles(); len(fs) != 2 { + t.Fatalf("a rewrite in the tick of the comparison reused its stale relation: %+v", fs) + } + // Once the tick is long past, the same stamps are cached. + codexDiscoveryNow = func() time.Time { return tick.Add(time.Hour) } + reads, compare := 0, compareCodexCopies + compareCodexCopies = func(a, b string) (int, error) { + reads++ + return compare(a, b) + } + t.Cleanup(func() { compareCodexCopies = compare }) + for range 3 { + if len(codexFiles()) != 2 { + t.Fatal("settled divergent copies were deduplicated") + } + } + if reads != 1 { + t.Fatalf("settled copies were compared %d times, want 1", reads) + } +} + func TestCodexArchivedReadErrorIsNotCached(t *testing.T) { d := setupCalls(t) name := "rollout-2026-09-20T10-00-00-0190aaaa-1111-7222-8333-444455556666.jsonl" diff --git a/internal/sessions/codex_discovery.go b/internal/sessions/codex_discovery.go index f1143c1f..c7984a24 100644 --- a/internal/sessions/codex_discovery.go +++ b/internal/sessions/codex_discovery.go @@ -2,13 +2,13 @@ package sessions import ( "bytes" - "fmt" "io" "os" "path/filepath" "reflect" "strings" "sync" + "time" ) const ( @@ -33,13 +33,24 @@ var codexCopyCache = struct { // Windows replaces this with a same-handle stat carrying ChangeTime. var codexDiscoveryStat = os.Stat var compareCodexCopies = readCodexCopies +var codexDiscoveryNow = time.Now + +// A file system stamps change times from the clock tick, not the precise +// clock: NTFS from Windows' system time (0.5 ms to 15.6 ms), Linux from the +// coarse kernel clock. Two writes within one tick get the same change time, +// so a comparison is cached only for files whose change time is older than +// this when it starts. A later rewrite is then in a later tick and changes +// the stamp; a file changed more recently is compared again. +const codexStampSettle = time.Second type codexDiscoveryInfo struct { os.FileInfo - ChangeTime string + ChangeTime time.Time } -func codexChangeStamp(info os.FileInfo) string { +// codexChangeTime is the file's change time, zero when the file system +// doesn't expose one. +func codexChangeTime(info os.FileInfo) time.Time { if wrapped, ok := info.(codexDiscoveryInfo); ok { return wrapped.ChangeTime } @@ -51,22 +62,29 @@ func codexChangeStamp(info os.FileInfo) string { v = v.Elem() } if v.Kind() != reflect.Struct { - return "" + return time.Time{} } for _, name := range []string{"Ctimespec", "Ctim"} { c := v.FieldByName(name) if c.IsValid() && c.Kind() == reflect.Struct { sec, nsec := c.FieldByName("Sec"), c.FieldByName("Nsec") if sec.IsValid() && nsec.IsValid() && sec.CanInt() && nsec.CanInt() { - return fmt.Sprintf("%d:%d", sec.Int(), nsec.Int()) + return time.Unix(sec.Int(), nsec.Int()) } } } - return "" + return time.Time{} +} + +// settledCodexStamp reports whether a comparison of this file, started at +// start, can be cached: it has a change time from before the current tick. +func settledCodexStamp(info os.FileInfo, start time.Time) bool { + c := codexChangeTime(info) + return !c.IsZero() && c.Before(start.Add(-codexStampSettle)) } func sameCodexSnapshot(a, b os.FileInfo) bool { - if a == nil || b == nil || a.Size() != b.Size() || !a.ModTime().Equal(b.ModTime()) || codexChangeStamp(a) != codexChangeStamp(b) { + if a == nil || b == nil || a.Size() != b.Size() || !a.ModTime().Equal(b.ModTime()) || !codexChangeTime(a).Equal(codexChangeTime(b)) { return false } if wrapped, ok := a.(codexDiscoveryInfo); ok { @@ -112,13 +130,14 @@ func distinctCodexFiles(files []file) []file { } func codexCopyRelation(first, second string) int { + start := codexDiscoveryNow() a, errA := codexDiscoveryStat(first) b, errB := codexDiscoveryStat(second) if errA != nil || errB != nil { return codexCopiesDiffer } key := codexCopyPair{first, second} - cacheable := codexChangeStamp(a) != "" && codexChangeStamp(b) != "" + cacheable := settledCodexStamp(a, start) && settledCodexStamp(b, start) codexCopyCache.Lock() saved, found := codexCopyCache.Pairs[key] codexCopyCache.Unlock() diff --git a/internal/sessions/codex_discovery_windows.go b/internal/sessions/codex_discovery_windows.go index 707d3014..f8727396 100644 --- a/internal/sessions/codex_discovery_windows.go +++ b/internal/sessions/codex_discovery_windows.go @@ -1,8 +1,8 @@ package sessions import ( - "fmt" "os" + "time" "unsafe" "golang.org/x/sys/windows" @@ -30,5 +30,6 @@ func statCodexDiscovery(path string) (os.FileInfo, error) { (*byte)(unsafe.Pointer(&basic)), uint32(unsafe.Sizeof(basic))); err != nil || basic.Change == 0 { return info, nil // no stamp means content comparisons are not cached } - return codexDiscoveryInfo{info, fmt.Sprint(basic.Change)}, nil + change := windows.Filetime{LowDateTime: uint32(basic.Change), HighDateTime: uint32(basic.Change >> 32)} + return codexDiscoveryInfo{info, time.Unix(0, change.Nanoseconds())}, nil }