From 62893f2bb10eea85abe9305c54e5513eccd45a9b Mon Sep 17 00:00:00 2001 From: Rupert-2634 Date: Sun, 18 Oct 2026 11:18:45 -0400 Subject: [PATCH] fix(workspace): disk reads check the mount root; two opens skip FIFOs openContainedFile, behind readFileFromDisk, readBinaryFromDisk and read_image, checked containment against the mount root's realpath taken during the read, so it checked that a file was inside whatever directory the root's path named then. A root renamed away and replaced by a link to another directory, or by another directory, was read as the mount. A root that was gone, or a file, was reported as a missing file, since the file was looked up before the root was resolved. The read now checks the root first (readRoot), through observe.ts's checkRoot, as a pass checks it. Once the workspace has recorded the root's identity, a root that is another directory is refused as mount_unavailable with no errno, at a new stage 'root', in ROOT_REPLACED's words, which ls and sync give it too. A gone root is mount_unavailable with realpath's ENOENT, and a file there with ENOTDIR, from a stat of the root (rootAbsence), since checkRoot gives only a reason. The read takes the root's identity from memory (rootIdentityForRead) in every state of the evidence's journal: the last identity whose append succeeded, since write() appends before it applies, which no reconcile can change, since root entries are written without a sync. So while the evidence awaits reopening after a failed append, when passes and tool calls fail, reads go on, checked; refusing them would stop every host read until a restart. Memory answers only once it is whole: a rebuild fills a new map and swaps it in at its end, and until the open's rebuild has finished, as when it couldn't read the journal back, the evidence refuses everything, reads included, as mount_unavailable with that reason and the root's errno where no directory is there. While the read runs, the checked root is asked again (stillHere): once the file is open and its checks pass, and, through one catch around the steps beneath the root (openBeneathRoot), before any refusal there is reported. A root that has moved gets the code and errno its state gets before the read (rootMoved); the bytes are then read through the checked file's descriptor. read_image passes a mount_unavailable refusal's message on. viewOf() builds a mount's view for a pass and for this check alike. readBinaryFromDisk checked maxBytes against the open's fstat, then read with handle.readFile(), which reads the file as it is by then, so a file grown past maxBytes came back whole. It now reads with readPrefix, as the other two readers do, and refuses such a file as "File too large", with a size from an fstat after the read, at least the bytes it found plus one. CheckedDir.sync() and ToolPresentation.read opened with 'r', so a FIFO put at a checked directory's path held the sync and the mount's turn, and a FIFO at the presentation file held the whole process, the read being synchronous. Both now open with O_RDONLY | O_NONBLOCK, and their existing checks refuse what isn't a directory or a regular file. Tests: a root that is a link elsewhere or another directory is refused by all three readers until a sync with acceptRoot; a root replaced at the open; a gone root and a file root, with their errno; each of removed, replaced by a file, by another directory, and by one holding the file, at three moments of the read; a mount configured through a link that is re-pointed to another directory at each of those moments; a mount with no recorded identity moved mid-read; a read while the evidence awaits reopening, of the mount's own file and through another directory; a sync whose acceptRoot append fails; an open that can't read the evidence back, or fails to record a changed mount; a rebuild whose append fails, reached directly, leaving memory as it was; readBinaryFromDisk's bound, its size after a shrink, and growth within maxBytes read whole; a FIFO at a checked directory's path during its sync; and a FIFO at the presentation file, read in a bounded child process. Co-Authored-By: Claude Opus 5.5 --- changelog.d/disk-read-root-and-fifos.fixed.md | 20 + src/modules/workspace/disk-agreement.ts | 51 ++- src/modules/workspace/index.ts | 214 +++++++-- src/modules/workspace/observe.ts | 16 +- src/tool-presentation.ts | 11 +- test/tool-presentation.test.ts | 21 + test/workspace-read-file-from-disk.test.ts | 417 +++++++++++++++++- test/workspace-three-way.test.ts | 62 ++- 8 files changed, 758 insertions(+), 54 deletions(-) create mode 100644 changelog.d/disk-read-root-and-fifos.fixed.md diff --git a/changelog.d/disk-read-root-and-fifos.fixed.md b/changelog.d/disk-read-root-and-fifos.fixed.md new file mode 100644 index 000000000..8c629c36a --- /dev/null +++ b/changelog.d/disk-read-root-and-fifos.fixed.md @@ -0,0 +1,20 @@ +- **The workspace's disk reads refuse a mount root that isn't the mount's + directory.** `readFileFromDisk`, `readBinaryFromDisk` and `read_image` read + whatever was at a mount's root path, so a root renamed away and replaced by + a link to another directory, or by another directory, was read as the + mount. Once the workspace has recorded the root's identity, with the first + evidence it gathers there, such a root is refused as `mount_unavailable`, + in the words a sync uses for it, until a sync with `acceptRoot` takes it. + A root that is gone, or is a file, is `mount_unavailable` too, with errno + ENOENT or ENOTDIR, where it was "File not found". A root that changes + during a read, until the file has been opened and checked, gets the same + code and errno as one changed before the read. +- **`readBinaryFromDisk`'s `maxBytes` bounds the read itself.** A file that + grows past it after the read's size check is refused as "File too large", + having loaded at most `maxBytes` bytes and one more, where it was read + whole. +- **Two opens no longer wait on a FIFO.** A directory's sync after a push + opened the directory blocking, so a FIFO put at its path held the sync + until something opened it for writing. The tool presentation file's read + did the same, synchronously, holding the whole process. Each now opens + without waiting, and refuses what isn't a directory or a regular file. diff --git a/src/modules/workspace/disk-agreement.ts b/src/modules/workspace/disk-agreement.ts index cd63fa899..c509d0c5c 100644 --- a/src/modules/workspace/disk-agreement.ts +++ b/src/modules/workspace/disk-agreement.ts @@ -181,6 +181,12 @@ export class DiskAgreement { * place owes none (recordWalk). */ private unsynced = false; + /** + * Whether memory holds a whole state: false until a rebuild first runs to + * its end. A rebuild fills a new map and swaps it in only at its end, so + * one that throws partway leaves whatever whole state came before it. + */ + private whole = false; /** Whether this host observes file modes (not on Windows): where it doesn't, no record it writes claims one. */ private readonly modesObserved: () => boolean; @@ -206,8 +212,10 @@ export class DiskAgreement { * recorded (the store may hold it where this process can't read it back), * and a decision from stale P can record what reopening contradicts, such * as a conflict that stays after the right evidence is back. So until the - * store is reopened, no evidence is read, written or checkpointed, and - * every pass and tool call that needs it fails. + * store is reopened, no evidence is read (but for rootIdentityForRead's + * one exception), written or checkpointed, and every pass and tool call + * that needs it fails. The same holds, the exception included, when the + * open's rebuild threw: there is no whole evidence to read. */ ensureReconciled(): void { if (this.journal.needsReopen) { @@ -216,6 +224,12 @@ export class DiskAgreement { 'an append to its journal failed inside the store, and the evidence may lack what it recorded', ); } + if (!this.whole) { + throw new Error( + 'Disk-agreement evidence is unavailable until the store is reopened, as at the next start: ' + + 'reading it back from the store failed', + ); + } if (this.journal.needsReconcile) this.rebuild(); } @@ -253,16 +267,24 @@ export class DiskAgreement { const mount = replayed.get(entry.mount); if (mount) apply(mount, entry); } - this.mounts.clear(); + // Filled apart and swapped in at the end, so an append that throws here + // leaves memory as it was, whole, rather than with the mounts after it + // missing. Nothing reaches that today: only an open with a moved mount + // appends here, the workspace opens each agreement once, and after the + // open a rebuild finds every mount's record and appends none. + const rebuilt = new Map(); for (const { name, root } of this.configured) { const found = replayed.get(name); if (found && sameRootPath(found.root, root)) { - this.mounts.set(name, found); + rebuilt.set(name, found); } else { - this.mounts.set(name, { root, paths: new Map(), walks: new Map() }); + rebuilt.set(name, { root, paths: new Map(), walks: new Map() }); this.journal.append({ t: 'mount', mount: name, root }); } } + this.mounts.clear(); + for (const [name, mount] of rebuilt) this.mounts.set(name, mount); + this.whole = true; } /** The identity of the root this mount's evidence was gathered under, once any was. */ @@ -271,6 +293,25 @@ export class DiskAgreement { return this.mounts.get(mount)?.identity; } + /** + * The identity a disk read checks the mount's root against: the one in + * memory once memory is whole, in every state of the journal, and an error + * until then. It is the last identity whose append succeeded, since write() + * appends before it applies, and no reconcile can change it, since root + * entries are written without a sync; were one made durable, its failed + * sync would leave the identity from before it: an old one, which refuses + * the new root, or none, as for a mount with no recorded identity. This + * is the one exception to ensureReconciled's "no evidence is read": the + * layer stops reading so that a decision from stale evidence can't record + * what reopening contradicts, and a read records nothing. Refusing reads + * instead would stop every host read of the workspace's disk until a + * restart. + */ + rootIdentityForRead(mount: string): RootIdentity | undefined { + if (!this.whole) this.ensureReconciled(); // throws: there's no whole state to read + return this.mounts.get(mount)?.identity; + } + /** * Record the root's identity just before the first evidence a mount gains: * the caller saw it as the root while gathering that evidence. A no-op once diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 420d258a4..d3a0f128d 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -40,7 +40,7 @@ import { MountWatcher, type FsChange } from './watcher.js'; import { hashContent, DEFAULT_MAX_FILE_SIZE } from './sync.js'; import { DiskAgreement, sameRootIdentity } from './disk-agreement.js'; import { BranchIntents } from './branch-intent.js'; -import { IGNORED, NONBLOCK, OTHER_MOUNT, checkDir, diskSpelling, filesystemTrustsCtime, inOtherMount, linkLeads, linkOnTheWay, linkRefusal, linkWords, locateRoot, nestedHolder, passedOverAt, readPrefix, type LinkFound } from './observe.js'; +import { IGNORED, NONBLOCK, OTHER_MOUNT, ROOT_REPLACED, ROOT_UNAVAILABLE, checkDir, checkRoot, diskSpelling, filesystemTrustsCtime, inOtherMount, linkLeads, linkOnTheWay, linkRefusal, linkWords, locateRoot, nestedHolder, passedOverAt, readPrefix, type CheckedDir, type LinkFound, type MountView } from './observe.js'; import { TOOL_BATCH_HOOK_TIMEOUT_MS, ensureStateRegistered } from '../../module-registry.js'; import { type MountRuntime, @@ -121,8 +121,11 @@ class WorkspaceImageReadError extends Error { /** * Why a workspace-owned filesystem read was refused. Distinguishes the cases a * peer needs to react to differently: a path that never resolved inside the - * mount (`unknown_mount`, `traversal`), a mount whose root is gone - * (`mount_unavailable`), an ordinary missing file (`not_found`), a directory + * mount (`unknown_mount`, `traversal`), a mount whose root is gone, isn't a + * directory, or isn't the directory the workspace was using + * (`mount_unavailable`: with errno ENOENT or ENOTDIR when no directory is + * there at all, with none for another directory), an ordinary missing file + * or one that isn't a regular file, such as a FIFO (`not_found`), a directory * (`directory`), a file that is itself a symlink, refused by the mount's * `followSymlinks` policy (`symlink`), a symlink among the path's directories * on a mount that doesn't follow symlinks, leading to a directory or a file @@ -133,7 +136,8 @@ class WorkspaceImageReadError extends Error { * (`changed`), and a path in another mount's directory nested in the one * named, or reached through a symlink among the path's directories leading * there (`other_mount`), whose message names the mount that holds it and the - * path to use there. + * path to use there. A `mount_unavailable` for a root whose realpath or stat + * fails otherwise carries that errno, such as EACCES. */ export type WorkspaceReadErrorCode = | 'unknown_mount' @@ -150,6 +154,8 @@ export type WorkspaceReadErrorCode = /** Where in the open sequence a `WorkspaceReadError` originated (diagnostics). */ export type WorkspaceReadStage = | 'parse' + /** The mount root's check: a directory, the one the workspace was using. */ + | 'root' | 'readlink' | 'lstat' | 'realpath_root' @@ -231,6 +237,20 @@ function errnoOf(err: unknown): string | undefined { : undefined; } +/** + * What is at a mount root's path now, followed as the root is: for a + * directory, undefined; for anything else, ENOTDIR; and where the path + * can't be followed to anything, the errno its stat failed with, ENOENT + * when nothing is there. + */ +async function rootAbsence(root: string): Promise { + try { + return (await stat(root)).isDirectory() ? undefined : 'ENOTDIR'; + } catch (err) { + return errnoOf(err); + } +} + /** read_image's refusal of a file over the mount's size limit, wherever it's found to be. */ function imageTooLarge(maxSize: number, mountPrefixedPath: string): WorkspaceImageReadError { return new WorkspaceImageReadError('too_large', `Image file exceeds max size (${maxSize} bytes): ${mountPrefixedPath}`); @@ -873,6 +893,25 @@ export class WorkspaceModule implements Module { return this.agreement; } + /** + * A mount as observation reads it, with the root identity the workspace + * recorded for it, if any: what a pass reads beneath (runtime), with + * rootIdentity's, and what a disk read checks its root against (readRoot), + * with rootIdentityForRead's. + */ + private viewOf(mount: MountState, rootIdentity: MountView['rootIdentity']): MountView { + return { + root: resolve(mount.config.path), + name: mount.config.name, + rootIdentity, + followSymlinks: mount.config.followSymlinks === true, + maxFileSize: mount.config.maxFileSize ?? DEFAULT_MAX_FILE_SIZE, + ignore: mount.config.ignore ?? [], + others: this.othersOf(mount.config.name), + nested: this.nestedOf(mount.config.name), + }; + } + /** The view reconciliation needs of a mount, refreshed per pass. */ private async runtime(mount: MountState): Promise { const root = resolve(mount.config.path); @@ -885,16 +924,7 @@ export class WorkspaceModule implements Module { } return { name: mount.config.name, - view: { - root, - name: mount.config.name, - rootIdentity: this.agreementOrThrow().rootIdentity(mount.config.name), - followSymlinks: mount.config.followSymlinks === true, - maxFileSize: mount.config.maxFileSize ?? DEFAULT_MAX_FILE_SIZE, - ignore: mount.config.ignore ?? [], - others: this.othersOf(mount.config.name), - nested: this.nestedOf(mount.config.name), - }, + view: this.viewOf(mount, this.agreementOrThrow().rootIdentity(mount.config.name)), rootReal, ctimeTrustedDev: trustedDev, treeStateId: mount.treeStateId, @@ -1810,6 +1840,11 @@ export class WorkspaceModule implements Module { * `followSymlinks` holds for the file and the directories on its way, and * what it reaches stays canonically inside the mount. Deliberately does NOT * sync, register, or hash anything — it reads and returns. + * + * `maxBytes` refuses a larger file as "File too large", having loaded at + * most `maxBytes` bytes and one more: the reads decide where the file ends, + * not the size fstat gave at the open, so the bound holds for a file that + * grows while it's read. */ async readBinaryFromDisk( mountPrefixedPath: string, @@ -1823,16 +1858,24 @@ export class WorkspaceModule implements Module { return { error: error instanceof Error ? error.message : String(error) }; } const absolutePath = resolve(mount.config.path, relativePath); + const maxBytes = opts.maxBytes; let tooLarge: string | undefined; try { - const { handle } = await this.openContainedFile(mount, relativePath, mountPrefixedPath, (fileStat) => { - if (opts.maxBytes !== undefined && fileStat.size > opts.maxBytes) { - tooLarge = `File too large: ${fileStat.size} > ${opts.maxBytes} bytes`; + const { handle, fileStat } = await this.openContainedFile(mount, relativePath, mountPrefixedPath, (fileStat) => { + if (maxBytes !== undefined && fileStat.size > maxBytes) { + tooLarge = `File too large: ${fileStat.size} > ${maxBytes} bytes`; throw new Error(tooLarge); } }); try { - return { data: await handle.readFile(), absolutePath }; + if (maxBytes === undefined) return { data: await handle.readFile(), absolutePath }; + // No more than maxBytes is loaded: a file grown past it since the + // fstat above is refused here, as one already over it was there. Its + // size is an fstat's after the read, and at least one more than the + // bytes kept, so the refusal's size is always past maxBytes. + const { bytes, more } = await readPrefix(handle, maxBytes, fileStat.size); + if (more) return { error: `File too large: ${Math.max((await handle.stat()).size, bytes.length + 1)} > ${maxBytes} bytes` }; + return { data: bytes, absolutePath }; } finally { await handle.close(); } @@ -2033,29 +2076,95 @@ export class WorkspaceModule implements Module { return this.validateImageBytes(blob, mountPrefixedPath, maxSize); } + /** + * The mount root a disk read reads beneath: resolved, then checked as + * observation checks it (checkRoot), so the read and the evidence agree on + * which directory the mount is. Once the workspace has recorded the root's + * identity, a root that resolves to any other directory is refused, in + * ROOT_REPLACED's words; with none recorded yet, it need only be a + * directory there. A refusal where no directory is at the root's path + * carries the errno that gives (rootAbsence: ENOENT when nothing is there + * and ENOTDIR when something else is, which a caller can read as no file, + * or the errno the root's stat failed with otherwise, such as EACCES). A + * root whose realpath fails carries realpath's errno, and one that is + * another directory carries none. The identity is rootIdentityForRead's, + * which answers while the evidence awaits reopening; where even it can't + * be read, as when the evidence was never read back whole, the read is + * refused too, with an errno only where no directory is at the root's path. + */ + private async readRoot(mount: MountState): Promise<{ real: string; dir: CheckedDir }> { + const name = mount.config.name; + const root = resolve(mount.config.path); + let real: string; + try { + real = await realpath(root); + } catch (err) { + throw new WorkspaceReadError('mount_unavailable', `Mount unavailable: ${name}`, 'realpath_root', errnoOf(err)); + } + let rootIdentity: MountView['rootIdentity']; + try { + rootIdentity = this.agreementOrThrow().rootIdentityForRead(name); + } catch (err) { + // The evidence can't be read back, so neither can the check be made. + // Where no directory is at the root's path, its errno still says so. + throw new WorkspaceReadError('mount_unavailable', + `Mount unavailable: ${name}: ${err instanceof Error ? err.message : String(err)}`, 'root', await rootAbsence(root)); + } + const checked = await checkRoot(this.viewOf(mount, rootIdentity), real); + if ('dir' in checked) return { real, dir: checked.dir }; + // Another directory is no absence. For any other refusal, what is at the + // root's path now decides: nothing, something that isn't a directory, or + // a directory the path has come to name since it was resolved (no errno). + const errno = checked.reason === ROOT_REPLACED ? undefined : await rootAbsence(root); + throw new WorkspaceReadError('mount_unavailable', `Mount unavailable: ${name}: ${checked.reason}`, 'root', errno); + } + + /** + * The refusal for a root that stopped being the directory a read checked, + * with the code and errno readRoot gives what is at its path now (its stage + * and words are this check's): gone, or not a directory, with ENOENT or + * ENOTDIR, so a caller reads a root gone mid-read as one gone before it; or + * another directory, with none. + */ + private async rootMoved(mount: MountState): Promise { + const errno = await rootAbsence(resolve(mount.config.path)); + const reason = errno === undefined ? ROOT_REPLACED : ROOT_UNAVAILABLE; + return new WorkspaceReadError('mount_unavailable', `Mount unavailable: ${mount.config.name}: ${reason}`, 'root', errno); + } + /** * Open a mount-relative file for reading with the mount boundary enforced on * the *filesystem*, not just the path string: * - * 0. On a mount that does not `followSymlinks`, refuse a path with a symlink + * 0. Check the root (readRoot) before anything beneath it: under a root + * that isn't the mount's directory, no path says anything about the + * mount's files, an absence included. + * 1. On a mount that does not `followSymlinks`, refuse a path with a symlink * among the directories on its way beneath the mount's root, wherever it * leads (linkOnTheWay), before anything resolves it: as `escape` for a * link leading outside the mount, as `other_mount` for one into a nested * mount's directory, and as `parent_symlink` for any other. - * 1. `lstat` the lexical path — refuse directories; refuse a final-component + * 2. `lstat` the lexical path — refuse directories; refuse a final-component * symlink when the mount does not `followSymlinks`; refuse anything else * that isn't a regular file, such as a FIFO, without opening it. - * 2. Open with `O_NOFOLLOW` where the platform has it (POSIX) so the refusal + * 3. Open with `O_NOFOLLOW` where the platform has it (POSIX) so the refusal * holds against a symlink swapped in between lstat and open; where it * doesn't (Windows), re-`lstat` after opening instead. `O_NONBLOCK`, so a * FIFO swapped in meanwhile doesn't wait for a writer. - * 3. `realpath` both the mount root and the opened path. On a mount that - * does not `followSymlinks`, refuse it again for a symlinked directory on - * its way, which catches one swapped in since step 0 (linkRefusal). Then - * require canonical containment — this is what catches an *intermediate* - * symlinked directory escaping the mount, and a followed symlink whose - * target lands outside, on every platform (junctions included). - * 4. Confirm the descriptor and the canonical path are the same inode. + * 4. `realpath` the opened path. On a mount that does not `followSymlinks`, + * refuse it again for a symlinked directory on its way, which catches one + * swapped in since step 1 (linkRefusal). Then require canonical + * containment in the root step 0 resolved — this is what catches an + * *intermediate* symlinked directory escaping the mount, and a followed + * symlink whose target lands outside, on every platform (junctions + * included). + * 5. Confirm the descriptor and the canonical path are the same inode, and + * the root still the directory step 0 checked. + * + * Whatever steps 1 to 5 refuse, a missing file included, is said of the + * mount's files only while the root is still that directory. If it isn't, + * the read is refused with the code and errno step 0 gives what is at the + * root's path then (rootMoved), as step 5's own check of the root does. * * `inspect` runs last, after the boundary's checks, so a caller's own check * of the file, such as a size limit, never answers for a file the boundary @@ -2066,6 +2175,25 @@ export class WorkspaceModule implements Module { relativePath: string, mountPrefixedPath: string, inspect?: (fileStat: Stats) => void, + ): Promise<{ handle: Awaited>; fileStat: Stats; realFilePath: string }> { + const root = await this.readRoot(mount); + try { + return await this.openBeneathRoot(mount, root, relativePath, mountPrefixedPath, inspect); + } catch (err) { + if (err instanceof WorkspaceReadError && err.code !== 'mount_unavailable' && !(await root.dir.stillHere())) { + throw await this.rootMoved(mount); + } + throw err; + } + } + + /** Steps 1 to 5 of openContainedFile, beneath the root that step 0 checked. */ + private async openBeneathRoot( + mount: MountState, + { real: realMountRoot, dir: rootDir }: { real: string; dir: CheckedDir }, + relativePath: string, + mountPrefixedPath: string, + inspect?: (fileStat: Stats) => void, ): Promise<{ handle: Awaited>; fileStat: Stats; realFilePath: string }> { const lexicalPath = resolve(mount.config.path, relativePath); const follow = mount.config.followSymlinks === true; @@ -2100,13 +2228,6 @@ export class WorkspaceModule implements Module { throw new WorkspaceReadError('not_found', `Not a regular file: ${mountPrefixedPath}`, 'lstat'); } - let realMountRoot: string; - try { - realMountRoot = await realpath(mount.config.path); - } catch (err) { - throw new WorkspaceReadError('mount_unavailable', `Mount unavailable: ${mount.config.name}`, 'realpath_root', errnoOf(err)); - } - let handle: Awaited>; try { // O_NONBLOCK: a FIFO swapped in since the lstat opens at once instead of @@ -2192,6 +2313,12 @@ export class WorkspaceModule implements Module { if (pathStat.dev !== fileStat.dev || pathStat.ino !== fileStat.ino) { throw new WorkspaceReadError('changed', `File changed during read: ${mountPrefixedPath}`, 'stat'); } + // Containment compares paths, so it can't see another directory put at + // the root's own path since step 0 (a drive unmounted mid-read): the + // root is asked again, as an effect asks it (effects.ts moved()). This + // sees the root as it is now, so a root replaced and put back before it + // isn't seen (observe.ts's header says why no check can be sure). + if (!(await rootDir.stillHere())) throw await this.rootMoved(mount); // The caller's checks of the file, such as a size limit, come after the // boundary's, so they never answer for a file the boundary refuses. @@ -2219,11 +2346,20 @@ export class WorkspaceModule implements Module { * (`truncated`). The reads decide where the file ends, not the size fstat * gave at the open, so the bound holds for a file that grows while it's * read. + * - Once the workspace has recorded the mount root's identity, with the + * first evidence it gathers there, a root that is another directory is + * an unavailable mount: a replaced directory, a link swapped in for it, + * or an unmounted drive's mountpoint. In any mount, recorded or not, a + * root that stops being the directory the read checked until the file + * has been opened and checked is unavailable too: with ENOENT or + * ENOTDIR where no directory is there, as before the read, and with no + * errno for another directory. The file's bytes are then read through + * the descriptor that was checked. * * Throws {@link WorkspaceReadError} with a `code` distinguishing unknown * mount, lexical traversal, a path in another mount's directory nested in - * the one named (`other_mount`), unavailable mount, missing file, - * directory, policy-denied symlink, outside-mount target, and + * the one named (`other_mount`), unavailable mount, missing or non-regular + * file, directory, policy-denied symlink, outside-mount target, and * file-changed-during-read. */ async readFileFromDisk( @@ -2274,7 +2410,9 @@ export class WorkspaceModule implements Module { const p = mountPrefixedPath; switch (err.code) { case 'mount_unavailable': - return new WorkspaceImageReadError('mount_unavailable', `Mount unavailable: ${mount.config.name}`); + // With why, where the root's check says, such as a directory that + // isn't the mount's. + return new WorkspaceImageReadError('mount_unavailable', err.message); case 'directory': return new WorkspaceImageReadError('directory', `Path is a directory: ${p}`); case 'symlink': diff --git a/src/modules/workspace/observe.ts b/src/modules/workspace/observe.ts index 772e5a181..3e730441f 100644 --- a/src/modules/workspace/observe.ts +++ b/src/modules/workspace/observe.ts @@ -74,10 +74,12 @@ export interface MountView { export type StatFingerprint = Omit; /** - * Opened with every read or write of a workspace path, where the platform has - * it: for a regular file it changes nothing, and a FIFO or other special file - * swapped in at the path neither blocks the open nor passes the regular-file - * check that follows. + * Opened with every open of a workspace path that may already be there, a + * read, a write in place or a directory's sync, where the platform has it: + * for a regular file or a directory it changes nothing, and a FIFO or other + * special file swapped in at the path neither blocks the open nor passes the + * check of its kind that follows. A create needs none: O_CREAT | O_EXCL fails + * at once on anything already at the path. */ export const NONBLOCK = typeof fsConstants.O_NONBLOCK === 'number' ? fsConstants.O_NONBLOCK : 0; @@ -462,7 +464,11 @@ export class CheckedDir { */ async sync(): Promise { if (process.platform === 'win32') return; - const handle = await open(this.real, 'r'); + // NONBLOCK: a FIFO put at its path since it was checked opens at once, + // and is refused below as not this directory, instead of holding the + // open, and with it the mount's turn, until something opens it for + // writing. + const handle = await open(this.real, fsConstants.O_RDONLY | NONBLOCK); try { const info = await handle.stat({ bigint: true }); if (!info.isDirectory() || info.dev !== this.dev || info.ino !== this.ino) throw new Error(CHANGED); diff --git a/src/tool-presentation.ts b/src/tool-presentation.ts index 9e45111c8..9168b0803 100644 --- a/src/tool-presentation.ts +++ b/src/tool-presentation.ts @@ -1,5 +1,5 @@ import { createHash, randomUUID } from 'node:crypto'; -import { fchmodSync, closeSync, fstatSync, lstatSync, openSync, readSync, renameSync, unlinkSync, writeFileSync } from 'node:fs'; +import { constants, fchmodSync, closeSync, fstatSync, lstatSync, openSync, readSync, renameSync, unlinkSync, writeFileSync } from 'node:fs'; import { closestNames } from './closest-names.js'; import { boundedErrorMessage, errorMessage } from './error-message.js'; import type { ToolDefinition, ToolResult } from './types/events.js'; @@ -24,6 +24,13 @@ export interface PresentationSnapshot { entries: { name: string; source: string; visible: boolean; originalDescription: string; description: string; descriptionSource?: string; inputSchema: ToolDefinition['inputSchema'] }[]; } const MAX_BYTES = 256 * 1024; +/** + * Opened with each read of a presentation file, as observe.ts opens workspace + * files: a FIFO at the operator's path opens at once and fails the regular-file + * check, where a blocking open would hold the whole process (it's synchronous) + * until something opened the FIFO for writing. + */ +const NONBLOCK = typeof constants.O_NONBLOCK === 'number' ? constants.O_NONBLOCK : 0; const MAX_DESCRIPTION_CHARS = 32768; const protectedNames = new Set(['set_tool_visibility', 'set_tool_description', 'workspace--read']); export const isPresentationTool = (name: string): boolean => name === 'set_tool_visibility' || name === 'set_tool_description'; @@ -173,7 +180,7 @@ export class ToolPresentation { constructor(readonly config: ToolPresentationConfig) {} private read(path = this.config.path, allowMissing = true): string { let fd: number; - try { fd = openSync(path, 'r'); } + try { fd = openSync(path, constants.O_RDONLY | NONBLOCK); } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === 'ENOENT') { diff --git a/test/tool-presentation.test.ts b/test/tool-presentation.test.ts index 092ec448e..a60e8b4e8 100644 --- a/test/tool-presentation.test.ts +++ b/test/tool-presentation.test.ts @@ -2,6 +2,7 @@ import fs from 'node:fs'; import {syncBuiltinESMExports} from 'node:module'; import {test} from 'node:test'; import assert from 'node:assert/strict'; +import {spawnSync} from 'node:child_process'; import {mkdtempSync,rmSync,writeFileSync,readFileSync,chmodSync,statSync,existsSync,mkdirSync} from 'node:fs'; import {tmpdir} from 'node:os'; import {join} from 'node:path'; @@ -499,3 +500,23 @@ test('a problem lists at most ten names, then how many more',()=>{ assert.ok(defaultsFile.includes(`(it sets "visible" for ${listed(twelve('big--t'))}, and a defaults file may set only descriptions`),defaultsFile); } finally {rmSync(dir,{recursive:true,force:true});} }); + +test('a FIFO at the presentation file is refused at once, not waited on with the process held',{skip:process.platform==='win32'?'no FIFOs':false},()=>{ + const dir=mkdtempSync(join(tmpdir(),'presentation-fifo-')); + try { + const path=join(dir,'tools.json'); + assert.equal(spawnSync('mkfifo',[path]).status,0,'mkfifo'); + // Read in a child process, bounded: the read is synchronous, so an open that + // waits for a writer holds the whole process it runs in. + const module=new URL('../src/tool-presentation.js',import.meta.url).href; + const script=`const {ToolPresentation}=await import(${JSON.stringify(module)});` + +`const s=new ToolPresentation({path:${JSON.stringify(path)},cataloguePath:'board/tools.md'}).resolve([{name:'example',description:'original',inputSchema:{type:'object'}}]);` + +'console.log(JSON.stringify({diagnostics:s.diagnostics,description:s.advertised[0].description}));'; + const child=spawnSync(process.execPath,['--input-type=module','-e',script],{encoding:'utf8',timeout:10_000}); + assert.equal(child.signal,null,`the read waited on the FIFO until the child was stopped (${child.signal})`); + assert.equal(child.status,0,child.stderr); + const out=JSON.parse(child.stdout); + assert.match(out.diagnostics[0],/^Your tool presentation file can't be used \(it isn't a regular file\)/); + assert.equal(out.description,'original','the default description stays'); + } finally {rmSync(dir,{recursive:true,force:true});} +}); diff --git a/test/workspace-read-file-from-disk.test.ts b/test/workspace-read-file-from-disk.test.ts index c3f57826e..2cf164597 100644 --- a/test/workspace-read-file-from-disk.test.ts +++ b/test/workspace-read-file-from-disk.test.ts @@ -12,19 +12,28 @@ * Symlink cases create real symlinks; on Windows that needs a privilege the * test runner may not have (EPERM), in which case those cases are skipped — * the non-symlink cases still run everywhere. + * + * Once the workspace has recorded the mount root's identity, the same read + * refuses a root that is another directory: a link swapped in for it passed + * containment, which compares canonical paths. It asks the root again while + * it reads, and answers what it finds as before the read. */ import test, { type TestContext } from 'node:test'; import assert from 'node:assert/strict'; import { execFileSync } from 'node:child_process'; -import { appendFileSync, chmodSync, closeSync, constants as fsConstants, mkdirSync, mkdtempSync, openSync, rmSync, statSync, symlinkSync, truncateSync, writeFileSync } from 'node:fs'; +import { appendFileSync, chmodSync, closeSync, constants as fsConstants, mkdirSync, mkdtempSync, openSync, realpathSync, renameSync, rmSync, statSync, symlinkSync, truncateSync, writeFileSync } from 'node:fs'; import { createRequire, syncBuiltinESMExports } from 'node:module'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { JsStore } from '@animalabs/chronicle'; import { WorkspaceModule, WorkspaceReadError } from '../src/modules/workspace/index.js'; +import { ROOT_REPLACED, ROOT_UNAVAILABLE } from '../src/modules/workspace/observe.js'; import { changedAfterFstat, countingLoads, withFileHandle } from './helpers/file-handle.js'; +/** A valid 1x1 PNG, for read_image. */ +const ONE_PIXEL_PNG = Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4////fwAJ+wP9KobjigAAAABJRU5ErkJggg==', 'base64'); + function setup(t: TestContext, options?: { followSymlinks?: boolean }) { const root = mkdtempSync(join(tmpdir(), 'af-read-from-disk-')); const mountDir = join(root, 'mount'); @@ -38,7 +47,36 @@ function setup(t: TestContext, options?: { followSymlinks?: boolean }) { store.close(); rmSync(root, { recursive: true, force: true }); }); - return { root, mountDir, workspace }; + return { root, mountDir, store, workspace }; +} + +/** A workspace whose mount has a synced file, so the root's identity is recorded. */ +async function recorded(t: TestContext) { + const env = setup(t); + writeFileSync(join(env.mountDir, 'a.txt'), 'inside'); + const synced = await env.workspace.handleToolCall({ id: 'seed', name: 'sync', input: { mount: 'work' } }); + assert.equal(synced.success, true, synced.error); + const agreement = (env.workspace as unknown as { agreement: { rootIdentity(mount: string): unknown } }).agreement; + assert.ok(agreement.rootIdentity('work'), 'the sync recorded the root'); + return env; +} + +/** Run `fn` with node:fs/promises' `name` replaced by `make(original)`, as the module under test sees it. */ +async function withFsPromises( + name: 'open' | 'lstat' | 'stat' | 'realpath', + make: (original: (...args: any[]) => Promise) => (...args: any[]) => Promise, + fn: () => Promise, +): Promise { + const fsp = createRequire(import.meta.url)('node:fs/promises') as Record Promise>; + const original = fsp[name]!; + fsp[name] = make(original); + syncBuiltinESMExports(); + try { + return await fn(); + } finally { + fsp[name] = original; + syncBuiltinESMExports(); + } } /** Create a symlink, or skip the test when the platform refuses (Windows without the privilege). */ @@ -96,7 +134,8 @@ test('missing files and directories are distinguished', async (t) => { const { mountDir, workspace } = setup(t); mkdirSync(join(mountDir, 'dir')); - await expectRefusal(workspace.readFileFromDisk('work/missing.txt'), 'not_found'); + const missing = await expectRefusal(workspace.readFileFromDisk('work/missing.txt'), 'not_found'); + assert.equal(missing.errno, 'ENOENT', 'the errno a host reads as no file'); await expectRefusal(workspace.readFileFromDisk('work/dir'), 'directory'); await expectRefusal(workspace.readFileFromDisk('work'), 'directory'); }); @@ -382,3 +421,375 @@ test('read_image keeps its historical error wording after sharing the containmen const missing = await workspace.handleToolCall({ id: 'c2', name: 'read_image', input: { path: 'work/nope.png' } }); assert.equal(missing.error, 'File not found: work/nope.png'); }); + +test('readBinaryFromDisk with maxBytes refuses a file that grows past it after the open, having loaded no more than maxBytes and one byte', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'upload.bin'); + for (const start of [Buffer.alloc(10, 0x61), Buffer.alloc(0)]) { + writeFileSync(path, start); // a file empty at the open, too: its fstat gives no size to read up to + const { result, loaded } = await countingLoads(() => changedAfterFstat(path, () => appendFileSync(path, Buffer.alloc(5000, 0x62)), + () => workspace.readBinaryFromDisk('work/upload.bin', { maxBytes: 100 }))); + assert.deepEqual(result, { error: `File too large: ${start.length + 5000} > 100 bytes` }, 'refused, with the size the read found'); + assert.ok(loaded <= 101, `loaded ${loaded} bytes: no more than maxBytes, and the one byte that shows the file goes on`); + } +}); + +test('readBinaryFromDisk refuses a file found past maxBytes with a size past it, even when the file shrinks once read', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'upload.bin'); + writeFileSync(path, Buffer.alloc(50, 0x61)); + const ino = String(statSync(path, { bigint: true }).ino); + let stats = 0; + const result = await withFileHandle({ + stat: (stat) => async function (this: unknown, ...args: unknown[]) { + const found = await stat.apply(this, args); + if (String(found.ino) !== ino) return found; + stats++; + if (stats === 1) appendFileSync(path, Buffer.alloc(5000, 0x62)); // grows after the open's fstat + if (stats === 2) truncateSync(path, 10); // and shrinks before the fstat after the read + return stats === 2 ? stat.apply(this, args) : found; + }, + }, () => workspace.readBinaryFromDisk('work/upload.bin', { maxBytes: 100 })); + assert.equal(stats, 2, 'the fstat after the read was made'); + assert.deepEqual(result, { error: 'File too large: 101 > 100 bytes' }, 'the bytes the read found, where the fstat after it says 10'); +}); + +test('readBinaryFromDisk with maxBytes reads a file that grows after the open but stays within it, whole', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'upload.bin'); + writeFileSync(path, Buffer.alloc(10, 0x61)); + const result = await changedAfterFstat(path, () => appendFileSync(path, Buffer.alloc(20, 0x62)), + () => workspace.readBinaryFromDisk('work/upload.bin', { maxBytes: 100 })); + assert.ok('data' in result, JSON.stringify(result)); + assert.deepEqual(result.data, Buffer.concat([Buffer.alloc(10, 0x61), Buffer.alloc(20, 0x62)])); +}); + +for (const replacement of ['link', 'directory'] as const) { + const what = replacement === 'link' ? 'a link to a directory outside the mount' : "another directory at the root's path"; + test(`once the workspace has recorded the mount root, a root that is ${what} is refused by every disk reader, until a sync accepts it`, async (t) => { + const { root, mountDir, workspace } = await recorded(t); + const elsewhere = join(root, 'elsewhere'); + mkdirSync(elsewhere); + writeFileSync(join(elsewhere, 'secret.txt'), 'SECRET'); + writeFileSync(join(elsewhere, 'pixel.png'), ONE_PIXEL_PNG); + renameSync(mountDir, join(root, 'mount-away')); + if (replacement === 'link') { + if (!symlinkOrSkip(t, elsewhere, mountDir, 'dir')) return; + } else { + renameSync(elsewhere, mountDir); // as an unmounted drive's mountpoint would be + } + + const replaced = `Mount unavailable: work: ${ROOT_REPLACED}`; + const err = await expectRefusal(workspace.readFileFromDisk('work/secret.txt'), 'mount_unavailable'); + assert.equal(err.message, replaced); + assert.equal(err.stage, 'root'); + assert.equal(err.errno, undefined, 'no errno: another directory is not a missing one'); + // Nor is anything beneath it absent: the workspace's own file isn't "not found" there. + assert.equal((await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable')).message, replaced); + assert.deepEqual(await workspace.readBinaryFromDisk('work/secret.txt'), { error: replaced }); + const image = await workspace.handleToolCall({ id: 'image', name: 'read_image', input: { path: 'work/pixel.png' } }); + assert.equal(image.error, replaced); + + // The remedy the refusal names: a sync with acceptRoot takes the root as it is now. + const accepted = await workspace.handleToolCall({ id: 'accept', name: 'sync', input: { mount: 'work', acceptRoot: true } }); + assert.equal(accepted.success, true, accepted.error); + assert.equal((await workspace.readFileFromDisk('work/secret.txt')).bytes.toString(), 'SECRET'); + }); +} + +test('a root replaced by another directory between its check and the open is refused, not read', async (t) => { + const { root, mountDir, workspace } = await recorded(t); + const file = join(mountDir, 'a.txt'); + let replaced = false; + const err = await withFsPromises('open', (open) => async (path: unknown, ...rest: unknown[]) => { + if (!replaced && String(path) === file) { + replaced = true; // the root was checked; the file is about to be opened through its path + renameSync(mountDir, join(root, 'mount-away')); + mkdirSync(mountDir); + writeFileSync(file, 'the replacement'); + } + return open(path, ...rest); + }, () => workspace.readFileFromDisk('work/a.txt').then( + (read) => assert.fail(`read the replacement's file: ${read.bytes.toString()}`), + (e: unknown) => e, + )); + assert.equal(replaced, true, 'the swap ran'); + assert.ok(err instanceof WorkspaceReadError && err.code === 'mount_unavailable', `refused: ${String(err)}`); + assert.equal(err.message, `Mount unavailable: work: ${ROOT_REPLACED}`); + assert.equal(err.stage, 'root'); +}); + +test('a mount whose directory is gone, or is a file, is unavailable before anything beneath it is looked at, with the errno a path there gets', async (t) => { + const { root, mountDir, workspace } = setup(t); + writeFileSync(join(mountDir, 'a.txt'), 'inside'); + renameSync(mountDir, join(root, 'mount-away')); + const gone = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.equal(gone.message, 'Mount unavailable: work'); + assert.equal(gone.errno, 'ENOENT'); + // readBinaryFromDisk answers with the refusal's message, where it said "File not found on disk". + assert.deepEqual(await workspace.readBinaryFromDisk('work/a.txt'), { error: 'Mount unavailable: work' }); + writeFileSync(mountDir, 'a file where the directory was'); + const file = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.equal(file.message, `Mount unavailable: work: ${ROOT_UNAVAILABLE}`); + assert.equal(file.errno, 'ENOTDIR'); + assert.deepEqual(await workspace.readBinaryFromDisk('work/a.txt'), { error: `Mount unavailable: work: ${ROOT_UNAVAILABLE}` }); +}); + +// The root is checked before anything beneath it, and asked again before +// the read answers anything about a path beneath it: a missing file, a file +// that changed, or the file it goes on to read. Each time, what is at the +// root's path then decides the code and errno, as at the first check. +const MOMENTS = { + // The file's lstat, the first thing looked up beneath the root. + 'before the file is found': { call: 'lstat', nth: 1, after: false }, + // The file's realpath, its recheck once it's open. + 'between the open and the file\'s recheck': { call: 'realpath', nth: 1, after: false }, + // The stat of the file's canonical path, the last check before the root's own. + 'once the file is open': { call: 'stat', nth: 1, after: true }, +} as const; +for (const [moment, { call, nth, after }] of Object.entries(MOMENTS)) { + for (const state of ['removed', 'replaced by a file', 'replaced by another directory', 'replaced by another directory holding the file'] as const) { + const skip = process.platform === 'win32' && moment !== 'before the file is found' + ? 'it renames the directory of a file the read holds open, which Windows may refuse' + : false; + test(`a root ${state} ${moment} is refused with the code and errno the root's check gives that state`, { skip }, async (t) => { + const { root, mountDir, workspace } = await recorded(t); + const file = join(mountDir, 'a.txt'); + const at = call === 'stat' ? realpathSync(file) : file; + let calls = 0; + let moved = false; + const move = () => { + moved = true; + renameSync(mountDir, join(root, 'mount-away')); + if (state === 'replaced by a file') writeFileSync(mountDir, 'a file where the directory was'); + if (state.startsWith('replaced by another directory')) mkdirSync(mountDir); + if (state === 'replaced by another directory holding the file') writeFileSync(file, 'the replacement'); + }; + const err = await withFsPromises(call, (original) => async (path: unknown, ...rest: unknown[]) => { + if (moved || String(path) !== at || ++calls !== nth) return original(path, ...rest); + if (!after) move(); + const result = await original(path, ...rest); + if (after) move(); + return result; + }, () => workspace.readFileFromDisk('work/a.txt').then( + (read) => assert.fail(`read through the moved root: ${read.bytes.toString()}`), + (e: unknown) => e, + )); + assert.equal(moved, true, 'the root moved'); + assert.ok(err instanceof WorkspaceReadError && err.code === 'mount_unavailable', `refused as unavailable: ${String(err)}`); + assert.equal(err.stage, 'root'); + if (state.startsWith('replaced by another directory')) { + assert.equal(err.message, `Mount unavailable: work: ${ROOT_REPLACED}`); + assert.equal(err.errno, undefined, 'no errno: another directory is not a missing one'); + } else { + assert.equal(err.message, `Mount unavailable: work: ${ROOT_UNAVAILABLE}`); + assert.equal(err.errno, state === 'removed' ? 'ENOENT' : 'ENOTDIR', 'the errno a root in that state gets before the read'); + } + }); + } +} + +// A mount configured through a link, re-pointed to another directory while a +// read runs, as a blue-green deploy swaps one. The directory the read checked +// is untouched: only the configured path's realpath shows that the mount is +// now another directory. +for (const [moment, { call, nth, after }] of Object.entries(MOMENTS)) { + const skip = process.platform === 'win32' ? 'it renames a link over the mount\'s link, which Windows may refuse' : false; + test(`a mount configured through a link that is re-pointed ${moment} is refused as replaced, not read`, { skip }, async (t) => { + const root = mkdtempSync(join(tmpdir(), 'af-read-from-disk-')); + let store: JsStore | undefined; + t.after(() => { + store?.close(); + rmSync(root, { recursive: true, force: true }); + }); + const blue = join(root, 'blue'); + const green = join(root, 'green'); + mkdirSync(blue); + mkdirSync(green); + writeFileSync(join(blue, 'a.txt'), 'blue'); + writeFileSync(join(green, 'a.txt'), 'green'); + const link = join(root, 'current'); + if (!symlinkOrSkip(t, blue, link, 'dir')) return; + store = JsStore.openOrCreate({ path: join(root, 'workspace.chronicle') }); + const workspace = new WorkspaceModule({ mounts: [{ name: 'work', path: link, mode: 'read-write', watch: 'never' }] }); + workspace.initStore(store); + const synced = await workspace.handleToolCall({ id: 'seed', name: 'sync', input: { mount: 'work' } }); + assert.equal(synced.success, true, synced.error); + + const file = join(link, 'a.txt'); + const at = call === 'stat' ? realpathSync(file) : file; + let calls = 0; + let moved = false; + const repoint = () => { + moved = true; + symlinkSync(green, join(root, 'next'), 'dir'); + renameSync(join(root, 'next'), link); + }; + const err = await withFsPromises(call, (original) => async (path: unknown, ...rest: unknown[]) => { + if (moved || String(path) !== at || ++calls !== nth) return original(path, ...rest); + if (!after) repoint(); + const result = await original(path, ...rest); + if (after) repoint(); + return result; + }, () => workspace.readFileFromDisk('work/a.txt').then( + (read) => assert.fail(`read through the re-pointed link: ${read.bytes.toString()}`), + (e: unknown) => e, + )); + assert.equal(moved, true, 'the link was re-pointed'); + assert.ok(err instanceof WorkspaceReadError && err.code === 'mount_unavailable', `refused as unavailable: ${String(err)}`); + assert.equal(err.message, `Mount unavailable: work: ${ROOT_REPLACED}`); + assert.equal(err.stage, 'root'); + assert.equal(err.errno, undefined, 'no errno: another directory is not a missing one'); + }); +} + +test('a mount with no recorded identity is asked about its root mid-read too', async (t) => { + const { root, mountDir, workspace } = setup(t); + const file = join(mountDir, 'a.txt'); + writeFileSync(file, 'inside'); + let moved = false; + const err = await withFsPromises('lstat', (lstat) => async (path: unknown, ...rest: unknown[]) => { + if (!moved && String(path) === file) { + moved = true; // the root was checked, with no identity to compare; the file is looked up next + renameSync(mountDir, join(root, 'mount-away')); + mkdirSync(mountDir); + } + return lstat(path, ...rest); + }, () => workspace.readFileFromDisk('work/a.txt').then( + (read) => assert.fail(`read through the moved root: ${read.bytes.toString()}`), + (e: unknown) => e, + )); + assert.equal(moved, true, 'the root moved'); + assert.ok(err instanceof WorkspaceReadError && err.code === 'mount_unavailable', `refused as unavailable: ${String(err)}`); + assert.equal(err.stage, 'root'); + assert.equal(err.errno, undefined, 'not reported missing from the directory put in its place'); +}); + +test('while the disk evidence awaits reopening, a read goes on, checked against the root identity last recorded', async (t) => { + const { root, mountDir, store, workspace } = await recorded(t); + writeFileSync(join(mountDir, 'b.txt'), 'more'); + // An append to the evidence fails inside the store, so the evidence can't + // be read again until the store is reopened. + const patched = store as unknown as { appendJson: (type: string, payload: unknown) => unknown }; + const append = patched.appendJson; + patched.appendJson = function (this: unknown, type: string, payload: unknown) { + if (type === 'workspace/disk-agreement') throw new Error('injected: the store refused the append'); + return append.call(this, type, payload); + }; + try { + const synced = await workspace.handleToolCall({ id: 'sync', name: 'sync', input: { mount: 'work' } }); + assert.equal(synced.success, false, 'the sync that would record b.txt failed'); + } finally { + patched.appendJson = append; + } + const again = await workspace.handleToolCall({ id: 'again', name: 'sync', input: { mount: 'work' } }); + assert.match(again.error ?? '', /until the store is reopened/, 'the evidence awaits reopening'); + + assert.equal((await workspace.readFileFromDisk('work/a.txt')).bytes.toString(), 'inside', 'the mount\'s own file is read'); + renameSync(mountDir, join(root, 'mount-away')); + mkdirSync(mountDir); + writeFileSync(join(mountDir, 'a.txt'), 'the replacement'); + const err = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.equal(err.message, `Mount unavailable: work: ${ROOT_REPLACED}`, 'another directory is refused, as before the failure'); +}); + +test("a sync with acceptRoot whose append fails leaves reads refusing the root it was to accept", async (t) => { + const { root, mountDir, store, workspace } = await recorded(t); + renameSync(mountDir, join(root, 'mount-away')); + mkdirSync(mountDir); + writeFileSync(join(mountDir, 'a.txt'), 'the replacement'); + const patched = store as unknown as { appendJson: (type: string, payload: unknown) => unknown }; + const append = patched.appendJson; + patched.appendJson = function (this: unknown, type: string, payload: unknown) { + if (type === 'workspace/disk-agreement') throw new Error('injected: the store refused the append'); + return append.call(this, type, payload); + }; + try { + const accepted = await workspace.handleToolCall({ id: 'accept', name: 'sync', input: { mount: 'work', acceptRoot: true } }); + assert.equal(accepted.success, false, 'the sync whose append failed fails'); + } finally { + patched.appendJson = append; + } + // The identity in memory is still the one from before: append comes before apply. + const err = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.equal(err.message, `Mount unavailable: work: ${ROOT_REPLACED}`); +}); + +/** + * Mounts recorded in a store, which is then closed and opened again, as at a + * restart, by a workspace whose mounts are `again`, with `breaking` making the + * store fail as that open reads or writes the disk evidence. Its initStore + * throws; the workspace is returned for reads all the same. + */ +async function reopenedBroken( + t: TestContext, + first: Array<{ name: string; path: string }>, + again: Array<{ name: string; path: string }>, + breaking: (store: Record unknown>) => void, +) { + const root = mkdtempSync(join(tmpdir(), 'af-read-from-disk-')); + const path = join(root, 'workspace.chronicle'); + let store: JsStore | undefined; + t.after(() => { + store?.close(); + rmSync(root, { recursive: true, force: true }); + }); + const mounts = (list: typeof first) => list.map(({ name, path: dir }) => ({ name, path: join(root, dir), mode: 'read-write' as const, watch: 'never' as const })); + for (const { path: dir } of [...first, ...again]) mkdirSync(join(root, dir), { recursive: true }); + for (const { path: dir } of first) writeFileSync(join(root, dir, 'a.txt'), `inside ${dir}`); + store = JsStore.openOrCreate({ path }); + const before = new WorkspaceModule({ mounts: mounts(first) }); + before.initStore(store); + for (const { name } of first) { + const synced = await before.handleToolCall({ id: `seed-${name}`, name: 'sync', input: { mount: name } }); + assert.equal(synced.success, true, synced.error); + } + store.close(); + store = JsStore.openOrCreate({ path }); + breaking(store as unknown as Record unknown>); + const workspace = new WorkspaceModule({ mounts: mounts(again) }); + assert.throws(() => workspace.initStore(store!), /injected/, 'the open fails'); + return { root, workspace }; +} + +test("a workspace whose open couldn't read its disk evidence back refuses disk reads, with no directory's errno only where there's none", async (t) => { + const mount = [{ name: 'work', path: 'mount' }]; + const { root, workspace } = await reopenedBroken(t, mount, mount, (store) => { + const ids = store.getRecordIdsByType!; + store.getRecordIdsByType = function (this: unknown, type: string) { + if (String(type).startsWith('workspace/disk-agreement')) throw new Error('injected: the store could not be read'); + return ids.call(this, type); + }; + }); + const err = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.equal(err.stage, 'root'); + assert.match(err.message, /^Mount unavailable: work: Disk-agreement evidence is unavailable until the store is reopened/); + assert.equal(err.errno, undefined, 'a directory is there'); + rmSync(join(root, 'mount'), { recursive: true }); + writeFileSync(join(root, 'mount'), 'a file where the directory was'); + const file = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.equal(file.errno, 'ENOTDIR', 'no directory is there, whatever the evidence'); +}); + +test("a workspace whose open failed to record a changed mount refuses disk reads of every mount, the ones after it included", async (t) => { + // The first mount's directory changed since the evidence was recorded, so + // the open appends its record first, and that append fails in the store. + const { root, workspace } = await reopenedBroken(t, + [{ name: 'work', path: 'mount' }, { name: 'other', path: 'other' }], + [{ name: 'work', path: 'moved' }, { name: 'other', path: 'other' }], + (store) => { + const append = store.appendJson!; + store.appendJson = function (this: unknown, type: string, payload: unknown) { + if (type === 'workspace/disk-agreement') throw new Error('injected: the store refused the append'); + return append.call(this, type, payload); + }; + }); + // A directory put at the later mount's root is refused, not read unchecked. + renameSync(join(root, 'other'), join(root, 'other-away')); + mkdirSync(join(root, 'other')); + writeFileSync(join(root, 'other', 'a.txt'), 'the replacement'); + const other = await expectRefusal(workspace.readFileFromDisk('other/a.txt'), 'mount_unavailable'); + assert.match(other.message, /^Mount unavailable: other: Disk-agreement evidence is unavailable until the store is reopened/); + const work = await expectRefusal(workspace.readFileFromDisk('work/a.txt'), 'mount_unavailable'); + assert.match(work.message, /^Mount unavailable: work: Disk-agreement evidence is unavailable until the store is reopened/); +}); diff --git a/test/workspace-three-way.test.ts b/test/workspace-three-way.test.ts index 611edbca0..8f48d5a9d 100644 --- a/test/workspace-three-way.test.ts +++ b/test/workspace-three-way.test.ts @@ -13,7 +13,7 @@ import { describe, test, type TestContext } from 'node:test'; import { TOOL_BATCH_HOOK_TIMEOUT_MS } from '../src/module-registry.js'; import assert from 'node:assert/strict'; import { spawnSync } from 'node:child_process'; -import { appendFileSync, chmodSync, constants as fsConstants, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, realpathSync, renameSync, rmSync, statSync, symlinkSync, truncateSync, unlinkSync, utimesSync, writeFileSync } from 'node:fs'; +import { appendFileSync, chmodSync, closeSync, constants as fsConstants, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, openSync, readdirSync, readFileSync, realpathSync, renameSync, rmSync, statSync, symlinkSync, truncateSync, unlinkSync, utimesSync, writeFileSync } from 'node:fs'; import { open, statfs } from 'node:fs/promises'; import { createHash } from 'node:crypto'; import { createRequire, syncBuiltinESMExports } from 'node:module'; @@ -3891,6 +3891,32 @@ describe('restarts and faults', () => { assert.equal(agreement.needsBarrier, true); }); + test('a rebuild whose append fails partway leaves memory as it was, whole, with the identity a disk read checks', (t) => { + // Nothing reaches this today: after the open, a rebuild finds every + // mount's record and appends none. A later open with a moved mount does. + const env = new Env(t); + const agreement = new DiskAgreement(env.store); + agreement.open([{ name: 'work', root: '/srv/work' }, { name: 'other', root: '/srv/other' }]); + const identity = { dev: '1', ino: '2' }; + agreement.recordRootIdentity('other', identity); + const store = env.store as unknown as { appendJson: (type: string, payload: unknown) => unknown }; + const append = store.appendJson; + store.appendJson = function (this: unknown, type: string, payload: unknown) { + if (type === DISK_AGREEMENT_RECORD_TYPE) throw new Error('injected: the store refused the append'); + return append.call(this, type, payload); + }; + try { + // 'work' has moved, so the rebuild appends its record before it reaches 'other'. + assert.throws(() => agreement.open([{ name: 'work', root: '/srv/moved' }, { name: 'other', root: '/srv/other' }]), /injected/); + } finally { + store.appendJson = append; + } + // Through a structural type, so this builds against a DiskAgreement + // without the accessor too, and fails there rather than not building. + const read = agreement as unknown as { rootIdentityForRead(mount: string): unknown }; + assert.deepEqual(read.rootIdentityForRead('other'), identity, 'the mount after the failed append keeps its identity'); + }); + test('an append that fails inside the store leaves no decision to stale evidence: passes refuse until a reopen, which finds the evidence', async (t) => { const env = new Env(t); let m = await env.open(); @@ -4745,6 +4771,40 @@ describe('trust and identity at the point of use', () => { assert.equal(read.hash, createHash('sha256').update(grown).digest('hex')); }); + test("a FIFO swapped in at a checked directory's path doesn't hold up its sync", { skip: process.platform === 'win32' ? 'no FIFOs or directory syncs' : false }, async (t) => { + const env = new Env(t); + mkdirSync(env.disk('sub'), { recursive: true }); + const rootReal = realpathSync(env.dir); + const dir = join(rootReal, 'sub'); + let swapped = false; + const synced = withFsPromises('open', (original) => async (...args: any[]) => { + if (!swapped && args[0] === dir) { + swapped = true; // checked as a directory, then a FIFO put at its path before the sync opens it + rmSync(dir, { recursive: true }); + assert.equal(spawnSync('mkfifo', [dir]).status, 0, 'mkfifo'); + } + return original(...args); + }, () => syncDirectories(viewOf(env.dir), rootReal, ['sub'])); + let timer: ReturnType | undefined; + try { + const failed = await Promise.race([synced, new Promise((_, reject) => { + // Only a sync held by the FIFO comes near this: without one, it settles at once. + timer = setTimeout(() => reject(new Error('the sync waited on the FIFO')), 10_000); + })]); + assert.equal(swapped, true, 'the swap ran'); + assert.match(failed.get('sub') ?? '', /changed while it was observed/); + } finally { + clearTimeout(timer); + // An open still waiting on the FIFO is let go before the mount is removed. + try { + closeSync(openSync(dir, fsConstants.O_WRONLY | fsConstants.O_NONBLOCK)); + } catch { + // ENXIO: nothing is waiting. + } + await synced.catch(() => {}); + } + }); + /** A root identified as it is now, and a way to replace it: renamed away, with an empty `sub` in its place. */ const replaceable = (env: Env) => { mkdirSync(env.disk('sub'), { recursive: true });