From 816267be842039f4f312348698cc90330fdcc7bb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 06:50:13 +0000 Subject: [PATCH 1/6] fix: confirm CoderWorkspacePoolMember bind extensions by Coder's deadline and check the start build initiator Change-Id: I431d9dab372a21a885caef16659f5e9782a2fad3 Signed-off-by: Thomas Kosiewski --- .../coderworkspacepoolmember_types.go | 6 + api/v1alpha1/zz_generated.deepcopy.go | 4 + .../coder.com_coderworkspacepoolmembers.yaml | 8 + .../coderworkspacepoolmember_extend.go | 90 ++++++--- .../coderworkspacepoolmember_extend_test.go | 181 +++++++++++++----- .../coderworkspacepoolmember_ready.go | 17 +- .../coderworkspacepoolmember_ready_test.go | 10 + 7 files changed, 246 insertions(+), 70 deletions(-) diff --git a/api/v1alpha1/coderworkspacepoolmember_types.go b/api/v1alpha1/coderworkspacepoolmember_types.go index 8e6fac492..2aa683311 100644 --- a/api/v1alpha1/coderworkspacepoolmember_types.go +++ b/api/v1alpha1/coderworkspacepoolmember_types.go @@ -168,6 +168,12 @@ type CoderWorkspacePoolMemberStatus struct { // BindExtended is true after the one deadline extension at bind. // +optional BindExtended bool `json:"bindExtended,omitempty"` + // BindExtendDeadline is the deadline that the extension at bind asks + // Coder for. The controller stores it before the first extend request. + // While BindExtended is false, a Coder deadline at or after it proves + // that the extension happened. + // +optional + BindExtendDeadline *metav1.Time `json:"bindExtendDeadline,omitempty"` // DeleteBuildID is the ID of the current delete build. // +optional DeleteBuildID string `json:"deleteBuildID,omitempty"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 0cbe1c488..8ef8b9caf 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1588,6 +1588,10 @@ func (in *CoderWorkspacePoolMemberStatus) DeepCopyInto(out *CoderWorkspacePoolMe in, out := &in.LastExtendTime, &out.LastExtendTime *out = (*in).DeepCopy() } + if in.BindExtendDeadline != nil { + in, out := &in.BindExtendDeadline, &out.BindExtendDeadline + *out = (*in).DeepCopy() + } if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]v1.Condition, len(*in)) diff --git a/config/crd/dormant/coder.com_coderworkspacepoolmembers.yaml b/config/crd/dormant/coder.com_coderworkspacepoolmembers.yaml index 83ba19f8f..e78ca4db4 100644 --- a/config/crd/dormant/coder.com_coderworkspacepoolmembers.yaml +++ b/config/crd/dormant/coder.com_coderworkspacepoolmembers.yaml @@ -225,6 +225,14 @@ spec: CoderWorkspacePoolMemberStatus defines the observed state of a CoderWorkspacePoolMember. properties: + bindExtendDeadline: + description: |- + BindExtendDeadline is the deadline that the extension at bind asks + Coder for. The controller stores it before the first extend request. + While BindExtended is false, a Coder deadline at or after it proves + that the extension happened. + format: date-time + type: string bindExtended: description: BindExtended is true after the one deadline extension at bind. diff --git a/internal/controller/coderworkspacepoolmember_extend.go b/internal/controller/coderworkspacepoolmember_extend.go index 45741b110..857f3cd64 100644 --- a/internal/controller/coderworkspacepoolmember_extend.go +++ b/internal/controller/coderworkspacepoolmember_extend.go @@ -44,13 +44,19 @@ func (r *CoderWorkspacePoolMemberReconciler) keepWarm( if step != nil || err != nil { return step, err } - deadline := workspace.LatestBuild.Deadline + build := workspace.LatestBuild + deadline, target := build.Deadline, member.Status.BindExtendDeadline switch { case (!deadline.Valid || deadline.Time.IsZero()) && bound: member.Status.BindExtended = true return coderWait(reason, "Workspace %s has no autostop deadline, so the bind needs no extension.", name), nil case !deadline.Valid || deadline.Time.IsZero(): return coderWait(reason, "Workspace %s is ready and has no autostop deadline.", name), nil + case bound && target != nil && !deadline.Time.Before(target.Time): + // An earlier extend request took effect, even if its status write + // was lost. + member.Status.BindExtended = true + return poolMemberBound(member, deadline.Time), nil case !bound && deadline.Time.Sub(now) >= poolMemberExtendBefore: return coderWait(reason, "Every agent of workspace %s is ready.", name), nil } @@ -63,49 +69,89 @@ func (r *CoderWorkspacePoolMemberReconciler) keepWarm( return nil, err } // The reads can take a while, so the new deadline counts from a fresh - // clock reading. Coder refuses one less than about 30 minutes ahead. - now = r.Clock.Now() - extendTo := now.Add(max(time.Duration(template.DefaultTTLMillis)*time.Millisecond, poolMemberMinExtension)) - if bound && !deadline.Time.Before(extendTo) { + // clock reading. + extendTo := func() (time.Time, bool) { + return poolMemberExtendTo(r.Clock.Now(), time.Duration(template.DefaultTTLMillis)*time.Millisecond, build.MaxDeadline) + } + to, ok := extendTo() + switch { + case bound && target == nil && !deadline.Time.Before(to): // The workspace already has a full TTL: an extension would shorten it. member.Status.BindExtended = true return coderWait(reason, "Workspace %s is bound to claim %s. Its deadline %s needs no extension.", name, member.Spec.ClaimRef.Name, deadline.Time.UTC().Format(time.RFC3339)), nil - } - if bound { - // At most one extension at bind: store the flag before the request, - // so a lost status write after it never leads to a second one. + case !ok && bound: member.Status.BindExtended = true + return coderWait(reason, "Workspace %s is bound to claim %s. Its max deadline %s allows no extension.", + name, member.Spec.ClaimRef.Name, build.MaxDeadline.Time.UTC().Format(time.RFC3339)), nil + case !ok: + // Coder would refuse the request. Rule 2 replaces the member after + // Coder stops the workspace. + return coderWait("MaxDeadline", "Coder stops workspace %s at its max deadline %s, which allows no extension.", + name, build.MaxDeadline.Time.UTC().Format(time.RFC3339)), nil + } + if bound && target == nil { + // Store the target before the request: a later read then proves + // whether the extension happened, whatever status write is lost. + member.Status.BindExtendDeadline = &metav1.Time{Time: to} if err := r.writeStatus(ctx, member, before); err != nil { return nil, err } *before = *member.Status.DeepCopy() } - err = sdk.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{Deadline: extendTo}) + if bound { + // The write can take a while: count again from a fresh clock + // reading. Never ask for less than the stored target, so that a + // successful request always proves the extension. + to, _ = extendTo() + if stored := member.Status.BindExtendDeadline.Time; to.Before(stored) { + to = stored + } + } + err = sdk.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{Deadline: to}) switch status := coderStatus(err); { case err == nil: case status == http.StatusConflict: // The workspace changed after the read, for example a new build. // Read again at the next poll. - member.Status.BindExtended = false return coderWait(reason, "Coder could not extend workspace %s now: %s", name, coderErrorSummary(err)), nil case status == http.StatusBadRequest: - // For example the template's max deadline. The deadline stays, and - // a bound member does not try again. - member.Status.BindExtended = bound + // The new deadline was too soon for Coder's clock, or past a max + // deadline that changed after the read. The deadline stays, and a + // bound member does not try again. + if bound { + member.Status.BindExtended = true + } else { + reason = "ExtendRefused" + } return coderWait(reason, "Coder refused to extend workspace %s: %s", name, coderErrorSummary(err)), nil default: - // Only a request that provably changed nothing is sent again. After - // an uncertain result, the bind extension counts as done. - if createHadNoEffect(err) { - member.Status.BindExtended = false - } + // The next poll reads the deadline: a bound member then sees + // whether the request took effect. return coderStepUnavailable("extend workspace", err), nil } - deadlineText := extendTo.UTC().Format(time.RFC3339) if bound { - return coderWait(reason, "Workspace %s is bound to claim %s. Its deadline is now %s.", name, member.Spec.ClaimRef.Name, deadlineText), nil + member.Status.BindExtended = true + return poolMemberBound(member, to), nil } member.Status.LastExtendTime = &metav1.Time{Time: now} - return coderWait(reason, "The controller extended the deadline of workspace %s to %s.", name, deadlineText), nil + return coderWait(reason, "The controller extended the deadline of workspace %s to %s.", name, to.UTC().Format(time.RFC3339)), nil +} + +// poolMemberExtendTo returns the deadline of an extension at now: now plus +// the template's default TTL, at least 30 minutes, at most the build's max +// deadline, in whole seconds as status stores them. It reports false when +// the max deadline leaves no extension that Coder accepts. +func poolMemberExtendTo(now time.Time, ttl time.Duration, maxDeadline codersdk.NullTime) (time.Time, bool) { + now = now.Truncate(time.Second) + to := now.Add(max(ttl, poolMemberMinExtension)).Truncate(time.Second) + if maxDeadline.Valid && !maxDeadline.Time.IsZero() && to.After(maxDeadline.Time) { + to = maxDeadline.Time.Truncate(time.Second) + } + return to, !to.Before(now.Add(poolMemberMinExtension)) +} + +func poolMemberBound(member *coderv1alpha1.CoderWorkspacePoolMember, deadline time.Time) *coderStep { + return coderWait("Bound", "Workspace %s is bound to claim %s. Its deadline is now %s.", + member.Status.WorkspaceName, member.Spec.ClaimRef.Name, deadline.UTC().Format(time.RFC3339)) } diff --git a/internal/controller/coderworkspacepoolmember_extend_test.go b/internal/controller/coderworkspacepoolmember_extend_test.go index 183d6ef3d..f88c07cd3 100644 --- a/internal/controller/coderworkspacepoolmember_extend_test.go +++ b/internal/controller/coderworkspacepoolmember_extend_test.go @@ -71,14 +71,29 @@ func TestPoolMemberKeepsUnclaimedWorkspaceWarm(t *testing.T) { requireReady(t, e.reconcileMember(t, key, 2), "") require.Equal(t, 1, e.fake.requestCount(routeExtendWorkspace), "the new deadline is an hour away") - // The template's max deadline refuses the extension: the deadline stays. - e.fake.setTemplateSchedule(e.tplID, time.Hour, 70*time.Minute) + // The template's max deadline bounds the extension, then ends it: the + // member never sends a request that Coder must refuse. + e.fake.setTemplateSchedule(e.tplID, time.Hour, 90*time.Minute) + e.clock.SetTime(started) + key, workspaceID = e.readyMember(t) + extends := e.fake.requestCount(routeExtendWorkspace) + e.clock.SetTime(started.Add(46 * time.Minute)) + requireReady(t, e.reconcileMember(t, key, 1), "extended the deadline") + require.True(t, started.Add(90*time.Minute).Equal(e.deadline(t, workspaceID).Time)) + for _, at := range []time.Duration{76 * time.Minute, 77 * time.Minute} { + e.clock.SetTime(started.Add(at)) + requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "MaxDeadline", "allows no extension") + } + require.Equal(t, extends+1, e.fake.requestCount(routeExtendWorkspace), "no request after the max deadline") + + // Any other refusal, for example after clock skew, is a wait that names it. e.clock.SetTime(started) key, workspaceID = e.readyMember(t) before := e.deadline(t, workspaceID) + e.fake.failNext(routeExtendWorkspace, fakeFault{Status: 400}) e.clock.SetTime(started.Add(46 * time.Minute)) member = e.reconcileMember(t, key, 1) - requireReady(t, member, "Coder refused to extend") + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "ExtendRefused", "Coder refused to extend") require.Nil(t, member.Status.LastExtendTime) require.Equal(t, before, e.deadline(t, workspaceID)) @@ -86,7 +101,7 @@ func TestPoolMemberKeepsUnclaimedWorkspaceWarm(t *testing.T) { e.fake.setTemplateSchedule(e.tplID, 0, 0) e.clock.SetTime(started) key, _ = e.readyMember(t) - extends := e.fake.requestCount(routeExtendWorkspace) + extends = e.fake.requestCount(routeExtendWorkspace) e.clock.SetTime(started.Add(10 * time.Hour)) requireReady(t, e.reconcileMember(t, key, 1), "no autostop deadline") require.Equal(t, extends, e.fake.requestCount(routeExtendWorkspace)) @@ -113,6 +128,7 @@ func TestPoolMemberExtendsOnceAtBind(t *testing.T) { member := e.reconcileMember(t, key, 1) requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "could not extend") require.False(t, member.Status.BindExtended) + require.True(t, member.Status.BindExtendDeadline.Time.Equal(e.clock.Now().Add(time.Hour))) require.Equal(t, time.Minute, e.lastStep.RequeueAfter) member = e.reconcileMember(t, key, 1) requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "bound to claim build-1") @@ -138,18 +154,20 @@ func TestPoolMemberExtendsOnceAtBind(t *testing.T) { require.Equal(t, before, e.deadline(t, workspaceID)) require.Equal(t, 2, e.fake.requestCount(routeExtendWorkspace)) - // The new deadline counts from a clock reading right before the request, - // not from the start of a slow reconcile. + // The new deadline counts from a clock reading after the status write + // that stores the target, not from the start of a slow reconcile. e.clock.SetTime(started) e.fake.setTemplateSchedule(e.tplID, 10*time.Minute, 0) key, workspaceID = e.readyMember(t) bind(t, key) late := started.Add(90 * time.Second) e.clock.SetTime(late) // The fake checks the 29-minute rule against this time. - r := &controller.CoderWorkspacePoolMemberReconciler{Client: k8sClient, Scheme: scheme, Clock: &lateClock{times: []time.Time{started, late}}} + r := &controller.CoderWorkspacePoolMemberReconciler{Client: k8sClient, Scheme: scheme, Clock: &lateClock{times: []time.Time{started, started, late}}} _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) require.NoError(t, err) - requireMember(t, e.reconcileMember(t, key, 0), coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "bound to claim build-1") + member = e.reconcileMember(t, key, 0) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "bound to claim build-1") + require.True(t, member.Status.BindExtendDeadline.Time.Equal(started.Add(30*time.Minute))) require.True(t, e.deadline(t, workspaceID).Time.Equal(late.Add(30*time.Minute))) // Without a deadline, the bind needs no extension. @@ -164,67 +182,124 @@ func TestPoolMemberExtendsOnceAtBind(t *testing.T) { require.Equal(t, extends, e.fake.requestCount(routeExtendWorkspace)) } -// lostBindWriteClient fails the status write that reports a done bind -// extension, like a conflict right after the extend request. -type lostBindWriteClient struct{ client.Client } +// failStatusWriteClient fails every member status write that match accepts, +// after the write landed when committed is true (like a crash right after +// it), or instead of it (like a conflict). +type failStatusWriteClient struct { + client.Client + match func(*coderv1alpha1.CoderWorkspacePoolMember) bool + committed bool +} -func (c lostBindWriteClient) Status() client.SubResourceWriter { - return lostBindWriter{c.Client.Status()} +func (c failStatusWriteClient) Status() client.SubResourceWriter { + return failStatusWriter{c.Client.Status(), c} } -type lostBindWriter struct{ client.SubResourceWriter } +type failStatusWriter struct { + client.SubResourceWriter + c failStatusWriteClient +} -func (w lostBindWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - if m, ok := obj.(*coderv1alpha1.CoderWorkspacePoolMember); ok && strings.Contains(m.Status.Message, "is bound to claim") { - return apierrors.NewConflict(schema.GroupResource{Group: "coder.com", Resource: "coderworkspacepoolmembers"}, m.Name, errors.New("stale object")) +func (w failStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + m, ok := obj.(*coderv1alpha1.CoderWorkspacePoolMember) + if !ok || !w.c.match(m) { + return w.SubResourceWriter.Update(ctx, obj, opts...) } - return w.SubResourceWriter.Update(ctx, obj, opts...) + if w.c.committed { + if err := w.SubResourceWriter.Update(ctx, obj, opts...); err != nil { + return err + } + } + return apierrors.NewConflict(schema.GroupResource{Group: "coder.com", Resource: "coderworkspacepoolmembers"}, m.Name, errors.New("stale object")) } -// TestPoolMemberBindExtensionAtMostOnce: the member stores bindExtended -// before the extend request, so neither a lost status write nor an uncertain -// answer leads to a second extension. A request that provably changed -// nothing is sent again. -func TestPoolMemberBindExtensionAtMostOnce(t *testing.T) { +// TestPoolMemberBindExtensionConfirmed: the member stores the target deadline +// before the extend request, and only Coder's deadline proves the extension. +// So no lost status write, crash, or uncertain answer leads to a second +// extension, or to a bind without one. +func TestPoolMemberBindExtensionConfirmed(t *testing.T) { t.Parallel() e := newPoolMemberEnv(t) e.fake.setTemplateSchedule(e.tplID, time.Hour, 0) started := e.clock.Now() - bound := func(t *testing.T) types.NamespacedName { + bound := func(t *testing.T) (types.NamespacedName, uuid.UUID) { t.Helper() e.clock.SetTime(started) - key, _ := e.readyMember(t) + key, workspaceID := e.readyMember(t) member := e.reconcileMember(t, key, 0) member.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: "build-1", UID: uuid.NewString()} require.NoError(t, k8sClient.Update(e.ctx, member)) e.clock.SetTime(started.Add(10 * time.Minute)) - return key + return key, workspaceID + } + failWrite := func(t *testing.T, key types.NamespacedName, c failStatusWriteClient) { + t.Helper() + c.Client = k8sClient + r := &controller.CoderWorkspacePoolMemberReconciler{Client: c, Scheme: scheme, Clock: e.clock} + _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) + require.True(t, apierrors.IsConflict(err), "the failed write ends the reconcile: %v", err) + } + // extendedAfter: a minute later, the next reconciles send wantRequests + // extend requests and end with the bind extended to the stored target + // or later. + extendedAfter := func(t *testing.T, key types.NamespacedName, workspaceID uuid.UUID, wantRequests int) { + t.Helper() + requests := e.fake.requestCount(routeExtendWorkspace) + e.clock.SetTime(e.clock.Now().Add(time.Minute)) + member := e.reconcileMember(t, key, 2) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "Its deadline is now") + require.True(t, member.Status.BindExtended) + require.Equal(t, requests+wantRequests, e.fake.requestCount(routeExtendWorkspace)) + require.False(t, e.deadline(t, workspaceID).Time.Before(member.Status.BindExtendDeadline.Time)) } - key := bound(t) - r := &controller.CoderWorkspacePoolMemberReconciler{Client: lostBindWriteClient{k8sClient}, Scheme: scheme, Clock: e.clock} - _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) - require.True(t, apierrors.IsConflict(err), "the lost write ends the reconcile: %v", err) + // The status write after a successful request is lost: the read proves + // the extension, so no second request. + key, workspaceID := bound(t) + failWrite(t, key, failStatusWriteClient{match: func(m *coderv1alpha1.CoderWorkspacePoolMember) bool { return m.Status.BindExtended }}) require.Equal(t, 1, e.fake.requestCount(routeExtendWorkspace)) - // A minute later, now + TTL is past the new deadline, so only the - // stored flag stops a second extension. - e.clock.SetTime(e.clock.Now().Add(time.Minute)) - require.True(t, e.reconcileMember(t, key, 2).Status.BindExtended) - require.Equal(t, 1, e.fake.requestCount(routeExtendWorkspace), "no second extension after a lost status write") + extendedAfter(t, key, workspaceID, 0) - key = bound(t) - e.fake.failNext(routeExtendWorkspace, fakeFault{Status: 502}) - member := e.reconcileMember(t, key, 1) - requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "CoderUnavailable", "extend workspace") - require.True(t, member.Status.BindExtended) - e.reconcileMember(t, key, 2) - require.Equal(t, 2, e.fake.requestCount(routeExtendWorkspace), "no second extension after an uncertain answer") + // A crash right after the target is stored, before the request: the + // next reconcile still extends. + key, workspaceID = bound(t) + requests := e.fake.requestCount(routeExtendWorkspace) + failWrite(t, key, failStatusWriteClient{committed: true, match: func(m *coderv1alpha1.CoderWorkspacePoolMember) bool { + return m.Status.BindExtendDeadline != nil && !m.Status.BindExtended + }}) + require.Equal(t, requests, e.fake.requestCount(routeExtendWorkspace)) + extendedAfter(t, key, workspaceID, 1) + + // A 409, then a lost status write: nothing claims the extension. + key, workspaceID = bound(t) + e.fake.failNext(routeExtendWorkspace, fakeFault{Status: 409}) + failWrite(t, key, failStatusWriteClient{match: func(m *coderv1alpha1.CoderWorkspacePoolMember) bool { + return strings.Contains(m.Status.Message, "could not extend") + }}) + extendedAfter(t, key, workspaceID, 1) + + // The next read decides an uncertain answer: the member sends the + // request again when it had no effect, never when it had one. + for _, c := range []struct { + fault fakeFault + wantRequests int + }{{fakeFault{Status: 502}, 1}, {fakeFault{Status: 502, AfterCommit: true}, 0}, {fakeFault{Status: 429}, 1}} { + key, workspaceID = bound(t) + e.fake.failNext(routeExtendWorkspace, c.fault) + member := e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "CoderUnavailable", "extend workspace") + require.False(t, member.Status.BindExtended) + extendedAfter(t, key, workspaceID, c.wantRequests) + } - key = bound(t) - e.fake.failNext(routeExtendWorkspace, fakeFault{Status: 429}) - require.False(t, e.reconcileMember(t, key, 1).Status.BindExtended, "a 429 changed nothing") - requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "Its deadline is now") - require.Equal(t, 4, e.fake.requestCount(routeExtendWorkspace)) + // Coder refuses the bind extension: the member does not try again. + key, _ = bound(t) + e.fake.failNext(routeExtendWorkspace, fakeFault{Status: 400}) + requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "Coder refused to extend") + requests = e.fake.requestCount(routeExtendWorkspace) + e.clock.SetTime(e.clock.Now().Add(time.Minute)) + require.True(t, e.reconcileMember(t, key, 2).Status.BindExtended) + require.Equal(t, requests, e.fake.requestCount(routeExtendWorkspace)) } // TestPoolMemberGuardsEveryExtension: the read right before an extension @@ -255,5 +330,17 @@ func TestPoolMemberGuardsEveryExtension(t *testing.T) { e.fake.addExternalBuild(workspaceID, uuid.New(), codersdk.WorkspaceTransitionStop) e.clock.SetTime(started.Add(50 * time.Minute)) requireMemberFailed(t, e.reconcileMember(t, key, 1), "ChangedExternally", "Someone else started build 2", metav1.ConditionFalse) + + // A status writer names a start build that the owner, not the operator, + // started on the member's own kwp- workspace. + e.clock.SetTime(started) + key, workspaceID = e.readyMember(t) + build := e.fake.addExternalBuild(workspaceID, e.tester, codersdk.WorkspaceTransitionStart) + e.fake.setBuildJob(build.ID, codersdk.ProvisionerJobSucceeded) + member = e.reconcileMember(t, key, 0) + member.Status.StartBuildID = build.ID.String() + require.NoError(t, k8sClient.Status().Update(e.ctx, member)) + e.clock.SetTime(started.Add(50 * time.Minute)) + requireMemberFailed(t, e.reconcileMember(t, key, 1), "OwnershipUnknown", "Nothing proves that the controller started build 2", metav1.ConditionUnknown) require.Zero(t, e.fake.requestCount(routeExtendWorkspace)) } diff --git a/internal/controller/coderworkspacepoolmember_ready.go b/internal/controller/coderworkspacepoolmember_ready.go index 35c057975..afc1f2038 100644 --- a/internal/controller/coderworkspacepoolmember_ready.go +++ b/internal/controller/coderworkspacepoolmember_ready.go @@ -146,7 +146,10 @@ func checkPoolMemberReadiness( // identity (plan 6.2, identity guard) against values that no status writer // can change: the owner from the immutable spec, the kwp- name derived from // the UID, and the template. It also requires that the latest build is the -// start build. A nil step means the workspace is this member's own. +// start build, and that the operator user started it with the pinned version: +// a status writer can name another build or another kwp- workspace, but not +// one that the operator built for this member. A nil step means the workspace +// is this member's own. func readOwnPoolMemberWorkspace( ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, ) (codersdk.Workspace, *coderStep, error) { @@ -156,6 +159,13 @@ func readOwnPoolMemberWorkspace( return codersdk.Workspace{}, nil, fmt.Errorf("assertion failed: pool member %s/%s has workspace ID %q and start build ID %q", member.Namespace, member.Name, member.Status.WorkspaceID, member.Status.StartBuildID) } + operator, err := sdk.User(ctx, codersdk.Me) + if err != nil { + return codersdk.Workspace{}, coderStepUnavailable("get operator user", err), nil + } + if operator.ID == uuid.Nil { + return codersdk.Workspace{}, nil, &coderAnswerError{msg: "assertion failed: Coder answered the operator user without an ID"} + } workspace, err := sdk.Workspace(ctx, workspaceID) // Only 410 proves deletion: Coder also answers 404 when the caller may // not read the workspace. @@ -180,5 +190,10 @@ func readOwnPoolMemberWorkspace( if build.ID.String() != member.Status.StartBuildID { return workspace, coderFail("ChangedExternally", "Someone else started build %d of workspace %s.", build.BuildNumber, name), nil } + if build.InitiatorID != operator.ID || build.Transition != codersdk.WorkspaceTransitionStart || build.TemplateVersionID.String() != spec.TemplateVersionID { + step := coderFail("OwnershipUnknown", "Nothing proves that the controller started build %d of workspace %s.", build.BuildNumber, name) + step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "OwnershipUnknown", Message: step.message} + return workspace, step, nil + } return workspace, nil, nil } diff --git a/internal/controller/coderworkspacepoolmember_ready_test.go b/internal/controller/coderworkspacepoolmember_ready_test.go index d5fb8333d..11482eadc 100644 --- a/internal/controller/coderworkspacepoolmember_ready_test.go +++ b/internal/controller/coderworkspacepoolmember_ready_test.go @@ -111,6 +111,16 @@ func TestPoolMemberReadinessFailures(t *testing.T) { member.Status.WorkspaceID, member.Status.StartBuildID = other.ID.String(), other.LatestBuild.ID.String() require.NoError(t, k8sClient.Status().Update(e.ctx, member)) }, reason: "OwnershipUnknown", message: "is not the workspace of this member", deleted: metav1.ConditionUnknown}, + // A status writer names a start build that the owner, not the + // operator, started on the member's own kwp- workspace. + {name: "start build by someone else in status", setup: func(t *testing.T, key types.NamespacedName, workspaceID, _ uuid.UUID) { + build := e.fake.addExternalBuild(workspaceID, e.tester, codersdk.WorkspaceTransitionStart) + e.fake.setBuildJob(build.ID, codersdk.ProvisionerJobSucceeded) + e.fake.setAgents(build.ID, agent("main", codersdk.WorkspaceAgentConnected, codersdk.WorkspaceAgentLifecycleReady)) + member := e.reconcileMember(t, key, 0) + member.Status.StartBuildID = build.ID.String() + require.NoError(t, k8sClient.Status().Update(e.ctx, member)) + }, reason: "OwnershipUnknown", message: "Nothing proves that the controller started build 2", deleted: metav1.ConditionUnknown}, } { key, workspaceID, buildID := e.createdMember(t, nil) tc.setup(t, key, workspaceID, buildID) From 10dc11cb158c86a4dec919ce04547c8455587816 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 06:53:52 +0000 Subject: [PATCH 2/6] fix: treat a full TTL as a done bind extension, and test a max deadline at bind Change-Id: I5b3d8d99f493c268d3b692baed5be0bec89e39a6 Signed-off-by: Thomas Kosiewski --- .../controller/coderworkspacepoolmember_extend.go | 2 +- .../coderworkspacepoolmember_extend_test.go | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/controller/coderworkspacepoolmember_extend.go b/internal/controller/coderworkspacepoolmember_extend.go index 857f3cd64..70d237705 100644 --- a/internal/controller/coderworkspacepoolmember_extend.go +++ b/internal/controller/coderworkspacepoolmember_extend.go @@ -75,7 +75,7 @@ func (r *CoderWorkspacePoolMemberReconciler) keepWarm( } to, ok := extendTo() switch { - case bound && target == nil && !deadline.Time.Before(to): + case bound && !deadline.Time.Before(to): // The workspace already has a full TTL: an extension would shorten it. member.Status.BindExtended = true return coderWait(reason, "Workspace %s is bound to claim %s. Its deadline %s needs no extension.", diff --git a/internal/controller/coderworkspacepoolmember_extend_test.go b/internal/controller/coderworkspacepoolmember_extend_test.go index f88c07cd3..6fcf43877 100644 --- a/internal/controller/coderworkspacepoolmember_extend_test.go +++ b/internal/controller/coderworkspacepoolmember_extend_test.go @@ -170,12 +170,25 @@ func TestPoolMemberExtendsOnceAtBind(t *testing.T) { require.True(t, member.Status.BindExtendDeadline.Time.Equal(started.Add(30*time.Minute))) require.True(t, e.deadline(t, workspaceID).Time.Equal(late.Add(30*time.Minute))) + // A max deadline too near for any extension ends the bind extension + // without a request that Coder must refuse. + e.clock.SetTime(started) + e.fake.setTemplateSchedule(e.tplID, time.Hour, 70*time.Minute) + key, _ = e.readyMember(t) + e.clock.SetTime(started.Add(50 * time.Minute)) + bind(t, key) + extends := e.fake.requestCount(routeExtendWorkspace) + member = e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "allows no extension") + require.True(t, member.Status.BindExtended) + require.Equal(t, extends, e.fake.requestCount(routeExtendWorkspace)) + // Without a deadline, the bind needs no extension. e.fake.setTemplateSchedule(e.tplID, 0, 0) e.clock.SetTime(time.Now().Truncate(time.Second)) key, _ = e.readyMember(t) bind(t, key) - extends := e.fake.requestCount(routeExtendWorkspace) + extends = e.fake.requestCount(routeExtendWorkspace) member = e.reconcileMember(t, key, 1) requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Bound", "no autostop deadline") require.True(t, member.Status.BindExtended) From 5be09049db4026292e7d40735f3b920f14414f29 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 07:09:37 +0000 Subject: [PATCH 3/6] fix: record the clock reading of the extension as lastExtendTime Change-Id: I17d4226fdbc86196ea985c14a760c32b1917fa73 Signed-off-by: Thomas Kosiewski --- .../controller/coderworkspacepoolmember_extend.go | 3 ++- .../coderworkspacepoolmember_extend_test.go | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/controller/coderworkspacepoolmember_extend.go b/internal/controller/coderworkspacepoolmember_extend.go index 70d237705..c6b7b963e 100644 --- a/internal/controller/coderworkspacepoolmember_extend.go +++ b/internal/controller/coderworkspacepoolmember_extend.go @@ -71,7 +71,8 @@ func (r *CoderWorkspacePoolMemberReconciler) keepWarm( // The reads can take a while, so the new deadline counts from a fresh // clock reading. extendTo := func() (time.Time, bool) { - return poolMemberExtendTo(r.Clock.Now(), time.Duration(template.DefaultTTLMillis)*time.Millisecond, build.MaxDeadline) + now = r.Clock.Now() // lastExtendTime is this reading. + return poolMemberExtendTo(now, time.Duration(template.DefaultTTLMillis)*time.Millisecond, build.MaxDeadline) } to, ok := extendTo() switch { diff --git a/internal/controller/coderworkspacepoolmember_extend_test.go b/internal/controller/coderworkspacepoolmember_extend_test.go index 6fcf43877..b82e95cff 100644 --- a/internal/controller/coderworkspacepoolmember_extend_test.go +++ b/internal/controller/coderworkspacepoolmember_extend_test.go @@ -70,6 +70,17 @@ func TestPoolMemberKeepsUnclaimedWorkspaceWarm(t *testing.T) { require.Equal(t, e.clock.Now().Add(time.Hour).UTC(), e.deadline(t, workspaceID).Time.UTC()) requireReady(t, e.reconcileMember(t, key, 2), "") require.Equal(t, 1, e.fake.requestCount(routeExtendWorkspace), "the new deadline is an hour away") + // After slow reads, lastExtendTime is the clock reading the new + // deadline counts from. + late := started.Add(time.Hour + 41*time.Minute) + e.clock.SetTime(late) + r := &controller.CoderWorkspacePoolMemberReconciler{Client: k8sClient, Scheme: scheme, Clock: &lateClock{times: []time.Time{started.Add(time.Hour + 40*time.Minute), late}}} + _, err := r.Reconcile(e.ctx, ctrl.Request{NamespacedName: key}) + require.NoError(t, err) + member = e.reconcileMember(t, key, 0) + requireReady(t, member, "extended the deadline") + require.True(t, member.Status.LastExtendTime.Time.Equal(late)) + require.True(t, e.deadline(t, workspaceID).Time.Equal(late.Add(time.Hour))) // The template's max deadline bounds the extension, then ends it: the // member never sends a request that Coder must refuse. From 2ae8d792ad49851fd007087bf30cd47d90fef943 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 07:03:20 +0000 Subject: [PATCH 4/6] feat: watch unclaimed ready CoderWorkspacePoolMember workspaces for use, agent loss, and dormancy Change-Id: I5ffdeabd7e32c5a6236ea4e6f25a7db99d536db3 Signed-off-by: Thomas Kosiewski --- .../coderworkspacepoolmember_controller.go | 9 +- ...oderworkspacepoolmember_controller_test.go | 4 +- .../coderworkspacepoolmember_extend.go | 17 +-- .../coderworkspacepoolmember_ready.go | 63 ++++++--- .../coderworkspacepoolmember_watch.go | 130 ++++++++++++++++++ .../coderworkspacepoolmember_watch_test.go | 122 ++++++++++++++++ 6 files changed, 307 insertions(+), 38 deletions(-) create mode 100644 internal/controller/coderworkspacepoolmember_watch.go create mode 100644 internal/controller/coderworkspacepoolmember_watch_test.go diff --git a/internal/controller/coderworkspacepoolmember_controller.go b/internal/controller/coderworkspacepoolmember_controller.go index fb9ef9baf..302ec951d 100644 --- a/internal/controller/coderworkspacepoolmember_controller.go +++ b/internal/controller/coderworkspacepoolmember_controller.go @@ -131,8 +131,10 @@ func (r *CoderWorkspacePoolMemberReconciler) Reconcile(ctx context.Context, req } keepBackoff = step.rateLimited && !step.failed switch { - case step.failed, member.Spec.ClaimRef != nil && member.Status.BindExtended: - // The watch rules of a bound member (plan 6.2 step 11 rule 3) come later. + case step.failed, member.Spec.ClaimRef != nil && member.Status.BindExtended, + meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed): + // The watch rules of a bound member (plan 6.2 step 11 rule 3) come + // with claims. A used member waits for a person to delete it. return ctrl.Result{}, nil case member.Status.Phase == coderv1alpha1.CoderWorkspacePoolMemberPhaseReady: return ctrl.Result{RequeueAfter: r.retryAfter(req.NamespacedName, member, step)}, nil @@ -445,6 +447,9 @@ func applyPoolMemberStep(member *coderv1alpha1.CoderWorkspacePoolMember, now tim switch { case !step.failed && member.Status.ReadyTime != nil: member.Status.Phase, ready = coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, metav1.ConditionTrue + if step.reason == "AgentNotReady" { + ready = metav1.ConditionFalse // Watch rule 3: the phase stays. + } case !step.failed && poolMemberMayHaveWorkspace(member): member.Status.Phase = coderv1alpha1.CoderWorkspacePoolMemberPhaseCreating case !step.failed: diff --git a/internal/controller/coderworkspacepoolmember_controller_test.go b/internal/controller/coderworkspacepoolmember_controller_test.go index 6f5d2ca52..8067a6db6 100644 --- a/internal/controller/coderworkspacepoolmember_controller_test.go +++ b/internal/controller/coderworkspacepoolmember_controller_test.go @@ -148,8 +148,8 @@ func requireMember(t *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, require.Equal(t, phase, member.Status.Phase, member.Status.Message) require.Equal(t, reason, member.Status.Reason, member.Status.Message) require.Contains(t, member.Status.Message, messagePart) - require.Equal(t, phase == coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, - meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady), "Ready=True exactly in phase Ready") + require.Equal(t, phase == coderv1alpha1.CoderWorkspacePoolMemberPhaseReady && reason != "AgentNotReady", + meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady), "Ready=True exactly in phase Ready, unless an agent is not ready") require.True(t, controllerutil.ContainsFinalizer(member, coderv1alpha1.CoderWorkspacePoolMemberCleanupFinalizer)) } diff --git a/internal/controller/coderworkspacepoolmember_extend.go b/internal/controller/coderworkspacepoolmember_extend.go index c6b7b963e..dbd05c31d 100644 --- a/internal/controller/coderworkspacepoolmember_extend.go +++ b/internal/controller/coderworkspacepoolmember_extend.go @@ -24,26 +24,19 @@ const ( ) // keepWarm runs the deadline steps of a ready member (plan 6.2 -// step 10 rule 5 and step 11 rule 1). An unclaimed workspace gets a new +// step 10 rule 5 and step 11 rule 1) on workspace, which the caller just +// read and proved to be this member's. An unclaimed workspace gets a new // deadline when its deadline is near. A bound workspace gets one extension at -// bind, so the claimant has a full TTL, and then never another. Each -// extension follows a fresh read that proves the workspace is this member's. +// bind, so the claimant has a full TTL, and then never another. func (r *CoderWorkspacePoolMemberReconciler) keepWarm( - ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, before *coderv1alpha1.CoderWorkspacePoolMemberStatus, now time.Time, + ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, before *coderv1alpha1.CoderWorkspacePoolMemberStatus, + workspace codersdk.Workspace, now time.Time, ) (*coderStep, error) { name, bound := member.Status.WorkspaceName, member.Spec.ClaimRef != nil - if bound && member.Status.BindExtended { - // The one extension at bind is done: Coder's own autostop applies. - return coderWait(member.Status.Reason, "%s", member.Status.Message), nil - } reason := "WorkspaceReady" // A successful read replaces an earlier wait. if bound { reason = "Bound" } - workspace, step, err := readOwnPoolMemberWorkspace(ctx, sdk, member) - if step != nil || err != nil { - return step, err - } build := workspace.LatestBuild deadline, target := build.Deadline, member.Status.BindExtendDeadline switch { diff --git a/internal/controller/coderworkspacepoolmember_ready.go b/internal/controller/coderworkspacepoolmember_ready.go index afc1f2038..aec9c13e5 100644 --- a/internal/controller/coderworkspacepoolmember_ready.go +++ b/internal/controller/coderworkspacepoolmember_ready.go @@ -36,7 +36,7 @@ func (r *CoderWorkspacePoolMemberReconciler) readWorkspace( case member.Status.ReadyTime == nil: return checkPoolMemberReadiness(ctx, sdk, member, now) default: - return r.keepWarm(ctx, sdk, member, before, now) + return r.watchReady(ctx, sdk, member, before, now) } } @@ -142,29 +142,40 @@ func checkPoolMemberReadiness( return coderWait("WorkspaceReady", "Every agent of workspace %s is ready.", name), nil } -// readOwnPoolMemberWorkspace reads the member's workspace and checks its -// identity (plan 6.2, identity guard) against values that no status writer -// can change: the owner from the immutable spec, the kwp- name derived from -// the UID, and the template. It also requires that the latest build is the -// start build, and that the operator user started it with the pinned version: -// a status writer can name another build or another kwp- workspace, but not -// one that the operator built for this member. A nil step means the workspace -// is this member's own. +// readOwnPoolMemberWorkspace reads the member's workspace, proves it is the +// member's own, and requires that the latest build is the start build +// (readPoolMemberWorkspace and poolMemberStartBuildStep). A nil step means +// the workspace is this member's own and still runs its start build. func readOwnPoolMemberWorkspace( ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, ) (codersdk.Workspace, *coderStep, error) { + workspace, operatorID, step, err := readPoolMemberWorkspace(ctx, sdk, member) + if step != nil || err != nil { + return workspace, step, err + } + return workspace, poolMemberStartBuildStep(member, workspace, operatorID), nil +} + +// readPoolMemberWorkspace reads the member's workspace and checks its +// identity (plan 6.2, identity guard) against values that no status writer +// can change: the owner from the immutable spec, the kwp- name derived from +// the UID, and the template. It also returns the operator user's ID. A nil +// step means the workspace is this member's own. +func readPoolMemberWorkspace( + ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, +) (codersdk.Workspace, uuid.UUID, *coderStep, error) { spec, name := member.Spec, member.Status.WorkspaceName workspaceID, err := uuid.Parse(member.Status.WorkspaceID) if err != nil || workspaceID == uuid.Nil || member.Status.StartBuildID == "" { - return codersdk.Workspace{}, nil, fmt.Errorf("assertion failed: pool member %s/%s has workspace ID %q and start build ID %q", + return codersdk.Workspace{}, uuid.Nil, nil, fmt.Errorf("assertion failed: pool member %s/%s has workspace ID %q and start build ID %q", member.Namespace, member.Name, member.Status.WorkspaceID, member.Status.StartBuildID) } operator, err := sdk.User(ctx, codersdk.Me) if err != nil { - return codersdk.Workspace{}, coderStepUnavailable("get operator user", err), nil + return codersdk.Workspace{}, uuid.Nil, coderStepUnavailable("get operator user", err), nil } if operator.ID == uuid.Nil { - return codersdk.Workspace{}, nil, &coderAnswerError{msg: "assertion failed: Coder answered the operator user without an ID"} + return codersdk.Workspace{}, uuid.Nil, nil, &coderAnswerError{msg: "assertion failed: Coder answered the operator user without an ID"} } workspace, err := sdk.Workspace(ctx, workspaceID) // Only 410 proves deletion: Coder also answers 404 when the caller may @@ -173,27 +184,35 @@ func readOwnPoolMemberWorkspace( case coderStatus(err) == http.StatusGone: step := coderFail("WorkspaceDeletedExternally", "Workspace %s was deleted by someone else.", name) step.deleted = &metav1.Condition{Status: metav1.ConditionTrue, Reason: "DeletedExternally", Message: step.message} - return workspace, step, nil + return workspace, operator.ID, step, nil case err != nil: - return workspace, coderStepUnavailable("get workspace", err), nil + return workspace, operator.ID, coderStepUnavailable("get workspace", err), nil } - build := workspace.LatestBuild if err := errors.Join(coderAnswerFor("workspace", workspaceID.String(), workspace.ID.String()), - coderAnswerFor("latest build workspace", workspaceID.String(), build.WorkspaceID.String())); err != nil { - return workspace, nil, err + coderAnswerFor("latest build workspace", workspaceID.String(), workspace.LatestBuild.WorkspaceID.String())); err != nil { + return workspace, operator.ID, nil, err } if workspace.OwnerID.String() != spec.OwnerID || workspace.Name != name || workspace.TemplateID.String() != spec.TemplateID { step := coderFail("OwnershipUnknown", "Workspace %s is not the workspace of this member.", workspaceID) step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "OwnershipUnknown", Message: step.message} - return workspace, step, nil + return workspace, operator.ID, step, nil } + return workspace, operator.ID, nil, nil +} + +// poolMemberStartBuildStep requires that the latest build of the member's +// workspace is its start build, and that the operator user started it with +// the pinned version: a status writer can name another build or another +// kwp- workspace, but not one that the operator built for this member. +func poolMemberStartBuildStep(member *coderv1alpha1.CoderWorkspacePoolMember, workspace codersdk.Workspace, operatorID uuid.UUID) *coderStep { + build, name := workspace.LatestBuild, member.Status.WorkspaceName if build.ID.String() != member.Status.StartBuildID { - return workspace, coderFail("ChangedExternally", "Someone else started build %d of workspace %s.", build.BuildNumber, name), nil + return coderFail("ChangedExternally", "Someone else started build %d of workspace %s.", build.BuildNumber, name) } - if build.InitiatorID != operator.ID || build.Transition != codersdk.WorkspaceTransitionStart || build.TemplateVersionID.String() != spec.TemplateVersionID { + if build.InitiatorID != operatorID || build.Transition != codersdk.WorkspaceTransitionStart || build.TemplateVersionID.String() != member.Spec.TemplateVersionID { step := coderFail("OwnershipUnknown", "Nothing proves that the controller started build %d of workspace %s.", build.BuildNumber, name) step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "OwnershipUnknown", Message: step.message} - return workspace, step, nil + return step } - return workspace, nil, nil + return nil } diff --git a/internal/controller/coderworkspacepoolmember_watch.go b/internal/controller/coderworkspacepoolmember_watch.go new file mode 100644 index 000000000..5c1b11c35 --- /dev/null +++ b/internal/controller/coderworkspacepoolmember_watch.go @@ -0,0 +1,130 @@ +package controller + +import ( + "context" + "fmt" + "slices" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +const ( + // poolMemberAgentLostAfter is how long an agent of a ready unclaimed + // workspace can stay not ready before the member fails. + poolMemberAgentLostAfter = 5 * time.Minute + // poolMemberTemplateWriteWindow: a last_used_at this close to the + // template's updated_at comes from a template schedule update. + poolMemberTemplateWriteWindow = 5 * time.Second +) + +// watchReady runs the rules of a ready member on one read of its workspace: +// the watch rules of an unclaimed member in plan order (6.2 step 10), and +// the deadline steps (step 10 rule 5, step 11 rule 1). +func (r *CoderWorkspacePoolMemberReconciler) watchReady( + ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, before *coderv1alpha1.CoderWorkspacePoolMemberStatus, now time.Time, +) (*coderStep, error) { + bound := member.Spec.ClaimRef != nil + if (bound && member.Status.BindExtended) || meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed) { + // The one extension at bind is done, or someone used the workspace + // without a claim: Coder's own autostop applies, and the controller + // never fails a used member. + return coderWait(member.Status.Reason, "%s", member.Status.Message), nil + } + workspace, operatorID, step, err := readPoolMemberWorkspace(ctx, sdk, member) + if step != nil || err != nil { + return step, err + } + name := member.Status.WorkspaceName + if !bound { + // Rule 1 runs first, so the later rules never fail a used member. + if step, err := r.checkPoolMemberUse(ctx, sdk, member, workspace, now); step != nil || err != nil { + return step, err + } + } + if step := poolMemberStartBuildStep(member, workspace, operatorID); step != nil { + return step, nil // Rule 2: a stop, an update, or an autostop. + } + if !bound { + if step := poolMemberAgentStep(member, workspace.LatestBuild, now); step != nil { + return step, nil // Rule 3. + } + if workspace.DormantAt != nil || workspace.DeletingAt != nil { + return coderFail("Dormant", "Workspace %s is dormant.", name), nil // Rule 4. + } + } + return r.keepWarm(ctx, sdk, member, before, workspace, now) +} + +// checkPoolMemberUse applies watch rule 1: a last_used_at later than the +// baseline from the ready read means that someone used the workspace without +// a claim. A template-wide write is not use: it moves the baseline instead. +// A nil step means the member is still unused. +func (r *CoderWorkspacePoolMemberReconciler) checkPoolMemberUse( + ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember, workspace codersdk.Workspace, now time.Time, +) (*coderStep, error) { + baseline := member.Status.ReadyLastUsedAt + if baseline == nil { + return nil, fmt.Errorf("assertion failed: ready pool member %s/%s has no readyLastUsedAt", member.Namespace, member.Name) + } + // Status stores whole seconds. Both times come from Coder's clock. + lastUsed := workspace.LastUsedAt.Truncate(time.Second) + if !lastUsed.After(baseline.Time) { + return nil, nil + } + template, err := sdk.Template(ctx, uuid.MustParse(member.Spec.TemplateID)) + if err != nil { + return coderStepUnavailable("get template", err), nil + } + if err := coderAnswerFor("template", member.Spec.TemplateID, template.ID.String()); err != nil { + return nil, err + } + // A template schedule update sets one last_used_at on every workspace of + // the template, and moves the template's updated_at (plan 3.1 item 7). + templateWide := lastUsed.Sub(template.UpdatedAt).Abs() <= poolMemberTemplateWriteWindow + if !templateWide { + // The template can change again after the write. Another unclaimed + // member of the pool that moved its baseline to the same value + // shows the same write. + members := &coderv1alpha1.CoderWorkspacePoolMemberList{} + label := coderv1alpha1.CoderWorkspacePoolSelectorLabel + if err := r.List(ctx, members, client.InNamespace(member.Namespace), client.MatchingLabels{label: member.Labels[label]}); err != nil { + return nil, fmt.Errorf("list members of pool %s/%s: %w", member.Namespace, member.Spec.PoolName, err) + } + templateWide = slices.ContainsFunc(members.Items, func(m coderv1alpha1.CoderWorkspacePoolMember) bool { + return m.UID != member.UID && m.Spec.ClaimRef == nil && m.Status.ReadyLastUsedAt != nil && m.Status.ReadyLastUsedAt.Time.Equal(lastUsed) + }) + } + if templateWide { + member.Status.ReadyLastUsedAt = &metav1.Time{Time: lastUsed} + return nil, nil + } + step := coderWait("Used", "Someone used workspace %s at %s without a claim. The pool releases it.", member.Status.WorkspaceName, lastUsed.UTC().Format(time.RFC3339)) + setPoolMemberCondition(member, now, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed, metav1.ConditionTrue, step.reason, step.message) + return step, nil +} + +// poolMemberAgentStep applies watch rule 3 to the start build: an agent that +// is no longer connected and ready makes the member not ready, and after +// poolMemberAgentLostAfter it fails. A nil step means every agent is ready. +func poolMemberAgentStep(member *coderv1alpha1.CoderWorkspacePoolMember, build codersdk.WorkspaceBuild, now time.Time) *coderStep { + step := workspaceBuildReadiness(build, member.Status.WorkspaceName) + if step == nil { + return nil + } + since := now + if c := meta.FindStatusCondition(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady); c != nil && + c.Status == metav1.ConditionFalse && c.Reason == "AgentNotReady" { + since = c.LastTransitionTime.Time + } + if now.Sub(since) >= poolMemberAgentLostAfter { + return coderFail("AgentLost", "%s It has not been ready since %s.", step.message, since.UTC().Format(time.RFC3339)) + } + return coderWait("AgentNotReady", "%s", step.message) +} diff --git a/internal/controller/coderworkspacepoolmember_watch_test.go b/internal/controller/coderworkspacepoolmember_watch_test.go new file mode 100644 index 000000000..7ac639215 --- /dev/null +++ b/internal/controller/coderworkspacepoolmember_watch_test.go @@ -0,0 +1,122 @@ +package controller_test + +import ( + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +func requireUsed(t *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, used bool) { + t.Helper() + require.Equal(t, used, meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed), member.Status.Message) +} + +// TestPoolMemberWatchesUse is mutation check 9: Coder sets last_used_at when +// it serves a create from one of its prebuilds, so only a value later than +// the one at ready is use. A used member never fails and is never extended. +func TestPoolMemberWatchesUse(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + e.fake.setTemplateSchedule(e.tplID, time.Hour, 0) + started := e.clock.Now() + key, workspaceID, buildID := e.createdMember(t, nil) + // A claimed prebuild: last_used_at is the claim, after created_at. + e.fake.markUsed(workspaceID, started.Add(2*time.Minute)) + e.fake.setBuildJob(buildID, codersdk.ProvisionerJobSucceeded) + e.fake.setAgents(buildID, agent("main", codersdk.WorkspaceAgentConnected, codersdk.WorkspaceAgentLifecycleReady)) + member := e.reconcileMember(t, key, 1) + require.Equal(t, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, member.Status.Phase, member.Status.Message) + e.clock.SetTime(started.Add(10 * time.Minute)) + requireUsed(t, e.reconcileMember(t, key, 2), false) + + // Use after ready: Used=True, and the later rules never fail the member. + e.fake.markUsed(workspaceID, started.Add(11*time.Minute+500*time.Millisecond)) + e.fake.addExternalBuild(workspaceID, uuid.New(), codersdk.WorkspaceTransitionStop) + e.clock.SetTime(started.Add(50 * time.Minute)) + member = e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Used", "without a claim") + requireUsed(t, member, true) + require.Zero(t, e.lastStep.RequeueAfter) + reads := e.fake.requestCount(routeWorkspace) + requireMember(t, e.reconcileMember(t, key, 2), coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "Used", "without a claim") + require.Equal(t, reads, e.fake.requestCount(routeWorkspace), "a used member reads nothing") + require.Zero(t, e.fake.requestCount(routeExtendWorkspace)) +} + +// TestPoolMemberTemplateWideWrite is mutation check 10: a template schedule +// update sets one last_used_at on every workspace of the template. That is +// not use, even after the template changed again. +func TestPoolMemberTemplateWideWrite(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + started := e.clock.Now() + first, _ := e.readyMember(t) + second, secondWorkspace := e.readyMember(t) + e.clock.SetTime(started.Add(10 * time.Minute)) + e.fake.touchTemplateWorkspaces(e.tplID, e.clock.Now()) + member := e.reconcileMember(t, first, 1) + requireUsed(t, member, false) + require.True(t, member.Status.ReadyLastUsedAt.Time.Equal(e.clock.Now()), "the baseline moves to the template-wide value") + + // The template changes again: the other member's baseline shows the write. + e.fake.mu.Lock() + tpl := e.fake.templates[e.tplID] + tpl.UpdatedAt = started.Add(time.Hour) + e.fake.templates[e.tplID] = tpl + e.fake.mu.Unlock() + requireUsed(t, e.reconcileMember(t, second, 1), false) + + // Real use is still use. + e.fake.markUsed(secondWorkspace, started.Add(12*time.Minute)) + requireUsed(t, e.reconcileMember(t, second, 1), true) + requireUsed(t, e.reconcileMember(t, first, 1), false) +} + +// TestPoolMemberWatchRules: an agent that stays not ready for 5 minutes, and +// a dormant workspace, fail an unclaimed ready member. +func TestPoolMemberWatchRules(t *testing.T) { + t.Parallel() + e := newPoolMemberEnv(t) + started := e.clock.Now() + key, workspaceID := e.readyMember(t) + buildID := uuid.MustParse(e.reconcileMember(t, key, 0).Status.StartBuildID) + ready := agent("main", codersdk.WorkspaceAgentConnected, codersdk.WorkspaceAgentLifecycleReady) + lost := agent("main", codersdk.WorkspaceAgentDisconnected, codersdk.WorkspaceAgentLifecycleReady) + requireNotReady := func(t *testing.T, at time.Duration) { + t.Helper() + e.clock.SetTime(started.Add(at)) + member := e.reconcileMember(t, key, 1) + requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseReady, "AgentNotReady", lost.ID.String()) + require.False(t, meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady)) + } + + // A short disconnect only makes the member not ready. + e.fake.setAgents(buildID, lost) + requireNotReady(t, time.Minute) + e.fake.setAgents(buildID, ready) + e.clock.SetTime(started.Add(3 * time.Minute)) + member := e.reconcileMember(t, key, 1) + requireReady(t, member, "is ready") + require.True(t, meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionReady)) + + // The 5 minutes count from the first read that found the agent lost. + e.fake.setAgents(buildID, lost) + requireNotReady(t, 4*time.Minute) + requireNotReady(t, 9*time.Minute-time.Second) + e.clock.SetTime(started.Add(9 * time.Minute)) + requireMemberFailed(t, e.reconcileMember(t, key, 1), "AgentLost", "has not been ready since", metav1.ConditionFalse) + + // Dormancy fails the member. + e.clock.SetTime(started) + key, workspaceID = e.readyMember(t) + dormant := started.Add(time.Minute) + e.fake.setDormant(workspaceID, &dormant, nil) + requireMemberFailed(t, e.reconcileMember(t, key, 1), "Dormant", "is dormant", metav1.ConditionFalse) +} From 5611da932553e8b29518f378d53dc8a498041212 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 07:08:00 +0000 Subject: [PATCH 5/6] fix: drop the self check that a member's earlier baseline already rules out Change-Id: I92de215625e1d4d7d6d86d29040b457b2e6997cf Signed-off-by: Thomas Kosiewski --- internal/controller/coderworkspacepoolmember_watch.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/controller/coderworkspacepoolmember_watch.go b/internal/controller/coderworkspacepoolmember_watch.go index 5c1b11c35..2f55f67a3 100644 --- a/internal/controller/coderworkspacepoolmember_watch.go +++ b/internal/controller/coderworkspacepoolmember_watch.go @@ -91,14 +91,15 @@ func (r *CoderWorkspacePoolMemberReconciler) checkPoolMemberUse( if !templateWide { // The template can change again after the write. Another unclaimed // member of the pool that moved its baseline to the same value - // shows the same write. + // shows the same write. This member's own baseline is earlier, so + // it never matches. members := &coderv1alpha1.CoderWorkspacePoolMemberList{} label := coderv1alpha1.CoderWorkspacePoolSelectorLabel if err := r.List(ctx, members, client.InNamespace(member.Namespace), client.MatchingLabels{label: member.Labels[label]}); err != nil { return nil, fmt.Errorf("list members of pool %s/%s: %w", member.Namespace, member.Spec.PoolName, err) } templateWide = slices.ContainsFunc(members.Items, func(m coderv1alpha1.CoderWorkspacePoolMember) bool { - return m.UID != member.UID && m.Spec.ClaimRef == nil && m.Status.ReadyLastUsedAt != nil && m.Status.ReadyLastUsedAt.Time.Equal(lastUsed) + return m.Spec.ClaimRef == nil && m.Status.ReadyLastUsedAt != nil && m.Status.ReadyLastUsedAt.Time.Equal(lastUsed) }) } if templateWide { From d35c7505071aba026a8913bf73a12e62f0c79343 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 07:32:00 +0000 Subject: [PATCH 6/6] test: drop an unused workspace ID in the watch rules test Change-Id: I7571e9c29a9e2093098ec6b3279e1230663091ba Signed-off-by: Thomas Kosiewski --- internal/controller/coderworkspacepoolmember_watch_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/coderworkspacepoolmember_watch_test.go b/internal/controller/coderworkspacepoolmember_watch_test.go index 7ac639215..2dc33918d 100644 --- a/internal/controller/coderworkspacepoolmember_watch_test.go +++ b/internal/controller/coderworkspacepoolmember_watch_test.go @@ -85,7 +85,7 @@ func TestPoolMemberWatchRules(t *testing.T) { t.Parallel() e := newPoolMemberEnv(t) started := e.clock.Now() - key, workspaceID := e.readyMember(t) + key, _ := e.readyMember(t) buildID := uuid.MustParse(e.reconcileMember(t, key, 0).Status.StartBuildID) ready := agent("main", codersdk.WorkspaceAgentConnected, codersdk.WorkspaceAgentLifecycleReady) lost := agent("main", codersdk.WorkspaceAgentDisconnected, codersdk.WorkspaceAgentLifecycleReady) @@ -115,7 +115,7 @@ func TestPoolMemberWatchRules(t *testing.T) { // Dormancy fails the member. e.clock.SetTime(started) - key, workspaceID = e.readyMember(t) + key, workspaceID := e.readyMember(t) dormant := started.Add(time.Minute) e.fake.setDormant(workspaceID, &dormant, nil) requireMemberFailed(t, e.reconcileMember(t, key, 1), "Dormant", "is dormant", metav1.ConditionFalse)