feat(kill-switch): admin/kill A2A verb, CancelAll, accepting-gate - #439
Conversation
Phase 1 of the agent kill switch — the forge-side primitive the platform (agent-builder) drives to disable an agent and kill its active work. - forge-core/runtime: add CancelReasonKillSwitch and CancellationRegistry.CancelAll(reason), which signals every in-flight invocation at once (snapshot-under-lock, cancel-outside-lock; each invocation's own release() pops its entry as executeTask unwinds). - forge-cli/runtime: a `killed` atomic gate on the Runner. New admin/kill JSON-RPC handler flips the gate and calls CancelAll — every cancelled invocation emits its own invocation_cancelled audit event with reason=kill_switch. tasks/send and tasks/sendSubscribe refuse new work once killed. admin/kill is behind the server-wide AuthMiddleware; the primary access control is agent-builder's admin-RBAC /kill endpoint, which calls this then scales the workload to zero regardless of outcome. Idempotent: a second kill re-signals an empty registry (0). Tests cover CancelAll signalling + reason propagation + the empty/idempotent path.
initializ-mk
left a comment
There was a problem hiding this comment.
Reviewed against the branch source. Strong core primitive, but one gap that undercuts the feature's central guarantee (Finding 1) and one audit gap (Finding 2) I would want addressed before merge. All 10 CI checks green.
What is solid (verified)
CancelAllis correct — snapshots cancel-funcs under the lock and invokes them outside it (matchesCancel's contention profile, avoids re-entrancy), and uses the identical cause type&cancelledByOrchestrator{Reason: reason}asCancel(cancellation.go:197 vs 175). SoCancellationReasonFromCauseunwraps it and each invocation'sinvocation_cancelledevent carriesreason=kill_switchvia the already-working path. Release-pops-own-entry is right; tests cover the count, per-ctx reason, and the idempotent empty path.kill_switchadded toIsValid()and tested.
Finding 1 — should-fix: the kill gate misses the REST ingress paths (2 of 4 sites)
The gate guards only the JSON-RPC tasks/send (1640) and tasks/sendSubscribe (1688). But registerRESTHandlers (live, registered at runner.go:1590) exposes two more new-work entry points with no killed check:
POST /tasks/send(runner.go:2251)POST /tasks/sendSubscribe(runner.go:2304)
I read both handler heads — they decode the body and admit work unconditionally. So a killed agent still accepts new work over REST. This is in-scope for "sync A2A": the JSON-RPC handler's own comment (1638) notes it "goes through the same wiring as REST POST /tasks/send." Scale-to-zero is a backstop, but the primitive's stated contract ("refuse new work, clear error") is silently violated on half the ingress surface, and any direct/test use of admin/kill without the k8s race leaves REST fully open. One-liner at the top of each REST handler, symmetric with the JSON-RPC ones. (tasks/get / tasks/cancel / /tasks/{id}/decisions correctly stay open — they act on existing work.)
Finding 2 — should-fix: the kill action itself is not in the audit stream
admin/kill records the actor only via r.logger.Info (ops log). It captures caller identity — good — but that never reaches the tamper-evident audit NDJSON. If the agent is idle (cancelled=0), the kill produces no audit event at all — no record that a destructive admin action happened or who did it. Emit an admin_kill audit event via EmitFromContext (caller / reason / cancelled), independent of whether any invocation was in flight.
Finding 3 — note (PR-acknowledged): authorization
admin/kill is behind only AuthMiddleware, so any authenticated caller can trip it — killing every peer's sessions and flipping the accepting gate (a DoS / privilege gap). The TODO(kill-switch hardening) + reliance on agent-builder RBAC on a different endpoint is acceptable for a Phase-1 primitive only if the A2A surface exposing admin/kill is platform-only / in-cluster, not end-user-facing. If end users reach the same authenticated A2A server, the TODO should land before this is relied upon. Worth confirming the topology assumption.
Minor
- The killed-refusal returns
ErrCodeInternalon both handlers — semantically it is a deliberate unavailability, not an internal error; a more specific code reads better. Cosmetic.
Nice, clean core — CancelAll and the cause/reason propagation are exactly right. Findings 1 and 2 are the ones I would resolve before this ships as a relied-upon control.
| // JSON-RPC path goes through the same audit + accumulator wiring as | ||
| // REST POST /tasks/send. See issue #87 / FWS-3. | ||
| srv.RegisterHandler("tasks/send", func(ctx context.Context, id any, rawParams json.RawMessage) *a2a.JSONRPCResponse { | ||
| if r.killed.Load() { |
There was a problem hiding this comment.
Finding 1 (should-fix): this gate is applied here and on tasks/sendSubscribe (1688), but the two REST mirrors — POST /tasks/send (runner.go:2251) and POST /tasks/sendSubscribe (2304), both live via registerRESTHandlers — have no r.killed.Load() check. I read both: they decode the body and admit work unconditionally. So a killed agent still accepts new work over REST, silently defeating the accepting-gate contract on half the sync-A2A ingress surface. Add the same guard at the top of each REST handler. (I could not inline-anchor on 2251/2304 themselves — they are unchanged lines, not in this diff — hence the note here.)
| if idn := auth.IdentityFromContext(ctx); idn != nil { | ||
| caller = idn.Email | ||
| } | ||
| r.logger.Info("admin/kill", map[string]any{ |
There was a problem hiding this comment.
Finding 2 (should-fix): the kill action is recorded only on the ops logger, so it never lands in the tamper-evident audit NDJSON. You already capture caller here — good — but if the agent is idle (cancelled=0) the kill emits NO audit event at all (no per-task invocation_cancelled either), leaving a destructive admin action with no forensic trail and the actor absent from the signed chain. Emit an admin_kill audit event via EmitFromContext (caller / reason / cancelled) unconditionally, independent of in-flight count.
Review findings on #439: - Finding 1 (should-fix): the killed gate guarded only the JSON-RPC tasks/send + tasks/sendSubscribe; the REST mirrors POST /tasks/send and POST /tasks/sendSubscribe admitted work unconditionally, leaving half the sync-A2A ingress open on a killed agent. Guard both REST handlers (503). - Finding 2 (should-fix): admin/kill recorded the actor only on the ops logger. Emit a new admin_kill audit event via EmitFromContext UNCONDITIONALLY (caller / reason / cancelled) so a destructive admin action always lands in the tamper-evident chain, even when the agent was idle (cancelled=0) and no invocation_cancelled fires. - Minor: the killed refusal returned ErrCodeInternal; add a server-defined ErrCodeUnavailable (-32000) for deliberate unavailability and use it on both JSON-RPC gates (REST uses HTTP 503). New test drives admin/kill then asserts all four ingress paths refuse work (JSON-RPC Unavailable + REST 503); the NDJSON confirms admin_kill emits with a correct seq even when idle. build/vet/golangci-lint(0)/gofmt/test all green.
|
Addressed in
On Finding 3 (authz) — you're right to push here, and I want to flag it honestly rather than wave it through: a deployed agent's A2A Service is ClusterIP, but agents can be exposed via public ingress (our own demo agent had a All checks green (build/vet/golangci-lint 0/gofmt/test, both modules). |
initializ-mk
left a comment
There was a problem hiding this comment.
Fix verified — all three findings addressed cleanly. ✅ Traced 76a5894 against the branch source; all 9 CI checks green (incl. the Test job carrying the new integration test).
Finding 1 (REST gating) — resolved. Both POST /tasks/send (2261) and POST /tasks/sendSubscribe (2318) now check r.killed.Load() at the very top, before body decode, returning 503 Service Unavailable — symmetric with the JSON-RPC gate. All four sync-A2A ingress sites are now covered. The new TestRunner_KillSwitch_RefusesNewWorkOnEveryIngress is exactly the contract test this needed: it stands up a real Runner, trips admin/kill, then asserts JSON-RPC tasks/send → Unavailable and both REST endpoints → 503. (It exercises 3 of the 4 sites; the untested one, JSON-RPC sendSubscribe, shares the identical guard at 1688 — so all four are covered in code.)
Finding 2 (audit the kill) — resolved. New AuditAdminKill = "admin_kill" is emitted unconditionally via EmitFromContext(ctx, …) with caller / reason / cancelled, before the ops-log line. I confirmed auditLogger is a parameter of registerHandlers (runner.go:1633), so this is the real logger and the event rides the tamper-evident chain with correlation/tenancy/seq from ctx. An idle-agent kill (cancelled==0) now leaves a forensic record with the actor — the exact gap that was open.
Minor (error code) — resolved. ErrCodeUnavailable (-32000, in the JSON-RPC server-defined range) replaces the ErrCodeInternal misuse on the JSON-RPC refusals; REST uses HTTP 503. Semantically a deliberate refusal now, not an internal fault.
Finding 3 (authorization) remains the acknowledged TODO(kill-switch hardening) — appropriate to defer for this Phase-1 primitive, with the caveat noted earlier: land it before admin/kill is relied upon as a control if the A2A surface is end-user-reachable.
No new issues. Core mechanics (CancelAll cause/reason propagation, release-pops-entry, idempotency) were already correct and are unchanged. LGTM for the Phase-1 scope — nice, responsive iteration.
Match the <entity>_<verb-past> audit-event naming convention (cf. agent_killed, egress_blocked); admin_kill was present-tense. Renames the event string and the AuditAdminKilled constant.
Review findings on #439: - Finding 1 (should-fix): the killed gate guarded only the JSON-RPC tasks/send + tasks/sendSubscribe; the REST mirrors POST /tasks/send and POST /tasks/sendSubscribe admitted work unconditionally, leaving half the sync-A2A ingress open on a killed agent. Guard both REST handlers (503). - Finding 2 (should-fix): admin/kill recorded the actor only on the ops logger. Emit a new admin_kill audit event via EmitFromContext UNCONDITIONALLY (caller / reason / cancelled) so a destructive admin action always lands in the tamper-evident chain, even when the agent was idle (cancelled=0) and no invocation_cancelled fires. - Minor: the killed refusal returned ErrCodeInternal; add a server-defined ErrCodeUnavailable (-32000) for deliberate unavailability and use it on both JSON-RPC gates (REST uses HTTP 503). New test drives admin/kill then asserts all four ingress paths refuse work (JSON-RPC Unavailable + REST 503); the NDJSON confirms admin_kill emits with a correct seq even when idle. build/vet/golangci-lint(0)/gofmt/test all green.
Phase 1 (forge) of the agent kill switch — the runtime primitive the platform drives to disable an agent and kill all its active sessions/tasks. Sync A2A only in this PR; async (orchestrator) and the Claude SDK runtime follow in their own repos/PRs.
What this adds
forge-core/runtime—CancellationRegistry.CancelAll(reason) int+CancelReasonKillSwitch.CancelAllcancels every in-flight invocation at once (snapshot cancel-funcs under the lock, invoke them outside it — same contention profile asCancel; each invocation's deferredrelease()pops its own entry asexecuteTaskunwinds).forge-cli/runtime— the kill gate +admin/killverb. Akilled atomic.Boolon theRunner:admin/killJSON-RPC handler → setskilled, callsCancelAll(kill_switch), returns{killed, cancelled}. Each cancelled invocation emits its owninvocation_cancelledaudit event withreason=kill_switch(distinct from a per-task operator cancel).tasks/send+tasks/sendSubscriberefuse new work once killed (clear error, not a dropped socket).How the platform uses it (context, not in this PR)
agent-builder's admin-RBAC
POST /agents/{id}/killcallsadmin/killover the in-cluster A2A channel (graceful cancel + audit), then scales the Deployment to zero regardless of the result so no new transaction is admitted even if this call timed out. A mirroredkilledflag on the record/registry stops the orchestrator re-dispatching and the console from offering run/URLs.Auth
admin/killis behind the server-wideAuthMiddleware(only authenticated callers reach any handler). The primary access control is agent-builder's admin-RBAC on the/killendpoint.TODO(hardening): additionally restrictadmin/killto the platform/agent-runtime identity via the verified role claim (theIdentity.Claimsrole key needs settling first).Tests / checks
CancelAllsignals every in-flight invocation + propagateskill_switchviacontext.Cause; empty/second-kill returns 0;kill_switchis a valid reason.go build+go vetclean (forge-core, forge-cli);golangci-lint run ./runtime/...= 0 issues both modules;go test ./runtime/passes both modules;gofmtclean.Idempotency
A second kill re-signals an empty registry (returns 0) and leaves
killedset — safe for the platform to call optimistically.