Repository navigation
ci(.github): stop each doc-check review when its run ends - #31720
Draft
nickvigilante wants to merge 3 commits into
Draft
nickvigilante wants to merge 3 commits into
nickvigilante wants to merge 3 commits into
Conversation
doc-check reviews on the local model took a median of 150 minutes, because doc-check alone kept about 40 review chats active and each model step slowed from about 20 seconds to over 200. Most of that load was wasted: a review kept running after its job stopped waiting, the next push queued another full review behind it, and a workspace left running past an hour held an expired GitHub token, so 159 of 244 reviews ended on a 401. A new push now cancels the PR's run in progress. A new script stops the PR's review chat and its sub-agents, drops their queued messages, and stops the review workspace. The job runs it before the review, so the review starts on an idle chat and a freshly started workspace, and after every run, including a cancelled one. The wait drops to 25 minutes, and a review past it is stopped and fails the job. Refs DOCS-1478
Contributor
Author
|
/coder-agents-review |
Contributor
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 4 | Last posted: Round 4, 43 findings (2 P2, 10 P3, 22 P4, 9 Nit), COMMENT. Review Finding inventoryFinding inventory: PR #31720Findings
Contested and acknowledgedCRF-4 (P4, doc-check-stop-review.sh:147) - CR strip handles input that cannot occur
CRF-6 (P4, doc-check-stop-review.sh:174) - Exact-name workspace lookup duplicated in three places
CRF-12 (P3, doc-check-stop-review.sh:62) - paused chat reported stopped
CRF-14 (P3, doc-check.yaml:490) - No bound on concurrent reviews across PRs
CRF-18 (P4, doc-check-stop-review.sh:178) - Prepare restarts only the workspace found by name
Round logRound 1Netero first pass: 1 P3, 5 P4, 1 Nit, 1 Note. No P0, panel proceeds. Law not run (effective additions 522). Round 2 updateBLOCKED. CRF-14 deferred without a ticket ("planned as a follow-up", no issue linked). No review. Churn guard: 24 addressed, 1 acknowledged (CRF-18), 2 contested (CRF-4, CRF-6), 1 deferred without ticket (CRF-14). Status cells unchanged until the panel runs. Head: 5a270bc. Round 3Churn guard PROCEED: CRF-14 now tracked in DOCS-1480. Head unchanged (5a270bc). Round type: fix round. Since the last panel round (R1, 38970da) no files were added or renamed, no PR goal changed, effective additions grew 117; Round 4 updateBLOCKED. CRF-10 silent: the R3 re-raise (runs tested 38970da) has no author reply, its thread was resolved without a response, and Testing still cites only runs 41729412601 and 41731313388 with no tested SHA. No review. Churn guard: 15 of 16 open findings addressed in bf1aa8b (CRF-48, 50 to 63), status cells unchanged until the panel verifies. Head: bf1aa8b. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Contributor
There was a problem hiding this comment.
This PR adds a script that stops a PR's doc-check chat, sub-agents, and workspace before and after each run, sets cancel-in-progress: true, and cuts the wait to 25 minutes. Findings: 2 P2, 7 P3, 13 P4, 6 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
.github/workflows/doc-check.yaml:215: the dispatch URL check accepts anyowner/repo, but the workspace name uses only the PR number, so a dispatch forother/repo/pull/123starts, and now also stops, the workspace of coder/coder PR 123..github/workflows/test-*.sh: no CI job runs the repository's shell test scripts, including this PR's, so each one stops matching the code it tests without anyone seeing a failure.coderd/x/chatd/agentadmission.go:12: chatd's only concurrent-agent cap is deployment-wide and fixed (5 root, 10 sub-agent slots, or off), so a deployment cannot cap one model or one bot user.coder-workflow-bottemplate (not in this repository): the workspace mints its GitHub installation token once at startup, so any session past one hour gets 401s..github/workflows/doc-check.yaml:352: the workspace start wait repeats the same 180-second budget inline (seq 36,sleep 5), so a change to the script's budget leaves this one unchanged..github/workflows/doc-check.yaml:342: Prepare the review workspace starts the workspace only forstop/stoppedand prints "Workspace state is usable." for a failed stop or start build, so the review runs on a workspace whose last build failed.
Notes:
- The
test-go-pgandtest-go-pg-17failures are not caused by this diff;.github/actions/test-go-pg/action.yamlreferences none of the changed files. - The cleanup step's worst case (3 interrupt rounds of 60 s plus a 180 s workspace wait, about 360 s) is longer than the 5 minutes GitHub gives
always()steps after a cancel. The newer run's Stop the previous review repeats the stop, so the result is a force-terminated step in the cancelled run's log. - The job now runs a checked-out script with
DOC_CHECK_CODER_SESSION_TOKEN. For same-repositorypull_requestruns the PR author already controls the workflow file at the same ref, so this adds no new capability.
🤖 This review was automatically generated with Coder Agents.
Key the concurrency group on the PR URL, so a manual run and a pull request run for one PR cancel each other instead of stopping each other's review. Keep forcing a review while the PR has the doc-check label, because a push now cancels the labeled run. In the script, interrupt again on the next poll, re-read chats after dropping their queues, warn on every failed request, give each request a 30-second limit, and treat only a stopped build as stopped: a failed start gets a stop, and a failed stop warns. Bound both cleanup steps, and say in the step summary when a review is stopped at the wait. Cover each warning branch, failed builds, and paused chats in the test, and keep transitions in the fake API for two reads, so the cases show that the script waits for them. Refs DOCS-1478
Contributor
Author
|
/coder-agents-review |
Contributor
There was a problem hiding this comment.
Re-review of 5a270bc. The author replied to every round 1 finding, and the commit addresses most of them. One finding is deferred with no ticket, so this round's review is blocked until it is resolved:
- No bound on concurrent reviews across PRs (
.github/workflows/doc-check.yaml:496). The reply says the cap is planned as a follow-up, once the post-PR concurrency is measured, but no issue is linked; DOCS-1478 is this PR's own ticket. This needs a human decision: link a ticket for the cap, or state that shipping without one is accepted, given that a burst of PRs past the model's slow-down point gives zero finished reviews.
The panel reviews the new commit, including the two contested findings and the acknowledged one, after this is resolved.
🤖 This review was automatically generated with Coder Agents.
Contributor
Author
|
/coder-agents-review |
Contributor
There was a problem hiding this comment.
Re-review of 5a270bc, which responds to round 1. Most round 1 findings are fixed, and reviewers confirmed the fixes in the code and tests. The carriage-return strip, the shared workspace lookup, the workspace restart by name, and leaving paused out of any_active are accepted as defended, and the concurrency cap is tracked in DOCS-1480. The end-to-end testing finding is reopened, because both dogfood runs ran 38970da, not this head. New findings: 3 P3, 9 P4, 3 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
.github/workflows/doc-check.yaml:304: the exact-namedoc-check-pr-<N>lookup exists in Prepare the review workspace, delete-workspace, anddoc-check-stop-review.sh, so a rename that misses one copy makes that lookup find no workspace..github/workflows/doc-check.yaml:220: the dispatch URL check accepts anyowner/repo, but the prompt hard-codescoder/coderand the workspace name uses only the PR number, so a dispatch for another repository reviews the wrong PR..github/workflows/doc-check.yaml:50: adoc-checklabel added to a merged PR passes the jobifand runs a review; if delete-workspace already deleteddoc-check-pr-<N>, the review creates a new one that no later event deletes.
🤖 This review was automatically generated with Coder Agents.
The stop script now ends within one time budget (STOP_BUDGET_SECONDS, 270 seconds by default) instead of per-loop sleep counts, and caps each request at the time left. It also stops a workspace whose earlier stop failed, re-reads a chat's queue after each delete, and looks up the doc-check-pr-<number> workspace only for this repository's pull requests. The workflow comments and summaries now match: any run in the group cancels the one in progress, the label reviews every later push, the timeout summary no longer repeats the wait or claims the stop worked, and both cleanup steps say what their 6-minute limit covers. Refs DOCS-1478
Contributor
Author
|
/coder-agents-review |
Contributor
There was a problem hiding this comment.
Re-review of bf1aa8b. The commit responds to every round 3 code finding except one. This round's review is blocked until that one gets a response:
- The end-to-end runs in Testing ran 38970da, not the code under review (
.github/workflows/doc-check.yaml:56). Its thread was resolved with no reply, and Testing still cites only runs 41729412601 and 41731313388 without naming the tested commit. Since then, 5a270bc and bf1aa8b rewrotestop_chatandstop_workspace, added the time budget, and moved the concurrency key to the PR URL; none of that has run on GitHub or the deployed Coder, and a manual run overlapping a pull request run has not run at any commit. Response needed: repeat the cancel and timeout runs at the current head (plus one manual run overlapping a pull request run) and name the SHA in Testing, or state in Testing that the runs tested 38970da and list what they do not cover.
The panel reviews bf1aa8b after this is resolved.
🤖 This review was automatically generated with Coder Agents.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
doc-check reviews on the local Solstice model take a median of 150 minutes, so most runs time out.
The skill isn't the bottleneck: at an unloaded step time, the same reviews would take a median of 13 minutes.
The model is slow because doc-check alone keeps a median of 40 review chats active, and most of that work is wasted:
This PR makes each run stop its review when the run ends, so active reviews never outnumber running jobs, and every review starts with a fresh token.
Linear: DOCS-1478
Changes
cancel-in-progress: true: a new push cancels the PR's run in progress instead of queueing behind it.New
.github/scripts/doc-check-stop-review.sh. For the PR's review chat (found by the chat action's labels), it:It ends within one time budget (
STOP_BUDGET_SECONDS, 270 seconds by default, a third of it for the chats), and caps each request at the time left.It looks up the
doc-check-pr-<N>workspace only for this repository's PRs, because the name has only the number.The job runs the script before the review (Stop the previous review and its workspace) and after every run, including a cancelled one (Stop the review and its workspace,
if: always()).The existing step that starts a stopped workspace then brings it up fresh, so the startup script mints a new installation token.
The concurrency group is keyed on the PR URL, so any new run for a PR, including a manual one, cancels the run in progress instead of stopping its review mid-run.
A PR with the
doc-checklabel gets a review on every push until the label is removed, because a push now cancels the labeled run in progress. The skip summary and the file header say so.The wait drops from 40 to 25 minutes, and
timeout-minutesfrom 50 to 45. A review past the wait is stopped, the job fails, and the step summary says the job tried to stop it.The old Stop the review workspace step is gone. It needed the action's
workspace-idoutput, which the action doesn't set on a timeout, so it never ran for the reviews that most needed it.Testing
End to end on dogfood, with two
workflow_dispatchruns of this branch against this PR:waiting5 seconds later, and stoppeddoc-check-pr-31720(22:30:47 to 22:31:10 UTC).ghcalls worked.waiting, and stopped the workspace.Neither review spawned a sub-agent, so interrupting sub-agents is covered only by the script test.
Run 2's review made only a few model steps in 25 minutes, because 35 doc-check chats from before this PR were still running on the model.
bash .github/scripts/doc-check-stop-review.test.shruns the real script against a fake Coder API (a fakecurlandsleeponPATH), in 24 cases.They cover running and idle reviews with sub-agents and queued messages,
requires_actionand paused chats (with and without a next queued message), a chat that ignores interrupts, workspace builds that are in progress or never finish, a failed start, a stop that failed in this run or an earlier one, every failed request, slow requests that use up the budget, the exact workspace name match, another repository's PR, and a PR URL with a trailing CR.In the fake, an interruption and a workspace stop each last two reads, so the cases show that the script waits for them.
Each of 14 deliberate breaks fails at least one case: no queue clearing, the queue read only once,
interruptingtreated as idle,requires_actionignored, no wait after the stop, a failed stop treated as stopped, no retry of an earlier failed stop, a substring name match, the name lookup run for another repository, no CR strip, no request time limit, request limits that ignore the budget, and no budget check in the chat or the workspace polls.No CI job runs shell tests, so this runs by hand on Linux, like the existing
.github/workflows/test-*.shscripts. The script needs bash 4 or later, whichubuntu-latesthas.shfmt, shellcheck,
make lint/actions/actionlint, andmake lint/actions/zizmorpass, with no zizmor findings.Decision log
Measurements: every
doc-check-botchat shared with the organization since Oct 30 17:54 UTC, read fromGET /api/v2/chatsand each chat's messages: 244 parent review turns and 468 sub-agent turns, each timed from its user message to its last message.Most of the tail is tool time spent waiting on sub-agents, which ran under the same load. Without that wait, the median is 9 minutes and p90 26.
The median is per review, so it is not the product of the median step count and the median step time.
47 hit it in their first 10 minutes, on a workspace that was still running from an earlier review. 112 hit it partway through, typically about 100 minutes in.
The 401s follow from slow reviews, because a review past an hour outlives its token, so they don't explain the step times.
Why stop instead of report: #31707 (closed) kept the 40-minute wait and turned a timeout on a running chat green. It assumed the review would still post later, but most of those reviews end on the 401, and the ones that keep running are the load.
Why interrupt sub-agents one by one: I found no code in
chatdthat cascades an interrupt from a parent chat to its children.Why
if: always()on the last step: GitHub runsalways()steps when a run is cancelled, so a run that a newer push cancels still stops its review before the newer run starts.The concurrency group is on the job, so a run whose job is skipped (for example, a label other than
doc-check) doesn't cancel the running review.The script ends within its 270-second budget, inside the 5 minutes GitHub gives an
always()step after a cancel. The steps' 6-minutetimeout-minutesonly catches a script that hangs past it.Tradeoffs Nick accepted: a new push discards the review of the older commit, and a review past 25 minutes is stopped and fails the check.
A push also cancels a manual run's review. On a PR whose changed files all skip review, the push run then skips too, so the workflow comment says to dispatch again after the push or add the label.
Next (separate PR): fewer steps per review. The workflow builds the review packet (PR metadata, diff, comments, changed docs) and runs Vale before the chat starts, and the skill uses one agent by default, with sub-agents only for large docs diffs.
Review findings not taken (accepted by the reviewer):
PR_URL: kept, because parsers here tolerate CRLF input by policy, and it costs one line.doc-check-pr-<N>: the chat createddoc-check-pr-31720in the end-to-end test, and a review whose workspace is stopped can start it itself.Follow-ups found in review (not changed here):
coder-workflow-bottemplate mints its GitHub installation token once at startup, so any session past an hour gets 401s.owner/repo, but the workspace name uses only the PR number. The stop script now skips the name lookup for other repositories; the other steps still use the number alone.Generated by Coder Agents on behalf of @nickvigilante.