diff --git a/cmd/entire/cli/session/state.go b/cmd/entire/cli/session/state.go index 7ce4cbc02..3a1cbcbc5 100644 --- a/cmd/entire/cli/session/state.go +++ b/cmd/entire/cli/session/state.go @@ -21,6 +21,10 @@ import ( const ( // SessionStateDirName is the directory name for session state files within git common dir. SessionStateDirName = "entire-sessions" + + // StaleSessionThreshold is the duration after which an ended session is considered stale + // and will be automatically deleted during load/list operations. + StaleSessionThreshold = 24 * time.Hour ) // State represents the state of an active session. @@ -205,6 +209,12 @@ func (s *State) NormalizeAfterLoad() { } } +// IsStale returns true when the session has ended and the time since it ended +// exceeds StaleSessionThreshold. Active sessions (EndedAt == nil) are never stale. +func (s *State) IsStale() bool { + return s.EndedAt != nil && time.Since(*s.EndedAt) > StaleSessionThreshold +} + // StateStore provides low-level operations for managing session state files. // // StateStore is a primitive for session state persistence. It is NOT the same as @@ -237,10 +247,9 @@ func NewStateStoreWithDir(stateDir string) *StateStore { } // Load loads the session state for the given session ID. -// Returns (nil, nil) when session file doesn't exist (not an error condition). +// Returns (nil, nil) when session file doesn't exist or session is stale (not an error condition). +// Stale sessions (ended longer than StaleSessionThreshold ago) are automatically deleted. func (s *StateStore) Load(ctx context.Context, sessionID string) (*State, error) { - _ = ctx // Reserved for future use - // Validate session ID to prevent path traversal if err := validation.ValidateSessionID(sessionID); err != nil { return nil, fmt.Errorf("invalid session ID: %w", err) @@ -261,6 +270,16 @@ func (s *StateStore) Load(ctx context.Context, sessionID string) (*State, error) return nil, fmt.Errorf("failed to unmarshal session state: %w", err) } state.NormalizeAfterLoad() + + if state.IsStale() { + logCtx := logging.WithComponent(ctx, "session") + logging.Debug(logCtx, "deleting stale session state", + slog.String("session_id", sessionID), + ) + _ = s.Clear(ctx, sessionID) //nolint:errcheck // best-effort cleanup of stale session + return nil, nil //nolint:nilnil // stale session treated as not found + } + return &state, nil } @@ -326,8 +345,6 @@ func (s *StateStore) RemoveAll() error { // List returns all session states. func (s *StateStore) List(ctx context.Context) ([]*State, error) { - _ = ctx // Reserved for future use - entries, err := os.ReadDir(s.stateDir) if os.IsNotExist(err) { return nil, nil @@ -351,7 +368,7 @@ func (s *StateStore) List(ctx context.Context) ([]*State, error) { continue // Skip corrupted state files } if state == nil { - continue + continue // Not found or stale (Load handles cleanup) } states = append(states, state) diff --git a/cmd/entire/cli/session/state_test.go b/cmd/entire/cli/session/state_test.go index 99fe48428..80bc31693 100644 --- a/cmd/entire/cli/session/state_test.go +++ b/cmd/entire/cli/session/state_test.go @@ -1,8 +1,12 @@ package session import ( + "context" "encoding/json" + "os" + "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -118,3 +122,124 @@ func TestState_NormalizeAfterLoad_JSONRoundTrip(t *testing.T) { }) } } + +func TestState_IsStale(t *testing.T) { + t.Parallel() + + t.Run("nil_EndedAt_is_not_stale", func(t *testing.T) { + t.Parallel() + state := &State{EndedAt: nil} + assert.False(t, state.IsStale()) + }) + + t.Run("recently_ended_is_not_stale", func(t *testing.T) { + t.Parallel() + recent := time.Now().Add(-1 * time.Hour) + state := &State{EndedAt: &recent} + assert.False(t, state.IsStale()) + }) + + t.Run("ended_over_24h_ago_is_stale", func(t *testing.T) { + t.Parallel() + old := time.Now().Add(-25 * time.Hour) + state := &State{EndedAt: &old} + assert.True(t, state.IsStale()) + }) + + t.Run("ended_just_under_threshold_is_not_stale", func(t *testing.T) { + t.Parallel() + // A session that ended just under the staleness threshold should not be stale. + // Use StaleSessionThreshold rather than a magic number so the test stays in sync + // if the threshold changes. + recent := time.Now().Add(-1 * (StaleSessionThreshold - time.Hour)) + state := &State{EndedAt: &recent} + assert.False(t, state.IsStale()) + }) +} + +func TestStateStore_Load_DeletesStaleSession(t *testing.T) { + t.Parallel() + + stateDir := filepath.Join(t.TempDir(), "entire-sessions") + require.NoError(t, os.MkdirAll(stateDir, 0o750)) + store := NewStateStoreWithDir(stateDir) + ctx := context.Background() + + // Create a stale session (ended >24h ago) + staleEnded := time.Now().Add(-48 * time.Hour) + stale := &State{ + SessionID: "stale-session", + BaseCommit: "def456", + StartedAt: time.Now().Add(-72 * time.Hour), + EndedAt: &staleEnded, + } + require.NoError(t, store.Save(ctx, stale)) + + // Verify file exists before load + stateFile := filepath.Join(stateDir, "stale-session.json") + _, err := os.Stat(stateFile) + require.NoError(t, err, "state file should exist before load") + + // Load should return (nil, nil) for stale session + loaded, err := store.Load(ctx, "stale-session") + require.NoError(t, err, "Load should not return error for stale session") + assert.Nil(t, loaded, "Load should return nil for stale session") + + // File should be deleted from disk + _, err = os.Stat(stateFile) + assert.True(t, os.IsNotExist(err), "stale session file should be deleted after Load") + + // Create an active session (no EndedAt) to verify non-stale sessions still work + active := &State{ + SessionID: "active-session", + BaseCommit: "abc123", + StartedAt: time.Now(), + } + require.NoError(t, store.Save(ctx, active)) + + loaded, err = store.Load(ctx, "active-session") + require.NoError(t, err) + assert.NotNil(t, loaded, "Load should return state for active session") + assert.Equal(t, "active-session", loaded.SessionID) +} + +func TestStateStore_List_DeletesStaleSession(t *testing.T) { + t.Parallel() + + stateDir := filepath.Join(t.TempDir(), "entire-sessions") + require.NoError(t, os.MkdirAll(stateDir, 0o750)) + store := NewStateStoreWithDir(stateDir) + ctx := context.Background() + + // Create an active session (no EndedAt) + active := &State{ + SessionID: "active-session", + BaseCommit: "abc123", + StartedAt: time.Now(), + } + require.NoError(t, store.Save(ctx, active)) + + // Create a stale session (ended >24h ago) + staleEnded := time.Now().Add(-48 * time.Hour) + stale := &State{ + SessionID: "stale-session", + BaseCommit: "def456", + StartedAt: time.Now().Add(-72 * time.Hour), + EndedAt: &staleEnded, + } + require.NoError(t, store.Save(ctx, stale)) + + // List should return only the active session + states, err := store.List(ctx) + require.NoError(t, err) + require.Len(t, states, 1) + assert.Equal(t, "active-session", states[0].SessionID) + + // Stale session file should be deleted from disk + _, err = os.Stat(filepath.Join(stateDir, "stale-session.json")) + assert.True(t, os.IsNotExist(err), "stale session file should be deleted") + + // Active session file should still exist + _, err = os.Stat(filepath.Join(stateDir, "active-session.json")) + assert.NoError(t, err, "active session file should still exist") +} diff --git a/cmd/entire/cli/strategy/session_state.go b/cmd/entire/cli/strategy/session_state.go index b159bd561..707fd490c 100644 --- a/cmd/entire/cli/strategy/session_state.go +++ b/cmd/entire/cli/strategy/session_state.go @@ -2,7 +2,6 @@ package strategy import ( "context" - "encoding/json" "fmt" "log/slog" "os" @@ -37,32 +36,19 @@ func sessionStateFile(sessionID string) (string, error) { } // LoadSessionState loads the session state for the given session ID. -// Returns (nil, nil) when session file doesn't exist (not an error condition). +// Returns (nil, nil) when session file doesn't exist or session is stale (not an error condition). +// Stale sessions are automatically deleted by the underlying StateStore. func LoadSessionState(sessionID string) (*SessionState, error) { - // Validate session ID to prevent path traversal - if err := validation.ValidateSessionID(sessionID); err != nil { - return nil, fmt.Errorf("invalid session ID: %w", err) - } - - stateFile, err := sessionStateFile(sessionID) + store, err := session.NewStateStore() if err != nil { - return nil, fmt.Errorf("failed to get session state file path: %w", err) + return nil, fmt.Errorf("failed to create state store: %w", err) } - data, err := os.ReadFile(stateFile) //nolint:gosec // stateFile is derived from sessionID, not user input - if os.IsNotExist(err) { - return nil, nil //nolint:nilnil // nil,nil indicates session not found (expected case) - } + state, err := store.Load(context.Background(), sessionID) if err != nil { - return nil, fmt.Errorf("failed to read session state: %w", err) - } - - var state SessionState - if err := json.Unmarshal(data, &state); err != nil { - return nil, fmt.Errorf("failed to unmarshal session state: %w", err) + return nil, fmt.Errorf("failed to load session state: %w", err) } - state.NormalizeAfterLoad() - return &state, nil + return state, nil } // SaveSessionState saves the session state atomically. diff --git a/cmd/entire/cli/strategy/session_state_test.go b/cmd/entire/cli/strategy/session_state_test.go index 5c598fa97..8bfd51403 100644 --- a/cmd/entire/cli/strategy/session_state_test.go +++ b/cmd/entire/cli/strategy/session_state_test.go @@ -415,3 +415,53 @@ func TestTransitionAndLog_ReturnsHandlerError(t *testing.T) { t.Error("TransitionAndLog() should return handler error") } } + +// TestLoadSessionState_DeletesStaleSession tests that LoadSessionState returns (nil, nil) +// for a stale session and deletes the file from disk. +func TestLoadSessionState_DeletesStaleSession(t *testing.T) { + dir := t.TempDir() + _, err := git.PlainInit(dir, false) + if err != nil { + t.Fatalf("failed to init git repo: %v", err) + } + + t.Chdir(dir) + + // Create a stale session (ended >24h ago) + staleEnded := time.Now().Add(-48 * time.Hour) + state := &SessionState{ + SessionID: "stale-load-test", + BaseCommit: "abc123def456", + StartedAt: time.Now().Add(-72 * time.Hour), + EndedAt: &staleEnded, + StepCount: 5, + } + + err = SaveSessionState(state) + if err != nil { + t.Fatalf("SaveSessionState() error = %v", err) + } + + // Verify file exists before load + stateFile, err := sessionStateFile("stale-load-test") + if err != nil { + t.Fatalf("sessionStateFile() error = %v", err) + } + if _, err := os.Stat(stateFile); err != nil { + t.Fatalf("state file should exist before load: %v", err) + } + + // Load should return (nil, nil) for stale session + loaded, err := LoadSessionState("stale-load-test") + if err != nil { + t.Errorf("LoadSessionState() error = %v, want nil for stale session", err) + } + if loaded != nil { + t.Error("LoadSessionState() returned non-nil for stale session") + } + + // File should be deleted from disk + if _, err := os.Stat(stateFile); !os.IsNotExist(err) { + t.Error("stale session file should be deleted after LoadSessionState()") + } +}