fix: close FileHandles explicitly to prevent Node 26 GC crash#99461
fix: close FileHandles explicitly to prevent Node 26 GC crash#99461999wwx wants to merge 1 commit into
Conversation
…aw#99263) Node 26 promotes GC-finalized FileHandle from a deprecation warning to ERR_INVALID_STATE. Three locations in @openclaw/fs-safe 0.3.0 skip explicit handle.close() when a stream's 'close' event fires (handleClosedByStream / sourceClosedByStream / tempClosedByStream), relying on the stream's autoClose to release the fd. On Node 26 the FileHandle object still needs an explicit .close() call to signal disposal — otherwise the GC finalizer throws ERR_INVALID_STATE. The leak affects the inbound-media path: copyFileFallback pipelines sourceStream (opened on media/inbound/<uuid>.png) through a bounded read stream into a temp write handle. After pipeline() resolves, both the source handle and temp handle are skipped for close because their stream-close flags are true. Fix: - Patch @openclaw/[email protected] to always call handle.close() in finally blocks regardless of stream-close flags, in three affected functions: writeStreamToTempSource, copyFileFallback, and writeZipFileEntry. Double-close is safe (wrapped in .catch()). - Fix gateway-lock.ts: if handle.writeFile() throws, the FileHandle was leaked (no finally block). Move to try/finally with a handle-local variable that is cleared on success and closed in the catch block on error. - Add regression tests verifying copyIn closes source handles. Refs openclaw#99263
github-actions
Bot
added
the
dependencies-changed
Dependency GuardThis PR changes dependency-related files. Maintainers should confirm these changes are intentional. Changed files:
Maintainer follow-up:
|
openclaw-barnacle
Bot
added
the
triage: needs-pr-context
|
Thanks for the contribution. This PR still needs the requested real-behavior proof, and the branch has been idle since that ask. Close this external PR as stalled and unproven: it has had no head/check activity since July 3, 2026 despite a dated real-proof request, remains merge-conflicted, and retains blocking dependency and test defects. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. So I’m closing this for now to keep the review queue honest. Please reopen or open a fresh PR with real-behavior proof (a live run, logs, or a reproducible validation transcript) and it will be reviewed again. Review detailsBest possible solution: If the inbound-media crash still reproduces, open a fresh, narrowly rebased fix that excludes the already-merged gateway-lock work, uses an approved upstream fs-safe release or a synchronized temporary patch, and includes observable Node 26/forced-GC proof. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was supplied for this PR. The linked issue has production Node 26 crash evidence and the patched dependency path plausibly skips explicit closes, but no after-fix runtime proof was posted. Is this the best way to solve the issue? No. Explicit cleanup is plausible, but this branch is not the best mergeable solution because its dependency patch is unsynchronized, its gateway-lock change is superseded, and its tests do not observe the claimed close behavior. Security review: Security review needs attention: The filesystem dependency patch is a supply-chain-sensitive change whose exact patched install state is not locked or independently proven.
AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 99046179a8fe. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
This pull request has been automatically marked as stale due to inactivity. |
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Summary
Node 26 promotes GC-finalized
FileHandlefrom a deprecation warning to a hardERR_INVALID_STATEerror. Three locations in@openclaw/fs-safe0.3.0 skip explicithandle.close()when a streamcloseevent fires (handleClosedByStream/sourceClosedByStream/tempClosedByStream), relying on the stream auto-close to release the fd. On Node 26 theFileHandleobject still needs an explicit.close()call — otherwise the GC finalizer throws.The leak affects the inbound-media path:
copyFileFallbackpipelines a bounded read stream (opened onmedia/inbound/<uuid>.png) into a temp write handle. Afterpipeline()resolves, both handles are skipped for close because their stream-close flags aretrue.Changes
Patch:
@openclaw/[email protected]Removes the
if (!handleClosedByStream)/if (!sourceClosedByStream)/if (!tempClosedByStream)guards beforehandle.close()calls in three functions:writeStreamToTempSource(file-store-boundary.js) — always closehandleafter pipeline, in both success and catch pathscopyFileFallback(root-impl.js) — always closesource.handleandtempHandleinfinally, unconditionallywriteZipFileEntry(archive.js) — always closetempHandleafter pipeline and infinallyDouble-close is safe (all calls are wrapped in
.catch()).Fix:
src/infra/gateway-lock.tsacquireGatewayLockopened aFileHandlewithfs.open(lockPath, "wx")but ifhandle.writeFile()threw, the handle was never closed (nofinallyblock). Fixed by movinghandleto a block-scoped variable that is cleared on success and closed in thecatchblock on error.Tests:
src/infra/fs-safe.test.tsAdded regression tests verifying that
copyInproperly closes sourceFileHandles, both on success and ontoo-largeerror.Test plan
copyInsucceeds and file is readablecopyInwithmaxBytesrejectstoo-largeand source is intactpnpm testpassescheck-package-patchesguard passes with new allowlist entryFixes #99263