Repository navigation
refactor(host): enter one runtime store once per daemon runtime (#3794) - #3818
Conversation
|
Coordinator review at A test-only export on a production port. In Remove it from the port. Have the runner tests get their handle from test support instead: Everything else matches #3794: one entered store, |
Size Report
Startup median (7 runs, lower is better):
|
b351089 to
37f6d1a
Compare
|
Round 1 — the coordinator review, mapped to its commit at head Host port carries a test-only symbol. Agreed — a production port must not widen for callers that are only tests, and R77 never needed widening: the rule refuses a direct host-kit import from
All of it folds into the platform layer commit Verification. Focused: platform-apple core+runner + composition + artifact/helper isolation — 127 files, 1299 tests green. Then |
37f6d1a to
13cda2a
Compare
|
Round 2 — the failed The failure. Fallow's production-export scan flagged The fix — no suppression, the export is gone.
Verification. |
|
The code at 13cda2a looks right to me, but I cannot call it ready yet because there is no live run of the daemon with this change. The PR body cites unit, focused and gate runs only. The one-store design is only proven through the test seams, not through a real daemon request. Please run a live daemon at 13cda2a with two checks. First, run an iOS simulator command that reaches the Apple runner, such as open plus snapshot, and show it finishes with no apple_runner_host_unbound error. That proves the host binding in the entered handle reaches request dispatch. Second, run an Android snapshot through the helper session. The Smoke Tests iOS and Android lanes at this head, or an equivalent local run with its output, satisfy this. Checks are still running (Smoke, Repo Guards, Coverage and Integration), and none has failed so far. All four cover this diff, and Repo Guards failed in round 2 on the facade re-export that is now removed. I found no conflicts. I did not run tests myself, so the results in the PR body are as you reported them, and I have not read any smoke output. Not blocking, and you can take or leave these: the test at packages/daemon/src/server/http/daemon-runtime-bounded-close.test.ts#L211 would still pass if composition ran outside the entered handle, so a clean close of a runtime whose composeDeps declares a slot with a disposer should assert that disposer ran once. Also, some comments in packages/host-kit/src/runtime-slots.ts#L18 and src/daemon-composition.ts narrate history and issue numbers, and could drop those clauses. Question on scope: the one-store design assumes host-kit/runtime-slots is a single module instance per process, which the bundled build gives the published package. Does any externally published provider plugin bundle its own host-kit copy and declare slots? I found no runtimeSlot use in the provider-* packages. On the earlier thread from thymikee (#3818 (comment)), the fix is in at this head: createRuntimeStateHandle is gone from the runner port, and the facade no longer re-exports getAppleRunnerHostEnvironment. It does not apply any more, so please resolve it. Before merge, the live run above must pass and the required checks must go green. |
|
@thymikee This is resolved at head
|
…e process (#3794) Each module used to declare its own async-local store and expose a handle the daemon had to aggregate. Modules now declare slot keys on one store: runtimeSlot reads a piece of state from the store the current runtime entered, and createRuntimeStateHandle makes the one handle a runtime enters and disposes. The standalone fallback stays, so a client process or a unit test reads its own state without composing a runtime. request-cancel drops its handle factory; the runtime's handle owns the slot and its disposer.
…one store (#3794) The Android helper and the Apple runner no longer own a store. Their modules declare slot keys on host-kit's single store, and the daemon's entered handle scopes them. The Apple runner keeps reaching the store through its host port, so no runner module imports host-kit (R77). The host binding createAppleRunnerRuntimeState used to wrap is the existing setAppleRunnerHostEnvironment called inside the runtime, so no per-platform handle factory remains.
startDaemonRuntime creates one handle and runs composition, dispatch and close inside it, so every module slot resolves from that runtime without a per-module handle being composed in. DaemonRuntimeDeps.runtimeStates, RuntimeContext and its extend aggregation are gone: the store has nothing left to fold together. Bounded close keeps disposing state only when no work is unfinished, now observed through a slot of the runtime's own store.
…3794) Composition runs inside the runtime's handle, so binding this runtime's host for the Apple runner writes that state directly and the deps no longer carry platform handles back.
|
At 13cda2a, this branch now conflicts with main after #3806 and #3797 merged. Please rebase it. The code verdict from my earlier comment has not changed: the round-1 thread is resolved, and the one open item is still a live daemon run (iOS open plus snapshot through the Apple runner, and an Android snapshot through the helper session) at the rebased head. |
13cda2a to
ad35782
Compare
|
Round 3 — the maintainer review at 1. Live daemon evidence (blocking) — done at this head. (a) Apple runner route — iOS simulator open + snapshot: The snapshot served from the XCTest runner backend ( (b) Android helper-session snapshot:
The CI Smoke Tests iOS and Android lanes at 2. Clean-close disposer assertion — applied ( 3. Comment narration — applied. 4. Scope question — confirmed: no published provider plugin bundles a second host-kit copy that declares slots. The one-store invariant holds because only four production modules call
5. Coordinator thread Rebase. main gained #3806/#3797 while this round was in flight; rebased onto Gate. |
|
The PR is ready at ad35782. The conflict that stopped the earlier review (13cda2a) is gone, and both of the earlier asks are now done: the runner port no longer exposes createRuntimeStateHandle, and the clean-close test now checks that the entered handle disposes its state composition. All 16 checks pass at this head, and Smoke, Integration and Repo Guards cover the daemon runtime entry and the platform slot sites this diff touches. Not blocking, take or leave: createTestRuntimeStateHandle() in packages/test-kit/src/runtime-state.ts only returns createRuntimeStateHandle(), and the TestRuntimeStateHandle alias adds nothing, yet the two runner tests under packages/platform-apple/src/runner/tests use it to reach an import that R77 forbids there. Either allow that import explicitly in R77 and import the constructor directly, or keep the helper, name it in R77 as the runner-test seam and drop the alias. The earlier inline threads on the runner port and on the clean-close assertion are fixed at this head, so you can resolve them: #3818 (comment) and #3818 (comment). The live iOS runner and Android helper runs, and the answer that no provider bundle holds a second host-kit store, come from your report. I did not re-run them and did not read the smoke output. I only confirmed that the provider sources at this head do not use runtimeSlot. I also did not run the test suites, and I judged the new clean-close test by reading runtime-slots.ts. Nothing else blocks it, so it is ready for a human review. |
thymikee
added
the
ready-for-human
|
1 participant
Closes #3794
Part of #3747. Second PR: #3798 landed part 1 (configuration reads take the host explicitly). This one is part 2, one entered runtime store.
What changes
Every module that kept per-runtime state declared its own
defineRuntimeSlots()store and exposed acreateXxxRuntimeState()handle;startDaemonRuntimecollected nine of those handles into one context — six constructed by the daemon plus two handed back throughDaemonRuntimeDeps.runtimeStates— nested theirruncalls, and folded the last two in later throughRuntimeContext.extend+enter(). That aggregation is now gone: one store per process, one handle entered once per runtime.packages/host-kit/src/runtime-slots.ts—defineRuntimeSlotsis replaced byruntimeSlot(key, create, dispose?)andcreateRuntimeStateHandle(). Modules declare slot keys, they no longer own a store. The entered-store-else-standalone rule is unchanged, so a client process or a unit test still reads its own state without composing a runtime.request-cancel(host-kit),interactor-resolution,provider-device-admission,artifact-tracking,resumable-upload,materialized-path-registry. Their slot keys and disposers stay exactly where they were.startDaemonRuntimecreates one handle and runs composition, request dispatch and close inside it, so every read resolves from that runtime's state.DaemonRuntimeDeps.runtimeStates,RuntimeContext,createRuntimeContextandextendare deleted;src/daemon-composition.tsno longer returns platform handles — it callssetAppleRunnerHostEnvironment(host)where it stands, inside the entered handle, which is wherecreateAppleRunnerRuntimeStateused to bind it.adb-host) and the Apple runner (runner-owner-state) keep their slot keys and lose their stores. The runner still reaches the store'sruntimeSlotthrough its host port, so norunner/**file imports host-kit (R77). Creating a handle is not a production port member: the only callers were tests, so test-kit gainedcreateTestRuntimeStateHandle(runtime-state.ts), and the runner tests enter their handles through it.runnerRuntimeSlotandhelperRuntimeSlotnow nameruntimeSlot.Done-when from the issue
runtimeStatesaggregation remains.defineRuntimeSlots,createRuntimeContext,RuntimeContext,runtimeStatesand.enter()have zero hits insrc/,packages/andtest/; the onlyruntime-slotsimporters are the modules that declare slots plus the daemon that enters the handle.runner-owner-state.test.ts,runner-session-runtime-isolation.test.ts,runner-lease-runtime-host.test.ts(Apple),snapshot-helper-session-runtime-isolation.test.ts(Android helper),daemon-runtime-isolation.test.ts(two real daemons: per-runtime admission, interactor resolution and artifact tracking, and closing one leaves the other serving), plusdaemon-composition.test.ts, which now asserts each composition's writes land in the handle it ran inside and that one runtime's host is not the other's. The PNG worker has no slot state to collapse — it is a process-shared worker with holder counting (retainPngWorker), and its holder test still passes;check:affectedran it.daemon-runtime-bounded-close.test.tsdeclares a slot on the runtime's own store and asserts its disposer has not run whenplatformOwnersispending.Scope notes
b5f47e7542with their changes intact. The single predicted conflict, the close path inpackages/daemon/src/server/http/daemon-runtime.ts, was resolved by keeping main'sfinishDaemonRegistrationblock with theruntimeContext→runtimeStaterename. No other file overlapped.Evidence
pnpm check:affected --run --head ad357823d9on the exact head,exactCommit=true(receiptrun-hpQD6R): format, lint, typecheck, layering, fallow (production-unused exports included), build, vitest-related and daemon-wire-compat all pass. Layering is the gate that proves the port still fronts host-kit for the runner subtree, and the eager-closure budgets were untouched.backends: {"xctest": 2}, zeroapple_runner_host_unbound), and Android snapshot through the helper session (backend: android-helper,helperVersionmatchespackage.json).createRuntimeStateHandlefrom the runner host port (test-kit owns test support, R83); round 2 removed thegetAppleRunnerHostEnvironmentfacade re-export after the production-unused-exports gate caught it as test-only — the composition test now observes the host binding throughresolveRunnerAppBundleId(), a production read the runner's resource cleanup already uses; round 3 added the live runs above, the clean-close disposer assertion, trimmed the narration comments, and answered the provider-bundling scope question. See the round comments below for the full mapping.src/daemon(551 files / 4087 tests), scripts (55 files / 1285 tests, includes the layering and wire-compat policy tests).