From f93e1f6611a7d3432a00aa4af3cd5d1e5d29d17b Mon Sep 17 00:00:00 2001 From: Smilewithoutfalling <91455578+Smilewithoutfalling@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:01:00 +0800 Subject: [PATCH] fix(dry-run): thread { dryRun } into the queue lock, so a preview writes nothing `--dry-run` promises no changes made. On a fresh self-mode clone one write still gets through: an empty `/.teamai/locks/` is created and left behind. It is only the directory, not a lock file, but it is a real filesystem change and the suite can see it -- `dry-run-load-path.test.ts` declares it as a tolerated entry for `pull` today, which is the honest way of saying the preview is not clean. The mechanism is already on main: #866 gave `acquireLock` an `options.dryRun` that reads the lock state instead of taking it, and `pull.ts:1873`, `push.ts:784` and `push.ts:839` pass it. The queue lock does not: learnings-publish.ts:75 syncLock === null || await acquireLock(syncLock) <- not passed learnings-publish.ts:93 listPendingForInstall(localConfig) <- not passed pending-learnings.ts:77 withQueueLock(...) -> acquireQueueLock(home) <- not passed pending-learnings.ts:66 if (await acquireLock(lockPath)) <- takes the real lock `pull` counts the queue so it can report how many learnings it would publish, and counting takes the queue lock, because the queue owns it. Taking that lock is a write: `acquireLock` ensures the lock parent (`update.ts:404`) and `releaseLock` removes the lock FILE but not that directory (`update.ts:464`), so the directory outlives the command. This threads the flag down. Three signatures gain an optional `{ dryRun?: boolean } = {}` and forward it; existing callers (`migrate.ts:413`, `migrate.ts:891`, and the `withQueueLock` inside `savePendingLearning`) send `{ dryRun: undefined }` and are unchanged. `readPendingForInstall` deliberately does not get the parameter: its only caller is behind `if (locked && !dryRun)` (`learnings-publish.ts:94`), so a preview never reaches it. The test gets stricter -- `pull`'s allowed list goes from `[PULL_LOCK_DIR]` to `[]`. `snapshotTree` records directories as `"/" = 'dir'`, so an empty directory is counted; that is why the entry had to be written at all. Verification: - before: `vitest run src/__tests__/dry-run-load-path.test.ts` -> 22 passed - after: same -> 22 passed, with `pull`'s allowed list empty - mutation: reverting `acquireLock(lockPath, options)` to `acquireLock(lockPath)` turns the case red and names the cause, "appeared": ["home\\.teamai\\locks/"] - no collateral: `lock-atomic` 22/22, `pull-post-checks` 13/13, `pull-placement-reconcile` 2/2; `tsc --noEmit` rc=0 and `oxlint --deny-warnings` rc=0 - the locally-failing files (`pending-learnings`, `git-kind-learnings`, `checkout-refusal-silent`) fail identically on the unmodified base: POSIX path assertions on Windows and EBUSY from git-worktree fixtures in workers --- src/__tests__/dry-run-load-path.test.ts | 31 ++++++++++++------------- src/utils/learnings-publish.ts | 10 ++++++-- src/utils/pending-learnings.ts | 24 +++++++++++++++---- 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/src/__tests__/dry-run-load-path.test.ts b/src/__tests__/dry-run-load-path.test.ts index 0be031fe5..e95e124c1 100644 --- a/src/__tests__/dry-run-load-path.test.ts +++ b/src/__tests__/dry-run-load-path.test.ts @@ -332,27 +332,26 @@ describe('--dry-run on a fresh self-mode clone (#866)', () => { fs.rmSync(root, { recursive: true, force: true }); }); - // Each command declares exactly which new entries it may leave behind. Both - // start empty: the point of #866 is that a preview writes nothing. + // Each command declares exactly which new entries it may leave behind, and + // both are empty: a preview writes nothing at all. // - // `pull` is allowed one, and it is not this change's. `pull` counts the - // contribution queue so it can report how many learnings it would publish, and - // `publishQueuedLearnings` lists it through `listPendingForInstall` - // (`utils/pending-learnings.ts`), which holds the queue lock — `acquireLock` - // creates the lock's parent, so an install with no `/locks/` - // gets one and keeps it. That call is unchanged here, and identical on the - // base commit; it only became reachable on a fresh self-mode clone once - // detection stopped aborting first (#850). Declared and counted rather than - // filtered out, so any OTHER new entry still fails this test. - const PULL_LOCK_DIR = `${path.join('home', '.teamai', 'locks')}/`; + // `pull` used to be allowed one — the empty `/locks/` its queue + // listing created. `pull` counts the contribution queue so it can report how + // many learnings it would publish, and `publishQueuedLearnings` lists it + // through `listPendingForInstall` (`utils/pending-learnings.ts`), which holds + // the queue lock. Taking that lock is a write: `acquireLock` creates the + // lock's parent, and `releaseLock` removes the lock file but not the + // directory. `dryRun` now reaches that `acquireLock` as it already reached the + // partition locks in `pull` and `push` (#866), so the preview creates neither + // and this list is empty. + const FETCH_HEAD = path.join('app', '.git', 'FETCH_HEAD'); // `git fetch` — which the preview performs on purpose, so that its plan is // based on the same `origin/` the real push would branch from — // leaves its own one-line record behind. It names no ref, changes no working - // tree, and git overwrites it on the next fetch. Same treatment as the entry - // above: declared and counted, so any other new entry still fails this test. - const FETCH_HEAD = path.join('app', '.git', 'FETCH_HEAD'); + // tree, and git overwrites it on the next fetch. Declared and counted, so any + // other new entry still fails this test. const SELF_COMMANDS: Array<[string, () => Promise, string[]]> = [ - ['pull --dry-run', () => pull({ dryRun: true }), [PULL_LOCK_DIR]], + ['pull --dry-run', () => pull({ dryRun: true }), []], ['push --dry-run', () => push({ dryRun: true }), [FETCH_HEAD]], ]; diff --git a/src/utils/learnings-publish.ts b/src/utils/learnings-publish.ts index 51d47ffc4..c978f61e2 100644 --- a/src/utils/learnings-publish.ts +++ b/src/utils/learnings-publish.ts @@ -71,8 +71,11 @@ export async function publishQueuedLearnings( // the learnings stay queued and the run that holds the lock publishes them. // `pull` already holds the lock when it calls this, and the lock is not // reentrant, so it says so instead of deadlocking against itself. + // Under a preview no lock is taken: `acquireLock` reads its state and answers + // what a real run would get instead, so nothing is created and nothing is + // left behind (#866). const syncLock = options.holdsSyncLock ? null : syncLockPath(localConfig); - const locked = syncLock === null || await acquireLock(syncLock); + const locked = syncLock === null || await acquireLock(syncLock, { dryRun: options.dryRun }); try { return await publishUnderSyncLock(localConfig, username, locked, options.dryRun === true); } finally { @@ -90,7 +93,10 @@ async function publishUnderSyncLock( // two commands at once would each queue the same file. if (locked && !dryRun) await queueImportRemnants(localConfig); - const listing = await listPendingForInstall(localConfig); + // `dryRun` travels with it: listing takes the queue lock, and taking a lock is + // a write — the empty `locks/` directory a preview would otherwise leave on an + // install that has none yet (#866). + const listing = await listPendingForInstall(localConfig, { dryRun }); switch (listing.status) { case 'listed': break; diff --git a/src/utils/pending-learnings.ts b/src/utils/pending-learnings.ts index e87ed457b..2588f91dc 100644 --- a/src/utils/pending-learnings.ts +++ b/src/utils/pending-learnings.ts @@ -59,11 +59,22 @@ const QUEUE_LOCK_RETRY_MS = 100; * Take the queue lock of `home`, retrying while another command holds it. * `acquired` is false when it still does after the wait; the caller must * release `lockPath` otherwise. + * + * Under `dryRun` no lock is taken: `acquireLock` reads the lock's state instead + * and reports the verdict a real run would get, so a preview takes exactly what + * the real command would and writes nothing else. Taking this lock is a write + * even when the queue behind it is only read — `acquireLock` creates the + * `locks/` directory that holds it, and `releaseLock` removes the lock file but + * not that directory, so a preview that took it would leave one behind on an + * install with no `locks/` yet (#866). */ -export async function acquireQueueLock(home: string): Promise<{ acquired: boolean; lockPath: string }> { +export async function acquireQueueLock( + home: string, + options: { dryRun?: boolean } = {}, +): Promise<{ acquired: boolean; lockPath: string }> { const lockPath = await queueLockPath(home); for (let attempt = 1; ; attempt++) { - if (await acquireLock(lockPath)) return { acquired: true, lockPath }; + if (await acquireLock(lockPath, options)) return { acquired: true, lockPath }; if (attempt === QUEUE_LOCK_ATTEMPTS) return { acquired: false, lockPath }; await new Promise((resolve) => setTimeout(resolve, QUEUE_LOCK_RETRY_MS)); } @@ -73,8 +84,9 @@ export async function acquireQueueLock(home: string): Promise<{ acquired: boolea export async function withQueueLock( home: string, fn: () => Promise, + options: { dryRun?: boolean } = {}, ): Promise<{ status: 'done'; value: T } | { status: 'busy'; lockPath: string }> { - const { acquired, lockPath } = await acquireQueueLock(home); + const { acquired, lockPath } = await acquireQueueLock(home, options); if (!acquired) return { status: 'busy', lockPath }; try { return { status: 'done', value: await fn() }; @@ -171,10 +183,12 @@ export async function savePendingLearning( * command loaded its config; had init switched the install meanwhile, the queue * would hold what the new install queued, and this one would publish it to the * previous repository. Listed under the queue lock, so nothing the switch - * leaves in the queue is on the list. + * leaves in the queue is on the list. A preview passes `dryRun` through, so + * listing the queue for a count takes no lock and writes nothing. */ export async function listPendingForInstall( localConfig: LocalConfig, + options: { dryRun?: boolean } = {}, ): Promise< | { status: 'listed'; queued: string[] } | { status: 'busy'; lockPath: string } @@ -184,7 +198,7 @@ export async function listPendingForInstall( const changed = await installChanged(localConfig); if (changed) return { status: 'changed' as const, ...changed }; return { status: 'listed' as const, queued: await listPendingLearnings(localConfig) }; - }); + }, options); return locked.status === 'done' ? locked.value : locked; }