fix(eval): preserve durable task traces

[bogdanovich]

What changed

• prevents startup reconciliation from replacing a complete durable task trace with an incomplete reconstruction under the same deterministic trace ID
• keeps interaction-owned delivery_failed traces open so later delivery recovery extends the canonical trace through delivered
• adds blocking, context-aware writer admission for shutdown so a full persistence queue cannot silently discard pending task traces
• bounds capacity-rejected terminal task traces to 128 in-memory entries, defers overflow to the durable task registry, and emits cumulative deferral/permanent-failure accounting
• deterministically merges retained task-event prefixes with pruned recovery suffixes only when the resulting event sequence is complete

Why

Recovery audit of merged PR #317 found four high-confidence durability and memory-bound defects in pkg/agent/trace_task_projector.go. Trace capture is enabled in production workspaces, so these are forward fixes for data loss, stale canonical outcomes, and unbounded memory growth.

User/developer impact

Task traces now preserve the strongest durable evidence across restart, record successful delivery recovery, use a bounded shutdown deadline and recover from the durable registry when admission cannot finish, and remain memory-bounded under prolonged writer saturation. No configuration or migration is required.

Validation

• go test ./...
• go test -race ./pkg/evalcapture ./pkg/agent
• focused race tests for restart downgrade, delivery recovery, bounded overflow, and shutdown admission
• go vet ./pkg/evalcapture ./pkg/agent
• ./scripts/pre-push-lint.sh --changed

Later PRs

PR #322 remains draft and paused. After this forward-fix merges, it will be updated onto latest main and adjusted to use the same durable admission behavior before review resumes.

Audit report: /home/server/.picoclaw/reviewer/agent-workspaces/forgeclaw/reports/recovery-audit-pr-317-dc6b500316ad.json

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

A malformed or otherwise unreadable existing task trace makes reconciliation permanently discard the valid recovered trace instead of repairing the canonical file.

Findings

1. [major] Repair unreadable existing task traces instead of dropping recovery (pkg/agent/trace_capture.go:195)

Store.Load returns an error for malformed, truncated, oversized, or validation-failing JSON. This path returns that error before the valid finalized candidate reaches the writer; trySubmitLocked only retries capacity errors, so it deletes the task state and records the generation completed. The unreadable file then remains canonical and a later restart will discard the candidate again. This defeats recovery after a damaged/interrupted trace write. Treat invalid stored content as replaceable (or quarantine it) and persist the validated candidate; retain/retry genuine transient filesystem failures. Add a test that pre-creates malformed JSON at the deterministic trace path and verifies that recovery replaces it with a valid trace.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

Two high-confidence durability/liveness issues remain in the task-trace admission and shutdown paths.

Findings

1. [blocking] Do not permanently drop traces when the retry spool fills (pkg/agent/trace_task_projector.go:330)

At the 128-entry limit this branch deletes the terminal trace and records it as completed. It is then neither retried after writer capacity returns nor reconstructed during this process lifetime, so a burst during a slow/stalled store permanently loses canonical task evidence. The added test explicitly accepts 17 such losses. Keep an on-disk retry marker/spool, or apply bounded producer backpressure while preserving recoverability; do not mark the trace completed until it has been durably admitted.

2. [major] Bound shutdown admission instead of waiting on a background context (pkg/agent/trace_task_projector.go:438)

close passes context.Background() to SubmitWait before the writer is closed. If the queue is full and the worker is stuck in Store.Save, no space signal will arrive and agent shutdown blocks indefinitely. Thread the service shutdown deadline/cancellation into this path (and retain a durable retry marker before giving up) so a stalled filesystem cannot prevent process shutdown forever.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

One major restart-reconciliation bug remains: a mutable completion ID changes historical record digests, so a recovered delivery outcome can be discarded in favor of stale canonical task evidence.

Findings

1. [major] Do not use the current record's completion ID for historical events (pkg/agent/trace_task_projector.go)

