Skip to content

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

Open
Smilewithoutfalling wants to merge 1 commit into
Tencent:mainfrom
Smilewithoutfalling:fix/dry-run-queue-lock
Open

Smilewithoutfalling wants to merge 1 commit into
Tencent:mainfrom
Smilewithoutfalling:fix/dry-run-queue-lock

Conversation

@Smilewithoutfalling

@Smilewithoutfalling Smilewithoutfalling commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

--dry-run promises no changes made. On a fresh self-mode clone one write still slips through:
an empty <home>/.teamai/locks/ is created and left behind.

It is only the directory — no partition, no lock file — but it is a real change to the filesystem and
the suite can see it: dry-run-load-path.test.ts declares that directory as a tolerated entry for
pull today. Tolerated is the honest word for it; the preview is not clean.

This is #866's leftover, and it is small — the mechanism is already on main, the call sites just
never passed the argument.

The mechanism exists

acquireLock grew options.dryRun in #866. A preview does not take the lock; it returns the verdict
a real run would get:

// update.ts:382
export async function acquireLock(lockPath?: string, options: { dryRun?: boolean } = {}): Promise<boolean> {
  const resolved = lockPath ?? expandHome(getUpdateLockPath());
  // A preview does not take the lock, because taking one is itself a write:
  // `ensureDir` below creates the lock's parent directory, which a fresh
  // self-mode clone has no partition for yet and which `releaseLock` has no
  // reason to remove — the directory would outlive the command (#866).
  if (options.dryRun) return (await lockState(resolved)) !== 'live';

pull.ts:1873, push.ts:784 and push.ts:839 all pass it. The queue lock does not:

pull.ts:755              publishQueuedLearnings(..., { holdsSyncLock: true, dryRun: options.dryRun })   ✓
learnings-publish.ts:77  publishUnderSyncLock(..., options.dryRun === true)                             ✓
learnings-publish.ts:75  const locked = syncLock === null || await acquireLock(syncLock);   ← dryRun not passed
learnings-publish.ts:93  const listing = await listPendingForInstall(localConfig);          ← dryRun not passed
pending-learnings.ts:77  const { acquired, lockPath } = await acquireQueueLock(home);       ← dryRun not passed
pending-learnings.ts:66  if (await acquireLock(lockPath)) return { acquired: true, ... };   ← takes the real lock
update.ts:404            await ensureDir(path.dirname(resolved));                           // creates locks/
update.ts:464            await fse.remove(resolved);                                        // removes the FILE only

pull only wants to report how many queued learnings it would publish, so it lists the queue — and
listing takes the queue lock, because the queue owns it. Taking that lock is a write. releaseLock
then removes the lock file but not the directory it was created in, so the empty directory stays.

The same gap sits one frame up: learnings-publish.ts:75 calls acquireLock(syncLock) with no options
either. pull short-circuits it with holdsSyncLock: true, but any caller that does not pass
holdsSyncLock still takes a real sync lock under a preview and creates its parent directory.

The fix

Thread dryRun down the chain. Three signatures gain an optional { dryRun?: boolean } = {} and
forward it; the single acquireLock they reach stops writing.

--- a/src/utils/pending-learnings.ts
-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 }> {
-    if (await acquireLock(lockPath)) return { acquired: true, lockPath };
+    if (await acquireLock(lockPath, options)) return { acquired: true, lockPath };

 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);

 export async function listPendingForInstall(
   localConfig: LocalConfig,
+  options: { dryRun?: boolean } = {},
 ): Promise<...> {
-  const locked = await withQueueLock(queueHome(localConfig), async () => { ... });
+  const locked = await withQueueLock(queueHome(localConfig), async () => { ... }, options);
--- a/src/utils/learnings-publish.ts
-  const locked = syncLock === null || await acquireLock(syncLock);
+  const locked = syncLock === null || await acquireLock(syncLock, { dryRun: options.dryRun });
-  const listing = await listPendingForInstall(localConfig);
+  const listing = await listPendingForInstall(localConfig, { dryRun });

Every new parameter is optional and defaults to {}, so a caller that passes nothing sends
{ dryRun: undefined }, if (options.dryRun) is false, and behaviour is unchanged — migrate.ts:413
and migrate.ts:891 call acquireQueueLock / withQueueLock and are untouched, as is the
withQueueLock inside savePendingLearning.

readPendingForInstall deliberately does not get the parameter. Its only caller
(learnings-publish.ts:241, inside queueImportRemnants) sits behind if (locked && !dryRun) at
learnings-publish.ts:94, so it is not reachable under a preview and a flag would be dead weight.

The test gets stricter

dry-run-load-path.test.ts currently declares the directory as a tolerated entry for pull:

-  // `pull` is allowed one, and it is not this change's. ...
-  const PULL_LOCK_DIR = `${path.join('home', '.teamai', 'locks')}/`;
   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]],
   ];

[] is the point of #866 and now holds for pull too: the assertion is
expect({ appeared, vanished }).toEqual({ appeared: allowed, vanished: [] }), and snapshotTree
records directories as "<rel>/" = 'dir', so an empty directory is counted. If the directory comes
back, this case fails — the entry was not decorative, and removing it is the actual claim.

Verified

