mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
fix(cli): use quartz clock in waitForTaskIdle for immediate first poll (#25648)
waitForTaskIdle used time.NewTicker(5s) which delays the first poll by 5 seconds. Debugger tracing proved the failure mechanism: on slow CI (Windows), the first poll at 5s sees "working" (idle patch has not landed due to goroutine scheduling), needs poll #2 at 10s, but the 25s context expires before it fires. Two changes: 1. Use r.clock.NewTicker (quartz) with time.Nanosecond initial interval and Reset(5s) for immediate first poll. Tests inject a mock clock via clitest.NewWithClock for deterministic control. 2. Rewrite WaitsForWorkingAppState test with quartz traps (NewTicker + TickerReset) for deterministic synchronization instead of racing goroutines. Fix PausedDuringWaitForReady sync point. Closes DEVEX-381
This commit is contained in:
committed by
GitHub
parent
8652ef3e3b
commit
7958ad6d04
+6
-3
@@ -11,6 +11,7 @@ import (
|
|||||||
|
|
||||||
"github.com/coder/coder/v2/cli/cliui"
|
"github.com/coder/coder/v2/cli/cliui"
|
||||||
"github.com/coder/coder/v2/codersdk"
|
"github.com/coder/coder/v2/codersdk"
|
||||||
|
"github.com/coder/quartz"
|
||||||
"github.com/coder/serpent"
|
"github.com/coder/serpent"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -107,7 +108,7 @@ func (r *RootCmd) taskSend() *serpent.Command {
|
|||||||
return xerrors.Errorf("task %q has status %s and cannot be sent input", display, task.Status)
|
return xerrors.Errorf("task %q has status %s and cannot be sent input", display, task.Status)
|
||||||
}
|
}
|
||||||
|
|
||||||
if err := waitForTaskIdle(ctx, inv, client, task, workspaceBuildID); err != nil {
|
if err := waitForTaskIdle(ctx, inv, r.clock, client, task, workspaceBuildID); err != nil {
|
||||||
return xerrors.Errorf("wait for task %q to be idle: %w", display, err)
|
return xerrors.Errorf("wait for task %q to be idle: %w", display, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -126,7 +127,7 @@ func (r *RootCmd) taskSend() *serpent.Command {
|
|||||||
// then polls until the task becomes active and its app state is idle.
|
// then polls until the task becomes active and its app state is idle.
|
||||||
// This merges build-watching and idle-polling into a single loop so
|
// This merges build-watching and idle-polling into a single loop so
|
||||||
// that status changes (e.g. paused) are never missed between phases.
|
// that status changes (e.g. paused) are never missed between phases.
|
||||||
func waitForTaskIdle(ctx context.Context, inv *serpent.Invocation, client *codersdk.Client, task codersdk.Task, workspaceBuildID uuid.UUID) error {
|
func waitForTaskIdle(ctx context.Context, inv *serpent.Invocation, clk quartz.Clock, client *codersdk.Client, task codersdk.Task, workspaceBuildID uuid.UUID) error {
|
||||||
if workspaceBuildID != uuid.Nil {
|
if workspaceBuildID != uuid.Nil {
|
||||||
if err := cliui.WorkspaceBuild(ctx, inv.Stdout, client, workspaceBuildID); err != nil {
|
if err := cliui.WorkspaceBuild(ctx, inv.Stdout, client, workspaceBuildID); err != nil {
|
||||||
return xerrors.Errorf("watch workspace build: %w", err)
|
return xerrors.Errorf("watch workspace build: %w", err)
|
||||||
@@ -162,13 +163,15 @@ func waitForTaskIdle(ctx context.Context, inv *serpent.Invocation, client *coder
|
|||||||
// TODO(DanielleMaywood):
|
// TODO(DanielleMaywood):
|
||||||
// When we have a streaming Task API, this should be converted
|
// When we have a streaming Task API, this should be converted
|
||||||
// away from polling.
|
// away from polling.
|
||||||
ticker := time.NewTicker(5 * time.Second)
|
const pollInterval = 5 * time.Second
|
||||||
|
ticker := clk.NewTicker(time.Nanosecond, "task_send", "poll")
|
||||||
defer ticker.Stop()
|
defer ticker.Stop()
|
||||||
for {
|
for {
|
||||||
select {
|
select {
|
||||||
case <-ctx.Done():
|
case <-ctx.Done():
|
||||||
return ctx.Err()
|
return ctx.Err()
|
||||||
case <-ticker.C:
|
case <-ticker.C:
|
||||||
|
ticker.Reset(pollInterval, "task_send", "poll")
|
||||||
task, err := client.TaskByID(ctx, task.ID)
|
task, err := client.TaskByID(ctx, task.ID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return xerrors.Errorf("get task by id: %w", err)
|
return xerrors.Errorf("get task by id: %w", err)
|
||||||
|
|||||||
+26
-4
@@ -21,6 +21,7 @@ import (
|
|||||||
"github.com/coder/coder/v2/codersdk/agentsdk"
|
"github.com/coder/coder/v2/codersdk/agentsdk"
|
||||||
"github.com/coder/coder/v2/pty/ptytest"
|
"github.com/coder/coder/v2/pty/ptytest"
|
||||||
"github.com/coder/coder/v2/testutil"
|
"github.com/coder/coder/v2/testutil"
|
||||||
|
"github.com/coder/quartz"
|
||||||
)
|
)
|
||||||
|
|
||||||
func Test_TaskSend(t *testing.T) {
|
func Test_TaskSend(t *testing.T) {
|
||||||
@@ -255,10 +256,10 @@ func Test_TaskSend(t *testing.T) {
|
|||||||
w := clitest.StartWithWaiter(t, inv)
|
w := clitest.StartWithWaiter(t, inv)
|
||||||
|
|
||||||
// Wait for the command to enter the build-watching phase
|
// Wait for the command to enter the build-watching phase
|
||||||
// of waitForTaskReady.
|
// of waitForTaskIdle.
|
||||||
pty.ExpectMatchContext(ctx, "Queued")
|
pty.ExpectMatchContext(ctx, "Waiting for task to become idle")
|
||||||
|
|
||||||
// Pause the task while waitForTaskReady is polling. Since
|
// Pause the task while waitForTaskIdle is polling. Since
|
||||||
// no agent is connected, the task stays initializing until
|
// no agent is connected, the task stays initializing until
|
||||||
// we pause it, at which point the status becomes paused.
|
// we pause it, at which point the status becomes paused.
|
||||||
pauseTask(ctx, t, setup.userClient, setup.task)
|
pauseTask(ctx, t, setup.userClient, setup.task)
|
||||||
@@ -284,14 +285,32 @@ func Test_TaskSend(t *testing.T) {
|
|||||||
Message: "busy",
|
Message: "busy",
|
||||||
}))
|
}))
|
||||||
|
|
||||||
|
// Set up mock clock and traps before starting the command.
|
||||||
|
mClock := quartz.NewMock(t)
|
||||||
|
tickTrap := mClock.Trap().NewTicker("task_send", "poll")
|
||||||
|
resetTrap := mClock.Trap().TickerReset("task_send", "poll")
|
||||||
|
|
||||||
// When: We send input while the app is working.
|
// When: We send input while the app is working.
|
||||||
inv, root := clitest.New(t, "task", "send", setup.task.Name, "some task input")
|
inv, root := clitest.NewWithClock(t, mClock, "task", "send", setup.task.Name, "some task input")
|
||||||
clitest.SetupConfig(t, setup.userClient, root)
|
clitest.SetupConfig(t, setup.userClient, root)
|
||||||
|
|
||||||
ctx := testutil.Context(t, testutil.WaitLong)
|
ctx := testutil.Context(t, testutil.WaitLong)
|
||||||
inv = inv.WithContext(ctx)
|
inv = inv.WithContext(ctx)
|
||||||
w := clitest.StartWithWaiter(t, inv)
|
w := clitest.StartWithWaiter(t, inv)
|
||||||
|
|
||||||
|
// Wait for ticker creation and release it.
|
||||||
|
tickCall := tickTrap.MustWait(ctx)
|
||||||
|
tickCall.MustRelease(ctx)
|
||||||
|
tickTrap.Close()
|
||||||
|
|
||||||
|
// Fire the immediate first poll (time.Nanosecond initial interval).
|
||||||
|
mClock.Advance(time.Nanosecond).MustWait(ctx)
|
||||||
|
|
||||||
|
// Wait for Reset (confirms first poll completed and saw "working").
|
||||||
|
resetCall := resetTrap.MustWait(ctx)
|
||||||
|
resetCall.MustRelease(ctx)
|
||||||
|
resetTrap.Close()
|
||||||
|
|
||||||
// Transition the app back to idle so waitForTaskIdle proceeds.
|
// Transition the app back to idle so waitForTaskIdle proceeds.
|
||||||
require.NoError(t, agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
|
require.NoError(t, agentClient.PatchAppStatus(ctx, agentsdk.PatchAppStatus{
|
||||||
AppSlug: "task-sidebar",
|
AppSlug: "task-sidebar",
|
||||||
@@ -299,6 +318,9 @@ func Test_TaskSend(t *testing.T) {
|
|||||||
Message: "ready",
|
Message: "ready",
|
||||||
}))
|
}))
|
||||||
|
|
||||||
|
// Fire second poll at the regular 5s interval.
|
||||||
|
mClock.Advance(5 * time.Second).MustWait(ctx)
|
||||||
|
|
||||||
// Then: The command should complete successfully.
|
// Then: The command should complete successfully.
|
||||||
require.NoError(t, w.Wait())
|
require.NoError(t, w.Wait())
|
||||||
})
|
})
|
||||||
|
|||||||
Reference in New Issue
Block a user