You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Joob1n (via GitHub)" <gi...@apache.org> on 2026/09/11 09:35:34 UTC
[PR] fix(storage): require an imported bundle's context to describe itself [maka]
Joob1n opened a new pull request, #5196:
URL: https://github.com/apache/maka/pull/5196
## Summary
The three paths @M4n5ter raised as non-blocking when approving #5186, under #5182.
## A bundle's context could describe more than the bundle
`validateContextSnapshot` proves 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 reference owned by a Session the bundle does not include.** References are released when their Session is retired, and that Session never arrives, so the reference is permanent.
- **Collection state from the workspace it left.** The export empties that queue on its private copy. One that survives names a blob the target now references, and collection treats a referenced candidate as corruption — it fails, and keeps failing.
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
`EEXIST` at 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 `EISDIR` from 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:
- The first version of the foreign-reference fixture moved `context_refs.session_id` without moving the usage rows, so `validateContextSnapshot` caught 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_NOFOLLOW` answers the symlink at `open`, 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-store` and `production-session-snapshot` are unchanged and green.
Gates: `@maka/core`, `@maka/storage`, `@maka/runtime` build and typecheck clean; `biome check` on both changed files; `check:asf-headers` passes. No schema change, no protocol epoch change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(storage): require an imported bundle's context to describe itself [maka]
Posted by "M4n5ter (via GitHub)" <gi...@apache.org>.
M4n5ter merged PR #5196:
URL: https://github.com/apache/maka/pull/5196
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(storage): require an imported bundle's context to describe itself [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #5196:
URL: https://github.com/apache/maka/pull/5196#issuecomment-5639835335
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 `O_NOFOLLOW` does not give on Windows" is wrong, and you are right about why: `lstat` there does not reliably report a file symlink as one, so the identity comparison cannot stand in for the flag. The comment now says what the reader actually gives on each platform — a mid-read identity change is caught everywhere, and the symlink refusal is `O_NOFOLLOW` on POSIX and only what the platform makes visible on Windows.
**The asymmetry.** Checked rather than assumed: `copyContextValueTree` reaches a payload only through `entry.isFile()` on a `Dirent`, and a symlink is `isSymbolicLink()` there, so `left` cannot be a link. That is now stated where the asymmetry is, rather than left to be rediscovered.
**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.
https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(storage): require an imported bundle's context to describe itself [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #5196:
URL: https://github.com/apache/maka/pull/5196#issuecomment-5642659462
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 `fsync` was called would test the call rather than the property. It rests on matching the ordering the payload path already uses.
Reverting the declared-only publication turns its test red. `session-export`, `context-offload-snapshot`, `session-bundle-policy`, `session-bundle-context-fence` and `production-session-snapshot` are unchanged and green; 27 import tests pass.
https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(storage): require an imported bundle's context to describe itself [maka]
Posted by "Joob1n (via GitHub)" <gi...@apache.org>.
Joob1n commented on PR #5196:
URL: https://github.com/apache/maka/pull/5196#issuecomment-5637342164
All three fixed in `f2a7074f`.
**1. Portable closure.** You were right that the second validator was the wrong shape of fix. `validateContextSnapshot` now states what a snapshot *is* — hashes, usage arithmetic in both directions, an empty collection queue, an empty deletion queue, no unreferenced blob — plus the Session restriction when a caller supplies the ids. `assertBundleContextClosure` is gone; a bundle-only check could only ever drift from the one that validates the export's own output.
Worth noting for the record: the other caller, `validateOperationalStateBackup`, validates the output of the same `copyContextSnapshot`, so tightening the shared check does not narrow it.
**2. Creation, not replacement.** The `pathExists`-then-branch shape was the actual defect: the answer can be stale by the time it is acted on. There is now one publication path — stage, then `link` — and the filesystem decides whether this is a fresh target or a merge. `EEXIST` means a Store got there first, and the import merges into it.
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 `readStableBoundedFile`: non-blocking, so a FIFO cannot hang the import, and it compares the opened file against the path, which is the protection `O_NOFOLLOW` does not give on Windows.
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 `rename` instead of `link`; the last turns five tests red, because replacing the database loses everything the target had.
`session-export`, `context-offload-snapshot`, `session-bundle-policy`, `session-bundle-context-fence` and `production-session-snapshot` are unchanged and green. Rebased onto current main.
https://claude.ai/code/session_014ajaRxC4jydavY9nYUFj5J
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org