Compare commits

...
Author SHA1 Message Date
Vasanthdev2004 b595420469 fix(sessions): keep a session lease for the life of the process, not of the Store
A session's lease file lived in the Store that took it, so once that Store
was unreachable the garbage collector closed the file, and closing it drops
the lock. That made TestForkAndChildHoldTheParentTheyAreCreatedFrom fail on
main whenever a collection ran before its dry-run prune. It can also happen
in an exec run: PrepareExec takes the calling or forked session's lease on a
Store the run never touches again, so the parent's lease can disappear part
way through and a prune in another terminal could remove it.

Leases now belong to the process, as Hold documents. Every Store holding a
session shares one lease file in a process-wide table, and the file is
unlocked and closed only when the last Store holding it releases it.
2026-10-15 09:18:09 +05:30
3 changed files with 173 additions and 18 deletions

No files matched your search

+73 -16
View File
@@ -143,21 +143,15 @@ func (store *Store) hold(sessionID string) (busy bool) {
if _, held := store.leases[sessionID]; held {
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 false
}
locked, err := tryLockLease(file, false)
if err != nil || !locked {
_ = file.Close()
return err == nil
path := store.leasePath(sessionID)
held, busy := takeProcessLease(path)
if !held {
return busy
}
if store.leases == nil {
store.leases = map[string]*os.File{}
store.leases = map[string]string{}
}
store.leases[sessionID] = file
store.leases[sessionID] = path
return false
}
@@ -165,17 +159,80 @@ func (store *Store) hold(sessionID string) (busy bool) {
// 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.
// call it for the session it leaves. The lease itself goes once no other Store
// in the process holds it either.
func (store *Store) Release(sessionID string) {
store.leasesMu.Lock()
defer store.leasesMu.Unlock()
file, held := store.leases[sessionID]
path, held := store.leases[sessionID]
if !held {
return
}
delete(store.leases, sessionID)
unlockLease(file)
_ = file.Close()
giveBackProcessLease(path)
}
// processLeases are the lease files this process holds, by path.
//
// A LEASE IS THE PROCESS'S, NOT THE STORE'S. A Store can go out of scope while
// the process goes on using what it holds: PrepareExec opens one for an exec run,
// takes the calling or forked session's lease on it, and the run never touches
// that Store again. Had the file stayed with the Store, the garbage collector
// would close it once the Store was unreachable, which drops the lock, and prune
// could remove a session the run still uses. So every Store holding a session
// shares one file here, and the file is closed only when the last of them
// releases it.
var processLeases = struct {
sync.Mutex
files map[string]*processLease
}{files: map[string]*processLease{}}
type processLease struct {
file *os.File
holders int
}
// takeProcessLease adds a holder to the process's lease on path, taking the lease
// first when nothing in the process holds it. It reports busy when the lease
// could not be taken because prune holds it exclusively right now.
func takeProcessLease(path string) (held bool, busy bool) {
processLeases.Lock()
defer processLeases.Unlock()
if lease := processLeases.files[path]; lease != nil {
lease.holders++
return true, false
}
// Creates the lease file, never the directory: a session that is gone stays
// gone.
file, err := openLeaseFile(path)
if err != nil {
return false, false
}
locked, err := tryLockLease(file, false)
if err != nil || !locked {
_ = file.Close()
return false, err == nil
}
processLeases.files[path] = &processLease{file: file, holders: 1}
return true, false
}
// giveBackProcessLease removes a holder from the process's lease on path, and
// unlocks and closes the file once no holder is left.
func giveBackProcessLease(path string) {
processLeases.Lock()
defer processLeases.Unlock()
lease := processLeases.files[path]
if lease == nil {
return
}
lease.holders--
if lease.holders > 0 {
return
}
delete(processLeases.files, path)
unlockLease(lease.file)
_ = lease.file.Close()
}
// HoldExclusive takes sessionID's lease exclusively without waiting, which is how
+97
View File
@@ -0,0 +1,97 @@
package sessions
import (
"runtime"
"testing"
"time"
)
// collectGarbage runs the collector and gives finalizers time to run, so a file
// left only to a Store that is gone would be closed by the time it returns.
func collectGarbage() {
for i := 0; i < 5; i++ {
runtime.GC()
time.Sleep(20 * time.Millisecond)
}
}
// A lease lasts for the life of the process, not of the Store that took it.
// PrepareExec takes the calling or forked session's lease on a Store the exec
// run never touches again, so a lease that went with the Store would be dropped
// once the garbage collector closed its file, and prune could remove a session
// the run still uses.
func TestLeaseOutlivesTheStoreThatTookIt(t *testing.T) {
for _, take := range []struct {
name string
do func(store *Store) error
}{
{"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
}},
{"hold", func(store *Store) error {
store.Hold("parent")
return nil
}},
} {
t.Run(take.name, func(t *testing.T) {
root := t.TempDir()
createFinishedSession(t, root, "parent", "2026-06-01T00:00:00Z", "")
// The Store is unreachable once this returns, as PrepareExec's is in
// an exec run.
func() {
store := NewStore(StoreOptions{RootDir: root, Now: fixedClock("2026-06-01T00:00:00Z")})
if err := take.do(store); err != nil {
t.Fatalf("%s: %v", take.name, err)
}
}()
collectGarbage()
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.Fatalf("parent kept for %q once the Store that took its lease was collected, want %q", reason, PruneKeptOpen)
}
})
}
}
// Stores in one process share the lease on a session, and it lasts until the
// last of them releases it.
func TestLeaseHeldByTwoStoresLastsUntilBothRelease(t *testing.T) {
root := t.TempDir()
createFinishedSession(t, root, "shared", "2026-06-01T00:00:00Z", "")
first := NewStore(StoreOptions{RootDir: root})
second := NewStore(StoreOptions{RootDir: root})
first.Hold("shared")
second.Hold("shared")
// Releasing twice from one Store gives up only that Store's hold.
first.Release("shared")
first.Release("shared")
if release, locked, err := pruneStore(root).HoldExclusive("shared"); err != nil || locked {
release()
t.Fatalf("prune took a session another Store still holds: locked=%v, %v", locked, err)
}
second.Release("shared")
release, locked, err := pruneStore(root).HoldExclusive("shared")
if err != nil || !locked {
t.Fatalf("prune could not take the session after both Stores released it: locked=%v, %v", locked, err)
}
release()
// Held again after the last release, the lease is taken afresh.
first.Hold("shared")
if release, locked, err := pruneStore(root).HoldExclusive("shared"); err != nil || locked {
release()
t.Fatalf("prune took a session held again after it was released: locked=%v, %v", locked, err)
}
first.Release("shared")
}
+3 -2
View File
@@ -235,9 +235,10 @@ type Store struct {
sessionLocks map[string]*sync.Mutex
idCounter atomic.Uint64
// leases are the sessions this Store holds open. See Hold.
// leases are the sessions this Store holds open, with the path of each
// lease file. The files themselves belong to the process. See Hold.
leasesMu sync.Mutex
leases map[string]*os.File
leases map[string]string
}
var sessionIDPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_-]{0,127}$`)