src: unlink temporary compile cache file on persist failure

[Hmache]

Title

src: unlink temporary compile cache file on persist failure

Fixes/Refs #N

Fixes #65473

What the issue was

When Node's module compile cache (NODE_COMPILE_CACHE / module.enableCompileCache()) fails to persist a cache entry to disk partway through — after the temporary file has already been created but before it is fully written, closed, and renamed into place — the temporary file and its open file descriptor are leaked instead of being cleaned up.

Under a persistent failure condition (ENOSPC, EDQUOT, EFBIG, or any other error that shows up once the temp file exists), this leaks one zero-byte temporary file and one open file descriptor for every module Node attempts to cache, for the remaining lifetime of the process. The reporter measured a single process leaving 3,424 zero-byte temp files behind in one minute during a real EDQUOT incident, turning a disk-quota failure into inode exhaustion.

Why it happened

The bug is in CompileCacheHandler::Persist() in src/compile_cache.cc. For each compile cache entry, Persist():

1. Calls uv_fs_mkstemp() to create a temporary file (entry->cache_filename + ".XXXXXX"), leaving an open file descriptor in mkstemp_req.result.
2. Calls uv_fs_write() to write the cache header and content to that descriptor.
3. Calls uv_fs_close() to close the descriptor.
4. Calls uv_fs_rename() to rename the temporary file to its final name.

Before this fix, steps 2–4 each checked their own err and, on failure, executed continue to move on to the next cache entry — without closing the descriptor from step 1 if it was still open, and without removing the temporary file created in step 1 if the rename in step 4 never happened or itself failed. The only scope guards present (cleanup_mkstemp, cleanup_write, cleanup_close, cleanup_rename) only call uv_fs_req_cleanup() on the corresponding uv_fs_t request structs; none of them close the file descriptor or unlink the temporary file itself.

How this PR fixes it

Right after uv_fs_mkstemp() succeeds, the patch installs an additional OnScopeLeave guard (cleanup_tmp_file) that:

• Closes mkstemp_req.result if it is still open (tracked with a fd_open bool, cleared right after the existing uv_fs_close() call is attempted, since a descriptor is not retried on close() failure — matching standard POSIX practice).
• Unlinks the temporary file at mkstemp_path unless the rename to the final cache filename has already succeeded (tracked with a renamed bool, set right after uv_fs_rename() succeeds).

Because the guard is a normal C++ scope guard declared inside the for loop body, it runs automatically on every existing continue in that iteration (write failure, close failure, rename failure) without needing separate cleanup code at each call site. The original error handling and Debug() logging are unchanged — only the leaked resources are cleaned up; the loop still moves on to the next cache entry exactly as before, and entry->persisted is only set to true on full success as before.

What isn't covered

This fix addresses only the resource leak (open descriptor + temporary file) on a failed persist. It does not change:

• What happens on a successful persist (unaffected).
• Any range/validation checks for the cache content itself.
• Non-POSIX-specific behavior; the fix uses the same uv_fs_* APIs already used throughout this function, so it applies uniformly across platforms, but the regression test itself only exercises the POSIX (ulimit/RLIMIT_FSIZE) failure path and is skipped on Windows (see below).

This is a full fix for the leak described in #65473, not a partial one — Fixes #65473 is used rather than Refs #65473.

How it was tested

Environment: cloud Linux sandbox (Ubuntu, gcc 13.3.0, Python 3.11.15, 2 CPU cores), built from a fresh shallow clone (--depth 50) of nodejs/node main at commit a3bb551ea70c7e0b1377c5b97d5d186cd66095cd, configured with ./configure --ninja (Release build).

Baseline (unfixed) result — the new regression test run against the unmodified source (fix stashed via git stash push -- src/compile_cache.cc, rebuilt incrementally, ~27s):

AssertionError [ERR_ASSERTION]: expected no leftover files in the compile cache directory, found:
/home/claude/work/node/test/.tmp.0/.compile_cache_dir/v27.0.0-pre-x64-310b2ce3-0/bc140ef7.dEC70X
+ actual - expected

+ [
+   '/home/claude/work/node/test/.tmp.0/.compile_cache_dir/v27.0.0-pre-x64-310b2ce3-0/bc140ef7.dEC70X'
+ ]
- []
...
Failed tests:
out/Release/node .../test/parallel/test-compile-cache-persist-write-failure.js

This confirms the test reproduces the real bug: a temp file matching the uv_fs_mkstemp() .XXXXXX pattern (bc140ef7.dEC70X) is left behind under the compile cache directory.

Fixed result — same test, fix restored, rebuilt incrementally (~27s):

[00:00|% 100|+   1|-   0]: Done
All tests passed.

Broader test results — full test/parallel/test-compile-cache-*.js suite (24 files, including the new one), with the fix in place:

All tests passed.

(24/24 passed: the 23 pre-existing compile-cache tests plus the new regression test.)

