Skip to content

Commit ff04d2f

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

4 files changed

Lines changed: 134 additions & 11 deletions

File tree

‎internal/cli/sessions_prune.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,11 @@ func formatPruneReport(report sessions.PruneReport) string {
121121
}
122122
}
123123
if len(report.Failed) > 0 {
124-
fmt.Fprintf(&out, "Could not remove %d:\n", len(report.Failed))
124+
failed := "Could not remove"
125+
if report.DryRun {
126+
failed = "Could not check"
127+
}
128+
fmt.Fprintf(&out, "%s %d:\n", failed, len(report.Failed))
125129
for _, entry := range report.Failed {
126130
fmt.Fprintf(&out, " %s\n", formatPruneEntry(entry, redact(entry.Reason)))
127131
}

‎internal/cli/sessions_prune_test.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,20 @@ func TestSessionsPruneDryRunListsWithoutRemoving(t *testing.T) {
7171
}
7272
}
7373

74+
// A dry run removes nothing, so a session it could not check is not reported
75+
// as one it could not remove.
76+
func TestSessionsPruneDryRunReportsFailuresAsUnchecked(t *testing.T) {
77+
failed := []sessions.PruneEntry{{SessionID: "old-session", UpdatedAt: "2020-01-01T00:00:00Z", Reason: "the session disappeared while pruning"}}
78+
dryRun := formatPruneReport(sessions.PruneReport{Cutoff: "2026-08-27T00:00:00Z", DryRun: true, Failed: failed})
79+
if !strings.Contains(dryRun, "Could not check 1:") || strings.Contains(dryRun, "Could not remove") {
80+
t.Errorf("dry run report:\n%s", dryRun)
81+
}
82+
run := formatPruneReport(sessions.PruneReport{Cutoff: "2026-08-27T00:00:00Z", Failed: failed})
83+
if !strings.Contains(run, "Could not remove 1:") {
84+
t.Errorf("report:\n%s", run)
85+
}
86+
}
87+
7488
func TestSessionsPruneRemovesOnlyOldSessions(t *testing.T) {
7589
root, _, deps := pruneCLIFixture(t)
7690
code, stdout, stderr := runPruneCLI(t, deps, "prune", "--older-than=30d")

‎internal/sessions/prune.go‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ type PruneOptions struct {
2323
// OlderThan makes a session a candidate when it was last updated at least
2424
// this long ago. It must be at least MinimumPruneAge.
2525
OlderThan time.Duration
26-
// DryRun decides everything a real run would and removes nothing.
26+
// DryRun decides everything a real run would and removes nothing. It takes
27+
// the same locks to decide, so a Zero opening a session at the moment it is
28+
// being checked is told to try again, as it would be during a real run.
2729
DryRun bool
2830
}
2931

@@ -48,8 +50,9 @@ type PruneReport struct {
4850
Removed []PruneEntry `json:"removed"`
4951
// Kept are sessions Prune left for a reason other than being recent.
5052
Kept []PruneEntry `json:"kept"`
51-
// Failed are sessions whose removal went wrong; Reason says how. A failure
52-
// after the metadata was removed leaves a directory List no longer shows.
53+
// Failed are sessions whose removal went wrong, or in a dry run could not be
54+
// checked; Reason says how. A failure after the metadata was removed leaves
55+
// a directory List no longer shows.
5356
Failed []PruneEntry `json:"failed"`
5457
}
5558

@@ -172,11 +175,7 @@ func (store *Store) Prune(options PruneOptions) (PruneReport, error) {
172175
continue
173176
}
174177
entry := store.pruneEntry(session, "")
175-
if options.DryRun {
176-
report.Removed = append(report.Removed, entry)
177-
continue
178-
}
179-
removed, keptReason, err := store.pruneSession(session.SessionID, cutoff, byID)
178+
removed, keptReason, err := store.pruneSession(session.SessionID, cutoff, byID, options.DryRun)
180179
switch {
181180
case err != nil:
182181
entry.Reason = err.Error()
@@ -268,8 +267,10 @@ func (store *Store) childCreatedAfterPlan(sessionID string, planned map[string]M
268267
// once the metadata is gone, even when leftovers could not be deleted (err says
269268
// what), and a keptReason when the session turned out to be open, was written
270269
// 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) {
270+
// session the plan saw. A dry run makes every one of those checks under the
271+
// same locks and stops before removing anything; removed then says a real run
272+
// would have gone on to remove the session.
273+
func (store *Store) pruneSession(sessionID string, cutoff time.Time, planned map[string]Metadata, dryRun bool) (removed bool, keptReason string, err error) {
273274
releaseLease, locked, err := store.HoldExclusive(sessionID)
274275
if err != nil {
275276
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
323324
if child {
324325
return false, PruneKeptParent, nil
325326
}
327+
if dryRun {
328+
return true, "", nil
329+
}
326330

327331
// THE METADATA FIRST. From here the session no longer exists to List or Get.
328332
if err := os.Remove(store.metadataPath(sessionID)); err != nil {

‎internal/sessions/prune_race_test.go‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package sessions
22

33
import (
44
"errors"
5+
"fmt"
56
"os"
67
"strings"
78
"testing"
@@ -43,6 +44,106 @@ func TestPruneKeepsAParentForkedAfterThePlan(t *testing.T) {
4344
}
4445
}
4546

47+
// A dry run decides what a real run would, including what only shows once a
48+
// session is held for removal: a fork made after the plan, a write since the
49+
// plan, and a process that opened the session since the plan. Each case runs
50+
// both ways from the same start, and both reports must be the one wanted.
51+
func TestPruneDryRunDecidesLikeARealRunAtRemoval(t *testing.T) {
52+
cases := []struct {
53+
name string
54+
// create lays out the sessions. afterPlan changes them once the plan is
55+
// made, and returns what to undo when Prune has finished.
56+
create func(t *testing.T, root string)
57+
afterPlan func(t *testing.T, root string) (undo func())
58+
want string
59+
}{
60+
{
61+
name: "a fork made after the plan",
62+
create: func(t *testing.T, root string) {
63+
createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "")
64+
},
65+
afterPlan: func(t *testing.T, root string) func() {
66+
other := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-09-25T00:00:00Z")})
67+
if _, err := other.Fork("parent", ForkInput{SessionID: "late"}); err != nil {
68+
t.Errorf("fork after the plan: %v", err)
69+
}
70+
other.Release("late")
71+
other.Release("parent")
72+
return func() {}
73+
},
74+
want: "removed [] kept [parent: " + PruneKeptParent + "] failed []",
75+
},
76+
{
77+
name: "a write since the plan",
78+
create: func(t *testing.T, root string) {
79+
createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "")
80+
createFinishedSession(t, root, "child", "2026-06-15T00:00:00Z", "parent")
81+
},
82+
afterPlan: func(t *testing.T, root string) func() {
83+
rewriteUpdatedAt(t, root, "child", "2026-09-25T23:00:00Z")
84+
return func() {}
85+
},
86+
want: "removed [] kept [child: " + PruneKeptUpdated + ", parent: " + PruneKeptParent + "] failed []",
87+
},
88+
{
89+
name: "a session opened since the plan",
90+
create: func(t *testing.T, root string) {
91+
createFinishedSession(t, root, "opened", "2026-06-01T00:00:00Z", "")
92+
createFinishedSession(t, root, "idle", "2026-06-02T00:00:00Z", "")
93+
},
94+
afterPlan: func(t *testing.T, root string) func() {
95+
other := NewStore(StoreOptions{RootDir: root})
96+
if busy := other.hold("opened"); busy {
97+
t.Error("SETUP INVALID: the session was busy when opened after the plan")
98+
}
99+
return func() { other.Release("opened") }
100+
},
101+
want: "removed [idle] kept [opened: " + PruneKeptOpen + "] failed []",
102+
},
103+
}
104+
for _, tc := range cases {
105+
t.Run(tc.name, func(t *testing.T) {
106+
defer func() { prunePlannedSeam = nil }()
107+
for _, dryRun := range []bool{false, true} {
108+
root := t.TempDir()
109+
tc.create(t, root)
110+
before := sessionDirNames(t, root)
111+
var undo func()
112+
prunePlannedSeam = func() { undo = tc.afterPlan(t, root) }
113+
report, err := pruneStore(root).Prune(PruneOptions{OlderThan: thirtyDays, DryRun: dryRun})
114+
prunePlannedSeam = nil
115+
if undo == nil {
116+
t.Fatal("SETUP INVALID: nothing changed after the plan")
117+
}
118+
undo()
119+
if err != nil {
120+
t.Fatalf("dry run %v: Prune: %v", dryRun, err)
121+
}
122+
if got := pruneSummary(report); got != tc.want {
123+
t.Errorf("dry run %v: the report is %s, want %s", dryRun, got, tc.want)
124+
}
125+
if !dryRun {
126+
continue
127+
}
128+
for _, id := range before {
129+
if _, err := os.Stat(pruneStore(root).metadataPath(id)); err != nil {
130+
t.Errorf("the dry run removed the metadata of %s: %v", id, err)
131+
}
132+
}
133+
}
134+
})
135+
}
136+
}
137+
138+
// pruneSummary puts what a report decided on one line, in report order.
139+
func pruneSummary(report PruneReport) string {
140+
kept := []string{}
141+
for _, entry := range report.Kept {
142+
kept = append(kept, entry.SessionID+": "+entry.Reason)
143+
}
144+
return fmt.Sprintf("removed %v kept [%s] failed %v", pruneIDs(report.Removed), strings.Join(kept, ", "), pruneIDs(report.Failed))
145+
}
146+
46147
// Prune removes the metadata first and unlinks lease.lock after it. A process
47148
// that picked the session a moment earlier and only now takes its lease creates
48149
// a fresh lease.lock in the directory prune is emptying, and locks it with

0 commit comments

Comments
 (0)