Skip to content

refactor(apple-runner): isolate startup budget mechanics - #3011

Merged
thymikee merged 1 commit into
mainfrom
codex/2967-start-budget
Sep 28, 2026
Merged

thymikee merged 1 commit into
mainfrom
codex/2967-start-budget

Conversation

@thymikee

@thymikee thymikee commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Summary

First review layer for #2967. Move the runner start budget, caller cancellation race, and detached-start diagnostics into runner-start-budget.ts. The session owner still registers the keyed lock before loading the budget code, preserving concurrent release ordering while keeping the new module out of public Apple facade import closures. No runner behavior or public API changes.

Rebased on current main at 3fe2e6929, final fb7244388: production +128/−118 (net +10); tests +2/−5; fixtures/docs 0; 3 files. The old in-file budget implementation is removed; the small net growth is the module boundary and its import. Rename-aware gross diff: 253 lines.

Validation

pnpm check:affected --run passed at fb7244388 (format, lint, typecheck, layering, fallow, build, 2,538 related tests). The speculative-start race test and 691 eager-closure assertions passed after moving the lazy load inside the lock. CI coverage, integration, and native lanes are pending; live device verification belongs to the completed top layer.

@thymikee
thymikee added this pull request to stack #3013 September 28, 2026 10:27
@thymikee thymikee changed the title codex/2967 start budget refactor(apple-runner): isolate startup budget mechanics Sep 28, 2026
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.88 MB 4.88 MB +916 B
Package (unpacked) 4.87 MB 4.88 MB +916 B
Package (download) 1.46 MB 1.46 MB +116 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 30.0 ms 29.6 ms -0.5 ms
CLI --help 82.0 ms 82.7 ms +0.8 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 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-start-budget.ts">

<violation number="1" location="packages/platform-apple/src/runner/runner-start-budget.ts:54">
P2: `openRunnerStartBudget` leaves the caller-signal listener installed after the start settles, because `close` only clears the timer. Repeated runner starts on one long-lived signal accumulate listeners and retain each filtered controller; return cleanup from the signal resolver and invoke it from `close`.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

exhausted.abort(runnerStartBudgetExhaustedError(timeoutMs, explicitTimeoutMs !== undefined));
}, timeoutMs);
timer.unref?.();
const startupSignal = resolveRunnerStartupSignal(options);

@cubic-dev-ai cubic-dev-ai Bot Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: openRunnerStartBudget leaves the caller-signal listener installed after the start settles, because close only clears the timer. Repeated runner starts on one long-lived signal accumulate listeners and retain each filtered controller; return cleanup from the signal resolver and invoke it from close.

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-start-budget.ts, line 54:

<comment>`openRunnerStartBudget` leaves the caller-signal listener installed after the start settles, because `close` only clears the timer. Repeated runner starts on one long-lived signal accumulate listeners and retain each filtered controller; return cleanup from the signal resolver and invoke it from `close`.</comment>

<file context>
@@ -0,0 +1,124 @@
+    exhausted.abort(runnerStartBudgetExhaustedError(timeoutMs, explicitTimeoutMs !== undefined));
+  }, timeoutMs);
+  timer.unref?.();
+  const startupSignal = resolveRunnerStartupSignal(options);
+  const signal = startupSignal
+    ? AbortSignal.any([startupSignal, exhausted.signal])
</file context>
Fix with cubic

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 5f62db4. The startup budget move is behavior-neutral and I found no blocking problem.

Not blocking: the two dynamic imports of runner-start-budget.ts in runner-session.ts (lines 141 and 168) defer nothing, because runner-session.ts already imports everything that module imports statically, so a static import with a +1 eager-closure budget row would drop an await from every ensureRunnerSession. Also, runner-start-budget.ts line 12 redeclares RunnerSessionOptions; it can use AppleRunnerLifecycleOptions directly.

CI: checks are still queued. Coverage, Integration and Repo Guards exercise this route, so their results apply to this change.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 28, 2026
@thymikee
thymikee force-pushed the codex/2967-start-budget branch from 5f62db4 to e8f1a11 Compare September 28, 2026 15:36
@thymikee
thymikee force-pushed the codex/2967-start-budget branch from e8f1a11 to fb72443 Compare September 28, 2026 16:54
@thymikee
thymikee merged commit 6542226 into main Sep 28, 2026
19 checks passed
@thymikee
thymikee deleted the codex/2967-start-budget branch September 28, 2026 18:01
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-28 18:02 UTC

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

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant