mirror of
https://github.com/esengine/DeepSeek-Reasonix.git
synced 2026-11-07 10:45:59 -10:00
Hold session locks across trash/delete and fix lint
Review follow-ups on the event-log hardening: - Close the probe-then-delete lease window: desktop trash, reconcile, and permanent delete now take a SessionRemovalGuard that holds the session's save lock and lease lock across the whole operation (built on #5977's delete-while-holding-flock primitive), so another process can no longer acquire the lease between a busy probe and the sidecar removal and then lose its freshly locked lease file. Live owners surface as a sanitized busy error; the lock files are deleted atomically with the release. - Run overlong-named sessions (pre-bounded recovery cascade names) in checkpoint-only mode: their event log/index sidecars would exceed the filesystem name limit, so the probe treats them as non-native instead of failing saves with ENAMETOOLONG. - Fix staticcheck QF1003: tagged switch for the save-reason mapping.
This commit is contained in:
1 parent
b942018011
commit
7bb7290c2f
7 files changed
+281
-49
No files matched your search
+7
-2
@@ -1586,9 +1586,11 @@ func removeDesktopSessionArtifacts(path string) error {
|
||||
if strings.TrimSpace(path) == "" {
|
||||
return nil
|
||||
}
|
||||
if sessionLeaseBusyCheck(path) {
|
||||
return errSessionBusyElsewhere
|
||||
guard, err := acquireSessionRemovalGuard(path)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer guard.Release()
|
||||
defer invalidateTopicSessionIndexForPath(path)
|
||||
for _, p := range sessionOwnedArtifactPaths(path) {
|
||||
if strings.TrimSpace(p) == "" {
|
||||
@@ -1598,6 +1600,9 @@ func removeDesktopSessionArtifacts(path string) error {
|
||||
return err
|
||||
}
|
||||
}
|
||||
if err := guard.RemoveSidecarsAndRelease(); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := removeSessionDisplay(filepath.Dir(path), path); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -140,15 +140,26 @@ func TestTrashSessionMatchesLiveSeesEventLogDivergence(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestTrashPathsBlockedWhenExternalOwnerHoldsLease(t *testing.T) {
|
||||
prev := sessionLeaseBusyCheck
|
||||
sessionLeaseBusyCheck = func(string) bool { return true }
|
||||
t.Cleanup(func() { sessionLeaseBusyCheck = prev })
|
||||
|
||||
func TestTrashPathsBlockedWhileLeaseHeld(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "session.jsonl")
|
||||
saveSnapshotTurns(t, path, 1)
|
||||
|
||||
// A live owner (any runtime — this process or another) holds the lease
|
||||
// lock on an open handle for its whole hold. Every destructive path must
|
||||
// refuse while it is held: probing once and deleting later would let the
|
||||
// owner's freshly locked lease file be unlinked out from under it.
|
||||
lease, err := agent.TryAcquireSessionLease(path)
|
||||
if err != nil {
|
||||
t.Fatalf("TryAcquireSessionLease: %v", err)
|
||||
}
|
||||
released := false
|
||||
defer func() {
|
||||
if !released {
|
||||
lease.Release()
|
||||
}
|
||||
}()
|
||||
|
||||
if err := trashSessionArtifactsBeforeMove(dir, path, "session.jsonl", nil); !errors.Is(err, errSessionBusyElsewhere) {
|
||||
t.Fatalf("trashSessionArtifactsBeforeMove err = %v, want errSessionBusyElsewhere", err)
|
||||
}
|
||||
@@ -159,10 +170,31 @@ func TestTrashPathsBlockedWhenExternalOwnerHoldsLease(t *testing.T) {
|
||||
t.Fatalf("removeDesktopSessionArtifacts err = %v, want errSessionBusyElsewhere", err)
|
||||
}
|
||||
if _, err := os.Stat(path); err != nil {
|
||||
t.Fatalf("session file touched despite external owner: %v", err)
|
||||
t.Fatalf("session file touched despite live owner: %v", err)
|
||||
}
|
||||
if _, err := os.Stat(store.SessionEventLog(path)); err != nil {
|
||||
t.Fatalf("event log touched despite external owner: %v", err)
|
||||
t.Fatalf("event log touched despite live owner: %v", err)
|
||||
}
|
||||
if _, err := os.Stat(store.SessionLeaseLock(path)); err != nil {
|
||||
t.Fatalf("lease lock deleted while held: %v", err)
|
||||
}
|
||||
|
||||
// Once the owner releases, the same trash call succeeds and the lock
|
||||
// sidecars are gone with it.
|
||||
lease.Release()
|
||||
released = true
|
||||
if err := trashSessionArtifactsBeforeMove(dir, path, "session.jsonl", nil); err != nil {
|
||||
t.Fatalf("trashSessionArtifactsBeforeMove after release: %v", err)
|
||||
}
|
||||
for _, p := range []string{
|
||||
path,
|
||||
store.SessionLockFile(path),
|
||||
store.SessionLeaseLock(path),
|
||||
store.SessionLeaseInfo(path),
|
||||
} {
|
||||
if _, err := os.Stat(p); !os.IsNotExist(err) {
|
||||
t.Errorf("artifact survived trash: %s (err=%v)", p, err)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+35
-37
@@ -153,36 +153,37 @@ func sessionTrashArtifacts(sessionPath, key string) []sessionTrashArtifact {
|
||||
}
|
||||
}
|
||||
|
||||
func sessionEphemeralArtifactPaths(sessionPath string) []string {
|
||||
return []string{
|
||||
store.SessionLockFile(sessionPath),
|
||||
store.SessionLeaseLock(sessionPath),
|
||||
store.SessionLeaseInfo(sessionPath),
|
||||
}
|
||||
}
|
||||
|
||||
// errSessionBusyElsewhere is the sanitized error surfaced when a destructive
|
||||
// session operation is blocked by a live external owner. It intentionally
|
||||
// carries no writer id, hostname, or path.
|
||||
// session operation is blocked by a live owner. It intentionally carries no
|
||||
// writer id, hostname, or path.
|
||||
var errSessionBusyElsewhere = errors.New("session is in use by another Reasonix window or process")
|
||||
|
||||
// sessionLeaseBusyCheck guards deletion of a session and its lease files: a
|
||||
// live external owner holds the lease lock on an open handle, and unlinking
|
||||
// the lock file would let a third runtime lock a fresh inode — two owners.
|
||||
// Swappable for tests.
|
||||
var sessionLeaseBusyCheck = agent.SessionLeaseHeldByOtherRuntime
|
||||
// acquireSessionRemovalGuard wraps agent.TryAcquireSessionRemovalGuard with
|
||||
// the sanitized busy error. The guard holds the session's save and lease
|
||||
// locks across the destructive operation and deletes the lock files
|
||||
// atomically with the release — a one-shot busy probe followed by RemoveAll
|
||||
// would let another process acquire the lease in between and then lose its
|
||||
// freshly locked lease file, breaking cross-process mutual exclusion.
|
||||
func acquireSessionRemovalGuard(sessionPath string) (*agent.SessionRemovalGuard, error) {
|
||||
guard, err := agent.TryAcquireSessionRemovalGuard(sessionPath)
|
||||
if err != nil {
|
||||
if errors.Is(err, agent.ErrSessionLeaseHeld) {
|
||||
return nil, errSessionBusyElsewhere
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
return guard, nil
|
||||
}
|
||||
|
||||
func sessionOwnedArtifactPaths(sessionPath string) []string {
|
||||
key := filepath.Base(sessionPath)
|
||||
artifacts := sessionTrashArtifacts(sessionPath, key)
|
||||
ephemeral := sessionEphemeralArtifactPaths(sessionPath)
|
||||
paths := make([]string, 0, len(artifacts)+len(ephemeral))
|
||||
paths := make([]string, 0, len(artifacts))
|
||||
for _, artifact := range artifacts {
|
||||
if strings.TrimSpace(artifact.src) != "" {
|
||||
paths = append(paths, artifact.src)
|
||||
}
|
||||
}
|
||||
paths = append(paths, ephemeral...)
|
||||
return paths
|
||||
}
|
||||
|
||||
@@ -204,9 +205,14 @@ func reconcileDesktopCleanupPending(dir string) error {
|
||||
}
|
||||
|
||||
func reconcileDesktopTrashSessionArtifacts(dir, sessionPath, key string) error {
|
||||
if sessionLeaseBusyCheck(sessionPath) {
|
||||
return errSessionBusyElsewhere
|
||||
// Hold the removal guard across the whole move so no runtime can acquire
|
||||
// the session (or save into it) while its artifacts are relocated; the
|
||||
// lock sidecars are deleted atomically with the guard release.
|
||||
guard, err := acquireSessionRemovalGuard(sessionPath)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer guard.Release()
|
||||
itemDir := filepath.Join(sessionTrashPath(dir), key)
|
||||
if err := os.MkdirAll(itemDir, 0o755); err != nil {
|
||||
return err
|
||||
@@ -219,7 +225,7 @@ func reconcileDesktopTrashSessionArtifacts(dir, sessionPath, key string) error {
|
||||
if err := trashSubagentArtifacts(dir, sessionPath, itemDir); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := removeEphemeralSessionArtifacts(sessionPath); err != nil {
|
||||
if err := guard.RemoveSidecarsAndRelease(); err != nil {
|
||||
return err
|
||||
}
|
||||
meta := trashedSessionMeta{Key: key, DeletedAt: time.Now().UnixMilli()}
|
||||
@@ -374,9 +380,6 @@ func sessionFileHasConversationContent(sessionPath string) bool {
|
||||
}
|
||||
|
||||
func trashSessionArtifactsBeforeMove(dir, sessionPath, key string, beforeMove func()) error {
|
||||
if sessionLeaseBusyCheck(sessionPath) {
|
||||
return errSessionBusyElsewhere
|
||||
}
|
||||
if err := validateSessionTrashTarget(dir, sessionPath, key); err != nil {
|
||||
return err
|
||||
}
|
||||
@@ -387,6 +390,13 @@ func trashSessionArtifactsBeforeMove(dir, sessionPath, key string, beforeMove fu
|
||||
if !shouldMove {
|
||||
return nil
|
||||
}
|
||||
// Acquired after prepareSessionTrashTarget: the duplicate-trash path in
|
||||
// there takes its own removal guard, and the guard is not reentrant.
|
||||
guard, err := acquireSessionRemovalGuard(sessionPath)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer guard.Release()
|
||||
itemDir := filepath.Join(sessionTrashPath(dir), key)
|
||||
if err := os.MkdirAll(itemDir, 0o755); err != nil {
|
||||
return err
|
||||
@@ -402,7 +412,7 @@ func trashSessionArtifactsBeforeMove(dir, sessionPath, key string, beforeMove fu
|
||||
if err := trashSubagentArtifacts(dir, sessionPath, itemDir); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := removeEphemeralSessionArtifacts(sessionPath); err != nil {
|
||||
if err := guard.RemoveSidecarsAndRelease(); err != nil {
|
||||
return err
|
||||
}
|
||||
meta := trashedSessionMeta{Key: key, DeletedAt: time.Now().UnixMilli()}
|
||||
@@ -498,18 +508,6 @@ func restoreTrashedSessionFile(dir, path string) error {
|
||||
return os.RemoveAll(itemDir)
|
||||
}
|
||||
|
||||
func removeEphemeralSessionArtifacts(sessionPath string) error {
|
||||
for _, path := range sessionEphemeralArtifactPaths(sessionPath) {
|
||||
if strings.TrimSpace(path) == "" {
|
||||
continue
|
||||
}
|
||||
if err := os.RemoveAll(path); err != nil && !os.IsNotExist(err) {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func purgeTrashedSessionFile(dir, path string) error {
|
||||
_, key, itemDir, err := validateTrashedSessionPath(dir, path)
|
||||
if err != nil {
|
||||
|
||||
@@ -249,9 +249,10 @@ func (s *Session) save(path string, mode sessionSaveMode) error {
|
||||
// writes leaves the newer transcript authoritative; the anchor rewrite
|
||||
// keeps the compatibility .jsonl fresh for direct readers.
|
||||
reason := "save"
|
||||
if mode == sessionSaveSnapshot {
|
||||
switch mode {
|
||||
case sessionSaveSnapshot:
|
||||
reason = "snapshot"
|
||||
} else if mode == sessionSaveRewrite {
|
||||
case sessionSaveRewrite:
|
||||
reason = "rewrite"
|
||||
}
|
||||
if repairLog {
|
||||
|
||||
@@ -114,15 +114,30 @@ type sessionEventLogProbe struct {
|
||||
schemaVersion int
|
||||
}
|
||||
|
||||
// sessionEventSidecarsFit reports whether the event log and index filenames
|
||||
// stay within the filesystem's name limit. Overlong transcript names (from the
|
||||
// pre-bounded recovery cascade, until reconcileOverlongSessionFilenames renames
|
||||
// them) must run checkpoint-only: creating their sidecars would fail with
|
||||
// ENAMETOOLONG mid-save.
|
||||
func sessionEventSidecarsFit(sessionPath string) bool {
|
||||
logName := filepath.Base(store.SessionEventLog(sessionPath))
|
||||
indexName := filepath.Base(store.SessionEventIndex(sessionPath))
|
||||
return len(logName) <= nameMaxBytes && len(indexName) <= nameMaxBytes
|
||||
}
|
||||
|
||||
// probeSessionEventLog inspects the first record of the event log to decide
|
||||
// whether the native persistence layer owns the file. Missing or empty logs
|
||||
// count as native (we may create/append); an undecodable or foreign first
|
||||
// record marks the file as not ours.
|
||||
// record — or a transcript name too long for the sidecars to fit — marks the
|
||||
// file as not ours.
|
||||
func probeSessionEventLog(sessionPath string) (sessionEventLogProbe, error) {
|
||||
path := store.SessionEventLog(sessionPath)
|
||||
if path == "" {
|
||||
return sessionEventLogProbe{native: true}, nil
|
||||
}
|
||||
if !sessionEventSidecarsFit(sessionPath) {
|
||||
return sessionEventLogProbe{}, nil
|
||||
}
|
||||
info, err := os.Stat(path)
|
||||
if err != nil {
|
||||
if os.IsNotExist(err) {
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
package agent
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"os"
|
||||
|
||||
"reasonix/internal/store"
|
||||
)
|
||||
|
||||
// SessionRemovalGuard holds a session's save lock and lease lock for the
|
||||
// duration of a destructive operation (trash, purge, permanent delete). While
|
||||
// held, no other runtime can acquire the session lease and no saver can write
|
||||
// the transcript, so artifacts can be moved or deleted without racing a live
|
||||
// owner; the lock files themselves are then deleted atomically with the
|
||||
// release (unlink-under-flock on Unix, delete-disposition on Windows), so a
|
||||
// later acquirer can never lock an inode that survived the deletion.
|
||||
//
|
||||
// This closes the probe-then-delete window: a one-shot busy check followed by
|
||||
// a plain RemoveAll lets another process acquire the lease between the two
|
||||
// steps and then loses its lock file, breaking cross-process mutual exclusion.
|
||||
type SessionRemovalGuard struct {
|
||||
path string
|
||||
saveLock *sessionLockFile
|
||||
leaseLock *sessionLockFile
|
||||
}
|
||||
|
||||
// TryAcquireSessionRemovalGuard takes both locks without blocking. A live
|
||||
// holder of either — including a lease held elsewhere in this process —
|
||||
// surfaces as ErrSessionLeaseHeld so callers report the session as busy
|
||||
// instead of deleting files out from under a running owner.
|
||||
func TryAcquireSessionRemovalGuard(path string) (*SessionRemovalGuard, error) {
|
||||
path = canonicalSessionSavePath(path)
|
||||
if sessionLeaseHeldLocally(path) {
|
||||
info, _ := LoadSessionLeaseInfo(path)
|
||||
return nil, &SessionLeaseError{Path: path, Info: info}
|
||||
}
|
||||
leaseLock, err := tryTakeSessionLockFile(store.SessionLeaseLock(path))
|
||||
if err != nil {
|
||||
if errors.Is(err, errSessionFileLockHeld) {
|
||||
info, _ := LoadSessionLeaseInfo(path)
|
||||
return nil, &SessionLeaseError{Path: path, Info: info}
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
saveLock, err := tryTakeSessionLockFile(store.SessionLockFile(path))
|
||||
if err != nil {
|
||||
leaseLock.Unlock()
|
||||
if errors.Is(err, errSessionFileLockHeld) {
|
||||
// A save is in flight; deleting mid-write would race it.
|
||||
return nil, &SessionLeaseError{Path: path}
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
return &SessionRemovalGuard{path: path, saveLock: saveLock, leaseLock: leaseLock}, nil
|
||||
}
|
||||
|
||||
// Release ends the guard without deleting the lock files — the abort path
|
||||
// when the destructive operation did not happen. Safe to call after
|
||||
// RemoveSidecarsAndRelease (it becomes a no-op).
|
||||
func (g *SessionRemovalGuard) Release() {
|
||||
if g == nil {
|
||||
return
|
||||
}
|
||||
if g.saveLock != nil {
|
||||
g.saveLock.Unlock()
|
||||
g.saveLock = nil
|
||||
}
|
||||
if g.leaseLock != nil {
|
||||
g.leaseLock.Unlock()
|
||||
g.leaseLock = nil
|
||||
}
|
||||
}
|
||||
|
||||
// RemoveSidecarsAndRelease deletes the lease info and both lock files
|
||||
// atomically with the release, then ends the guard. The lease info goes first,
|
||||
// while the lease lock is still held, so no probe can adopt it mid-removal.
|
||||
func (g *SessionRemovalGuard) RemoveSidecarsAndRelease() error {
|
||||
if g == nil {
|
||||
return nil
|
||||
}
|
||||
var errs []error
|
||||
if err := os.Remove(store.SessionLeaseInfo(g.path)); err != nil && !os.IsNotExist(err) {
|
||||
errs = append(errs, err)
|
||||
}
|
||||
if g.saveLock != nil {
|
||||
if err := g.saveLock.RemoveAndUnlock(); err != nil {
|
||||
errs = append(errs, err)
|
||||
}
|
||||
g.saveLock = nil
|
||||
}
|
||||
if g.leaseLock != nil {
|
||||
if err := g.leaseLock.RemoveAndUnlock(); err != nil {
|
||||
errs = append(errs, err)
|
||||
}
|
||||
g.leaseLock = nil
|
||||
}
|
||||
return errors.Join(errs...)
|
||||
}
|
||||
@@ -0,0 +1,83 @@
|
||||
package agent
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
|
||||
"reasonix/internal/provider"
|
||||
"reasonix/internal/store"
|
||||
)
|
||||
|
||||
func TestSessionRemovalGuardBlocksWhileLeaseHeld(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "session.jsonl")
|
||||
s := NewSession("sys")
|
||||
s.Add(provider.Message{Role: provider.RoleUser, Content: "work"})
|
||||
if err := s.SaveSnapshot(path); err != nil {
|
||||
t.Fatalf("SaveSnapshot: %v", err)
|
||||
}
|
||||
|
||||
lease, err := TryAcquireSessionLease(path)
|
||||
if err != nil {
|
||||
t.Fatalf("TryAcquireSessionLease: %v", err)
|
||||
}
|
||||
defer lease.Release()
|
||||
|
||||
if _, err := TryAcquireSessionRemovalGuard(path); !errors.Is(err, ErrSessionLeaseHeld) {
|
||||
t.Fatalf("guard under live lease err = %v, want ErrSessionLeaseHeld", err)
|
||||
}
|
||||
if _, err := os.Stat(store.SessionLeaseLock(path)); err != nil {
|
||||
t.Fatalf("lease lock disturbed by failed guard: %v", err)
|
||||
}
|
||||
|
||||
lease.Release()
|
||||
guard, err := TryAcquireSessionRemovalGuard(path)
|
||||
if err != nil {
|
||||
t.Fatalf("guard after release: %v", err)
|
||||
}
|
||||
// While the guard holds both locks, no new lease can be acquired: this is
|
||||
// the window that used to allow probe-then-delete races.
|
||||
if _, err := TryAcquireSessionLease(path); !errors.Is(err, ErrSessionLeaseHeld) {
|
||||
guard.Release()
|
||||
t.Fatalf("lease acquired while removal guard held, err = %v", err)
|
||||
}
|
||||
if err := guard.RemoveSidecarsAndRelease(); err != nil {
|
||||
t.Fatalf("RemoveSidecarsAndRelease: %v", err)
|
||||
}
|
||||
for _, p := range []string{
|
||||
store.SessionLockFile(path),
|
||||
store.SessionLeaseLock(path),
|
||||
store.SessionLeaseInfo(path),
|
||||
} {
|
||||
if _, err := os.Stat(p); !os.IsNotExist(err) {
|
||||
t.Errorf("sidecar survived removal: %s (err=%v)", p, err)
|
||||
}
|
||||
}
|
||||
// The path is free again for a normal acquire afterwards.
|
||||
after, err := TryAcquireSessionLease(path)
|
||||
if err != nil {
|
||||
t.Fatalf("lease after removal: %v", err)
|
||||
}
|
||||
after.Release()
|
||||
}
|
||||
|
||||
func TestSessionRemovalGuardReleaseKeepsSidecars(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "session.jsonl")
|
||||
guard, err := TryAcquireSessionRemovalGuard(path)
|
||||
if err != nil {
|
||||
t.Fatalf("guard: %v", err)
|
||||
}
|
||||
guard.Release()
|
||||
// Abort path: locks released, files left in place for the next owner.
|
||||
if _, err := os.Stat(store.SessionLeaseLock(path)); err != nil {
|
||||
t.Fatalf("lease lock missing after abort: %v", err)
|
||||
}
|
||||
lease, err := TryAcquireSessionLease(path)
|
||||
if err != nil {
|
||||
t.Fatalf("lease after abort: %v", err)
|
||||
}
|
||||
lease.Release()
|
||||
}
|
||||
Reference in new issue
Block a user