Repository navigation
Keep the time budget whatever the backend does - #26
solvyxtech wants to merge 3 commits into
Conversation
solvyxtech
left a comment
solvyxtech
left a comment
There was a problem hiding this comment.
Grok review: changes requested (posted as a comment because GitHub won't let the shared account request changes on its own PR).
Good fix overall: bounding every ACP wait at the turn deadline, session/cancel then a tree kill, tool time excluded from the stall clock, and providerStall journalled as a provider issue are all the right shape, and the CLI-level hung-agent test is exactly the regression we needed. Stacked on claude/sync-local-work (#23), so this lands after #23, not on main directly — fine by the branch rules.
Pairing rules check: clear. No subscription backends come back (removedSubscriptionProblem still guards asks), no competitor names in user-facing copy (only the Grok Build / OpenCode labels we kept), nothing touches fine-tune data.
Blocking:
- Signals now orphan the agent tree.
groupSpawn()makes every ACP/OpenCode childdetached, so it is no longer in the terminal's foreground process group and doesn't get Ctrl-C / SIGHUP with Maat. The only cleanup isprocess.on("exit")inproctree.ts, but Node doesn't runexithandlers when it dies from a signal it has no listener for —src/background.ts(hookExit) already documents this and installs SIGINT/SIGTERM listeners for that reason.proctree.tsdoesn't, so a headlessmaat runon Grok Build that gets Ctrl-C, or a bench harness that SIGTERMs Maat on its own timeout, now leavesgrokand its helpers running — the exact leak this PR is trying to close, and worse than before for the Ctrl-C case. Suggest: hook SIGINT/SIGTERM/SIGHUP the same waybackground.tsdoes (only when no listener exists, re-raise exit 130/143), or share one group registry withbackground.ts. A test that sends SIGTERM to the CLI mid-hang and asserts both pids are gone would pin it.
Non-blocking:
trackGroup's leader-exit handler SIGKILLs the group immediately. WhenkillTreesends SIGTERM and the leader honours it, its children get SIGKILL at once instead of theKILL_GRACE_MSgrace. Probably fine, but the doc comment promises SIGTERM-then-SIGKILL for the whole group.autoDraftandstartReferencecomputedeadlineAtasDate.now() + args.forMsat their own call time, not from when the job started, so drafting can run a little past the overall--forbudget. Worth deriving both from one start timestamp.- Doc-comment placement:
deadlineGraceMswas inserted betweencriteriaWaitMs's JSDoc and its function, and inacp.tsthe oldsendJSDoc now sits abovelastActivity(with a second copy abovesend). Just move them back. bench/local/run.pyrecordsproviderStall, but nothing in the bench summary excludes stalled tasks from the pass/fail count yet the wayprovider_cappeddoes. Fine as a follow-up if that's the plan.
CI (typecheck-and-test) was still pending when I looked. Not merging from here.
| groups.add(pid); | ||
| if (!hooked) { | ||
| hooked = true; | ||
| process.on("exit", () => { |
There was a problem hiding this comment.
Blocking: this exit hook doesn't run when Maat dies from SIGINT/SIGTERM/SIGHUP with no listener (see hookExit in src/background.ts). Since the children are now detached, they also no longer receive the terminal's Ctrl-C, so a signalled Maat leaves the whole agent group running. Please add signal listeners here (or share the registry with background.ts).
| groups.clear(); | ||
| }); | ||
| } | ||
| child.once("exit", () => { |
There was a problem hiding this comment.
Nit: after killTree's SIGTERM, if the leader exits promptly this SIGKILLs its children right away, skipping KILL_GRACE_MS for them.
| ): Promise<ReturnType<typeof taskChecksFrom>> { | ||
| const none: ReturnType<typeof taskChecksFrom> = { taskChecks: [], taskNotes: [] }; | ||
| // Under --for, no draft, critique or retry waits past the run's budget. | ||
| const deadlineAt = args.forMs ? Date.now() + args.forMs : undefined; |
There was a problem hiding this comment.
Nit: this budget starts when drafting starts, not when the job started, so a draft can run past the overall --for. Same at line 1260 for the reference draft.
| * between 1 s and 30 s. The summary is a courtesy; a run given 540 s must not | ||
| * spend another five minutes on it (or an hour, on a backend that hangs). | ||
| */ | ||
| export function deadlineGraceMs(budgetMs: number): number { |
There was a problem hiding this comment.
Nit: this landed between criteriaWaitMs's JSDoc and criteriaWaitMs, so that doc now belongs to nothing. Move this function (and its comment) above the older JSDoc.
A bench run given --for 540s on Grok Build logged job_end durationMs 3569961: one ACP prompt turn hung for an hour and nothing bounded the wait for its reply. - Every wait on an ACP agent (session start, session/prompt, the closing summary) is bounded by the turn's deadline. At the deadline Maat sends session/cancel, gives the agent 500 ms, then ends its whole process tree: backends now lead their own process group (src/proctree.ts), SIGTERM then SIGKILL, and any group left at exit is killed. - The job then ends with the normal deadline outcome: the sealed bar judges the tree on disk, and a pass there stays unverified without a claim (7abf0fa). - Stall watchdog: an ACP agent that sends nothing for MAAT_BACKEND_STALL_MS (default 5 min, 0 = off; Maat's own tool time does not count) is a provider stall. Its turn is cancelled, the journal records an error with providerIssue/providerStall, and job_end carries providerStall: a provider issue, not a task failure. - The same pattern elsewhere: the HTTP closing summary had no deadline at all; it now gets a short grace past the budget (a tenth of it, 1-30 s). askModel takes deadlineAt and cuts each ask, retry and overload pause at the budget; review, arbiter, criteria drafting and the reference draft pass it. OpenCode asks and the auth probe are bounded and kill their process trees. - bench/local/run.py records providerStall from job_end.
…the reference Backends lead their own process group, so Ctrl-C, a hangup or a harness's SIGTERM to Maat no longer reached them, and Node runs no exit handlers on a signal nobody listens for: the agent and its helpers were orphaned. proctree now installs SIGINT/SIGTERM/SIGHUP listeners where none exist (as background.ts does), ends every tracked group and exits 130/143/129. - A leader that exits gets the same SIGTERM-then-SIGKILL for its group as killTree, instead of an immediate SIGKILL, and keeps killTree's timer. - The criteria drafter and the reference draft share one deadline taken when the job starts them, instead of each taking its own Date.now(). - Doc comments put back above deadlineGraceMs/criteriaWaitMs and send().
455f696 to
1f30e14
Compare
…ools are not a stall - A deadline or stall closed the agent but left the bash/write_file it was waiting on running, so the tree went on changing while it was judged. The interrupt now aborts the in-flight tool call (the per-call controller the engine already uses for Ctrl-C), waits up to 5 s for it to end, and refuses any tool call that arrives after it. - The stall clock excused only Maat's own tool time. A builtin the agent announced and has not finished (GROK_OWN_TOOLS running a test suite) is now excused the same way; the deadline still bounds it. - Test helper: a zombie counts as gone.
|
Rebased onto the new How each review item was handled. First review
Second review (bake-off)
|
1 participant
Measured 2026-10-07: a bench run with
--for 540son Grok Build loggedjob_enddurationMs 3569961. One ACP prompt turn hung for about an hour, and Maat's time budget never fired.What changed
session/promptand the closing summary all stop at the turn's deadline. At the deadline Maat sendssession/canceland waits 500 ms. Then it kills the agent's whole process tree. Each backend now runs as the leader of its own process group (src/proctree.ts): SIGTERM first, then SIGKILL, and any group still alive when Maat exits is killed then.MAAT_BACKEND_STALL_MS(default 5 min, 0 turns it off), its provider counts as stalled. Time Maat spends running its own tools does not count as silence. The turn is cancelled and the agent is killed. The journal records anerrorwithproviderIssue/providerStall, andjob_endcarriesproviderStall. This is logged as a provider issue, not a task failure.askModeltakesdeadlineAt. Each ask, retry and overload pause stops at the budget. Review, arbiter, criteria drafting and the reference draft all pass it in.bench/local/run.pyrecordsproviderStall.Tests
test/acp-deadline.test.ts:--for 5sagainst a fakegrokthat takes the prompt and never answers. The fake ignores SIGTERM and starts asleep 300child. The job ends in about 7 s, both processes are gone, the agent receivedsession/cancel, and the journal has adeadlinerow plus a note saying the agent was cancelled and killed.npm run checkis green (1947 tests, e2e, self-check), run with~/.local/binremoved from PATH.