fix(demo): report a failed cleanup and stop the writer that most likely raced it

src/commands/demo.ts

@@ -83,6 +83,16 @@ const git = (args: string[], cwd: string): string =>
     },
   }).trim();
 
+/**
+ * Why a cleanup failed, in one line.
+ *
+ * `Error.message` from `fs` already carries the errno and the syscall --
+ * `EACCES: permission denied, rmdir '/tmp/...'` -- which is the part that says
+ * whether the next occurrence is a race, a permission, or a mount.
+ */
+const reasonFor = (error: unknown): string =>
+  error instanceof Error ? error.message : String(error);
+
 /**
  * Runs the demo scenario in a temporary repository.
  *
@@ -101,10 +111,21 @@ export const runDemo = async (opts: DemoOptions = {}): Promise<DemoResult> => {
   // Signal handler for cleanup on interrupt
   const cleanup = (): void => {
     if (tmpDir !== undefined) {
+      const removing = tmpDir;
       try {
-        rmSync(tmpDir, { recursive: true, force: true });
-      } catch {
-        // Best-effort cleanup
+        // `maxRetries` because the failure being handled is a race with a
+        // writer rather than a permanent condition: node retries `EBUSY`,
+        // `EMFILE`, `ENFILE`, `ENOTEMPTY` and `EPERM` for this option, which is
+        // the set a concurrent writer produces.
+        rmSync(removing, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 });
+      } catch (error) {
+        // Reported, never rethrown. This also runs from a signal handler, where
+        // a throw has nowhere to go, and on the crash path it must not mask the
+        // error it is unwinding. What cannot happen again is losing it: the
+        // leftover directory reached CI with no cause attached, and the `catch`
+        // that discarded the errno was the only reason it could not be read
+        // (#1163).
+        process.stderr.write(`commitlore demo: could not remove ${removing}: ${reasonFor(error)}\n`);
       }
       tmpDir = undefined;
     }
@@ -136,6 +157,17 @@ export const runDemo = async (opts: DemoOptions = {}): Promise<DemoResult> => {
     git(['config', 'user.name', 'CommitLore Demo'], tmpDir);
     git(['config', 'user.email', '[email protected]'], tmpDir);
     git(['config', 'commit.gpgsign', 'false'], tmpDir);
+    // A throwaway repository must not start anything that outlives the command.
+    // `git commit` may spawn background maintenance (`gc.auto`,
+    // `maintenance.auto`), and a git process still writing inside the directory
+    // while `rmSync` walks it is the most plausible reading of the one leftover
+    // directory CI has reported: a removal that raced a writer, not one that
+    // never ran (#1163). Written into the repository's own config rather than
+    // passed per invocation, so anything this demo starts later -- `runInit`'s
+    // hooks included -- inherits it, and so the setting is readable on a
+    // directory that outlived a failed cleanup.
+    git(['config', 'gc.auto', '0'], tmpDir);
+    git(['config', 'maintenance.auto', 'false'], tmpDir);
 
     // Create the target file so the path exists
     const targetFullPath = join(tmpDir, targetPath);

test/demo.test.ts

@@ -11,7 +11,7 @@ import { execFileSync } from 'node:child_process';
 import { existsSync, mkdtempSync, readdirSync, rmSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
-import { afterAll, beforeAll, describe, expect, it } from 'vitest';
+import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest';
 
 import { createTestRepo } from './git-fixtures.js';
 import { runDemo } from '../src/commands/demo.js';
@@ -82,6 +82,90 @@ describe('commitlore demo', () => {
     expect(readdirSync(caseRoot)).toEqual([]);
   });
 
+  /**
+   * bug-issue-1163. The crash-cleanup test went red once in CI naming a
+   * leftover `commitlore-demo-*` directory, passed on re-run of the same
+   * commit, and passed on the other node leg of the same run. Nothing said why,
+   * because `cleanup` discarded the `rmSync` error — so the one occurrence
+   * carried no errno, and a race, a permission and a full disk were
+   * indistinguishable from each other and from "the removal never ran".
+   *
+   * The failure is injected rather than provoked: a real race is not reliably
+   * reproducible, and a test that waits for one would be the flake it is meant
+   * to explain. What is pinned here is the reporting — a cleanup that fails
+   * says so, naming the directory and the reason — plus the repository setting
+   * that removes the most plausible writer.
+   */
+  it('reports a cleanup failure instead of discarding it (bug-issue-1163)', async () => {
+    const caseRoot = mkdtempSync(join(demoRoot, 'cleanupfail-'));
+    const stderr: string[] = [];
+
+    vi.resetModules();
+    vi.doMock('node:fs', async (importOriginal) => {
+      const actual = await importOriginal<typeof import('node:fs')>();
+      return {
+        ...actual,
+        default: actual,
+        rmSync: (): never => {
+          throw Object.assign(
+            new Error(`EACCES: permission denied, rmdir '${caseRoot}/injected'`),
+            { code: 'EACCES' },
+          );
+        },
+      };
+    });
+
+    try {
+      const { runDemo: isolated } = await import('../src/commands/demo.js');
+      const spy = vi
+        .spyOn(process.stderr, 'write')
+        .mockImplementation((chunk: unknown): boolean => {
+          stderr.push(String(chunk));
+          return true;
+        });
+
+      let thrown: unknown;
+      try {
+        await isolated({ cwd: userRepo, crashTest: true, tmpRoot: caseRoot });
+      } catch (error) {
+        thrown = error;
+      } finally {
+        spy.mockRestore();
+      }
+
+      // The error being unwound still reaches the caller: reporting the cleanup
+      // failure must not replace the reason the run ended.
+      expect((thrown as Error | undefined)?.message).toContain('simulated crash');
+
+      const reported = stderr.join('');
+      expect(reported).toContain('could not remove');
+      // The two things the CI occurrence lacked: which directory, and why.
+      expect(reported).toContain(caseRoot);
+      expect(reported).toContain('EACCES');
+    } finally {
+      vi.doUnmock('node:fs');
+      vi.resetModules();
+    }
+
+    // Arrival: the injection really did stop the removal, so the assertions
+    // above were made about a cleanup that failed rather than one that never
+    // happened. The directory is also the artifact the next assertion reads.
+    const leftOver = readdirSync(caseRoot);
+    expect(leftOver).toHaveLength(1);
+    const repo = join(caseRoot, leftOver[0] as string);
+
+    // The demo's repository forbids background maintenance, so `git commit`
+    // cannot leave a process writing inside the directory that is about to be
+    // removed — the mechanism this issue's one occurrence is most consistent
+    // with.
+    const config = (key: string): string =>
+      execFileSync('git', ['-C', repo, 'config', '--get', key], { encoding: 'utf8' }).trim();
+    expect(config('gc.auto')).toBe('0');
+    expect(config('maintenance.auto')).toBe('false');
+
+    rmSync(caseRoot, { recursive: true, force: true });
+  });
+
   it('user repository is never written to (safety property)', async () => {
     await runDemo({ cwd: userRepo, tmpRoot: demoRoot });
     // HEAD must be unchanged