diff --git a/src/commands/demo.ts b/src/commands/demo.ts index 520ef4c5..b5c0998a 100644 --- a/src/commands/demo.ts +++ b/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 => { // 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 => { git(['config', 'user.name', 'CommitLore Demo'], tmpDir); git(['config', 'user.email', 'demo@commitlore.example'], 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); diff --git a/test/demo.test.ts b/test/demo.test.ts index 562b2071..993bf3c3 100644 --- a/test/demo.test.ts +++ b/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(); + 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