Repository navigation
Conversation
… process startDaemonRuntime no longer calls process.exit or installs process handlers; src/daemon.ts owns signals, fatal errors and the exit code. close() is idempotent and resolves to a ShutdownReport. A runtime that fails during startup rolls back what it acquired. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… uploads and requests Provider admission, interactor resolution, artifact and upload tracking, resumable uploads, materialized paths, session-open locks and request cancellation are owned by the runtime instance instead of module scope. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Apple runner's sessions, locks, recycle ledger and owner state belong to the daemon runtime that composed them. The PNG worker is held by every runtime that uses it and stops with the last one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 52 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…moves temp files Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…one removal helper Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Closing one runtime can still kill another runtime's Apple runner build, so this needs one more change before merge (reviewed at c047776). CI is green (12 checks, none failing), and I know of no other merge issue. The Apple runner state in runner-artifact.ts is still process-wide. I found this by reading the code. I did not run two runtimes with an Apple runner. The PR body shows no artifacts for the live simulator run, and that run covers one runtime only, so per-runtime Apple isolation rests on Not blocking, and fine to take or leave: Could On the open review threads, all are addressed at this head except none still apply as live issues. These can be resolved: PNG/plugins/lock release in closeOnHost, isolation test cleanup, close test assertions, materialized-path dispose, resumable-upload dispose, shutdown error in daemon.ts, artifact-tracking dispose, releasePngWorker, composition test resolver, infoPath and lockPath assertions, removeBestEffort, all fixed at this head. The dispose ordering thread is benign: dispose runs after the servers close and in-flight work settles, and only a force-cut handler could still touch the temp dir, with its connection already gone. Before merge, Apple start admissions and the prep-process ledger must be per runtime, with a regression test showing B's prep child survives A's stop-all. |
…s to the runtime Closing one daemon runtime no longer retires another runtime's start admissions or signals its prep builds. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The runtime context, the per-runtime state list and the session teardown helpers move beside the other shutdown mechanics. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ranch * refactor/host-environment: fix(host): resolve every client read from the client's host
|
Pushed
Not done here: consolidating the per-module runtime-state owners into one is a follow-up. |
…-no-process-globals * refactor/host-environment: style: format the daemon policy reuse test fix(host): resolve the daemon policy path against the host working directory # Conflicts: # packages/daemon/src/server/http/daemon-runtime.ts
|
The PR is ready at 817930f. Not blocking, and you can take or leave these: the four test files daemon-runtime-device-claims, -recording-teardown, -snapshot-shutdown and -web-close-teardown still carry the empty This follows the earlier review at c047776 (#3772 (comment)). That review's findings are fixed. All 12 checks pass at 817930f. One thing I did not run: two daemon runtimes with a real xcodebuild prep in one process. Isolation rests on runner-session-runtime-isolation.test.ts, whose fake child never emits 'close'. The live simulator run in the PR body predates this delta, so the ALS lookups for prep and admission in a single live runtime are not live-validated. They do match the runnerSessions slot pattern that run covered. I also have not confirmed whether daemon composition reads Apple runner state before The PR has merge conflicts with main. Before merge, the base branch refactor/host-environment has to land, and the PR has to be rebased to clear the conflicts. On the open inline threads, no P1 or P2 thread still applies, and no lower-priority threads still apply. These threads no longer apply, so please resolve them: #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment), #3772 (comment) and #3772 (comment) are fixed, since the patches they cover are unchanged in this update. #3772 (comment) is harmless, because dispose runs after the servers close and in-flight work settles, so only a force-cut handler whose connection is already gone could touch the temp dir. The branch has a merge conflict with refactor/host-environment, so it needs a rebase before it can merge. |
|
This PR is ready at 012b50d, and the earlier findings from c047776 are now fixed. Not blocking: Smoke Tests is still running at 012b50d and no job has failed. The diff touches the smoke route, in daemon startup (the policy-load merge resolution) and in close (the moved Some limits on what the review covered. I read the code and did not run tests or gates. I did not run two daemon runtimes with a real xcodebuild prep in one process. The fake child in the isolation test never emits 'close'. The 'close' listener at runner-artifact.ts:392 looks up the ledger when the event fires, so it relies on the child-process handle carrying the spawn's async context. That holds in Node, but nobody has exercised it here. The simulator run in the PR body predates 22cea00, so no live cold start (prep build) has run at this head. The same On the open review threads, I could not read who wrote them. Ten fixed ones can be resolved because the code at this head already does what they asked: #3772 (comment) (closeOnHost finally still releases the PNG worker, plugins, registration and lock config), #3772 (comment) (isolation test unchanged since c047776), #3772 (comment) (close test unchanged), #3772 (comment) (materialized-path-registry dispose still in createRuntimeContext), #3772 (comment) (resumable-upload unchanged), #3772 (comment) (src/daemon.ts:33-35 still reports the shutdown error and sets exit code 1), #3772 (comment) (artifact-tracking unchanged), #3772 (comment) (releasePngWorker still idempotent and called in the finally), #3772 (comment) (composition test unchanged), #3772 (comment) (infoPath/lockPath assertions unchanged). Two more can be resolved too: #3772 (comment) (benign, since |
… migration Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: test: keep the registration-owner test under its size ratchet fix(host): start a launched daemon in the launching host's working directory test: merge the duplicate test-kit import in the handler chain routing test test: move the daemon-composition tests off the daemon's testing exports (#3742) fix(android): ignore emulator log lines in -list-avds output (#3743)
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pushed
Nothing else changed since the review at |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: refactor(daemon): read configuration from the request host instead of the ambient scope test: merge the duplicate test-kit import in the stale provider admission scenarios feat(cli): ensure the daemon explicitly before attaching in the CLI and MCP (#3766) feat(client): daemonLifecycle attach mode and ensureDaemon() (#3765) feat(daemon): quiesce request that retires an idle daemon atomically (#3764) fix(daemon): answer a named missing session on the session, not the device (#3701) test: route test/ and scripts/ off the daemon's testing exports (#3733) refactor(plugins): make the plugin framework provider-independent (#3722) fix(apple): name an Xcode too old for its host macOS as a runner startup failure (#3739) # Conflicts: # packages/daemon/src/server/http/daemon-runtime.ts
|
The code at 7eedaf6 looks right, and the earlier review of 012b50d still holds. CI is green on all 6 checks at that commit, and the new changes touch only tests and an upstream base merge. The PR has merge conflicts with main. The stack base refactor/host-environment has also moved to 7eb7379, and merging it conflicts in packages/daemon/src/server/http/daemon-runtime.ts, so please merge it, resolve that file, and re-run check:affected before merge. Not blocking, and you can take or leave it: the base now carries daemon-runtime-quiesce.test.ts from #3764, and it still passes Is the per-runtime state handle ( The evidence is not complete yet. I did not run the Limrun test locally, so that it passes at 7eedaf6 rests on the green CI and on your check:affected run. I also did not inspect the conflicting hunks of daemon-runtime.ts against 7eb7379, and I assumed no review threads were resolved since the last review, so any open inline thread that still applies still stands. |
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: fix(daemon): drop the unused host from install-from-source and tighten test envs
|
Pushed
|
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: test: pass the host to the iOS e2e live runner's request
|
Pushed
|
|
Thanks for the update. At 5a8f1cd the daemon exit-code fix still holds, but two problems remain, and one needs live evidence before merge. Android snapshot-helper warm sessions, failed-start backoff, pending retirements and shell-protocol probes are now found through a root slot store bound into The new Would one Not blocking: Eight checks were still running when I reviewed: the four Smoke Tests, CodeQL Analyze, Repo Guards & Compatibility, Coverage and Integration Tests. None has failed yet. The diff overlaps their routes (daemon runtime close and composition, the Android helper slots, and the layering policy and host-kit exports), so a failure there needs a look. I did not inspect the merge commit d5682c7 hunk by hunk beyond the slot files, and the range-diff shows commits 1-19 unchanged. Before merge, the Android emulator run above must pass, and a maintainer must decide on the +1-module budget so Apple and |
|
Decision on the budget: I accept the +1-module eager-closure budget for |
…fineRuntimeSlots The maintainer accepted the one-module eager-closure addition for host-kit's runtime-slots.ts, recorded as an approved closure addition (#3752) in the budget gate. The Android port drops the unused disposer parameter. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pushed
|
…ntimeSlots slots Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…ename of a base module, and require its reason Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pushed
|
|
Thanks for the update. The earlier review of 5a8f1cd is mostly addressed at 0658db9: the two eager-closure threads from the other reviewer are fixed, and CI shows 20/20 checks passing. One item still needs a real run before merge. I did not run tests or the eager-closure gate myself, and I took the budget numbers and the check:affected pass from the CI size report and your notes. Android helper state (warm sessions, failed-start backoff, pending retirements, shell-protocol probes) is now reached through the root slot accessor bound into Not blocking, take or leave: The two eager-closure threads from the other reviewer are fixed at this head, so please resolve them: closure-growth-rule rename mapping and budget row reason check. Before merge, the Android emulator output above must be in the PR body. |
…ts, not the request re-export Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: refactor(react-native): drop the redundant outer catch around the scoped Metro session read; changelog the PNG worker change
|
Pushed
|
|
The code at 70c901d looks good, and the live Android run in the PR body meets my earlier evidence request. The run was taken at 0658db9; the only change since then is type-only imports, so runtime behavior is the same. The earlier Cubic findings are fixed at this head. All 20 checks pass, and there are no conflicts. I ran no tests myself. One question on simplicity before this is ready for review. Apple keeps its runtime store in its own package: runner-owner-state.ts calls This PR is based on refactor/host-environment (#3763), so it also waits for that one. |
…dule, as Apple does for the runner The root no longer keeps a store and binds its accessor into the adb host port. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…r the helper runtime state Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: refactor(capture-kit): report an unavailable PNG worker in every scope that takes the fallback; the daemon pre-warms it inside a daemon diagnostics scope # Conflicts: # packages/capture-kit/src/png-worker-client-unavailable.test.ts # packages/capture-kit/src/png-worker-client.ts # packages/daemon/src/server/http/daemon-runtime.ts
|
Pushed
|
|
The code at 029e82d is clean. The earlier concern from 70c901d is fixed: the runtime now owns its state, and the merge keeps the PNG worker leak fix. All 20 checks pass at 029e82d, and the author reports check:affected passed on the same commit. I ran no tests. I judged the isolation and composition tests and the layering policy from the diff. The live Android emulator run is from 0658db9 and was not re-run at 029e82d. It still counts, because the later change only moves where the slot store is defined and touches no adb or device call. The PNG worker report behavior comes from #3763, and I checked only the merge resolution here, not that PR's own logic. Not blocking: The cubic-dev-ai P2 thread on the PNG worker leak no longer applies, because This PR is stacked on #3763 (refactor/host-environment). #3763 must land first, and then this PR needs a retarget onto main. |
…nger needs Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: refactor(capture-kit): report each PNG worker transition to unavailable once, through a daemon-registered sink with a typed reason # Conflicts: # packages/capture-kit/src/png-worker-client-unavailable.test.ts # packages/capture-kit/src/png-worker-client.ts # packages/daemon/src/server/http/daemon-runtime.ts
|
Pushed
|
|
Smoke Tests failed at |
|
Summary
Part of #3747, stacked on #3763. Closes #3752.
A daemon runtime no longer assumes it owns the process.
startDaemonRuntimenever callsprocess.exitand installs noprocess.on/oncehandler.src/daemon.tsowns signals, fatal errors and the exit code. A startup refusal rejects withdetails.exitCode(daemonStartupExitCode).close()is idempotent: concurrent calls share one promise that resolves to aShutdownReport(provider releases, runners, sessions, plugins).closedsettles whoever closes (idle reap). Provider runtimes dispose exactly once; a startup that fails rolls back what it acquired.daemon stop --jsonstill reportsproviderReleases.pending.AsyncResource.bind). The PNG worker is held by every runtime that uses it and stops with the last.Validation
pnpm check:affected --run --head 66f0b27ea4passed (exact commit). New tests: two runtimes in one process serve, close and keep state independently; idempotentclose()and rollback; runner state per runtime; PNG worker refcount.Live on iPhone 17e simulator (clean daemon,
/private/tmpstate dir): open, snapshot, close; gracefuldaemon stophanded the runner off (ios_runner_session_detached, runner kept alive) and a new daemon adopted it with no second xcodebuild;daemon stop --cleananddevice statusleft no claims or runners.Live Android evidence (daemon route, source head 0658db9)
Headless emulator
emulator-5554(ReactNative API 35, API 35), isolated state dir under/private/tmp, no device claim held beforehand (~/.agent-device/device-claimsempty), helper built withpnpm build:android. Commands were run through the built CLI against a freshly started daemon.So the warm session is found again by the second and third snapshots through the root slot accessor in the daemon's entered store, and the close-time reset in the same store stops the helper and releases the claim. The daemon log at its default level records none of this, so the process id, logcat and the
helperSessionReusedfield are the evidence; the three source-level changes since this run only move imports (RuntimeStateHandle's home) and approval wiring in the closure gate.🤖 Generated with Claude Code