Skip to content

perf(daemon): stop eagerly loading the CLI schema layer and screenshot surface - #3373

Merged
thymikee merged 1 commit into
mainfrom
perf/eager-closure-cuts
Oct 11, 2026
Merged

thymikee merged 1 commit into
mainfrom
perf/eager-closure-cuts

Conversation

@thymikee

@thymikee thymikee commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Summary

The daemon's eager closure carried two edges that did not match what their callers needed.

Plugin discovery imported the CLI config loader for one path helper, resolveUserConfigPath, pulling option schemas and every command family into daemon startup. The helper needs only env and the home expansions it already sat beside, so it now lives in @agent-device/host-kit/file, which both callers already imported. That plugins -> commands value edge was the zone graph's only one between those zones; it is gone.

The generic dispatcher imported the screenshot runtime statically while awaiting it on a request-only branch, beneath which the leaf owns an in-process command surface. It now loads at the branch.

8 files, no scope beyond these two edges.

Validation

files loc
merge-base 6eef69a9f8 651 103,666
head 37b22baf99 468 76,230

pnpm depgraph, over value edges from src/daemon.ts; both cuts are needed for the full number. Rebased onto current main, which added eagerly-imported platform files; the cut holds.

pnpm check:affected --run on 37b22baf99: all runnable checks passed, plus layering, format, lint, typecheck and the eager-closure budget gate, no stale approval row.

The new completeness test enters the screenshot arm, which no existing test did, and fails on a typo'd specifier. The shipped bundle resolves the specifier relative to the awaiting chunk, and the Linux replay lane captured real screenshots through the changed arm.

loc is a proxy, not a timing claim: the distribution bundles packages, so no startup-latency number is asserted.

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.16 MB 5.17 MB +3.1 kB
Package (unpacked) 5.16 MB 5.17 MB +3.1 kB
Package (download) 1.56 MB 1.56 MB -219 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 21.1 ms 21.2 ms +0.0 ms
CLI --help 57.9 ms 59.0 ms +1.1 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.

All reported issues were addressed across 8 files

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

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/host-kit/src/internal/path-resolution.test.ts Outdated
Comment thread src/daemon/generic-runtime-execution.ts Outdated
@thymikee
thymikee force-pushed the perf/eager-closure-cuts branch from dcc9524 to 72f840a Compare October 10, 2026 15:22
@thymikee

Copy link
Copy Markdown
Member Author

Review comments addressed in 72f840ab02 (both threads resolved; check:affected --run re-run on this head — all runnable checks passed, 786 files / 6494 tests, and pnpm depgraph still reports 468 eager files).

On the size report's startup medians: those two scenarios exercise the CLI surface, which does not traverse the closure this PR trims, so they neither confirm nor refute the claim. They do show no regression there (+1.5 ms on --help, within a 7-run spread; --version unchanged), and installed size is flat. The eager-closure figure remains a module-count measurement of src/daemon.ts over value edges, not a wall-clock startup claim, exactly as the body says.

@thymikee

Copy link
Copy Markdown
Member Author

The red iOS Smoke Tests lane on this head was a one-off, not a regression from this change.

gh run rerun --failed on the unchanged head 72f840ab02 passed (run 38063373947), and every check run now reports success or skipped. All checks green, no code change since 72f840ab02.

Why it could not have been caused by the diff. The failing step was the open --relaunch --launch-url step of smoke:regular-visible-depth-frontier, and the failing process was xcrun simctl openurl returning NSPOSIXErrorDomain code 60 after ~13 s — CoreSimulator gave up before our own 20 s IOS_SIMULATOR_OPENURL_TIMEOUT_MS bound fired, so no agent-device timeout path was involved. The scenario exercises open and snapshot, neither of which reaches a module this PR changed: from src/daemon.ts over value edges on this head, daemon/screenshot-runtime.ts and commands/schema/cli-config.ts are absent from the eager closure.

The risk that deferring 183 files could actually carry is losing an import-time registration. I enumerated all 183 removed modules and scanned each for module-scope side-effect statements: none. Deferral therefore cannot drop a registration the open path depended on.

