diff --git a/app/cli/internal/trace/state/session.go b/app/cli/internal/trace/state/session.go index 11de110bf..31e4bd31b 100644 --- a/app/cli/internal/trace/state/session.go +++ b/app/cli/internal/trace/state/session.go @@ -71,20 +71,23 @@ func (s *Store) SaveSessionRecord(rec *SessionRecord) error { return err } - return os.WriteFile(filepath.Join(dir, sanitizeID(rec.SessionID)+".json"), data, 0600) + return os.WriteFile(s.sessionRecordPath(rec.SessionID), data, 0600) +} + +// sessionRecordPath returns the path of the record of a session. +func (s *Store) sessionRecordPath(sessionID string) string { + return filepath.Join(s.traceDirPath(), traceDirSessions, sanitizeID(sessionID)+sessionRecordExt) } // SessionRecordExists checks whether a session record file exists for the given session ID. func (s *Store) SessionRecordExists(sessionID string) bool { - path := filepath.Join(s.traceDirPath(), traceDirSessions, sanitizeID(sessionID)+".json") - _, err := os.Stat(path) + _, err := os.Stat(s.sessionRecordPath(sessionID)) return err == nil } // LoadSessionRecord reads the SessionRecord for sessionID. Returns nil, nil when not found. func (s *Store) LoadSessionRecord(sessionID string) (*SessionRecord, error) { - path := filepath.Join(s.traceDirPath(), traceDirSessions, sanitizeID(sessionID)+".json") - data, err := os.ReadFile(path) + data, err := os.ReadFile(s.sessionRecordPath(sessionID)) if err != nil { if os.IsNotExist(err) { return nil, nil diff --git a/app/cli/internal/trace/state/skills_test.go b/app/cli/internal/trace/state/skills_test.go index bee4ecb95..e03775954 100644 --- a/app/cli/internal/trace/state/skills_test.go +++ b/app/cli/internal/trace/state/skills_test.go @@ -69,8 +69,10 @@ func TestGCOrphansDropsSkillCopies(t *testing.T) { require.NoError(t, store.SaveCommitRecord(&CommitRecord{SHA: "sha-skills-live", SessionIDs: []string{live}, Timestamp: committed})) require.NoError(t, store.SaveCommitRecord(&CommitRecord{SHA: "sha-skills-orphan", SessionIDs: []string{orphan}, Timestamp: committed})) + // The orphan session is past the retention, so the GC drops its record. + startedAt := map[string]time.Time{live: time.Now().UTC(), orphan: time.Now().UTC().Add(-31 * 24 * time.Hour)} for _, id := range []string{live, orphan} { - require.NoError(t, store.SaveSessionRecord(&SessionRecord{SessionID: id, Active: true, StartedAt: time.Now().UTC().Format(time.RFC3339)})) + require.NoError(t, store.SaveSessionRecord(&SessionRecord{SessionID: id, Active: true, StartedAt: startedAt[id].Format(time.RFC3339)})) copyDir := store.SkillCopyDir(id, "/skills/x") require.NoError(t, os.MkdirAll(copyDir, 0o700)) require.NoError(t, os.WriteFile(filepath.Join(copyDir, "info.json"), []byte("{}"), 0o600)) diff --git a/app/cli/internal/trace/state/state.go b/app/cli/internal/trace/state/state.go index b4f625f0f..5a9c50c03 100644 --- a/app/cli/internal/trace/state/state.go +++ b/app/cli/internal/trace/state/state.go @@ -215,9 +215,16 @@ func (s *Store) RemoveSpecRedactions(sessionID string) error { // // 1. Walk commits/, drop any record whose SHA isn't in liveSHAs (post-rebase // orphan or commit on a deleted branch). -// 2. Walk ai-lines/ and sessions/, drop any entry whose session ID is no -// longer referenced by a surviving CommitRecord, or which is older than -// staleSessionRetention and has nothing left to attest. +// 2. Walk ai-lines/ and sessions/, drop any entry older than +// staleSessionRetention that has nothing left to attest. +// +// Step 2 does not drop a session only because no surviving CommitRecord +// references it. That is also the state of a session that is still running +// and has not committed yet, and of a session whose commits were rewritten +// where the hooks of this trace directory did not run (another worktree, +// another clone, hooks disabled). Dropping its ledger there makes the next push +// attribute all its lines to a human (issue 3778). The retention bounds the +// growth instead. // // An empty liveSHAs is treated as a pathological signal (no local branches // readable, or the caller fed us garbage) and the GC short-circuits — wiping @@ -234,7 +241,6 @@ func (s *Store) GCOrphans(liveSHAs map[string]bool) error { return fmt.Errorf("read commits dir: %w", err) } - liveSessions := make(map[string]struct{}) // Sessions with a surviving commit that has not been attested yet. Their // state must survive the age check — the next push still needs it. unattestedSessions := make(map[string]struct{}) @@ -253,44 +259,24 @@ func (s *Store) GCOrphans(liveSHAs map[string]bool) error { _ = os.Remove(path) continue } - // A session that a commit joined is live too: the next push of that - // commit counts its replacing lines for the session. + // An attested commit does not protect its sessions: the push that + // attested it already used their state. + if rec.Tracked { + continue + } for _, sid := range rec.AllSessionIDs() { - liveSessions[sanitizeID(sid)] = struct{}{} - if !rec.Tracked { - unattestedSessions[sanitizeID(sid)] = struct{}{} - } + unattestedSessions[sanitizeID(sid)] = struct{}{} } } - stale := s.staleSessionIDs(unattestedSessions) - - for _, child := range []struct { - sub string - ext string - }{ - {traceDirAILines, aiLinesExt}, - {traceDirSessions, sessionRecordExt}, - } { - dir := filepath.Join(s.traceDirPath(), child.sub) - entries, err := os.ReadDir(dir) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - continue - } + stale, err := s.staleSessionIDs(unattestedSessions) + if err != nil { + return err + } - return fmt.Errorf("read %s dir: %w", child.sub, err) - } - for _, entry := range entries { - if entry.IsDir() || !strings.HasSuffix(entry.Name(), child.ext) { - continue - } - id := strings.TrimSuffix(entry.Name(), child.ext) - _, alive := liveSessions[id] - if _, expired := stale[id]; !alive || expired { - _ = os.Remove(filepath.Join(dir, entry.Name())) - } - } + for id := range stale { + _ = os.Remove(s.aiLinesPath(id)) + _ = os.Remove(s.sessionRecordPath(id)) } // The redacted spec copies of a session go with its records. The session @@ -314,10 +300,12 @@ func (s *Store) GCOrphans(liveSHAs map[string]bool) error { return s.gcSkillCopies() } -// staleSessionIDs returns the sanitized IDs of session records older than -// staleSessionRetention whose work has all been attested. Records with an -// unparseable or missing StartedAt are never reported stale, so a bad -// timestamp cannot cause data loss. +// staleSessionIDs returns the sanitized IDs of the sessions in sessions/ and +// ai-lines/ that are older than staleSessionRetention and that no surviving +// unattested commit references. The age of a session is the StartedAt of its +// record. A session with no record, or with an unparseable or missing +// StartedAt, ages by the newest modification time of its files instead, so it +// cannot stay forever, and recent data is never reported stale. // // Liveness is deliberately not consulted: session-end does not always run, so // a leaked record stays Active forever and would never expire. @@ -329,29 +317,67 @@ func (s *Store) GCOrphans(liveSHAs map[string]bool) error { // file and pull the commit into its evidence, and by then its transcript is // usually gone — raw/ is wiped on every push — so the attestation it wins // yields nothing anyway. Under-crediting beats crediting the wrong session. -func (s *Store) staleSessionIDs(unattested map[string]struct{}) map[string]struct{} { +func (s *Store) staleSessionIDs(unattested map[string]struct{}) (map[string]struct{}, error) { records, err := s.LoadAllSessionRecords() if err != nil { - return nil + return nil, fmt.Errorf("load session records: %w", err) + } + startedAt := make(map[string]time.Time, len(records)) + for _, rec := range records { + if t, err := time.Parse(time.RFC3339, rec.StartedAt); err == nil { + startedAt[sanitizeID(rec.SessionID)] = t + } + } + + // The newest modification time of the files of each session. + modified := make(map[string]time.Time) + for _, child := range []struct { + sub string + ext string + }{ + {traceDirAILines, aiLinesExt}, + {traceDirSessions, sessionRecordExt}, + } { + dir := filepath.Join(s.traceDirPath(), child.sub) + entries, err := os.ReadDir(dir) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + + return nil, fmt.Errorf("read %s dir: %w", child.sub, err) + } + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), child.ext) { + continue + } + info, err := entry.Info() + if err != nil { + continue + } + id := strings.TrimSuffix(entry.Name(), child.ext) + if info.ModTime().After(modified[id]) { + modified[id] = info.ModTime() + } + } } cutoff := time.Now().UTC().Add(-staleSessionRetention) stale := make(map[string]struct{}) - for _, rec := range records { - id := sanitizeID(rec.SessionID) + for id, mtime := range modified { if _, ok := unattested[id]; ok { continue } - startedAt, err := time.Parse(time.RFC3339, rec.StartedAt) - if err != nil { - continue + age, ok := startedAt[id] + if !ok { + age = mtime } - if startedAt.Before(cutoff) { + if age.Before(cutoff) { stale[id] = struct{}{} } } - return stale + return stale, nil } // RemoveTraceDir removes the trace state but preserves local-sessions/, the diff --git a/app/cli/internal/trace/state/state_test.go b/app/cli/internal/trace/state/state_test.go index 9b44458d5..94c570fb1 100644 --- a/app/cli/internal/trace/state/state_test.go +++ b/app/cli/internal/trace/state/state_test.go @@ -783,6 +783,95 @@ func TestGCOrphans_StaleSessions(t *testing.T) { }) } +// A session that no live commit references is not proof that its work is +// gone: it can still be running with uncommitted edits, or its commits can +// have been rewritten where this trace directory's hooks did not run. Its +// state stays for the retention period, like any other session (issue 3778). +func TestGCOrphans_SessionsWithoutLiveCommits(t *testing.T) { + recent := time.Now().UTC().Add(-1 * time.Hour) + old := time.Now().UTC().Add(-31 * 24 * time.Hour) + + testCases := []struct { + name string + // withRecord saves a session record started at startedAt. + withRecord bool + startedAt string + // fileAge is the modification time of the session's files. + fileAge time.Time + // commit, when set, is a commit record of the session that is not + // reachable any more (a rebase outside this trace directory). + orphanCommit bool + wantKept bool + }{ + { + name: "running session with uncommitted edits survives another session's push", + withRecord: true, startedAt: recent.Format(time.RFC3339), fileAge: recent, + wantKept: true, + }, + { + name: "session whose commits were rewritten elsewhere keeps its ledger", + withRecord: true, startedAt: recent.Format(time.RFC3339), fileAge: recent, + orphanCommit: true, wantKept: true, + }, + { + name: "session without live commits is dropped after the retention", + withRecord: true, startedAt: old.Format(time.RFC3339), fileAge: old, + orphanCommit: true, wantKept: false, + }, + { + name: "ledger without a session record ages by its modification time", + fileAge: recent, wantKept: true, + }, + { + name: "old ledger without a session record is dropped", + fileAge: old, wantKept: false, + }, + { + name: "unparseable StartedAt ages by modification time", + withRecord: true, startedAt: "not-a-timestamp", fileAge: old, + wantKept: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + store := NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + + // Another session has a live, attested commit, so the GC has + // something reachable and does not short-circuit. + require.NoError(t, store.SaveCommitRecord(&CommitRecord{ + SHA: "sha-other", SessionIDs: []string{"sess-other"}, Timestamp: recent.Format(time.RFC3339), Tracked: true, + })) + + const sid = "sess-target" + if tc.withRecord { + require.NoError(t, store.SaveSessionRecord(&SessionRecord{SessionID: sid, Active: true, StartedAt: tc.startedAt})) + require.NoError(t, os.Chtimes(store.sessionRecordPath(sid), tc.fileAge, tc.fileAge)) + } + if tc.orphanCommit { + require.NoError(t, store.SaveCommitRecord(&CommitRecord{ + SHA: "sha-rewritten", SessionIDs: []string{sid}, Timestamp: tc.startedAt, Tracked: true, + })) + } + require.NoError(t, store.RecordLineRanges(sid, fileA, []aicodingsession.LineRange{{Start: 1, End: 5}})) + require.NoError(t, os.Chtimes(store.aiLinesPath(sid), tc.fileAge, tc.fileAge)) + + require.NoError(t, store.GCOrphans(map[string]bool{"sha-other": true})) + + assert.Equal(t, tc.wantKept, len(store.LoadAILineAttribution(sid).Files) > 0, "ledger kept") + if tc.withRecord { + assert.Equal(t, tc.wantKept, store.SessionRecordExists(sid), "session record kept") + } + + commits, err := store.LoadAllCommitRecords() + require.NoError(t, err) + require.Len(t, commits, 1, "an unreachable commit record is still dropped") + assert.Equal(t, "sha-other", commits[0].SHA) + }) + } +} + // TestSpecRedactions covers where the redacted copies of a session's spec // files live: inside the trace state, one directory per session, kept across // pushes and dropped only for the session that ends. @@ -880,8 +969,10 @@ func TestGCOrphansDropsSpecRedactions(t *testing.T) { require.NoError(t, store.SaveCommitRecord(&CommitRecord{SHA: "sha-live", SessionIDs: []string{"sess-live"}, Timestamp: "2026-03-28T00:00:00Z"})) require.NoError(t, store.SaveCommitRecord(&CommitRecord{SHA: "sha-orphan", SessionIDs: []string{"sess-orphan"}, Timestamp: "2026-03-28T00:00:00Z"})) + // The orphan session is past the retention, so the GC drops its record. + startedAt := map[string]time.Time{"sess-live": time.Now().UTC(), "sess-orphan": time.Now().UTC().Add(-31 * 24 * time.Hour)} for _, id := range []string{"sess-live", "sess-orphan"} { - require.NoError(t, store.SaveSessionRecord(&SessionRecord{SessionID: id, Active: true, StartedAt: time.Now().UTC().Format(time.RFC3339)})) + require.NoError(t, store.SaveSessionRecord(&SessionRecord{SessionID: id, Active: true, StartedAt: startedAt[id].Format(time.RFC3339)})) require.NoError(t, os.MkdirAll(store.SpecRedactionDir(id), 0o700)) require.NoError(t, os.WriteFile(filepath.Join(store.SpecRedactionDir(id), "copy"), []byte("x"), 0o600)) }