Skip to content

fix(apple-runner): inline status probes must not clear outstanding command charges #2965

Description

@thymikee

Problem

Deletion-first acceptance

This is part of #2803's simplify-and-shrink initiative. Before production simplification, name the production mechanism, duplicated decision, state, fallback, forwarding layer, or public/internal interface this change removes. Compare deletion and inlining against extraction. Moving the same state into another file is not a deletion case.

Every PR reports baseline/final SHAs; added, deleted and net production lines; test/fixture/documentation changes separately; and the exact mechanisms removed. Use a rename-aware diff (git diff --numstat -M BASE...HEAD) and account for all affected production paths, including destination packages. Moving tests out of production files, generated output, formatting churn, and moving code elsewhere are not production-code deletion. Preserve readable code; no compressed formatting to hit a number.

Default for simplification work: net production reduction. A required behavior-preserving size split may be line-neutral. Growth needs an explicit, quantified explanation of the correctness or type guarantee it buys, why deletion/inlining cannot achieve it, and what old mechanism is removed; it is not automatically acceptable because ownership looks cleaner. If the only benefit is relocating fields or adding a wrapper, defer the change. Required regression tests and a minimal correctness fix are allowed to grow without inventing unrelated deletion to offset them.

Priority and scope gate

Priority: first correctness work. The accounting-model decision is still required; code reduction must not weaken command-specific, exactly-once settlement. Resolve the transition table first, then implement the smallest owning fix. Implementation scheduling is blocked by #2963 merging (or an explicitly declared stack on its exact published head), because both edit runner-session state and lifecycle tests; the design table can be completed now. #2911 is already merged. Only the minimal accounting extraction needed by the >1,000-line rule belongs in this fix; the broader split remains #2967.

A successful inline runner probe can clear the host's outstanding-command accounting while the command it probes is still executing. Graceful shutdown can then consider that runner eligible for handoff.

Verified by source inspection against main at 1206495222385424d96b11bd9c9be25eb267455e; no live-device reproduction yet. The trigger is a retryable post-send transport failure while the request signal still permits a status probe. An already-aborted request generally makes the probe fail and follows existing invalidation instead.

The path is:

  • packages/platform-apple/src/runner/runner-command-recovery.ts sends status through executeRunnerCommandWithSession.
  • runner-session.ts calls settleRunnerCommandAnswered for a decoded answer, settling both this exchange and one previous abandoned charge.
  • Swift RunnerTests+Transport.swift serves status and uptime inline, outside the serial command queue. RunnerTests+MainThreadWork.swift reports busy only for watchdog-abandoned work, so lifecycleState: 'started' with runnerMainThreadBusy: false is legitimate.
  • runner-session-types.ts then allows handoff because the outstanding count, abandoned flag, and busy flag all read idle.

Current accounting for one lost mutation response:

Step inFlightCommands hasAbandonedCommands
Mutation sent 1 false
Transport fails 1 true
Status sent 2 true
Status answers started, busy=false 0 false

The last row must retain the original outstanding charge. runner-session-lifecycle.test.ts currently asserts that any answered exchange forgives it; recovery-only fixtures start with zero charges and do not catch this composition.

Required change

  1. Extract the command-charge accounting from the 1,353-line runner-session.ts into a cohesive module under the same runner directory, with its matching test. Preserve behavior in the extraction, then apply the fix there. This satisfies the repository's split-before-behavior rule without requiring the full session/exchange redesign. The existing session remains the sole state owner; do not add a parallel registry.
  2. Settle an inline probe's own exchange without forgiving earlier abandoned work. Use the existing readinessProbe trait for status and uptime, and pin its correspondence with Swift inline routing in a contract test; do not add another command-name allowlist.
  3. In status recovery, terminal evidence (completed or failed) for the affected command may settle its abandoned charge exactly once. accepted and started must retain it. Unknown/missing status and probe failures keep their current invalidation policy. Repeated evidence must not discharge another command's charge.
  4. Preserve ordinary queued-answer settlement: the serial queue makes a later queued answer evidence that earlier queued handling finished. Preserve separate native busy accounting; terminal journal status alone must never bypass runnerMainThreadBusy or other handoff refusals.

Do not change the wire format, Swift execution/occupancy semantics, mutation replay policy, startup/adoption/disposal, or device-specific handoff eligibility. Do not broaden this into the full exchange extraction, target-state refactor, or a new command-tracking framework unless the regression demonstrates the existing representation cannot meet this contract.

Acceptance and validation

Extend the existing fake-runner coverage in runner-recovery-wiring.test.ts, entering through runAppleRunnerCommand with real command-ID assignment, send, parse, and recovery. Mock process creation as the harness already does. Assert the resulting production handoff decision; helper-only tests are insufficient.

  • Lost mutation response → status accepted or started, busy=false: original charge remains and handoff refuses with command_in_flight; mutation was sent once.
  • Lost mutation response → status completed or failed for that command: charge settles once; normal response/error semantics remain, and handoff is permitted only when all other conditions allow it.
  • uptime never forgives an abandoned command. Repeated terminal evidence cannot consume another outstanding charge.
  • Terminal status with busy=true still refuses handoff. A later ordinary queued answer retains the existing conservative settlement behavior.
  • Cancellation/probe failure and existing healthy simulator/physical handoff cases retain their behavior.
  • Observe the new wiring regression fail on the extracted pre-fix behavior and pass with the fix. Update the existing overly broad lifecycle assertion, retaining ordinary queued-answer coverage.

For a fresh worktree, first run pnpm install --frozen-lockfile && pnpm build. Run focused accounting, recovery-wiring, recovery, session-lifecycle, and command-trait tests with pnpm exec vitest run --project apple-runner <test-paths>, then pnpm check:affected --run on the final diff. Preserve selected CI/device obligations and report fake-runner evidence separately from any live-device verification. No new test lane is needed.

Implementation-readiness gate

The primary failure is established, but this issue is not yet AFK-ready. Before implementation, specify the identity and settlement model inside the existing session; the current inFlightCommands counter and hasAbandonedCommands boolean cannot enforce the command-specific, exactly-once requirement above.

Resolve these cases in a transition table:

  • Abandon command A, abandon command B, then queued command C answers. Current conservative behavior forgives one abandoned charge: which command's charge remains?
  • Terminal status for A arrives after that sequence, then arrives again. Each observation must have one unambiguous effect and must not consume B's charge.
  • Multiple exchange attempts share one logical commandId (read-only retries reuse it). State whether terminal evidence settles one attempt or all abandoned attempts for that command; live waits must remain charged until their own exchange settles.

A small identity-bearing record owned by the existing session is permitted if needed; a second registry or generic tracking framework is not. Choose and independently check these observable outcomes before restoring ready-for-agent. The full session/exchange extraction is not a prerequisite.

Dependencies: the accounting design gate remains open; implementation scheduling also follows #2963 as specified above. The broader session/resource versus command-exchange split is a separate follow-up.

Architecture follow-through

Parent: #2803. The broader command-exchange extraction is downstream and must start from this issue's exact merged implementation. For every row in the accounting design gate, record the resulting outstanding exchange identities/charges and the production handoff verdict. Independently check that table before restoring ready-for-agent; no production implementation is authorized by treating unspecified identity semantics as an implementation detail.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingneeds-triageNew or unreviewed issue

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions