Repository navigation
Update suspend and pause tests to use SuspendActor and PauseActor APIs - #2688
Conversation
5b7afc7 to
3ed75fe
Compare
|
|
||
| ctx := context.Background() | ||
| persistence := newTestPersistence(t) | ||
| seedSuspendTemplate(t, ctx, persistence, &ateapipb.SnapshotConfig{StorageLocation: testStorageLocation, PreferredFidelity: volumes}) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The matrix never seeds the worker that wireTestAssignment() names. As a result, every row that succeeds goes through finalize's "worker already gone, skipping release" branch, and the normal ReleaseActorFromWorker path is not pinned here. A refactor that stopped releasing the worker would still pass this matrix, even though this is the suite the next PR depends on to prove it kept the behavior.
Give each row its own store, call seedWireWorker(t, ctx, st, assignmentOf(actor)), and add whether the worker's assignment was released to suspendOutcome. That way, rejected rows also show the worker was left alone.
There was a problem hiding this comment.
Fixed in 7aef594. Each sync/async run now gets its own store and calls seedWireWorker. The worker holds the actor's assignment when the row has a WorkerAssignment, and is empty otherwise. suspendOutcome gained workerHeld, read from the worker's assignments after the run. Every successful row pins false (released). The three rejected rows that start with an assignment (suspend in progress/async, pause in progress/sync, golden/async) pin true, so the worker is shown untouched.
While here, the matrix now compares the atelet RPC and the stored snapshots as typed structs (ateletCall, snapshotOutcome) instead of formatted strings.
| } | ||
| if want := ateattr.SnapshotFidelityValue(tc.want); wireFidelity != want { | ||
| t.Errorf("wireFidelity = %q, want %q", wireFidelity, want) | ||
| if _, err := w.PauseActor(ctx, actorRef); err != nil { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The old test checked the wireFidelity that ensureAteletPaused returned. That value becomes the snapshot.fidelity label on the pause and suspend lifecycle-op metric. Without that check, no test covers this mapping through PauseActor or SuspendActor, so the refactor could drop or change the label without a test failing.
Set w.instruments from newTestInstruments(t) and assert the attribute on the recorded data point. Do the same in the suspend wire-request and promote tests.
There was a problem hiding this comment.
Fixed in 7aef594. Added assertLifecycleFidelity, which reads the lifecycle-op datapoint's snapshot.fidelity attribute. These tests now set w.instruments from newTestInstruments(t) and assert it:
TestPauseActor_AteletPaused_ForwardsPreferredFidelity(each fidelity row)TestSuspendActor_AteletSuspended_WireRequestTestSuspendActor_Finalized_NoAssignment, the test that drives thePromoteSnapshotpath
| func TestSuspendAndPause_FinalizeWithoutAssignment(t *testing.T) { | ||
| wantState := map[string]ateapipb.ActorState{ | ||
| "sync": ateapipb.ActorState_ACTOR_STATE_SUSPENDED, | ||
| "async": ateapipb.ActorState_ACTOR_STATE_SUSPENDING, |
There was a problem hiding this comment.
🤖 question 🟢 – In the async row, PauseActor reports success but leaves the actor SUSPENDING with no worker assignment, and nothing appears to move it on from there. Is that intended, or a second bug next to #2676? Either way, the comment should say which side is the known bug. Then whoever fixes it knows which pin to change, rather than reading both as the intended behavior.
There was a problem hiding this comment.
Good catch. That shape doesn't happen in practice: no writer clears the assignment and leaves the actor SUSPENDING. The real writer is the DeleteWorker workflow, which sets CRASHED and clears the assignment in one update. In 7aef594 the test's mid-Checkpoint mutation does the same. Now the async side keeps CRASHED, which is the intended behavior (pause finalize doesn't overwrite an out-of-band transition). The sync side commits SUSPENDED over the crash, which is the #2676 bug. The test comment says so, and says the fix should change the sync pin to CRASHED.
There was a problem hiding this comment.
🤖 Following up: the pause side now ends CRASHED but returns OK, while TestPauseActor_Finalized_AlreadyCrashed pins FailedPrecondition for a crash mid-pause. Is OK on a crashed actor intended? If not, it may belong next to #2676 as a known bug rather than a pin the merge must keep.
There was a problem hiding this comment.
I updated 2676 to include this bug as well. I added the fix to #2694
| w, atelet := newWireCaptureWorkflow(t, persistence) | ||
|
|
||
| _, err := mode.run(w, ctx, actorRef) | ||
| if got := apierror.Code(err); got != want.code { |
There was a problem hiding this comment.
🤖 nit 🟢 – code is checked here and again in the cmp.Diff below, so a mismatch is reported twice. The cmp.Diff alone is enough.
There was a problem hiding this comment.
Fixed in 7aef594. Removed the separate code check. The cmp.Diff failure message now includes err, so the error detail is still reported.
4a7a466 to
7aef594
Compare
7aef594 to
3550e8b
Compare
…Actor The step-level tests called the private steps of both workflows directly (ensureMarkedPausing, ensureAteletPaused, ensurePausedFinalized, ensureMarkedSuspending, ensureAteletSuspended, ensureSuspendedFinalized). Drive them through the public SuspendActor and PauseActor entrypoints instead, so the two workflows can share their steps without touching a test file. - A dangling atelet dialer stops a workflow right after its mark step, for tests that pin what the mark commits. - The capturing atelet fake now records PromoteSnapshot, counts calls, and can mutate the store mid-Checkpoint, for tests that pin a race between the atelet call and finalize. - TestSuspendAndPause_CharacteristicMatrix pins, for each actor shape, the code, atelet RPC, and stored record of both a sync (SuspendActor) and an async (PauseActor) suspend. - TestSuspendAndPause_FinalizeWithoutAssignment pins the finalize divergence tracked in agent-substrate#2676. Test-only change.
- TestSuspendAndPause_CharacteristicMatrix gives each run its own store, seeds the worker wireTestAssignment names, and pins whether the worker still holds the actor's assignment, so a dropped release is caught. - The matrix compares the atelet RPC and the stored snapshots as typed structs (ateletCall, snapshotOutcome) instead of formatted strings. - The pause wire test, the suspend wire-request test, and the promote test assert the snapshot fidelity label on the lifecycle-op metric. - TestSuspendAndPause_FinalizeWithoutAssignment now crashes the actor as the DeleteWorker workflow does, and names the suspend side as the agent-substrate#2676 bug. - Drop the duplicate code check in the matrix. Test-only change.
…pins Key the suspendOutcome literals in the characteristic matrix so rows can be checked against the test doc, mark both sides of TestSuspendAndPause_FinalizeWithoutAssignment as agent-substrate#2676 bugs, and use newMarkOnlyWorkflow in the paused-snapshot precondition tests.
…flows TestHostingNodeAfterWorkerRelease called ensurePausedFinalized and ensureSuspendedFinalized directly, the last private step calls in the package. Run those rows through PauseActor and SuspendActor against the wire capture workflow instead, seeding the worker wireTestAssignment names and the suspend test template so the atelet call reaches the fake.
8d321f4 to
d0df266
Compare
agent-substrate#2707) Fixes agent-substrate#2676. If worker deletion or another crash writer marks an actor CRASHED after checkpoint succeeds but before finalize re-reads it, SuspendActor currently overwrites that crash with SUSPENDED, while PauseActor reports success for the CRASHED actor. Both finalize steps now require the re-read actor to be SUSPENDING before validating snapshots, releasing the worker, or writing the final state. Otherwise they return FailedPrecondition and preserve the existing state, crash reason, and snapshot records. The existing version precondition still rejects a concurrent crash after the read with Aborted. Completed suspend and pause calls retain their existing success paths at workflow entry. The full-workflow regression now checks FailedPrecondition and preservation of the original crash in both modes, updating the behavior pinned by agent-substrate#2688. The already-crashed pause fixture also clears WorkerAssignment, matching a real crash; it previously reached FailedPrecondition only through the missing-node branch. Validation of the production change on Linux with CI=true and real PostgreSQL test containers: - RED: before the production fix, the corrected already-crashed pause test and both modes of the full-workflow regression fail because the calls incorrectly return OK. - GREEN: `go test -race -count=1 -json ./...` passes: 158 test-bearing packages and 9,670 test entries, including all 763 controlapi entries and 169 API functional entries. - `go build ./cmd/ateapi ./cmd/atelet ./cmd/kubectl-ate`, `hack/verify-all.sh`, and the tools/apitool race tests pass. The verifier leaves the source tree unchanged. - The full suite skips 41 tests requiring root privileges, IPv6 loopback, or configured external object storage. The affected PostgreSQL regressions do not skip. E2E follow-up: The original CI passed both gVisor suites but failed both microVM suites in `TestGracefulWorkerTerminationSuspend`. That test unconditionally expected SuspendActor to succeed after worker deletion, even when deletion had already crashed the actor. The test now ties the RPC result to the stored state. OK requires SUSPENDED with no crash record. FailedPrecondition or Aborted requires CRASHED, a crash reason, and the previous completed durable snapshot. Both outcomes require the worker and its assignment to be removed. All other errors fail the test. Follow-up validation on Linux with CI=true and real PostgreSQL and envtest: - The four affected controlapi regressions pass with `-race -count=3`. - All 763 controlapi test entries and all 169 functionaltest entries pass, with no skips. The functional package was rerun after initializing the isolated source directory's Git metadata, which envtest setup requires. - `go test -race -c ./internal/e2e/suites/demo` and `go vet ./internal/e2e/suites/demo` pass. Native compilation, gofmt, and whitespace checks also pass. - Real runtime E2E has not run at this head. The new [pr-workflow run](https://github.com/agent-substrate/substrate/actions/runs/38817919692) is `action_required` and needs maintainer approval. Draft agent-substrate#2694 refactors these same workflows and currently preserves an out-of-band state while returning success. This focused fix also reports FailedPrecondition, as required by agent-substrate#2676; that behavior should carry forward into the refactor. - [x] Local validation passes; runtime E2E awaits workflow approval. - [x] Documentation reviewed; finalize and regression-test comments describe the corrected behavior. No guide change is needed.
) This is the next step toward [async suspend](https://github.com/zoez7/substrate/blob/async-suspend-design/docs/dev/async-suspend-en.md), following agent-substrate#2688. `SuspendActor` and `PauseActor` now run one set of shared steps behind a `suspendMode`: - `suspendSync` (`SuspendActor`): the snapshot is durable before the workflow returns. - `suspendAsync` (`PauseActor`): the snapshot stays on the worker's node. Both APIs stay for now. `workflow_pause.go` is removed. Behavior is unchanged. The tests agent-substrate#2688 moved testing of ensurePaused* and ensureSuspended* functions onto `PauseActor` and `SuspendActor`. The only test edit is in `workflow_test.go`, which called the removed `ensurePausedFinalized`. The fix for agent-substrate#2676 (finalize not checking that the actor is still `SUSPENDING`) will come in a separate PR. - [x] Tests pass - [ ] Appropriate changes to documentation are included in the PR
4 participants
This is a step toward async suspend. The next PR merges
SuspendActorandPauseActorinto one set of shared steps: sync suspend, and async suspend (today's pause). Both APIs stay.The current unit tests call the private step methods (
ensureMarked*,ensureAtelet*,ensure*Finalized). But the next PR will rewrite exactly those steps. If the tests stay as they are, they get rewritten in the same PR as the refactor, and nothing shows that the refactor kept the behavior.This PR moves the tests onto the public
SuspendActorandPauseActorentrypoints and checks them against the current implementation. The merge PR then shouldn't need to touch any test file: if this suite still passes unchanged, behavior is preserved.Test-only change.