fix(storage): require an imported bundle's context to describe itself - #5196
fix(storage): require an imported bundle's context to describe itself#5196Joob1n wants to merge 2 commits into
Conversation
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
The normal exporter/importer path is mergeable. The remaining findings require a hand-built or tampered bundle, a narrow initialization race, or a pre-planted filesystem entry, so I consider them non-blocking follow-ups:
-
Portable context closure is still not canonical.
assertBundleContextClosure(packages/storage/src/session-bundle-policy.ts:1385) rejects foreign refs and GC candidates, but still acceptscontext_file_deletions, unreferencedcontext_blobs, and surpluscontext_session_usage. A fresh target adopts that database wholesale at line 1420, which can leave unreclaimable quota usage, corrupt physical-byte accounting when the deletion queue drains, or make later writes fail against forged session usage.copyContextSnapshotalready removes these states. The simpler fix is to makevalidateContextSnapshotenforce the exact portable shape in both directions and remove the bundle-specific second validator. -
The fresh-database publication is atomic replacement, not atomic creation. The absence check followed by
rename(staging, targetContext)at lines 1420-1428 can overwrite a database created by a concurrent same-lease Context Store initializer. On POSIX, the open Store remains attached to the now-unlinked inode while imports and later opens use the renamed inode, causing invisible writes and data loss after restart. Use no-replace publication and merge onEEXIST, or serialize Store construction on the same context mutation gate. -
The collision reader is not fully special-file and cross-platform safe.
readRegularFileat line 1202 can block opening a FIFO on POSIX because it omitsO_NONBLOCK; on Windows,O_NOFOLLOWdoes not provide the promised final-symlink protection. Reuse the repository's stable regular-file reader or add the equivalentlstat/fstatidentity checks and non-blocking open.
中文
常规 exporter/importer 链路已经可以合入。剩余问题分别依赖手工构造或篡改的 bundle、很窄的初始化竞态，或预先占据目标路径，因此我建议作为不阻塞合入的 follow-up：
-
Portable context 的闭包仍不完整。
assertBundleContextClosure（packages/storage/src/session-bundle-policy.ts:1385）只拒绝外部 Session 引用和 GC candidate，没有拒绝context_file_deletions、无引用的context_blobs，也没有发现多余的context_session_usage。fresh target 会在第 1420 行直接采用整份数据库，后果可能是配额被永久占用、删除队列执行后 physical bytes 记账失真，或者伪造的 Session usage 让后续写入一直失败。copyContextSnapshot本来就会清理这些状态；更简单的收口方式是让validateContextSnapshot双向校验唯一的 portable shape，再删掉 bundle 层的第二套 validator。 -
fresh database 当前是“原子替换”，不是“原子创建”。 第 1420-1428 行先判断目标不存在，再执行
rename(staging, targetContext)；如果同一 lease 下的 Context Store 恰好在复制期间完成初始化，POSIXrename会覆盖它。已经打开的 Store 继续写入失去路径的旧 inode，import 和后续进程则读取新 inode，最终形成不可见写入，并在重启后丢失。这里应使用 no-replace publication，遇到EEXIST后转入 merge；或者让 Store 初始化也经过同一个 context mutation gate。 -
碰撞读取对特殊文件和 Windows 的处理还不完整。 第 1202 行的
readRegularFile在 POSIX 上可能阻塞于 FIFO，因为没有O_NONBLOCK；Windows 上的O_NOFOLLOW也不能提供注释承诺的 final-symlink 防护。建议复用仓库已有的 stable regular-file reader，或补齐非阻塞打开及lstat/fstat身份校验。
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
d76ae78 to
f2a7074
Compare
…reating Three follow-ups M4n5ter raised on apache#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 apache#5182 Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
|
All three fixed in 1. Portable closure. You were right that the second validator was the wrong shape of fix. Worth noting for the record: the other caller, 2. Creation, not replacement. The This also made the property testable. The race window version I wrote first could not land its injection reliably, and a test that silently takes the other path is worse than none; with a single path, "the target already has a database" exercises exactly the same branch. 3. Collision reader. Replaced with Tests: a bundle carrying an unreferenced payload, one carrying usage for a Session it does not hold, and a target whose existing context database keeps its own reference through the import. Each checked by reverting what it covers — allowing the orphan blob, allowing the surplus usage row, and
|
…reating Three follow-ups M4n5ter raised on apache#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 apache#5182 Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
f2a7074 to
097fcbc
Compare
|
Thanks. Two of the three uncertainties were worth acting on, because both were my comments claiming more than the code gives: The Windows claim was too strong. "the final-link check The asymmetry. Checked rather than assumed: Comment hygiene — you were right, I had appended the new note without removing the one it replaced. One comment now. No behaviour change; the 25 import tests and the gates are unchanged and green. |
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
The three findings from the previous review are fixed: the snapshot validator now enforces the missing database invariants, fresh-database publication no longer replaces a concurrent Store initializer, and collision reads use the stable bounded reader. I found two additional correctness gaps:
-
Fresh context-database publication is atomic, but not durable.
mergeBundleContextDatabasecopies to staging and linkscontext-offload.sqliteatpackages/storage/src/session-bundle-policy.ts:1388-1397, but it syncs neither the copied inode nor the new directory entry beforemergeBundleDatabasecommits the imported Session at line 1009. After power loss, the Session can survive while its context database link or contents do not. The managed-payload path at lines 1570-1584 already has the required ordering: sync the staging file, link it, then sync the directory chain before publishing references. Apply the same barriers to the fresh database branch. -
Validation checks declared payloads, while import copies the entire values tree.
validateContextSnapshotiteratescontext_blobsatpackages/storage/src/context-offload-snapshot.ts:256-277, so it verifies every database locator but never rejects extra files.copyContextValueTreethen recursively publishes every regular file undercontext-offload-valuesatpackages/storage/src/session-bundle-policy.ts:1482-1504. A digest-valid hand-built bundle can therefore add undeclared bytes that are neither charged to usage nor reachable by GC. Either require the hydrated tree inventory to exactly match the managed-file locators, or publish only the locator set that the validator admitted.
中文
上次 review 的三项问题已经修复：snapshot validator 补齐了数据库不变量，fresh database 的发布不会再覆盖并发初始化的 Store，碰撞读取也改用了 stable bounded reader。复审中又发现两处 correctness 缺口：
-
fresh context database 的发布具备原子性，但没有持久性保证。
packages/storage/src/session-bundle-policy.ts:1388-1397先复制 staging 文件，再为它创建context-offload.sqlite硬链接；在第 1009 行提交导入的 Session 之前，却没有同步 staging inode，也没有同步新目录项。掉电后可能出现 Session 已经保留，但 context database 的链接或内容丢失。第 1570-1584 行的 managed payload 发布已经给出了正确顺序：先同步 staging 文件，再创建链接，最后同步目录链，然后才能发布引用。fresh database 分支也应使用同样的 barrier。 -
校验只覆盖数据库声明的 payload，导入却会复制整棵 values tree。
packages/storage/src/context-offload-snapshot.ts:256-277的validateContextSnapshot会遍历context_blobs，因此能验证每个数据库 locator，却不会拒绝额外文件；随后packages/storage/src/session-bundle-policy.ts:1482-1504的copyContextValueTree会递归发布context-offload-values下的所有普通文件。手工构造且 digest 合法的 bundle 因而可以夹带未声明的数据，这些字节既不计入 usage，也无法被 GC 找到。应让 hydrated tree 的文件清单与 managed-file locator 完全一致，或者只发布 validator 已确认的 locator 集合。
…reating Three follow-ups M4n5ter raised on apache#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 apache#5182 Claude-Session: https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
097fcbc to
62e18ae
Compare
|
Both fixed. 1. Durability. The staging file is synced before it is named and the directory chain after, matching the managed-payload ordering you pointed at. The Session rows commit later, so a loss between the two must not leave a Session whose context database is a name with nothing behind it — or no name at all. 2. Undeclared payloads. You identified the real asymmetry: the validator reads rows and the copy read the tree, so the two were never checking the same thing. The copy now publishes the locator set the database declares, and nothing else — a row whose bytes are absent is refused rather than half-published. Walking the tree was the wrong source of truth, not a missing check on top of it. Tests: a bundle carrying bytes no row names, asserting they did not travel while the declared payload did; and a bundle declaring a payload it does not carry. What is not covered: the durability barriers. A unit test cannot lose power, and asserting that Reverting the declared-only publication turns its test red. |
3 participants
Summary
The three paths @M4n5ter raised as non-blocking when approving #5186, under #5182.
A bundle's context could describe more than the bundle
validateContextSnapshotproves every payload is the bytes its row claims. It says nothing about who those rows belong to, and the archive digest authenticates the archive rather than the state inside it — so a bundle that was assembled rather than exported passes both checks while carrying either of:A fresh target adopts the bundle's context database whole, so neither is transient. Both are refused before anything is written.
A fresh target received its context database by progressive copy
That destination is the path 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 that is only partly there. It is now staged and renamed: absent, or complete.
A payload path could be a symlink onto matching bytes
EEXISTat a content-addressed path is normally the same bytes arriving twice, and an ordinary read through a planted symlink to matching content says exactly that. But the Context Store will not read through a link — it reports the payload corrupt — so accepting it imports a tree the Store cannot use.The comparison now opens no-follow and requires a regular file. A planted link is different content rather than the same content, and so is a directory, which previously surfaced as a raw
EISDIRfrom the read instead of saying what was wrong.Tests
4 new in the import suite: a reference naming a Session outside the bundle, a surviving collection candidate, a symlink at the payload path pointing at identical bytes, and a directory at the payload path.
Each was checked by reverting the implementation it covers — skipping the closure check, following symlinks in the comparison, and dropping the regular-file requirement — and each turns the matching test red.
Two notes from getting there, both about tests that looked like they worked and did not:
context_refs.session_idwithout moving the usage rows, sovalidateContextSnapshotcaught the tampering first and the new guard was never reached. The fixture now keeps usage consistent, so it fails on the guard it is named after.O_NOFOLLOWanswers the symlink atopen, which means the regular-file requirement is never what rejects a link. The directory case is what exercises it.session-export,session-bundle-policy,session-bundle-context-fence,context-value-mutation-gate,context-offload-snapshot,sqlite-context-offload-storeandproduction-session-snapshotare unchanged and green.Gates:
@maka/core,@maka/storage,@maka/runtimebuild and typecheck clean;biome checkon both changed files;check:asf-headerspasses. No schema change, no protocol epoch change.