fix(bash): delete pre-created snapshot file on creation failure
getOrCreateSnapshot in shell-snapshot.ts pre-creates snapshotPath as an empty temp file and returns null on spawn failure, timeout, or nonzero exit without deleting it, leaving stale files in os.tmpdir()/omp-shell-snapshots/. Track whether snapshot creation succeeded and remove the pre-created file in a finally block on every failure path using fs.rmSync(snapshotPath, { force: true }), with best-effort error suppression so cleanup failures never propagate. Verified with `bunx tsc --noEmit -p packages/coding-agent/tsconfig.json` and `bun test packages/coding-agent/test/shell-snapshot.test.ts` (22 pass).
Closes #4236
This commit is contained in:
@@ -2,6 +2,11 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Delete pre-created shell snapshot file when snapshot creation fails ([#4236](https://github.com/can1357/oh-my-pi/issues/4236))
|
||||
|
||||
|
||||
## [16.3.0] - 2026-07-02
|
||||
|
||||
### Added
|
||||
|
||||
@@ -260,6 +260,7 @@ export async function getOrCreateSnapshot(
|
||||
// Generate and execute snapshot script
|
||||
const script = generateSnapshotScript(shell, snapshotPath, rcFile);
|
||||
|
||||
let succeeded = false;
|
||||
try {
|
||||
const snapshotEnv = sanitizeSnapshotEnv(env);
|
||||
const spawnEnv: Record<string, string> = {};
|
||||
@@ -289,10 +290,19 @@ export async function getOrCreateSnapshot(
|
||||
}
|
||||
scrubSnapshotInPlace(snapshotPath);
|
||||
cachedSnapshotPaths.set(cacheKey, snapshotPath);
|
||||
succeeded = true;
|
||||
return snapshotPath;
|
||||
}
|
||||
} catch {
|
||||
// Snapshot creation failed, proceed without it
|
||||
} finally {
|
||||
if (!succeeded) {
|
||||
try {
|
||||
fs.rmSync(snapshotPath, { force: true });
|
||||
} catch {
|
||||
// best-effort cleanup; force: true ignores ENOENT
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return null;
|
||||
|
||||
@@ -315,4 +315,81 @@ describe("getOrCreateSnapshot", () => {
|
||||
const content = await fs.readFile(snapshotPath!, "utf8");
|
||||
expect(content).toContain(`export __MISE_EXE='${REAL_ECHO}'`);
|
||||
});
|
||||
it("cleans up the empty snapshot file when the shell exits with a non-zero code", async () => {
|
||||
const testRoot = await fs.mkdtemp(path.join(os.tmpdir(), "omp-snap-fail-"));
|
||||
const originalTmpDir = process.env.TMPDIR;
|
||||
process.env.TMPDIR = testRoot;
|
||||
try {
|
||||
const fakeShell = path.join(testRoot, "fail-shell.sh");
|
||||
await fs.writeFile(fakeShell, `#!/bin/sh\nexit 1\n`);
|
||||
await fs.chmod(fakeShell, 0o755);
|
||||
|
||||
const env = { ...process.env, HOME: testRoot };
|
||||
const snapshotDir = path.join(testRoot, "omp-shell-snapshots");
|
||||
|
||||
const snapshotPath = await getOrCreateSnapshot(fakeShell, env);
|
||||
expect(snapshotPath).toBeNull();
|
||||
|
||||
// Verify that no snapshot files were left behind in the isolated tmpdir
|
||||
if (existsSync(snapshotDir)) {
|
||||
const files = await fs.readdir(snapshotDir);
|
||||
expect(files).toEqual([]);
|
||||
}
|
||||
} finally {
|
||||
if (originalTmpDir === undefined) delete process.env.TMPDIR;
|
||||
else process.env.TMPDIR = originalTmpDir;
|
||||
await fs.rm(testRoot, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("cleans up the empty snapshot file when the shell fails to spawn", async () => {
|
||||
const testRoot = await fs.mkdtemp(path.join(os.tmpdir(), "omp-snap-spawn-fail-"));
|
||||
const originalTmpDir = process.env.TMPDIR;
|
||||
process.env.TMPDIR = testRoot;
|
||||
try {
|
||||
const fakeShell = path.join(testRoot, "does-not-exist-shell");
|
||||
|
||||
const env = { ...process.env, HOME: testRoot };
|
||||
const snapshotDir = path.join(testRoot, "omp-shell-snapshots");
|
||||
|
||||
const snapshotPath = await getOrCreateSnapshot(fakeShell, env);
|
||||
expect(snapshotPath).toBeNull();
|
||||
|
||||
if (existsSync(snapshotDir)) {
|
||||
const files = await fs.readdir(snapshotDir);
|
||||
expect(files).toEqual([]);
|
||||
}
|
||||
} finally {
|
||||
if (originalTmpDir === undefined) delete process.env.TMPDIR;
|
||||
else process.env.TMPDIR = originalTmpDir;
|
||||
await fs.rm(testRoot, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("cleans up the empty snapshot file when the shell execution times out", async () => {
|
||||
const testRoot = await fs.mkdtemp(path.join(os.tmpdir(), "omp-snap-timeout-"));
|
||||
const originalTmpDir = process.env.TMPDIR;
|
||||
process.env.TMPDIR = testRoot;
|
||||
try {
|
||||
const fakeShell = path.join(testRoot, "timeout-shell.sh");
|
||||
// Sleep longer than SNAPSHOT_TIMEOUT_MS (2000)
|
||||
await fs.writeFile(fakeShell, `#!/bin/sh\nsleep 3\n`);
|
||||
await fs.chmod(fakeShell, 0o755);
|
||||
|
||||
const env = { ...process.env, HOME: testRoot };
|
||||
const snapshotDir = path.join(testRoot, "omp-shell-snapshots");
|
||||
|
||||
const snapshotPath = await getOrCreateSnapshot(fakeShell, env);
|
||||
expect(snapshotPath).toBeNull();
|
||||
|
||||
if (existsSync(snapshotDir)) {
|
||||
const files = await fs.readdir(snapshotDir);
|
||||
expect(files).toEqual([]);
|
||||
}
|
||||
} finally {
|
||||
if (originalTmpDir === undefined) delete process.env.TMPDIR;
|
||||
else process.env.TMPDIR = originalTmpDir;
|
||||
await fs.rm(testRoot, { recursive: true, force: true });
|
||||
}
|
||||
}, 5000); // increase test timeout to 5s to accommodate the 2s snapshot timeout
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user