fix(control-plane): don't reap a parent whose child just finished (#1059)

[7vignesh]

Summary

Fixes the stale-execution reaper race where a running parent is timed out in the brief window (~50-600 ms) between its child reaching a terminal state and the parent posting its own result. The parent's real succeeded callback was then rejected with HTTP 409. Closes #1059.

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Docs only
• Tests only
• CI / tooling
• Breaking change

Root cause

A parent's own updated_at does not move while it waits on a child. The reaper's guard only skipped a parent while it had a child in running/pending/queued (plus waiting for workflows). The moment the child posted succeeded, that guard vanished and nothing refreshed the parent's clock, so a cleanup tick in that window set the parent (and its workflow) to timeout.

Fix (issue's Option 1, query-contained)

In MarkStaleExecutions and MarkStaleWorkflowExecutions (control-plane/internal/storage/execution_records.go), the child guard now also skips a parent when a child reached a terminal state after the cutoff:

c.status IN (<non-terminal>)
OR (c.status != 'timeout' AND COALESCE(c.completed_at, c.updated_at) > cutoff)

• A child that finished long before the cutoff no longer shields the parent, so a genuinely stuck parent is still reaped and orphan cleanup keeps working.
• timeout children are excluded from the shield: the reaper's own kills set a recent completed_at/updated_at, and letting them shield would stall the bottom-up chain unwind (one level per sweep). This keeps the existing UnwindsChainBottomUp behavior intact.
• Applied to both the candidate SELECT and the re-evaluating UPDATE in each function.

#1046 protected a workflow whose own activity clock advances; this covers the distinct case where the parent's clock never advances while it waits on a child.

Acceptance criteria (#1059)

• A parent stale by its own clock whose child finished after the cutoff is not reaped — TestMarkStaleExecutions_RecentlyFinishedChildShieldsParent, TestMarkStaleWorkflowExecutions_RecentlyFinishedChildShieldsParent.
• A parent whose child finished long before the cutoff and idle since is still reaped — TestMarkStaleExecutions_LongFinishedChildDoesNotShieldParent, TestMarkStaleWorkflowExecutions_LongFinishedChildDoesNotShieldParent.
• Same cases hold for MarkStaleWorkflowExecutions.

Test plan

• cd control-plane && go test ./internal/storage/ -run 'Stale|Reaper|Retry' -count=1
• cd control-plane && go test ./internal/storage/ -count=1 (full package)
• cd control-plane && go build ./... && go vet ./internal/storage/
• New tests added: recently-finished child shields parent (executions + workflows), long-finished child does not (executions + workflows), reaper-timeout child does not shield (executions). Existing bottom-up-unwind and blocked-on-active-child tests still pass.
• Postgres reaper live tests skip locally without a DB; the SQL uses the existing staleTimestampExpr/cutoffExpr dialect helpers, so the functional-tests-postgres CI job exercises the Postgres path.

Test coverage

• I ran tests for the surface(s) I changed locally.
• New code paths are covered by tests in this PR.
• The coverage gate check is green in CI before requesting review.

Checklist

• I have read CONTRIBUTING.md (if present) and docs/DEVELOPMENT.md.
• Commits are signed and follow conventional-commits style. (conventional-commits: yes; commit signing not configured in my environment)
• I have linked any related issues.

Related issues / PRs

Closes #1059. Related to #1046 / #1047 (distinct cause).

Notes for reviewers

• I renamed the two existing TerminalChildDoesNotShieldParent executions tests to LongFinishedChildDoesNotShieldParent and backdated their child's completion to before the cutoff. They previously relied (by accident) on CreateExecutionRecord stamping updated_at at "now", so a recent terminal child slipped through the old guard; that recency is exactly what this fix now protects, so the tests were updated to assert the intended "long-finished child" semantics.
• RetryStaleWorkflowExecutions has the same guard shape and the same race. I scoped this PR to the two functions the issue names; happy to extend the fix to the retry path in this PR or a follow-up — your call.
• @JaredAung expressed interest in this issue on 2026-09-17; there was no assignment or PR, so I picked it up. Happy to coordinate.

[github-actions]

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface	Current	Baseline	Δ
control-plane	87.90%	87.40%	↑ +0.50 pp	🟡
sdk-go	93.20%	92.00%	↑ +1.20 pp	🟢
sdk-python	94.72%	93.73%	↑ +0.99 pp	🟢
sdk-typescript	91.84%	90.42%	↑ +1.42 pp	🟢
web-ui	84.77%	84.79%	↓ -0.02 pp	🟡
aggregate	85.92%	85.75%	↑ +0.17 pp	🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

[github-actions]

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface	Touched lines	Patch coverage	Status
control-plane	56	96.00%	✅
sdk-go	0	—	➖ no changes
sdk-python	0	—	➖ no changes
sdk-typescript	0	—	➖ no changes
web-ui	0	—	➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

[AbirAbbas]

Pulled this down and ran it against a live control plane rather than just the unit tests, and the plain case is fixed: an agent whose parent reasoner makes sequential app.calls to a 6s child (3s of local work between calls, stale timeout 20s, cleanup interval 2s). On main the parent is reaped 1.9s after the third child succeeds — timeout / "execution timed out (no activity)" with four succeeded children under it. On this branch the same run walks all eight sub-calls and ends succeeded. go build ./... && go vet ./... and go test ./internal/storage/ ./internal/handlers/ are green locally too.

One case still gets through, and it's about which column the shield reads — left that inline.

Two smaller things:

RetryStaleWorkflowExecutions, which you asked about: I'd do it in this PR. It runs before both reapers when max_retries > 0 (internal/handlers/execution_cleanup.go:164) and carries the identical guard, so on that configuration the same race resets a live parent to pending mid-flight instead of timing it out, which is a worse outcome than the one you're fixing. TestRetryStaleWorkflowExecutions_TerminalChildDoesNotShieldParent pins the current behaviour and would need the same "long-finished" backdating your other two tests got.

The doc comment above MarkStaleExecutions still says "Deliberately no recency test on the child: the chain unwinds bottom-up instead ... it just takes one sweep per level". Both halves are now wrong — there is a recency test, and unwinding can take a stale window per level rather than a sweep. That comment is the thing the next person will read to decide whether they're allowed to touch this query.

[7vignesh]

Thanks for the live-run review, all three addressed in 68a63fd.

1. The clock question (the important one). You're right that completed_at is the agent's clock and the cutoff is the CP's, so a skewed clock or a late-landing callback expires the shield before it's installed. Rather than pick one column, the guard now reads the later of the two via a new childTerminalRecencyExpr: GREATEST(...) on postgres, MAX(julianday(...), ...) on sqlite. updated_at is the CP's own write clock, so the shield holds whenever either clock says the child finished after the cutoff. Your completed_at=45m-ago / updated_at=now repro now passes; I added it verbatim as TestMarkStaleExecutions_LateChildCallbackStillShieldsParent.

On the indefinite-shield worry with updated_at: for the executions table it can't happen, since UpdateExecutionStatusHandler rejects terminal->terminal (409), so a terminal row's updated_at is frozen. The re-bump you pointed at (applyEventToWorkflowExecution:134) is on workflow_executions only. A cancelled child there has null completed_at, so MAX falls to updated_at, and late events could in principle keep it recent. I think the fully-correct end state is your other suggestion, stamp completed_at from the CP clock on every terminal transition (cancel included), which would let the guard drop the updated_at half entirely. Happy to do that here if you'd prefer it over the MAX approach; I went with MAX first because it's contained to the reaper queries and doesn't change terminal-write semantics or user-facing completed_at/duration. Your call.

2. RetryStaleWorkflowExecutions. Done in this PR. Same shield on its SELECT and re-evaluating UPDATE. Backdated TestRetryStaleWorkflowExecutions_TerminalChildDoesNotShieldParent to the long-finished case as you noted, and added TestRetryStaleWorkflowExecutions_RecentlyFinishedChildShieldsParent.

3. The doc comment. Rewritten, it now documents the recency shield, the timeout-child exclusion, and that unwinding can take a stale window per level rather than a sweep.

go build ./... && go vet ./... green; go test ./internal/storage/ ./internal/handlers/ green.

[AbirAbbas]

Re-verified on 68a63fd.

MAX/GREATEST is the right shape for this PR — it's contained to the three queries and doesn't touch terminal-write semantics. The double COALESCE is what makes it safe rather than incidental: sqlite's two-arg max() returns NULL if either side is NULL, so a cancelled child with no completed_at would otherwise lose its shield entirely. I'd keep the "stamp completed_at from the CP clock on every terminal transition" idea as its own PR, since it changes a user-visible field.

Live re-runs on your head (20s stale timeout, 2s sweep, parent reasoner making 8 sequential child calls with local work in between): the parent walks all 8 and ends succeeded, twice, where main reaps it mid-flight. With max_retries=3 the difference is starker — on d9ce77a the retry sweep resets the live parent to pending three times and it ends timeout with retry_count=3 while every one of its children succeeded; on 68a63fd the same run ends succeeded with retry_count=0. The late-callback test fails on d9ce77a and passes here, so it's guarding something real rather than restating the code.

Two small things. The comment on childTerminalRecencyExpr says a terminal row's updated_at is frozen on the executions table because terminal->terminal is rejected — execution notes go through UpdateExecutionRecord, which always stamps updated_at, so a note on a terminal child does extend its parent's shield by a window. Harmless, but it's more than the comment can promise. And the PR description still says the retry path is scoped out of this PR.