Repository navigation
refactor(coderd/x/chatd): delete the runner's unreachable task index - #30936
ibetitsmike wants to merge 3 commits into
Conversation
spawnTaskIfNeeded skipped a spawn when the active task already had the same kind, history version, and status. That guard cannot fire: processState cancels the active task, which clears activeTaskSet, before every spawn after the first. Delete the guard, the tasksByIndex map that only it read, the taskIndexKey type, and the taskRecord fields that only fed the index. Rename spawnTaskIfNeeded to spawnTask. Delete TestRunner_CleansUpOnOwnershipTakeover. Every step of it is also in TestWorker_CleanupStopsRoutingAndCancelsTasks, which then publishes the new state again and checks that no task starts.
…eTask activeTaskSet implies hasAcceptedState: acceptState runs right after every spawn and nothing resets hasAcceptedState. cancelActiveTask already returns when no task is active, so processState calls it unconditionally.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🚀 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". |
|
@codex review
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
Deletes the runner's secondary task index.
spawnTaskIfNeededskipped a spawn when the active task had the same kind, history version, and status, but that guard cannot fire. The PR removes the guard, thetasksByIndexmap that only it read, thetaskIndexKeytype, thetaskRecordfields that fed the index, and a redundant check beforecancelActiveTask. It renamesspawnTaskIfNeededtospawnTaskand deletesTestRunner_CleansUpOnOwnershipTakeover, whose steps all repeat inTestWorker_CleanupStopsRoutingAndCancelsTasks.Why it is safe
spawnTaskIfNeededsetsactiveTaskSet, andprocessStatecallsacceptState(which setshasAcceptedState) right after every spawn. On each later event,processStatecancels the active task, which clearsactiveTaskSet, before it spawns. So the guard's first operand is always false.activeTaskSetimplieshasAcceptedState, soprocessStatedrops ther.hasAcceptedState && r.activeTaskSetcheck and callscancelActiveTask, which already returns when no task is active.if r.activeTaskSet { panic(...) },go test ./coderd/x/chatd/andgo test ./coderd/ -run Chatpass.localLocksstill serializes a canceled task with its same-key replacement (TestRunner_SerializesReplacementTasksForSameHistoryAndStatus), and the task-ID map stays.git grepfinds no other use oftaskIndexKey,tasksByIndex, orspawnTaskIfNeeded.Each deleted check, its copy in the remaining test, and a mutant that fails both tests on
mainand the remaining test here:waitCallrunningspawns an interrupt taskrequireTaskCanceledrequestCleanupassertNoCallafter one more publishReview focus
The dead-guard argument depends on the order in
processState: cancel the active task, spawn, thenacceptState. ThenewRunnerandtaskRecordhunks otherwise only realign fields after gofmt.Validation
go test ./coderd/x/chatd/ -race -count=1 -run '^(TestRunner_|TestWorker)': 67 tests pass onmain, 66 on this branch.go test ./coderd/x/chatd/ -count=1: pass.go test -overlay): each fails as listed. A copy with a syntax error fails to build, so the overlay applies.make pre-commit) passed.