fix(storage): workflow reaper consults execution activity clock - #1046
Conversation
Evidence from the local reproductionI read these values directly from the local SQLite database and the execution records. The rows used UTC timestamps. Three sampled coder executions showed the problem:
The heartbeat was fresh when the cleanup ran. For example, one During the investigation I checked the other likely explanations. All 66 same-day rows used After the node heartbeat and workflow-clock changes, a full build survived a 17-minute silent coder interval with no no-activity reap. A separate 27-execution short-fuse round also completed with no timeout signatures. |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
santoshkumarradha
left a comment
santoshkumarradha
left a comment
There was a problem hiding this comment.
The focused storage tests and CI pass, and the join handles heartbeats that arrive before candidate selection. There is still a narrow concurrent-heartbeat race in the final update that can produce the same false timeout, so I think we should close that before merging.
MarkStaleWorkflowExecutions now repeats its candidate predicates in the conditional UPDATE, but MarkStaleExecutions still only re-checked status. A heartbeat that lands between its candidate selection and that UPDATE therefore still flips a live execution row to timeout — the same false timeout the workflow reaper just stopped producing, through a narrower window (its candidate query reads the clock the heartbeat writes, so the race is the millisecond gap between the two statements rather than the whole run). Give it the same treatment: the conditional UPDATE re-evaluates the activity clock against the sweep cutoff and the non-terminal-child guard, and the body moves behind the same post-selection seam the workflow reaper uses so the interleaving is testable without sleeps. Tests: a real execution-note write landing in that window leaves the row running with its note intact; a seam that writes nothing still reaps the silent row with the existing "no activity" message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@ddbaron pushed one commit on top of your branch so this can land. I reproduced your report end to end on a real control plane with a 20s stale fuse and a 5s sweep, driving a real agent node and using Separately, and not caused by this PR: on the PostgreSQL backend neither reaper runs at all. and the row that is stale on both clocks stays |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Verified at 8c2395e.
Gates run locally on the control-plane surface exactly as CI runs them (gofmt on the touched files, web UI npm ci && npm run build, go build ./..., go vet ./..., the full go test -tags sqlite_fts5 ./... coverage sweep, and the patch gate): all green, patch coverage 100% on the 63 touched lines, storage package coverage 86.6%.
Anti-tautology: with only the two conditional-UPDATE guards stripped and the seams and tests kept, both interleaving tests fail with expected 0 / actual 1 while SeamWithoutActivityStillReaps and SilentRowsReapAndSyncExecution still pass. Against origin/main's storage source, the PR's own ExecutionActivityProtectsWorkflow fails with the original symptom.
Manual: real control plane with a 20s stale fuse and a 5s sweep, a real agent node, three dispatched executions, notes as heartbeats. On main the heart-beating run is reaped at t=31s and its late completion gets 409; on this branch it is still running at t=91s with the workflow clock frozen and the execution clock advancing, completion returns 200, the silent run is still reaped at t=30s and the one whose heartbeats stop at t=25s is reaped at t=51s.
Caveat recorded in the PR comment, not a blocker for this change: the reapers never execute on the Postgres backend today (sqlTx has no PrepareContext, so ? is never rebound), so this only changes behaviour on the local backend until that is fixed separately.
3 participants
Proposal
Keep the workflow cleanup path from reaping a workflow while its paired execution still has recent activity. The query should check the workflow timestamp and the timestamp on the paired active execution before declaring the workflow stale.
Observed failure
Each execution has two activity timestamps. Heartbeats and status writes update
executions.updated_at.MarkStaleWorkflowExecutionsreadsworkflow_executions.updated_at, which remains unchanged during a leaf wait.Four coder runs were reaped 10 to 13 minutes after starting on a 10-minute fuse. Heartbeats were arriving every 90 seconds. One run was reaped 57 seconds after its latest heartbeat. File commits continued after the reap, and the late completion updates returned HTTP 409 because the records were already terminal.
Implementation
This PR makes the workflow reaper join each workflow row to its paired active execution. It requires the workflow timestamp and the execution timestamp to be older than the cutoff before selecting the workflow for reaping.
The query still allows cleanup when the paired execution is absent or already terminal. The existing workflow-to-execution terminal synchronization remains in place.
The regression tests cover two cases:
executions.updated_atkeeps an old workflow row running.The change is on the read path, so existing heartbeat and note writes do not create another database write. Updating both timestamps on every activity event is another reasonable implementation. I am open to changing to that approach if it fits the control plane better.
Validation and evidence
The storage and handler tests for this path pass, along with
go vetand formatting checks. At the time of this update, upstream checks report 27 passed, 0 failed, and 22 skipped.The local reproduction is documented in the comment below. It includes the sampled execution IDs, timestamps, the 57-second interval between a heartbeat and a reap, the HTTP 409 completion errors, and the post-fix smoke results.
This PR is a working implementation for maintainer review. The goal is to keep the existing stale cleanup behavior for genuinely inactive workflows while allowing recent activity in the paired execution to count.