Skip to content

Commit 94c9087

Browse files
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.
1 parent dc3ec65 commit 94c9087

11 files changed

Lines changed: 364 additions & 21 deletions

File tree

‎internal/acp/agent.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -914,6 +914,12 @@ func (a *Agent) loadHistory(sessionID string, requireHistoryLog bool) ([]turnRec
914914
// enough: rehydration substitutes the compaction event in place of the events
915915
// it replaced, so a loop that skips everything but EventMessage would drop the
916916
// summary exactly as before. It is projected below. Reported by @jatmn.
917+
//
918+
// The session was picked from its metadata a moment ago. Make sure it is
919+
// still there, and held, before restoring it: see sessions.HoldToContinue.
920+
if err := a.deps.Store.HoldToContinue(sessionID); err != nil {
921+
return nil, nil, nil, err
922+
}
917923
events, eventLogPresent, err := a.deps.Store.ReadRehydratedEventsWithPresence(sessionID)
918924
var rehydrateWarning error
919925
if err != nil {

‎internal/acp/agent_test.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2455,3 +2455,31 @@ func TestACPLoadAndResumeAreRefusedWhilePruneHoldsTheSession(t *testing.T) {
24552455
t.Fatal("a session refused while prune held it was still promptable")
24562456
}
24572457
}
2458+
2459+
// Activation reads the session's metadata before it restores the history. A
2460+
// session prune removes in between is refused as removed, which activation then
2461+
// refuses to publish for load as well as resume, instead of restoring it as an
2462+
// empty conversation.
2463+
func TestACPLoadHistoryRefusesASessionRemovedAfterItWasPicked(t *testing.T) {
2464+
deps := testDeps(t)
2465+
meta, err := deps.Store.Create(sessions.CreateInput{Title: "ACP session", Cwd: t.TempDir()})
2466+
if err != nil {
2467+
t.Fatalf("create session: %v", err)
2468+
}
2469+
deps.Store.Release(meta.SessionID)
2470+
dir := filepath.Join(deps.Store.RootDir, meta.SessionID)
2471+
if err := os.Remove(filepath.Join(dir, sessions.MetadataFile)); err != nil {
2472+
t.Fatal(err)
2473+
}
2474+
2475+
a := &Agent{deps: deps}
2476+
if _, _, _, err := a.loadHistory(meta.SessionID, false); !errors.Is(err, sessions.ErrPruning) {
2477+
t.Errorf("load history of a session prune is removing: err = %v, want it refused", err)
2478+
}
2479+
if err := os.RemoveAll(dir); err != nil {
2480+
t.Fatal(err)
2481+
}
2482+
if _, _, _, err := a.loadHistory(meta.SessionID, false); !errors.Is(err, sessions.ErrPruning) {
2483+
t.Errorf("load history of a session prune removed: err = %v, want it refused", err)
2484+
}
2485+
}

‎internal/sessions/exec_session.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,11 @@ func PrepareExec(options PrepareExecOptions) (PreparedExec, error) {
152152
}
153153

154154
func readExecContextEvents(store *Store, sessionID string) ([]Event, error) {
155+
// The session was picked from its metadata a moment ago. Make sure it is
156+
// still there, and held, before reading it to continue: see HoldToContinue.
157+
if err := store.HoldToContinue(sessionID); err != nil {
158+
return nil, err
159+
}
155160
contextEvents, err := store.ReadRehydratedEvents(sessionID)
156161
if err == nil {
157162
return contextEvents, nil

‎internal/sessions/lease.go‎

Lines changed: 86 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package sessions
33
import (
44
"errors"
55
"fmt"
6+
"io/fs"
67
"os"
78
"path/filepath"
89
"sync"
@@ -36,24 +37,101 @@ func (store *Store) Hold(sessionID string) {
3637
store.hold(sessionID)
3738
}
3839

39-
// ErrPruning is returned, wrapped, when a process tries to continue a session,
40-
// or to create a session under it, while zero sessions prune holds it.
41-
var ErrPruning = errors.New("locked by zero sessions prune; try again")
40+
// ErrPruning is matched, through errors.Is, by the error a process gets when it
41+
// tries to continue a session, or to create a session under it, while zero
42+
// sessions prune holds that session or once it has removed it.
43+
var ErrPruning = errors.New("locked by zero sessions prune")
44+
45+
// pruneRefusal is an error matching ErrPruning whose message says which of the
46+
// two it was.
47+
type pruneRefusal struct{ message string }
48+
49+
func (refusal pruneRefusal) Error() string { return refusal.message }
50+
51+
func (refusal pruneRefusal) Is(target error) bool { return target == ErrPruning }
52+
53+
func pruneBusy(sessionID string) error {
54+
return pruneRefusal{"zero session " + sessionID + " is locked by zero sessions prune; try again"}
55+
}
56+
57+
func pruneRemoved(sessionID string) error {
58+
return pruneRefusal{"zero session " + sessionID + " was removed while it was being opened"}
59+
}
4260

4361
// holdOrRefuse holds a session that is about to be read in order to continue it
4462
// (the rehydrated read behind the TUI's resume, exec --resume and --fork, and
4563
// ACP's session/load and session/resume) or to create a session under it (Fork,
46-
// CreateChild). When prune holds it at this moment the caller is refused rather
47-
// than left to read it without the lease: prune could then remove a session
48-
// this process goes on to use, or one a new session is created under, whose
49-
// Lineage and Tree fail on a missing ancestor.
64+
// CreateChild, Create with a parent). When prune holds it at this moment the
65+
// caller is refused rather than left to read it without the lease: prune could
66+
// then remove a session this process goes on to use, or one a new session is
67+
// created under, whose Lineage and Tree fail on a missing ancestor.
5068
func (store *Store) holdOrRefuse(sessionID string) error {
5169
if store.hold(sessionID) {
52-
return fmt.Errorf("zero session %s is %w", sessionID, ErrPruning)
70+
return pruneBusy(sessionID)
71+
}
72+
return nil
73+
}
74+
75+
// holdParent is holdOrRefuse for the session a new one is created under: Fork,
76+
// CreateChild, and Create with a ParentSessionID.
77+
//
78+
// HOLDING A LEASE FILE DOES NOT PROVE THE SESSION IS STILL THERE. Prune removes
79+
// the metadata first and unlinks lease.lock after it, so a process that takes
80+
// the lease just then creates a fresh lease.lock in a directory prune is
81+
// emptying, and locks it with nothing to contend with. So a parent whose
82+
// directory still exists without its metadata is refused once the lease is
83+
// held. A parent whose directory is gone altogether is left alone, as before: a
84+
// session may name a parent this store never had.
85+
func (store *Store) holdParent(parentSessionID string) error {
86+
if err := store.holdOrRefuse(parentSessionID); err != nil {
87+
return err
88+
}
89+
if store.beingRemoved(parentSessionID) {
90+
store.dropStrayLease(parentSessionID)
91+
return pruneRemoved(parentSessionID)
5392
}
5493
return nil
5594
}
5695

96+
// HoldToContinue holds a session the caller has already picked, having read its
97+
// metadata, and is about to continue: the TUI's resume, exec --resume and
98+
// --fork, and ACP's session/load and session/resume. It refuses, with an error
99+
// matching ErrPruning, a session prune holds and one that is gone by the time it
100+
// is held, whether prune is part way through removing it or has finished.
101+
// Callers must not fall back to reading the session another way on that error.
102+
// ReadRehydratedEvents on its own still reads a session that does not exist as
103+
// an empty one, for callers that picked nothing.
104+
func (store *Store) HoldToContinue(sessionID string) error {
105+
if err := store.holdOrRefuse(sessionID); err != nil {
106+
return err
107+
}
108+
if _, err := os.Stat(store.metadataPath(sessionID)); errors.Is(err, fs.ErrNotExist) {
109+
store.dropStrayLease(sessionID)
110+
return pruneRemoved(sessionID)
111+
}
112+
return nil
113+
}
114+
115+
// beingRemoved reports a session directory that exists without its metadata:
116+
// one prune is part way through removing.
117+
func (store *Store) beingRemoved(sessionID string) bool {
118+
if _, err := os.Stat(store.metadataPath(sessionID)); !errors.Is(err, fs.ErrNotExist) {
119+
return false
120+
}
121+
info, err := os.Stat(store.sessionPath(sessionID))
122+
return err == nil && info.IsDir()
123+
}
124+
125+
// dropStrayLease gives back a lease taken on a session that turned out to be
126+
// gone, and removes the lease file it may have created afresh in a directory
127+
// prune is emptying, so that prune can still remove the directory.
128+
func (store *Store) dropStrayLease(sessionID string) {
129+
store.Release(sessionID)
130+
if store.beingRemoved(sessionID) {
131+
_ = os.Remove(store.leasePath(sessionID))
132+
}
133+
}
134+
57135
// hold is Hold, reporting busy when the lease could not be taken because prune
58136
// holds it exclusively right now.
59137
func (store *Store) hold(sessionID string) (busy bool) {

‎internal/sessions/lineage.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ func (store *Store) CreateChild(parentSessionID string, input ChildInput) (Metad
1010
if !ValidSessionID(parentSessionID) {
1111
return Metadata{}, fmt.Errorf("invalid zero session id %q", parentSessionID)
1212
}
13-
if err := store.holdOrRefuse(parentSessionID); err != nil {
13+
if err := store.holdParent(parentSessionID); err != nil {
1414
return Metadata{}, err
1515
}
1616
parent, err := store.Get(parentSessionID)

‎internal/sessions/prune.go‎

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,16 @@ var pruneRemoveSeam func(sessionID string)
6969
// says whether Prune still holds the lease. Nil in production.
7070
var pruneRemoveDirSeam func(sessionID string, leaseHeld bool)
7171

72+
// prunePlannedSeam runs once Prune has made its plan and before it removes
73+
// anything. Nil in production.
74+
var prunePlannedSeam func()
75+
7276
// Prune removes sessions last updated before the cutoff that OlderThan sets.
7377
//
7478
// Only on request: nothing in Zero calls it by itself (#971). It never removes:
7579
// - a session another process has open, which holds its lease (see Hold);
76-
// - a session with a descendant that is kept, because Lineage and Tree fail
77-
// on a missing ancestor;
80+
// - a session with a descendant that is kept, including one created after the
81+
// plan was made, because Lineage and Tree fail on a missing ancestor;
7882
// - a session written between the plan and its removal;
7983
// - a session whose last update time cannot be read.
8084
//
@@ -157,6 +161,9 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) {
157161
}
158162
return order[left].SessionID < order[right].SessionID
159163
})
164+
if prunePlannedSeam != nil {
165+
prunePlannedSeam()
166+
}
160167

161168
keep := map[string]bool{}
162169
for _, session := range order {
@@ -169,7 +176,7 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) {
169176
report.Removed = append(report.Removed, entry)
170177
continue
171178
}
172-
removed, keptReason, err := store.pruneSession(session.SessionID, cutoff)
179+
removed, keptReason, err := store.pruneSession(session.SessionID, cutoff, byID)
173180
switch {
174181
case err != nil:
175182
entry.Reason = err.Error()
@@ -230,11 +237,39 @@ func (store *Store) sessionBytes(sessionID string) int64 {
230237
return total
231238
}
232239

240+
// childCreatedAfterPlan reports whether a session the plan did not see names
241+
// sessionID as its parent. Only sessions missing from planned are read, so the
242+
// cost is one directory listing plus whatever was created since the plan.
243+
func (store *Store) childCreatedAfterPlan(sessionID string, planned map[string]Metadata) (bool, error) {
244+
entries, err := os.ReadDir(store.RootDir)
245+
if err != nil {
246+
return false, err
247+
}
248+
for _, entry := range entries {
249+
id := entry.Name()
250+
if !entry.IsDir() || id == sessionID {
251+
continue
252+
}
253+
if _, seen := planned[id]; seen {
254+
continue
255+
}
256+
child, err := store.readMetadata(id)
257+
if err != nil {
258+
continue // not a session, or one without its metadata
259+
}
260+
if child.ParentSessionID == sessionID {
261+
return true, nil
262+
}
263+
}
264+
return false, nil
265+
}
266+
233267
// pruneSession removes one session that planning chose. It reports removed
234268
// once the metadata is gone, even when leftovers could not be deleted (err says
235-
// what), and a keptReason when the session turned out to be open or was written
236-
// since the plan.
237-
func (store *Store) pruneSession(sessionID string, cutoff time.Time) (removed bool, keptReason string, err error) {
269+
// what), and a keptReason when the session turned out to be open, was written
270+
// since the plan, or has a child the plan did not know about. planned is every
271+
// session the plan saw.
272+
func (store *Store) pruneSession(sessionID string, cutoff time.Time, planned map[string]Metadata) (removed bool, keptReason string, err error) {
238273
releaseLease, locked, err := store.HoldExclusive(sessionID)
239274
if err != nil {
240275
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
276311
if !updated.Before(cutoff) {
277312
return false, PruneKeptUpdated, nil
278313
}
314+
// A session forked or given a child after the plan was made, by a process that
315+
// has exited since, is not in the plan, and nothing else would stop its parent
316+
// going. Every way of creating a session under a parent holds that parent
317+
// first (holdParent), so none can start while this lease is held exclusively,
318+
// and one that finished has its metadata on disk.
319+
child, err := store.childCreatedAfterPlan(sessionID, planned)
320+
if err != nil {
321+
return false, "", fmt.Errorf("look for sessions created under it: %w", err)
322+
}
323+
if child {
324+
return false, PruneKeptParent, nil
325+
}
279326

280327
// THE METADATA FIRST. From here the session no longer exists to List or Get.
281328
if err := os.Remove(store.metadataPath(sessionID)); err != nil {
Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
package sessions
2+
3+
import (
4+
"errors"
5+
"os"
6+
"strings"
7+
"testing"
8+
)
9+
10+
// A fork made after prune's plan, by a process that has exited since, is not in
11+
// the plan and holds nothing. It still keeps its parent: once prune holds that
12+
// parent exclusively it looks for children the plan did not know about.
13+
func TestPruneKeepsAParentForkedAfterThePlan(t *testing.T) {
14+
root := t.TempDir()
15+
createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "")
16+
17+
forked := false
18+
prunePlannedSeam = func() {
19+
other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-09-25T00:00:00Z")})
20+
if _, err := other.Fork("parent", ForkInput{SessionID: "late"}); err != nil {
21+
t.Errorf("fork after the plan: %v", err)
22+
return
23+
}
24+
forked = true
25+
// The forking process exits, and its leases go with it.
26+
other.Release("late")
27+
other.Release("parent")
28+
}
29+
defer func() { prunePlannedSeam = nil }()
30+
31+
report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays})
32+
if err != nil {
33+
t.Fatalf("Prune: %v", err)
34+
}
35+
if !forked {
36+
t.Fatal("SETUP INVALID: the fork after the plan never happened")
37+
}
38+
if reason := keptReason(report, "parent"); reason != PruneKeptParent {
39+
t.Errorf("parent kept for %q, want %q (removed %v)", reason, PruneKeptParent, pruneIDs(report.Removed))
40+
}
41+
if lineage, err := pruneStore(root).Lineage("late"); err != nil || len(lineage) != 2 {
42+
t.Fatalf("the late fork's lineage is broken: %d entries, %v", len(lineage), err)
43+
}
44+
}
45+
46+
// Prune removes the metadata first and unlinks lease.lock after it. A process
47+
// that picked the session a moment earlier and only now takes its lease creates
48+
// a fresh lease.lock in the directory prune is emptying, and locks it with
49+
// nothing to contend with. Continuing the session, or creating one under it, is
50+
// refused all the same, and prune still removes the directory.
51+
func TestContinuingASessionPruneIsRemovingIsRefused(t *testing.T) {
52+
root := t.TempDir()
53+
createFinishedSession(t, root, "old", "2026-06-01T00:00:00Z", "")
54+
other := NewStore(StoreOptions{RootDir: root})
55+
56+
reached := false
57+
pruneRemoveDirSeam = func(id string, _ bool) {
58+
if id != "old" {
59+
return
60+
}
61+
reached = true
62+
if _, err := os.Stat(other.metadataPath("old")); !errors.Is(err, os.ErrNotExist) {
63+
t.Fatalf("SETUP INVALID: the metadata is still there at the seam: %v", err)
64+
}
65+
if err := other.HoldToContinue("old"); !errors.Is(err, ErrPruning) || !strings.Contains(err.Error(), "was removed") {
66+
t.Errorf("continue a session prune is removing: err = %v, want it refused as removed", err)
67+
}
68+
if events, err := readExecContextEvents(other, "old"); !errors.Is(err, ErrPruning) || events != nil {
69+
t.Errorf("exec context read of a session prune is removing: %d events, err = %v, want it refused", len(events), err)
70+
}
71+
if _, err := other.Create(CreateInput{SessionID: "child", ParentSessionID: "old"}); !errors.Is(err, ErrPruning) {
72+
t.Errorf("create a session under one prune is removing: err = %v, want it refused", err)
73+
}
74+
}
75+
defer func() { pruneRemoveDirSeam = nil }()
76+
77+
report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays})
78+
if err != nil {
79+
t.Fatalf("Prune: %v", err)
80+
}
81+
if !reached {
82+
t.Fatal("SETUP INVALID: prune never reached the directory removal")
83+
}
84+
if strings.Join(pruneIDs(report.Removed), ",") != "old" || len(report.Failed) != 0 {
85+
t.Errorf("removed %v, failed %v: the refused continuation must not keep prune from removing the directory", pruneIDs(report.Removed), pruneIDs(report.Failed))
86+
}
87+
if sessionDirExists(t, root, "old") || sessionDirExists(t, root, "child") {
88+
t.Errorf("left behind: old=%v child=%v", sessionDirExists(t, root, "old"), sessionDirExists(t, root, "child"))
89+
}
90+
91+
// Once prune has finished, the session the caller picked is simply gone, and
92+
// continuing it is refused the same way.
93+
if err := other.HoldToContinue("old"); !errors.Is(err, ErrPruning) {
94+
t.Errorf("continue a session prune removed: err = %v, want it refused", err)
95+
}
96+
// A caller that picked nothing still reads a missing session as empty.
97+
events, present, err := other.ReadRehydratedEventsWithPresence("never-existed")
98+
if err != nil || present || len(events) != 0 {
99+
t.Errorf("rehydrated read of a session that never existed = %d events, present=%v, err=%v; want empty", len(events), present, err)
100+
}
101+
}

0 commit comments

Comments
 (0)