From 467a162c7969fb406ccead9a18becd09f9847ef7 Mon Sep 17 00:00:00 2001 From: Junjun Zhang Date: Tue, 20 Oct 2026 23:07:30 +0800 Subject: [PATCH] fix(memory): ReadMemoryAsOf double-delete keeps both deleted gaps via per-deletion tombstones (#4334) Co-Authored-By: ggcode --- internal/memory/gc.go | 11 ++- internal/memory/history.go | 59 +++++++++----- internal/memory/zz_issue4334_test.go | 113 +++++++++++++++++++++++++++ 3 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 internal/memory/zz_issue4334_test.go diff --git a/internal/memory/gc.go b/internal/memory/gc.go index 01052b958..126b5bd18 100644 --- a/internal/memory/gc.go +++ b/internal/memory/gc.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strconv" "time" "github.com/topcheer/ggcode/internal/debug" @@ -139,11 +140,17 @@ func (am *AutoMemory) DeleteMemory(key string) error { if err := os.MkdirAll(hdir, 0755); err != nil { return err } - tomb := filepath.Join(hdir, safe+".tombstone") + // #4334: each deletion gets its OWN timestamp-suffixed tombstone. The + // old single-path write overwrote the previous deletion instant on a + // delete->recreate->delete sequence, so ReadMemoryAsOf could not see the + // first deleted gap. The read side unions these with the legacy + // single-tombstone file (which still records one valid instant). + now := time.Now() + tomb := filepath.Join(hdir, safe+".tombstone."+strconv.FormatInt(now.UnixNano(), 10)) if err := os.WriteFile(tomb, []byte(key+"\n"), 0o644); err != nil { return err } - if err := os.Chtimes(tomb, time.Now(), time.Now()); err != nil { + if err := os.Chtimes(tomb, now, now); err != nil { return err } // sa-85: drop the provenance/usage record so deleted keys do not diff --git a/internal/memory/history.go b/internal/memory/history.go index dbd998498..3114f8bfa 100644 --- a/internal/memory/history.go +++ b/internal/memory/history.go @@ -110,6 +110,25 @@ func (am *AutoMemory) HistoryVersions(key string) []MemoryVersion { // participates as the newest version; archived versions fill earlier // intervals [version.ModTime, nextVersion.ModTime). Returns found=false when // the key did not exist at asOf yet. +// tombstoneTimes returns every recorded deletion instant for safe: the +// #4334 timestamp-suffixed tombstones plus the legacy single tombstone +// (kept so pre-#4334 deletion instants remain visible). sanitized keys are +// restricted to [a-zA-Z0-9_-], so the glob pattern itself is safe. +func (am *AutoMemory) tombstoneTimes(safe string) []time.Time { + var times []time.Time + if matches, gErr := filepath.Glob(filepath.Join(am.dir, historyDirName, safe+".tombstone.*")); gErr == nil { + for _, m := range matches { + if info, statErr := os.Stat(m); statErr == nil { + times = append(times, info.ModTime()) + } + } + } + if info, statErr := os.Stat(filepath.Join(am.dir, historyDirName, safe+".tombstone")); statErr == nil { + times = append(times, info.ModTime()) + } + return times +} + func (am *AutoMemory) ReadMemoryAsOf(key string, asOf time.Time) (content string, held time.Time, found bool, err error) { safe := disambiguateKey(key, sanitizeKey(key)) path := filepath.Join(am.dir, safe+".md") @@ -125,19 +144,17 @@ func (am *AutoMemory) ReadMemoryAsOf(key string, asOf time.Time) (content string if info, statErr := os.Stat(path); statErr == nil { versions = append(versions, snap{modTime: info.ModTime(), path: path}) } - // #3880/#3924: a tombstone (written by DeleteMemory) marks the deletion - // instant and CUTS the interval of the last pre-delete version: an - // as_of at/after the deletion resolves nothing UNLESS a newer version - // (a recreate) was written after the tombstone - then that version's - // own interval starts and selection picks it naturally. This keeps the - // three-segment truth: old version / deleted gap / recreated version. - // (The tombstone has no .md suffix, so historyVersions never lists it - // as a content version.) - tombPath := filepath.Join(am.dir, historyDirName, safe+".tombstone") - tombMTime := time.Time{} - if tInfo, tErr := os.Stat(tombPath); tErr == nil { - tombMTime = tInfo.ModTime() - } + // #3880/#3924/#4334: tombstones (written by DeleteMemory) mark deletion + // instants and CUT the interval of the last pre-delete version: an as_of + // at/after ANY deletion newer than the chosen version resolves nothing + // UNLESS a newer version (a recreate) was written after it - then that + // version's own interval starts and selection picks it naturally. This + // keeps the three-segment truth: old version / deleted gap / recreated + // version, and #4334 extends it to delete->recreate->delete (every + // deletion keeps its own tombstone; the legacy single tombstone is + // still honored). (Tombstones have no .md suffix, so historyVersions + // never lists them as content versions.) + tombTimes := am.tombstoneTimes(safe) if len(versions) == 0 { return "", time.Time{}, false, nil } @@ -158,12 +175,16 @@ func (am *AutoMemory) ReadMemoryAsOf(key string, asOf time.Time) (content string if readErr != nil { return "", time.Time{}, false, readErr } - // #3924: the chosen version predates the tombstone and the as_of is at - // or after the deletion - this is the DELETED GAP (no recreate landed - // before as_of), not the old version's interval. - if !tombMTime.IsZero() && versions[chosen].modTime.Before(tombMTime) && !asOf.Before(tombMTime) { - debug.Log("memory", "as_of %s falls in the deleted gap of %q", asOf.Format(time.RFC3339), key) - return "", time.Time{}, false, nil + // #3924/#4334: the chosen version predates a tombstone and the as_of is + // at or after that deletion - this is a DELETED GAP (no recreate landed + // before as_of), not the old version's interval. Every recorded + // deletion instant is checked independently so a double delete keeps + // BOTH gaps. + for _, t := range tombTimes { + if versions[chosen].modTime.Before(t) && !asOf.Before(t) { + debug.Log("memory", "as_of %s falls in the deleted gap of %q", asOf.Format(time.RFC3339), key) + return "", time.Time{}, false, nil + } } return string(data), versions[chosen].modTime, true, nil } diff --git a/internal/memory/zz_issue4334_test.go b/internal/memory/zz_issue4334_test.go new file mode 100644 index 000000000..1c4fcf8c7 --- /dev/null +++ b/internal/memory/zz_issue4334_test.go @@ -0,0 +1,113 @@ +package memory + +// #4334 probes: a delete->recreate->delete sequence must keep BOTH deleted +// gaps - the old single-tombstone overwrite lost the first deletion +// instant, so an as_of inside the first gap time-traveled back to the +// already-deleted v1. + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestIssue4334_DoubleDeleteKeepsFirstGap(t *testing.T) { + dir := t.TempDir() + am := NewProjectAutoMemory(dir) + if am == nil { + t.Fatal("auto memory nil") + } + if err := am.SaveMemory("dd", "v1"); err != nil { + t.Fatal(err) + } + preDelete := time.Now() + time.Sleep(10 * time.Millisecond) + if err := am.DeleteMemory("dd"); err != nil { // T1 + t.Fatal(err) + } + firstGapAt := time.Now() + time.Sleep(10 * time.Millisecond) + if err := am.SaveMemory("dd", "v2"); err != nil { // recreate + t.Fatal(err) + } + midV2 := time.Now() + time.Sleep(10 * time.Millisecond) + if err := am.DeleteMemory("dd"); err != nil { // T2 + t.Fatal(err) + } + afterSecond := time.Now() + time.Sleep(10 * time.Millisecond) + + // The core regression: as_of inside the FIRST deleted gap [T1, recreate) + // must find nothing - the single-tombstone bug returned v1 here. + if _, _, found, _ := am.ReadMemoryAsOf("dd", firstGapAt); found { + t.Fatal("as_of in the first deleted gap must find nothing (tombstone overwrite regression)") + } + // After the second deletion: also nothing. + if _, _, found, _ := am.ReadMemoryAsOf("dd", afterSecond); found { + t.Fatal("as_of after the second delete must find nothing") + } + // Before the first deletion: v1 is exact. + if content, _, found, _ := am.ReadMemoryAsOf("dd", preDelete); !found || content != "v1" { + t.Fatalf("pre-delete as_of = (%q, %v), want (v1, true)", content, found) + } + // Between the recreate and the second deletion: v2 is exact. + if content, _, found, _ := am.ReadMemoryAsOf("dd", midV2); !found || content != "v2" { + t.Fatalf("recreated-interval as_of = (%q, %v), want (v2, true)", content, found) + } +} + +// Legacy compatibility: a pre-#4334 single tombstone (no timestamp suffix) +// must still cut the interval after its deletion instant. +func TestIssue4334_LegacySingleTombstoneStillHonored(t *testing.T) { + dir := t.TempDir() + am := NewProjectAutoMemory(dir) + if am == nil { + t.Fatal("auto memory nil") + } + if err := am.SaveMemory("legacy-tomb", "v1"); err != nil { + t.Fatal(err) + } + midAt := time.Now() + time.Sleep(10 * time.Millisecond) + if err := am.DeleteMemory("legacy-tomb"); err != nil { + t.Fatal(err) + } + delAt := time.Now() + time.Sleep(10 * time.Millisecond) + + // Reshape to pre-#4334 on-disk state: replace every timestamp-suffixed + // tombstone with one legacy single-path file carrying the same mtime. + hdir := filepath.Join(am.dir, historyDirName) + safe := disambiguateKey("legacy-tomb", sanitizeKey("legacy-tomb")) + matches, _ := filepath.Glob(filepath.Join(hdir, safe+".tombstone.*")) + var legacyMTime time.Time + for _, m := range matches { + if info, err := os.Stat(m); err == nil { + legacyMTime = info.ModTime() + } + if err := os.Remove(m); err != nil { + t.Fatal(err) + } + } + if legacyMTime.IsZero() { + t.Fatal("expected a timestamp-suffixed tombstone to reshape") + } + legacy := filepath.Join(hdir, safe+".tombstone") + if err := os.WriteFile(legacy, []byte("legacy-tomb\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(legacy, legacyMTime, legacyMTime); err != nil { + t.Fatal(err) + } + + // After the deletion instant (legacy tombstone only): found=false. + if _, _, found, _ := am.ReadMemoryAsOf("legacy-tomb", delAt); found { + t.Fatal("legacy single tombstone must still cut the interval after deletion") + } + // Before the deletion instant: v1 resolves exactly. + if content, _, found, _ := am.ReadMemoryAsOf("legacy-tomb", midAt); !found || content != "v1" { + t.Fatalf("pre-delete as_of = (%q, %v), want (v1, true)", content, found) + } +}