diff --git a/llp/0104-hyp-purge.decision.md b/llp/0104-hyp-purge.decision.md index 4bd6368d..727b9ea6 100644 --- a/llp/0104-hyp-purge.decision.md +++ b/llp/0104-hyp-purge.decision.md @@ -117,15 +117,39 @@ one directory; comparing the pair directly needs no inference and no assumption that the probed volume is the volume both spellings live on. **A retention that cannot be proven is reported, never silent.** When the fold -proposes an alias and the filesystem refuses it (the usual reasons: this really -is a case-sensitive volume, or the aliased directory no longer exists), the rows -stay, which is correct, and `PurgeSummary` carries `retainedAliasRows` / -`retainedAliasCwds` so the CLI can name them on stderr. That closes the half of -the defect the counts alone do not: the original complaint was not only that -rows survived but that `purged 0 rows` with empty stderr is byte-identical to -"that directory had nothing cached". Note the inversion this repairs - a purge -that *deletes* prints the resurrection warning, so before this the failing case -was the quieter of the two. +proposes an alias and the filesystem does not confirm it, the rows stay, which is +correct, and `PurgeSummary` carries `retainedAliasRows` / `retainedAliasCwds` so +the CLI can name them on stderr. That closes the half of the defect the counts +alone do not: the original complaint was not only that rows survived but that +`purged 0 rows` with empty stderr is byte-identical to "that directory had +nothing cached". Note the inversion this repairs - a purge that *deletes* prints +the resurrection warning, so before this the failing case was the quieter of the +two. + +**The report enumerates three causes, because there are three.** A note that +under-enumerates is making the same unearned claim it was written to avoid, so +this is a constraint on the wording and not a stylistic preference. Unproven +covers: + +1. **Two directories.** This really is a case-sensitive (or normalization- + sensitive) volume and the two spellings have two inodes. Only this one is the + filesystem adjudicating "different". +2. **Not on disk.** The aliased spelling no longer exists, so the `stat` landed + on nothing. The ordinary case: the user is purging a project directory they + already deleted. +3. **Not checkable.** The `stat` could not be taken at all. + `sameDirectoryOnDisk` answers `false` for **every** error, not only `ENOENT`, + so an `EACCES` on an ancestor, an `ELOOP` on a self-referential symlink and + an `ENOTDIR` all arrive here. The spelling may well be present and simply + unreadable. + +That collapse in `sameDirectoryOnDisk` is deliberate and stays: an unprovable +alias must not be widened onto whatever the errno was, or the `dev`/`ino` proof +above stops being a proof. It bounds only what the *message* may assert. So the +stderr note says "genuinely different, no longer on disk, or could not be +checked" and asserts none of the three; the concrete errno stays a diagnostic on +the `usage_policy.alias_probe_skipped` log line, since surfacing it per row would +mean threading a cause through the deletion predicate to improve a sentence. **Deliberately unchanged.** Only the deletion predicate opts in. `policy unset`, `policy show` and `hyp ignore --check` share `scopeGoverns` and keep their @@ -150,5 +174,15 @@ already classified each row through the folded gate. target issues no extra syscall. `purgeCache` memoizes the verdict per `cwd` for the run. - The residue is one-directional and safe: an aliased directory that has since - been deleted cannot be proven, so its rows are retained. They are reported, so - the user can name that spelling directly. + been deleted, or that cannot be `stat`ed for any other reason, cannot be + proven, so its rows are retained. They are reported, so the user can name that + spelling directly. +- A row whose `cwd` is *lexically* inside the target while a symlink inside the + target resolves it elsewhere is deleted. That is not this section's fold: it + is the plain `canonicalSpellings` / `matchDepth` match, and it predates + `proveAliases` (`scopeGoverns` returns `true` for the same fixture with the + opt-in off). It is also what keeps the first consequence above true, since + purge has to be able to clear exactly what the gate governs. Revisiting it is + a change to + [LLP 0050 §canonicalization](./0050-ignore-enforced-in-adapters.decision.md#canonicalization)'s + set-of-spellings semantics, not to this decision. diff --git a/src/core/cache/purge.js b/src/core/cache/purge.js index e070b350..6b796f38 100644 --- a/src/core/cache/purge.js +++ b/src/core/cache/purge.js @@ -92,8 +92,9 @@ export async function purgeCache({ cacheRoot, target, deps }) { * @param {{ rows: number, cwds: Set }} retainedAliases sink for the * `subtree` near-misses: rows whose `cwd` is spelled as if it were inside the * target without this filesystem confirming the two spellings are one - * directory, whether because they are two directories with two inodes or - * because a spelling could not be `stat`ed at all + * directory, whether because they are two directories with two inodes, + * because a spelling is no longer on disk, or because the `stat` could not be + * taken at all (any errno, not only `ENOENT`) * @param {{ realpathSync?: (p: string) => string, statSync?: (p: string) => { dev: number, ino: number } }} [deps] * @returns {{ predicate: (row: Record) => boolean, columns: string[] }} */ diff --git a/src/core/cache/types.d.ts b/src/core/cache/types.d.ts index d13d88f1..7e7d088f 100644 --- a/src/core/cache/types.d.ts +++ b/src/core/cache/types.d.ts @@ -44,11 +44,15 @@ export type PurgeTarget = // of it) which this filesystem does *not* report as the target directory, so // they were correctly left in place. Purge widens onto a respelling only when // `dev`/`ino` proves the two spellings are one directory; when it cannot - -// because they really are two directories, or because the respelling is no -// longer on disk to be `stat`ed at all - retaining is the safe direction, and -// saying so is what keeps the run distinguishable from "that directory had -// nothing cached" (LLP 0104 #spellings). A caller rendering these must not -// claim the filesystem adjudicated: only the first reason is a verdict. +// because they really are two directories, because the respelling is no longer +// on disk to be `stat`ed at all, or because the `stat` could not be taken (an +// `EACCES` on an ancestor, an `ELOOP`, an `ENOTDIR`: `sameDirectoryOnDisk` +// answers `false` for every error, not only `ENOENT`) - retaining is the safe +// direction, and saying so is what keeps the run distinguishable from "that +// directory had nothing cached" (LLP 0104 #spellings). A caller rendering these +// must not claim the filesystem adjudicated, and must not claim the spelling is +// absent either: only the first reason is a verdict, and only the second is a +// statement that the directory is gone. export interface PurgeSummary { rowsDeleted: number partitionsAffected: number diff --git a/src/core/commands/purge.js b/src/core/commands/purge.js index b3e58940..02ac6dc4 100644 --- a/src/core/commands/purge.js +++ b/src/core/commands/purge.js @@ -137,14 +137,23 @@ export async function runPurge(argv, ctx) { // is byte-identical to "that directory had nothing cached", which is exactly // how the pre-fix silent retention read. // - // The wording claims only what was established. `aliased` covers two - // outcomes, and only one of them is the filesystem adjudicating: two live - // directories with two inodes, *or* a `stat` that never landed because the - // spelling is no longer on disk (routine, and the common case when the user - // purges a project directory they already deleted). Saying "this filesystem - // reports it is a different directory" would assert a verdict that an - // `ENOENT` did not produce, so the note states the retention and the two - // reasons instead. LLP 0104 #spellings names both. + // The wording claims only what was established, and the enumeration has to be + // exhaustive or it is making the same kind of claim it exists to avoid. + // `aliased` covers three outcomes and only the first is the filesystem + // adjudicating: two live directories with two inodes; a `stat` that landed on + // nothing because the spelling is no longer on disk (routine, and the common + // case when the user purges a project directory they already deleted); or a + // `stat` that could not be taken at all, because `sameDirectoryOnDisk` + // returns `false` for *any* error, so an `EACCES` on an ancestor, an `ELOOP` + // on a self-referential symlink and an `ENOTDIR` all land here too. Saying + // "this filesystem reports it is a different directory" would assert a + // verdict none of the last two produced; naming only the first two would + // assert that the spelling is absent when it may be present and merely + // unreadable. So the note states the retention and all three reasons. The + // real `errno` stays in the `usage_policy.alias_probe_skipped` debug log + // rather than on stderr: it is a diagnostic, and surfacing it would mean + // plumbing a per-row cause through the deletion predicate for a message-only + // gain. LLP 0104 #spellings names all three. if (retainedAliases.length > 0) { const rows = summary.retainedAliasRows const dirs = retainedAliases.length @@ -152,7 +161,7 @@ export async function runPurge(argv, ctx) { `note: ${rows} cached row${rows === 1 ? '' : 's'} under a similarly spelled ` + `director${dirs === 1 ? 'y' : 'ies'} ${rows === 1 ? 'was' : 'were'} left in place - ` + `this filesystem does not report ${dirs === 1 ? 'it' : 'them'} as the directory you named ` + - `(genuinely different, or no longer on disk):\n` + `(genuinely different, no longer on disk, or could not be checked):\n` ) for (const dir of retainedAliases) ctx.stderr.write(` ${dir}\n`) ctx.stderr.write('tip: purge that exact spelling too if you meant it as well\n') diff --git a/src/core/usage-policy/fold.js b/src/core/usage-policy/fold.js index a79c2315..8cf46010 100644 --- a/src/core/usage-policy/fold.js +++ b/src/core/usage-policy/fold.js @@ -108,10 +108,21 @@ export function foldPath(p, { caseInsensitive = false } = {}) { * (LLP 0104 §spellings): in a deletion predicate a wrong widening destroys data, * so the fold may only ever *propose* a spelling and the filesystem decides. * - * Never throws. A spelling that cannot be `stat`ed (the usual reason: the - * directory no longer exists, which is routine for a recorded `cwd`) is not the - * same directory as anything, which is the conservative answer in both - * directions of a deletion. + * Never throws, and swallows the errno **deliberately**. A spelling that cannot + * be `stat`ed is not the same directory as anything, which is the conservative + * answer in both directions of a deletion, and that is true whatever stopped + * the `stat`: `ENOENT` (the usual reason, the directory no longer exists, which + * is routine for a recorded `cwd`) is not privileged over `EACCES` on an + * ancestor, `ELOOP`, or `ENOTDIR`. Widening on a failed probe to "assume same" + * would delete rows under a directory nobody identified with the target, so + * every error has to collapse to the same `false`. + * + * What the collapse costs is **precision of explanation, not of decision**: a + * caller only ever learns "unproven", so a caller that renders the retention to + * a user must not narrate it as "no longer on disk" (LLP 0104 #spellings names + * all three causes; `src/core/commands/purge.js` renders them). The errno that + * was actually seen is not discarded, only kept out of the return value: it + * goes to the `usage_policy.alias_probe_skipped` log line below. * * `statSync` follows symlinks, so this also answers `true` for a symlink * spelling; that overlaps `canonicalSpellings` and costs nothing. diff --git a/src/core/usage-policy/matcher.js b/src/core/usage-policy/matcher.js index 9f659814..e5c70491 100644 --- a/src/core/usage-policy/matcher.js +++ b/src/core/usage-policy/matcher.js @@ -665,9 +665,17 @@ export function scopeGoverns(cwd, dir, deps = {}) { * govern `cwd` (`governs`), is `cwd` unrelated to it (`outside`), or is `cwd` * spelled as if it were inside `dir` on a volume that folds spellings, without * *this* filesystem confirming the two spellings are one directory (`aliased`)? - * `aliased` is the absence of a proof, not a proof of difference: it covers - * both a live pair with two inodes and a spelling that could not be `stat`ed at - * all, so a caller rendering it must not report a verdict the second never gave. + * `aliased` is the absence of a proof, not a proof of difference. It covers + * three distinct situations and only the first is the filesystem adjudicating: + * a live pair with two inodes; a spelling that is no longer on disk, so the + * `stat` landed on nothing; and a spelling that could not be `stat`ed at all, + * because {@link sameDirectoryOnDisk} answers `false` for *every* error and not + * only for `ENOENT`, so an `EACCES` on an ancestor, an `ELOOP` or an `ENOTDIR` + * arrive here too. A caller rendering this verdict must therefore claim neither + * that the filesystem adjudicated (only the first gave a verdict) nor that the + * spelling is gone (only the second says so); `src/core/commands/purge.js` names + * all three, and LLP 0104 #spellings records why an enumeration that stops at + * two is making the same unearned claim the report exists to avoid. * * The third answer is the point, and it exists because `hyp purge` deletes. At * the gate, folding two spellings together is free: the resolved class is diff --git a/test/core/purge-command.test.js b/test/core/purge-command.test.js index ce147b39..3c77db76 100644 --- a/test/core/purge-command.test.js +++ b/test/core/purge-command.test.js @@ -710,13 +710,17 @@ test('scopeGovernance stays unwidened for callers that do not opt in', () => { }) // The near-miss note has to be true of *this* run, not only of the run the -// report was designed around. `aliased` has two causes and only one of them is -// the filesystem adjudicating; the other is an `ENOENT`, which is the ordinary -// case when the user purges a project directory they already deleted. Both -// tests below use spellings that exist on no host, so neither depends on -// whether the test volume folds. +// report was designed around. `aliased` has three causes and only the first is +// the filesystem adjudicating: two live directories with two inodes, a spelling +// no longer on disk (the ordinary case, when the user purges a project +// directory they already deleted), and a `stat` that could not be taken at all, +// because `sameDirectoryOnDisk` answers `false` for every errno rather than for +// `ENOENT` alone. The tests below use spellings that exist on no host, or exist +// but cannot be read, so none of them asks the volume for a folding verdict. The +// last one is the single fixture in this file that has to *build* both +// spellings, so it alone needs them to be two entries, and skips where they are +// not. // @ref LLP 0104#spellings [tests]: the retention note claims only what the run established - test('the near-miss note does not claim a verdict an ENOENT never gave', async () => { const cacheRoot = await makeTmpDir('cli-alias-gone') const hypHome = await makeTmpDir('cli-alias-gone-home') @@ -766,3 +770,98 @@ test('the near-miss note agrees in number with the rows it is counting', async ( await fs.rm(projects, { recursive: true, force: true }) } }) + +// The third cause, and the one the note used to misdescribe (#497 finding 1). +// `sameDirectoryOnDisk` answers `false` for *any* `stat` error, so an alias that +// is present on disk but unreadable retains exactly like one that is absent, and +// the run has no way to tell them apart. That collapse is correct and stays: the +// alternative is widening a deletion onto an unproven pair. What it constrains +// is the sentence, which used to offer "genuinely different, or no longer on +// disk" as an exhaustive pair when neither holds here. +// +// The fixture uses a self-referential symlink for `ELOOP` rather than a +// chmod-ed ancestor for `EACCES`: it needs no injection, needs the real +// predicate rather than a stubbed one, and unlike a permission bit it still +// fails the `stat` when the suite runs as root. +// +// Unlike every other fixture here it creates *both* spellings, which is the one +// thing a normalization-insensitive volume will not allow: on APFS or HFS+ the +// NFD spelling already names the NFC directory just made, so the `symlink` below +// would land `EEXIST` instead of building the loop. That host cannot host this +// cause at all (a folded volume proves the alias and deletes), so it skips +// rather than erroring, and the `ENOENT` case above still covers the retention. +test('the near-miss note does not say "no longer on disk" of an alias that is on disk but unreadable', async (t) => { + const cacheRoot = await makeTmpDir('cli-alias-eloop') + const hypHome = await makeTmpDir('cli-alias-eloop-home') + const projects = await fs.mkdtemp(path.join(os.tmpdir(), 'hyp-purge-eloop-')) + try { + const root = await fs.realpath(projects) + await fs.mkdir(path.join(root, NFC)) + const folds = await fs.lstat(path.join(root, NFD)).then(() => true, () => false) + if (folds) { + t.skip('this volume folds the two spellings into one entry, so the alias cannot be made unreadable here') + return + } + // Present (`lstat` finds it), unreadable (`stat` gives `ELOOP`), and so + // neither "genuinely different" nor "no longer on disk". + await fs.symlink(path.join(root, NFD), path.join(root, NFD)) + assert.ok(await fs.lstat(path.join(root, NFD)), 'the alias spelling is on disk') + await assert.rejects(fs.stat(path.join(root, NFD)), { code: 'ELOOP' }) + + await seed(cacheRoot, [ + { session_id: 's1', cwd: path.join(root, NFD, 'sub'), part_id: 'm1#0', timestamp: '2026-07-01T00:00:00Z' }, + ]) + const { ctx, stdout, stderr } = makeCtx({ cacheRoot, hypHome }) + assert.equal(await runPurge([path.join(root, NFC), '--yes'], ctx), 0) + + // Retention is the safe direction and must not move: an unprovable alias is + // never deleted, whatever stopped the `stat`. + assert.match(stdout.text, /purged 0 rows /) + assert.deepEqual((await remainingRows(cacheRoot)).map((r) => r.part_id), ['m1#0']) + + assert.match(stderr.text, /1 cached row under a similarly spelled directory was left in place/) + assert.match( + stderr.text, + /\(genuinely different, no longer on disk, or could not be checked\)/, + 'the enumeration has to admit the cause this run actually hit' + ) + assert.doesNotMatch( + stderr.text, + /\(genuinely different, or no longer on disk\)/, + 'the alias is on disk and nothing adjudicated it different, so neither of the old two holds' + ) + } finally { + await fs.rm(cacheRoot, { recursive: true, force: true }) + await fs.rm(hypHome, { recursive: true, force: true }) + await fs.rm(projects, { recursive: true, force: true }) + } +}) + +// The other half of #497 finding 1, and the reason it is a message defect and +// not a data-loss one: pin that a swallowed error resolves to `aliased` and +// never to `governs`, for every errno anyone has proposed. A future attempt to +// "improve" the swallow by treating some errno as reachable would delete rows +// under a directory no filesystem ever identified with the target, and would +// fail here rather than in someone's cache. +// @ref LLP 0104#spellings [tests]: an unprovable alias is retained whatever the errno, so the fix to the wording cannot move a deletion +test('no stat errno makes an unprovable alias deletable', () => { + for (const code of ['ENOENT', 'EACCES', 'EPERM', 'ELOOP', 'ENOTDIR', 'EIO']) { + let calls = 0 + const statSync = () => { + calls++ + const err = new Error(code) + Object.assign(err, { code }) + throw err + } + assert.equal( + scopeGovernance(`/vol/${NFD}/sub`, `/vol/${NFC}`, { proveAliases: true, statSync }), + 'aliased', + `${code} must retain and report, never widen the deletion` + ) + // Without this the test could not fail: neither spelling exists on any + // host, so a build that stopped threading `statSync` down to + // `sameDirectoryOnDisk` would reach the same `aliased` through a real + // `ENOENT`, and the errno under test would never have been raised at all. + assert.ok(calls > 0, `the injected ${code} has to be the errno the predicate actually saw`) + } +})