diff --git a/src/__tests__/dry-run-load-path.test.ts b/src/__tests__/dry-run-load-path.test.ts index 0be031fe..e95e124c 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 51d47ffc..c978f61e 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 e87ed457..2588f91d 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; }