fix(system): bind the session-Trash delete to descriptors, not names - #7011
Conversation
github-actions
Bot
added
the
readiness: checking
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS A single new kept-reason string that maps the backend The message "One batch changed on disk after you selected it, so it was kept." reads clean cold, states what happened without leaking the internal code, renders in [UX-REVIEWED] 4d3e224 |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound descriptor-pinned design; the risk is what rides on the follow-up — two sibling paths keep the exact data-loss exposure this PR exists to close. Watch
[DESIGN-REVIEWED] 4d3e224 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of Confirmed: First-Principles-Verdict: CONCERNS A cause-level fix to an irreversible delete where every guard closes a named data-loss window — but it point-fixes the empty path and leaves two confirmed same-cause siblings still deleting by re-resolved path. What this change shipsIntent: make emptying the session Trash destroy exactly the batch the user approved and never escape the trash, instead of
WatchPoint patch with 2 counted, same-cause siblings ( [FIRST-PRINCIPLES-REVIEWED] 4d3e224 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBased on my independent examination of the production code changes ( No findings. [OPUS-REVIEWED] 4d3e224 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
e03450e to
11d3b58
Compare
github-actions
Bot
added
readiness: checking
|
All three GPT findings were real. Fixed in 1. Mutable descendant names can redirect deletion -- correct, and my first attempt at it was wrong in an instructive way. I first tried verifying each component against a So the delete now takes one pinned traversal of the batch before it removes anything ( Mutation-verified: with the inode comparison neutered, 2. The manifest can disappear before final removal -- correct, and the consequence is exactly as you state: Taken your suggested shape. The manifest is RENAMED to the trash root under New test 3. Identity verification is skipped on the coarse path -- correct, and dropping the check on the platform with the weakest removal is the wrong way round. The coarse path now compares the expected identity too, by Stated plainly in the code rather than implied, because it is weaker than the descriptor check by construction -- the stat and the rmtree are two lookups, so a swap between them still wins -- and it is vacuous on a filesystem that reports no usable inode, since the snapshot then recorded the same unusable value. Gates re-run on the new head: 238 passed in Screenshot Evidence is still red and I am producing the capture; that gate wants a rendered image of the new refusal row, which is the one thing a diff cannot show. |
github-actions
Bot
added
readiness: action required
11d3b58 to
137a64d
Compare
github-actions
Bot
added
readiness: checking
137a64d to
30d8724
Compare
github-actions
Bot
added
readiness: checking
|
Both round-2 findings were real. Fixed in Interior replacements are trusted when the deletion-time scan records them -- yes, and this is the same mistake one level up from round 1. I moved the identity earlier when the finding said to, but not far enough: The map now comes from Two consequences worth stating rather than leaving to be discovered:
The new test needs no monkeypatching at all -- the swap simply happens between Worth recording: the crude version of this attack was already refused, just not by the identity check. A directory holding anything the manifest does not name is caught by the unlisted-file guard, so my first attempt at the test failed with Coarse deletion re-resolves the batch after checking its identity -- correct, and my round-1 fix was decorative for precisely that reason: The coarse path now renames the batch to I did not take the suggested fail-closed, and want to be explicit about why. The API path always supplies an identity, so "refuse when descriptor-safe deletion is unavailable" means emptying the Trash never works on Windows -- the staged bytes stay on disk forever with no way to reclaim them, on the platform whose users cannot switch. That is a worse outcome than a residual window an attacker has to guess an 8-hex-character name inside. The ordering is now pinned by a test that distinguishes it: the swap fires from inside the identity Gates on the new head: 241 passed across |
github-actions
Bot
added
readiness: action required
30d8724 to
16aeaa4
Compare
github-actions
Bot
removed
the
readiness: action required
|
GPT round 8 disposition, on The finding. The chain check admits only the directory the approval named, but Why this one is fixed rather than accepted like the leaf unlink. That residual is per FILE: closing it costs a rename and a re-stat for every file in a batch that reaches 55k sessions. Directories are a handful per session, so the same technique is affordable here -- and it is already in this file twice, for the manifest and for the batch directory. The cost argument that justifies the leaf residual simply does not transfer, so declining this one would have been borrowing an excuse rather than making one. On the evidence, including a correction. My first attempt at the test hooked every chain open, which fired during the FILE phase rather than the removal pass -- so it re-tested the sibling scenario an existing check already covers, and BOTH mutations passed while I was on the verge of reporting them as proof. Gating the swap to the removal pass fixed it. Now: disabling the identity re-check makes Gates on this head: 256 pass across Still outstanding, and not mine to decide. The one accepted residual is the POSIX leaf unlink and the coarse |
|
GPT round 9 disposition, on This is the same defect one level up. Last round I bound the interior directory removals to a re-pinned identity; the batch's own directory still went by name. I fixed the instances GPT named instead of generalising the class, which is why this took two rounds rather than one. It is also the version that did the most damage, not the least. The final scan proves the batch empty by DESCRIPTOR, and by the time the batch is removed the manifest has already been moved aside -- so a swap in that interval removed an empty replacement and left the real batch holding data with nothing to list it, while
Evidence. One existing test needed updating, and I want to be explicit that I changed a test rather than only code. 257 tests pass across Tally: twelve findings, eleven fixed, one accepted residual -- the POSIX leaf unlink, whose override was recorded against a pre-rebase head and which I have not re-applied, since that sign-off was for that finding on that head. |
|
Not a review response -- something I found by auditing, on Having been told twice in a row that a removal addressed a name, I stopped fixing named instances and swept every The comment there argued "removing a link destroys nothing, because the thing it points at is untouched". That is true of the link the scan SAW. It is not true of whatever holds that name when the pass runs: a regular file moved onto a recorded link's name is data, and unlinking it is exactly the loss the file pass's identity check exists to prevent. So the reasoning was sound about links and silent about substitution, which is how it survived nine review rounds.
The test needed a correction of the same kind I made last round, which is worth recording because I keep making it. My first version swapped the file inside Audit result for the rest, so the sweep is on the record rather than just its one finding:
258 tests pass across |
|
GPT round 10 disposition, on The finding. My manifest refusal sat inside the descriptor branch, so the coarse path returned before reaching it. On a platform without descriptors The check now sits ABOVE the platform branch and above the manifest read, so nothing is deleted and no listing is even consumed on the strength of a link. It uses I checked whether the scan-based check is now redundant instead of assuming either way. It is not: the hoisted check is computed from a path and cannot see a link planted after it, while the pinned scan's view can. Each is pinned by its own test, and the mutations separate cleanly -- removing the hoisted check reds only the coarse test (148 bytes deleted), removing the scan check reds only the planted-after test. Neither covers for the other. One test was wrong twice before it was right, and the failure mode is worth naming. My first version planted the link inside 260 tests pass across Tally: fourteen findings, thirteen fixed, one accepted residual -- the POSIX leaf unlink, whose override was recorded against a pre-rebase head and which I have not re-applied. |
|
GPT round 11 disposition, on The finding. The manifest is moved aside so the batch can be removed and put back if that fails. The putting-back used The restore is now Evidence. On the mutation, because it nearly fooled me again. My first attempt patched the wrong line -- the sed matched a 261 tests pass across Tally: fifteen findings, fourteen fixed, one accepted residual -- the POSIX leaf unlink. Its override was recorded against the pre-rebase head and I have not re-applied it, since that sign-off was for that finding on that head. |
|
GPT round 12 disposition, on The finding. The post-condition proved "nothing left but the manifest" by NAME. Everything after it treats whatever answers to that name as the batch's own manifest -- renamed aside, then unlinked as debris once the batch is gone -- so a file substituted at that name after the first scan satisfied the check and was then destroyed. An unapproved file, whose only copy it was, deleted for matching a name. The survivor's inode must now equal the one the first scan recorded in Evidence. The pattern, which matters more than this finding. Six consecutive rounds have found the same mistake in a different place: a check that establishes an identity, followed by an action that addresses a name. The file pass, the interior directories, the batch directory, the link pass, the coarse path, the recovery, and now the post-condition. Each time I fixed the instance you named and each time the next instance was one function away. The class is now closed everywhere it can be: the only name-addressed action left is the leaf unlink, which is the residual #5430 accepted up front because POSIX has no unlink-by-inode. I would rather say plainly that this took six rounds than present it as a single clean fix. If a seventh instance exists I have not found it -- my own sweep of every 262 tests pass across Tally: sixteen findings, fifteen fixed, one accepted residual. That residual's override was recorded against the pre-rebase head and has not been re-applied, which is a maintainer decision rather than mine. |
|
Follow-up to the round-12 disposition, on I said the class was "closed everywhere it can be closed". That was one instance too optimistic, and re-reading my own sentence is what caught it: the post-condition now verifies the manifest's inode, but the rename that moves it aside addresses its NAME two syscalls later, and nothing afterwards asked whether what landed was the file that had been verified. The unlink that ends the successful path would then destroy a substitute. It cannot be checked before the fact -- POSIX has no rename-by-inode, which is the same wall the leaf unlink hits. What CAN be checked is the result, so the debris's inode is compared against the one the first scan recorded, and on a mismatch the file is LEFT as debris rather than removed, with both names and both inodes logged at ERROR. The real manifest was already replaced by then and that loss is not this code's to undo; it just does not have to add a second one.
Where the class now stands, having re-walked the module rather than assuming: every name-addressed action on this path either resolves through a descriptor chain that admits only an approved inode, or addresses a random name nothing can predict, or checks what landed afterwards. The one exception is the leaf unlink, which is the residual #5430 accepted up front. 263 tests pass across One self-inflicted detour worth recording because it cost a cycle: my first version of this fix put an Tally: seventeen findings -- fifteen from review, two from my own audits -- sixteen fixed, one accepted residual whose override remains a maintainer decision on this head. |
|
GPT round 13 disposition, on The finding. The fix is your wording exactly: both halves from one pinned descriptor. Evidence. 264 tests pass across One thing I did badly this round. Twice I made a spec edit whose old and new text differed only by a newline, which silently joined two words -- "batchesan", then "removaladdresses". I caught both by re-reading, but making the same careless mistake twice in one cycle is worth saying out loud rather than burying in a squashed commit. I switched to a script that asserts its anchor and greps for joined words afterwards, and the resulting spec diff is a pure insertion, which is what it should have been the first time. Tally: eighteen findings -- sixteen from review, two from my own audits -- seventeen fixed, one accepted residual. That residual is the POSIX leaf unlink, and its override remains a maintainer decision on this head. |
|
GPT round 14 disposition, on The finding. Round five of the original review moved the interior DIRECTORY map from delete time to approval time, precisely because a map built at delete time records the impostor along with everything else. The FILES never made that move:
I did weigh declining this one, and I want to show the reasoning rather than just the outcome. The memory objection behind the accepted leaf residual -- tens of MB on a 55k-session batch -- looks like it applies to storing an inode per file. But the approval ALREADY stores one per directory, and a batch of that size has directories in the same order as files. So this is a constant factor on something the PR shipped four rounds ago, not a new category of cost, and declining would have meant borrowing an argument that does not actually fit the case. That distinction is the whole reason the leaf unlink stays a residual while this does not. Evidence. 265 tests pass across Tally: nineteen findings -- seventeen from review, two from my own audits -- eighteen fixed, one accepted residual. Every one since the rebase has been the same shape: an identity established, then an action or a check that trusted a name. This is the last place in the module where an approval-time map existed but was not consulted; the leaf unlink remains the one name-addressed action with no fix, and its override is a maintainer decision on this head. |
|
GPT round 15 disposition, on Manifest contents not bound to approval -- fixed. Its inode was bound two rounds ago, but rewritten in place the manifest keeps that inode, every file identity still matches because no file changed, and what the rewrite alters is which files the delete believes it may unlink. Writing that test corrected me on my own code, which is worth reporting. I asserted that the approval refuses a batch holding an unlisted file -- it does not; that refusal lives at delete time. So the path is SHORTER than I described when I started: nothing upstream stands in the way, and the digest is the only thing between a rewritten listing and a deleted bystander. The wrong assertion is gone and the test now says where the refusal actually is. Unapprovable batches becoming silent successes -- fixed, narrower. You are right about the harm, and it is partly mine: last round I widened I did not make it raise on the unnamed sweep, and I want that departure on the record rather than buried. Raising there would let one batch damaged by a crash mid-append make the whole trash un-emptyable, and this module's delete loop deliberately skips rather than aborts for that reason ("one unreadable batch must not make the whole trash un-emptyable" is in the code). Named selection is different: the caller asked for that batch and it is not going to be deleted, so silence is a lie. If you would rather the sweep also refuse, that is a product call about whether a damaged batch should block the button, and I would rather you make it than have me pick. I also checked rather than assumed that the new raise surfaces properly: the dashboard already wraps this call in 267 tests pass across Tally: twenty-one findings, twenty fixed, one accepted residual. Eleven consecutive rounds have each found one more place where an identity was established and then a name, an inode, or now a listing was trusted instead. I am not arguing the findings are wrong -- they have all been real -- but that rate is itself information about this diff, and worth weighing against shipping the containment in stages. |
|
GPT round 16 disposition, on The finding. I computed the digest by PATH and AFTER the interior scan. Both halves are wrong. A manifest rewritten between the scan and the digest is recorded as the NEW listing against the OLD inode maps, which authorises exactly the file the digest exists to refuse; read by path it could describe a different directory's manifest entirely. So last round's fix contained a narrower instance of the hole it closed. It is now captured first, through On not adding a second parser. Evidence. 268 tests pass across Something I should say plainly. This is the second time in this PR that a fix of mine shipped the same class of mistake it was closing. The first was reasoning about POSIX Tally: twenty-two findings, twenty-one fixed, one accepted residual -- the POSIX leaf unlink, whose override is a maintainer decision on this head. |
|
GPT round 17 disposition, on Final-component symlinks followed during approval -- fixed as suggested. My first test for it was worthless, and the mutation is what told me. It pointed the name at a directory with NO manifest, which the approval refuses for that reason -- so it passed with the bug present, and I would have reported it as verified had I not printed the mutated line and re-run. Pointing the name at a SECOND real batch instead leaves the resolution as the only deciding factor: with Empty-all silently dropping unverifiable batches -- fixed, and I was wrong to frame it as a product call. I declined last round on the grounds that raising would let one crash-damaged batch make the whole trash un-emptyable. That reasoning still holds, but it was a false choice: raise-or-be-silent are not the only options. The batch now stays in the id list WITHOUT an approval, and That refusal is worth more than the reported bug. With the membership check disabled the unverified batch is DELETED -- 552 bytes freed in the test. So before this, an id reaching the worker without an approval was not merely mis-reported, it was deleted unchecked: 270 tests pass across On the other red this round, with evidence rather than a shrug. Frontend Tests shard 3 reported 421 files and 6698 tests passed, 0 failed, and then died with Tally: twenty-four findings, twenty-three fixed, one accepted residual. |
|
GPT round 18 disposition, on The finding is right about the shape. Renaming a refused object back onto the listed name writes to a name the refusal has just proved is not ours, and The severity is lower than stated, and I checked rather than assumed. For a DIRECTORY the described loss is not reachable: POSIX rename fails against a file ( So the fix stands on narrower ground than "prevents data loss": it stops us writing to a name we have just disowned. The wider ground is real elsewhere and you found it first -- the same courtesy applied to the manifest, which IS a file, and there it destroyed the only copy, which is why that path now uses Two of my own tests asserted the opposite contract -- that the intruder is renamed back and no staging debris remains -- and I have rewritten both. I want to be explicit that I changed tests rather than code to make them pass, and why that is right here: the contract they encoded was wrong. My reasoning for it at the time was "a refusal must not leave the directory under an unrecognisable name", which loses to yours. Reinstating the rollback now reds 270 tests pass across Also worth recording: I damaged the function mid-edit this round -- my replacement duplicated the stat block and truncated the branch above it -- and caught it by reading the region back rather than trusting the edit. mypy would have caught the syntax, but not a silently duplicated check. Tally: twenty-five findings, twenty-four fixed, one accepted residual. |
|
Disposition for both advisories on First Principles: "Description contradicts the diff" -- correct, and it was the description that was wrong. A Limits bullet still claimed an explicit First Principles subtraction: drop Design: the class is closed on one of three call sites. Accurate, and the follow-up is FILED rather than promised: issue #7113 covers Design: three inline spellings of rename-verify-remove, and I am not doing the extraction inside this PR, and the reason is not scope-protection. Shaping 270 tests pass across One note on the tree, since it looked alarming locally and might to a reviewer too: |
|
GPT round 19 disposition, on The finding. The approval binds identity, files, size and listing to one pinned directory -- and all of that can still describe the WRONG batch. A directory renamed into the selected name after Why this does not contradict "the directory is the batch's identity, not the manifest header". I checked that before implementing, because it looked like your fix asks me to trust attacker-controlled content. It does not: A test of mine got stronger rather than being bent to fit. 270 tests pass across Tally: twenty-six findings, twenty-five fixed, one accepted residual. On the previous head you reported no blocking findings with no |
|
GPT round 20 disposition, on The finding. My header comparison read It now demands equality. I checked the premise before tightening rather than after: Why the leniency was there, since it explains the mistake rather than excusing it. I copied the condition from I have deliberately left Evidence. 271 tests pass across Tally: twenty-seven findings, twenty-six fixed, one accepted residual. |
|
GPT round 21 disposition, on The finding. The test needed two attempts, and the first failure is worth recording because it is the same trap as before in a new disguise. I hooked That is now the fourth time in this PR a hook has landed outside the interval it was aiming at. The rule I keep re-deriving: find the two operations the guard sits between and hook the SECOND one, because hooking the first usually moves the whole window rather than splitting it. 272 tests pass across Tally: twenty-eight findings, twenty-seven fixed, one accepted residual. No override is posted; the leaf-unlink residual has not been re-raised since head |
|
Design disposition, on
Ran "Hold the sibling fix to the extraction" -- recorded as an instruction on #7113, comment 5474061837, in those words rather than as a suggestion. I also carried across the evidence you cited, because it is more persuasive than the principle: this PR went through several rounds of "my fix had the bug it was fixing" -- a rollback that renamed over a name it had just refused, a digest captured after the scan it was meant to precede, a header check that fail-opened on a missing field. Three of those were in guards I had just written. That is what per-call-site respelling looks like from the inside. The Windows coarse path -- I am not ratifying this myself, and that is the point. You are asking for the raceable Everything else on this head: GPT no blocking findings with no |
|
CI red on What happened. The test replaces a listed file with a different one of the same size and expects the identity check to refuse the batch. It did that by unlinking the original and writing a new file at the same path -- which frees the inode, and a filesystem is entirely free to hand the same number straight back to the next allocation. CI's did. The two inode maps then matched, the delete proceeded exactly as designed, and my test failed against correct code. Worth being clear about the direction of the error, because "CI-only failure" usually means the opposite: this was not a real defect that only manifests on CI, and it was not an infrastructure flake either. It was an assumption in my test -- that a fresh file at a reused path has a fresh inode -- that happens to hold on my filesystem and not on the runner's. The fix is deterministic rather than retried. The replacement is now written alongside the original and renamed over it with I audited the rest rather than waiting for CI to find them one at a time. Two sibling tests had the identical unlink-then-rewrite pattern and the identical latent flake -- I also re-ran the mutations after changing the setups, since a hardened test is a new test: disabling each guard still reds its own case and only its own. One of those reds arrived as a 325 tests pass across |
|
GPT round 22 disposition, on The finding. Why I did not take the suggested fix. "Revert the fd-safe delete hunk until hard-link support is verified" would remove the descriptor-bound delete -- the entire subject of this PR and of issue #5430 -- to close a failure mode in one recovery branch. It also trades a rare strand for the original data-loss TOCTOU, which is the worse of the two. So I treated it as a correct diagnosis with a remedy scoped wider than the defect. What I did instead. The failure is split by kind, because the two kinds mean different things:
That keeps the strong guarantee everywhere Evidence, two tests for the two halves. 327 tests pass across One process note: my edit inserted the two new tests in the middle of an existing one, stranding its final assertion inside mine. I caught it because the failure message named a test that was not the one failing, and put the assertion back. That is the second edit of mine this session to damage a neighbouring block -- worth saying rather than filing quietly. |
|
GPT rounds 22 and 23 dispositioned together, on Round 22 (on Round 23 (on Why I did not take round 23's remedy either. "Leave the debris and report incomplete" is exactly what round 22 called the loss. Taken literally the two rounds ask for opposite code, so the honest move is not to pick a side but to find the option neither names.
One consequence I had to handle rather than discover later: a copy can fail part way, and a manifest holding SOME of its entries is worse than none -- it lists some sessions and silently drops the rest, reading as a smaller batch rather than as damage. A failed copy now removes what it wrote and reports the failure, leaving the whole manifest recoverable as debris. Three tests, each mutation-checked separately. Dropping 328 tests pass across |
Emptying the Trash removed each batch with a path-resolving rmtree, so an ancestor directory swapped to a link between selection and removal was followed and the delete landed outside the trash. The worker also received only batch IDs, and an id is a name: a directory moved into an approved name was opened by that name and destroyed unapproved. The batch is now opened by walking from the filesystem root one component at a time with O_NOFOLLOW, each listed file is removed by (directory fd, name) from the manifest rather than by traversal, the emptied directories go bottom-up by descriptor including the batch through its parent's fd, and the (st_dev, st_ino) the snapshot saw under the mutation lock is re-checked with fstat on the opened descriptor - kept as identity_changed on mismatch. Where the platform has neither openat nor O_NOFOLLOW the coarse rmtree stays, with its measured byte figure and its post-condition check. Closes #5430
|
GPT round 24 disposition, on The finding. An explicit delete whose snapshot raises now returns 202 before Why it is mine, stated precisely, because my first reading was wrong. I initially checked whether the flagged branch was in my diff, saw only the Fix. The refusal is audited where it returns, outcome An irreversible operation that can be ATTEMPTED with no record of the attempt is a worse hole than a missing progress denominator, which is what the old code traded it for. One adjacent gap I did NOT fold in. The four sibling Note on my own pattern, since this is the sixth time. Each of these last few rounds found a defect inside the fix for the previous one -- rename-replaces three times, a fail-open threshold, a check-then-rename, and now a dropped audit. The common shape is that closing a hole moves control flow, and I check the new path for the property I was fixing while not re-checking the properties it used to satisfy on the way past. I have added that as a pre-push question rather than only noting it here. 329 tests pass across the three suites; flake8, isort, black and mypy clean. |
|
Disposition for the two advisory verdicts on 1. Two sibling paths still Acted on Design's specific ask that the follow-up's priority reflect what it carries: #7113 already had Why it stays out of this PR is unchanged and is the same reason Design gives: 2. The rename-verify-remove pattern is spelled three times inline. Agreed, and I want to be precise about what I am NOT claiming: this is not three coincidental similar blocks, it is one rule applied at three levels (interior dirs, batch dir, coarse path) and it should be one function. I did not extract it in this PR because the extraction's natural shape is decided by the sibling call sites in #7113 -- extracting against three call sites now and then reshaping it when two more arrive is worse than extracting once against five. Named on #7113 so the extraction is the unit of work there rather than an afterthought. 3. The Windows coarse path should be ratified by a human at merge, not inherited by default. Agreed, and deliberately not settled by me. The trade is stated plainly so it can be decided rather than discovered: the coarse path renames the batch to Everything else in both reviews is recorded as justified, including the two changes from the last rounds (named selection failing closed on an unreadable snapshot, and the refused empty now being SEL-audited). |
2 participants
What is the problem?
Emptying the session Trash removed each staged batch with
shutil.rmtree(batch, ignore_errors=True), which resolves the path it is given. The trash lives under the user's own data home, so every ancestor directory is writable by the same account - and by an auto-approved agent shell. An ancestor swapped to a symlink between the check that selected the batch and the removal that walks to it was followed, and the removal landed outside the trash.There was a second gap in the same path. The set handed to the background worker was a list of batch IDs, and an id is only a name. The mutation lock is released for the handoff, so a directory moved into an approved name was opened by that name and deleted - session data the user was never shown and never approved.
Why this issue matters to the user
Both failures destroy the only copy of session data. A staged batch is not a second copy of anything:
move_to_trashMOVES the files, so whatever is inside a batch exists nowhere else. A delete that leaves the trash, or that removes a batch the user did not approve, is unrecoverable.The window is not theoretically small either. #5195 made emptying a background job, so the gap between "the user approved this set" and "the bytes are gone" went from milliseconds to minutes on a large store - one staged batch can hold tens of thousands of sessions.
How our fix solves it
Symptom: bytes disappear from outside the trash, or a batch the user never saw disappears. Root cause: every step of the removal addressed a NAME that the kernel re-resolved at the moment of use, and consent was recorded against a name rather than against an object. So each step stops resolving a path:
O_NOFOLLOW. That flag constrains only the LAST component, which is why opening the batch by path left the whole prefix to be re-resolved. The path walked is the RESOLVED one, so an install whose data home legitimately sits behind a symlinked home directory is not refused.(directory fd, name), with the files named by the MANIFEST rather than discovered byos.walk, and the emptied directories removed bottom-up by descriptor - including the batch itself, through its parent's descriptor. Finishing withrmtree(batch)would re-resolve the whole prefix and undo the walk above it. Manifest-driven naming is independently necessary on Windows, where a junction is not a symlink andos.walkdescends into one.(st_dev, st_ino)travels with its id fromstaged_targets()into the worker and is re-checked withfstaton the descriptor that was opened -identity_changedif it differs, and the batch is kept. Checked on the descriptor rather than by a secondstatof the path, because the fd is the object every removal addresses: a swap after that point cannot reach the data.Two smaller consequences fell out of naming the files. Progress is now reported per file rather than per batch, which is what the reporting was for. And the manifest is removed LAST, only once the sweep confirms nothing else is left -
list_trash()omits a batch with no readable manifest, so removing it while a listed file survived left data on disk the user could neither see nor restore. An entry naming the manifest is refused for the same reason.Limits, stated rather than hidden:
openatnorO_NOFOLLOW, so it keeps the coarsermtreewith its measured byte figure and its post-condition check, and progress there stays per batch. A smoother bar is not worth a weaker delete.unlinkstill addresses a NAME, so a final-component swap remains possible: POSIX has no unlink-by-inode, and the stdlib's own_rmtree_safe_fdhas the identical property. What changed is that the exposure is now that one component, on a file the manifest lists, instead of the whole path prefix._discard_restored_batchandmove_to_trash's empty-batch cleanup still remove a trash directory by re-resolvedpath, so the ancestor-swap exposure remains on those two paths. This PR point-fixes
the empty, which is the one the issue is about and the one that runs minutes after the
user's click; the other two are a follow-up.
batch_idsselection as much as for "everything currently staged", and says so on the job. An earlier revision of this bullet said the explicit case ran without the identity check; that described a proceed-anyway branch this PR deletes, and First Principles review caught the description still promising it. The code is the stricter side, andtest_a_named_selection_fails_closed_when_the_snapshot_cannot_be_readpins it.What the user sees
A batch kept for this reason now says so, in the reader's language, and stays in the
Trash where it can still be restored:
Captured from an isolated pod running this branch. The empty-job status and the
session inventory are supplied by the harness so the refusal can be photographed
deterministically; the component, the styling and the locale bundle are the real
build. Before this change the same code path fell through to the generic
kept_unknownsentence.What tests we did
Nine cases in
test/test_session_storage.py, plus the existing coarse-path cases pinned to thermtreebranch. Six mutations were applied to the implementation and each turned the intended test red:O_NOFOLLOWdropped from the dir-open flags[]instead of['identity_changed'](dir_fd, name)rmdirreplaced byrmtreeGates:
pytest test/test_session_storage.py test/test_session_storage_api.pygives 245 passed. flake8, isort and mypy are clean on the touched modules; on thecheck_black_formatting.pyquestion, all four Python files are black-clean and none is in the baseline. Frontend:tsc --noEmitclean,vitest run SessionStorageScreen.test.tsx i18nAllLanguagesEntry.test.ts i18nGateTable.test.tsgives 87 passed, and eslint on the touched file shows only its three pre-existing a11y warnings. The full suite is left to CI.Round 2, after review. All three GPT blocking findings were legitimate and are fixed
rather than overridden, each with its own mutation-verified test:
O_NOFOLLOWrefuses a link, but a realdirectory RENAMED onto a staged directory's name is not a link, and pinning the batch
does not cover a rename that happens inside it. The delete now takes one pinned
traversal before it removes anything, recording every interior directory's inode, and
admits a component only as that inode on the batch's own device. Verifying against a
listing re-read at open time does NOT work - it reports the impostor - which the test
caught before review would have.
rmdir. It is now renamed to thetrash root under a debris name and renamed back if the removal does not complete, so
the batch is never unlistable with data still inside it.
lstatbefore
rmtree, with the residual window stated in the code.Round 3, after review. GPT held the line on two points and was right on both:
handoff was recorded as legitimate by the very scan meant to catch it. The map now comes
from
staged_targets()under the mutation lock, carried in aBatchIdentityalongside thebatch's own
(st_dev, st_ino), and the delete demands equality in both directions - adirectory added, removed or replaced since approval is a refusal. A concurrent restore that
removed a staged directory lands here too, and refusing is right: the approval no longer
describes the batch.
rmtree, whichre-resolves it. It now RENAMES the batch to
.<batch-id>.removing-<random>first, verifiesthe renamed directory, and removes it under that name; a swap before the rename is caught
and the impostor renamed back, and the name finally removed is unguessable. I did not take
the suggested fail-closed instead, because the API always supplies an identity, so failing
closed would make emptying the Trash impossible on Windows - a worse outcome than a window
an attacker must guess their way into. Flipping it to strict fail-closed is a one-line
change if the maintainer prefers that trade.
241 targeted tests pass on this head.
Round 4, after review. Two more, both real, and the first one's root cause was the
recursion I had flagged as acceptable stdlib parity:
nested staged tree made the recursive scan raise
RecursionError- notOSError, so itescaped every handler that turns a failed read into a refusal - and the request handler
answered a named selection by dispatching it unchecked. Both walks are now iterative
(one descriptor per level of the current path, so a deep tree fails with
EMFILE, whichIS handled), and the handler fails closed for a named selection exactly as it already
did for "everything staged": settled job, reason on it, no worker dispatched.
descended into whatever answered, so a directory swapped in after verification had its
links and empty directories removed. The recursive sweep is gone: directory removal is
driven by the approved map, deepest first, each reached through a descriptor chain that
admits only the approved inode - and the directory being removed is itself checked, since
rmdiraddresses a name and a top-level staged directory's parent is the batch. "Is itempty" is now a separate fresh scan rather than a side effect of the code deciding what
to delete.
Writing the second test found a third defect neither review caught: the descriptor cache
from the file phase satisfied the leaf check with the identity of the directory that was
there THEN, so the cache is now dropped before the removal phase and every directory is
re-opened and re-verified.
One behaviour change to call out for review: a named selection whose snapshot fails now
deletes nothing where it used to proceed. The test that encoded the old contract is
rewritten rather than deleted, with the reasoning in its docstring. 243 targeted tests pass.
Round 5, after review. The finding was the leaf
unlink, which is the residual this PR hasdeclared from the start: POSIX has no unlink-by-inode and the stdlib's own
_rmtree_safe_fdaddresses names too. Half of it turned out to be closable for free, andthat half is now closed - the pinned scan records each file's inode straight from the
directory block, and the unlink refuses a name that no longer denotes the object the scan
saw. What remains is the two syscalls between that check and the unlink, and a file
substituted BEFORE the scan under a name the manifest already lists.
Closing the rest needs per-FILE inodes carried from the approval snapshot and a
rename-verify-delete per leaf: roughly 50% more syscalls on a path whose whole point is a
batch of tens of thousands of files, plus tens of megabytes of map held for the job's
lifetime. That is a cost/benefit call on a limit the issue explicitly accepted, so it is
raised for a maintainer decision rather than decided here.
244 targeted tests pass on this head.
The Hindi string in this PR broke
src/i18n/style/hiStyle.test.ts: section 4 of the Hindistyle guide asks for the informal second-person pronoun rather than the formal one, and both
the original branch's line and my corrected one opened with the formal possessive. It uses
the informal form now, and all 11 locale style suites pass (83 tests). Worth naming because I
first mis-classified that shard as an infra flake on the strength of a log I could not fully
retrieve; it was mine all along.
Round 6, after review. One genuinely new finding and it was mine: the manifest's debris name
was deterministic while the coarse path's staging name was already random, and
os.renamereplaces its destination silently on POSIX - so a file planted at that name in the trash root
would have had its only copy destroyed without a word. It carries a random suffix now.
245 targeted tests pass on this head.
Round 7. First Principles flagged that the opening walk was a SECOND spelling of the
repository's pinned-filesystem module -- created after two closed PRs (#2446, #2447)
precisely so this mechanism would not be restated per call site. Fair hit, and adopted
rather than argued: the root walk is now that module's
pin_parent, the capability probe isits
supports_pinned_tree_walk()plus the three mutating dir_fd calls this path adds, andthe flags come from its
_dir_flags()-- called, not captured at import, so theWindows-simulation tests that delete
os.O_NOFOLLOWat runtime still see the truth. Theapproval-map machinery (
_scan_batch,_open_chain) is genuinely new and stays. 245targeted tests pass, and that module's own 61 tests still pass alongside them.
One subtraction declined with a reason: dropping the
expect=Nonedefault onempty_trash.The None branch is not test-only --
expect.get(batch_id)returns None for any batch thesnapshot dropped, which is the documented unreadable-at-snapshot case -- so removing the
default would not remove the branch, only make the library API harsher.
Every test that needs
openatskips explicitly where the platform lacks it, so the Windows shards exercise the coarse path rather than silently asserting nothing.A tenth finding, after the rebase
The rebase voided the recorded override, GPT re-reviewed the new head, and it found
something the earlier rounds had not: a symlink at the manifest's name was unlinked
before the batch finished.
It is a hole in this PR's own fix rather than a pre-existing one. The manifest is
excluded from the FILE removal pass precisely so it can go last -- it is the only thing
that keeps a batch restorable, and
list_trash()omits a batch without a readable one.But the scan classifies a symlink as a LINK, and the link pass unlinks every link it
recorded, with no manifest exemption. So a symlink at that name went early, and a batch
that then failed to finish -- one unwritable directory, one file held open -- left files
on disk the user could neither see nor restore. That is the exact outcome the
manifest-last rule exists to prevent, reached by the one path that skipped it.
The batch is now refused before anything is removed, reported as
unreadable_batch.Refusing beats deferring the symlink to the end alongside the real manifest: the product
writes that file with
atomic_write, so a symlink there was not written by us, and theentries the approval was computed from were read THROUGH it -- they may not describe this
batch at all. A batch kept costs the user a second attempt; a batch deleted on a
substituted listing costs them the data.
No new user-facing string:
unreadable_batchalready means "could not be read in full,so it was left alone", which is exactly what happened, and inventing a sixth reason
would have shipped 13 more locale entries for an adversarial edge case.
Mutation-verified: with the guard disabled the batch proceeds and frees 10 bytes instead
of refusing, which is the data-loss path itself, and
test_a_symlinked_manifest_is_refused_rather_than_unlinkedturns red. 254 tests passacross the two storage files, flake8 / isort / black / mypy clean. The spec records the
rule and names the test.
Tally after this: ten findings, nine fixed, one override -- the POSIX leaf-unlink and
coarse-rmtree residual that issue #5430 accepted up front. That override was recorded
against the pre-rebase head and does not carry to this one.
An eleventh and twelfth finding, same round
GPT raised a second new one on the next head: the directory removals still addressed a
name. The chain check admits only the directory the approval named, but
rmdiraddresses a name and so did that check, so an actor with write access to the parent could
swap the name between the two and have an unapproved directory removed on another one's
approval.
Fixed the way this PR already handles the manifest and the batch directory:
_remove_scanned_dirs()renames each directory to.<name>.removing-<random>in the sameparent, re-checks dev/ino there against the approved map, and removes THAT. A swap that
beats the rename gets the intruder moved within its own parent rather than deleted, is
then refused, and is renamed back -- so a refusal never leaves a directory under a name
the user cannot recognise.
Worth saying why this one is fixed rather than accepted like the leaf unlink. The leaf
residual is per FILE: closing it on the 55k-session batch this serves costs a rename and
a re-stat per file. Directories are a handful per session, so the same technique is
affordable here, and it is already in this file twice.
Two tests, and getting the evidence right took two attempts. My first version hooked
every chain open, which fired during the FILE phase instead -- so it re-tested the
sibling case an existing check already covers, and BOTH mutations passed while I was
claiming they proved something. Gating the swap to the removal pass fixed that.
test_a_directory_swapped_after_its_identity_check_is_not_removednow reds when there-check is disabled (the empty SUCCEEDS, which is the unapproved directory being
deleted), and
test_a_directory_is_removed_under_a_name_nothing_can_predictreds when thestaging rename is removed. Each mutation reds its own test and only its own.
256 tests pass across the two storage files; flake8, isort, black and mypy clean. The spec
records both halves and names both tests.
Tally: twenty-six findings (twenty-four from review, two from my own audits), twenty-five fixed, one accepted residual -- the POSIX leaf unlink, which
has no fix that is not a per-file cost on a six-figure batch.
The twelfth was the same defect one level up, at the batch's own directory, and I should
have generalised when I fixed the interior ones instead of waiting to be told twice. It is
also the version that did the most damage rather than the least: the final scan proves the
batch empty by DESCRIPTOR, and by the time it is removed the manifest has already been
moved aside -- so a swap in that interval removed an empty replacement and left the real
batch holding data with nothing to list it, while the caller reported SUCCESS. Silent, and
on the success path.
_remove_pinned_batch()now moves the name to.<batch id>.removing-<random>, checks itagainst
os.fstat(batch_fd), and removes only that. It raises on refusal rather thanreporting, which lets the existing recovery run unchanged: the manifest is renamed back
through the descriptor, so it lands in the REAL batch and that batch stays listed and
restorable.
test_a_batch_swapped_before_its_removal_is_refused_and_keeps_its_manifestpins it, withthe manifest move as the trigger so the swap lands exactly in the interval. Disabling the
identity check makes it fail by reporting success with no skip -- the defect itself, not a
proxy for it.
One existing test needed updating rather than fixing:
test_a_batch_whose_directory_will_not_go_reports_a_reasonforced the failure by matchingrmdiragainst the batch id, which the staging name no longer is. Its assertion isunchanged; only the way it provokes the failure moved, and it now recognises both names.
Left as it was it would have passed while quietly testing nothing.
257 tests pass across the two storage files; flake8, isort, black and mypy clean.
One more, found by auditing rather than by review
Having been told twice that a removal addressed a name, I stopped waiting and swept every
removal in the module. One more had the same shape, and it was the weakest of them: the
link pass unlinked every recorded link with NO identity check at all -- not even the
two-syscall one the leaf file has.
The reasoning in the code was "removing a link destroys nothing, because the thing it
points at is untouched". True of the link the scan SAW. Not true of whatever holds that
name when the pass runs: a regular file moved onto a recorded link's name is data, and
unlinking it is precisely the loss the file pass's identity check exists to prevent.
_scan_batch()now records each link's inode rather than just its path, and the passdemands
S_ISLNKplus the recorded dev/ino before unlinking. That closes thescan-to-unlink interval. The two syscalls between the check and the unlink stay open, and
that is the same POSIX residual the leaf file carries -- named, not implied.
Getting the test honest took a correction again. My first version swapped the file inside
os.stat, which put it between my own check and my own unlink -- the irreducible window,not the one the fix closes -- so it failed against a correct fix. Swapping right after the
scan tests the interval the check actually covers:
test_a_file_swapped_onto_a_scanned_link_is_not_unlinkedpasses with the check and redswithout it, with the planted file deleted.
258 tests pass across the two storage files; flake8, isort, black and mypy clean.
The Windows path had the same hole
GPT's next one: my manifest refusal lived inside the descriptor branch, so the coarse path
returned before reaching it. On a platform without descriptors,
rmtreewould remove alinked manifest and leave any staged file it could not delete -- a locked one -- so the
batch loses its listing and keeps its data. The same loss, reached by the branch that has
no descriptors to reason with.
The check is now ABOVE the platform branch and above the manifest read, so nothing is
deleted, and no listing is even consumed, on the strength of a link. It uses
platform_compat.is_link_or_junction()rather thanis_symlink(), because on Windows ajunction reports False for the latter and the coarse path IS the Windows path -- the module
already had that helper for exactly this reason.
The descriptor path still checks the same thing from its pinned scan, and I checked whether
that was now redundant rather than assuming: the path check cannot see a link planted after
it, the scan's view can, and each is pinned by its own test.
Three tests, and one of them was wrong twice before it was right. My first attempt planted
the link inside
_manifest_rels, which corrupted the CALLER's unlisted-files read instead:the refusal came from a guard one level up, the test passed for the wrong reason, and
mutating the check it claimed to cover changed nothing. Re-hooking to
_open_absolute_nofollow-- after the path check and after the caller's read, the onlyinterval left -- makes it red properly, with
incompleteinstead ofunreadable_batch.Mutation results are now cleanly separated: removing the hoisted check reds only the coarse
test (148 bytes deleted), removing the scan check reds only the planted-after test. Neither
covers for the other, which is the evidence that both earn their place.
260 tests pass across the two storage files; flake8, isort, black and mypy clean.
The recovery could overwrite data too
The manifest is moved aside so the batch can be removed, and put back if that fails. The
putting-back used
rename, and POSIX rename REPLACES its destination silently -- which isthe exact property the debris name three paragraphs up is randomised to be safe against. I
reasoned about that property in one direction and not the other. If anything writes a
manifest.jsonlinto the batch while ours is aside, the recovery destroyed the only copy ofa file this code has never read.
The restore is now
os.link, which fails withEEXISTinstead, and the debris is unlinkedonly after the batch has its manifest back, so no window has neither.
os.linkjoins the_FD_SAFE_DELETEcapability set: a platform that cannot do it now takes the coarse pathrather than reaching a recovery it cannot perform safely.
test_manifest_recovery_never_overwrites_a_manifest_that_arrived_sinceplants the arrivingfile in the window, which also makes the batch non-empty so the removal fails on its own and
the recovery runs for real. Reverting the restore to
renamereds it, with our manifest'sbytes where the arriving file's should be.
A note on how that mutation went, since it is the same trap as before: my first attempt
patched the wrong line and the test stayed green. A green mutation run is not evidence, it
is a signal that the mutation did not land -- so I now print the mutated line and read it
before believing any result. That check is what caught it.
261 tests pass across the two storage files; flake8, isort, black and mypy clean. While
editing the spec I clobbered a heading with a careless replacement and restored it in the
same pass; the section now has all five containment rules.
The post-condition trusted a name, and so did the step after it
The final scan proved "nothing left but the manifest" by NAME. Everything after it treats
whatever answers to that name as the batch's own manifest: it is renamed aside, and once the
batch is gone the debris is unlinked. So a file substituted at that name after the first
scan satisfied the post-condition and was then destroyed -- an unapproved file, whose only
copy it was, deleted for matching a name.
The survivor's inode must now equal the one the first scan recorded, and a mismatch reports
incompleteand leaves the file alone -- not even moved aside.test_a_file_substituted_at_the_manifests_name_is_not_destroyedswaps the file between thedirectory removal and the post-condition scan; reducing the comparison back to the name reds
it, and it reds by reporting SUCCESS, which is the defect rather than a proxy for it.
This one is worth naming as a pattern rather than an incident. Six rounds in a row have
found the same mistake in a different place: a check that establishes an identity, followed
by an action that addresses a name. I fixed the file pass, then the directories, then the
batch, then the links, then the coarse path, then the recovery, and each time I fixed the
instance rather than the class. The remaining name-addressed action is the leaf unlink, which
is the accepted residual -- POSIX has no unlink-by-inode -- so the class is now closed
everywhere it can be closed, but I would rather record that it took six rounds than imply it
took one.
262 tests pass across the two storage files; flake8, isort, black and mypy clean. The spec
now carries six containment rules, and I re-read the edited region this time.
Having named the pattern, I went looking for the next instance instead of waiting for it,
and found one: the rename that moves the manifest aside. The post-condition now verifies
that file's inode, but the rename addresses its NAME two syscalls later, and nothing
afterwards asked whether what landed was the file that had been verified -- so the unlink
that ends the successful path would have destroyed a substitute.
It cannot be checked before the fact, because POSIX has no rename-by-inode. What can be
checked is the result: the debris's inode is compared against the one the first scan
recorded, and on a mismatch it is LEFT as debris rather than removed, with both names and
both inodes logged at ERROR. The real manifest was already replaced by then, and that loss
is not this code's to undo -- but it does not have to add a second one.
test_a_file_swapped_after_the_post_condition_is_not_deleted_as_debrisswaps the filebetween the post-condition scan and the rename, and asserts the moved file survives with
its contents. Removing the landed check reds it by reporting success.
That is now every instance of the class this path contains. The only name-addressed action
left without a result check is the leaf unlink -- the residual #5430 accepted up front --
and I have re-walked the module to say that rather than assume it.
263 tests pass across the two storage files; flake8, isort, black and mypy clean. One
self-inflicted detour worth recording: my first version of this used
elifafter anexceptblock, which is not valid Python, and mypy caught it as a syntax error before anytest ran.
The approval bound a name too
The class turned out to reach one surface further than the delete path: the SNAPSHOT.
staged_targets()recorded the batch's identity by opening its path, and paired that withthe byte total
list_trash()had read from the same path earlier, under no lock. A swap inbetween pairs the REPLACEMENT's identity with the original's numbers -- and the delete, which
faithfully checks the identity it was handed, would then destroy session data the user was
never shown, having been asked to approve a different batch's size.
Both halves now come from one pinned descriptor (
_approve_batch()): the directory is openedO_NOFOLLOW,fstatgives the identity, the interior map comes from that descriptor, and themanifest is re-read THROUGH it rather than by path.
_manifest_records()already took an openhandle, so this needed an opener rather than a second parser.
_identify_batch()is gone -_approve_batch()supersedes it, and leaving both would have been two spellings of the samequestion.
test_the_approval_binds_identity_and_size_to_one_directoryswaps a DIFFERENT staged batchinto the selected one's name after the listing is read, then asserts that whatever is approved
has its identity and its size describing the same directory. Restoring the listing's byte
total reds it: 66 bytes from the vanished batch paired with the impostor's identity.
264 tests pass across the two storage files; flake8, isort, black and mypy clean.
Two process notes I would rather write down than leave in the commit history. I twice made the
same careless spec edit this round -- a replacement whose old and new text differed only by a
newline, which silently joined two words ("batchesan", "removaladdresses"). I caught both by
re-reading, then switched to a script that asserts its anchor and greps for joined words
afterwards. The resulting spec diff is a pure insertion, which is what it should have been the
first time.
The approval kept the directories and threw the files away
The last one in the family, and it had been hiding behind a fix from earlier in this PR.
Round five moved the interior DIRECTORY map from delete time to approval time, because a map
built at delete time records the impostor along with everything else. The FILES never made
that move:
_approve_batchscanned them and discarded them, so the per-file identity checkcompared each name against the delete's own scan -- self-consistent, and authorising nothing.
A listed file replaced during the handoff had its replacement's inode recorded, matched, and
was unlinked.
BatchIdentitynow carriesfilesandlinks, and the delete demands equality of the wholemap in both directions, exactly as it already did for
dirs. A file added, removed orreplaced since the approval is
identity_changedrather than something to reconcile, and aconcurrent restore lands there too -- the same answer the directories give, for the same
reason.
I weighed declining this one. The memory objection that justifies the accepted leaf residual
(tens of MB on a 55k-session batch) seems to apply to storing an inode per file -- but the
approval ALREADY stores one per directory, and a batch of that size has directories in the
same order as files. So the cost is a constant factor on something this PR shipped four rounds
ago, not a new category, and declining would have been borrowing an argument that does not
fit.
test_a_listed_file_replaced_after_approval_is_not_unlinkedreplaces a listed file with oneof the SAME SIZE after the approval, so only the identity distinguishes them. Removing the
comparison reds it by unlinking the replacement and reporting 598 bytes freed.
265 tests pass across the two storage files; flake8, isort, black and mypy clean. The spec
records it, and the spec diff is a pure insertion this time -- twelve added lines, nothing
removed.
The listing itself, and a silent success
Two findings this round.
The manifest's CONTENTS were not bound to the approval. Its inode was, as of two rounds
ago -- but rewritten in place the manifest keeps that inode, every file identity still
matches because no file changed, and what the rewrite alters is which files the delete
believes it may unlink. A file already sitting in the batch, unlisted, is refused at delete
time; add it to the listing after the approval and it is deleted as though the user had
approved it.
BatchIdentitynow carries a digest of the approved rels, and a mismatch isidentity_changed. A digest rather than the rels keeps the approval constant-size on a batchwith six figures of entries.
Writing the test corrected me on my own code: I asserted that the approval refuses a batch
holding an unlisted file, and it does not -- that refusal lives at delete time. Which makes
the path SHORTER than I described when I started, because nothing upstream stands in the way.
The assertion is gone and the test says where the refusal actually is.
A selected batch that cannot be approved was silently omitted, so the worker reported
success for a batch still on screen -- the same bug the missing-id refusal ten lines above
exists to prevent. Partly mine: last round I widened
_approve_batchto also return Nonewhen the manifest cannot be summarised, so I made the quiet path quieter.
It now raises for a NAMED selection, which is exactly the asymmetry that missing-id rule
already draws. I did NOT make it raise on the unnamed sweep, and that is a deliberate
departure from the suggested fix: one batch damaged by a crash mid-append would make the
whole trash un-emptyable, and the delete path skips rather than aborts for that reason. The
dashboard already answers a
SessionStorageErrorfrom this call as a 400 rather than a job,so the refusal surfaces as a refusal rather than a 500 -- checked, not assumed.
267 tests pass across the two storage files; flake8, isort, black and mypy clean. Each
mutation reds only its own test: dropping the digest deletes the newly-listed file, dropping
the raise restores the quiet success.
My own fix had the bug it was fixing
The digest I added last round was computed by PATH and AFTER the interior scan. Both halves
of that are wrong, and they reintroduce a narrower version of the hole the digest closes: a
manifest rewritten between the scan and the digest is recorded as the NEW listing against
the OLD inode maps, which authorises precisely the file the digest exists to refuse. Read by
path it could also describe a different directory's manifest entirely.
It is now captured first, through the pinned descriptor. Ordering it first is what makes it
fail closed: a rewrite after that point leaves the digest describing the old listing, so the
delete refuses.
_read_manifest()and_manifest_rels()grew the same optionaldir_fdthat_summarize_manifest()already had, so the pinned read has ONE spelling rather than asecond parser beside the first -- the divergence that an earlier round of this review
rightly objected to.
test_a_manifest_rewritten_during_the_approval_does_not_authorize_itrewrites the manifestin place inside the approval, keeping its inode. Moving the capture back after the scan reds
it, and reds it by deleting the smuggled file.
268 tests pass across the two storage files; flake8, isort, black and mypy clean.
Worth naming plainly: this is the second time a fix of mine in this PR shipped the same class
of mistake it was closing -- the first was reasoning about
renamereplacing its destinationin one direction and not the other. Both times the fix was right about the mechanism and
careless about ordering, which is a pattern in how I write these rather than an accident.
The approval followed a link, and the sweep still lied
Path.resolve()follows the final component. So a batch directory replaced by a symlinkresolved to its TARGET, and the pinned walk then pinned that target: the approval recorded
another directory's identity under this batch's id, and the delete, faithfully checking the
identity it was handed, would destroy session data from outside the trash. The approval now
resolves only the parent and re-joins the batch's own name, which keeps
O_NOFOLLOWon thecomponent that matters.
My first test for this was worthless and the mutation is what told me. It pointed the name at
a directory with no manifest, which the approval refuses for THAT reason -- so it passed with
the bug present. Pointing the name at a SECOND real batch, manifest and all, leaves the
resolution as the only thing that decides: with the fix the approval refuses, and with
resolve()restored it hands back the other batch's identity, files and digest under thefirst batch's id.
The unnamed sweep, which I declined to change last round. I argued that raising would let
one crash-damaged batch make the whole trash un-emptyable, and that stands. What I missed is
that those are not the only two options. The batch now stays in the id list WITHOUT an
approval, and
empty_trashrefuses an id that a supplied approval map does not name -- so itcomes back as a skip the user can read rather than vanishing from the job beneath a success
message. No signature change, no product decision deferred.
That refusal turned out to matter more than the reported bug: with the membership check
disabled the unverified batch is DELETED, 552 bytes freed. So the omission was not merely
mis-reported before -- an id reaching the worker without an approval was being deleted
unchecked, which is the outcome the whole map exists to prevent.
270 tests pass across the two storage files; flake8, isort, black and mypy clean.
One CI red this round was neither mine nor a real failure: Frontend Tests shard 3 reported
421 files and 6698 tests passed, 0 failed, then died with
[vitest-pool]: Worker exited unexpectedlyafter the suite finished. Frontend Coverage Merge is downstream of that shard.Nothing in this round touches the frontend, and the push has since started a fresh run.
A refusal no longer writes to the name it just refused
Third instance of the same blind spot of mine:
renamereplaces its destination, and Ireasoned about that for the manifest recovery two rounds ago and then wrote the directory
rollback with the same courtesy. Renaming a refused object back onto the listed name writes
to a name the refusal has just proved is NOT ours. Now only a matched identity is renamed
back -- when the object is ours and only the removal failed. A mismatch, or a re-check that
could not be read, leaves it under the unguessable staging name with both names logged.
Two of my own earlier tests asserted the opposite contract -- that the intruder is renamed
back and no staging debris remains -- and I have rewritten both, because that contract was
wrong rather than because they were inconvenient. The reasoning I gave for it then ("a
refusal must not leave the directory under an unrecognisable name") loses to this: not
writing to a name that is not ours beats leaving a tidy tree.
One correction to the finding's severity. It describes the rollback destroying a victim
placed at the listed name. For a DIRECTORY that is not reachable: POSIX rename fails against
a file (
ENOTDIR) and against a non-empty directory (ENOTEMPTY), so the most it can destroyis an empty directory. I wrote a test to demonstrate the data loss, watched it pass with the
bug deliberately reinstated, and deleted it rather than ship evidence that proves nothing --
the mutation passing IS the answer here, not a testing mistake. The fix stands on the narrower
ground, and the wider ground is real elsewhere: the same courtesy applied to the manifest,
which is a file, and there it did destroy the only copy, which is why that path uses
os.link.The behaviour is pinned by the two rewritten tests: reinstating the rollback on mismatch reds
test_a_directory_swapped_after_its_identity_check_is_not_removed, because the intruder endsup back under the listed name with no debris.
270 tests pass across the two storage files; flake8, isort, black and mypy clean.
Where the rest of the class lives, and who owns the extraction
Two advisories on this head, and both are about scope rather than correctness. Recording the
answers here because they are the shape of the follow-up, not loose ends.
The two remaining
shutil.rmtree-by-path siblings are_discard_restored_batchandmove_to_trash's empty-batch cleanup. The ancestor-swap exposure survives on both until thatlands, and the follow-up is FILED rather than promised in prose: issue #7113.
Design review's stronger point is that the rename-verify-remove pattern is now spelled three
times inline -- interior directories, the batch directory, the coarse path -- and that
pinned_fsexists precisely because per-call-site respellings of this mechanism failed twicebefore (#2446, #2447). That is the same objection an earlier round of this review made about
the link cell, one level up, and it is right: fixing the siblings by copying this machinery a
fourth time would repeat the history the module already has. The extraction belongs with that
work, where two more call sites will show which primitives are genuinely batch-agnostic, and
it is recorded on #7113 rather than left in this thread.
The selected NAME needed checking, not just the pinned directory
The approval binds identity, files, size and listing to one pinned directory -- and all of
that can still describe the wrong batch. A directory renamed into the selected name after
list_trash()brings its own manifest, so the approval is perfectly self-consistent andauthorises deleting a batch the user never selected. The name is the only link back to the
selection, and nothing was checking it.
This PR already has the rule: "the directory is the batch's identity, not the manifest
header", and
list_trash()withholds a batch whose header claims a different id. The rule wassimply enforced at LISTING time only, while the approval reads the manifest again afterwards --
and the swap lands between the two reads.
_header_names_this_batch()applies the same rule onthe second read.
It is worth being precise about why this does not contradict the rule it comes from. The header
is only ever COMPARED, never resolved in its favour: a disagreement withholds the batch, which
is exactly what the listing does. Trusting the header to decide WHAT to delete would be the
thing that rule forbids.
A test of mine got stronger rather than being rewritten to fit.
test_the_approval_binds_identity_and_size_to_one_directoryused to assert only that whatevercame back had its identity and size describing ONE directory -- the impostor's -- because
binding the pair was all the code could then promise. It now expects the swap to be refused
outright. Disabling the comparison reds it with DID NOT RAISE.
270 tests pass across the two storage files; flake8, isort, black and mypy clean.
Rebase, 2026-08-31
Rebased onto main at maintainer request (Raymond), which supersedes the push-forbidden
state the recorded override put this branch in. One conflict, in
test/test_session_storage.py: main and this branch had each added imports to the sameblock. Resolved as the union of both sides in isort order --
shutilandPurePosixPathare this branch's,timeandCallableare main's, and all four arestill used by the tests that introduced them.
Still exactly one commit. Targeted gates on the rebased head: 253 pass across
test_session_storage.pyandtest_session_storage_api.py(up from 245 -- main addedtests to these files while this branch was open), plus
test_subagent_scale.pyat 52pass because it is the only other file naming any of the changed APIs. flake8, isort,
black and mypy clean on the four touched Python files;
tscclean and 133 frontendtests pass across the storage screen and the locale style suites. The three eslint
warnings on
SessionStorageScreen.tsxare byte-identical on main's copy of the file atthe same line numbers, so they are pre-existing rather than introduced here.
Two things a reviewer should know. First, the human override recorded against
fcdd6f247does not carry to this head -- a rebase voids it by design -- so the GPTlane will re-raise the residual that override covered, and it needs re-applying on the
new head before this can go green. Second, the pre-push secret gate blocked the push
with 47 hits and I used its audited override: every hit was in main's OWN commit
messages, which the force-push swept into the scanned range because that path computes
remote..localwithout the--not --remotesguard its new-branch path uses. Runningthe same scanner over only the commits authored here returns clean, and main's history
is already on the remote, so nothing new was exposed. Naming it rather than leaving it
to the audit log.
Any other suggestions on the work
_clear_by_descriptorrecurses, matching the stdlib's_rmtree_safe_fd. The depth is the depth of the tree on disk and a batch this module writes is two levels deep, so this is parity rather than a new risk - but an iterative sweep would remove the question entirely if a future batch layout gets deeper.os.renameeach approved batch to a delete-staging name atomically under the lock at request time. It is strictly weaker than binding the delete to an fd (the staging name is still a name), so it is not a substitute, but it would shrink the window for any caller that cannot take the descriptor path.Closes #5430