atenet: never cancel an in-flight resume at the park budget - #991
atenet: never cancel an in-flight resume at the park budget#991Omer Yahud (omeryahud) wants to merge 1 commit into
Conversation
|
Zoe Zhao (@zoez7) yufan-su Hi guys, would appreciate your review here |
Aditya Shantanu (aditya-shantanu)
left a comment
Aditya Shantanu (aditya-shantanu)
left a comment
There was a problem hiding this comment.
The fix itself looks right, but the branch needs a rebase before it can merge — #1025 landed after it branched and renamed the proto surface the new tests use, so the PR no longer compiles against current main. Holding off on approval until that's done; happy to approve after the rebase.
What I verified:
context.WithoutCancel(bgCtx)for the attempt plus keepingbgCtxonwait.ExponentialBackoffWithContextgives exactly the intended split: the budget stops new attempts, the in-flight attempt runs to completion, and an overshooting success is honored by the backoff loop (condition returning true short-circuits the ctx check). ThelastRetryErr != nil && (bgCtx.Err() != nil || wait.Interrupted(err))classification correctly maps a late retryable failure to the capacity 503.- The "bounded by ateapi's own server-side RPC deadline" claim holds:
MaxDeadlineUnaryInterceptor(maxRPCDeadline)incmd/ateapi/main.gocaps it at 10m. go test -race ./cmd/atenet/internal/router/ingress/passes on the branch as-is.
Rebase breakage (merge with main is textually clean but doesn't build):
resumer_test.go: the new tests useateapipb.Actor_STATUS_RUNNINGand a top-levelWorkerAssignmentonActor; on main this is nowStatus: &ateapipb.ActorStatus{State: ateapipb.ActorState_ACTOR_STATE_RUNNING, WorkerAssignment: ...}.internal/e2e/suites/parking/parking_test.go: the new follow-up assertion callswaitForActorStatus(..., ateapipb.Actor_STATUS_RUNNING); main renamed the helper towaitForActorState(..., ateapipb.ActorState_ACTOR_STATE_RUNNING).
One trade-off worth a sentence in the doc if you think it matters: attempts now have no client-side deadline at all, so a pathologically slow ResumeActor holds its parking-lot slot for up to ateapi's 10m ceiling rather than the budget. Seems acceptable given the alternative is a stranded worker, but it does change worst-case slot occupancy.
Aditya Shantanu (aditya-shantanu)
left a comment
Aditya Shantanu (aditya-shantanu)
left a comment
There was a problem hiding this comment.
Inline notes for the rebase (details in my earlier review).
| ctxErrAtReturn = ctx.Err() | ||
| mu.Unlock() | ||
| return &ateapipb.ResumeActorResponse{ | ||
| Actor: &ateapipb.Actor{Metadata: &ateapipb.ResourceMetadata{Name: testActorName}, Status: ateapipb.Actor_STATUS_RUNNING, WorkerAssignment: &ateapipb.WorkerAssignment{WorkerPodIp: expectedIP}}, |
There was a problem hiding this comment.
After #1025 this no longer compiles on main: Status is now &ateapipb.ActorStatus{State: ateapipb.ActorState_ACTOR_STATE_RUNNING, WorkerAssignment: ...} — WorkerAssignment moved inside ActorStatus and the enum was renamed. The merge with main is textually clean, so CI won't flag it until the test workflows run against the merge commit.
| // The flake's root cause stranded actors in RESUMING with the worker | ||
| // claimed (#675): pin that B really converges and a follow-up request | ||
| // is served warm — a stranded actor would 503 it. | ||
| waitForActorStatus(ctx, t, clients, actorB, ateapipb.Actor_STATUS_RUNNING) |
There was a problem hiding this comment.
Same #1025 rebase issue: main renamed this helper to waitForActorState and the enum to ateapipb.ActorState_ACTOR_STATE_RUNNING.
| // the restore away and strands the worker (#675). An attempt still | ||
| // running when the budget elapses is waited for and its real result | ||
| // classified below; ateapi's own server-side RPC deadline bounds it. | ||
| attemptCtx := context.WithoutCancel(bgCtx) |
There was a problem hiding this comment.
Worth a sentence in docs/request-parking.md: with no client-side deadline on the attempt, a pathologically slow ResumeActor now holds its parking-lot slot up to ateapi's 10m maxRPCDeadline instead of the budget. Acceptable vs. stranding a worker, but it changes worst-case slot occupancy.
80951bc to
66f3f30
Compare
|
Thanks Aditya Shantanu (@aditya-shantanu), I've rebased and addressed your doc comment |
The park budget doubled as the ResumeActor RPC deadline, so a resume that outlived it was cancelled mid-restore. ateapi claims the worker and persists RESUMING before the restore begins, rolls back neither on cancellation, and nothing reclaims a RESUMING actor on a live worker - a budget cancel therefore discarded the restore and stranded the worker, which is the TestRequestParking/ParkThenServed flake (agent-substrate#675). The budget now bounds the retry loop only: an attempt still in flight when it elapses runs to completion on a non-cancellable context (bounded by ateapi's own server-side RPC deadline, with Envoy's ext_proc timeout unchanged as the client-side ceiling), and its real result is classified - an overshooting restore is served late instead of failed, and a late retryable error still surfaces as the capacity 503. This also closes a mapping hole where budget expiry with zero completed attempts escaped as a raw DeadlineExceeded and surfaced as a 504 (what actually failed BudgetExhaustion in run 30476804701): the loop can no longer exit before its first attempt has completed. The e2e window widens to admit a served-late overshoot, and the test now pins the invariants the flake exposed: actor B reaches RUNNING and a follow-up request is served warm. Fixes agent-substrate#675
66f3f30 to
aa5324f
Compare
Adopted from agent-substrate#991 by @omeryahud to unblock CI velocity. Fixes the TestRequestParking flake (agent-substrate#675): the park budget doubled as the ResumeActor RPC deadline, cancelling restores mid-flight and stranding RESUMING actors on live workers. Co-authored-by: Omer Yahud <oyahud@nvidia.com>
SuspendActor returns before the suspend completes, and on the micro-VM class the snapshot upload routinely outlives the 5s park budget under CI contention — the router's budget-exhausted 503 is then correct behavior, not a failure. Retry the request (bounded at 3 attempts): each attempt parks anew and the completed suspend lets one resume the actor. A stranded worker (agent-substrate#675's root cause) fails all attempts, so the regression stays pinned. This was the dominant historical failure mode of the agent-substrate#675 flake (identical parking_test.go:113 503s in pre-PR runs 32305728993, 32397291519, 32487958077) — distinct from the mid-restore cancellation that agent-substrate#991 fixes.
3 participants
The park budget doubled as the ResumeActor RPC deadline, so a resume that outlived it was cancelled mid-restore. ateapi claims the worker and persists RESUMING before the restore begins, rolls back neither on cancellation, and nothing reclaims a RESUMING actor on a live worker - a budget cancel therefore discarded the restore and stranded the worker, which is the TestRequestParking/ParkThenServed flake (#675).
The budget now bounds the retry loop only: an attempt still in flight when it elapses runs to completion on a non-cancellable context (bounded by ateapi's own server-side RPC deadline, with Envoy's ext_proc timeout unchanged as the client-side ceiling), and its real result is classified - an overshooting restore is served late instead of failed, and a late retryable error still surfaces as the capacity
503.
The e2e window widens to admit a served-late overshoot, and the test now pins the invariants the flake exposed: actor B reaches RUNNING and a follow-up request is served warm.
Fixes #675