The scenario also failed at different global step numbers across the two attempts (83 then 93), consistent with a host-side CoreSimulator stall in the area upstream is actively settling (#3356, #3354).

@thymikee

Copy link
Copy Markdown
Member Author

The code in 72f840a looks right to me, but I'm holding the final call until two checks that were not run get run. I did not re-run the base closure count (651/102,898); I reproduced only the head count, from an exported copy of the commit. I also did not build the dist bundle, so nobody has checked that the deferred ./screenshot-runtime.ts chunk resolves in the shipped output. I relied on the package check and smoke lanes for that. I did not re-run the test suite either, and the claim that the new test fails on a typo'd specifier comes from reading the code path, not from a mutation run. Could you confirm the base count and that the built daemon loads the screenshot chunk?

All 19 checks pass on this head. One iOS Smoke lane failed earlier at xcrun simctl openurl (NSPOSIXErrorDomain 60) in the regular-visible-depth-frontier step and passed on rerun of the same head, and the diff does not touch that route. There are no conflicts, and nothing else stands between this and merge once the two checks above are in.

Not blocking, and you can take or leave these: the docblock on resolveUserConfigPath in path-resolution.ts could be one line stating the contract, without the zone count that will go stale. The comment above the dynamic import in generic-runtime-execution.ts and the new test comment at generic-route-runtime-completeness.test.ts:149 narrate more than AGENTS.md wants, so one line or nothing is enough.

Is there a simpler shape than the two edge cuts here? I looked and found none. The helper could not stay in cli-config.ts, because that file pulls in the schema layer.

Both earlier inline threads are fixed at this head, so you can resolve them: the HOME pin in the path-resolution tests (#3373 (comment)) and the timing-narration comment (#3373 (comment)).

…t surface

Plugin discovery imported the CLI config loader for one path helper, dragging
option schemas and every command family into the daemon's eager closure; that
plugins -> commands edge was the zone graph's only one. The helper needs only
env and the home expansions, so it now lives beside them in host-kit, which
both callers already imported.

The generic dispatcher imported the screenshot runtime statically while
awaiting it on a request-only branch. Behind that branch it owns the in-process
command surface and the interaction implementations beneath it.

Measured over value edges from src/daemon.ts: 651 files / 103,629 loc -> 468
files / 76,166 loc. Both cuts are needed for the full win; seven files drop
only when neither path stays eager.
@thymikee
thymikee force-pushed the perf/eager-closure-cuts branch from 72f840a to 37b22ba Compare October 10, 2026 17:04
@thymikee

Copy link
Copy Markdown
Member Author

Correction + full diagnosis of the iOS Smoke Tests lane.

My earlier comment claimed the red lane was a one-off. It is not: across four attempts the lane failed three times, in three different places.

  • attempt 1 dcc95248fb — smoke:regular-visible-depth-frontier, xcrun simctl openurl → NSPOSIXErrorDomain code 60
  • attempt 2 72f840ab02 rerun — passed
  • attempt 3 37b22baf99 — preflight prepare ios-runner, daemon_startup_failed (15 s budget, 1 attempt)
  • attempt 4 37b22baf99 rerun — smoke:automation-input, wait text Automation lab → "app is not running"

The lane is pre-existing flaky infrastructure, and I should have checked its history before reasoning from first principles. It has failed the same way on unrelated branches and on main itself: run 37975032853 (main) failed with smoke:automation-input, run 37979887204 (a docs-only PR) failed with Failed to start daemon. Same two failure modes, same lane, nothing resembling this diff.

For the startup failure specifically I measured rather than argued, A/B on the built artifact (5 boots each, fresh state dir, timing until daemon.json appears): base c2c09d476a median 210 ms with a 977 ms outlier; this head median 178 ms, max 182 ms. Registration got faster and tighter against a 15 s budget, so this diff is not plausibly what makes a daemon miss it.

One correction to a claim in my previous comment. I said I had scanned the 183 deferred modules for import-time side effects and found none. My filter skipped export const X = f(...), so it under-reported: 94 of the 183 do run work at import time (command-family and facet builders, frozen constant tables).

Re-scanned properly, the property that actually matters is that none of it is a hidden registration. There are zero bare side-effect imports (import './x.ts') among the 183, and every builder they evaluate is consumed by explicit named import, so a module's exports are only reachable by a consumer that asks for them — nothing is registered by the act of loading. packages/command-registry/src/registry.ts, which holds the descriptors the daemon's route-completeness checks read, remains eager. The families and facets themselves stay in the closure that cli.ts evaluates — the CLI needs them for help and parsing — and drop out of the daemon's, where they are reached through the request path instead. That asymmetry is the reduction this PR measures. So deferral still cannot drop a registration, but the reason is the absence of self-registration, not the absence of import-time work.

@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready. At 37b22ba the evidence asked for earlier is reproduced: the base count and the built chunk both resolve and load. The two inline fixes also landed, and the two cubic-dev-ai threads (#3373 (comment) and #3373 (comment)) are fixed at this head, so the author can resolve them.

Not blocking: the comment at src/daemon/tests/generic-route-runtime-completeness.test.ts#L66 says a bundler that dropped the chunk would fail the test, but the test imports from source, so it never loads the built chunk; drop that phrase or add a packaging check that imports the built screenshot-runtime chunk. Take it or leave it.

All 19 checks pass or are skipped. The iOS Smoke lane failed earlier in three places, and the author reports the same failures on main. It is green on this head, and the author's boot timing shows no slowdown. I did not run the test suite or a mutation run of the new completeness test, and I did not take a live screenshot through the built daemon. The Linux replay lane the author cites is the screenshot evidence. The iOS Smoke history on main and the 5-boot timing are the author's numbers, and I did not re-derive them. No conflicts. Nothing else stands in the way of a maintainer merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 10, 2026
@thymikee
thymikee merged commit bc40285 into main Oct 11, 2026
19 of 21 checks passed
@thymikee
thymikee deleted the perf/eager-closure-cuts branch October 11, 2026 06:36
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-11 06:36 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