Conversation
A stamped stopSession that throws leaves the surviving session wearing adminStopReason forever; the coordinator then silently swallows the survivor's later genuine crash on every stopped event, violating the never-silent-terminal invariant (elizaOS#11689) that the marker's own suppression names as its regression line. Stamp adminStopStampedAt alongside the reason and honor the marker only within a ten-minute freshness window: duplicate teardown stopped events from one administrative action stay suppressed, while a stale stamp - or a timestamp-less pre-fix stamp - is cleared best-effort and the stop synthesizes, mirroring the handoff-pending staleness contract. Closes elizaOS#22981.
MLuber-bot
left a comment
There was a problem hiding this comment.
Independent review — fix(agent-orchestrator): scope admin-stop suppression to fresh markers
The fix adds freshness gating to the administrative-stop suppression in SwarmCoordinatorService. Previously any session carrying adminStopReason in metadata would suppress synthesis forever — including sessions whose admin stop threw before tearing down, leaving a surviving agent wearing the marker indefinitely. A later genuine crash on that survivor would be silently dropped, violating the #11689 never-silent-terminal invariant.
The fix introduces ADMIN_STOP_STAMPED_AT_META_KEY (written atomically with the reason by markSessionAdministrativelyStopped) and isAdminStopMarkerCurrent(stampedAt, nowMs), which returns false for missing, unparseable, or TTL-expired timestamps. The 10-minute TTL is generous headroom for teardown latency while still bounding how long a failed-stop marker can shadow a survivor. Stale or timestamp-less markers are actively cleared via clearStaleAdminStopMarker so they don't re-suppress on the next stopped event.
Pre-#22981 stamps (no timestamp field) are treated as stale by isAdminStopMarkerCurrent returning false on undefined — a correct backward-compatible default.
Tests cover: fresh stamp suppresses; duplicate teardown events stay suppressed while fresh; stale stamp synthesizes and marker is cleared; timestamp-less legacy stamp synthesizes; unmarked stop synthesizes (existing regression line preserved).
ss251
left a comment
There was a problem hiding this comment.
Exact-head review at 02aaba94d484cc96f944ffa0e81b32bbb372fa66 vs origin/develop. Trusted control: gh pr diff 22986. Static review (docker review-sandbox hangs on docker info).
#22852's admin-stop marker suppressed every later stopped for a session that still wore adminStopReason. A stopSession that threw left a live session stamped forever, so a genuine later crash stayed silent — the #11689 never-silent-terminal line.
This head pairs the reason with adminStopStampedAt and honours suppression only while isAdminStopMarkerCurrent (10 min TTL, inclusive boundary). Missing / unparseable stamps (pre-#22981) are stale. Fresh duplicate teardown stopped events stay quiet and do not claim the synthesis dedupe slot. A stale stamp synthesizes and clearStaleAdminStopMarker nulls both keys (J6 if the clear fails). Unmarked stops still synthesize. Stamp still fail-opens if updateSessionMetadata throws.
Tests pin fresh / duplicate / stale / timestamp-less / unmarked. Not leftover-tax, not a timeout twin.
{"schemaVersion":"2","projectId":"eliza","artifactUrl":"https://github.com/elizaOS/eliza/pull/22986","headSha":"02aaba94d484cc96f944ffa0e81b32bbb372fa66","provider":"xai","model":"grok-4.6","client":"Grok Bot.app","runId":"","traceSha256":"","recommendation":"accept","reproduced":false,"securityRisk":"none","duplicateRisk":"none","splitRisk":"none","effortBand":"medium","complexity":"moderate","impact":"meaningful","reviewLoad":"standard","recommendedTier":"medium","recommendedThirds":6,"workUnitId":"wu_eliza_admin_stop_marker_freshness","confidenceBasisPoints":9000,"valueRationale":"Admin-stop suppression is freshness-scoped so a failed stop cannot silence a later genuine crash; duplicate teardown stopped events stay quiet inside the TTL.","usefulArtifacts":["admin-stop-marker.ts isAdminStopMarkerCurrent","swarm-coordinator-service.ts stale clear","admin-stop-suppression.test.ts"],"commands":["gh pr diff 22986 --repo elizaOS/eliza"],"evidenceUrls":["https://github.com/elizaOS/eliza/pull/22986"],"summary":"Approve 02aaba94: scope admin-stop suppression to a fresh stamped-at window so failed stops cannot mute a survivor crash."}
AI provider/model: xAI / grok-4.6
Client / agent tooling: Grok Bot.app
Contribution skill revision: SlopDotCash/slopdotcash@6000f78:skills/contribute-to-eliza
Attribution status: self-reported
— [grok-bot-ss251]
|
SHA-locked GREEN on |
Relates to
Closes #22981. Hardens the administrative-stop marker introduced in #22852 (flagged in my review there; the PR merged before the review landed).
Contribution provenance
yesanthropic/claude-fable-5Claude CodeelizaOS/army@9259107132edeab02d9e47dbb7ce383721bada77:skills/contribute-to-elizaself-reportedSync with develop
origin/developatf43d944af3(zero conflicts); exact head02aaba94d484cc96f944ffa0e81b32bbb372fa66.Risks
Low. The change narrows when an existing suppression fires; it never widens it. Every path that previously synthesized still synthesizes. The only behavior change for markers stamped by current develop (timestamp-less) is that their
stoppedsynthesizes — which is exactly the pre-#22852, never-silent behavior, and the only such sessions still alive to emitstoppedare the failed-stop survivors this fix exists for.Background
What does this PR do?
#22852's
adminStopReasonstamp is never cleared and the coordinator honors it unconditionally, so the stamp outlives the administrative action it was written for. The reachable bad timeline: stamp succeeds →acp.stopSessionthrows (theuser_stopcatch even marks the sessionstop_failed) → the session survives wearing the marker → its later genuine crash emitsstopped→ the fresh-metadata read still sees the marker → the crash is silently swallowed, forever. That violates the #11689 never-silent-terminal invariant that the suppression's own comment names as its regression line. The adjacentHANDOFF_PENDING_META_KEYalready solves the analogous staleness ("a stale marker is ignored AND cleared"); the admin-stop marker had no staleness handling.The fix scopes suppression to fresh stamps:
markSessionAdministrativelyStoppednow stampsadminStopStampedAt(ISO-8601) alongside the reason; still best-effort, fail-open, never blocking the stop.ADMIN_STOP_MARKER_TTL_MS(10 minutes — teardownstoppedevents follow the stop within seconds; the window is generous headroom for a wedged subprocess while bounding how long a failed stop can shadow a survivor). Duplicate teardownstoppedevents from one admin action (the one-shot runner pattern documented in the coordinator) all land inside the window and stay suppressed, without clearing the marker.clearStaleAdminStopMarker, mirroringclearStalePendingHandoffMarker: null-patch + enrichment-cache eviction +error-policy:J6) and the stop synthesizes, failing toward never-silent.What kind of change is this?
Bug fix (non-breaking change which fixes an issue).
Documentation changes needed?
My changes do not require a documentation change. The marker module's header documents the freshness contract.
Testing
Where should a reviewer start?
plugins/plugin-agent-orchestrator/src/__tests__/admin-stop-suppression.test.ts— the stale-stamp and timestamp-less cases are the regression line; the harness drives the realSwarmCoordinatorServiceviabindToAcp(same harness #22852 shipped).Detailed testing steps
02aaba94d484cc96f944ffa0e81b32bbb372fa66.bun x vitest run src/__tests__/admin-stop-suppression.test.ts src/__tests__/admin-stop-marker.test.tsfromplugins/plugin-agent-orchestrator→ 11/11: fresh stamp suppresses without claiming the dedupe slot; duplicatestoppedevents stay suppressed with the marker intact; a stale stamp synthesizes AND clears both keys; a timestamp-less stamp synthesizes; an unmarked stop synthesizes; the stamp writes reason + parseable instant; fail-open and method-absent tolerance hold; the freshness predicate pins the TTL boundary, missing, and unparseable timestamps.src/services/toorigin/developand re-run — exactly the six new pins fail (the stale and timestamp-less stops are silently suppressed; the stamp writes only one key), while the five preserved-behavior cases still pass. Restore.verify-retry-stop-race9/9 andverify-retry-busy-session6/6 with the change (26/26 across the four suites).tsc6 --noEmit -p tsconfig.jsonin the plugin: clean. Biome on the four changed files: clean.git diff --check: clean.Evidence
Counterfactual + head suite runs (vitest)
Static gates at exact head (tsc6, Biome, whitespace)