fix: align autostart tests with persisted next_start_at#26037
Conversation
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 1 | Last posted: Round 1, 4 findings (1 P3, 2 Nit, 1 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. 1 P3, 2 Nit, 1 Note. Reviewed against 167ac7b..387fc54. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean, well-targeted fix. The core insight is right: tests should consume the system's computed schedule state rather than recomputing it from a different base time. The NextAutostartTick helper is minimal, well-guarded, and the enterprise fix at line 1548 (capturing ws from MustTransitionWorkspace) shows the author traced the data dependency correctly. The root cause analysis in the commit message is precise.
As Ryosuke put it: "don't recompute what the system already computed."
1 P3, 2 Nit, 1 Note. The P3 is a missed sibling that's subject to the same race this PR fixes.
coderd/autobuild/lifecycle_executor_test.go:1243
P3 [CRF-1] TestExecutorRequireActiveVersion still uses the old sched.Next(ws.LatestBuild.CreatedAt) pattern in a positive autostart test. Line 1247 asserts require.Len(t, stats.Transitions, 1), it uses a real provisioner with no mocked clock, and it's subject to the same straddling race as the eight sites this PR fixes. When the stop build at line 1235 straddles the hour boundary, the executor filters the workspace out and the test fails with an empty transitions map.
Fix: tickTime := coderdtest.NextAutostartTick(t, ws). The workspace is refreshed from MustTransitionWorkspace at line 1235, so ws.NextStartAt is populated.
This is the gap between "fix siblings" and "fix the one I found."
(Netero P3, Bisky P3, Mafu-san P2, Ryosuke P3, Kite P3)
🤖
🤖 This review was automatically generated with Coder Agents.
TestExecutorAutostartOK and its sibling positive autostart tests computed the autobuild tick from sched.Next(workspace.LatestBuild.CreatedAt). The server sets workspace.next_start_at from the build's completion time, and sched.Next is strictly monotonic, so when build creation and completion straddle the schedule's next fire time the test's tick is strictly less than the persisted next_start_at. The executor's eligibility query (next_start_at <= currentTick) then filters the workspace out and the test fails with an empty transitions map. Add coderdtest.NextAutostartTick(t, workspace) which returns *workspace.NextStartAt with a non-nil assertion, and use it in the affected positive autostart paths in coderd/autobuild, coderd, and enterprise/coderd. Negative tests, mocked-clock prebuild paths, and paths that assert no transition are unchanged. Closes coder/internal#1456 Generated with assistance from Coder Agents.
387fc54 to
90cfbf4
Compare
TestExecutorAutostartOKand its sibling positive autostart tests compute the autobuild tick fromsched.Next(workspace.LatestBuild.CreatedAt), but the server persistsnext_start_atfrom the build's completion time. When build creation and completion straddle the schedule's next fire time, the persisted value advances past the test's tick, the executor's eligibility query (next_start_at <= tick) drops the workspace, and the test fails with an empty transitions map. This surfaced in flaky test runs.Add
coderdtest.NextAutostartTick(t, workspace)which returns*workspace.NextStartAt, and use it across the affected positive autostart paths incoderd/autobuild,coderd, andenterprise/coderd.Closes coder/internal#1456
Generated with assistance from Coder Agents.