agent-substrate / agent-substrate/substrate

[Bug]: Untolerated snapshot GC runs before the SUSPENDED commit, so a failed delete wedges the actor in SUSPENDING with no way out

Đang mở
#1,527 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
area/node kind/bug prio/P2
Ngôn ngữ chính
Go
Star
1.8k
Fork
316
Merge trung bình
2 ngày 43 phút
Pull request đã merge (30 ngày)
287

Mô tả

### What happened?

`#1417` added a `ReleaseReplacedSnapshot` step to the suspend workflow, which deletes the snapshot
each new one replaces. It runs from `ate-api-server` rather than from the worker.

`releaseReplacedSnapshot` (`cmd/ateapi/internal/controlapi/workflow_suspend.go`, ~L454) calls
`objectstore.DeletePrefix` and **returns the error un-tolerated**. It is **step 3** in the workflow,
and **step 4** is the commit to `ACTOR_STATE_SUSPENDED`.

So any failure of that delete leaves the actor in `SUSPENDING`, with:

- the new snapshot already written to the bucket, complete,
- `Status.ExternalSnapshot` still pointing at the *old* URI,
- `InProgressSnapshotName` still set.

**There is no reconcile out of that state.** A re-issued suspend hits
`markSkipped("actor already SUSPENDING")` (L130-131). A `SUSPENDING` actor with no active worker is
later declared `CRASHED` (L214-218), which is terminal.

**The error message is also wrong in a way that misdirects the operator.** It reports that no
snapshot was written, when in our reproduction a full 238 MB snapshot had been written successfully
moments earlier. Someone debugging this looks at the checkpoint path, which is fine, rather than at
the cleanup step, which is not.

**The workflow already anticipated this exact failure class and then reintroduced it.** The comment
at L396-398 reads:

> the finalize must run even with no worker assignment (nothing to free), or the actor would be left
> SUSPENDING forever with the workflow reporting success

which is precisely what an untolerated storage call placed in front of the finalize brings back. And
the docstring at L450-452 says:

> an interrupted release is rediscoverable: the retry deletes whatever is left

which assumes a retry that L130 shows does not exist.

### Expected Behavior

**Snapshot garbage collection is housekeeping and must never be able to strand an actor.** A failure
to delete a superseded snapshot should cost some wasted bucket storage, not the actor.

Concretely, any one of:

1. Move `ReleaseReplacedSnapshot` after the `SUSPENDED` commit, so the actor reaches a good state
before anything is deleted. This is the cleanest fix.
2. Or tolerate the error: log it, record the orphaned URI for a later collection pass, and let the
commit proceed.

Two things worth fixing alongside, both of which make this survivable even if the ordering stays:

3. A suspend issued against an actor already in `SUSPENDING` should resume the workflow rather than
`markSkipped`, so there is a way to retry a wedge.
4. The error should say what actually failed. Reporting "no snapshot written" when one was written
sends the operator to the wrong subsystem.

### Steps to Reproduce

The reproduction is cheap and deterministic: revoke the binding the GC step needs, then suspend the
same actor twice. The first suspend succeeds because there is no previous snapshot to release, and
the second fails.

```
1. On a working install, note the ate-api-server bucket bindings:

gcloud storage buckets get-iam-policy "gs://$BUCKET_NAME" --format=json \
| grep -A3 ate-api-server

2. Remove ate-api-server's write access to the snapshot bucket
(this simulates the transient GCS failure; it is not itself the bug):

WI="principal://iam.googleapis.com/projects/${PROJECT_NUMBER}/locations/global/workloadIdentityPools/${PROJECT_ID}.svc.id.goog/subject/ns/ate-system/sa/ate-api-server"
gcloud storage buckets remove-iam-policy-binding "gs://$BUCKET_NAME" \
--member="$WI" --role=roles/storage.objectAdmin

3. Create an actor and drive one request into it, then suspend it:

kubectl-ate create actor -a agents actor-1 --template-ref agent
curl -sS "$INGRESS" -d '{"prompt":"hello"}'
kubectl-ate suspend actor -a agents actor-1
=> succeeds. There is no previous snapshot, so the GC step is skipped.

4. Resume it, drive another request, and suspend it a SECOND time:

kubectl-ate resume actor -a agents actor-1
curl -sS "$INGRESS" -d '{"prompt":"hello again"}'
kubectl-ate suspend actor -a agents actor-1
=> fails on the GC step.

5. Observe the wedge:

kubectl-ate get actor -a agents actor-1 -o json | jq .status
=> state: ACTOR_STATE_SUSPENDING, indefinitely

The new snapshot IS in the bucket:
gcloud storage ls -r "gs://$BUCKET_NAME/**/actors//snapshots/"

6. Try to retry, and to get out:

kubectl-ate suspend actor -a agents actor-1 # markSkipped, no-op
kubectl-ate resume actor -a agents actor-1 # FailedPrecondition
=> the actor eventually transitions to CRASHED, which is terminal.
```

### Sandbox Runtime

gVisor (runsc)

### Agent Substrate Version / Commit SHA

`c48b3a3c` (`release-0.1`), which carries `#1417` as `9b333c6f`

### Kubernetes Version & Environment

GKE 1.35 (`v1.35.7-gke.1150000`), us-central1-c

### Host OS & Architecture

Linux 6.6 (x86_64)

### Relevant Logs and Diagnostic Output

```shell
# Source, release-0.1 @ c48b3a3c
cmd/ateapi/internal/controlapi/workflow_suspend.go

L130-131 markSkipped("actor already SUSPENDING") <- no retry path
L214-218 SUSPENDING + no active worker => CRASHED <- terminal
L396-398 "...or the actor would be left SUSPENDING forever with the
workflow reporting success" <- the anticipated case
L450-452 "an interrupted release is rediscoverable: the retry deletes
whatever is left" <- assumes a retry
~L454 releaseReplacedSnapshot -> objectstore.DeletePrefix
error returned un-tolerated, step 3 of 4
step 4 commit ACTOR_STATE_SUSPENDED <- never reached
```

### Additional Context

**How this was found, and why it is P2 rather than P0.** It surfaced during acceptance testing as
what looked like a fresh-install defect: the theory was that nothing grants `ate-api-server` access
to the snapshot bucket, which would make this fire on the second suspend of every new cluster.

**That theory is wrong**, and it is recorded here so nobody re-derives it. `tools/setup-gcp` grants
those bindings, `--bucket-bindings` defaults to `true`, and `tools/setup-gcp/README.md` documents
them in a roles table that explicitly names snapshot GC as the reason `ate-api-server` needs the
bucket. Two people reached the wrong conclusion independently by searching `hack/` and `docs/` and
not `tools/`, and one of them hit it for real by installing from source, which provisions nothing in
the cloud project. That is a from-source-path documentation matter, not this bug.

**With the grant present, the trigger is a transient storage failure** in the window between
checkpoint and commit: a GCS 503, a revoked or expired binding, a quota trip, or a bucket lifecycle
rule. Low probability per suspend, and not low across a fleet's suspends over months, since the
consequence is a permanently lost actor rather than a retried operation.

**One thing that raises or lowers the severity of this issue and should be read together with it:**
the only reason this wedge is data loss rather than an annoyance is that `SUSPENDING` drains into
`CRASHED`, and `CRASHED` is currently terminal with no recovery path even when a good snapshot
exists. If that is fixed, this becomes a retryable stuck suspend and P2 is generous. If it ships
unrecoverable, this is a rare route to permanent loss.

### Confirmation

- [x] I have searched existing issues and verified that this is not a duplicate.
- [ ] I have verified that this issue occurs on the latest commit on `main`.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.