Skip to content

refactor(storage): verify replaceable execution persistence - #5090

Open
MicroGery wants to merge 7 commits into
mainfrom
codex/local-runtime-persistence-contracts
Open

refactor(storage): verify replaceable execution persistence#5090
MicroGery wants to merge 7 commits into
mainfrom
codex/local-runtime-persistence-contracts

Conversation

@MicroGery

@MicroGery MicroGery commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Refs #2370. This is PR1 of the 2026-09-09 local Runtime follow-up, not the earlier SessionRepository PR. It now takes responsibility for demonstrating the live-state persistence boundary, not only extracting interfaces.

Supported composition, with the existing authority checks

  • Trusted Host composition accepts an ExecutionPersistenceProvider; the default remains Local. Clients, model output and per-Session options cannot select a backend.
  • Storage composition wraps the selected group in the existing authenticated, root-lease-scoped capabilities. One provider owns a lease; retained calls after close/revocation fail, entered operations drain, and an uncertain open/failed close cannot silently switch backends.
  • The group includes Session, AgentRun/RuntimeEvent, Interaction, Graph control, Goal and operational cleanup. Graph provisioning and Session/Goal retirement already cross those transaction boundaries; keeping a separately opened Local Graph/Goal authority would break substitution. Their product semantics are unchanged.

Independent reference backend and shared evidence

  • Add a test-only, Map-based Memory provider with copy-on-write transactions, detached reads, and no SQLite construction, wrapper or fallback. It implements the declared execution ports, not a handful of coordinator mocks.
  • Local and Memory run the same 40 conformance cases: stable identity/CAS, immutable fields, admission/handoff/ordering, WorkHub target+admission+coordination atomicity, Tool T1/T2 event+operation+journal atomicity, lost acknowledgements and exact retries, Graph schedule-fenced provisioning, Goal retirement, operational purge, authenticated access and lifecycle failure.
  • Failure-before-commit setup is backend-specific only in the harness (SQLite aborting triggers versus an unpublished Memory draft); assertions and consumer operations are shared.
  • A real Host subprocess selects Memory through the supported composition entry. A deterministic fake model drives ordinary message submission, transcript history, delegation to an existing Session and creation/delegation of a new Session. Exact retries do not dispatch twice; inspecting Local afterward confirms those execution records never landed there. No coordinator contains Local/Memory branches.
  • Pure canonical validators/projection helpers are extracted so the independent backend does not import SQLite. The existing public exports are preserved. These moves account for much of the diff; they are not a new persistence format.

Retained Local recovery and attachment fixes

  • The existing real Host SIGKILL/restart tests still cover assignment committed before first target dispatch, for create_new and delegate_existing, with and without attachments.
  • WorkHub assignment validates ordered attachment metadata/source-target ownership and replay identity. Verified attachment-copy reuse checks complete metadata and bounded payload digests under the Artifact writer lock; conflicts are not overwritten. Retry-after-copy-failure and partial/concurrent replay regressions remain covered.

See the execution persistence boundary note for composition, transaction ownership, lifecycle and limitations.

Explicit limits

  • Local remains the default durable live-state authority. No schema migration, checkpoint publication/loading, S3/live remote adapter, distributed fencing or WorkHub UI/routing redesign.
  • Memory is an independent process-lifetime reference, not durable storage. Reopening the provider in one process is not crash-recovery evidence. The Local subprocess tests supply that evidence, limited to the before-first-target-dispatch window—not exactly-once arbitrary external effects.
  • Artifact payloads, runtime policy, long-term memory and other independently composed domains remain local/outside this provider. Optional managed-workspace authority is not invented or silently delegated to Local.
  • Host reads use the selected group's read methods. Standalone Local read utilities are not a generic remote-reader API. Existing transcript-index compatibility operations remain; subscriptions are instance-local invalidations, not a remote changefeed.
  • This validates the current bounded execution contract, not a final representation-independent API for all Maka state.

Verification

macOS, Node 24.18.0:

Suite Passed Skipped Failed
Storage 1,265 8 0
Runtime 3,349 13 0
Runtime Host 1,844 12 0
  • Shared Local/Memory conformance: 40 passed, no skips.
  • Real Host Memory substitution and existing Local process-loss/attachment scenarios are included in the Host suite.
  • Storage/Host build, full-workspace typecheck, scoped Biome lint/format, changed-file ASF headers, Windows skip inventory and diff whitespace checks passed.
  • No Windows execution, Desktop E2E, remote-backend qualification or production performance benchmark was run. Copy-on-write Memory is intentionally a correctness reference, not a performance implementation.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

OpenAI Codex assisted with implementation, tests, documentation, verification and PR preparation. Implementation commits carry Generated-by: OpenAI Codex. Independent human review and the merge decision remain required.

Checklist

  • Tests cover the new boundary and the regression cases
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — trusted provider selection and the WorkHub attachment fixes described above
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 9, 2026
@likun666661

