Repository navigation
fix: scope suspend cancellation classification - #273
Anish Ramasekar (aramase) merged 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues requiring resolution for the snapshot cancellation classification change.
No open issues 🎉
What changed in this PR
This follow-up to #271 limits snapshot cancellation classification to public Suspend calls, preserving the existing operational-error behavior of Exec/Resume approval handoffs.
Changes:
- Apply caller-context classification only when the shared suspend helper receives a nil cursor.
- Add a deterministic regression that cancels Exec during an approval-handoff snapshot failure.
- Clarify the compatibility contract in the specification and existing changelog fragment.
| File | Description |
|---|---|
spec.md |
Distinguishes public Suspend cancellation from approval-handoff failures. |
placement/placement.go |
Restricts snapshot cancellation classification to public Suspend. |
placement/approval_test.go |
Verifies canceled handoff snapshots preserve the operational cause without an interruption marker. |
changelog/unreleased/error-reasons-compat.md |
Documents preserved approval-handoff error classification. |
Review effort: Balanced
Anish Ramasekar (aramase)
left a comment
Anish Ramasekar (aramase)
left a comment
There was a problem hiding this comment.
LGTM, thanks for the quick follow-up. Scoping the wrap to the nil-cursor path keeps the approval handoff on its pre-#271 behavior, and the new row fails without the fix.
One optional nit outside the diff: the ErrAdmissionInterrupted godoc (placement/placement.go:66-71) and admissionInterruptedCode (session/service.go:1281-1283) still say any "runtime operation". Scoping that to public Suspend/Fork, as spec.md now does, would keep them in sync.
| } | ||
| desc, _ := har.Describe(t.Context()) | ||
| b := &approvalBackend{Backend: substrate.New(ctl, "space", substrate.ObjectRef{Name: "gate"}, desc)} | ||
| ctx := t.Context() |
There was a problem hiding this comment.
nit (optional): Resume reaches the same handoffPark path, so a Resume variant of this row would pin the spec's "Exec/Resume" wording if the two paths ever split.
78a368b
into
agent-primitives:main
Summary
Context
Follow-up to the non-blocking review note on #271:
p.suspendalso serveshandoffPark, where a non-nil cursor commits the automatic approval-gate SUSPEND under the turn fence. Classifying that snapshot from the caller context would change Exec/Resume behavior outside #260.Both public Suspend entry points pass a nil cursor; approval handoff is the only non-nil caller. The patch therefore applies
callerContextErroronly to the nil-cursor path.The regression cancels the exact Exec context synchronously as the approval-handoff snapshot begins, injects a snapshot failure, and verifies that the operational cause remains while
ErrAdmissionInterruptedis absent.Verification
go test ./placement -run '^TestPlacementApprovalHandoffPhases$' -count=10go test -race -shuffle=on ./placement ./sessiongo vet ./placement ./sessiongo build ./...go run ./hack/doccheckgit diff --checkgofmt -l .Follow-up to #271.