From d16dce04b4b4f4797a542ac8ae9994c3bae20e1e Mon Sep 17 00:00:00 2001 From: Rupert-2634 Date: Thu, 15 Oct 2026 18:04:07 -0400 Subject: [PATCH 1/2] fix(workspace): readFileFromDisk's maxBytes and read_image's size limit bound the read itself readFileFromDisk's maxBytes and read_image's size limit (the mount's maxFileSize) were checked against the size the open's fstat reported. Then a file within the limit was read with FileHandle.readFile, which in Node does its own fstat and reads up to the size it finds there, or, for a size of 0, to the file's end. So a file that grew between the two was loaded whole. readFileFromDisk returned all of it with truncated false, against its documented "at most maxBytes bytes are ever loaded", and read_image loaded it before refusing it as too large. Both now read through readPrefix (observe.ts), which reads at most the limit and says whether the file goes on past it. The fstat size only sizes the first buffer. Once a buffer is full, one byte read past it tells whether the file goes on. At the limit, that byte means it does, and isn't kept. Short of the limit, it means the file has grown since its fstat, so readPrefix keeps the byte and reads on into a larger buffer, up to the limit. A file that shrank ends where the reads find its end. A fractional limit allows the whole bytes in it. readFileFromDisk reports a file that goes on as truncated, and read_image refuses it as too_large, in the words its fstat check and validateImageBytes use (imageTooLarge, now in one place). readFileFromDisk's size now agrees with its bytes and truncated. It's bytes.length unless truncated, and then the file's size by an fstat after the read, or maxBytes + 1 if that's less, since the read found at least that many bytes. Before, size was the open's fstat size, so a file that changed in between could return a size that agreed with neither its bytes nor truncated, and one that shrank under maxBytes after the open was reported truncated. Without maxBytes, the read is FileHandle.readFile, as before. readBinaryFromDisk's maxBytes has the same shape (a stat, then readFile). #282 rewrites that function to read through openContainedFile, as readFileFromDisk does, so a change after #282 merges bounds it there. New tests, each failing on main: - workspace-read-file-from-disk: a file that grows past maxBytes after the open's fstat (an fstat hook appends to it), from 10 bytes and from empty, is read only to maxBytes, truncated, with its size once read, having loaded no more than maxBytes and the byte after it (FileHandle read and readFile counted); one that shrinks under maxBytes after the open is read to its end and not truncated; one that grows within maxBytes, or with no maxBytes, reports a size equal to its bytes; and a truncated read whose file is cut to 10 bytes before the fstat after the read reports maxBytes + 1. - workspace-read-image: an image that grows past maxFileSize after the open is refused having loaded no more than the limit and the byte after it. The existing maxBytes test also covers maxBytes 0, an empty file with it, and a file of exactly maxBytes. Co-Authored-By: Claude Opus 5.5 --- changelog.d/bounded-read-growth.fixed.md | 19 ++++++ src/modules/workspace/index.ts | 76 ++++++++++++---------- src/modules/workspace/observe.ts | 31 +++++++++ test/helpers/file-handle.ts | 63 ++++++++++++++++++ test/workspace-read-file-from-disk.test.ts | 71 +++++++++++++++++++- test/workspace-read-image.test.ts | 14 +++- 6 files changed, 239 insertions(+), 35 deletions(-) create mode 100644 changelog.d/bounded-read-growth.fixed.md create mode 100644 test/helpers/file-handle.ts diff --git a/changelog.d/bounded-read-growth.fixed.md b/changelog.d/bounded-read-growth.fixed.md new file mode 100644 index 000000000..9255fc4ec --- /dev/null +++ b/changelog.d/bounded-read-growth.fixed.md @@ -0,0 +1,19 @@ +- **`readFileFromDisk`'s `maxBytes` and `read_image`'s size limit now bound + the read itself, so they hold for a file that grows while it's read.** + Both were checked against the size the open's `fstat` reported (for + `read_image`, the mount's `maxFileSize`), and a file within the limit + then was read with `readFile`, which in Node reads to the size its own + `fstat` finds, or for a size of 0, to the file's end. So a file that + grew in between was loaded whole. `readFileFromDisk` returned all of it + with `truncated` false, and `read_image` loaded it before refusing it as + too large. + - Each now reads no more than its limit and one byte past it, which + isn't kept and says whether the file goes on. `readFileFromDisk` reports + a file that goes on as `truncated`, and `read_image` refuses it with its + usual "Image file exceeds max size" words. + - `readFileFromDisk`'s `truncated` and `size` now say what the read found. + A file that shrank under `maxBytes` after the open is no longer reported + `truncated`. `size` is `bytes.length` unless `truncated`, and then the + file's size by an `fstat` after the read, or `maxBytes + 1` if that's + less. Before, `size` was the open's `fstat` size, so it could disagree + with the bytes returned for a file that changed in between. diff --git a/src/modules/workspace/index.ts b/src/modules/workspace/index.ts index 479000375..53b61aa42 100644 --- a/src/modules/workspace/index.ts +++ b/src/modules/workspace/index.ts @@ -39,7 +39,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 { coveredByIgnore, filesystemTrustsCtime, locateRoot } from './observe.js'; +import { coveredByIgnore, filesystemTrustsCtime, locateRoot, readPrefix } from './observe.js'; import { TOOL_BATCH_HOOK_TIMEOUT_MS } from '../../module-registry.js'; import { type MountRuntime, @@ -156,13 +156,23 @@ export class WorkspaceReadError extends Error { /** Successful `readFileFromDisk()` result. */ export interface WorkspaceDiskReadResult { - /** File bytes — the whole file, or its first `maxBytes` when `truncated`. */ + /** + * File bytes: the whole file, or its first `maxBytes` when `truncated`. + * Without `maxBytes` the file is read with `readFile`, with no limit. In + * Node, that reads up to the size its own fstat finds, or to the file's end + * if that size is 0. + */ bytes: Buffer; - /** Full on-disk size in bytes, regardless of truncation. */ + /** + * The file's size, agreeing with `bytes` and `truncated`: `bytes.length`, + * unless `truncated`. Then it's the file's size by an fstat after the read, + * or `maxBytes + 1` if that's less, since the read found at least that many + * bytes. + */ size: number; - /** True when `maxBytes` was set and the file was larger; `bytes` is a prefix. */ + /** True when `maxBytes` was set and the read found the file going on past it; `bytes` is a prefix. */ truncated: boolean; - /** mtime of the file actually read (for change-detection caches). */ + /** mtime of the file actually read, from its fstat at the open (for change-detection caches). */ mtimeMs: number; /** Canonical (realpath) location of the file read — inside the canonical mount root by construction. */ realPath: string; @@ -171,7 +181,12 @@ export interface WorkspaceDiskReadResult { } export interface ReadFileFromDiskOptions { - /** Read at most this many bytes (a bounded prefix read — the rest of the file is never loaded). */ + /** + * Keep at most this many bytes (a bounded prefix read): the read loads them + * and one byte past them, which isn't kept and tells whether the file goes + * on, and never the rest of the file. It bounds the read itself, so it + * holds for a file that grows while it's read. + */ maxBytes?: number; } @@ -181,6 +196,11 @@ function errnoOf(err: unknown): string | undefined { : undefined; } +/** 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}`); +} + const PNG_SIGNATURE = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]); const JPEG_SIGNATURE = Buffer.from([0xff, 0xd8, 0xff]); const GIF87A_SIGNATURE = Buffer.from('GIF87a', 'ascii'); @@ -1627,12 +1647,7 @@ export class WorkspaceModule implements Module { if (bytes.byteLength === 0) { throw new WorkspaceImageReadError('empty', `Image file is empty: ${mountPrefixedPath}`); } - if (bytes.byteLength > maxSize) { - throw new WorkspaceImageReadError( - 'too_large', - `Image file exceeds max size (${maxSize} bytes): ${mountPrefixedPath}`, - ); - } + if (bytes.byteLength > maxSize) throw imageTooLarge(maxSize, mountPrefixedPath); return { bytes, mimeType: detectImageMimeType(bytes, mountPrefixedPath), @@ -1809,7 +1824,10 @@ export class WorkspaceModule implements Module { * - With symlinks allowed, the canonical target must stay beneath the * canonical mount root; a sibling-prefix root (`/mount-other`) does not count. * - `maxBytes` performs a bounded prefix read: at most `maxBytes` bytes are - * ever loaded, and `truncated` reports that the file was larger. + * kept, and one byte past them is read to tell whether the file goes on + * (`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. * * Throws {@link WorkspaceReadError} with a `code` distinguishing unknown * mount, lexical traversal, unavailable mount, missing file, directory, @@ -1837,23 +1855,17 @@ export class WorkspaceModule implements Module { const { handle, fileStat, realFilePath } = await this.openContainedFile(mount, relativePath, mountPrefixedPath); try { - const size = fileStat.size; const cap = options.maxBytes; let bytes: Buffer; let truncated = false; - if (cap !== undefined && size > cap) { - truncated = true; - bytes = Buffer.alloc(cap); - let offset = 0; - while (offset < cap) { - const { bytesRead } = await handle.read(bytes, offset, cap - offset, offset); - if (bytesRead === 0) break; - offset += bytesRead; - } - if (offset < cap) bytes = bytes.subarray(0, offset); - } else { + if (cap === undefined) { bytes = await handle.readFile(); + } else { + ({ bytes, more: truncated } = await readPrefix(handle, cap, fileStat.size)); } + // What the read found, not what the fstat before it said, so `size`, + // `bytes` and `truncated` agree even for a file that changed in between. + const size = truncated ? Math.max((await handle.stat()).size, bytes.length + 1) : bytes.length; return { bytes, size, truncated, mtimeMs: fileStat.mtimeMs, realPath: realFilePath, mount: mount.config.name }; } catch (err) { if (err instanceof WorkspaceReadError) throw err; @@ -1904,12 +1916,7 @@ export class WorkspaceModule implements Module { if (fileStat.size === 0) { throw new WorkspaceImageReadError('empty', `Image file is empty: ${mountPrefixedPath}`); } - if (fileStat.size > maxSize) { - throw new WorkspaceImageReadError( - 'too_large', - `Image file exceeds max size (${maxSize} bytes): ${mountPrefixedPath}`, - ); - } + if (fileStat.size > maxSize) throw imageTooLarge(maxSize, mountPrefixedPath); }); } catch (err) { if (err instanceof WorkspaceImageReadError) throw err; @@ -1919,9 +1926,12 @@ export class WorkspaceModule implements Module { } throw new WorkspaceImageReadError('not_found', `Unable to read image file: ${mountPrefixedPath}`); } - const { handle } = opened; + const { handle, fileStat } = opened; try { - const bytes = await handle.readFile(); + // No more than maxSize is loaded: a file grown past it since the fstat + // above is refused here, as one already over it was there. + const { bytes, more } = await readPrefix(handle, maxSize, fileStat.size); + if (more) throw imageTooLarge(maxSize, mountPrefixedPath); return this.validateImageBytes(bytes, mountPrefixedPath, maxSize); } catch (err) { if (err instanceof WorkspaceImageReadError) throw err; diff --git a/src/modules/workspace/observe.ts b/src/modules/workspace/observe.ts index 45c5559db..3bdc4a22f 100644 --- a/src/modules/workspace/observe.ts +++ b/src/modules/workspace/observe.ts @@ -570,6 +570,37 @@ async function openContained( } } +/** + * Read an open file from its start, at most `limit` bytes, and whether it + * goes on past them. Where the file ends is what the reads find: `sizeHint`, + * the size its fstat gave, only sizes the first buffer. Once a buffer is + * full, one byte read past it tells whether the file goes on. At the limit, + * that byte says it does, and isn't kept. Short of it, the file has grown + * since its fstat: the byte is kept, and the read goes on in a larger buffer, + * up to the limit. A file that has shrunk ends where the reads find its end. + * A fractional `limit` allows the whole bytes in it. + */ +export async function readPrefix(handle: FileHandle, limit: number, sizeHint: number): Promise<{ bytes: Buffer; more: boolean }> { + const cap = Math.floor(limit); + const next = Buffer.alloc(1); + let bytes = Buffer.alloc(Math.min(cap, sizeHint)); + let length = 0; + for (;;) { + while (length < bytes.length) { + const { bytesRead } = await handle.read(bytes, length, bytes.length - length, length); + if (bytesRead === 0) return { bytes: bytes.subarray(0, length), more: false }; + length += bytesRead; + } + if ((await handle.read(next, 0, 1, length)).bytesRead === 0) return { bytes, more: false }; + if (length === cap) return { bytes, more: true }; + // Longer than its fstat said: read on, up to the limit. + const grown = Buffer.alloc(Math.min(cap, Math.max(2 * length, length + 64 * 1024))); + bytes.copy(grown, 0, 0, length); + grown[length++] = next[0]; + bytes = grown; + } +} + /** * Read and hash a mount-relative file through one located descriptor. The * whole file is returned when it is at most `limit` bytes; larger files are diff --git a/test/helpers/file-handle.ts b/test/helpers/file-handle.ts new file mode 100644 index 000000000..5f02a443b --- /dev/null +++ b/test/helpers/file-handle.ts @@ -0,0 +1,63 @@ +import { statSync } from 'node:fs'; +import { open } from 'node:fs/promises'; + +type Method = 'stat' | 'read' | 'readFile'; +type Fn = (...args: any[]) => Promise; + +/** + * Run `fn` with FileHandle's methods replaced, for every handle, by what each + * hook makes of the original (which it calls with `original.apply(this, args)`). + */ +export async function withFileHandle(hooks: Partial Fn>>, fn: () => Promise): Promise { + const probe = await open(process.execPath, 'r'); + const proto = Object.getPrototypeOf(probe) as Record; + await probe.close(); + const originals = new Map(); + for (const [method, make] of Object.entries(hooks) as Array<[Method, (original: Fn) => Fn]>) { + originals.set(method, proto[method]); + proto[method] = make(proto[method]); + } + try { + return await fn(); + } finally { + for (const [method, original] of originals) proto[method] = original; + } +} + +/** Run `fn`, counting the bytes that every FileHandle read and readFile loads meanwhile. */ +export async function countingLoads(fn: () => Promise): Promise<{ result: T; loaded: number }> { + let loaded = 0; + const result = await withFileHandle({ + read: (read) => async function (this: unknown, ...args: unknown[]) { + const done = await read.apply(this, args); + loaded += done.bytesRead; + return done; + }, + readFile: (readFile) => async function (this: unknown, ...args: unknown[]) { + const bytes = await readFile.apply(this, args); + loaded += bytes.length; + return bytes; + }, + }, fn); + return { result, loaded }; +} + +/** + * Run `fn` with `change` made once, right after a FileHandle stat of the file + * at `path` returns: in a read, just after its open has stat'd the file, so + * the read then finds something other than that stat said. + */ +export async function changedAfterFstat(path: string, change: () => void, fn: () => Promise): Promise { + const ino = String(statSync(path, { bigint: true }).ino); + let changed = false; + return withFileHandle({ + stat: (stat) => async function (this: unknown, ...args: unknown[]) { + const result = await stat.apply(this, args); + if (!changed && String(result.ino) === ino) { + changed = true; + change(); + } + return result; + }, + }, fn); +} diff --git a/test/workspace-read-file-from-disk.test.ts b/test/workspace-read-file-from-disk.test.ts index e7b6ffc9d..7d1de68dc 100644 --- a/test/workspace-read-file-from-disk.test.ts +++ b/test/workspace-read-file-from-disk.test.ts @@ -16,11 +16,12 @@ import test, { type TestContext } from 'node:test'; import assert from 'node:assert/strict'; -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { appendFileSync, mkdirSync, mkdtempSync, rmSync, statSync, symlinkSync, truncateSync, writeFileSync } from 'node:fs'; 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 { changedAfterFstat, countingLoads, withFileHandle } from './helpers/file-handle.js'; function setup(t: TestContext, options?: { followSymlinks?: boolean }) { const root = mkdtempSync(join(tmpdir(), 'af-read-from-disk-')); @@ -176,9 +177,77 @@ test('maxBytes performs a bounded prefix read and reports truncation', async (t) assert.equal(whole.truncated, false); assert.equal(whole.bytes.length, 10_000); + const exact = await workspace.readFileFromDisk('work/big.txt', { maxBytes: 10_000 }); + assert.equal(exact.truncated, false, 'a file of exactly maxBytes goes on no further'); + assert.equal(exact.bytes.length, 10_000); + + const none = await workspace.readFileFromDisk('work/big.txt', { maxBytes: 0 }); + assert.deepEqual([none.bytes.length, none.truncated, none.size], [0, true, 10_000]); + writeFileSync(join(mountDir, 'empty.txt'), ''); + const empty = await workspace.readFileFromDisk('work/empty.txt', { maxBytes: 0 }); + assert.deepEqual([empty.bytes.length, empty.truncated, empty.size], [0, false, 0]); + await assert.rejects(workspace.readFileFromDisk('work/big.txt', { maxBytes: -1 }), RangeError); }); +test('maxBytes bounds the read itself: a file that grows past it after the open is still read no further', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'log.txt'); + for (const start of ['a'.repeat(10), '']) { + writeFileSync(path, start); // a file empty at the open, too: its fstat gives no size to read up to + const { result: read, loaded } = await countingLoads(() => changedAfterFstat(path, () => appendFileSync(path, 'b'.repeat(5000)), + () => workspace.readFileFromDisk('work/log.txt', { maxBytes: 100 }))); + assert.equal(read.bytes.toString(), (start + 'b'.repeat(5000)).slice(0, 100), 'its first maxBytes'); + assert.equal(read.truncated, true, 'the read found the file going on past maxBytes'); + assert.equal(read.size, start.length + 5000, 'its size once read, not the size the open\'s fstat gave'); + assert.ok(loaded <= 101, `loaded ${loaded} bytes: no more than maxBytes, and the one byte that shows the file goes on`); + } +}); + +test('a truncated read reports a size past maxBytes even when the file shrinks under it once read', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'log.txt'); + writeFileSync(path, 'a'.repeat(200)); + const ino = String(statSync(path, { bigint: true }).ino); + let stats = 0; + const read = await withFileHandle({ + stat: (stat) => async function (this: unknown, ...args: unknown[]) { + const result = await stat.apply(this, args); + if (String(result.ino) !== ino || ++stats === 1) return result; // the open's fstat + truncateSync(path, 10); // the file shrinks before the fstat after the read + return stat.apply(this, args); + }, + }, () => workspace.readFileFromDisk('work/log.txt', { maxBytes: 100 })); + assert.equal(stats, 2, 'the fstat after the read was made'); + assert.equal(read.bytes.toString(), 'a'.repeat(100)); + assert.equal(read.truncated, true, 'the read found the file going on past maxBytes'); + assert.equal(read.size, 101, 'the bytes the read found, where the fstat after it says 10'); +}); + +test('a file that shrinks under maxBytes after the open is read to its end, and not reported truncated', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'log.txt'); + writeFileSync(path, 'a'.repeat(5000)); + const read = await changedAfterFstat(path, () => truncateSync(path, 50), + () => workspace.readFileFromDisk('work/log.txt', { maxBytes: 100 })); + assert.equal(read.bytes.toString(), 'a'.repeat(50)); + assert.equal(read.truncated, false, 'the read reached the file\'s end'); + assert.equal(read.size, 50); +}); + +test('size is what the read found: a file that grows after the open, within maxBytes or with none, reports the bytes it gave', async (t) => { + const { mountDir, workspace } = setup(t); + const path = join(mountDir, 'log.txt'); + for (const options of [{}, { maxBytes: 100 }]) { + writeFileSync(path, 'a'.repeat(10)); + const read = await changedAfterFstat(path, () => appendFileSync(path, 'b'.repeat(20)), + () => workspace.readFileFromDisk('work/log.txt', options)); + assert.equal(read.bytes.toString(), 'a'.repeat(10) + 'b'.repeat(20), JSON.stringify(options)); + assert.equal(read.truncated, false); + assert.equal(read.size, 30, `size agrees with the bytes (${JSON.stringify(options)})`); + } +}); + test('read_image keeps its historical error wording after sharing the containment core', async (t) => { // The image reader was the original owner of this algorithm; its messages // are pinned by workspace-read-image.test.ts. Spot-check the two codes the diff --git a/test/workspace-read-image.test.ts b/test/workspace-read-image.test.ts index f0d60e24a..c723de1ee 100644 --- a/test/workspace-read-image.test.ts +++ b/test/workspace-read-image.test.ts @@ -1,6 +1,6 @@ import test, { type TestContext } from 'node:test'; import assert from 'node:assert/strict'; -import { existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { appendFileSync, existsSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { JsStore } from '@animalabs/chronicle'; @@ -8,6 +8,7 @@ import { AgentFramework } from '../src/framework.js'; import { WorkspaceModule } from '../src/modules/workspace/index.js'; import { toolResultDataToHistoryString } from '../src/tool-result-history.js'; import { MockMembrane, createMockResponse as createStreamResponse } from './helpers/mock-membrane.js'; +import { changedAfterFstat, countingLoads } from './helpers/file-handle.js'; import type { Module, ModuleContext, ProcessState, EventResponse } from '../src/types/module.js'; import type { ProcessEvent, ToolCall, ToolResult } from '../src/types/events.js'; @@ -378,6 +379,17 @@ test('extension and user-visible name do not override magic-byte MIME detection' expectImageResult(result, 'work/mismatch.jpg', TINY_IMAGES.png); }); +test('an image that grows past maxFileSize after the open is refused without loading more than the limit and the byte after it', async (t) => { + const { mountDir, workspace } = setupWorkspace(t, { maxFileSize: 100 }); + const path = join(mountDir, 'growing.png'); + writeFileSync(path, TINY_IMAGES.png.bytes); + assert.ok(TINY_IMAGES.png.bytes.length <= 100, 'within the limit at the open'); + const { result, loaded } = await countingLoads(() => changedAfterFstat(path, () => appendFileSync(path, Buffer.alloc(4096)), + () => callReadImage(workspace, 'work/growing.png'))); + assert.equal(result.error, 'Image file exceeds max size (100 bytes): work/growing.png'); + assert.ok(loaded <= 101, `loaded ${loaded} bytes: no more than the limit, and the one byte that shows the file goes on`); +}); + test('unknown mount, mount root, directory, unknown/truncated/empty/oversize files fail closed', async (t) => { const { mountDir, workspace } = setupWorkspace(t, { maxFileSize: 16 }); mkdirSync(join(mountDir, 'folder')); From aae1caaf3ed15d3b8948af10ea9f35503f40d875 Mon Sep 17 00:00:00 2001 From: Rupert-2634 Date: Fri, 16 Oct 2026 16:30:27 -0400 Subject: [PATCH 2/2] fix(workspace): a pass's read takes a file grown past maxFileSize since its fstat as oversize observe.ts's readPath, which reads a file that a sync, listing or scan hashes, took a file whose fstat size was within the mount's maxFileSize with FileHandle.readFile, which in Node reads up to the size its own fstat finds. So a file that grew past the limit between the two was loaded whole and returned as a file within the limit, which a sync stores whole if it's text. It now reads a file within the limit at its fstat through readPrefix, with maxFileSize as the limit. If the file goes on past it, readPath takes the oversize path it already has, which hashes the file in chunks from its start and keeps only its head. A file over the limit at its fstat takes that path without the read, as before. Tests, in workspace-three-way: readPath of a file within the limit at its fstat that an fstat hook then grows past it, with a limit of 1024 and of 1024.5, returns no bytes, its full size and hash, and its head (both fail on main); readPath of a file that grows within the limit after its fstat, from 10 bytes to 200 KiB under 1 MiB, returns all of it, with size and the fingerprint's size those of the bytes read. The tests that switch branches while a pass reads a file hooked FileHandle.readFile, which the pass no longer calls. Their hook (withReadHook) now fires at a handle's first FileHandle read at position 0, so it still fires once each time the pass reads a file. Co-Authored-By: Claude Opus 5.5 --- changelog.d/bounded-read-growth.fixed.md | 6 ++ src/modules/workspace/observe.ts | 14 +++-- test/workspace-three-way.test.ts | 73 +++++++++++++++++++----- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/changelog.d/bounded-read-growth.fixed.md b/changelog.d/bounded-read-growth.fixed.md index 9255fc4ec..10949dd02 100644 --- a/changelog.d/bounded-read-growth.fixed.md +++ b/changelog.d/bounded-read-growth.fixed.md @@ -17,3 +17,9 @@ file's size by an `fstat` after the read, or `maxBytes + 1` if that's less. Before, `size` was the open's `fstat` size, so it could disagree with the bytes returned for a file that changed in between. +- **A sync, listing or scan now takes a file that grew past the mount's + `maxFileSize` after its `fstat` as oversize.** The workspace hashes it + in chunks and keeps only its head, as for any file over the limit. + Before, its read of a file within the limit at its `fstat` used + `readFile`, so a file that grew in between was loaded whole, and a sync + could store the whole of a text file over the limit. diff --git a/src/modules/workspace/observe.ts b/src/modules/workspace/observe.ts index 3bdc4a22f..ca92c511c 100644 --- a/src/modules/workspace/observe.ts +++ b/src/modules/workspace/observe.ts @@ -603,8 +603,9 @@ export async function readPrefix(handle: FileHandle, limit: number, sizeHint: nu /** * Read and hash a mount-relative file through one located descriptor. The - * whole file is returned when it is at most `limit` bytes; larger files are - * hashed in chunks and only their head is kept. + * whole file is returned when it is at most `limit` bytes, both at its fstat + * and as read; any other file, including one that grew past `limit` since its + * fstat, is hashed in chunks and only its head is kept. */ export async function readPath( view: MountView, @@ -619,8 +620,13 @@ export async function readPath( const hashedAt = Date.now(); const size = Number(info.size); const fp: Fingerprint = { ...statFingerprint(info), hashedAt }; - if (size <= limit) { - const bytes = await handle.readFile(); + // A file within the limit at its fstat is read no further than the limit, + // so one that has grown past it since is hashed below, from its start, + // never loaded whole. One over the limit at its fstat is hashed below + // without that read. + const within = size <= limit ? await readPrefix(handle, limit, size) : null; + if (within && !within.more) { + const { bytes } = within; return { kind: 'read', size: bytes.byteLength, diff --git a/test/workspace-three-way.test.ts b/test/workspace-three-way.test.ts index 250e929d8..686f3c797 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 { 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, constants as fsConstants, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, 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'; @@ -29,6 +29,7 @@ import { pushPaths, settleAdoptionBeforeMutation } from '../src/modules/workspac import type { MountConfig, WorkspaceConfig } from '../src/modules/workspace/types.js'; import type { ModuleContext } from '../src/types/module.js'; import type { ProcessEvent } from '../src/types/events.js'; +import { changedAfterFstat, withFileHandle } from './helpers/file-handle.js'; const FAULT_FIXTURE = join(import.meta.dirname, 'fixtures/workspace-fault.mjs'); const TREE = 'workspace/work/tree'; @@ -190,22 +191,30 @@ const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); /** The hash evidence and disk facts carry for this text. */ const sha = (text: string) => createHash('sha256').update(text).digest('hex'); -/** Run `fn` with FileHandle.readFile switching branches as `onRead` says. */ +/** + * Run `fn` with each file a pass reads switching branches as `onRead` says, + * once per file, just before its read starts: at a FileHandle readFile, or at + * a handle's first FileHandle read at position 0 (a file that grew past the + * size limit is read from its start again, on the same handle, to hash it). + * Any other such read in the window counts too, such as a write's + * verification or a listing's head read. + */ async function withReadHook(onRead: (calls: number) => void, fn: () => Promise): Promise { - const probe = await open(join(tmpdir(), `read-hook-${process.pid}`), 'w'); - const proto = Object.getPrototypeOf(probe) as { readFile: (...args: unknown[]) => Promise }; - await probe.close(); - const readFile = proto.readFile; let calls = 0; - proto.readFile = async function (this: unknown, ...args: unknown[]) { - onRead(++calls); - return readFile.apply(this, args); - }; - try { - return await fn(); - } finally { - proto.readFile = readFile; - } + const started = new WeakSet(); + return withFileHandle({ + readFile: (readFile) => async function (this: unknown, ...args: unknown[]) { + onRead(++calls); + return readFile.apply(this, args); + }, + read: (read) => async function (this: object, ...args: unknown[]) { + if (args[3] === 0 && !started.has(this)) { + started.add(this); + onRead(++calls); + } + return read.apply(this, args); + }, + }, fn); } /** Run `fn` with node:fs/promises' `name` replaced by `make(original)`, as the code under test sees it. */ @@ -1900,6 +1909,40 @@ describe('trust and identity at the point of use', () => { } }); + // A fractional limit, too, which allows the whole bytes in it. + for (const limit of [1024, 1024.5]) { + test(`a file within the size limit (${limit}) at its fstat that grows past it before the read is hashed as oversize, not loaded whole`, async (t) => { + const env = new Env(t); + const rootReal = realpathSync(env.dir); + env.writeDisk('grow.txt', 'a'.repeat(100)); + const path = env.disk('grow.txt'); + const read = await changedAfterFstat(path, () => appendFileSync(path, 'b'.repeat(4000)), + () => readPath(viewOf(env.dir), rootReal, 'grow.txt', limit)); + assert.equal(read.kind, 'read'); + if (read.kind !== 'read') return; + assert.equal(read.bytes, null, 'over the limit as read, so not kept, and so not ingested'); + assert.equal(read.size, 4100, 'hashed to its end'); + assert.equal(read.hash, createHash('sha256').update('a'.repeat(100) + 'b'.repeat(4000)).digest('hex')); + assert.equal(read.head.toString(), 'a'.repeat(100) + 'b'.repeat(4000), 'its head, as for any oversize file'); + }); + } + + test('a file that grows within the size limit after its fstat is read whole, as the read found it', async (t) => { + const env = new Env(t); + const rootReal = realpathSync(env.dir); + env.writeDisk('grow.txt', 'a'.repeat(10)); + const path = env.disk('grow.txt'); + const grown = 'a'.repeat(10) + 'b'.repeat(200 * 1024); + const read = await changedAfterFstat(path, () => appendFileSync(path, 'b'.repeat(200 * 1024)), + () => readPath(viewOf(env.dir), rootReal, 'grow.txt', 1024 * 1024)); + assert.equal(read.kind, 'read'); + if (read.kind !== 'read') return; + assert.equal(read.bytes?.toString(), grown, 'read on, in larger buffers, past the 10 bytes its fstat gave'); + assert.equal(read.size, grown.length); + assert.equal(read.fp.size, grown.length, 'the fingerprint gives the size of the bytes hashed'); + assert.equal(read.hash, createHash('sha256').update(grown).digest('hex')); + }); + /** 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 });