Repository navigation
feat: make coder task send resumes paused tasks - #22203
DanielleMaywood merged 14 commits into
Conversation
a9dc59d to
065118a
Compare
DanielleMaywood
changed the title
coder task send resumes when task is pausedcoder task send resumes paused tasks
065118a to
cbdf745
Compare
Documentation CheckUpdates Needed
Automated review via Coder Tasks |
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Looks good in general, but caught a potential UX issue we haven't considered.
DanielleMaywood
changed the title
coder task send resumes paused taskscoder task send will resumes paused tasks
DanielleMaywood
changed the title
coder task send will resumes paused taskscoder task send resumes paused tasks
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
I think we still need a few adjustments here to avoid bad UX, wdyt?
| if err != nil { | ||
| return xerrors.Errorf("get task by id: %w", err) | ||
| } else if task.Status == codersdk.TaskStatusError || task.Status == codersdk.TaskStatusUnknown { | ||
| return xerrors.Errorf("task entered %s state while waiting for it to become active", task.Status) |
There was a problem hiding this comment.
I've unfortunately observed that error might pop up ephemerally during startup (I'm assuming it's the agentapi health check that fails long enough on startup that this happens):
I'm not sure what the best options here would be. Give a grace-period for the status to change? Hopefully something we can fix on the API/module side.
There was a problem hiding this comment.
Roughly how long of a grace period should we give (based on your observations)?
There was a problem hiding this comment.
Maybe 1-2 minutes is fine, but I'd still say 5 just in case. It really depends on how long the AI agent module takes to set itself up and what the agentapi health check grace period is set to (in this case, it was 63s as per agentapi module).
The template I'm using builds agentapi from a custom branch, so it's possible that pushed it just over the 63s limit and became unhealthy.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Caught a few more states we need to handle but otherwise I think this is looking good.
| // When we have a streaming Task API, this should be converted away from polling. | ||
|
|
||
| ticker := time.NewTicker(time.Second) | ||
| // TODO(DanielleMaywood): |
There was a problem hiding this comment.
Nit: What's the todo, to remove this once we have a better way to detect non-error states during startup?
| } | ||
|
|
||
| if task.Status == codersdk.TaskStatusActive { | ||
| switch task.Status { |
There was a problem hiding this comment.
I think we need to handle all states here, consider if the task gets paused in-between the checks. We all never go to error or unknown and active also don't be reached.
| return xerrors.Errorf("task has completed and cannot be sent input") | ||
| case codersdk.TaskStateFailed: | ||
| return xerrors.Errorf("task has failed and cannot be sent input") | ||
| case codersdk.TaskStateWorking: |
There was a problem hiding this comment.
At least theoretically, a task could be paused and this state will remain working indefinitely.
- Use backtick string for Long description to avoid escaped quotes. - Handle all states in waitForTaskIdle: add default branch that returns an error for unexpected app states (e.g. task paused externally) instead of polling indefinitely. - Handle all task statuses in waitForTaskReady: give Paused, Pending, and Unknown their own explicit cases instead of grouping Paused with Unknown. - Clarify TODO comment on the error grace period to explain it should be removed once the upstream agentapi health check no longer reports transient errors during startup. - Add test for waitForTaskReady detecting a task paused mid-poll. - Add test for waitForTaskIdle polling through working app state.
| if err != nil { | ||
| return xerrors.Errorf("get task by id: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
It seems I conveyed this poorly, but in my previous review, but my intent was that we repeat the waitForTaskReady logic here (i.e. extract it into a shared utility function). That way if the task suddenly becomes paused (or similar) after we've escaped waitForTaskReady, we don't end up looping forever (CurrentState may remain nil).
Actually, an inversion of this logic seems logical, waitForTaskReady (can be renamed) calls waitForTaskIdle which doesn't have any ticker or for-loop. Simple check. That way we can re-use the single grace-period in case the task goes into error state again at a later time.
Merges the two sequential polling functions into a single waitForTaskIdle that handles both build-watching and idle-polling. This prevents status changes (e.g. paused) from being missed between the two phases and avoids polling forever when CurrentState is nil during the transition.
There was a problem hiding this comment.
The core logic is pretty good now. But I found two more issues:
- No indication after workspace build of what we're doing
CurrentState == nilcheck will never resolve because because the agent isn't reporting an update on resume, and I suppose the MCP either isn't running or isn't reporting the initial idle state either 🤔
== ✔ Starting workspace [4826ms]
==> ⧗ Cleaning Up
=== ✔ Cleaning Up [46ms]
[stuck here for a long time, no updates, task ready, current state == nil]
Print "Waiting for task to become idle..." after the workspace build completes so users know what the CLI is doing. Add a 30s grace period for Active tasks with nil CurrentState. On resume the MCP may never report an initial app status, causing the polling loop to hang indefinitely. After the grace period expires we treat nil as idle and proceed with sending input.
Closes COCO-22 / Closes coder/internal#1266
The test for this was a little awkward to write, so a little refactoring was done. Happy to have another go if we think they're a little unwieldy.