fix(coderd/x/chatd): bound task attempts by progress instead of a fixed deadline - #27371
ibetitsmike wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
7aad78e to
db15eaa
Compare
7b7cd6d to
2cc3b31
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
db15eaa to
a0e39db
Compare
2cc3b31 to
c717bbb
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
c717bbb to
1191ea5
Compare
a0e39db to
a19b772
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
a19b772 to
b89e001
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
I think this PR should be reconsidered after #27369 and #27370 establish the revised agent operation.
The remaining problem is narrow: while a foreground command runs, chatd repeatedly asks the agent for process output. Each successful response proves that the agent is reachable and the task is still making progress. The current fixed task timeout treats that valid wait as a stuck task.
Changing the task timeout into an idle timeout may be reasonable, but this PR goes further by adding:
- a 24-hour total limit;
- a new maximum for
wait_agent; - new limits for computer use;
- rules for how a progressing process affects sibling tools.
Those changes alter unrelated tool behavior and make the timeout model harder to follow.
Could this PR be reduced to the behavior required by execute?
- A successful process-output response counts as progress.
- A silent or unreachable agent does not count as progress.
- The timeout requested by
executecontrols how long chatd waits for the command. - The model stream keeps its existing silence timeout.
If the revised agent operation changes how chatd waits for results, the implementation may become smaller still. I would avoid settling the wider timeout policy before that boundary is final.
🤖 This review was automatically generated with Coder Agents.
f75158c to
30f965a
Compare
639293f to
dc2b188
Compare
30f965a to
054e270
Compare
dc2b188 to
7a8997c
Compare
ibetitsmike
changed the title
|
Kept the implementation but rewrote the argument, because the description failed to explain why the caps are in this PR at all. Reading them as unrelated timeout policy was fair given what I wrote. The four behaviors you asked for are what this implements: a successful process-output response counts as progress, a silent or unreachable agent does not, On the extra limits, here is the part I left out. Tool calls in a batch fan out as a goroutine per call under one I added If you still read the caps as out of scope, I would rather split them into a follow-up than drop them, since removing them leaves sibling waits unbounded once the idle window lands. Say the word and I will restructure. On sequencing: this now sits on the revised #27369 and #27370, and the boundary they establish did not shrink this PR. Chatd still waits for the command itself, so the idle window is still needed.
|
054e270 to
6aefea3
Compare
7a8997c to
7a489fb
Compare
6aefea3 to
dbdb8fe
Compare
7a489fb to
d3294e6
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
I would prefer a design that does not require this watchdog.
It's unclear what we're trying to solve with it and I see no reason to have it.
Take execute or process_output.
As long as the HTTP request to the workspace agent is alive, that's liveness. There is no "progress" that needs to be observed here. A command must be allowed to be silent for 2 hours, only to then produce some output.
I would want to see a clearly, non-LLM, written justification for this feature as I can't make heads or tails of the PR description.
| // it, an oversized wait could ride an attempt whose watchdog a | ||
| // concurrently progressing execute keeps kicking, far past the | ||
| // attempt idle window. The model repeats the wait action when a UI | ||
| // needs longer to settle. |
There was a problem hiding this comment.
Agreed, and this PR is now parked as a draft (see the note on the PR), so this comment travels with the rework rather than getting a cosmetic edit here.
The metaphor register goes with it. KickAttemptKeepalive becomes ResetAttemptIdleTimeout, and this comment gets rewritten in plain English rather than trimmed.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // concurrently progressing execute keeps kicking, far past the | ||
| // attempt idle window. The model repeats the wait action when a UI | ||
| // needs longer to settle. | ||
| const maxComputerWait = 30 * time.Second |
There was a problem hiding this comment.
Why is 30 seconds the maximum wait? That seems to me like it will lead to trashing where the model has to repeatedly call wait, wasting tokens.
There was a problem hiding this comment.
Fair challenge, and I do not want to defend the current number. This PR is parked as a draft (note on the PR), so I would rather argue the wait bound against the reworked design than justify 30s as it stands.
For the follow-up: your thrashing concern is the right frame. The cap exists only because a long local wait can outlive the attempt window, so if the attempt window stops being the constraint, the cap can grow or go. That ordering is what the rework has to settle first.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| } | ||
| return errorResult(enrichStartError(fmt.Sprintf("start background process: %v", err))) | ||
| } | ||
| KickAttemptKeepalive(ctx) |
There was a problem hiding this comment.
Yes. Parked as a draft (note on the PR), and the rename goes into the rework: KickAttemptKeepalive -> ResetAttemptIdleTimeout. That says what it does and drops the metaphor you have now flagged twice.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
| // the idle watchdog, so the resumable timeout result has to | ||
| // commit before the watchdog cancels the attempt. A timeout is | ||
| // not a failure; the model resumes with another wait_agent call. | ||
| maxSubagentWaitTimeout = 10 * time.Minute |
There was a problem hiding this comment.
We honestly shouldn't have a timeout here. If the agent decides it's a long task, let it wait 1 hour.
There was a problem hiding this comment.
This is the objection that parked the PR, so it is doing real work. Note on the PR has the detail.
Short version: I am not going to defend the 10m cap, because reducing this PR to the behaviors you endorsed deletes it along with the 24h cap and the computer-use clamp, and that reopens the sibling-extension hole I used to justify them. That is a rework, not an edit.
One correction I want on the record before the follow-up, since it changes what that PR has to argue: every blocking process-output request is capped at 5 minutes agent-side (agentproc/api.go), so chatd polls in a loop and there is no long-lived request to read liveness from. The keepalive fires on each successful poll, not on output, so a command silent for two hours already keeps the attempt alive today. It measures whether the agent answers, never whether the command talks. The follow-up will ship that as a test rather than prose: a stalled ProcessOutput request must not reset the idle window.
Reply written by Mux, an AI coding agent, operating on Mike's behalf.
dbdb8fe to
202fa50
Compare
d3294e6 to
f98de03
Compare
202fa50 to
a3f2866
Compare
f98de03 to
d67cdfd
Compare
|
Parking this PR and unblocking the rest of the stack. You have now rejected the premise twice, and I think you are right that it does not belong in this stack. #27372 was re-parented onto #27370, so the three run-once correctness PRs no longer wait on timeout policy. The dependency was prose only: three mentions of "attempt watchdog", reworded to "attempt timeout". Your four inline items and the two review-body points travel with this branch. Reducing it to the behaviors you endorsed means deleting the 24h cap, the Two things I will carry into the follow-up: The One correction for the record, briefly, because it changes what the follow-up has to argue. Every blocking process-output request is capped at 5 minutes agent-side ( On the 30-second computer-use wait and the
|
…ed deadline While a foreground command runs, chatd repeatedly asks the agent for process output. Each successful response proves the agent is reachable and the task is progressing, but the fixed per-attempt deadline canceled that valid wait anyway, so a command could not outlive it. The attempt deadline becomes an idle window that only a successful process-API round-trip resets, via the execute and process_output tools. A silent or unreachable agent resets nothing, the model stream keeps its own silence guard, and the timeout the model asked execute for still bounds how long chatd waits for that command. Converting an absolute deadline into a progress-based one removes the only bound sibling tool calls had. Calls in a batch run concurrently against one attempt context, so a kick from a progressing execute call extends the deadline every sibling was relying on. Tools that block on a model-supplied duration are therefore bounded per call instead: a single wait_agent block to 10 minutes and a computer wait action to 30 seconds, clamped before the millisecond conversion so an oversized value cannot overflow. An absolute cap still bounds one attempt regardless of progress. Attempt-watchdog cancellations classify as retryable, and the replay re-attaches to the still-running process rather than starting it again.
d67cdfd to
530cd33
Compare
a3f2866 to
0599aa1
Compare
2 participants
Stack context
Part 3 of a 4-PR stack (CODAGT-757). Retitled and reframed after review; the description previously listed the per-tool caps without saying why they belong here.
The narrow problem
While a foreground command runs, chatd repeatedly asks the agent for process output. Each successful response proves the agent is reachable and the task is progressing. The fixed per-attempt deadline treated that valid wait as a stuck task, so a command could not outlive it.
This PR implements exactly the behavior asked for in review:
executestill controls how long chatd waits for that command;Why the caps are part of this change, not extra policy
Converting an absolute attempt deadline into a progress-based one removes the only bound sibling tool calls had.
Tool calls in one batch fan out as a goroutine per call under a single
sync.WaitGroup, all sharing one attempt context. Before this PR, every one of them was bounded by the fixed attempt deadline. Onceexecutecan reset that deadline, a long command keeps the attempt alive and a siblingwait_agentorcomputerwait inherits the extension for as long as the command runs.So tools that block on a model-supplied duration are bounded per call instead: a single
wait_agentblock to 10 minutes, acomputerwait action to 30 seconds (clamped before the millisecond conversion, so an oversized value cannot overflow). An absolute cap still bounds a single attempt regardless of progress.TestTaskAttemptContext_KicksExtendTheWindowSiblingToolsSharedemonstrates the mechanism: a sibling holding the shared attempt context outlives multiple idle windows purely because another call kept kicking, and ends only once the kicks stop.process_outputis unchanged in behavior. It only reads output the process manager already holds, so chatd can call it repeatedly without repeating the command.Attempt-watchdog cancellations classify as retryable, and the replay from #27370 re-attaches to the still-running process rather than starting it again.
If the caps still read as out of scope
I would rather split them into a follow-up than drop them, since removing them leaves sibling waits unbounded once the idle window lands. Happy to do that if you prefer.