./out/Release/cctest --gtest_filter="*CompileCache*" matched no tests (there is no existing cctest coverage for this class), so no C++ unit test regressions apply here.

Formatting/lint/build results:

• python3 tools/cpplint.py --quiet src/compile_cache.cc — exit 0, no findings.
• python3 tools/checkimports.py src/compile_cache.cc — exit 0, no findings.
• clang-format-18 -style=file src/compile_cache.cc — byte-for-byte identical to the committed file (empty diff).
• node tools/eslint/node_modules/eslint/bin/eslint.js --max-warnings=0 --report-unused-disable-directives --concurrency auto test/parallel/test-compile-cache-persist-write-failure.js — exit 0, no findings.
• Full build (./configure --ninja && make -j2) completed with no errors or new warnings attributable to this change; incremental rebuilds after each stash/restore step completed in ~27s each with only compile_cache.o recompiled and node/embedtest/cctest relinked.

Tests that could not be run, and why:

• The full make -j2 test suite (all of test/parallel + test/sequential + test/message, etc.) was not run in full — the sandbox has only 2 CPU cores and running the entire suite would take substantially longer than the targeted subsystem run above. Only the compile-cache subsystem tests (directly relevant to the changed file) and the new regression test were run, per the "smallest relevant test file first, then the appropriate broader suite" guidance — the broader suite here is the compile-cache subsystem, since the change is isolated to CompileCacheHandler::Persist().
• Windows- and macOS-specific behavior was not tested; this sandbox is Linux-only. The fix itself is platform-independent (same uv_fs_* calls used elsewhere in the same function), but the regression test is POSIX-only (common.isWindows skip) because it relies on ulimit -f/RLIMIT_FSIZE, which has no Windows equivalent.
• test-fs-write-sigxfsz.js-style shared-library-mode behavior: the regression test also skips when process.config.variables.node_shared is true, matching the existing convention in test-fs-write-sigxfsz.js, since Node's SIGXFSZ-to-EFBIG translation is not installed in that configuration. This sandbox's build is not shared-library mode, so the primary path was exercised; the shared-library-mode path was not.

Steps to test

# From a nodejs/node checkout at (or rebased onto) commit a3bb551ea70c7e0b1377c5b97d5d186cd66095cd:
git checkout -b test/compile-cache-persist-leak a3bb551ea70c7e0b1377c5b97d5d186cd66095cd
git apply /path/to/0001-src-unlink-temporary-compile-cache-file-on-persist-f.patch

./configure --ninja
make -j$(nproc)

# Run just the new regression test (expect: All tests passed).
python3 tools/test.py --verbose test/parallel/test-compile-cache-persist-write-failure.js

# Run the full compile-cache subsystem suite (expect: All tests passed, 24/24).
python3 tools/test.py --verbose test/parallel/test-compile-cache-*.js

# --- Demonstrate the regression test fails without the fix, and passes with it ---

# 1. Stash only the source fix (keep the new test file in place).
git stash push -- src/compile_cache.cc

# 2. Rebuild (fast incremental rebuild, ~30s) and run the test: expect FAILURE,
#    with a leftover temp file reported under the compile cache directory.
make -j$(nproc)
python3 tools/test.py --verbose test/parallel/test-compile-cache-persist-write-failure.js

# 3. Restore the fix and rebuild.
git stash pop
make -j$(nproc)

# 4. Re-run the test: expect PASS again.
python3 tools/test.py --verbose test/parallel/test-compile-cache-persist-write-failure.js

# --- Lint / format checks ---
python3 tools/cpplint.py --quiet src/compile_cache.cc
python3 tools/checkimports.py src/compile_cache.cc
clang-format-18 -style=file src/compile_cache.cc | diff -u src/compile_cache.cc -
cd tools/eslint && npm ci && cd ../..
node tools/eslint/node_modules/eslint/bin/eslint.js --max-warnings=0 \
  --report-unused-disable-directives --concurrency auto \
  test/parallel/test-compile-cache-persist-write-failure.js

---

Note on process: this patch was prepared with AI assistance (an autonomous Claude agent run) per AGENTS.md in this repository — disclosed via the Assisted-by: Claude commit trailer, alongside a Signed-off-by line for the human author. Per that same policy, this draft should be reviewed, tested independently, and actively maintained by a human contributor before (and after) it is opened as a real pull request; it has not been pushed or opened as a PR by the agent.

[github-actions]

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

• the guide for first-time contributors
• the contribution and automation policies
• the pull request guide
• the AI use policy
• the Code of Conduct

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

[aduh95]

Per that same policy, this draft should be reviewed, tested independently, and actively maintained by a human contributor before (and after) it is opened as a real pull request

There are no sign that this has happened, closing

[bmuenzenmeyer]

Was drafting this too:

Post AI-generated messages directly into pull requests, issues, or project communication channels without direct human review and editing to ensure clarity, accuracy, and respect for collaborator time.