fix(memory): ReadMemoryAsOf double-delete keeps both deleted gaps (#4334)

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

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
 }

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)
+	}
+}