Copy link
Copy Markdown
Member

Architecture review: moving contracts is not backend substitutability

Reviewed at 3d07d301, specifically against this question: can Memory/Mock, Local, and S3-backed implementations serve the same upper-layer code without backend-specific branches?

Not yet. This PR improves dependency hygiene, but does not establish a replaceable persistence boundary. The description explicitly excludes a remote adapter, so the points below are gaps against that broader architecture goal, not claims of newly introduced local regressions.

1. The actual composition boundary still only admits the local implementation

execution-stores.ts authenticates stores through a private registry, while its construction path hardcodes createSessionStore(lease.canonicalPath), createSqliteAgentRunStore(...), and local runtime persistence. Host composition has no storage-provider injection point.

A structurally conforming implementation is therefore insufficient: a separately constructed complete store fails authentication. Narrow Pick<...> mocks are possible; replacing the complete Host persistence backend is not.

Keep the lease/authentication protections. But let trusted composition construct an alternative backend and wrap it in the same authenticated lifecycle facade. If even a stateful in-memory implementation cannot enter through the supported composition path, S3-readiness is premature.

2. The hard part is preserving transactions, not extracting DTOs

The atomic assignWorkHubMessage operation is the right abstraction: coordination linkage, target admission, optional target creation/supersession must remain one commit. Tool T1/T2 likewise require inseparable event/journal commits.

Do not weaken these into CRUD to make an object-store adapter look easy. S3 provides strong consistency and conditional writes, not native multi-key transactions (AWS). Independent PUTs to per-Session objects cannot satisfy this contract.

An S3-backed implementation needs an explicit commit protocol, such as immutable state plus a CAS-published manifest covering the transaction's full scope, or a transactional metadata service. A separate head per Session does not solve a cross-Session commit.

3. The extracted interface still carries the current implementation's organization

SessionAuthorityStore combines domain commands with legacy/runtime transcript-index maintenance, retirement cleanup, and change subscriptions.

Some of these are legitimate capabilities, but exposing the current legacy/runtime index machinery obliges every new backend to understand that representation. Move rebuildable indexing/migration concerns behind the implementation where possible, and expose narrow consumer-facing capabilities. Also specify whether transcript subscriptions observe this instance's commits or remote writers.

Numeric revisions and event ordinals are not inherently SQLite leaks. The concern is semantic responsibility, not cosmetic type purity.

4. The tests establish local correctness, not substitutability

The new contract tests instantiate the real local composition and inject failures with SQLite triggers. The dependency guards inspect imports. Both are useful; neither demonstrates that an independent implementation satisfies the same contract.

The minimum convincing next step is:

  • A supported provider factory, with no SQLite construction required by the in-memory backend.
  • One shared behavioral contract suite run against Memory and Local.
  • Atomic visibility, identity conflicts, idempotent replay, lost commit acknowledgements, snapshot semantics, and closed-owner rejection covered by that suite.
  • A Host integration test using the alternative persistence provider without changing coordinators.
  • An explicit remote ownership/fencing and commit design before claiming S3-backed live-state support.

Bottom line: this is a reasonable local dependency cleanup. It is not yet evidence of a clean, interchangeable storage architecture. Keep live transactional state, immutable object storage, and checkpoint publication as distinct contracts; do not conflate “can publish checkpoints to S3” with “can replace live SQLite with S3.”

Static architecture review only; I did not rerun the tests. This review comment was prepared and posted with OpenAI Codex assistance.

@MicroGery MicroGery changed the title refactor(storage): isolate local persistence contracts refactor(storage): verify replaceable execution persistence Sep 9, 2026
@github-actions github-actions Bot added effort/XXL Over 2500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 10, 2026
@likun666661

Copy link
Copy Markdown
Member

Follow-up architecture review at e3ae678b

The provider injection, independent Map implementation, shared conformance tests, and real Host substitution test materially address the previous review. I withdraw the earlier objection that a complete Memory backend cannot enter through supported composition. Keeping Graph and Goal in the same consistency domain is also the right direction.

There are still two concrete behavioral mismatches in the reference implementation:

[P2] Unpublished conversation copies become visible in Memory catalogs

selectCatalog() does not exclude conversationCopy.state === 'preparing'. Both list() and listCatalogPage() use this helper, so a target whose copy is still being prepared can appear in ordinary catalog results.

Local explicitly excludes these records in list() and the paginated SQL query.

Please preserve the same publication visibility rule in Memory and add shared tests for a preparing copy being absent, then becoming visible only after publication. This is an observable difference for unchanged consumers, not just an internal representation difference.

[P2] Memory pagination uses incompatible sort and cursor comparisons

Memory sorts equal-activity entries with localeCompare(), but advances the cursor with id > cursor.sessionId.

