Uh oh!
There was an error while loading. Please reload this page.
refactor(storage): centralize root writer lifecycle - #3295
Conversation
6357252 to
3f3dc4aCompare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 3f3dc4ab. The consolidation is the right move and the shape is good: one owner for writer open order, reverse-order rollback on partial failure, and a single storage.close() replacing twenty-odd individually-tracked let store: X | undefined variables and their hand-written cleanup ladder in execution-composition.ts. Removing 150 lines of that ladder is a genuine simplification, and the failure path is now correct where it previously depended on every future writer being remembered in three places.
I checked the two things most likely to break in a refactor like this and both are fine: execution.sessionStore.close() is the full teardown — closeExecutionStorePersistence closes runtimePersistence, agentRunStore, conversationOperationalStateStore, messageReceiptStore and the interaction-store facade — so closing only sessionStore is not a leak; and runtimePolicy / memoryBundle are the two writers passed without a close callback because they genuinely expose none. Closing all stores after every domain module has closed, rather than interleaved per module, is also an improvement: coordinators now reliably shut down before the stores they write to.
The architectural concern is ownership, and it is what all three inline findings share. This PR does not just centralize the lifecycle, it also makes the composition a process-wide singleton per lease — openStorageWriterComposition caches on the lease object and hands the same frozen object to every caller. Before this change, the execution stores were already lease-cached, but the other thirteen writers were not: two callers each got their own project-catalog writer, and one closing did not disturb the other. Now close() is an unconditional teardown of a shared object with no reference count, and the second holder's close() silently resolves to the first holder's memoized closeTask. The new test asserts first === second and then closes twice — which encodes the sharing but never exercises the hazard it creates.
That is worth deciding deliberately rather than inheriting: either the composition is genuinely a singleton whose lifetime the root owner controls (in which case close() should not be on the object every caller holds), or it is shared and needs a reference count. Today production reaches it through a single root owner so the window is narrow, but createExecutionRuntimeHostCompositionSource is a source — it can build a composition more than once over a process lifetime, and any overlap between one teardown and the next open lands exactly in that window.
Reviewed with Claude Opus as an analysis assistant. I verified the store close surfaces, the old and new cleanup paths, and the added test by reading source at this head; the race in the first finding is reasoned from the code path and not reproduced by execution.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
3f3dc4a to
a9fd1a1Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at a9fd1a14. All four findings are fixed, and the author picked the stronger of the two options I offered rather than the cheaper one.
Replacing the per-lease cache and single-flight map with an activeCompositions WeakSet removes the sharing that caused three of the four findings at once:
- Handing out a closing composition — impossible now. The lease stays marked active until
close()settles, so an overlapping open throws instead of adopting a dying composition. The new test pins exactly this: it rejects a second open while the first is alive, rejects again duringclose(), and then succeeds once the close resolves. - Shared teardown with no reference count — moot, since two holders can no longer exist. This also removes the silent-no-op second
close(), which was the part I liked least. - Order-dependent
afterRuntimePolicyOpened— moot for the same reason, and the doc comment was correctly rewritten from "runs only when this call starts a new composition" to what the hook now unconditionally does. - Empty
close: []placeholders — removed from all three domain modules.
Choosing "one composition per lease, loudly enforced" over "shared with a reference count" is the right call: it makes the lifetime rule checkable at the boundary instead of distributed across holders, and the failure is an error at open rather than a use-after-close later. The duplicate activeCompositions.has check before and after assertStorageRootLease is not redundant in a way that matters — the pair is atomic because there is no await between the second check and add, and the early one just avoids paying for the lease assertion on a busy lease.
One thing worth confirming rather than a finding: this converts a previously silent situation into a hard throw. On main, two callers on one lease each opened their own writers; now the second gets Storage writer composition is already active for this lease. createExecutionRuntimeHostCompositionSource builds a composition per context and test-only/desktop-e2e-execution.ts builds one too — please confirm no path constructs two concurrently against a single owner's lease, because that now fails at startup instead of degrading. Failing loud is the right behaviour; I just want it to be a decision rather than a discovery.
Reviewed with Claude Opus as an analysis assistant; verified by reading the full new file, the rewritten test, and the diff against the head I previously reviewed. Nothing was executed.
Uh oh!
There was an error while loading. Please reload this page.
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at a9fd1a14. All four findings are resolved, and by the stronger route: one composition per lease, enforced at the boundary, with an overlapping open rejected until close() settles. That removes the shared-teardown and use-after-close hazards outright rather than papering over them. The one P3 left on the previous review — what a failed close should do to the lease — is a decision to record in a comment, not a blocker.
Generated-by: Codex
a9fd1a1 to
38b816cCompare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 38b816c8 after the force-push dismissed my approval. The P3 from my previous review is fixed, and fixed as the decision I asked for rather than as a patch: .finally became .then, so a close that fails keeps the lease unavailable, with a comment on the line saying exactly why — a failed close may have left a writer handle open. That closes the double-open hazard.
Two things I checked because they follow from that change:
- Dropping the
try/catchthat deleted the lease on open failure is correct, not an omission.failOpenalready routes throughclose(), so a successful rollback releases the lease and a failed one holds it — the old outercatchwould have released unconditionally and contradicted the new policy. - The cost is now explicit: a close failure makes that lease unusable for the rest of the process lifetime, recoverable only by restarting. That is the right side of the trade to land on, since the alternative silently opens the same SQLite files twice, and it is now stated in the code instead of implied.
The added test earns it — it forces a real close failure by closing the operational-state database underneath the composition, asserts close() rejects with an AggregateError, and then asserts the reopen is rejected. That is the assertion that would catch a future revert to .finally.
Approving. Reviewed with Claude Opus as an analysis assistant; verified by diffing both files against the head I previously approved. Nothing was executed.
Uh oh!
There was an error while loading. Please reload this page.
… guards - Point the release smoke script's deep import at dist/workspace-root.js, which owns resolveMakaDataRoots now that dist/index.js is not emitted, and guard every such by-path import with a release file-policy test. - Import openStorageWriterComposition through its published subpath; the bare specifier resolved to the removed barrel entrypoint after apache#3295. public-entrypoints.test.ts now rejects bare @maka/storage imports anywhere in the tree, so the next stale-merge of this kind fails a test instead of a build. - Add ./storage-writer-composition to SQLITE_BACKED_ENTRYPOINTS: it statically imports execution-stores and thirteen other SQLite-backed modules. - Detect the SQLite boundary with a module.registerHooks resolve hook instead of matching Node's ExperimentalWarning text, which Node 26 has already reworded. - Drop the dangling main/types manifest fields and assert that every published entrypoint target is emitted by the build. - Assert internals stay private by loading every published entrypoint and checking the union of reachable symbols, not the export map's targets, so a future re-export cannot leak them silently. - Run Biome over the two files format:check rejected. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… guards - Point the release smoke script's deep import at dist/workspace-root.js, which owns resolveMakaDataRoots now that dist/index.js is not emitted, and guard every such by-path import with a release file-policy test. - Import openStorageWriterComposition through its published subpath; the bare specifier resolved to the removed barrel entrypoint after apache#3295. public-entrypoints.test.ts now rejects bare @maka/storage imports anywhere in the tree, so the next stale-merge of this kind fails a test instead of a build. - Add ./storage-writer-composition to SQLITE_BACKED_ENTRYPOINTS: it statically imports execution-stores and thirteen other SQLite-backed modules. - Detect the SQLite boundary with a module.registerHooks resolve hook instead of matching Node's ExperimentalWarning text, which Node 26 has already reworded. - Drop the dangling main/types manifest fields and assert that every published entrypoint target is emitted by the build. - Assert internals stay private by loading every published entrypoint and checking the union of reachable symbols, not the export map's targets, so a future re-export cannot leak them silently. - Run Biome over the two files format:check rejected. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… guards - Point the release smoke script's deep import at dist/workspace-root.js, which owns resolveMakaDataRoots now that dist/index.js is not emitted, and guard every such by-path import with a release file-policy test. - Import openStorageWriterComposition through its published subpath; the bare specifier resolved to the removed barrel entrypoint after apache#3295. public-entrypoints.test.ts now rejects bare @maka/storage imports anywhere in the tree, so the next stale-merge of this kind fails a test instead of a build. - Add ./storage-writer-composition to SQLITE_BACKED_ENTRYPOINTS: it statically imports execution-stores and thirteen other SQLite-backed modules. - Detect the SQLite boundary with a module.registerHooks resolve hook instead of matching Node's ExperimentalWarning text, which Node 26 has already reworded. - Drop the dangling main/types manifest fields and assert that every published entrypoint target is emitted by the build. - Assert internals stay private by loading every published entrypoint and checking the union of reachable symbols, not the export map's targets, so a future re-export cannot leak them silently. - Run Biome over the two files format:check rejected. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… guards - Point the release smoke script's deep import at dist/workspace-root.js, which owns resolveMakaDataRoots now that dist/index.js is not emitted, and guard every such by-path import with a release file-policy test. - Import openStorageWriterComposition through its published subpath; the bare specifier resolved to the removed barrel entrypoint after apache#3295. public-entrypoints.test.ts now rejects bare @maka/storage imports anywhere in the tree, so the next stale-merge of this kind fails a test instead of a build. - Add ./storage-writer-composition to SQLITE_BACKED_ENTRYPOINTS: it statically imports execution-stores and thirteen other SQLite-backed modules. - Detect the SQLite boundary with a module.registerHooks resolve hook instead of matching Node's ExperimentalWarning text, which Node 26 has already reworded. - Drop the dangling main/types manifest fields and assert that every published entrypoint target is emitted by the build. - Assert internals stay private by loading every published entrypoint and checking the union of reachable symbols, not the export map's targets, so a future re-export cannot leak them silently. - Run Biome over the two files format:check rejected. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) * refactor(storage): drop the barrel and publish narrow entrypoints `maka --help` printed Node's SQLite ExperimentalWarning to stderr, on a command that never opens a database. The cause was structural, not local: `@maka/storage` published one `export *` barrel, and `cli-core.ts` imported it to reach `resolveMakaDataRoots`. Three modules in that barrel's graph take a static value import of `node:sqlite`, and Node evaluates a builtin the moment it enters a module graph, so every consumer of the barrel loaded SQLite whether or not it wanted a database. `operational-target-schema.ts` was the amplifier: `operational-state-store.ts` imports it, and roughly forty modules import that, which is how three import statements reached 45 of the package's 110 modules and 24 of the barrel's 43 export entries. Remove the barrel instead of working around it. `.` is gone from the exports map, `src/index.ts` is deleted, and the 20 modules that consumers actually reached through it are published as narrow subpaths. This is already the prevailing convention here — `root-authority` has 141 call sites and `execution-stores` 108, against 31 non-test sites on the bare specifier. The three static `node:sqlite` imports stay exactly as they were. They are honest: those modules do need SQLite. What changes is that needing SQLite is now visible in the import path, so `@maka/storage/workspace-root` costs nothing and `@maka/storage/session-store` costs what it should. No lazy-load indirection and no warning suppression are involved. `public-entrypoints.test.ts` pins the boundary: no `.` export, and exactly 28 of the 54 published entrypoints load `node:sqlite`. Widening that set now requires editing the list and saying why. Two tests moved off the barrel's shape rather than its contents: `managed-workspace-baseline` asserted internals were absent from the barrel object and now asserts their modules are absent from the exports map; `provider-request-capture-artifact` reaches its subject directly. Generated-by: Claude Code * fix(storage): address review — repair path imports, harden entrypoint guards - Point the release smoke script's deep import at dist/workspace-root.js, which owns resolveMakaDataRoots now that dist/index.js is not emitted, and guard every such by-path import with a release file-policy test. - Import openStorageWriterComposition through its published subpath; the bare specifier resolved to the removed barrel entrypoint after #3295. public-entrypoints.test.ts now rejects bare @maka/storage imports anywhere in the tree, so the next stale-merge of this kind fails a test instead of a build. - Add ./storage-writer-composition to SQLITE_BACKED_ENTRYPOINTS: it statically imports execution-stores and thirteen other SQLite-backed modules. - Detect the SQLite boundary with a module.registerHooks resolve hook instead of matching Node's ExperimentalWarning text, which Node 26 has already reworded. - Drop the dangling main/types manifest fields and assert that every published entrypoint target is emitted by the build. - Assert internals stay private by loading every published entrypoint and checking the union of reachable symbols, not the export map's targets, so a future re-export cannot leak them silently. - Run Biome over the two files format:check rejected. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(storage): narrow published surfaces to the barrel's picks - Repoint the real-model computer-use script at dist/agent-run-store.js; the release file-policy test now walks every script for both the node_modules and the relative packages/*/dist import forms and asserts each target is still emitted, which catches this whole class. - Publish operational-state-store and credential-store through facades that re-export exactly the names the deleted barrel picked. The schema-migration internals and the credential file lock return to package-private. artifact-store needs no facade: nothing outside the package imports it on current main, so it is not published at all and the lease-gated write authority stays private with the rest of the module. The reachable-symbol union test names all five withheld symbols. - Drop the five session-bundle entrypoints with no consumer outside the package; public-entrypoints.test.ts now asserts the exact set of consumer-less entrypoints, allowlisting only those that predate this change, and that every imported subpath is published. - Extend the bare-specifier guard to the side-effect import form and cap the SQLite probe children at four concurrent. - Delete the storybook path mapping to the removed src/index.ts. Generated-by: Claude Code Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(storage): keep the credential-store entrypoint's existing surface `./credential-store` was a published subpath before this branch and mapped straight to `credential-store.js`, so `withCredentialFileLock` was reachable through it. Routing it through a facade turned that symbol into `undefined` — a contract change to a pre-existing entrypoint, and one that has nothing to do with removing the barrel. The facade was applying the barrel's picks to an entrypoint the barrel never owned. That rule is right for the subpaths this branch publishes for the first time, where the surface is still being chosen; it is not a reason to narrow one that already shipped. Whether the file lock should be package-private is a separate compatibility decision, and stays open. The export map now only drops `.` and adds subpaths — no pre-existing entrypoint changes target, which one command shows: git diff upstream/main...HEAD -- packages/storage/package.json `withCredentialFileLock` also leaves the reachable-symbol denylist in `managed-workspace-baseline`, because it is publicly reachable again, exactly as it was before this branch. Generated-by: Claude Code Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Add a storage writer composition that owns the Runtime Host writer lifecycle.
The composition now:
close()idempotentDomain writer interfaces, authorization checks, and persistence behavior remain unchanged. This PR does not introduce a repository framework or merge domain interfaces.
The Runtime Host still bootstraps runtime policy immediately after opening its stores and before opening the remaining writers.
Verification
npm --workspace @maka/storage run typecheck— passednpm --workspace @maka/runtime-host run typecheck— passedgit diff --check— passedAI use
Select exactly one:
Tool(s) and scope: Codex implemented the storage writer lifecycle refactor, drafted its tests, and addressed code-review feedback. The human contributor reviewed the final diff and owns the decision to submit it. The commit carries a
Generated-by: Codextrailer.