From 8fc019e8c5703bc0fd09de52026093a3d20c8e20 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:24:38 +0530 Subject: [PATCH 01/14] feat(sessions): prune old sessions, never one another process has open Store.Prune removes sessions last updated before a cutoff of at least a day, and reports what it removed, what it kept and why, and what failed. A dry run makes the same decisions and removes nothing. A session holds no lock between writes, so nothing told a prune in one terminal that another terminal still had the session open. Each session now has a lease file: a process that writes a session, or loads it through the rehydrated reads the TUI, exec --resume and ACP use to continue one, holds a shared lock on it for its lifetime, and prune takes it exclusively and leaves alone any session it cannot get. On Windows the lease file is opened with FILE_SHARE_DELETE so a held lease never blocks removing the directory. Prune also keeps every ancestor of a session that stays, since Lineage and Tree fail on a missing ancestor, removes descendants before their ancestors, re-checks each session under its lease and write lock, and removes the metadata first so a removal that fails part way leaves nothing that looks like a session. Part of #971. --- internal/sessions/lease.go | 85 ++++++++ internal/sessions/lease_unix.go | 40 ++++ internal/sessions/lease_windows.go | 56 ++++++ internal/sessions/prune.go | 305 +++++++++++++++++++++++++++++ internal/sessions/prune_test.go | 300 ++++++++++++++++++++++++++++ internal/sessions/replay.go | 4 + internal/sessions/store.go | 13 ++ 7 files changed, 803 insertions(+) create mode 100644 internal/sessions/lease.go create mode 100644 internal/sessions/lease_unix.go create mode 100644 internal/sessions/lease_windows.go create mode 100644 internal/sessions/prune.go create mode 100644 internal/sessions/prune_test.go diff --git a/internal/sessions/lease.go b/internal/sessions/lease.go new file mode 100644 index 000000000..9779b2862 --- /dev/null +++ b/internal/sessions/lease.go @@ -0,0 +1,85 @@ +package sessions + +import ( + "os" + "path/filepath" +) + +// leaseFileName is the file whose lock says a process has the session open. +const leaseFileName = "lease.lock" + +func (store *Store) leasePath(sessionID string) string { + return filepath.Join(store.sessionPath(sessionID), leaseFileName) +} + +// Hold marks sessionID as open in this process until the process exits. +// +// A SESSION HOLDS NO LOCK BETWEEN WRITES. session.lock is taken around each +// write and released straight after, so nothing told `zero sessions prune` in +// another terminal that a TUI, an exec run or an editor still had the session +// open, and an idle open session looked exactly like an abandoned one. Hold takes +// a shared lock on the session's lease file and keeps it for the life of the +// process; prune takes the same lock exclusively and leaves alone any session it +// cannot get. Every write holds its session this way (lockSession), and so does +// every rehydrated read, which is how the TUI, `exec --resume` and ACP load a +// session in order to continue it. +// +// It never waits and never fails its caller. A lease that cannot be taken, +// because the session directory is gone or prune holds it at this moment, only +// means prune cannot see this process; the operation that asked carries on and +// meets a removed session on its own terms. +func (store *Store) Hold(sessionID string) { + if !ValidSessionID(sessionID) { + return + } + store.leasesMu.Lock() + defer store.leasesMu.Unlock() + if _, held := store.leases[sessionID]; held { + return + } + // Creates the lease file, never the directory: a session that is gone stays + // gone. + file, err := openLeaseFile(store.leasePath(sessionID)) + if err != nil { + return + } + if locked, err := tryLockLease(file, false); err != nil || !locked { + _ = file.Close() + return + } + if store.leases == nil { + store.leases = map[string]*os.File{} + } + store.leases[sessionID] = file +} + +// Release gives up this Store's lease on sessionID, for a long-lived process +// that is done with a session before it exits. A process that exits releases +// every lease with it. +func (store *Store) Release(sessionID string) { + store.leasesMu.Lock() + defer store.leasesMu.Unlock() + file, held := store.leases[sessionID] + if !held { + return + } + delete(store.leases, sessionID) + unlockLease(file) + _ = file.Close() +} + +// acquireLeaseExclusive takes the lease exclusively for as long as prune is +// removing the session, so no process can open it part way through. It reports +// false, holding nothing, when a lease is already held. +func (store *Store) acquireLeaseExclusive(sessionID string) (*os.File, bool, error) { + file, err := openLeaseFile(store.leasePath(sessionID)) + if err != nil { + return nil, false, err + } + locked, err := tryLockLease(file, true) + if err != nil || !locked { + _ = file.Close() + return nil, false, err + } + return file, true, nil +} diff --git a/internal/sessions/lease_unix.go b/internal/sessions/lease_unix.go new file mode 100644 index 000000000..d323bcf57 --- /dev/null +++ b/internal/sessions/lease_unix.go @@ -0,0 +1,40 @@ +//go:build !windows + +package sessions + +import ( + "errors" + "os" + + "golang.org/x/sys/unix" +) + +// tryLockLease takes the lease lock on file without waiting: shared for a +// process that has the session open, exclusive for prune. It reports false when +// another holder's lock conflicts. flock locks belong to the open file +// description, so a second descriptor in the same process conflicts like another +// process would. +func tryLockLease(file *os.File, exclusive bool) (bool, error) { + how := unix.LOCK_SH + if exclusive { + how = unix.LOCK_EX + } + err := unix.Flock(int(file.Fd()), how|unix.LOCK_NB) + if err == nil { + return true, nil + } + if errors.Is(err, unix.EWOULDBLOCK) { + return false, nil + } + return false, err +} + +// openLeaseFile opens (creating it if needed) the lease file of a session whose +// directory exists. +func openLeaseFile(path string) (*os.File, error) { + return os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0o600) +} + +func unlockLease(file *os.File) { + _ = unix.Flock(int(file.Fd()), unix.LOCK_UN) +} diff --git a/internal/sessions/lease_windows.go b/internal/sessions/lease_windows.go new file mode 100644 index 000000000..207bb7b1e --- /dev/null +++ b/internal/sessions/lease_windows.go @@ -0,0 +1,56 @@ +//go:build windows + +package sessions + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// tryLockLease takes the lease lock on file without waiting: shared for a +// process that has the session open, exclusive for prune. It reports false when +// another holder's lock conflicts. LockFileEx locks belong to the handle, so a +// second handle in the same process conflicts like another process would. +func tryLockLease(file *os.File, exclusive bool) (bool, error) { + flags := uint32(windows.LOCKFILE_FAIL_IMMEDIATELY) + if exclusive { + flags |= windows.LOCKFILE_EXCLUSIVE_LOCK + } + err := windows.LockFileEx(windows.Handle(file.Fd()), flags, 0, 1, 0, new(windows.Overlapped)) + if err == nil { + return true, nil + } + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return false, nil + } + return false, err +} + +// openLeaseFile opens (creating it if needed) the lease file of a session whose +// directory exists. +// +// WITH FILE_SHARE_DELETE, unlike session.lock. A lease is held for the life of +// the process, and a file held open without share-delete cannot be removed: the +// session directory could then never be deleted while any process had it open, +// by prune or by anything else, including a test's temp-directory cleanup. With +// it, a delete removes the name at once (POSIX semantics) while the holder's +// handle stays valid. +func openLeaseFile(path string) (*os.File, error) { + pathUTF16, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, &os.PathError{Op: "open", Path: path, Err: err} + } + handle, err := windows.CreateFile(pathUTF16, windows.GENERIC_READ|windows.GENERIC_WRITE, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, windows.OPEN_ALWAYS, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + return nil, &os.PathError{Op: "open", Path: path, Err: err} + } + return os.NewFile(uintptr(handle), path), nil +} + +func unlockLease(file *os.File) { + _ = windows.UnlockFileEx(windows.Handle(file.Fd()), 0, 1, 0, new(windows.Overlapped)) +} diff --git a/internal/sessions/prune.go b/internal/sessions/prune.go new file mode 100644 index 000000000..2fca93fe6 --- /dev/null +++ b/internal/sessions/prune.go @@ -0,0 +1,305 @@ +package sessions + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "sort" + "time" +) + +// MinimumPruneAge is the shortest cutoff Prune accepts. +// +// A floor under the lease, not a substitute for it. An older Zero, or anything +// else that writes a session without taking the lease, is invisible to prune, +// and a day is long enough that a session such a writer is actively using has +// been written since. +const MinimumPruneAge = 24 * time.Hour + +// PruneOptions chooses what Prune removes. +type PruneOptions struct { + // OlderThan makes a session a candidate when it was last updated at least + // this long ago. It must be at least MinimumPruneAge. + OlderThan time.Duration + // DryRun decides everything a real run would and removes nothing. + DryRun bool +} + +// PruneEntry is one session Prune removed (or in a dry run would remove), kept +// although it was old enough, or failed to remove. +type PruneEntry struct { + SessionID string `json:"sessionId"` + Title string `json:"title,omitempty"` + Kind SessionKind `json:"kind,omitempty"` + UpdatedAt string `json:"updatedAt"` + Bytes int64 `json:"bytes"` + Reason string `json:"reason,omitempty"` +} + +// PruneReport says what Prune did, or in a dry run what it would do. Sessions +// newer than the cutoff are not listed. +type PruneReport struct { + Cutoff string `json:"cutoff"` + DryRun bool `json:"dryRun"` + // Removed are the sessions removed, or in a dry run the ones a real run + // would remove. + Removed []PruneEntry `json:"removed"` + // Kept are sessions Prune left for a reason other than being recent. + Kept []PruneEntry `json:"kept"` + // Failed are sessions whose removal went wrong; Reason says how. A failure + // after the metadata was removed leaves a directory List no longer shows. + Failed []PruneEntry `json:"failed"` +} + +// The reasons Prune keeps a session that is old enough to remove. +const ( + PruneKeptOpen = "open in another Zero process" + PruneKeptParent = "parent of a session that is kept" + PruneKeptUpdated = "updated while pruning" + PruneKeptUndated = "its last update time could not be read" +) + +// pruneRemoveSeam runs after Prune holds a session's lease and write lock and +// before it re-reads the metadata. Nil in production. +var pruneRemoveSeam func(sessionID string) + +// Prune removes sessions last updated before the cutoff that OlderThan sets. +// +// Only on request: nothing in Zero calls it by itself (#971). It never removes: +// - a session another process has open, which holds its lease (see Hold); +// - a session with a descendant that is kept, because Lineage and Tree fail +// on a missing ancestor; +// - a session written between the plan and its removal; +// - a session whose last update time cannot be read. +// +// Descendants are removed before their ancestors, so a descendant that turns +// out to be open part way through still keeps every ancestor above it. Within a +// session the metadata goes first, under both the lease and the write lock: +// from then on List and Get do not show it, so a removal that fails part way +// leaves nothing that looks like a session. +func (store *Store) Prune(options PruneOptions) (PruneReport, error) { + if options.OlderThan < MinimumPruneAge { + return PruneReport{}, fmt.Errorf("prune cutoff %s is shorter than the minimum of %s", options.OlderThan, MinimumPruneAge) + } + cutoff := store.now().UTC().Add(-options.OlderThan) + report := PruneReport{ + Cutoff: cutoff.Format(time.RFC3339), + DryRun: options.DryRun, + Removed: []PruneEntry{}, + Kept: []PruneEntry{}, + Failed: []PruneEntry{}, + } + all, err := store.List() + if err != nil { + return report, err + } + byID := make(map[string]Metadata, len(all)) + for _, session := range all { + byID[session.SessionID] = session + } + + candidates := map[string]bool{} + for _, session := range all { + updated, err := time.Parse(time.RFC3339, session.UpdatedAt) + if err != nil { + report.Kept = append(report.Kept, store.pruneEntry(session, PruneKeptUndated)) + continue + } + if !updated.Before(cutoff) { + continue + } + // Open elsewhere at planning time. Checked again under the lease when the + // session is removed; this pass is what lets its ancestors be kept too. + lease, locked, err := store.acquireLeaseExclusive(session.SessionID) + if err != nil { + report.Kept = append(report.Kept, store.pruneEntry(session, "its lease could not be checked: "+err.Error())) + continue + } + if !locked { + report.Kept = append(report.Kept, store.pruneEntry(session, PruneKeptOpen)) + continue + } + unlockLease(lease) + _ = lease.Close() + candidates[session.SessionID] = true + } + + // Every ancestor of a session that stays, stays. + for _, session := range all { + if candidates[session.SessionID] { + continue + } + for _, ancestor := range pruneAncestors(byID, session.SessionID) { + if candidates[ancestor] { + delete(candidates, ancestor) + report.Kept = append(report.Kept, store.pruneEntry(byID[ancestor], PruneKeptParent)) + } + } + } + + order := make([]Metadata, 0, len(candidates)) + for id := range candidates { + order = append(order, byID[id]) + } + depth := func(id string) int { return len(pruneAncestors(byID, id)) } + sort.Slice(order, func(left, right int) bool { + leftDepth, rightDepth := depth(order[left].SessionID), depth(order[right].SessionID) + if leftDepth != rightDepth { + return leftDepth > rightDepth + } + if order[left].UpdatedAt != order[right].UpdatedAt { + return order[left].UpdatedAt < order[right].UpdatedAt + } + return order[left].SessionID < order[right].SessionID + }) + + keep := map[string]bool{} + for _, session := range order { + if keep[session.SessionID] { + report.Kept = append(report.Kept, store.pruneEntry(session, PruneKeptParent)) + continue + } + entry := store.pruneEntry(session, "") + if options.DryRun { + report.Removed = append(report.Removed, entry) + continue + } + removed, keptReason, err := store.pruneSession(session.SessionID, cutoff) + switch { + case err != nil: + entry.Reason = err.Error() + report.Failed = append(report.Failed, entry) + case keptReason != "": + entry.Reason = keptReason + report.Kept = append(report.Kept, entry) + case removed: + report.Removed = append(report.Removed, entry) + } + if !removed { + for _, ancestor := range pruneAncestors(byID, session.SessionID) { + keep[ancestor] = true + } + } + } + return report, nil +} + +// pruneAncestors lists a session's ancestors, nearest first, stopping at a +// missing one or a cycle. +func pruneAncestors(byID map[string]Metadata, sessionID string) []string { + var ancestors []string + seen := map[string]bool{sessionID: true} + current, ok := byID[sessionID] + for ok && current.ParentSessionID != "" && !seen[current.ParentSessionID] { + parentID := current.ParentSessionID + seen[parentID] = true + ancestors = append(ancestors, parentID) + current, ok = byID[parentID] + } + return ancestors +} + +func (store *Store) pruneEntry(session Metadata, reason string) PruneEntry { + return PruneEntry{ + SessionID: session.SessionID, + Title: session.Title, + Kind: session.SessionKind, + UpdatedAt: session.UpdatedAt, + Bytes: store.sessionBytes(session.SessionID), + Reason: reason, + } +} + +// sessionBytes is what a session occupies on disk, without following links. +func (store *Store) sessionBytes(sessionID string) int64 { + var total int64 + _ = filepath.WalkDir(store.sessionPath(sessionID), func(_ string, entry fs.DirEntry, err error) error { + if err != nil || entry.IsDir() { + return nil + } + if info, err := entry.Info(); err == nil && info.Mode().IsRegular() { + total += info.Size() + } + return nil + }) + return total +} + +// pruneSession removes one session that planning chose. It reports removed +// once the metadata is gone, even when leftovers could not be deleted (err says +// what), and a keptReason when the session turned out to be open or was written +// since the plan. +func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bool, keptReason string, err error) { + lease, locked, err := store.acquireLeaseExclusive(sessionID) + if err != nil { + return false, "", fmt.Errorf("check the session's lease: %w", err) + } + if !locked { + return false, PruneKeptOpen, nil + } + defer func() { + unlockLease(lease) + _ = lease.Close() + }() + release, err := store.lockSessionWithoutLease(sessionID) + if err != nil { + return false, "", err + } + released := false + defer func() { + if !released { + release() + } + }() + if pruneRemoveSeam != nil { + pruneRemoveSeam(sessionID) + } + + session, err := store.readMetadata(sessionID) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return false, "", fmt.Errorf("the session disappeared while pruning") + } + return false, "", err + } + updated, err := time.Parse(time.RFC3339, session.UpdatedAt) + if err != nil { + return false, PruneKeptUndated, nil + } + if !updated.Before(cutoff) { + return false, PruneKeptUpdated, nil + } + + // THE METADATA FIRST. From here the session no longer exists to List or Get. + if err := os.Remove(store.metadataPath(sessionID)); err != nil { + return false, "", fmt.Errorf("remove the session metadata: %w", err) + } + dir := store.sessionPath(sessionID) + entries, err := os.ReadDir(dir) + if err != nil { + return true, "", fmt.Errorf("list what is left of the session: %w", err) + } + for _, entry := range entries { + // session.lock is held right here; it goes after the release below. The + // lease file is held too, but it was opened for deletion (openLeaseFile). + if entry.Name() == filepath.Base(store.lockPath(sessionID)) { + continue + } + if err := os.RemoveAll(filepath.Join(dir, entry.Name())); err != nil { + return true, "", fmt.Errorf("remove %s: %w", entry.Name(), err) + } + } + release() + released = true + // A writer that takes session.lock in the gap only keeps an empty directory + // alive, and its own write then fails: the metadata is already gone. + if err := os.Remove(store.lockPath(sessionID)); err != nil && !errors.Is(err, fs.ErrNotExist) { + return true, "", fmt.Errorf("remove the session lock: %w", err) + } + if err := os.Remove(dir); err != nil && !errors.Is(err, fs.ErrNotExist) { + return true, "", fmt.Errorf("remove the session directory: %w", err) + } + return true, "", nil +} diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go new file mode 100644 index 000000000..b8d306ee0 --- /dev/null +++ b/internal/sessions/prune_test.go @@ -0,0 +1,300 @@ +package sessions + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// pruneNow is "now" for every prune in these tests. +const pruneNow = "2026-09-26T00:00:00Z" + +const thirtyDays = 30 * 24 * time.Hour + +func pruneStore(root string) *Store { + return NewStore(StoreOptions{RootDir: root, Now: fixedClock(pruneNow)}) +} + +// createFinishedSession leaves a session the way a run that has ended leaves +// it: created and written at `at` by a Store that no longer holds it open. +func createFinishedSession(t *testing.T, root, id, at, parent string) { + t.Helper() + store := NewStore(StoreOptions{RootDir: root, Now: fixedClock(at)}) + var err error + if parent == "" { + _, err = store.Create(CreateInput{SessionID: id, Title: "title " + id}) + } else { + _, err = store.Fork(parent, ForkInput{SessionID: id, Title: "title " + id}) + } + if err != nil { + t.Fatalf("create %s: %v", id, err) + } + if _, err := store.AppendEvent(id, AppendEventInput{Type: EventMessage, Payload: map[string]string{"content": "hello from " + id}}); err != nil { + t.Fatalf("append to %s: %v", id, err) + } + store.Release(id) + if parent != "" { + store.Release(parent) + } +} + +func sessionDirExists(t *testing.T, root, id string) bool { + t.Helper() + _, err := os.Stat(filepath.Join(root, id)) + if err == nil { + return true + } + if errors.Is(err, os.ErrNotExist) { + return false + } + t.Fatalf("stat %s: %v", id, err) + return false +} + +func pruneIDs(entries []PruneEntry) []string { + ids := []string{} + for _, entry := range entries { + ids = append(ids, entry.SessionID) + } + return ids +} + +func keptReason(report PruneReport, id string) string { + for _, entry := range report.Kept { + if entry.SessionID == id { + return entry.Reason + } + } + return "" +} + +// rewriteUpdatedAt changes a session's recorded last update behind the store's +// back, the way a writer that takes no lease would. +func rewriteUpdatedAt(t *testing.T, root, id, updatedAt string) { + t.Helper() + path := filepath.Join(root, id, "metadata.json") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s metadata: %v", id, err) + } + var fields map[string]any + if err := json.Unmarshal(data, &fields); err != nil { + t.Fatalf("decode %s metadata: %v", id, err) + } + fields["updatedAt"] = updatedAt + data, err = json.Marshal(fields) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("write %s metadata: %v", id, err) + } +} + +func TestPruneRemovesOnlySessionsOlderThanTheCutoff(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old-a", "2026-07-01T00:00:00Z", "") + createFinishedSession(t, root, "old-b", "2026-07-15T00:00:00Z", "") + createFinishedSession(t, root, "recent", "2026-09-25T12:00:00Z", "") + + // A checkpoint blob, which has to go with its session. + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "a.txt"), []byte("before"), 0o644); err != nil { + t.Fatal(err) + } + old := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-07-01T00:00:00Z")}) + if _, err := old.CaptureToolCheckpoint("old-a", workspace, "edit_file", []string{"a.txt"}); err != nil { + t.Fatalf("capture a checkpoint: %v", err) + } + old.Release("old-a") + if blobs, err := os.ReadDir(filepath.Join(root, "old-a", CheckpointsDir, "blobs")); err != nil || len(blobs) == 0 { + t.Fatalf("SETUP INVALID: no checkpoint blob to remove (%v)", err) + } + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if got := strings.Join(pruneIDs(report.Removed), ","); got != "old-a,old-b" { + t.Fatalf("removed %q, want old-a,old-b", got) + } + if len(report.Kept) != 0 || len(report.Failed) != 0 { + t.Fatalf("kept %v and failed %v, want neither", report.Kept, report.Failed) + } + for _, entry := range report.Removed { + if entry.Bytes <= 0 { + t.Errorf("%s was reported at %d bytes", entry.SessionID, entry.Bytes) + } + if sessionDirExists(t, root, entry.SessionID) { + t.Errorf("%s was reported removed and its directory is still there", entry.SessionID) + } + } + if got, err := pruneStore(root).Get("recent"); err != nil || got == nil { + t.Fatalf("the recent session is gone or unreadable: %v %v", got, err) + } +} + +func TestPruneRefusesACutoffShorterThanADay(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old", "2026-07-01T00:00:00Z", "") + _, err := pruneStore(root).Prune(PruneOptions{OlderThan: 23 * time.Hour}) + if err == nil || !strings.Contains(err.Error(), "minimum") { + t.Fatalf("a 23h cutoff was not refused for being under the minimum: %v", err) + } + if !sessionDirExists(t, root, "old") { + t.Fatal("a refused prune removed a session") + } +} + +func TestPruneDryRunDecidesTheSameAndRemovesNothing(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old-a", "2026-07-01T00:00:00Z", "") + createFinishedSession(t, root, "old-b", "2026-07-15T00:00:00Z", "") + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: true}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if !report.DryRun || strings.Join(pruneIDs(report.Removed), ",") != "old-a,old-b" { + t.Fatalf("dry run report = %+v, want both sessions listed as would-be removals", report) + } + for _, id := range []string{"old-a", "old-b"} { + if !sessionDirExists(t, root, id) { + t.Errorf("a dry run removed %s", id) + } + } +} + +// A SESSION ANOTHER PROCESS HAS OPEN STAYS, HOWEVER OLD. The other Store stands +// in for the other process: lease locks conflict between handles, not only +// between processes. Each way of having a session open is covered: holding it +// outright, writing to it, and loading it to resume it. +func TestPruneLeavesASessionAnotherProcessHasOpen(t *testing.T) { + for _, open := range []struct { + name string + do func(t *testing.T, other *Store, id string) + }{ + {"held", func(t *testing.T, other *Store, id string) { other.Hold(id) }}, + {"written", func(t *testing.T, other *Store, id string) { + if _, err := other.AppendEvent(id, AppendEventInput{Type: EventMessage, Payload: map[string]string{"content": "still here"}}); err != nil { + t.Fatalf("append: %v", err) + } + }}, + {"resumed", func(t *testing.T, other *Store, id string) { + if _, err := other.ReadRehydratedEvents(id); err != nil { + t.Fatalf("resume: %v", err) + } + }}, + } { + t.Run(open.name, func(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old", "2026-07-01T00:00:00Z", "") + // The other process's clock is as old as the session, so a write + // leaves it just as old and only the lease can keep it. + other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-07-01T00:00:00Z")}) + open.do(t, other, "old") + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if reason := keptReason(report, "old"); reason != PruneKeptOpen { + t.Fatalf("kept reason %q, want %q (report %+v)", reason, PruneKeptOpen, report) + } + if !sessionDirExists(t, root, "old") { + t.Fatal("a session another process had open was removed") + } + + // The lease was the only thing keeping it. + other.Release("old") + report, err = pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune after release: %v", err) + } + if strings.Join(pruneIDs(report.Removed), ",") != "old" { + t.Fatalf("after the other process let go, removed %v", pruneIDs(report.Removed)) + } + }) + } +} + +// Lineage and Tree fail on a missing ancestor, so an ancestor of anything kept +// stays, however old. +func TestPruneKeepsEveryAncestorOfAKeptSession(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "grandparent", "2026-06-01T00:00:00Z", "") + createFinishedSession(t, root, "parent", "2026-06-15T00:00:00Z", "grandparent") + createFinishedSession(t, root, "child", "2026-09-25T00:00:00Z", "parent") + createFinishedSession(t, root, "unrelated", "2026-06-01T00:00:00Z", "") + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if got := strings.Join(pruneIDs(report.Removed), ","); got != "unrelated" { + t.Fatalf("removed %q, want only the unrelated session", got) + } + for _, id := range []string{"parent", "grandparent"} { + if reason := keptReason(report, id); reason != PruneKeptParent { + t.Errorf("%s kept for %q, want %q", id, reason, PruneKeptParent) + } + } + if lineage, err := pruneStore(root).Lineage("child"); err != nil || len(lineage) != 3 { + t.Fatalf("the kept child's lineage is broken: %d entries, %v", len(lineage), err) + } +} + +// Descendants go first, and one that turns out to have been written since the +// plan keeps its ancestors, which are old enough themselves. +func TestPruneKeepsTheAncestorsOfASessionWrittenWhilePruning(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") + createFinishedSession(t, root, "child", "2026-06-15T00:00:00Z", "parent") + + var seen []string + pruneRemoveSeam = func(id string) { + seen = append(seen, id) + if id == "child" { + rewriteUpdatedAt(t, root, "child", "2026-09-25T23:00:00Z") + } + } + defer func() { pruneRemoveSeam = nil }() + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if strings.Join(seen, ",") != "child" { + t.Fatalf("removal reached %v, want the child first and the parent never", seen) + } + if reason := keptReason(report, "child"); reason != PruneKeptUpdated { + t.Errorf("child kept for %q, want %q", reason, PruneKeptUpdated) + } + if reason := keptReason(report, "parent"); reason != PruneKeptParent { + t.Errorf("parent kept for %q, want %q", reason, PruneKeptParent) + } + if len(report.Removed) != 0 || !sessionDirExists(t, root, "parent") || !sessionDirExists(t, root, "child") { + t.Fatalf("something was removed: %+v", report) + } +} + +func TestPruneKeepsASessionWhoseLastUpdateCannotBeRead(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "undated", "2026-06-01T00:00:00Z", "") + rewriteUpdatedAt(t, root, "undated", "sometime last spring") + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if reason := keptReason(report, "undated"); reason != PruneKeptUndated { + t.Fatalf("kept for %q, want %q", reason, PruneKeptUndated) + } + if !sessionDirExists(t, root, "undated") { + t.Fatal("a session with an unreadable update time was removed") + } +} diff --git a/internal/sessions/replay.go b/internal/sessions/replay.go index d849e3f36..19253e80b 100644 --- a/internal/sessions/replay.go +++ b/internal/sessions/replay.go @@ -233,7 +233,11 @@ func (store *Store) ReadRehydratedEvents(sessionID string) ([]Event, error) { // ReadRehydratedEventsWithPresence carries the underlying event-log presence // through compaction projection without changing ReadRehydratedEvents' existing // empty-on-missing contract. +// +// It is how a session is loaded to be continued (the TUI's resume, `exec +// --resume`, ACP session/load), so it holds the session open. See Hold. func (store *Store) ReadRehydratedEventsWithPresence(sessionID string) ([]Event, bool, error) { + store.Hold(sessionID) events, present, err := store.ReadEventsWithPresence(sessionID) if err != nil { return nil, present, err diff --git a/internal/sessions/store.go b/internal/sessions/store.go index a811779b3..332e306d9 100644 --- a/internal/sessions/store.go +++ b/internal/sessions/store.go @@ -233,6 +233,10 @@ type Store struct { // is accepted deliberately rather than risk an unsafe eviction. sessionLocks map[string]*sync.Mutex idCounter atomic.Uint64 + + // leases are the sessions this Store holds open. See Hold. + leasesMu sync.Mutex + leases map[string]*os.File } var sessionIDPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_-]{0,127}$`) @@ -332,6 +336,7 @@ func (store *Store) Create(input CreateInput) (Metadata, error) { if err := file.Close(); err != nil { return Metadata{}, fmt.Errorf("close zero session events file: %w", err) } + store.Hold(sessionID) return session, nil } @@ -897,6 +902,14 @@ func (store *Store) sessionLock(sessionID string) *sync.Mutex { // reverse order. The OS lock is best-effort: if it cannot be acquired (e.g. an // unsupported platform) the in-memory mutex still applies. func (store *Store) lockSession(sessionID string) (func(), error) { + // A process that writes a session has it open. See Hold. + store.Hold(sessionID) + return store.lockSessionWithoutLease(sessionID) +} + +// lockSessionWithoutLease is lockSession for prune, which must not mark as open +// the session it is about to remove. +func (store *Store) lockSessionWithoutLease(sessionID string) (func(), error) { mu := store.sessionLock(sessionID) mu.Lock() release, err := store.acquireFileLock(sessionID) From ac1efe3747fd29c16bce970023fa6d0746ab11bf Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:28:31 +0530 Subject: [PATCH 02/14] feat(cli): add zero sessions prune, with a retention setting read from user config only `zero sessions prune --older-than 30d` removes sessions last updated before the cutoff, `--dry-run` lists what it would remove and why it keeps the rest, and `--json` prints the report. The cutoff is days or a Go duration and never under a day. Without --older-than, prune uses sessions.retentionDays; with neither it refuses and says how to set one, so nothing is removed unless asked for. sessions.retentionDays is read straight from the user config file. Sessions belong to the user rather than to a repository, so a project's .zero/config.json cannot choose what prune deletes, and no resolver merge copies the setting from anywhere. Closes #971. --- README.md | 2 +- internal/cli/sessions.go | 36 ++++- internal/cli/sessions_prune.go | 167 +++++++++++++++++++++ internal/cli/sessions_prune_test.go | 184 ++++++++++++++++++++++++ internal/config/sessions_config.go | 20 +++ internal/config/sessions_config_test.go | 76 ++++++++++ internal/config/types.go | 23 +++ 7 files changed, 506 insertions(+), 2 deletions(-) create mode 100644 internal/cli/sessions_prune.go create mode 100644 internal/cli/sessions_prune_test.go create mode 100644 internal/config/sessions_config.go create mode 100644 internal/config/sessions_config_test.go diff --git a/README.md b/README.md index d8a0d84a2..b201dfea4 100644 --- a/README.md +++ b/README.md @@ -313,7 +313,7 @@ zero context context-budget report zero repo-map deterministic repository map zero repo-info local repository summary zero search | find search local session history -zero sessions inspect, resume, fork, and rewind sessions +zero sessions inspect, resume, fork, rewind, and prune sessions zero spec manage spec-mode drafts zero specialist manage specialist subagents zero skills manage markdown instruction skills diff --git a/internal/cli/sessions.go b/internal/cli/sessions.go index b2f76f596..26dc41e90 100644 --- a/internal/cli/sessions.go +++ b/internal/cli/sessions.go @@ -18,6 +18,8 @@ type sessionCommandOptions struct { excludeTarget bool preserveLast int maxPromptChars int + olderThan string + dryRun bool } func runSessions(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { @@ -72,6 +74,11 @@ func runSessions(args []string, stdout io.Writer, stderr io.Writer, deps appDeps return writeExecUsageError(stderr, "sessions compact-plan requires a session id") } return runSessionsCompactPlan(store, remaining[0], options, stdout, stderr) + case "prune": + if len(remaining) != 0 { + return writeExecUsageError(stderr, "sessions prune does not accept positional arguments") + } + return runSessionsPrune(store, options, stdout, stderr, deps) default: return writeExecUsageError(stderr, fmt.Sprintf("unknown sessions command %q", command)) } @@ -91,6 +98,8 @@ func parseSessionsArgs(args []string) (string, []string, sessionCommandOptions, options.json = true case "--exclude-target": options.excludeTarget = true + case "--dry-run": + options.dryRun = true case "--kind": value, next, err := nextFlagValue(args, index, arg) if err != nil { @@ -168,6 +177,21 @@ func parseSessionsArgs(args []string) (string, []string, sessionCommandOptions, } options.preserveLast = preserveLast continue + case arg == "--older-than": + value, next, err := nextFlagValue(args, index, arg) + if err != nil { + return command, remaining, options, false, err + } + options.olderThan = value + index = next + continue + case strings.HasPrefix(arg, "--older-than="): + value, err := parseNonEmptySessionsFlag("--older-than", strings.TrimPrefix(arg, "--older-than=")) + if err != nil { + return command, remaining, options, false, err + } + options.olderThan = value + continue case arg == "--max-prompt-chars": value, next, err := nextFlagValue(args, index, arg) if err != nil { @@ -225,7 +249,7 @@ func parseSessionKindFlag(value string) (sessions.SessionKind, error) { func isSessionsCommand(command string) bool { switch command { - case "list", "children", "lineage", "tree", "rewind-plan", "rewind", "compact-plan": + case "list", "children", "lineage", "tree", "rewind-plan", "rewind", "compact-plan", "prune": return true default: return false @@ -244,6 +268,9 @@ func validateSessionCommandFlags(command string, options sessionCommandOptions) if hasCompactionFlag && command != "compact-plan" { return execUsageError{"--preserve-last and --max-prompt-chars are only valid for sessions compact-plan"} } + if (options.olderThan != "" || options.dryRun) && command != "prune" { + return execUsageError{"--older-than and --dry-run are only valid for sessions prune"} + } return nil } @@ -532,6 +559,7 @@ Commands: rewind-plan Preview events kept and dropped by a rewind rewind Restore workspace files and truncate the log to a checkpoint compact-plan Preview events compacted and preserved by compaction + prune Remove sessions last updated before a cutoff Flags: --json Print JSON output @@ -541,7 +569,13 @@ Flags: --exclude-target Drop the target event (rewind-plan, rewind) --preserve-last Keep recent events in compact-plan --max-prompt-chars Limit compact-plan summary prompt + --older-than Prune cutoff, as days (30d) or a duration (720h); at least 1d. + Without it, prune uses sessions.retentionDays from your user config. + --dry-run List what prune would remove, and why it keeps the rest -h, --help Show this help + +prune only runs when you run it. It never removes a session another Zero process +has open, or an ancestor of a session it keeps, and --dry-run shows why. `) return err } diff --git a/internal/cli/sessions_prune.go b/internal/cli/sessions_prune.go new file mode 100644 index 000000000..199837fc9 --- /dev/null +++ b/internal/cli/sessions_prune.go @@ -0,0 +1,167 @@ +package cli + +import ( + "fmt" + "io" + "strconv" + "strings" + "time" + + "github.com/Gitlawb/zero/internal/config" + "github.com/Gitlawb/zero/internal/redaction" + "github.com/Gitlawb/zero/internal/sessions" +) + +// maxPruneDays keeps a day count from overflowing time.Duration. +const maxPruneDays = 36500 + +// runSessionsPrune removes sessions last updated before a cutoff, or with +// --dry-run lists what would go and why. It runs only when asked (#971). +func runSessionsPrune(store *sessions.Store, options sessionCommandOptions, stdout io.Writer, stderr io.Writer, deps appDeps) int { + olderThan, err := pruneCutoff(options, deps) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + report, err := store.Prune(sessions.PruneOptions{OlderThan: olderThan, DryRun: options.dryRun}) + if err != nil { + return writeSessionCommandError(stderr, err) + } + if options.json { + if err := writePrettyJSON(stdout, redaction.RedactValue(report, redaction.Options{})); err != nil { + return exitCrash + } + } else if _, err := fmt.Fprint(stdout, formatPruneReport(report)); err != nil { + return exitCrash + } + if len(report.Failed) > 0 { + return exitCrash + } + return exitSuccess +} + +// pruneCutoff is --older-than, or else sessions.retentionDays from the user's +// own config, and never below sessions.MinimumPruneAge. Project config has no +// say in it: see config.SessionsConfig. +func pruneCutoff(options sessionCommandOptions, deps appDeps) (time.Duration, error) { + var age time.Duration + if options.olderThan != "" { + parsed, err := parsePruneAge(options.olderThan) + if err != nil { + return 0, err + } + age = parsed + } else { + days := 0 + if deps.userConfigPath != nil { + path, err := deps.userConfigPath() + if err != nil { + return 0, fmt.Errorf("sessions prune could not find your config: %w", err) + } + settings, err := config.ReadSessionsConfig(path) + if err != nil { + return 0, fmt.Errorf("sessions prune could not read sessions.retentionDays: %w", err) + } + days = settings.RetentionDays + } + if days == 0 { + return 0, execUsageError{"sessions prune needs a cutoff: pass --older-than (for example --older-than 30d) or set sessions.retentionDays in your user config"} + } + if days > maxPruneDays { + return 0, execUsageError{fmt.Sprintf("sessions.retentionDays %d is more than %d", days, maxPruneDays)} + } + age = time.Duration(days) * 24 * time.Hour + } + if age < sessions.MinimumPruneAge { + return 0, execUsageError{fmt.Sprintf("sessions prune does not remove anything updated in the last %d hours; use --older-than 1d or more", int(sessions.MinimumPruneAge.Hours()))} + } + return age, nil +} + +// parsePruneAge accepts a whole number of days ("30d") or a Go duration +// ("720h"). +func parsePruneAge(value string) (time.Duration, error) { + invalid := execUsageError{fmt.Sprintf("invalid --older-than %q: expected days like 30d or a duration like 720h", value)} + trimmed := strings.TrimSpace(value) + if days, ok := strings.CutSuffix(trimmed, "d"); ok { + count, err := strconv.Atoi(days) + if err != nil || count <= 0 || count > maxPruneDays { + return 0, invalid + } + return time.Duration(count) * 24 * time.Hour, nil + } + age, err := time.ParseDuration(trimmed) + if err != nil || age <= 0 { + return 0, invalid + } + return age, nil +} + +func formatPruneReport(report sessions.PruneReport) string { + var out strings.Builder + verb := "Removed" + if report.DryRun { + verb = "Would remove" + } + if len(report.Removed) == 0 { + fmt.Fprintf(&out, "No sessions to remove: nothing last updated before %s can go.\n", report.Cutoff) + } else { + var total int64 + for _, entry := range report.Removed { + total += entry.Bytes + } + fmt.Fprintf(&out, "%s %d %s last updated before %s (%s):\n", verb, len(report.Removed), plural(len(report.Removed), "session", "sessions"), report.Cutoff, formatPruneBytes(total)) + for _, entry := range report.Removed { + fmt.Fprintf(&out, " %s\n", formatPruneEntry(entry, redact(entry.Title))) + } + } + if len(report.Kept) > 0 { + fmt.Fprintf(&out, "Kept %d old enough to remove:\n", len(report.Kept)) + for _, entry := range report.Kept { + fmt.Fprintf(&out, " %s\n", formatPruneEntry(entry, entry.Reason)) + } + } + if len(report.Failed) > 0 { + fmt.Fprintf(&out, "Could not remove %d:\n", len(report.Failed)) + for _, entry := range report.Failed { + fmt.Fprintf(&out, " %s\n", formatPruneEntry(entry, redact(entry.Reason))) + } + } + if report.DryRun { + out.WriteString("Dry run: nothing was removed.\n") + } + return out.String() +} + +func formatPruneEntry(entry sessions.PruneEntry, note string) string { + updated := entry.UpdatedAt + if len(updated) >= len("2006-01-02") { + updated = updated[:len("2006-01-02")] + } + line := redact(entry.SessionID) + " " + updated + if note = strings.TrimSpace(note); note != "" { + line += " " + note + } + return line +} + +func formatPruneBytes(bytes int64) string { + const unit = 1024 + if bytes < unit { + return fmt.Sprintf("%d B", bytes) + } + value, suffix := float64(bytes)/unit, "KB" + for _, next := range []string{"MB", "GB", "TB"} { + if value < unit { + break + } + value, suffix = value/unit, next + } + return fmt.Sprintf("%.1f %s", value, suffix) +} + +func plural(count int, one, many string) string { + if count == 1 { + return one + } + return many +} diff --git a/internal/cli/sessions_prune_test.go b/internal/cli/sessions_prune_test.go new file mode 100644 index 000000000..5986ad12b --- /dev/null +++ b/internal/cli/sessions_prune_test.go @@ -0,0 +1,184 @@ +package cli + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/Gitlawb/zero/internal/sessions" +) + +// pruneCLIFixture is a session root holding one session last written in 2020 +// and one written now, neither held open, plus deps whose user config path is +// the returned file (which does not exist until a test writes it). +func pruneCLIFixture(t *testing.T) (string, string, appDeps) { + t.Helper() + root := t.TempDir() + for _, session := range []struct { + id string + at func() time.Time + }{ + {"old-session", func() time.Time { return time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) }}, + {"recent-session", time.Now}, + } { + store := sessions.NewStore(sessions.StoreOptions{RootDir: root, Now: session.at}) + if _, err := store.Create(sessions.CreateInput{SessionID: session.id, Title: "title of " + session.id}); err != nil { + t.Fatalf("create %s: %v", session.id, err) + } + store.Release(session.id) + } + configPath := filepath.Join(t.TempDir(), "config.json") + deps := appDeps{ + newSessionStore: func() *sessions.Store { return sessions.NewStore(sessions.StoreOptions{RootDir: root}) }, + userConfigPath: func() (string, error) { return configPath, nil }, + } + return root, configPath, deps +} + +func runPruneCLI(t *testing.T, deps appDeps, args ...string) (int, string, string) { + t.Helper() + var stdout, stderr bytes.Buffer + code := runWithDeps(append([]string{"sessions"}, args...), &stdout, &stderr, deps) + return code, stdout.String(), stderr.String() +} + +func pruneSessionExists(t *testing.T, root, id string) bool { + t.Helper() + _, err := os.Stat(filepath.Join(root, id)) + return err == nil +} + +func TestSessionsPruneDryRunListsWithoutRemoving(t *testing.T) { + root, _, deps := pruneCLIFixture(t) + code, stdout, stderr := runPruneCLI(t, deps, "prune", "--older-than", "30d", "--dry-run") + if code != exitSuccess { + t.Fatalf("exit %d, stderr %q", code, stderr) + } + for _, want := range []string{"Would remove 1 session", "old-session", "Dry run: nothing was removed."} { + if !strings.Contains(stdout, want) { + t.Errorf("output does not contain %q:\n%s", want, stdout) + } + } + if strings.Contains(stdout, "recent-session") { + t.Errorf("the recent session was listed:\n%s", stdout) + } + if !pruneSessionExists(t, root, "old-session") { + t.Fatal("a dry run removed the session") + } +} + +func TestSessionsPruneRemovesOnlyOldSessions(t *testing.T) { + root, _, deps := pruneCLIFixture(t) + code, stdout, stderr := runPruneCLI(t, deps, "prune", "--older-than=30d") + if code != exitSuccess { + t.Fatalf("exit %d, stderr %q", code, stderr) + } + if !strings.Contains(stdout, "Removed 1 session") || !strings.Contains(stdout, "old-session") { + t.Fatalf("output does not report the removal:\n%s", stdout) + } + if pruneSessionExists(t, root, "old-session") { + t.Fatal("the old session is still there") + } + if !pruneSessionExists(t, root, "recent-session") { + t.Fatal("the recent session was removed") + } +} + +func TestSessionsPruneReportsJSON(t *testing.T) { + _, _, deps := pruneCLIFixture(t) + code, stdout, stderr := runPruneCLI(t, deps, "prune", "--older-than", "720h", "--json") + if code != exitSuccess { + t.Fatalf("exit %d, stderr %q", code, stderr) + } + var report sessions.PruneReport + if err := json.Unmarshal([]byte(stdout), &report); err != nil { + t.Fatalf("output is not a prune report: %v\n%s", err, stdout) + } + if len(report.Removed) != 1 || report.Removed[0].SessionID != "old-session" || report.DryRun { + t.Fatalf("report = %+v", report) + } +} + +func TestSessionsPruneRefusesABadOrShortCutoff(t *testing.T) { + root, _, deps := pruneCLIFixture(t) + for _, tc := range []struct { + value string + want string + }{ + {"12h", "last 24 hours"}, + {"0d", "invalid --older-than"}, + {"soon", "invalid --older-than"}, + {"-5d", "invalid --older-than"}, + } { + code, _, stderr := runPruneCLI(t, deps, "prune", "--older-than="+tc.value) + if code != exitUsage || !strings.Contains(stderr, tc.want) { + t.Errorf("--older-than %s: exit %d, stderr %q, want a usage error naming %q", tc.value, code, stderr, tc.want) + } + } + if !pruneSessionExists(t, root, "old-session") { + t.Fatal("a refused prune removed a session") + } +} + +func TestSessionsPruneNeedsACutoff(t *testing.T) { + root, _, deps := pruneCLIFixture(t) + code, _, stderr := runPruneCLI(t, deps, "prune") + if code != exitUsage || !strings.Contains(stderr, "needs a cutoff") { + t.Fatalf("exit %d, stderr %q, want a usage error asking for a cutoff", code, stderr) + } + if !pruneSessionExists(t, root, "old-session") { + t.Fatal("prune without a cutoff removed a session") + } +} + +func TestSessionsPruneUsesTheUserRetentionSetting(t *testing.T) { + root, configPath, deps := pruneCLIFixture(t) + if err := os.WriteFile(configPath, []byte(`{"sessions":{"retentionDays":30}}`), 0o600); err != nil { + t.Fatal(err) + } + code, stdout, stderr := runPruneCLI(t, deps, "prune") + if code != exitSuccess { + t.Fatalf("exit %d, stderr %q", code, stderr) + } + if !strings.Contains(stdout, "Removed 1 session") || pruneSessionExists(t, root, "old-session") { + t.Fatalf("the user retention setting was not applied:\n%s", stdout) + } +} + +// A REPOSITORY CANNOT CHOOSE WHAT PRUNE DELETES. Sessions are the user's, not +// the project's, so sessions.retentionDays in a workspace's .zero/config.json +// is not a cutoff, even when prune runs from inside that workspace. +func TestSessionsPruneIgnoresRetentionInProjectConfig(t *testing.T) { + root, _, deps := pruneCLIFixture(t) + workspace := t.TempDir() + if err := os.MkdirAll(filepath.Join(workspace, ".zero"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(workspace, ".zero", "config.json"), []byte(`{"sessions":{"retentionDays":1}}`), 0o600); err != nil { + t.Fatal(err) + } + deps.getwd = func() (string, error) { return workspace, nil } + t.Chdir(workspace) + + code, _, stderr := runPruneCLI(t, deps, "prune") + if code != exitUsage || !strings.Contains(stderr, "needs a cutoff") { + t.Fatalf("exit %d, stderr %q: the project's retention setting was used", code, stderr) + } + if !pruneSessionExists(t, root, "old-session") { + t.Fatal("a project config setting removed a session") + } +} + +func TestSessionsPruneFlagsBelongToPrune(t *testing.T) { + _, _, deps := pruneCLIFixture(t) + for _, args := range [][]string{{"list", "--dry-run"}, {"list", "--older-than", "30d"}} { + code, _, stderr := runPruneCLI(t, deps, args...) + if code != exitUsage || !strings.Contains(stderr, "only valid for sessions prune") { + t.Errorf("%v: exit %d, stderr %q", args, code, stderr) + } + } +} diff --git a/internal/config/sessions_config.go b/internal/config/sessions_config.go new file mode 100644 index 000000000..22c1a57b6 --- /dev/null +++ b/internal/config/sessions_config.go @@ -0,0 +1,20 @@ +package config + +import ( + "errors" + "os" +) + +// ReadSessionsConfig reads the sessions settings from the config file at path, +// which callers pass as the user config. A missing file is an empty config. See +// SessionsConfig for why nothing else supplies these settings. +func ReadSessionsConfig(path string) (SessionsConfig, error) { + cfg, err := loadConfigFile(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return SessionsConfig{}, nil + } + return SessionsConfig{}, err + } + return cfg.Sessions, nil +} diff --git a/internal/config/sessions_config_test.go b/internal/config/sessions_config_test.go new file mode 100644 index 000000000..643a16cd3 --- /dev/null +++ b/internal/config/sessions_config_test.go @@ -0,0 +1,76 @@ +package config + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSessionsConfigRoundTripsAsAKnownKey(t *testing.T) { + var cfg FileConfig + if err := json.Unmarshal([]byte(`{"sessions":{"retentionDays":14}}`), &cfg); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if cfg.Sessions.RetentionDays != 14 { + t.Fatalf("retentionDays = %d, want 14", cfg.Sessions.RetentionDays) + } + if _, stray := cfg.Extra["sessions"]; stray { + t.Fatal("sessions was kept as an unknown key as well as parsed") + } + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if !strings.Contains(string(data), `"sessions":{"retentionDays":14}`) { + t.Fatalf("marshalled config lost the setting: %s", data) + } + empty, err := json.Marshal(FileConfig{}) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(empty), "sessions") { + t.Fatalf("an unset sessions block was written out: %s", empty) + } +} + +func TestSessionsConfigRejectsANegativeRetention(t *testing.T) { + var cfg FileConfig + err := json.Unmarshal([]byte(`{"sessions":{"retentionDays":-1}}`), &cfg) + if err == nil || !strings.Contains(err.Error(), "retentionDays") { + t.Fatalf("a negative retention was accepted: %v", err) + } +} + +// A write that changes something else must not drop the setting. +func TestSessionsConfigSurvivesAnUnrelatedWrite(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte(`{"sessions":{"retentionDays":21}}`), 0o600); err != nil { + t.Fatal(err) + } + if _, err := SetTheme(path, "dark"); err != nil { + t.Fatalf("SetTheme: %v", err) + } + settings, err := ReadSessionsConfig(path) + if err != nil { + t.Fatalf("ReadSessionsConfig: %v", err) + } + if settings.RetentionDays != 21 { + t.Fatalf("after an unrelated write retentionDays = %d, want 21", settings.RetentionDays) + } +} + +func TestReadSessionsConfig(t *testing.T) { + dir := t.TempDir() + if settings, err := ReadSessionsConfig(filepath.Join(dir, "missing.json")); err != nil || settings != (SessionsConfig{}) { + t.Fatalf("a missing config = %+v, %v; want empty and no error", settings, err) + } + broken := filepath.Join(dir, "broken.json") + if err := os.WriteFile(broken, []byte(`{"sessions":`), 0o600); err != nil { + t.Fatal(err) + } + if _, err := ReadSessionsConfig(broken); err == nil { + t.Fatal("a config that does not parse was read as empty") + } +} diff --git a/internal/config/types.go b/internal/config/types.go index 9343c7c94..adade9620 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -177,6 +177,19 @@ const ( STTProviderDeepgram STTProviderKind = "deepgram" ) +// SessionsConfig holds settings for Zero's saved sessions. +// +// READ FROM THE USER'S OWN CONFIG ONLY. It decides what `zero sessions prune` +// deletes, and sessions belong to the user rather than to a repository, so a +// project's .zero/config.json has no say: the resolver does not merge it, and +// the prune command reads it straight from the user config file. +type SessionsConfig struct { + // RetentionDays is the cutoff `zero sessions prune` uses when it is given no + // --older-than. Unset (0) means prune needs an explicit cutoff; nothing is + // ever removed without one being asked for. + RetentionDays int `json:"retentionDays,omitempty"` +} + // STTConfig configures speech-to-text dictation. All fields are optional; empty // values take the documented defaults. Booleans that need a real tri-state // (distinguishing "unset" from "false") use *bool, matching PreferencesConfig.Recaps. @@ -364,6 +377,7 @@ type FileConfig struct { LocalControl LocalControlConfig `json:"localControl,omitempty"` STT STTConfig `json:"stt,omitempty"` CrossSessionInbound string `json:"crossSessionInbound,omitempty"` + Sessions SessionsConfig `json:"sessions,omitempty"` // maxTurnsSet records that some merge source supplied a positive maxTurns — // i.e. the value is configured, not the built-in default. Unexported like // Tools.deferThresholdSet: merge bookkeeping, not a config key. @@ -388,6 +402,7 @@ func (cfg FileConfig) MarshalJSON() ([]byte, error) { LocalControl *LocalControlConfig `json:"localControl,omitempty"` STT *STTConfig `json:"stt,omitempty"` CrossSessionInbound string `json:"crossSessionInbound,omitempty"` + Sessions *SessionsConfig `json:"sessions,omitempty"` } raw := rawConfig{ ActiveProvider: cfg.ActiveProvider, @@ -408,6 +423,9 @@ func (cfg FileConfig) MarshalJSON() ([]byte, error) { if !cfg.STT.Empty() { raw.STT = &cfg.STT } + if cfg.Sessions != (SessionsConfig{}) { + raw.Sessions = &cfg.Sessions + } known, err := json.Marshal(raw) if err != nil || len(cfg.Extra) == 0 { return known, err @@ -546,6 +564,7 @@ func (cfg *FileConfig) UnmarshalJSON(data []byte) error { LocalControl LocalControlConfig `json:"localControl"` STT STTConfig `json:"stt"` CrossSessionInbound string `json:"crossSessionInbound"` + Sessions SessionsConfig `json:"sessions"` MCPServers map[string]MCPServerConfig `json:"mcpServers"` MCPServersSnake map[string]MCPServerConfig `json:"mcp_servers"` } @@ -587,6 +606,10 @@ func (cfg *FileConfig) UnmarshalJSON(data []byte) error { cfg.LocalControl = raw.LocalControl cfg.STT = raw.STT cfg.CrossSessionInbound = raw.CrossSessionInbound + if raw.Sessions.RetentionDays < 0 { + return fmt.Errorf("invalid sessions.retentionDays %d: must be >= 0", raw.Sessions.RetentionDays) + } + cfg.Sessions = raw.Sessions cfg.Extra = extra if cfg.MCP.Servers == nil && (len(raw.MCPServers) > 0 || len(raw.MCPServersSnake) > 0) { cfg.MCP.Servers = map[string]MCPServerConfig{} From 9aabaf6d2600ea087aab87ba0729375c40e59fc0 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:29:07 +0530 Subject: [PATCH 03/14] test(sessions): a dry run reports a session another process has open as kept --- internal/sessions/prune_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index b8d306ee0..6cb808012 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -198,6 +198,15 @@ func TestPruneLeavesASessionAnotherProcessHasOpen(t *testing.T) { other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-07-01T00:00:00Z")}) open.do(t, other, "old") + // A dry run has to say so too, not list it as removable. + preview, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: true}) + if err != nil { + t.Fatalf("dry run: %v", err) + } + if reason := keptReason(preview, "old"); reason != PruneKeptOpen || len(preview.Removed) != 0 { + t.Fatalf("dry run kept reason %q and would remove %v, want it kept as open", reason, pruneIDs(preview.Removed)) + } + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) if err != nil { t.Fatalf("Prune: %v", err) From 316c898cfdb0289c21c3e2842a664c577905ef7e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:30:15 +0530 Subject: [PATCH 04/14] test(sessions): pin the dry run for undated sessions and the lease a new session takes --- internal/sessions/prune_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index 6cb808012..27c3fff7e 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -296,6 +296,14 @@ func TestPruneKeepsASessionWhoseLastUpdateCannotBeRead(t *testing.T) { createFinishedSession(t, root, "undated", "2026-06-01T00:00:00Z", "") rewriteUpdatedAt(t, root, "undated", "sometime last spring") + preview, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: true}) + if err != nil { + t.Fatalf("dry run: %v", err) + } + if reason := keptReason(preview, "undated"); reason != PruneKeptUndated || len(preview.Removed) != 0 { + t.Fatalf("dry run kept reason %q and would remove %v", reason, pruneIDs(preview.Removed)) + } + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) if err != nil { t.Fatalf("Prune: %v", err) @@ -307,3 +315,26 @@ func TestPruneKeepsASessionWhoseLastUpdateCannotBeRead(t *testing.T) { t.Fatal("a session with an unreadable update time was removed") } } + +// A session another process has just created, and not yet written to, is as +// open as one it has been writing for hours: a TUI creates its session before +// the first prompt. +func TestPruneLeavesASessionAnotherProcessJustCreated(t *testing.T) { + root := t.TempDir() + other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-07-01T00:00:00Z")}) + if _, err := other.Create(CreateInput{SessionID: "fresh", Title: "not written yet"}); err != nil { + t.Fatalf("create: %v", err) + } + defer other.Release("fresh") + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if reason := keptReason(report, "fresh"); reason != PruneKeptOpen { + t.Fatalf("kept reason %q, want %q (report %+v)", reason, PruneKeptOpen, report) + } + if !sessionDirExists(t, root, "fresh") { + t.Fatal("a session another process had just created was removed") + } +} From 4f1dbd97b8a2acf79ab9c6e0c071734046dbe081 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:36:22 +0530 Subject: [PATCH 05/14] test(sessions): report every failure when a session is written while pruning --- internal/sessions/prune_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index 27c3fff7e..db1789ab7 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -278,7 +278,7 @@ func TestPruneKeepsTheAncestorsOfASessionWrittenWhilePruning(t *testing.T) { t.Fatalf("Prune: %v", err) } if strings.Join(seen, ",") != "child" { - t.Fatalf("removal reached %v, want the child first and the parent never", seen) + t.Errorf("removal reached %v, want the child first and the parent never", seen) } if reason := keptReason(report, "child"); reason != PruneKeptUpdated { t.Errorf("child kept for %q, want %q", reason, PruneKeptUpdated) From 5fe047a56418ba2f6e46b41eec6342ff25563602 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:47:50 +0530 Subject: [PATCH 06/14] docs(readme): list sessions prune in the Chinese README as well --- README_ZH.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README_ZH.md b/README_ZH.md index 9ff7f7b84..2da2a9e2a 100644 --- a/README_ZH.md +++ b/README_ZH.md @@ -218,7 +218,7 @@ zero context 上下文预算报告 zero repo-map 确定性仓库映射 zero repo-info 本地仓库摘要 zero search | find 搜索本地会话历史 -zero sessions 检查、恢复、分叉和回滚会话 +zero sessions 检查、恢复、分叉、回滚和清理会话 zero spec 管理规范模式草稿 zero specialist 管理专业子智能体 zero skills 管理 Markdown 指令技能 From c0f4742841787ea5aa5e5a56c1457f2640e7c079 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 11:51:36 +0530 Subject: [PATCH 07/14] fix(sessions): a fork or child holds its parent, and is refused while prune holds it A lease is taken best effort, so a fork whose parent prune held at that moment went ahead: the rehydrated read in exec --fork could not hold the parent, Fork read it anyway, and the fork was created under a parent prune was removing. Fork and CreateChild now take the parent's lease themselves before reading it, and refuse when prune holds it, so the parent of a session being created is never removed under it. --- internal/sessions/lease.go | 33 ++++++++++++++++++---- internal/sessions/lineage.go | 3 ++ internal/sessions/prune_test.go | 50 +++++++++++++++++++++++++++++++++ internal/sessions/store.go | 3 ++ 4 files changed, 83 insertions(+), 6 deletions(-) diff --git a/internal/sessions/lease.go b/internal/sessions/lease.go index 9779b2862..dc86c7d44 100644 --- a/internal/sessions/lease.go +++ b/internal/sessions/lease.go @@ -1,6 +1,7 @@ package sessions import ( + "fmt" "os" "path/filepath" ) @@ -22,35 +23,55 @@ func (store *Store) leasePath(sessionID string) string { // process; prune takes the same lock exclusively and leaves alone any session it // cannot get. Every write holds its session this way (lockSession), and so does // every rehydrated read, which is how the TUI, `exec --resume` and ACP load a -// session in order to continue it. +// session in order to continue it. Fork and CreateChild hold the parent they +// read from (holdParent). // // It never waits and never fails its caller. A lease that cannot be taken, // because the session directory is gone or prune holds it at this moment, only // means prune cannot see this process; the operation that asked carries on and // meets a removed session on its own terms. func (store *Store) Hold(sessionID string) { + store.hold(sessionID) +} + +// holdParent holds the session a new one is about to be created under, before +// it is read. A parent that prune is checking or removing at this moment is +// refused rather than read: a session created under it would outlive it, and +// Lineage and Tree fail on a missing ancestor. +func (store *Store) holdParent(parentSessionID string) error { + if store.hold(parentSessionID) { + return fmt.Errorf("zero session %s is locked by zero sessions prune; try again", parentSessionID) + } + return nil +} + +// hold is Hold, reporting busy when the lease could not be taken because prune +// holds it exclusively right now. +func (store *Store) hold(sessionID string) (busy bool) { if !ValidSessionID(sessionID) { - return + return false } store.leasesMu.Lock() defer store.leasesMu.Unlock() if _, held := store.leases[sessionID]; held { - return + return false } // Creates the lease file, never the directory: a session that is gone stays // gone. file, err := openLeaseFile(store.leasePath(sessionID)) if err != nil { - return + return false } - if locked, err := tryLockLease(file, false); err != nil || !locked { + locked, err := tryLockLease(file, false) + if err != nil || !locked { _ = file.Close() - return + return err == nil } if store.leases == nil { store.leases = map[string]*os.File{} } store.leases[sessionID] = file + return false } // Release gives up this Store's lease on sessionID, for a long-lived process diff --git a/internal/sessions/lineage.go b/internal/sessions/lineage.go index 433dbe5dd..36d4dd7ac 100644 --- a/internal/sessions/lineage.go +++ b/internal/sessions/lineage.go @@ -10,6 +10,9 @@ func (store *Store) CreateChild(parentSessionID string, input ChildInput) (Metad if !ValidSessionID(parentSessionID) { return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID) } + if err := store.holdParent(parentSessionID); err != nil { + return Metadata{}, err + } parent, err := store.Get(parentSessionID) if err != nil { return Metadata{}, err diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index db1789ab7..94fb21aae 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -338,3 +338,53 @@ func TestPruneLeavesASessionAnotherProcessJustCreated(t *testing.T) { t.Fatal("a session another process had just created was removed") } } + +// A fork or child session holds the parent it is created from, and is refused +// while prune holds that parent, rather than created under a session that is +// about to go. +func TestForkAndChildHoldTheParentTheyAreCreatedFrom(t *testing.T) { + for _, create := range []struct { + name string + do func(store *Store) error + }{ + {"fork", func(store *Store) error { + _, err := store.Fork("parent", ForkInput{SessionID: "new"}) + return err + }}, + {"child", func(store *Store) error { + _, err := store.CreateChild("parent", ChildInput{SessionID: "new"}) + return err + }}, + } { + t.Run(create.name, func(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") + other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-06-01T00:00:00Z")}) + + lease, locked, err := pruneStore(root).acquireLeaseExclusive("parent") + if err != nil || !locked { + t.Fatalf("take the parent's lease the way prune does: locked=%v, %v", locked, err) + } + err = create.do(other) + unlockLease(lease) + _ = lease.Close() + if err == nil || !strings.Contains(err.Error(), "locked by zero sessions prune") { + t.Errorf("%s from a parent prune holds: err = %v, want it refused", create.name, err) + } + if sessionDirExists(t, root, "new") { + t.Errorf("the refused %s was created anyway", create.name) + } + + if err := create.do(other); err != nil { + t.Fatalf("%s once prune let go: %v", create.name, err) + } + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: true}) + if err != nil { + t.Fatalf("dry run: %v", err) + } + if reason := keptReason(report, "parent"); reason != PruneKeptOpen { + t.Errorf("parent kept for %q, want %q: the %s's process holds it", reason, PruneKeptOpen, create.name) + } + }) + } +} diff --git a/internal/sessions/store.go b/internal/sessions/store.go index 332e306d9..928fff41e 100644 --- a/internal/sessions/store.go +++ b/internal/sessions/store.go @@ -434,6 +434,9 @@ func (store *Store) Fork(parentSessionID string, input ForkInput) (Metadata, err if !ValidSessionID(parentSessionID) { return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID) } + if err := store.holdParent(parentSessionID); err != nil { + return Metadata{}, err + } parent, err := store.Get(parentSessionID) if err != nil { return Metadata{}, err From 4cfbb885edbc4b4a90e3b9b4f8f8777413e4f4e9 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 12:00:48 +0530 Subject: [PATCH 08/14] docs(sessions): say that nothing calls Release yet, and what it is for --- internal/sessions/lease.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/sessions/lease.go b/internal/sessions/lease.go index dc86c7d44..dad5b8677 100644 --- a/internal/sessions/lease.go +++ b/internal/sessions/lease.go @@ -74,9 +74,11 @@ func (store *Store) hold(sessionID string) (busy bool) { return false } -// Release gives up this Store's lease on sessionID, for a long-lived process -// that is done with a session before it exits. A process that exits releases -// every lease with it. +// Release gives up this Store's lease on sessionID. Nothing in Zero calls it +// yet: a process holds every session it has touched until it exits, which +// releases every lease with it, and tests use Release to stand in for that +// exit. A long-lived process that switches sessions, like the TUI on /new, could +// call it for the session it leaves. func (store *Store) Release(sessionID string) { store.leasesMu.Lock() defer store.leasesMu.Unlock() From 2bacb7e6ab2855a8bf6439bdb5a9113937f96182 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 12:48:53 +0530 Subject: [PATCH 09/14] fix(sessions): let go of the lease before removing a pruned session's directory lease.lock is deleted with the rest of the session while prune still holds it. Where a delete only takes effect once the last handle closes (Windows without POSIX delete semantics: older builds, or FAT and exFAT volumes), prune's own handle kept the directory from being empty, so the removal failed and left an empty directory behind. Prune now closes the lease after session.lock is gone and before the directory goes. --- internal/sessions/prune.go | 25 +++++++++++++++++++++---- internal/sessions/prune_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/internal/sessions/prune.go b/internal/sessions/prune.go index 2fca93fe6..8c8b49db9 100644 --- a/internal/sessions/prune.go +++ b/internal/sessions/prune.go @@ -65,6 +65,10 @@ const ( // before it re-reads the metadata. Nil in production. var pruneRemoveSeam func(sessionID string) +// pruneRemoveDirSeam runs just before Prune removes a session's directory, and +// says whether Prune still holds the lease. Nil in production. +var pruneRemoveDirSeam func(sessionID string, leaseHeld bool) + // Prune removes sessions last updated before the cutoff that OlderThan sets. // // Only on request: nothing in Zero calls it by itself (#971). It never removes: @@ -239,10 +243,15 @@ func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bo if !locked { return false, PruneKeptOpen, nil } - defer func() { - unlockLease(lease) - _ = lease.Close() - }() + leaseHeld := true + letLeaseGo := func() { + if leaseHeld { + leaseHeld = false + unlockLease(lease) + _ = lease.Close() + } + } + defer letLeaseGo() release, err := store.lockSessionWithoutLease(sessionID) if err != nil { return false, "", err @@ -298,6 +307,14 @@ func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bo if err := os.Remove(store.lockPath(sessionID)); err != nil && !errors.Is(err, fs.ErrNotExist) { return true, "", fmt.Errorf("remove the session lock: %w", err) } + // Let go of the lease before the directory. lease.lock went with the rest, but + // where a delete only takes effect once the last handle closes (Windows without + // POSIX delete semantics: older builds, or FAT and exFAT volumes), this handle + // would keep the directory from being empty. + letLeaseGo() + if pruneRemoveDirSeam != nil { + pruneRemoveDirSeam(sessionID, leaseHeld) + } if err := os.Remove(dir); err != nil && !errors.Is(err, fs.ErrNotExist) { return true, "", fmt.Errorf("remove the session directory: %w", err) } diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index 94fb21aae..f2cf64475 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -388,3 +388,27 @@ func TestForkAndChildHoldTheParentTheyAreCreatedFrom(t *testing.T) { }) } } + +// Where a delete only takes effect once the last handle closes, the lease Prune +// holds would keep the session directory from being empty, so it lets go first. +func TestPruneLetsGoOfTheLeaseBeforeRemovingTheDirectory(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old", "2026-06-01T00:00:00Z", "") + + var reached []string + pruneRemoveDirSeam = func(id string, leaseHeld bool) { + reached = append(reached, id) + if leaseHeld { + t.Errorf("%s: the directory is removed while prune still holds the lease", id) + } + } + defer func() { pruneRemoveDirSeam = nil }() + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if strings.Join(reached, ",") != "old" || strings.Join(pruneIDs(report.Removed), ",") != "old" || sessionDirExists(t, root, "old") { + t.Fatalf("reached %v and removed %v, want old reached and removed", reached, pruneIDs(report.Removed)) + } +} From ff28d8fa56fa107efc28af6a4990a53d075c3ef3 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 12:48:54 +0530 Subject: [PATCH 10/14] test(cli): an empty --older-than is a usage error, not the retention setting --- internal/cli/sessions_prune_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/cli/sessions_prune_test.go b/internal/cli/sessions_prune_test.go index 5986ad12b..aaf38cd75 100644 --- a/internal/cli/sessions_prune_test.go +++ b/internal/cli/sessions_prune_test.go @@ -182,3 +182,25 @@ func TestSessionsPruneFlagsBelongToPrune(t *testing.T) { } } } + +// An empty cutoff is a usage error, never a fall back to sessions.retentionDays. +func TestSessionsPruneRejectsAnEmptyCutoff(t *testing.T) { + root, configPath, deps := pruneCLIFixture(t) + if err := os.WriteFile(configPath, []byte(`{"sessions":{"retentionDays":1}}`), 0o600); err != nil { + t.Fatal(err) + } + for _, args := range [][]string{ + {"prune", "--older-than", ""}, + {"prune", "--older-than", " "}, + {"prune", "--older-than="}, + {"list", "--older-than", ""}, + } { + code, _, stderr := runPruneCLI(t, deps, args...) + if code != exitUsage || !strings.Contains(stderr, "--older-than requires a value") { + t.Errorf("%q: exit %d, stderr %q, want a usage error", args, code, stderr) + } + } + if !pruneSessionExists(t, root, "old-session") { + t.Fatal("an empty cutoff fell back to the retention setting and removed a session") + } +} From 7fa20170dbdb4b1d50a30ebda9860acb55606e33 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 13:23:29 +0530 Subject: [PATCH 11/14] fix(sessions): refuse to continue a session prune holds, rather than read it without the lease A resume that started while prune held the session could not take the lease, read the session anyway, and went on to use one prune was free to remove: a read changes nothing prune checks again. The rehydrated read behind the TUI resume, exec --resume and --fork, and ACP session/load and session/resume now fails with ErrPruning at that moment, and none of the three falls back to a raw read on it. ACP load stays best effort about a history it cannot read, but not about this one. Writes are unchanged: a write lands under session.lock, where prune checks the session again. Prune takes the lease through the new Store.HoldExclusive, which the TUI and ACP tests use to hold a session the way prune does. --- internal/acp/agent.go | 11 +++++ internal/acp/agent_test.go | 37 +++++++++++++++++ internal/sessions/exec_session.go | 6 +++ internal/sessions/lease.go | 63 +++++++++++++++++++---------- internal/sessions/lineage.go | 2 +- internal/sessions/prune.go | 14 +++---- internal/sessions/prune_test.go | 40 ++++++++++++++++-- internal/sessions/replay.go | 9 ++++- internal/sessions/store.go | 2 +- internal/tui/resume_pruning_test.go | 35 ++++++++++++++++ internal/tui/session.go | 5 +++ 11 files changed, 187 insertions(+), 37 deletions(-) create mode 100644 internal/tui/resume_pruning_test.go diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 7c301044c..e647c74f4 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -301,6 +301,12 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam if operation == persistedSessionResume && historyErr != nil { return nil, RPCError(codeInternalError, "restore session history: "+historyErr.Error()) } + // Load stays best effort about a history it cannot read, but not about one + // prune holds: publishing it would leave the client using a session prune may + // be removing, one this process could not hold open. + if errors.Is(historyErr, sessions.ErrPruning) { + return nil, RPCError(codeInternalError, historyErr.Error()) + } model, models, restrictModels, err := a.resolveModelChoices(ctx, root) if err != nil { return nil, RPCError(codeInternalError, "config: "+err.Error()) @@ -911,6 +917,11 @@ func (a *Agent) loadHistory(sessionID string, requireHistoryLog bool) ([]turnRec events, eventLogPresent, err := a.deps.Store.ReadRehydratedEventsWithPresence(sessionID) var rehydrateWarning error if err != nil { + if errors.Is(err, sessions.ErrPruning) { + // Not a rehydration failure: the raw read would continue the session + // without holding it while zero sessions prune may be removing it. + return nil, nil, nil, err + } rehydrateWarning = err events, eventLogPresent, err = a.deps.Store.ReadEventsWithPresence(sessionID) if err != nil { diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 41457549e..b3aebd829 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -2418,3 +2418,40 @@ func payloadString(payload any, key string) string { value, _ := decoded[key].(string) return value } + +// Load is best effort about a history it cannot read, but a session that zero +// sessions prune holds is refused by load and resume alike, and not published: +// this process could not hold it open while prune may be removing it. +func TestACPLoadAndResumeAreRefusedWhilePruneHoldsTheSession(t *testing.T) { + deps := testDeps(t) + cwd := t.TempDir() + meta, err := deps.Store.Create(sessions.CreateInput{Title: "ACP session", Cwd: cwd}) + if err != nil { + t.Fatalf("create session: %v", err) + } + // Created by an earlier process, which has since exited. + deps.Store.Release(meta.SessionID) + + release, locked, err := sessions.NewStore(sessions.StoreOptions{RootDir: deps.Store.RootDir}).HoldExclusive(meta.SessionID) + if err != nil || !locked { + t.Fatalf("take the lease the way prune does: locked=%v, %v", locked, err) + } + defer release() + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + h := newHarness(t, deps) + defer h.stop() + if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{SessionID: meta.SessionID, Cwd: cwd, McpServers: []McpServer{}}, &LoadSessionResult{}); err == nil || !strings.Contains(err.Error(), "locked by zero sessions prune") { + t.Errorf("session/load while prune holds the session: err = %v, want it refused", err) + } + if err := h.client.Call(ctx, MethodSessionResume, ResumeSessionParams{SessionID: meta.SessionID, Cwd: cwd, McpServers: []McpServer{}}, &ResumeSessionResult{}); err == nil || !strings.Contains(err.Error(), "locked by zero sessions prune") { + t.Errorf("session/resume while prune holds the session: err = %v, want it refused", err) + } + if err := h.client.Call(ctx, MethodSessionPrompt, PromptParams{ + SessionID: meta.SessionID, + Prompt: []ContentBlock{TextBlock("carry on")}, + }, &PromptResult{}); err == nil { + t.Fatal("a session refused while prune held it was still promptable") + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index 28e778ab3..3fd061da4 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -2,6 +2,7 @@ package sessions import ( "encoding/json" + "errors" "fmt" "log" "sort" @@ -155,6 +156,11 @@ func readExecContextEvents(store *Store, sessionID string) ([]Event, error) { if err == nil { return contextEvents, nil } + if errors.Is(err, ErrPruning) { + // Not a rehydration failure: the raw read would continue the session + // without holding it while prune may be removing it. + return nil, err + } rawEvents, rawErr := store.ReadEvents(sessionID) if rawErr != nil { return nil, err diff --git a/internal/sessions/lease.go b/internal/sessions/lease.go index dad5b8677..acd9e3676 100644 --- a/internal/sessions/lease.go +++ b/internal/sessions/lease.go @@ -1,9 +1,11 @@ package sessions import ( + "errors" "fmt" "os" "path/filepath" + "sync" ) // leaseFileName is the file whose lock says a process has the session open. @@ -21,26 +23,33 @@ func (store *Store) leasePath(sessionID string) string { // open, and an idle open session looked exactly like an abandoned one. Hold takes // a shared lock on the session's lease file and keeps it for the life of the // process; prune takes the same lock exclusively and leaves alone any session it -// cannot get. Every write holds its session this way (lockSession), and so does -// every rehydrated read, which is how the TUI, `exec --resume` and ACP load a -// session in order to continue it. Fork and CreateChild hold the parent they -// read from (holdParent). +// cannot get. Every write holds its session this way (lockSession). // -// It never waits and never fails its caller. A lease that cannot be taken, +// Hold never waits and never fails its caller. A lease that cannot be taken, // because the session directory is gone or prune holds it at this moment, only -// means prune cannot see this process; the operation that asked carries on and -// meets a removed session on its own terms. +// means prune cannot see this process, and that is enough for a write: it lands +// under session.lock, where prune checks the session again, so either prune sees +// the write and keeps the session or the write finds the session gone. A read +// changes nothing prune checks, so the reads that continue a session, and the +// creations that read a parent, use holdOrRefuse instead. func (store *Store) Hold(sessionID string) { store.hold(sessionID) } -// holdParent holds the session a new one is about to be created under, before -// it is read. A parent that prune is checking or removing at this moment is -// refused rather than read: a session created under it would outlive it, and +// ErrPruning is returned, wrapped, when a process tries to continue a session, +// or to create a session under it, while zero sessions prune holds it. +var ErrPruning = errors.New("locked by zero sessions prune; try again") + +// holdOrRefuse holds a session that is about to be read in order to continue it +// (the rehydrated read behind the TUI's resume, exec --resume and --fork, and +// ACP's session/load and session/resume) or to create a session under it (Fork, +// CreateChild). When prune holds it at this moment the caller is refused rather +// than left to read it without the lease: prune could then remove a session +// this process goes on to use, or one a new session is created under, whose // Lineage and Tree fail on a missing ancestor. -func (store *Store) holdParent(parentSessionID string) error { - if store.hold(parentSessionID) { - return fmt.Errorf("zero session %s is locked by zero sessions prune; try again", parentSessionID) +func (store *Store) holdOrRefuse(sessionID string) error { + if store.hold(sessionID) { + return fmt.Errorf("zero session %s is %w", sessionID, ErrPruning) } return nil } @@ -91,18 +100,30 @@ func (store *Store) Release(sessionID string) { _ = file.Close() } -// acquireLeaseExclusive takes the lease exclusively for as long as prune is -// removing the session, so no process can open it part way through. It reports -// false, holding nothing, when a lease is already held. -func (store *Store) acquireLeaseExclusive(sessionID string) (*os.File, bool, error) { +// HoldExclusive takes sessionID's lease exclusively without waiting, which is how +// prune keeps every other process out of a session while it checks or removes +// it. It reports false, holding nothing, when any process holds the lease. While +// it is held, Hold takes nothing and holdOrRefuse refuses. release lets it go, +// and does nothing after the first call. +func (store *Store) HoldExclusive(sessionID string) (release func(), locked bool, err error) { + nothing := func() {} + if !ValidSessionID(sessionID) { + return nothing, false, fmt.Errorf("invalid zero session id %q", sessionID) + } file, err := openLeaseFile(store.leasePath(sessionID)) if err != nil { - return nil, false, err + return nothing, false, err } - locked, err := tryLockLease(file, true) + locked, err = tryLockLease(file, true) if err != nil || !locked { _ = file.Close() - return nil, false, err + return nothing, false, err } - return file, true, nil + var once sync.Once + return func() { + once.Do(func() { + unlockLease(file) + _ = file.Close() + }) + }, true, nil } diff --git a/internal/sessions/lineage.go b/internal/sessions/lineage.go index 36d4dd7ac..8b95bbef7 100644 --- a/internal/sessions/lineage.go +++ b/internal/sessions/lineage.go @@ -10,7 +10,7 @@ func (store *Store) CreateChild(parentSessionID string, input ChildInput) (Metad if !ValidSessionID(parentSessionID) { return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID) } - if err := store.holdParent(parentSessionID); err != nil { + if err := store.holdOrRefuse(parentSessionID); err != nil { return Metadata{}, err } parent, err := store.Get(parentSessionID) diff --git a/internal/sessions/prune.go b/internal/sessions/prune.go index 8c8b49db9..88b623089 100644 --- a/internal/sessions/prune.go +++ b/internal/sessions/prune.go @@ -116,7 +116,7 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) { } // Open elsewhere at planning time. Checked again under the lease when the // session is removed; this pass is what lets its ancestors be kept too. - lease, locked, err := store.acquireLeaseExclusive(session.SessionID) + release, locked, err := store.HoldExclusive(session.SessionID) if err != nil { report.Kept = append(report.Kept, store.pruneEntry(session, "its lease could not be checked: "+err.Error())) continue @@ -125,8 +125,7 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) { report.Kept = append(report.Kept, store.pruneEntry(session, PruneKeptOpen)) continue } - unlockLease(lease) - _ = lease.Close() + release() candidates[session.SessionID] = true } @@ -236,7 +235,7 @@ func (store *Store) sessionBytes(sessionID string) int64 { // what), and a keptReason when the session turned out to be open or was written // since the plan. func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bool, keptReason string, err error) { - lease, locked, err := store.acquireLeaseExclusive(sessionID) + releaseLease, locked, err := store.HoldExclusive(sessionID) if err != nil { return false, "", fmt.Errorf("check the session's lease: %w", err) } @@ -245,11 +244,8 @@ func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bo } leaseHeld := true letLeaseGo := func() { - if leaseHeld { - leaseHeld = false - unlockLease(lease) - _ = lease.Close() - } + leaseHeld = false + releaseLease() } defer letLeaseGo() release, err := store.lockSessionWithoutLease(sessionID) diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index f2cf64475..708a8cf0a 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -361,13 +361,12 @@ func TestForkAndChildHoldTheParentTheyAreCreatedFrom(t *testing.T) { createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-06-01T00:00:00Z")}) - lease, locked, err := pruneStore(root).acquireLeaseExclusive("parent") + release, locked, err := pruneStore(root).HoldExclusive("parent") if err != nil || !locked { t.Fatalf("take the parent's lease the way prune does: locked=%v, %v", locked, err) } err = create.do(other) - unlockLease(lease) - _ = lease.Close() + release() if err == nil || !strings.Contains(err.Error(), "locked by zero sessions prune") { t.Errorf("%s from a parent prune holds: err = %v, want it refused", create.name, err) } @@ -412,3 +411,38 @@ func TestPruneLetsGoOfTheLeaseBeforeRemovingTheDirectory(t *testing.T) { t.Fatalf("reached %v and removed %v, want old reached and removed", reached, pruneIDs(report.Removed)) } } + +// Loading a session to continue it is refused while prune holds it, instead of +// read without the lease, and the exec path does not fall back to the raw log on +// that refusal. +func TestResumeIsRefusedWhilePruneHoldsTheSession(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old", "2026-06-01T00:00:00Z", "") + other := NewStore(StoreOptions{RootDir: root}) + + release, locked, err := pruneStore(root).HoldExclusive("old") + if err != nil || !locked { + t.Fatalf("take the lease the way prune does: locked=%v, %v", locked, err) + } + _, _, readErr := other.ReadRehydratedEventsWithPresence("old") + events, execErr := readExecContextEvents(other, "old") + release() + if !errors.Is(readErr, ErrPruning) { + t.Errorf("rehydrated read while prune holds the session: err = %v, want ErrPruning", readErr) + } + if !errors.Is(execErr, ErrPruning) || events != nil { + t.Errorf("exec context read while prune holds the session: %d events, err = %v, want it refused, not read from the raw log", len(events), execErr) + } + + // Once prune lets go, the read succeeds and holds the session. + if _, err := other.ReadRehydratedEvents("old"); err != nil { + t.Fatalf("read once prune let go: %v", err) + } + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: true}) + if err != nil { + t.Fatalf("dry run: %v", err) + } + if reason := keptReason(report, "old"); reason != PruneKeptOpen { + t.Errorf("old kept for %q, want %q", reason, PruneKeptOpen) + } +} diff --git a/internal/sessions/replay.go b/internal/sessions/replay.go index 19253e80b..d17d8dd24 100644 --- a/internal/sessions/replay.go +++ b/internal/sessions/replay.go @@ -235,9 +235,14 @@ func (store *Store) ReadRehydratedEvents(sessionID string) ([]Event, error) { // empty-on-missing contract. // // It is how a session is loaded to be continued (the TUI's resume, `exec -// --resume`, ACP session/load), so it holds the session open. See Hold. +// --resume` and --fork, ACP's session/load and session/resume), so it holds the +// session open, and it fails with ErrPruning while prune holds it. Callers that +// fall back to ReadEvents when rehydration fails must not fall back on that +// error. See holdOrRefuse. func (store *Store) ReadRehydratedEventsWithPresence(sessionID string) ([]Event, bool, error) { - store.Hold(sessionID) + if err := store.holdOrRefuse(sessionID); err != nil { + return nil, false, err + } events, present, err := store.ReadEventsWithPresence(sessionID) if err != nil { return nil, present, err diff --git a/internal/sessions/store.go b/internal/sessions/store.go index 928fff41e..588482b81 100644 --- a/internal/sessions/store.go +++ b/internal/sessions/store.go @@ -434,7 +434,7 @@ func (store *Store) Fork(parentSessionID string, input ForkInput) (Metadata, err if !ValidSessionID(parentSessionID) { return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID) } - if err := store.holdParent(parentSessionID); err != nil { + if err := store.holdOrRefuse(parentSessionID); err != nil { return Metadata{}, err } parent, err := store.Get(parentSessionID) diff --git a/internal/tui/resume_pruning_test.go b/internal/tui/resume_pruning_test.go new file mode 100644 index 000000000..6ffb5481f --- /dev/null +++ b/internal/tui/resume_pruning_test.go @@ -0,0 +1,35 @@ +package tui + +import ( + "errors" + "testing" + + "github.com/Gitlawb/zero/internal/sessions" +) + +// A resume that finds zero sessions prune holding the session is refused, not +// read from the raw log without the lease. +func TestResumeEventsIsRefusedWhilePruneHoldsTheSession(t *testing.T) { + root := t.TempDir() + creator := sessions.NewStore(sessions.StoreOptions{RootDir: root}) + meta, err := creator.Create(sessions.CreateInput{Title: "resume me"}) + if err != nil { + t.Fatalf("create: %v", err) + } + if _, err := creator.AppendEvent(meta.SessionID, sessions.AppendEventInput{Type: sessions.EventMessage, Payload: map[string]string{"content": "hello"}}); err != nil { + t.Fatalf("append: %v", err) + } + creator.Release(meta.SessionID) + + release, locked, err := sessions.NewStore(sessions.StoreOptions{RootDir: root}).HoldExclusive(meta.SessionID) + if err != nil || !locked { + t.Fatalf("take the lease the way prune does: locked=%v, %v", locked, err) + } + defer release() + + m := model{sessionStore: sessions.NewStore(sessions.StoreOptions{RootDir: root})} + events, err := m.resumeEvents(meta.SessionID) + if !errors.Is(err, sessions.ErrPruning) || events != nil { + t.Fatalf("resume while prune holds the session: %d events, err = %v, want it refused", len(events), err) + } +} diff --git a/internal/tui/session.go b/internal/tui/session.go index a64e43c75..ca88e38fb 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -314,6 +314,11 @@ func (m model) resumeEvents(sessionID string) ([]sessions.Event, error) { if err == nil { return events, nil } + if errors.Is(err, sessions.ErrPruning) { + // Not a rehydration failure: the raw read would resume the session without + // holding it while zero sessions prune may be removing it. + return nil, err + } raw, rawErr := m.sessionStore.ReadEvents(sessionID) if rawErr != nil { // Surface the raw-read failure (the actual fallback error), not the earlier From dc3ec65100c42f2b9d77e024f459e5c990dbf27b Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 26 Sep 2026 21:40:05 +0530 Subject: [PATCH 12/14] test(sessions): a lease that cannot be taken keeps the session, and does not stop a resume Prune fails closed when it cannot take a session's lease at all, so a process whose own hold fails the same way can carry on: prune cannot remove the session either. Refusing there instead would stop resume and fork wherever locking does not work, for people who never prune. --- internal/sessions/prune_test.go | 42 +++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index 708a8cf0a..163b5d460 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -446,3 +446,45 @@ func TestResumeIsRefusedWhilePruneHoldsTheSession(t *testing.T) { t.Errorf("old kept for %q, want %q", reason, PruneKeptOpen) } } + +// A lease that cannot be taken at all is not a free one: prune keeps the +// session. That is why a process whose own hold fails the same way carries on +// with the session instead of being refused. Prune cannot remove it either, and +// refusing would stop resume and fork wherever locking does not work, for people +// who never prune. +func TestPruneKeepsASessionWhoseLeaseCannotBeChecked(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old", "2026-06-01T00:00:00Z", "") + lease := filepath.Join(root, "old", leaseFileName) + if err := os.Remove(lease); err != nil && !errors.Is(err, os.ErrNotExist) { + t.Fatalf("remove the lease file: %v", err) + } + // A directory where the lease file goes cannot be opened as one. + if err := os.Mkdir(lease, 0o700); err != nil { + t.Fatalf("SETUP INVALID: %v", err) + } + if _, locked, err := pruneStore(root).HoldExclusive("old"); err == nil || locked { + t.Fatalf("SETUP INVALID: the lease could still be taken: locked=%v, %v", locked, err) + } + + for _, dryRun := range []bool{true, false} { + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: dryRun}) + if err != nil { + t.Fatalf("Prune (dry run %v): %v", dryRun, err) + } + if reason := keptReason(report, "old"); !strings.HasPrefix(reason, "its lease could not be checked") || len(report.Removed) != 0 || len(report.Failed) != 0 { + t.Fatalf("dry run %v: kept for %q, removed %v, failed %v, want it kept because its lease could not be checked", dryRun, reason, pruneIDs(report.Removed), pruneIDs(report.Failed)) + } + } + if !sessionDirExists(t, root, "old") { + t.Fatal("a session whose lease could not be checked was removed") + } + + other := NewStore(StoreOptions{RootDir: root}) + if _, err := other.ReadRehydratedEvents("old"); err != nil { + t.Fatalf("resume a session whose lease cannot be taken: %v", err) + } + if _, err := other.Fork("old", ForkInput{SessionID: "fork"}); err != nil { + t.Fatalf("fork a session whose lease cannot be taken: %v", err) + } +} From 94c90879fdeffd424472ee46f911e260af19b8bc Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 28 Sep 2026 09:16:07 +0530 Subject: [PATCH 13/14] fix(sessions): keep a parent forked after the plan, and refuse sessions removed under a caller Three gaps jatmn found in the prune lease rules. A fork made after prune's plan, by a process that has since exited, was not in the plan, so its old parent was removed and the fork was left under a missing ancestor. Once prune holds a parent exclusively it now looks for sessions the plan did not see that name it as parent, and keeps it if there is one. Store.Create with a ParentSessionID, which exec --calling-session-id and spec implementations use, did not hold the parent at all. It now holds it the way Fork and CreateChild do, and all three refuse a parent whose directory is still there without its metadata: prune removes the metadata first and unlinks lease.lock after it, so a lease taken at that moment lands on a fresh lease file and proves nothing. A resume, exec --resume or --fork, or ACP load that picked a session a moment before prune removed it could take a fresh lease and read the session as empty. HoldToContinue now holds the picked session and requires its metadata, and those callers go through it. The rehydrated read on its own still reads a missing session as empty. --- internal/acp/agent.go | 6 ++ internal/acp/agent_test.go | 28 ++++++++ internal/sessions/exec_session.go | 5 ++ internal/sessions/lease.go | 94 ++++++++++++++++++++++--- internal/sessions/lineage.go | 2 +- internal/sessions/prune.go | 59 ++++++++++++++-- internal/sessions/prune_race_test.go | 101 +++++++++++++++++++++++++++ internal/sessions/prune_test.go | 40 +++++++++-- internal/sessions/store.go | 10 ++- internal/tui/resume_pruning_test.go | 35 ++++++++++ internal/tui/session.go | 5 ++ 11 files changed, 364 insertions(+), 21 deletions(-) create mode 100644 internal/sessions/prune_race_test.go diff --git a/internal/acp/agent.go b/internal/acp/agent.go index e647c74f4..27f16b281 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -914,6 +914,12 @@ func (a *Agent) loadHistory(sessionID string, requireHistoryLog bool) ([]turnRec // enough: rehydration substitutes the compaction event in place of the events // it replaced, so a loop that skips everything but EventMessage would drop the // summary exactly as before. It is projected below. Reported by @jatmn. + // + // The session was picked from its metadata a moment ago. Make sure it is + // still there, and held, before restoring it: see sessions.HoldToContinue. + if err := a.deps.Store.HoldToContinue(sessionID); err != nil { + return nil, nil, nil, err + } events, eventLogPresent, err := a.deps.Store.ReadRehydratedEventsWithPresence(sessionID) var rehydrateWarning error if err != nil { diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index b3aebd829..364fa120d 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -2455,3 +2455,31 @@ func TestACPLoadAndResumeAreRefusedWhilePruneHoldsTheSession(t *testing.T) { t.Fatal("a session refused while prune held it was still promptable") } } + +// Activation reads the session's metadata before it restores the history. A +// session prune removes in between is refused as removed, which activation then +// refuses to publish for load as well as resume, instead of restoring it as an +// empty conversation. +func TestACPLoadHistoryRefusesASessionRemovedAfterItWasPicked(t *testing.T) { + deps := testDeps(t) + meta, err := deps.Store.Create(sessions.CreateInput{Title: "ACP session", Cwd: t.TempDir()}) + if err != nil { + t.Fatalf("create session: %v", err) + } + deps.Store.Release(meta.SessionID) + dir := filepath.Join(deps.Store.RootDir, meta.SessionID) + if err := os.Remove(filepath.Join(dir, sessions.MetadataFile)); err != nil { + t.Fatal(err) + } + + a := &Agent{deps: deps} + if _, _, _, err := a.loadHistory(meta.SessionID, false); !errors.Is(err, sessions.ErrPruning) { + t.Errorf("load history of a session prune is removing: err = %v, want it refused", err) + } + if err := os.RemoveAll(dir); err != nil { + t.Fatal(err) + } + if _, _, _, err := a.loadHistory(meta.SessionID, false); !errors.Is(err, sessions.ErrPruning) { + t.Errorf("load history of a session prune removed: err = %v, want it refused", err) + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index 3fd061da4..41540e9b5 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -152,6 +152,11 @@ func PrepareExec(options PrepareExecOptions) (PreparedExec, error) { } func readExecContextEvents(store *Store, sessionID string) ([]Event, error) { + // The session was picked from its metadata a moment ago. Make sure it is + // still there, and held, before reading it to continue: see HoldToContinue. + if err := store.HoldToContinue(sessionID); err != nil { + return nil, err + } contextEvents, err := store.ReadRehydratedEvents(sessionID) if err == nil { return contextEvents, nil diff --git a/internal/sessions/lease.go b/internal/sessions/lease.go index acd9e3676..469304ee2 100644 --- a/internal/sessions/lease.go +++ b/internal/sessions/lease.go @@ -3,6 +3,7 @@ package sessions import ( "errors" "fmt" + "io/fs" "os" "path/filepath" "sync" @@ -36,24 +37,101 @@ func (store *Store) Hold(sessionID string) { store.hold(sessionID) } -// ErrPruning is returned, wrapped, when a process tries to continue a session, -// or to create a session under it, while zero sessions prune holds it. -var ErrPruning = errors.New("locked by zero sessions prune; try again") +// ErrPruning is matched, through errors.Is, by the error a process gets when it +// tries to continue a session, or to create a session under it, while zero +// sessions prune holds that session or once it has removed it. +var ErrPruning = errors.New("locked by zero sessions prune") + +// pruneRefusal is an error matching ErrPruning whose message says which of the +// two it was. +type pruneRefusal struct{ message string } + +func (refusal pruneRefusal) Error() string { return refusal.message } + +func (refusal pruneRefusal) Is(target error) bool { return target == ErrPruning } + +func pruneBusy(sessionID string) error { + return pruneRefusal{"zero session " + sessionID + " is locked by zero sessions prune; try again"} +} + +func pruneRemoved(sessionID string) error { + return pruneRefusal{"zero session " + sessionID + " was removed while it was being opened"} +} // holdOrRefuse holds a session that is about to be read in order to continue it // (the rehydrated read behind the TUI's resume, exec --resume and --fork, and // ACP's session/load and session/resume) or to create a session under it (Fork, -// CreateChild). When prune holds it at this moment the caller is refused rather -// than left to read it without the lease: prune could then remove a session -// this process goes on to use, or one a new session is created under, whose -// Lineage and Tree fail on a missing ancestor. +// CreateChild, Create with a parent). When prune holds it at this moment the +// caller is refused rather than left to read it without the lease: prune could +// then remove a session this process goes on to use, or one a new session is +// created under, whose Lineage and Tree fail on a missing ancestor. func (store *Store) holdOrRefuse(sessionID string) error { if store.hold(sessionID) { - return fmt.Errorf("zero session %s is %w", sessionID, ErrPruning) + return pruneBusy(sessionID) + } + return nil +} + +// holdParent is holdOrRefuse for the session a new one is created under: Fork, +// CreateChild, and Create with a ParentSessionID. +// +// HOLDING A LEASE FILE DOES NOT PROVE THE SESSION IS STILL THERE. Prune removes +// the metadata first and unlinks lease.lock after it, so a process that takes +// the lease just then creates a fresh lease.lock in a directory prune is +// emptying, and locks it with nothing to contend with. So a parent whose +// directory still exists without its metadata is refused once the lease is +// held. A parent whose directory is gone altogether is left alone, as before: a +// session may name a parent this store never had. +func (store *Store) holdParent(parentSessionID string) error { + if err := store.holdOrRefuse(parentSessionID); err != nil { + return err + } + if store.beingRemoved(parentSessionID) { + store.dropStrayLease(parentSessionID) + return pruneRemoved(parentSessionID) } return nil } +// HoldToContinue holds a session the caller has already picked, having read its +// metadata, and is about to continue: the TUI's resume, exec --resume and +// --fork, and ACP's session/load and session/resume. It refuses, with an error +// matching ErrPruning, a session prune holds and one that is gone by the time it +// is held, whether prune is part way through removing it or has finished. +// Callers must not fall back to reading the session another way on that error. +// ReadRehydratedEvents on its own still reads a session that does not exist as +// an empty one, for callers that picked nothing. +func (store *Store) HoldToContinue(sessionID string) error { + if err := store.holdOrRefuse(sessionID); err != nil { + return err + } + if _, err := os.Stat(store.metadataPath(sessionID)); errors.Is(err, fs.ErrNotExist) { + store.dropStrayLease(sessionID) + return pruneRemoved(sessionID) + } + return nil +} + +// beingRemoved reports a session directory that exists without its metadata: +// one prune is part way through removing. +func (store *Store) beingRemoved(sessionID string) bool { + if _, err := os.Stat(store.metadataPath(sessionID)); !errors.Is(err, fs.ErrNotExist) { + return false + } + info, err := os.Stat(store.sessionPath(sessionID)) + return err == nil && info.IsDir() +} + +// dropStrayLease gives back a lease taken on a session that turned out to be +// gone, and removes the lease file it may have created afresh in a directory +// prune is emptying, so that prune can still remove the directory. +func (store *Store) dropStrayLease(sessionID string) { + store.Release(sessionID) + if store.beingRemoved(sessionID) { + _ = os.Remove(store.leasePath(sessionID)) + } +} + // hold is Hold, reporting busy when the lease could not be taken because prune // holds it exclusively right now. func (store *Store) hold(sessionID string) (busy bool) { diff --git a/internal/sessions/lineage.go b/internal/sessions/lineage.go index 8b95bbef7..36d4dd7ac 100644 --- a/internal/sessions/lineage.go +++ b/internal/sessions/lineage.go @@ -10,7 +10,7 @@ func (store *Store) CreateChild(parentSessionID string, input ChildInput) (Metad if !ValidSessionID(parentSessionID) { return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID) } - if err := store.holdOrRefuse(parentSessionID); err != nil { + if err := store.holdParent(parentSessionID); err != nil { return Metadata{}, err } parent, err := store.Get(parentSessionID) diff --git a/internal/sessions/prune.go b/internal/sessions/prune.go index 88b623089..1acf52e6e 100644 --- a/internal/sessions/prune.go +++ b/internal/sessions/prune.go @@ -69,12 +69,16 @@ var pruneRemoveSeam func(sessionID string) // says whether Prune still holds the lease. Nil in production. var pruneRemoveDirSeam func(sessionID string, leaseHeld bool) +// prunePlannedSeam runs once Prune has made its plan and before it removes +// anything. Nil in production. +var prunePlannedSeam func() + // Prune removes sessions last updated before the cutoff that OlderThan sets. // // Only on request: nothing in Zero calls it by itself (#971). It never removes: // - a session another process has open, which holds its lease (see Hold); -// - a session with a descendant that is kept, because Lineage and Tree fail -// on a missing ancestor; +// - a session with a descendant that is kept, including one created after the +// plan was made, because Lineage and Tree fail on a missing ancestor; // - a session written between the plan and its removal; // - a session whose last update time cannot be read. // @@ -157,6 +161,9 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) { } return order[left].SessionID < order[right].SessionID }) + if prunePlannedSeam != nil { + prunePlannedSeam() + } keep := map[string]bool{} for _, session := range order { @@ -169,7 +176,7 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) { report.Removed = append(report.Removed, entry) continue } - removed, keptReason, err := store.pruneSession(session.SessionID, cutoff) + removed, keptReason, err := store.pruneSession(session.SessionID, cutoff, byID) switch { case err != nil: entry.Reason = err.Error() @@ -230,11 +237,39 @@ func (store *Store) sessionBytes(sessionID string) int64 { return total } +// childCreatedAfterPlan reports whether a session the plan did not see names +// sessionID as its parent. Only sessions missing from planned are read, so the +// cost is one directory listing plus whatever was created since the plan. +func (store *Store) childCreatedAfterPlan(sessionID string, planned map[string]Metadata) (bool, error) { + entries, err := os.ReadDir(store.RootDir) + if err != nil { + return false, err + } + for _, entry := range entries { + id := entry.Name() + if !entry.IsDir() || id == sessionID { + continue + } + if _, seen := planned[id]; seen { + continue + } + child, err := store.readMetadata(id) + if err != nil { + continue // not a session, or one without its metadata + } + if child.ParentSessionID == sessionID { + return true, nil + } + } + return false, nil +} + // pruneSession removes one session that planning chose. It reports removed // once the metadata is gone, even when leftovers could not be deleted (err says -// what), and a keptReason when the session turned out to be open or was written -// since the plan. -func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bool, keptReason string, err error) { +// what), and a keptReason when the session turned out to be open, was written +// since the plan, or has a child the plan did not know about. planned is every +// session the plan saw. +func (store *Store) pruneSession(sessionID string, cutoff time.Time, planned map[string]Metadata) (removed bool, keptReason string, err error) { releaseLease, locked, err := store.HoldExclusive(sessionID) if err != nil { return false, "", fmt.Errorf("check the session's lease: %w", err) @@ -276,6 +311,18 @@ func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bo if !updated.Before(cutoff) { return false, PruneKeptUpdated, nil } + // A session forked or given a child after the plan was made, by a process that + // has exited since, is not in the plan, and nothing else would stop its parent + // going. Every way of creating a session under a parent holds that parent + // first (holdParent), so none can start while this lease is held exclusively, + // and one that finished has its metadata on disk. + child, err := store.childCreatedAfterPlan(sessionID, planned) + if err != nil { + return false, "", fmt.Errorf("look for sessions created under it: %w", err) + } + if child { + return false, PruneKeptParent, nil + } // THE METADATA FIRST. From here the session no longer exists to List or Get. if err := os.Remove(store.metadataPath(sessionID)); err != nil { diff --git a/internal/sessions/prune_race_test.go b/internal/sessions/prune_race_test.go new file mode 100644 index 000000000..058ea31f6 --- /dev/null +++ b/internal/sessions/prune_race_test.go @@ -0,0 +1,101 @@ +package sessions + +import ( + "errors" + "os" + "strings" + "testing" +) + +// A fork made after prune's plan, by a process that has exited since, is not in +// the plan and holds nothing. It still keeps its parent: once prune holds that +// parent exclusively it looks for children the plan did not know about. +func TestPruneKeepsAParentForkedAfterThePlan(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") + + forked := false + prunePlannedSeam = func() { + other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-09-25T00:00:00Z")}) + if _, err := other.Fork("parent", ForkInput{SessionID: "late"}); err != nil { + t.Errorf("fork after the plan: %v", err) + return + } + forked = true + // The forking process exits, and its leases go with it. + other.Release("late") + other.Release("parent") + } + defer func() { prunePlannedSeam = nil }() + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if !forked { + t.Fatal("SETUP INVALID: the fork after the plan never happened") + } + if reason := keptReason(report, "parent"); reason != PruneKeptParent { + t.Errorf("parent kept for %q, want %q (removed %v)", reason, PruneKeptParent, pruneIDs(report.Removed)) + } + if lineage, err := pruneStore(root).Lineage("late"); err != nil || len(lineage) != 2 { + t.Fatalf("the late fork's lineage is broken: %d entries, %v", len(lineage), err) + } +} + +// Prune removes the metadata first and unlinks lease.lock after it. A process +// that picked the session a moment earlier and only now takes its lease creates +// a fresh lease.lock in the directory prune is emptying, and locks it with +// nothing to contend with. Continuing the session, or creating one under it, is +// refused all the same, and prune still removes the directory. +func TestContinuingASessionPruneIsRemovingIsRefused(t *testing.T) { + root := t.TempDir() + createFinishedSession(t, root, "old", "2026-06-01T00:00:00Z", "") + other := NewStore(StoreOptions{RootDir: root}) + + reached := false + pruneRemoveDirSeam = func(id string, _ bool) { + if id != "old" { + return + } + reached = true + if _, err := os.Stat(other.metadataPath("old")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("SETUP INVALID: the metadata is still there at the seam: %v", err) + } + if err := other.HoldToContinue("old"); !errors.Is(err, ErrPruning) || !strings.Contains(err.Error(), "was removed") { + t.Errorf("continue a session prune is removing: err = %v, want it refused as removed", err) + } + if events, err := readExecContextEvents(other, "old"); !errors.Is(err, ErrPruning) || events != nil { + t.Errorf("exec context read of a session prune is removing: %d events, err = %v, want it refused", len(events), err) + } + if _, err := other.Create(CreateInput{SessionID: "child", ParentSessionID: "old"}); !errors.Is(err, ErrPruning) { + t.Errorf("create a session under one prune is removing: err = %v, want it refused", err) + } + } + defer func() { pruneRemoveDirSeam = nil }() + + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays}) + if err != nil { + t.Fatalf("Prune: %v", err) + } + if !reached { + t.Fatal("SETUP INVALID: prune never reached the directory removal") + } + if strings.Join(pruneIDs(report.Removed), ",") != "old" || len(report.Failed) != 0 { + t.Errorf("removed %v, failed %v: the refused continuation must not keep prune from removing the directory", pruneIDs(report.Removed), pruneIDs(report.Failed)) + } + if sessionDirExists(t, root, "old") || sessionDirExists(t, root, "child") { + t.Errorf("left behind: old=%v child=%v", sessionDirExists(t, root, "old"), sessionDirExists(t, root, "child")) + } + + // Once prune has finished, the session the caller picked is simply gone, and + // continuing it is refused the same way. + if err := other.HoldToContinue("old"); !errors.Is(err, ErrPruning) { + t.Errorf("continue a session prune removed: err = %v, want it refused", err) + } + // A caller that picked nothing still reads a missing session as empty. + events, present, err := other.ReadRehydratedEventsWithPresence("never-existed") + if err != nil || present || len(events) != 0 { + t.Errorf("rehydrated read of a session that never existed = %d events, present=%v, err=%v; want empty", len(events), present, err) + } +} diff --git a/internal/sessions/prune_test.go b/internal/sessions/prune_test.go index 163b5d460..ce452e24b 100644 --- a/internal/sessions/prune_test.go +++ b/internal/sessions/prune_test.go @@ -339,9 +339,26 @@ func TestPruneLeavesASessionAnotherProcessJustCreated(t *testing.T) { } } -// A fork or child session holds the parent it is created from, and is refused -// while prune holds that parent, rather than created under a session that is -// about to go. +// sessionDirNames lists the session directories under root. +func sessionDirNames(t *testing.T, root string) []string { + t.Helper() + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + names := []string{} + for _, entry := range entries { + if entry.IsDir() { + names = append(names, entry.Name()) + } + } + return names +} + +// Every way of creating a session under a parent holds that parent, and is +// refused while prune holds it, rather than creating a session under one that is +// about to go: Fork, CreateChild, and the direct Create that exec +// --calling-session-id and spec implementations use. func TestForkAndChildHoldTheParentTheyAreCreatedFrom(t *testing.T) { for _, create := range []struct { name string @@ -355,11 +372,24 @@ func TestForkAndChildHoldTheParentTheyAreCreatedFrom(t *testing.T) { _, err := store.CreateChild("parent", ChildInput{SessionID: "new"}) return err }}, + {"create with a parent", func(store *Store) error { + _, err := store.Create(CreateInput{SessionID: "new", ParentSessionID: "parent"}) + return err + }}, + {"exec calling session", func(store *Store) error { + _, err := PrepareExec(PrepareExecOptions{Store: store, SessionID: "new", CallingSessionID: "parent"}) + return err + }}, + {"spec implementation", func(store *Store) error { + _, _, err := store.EnsureSpecImplementation(EnsureSpecImplementationInput{SpecID: "spec", SpecSourceSessionID: "parent", Prompt: "build it"}) + return err + }}, } { t.Run(create.name, func(t *testing.T) { root := t.TempDir() createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-06-01T00:00:00Z")}) + before := strings.Join(sessionDirNames(t, root), ",") release, locked, err := pruneStore(root).HoldExclusive("parent") if err != nil || !locked { @@ -370,8 +400,8 @@ func TestForkAndChildHoldTheParentTheyAreCreatedFrom(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "locked by zero sessions prune") { t.Errorf("%s from a parent prune holds: err = %v, want it refused", create.name, err) } - if sessionDirExists(t, root, "new") { - t.Errorf("the refused %s was created anyway", create.name) + if after := strings.Join(sessionDirNames(t, root), ","); after != before { + t.Errorf("the refused %s created a session anyway: %s, was %s", create.name, after, before) } if err := create.do(other); err != nil { diff --git a/internal/sessions/store.go b/internal/sessions/store.go index 588482b81..cab1faa5d 100644 --- a/internal/sessions/store.go +++ b/internal/sessions/store.go @@ -283,6 +283,14 @@ func (store *Store) Create(input CreateInput) (Metadata, error) { if input.Depth < 0 { return Metadata{}, fmt.Errorf("invalid zero session depth %d", input.Depth) } + // A session created under a parent holds that parent, the way Fork and + // CreateChild do, and is refused while prune holds or is removing it: exec + // --calling-session-id and spec implementations create their children here. + if parent := strings.TrimSpace(input.ParentSessionID); parent != "" { + if err := store.holdParent(parent); err != nil { + return Metadata{}, err + } + } timestamp := store.timestamp() session := Metadata{ @@ -434,7 +442,7 @@ func (store *Store) Fork(parentSessionID string, input ForkInput) (Metadata, err if !ValidSessionID(parentSessionID) { return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID) } - if err := store.holdOrRefuse(parentSessionID); err != nil { + if err := store.holdParent(parentSessionID); err != nil { return Metadata{}, err } parent, err := store.Get(parentSessionID) diff --git a/internal/tui/resume_pruning_test.go b/internal/tui/resume_pruning_test.go index 6ffb5481f..fcfc60eb2 100644 --- a/internal/tui/resume_pruning_test.go +++ b/internal/tui/resume_pruning_test.go @@ -2,6 +2,8 @@ package tui import ( "errors" + "os" + "path/filepath" "testing" "github.com/Gitlawb/zero/internal/sessions" @@ -33,3 +35,36 @@ func TestResumeEventsIsRefusedWhilePruneHoldsTheSession(t *testing.T) { t.Fatalf("resume while prune holds the session: %d events, err = %v, want it refused", len(events), err) } } + +// A session picked for /resume that prune removes before it is read is refused +// too, not resumed as an empty conversation: part way through the removal, with +// the directory still there and its metadata gone, and once it is gone. +func TestResumeEventsIsRefusedOnceThePickedSessionIsRemoved(t *testing.T) { + root := t.TempDir() + creator := sessions.NewStore(sessions.StoreOptions{RootDir: root}) + meta, err := creator.Create(sessions.CreateInput{Title: "resume me"}) + if err != nil { + t.Fatalf("create: %v", err) + } + if _, err := creator.AppendEvent(meta.SessionID, sessions.AppendEventInput{Type: sessions.EventMessage, Payload: map[string]string{"content": "hello"}}); err != nil { + t.Fatalf("append: %v", err) + } + creator.Release(meta.SessionID) + dir := filepath.Join(root, meta.SessionID) + + if err := os.Remove(filepath.Join(dir, sessions.MetadataFile)); err != nil { + t.Fatal(err) + } + m := model{sessionStore: sessions.NewStore(sessions.StoreOptions{RootDir: root})} + if events, err := m.resumeEvents(meta.SessionID); !errors.Is(err, sessions.ErrPruning) || events != nil { + t.Errorf("resume while prune is removing the session: %d events, err = %v, want it refused", len(events), err) + } + + if err := os.RemoveAll(dir); err != nil { + t.Fatal(err) + } + m = model{sessionStore: sessions.NewStore(sessions.StoreOptions{RootDir: root})} + if events, err := m.resumeEvents(meta.SessionID); !errors.Is(err, sessions.ErrPruning) || events != nil { + t.Errorf("resume after prune removed the session: %d events, err = %v, want it refused", len(events), err) + } +} diff --git a/internal/tui/session.go b/internal/tui/session.go index ca88e38fb..95a7c8319 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -310,6 +310,11 @@ func (m model) resolveResumeSession(args string) (*sessions.Metadata, error) { // the CLI's `zero exec --resume` (readExecContextEvents) and the in-TUI /compact // reload. Falls back to the raw log if rehydration fails. func (m model) resumeEvents(sessionID string) ([]sessions.Event, error) { + // Picked from its metadata a moment ago. Make sure it is still there, and + // held, before resuming it: see sessions.HoldToContinue. + if err := m.sessionStore.HoldToContinue(sessionID); err != nil { + return nil, err + } events, err := m.sessionStore.ReadRehydratedEvents(sessionID) if err == nil { return events, nil From ff04d2fcd185be4f49c42d133aad446c2207ae78 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 28 Sep 2026 23:12:52 +0530 Subject: [PATCH 14/14] fix(sessions): make a dry run decide at removal time like a real prune A dry run appended every planned session to Removed without calling pruneSession, so none of the checks made once a session is held for removal ran: the second lease check, the re-read of the last update, and the scan for a child created after the plan. On the same disk, a dry run could list a session that a real run keeps. pruneSession now takes a dry-run flag, makes every one of those checks under the same locks, and stops before removing anything. The prune command reports a session a dry run could not check as such, not as one it could not remove. --- internal/cli/sessions_prune.go | 6 +- internal/cli/sessions_prune_test.go | 14 ++++ internal/sessions/prune.go | 24 ++++--- internal/sessions/prune_race_test.go | 101 +++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 11 deletions(-) diff --git a/internal/cli/sessions_prune.go b/internal/cli/sessions_prune.go index 199837fc9..4a7a84994 100644 --- a/internal/cli/sessions_prune.go +++ b/internal/cli/sessions_prune.go @@ -121,7 +121,11 @@ func formatPruneReport(report sessions.PruneReport) string { } } if len(report.Failed) > 0 { - fmt.Fprintf(&out, "Could not remove %d:\n", len(report.Failed)) + failed := "Could not remove" + if report.DryRun { + failed = "Could not check" + } + fmt.Fprintf(&out, "%s %d:\n", failed, len(report.Failed)) for _, entry := range report.Failed { fmt.Fprintf(&out, " %s\n", formatPruneEntry(entry, redact(entry.Reason))) } diff --git a/internal/cli/sessions_prune_test.go b/internal/cli/sessions_prune_test.go index aaf38cd75..b23f3f5c5 100644 --- a/internal/cli/sessions_prune_test.go +++ b/internal/cli/sessions_prune_test.go @@ -71,6 +71,20 @@ func TestSessionsPruneDryRunListsWithoutRemoving(t *testing.T) { } } +// A dry run removes nothing, so a session it could not check is not reported +// as one it could not remove. +func TestSessionsPruneDryRunReportsFailuresAsUnchecked(t *testing.T) { + failed := []sessions.PruneEntry{{SessionID: "old-session", UpdatedAt: "2020-01-01T00:00:00Z", Reason: "the session disappeared while pruning"}} + dryRun := formatPruneReport(sessions.PruneReport{Cutoff: "2026-08-27T00:00:00Z", DryRun: true, Failed: failed}) + if !strings.Contains(dryRun, "Could not check 1:") || strings.Contains(dryRun, "Could not remove") { + t.Errorf("dry run report:\n%s", dryRun) + } + run := formatPruneReport(sessions.PruneReport{Cutoff: "2026-08-27T00:00:00Z", Failed: failed}) + if !strings.Contains(run, "Could not remove 1:") { + t.Errorf("report:\n%s", run) + } +} + func TestSessionsPruneRemovesOnlyOldSessions(t *testing.T) { root, _, deps := pruneCLIFixture(t) code, stdout, stderr := runPruneCLI(t, deps, "prune", "--older-than=30d") diff --git a/internal/sessions/prune.go b/internal/sessions/prune.go index 1acf52e6e..2077bf1b6 100644 --- a/internal/sessions/prune.go +++ b/internal/sessions/prune.go @@ -23,7 +23,9 @@ type PruneOptions struct { // OlderThan makes a session a candidate when it was last updated at least // this long ago. It must be at least MinimumPruneAge. OlderThan time.Duration - // DryRun decides everything a real run would and removes nothing. + // DryRun decides everything a real run would and removes nothing. It takes + // the same locks to decide, so a Zero opening a session at the moment it is + // being checked is told to try again, as it would be during a real run. DryRun bool } @@ -48,8 +50,9 @@ type PruneReport struct { Removed []PruneEntry `json:"removed"` // Kept are sessions Prune left for a reason other than being recent. Kept []PruneEntry `json:"kept"` - // Failed are sessions whose removal went wrong; Reason says how. A failure - // after the metadata was removed leaves a directory List no longer shows. + // Failed are sessions whose removal went wrong, or in a dry run could not be + // checked; Reason says how. A failure after the metadata was removed leaves + // a directory List no longer shows. Failed []PruneEntry `json:"failed"` } @@ -172,11 +175,7 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) { continue } entry := store.pruneEntry(session, "") - if options.DryRun { - report.Removed = append(report.Removed, entry) - continue - } - removed, keptReason, err := store.pruneSession(session.SessionID, cutoff, byID) + removed, keptReason, err := store.pruneSession(session.SessionID, cutoff, byID, options.DryRun) switch { case err != nil: entry.Reason = err.Error() @@ -268,8 +267,10 @@ func (store *Store) childCreatedAfterPlan(sessionID string, planned map[string]M // once the metadata is gone, even when leftovers could not be deleted (err says // what), and a keptReason when the session turned out to be open, was written // since the plan, or has a child the plan did not know about. planned is every -// session the plan saw. -func (store *Store) pruneSession(sessionID string, cutoff time.Time, planned map[string]Metadata) (removed bool, keptReason string, err error) { +// session the plan saw. A dry run makes every one of those checks under the +// same locks and stops before removing anything; removed then says a real run +// would have gone on to remove the session. +func (store *Store) pruneSession(sessionID string, cutoff time.Time, planned map[string]Metadata, dryRun bool) (removed bool, keptReason string, err error) { releaseLease, locked, err := store.HoldExclusive(sessionID) if err != nil { return false, "", fmt.Errorf("check the session's lease: %w", err) @@ -323,6 +324,9 @@ func (store *Store) pruneSession(sessionID string, cutoff time.Time, planned map if child { return false, PruneKeptParent, nil } + if dryRun { + return true, "", nil + } // THE METADATA FIRST. From here the session no longer exists to List or Get. if err := os.Remove(store.metadataPath(sessionID)); err != nil { diff --git a/internal/sessions/prune_race_test.go b/internal/sessions/prune_race_test.go index 058ea31f6..0e80d4861 100644 --- a/internal/sessions/prune_race_test.go +++ b/internal/sessions/prune_race_test.go @@ -2,6 +2,7 @@ package sessions import ( "errors" + "fmt" "os" "strings" "testing" @@ -43,6 +44,106 @@ func TestPruneKeepsAParentForkedAfterThePlan(t *testing.T) { } } +// A dry run decides what a real run would, including what only shows once a +// session is held for removal: a fork made after the plan, a write since the +// plan, and a process that opened the session since the plan. Each case runs +// both ways from the same start, and both reports must be the one wanted. +func TestPruneDryRunDecidesLikeARealRunAtRemoval(t *testing.T) { + cases := []struct { + name string + // create lays out the sessions. afterPlan changes them once the plan is + // made, and returns what to undo when Prune has finished. + create func(t *testing.T, root string) + afterPlan func(t *testing.T, root string) (undo func()) + want string + }{ + { + name: "a fork made after the plan", + create: func(t *testing.T, root string) { + createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") + }, + afterPlan: func(t *testing.T, root string) func() { + other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-09-25T00:00:00Z")}) + if _, err := other.Fork("parent", ForkInput{SessionID: "late"}); err != nil { + t.Errorf("fork after the plan: %v", err) + } + other.Release("late") + other.Release("parent") + return func() {} + }, + want: "removed [] kept [parent: " + PruneKeptParent + "] failed []", + }, + { + name: "a write since the plan", + create: func(t *testing.T, root string) { + createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "") + createFinishedSession(t, root, "child", "2026-06-15T00:00:00Z", "parent") + }, + afterPlan: func(t *testing.T, root string) func() { + rewriteUpdatedAt(t, root, "child", "2026-09-25T23:00:00Z") + return func() {} + }, + want: "removed [] kept [child: " + PruneKeptUpdated + ", parent: " + PruneKeptParent + "] failed []", + }, + { + name: "a session opened since the plan", + create: func(t *testing.T, root string) { + createFinishedSession(t, root, "opened", "2026-06-01T00:00:00Z", "") + createFinishedSession(t, root, "idle", "2026-06-02T00:00:00Z", "") + }, + afterPlan: func(t *testing.T, root string) func() { + other := NewStore(StoreOptions{RootDir: root}) + if busy := other.hold("opened"); busy { + t.Error("SETUP INVALID: the session was busy when opened after the plan") + } + return func() { other.Release("opened") } + }, + want: "removed [idle] kept [opened: " + PruneKeptOpen + "] failed []", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { prunePlannedSeam = nil }() + for _, dryRun := range []bool{false, true} { + root := t.TempDir() + tc.create(t, root) + before := sessionDirNames(t, root) + var undo func() + prunePlannedSeam = func() { undo = tc.afterPlan(t, root) } + report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: dryRun}) + prunePlannedSeam = nil + if undo == nil { + t.Fatal("SETUP INVALID: nothing changed after the plan") + } + undo() + if err != nil { + t.Fatalf("dry run %v: Prune: %v", dryRun, err) + } + if got := pruneSummary(report); got != tc.want { + t.Errorf("dry run %v: the report is %s, want %s", dryRun, got, tc.want) + } + if !dryRun { + continue + } + for _, id := range before { + if _, err := os.Stat(pruneStore(root).metadataPath(id)); err != nil { + t.Errorf("the dry run removed the metadata of %s: %v", id, err) + } + } + } + }) + } +} + +// pruneSummary puts what a report decided on one line, in report order. +func pruneSummary(report PruneReport) string { + kept := []string{} + for _, entry := range report.Kept { + kept = append(kept, entry.SessionID+": "+entry.Reason) + } + return fmt.Sprintf("removed %v kept [%s] failed %v", pruneIDs(report.Removed), strings.Join(kept, ", "), pruneIDs(report.Failed)) +} + // Prune removes the metadata first and unlinks lease.lock after it. A process // that picked the session a moment earlier and only now takes its lease creates // a fresh lease.lock in the directory prune is emptying, and locks it with