Skip to content

Commit de5d1d9

Browse files
authored
Make worker assignment checks atespace and actor name, also updated locks. (#411)
Current we only check & lock on actor name. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR
1 parent af1f005 commit de5d1d9

6 files changed

Lines changed: 165 additions & 8 deletions

File tree

cmd/ateapi/internal/controlapi/workflow.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ func (w *ActorWorkflow) ResumeActor(ctx context.Context, atespace, name string,
158158

159159
// Acquire lock and get the timeout context for the workflow
160160
// Lock TTL is 30 seconds, with 2 seconds padding for workflow timeout
161-
ctx, releaseLock, err := w.acquireActorLock(ctx, name, 30*time.Second, 2*time.Second)
161+
ctx, releaseLock, err := w.acquireActorLock(ctx, atespace, name, 30*time.Second, 2*time.Second)
162162
if err != nil {
163163
return nil, err
164164
}
@@ -188,7 +188,7 @@ func (w *ActorWorkflow) SuspendActor(ctx context.Context, atespace, name string)
188188

189189
// Acquire lock and get the timeout context for the workflow
190190
// Lock TTL is 30 seconds, with 2 seconds padding for workflow timeout
191-
ctx, releaseLock, err := w.acquireActorLock(ctx, name, 30*time.Second, 2*time.Second)
191+
ctx, releaseLock, err := w.acquireActorLock(ctx, atespace, name, 30*time.Second, 2*time.Second)
192192
if err != nil {
193193
return nil, err
194194
}
@@ -218,7 +218,7 @@ func (w *ActorWorkflow) PauseActor(ctx context.Context, atespace, name string) (
218218

219219
// Acquire lock and get the timeout context for the workflow
220220
// Lock TTL is 30 seconds, with 2 seconds padding for workflow timeout
221-
ctx, releaseLock, err := w.acquireActorLock(ctx, name, 30*time.Second, 2*time.Second)
221+
ctx, releaseLock, err := w.acquireActorLock(ctx, atespace, name, 30*time.Second, 2*time.Second)
222222
if err != nil {
223223
return nil, err
224224
}
@@ -238,8 +238,8 @@ func (w *ActorWorkflow) PauseActor(ctx context.Context, atespace, name string) (
238238
return state.Actor, nil
239239
}
240240

241-
func (w *ActorWorkflow) acquireActorLock(ctx context.Context, name string, ttl time.Duration, padding time.Duration) (context.Context, func(), error) {
242-
lockKey := "lock:actor:" + name
241+
func (w *ActorWorkflow) acquireActorLock(ctx context.Context, atespace, name string, ttl time.Duration, padding time.Duration) (context.Context, func(), error) {
242+
lockKey := "lock:actor:" + atespace + ":" + name
243243
lockValue := uuid.New().String()
244244

245245
// Create a child context for the workflow that expires BEFORE the lock

cmd/ateapi/internal/controlapi/workflow_pause.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ func (s *FinalizePausedStep) Execute(ctx context.Context, input *PauseInput, sta
186186
// Only free it if it still belongs to us
187187

188188
if wass := worker.Assignment; wass != nil {
189-
if wass.Actor.Name == input.ActorName {
189+
if wass.Actor.Atespace == input.Atespace && wass.Actor.Name == input.ActorName {
190190
worker.Assignment = nil
191191
err = s.store.UpdateWorker(ctx, worker, worker.Version)
192192
if err != nil {

cmd/ateapi/internal/controlapi/workflow_resume.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func (s *AssignWorkerStep) Execute(ctx context.Context, input *ResumeInput, stat
132132
if worker.Assignment == nil {
133133
continue
134134
}
135-
if worker.Assignment.Actor.Name != input.ActorName {
135+
if worker.Assignment.Actor.Atespace != input.Atespace || worker.Assignment.Actor.Name != input.ActorName {
136136
continue
137137
}
138138
eligible, err := isWorkerEligibleForActor(worker, state.ActorTemplate.Spec.SandboxClass, state.ActorTemplate.Spec.WorkerSelector, state.Actor.GetWorkerSelector())

cmd/ateapi/internal/controlapi/workflow_resume_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,15 @@
1515
package controlapi
1616

1717
import (
18+
"context"
1819
"testing"
20+
"time"
1921

22+
"github.com/agent-substrate/substrate/cmd/ateapi/internal/workercache"
2023
atev1alpha1 "github.com/agent-substrate/substrate/pkg/api/v1alpha1"
2124
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
25+
"google.golang.org/grpc/codes"
26+
"google.golang.org/grpc/status"
2227
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2328
)
2429

@@ -162,3 +167,52 @@ func TestIsWorkerEligibleForActor(t *testing.T) {
162167
})
163168
}
164169
}
170+
171+
func TestAssignWorkerStep_SkipsWorkerAssignedInOtherAtespace(t *testing.T) {
172+
ctx := context.Background()
173+
persistence := newTestPersistence(t)
174+
175+
// The only worker is held by a same-named actor in another atespace. It is
176+
// eligible for the template, so a name-only match would adopt it.
177+
worker := &ateapipb.Worker{
178+
WorkerNamespace: "worker-ns",
179+
WorkerPool: "pool",
180+
WorkerPod: "pod-1",
181+
SandboxClass: "gvisor",
182+
Assignment: &ateapipb.Assignment{
183+
Actor: &ateapipb.ActorRef{Atespace: "team-b", Name: "shared"},
184+
},
185+
}
186+
if err := persistence.CreateWorker(ctx, worker); err != nil {
187+
t.Fatalf("CreateWorker: %v", err)
188+
}
189+
190+
cacheCtx, cancel := context.WithCancel(ctx)
191+
defer cancel()
192+
wc := workercache.New(persistence, time.Minute)
193+
if err := wc.Start(cacheCtx); err != nil {
194+
t.Fatalf("workercache.Start: %v", err)
195+
}
196+
197+
step := &AssignWorkerStep{store: persistence, workerCache: wc}
198+
state := &ResumeState{
199+
Actor: &ateapipb.Actor{
200+
Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared"},
201+
},
202+
ActorTemplate: &atev1alpha1.ActorTemplate{
203+
Spec: atev1alpha1.ActorTemplateSpec{SandboxClass: atev1alpha1.SandboxClassGvisor},
204+
},
205+
}
206+
err := step.Execute(ctx, &ResumeInput{ActorName: "shared", Atespace: "team-a"}, state)
207+
if status.Code(err) != codes.FailedPrecondition {
208+
t.Fatalf("Execute() error = %v, want FailedPrecondition (no free workers)", err)
209+
}
210+
211+
stored, err := persistence.GetWorker(ctx, "worker-ns", "pool", "pod-1")
212+
if err != nil {
213+
t.Fatalf("GetWorker: %v", err)
214+
}
215+
if got := stored.GetAssignment().GetActor().GetAtespace(); got != "team-b" {
216+
t.Errorf("worker assignment atespace = %q, want %q (assignment: %v)", got, "team-b", stored.GetAssignment())
217+
}
218+
}

cmd/ateapi/internal/controlapi/workflow_suspend.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ func (s *FinalizeSuspendedStep) Execute(ctx context.Context, input *SuspendInput
184184
} else {
185185
// Only free it if it still belongs to us
186186
if wass := worker.Assignment; wass != nil {
187-
if wass.Actor.Name == input.ActorName {
187+
if wass.Actor.Atespace == input.Atespace && wass.Actor.Name == input.ActorName {
188188
worker.Assignment = nil
189189
err = s.store.UpdateWorker(ctx, worker, worker.Version)
190190
if err != nil {
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// Copyright 2026 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package controlapi
16+
17+
import (
18+
"context"
19+
"testing"
20+
21+
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
22+
"github.com/agent-substrate/substrate/cmd/ateapi/internal/store/ateredis"
23+
"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
24+
"github.com/alicebob/miniredis/v2"
25+
"github.com/redis/go-redis/v9"
26+
)
27+
28+
// newTestPersistence returns a store backed by a throwaway miniredis.
29+
func newTestPersistence(t *testing.T) store.Interface {
30+
t.Helper()
31+
mr, err := miniredis.Run()
32+
if err != nil {
33+
t.Fatalf("failed to start miniredis: %v", err)
34+
}
35+
t.Cleanup(mr.Close)
36+
rdb := redis.NewClusterClient(&redis.ClusterOptions{Addrs: []string{mr.Addr()}})
37+
t.Cleanup(func() { rdb.Close() }) //nolint:errcheck // test cleanup
38+
return ateredis.NewPersistence(rdb)
39+
}
40+
41+
func TestFinalizeSuspendedStep_ReleasesOnlyOwnWorker(t *testing.T) {
42+
tests := []struct {
43+
name string
44+
assignmentAtespace string
45+
wantReleased bool
46+
}{
47+
{
48+
name: "frees worker assigned to this actor",
49+
assignmentAtespace: "team-a",
50+
wantReleased: true,
51+
},
52+
{
53+
name: "keeps worker assigned to same-named actor in another atespace",
54+
assignmentAtespace: "team-b",
55+
wantReleased: false,
56+
},
57+
}
58+
59+
for _, tt := range tests {
60+
t.Run(tt.name, func(t *testing.T) {
61+
ctx := context.Background()
62+
persistence := newTestPersistence(t)
63+
64+
worker := &ateapipb.Worker{
65+
WorkerNamespace: "worker-ns",
66+
WorkerPool: "pool",
67+
WorkerPod: "pod-1",
68+
Assignment: &ateapipb.Assignment{
69+
Actor: &ateapipb.ActorRef{Atespace: tt.assignmentAtespace, Name: "shared"},
70+
},
71+
}
72+
if err := persistence.CreateWorker(ctx, worker); err != nil {
73+
t.Fatalf("CreateWorker: %v", err)
74+
}
75+
76+
actor := &ateapipb.Actor{
77+
Metadata: &ateapipb.ResourceMetadata{Atespace: "team-a", Name: "shared"},
78+
Status: ateapipb.Actor_STATUS_SUSPENDING,
79+
AteomPodNamespace: "worker-ns",
80+
AteomPodName: "pod-1",
81+
WorkerPoolName: "pool",
82+
InProgressSnapshot: "gs://snapshots/shared/1",
83+
}
84+
if _, err := persistence.CreateActor(ctx, actor); err != nil {
85+
t.Fatalf("CreateActor: %v", err)
86+
}
87+
88+
step := &FinalizeSuspendedStep{store: persistence}
89+
input := &SuspendInput{ActorName: "shared", Atespace: "team-a"}
90+
if err := step.Execute(ctx, input, &SuspendState{}); err != nil {
91+
t.Fatalf("Execute: %v", err)
92+
}
93+
94+
stored, err := persistence.GetWorker(ctx, "worker-ns", "pool", "pod-1")
95+
if err != nil {
96+
t.Fatalf("GetWorker: %v", err)
97+
}
98+
if released := stored.GetAssignment() == nil; released != tt.wantReleased {
99+
t.Errorf("worker released = %t, want %t (assignment: %v)", released, tt.wantReleased, stored.GetAssignment())
100+
}
101+
})
102+
}
103+
}

0 commit comments

Comments
 (0)