-
Notifications
You must be signed in to change notification settings - Fork 144
fix: skip empty node name in local snapshot info after pause finalization #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Zoe Zhao (zoez7)
merged 6 commits into
agent-substrate:main
from
mesutoezdil:fix/pause-empty-nodename
Jul 17, 2026
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f2bf5b1
fix: skip empty node name in local snapshot info after pause finaliza…
mesutoezdil f3cc602
fix: crash actor when node name is unknown during pause finalization
mesutoezdil 60ac20b
style: replace em dashes with commas in test comments
mesutoezdil 0a5e21d
fix: use slog.ErrorContext for crashed-actor log in finalize pause
mesutoezdil 7853136
fix: derive NodeVmsWithLocalSnapshots guard from actor status, not no…
mesutoezdil 9dd3ac7
Merge remote-tracking branch 'origin/main' into fix/pause-empty-nodename
mesutoezdil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| // Copyright 2026 Google LLC | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package controlapi | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/agent-substrate/substrate/cmd/ateapi/internal/store/storetest" | ||
| "github.com/agent-substrate/substrate/pkg/proto/ateapipb" | ||
| ) | ||
|
|
||
| // TestFinalizePausedStep_WorkerGone reproduces the scenario where the worker pod | ||
| // disappears from the DB during pause finalization, so the node it ran on is | ||
| // unknown. | ||
| // | ||
| // Old behavior: NodeVmsWithLocalSnapshots = []string{""}, which made | ||
| // findFreeWorker search for a worker with node name "", never found, a | ||
| // permanent "no free workers available" on resume. | ||
| // | ||
| // Current behavior: NodeVmsWithLocalSnapshots is left nil, and the actor is | ||
| // crashed instead of left PAUSED, since a local snapshot with an unknown node | ||
| // can never be safely resumed. | ||
| func TestFinalizePausedStep_WorkerGone(t *testing.T) { | ||
| st, cleanup := storetest.SetupTestStore(t) | ||
| defer cleanup() | ||
|
|
||
| ctx := context.Background() | ||
| const atespace, actorName = "team-a", "actor-1" | ||
|
|
||
| actor := &ateapipb.Actor{ | ||
| Metadata: &ateapipb.ResourceMetadata{Atespace: atespace, Name: actorName}, | ||
| Status: ateapipb.Actor_STATUS_PAUSING, | ||
| AteomPodNamespace: "default", | ||
| AteomPodName: "worker-pod-1", | ||
| WorkerPoolName: "pool1", | ||
| InProgressSnapshot: "snap-prefix", | ||
| } | ||
| if _, err := st.CreateActor(ctx, actor); err != nil { | ||
| t.Fatalf("CreateActor: %v", err) | ||
| } | ||
| // Intentionally NOT creating the worker in store, simulates worker already gone. | ||
|
|
||
| step := &FinalizePausedStep{store: st} | ||
| input := &PauseInput{Atespace: atespace, ActorName: actorName} | ||
| state := &PauseState{} | ||
| if err := step.Execute(ctx, input, state); err != nil { | ||
| t.Fatalf("Execute: %v", err) | ||
| } | ||
|
|
||
| got, err := st.GetActor(ctx, atespace, actorName) | ||
| if err != nil { | ||
| t.Fatalf("GetActor: %v", err) | ||
| } | ||
|
|
||
| if got.GetStatus() != ateapipb.Actor_STATUS_CRASHED { | ||
| t.Errorf("status = %v, want CRASHED (node name unknown, cannot resume safely)", got.GetStatus()) | ||
| } | ||
| for _, n := range got.GetLatestSnapshotInfo().GetLocal().GetNodeVmsWithLocalSnapshots() { | ||
| if n == "" { | ||
| t.Errorf("BUG: empty string in NodeVmsWithLocalSnapshots, findFreeWorker would never match") | ||
| } | ||
| } | ||
|
|
||
| state.Actor = got | ||
| done, err := step.IsComplete(ctx, input, state) | ||
| if err != nil { | ||
| t.Fatalf("IsComplete: %v", err) | ||
| } | ||
| if !done { | ||
| t.Error("IsComplete = false, want true once the actor is CRASHED and the worker is freed") | ||
| } | ||
| } | ||
|
|
||
| // TestFindFreeWorker_EmptyNodeRestriction shows the root symptom the fix | ||
| // avoids: old code wrote []string{""} into NodeVmsWithLocalSnapshots when the | ||
| // node name was unknown, and findFreeWorker required worker.NodeName == "", | ||
| // which never matches a real worker. | ||
| func TestFindFreeWorker_EmptyNodeRestriction(t *testing.T) { | ||
| workers := []*ateapipb.Worker{ | ||
| {WorkerNamespace: "default", WorkerPool: "pool1", WorkerPod: "w1", NodeName: "node1"}, | ||
| {WorkerNamespace: "default", WorkerPool: "pool1", WorkerPod: "w2", NodeName: "node2"}, | ||
| } | ||
|
|
||
| s := &AssignWorkerStep{} | ||
|
|
||
| // Old behavior: []string{""}, no worker has NodeName == "", returns nil. | ||
| got, err := s.findFreeWorker(workers, "", nil, nil, []string{""}) | ||
| if err != nil { | ||
| t.Fatalf("findFreeWorker: %v", err) | ||
| } | ||
| if got != nil { | ||
| t.Errorf("expected nil with old buggy input, got %v", got) | ||
| } | ||
|
|
||
| // Fixed behavior: nil restrictions, any free worker matches. | ||
| got, err = s.findFreeWorker(workers, "", nil, nil, nil) | ||
| if err != nil { | ||
| t.Fatalf("findFreeWorker: %v", err) | ||
| } | ||
| if got == nil { | ||
| t.Error("expected a worker with nil restrictions, got nil") | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resume from "SnapshotType_SNAPSHOT_TYPE_LOCAL" supposed to land on machine where files are stored locally, otherwise resume will fail.
we need to find a root cause of the issue that node is empty. This is a bug/exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be a separete issue i think,i dont solve it in this pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Dmitry Berkovich (@dberkov) I think you described a different bug than the one this PR is solving
I think this PR solves a idempotency problem that we don't handle today.
Now we don't know which node the snapshot is on, we can only crash the actor.
I think we should check this PR in as a temporary fix, but we also need to add node name as a field in the actor API, so Pause can be idempotent.