During startup reconstruction, every retained event is normalized against the latest task record. For an older task.delivery_changed event whose payload has no completion_id, this fallback inserts state.LastCompletionID into its correlation. A trace written before that mutable field was set therefore has a different digest for the same historical event than the reconstructed candidate. When a later delivery event should extend the canonical trace, traceRecordsExtend rejects the candidate as a non-prefix; because both traces are complete, reconciliation keeps the stale file and loses the newer delivery outcome. Persist completion identity in the relevant immutable task event payload (or otherwise preserve the old event's correlation during reconstruction), and add a full-history restart test covering an existing complete trace followed by delivery recovery.

Coverage Notes

• Placement warning: 1 finding(s) could not be anchored safely to changed diff lines and were kept summary-only.

[bogdanovich]

Addressed the latest summary-only finding in fc494a13.

• task.delivery_changed now persists its own immutable completion_id in the event payload.
• Trace normalization no longer falls back to mutable Record.LastCompletionID for historical events.
• Added a full-history restart regression: persist complete delivery_failed evidence under completion-1, change the record to completion-2, restart capture, recover to delivered, and verify the canonical trace extends with both identities.

Validation: go test ./..., go test -race ./pkg/tasks ./pkg/evaltrace ./pkg/evalcapture ./pkg/agent, go vet on those packages, and changed-package lint.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

Two high-confidence durability defects remain in task-trace retry accounting and asynchronous writer failure handling.

Findings

1. [major] Release the pending-admission slot when a retryable trace reopens (pkg/agent/trace_task_projector.go)

A capacity rejection sets state.retryable and increments pendingAdmissions. When a subsequent delivery-recovery event reopens that generation, this code clears retryable without decrementing the counter. The later successful terminal submission therefore cannot release the slot either, because it no longer sees retryable. Repeating this flow leaks slots until the 128-entry bound appears permanently full, preventing deferred generations from being reconstructed until restart. Decrement pendingAdmissions before clearing a retryable state, and add a regression that rejects admission, reopens the failed-delivery task, then recovers it and verifies the count returns to zero.

2. [major] Do not treat asynchronous queue admission as durable task-trace completion (pkg/agent/trace_capture.go)

trySubmitLocked removes the task state and records it completed immediately after Writer.Submit accepts it, but the writer persists later and can exhaust storage retries, emitting only EventPermanentlyFailed. The configured sink merely logs that event, so no retry/reconciliation is triggered while this process remains alive. If registry retention later removes the terminal task, the canonical trace is lost permanently. Feed durable persistence failures back into task-trace recovery (or retain a recoverable registry-backed marker until persistence succeeds) rather than considering queue admission final completion.

Coverage Notes

• Placement warning: 2 finding(s) could not be anchored safely to changed diff lines and were kept summary-only.

[bogdanovich]

Addressed both findings from the latest review in 53dd75b.

1. Reopening a retryable trace now releases its pending-admission slot before processing the newer lifecycle event. TestTaskTraceProjectorReopenReleasesRetrySlot covers failed delivery reopening and successful recovery without leaking capacity.

2. Queue admission is no longer treated as durable completion. The writer emits an explicit persisted event only after Store.Save succeeds and a permanently_failed event after storage retries are exhausted. The task projector keeps the trace in flight until that acknowledgement, retries permanent storage failures, and stores trace_capture_pending as durable operational metadata in the task registry. Retention cannot prune a terminal task while its trace is pending; the marker is cleared only after confirmed persistence (or when reconciliation proves the canonical trace is already durable). Shutdown keeps callbacks attached through writer drain, and a timeout leaves the registry marker for startup recovery.

Validation:

• go test ./...
• go test -race ./pkg/tasks ./pkg/evaltrace ./pkg/evalcapture ./pkg/agent
• go vet ./pkg/tasks ./pkg/evaltrace ./pkg/evalcapture ./pkg/agent
• ./scripts/pre-push-lint.sh --changed

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

A terminal task can be pruned by the registry before the asynchronous trace projector installs its durable trace-pending marker, losing the only recovery source before a task trace is submitted.

Findings

1. [major] Protect terminal traces before registry pruning (pkg/tasks/registry.go:188)

The terminal update is pruned while TraceCapturePending is still false; the projector sets that flag only after the registry commits and delivers the observer callback. With MaxRecords already occupied by active/protected records (or under the snapshot-byte budget), this newly terminal record is the only prune candidate, so pruneMutationLocked deletes it and its events. retainedEventsLocked then suppresses the observer notification, leaving neither an in-memory trace nor a registry record for retry/startup recovery. Reserve the trace-pending marker atomically as part of making a task terminal (or defer pruning of newly terminal records until the trace projector has protected them).

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

A task trace that has already received its persistence acknowledgement is permanently tombstoned in memory, so a later delivery-recovery event for the same generation is silently omitted from the canonical trace.

Findings

1. [blocking] Reopen completed task traces for later delivery recovery (pkg/agent/trace_task_projector.go:4)

After EventPersisted, observeWriterEvent removes the state and recordCompletedLocked adds this generation to completed. Every later registry event then returns here before it can rebuild or extend the trace. A task can still reopen after that acknowledgement (for example a delivery retry transitions a previously final delivery back to pending and then delivered), so the later completion ID/outcome never reaches the canonical trace. The existing dirty path only covers updates that arrive before the write acknowledgement. Keep a sequence watermark or reconstruct the state when a newer event arrives, re-protect the registry record, and reconcile the extended candidate. Add an acknowledgement-controlled test for failed delivery -> persisted/marker cleared -> delivery recovery in the same process.

2. [major] Do not clear pending trace markers when capture is disabled (pkg/agent/trace_task_projector.go:141)

Disabling capture clears all projector state and then calls SetTraceCaptureProtection(false), which clears every TraceCapturePending marker and prunes terminal records. That can happen while an admitted writer item is still queued/in flight or while overflow-deferred traces exist only in the registry. If retention or MaxRecords pressure applies, the registry can delete the recovery source before persistence is acknowledged; a later writer failure then has nothing to retry on restart. Stop accepting new capture on disable, but retain pending/in-flight durability markers until each trace is acknowledged or deliberately preserved for startup recovery.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

A stale writer acknowledgement can release the durable task-trace marker before the newer shutdown snapshot is persisted, allowing the newest task events to be lost on a crash.

Findings

1. [major] Do not acknowledge a newer shutdown snapshot with an older write (pkg/agent/trace_task_projector.go:714)

When a previously admitted trace becomes dirty, closeWithContext admits a second snapshot with the same trace ID and clears dirty here. Writer events are keyed only by TraceID, so the persistence acknowledgement for the older queued snapshot is indistinguishable from the acknowledgement for this newer one. observeWriterEvent can then clear TraceCapturePending and remove the state even though the newer events have not been durably written. If the process crashes (or the second write later fails), restart recovery has no marker and silently loses those events.

2. [major] Preserve incompleteness reasons unrelated to repaired history (pkg/agent/trace_task_projector.go:1068)

A contiguous merged task-event sequence does not prove that every incompleteness reason was repaired. For example, a task can reopen after an earlier complete persistence and then be captured during shutdown while delivery is still nonterminal; that candidate is marked runtime_closed_before_terminal_task_delivery. This assignment erases that signal and produces a trace that appears complete despite lacking a terminal delivery outcome.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

A persistence acknowledgement can clear the registry protection marker before a concurrently committed newer task event is projected, leaving the newer revision vulnerable to pruning on restart.

Findings

1. [blocking] Keep the marker until the latest committed task revision is persisted (pkg/agent/trace_task_projector.go:507)

EventPersisted takes p.mu before clearing the marker. A registry update can commit a newer terminal event just before this callback, while its observer is blocked on the same mutex; state.dirty is still false here, so this older acknowledgement clears TraceCapturePending and tombstones the state. If the process exits before the queued observation runs, registry loading can prune that now-unprotected terminal record under retention or capacity pressure, so startup has no newer event history to reconstruct. Before clearing the marker, compare the current registry generation/LastEventSeq with state.lastSeq; if it advanced, retain protection and rebuild/submit the newer revision. Add a barrier-driven interleaving test.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

Task-ID reuse can overwrite a terminal generation while its trace is still pending, removing the durable recovery anchor before persistence is confirmed.

Findings

1. [major] Preserve trace-pending generations across TaskID reuse (pkg/tasks/registry.go:188)

Upsert replaces the only record for this TaskID even when the previous terminal generation has TraceCapturePending. A reused ID can therefore erase the old generation's marker and event history while its trace is queued, deferred, or retrying. Subsequent acknowledgement/recovery only sees the new generation (and marker clearing fails on the generation mismatch), so a failed write or restart silently loses the old trace. Reject/defer reuse until the pending generation is acknowledged, or retain pending generations in a durable generation-keyed outbox; add a saturation/restart regression that verifies both generations recover.

Coverage Notes

• Required Codex advisory did not complete in the wrapper's terminal JSON format; direct review produced an independently validated finding.
• Remote Linter and Tests checks were still pending at review time.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

One high-confidence durability race remains: a stale persistence acknowledgement can clear trace protection after a newer registry revision commits. The focused task-ID reuse test is also deterministically failing at this head.

Findings

1. [blocking] Make revision confirmation and marker clearing atomic (pkg/agent/trace_task_projector.go:514)

rebuildCommittedRevisionLocked reads LastEventSeq and returns before this separate call clears TraceCapturePending. A newer task update can commit in that gap while its observer is blocked on p.mu, so this stale acknowledgement clears the new revision's recovery marker. Under pruning/reuse pressure, a crash before the observer restores protection can lose the newer event history. The errTaskTraceAlreadyDurable path has the same unguarded clear. Clear the marker through a registry operation that atomically verifies the generation and expected LastEventSeq; if it advanced, retain protection and rebuild/resubmit instead.

2. [minor] Wait for persistence acknowledgement before reusing the task ID (pkg/agent/trace_task_projector_test.go:4)

This waits for the trace file, but Store.Save creates that file before the writer's persisted callback clears TraceCapturePending. The next finishTaskForTrace consequently races the new reuse guard and fails with ErrTraceCapturePending; the focused test failed repeatedly at this head. Wait until the first generation's pending marker is cleared before performing the second Upsert.

Coverage Notes

• Integration Tests, Linter, and Tests GitHub checks were pending at review time.

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

Direct inspection found no additional high-confidence issue in the current head, but the required Codex advisory analysis could not complete because ACPx returned a runtime error.

Coverage Notes

• Required Codex advisory analysis did not reach a terminal result; this review is inconclusive despite the direct pass.

[bogdanovich]

Re-running review because the previous pass was explicitly inconclusive after a transient ACPx advisory runtime error; head and green checks are unchanged.\n\n/forgeclaw review

[bogdanovich]

/forgeclaw review

[bogdanovich]

/forgeclaw review

[bogdanovich]

Automated review by Reviewer agent.

Review Summary

Inconclusive: the required Codex advisory analysis could not run because ACPx returned an internal runtime error. Direct inspection and GitHub Actions CI found no additional independently validated findings at head ed3b691.

Coverage Notes

• Required Codex advisory analysis failed before producing a terminal response; this result is inconclusive rather than clean.