fix(session-store): the write edge reports an A, B, A instead of reporting it as promoted (#390) - #410
Merged
Conversation
…rting it as promoted (#390) The read path answers a key directory swapped under a running job with two checks that between them cover every ordering: it stages the transcript from one DESCRIPTOR, so bytes cannot come from a file that replaced the name afterwards, and it re-checks the name by path after the copy, so a swap that happened before the open is caught too. The write path has neither. Node exposes no `openat`/`renameat`, so `promoteSession` re-checks the key directory's `dev:ino` under the lock and again after the rename. Both catch a swap that is STILL STANDING. Neither can see one that is put back: ``` promote begins key dir is real, its dev:ino recorded attacker swaps in key dir is now a symlink to somewhere else ... the transcript copy, the rename and the venue sentinel all land there ... attacker swaps back key dir is the real one again post-swap re-check dev:ino matches, so the promotion reports promoted: true ``` Measured here with a deterministic probe: `{"promoted":true,"reason":"promoted","bytes":60}`, the transcript in the attacker's directory, and the real key holding only `pi-version`, `resume-chain` and `venue`. The next job on that key cold-starts as `absent` while the record says the work was promoted, which is the silent no-op `CLAUDE.md` calls the worst outcome available. ## What the shape comparison cannot see, the inode can `rename` PRESERVES the inode. So the transcript's identity is read from the temp before the rename and compared against what the canonical name holds after it. If the rename landed where it was meant to, that is the same file. If the directory was swapped for the rename and swapped back afterwards, the canonical name resolves into the real key again and holds its OLD transcript, or nothing -- a different inode either way, and a fact that is still true once the shape is right again. **Detection, not prevention**, like the check beside it: the bytes are already wherever the name pointed and nothing host-side can recall them. What changes is that the record stops claiming otherwise. The new token is `transcript-diverted`, a closed-enum addition on the promote path, because `key-not-a-directory` says the name is the wrong SHAPE and by this point it is not. The fail-closed arm is deliberate and pinned: if BOTH identity reads fail, a bare comparison is `null !== null`, which is false, so the promotion would report success having verified nothing. Two failed reads are the one case where "they match" and "I could not tell" look alike. **The sidecar writes after the rename deliberately get no equivalent check.** The transcript's own identity is what decides whether the promotion landed; a sidecar diverted by the same swap is covered by the same refusal, and a second identity dance per sidecar would buy nothing the first does not already say. ## The residual this leaves, which a review pass found and the contract now states A swap put back FOR THE DURATION OF A CHECK and re-applied afterwards defeats any check that resolves the name again. An A, B, A, B around the two post-rename checks still reports `promoted: true` with the transcript outside the store -- and on a key that already held one, the next job RESUMES the superseded transcript rather than cold-starting, so the lost turn is invisible on both edges. Every path-based check has this shape; closing it needs `openat`/`renameat`, not a third check. A 45 second unsynchronised live race over 215,372 promotions against 105,888 swap cycles hit it 0 times, so it is a deterministic-window finding of exactly the class this issue was. `INT-SESSION-STORE-CONTRACT` says so, and its residual count moves from two to three. One assumption is added and named, because it is the first in that file to rest on it: that `rename` PRESERVES the inode. It does on every filesystem this project supports. On one that derives inode numbers from the path (SMB/CIFS without `serverino`, some FUSE) every promotion would refuse with a token that reads as an attack, which is the same class as the `O_EXCL` semantics the lock already assumes. ## Tests Six mutations, all red, and the sixth is the one a review pass found surviving: a check that asks "is anything at the name" instead of "is it MY file". Against a FRESH key those are indistinguishable, because after the revert the canonical name holds nothing -- so there is a third test on a key that ALREADY holds a transcript, which is the case the issue asked to be driven and the one where the lie is worst. The A, B, A is driven by injected fs calls rather than by a sleep, because the window is two calls wide and a timing test cannot hit that ordering: the issue's own 45 second unsynchronised race did not hit it once in 16,591 promotions. Getting the fixture wrong is instructive enough to be written on the test. The swap has to go in before the COPY, not before the rename: the `.incoming` temp is created inside the key directory, so a swap after it exists carries the temp away with the real directory and the rename fails with ENOENT -- a refusal, but the wrong one, and not the lie under test. A second test pins that an ordinary promotion still passes, without which a check that refused everything would look identical. `INT-SESSION-STORE-CONTRACT` amended, and the sentence that said this edge "has no answer to it" now names the answer. `DES-SESSION-KEY-IS-DERIVED-NOT-INDEXED` UNCHANGED, checked: the key is still a derived hash, which is what makes the path precomputable and therefore what this defends. **Precondition unchanged**: write access to `PI_SESSIONS_DIR`, which is what the whole store already concedes. No version moves. Closes #390 Signed-off-by: Rob Boerman <robboerman@live.nl>
edgehero
force-pushed
the
fix/promote-aba-390
branch
from
September 23, 2026 21:30
9d20a4f to
1ea1766
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The read path answers a key directory swapped under a running job with two checks that between them cover
every ordering: it stages the transcript from one DESCRIPTOR, so bytes cannot come from a file that replaced
the name afterwards, and it re-checks the name by path after the copy, so a swap that happened before the
open is caught too.
The write path has neither. Node exposes no
openat/renameat, sopromoteSessionre-checks the keydirectory's
dev:inounder the lock and again after the rename. Both catch a swap that is STILL STANDING.Neither can see one that is put back:
Measured here with a deterministic probe:
{"promoted":true,"reason":"promoted","bytes":60}, the transcriptin the attacker's directory, and the real key holding only
pi-version,resume-chainandvenue. Thenext job on that key cold-starts as
absentwhile the record says the work was promoted, which is thesilent no-op
CLAUDE.mdcalls the worst outcome available.What the shape comparison cannot see, the inode can
renamePRESERVES the inode. So the transcript's identity is read from the temp before the rename andcompared against what the canonical name holds after it. If the rename landed where it was meant to, that is
the same file. If the directory was swapped for the rename and swapped back afterwards, the canonical name
resolves into the real key again and holds its OLD transcript, or nothing -- a different inode either way,
and a fact that is still true once the shape is right again.
Detection, not prevention, like the check beside it: the bytes are already wherever the name pointed and
nothing host-side can recall them. What changes is that the record stops claiming otherwise. The new token
is
transcript-diverted, a closed-enum addition on the promote path, becausekey-not-a-directorysays thename is the wrong SHAPE and by this point it is not.
The fail-closed arm is deliberate and pinned: if BOTH identity reads fail, a bare comparison is
null !== null, which is false, so the promotion would report success having verified nothing. Two failed reads arethe one case where "they match" and "I could not tell" look alike.
The sidecar writes after the rename deliberately get no equivalent check. The transcript's own identity
is what decides whether the promotion landed; a sidecar diverted by the same swap is covered by the same
refusal, and a second identity dance per sidecar would buy nothing the first does not already say.
The residual this leaves, which a review pass found and the contract now states
A swap put back FOR THE DURATION OF A CHECK and re-applied afterwards defeats any check that resolves the
name again. An A, B, A, B around the two post-rename checks still reports
promoted: truewith thetranscript outside the store -- and on a key that already held one, the next job RESUMES the superseded
transcript rather than cold-starting, so the lost turn is invisible on both edges. Every path-based check
has this shape; closing it needs
openat/renameat, not a third check. A 45 second unsynchronised liverace over 215,372 promotions against 105,888 swap cycles hit it 0 times, so it is a deterministic-window
finding of exactly the class this issue was.
INT-SESSION-STORE-CONTRACTsays so, and its residual countmoves from two to three.
One assumption is added and named, because it is the first in that file to rest on it: that
renamePRESERVES the inode. It does on every filesystem this project supports. On one that derives inode numbers
from the path (SMB/CIFS without
serverino, some FUSE) every promotion would refuse with a token that readsas an attack, which is the same class as the
O_EXCLsemantics the lock already assumes.Tests
Six mutations, all red, and the sixth is the one a review pass found surviving: a check that asks "is
anything at the name" instead of "is it MY file". Against a FRESH key those are indistinguishable, because
after the revert the canonical name holds nothing -- so there is a third test on a key that ALREADY holds a
transcript, which is the case the issue asked to be driven and the one where the lie is worst. The A, B, A is driven by injected fs calls rather than by a sleep, because the
window is two calls wide and a timing test cannot hit that ordering: the issue's own 45 second unsynchronised
race did not hit it once in 16,591 promotions.
Getting the fixture wrong is instructive enough to be written on the test. The swap has to go in before the
COPY, not before the rename: the
.incomingtemp is created inside the key directory, so a swap after itexists carries the temp away with the real directory and the rename fails with ENOENT -- a refusal, but the
wrong one, and not the lie under test. A second test pins that an ordinary promotion still passes, without
which a check that refused everything would look identical.
INT-SESSION-STORE-CONTRACTamended, and the sentence that said this edge "has no answer to it" now namesthe answer.
DES-SESSION-KEY-IS-DERIVED-NOT-INDEXEDUNCHANGED, checked: the key is still a derived hash,which is what makes the path precomputable and therefore what this defends.
Precondition unchanged: write access to
PI_SESSIONS_DIR, which is what the whole store alreadyconcedes.
No version moves.
Closes #390