fix: bound task-deferred goal continuation - #42
Conversation
danyel117
left a comment
There was a problem hiding this comment.
Thanks for tackling the task-deferral latch. I found two blocking lifecycle issues before this is safe to merge:
-
In the V1 path, the new ceiling never expires a listed terminal child whose result is unreconciled.
refreshLiveChildren()callsmarkTerminal()for the same idle child on every poll (src/server.ts:632-634). Because the existing record hasterminalUnreconciled === true,markTerminal()falls through and rewritesterminalAtwithDate.now()(src/server.ts:664-680).taskBlockExpired()therefore always sees a fresh timestamp, so the documented terminal-unreconciled case remains blocked forever. Please preserve the original terminal timestamp and add a V1 regression test with a child that remains listed as idle and unreconciled. -
Both continuation paths check/re-arm task deferral before loading the current goal (
src/server.ts:1117-1137andsrc/server.ts:1752-1788). The scheduled task-deferral timer uses the defaultsettlepurpose, whose callback does not validate goal state. As a result, completing, clearing, or pausing a goal while a child blocks leaves a 1 Hz polling loop running until the ceiling; withmax_task_block_seconds: 0, it runs indefinitely. Please validate that the goal still exists and is continuable before re-arming, cancel/stop polling for closed goals, and cover this lifecycle in both V1 and V2 tests.
The existing 216 tests, lint, and typecheck pass locally, but the new tests only exercise continuously busy children and do not catch these cases.
… goals Addresses both review findings on prevalentWare#42. 1. markTerminal rewrote terminalAt on every poll. refreshLiveChildren re-marks a listed idle child each pass, and once the record is terminal-unreconciled the early-return guard no longer fires, so taskBlockExpired always saw a fresh timestamp and the ceiling could never expire the documented terminal-unreconciled case. Carry the original terminalAt - and with it the assistant marker captured when the child first went terminal, so reconciliation also measures from that moment - across repeat marks of the same state. A genuine state change or an explicit resetReconciled starts a new clock. 2. Both continuation paths re-armed task deferral before loading the goal. The settle timer's callback does not validate goal state, so completing, clearing, or pausing a goal while a child blocked left a 1 Hz poll running until the ceiling, and indefinitely with max_task_block_seconds: 0. Both paths now load the goal first and stop - releasing the deferral and cancelling the scheduled continuation - when it is missing, closed, or paused. budgetLimited/usageLimited still re-arm, matching reserveContinuation, which serves those a wrap-up continuation. Regressions: one V1 test per finding plus a paused variant, and a V2 lifecycle test. Each was checked against a reverted fix; see the PR comment for the vacuity control output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014UpT3nRyp8RkxD5FpcxLQ7
|
Both findings are fixed in [1] The ceiling never expired a terminal-unreconciled child. Confirmed.
One thing I extended beyond the literal finding: the same rewrite also reset [2] The deferral outlived the goal. Confirmed, in both paths. The task-block check and re-arm ran before Both paths now load the goal first and, when it is missing, closed, or paused, release the deferral and Regressions — four tests, each checked against a reverted fix.
The V1 lifecycle tests assert on A note on the V2 test, because my first version of it was vacuous and I want that on the record. V2's Verification (bun 1.4.0): The one failure is |
danyel117
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The original terminal timestamp fix now looks correct, and I verified the branch merges cleanly with current main; lint, typecheck, 241 tests, build, and pack dry-run all pass on that integrated tree.
Two blocking items remain:
-
taskDeferralGoalContinuable()still treats everybudgetLimited/usageLimitedgoal as continuable. That is only true until the one wrap-up is reserved:reserveWrapup()returnsnulloncebudgetWrapupSentis true. If a child remains blocking after the wrap-up has been sent, the 1 Hz task-deferral loop keeps re-arming despite no possible continuation (indefinitely whenmax_task_block_seconds: 0). Please make this predicate match the actual reservation rules—limited goals should only continue while their wrap-up is still unsent—and add V1/V2 regressions for the post-wrap-up blocked-child case. -
The lifecycle coverage requested in the prior review is still asymmetric. V1 covers clear + pause, V2 only clear, and neither path exercises a completed/unmet goal. Please add a V2 paused case and closed-goal coverage for both paths so the new
isClosedGoalbranch and both continuation implementations are proven.
I approved the fork workflow so CI can run on aba7497; these findings are independent of its result.
A Task child session that never reports a terminal state, or whose terminal result is never reconciled by an orchestrator turn, could defer goal continuation forever. Bound it with a `max_task_block_seconds` ceiling (default 900, `0` to disable), measured from `runningSince` for a listed child and from `terminalAt` for an unreconciled terminal one. `markTerminal` now carries the original `terminalAt` when it re-marks the same still-unreconciled terminal state, along with the assistant marker captured at that moment. `refreshLiveChildren` re-marks a listed idle child on every poll, so rewriting the timestamp each time meant the terminal branch of the ceiling could never fire for the case it exists to bound. The deferral also validates the goal before re-arming. The retry runs at 1 Hz and writes nothing to the goal, so a goal closed, cleared, or paused while a child still blocks would otherwise keep polling until the ceiling - and forever with `max_task_block_seconds: 0`. Rebased onto b7e185c (prevalentWare#46). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MLdQ88tCqSgfEsaGR978UF
`taskDeferralGoalContinuable` treated every `budgetLimited` / `usageLimited` goal as continuable. That holds only until the single wrap-up is reserved: `reserveWrapup` returns null once `budgetWrapupSent` is set, so a child still blocking after the wrap-up had been sent kept the 1 Hz poll re-arming with no possible continuation - indefinitely with `max_task_block_seconds: 0`. The predicate now mirrors the reservation rules instead of restating them: a limited goal is continuable only while its wrap-up is unspent, and every other non-active status fails `canContinue` outright. Lifecycle coverage is now symmetric. V1 and V2 each cover cleared, paused, completed, and the two-legged wrap-up case. Leg A of that pair is the control: a predicate that simply refused every non-active status would pass leg B while silently dropping the wrap-up a blocked limited goal is still owed. Both wrap-up tests also assert the deferral returns after `update_goal_status active` clears `budgetWrapupSent`, proving the loop stayed reachable and only the predicate was holding it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MLdQ88tCqSgfEsaGR978UF
aba7497 to
963d18c
Compare
|
Pushed Rebase first#46 landed on
[1] The limited-goal predicateYou are right, and the failure mode is exactly as you describe. The predicate now mirrors function taskDeferralGoalContinuable(goal: GoalSnapshot | null | undefined) {
if (!goal) return false
if (goal.status === "budgetLimited" || goal.status === "usageLimited") return !goal.budgetWrapupSent
return goal.status === "active"
}That is a direct read of [2] Lifecycle coverageNow symmetric — cleared, paused, completed, and the wrap-up case on both paths:
The two wrap-up tests are one regression with two legs. Leg A is the control: a predicate that simply refused every non-active status would pass leg B while silently dropping the one wrap-up a blocked limited goal is still owed, so the test first proves the deferral survives I also lifted the re-arm counter out of the one V2 test that had it inline into a The first version of leg B passed for the wrong reasonWorth flagging, because it is the kind of thing a green check hides. My first draft asserted only "the poll count stops growing" after re-blocking, and it passed with the fix reverted. Instrumenting Both wrap-up tests now settle for the in-flight continuation first, and each carries a positive control that the loop was genuinely reachable:
Mutation checksEvery new test was run against a deliberately broken tree, not just a green one:
Gates on
|
danyel117
left a comment
There was a problem hiding this comment.
Re-reviewed head 963d18c after the rebase and corrections. The limited-goal predicate now matches reserveContinuation, V1/V2 lifecycle coverage is symmetric, and no spec or blocking standards findings remain. Independent local gate: lint, typecheck, 260 tests, build, and pack dry-run all pass. The only non-blocking note is duplicated V1/V2 deferral orchestration, which is pre-existing structural symmetry rather than a correctness issue.
Summary
max_task_block_seconds(default900,0disables) as a wall-clock ceiling on how long one child session may defer goal continuationREADME.mdWhy
runAutoContinuehas one deferral path that writes nothing, records nothing and schedules nothing. When a task block is active andtaskBlockStatusreports noretryAt, it returns after only adding the session to an in-memory set:retryAtis non-null only while a snapshot-idle hold exists, andmarkSnapshotIdleis reachable only for a trackedrunningchild that is absent from the live child list, or for a liveidlechild that is not tracked at all. A child that is tracked asrunningand still listed bysession.children()matches neither, soretryAtisnulland nothing is scheduled. Because nothing is scheduled, nothing callstaskBlockStatusagain — andrefreshLiveChildrenonly runs from insidetaskBlockStatus, so the plugin also stops re-observing live children. One such child ends auto-continuation for that goal permanently.Nothing surfaces it. The goal keeps reading
status: "active",stopReason: null,continuationFailures: 0,pendingAttempt: null, with no history entry and nolastStatuschange — indistinguishable from a healthy goal that simply has not been prompted yet. The only symptom is thatautoTurnsstops advancing in wall-clock time.This is the complement of the case fixed in #6. #6 handles the child that disappears from live child status; this is the child that is still present and listed, so
markAbsentRunningChildrennever fires for it and the pruning path #6 added is never reached. #6's own rationale already describes the deadlock class — "Since there is no terminal event to reconcile, the old task can block continuation forever." This is the same deadlock through the other door. The same reasoning covers aterminalUnreconciledrecord whose orchestrator turn never arrives.The block appears twice, identically — once in the V1
runAutoContinueand once in the V2 copy — so a fix that patches only one site lands in only half the plugin. This change patches both.The
max_turn_timewatchdog added in #18 does not rescue this. It is unset by default, so in a default configuration it never arms. More importantly, when it is configured it re-runs the same block check and returns on it, andREADME.md:162documents that as intended: "Idle, built-in retry, session deletion, active Task children, and restricted agents suppress the retry." Active Task children are excluded from the watchdog by design, so it is not even a partial mitigation for this path. #20's bounded continuation retries do not apply either: they key off apendingAttemptor a countedcontinuationFailures, and the block check runs beforereserveContinuation, so this path creates neither.The deferral itself is not the bug — #4's reasoning still holds, and a goal should not burn continuations prompting a parent that is waiting on a subagent. The bug is that this one deferral has no way back, and it is on by default.
Related issue
None filed; the analysis is in this PR. Happy to open one if you would rather track it separately.
Changes
src/server.ts— the task-deferral branch in bothrunAutoContinuecopies now always callsscheduleSettledContinuation, falling back to a short bounded retry (TASK_BLOCK_RETRY_MS = 1_000) when there is no snapshot-idle hold to wait on. The retry re-enters withfromTaskDeferral = true, which the existing guard already expects, and it restores periodicrefreshLiveChildrenso fix: prune missing task sessions #6's pruning can still fire for a child that later vanishes.src/server.ts—TaskRecordgainsrunningSince, set bymarkRunningand preserved across consecutive running marks so a child that keeps reportingbusyaccumulates real age instead of resetting on every refresh;markTerminalclears it. AtaskBlockExpiredpredicate bounds arunningtask byrunningSinceand aterminalUnreconciledone by the existingterminalAt, andhasBlockingTasksskips expired records.TaskTrackeris shared, so this reaches both surfaces through one edit.src/server.ts— newmax_task_block_secondsoption, wired through the existingtimeoutMillisecondsFromSecondshelper exactly likemax_turn_time. Defaulted on at900deliberately: an opt-in ceiling would leave the default configuration as wedgeable as it is today.0removes the ceiling.test/server.test.ts— a test that a task deferral re-polls live children with no further idle event, and a test that a permanently-listedbusychild stops blocking after the ceiling.test/server-v2.test.ts— the V2 equivalent of the ceiling test, modelled on the existing "V2 idle continuation waits for a running child session" test with itssession.deletedrescue removed.waitForgained an optional deadline argument.README.md— documentmax_task_block_seconds, note that a deferral now re-checks children on a short timer, and mention the ceiling in the behavior overview.dist/server.js— rebuilt.Verification
Bun 1.4.0, branched from
mainat95df754:bun run lintbun run typecheckbun run test— 215 pass / 1 fail locally. The suite is not fully green on this machine, and was not before this change either. The one failure is the timing-sensitiveV2 watchdog no-response counts a failure on idle even with auto_continue false, which fails the same way on unpatchedmainhere (3/3 runs in isolation) while the Publish workflow is green onmainat95df754— so it is a local timing flake on this machine, not something this change introduces.bun run build—dist/server.jsis byte-identical to the committed bundlebun run pack:dry-run— 5 files, unchanged file setsrc/server.ts(each times out at the wait), so they do exercise the fixReproduced independently on a second machine: the same suite is noisy there in both directions (patched and unpatched baseline both fail several tests), and the three new tests still fail against unpatched source and pass against patched. Full-suite counts on any one host are therefore not a clean signal; the focused tests are.
Separately verified against the published
dist/server.jswith a mocked OpenCode event bus: a control run with no child sessions delivered 8 continuations over 9 turns, while an identical run that emits onesession.createdwith aparentIDand keeps that child listed froze atautoTurns: 2for the rest of the run withstatus: "active",stopReason: null,continuationFailures: 0. The only difference between the two runs is the child session.AI attribution
Written with Claude Code (model: Claude Opus 5) and reviewed by a human before submission.
Checklist
bun run test— new behavior has regression coverage; the suite is green apart from one pre-existing timing-sensitive failure that reproduces identically on unpatchedmain(see Verification)bun run lintpassesbun run typecheckpassesbun run buildpasses anddist/server.jsis committed if server code changed