An end-to-end record is required for a runtime-behavior change (AGENTS.md → ## Code Review Rules),
so here is one, plus the control that makes it mean something.

Unit — vitest run src/__tests__/dry-run-load-path.test.ts

  • Before: 22 passed, with pull's entry tolerated.
  • After: 22 passed, pull's allowed list empty.
  • Mutation: reverting the single line acquireLock(lockPath, options) → acquireLock(lockPath) turns the
    case red, and it names the cause: "appeared": ["home\\.teamai\\locks/"].

Real CLI — node dist/index.js --dry-run pull, against a live git fixture

--dry-run is a global option (src/index.ts:79) and pull's action spreads globalOpts, so the
invocation is teamai --dry-run pull — teamai pull --dry-run is not accepted.

The fixture is the shape the defect needs and nothing else: a fresh self-mode clone, where the partition
does not exist yet. It mirrors setupSelfModeClone in dry-run-load-path.test.ts — app/.teamai/teamai.yaml
with mode: self, app/.teamai/manifest/roles.yaml, a git repo with an origin remote, and HOME
pointing at an otherwise empty home/ carrying .teamai/ and .claude/. The whole tree is hashed before
and after (directories included, recorded as <rel>/), so an empty directory counts as a write.

$ node dist/index.js --dry-run pull          # cwd=app, HOME=home
ℹ [dry-run] Would bootstrap this single-repo project if you are authenticated with github
           (not checked in a dry run): look up your username, write its config and state to
           <home>/.teamai/projects/<slug>, seed tool dirs, inject hooks and register you as a member
ℹ project scope detected, skipped user scope
ℹ [project] No resources to sync
✔ [project] Team repo: single-repo (knowledge on main)
exit status = 0
build exit home/.teamai/locks/ new entries after the run
this PR 0 absent home/.teamai/debug.log
same build, acquireLock(lockPath, options) reverted to acquireLock(lockPath) 0 created home/.teamai/debug.log, home/.teamai/locks/

The second row is the point of the first. Because the control is built from this diff with one line
reverted, the directory appearing there is attributable to that line — and it also rules out the reading
that "no locks/" means the preview never got that far. It did get that far: the same run reports
project scope detected and [project] Team repo: single-repo (knowledge on main), i.e. it reaches the
project scope, which is where publishQueuedLearnings → listPendingForInstall → acquireQueueLock sits.

debug.log is the only other new entry and it is not this change's: it is the logger's own file, written
by every command, and it appears in both rows. Nothing that already existed was rewritten or removed in
either run. The run performs no fetch against its placeholder remote — the debug log shows local work only.

The driver is a local script, not added to this PR: the repo's own vitest.e2e.config.ts site already
spawns dist/index.js, which is what CI's E2E (fork-safe, no credentials) job runs. Say the word and I
will land the driver as scripts/dry-run-e2e.mjs next to scripts/model-routes-e2e.mjs, which is the
existing precedent for a standalone real-CLI check.

Static — tsc --noEmit rc=0; oxlint --deny-warnings --report-unused-disable-directives rc=0.

Locally red, and not attributed to this change — pending-learnings, git-kind-learnings and
checkout-refusal-silent fail identically on the unmodified base: POSIX-hardcoded path assertions on
Windows (\home\user\... vs /home/user/...), and EBUSY from git-worktree fixtures inside vitest
workers. Established by running each file on the unmodified base, not by inspection.

Not touched, on purpose

releaseLock still removes the lock file but not the directory it was created in. Making it symmetric
is tempting and is the wrong end: the comment above acquireLock already states the intended answer —
a preview does not take the lock — and a real run creating <home>/.teamai/locks/ is correct, that
directory is where locks live. releaseLock also has callers that pass a path whose parent existed
first (bootstrap.ts:233, models/switch.ts:66, dashboard-collector.ts:1540); removing a parent
there would let two concurrent commands delete each other's lock directory.

…tes nothing

`--dry-run` promises no changes made. On a fresh self-mode clone one write still
gets through: an empty `<home>/.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: Tencent#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 `"<rel>/" = '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
@jeff-r2026 jeff-r2026 self-assigned this Sep 29, 2026
@github-actions

Copy link
Copy Markdown
  • [P1 blocking] The PR changes runtime dry-run behavior, but the Verified section lists only Vitest, type-checking, and linting. Per ## Code Review Rules, it must include one representative end-to-end/real-CLI invocation and its result before merge.

@Smilewithoutfalling

Copy link
Copy Markdown
Contributor Author

The [P1 blocking] finding is about the description, not the diff — so it is addressed there rather than in a new commit. The head is unchanged at f93e1f6611, and the body's ### Verified section now carries the real-CLI record ## Code Review Rules asks for.

In one line: node dist/index.js --dry-run pull against a live self-mode fixture exits 0 and leaves no home/.teamai/locks/; the same build with acquireLock(lockPath, options) reverted to acquireLock(lockPath) does create that directory. The control is what makes the absence attributable to that single line, instead of to the preview stopping before it reaches publishQueuedLearnings.

CI on this head is green: 12 checks pass, the single skip being E2E (GitHub provider, full surface), which needs credentials a fork PR does not have.

If a fresh automated pass would help, that reviewer re-runs on a push or on a draft→ready transition — and this PR already carries a maintainer as assignee, so its authorization gate would pass. Happy to toggle it if you want one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants