feat(storage): add durable file SessionRepository - #4674
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 3b086e70a3b5307de0a70e5e565cf02ebebc0370 (OPEN, MERGEABLE, no hosted check-runs yet). One P1 and one P2 below; the diff is 2 files (+1215: 1004 implementation, 211 tests).
P1 — the new adapter has no package surface and no production consumer, if it claims to deliver a usable workspace adapter
packages/storage/src/file-session-repository.ts:72-80 exports openFileSessionRepository, but packages/storage/package.json:7-62 has no ./file-session-repository export. Importing it from the built package entry reproducibly yields ERR_PACKAGE_PATH_NOT_EXPORTED, and a repo-wide search finds only the new tests importing via relative path — no production caller. So the 1004-line implementation is currently unreachable to @maka/storage consumers; the feature is effectively test-only. If this is intentionally an intermediate foundation with no consumer yet, please state that staging boundary in the PR; otherwise add the export, a real caller, and integration tests before merging.
P2 — first creation of an object-prefix directory does not durably publish the full directory chain
file-session-repository.ts:290-303 creates objects/<2-char-prefix> with recursive mkdir, writes the temp file with sync(), then after linking only calls syncDirectory(dirname(destination)). But session-repository.ts:77-85 requires publish to return only once the exact bytes/metadata are durably readable. When the prefix (or objects) is newly created in this call, fsyncing just the leaf directory does not persist the parent directory entries; after a crash the whole new prefix can be lost, violating that contract. The existing stable-storage.ts:102-123 already provides syncDirectoryChain with a root boundary, and other publish paths in this repo use it after new-directory creation. Please sync to the storage root (or an equivalent full boundary) and add a crash/persistence test for first-time new-prefix creation.
What was checked on this head
@maka/core and @maka/storage builds pass; compiled file-session tests 4/4 and existing session-repository tests 18/18 pass; git diff --check clean. What I could not judge: real power-loss directory-entry durability is not proven by local tests, and there are no hosted checks on this head yet.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @未开智选手 的审查。我自己没有读这份 diff；我核的是当前 head 有没有漂移、以及 exact-head 的 CI 状态。当前 head 是 3b086e7，可合并未关闭，暂无线上检查。P1 是新增实现没有包导出也没有生产调用方，P2 是首次建目录链时持久化发布不同步完整链。修好或明确阶段边界后再审。
Generated-by: Codex
Generated-by: Codex
3b086e7 to
47c5c50
Compare
likun666661
left a comment
likun666661
left a comment
There was a problem hiding this comment.
Reviewed exact head 47c5c509d2f56430b71bee7f6cdb4e18ed81663a and the stacked interaction with #4662. Focused tests pass on the exact head and on a synthetic merge with current main; the full Storage suite passes with Node experimental warnings suppressed. Two P1 correctness/durability gaps and two P2 conformance/operability gaps are inline. No hosted checks are currently reported for this head.
| sessionId, | ||
| agentId, | ||
| head, | ||
| nextRevisionNumber: requireRevisionNumber(value.nextRevisionNumber), |
There was a problem hiding this comment.
[P1] Validate revision allocation against the decoded head
nextRevisionNumber is admitted independently with only a >= 2 check. A syntactically valid damaged state can therefore reopen successfully with head r2 and nextRevisionNumber: 2; the next commit then publishes a different checkpoint under r2 again. I reproduced exactly that, so one SessionRevisionRef can denote two different checkpoints instead of the adapter failing closed. Decode should validate the complete session invariant: created revision is r1, the current revision and next counter are canonical and monotonic, lineage agrees across the session records, and commit receipts cannot contradict the session. Add a regression that mutates the counter in otherwise-valid JSON and expects integrity_mismatch.
| } | ||
| try { | ||
| await linkNoReplace(temporary, destination); | ||
| await syncDirectoryChain(dirname(destination), this.storageRoot); |
There was a problem hiding this comment.
[P1] Establish the durability barrier on the EEXIST path too
A concurrent publisher can observe EEXIST after another process has linked destination but before that process fsyncs the directory chain. This branch suppresses EEXIST and proceeds directly to assertReadable, so the second publish may return successfully while the directory entry is still not durable; a power loss in that window can lose an object whose publication already returned. Run syncDirectoryChain(dirname(destination), storageRoot) after the link attempt regardless of whether this writer created the link or observed EEXIST, then verify readability. A multi-process fault-injection test should pin this race.
| await assertCheckpointReadable(this.objectStore, prior.result.checkpoint); | ||
| return copyCommittedSessionRevision(prior.result); | ||
| } | ||
| await assertCheckpointReadable(this.objectStore, admitted.checkpoint); |
There was a problem hiding this comment.
[P2] Preserve the contract error precedence before reading candidate objects
Unlike the in-memory conformance implementation, this path verifies the candidate checkpoint before checking whether the Session exists or whether expectedRevision is stale. Reproduction: advance r1 to r2, remove the now-unreferenced candidate Manifest, then call commit with expected r1; this adapter returns object_not_found instead of the contract-required stable revision_conflict. createSession has the same ordering mismatch for an already-existing Session. Perform a state preflight before the expensive object read, then recheck under the mutation lock, and run the same parameterized conformance cases against both implementations.
|
|
||
| async publish(input: ImmutableObjectInput): Promise<ImmutableObjectRef> { | ||
| const admitted = admitImmutableObjectInput(input); | ||
| const bytes = await readSourceBytes(admitted); |
There was a problem hiding this comment.
[P2] Stream file-backed objects instead of buffering every Bundle
Both publication here and assertReadable below use readFile, so every large Session Bundle is loaded completely into memory before it is copied or verified. Checkout also hashes the whole Bundle through that path. This makes the file adapter vulnerable to memory exhaustion on exactly the large immutable objects this port is intended to hold, and there is no adapter-owned byte bound. Stream file inputs into the private temporary file while counting and hashing, and stream verification with an explicit caller-owned limit; the bytes variant can remain bounded in memory.
3 participants
Summary
Implements phase 2 of #2370 on top of #4662: an internal durable local adapter for the established Session checkpoint contract. It is intentionally not yet a public package entrypoint or a production composition consumer.
The adapter keeps immutable Bundle and Manifest bytes in non-overwritable local files verified by digest. Session heads, CAS revisions, commit receipts, and Fork records live in a small atomically replaced local control-plane document.
All control-plane state mutations take a cross-process atomic-mkdir lock. The control-plane document fsyncs a private temporary file before atomic rename and syncs its containing directory. Immutable objects are independently published through a fsynced temporary file, no-replace link, and directory-chain sync to the storage root. A crash may leave a lock directory, in which case writers fail closed instead of guessing that it is safe to steal; reads continue from the last complete state document.
Semantics covered
This is a stacked PR: its base is the #4662 branch. It must merge after #4662 is merged or be retargeted and rebased then.
Non-goals
Refs #2370
Verification
AI use
Tool(s) and scope: Codex implemented the local durable adapter, its tests, and this PR description under human contributor ownership.
Checklist
Does this PR entail a change in behavior?