diff --git a/.changeset/metadata-fs-watcher-atomic-declared.md b/.changeset/metadata-fs-watcher-atomic-declared.md new file mode 100644 index 0000000000..1db153b6eb --- /dev/null +++ b/.changeset/metadata-fs-watcher-atomic-declared.md @@ -0,0 +1,33 @@ +--- +"@objectstack/metadata-fs": patch +--- + +fix(metadata-fs): declare `startWatcher()`'s chokidar `atomic` option explicitly (#12696) + +`FileSystemRepository.startWatcher()` constructed its chokidar watcher with +`usePolling: true` but never passed `atomic`, leaving it to inherit chokidar's +default. That default is unconditionally `true` in the installed version +(chokidar 5.0.0): the defaults literal assigns `atomic: true` *before* the +caller's options are spread in, so chokidar's own default-correction +(`if (opts.atomic === undefined) opts.atomic = !opts.usePolling`) can never +fire — it only runs when `atomic` is literally `undefined` after the merge, +which it never is. The comment beside that correction ("Editor atomic write +normalization enabled by default with fs.watch") reads as "off under +polling"; the actual resolved behaviour was on regardless. + +This change passes `atomic: true` explicitly at the call site, with a comment +explaining why. **Patch, not a behaviour change**: verified at runtime +(constructing a watcher the way `startWatcher()` does and reading back +`watcher.options.atomic`) that the resolved value is identical before and +after — `true` either way, today. The only thing that changes is that the +value is now DECLARED rather than inherited from an upstream branch that +cannot execute, so a future chokidar release that fixes the ordering (making +the correction real) cannot silently flip this repository's watcher to +`atomic: false` under polling and change behaviour with no diff to review. + +Not addressed here (see #12696): whether `atomic: true` (the 100ms +unlink-coalescing deferral and the `DOT_RE` editor-temp-file matcher it turns +on) is actually the right value. No evidence surfaced that either has ever +affected a run; flipping it to `false` is a deliberate behaviour change to a +live delivery path that needs its own reverse verification, and is out of +scope for this card. diff --git a/packages/metadata-fs/src/repository.ts b/packages/metadata-fs/src/repository.ts index 68ee5554fd..d551426a75 100644 --- a/packages/metadata-fs/src/repository.ts +++ b/packages/metadata-fs/src/repository.ts @@ -595,6 +595,23 @@ export class FileSystemRepository implements MetadataRepository { usePolling: true, interval: 1000, binaryInterval: 2000, + // Declared explicitly, not inherited. chokidar's own default-correction + // (`if (opts.atomic === undefined) opts.atomic = !opts.usePolling`) can + // only fire when the caller omits `atomic`, but its defaults literal + // already assigns `atomic: true` *before* the caller's options are + // spread in — so leaving `atomic` unset here does not mean "off under + // polling" the way the correction's own comment claims, it silently + // resolves to `true` regardless of `usePolling`. That has been this + // repository's actual runtime behaviour all along (verified by reading + // back the resolved option from a real watcher instance, #12696): every + // `unlink` gets chokidar's 100ms editor-atomic-write deferral, and + // `DOT_RE` (vim swap files, `~`, sublime tmp) is folded into + // `_isIgnored` on top of this repository's own `isIgnoredWatchPath` + // (#7150). `atomic: true` here keeps that behaviour byte-for-byte — + // this is a declaration, not a change. Flipping it to `false` would + // remove both behaviours from a live delivery path and needs its own + // reverse verification; see #12696 for the analysis. + atomic: true, }); w.on('add', (p) => void this.handleFsChange(p, 'add')); w.on('change', (p) => void this.handleFsChange(p, 'change')); diff --git a/packages/metadata-fs/test/watcher-atomic-declared.test.ts b/packages/metadata-fs/test/watcher-atomic-declared.test.ts new file mode 100644 index 0000000000..47e74843bd --- /dev/null +++ b/packages/metadata-fs/test/watcher-atomic-declared.test.ts @@ -0,0 +1,113 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12696 — `startWatcher()` must pass `atomic` to chokidar EXPLICITLY. + * + * chokidar 5's defaults literal assigns `atomic: true` BEFORE the caller's + * options are spread in (`node_modules/chokidar/index.js`): + * + * const opts = { + * // Defaults + * ... + * atomic: true, // NOTE: overwritten later (depends on usePolling) + * ..._opts, + * ... + * }; + * ... + * // Editor atomic write normalization enabled by default with fs.watch + * if (opts.atomic === undefined) + * opts.atomic = !opts.usePolling; + * + * so the "overwritten later" comment is aspirational: the correction can + * only run when the caller omits `atomic` AND that omission left + * `opts.atomic === undefined`, but it never does — the defaults literal + * already assigned `true`, and the caller's spread has no `atomic` key to + * override it with. The branch is dead. + * + * The consequence for THIS pin: the RESOLVED value chokidar hands back + * (`watcher.options.atomic`) is `true` whether `startWatcher()` declares it + * or not — a merged-value read cannot tell "declared here" apart from + * "inherited a dead branch that happens to agree today". A pin written + * against that merged read would stay green across the exact regression it + * exists to catch: a future chokidar release that fixes the ordering (making + * the correction real) would then silently flip this repository's watcher to + * `atomic: false` under `usePolling` — losing the 100ms unlink-coalescing + * deferral and the `DOT_RE` editor-temp matcher from a live delivery path — + * and nothing here would notice. + * + * So this pin does not read the merged option. It reads the ACTUAL call + * `startWatcher()` makes to `chokidar.watch()`, via a spy that lets the real + * call through unchanged (this pins DECLARATION, not delivery — every other + * watcher behaviour in this package must keep working identically). That is + * a runtime observation of what this repository hands off, not a grep of the + * call-site literal, and it is the one thing whose presence a future + * chokidar default cannot silently override. + * + * ⛔ No wall-clock wait anywhere in this file, deliberately (see + * `watch-dot-root.test.ts` for the standing prohibition and its history of + * merge-queue ejections). None is needed: the root is created by `mkdtemp()` + * before `start()` runs, so `start()` arms the watcher SYNCHRONOUSLY inside + * its own call (see its comment on #7000/#9339) — `chokidar.watch()` is + * invoked, and the spy has recorded the call, by the time `start()`'s + * promise resolves. This case never touches the 100ms unlink window itself + * (out of scope for #12696 — see the card's "carry forward" section). + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import fs from 'node:fs/promises'; +import path from 'node:path'; +import os from 'node:os'; +import chokidar from 'chokidar'; +import { FileSystemRepository } from '../src/index.js'; + +describe('FileSystemRepository — startWatcher() declares `atomic` explicitly (#12696)', () => { + let root: string; + let repo: FileSystemRepository | undefined; + let watchSpy: ReturnType; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'objectstack-fsatomic-')); + // Let the real call through — nothing here is faked, so every other + // watcher behaviour this case exercises stays exactly as it runs in + // production. + watchSpy = vi.spyOn(chokidar, 'watch'); + }); + + afterEach(async () => { + if (repo) await repo.close().catch(() => undefined); + repo = undefined; + watchSpy.mockRestore(); + await fs.rm(root, { recursive: true, force: true }); + }); + + it('passes `atomic: true` as an OWN key of the options object handed to chokidar.watch()', async () => { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: false }); + await repo.start(); + + expect(watchSpy).toHaveBeenCalledTimes(1); + const [, options] = watchSpy.mock.calls[0] as [string, Record]; + + // The behaviour this pin exists to protect: `atomic` must be an OWN key + // of the call-site options object, not merely absent-and-coincidentally + // `true` after chokidar's internal merge (see file header). + expect(Object.prototype.hasOwnProperty.call(options, 'atomic')).toBe(true); + expect(options.atomic).toBe(true); + }); + + // Positive control (#12696 ablation) — a SEPARATE case so it runs to + // completion, and so its verdict, on its own assertions, is independent of + // whatever the case above does. `usePolling` is unconditionally declared + // today and untouched by this card's fix; it must stay green under the + // ablation that removes the explicit `atomic`. If it ever went red too, + // the spy/harness would be the suspect, not the `atomic` declaration. + it('[control] passes `usePolling: true` as an OWN key of the same options object', async () => { + repo = new FileSystemRepository({ root, org: 'system', disableWatch: false }); + await repo.start(); + + expect(watchSpy).toHaveBeenCalledTimes(1); + const [, options] = watchSpy.mock.calls[0] as [string, Record]; + + expect(Object.prototype.hasOwnProperty.call(options, 'usePolling')).toBe(true); + expect(options.usePolling).toBe(true); + }); +});