Those are different orderings. With equal activity timestamps, a simple Node reproduction sorts IDs as a, A, b, B; after returning a, the cursor predicate excludes A and advances to b. This can skip records even when the catalog is unchanged. Local's SQL ordering and cursor predicate use consistent comparisons.

Please use one deterministic ordering compatible with the Local contract for both sorting and cursor advancement. Add a shared pagination test with equal timestamps, mixed-case IDs, and page size 1, asserting that traversing every page returns each expected Session exactly once.

Updated assessment

  • Supported Memory/Local composition: addressed.
  • Behavioral equivalence: substantially improved, but the catalog cases above are missing from the shared suite.
  • Representation independence: still intentionally bounded by legacy/runtime transcript-index compatibility operations.
  • S3-backed live state: now has an injection point, not a qualified implementation. Transaction publication and remote ownership/fencing remain separate work.
  • Whole-application storage substitution: still out of scope; Artifact payloads, runtime policy and other independently composed domains remain local, as the updated description correctly states.

These findings concern the new Memory reference, not evidence of a regression in default Local behavior. The architectural progress is real; the remaining requirement is to make the reference and conformance suite accurately enforce the consumer-visible contract.

Verification: static review plus a small Node reproduction of the comparator mismatch; I did not rerun the project suites. This comment was prepared and posted with OpenAI Codex assistance.

Preserve existing leased SQLite domain transactions and verify WorkHub recovery across commit-before-dispatch process death. Fix copied attachment validation and durable replay identity.

Generated-by: OpenAI Codex
Opt WorkHub into locked, metadata- and payload-verified reuse without weakening strict conversation-copy defaults. Cover duplicate delegations, pre-assignment failures, partial batches, reopen/concurrent replay and conflicting copies.

Generated-by: OpenAI Codex
Compose an authenticated execution provider with the existing lease lifecycle. Keep Graph provisioning and Goal retirement in its transaction scope, implement an independent Memory reference, and run shared conformance plus a real Host replacement scenario.

Generated-by: OpenAI Codex
Retain grouped Goal and Interaction backend bindings until successful group shutdown. Validate copied runtime ledgers and rebuild Memory Tool projections atomically. Preserve approved sandbox profiles through configuration changes and temporary permission modes.

Add shared Local/Memory regressions for child lifecycle, copy identity and replay, recovery evidence, sandbox authority and metadata conflicts.

Generated-by: OpenAI Codex
Adapt real Host recovery tests to admitted coordination Turns, bind delegated text and target attachments in the reference provider, and preserve recoverable catalog role reads after rebasing main.

Generated-by: OpenAI Codex
Hide preparing conversation copies from catalog queries while preserving recovery access. Use one ASCII Session ID comparator for ordering and cursor advancement, with shared Local and Memory regression coverage.

Generated-by: OpenAI Codex
@MicroGery
MicroGery force-pushed the codex/local-runtime-persistence-contracts branch from e3ae678 to bcc0ca2 Compare September 10, 2026 08:26
@likun666661

Copy link
Copy Markdown
Member

Follow-up review at bcc0ca2e

The two previous catalog findings are addressed: preparing copies are hidden from catalog reads while remaining available for recovery, and pagination now uses the same ASCII comparison for sorting and cursor advancement. The shared publication and mixed-case pagination tests are appropriate coverage. Those two findings can be closed.

One further consumer-visible mismatch remains:

[P2] Memory's unfiltered catalog excludes subagent Sessions, unlike Local

In selectCatalog(), the missing-filter branch evaluates !r.header.subagentParent. Thus both list() and listCatalogPage() silently exclude every subagent Session when subagentParentSessionId is omitted.

Local only adds a parent predicate when the filter is explicitly supplied: see the paginated query and list predicate. An unfiltered Local catalog does not exclude otherwise eligible subagent Sessions.

This reaches an actual consumer: Host catalog queries pass undefined as the filter. Selecting Memory therefore changes the returned Session inventory without changing the request or coordinator.

Please make absence of the filter mean “no parent restriction,” matching Local. Add shared Local/Memory cases with two parent Sessions and their children, verifying:

  • Unfiltered list() and a complete listCatalogPage() traversal include all otherwise eligible parent and child Sessions.
  • An explicit parent filter returns only that parent's children.
  • Pagination neither loses nor duplicates children.

The new catalog tests create ordinary Sessions but do not exercise parent filtering, so they do not catch this difference.

This is a mismatch in the new Memory reference, not evidence of a regression in default Local behavior. The supported substitution entry point is now established; the remaining issue is behavioral conformance.

Verification: static incremental review only; I did not rerun project tests. This comment was prepared and posted with OpenAI Codex assistance.

Match Local catalog semantics by restricting subagent parentage only when explicitly requested. Add shared Local and Memory list and pagination coverage for two parent Sessions and their children, including empty filters, unmatched parents, and complete traversal.

Generated-by: OpenAI Codex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants