diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 9b064f0c..ad8ff7ee 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -104,6 +104,22 @@ `./scripts/test-regression.sh` passed with 85.6% total coverage and zero uncovered functions. +## 2026-07-30 (Subscriber-Pinned Retention Quota — Issue #1048) + +- Symptom: hosted race CI could not subscribe to a just-completed extra run + because pruning had already removed it. +- Cause: a persisted terminal run with an active subscriber consumed the + retention quota despite being ineligible for deletion. +- Planned fix: compute pruning pressure only from persisted, terminal, + zero-subscriber candidates; pinned states remain protected exceptions until + cancellation re-runs pruning. +- Verification contract: focused/adjacent normal and race stress, harness + normal/race, full regression, and GitHub required checks. +- Result: focused normal/race passed at `-count=100`, adjacent pruning + normal/race passed at `-count=20`, the complete harness package passed + normal/race, and the final full regression passed with 85.6% coverage and + zero uncovered functions. + ## 2026-07-30 (Workflow Subscription Cancellation Test — Issue #1035) - Symptom: the full race gate failed in diff --git a/docs/logs/long-term-thinking-log.md b/docs/logs/long-term-thinking-log.md index 39fe2fc5..a7729e92 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -89,6 +89,21 @@ assertion and cleared focused stress, package stress, normal, race, and coverage verification. +## 2026-07-30 (Subscriber-Pinned Retention Quota — Issue #1048) + +- Command intent: repair the run-retention defect exposed while proving the + cron/callback GUI merge chain. +- User intent: keep API, TUI, and GUI conversation streams reliable under real + concurrent lifecycle pressure, not merely green in isolated tests. +- Success definition: pinned runs stay readable, newly completed runs remain + subscribable, unpinned history stays bounded, and cancellation reclaims the + exception. +- Guardrails: issue-first isolated worktree, preserve persistent records and + terminal ordering, no config or wire changes. +- Outcome: pruning pressure now comes only from drainable terminal candidates; + pinned states remain temporary exceptions, subscriber cancellation still + triggers cleanup, and all focused through full gates are green. + ## 2026-07-30 (Workflow Subscription Cancellation Test — Issue #1035) - Command intent: clear the race-gate blocker exposed while verifying the diff --git a/docs/plans/2026-07-30-issue-1048-pinned-retention-quota-impact-map.md b/docs/plans/2026-07-30-issue-1048-pinned-retention-quota-impact-map.md new file mode 100644 index 00000000..e1f5f4ef --- /dev/null +++ b/docs/plans/2026-07-30-issue-1048-pinned-retention-quota-impact-map.md @@ -0,0 +1,76 @@ +# Cross-Surface Impact Map: Issue #1048 Pinned Retention Quota + +## Task + +- Task / issue: exclude subscriber-pinned terminal states from the drainable + retention quota, #1048. +- Plan: `2026-07-30-issue-1048-pinned-retention-quota-plan.md`. +- Owner: Codex. +- Status: implemented and fully verified locally; hosted checks pending. + +## Current Ownership, Callers, and Data Flow + +- Entry: terminal completion/failure/cancellation calls + `pruneCompletedRuns`. +- Source of truth: `Runner.runs`; only persisted terminal states with zero + subscribers are deletion candidates. +- Current calculation: all persisted terminal states increment + `terminalCount`, while only drainable states enter `candidates`. +- Caller impact: `StartRun` followed by `Subscribe` can lose a fast run when a + pinned terminal state consumes the quota. + +## Config, API, CLI, and Tools + +- Config: retain `MaxCompletedRetention` and its default. +- API: `Subscribe` no longer loses the newest drainable run solely because a + different run remains pinned. +- CLI/TUI/tools: wire shapes and commands unchanged. + +## Persistence and Compatibility + +- Persistent Store records/events: unchanged and never deleted by this path. +- Schemas/migrations/caches: none. +- Compatibility: memory may hold configured drainable states plus pinned + exceptions, matching the existing after-subscribers-drain documentation. + +## Lifecycle, Security, and Reliability + +- Concurrency: existing Runner lock remains the sole synchronization boundary. +- Cleanup: subscriber cancellation re-runs pruning. +- Auth/privacy/secrets: none. +- Failure/recovery: persistent store fallback remains available. + +## Product and Integration Surfaces + +- Harness/API: fast completed runs remain subscribable under retention pressure. +- TUI/macOS GUI: run-scoped streams avoid a transient `run not found` failure. +- Conversation-level callback/cron streams: wire behavior unchanged; their run + state remains stable while related run subscribers are pinned. +- Providers/web/accessibility: none. + +## Deployment and Operations + +- Deployment/migration/flags: none. +- Observability: fewer premature `run not found` errors. +- Rollback: revert if unpinned candidates exceed the configured cap after + pruning or cancellation fails to reclaim pinned state. + +## Regression Tests + +- Red: hosted `collect extra run 2 events: run ... not found`. +- Green: focused normal/race `-count=100`. +- Controls: active subscriber remains pinned; the newest unpinned run remains + available; older unpinned runs prune; cancellation restores the cap. +- Full: adjacent pruning, harness normal/race, and + `./scripts/test-regression.sh`. + +## Documentation and Handoff + +- Update engineering log, long-term log, plan, impact map, and plans index. +- No public docs because behavior now matches the existing config contract. + +## Warning Check + +- Retention semantics and run-stream consumers are the only affected surfaces; + persistence, conversation mirrors, tools, providers, and schemas were + searched and are unchanged. diff --git a/docs/plans/2026-07-30-issue-1048-pinned-retention-quota-plan.md b/docs/plans/2026-07-30-issue-1048-pinned-retention-quota-plan.md new file mode 100644 index 00000000..9389f91c --- /dev/null +++ b/docs/plans/2026-07-30-issue-1048-pinned-retention-quota-plan.md @@ -0,0 +1,65 @@ +# Plan: Exclude Subscriber-Pinned Runs from the Drainable Retention Quota + +## Context + +- Governing GitHub issue: #1048. +- Problem: persisted terminal runs with active subscribers count toward + `MaxCompletedRetention` even though pruning cannot delete them. Under pressure + a just-completed run can be deleted before its caller subscribes. +- Red evidence: hosted race CI failed + `TestRunner_PruneKeepsCompletedRunWithActiveSubscriber` because an extra run + was already missing at subscription time. +- Constraint: preserve pinned-run protection, persistent records, and bounded + retention after subscribers drain. + +## Scope + +- In scope: count only eligible zero-subscriber terminal candidates when + calculating pruning pressure. +- Out of scope: conversation retention, store deletion, subscription delivery, + terminal persistence, and configuration shape/defaults. + +## Documentation Contract + +- Feature status: harness reliability bug repair. +- Public docs affected: none; the change aligns behavior with the existing + `RunnerConfig` comment. +- Evidence: engineering and long-term logs, plan, impact map, plans index. + +## Test Plan + +- Red: retain the exact hosted run-not-found failure as regression evidence. +- Green: focused pruning test normal/race at `-count=100`. +- Adjacent/package: all pruning tests and `internal/harness` normal/race. +- Full: `./scripts/test-regression.sh` and GitHub required checks. + +## Cross-Surface Impact Map + +- See `2026-07-30-issue-1048-pinned-retention-quota-impact-map.md`. + +## Implementation Checklist + +- [x] Create contract-complete bug #1048. +- [x] Capture hosted failure and ownership/search evidence. +- [x] Write plan and impact map before code. +- [x] Calculate the cap from drainable terminal candidates. +- [x] Run focused stress, adjacent, package, and full local gates. +- [ ] Pass hosted required checks. +- [ ] Merge through a closing PR. + +## Verification + +- The focused regression passed normal/race at `-count=100`. +- All adjacent pruning tests passed normal/race at `-count=20`. +- The complete `internal/harness` package passed normal and race. +- The isolated coverage phase and a subsequent complete + `./scripts/test-regression.sh` passed with 85.6% coverage and zero uncovered + functions. + +## Risks and Mitigations + +- Risk: pinned runs can temporarily exceed the numeric cap. +- Mitigation: this is the documented exception; cancellation re-runs pruning. +- Risk: the change could retain too many unpinned runs. +- Mitigation: assert the zero-subscriber candidate set stays at or below the + configured limit after every pruning trigger. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index 1ad32473..948eea48 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -12,6 +12,8 @@ - `2026-07-30-issue-1039-worktree-containment-ci-impact-map.md` — Cross-surface impact map for Issue #1039. - `2026-07-30-issue-1042-default-registry-repeat-plan.md` — Issue #1042 planned repeatable top-level workspace registry contract test. - `2026-07-30-issue-1042-default-registry-repeat-impact-map.md` — Cross-surface impact map for Issue #1042. +- `2026-07-30-issue-1048-pinned-retention-quota-plan.md` — Issue #1048 planned exclusion of subscriber-pinned runs from the drainable terminal retention quota. +- `2026-07-30-issue-1048-pinned-retention-quota-impact-map.md` — Cross-surface impact map for Issue #1048. - `2026-07-30-issue-1035-workflow-subscription-cancel-plan.md` — Issue #1035 planned deterministic workflow subscription-cancellation regression repair. - `2026-07-30-issue-1035-workflow-subscription-cancel-impact-map.md` — Cross-surface impact map for Issue #1035. - `2026-07-30-issue-1026-feedback-direct-publish-plan.md` — Issue #1026 attached-image, publish-by-default GitHub feedback intake (implemented and verified; promotion pending). diff --git a/internal/harness/runner.go b/internal/harness/runner.go index 2b34cc62..388be4e9 100644 --- a/internal/harness/runner.go +++ b/internal/harness/runner.go @@ -528,13 +528,11 @@ func (r *Runner) pruneCompletedRunsLocked() { limit = defaultMaxCompletedRetention } - terminalCount := 0 candidates := make([]retainedRunCandidate, 0) for runID, state := range r.runs { if state == nil || !isTerminalRunStatus(state.run.Status) || !state.terminalEventPersisted { continue } - terminalCount++ if len(state.subscribers) == 0 { candidates = append(candidates, retainedRunCandidate{ id: runID, @@ -542,7 +540,10 @@ func (r *Runner) pruneCompletedRunsLocked() { }) } } - if terminalCount <= limit || len(candidates) == 0 { + // Subscriber-pinned terminal runs are protected exceptions until their + // subscribers drain; they must not consume the quota for runs that are + // actually eligible for pruning. + if len(candidates) <= limit { return } @@ -553,10 +554,7 @@ func (r *Runner) pruneCompletedRunsLocked() { return candidates[i].updatedAt.Before(candidates[j].updatedAt) }) - toDelete := terminalCount - limit - if toDelete > len(candidates) { - toDelete = len(candidates) - } + toDelete := len(candidates) - limit for i := 0; i < toDelete; i++ { delete(r.runs, candidates[i].id) } diff --git a/internal/harness/runner_prune_test.go b/internal/harness/runner_prune_test.go index caeb142f..43291ec6 100644 --- a/internal/harness/runner_prune_test.go +++ b/internal/harness/runner_prune_test.go @@ -96,22 +96,37 @@ func TestRunner_PruneKeepsCompletedRunWithActiveSubscriber(t *testing.T) { } } + extraRunIDs := make([]string, 0, 3) for i := 0; i < 3; i++ { run, err := runner.StartRun(RunRequest{Prompt: fmt.Sprintf("extra %d", i)}) if err != nil { t.Fatalf("start extra run %d: %v", i, err) } + extraRunIDs = append(extraRunIDs, run.ID) if _, err := collectRunEvents(t, runner, run.ID); err != nil { t.Fatalf("collect extra run %d events: %v", i, err) } } - runner.mu.RLock() - _, ok := runner.runs[pinned.ID] - runner.mu.RUnlock() - if !ok { - t.Fatal("completed run with an active subscriber was pruned") - } + waitForRunnerPrune(t, runner, func() bool { + runner.mu.RLock() + defer runner.mu.RUnlock() + if len(runner.runs) != 2 { + return false + } + if _, ok := runner.runs[pinned.ID]; !ok { + return false + } + if _, ok := runner.runs[extraRunIDs[len(extraRunIDs)-1]]; !ok { + return false + } + for _, runID := range extraRunIDs[:len(extraRunIDs)-1] { + if _, ok := runner.runs[runID]; ok { + return false + } + } + return true + }) cancelPinned()