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

オープン
#1,527 コメント 1 件 リアクション 0 件 担当者 0 名 GitHub で見る
area/node kind/bug prio/P2
主要言語
Go
スター
1.8k
フォーク
316
平均マージ
2日 43分
マージ済み PR(30日)
287

説明

### 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`.

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。