fix(workspace): three reads' size limits bound the read itself

changelog.d/bounded-read-growth.fixed.md

@@ -0,0 +1,25 @@
+- **`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.
+- **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.

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;

src/modules/workspace/observe.ts

@@ -570,10 +570,42 @@ 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
- * 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,
@@ -588,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,

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<any>;
+
+/**
+ * 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<T>(hooks: Partial<Record<Method, (original: Fn) => Fn>>, fn: () => Promise<T>): Promise<T> {
+  const probe = await open(process.execPath, 'r');
+  const proto = Object.getPrototypeOf(probe) as Record<Method, Fn>;
+  await probe.close();
+  const originals = new Map<Method, Fn>();
+  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<T>(fn: () => Promise<T>): 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<T>(path: string, change: () => void, fn: () => Promise<T>): Promise<T> {
+  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);
+}

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