fix(apple-runner): stop an inline status probe clearing an outstanding command charge - #2994
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/runner/runner-command-accounting.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-command-accounting.ts:112">
P2: `settleAnswered` forgives the oldest abandoned residue in the lane without requiring it to predate the exchange being answered. If commands run concurrently on one session and a newer exchange (cmd-b) drops its transport while an older exchange (cmd-a) is still in flight, cmd-a's later answer discharges cmd-b's abandoned charge even though the runner's serial queue places cmd-b after cmd-a — so cmd-a's answer proves nothing about cmd-b. That is the same wrong-discharge class this PR eliminates for probe traffic, and it can clear a charge for a command the runner may still be executing, allowing the handoff mid-mutation. The handful of tests pin only the in-order case (answered exchange newer than the forgiven residue); the out-of-order case is unpinned.</violation>
<violation number="2" location="packages/platform-apple/src/runner/runner-command-accounting.ts:128">
P1: Malformed response envelopes can discharge an outstanding command here. `parseRunnerResponse` turns valid non-object JSON such as `null` into `{}`, and `buildRunnerResponseError` attaches it as `details.runner`. This branch therefore treats an unstructured response as answered and may permit handoff while queued work still executes; require a validated runner envelope before settling.</violation>
</file>
<file name="packages/platform-apple/src/runner/runner-command-recovery.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-command-recovery.ts:196">
P3: `RUNNER_TERMINAL_LIFECYCLE_STATES` and `handleRunnerCommandStatusRecovery` encode the same lifecycle vocabulary as two separate literal lists ('completed'/'failed' in the set, plus 'accepted'/'started' in the handler's branches). Only the new set is pinned to the Swift journal declaration; the handler's literals are not derived from it, so a state change on either side (or a new terminal state) can make settlement and the invalidation/recovery verdict diverge silently. Pull the handler's laborious branches off the same documented set, or at least key both off one exported constant.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| route: RunnerCommandChargeRoute, | ||
| error: unknown, | ||
| ): void { | ||
| if (isStructuredRunnerFailure(error)) this.settleAnswered(commandId, route); |
There was a problem hiding this comment.
P1: Malformed response envelopes can discharge an outstanding command here. parseRunnerResponse turns valid non-object JSON such as null into {}, and buildRunnerResponseError attaches it as details.runner. This branch therefore treats an unstructured response as answered and may permit handoff while queued work still executes; require a validated runner envelope before settling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/runner/runner-command-accounting.ts, line 128:
<comment>Malformed response envelopes can discharge an outstanding command here. `parseRunnerResponse` turns valid non-object JSON such as `null` into `{}`, and `buildRunnerResponseError` attaches it as `details.runner`. This branch therefore treats an unstructured response as answered and may permit handoff while queued work still executes; require a validated runner envelope before settling.</comment>
<file context>
@@ -0,0 +1,195 @@
+ route: RunnerCommandChargeRoute,
+ error: unknown,
+ ): void {
+ if (isStructuredRunnerFailure(error)) this.settleAnswered(commandId, route);
+ else this.markAbandoned(commandId, route);
+ }
</file context>
There was a problem hiding this comment.
Valid and fixed at d420419fb. The root cause was the shared decoder, not this branch: decodeRunnerResponseBody returned {} for any non-object JSON and buildRunnerResponseError attached it as details.runner, so isStructuredRunnerFailure read a proxy page or a half-written body as an answer. It now throws COMMAND_FAILED for JSON scalars and arrays via isRunnerEnvelopeObject, so all three readers agree a non-object body answered nothing and the ledger only sees a real envelope. Pinned in runner-response.test.ts: null, 42, "ok" and [] throw, and a null body is asserted transport-shaped rather than structured. Restoring the old ternary turns three of those tests red.
| settleAnswered(commandId: string | undefined, route: RunnerCommandChargeRoute): void { | ||
| const answered = this.takeCharge(commandId, route); | ||
| if (!answered || answered.abandoned) return; | ||
| const residue = this.charges.find((charge) => charge.abandoned && charge.route === route); | ||
| if (residue) this.charges.splice(this.charges.indexOf(residue), 1); | ||
| } | ||
|
|
||
| /** | ||
| * Settles the charge for an exchange that ended outside the success path. A structured runner reply |
There was a problem hiding this comment.
P2: settleAnswered forgives the oldest abandoned residue in the lane without requiring it to predate the exchange being answered. If commands run concurrently on one session and a newer exchange (cmd-b) drops its transport while an older exchange (cmd-a) is still in flight, cmd-a's later answer discharges cmd-b's abandoned charge even though the runner's serial queue places cmd-b after cmd-a — so cmd-a's answer proves nothing about cmd-b. That is the same wrong-discharge class this PR eliminates for probe traffic, and it can clear a charge for a command the runner may still be executing, allowing the handoff mid-mutation. The handful of tests pin only the in-order case (answered exchange newer than the forgiven residue); the out-of-order case is unpinned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/runner/runner-command-accounting.ts, line 112:
<comment>`settleAnswered` forgives the oldest abandoned residue in the lane without requiring it to predate the exchange being answered. If commands run concurrently on one session and a newer exchange (cmd-b) drops its transport while an older exchange (cmd-a) is still in flight, cmd-a's later answer discharges cmd-b's abandoned charge even though the runner's serial queue places cmd-b after cmd-a — so cmd-a's answer proves nothing about cmd-b. That is the same wrong-discharge class this PR eliminates for probe traffic, and it can clear a charge for a command the runner may still be executing, allowing the handoff mid-mutation. The handful of tests pin only the in-order case (answered exchange newer than the forgiven residue); the out-of-order case is unpinned.</comment>
<file context>
@@ -0,0 +1,195 @@
+ * the serial queue, so it forgives no queued residue, and the command it probed stays charged until
+ * its own answer or its own terminal evidence lands (#2965).
+ */
+ settleAnswered(commandId: string | undefined, route: RunnerCommandChargeRoute): void {
+ const answered = this.takeCharge(commandId, route);
+ if (!answered || answered.abandoned) return;
</file context>
| settleAnswered(commandId: string | undefined, route: RunnerCommandChargeRoute): void { | |
| const answered = this.takeCharge(commandId, route); | |
| if (!answered || answered.abandoned) return; | |
| const residue = this.charges.find((charge) => charge.abandoned && charge.route === route); | |
| if (residue) this.charges.splice(this.charges.indexOf(residue), 1); | |
| } | |
| /** | |
| * Settles the charge for an exchange that ended outside the success path. A structured runner reply | |
| settleAnswered(commandId: string | undefined, route: RunnerCommandChargeRoute): void { | |
| const answeredIndex = this.findChargeIndex(commandId, route); | |
| if (answeredIndex === -1) return; | |
| const [answered] = this.charges.splice(answeredIndex, 1); | |
| if (answered.abandoned) return; | |
| const residueIndex = this.charges.findIndex( | |
| (charge, index) => charge.abandoned && charge.route === route && index < answeredIndex, | |
| ); | |
| if (residueIndex !== -1) this.charges.splice(residueIndex, 1); | |
| } |
There was a problem hiding this comment.
Valid, and the wrong-discharge it describes is real. Fixed at d420419fb: settleAnswered records charges in send order and forgives only a residue that predates the answer — index < answeredIndex && charge.abandoned — so cmd-a's answer can no longer clear cmd-b's abandoned charge even though both sit in one ledger. runner-session-types.test.ts's "does not forgive an abandoned charge sent after it" pins exactly your cmd-a/cmd-b interleaving; dropping the index < answeredIndex guard turns it red, which I verified. An answer landing on a charge already marked abandoned is treated as that exchange's own late reply and forgives nothing further.
| * `started` are written as execution opens, and `notAccepted` is what `status` reports for an id the | ||
| * journal does not hold — none of them says the command finished. | ||
| */ | ||
| const RUNNER_TERMINAL_LIFECYCLE_STATES: ReadonlySet<string> = new Set(['completed', 'failed']); |
There was a problem hiding this comment.
P3: RUNNER_TERMINAL_LIFECYCLE_STATES and handleRunnerCommandStatusRecovery encode the same lifecycle vocabulary as two separate literal lists ('completed'/'failed' in the set, plus 'accepted'/'started' in the handler's branches). Only the new set is pinned to the Swift journal declaration; the handler's literals are not derived from it, so a state change on either side (or a new terminal state) can make settlement and the invalidation/recovery verdict diverge silently. Pull the handler's laborious branches off the same documented set, or at least key both off one exported constant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/runner/runner-command-recovery.ts, line 196:
<comment>`RUNNER_TERMINAL_LIFECYCLE_STATES` and `handleRunnerCommandStatusRecovery` encode the same lifecycle vocabulary as two separate literal lists ('completed'/'failed' in the set, plus 'accepted'/'started' in the handler's branches). Only the new set is pinned to the Swift journal declaration; the handler's literals are not derived from it, so a state change on either side (or a new terminal state) can make settlement and the invalidation/recovery verdict diverge silently. Pull the handler's laborious branches off the same documented set, or at least key both off one exported constant.</comment>
<file context>
@@ -179,6 +185,40 @@ async function tryRecoverRunnerCommandAfterTransportError(
+ * `started` are written as execution opens, and `notAccepted` is what `status` reports for an id the
+ * journal does not hold — none of them says the command finished.
+ */
+const RUNNER_TERMINAL_LIFECYCLE_STATES: ReadonlySet<string> = new Set(['completed', 'failed']);
+
+/**
</file context>
There was a problem hiding this comment.
Fixed at d420419fb, at the shared vocabulary rather than the two sites. runner-command-recovery.ts now declares RUNNER_TERMINAL_LIFECYCLE_STATES and RUNNER_IN_FLIGHT_LIFECYCLE_STATES side by side, and handleRunnerCommandStatusRecovery routes accepted/started through the in-flight set instead of its own literals, so settlement and the invalidation verdict read one declaration. completed and failed keep separate branches only because their verdicts genuinely differ (a completed entry replays its payload, a failed one reports the runner's error). A state the runner adds cannot slip through silently: both sets and the recovery rows are pinned to Swift's own RunnerCommandLifecycleState by requireLifecycleSettlementRows, which asserts in both directions, so an unread state fails as a missing row.
|
Reviewed at 9fb94ee. The fix stops a queued reply from clearing an inline probe's charge, but the new runner-command-accounting.ts module puts a fresh static edge into the eager-closure of every Apple façade that reaches runner-session.ts, and CI Coverage is failing on that: scripts/tests/eager-closure-budgets.test.ts names the exact route this PR adds at https://github.com/callstack/agent-device/blob/9fb94ee/packages/platform-apple/src/runner/runner-session.ts#L48 and https://github.com/callstack/agent-device/blob/9fb94ee/packages/platform-apple/src/runner/runner-adoption.ts#L12, so this looks caused by the diff rather than a flake. Could the ledger live in runner-session-types.ts next to resolveRunnerDetachDecision instead, since that module is already in the closure and is the ledger's only reader, and could inline (readiness-probe) exchanges go uncharged altogether the way the preflight uptime already is, so the route field, the blank-id lane, and the inline-residue forgiveness in https://github.com/callstack/agent-device/blob/9fb94ee/packages/platform-apple/src/runner/runner-command-accounting.ts#L115 simply disappear and an inline answer settles nothing? I did not run the tests or the author's mutation of the extraction-commit code; the regression read comes from tracing the pre-change arithmetic by hand. Whether the abandoned-inline-probe scenario is reachable in practice depends on how a request-cancelled uptime or prewarm skips session invalidation, and I did not confirm how often that path is hit; the retry and cancel paths I checked do invalidate the session. The lost-response case was exercised only through the fake-runner harness via runAppleRunnerCommand, not on a live device; device lanes passed but don't assert this handoff. The concurrency and null-envelope questions raised elsewhere both sit on predicates this PR didn't introduce, and I haven't verified whether one runner session can see concurrent executeRunnerCommandWithSession calls. Before this can merge, the eager-closure-budgets check needs to go green, which means moving the ledger into a module the closure already evaluates and rerunning Coverage. Not blocking: the accounting module's comments read like issue history rather than invariants and one test comment contradicts its own assertion, RUNNER_TERMINAL_LIFECYCLE_STATES duplicates the completed/failed branching already in runner-command-recovery.ts, and the Swift fixture walker in runner-swift-settlement-fixtures.ts skips some multi-line case-arm shapes even though today's fixtures still parse correctly — all take-it-or-leave-it. |
9fb94ee to
d420419
Compare
|
Reworked along exactly the shape you proposed, at Blocker. The ledger now lives in Design. Inline (readiness-probe) exchanges are uncharged, keyed on the Non-blocking nits, all taken: the ledger's comments state invariants rather than issue history; the contradicting test comment is gone and that test now actually pins the invariant its comment names ( On your reachability caveat — agreed it was traced, not device-observed; the PR body records the same unresolved risk. The handoff rows are pinned through |
|
Reviewed at d420419. The fix looks right: the status probe no longer clears an outstanding command charge, so a real reply can still settle it. This is ready for human review. The pre-change arithmetic trace (regression tests going red on the old code, dropping the ordering guard, markAbandoned taking another charge) comes from hand-tracing, not a run. The Swift fixture walker was only checked against today's inlineResponse(for:) arms. The shutdown handoff is proven only through the fake-runner harness via runAppleRunnerCommand; no device run confirms the abandoned-exchange-then-uptime path in practice. Whether one runner session can see concurrent executeRunnerCommandWithSession calls wasn't checked either; per-id charges make it less likely but don't rule it out. Not blocking: charge(commandId) could take a non-optional string since withRunnerCommandId always stamps the id before charging, letting you drop the no-id test at packages/platform-apple/src/runner/runner-session-types.ts#L235; and the PR body could mention the decodeRunnerResponseBody tightening at packages/platform-apple/src/runner/runner-contract.ts#L40, which is new in this revision and tested, separate from the unrelated APP_NOT_RUNNING_RUNNER_CODE un-export that can stay or go — take or leave either. Smoke Tests is still running on d420419 with no failure yet; since this diff touches the charge/settle path around every runner send that the iOS smoke lanes exercise, a failure there would need reading before calling it unrelated. Wait for that to finish green before merging. |
thymikee
added
the
ready-for-human
d420419 to
595b9e5
Compare
|
Follow-up: pushed |
|
CI note on |
595b9e5 to
285ee8a
Compare
|
Sized down for #2803 at
No regression in the fix: each mutation still turns tests red — charging probes (6), forgiving later residue (1), terminal evidence consuming an awaited charge (2), terminal evidence ignoring the id (2), |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
285ee8a to
5c63249
Compare
|
Reviewed at 5c63249. The code looks correct: the inline status probe still does not clear an outstanding command charge, and the boolean This delta changes no device-facing behavior, so the live evidence from the 595b9e5 review still covers it. The mutation and CI: Smoke Tests, Repo Guards, Coverage and CodeQL were still running when I looked. None had failed. These jobs exercise the changed route, so their result is the remaining gate. Two optional notes. The |
5c63249 to
6902f10
Compare
|
Reviewed at 6902f10. This is clean. The new The Smoke Tests failure looks unrelated. That job timed out waiting for "Agent Device Tester" in I could not reproduce the old regex blowing up with synthetic inputs of about 50 arms. Do you have a captured slow input that is worth pinning as a regression check? This is not blocking. Nothing here blocks a human review. |
|
Final head for this round is All checks green on |
|
2 participants
Summary
The runner answers the
statusanduptimereadiness probes inline — off its journal and off its serial command queue — so a probe reply is no evidence about queued work. The session charged every request alike, so a probe's answer could discharge a mutation this process had already given up on, and graceful shutdown handed a runner to the next daemon mid-mutation.Only queued commands are charged now. A charge is released by that command's own answer or its own terminal journal evidence; a queued answer forgives at most one abandoned charge sent before it. Per review, the ledger lives in
runner-session-types.tsbesideresolveRunnerDetachDecision— its only reader, and already in the Apple eager closure — so the PR adds no static edge and the route/lane machinery is gone. A JSON body that is not an object is now transport-shaped rather than an empty runner envelope, so it cannot settle a charge.17 files: 5 production (+270/−96).
Validation
At
6902f103e:pnpm check:affected --rungreen (394 files, 2,766 tests);check:fallow --base origin/mainclean;eager-closure-budgets.test.tsgreen at 691 tests;check:production-exportsunchanged at 68. Handoff rows are derived from the runner journal's ownRunnerCommandLifecycleStatedeclaration, and the daemon'sreadinessProbetrait is pinned to the runner'sinlineResponse(for:)arms. Named mutations each turn a test red: charging probes, forgiving later residue, terminal evidence consuming an awaited or a stranger's charge,markAbandonedguessing an id.Unresolved risk: handoff refusal is exercised through the fake-runner harness; no device lane asserts it.