fix(dry-run): thread { dryRun } into the queue lock, so a preview stops creating <home>/.teamai/locks/

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 `<getTeamaiHome>/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 `<getTeamaiHome>/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/<default>` 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<void>, string[]]> = [
-    ['pull --dry-run', () => pull({ dryRun: true }), [PULL_LOCK_DIR]],
+    ['pull --dry-run', () => pull({ dryRun: true }), []],
     ['push --dry-run', () => push({ dryRun: true }), [FETCH_HEAD]],
   ];
 

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;

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<T>(
   home: string,
   fn: () => Promise<T>,
+  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;
 }