From 99e2d52786cd55d069b74bd5f20b099fab1c22ba Mon Sep 17 00:00:00 2001 From: operator Date: Sun, 4 Oct 2026 05:48:04 +0900 Subject: [PATCH] fix(demo): say why a cleanup failed, and stop the writer that most likely raced it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The crash-cleanup test went red once in CI naming a leftover `commitlore-demo-*` directory, passed on a re-run of the same commit, and passed on the other node leg of the same run. Nothing said why: `cleanup` caught the `rmSync` error and discarded it, so the one occurrence carried no errno, and a race, a permission and a full disk were indistinguishable from each other and from a removal that never ran. Three changes, in order of what they are worth: - The failure is reported — the directory and the reason, on stderr, never rethrown. `cleanup` 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. - The demo's repository sets `gc.auto=0` and `maintenance.auto=false`, so `git commit` cannot leave background maintenance writing inside a directory that is about to be removed. That is the mechanism the occurrence is most consistent with, and a throwaway repository should not start one regardless. - `rmSync` gets `maxRetries`, which covers the errno set a concurrent writer produces. What this does not do is prove the cause. The new test injects the failure through a mocked `rmSync` because a real race is not reliably reproducible, so what it pins is the reporting: a cleanup that fails names the path and the errno. The next occurrence will say what this one could not. Limit: the evidence is one CI occurrence that passed on re-run and on the other node leg of the same run, so the cause was never observed Ruled-out: waiting for a reproduction before changing anything | a real removal race is not reliably reproducible, and the discarded errno is the reason the one occurrence could not be read at all Ruled-out: calling the retry a root fix | a retry makes a race survivable without showing that a race is what happened, and the cause is still unobserved Ruled-out: failing the command when cleanup fails | a leftover temporary directory is a leak, and a non-zero exit would make `commitlore demo` fail for something the user cannot act on Warn: the new test injects the failure through a mocked `rmSync`, so it pins the reporting and not the cause; if this goes red again the stderr line carries the errno -- read it before changing anything else Blast: local Undo: easy Certainty: tentative Unverified: whether background maintenance was the writer; the repository config change removes that mechanism and nothing observed it happening Record-Id: r-cleanupreport1163 Provenance: drafted --- src/commands/demo.ts | 38 ++++++++++++++++++-- test/demo.test.ts | 86 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 4 deletions(-) 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