fix(storage): publish imported context payloads atomically - #5186
Conversation
`copyContextValueTree` filled its destination with `copyFile` and treated `EEXIST` as nothing to do. Both halves are wrong, and together they lose data: a copy interrupted midway leaves a truncated payload at the path other Sessions read, and the retry accepts it as already present. Payloads now become visible the way the Context Store publishes its own, by `link` from a staging name, so the final path is never observable half-written -- measured at 38 distinct intermediate sizes while copying 64 MiB through `copyFile`, and none through `link`. An interrupted attempt leaves its bytes under the staging name, which the next attempt clears. Reaching `EEXIST` at the final path now means the payload was already there; content addressing says it must be byte-identical, and a payload that is not is reported rather than silently accepted. Also let a caller supply the Storage Root authority instead of electing it. The owner lock is an election taken with `tryLock`: it refuses a second exclusive hold even inside the process that already has one, so a Runtime Host cannot prepare or accept a bundle by calling these at all -- it can only lend the authority it took at startup. A lease must name the same root it is used on. Callers that hold no authority, the CLI included, omit it and elect as before. Refs apache#5182
M4n5ter
left a comment
There was a problem hiding this comment.
English
Review summary
Scope: 898ac52d through c9391ee.
Blocking findings
-
A lent lease does not fence Context Store mutations.
withOfflineContextSnapshotmarks a lent lease ascontextLocked, butrunWithStorageRootLeaseonly tracks lifetime, while the Context Store permits concurrent operations (writer facade). Import publishes files before inserting rows (merge path); GC can observe the old database state and unlink that payload after the import commits its reference (GC check). A fresh target has a second race becausecopyFilecan overlap first Store initialization and replace its SQLite file. The artifact lock does not cover Context Store operations. The required boundary is one root-scoped context mutation fence shared by import, Store initialization/publication, and GC. -
The supplied lease can be detached from the actual root used by the bundle operation. Both policy paths validate the lease and then reacquire an artifact lock from mutable
input.stateRoot(export, import). If an alias or root is replaced between those steps,withArtifactWriterLockcan bind a replacement root, including an unmarked one, while the lease still names the original root. The existingprepareArtifactWriterLockAuthorityForLeaseandwithLeaseBoundArtifactWriterLockare the appropriate boundary. -
Hydrated context is not semantically validated before import.
mergeBundleContextcopies files and rows without calling the existingvalidateContextSnapshot. Archive digests authenticate the archive representation but do not bindblob_id, locator, size, and payload bytes. A hydrated state whose database identifiedABCwhile its managed file containedXYZimported successfully with a context reference, leaving an unreadable payload. Validate before any target writes and restrict refs to imported Session ids.
Important findings
-
The new hard-link publisher lacks a directory durability barrier. It syncs the staging file, then links and unlinks it without syncing the containing directory (publisher). A crash can preserve committed rows while losing the final directory entry. The Store's equivalent explicitly syncs the directory chain (Store publisher).
-
Destination managed paths are not fail-closed.
resolveInsidechecks lexical containment only, andcopyContextValueTreefollows pre-existing symlinked ancestors duringmkdir, cleanup, andlink. Withcontext-offload-valuessymlinked outside the Storage Root, import writes the payload there. The Store already has the required managed-directorylstat/realpathinvariant (Store check). -
Importing an existing blob does not clear its GC candidate. The import inserts/ignores blobs and refs (context merge) but does not remove
context_gc_candidates. When a duplicate blob is already a candidate, the next GC fails withContext garbage candidate is still referenced or missing. Normal Store insertion clears this candidate (Store insertion).
Verdict and validation
- Correctness: not acceptable until the three blocking paths are closed.
- Design: not acceptable because
contextLockedrepresents ownership lifetime, not mutation exclusion. - No safe deletion or consolidation is apparent; the lease and staging concepts are required by the stated behavior.
- Core, storage, MCP, and runtime builds passed; the selected runtime/storage suites passed 95/95. Biome,
git diff --check, and the PR CI checks were green.
中文
审查总结
审查范围:898ac52d 到 c9391ee。
Blocking 问题
-
借用的 lease 没有阻止 Context Store 并发 mutation。
withOfflineContextSnapshot把 lease 当成contextLocked,但runWithStorageRootLease只跟踪生命周期,Context Store 仍允许并发操作。导入先发布文件再写数据库行,GC 可能依据旧状态删除刚被导入并引用的 payload;新建 Context Store 时,copyFile也可能覆盖并发初始化的 SQLite 文件。需要由导入、Store 初始化/发布和 GC 共享 root-scoped context mutation fence。 -
lease 校验后,bundle 操作仍可能绑定到另一个 root。 导入和导出重新从可变的
input.stateRoot获取 Artifact lock;如果 alias 或 root 在两步之间被替换,操作可能在原 lease 授权下读写替换后的 root。应使用已有的prepareArtifactWriterLockAuthorityForLease与withLeaseBoundArtifactWriterLock。 -
导入前没有验证 hydrated context 的语义一致性。 当前只验证归档表示的 digest,没有验证
blob_id、locator、size 和实际 payload 的对应关系。我复现了数据库声明ABC的 hash、文件实际为XYZ但导入仍成功的情况,之后 payload 不可读。应在任何目标写入前调用 context snapshot validator,并限制 refs 属于导入的 Session。
Important 问题
-
hard-link 发布缺少目录持久化屏障。 staging 文件同步后直接
link/unlink,没有同步目标目录;崩溃后数据库行可能保留而目录项丢失。 -
目标 managed path 没有 fail-closed 校验。 词法 containment 不能阻止 symlink ancestor;我复现了通过
context-offload-valuessymlink 把 payload 写到 Storage Root 外部。 -
导入已有 blob 后没有清理 GC candidate。 目标 blob 已处于 candidate 时,导入新 ref 不会删除 candidate;后续 GC 会持续失败。
结论与验证
- Correctness:三个 Blocking 路径关闭前为 not acceptable。
- Design:not acceptable,因为
contextLocked代表的是持有权生命周期,不是 mutation 排他性。 - 没有可安全删除或合并的概念;lease 和 staging 都是需求所需。
- core、storage、MCP、runtime 构建通过;相关测试 95/95 通过;Biome、
git diff --check和 PR CI 均通过。
Closes the six findings from review. **A lent lease did not fence Context Store mutations.** The Store's own operations read database state, await, and only then act on files: collection decides a payload is unreferenced, awaits, unlinks it. An import publishing inside that await leaves a committed reference pointing at a file about to be removed, and the re-check collection performs cannot see it because the check and the unlink straddle the await. The Store already serialised those operations, through a promise tail private to the instance, which fenced nothing outside it. That queue is now keyed by Storage Root and shared, so an importer takes the same turn. In-process is the whole boundary: the interactive write authority is an exclusive election, so one process at a time mutates a root's context. **The lease could be detached from the root actually used.** Both paths validated the lease and then derived the Artifact writer lock from `input.stateRoot` again, so an alias or a replaced directory between the two steps would bind the operation to one root while the lease named another -- including an unmarked one, which takes no lock at all. With a lease, the lock is now derived from the lease. **Hydrated context was not validated.** An archive digest authenticates the archive, not the state inside it: a row claiming a hash and a file that does not hash to it survives it. `validateContextSnapshot` already exists and the export already runs it on its own output; the import now runs it on the bundle before writing anything to the target. Also: sync the directory chain after linking a payload, so a crash cannot keep the committed row and lose the name; refuse a destination payload directory whose realpath leaves the Storage Root, since lexical containment says nothing about a symlinked `context-offload-values`; and clear `context_gc_candidates` for blobs the import references again, which otherwise makes every later collection fail. Refs apache#5182
|
All six fixed in 1. Context mutation fence. The Store already serialised its own publication against its own collection, through a promise tail private to the instance — so the mechanism existed and fenced nothing outside it. That queue is now keyed by Storage Root and shared, and the import takes the same turn. In-process is the whole boundary: the interactive write authority is an exclusive election, so one process at a time mutates a root's context. 2. Lease detached from the root used. With a lease, the Artifact writer lock is now derived from the lease rather than from 3. Hydrated context not validated. 4. The directory chain is synced after the link. 5. A destination payload directory whose Tests: the fence has unit tests for serialisation, failure release, and per-root independence, plus one that holds a turn on a real workspace and asserts an import waits for it — built on the real Each was checked by reverting the implementation it covers — skipping validation, skipping the destination check, leaving the candidates, and bypassing the fence — and each turns the matching test red. One thing I did not do: the gate is not a published entrypoint. Its only consumers are inside |
M4n5ter
left a comment
There was a problem hiding this comment.
English
Approved at fd71912b. The prior blocking paths are closed: live Store mutations now share the import fence, lease-bound operations keep authority attached to the canonical root, hydrated payload identity is validated, payload publication is durable, destination directory symlinks are refused, and re-referenced target blobs leave the GC candidate set.
The remaining paths do not block this PR:
- Important — crafted bundle context closure. Normal exports clear transient GC state and retain refs only for exported Sessions. A checksum-valid bundle can still be crafted with a live blob in
context_gc_candidatesor a ref owned by a Session absent from the bundle. A fresh-target import copies that database as-is; the former makes later GC fail and the latter leaves an unreachable ref. This is limited to crafted input and does not cross an authority boundary, but import validation should eventually require every context ref to belong to an imported Session and reject transient GC state. - Follow-up — fresh Store initialization. The offline path excludes a live Store, and the Host opens storage before registering handlers. The race requires a caller holding one lease to start fresh-target import and Store initialization concurrently; Store initialization can then observe the progressive SQLite copy. Retrying the Store open after import recovers.
- Follow-up — final payload symlink. This requires a pre-tampered target with a symlink at the exact content-addressed payload path whose target already contains identical bytes. Import accepts it through the
EEXISTcomparison, while a later Store read rejects it as corrupt. A no-follow regular-file comparison would harden this edge case.
Core, storage, and runtime builds passed; the relevant suites passed 73/73; Biome, git diff --check, and CI passed.
中文
批准 fd71912b。此前的阻塞路径已经关闭:live Store mutation 与导入共享 fence;lease authority 始终绑定 canonical root;hydrated payload identity 得到校验;payload 发布具备持久化屏障;目标目录 symlink 被拒绝;重新获得引用的目标 blob 会离开 GC candidate 集合。
剩余问题不阻塞本 PR:
- Important — 构造 bundle 的 context 闭包。 正常导出会清空 transient GC 状态,并只保留导出 Session 的 refs;但仍可构造 digest 合法、同时包含 live
context_gc_candidates或无对应 Session ref 的 bundle。新目标会原样复制该数据库,前者使后续 GC 失败,后者留下无法通过 Session 生命周期清理的 ref。该路径限于构造输入,不跨越 authority 边界;后续应要求所有 context refs 属于本次导入的 Session,并拒绝 transient GC 状态。 - Follow-up — 新目标的 Store 初始化。 离线路径会排斥 live Store,Host 也会在注册 handler 前完成 storage 初始化。只有持有同一 lease 的调用者主动并发启动新目标导入和 Store 初始化时才会触发;Store 可能看到渐进复制中的 SQLite 文件,导入完成后重试 Store 打开即可恢复。
- Follow-up — 最终 payload symlink。 需要目标 workspace 预先被篡改,在精确的 content-addressed payload 路径放置 symlink,并且目标文件已有完全相同的 bytes。导入会通过
EEXIST比较接受它,之后 Store 读取会判定为 corrupt。后续可用 no-follow regular-file comparison 加固。
core、storage、runtime 构建通过;相关测试 73/73 通过;Biome、git diff --check 和 CI 通过。
Three paths M4n5ter raised as non-blocking on apache#5186. **A bundle's context could describe more than the bundle.** The snapshot validator proves each payload is the bytes its row claims and says nothing about who those rows belong to; the archive digest authenticates the archive, not the state inside it. So a bundle that was assembled rather than exported passes both while carrying a reference owned by a Session it does not include -- which can never be released, because that happens when its Session is retired -- or collection state from the workspace it left, which names a blob the target now references and makes every later collection fail. A fresh target adopts the bundle's database whole, so neither is transient. Both are refused. **A fresh target received its context database by progressive copy.** That path is the one a Context Store opens to decide whether the workspace has a store at all, so a Store initialising alongside the copy could read a database only partly there. Staged and renamed, it is absent or complete. **A payload path could be a symlink onto matching bytes.** Compared through an ordinary read it looked like the same content arriving twice, and the import accepted a tree the Store will not read -- it refuses to read through a link and reports the payload corrupt. The comparison now opens no-follow and requires a regular file, so a planted link, or a directory, is different content rather than the same content. Refs apache#5182
…#5196) * fix(storage): require an imported bundle's context to describe itself Three paths M4n5ter raised as non-blocking on #5186. **A bundle's context could describe more than the bundle.** The snapshot validator proves each payload is the bytes its row claims and says nothing about who those rows belong to; the archive digest authenticates the archive, not the state inside it. So a bundle that was assembled rather than exported passes both while carrying a reference owned by a Session it does not include -- which can never be released, because that happens when its Session is retired -- or collection state from the workspace it left, which names a blob the target now references and makes every later collection fail. A fresh target adopts the bundle's database whole, so neither is transient. Both are refused. **A fresh target received its context database by progressive copy.** That path is the one a Context Store opens to decide whether the workspace has a store at all, so a Store initialising alongside the copy could read a database only partly there. Staged and renamed, it is absent or complete. **A payload path could be a symlink onto matching bytes.** Compared through an ordinary read it looked like the same content arriving twice, and the import accepted a tree the Store will not read -- it refuses to read through a link and reports the payload corrupt. The comparison now opens no-follow and requires a regular file, so a planted link, or a directory, is different content rather than the same content. Refs #5182 * fix(storage): state the portable context shape once, and publish by creating Three follow-ups M4n5ter raised on #5196. **One validator, not two.** `validateContextSnapshot` checked payload hashes and usage arithmetic; a second, bundle-only check added the Session closure. Neither covered the transient state a snapshot settles, so a tree could decode and still be unusable: a surviving deletion queue drains bytes the target never had, an unreferenced blob is quota nothing reclaims, and a surplus usage row fails that Session's next write. The shape a snapshot writes is now stated in the one place that validates it, including the Session restriction a bundle needs, and the second check is gone -- it could only ever drift from the first. **Publication creates; it never replaces.** Asking whether the context database exists and branching on the answer is a decision that can be stale by the time it is acted on: a Context Store initialising under the same lease creates that file, and an import that already decided "absent" replaced it. On POSIX the Store then keeps writing to the unlinked inode while every later open reads the new one, so its writes are invisible and gone at the next restart. There is now one publication path -- stage, then `link` -- and the filesystem decides which case it is. **The collision reader is the repository's.** `readStableBoundedFile` is non-blocking, so a FIFO planted at a payload path cannot hang the import, and it compares the opened file against the path, which is the final-link check `O_NOFOLLOW` does not give on Windows. Refs #5182 Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
Summary
Two changes to how a Session bundle reaches a workspace, both needed before the desktop can offer this at all (#5182).
A payload could arrive truncated, and the retry would accept it
copyContextValueTreecopied withcopyFileand swallowedEEXIST:copyFilefills its destination progressively, so the final path — the one other Sessions read — is observable half-written. Measured by copying 64 MiB and stat-ing the destination every millisecond:Put the two together and an import interrupted midway leaves a truncated payload at the final path, and the next attempt reads
EEXIST, concludes the payload is already there, and reports success over it.Payloads now become visible the way
SqliteContextOffloadStorepublishes its own: assembled under a staging name,fsynced, then made visible by a singlelink. An interrupted attempt leaves its bytes under the staging name, which the next attempt clears. The staging name is the Store's with a different suffix — two imports cannot race, since both need the write authority and it is exclusive, but an import and the Store publishing the same blob can, and they must not share a name.Reaching
EEXISTat the final path now means the payload was genuinely already there. Payloads are content-addressed, so the same path in two workspaces is supposed to mean the same bytes; when it does not, the target holds something this bundle cannot explain, and that is reported rather than resolved by overwriting another Session's payload.Authority can now be lent instead of elected
exportSessionBundleStateandimportSessionBundleStatetake an optionallease. Without one they elect the owner exactly as before, so the CLI path is unchanged.The reason this is needed: a Runtime Host takes the Storage Root owner at startup and holds it for its lifetime, and that lock is an election, not a mutex. It is taken with
tryLock, and a second exclusive hold is refused even inside the process that already has one:So a Host cannot reach either function by calling it — it would be refused by its own lock. Lending the lease is the only way, and it is what lets the app offer export and import without asking the user to close it first.
withLeaseBoundArtifactWriterLockis the same pattern, already in this package.A lease authorises only the root it names, and one naming a different root is refused: a valid lease for somewhere else would otherwise authorise writing to a directory nobody holds.
Tests
6 new, across
@maka/runtime's export and import suites — that is where the fixtures for these functions live.Each was checked by reverting the implementation it covers: the old copy-and-swallow, not clearing a leftover staging file, leaking the staging file, skipping the lease root check, ignoring the lease, and dropping the lease passthrough in the runtime wrapper. Every one turns the matching test red.
session-bundle-policy,context-offload-snapshot,context-offload-store,sqlite-context-offload-store,artifact-writer-lock,production-session-snapshotandpublic-entrypointsare unchanged and green — those cover the callers that do not pass a lease.Gates:
@maka/core,@maka/storage,@maka/runtimebuild and typecheck clean;biome checkon every changed file;check:asf-headerspasses. No schema change, no protocol epoch change.What this does not do
Nothing calls the lease yet. The Host operations and the desktop surfaces are the next PR under #5182.