Adopt Explicit Resource Management for using/dispose lifecycle - #950
Merged
Conversation
…de >=24.11 Add disposableListener() and mkdtempDisposable() so callers can register event listeners and temp directories as `using`-compatible resources. Fix raceWithTimeout() leaking its timer when the raced promise rejects (the timer only cleared on the timeout-wins and resolve paths before), and simplify the implementation via Promise.withResolvers()/try-finally. engines.node now requires >=24.11.0, the runtime baseline this package family targets going forward.
…/extract zip() left its output stream open when finalize() failed, and its error listener was registered too late to catch failures that happen while finalize() is still pending. unzip() never listened for the source stream's errors, since .pipe() doesn't forward them to the destination. Both now guarantee cleanup via try/finally, and unzip() waits for the extractor's 'close' event (all entries written) rather than 'finish' (input fully read), which previously could resolve before extraction of the last entries actually completed. engines.node now requires >=24.11.0.
…ableStack server.close() and clearTimeout() calls were duplicated across four exit paths (success, OAuth error, timeout, listen failure), making it easy for a new exit path to miss one. Register both once via AsyncDisposableStack and dispose from every exit point; closeAllConnections() is called alongside close() so lingering keep-alive connections don't delay shutdown. The 404 fallback path intentionally does not dispose, since it means the callback is still awaited. engines.node now requires >=24.11.0.
SuppressedError (thrown when a using-scoped body error and a disposal error occur together) hides the real cause behind a generic message. unwrapSuppressedError() recursively flattens it into the underlying causes so CLI error handlers can print them instead of the placeholder text.
…nup timing Display and Lanes now implement Symbol.dispose (delegating to close(), which is deprecated in favor of `using`). deal() switches its lanes variable to `using` and restructures the completion wait from `return new Promise(...)` to an awaited Promise.withResolvers() pair, so dispose runs after the worker loop actually finishes rather than racing ahead of it — returning early would have disposed lanes (and closed its timer/SIGINT handler) before completion, and would have skipped cleanup entirely if setup() threw. engines.node now requires >=24.11.0.
close() re-armed once('exit', ...) on every call; a second call after the
child had already exited would never resolve, since 'exit' had already
fired and won't fire again. It's now memoized behind a single Promise
and short-circuits when exitCode/signalCode is already set.
send() is switched to its callback form and gated on proc.connected:
without a callback, a failed send on a closed IPC channel emits an
unhandled 'error' event that crashes the parent process, and the
previous send()===false fallback also fired on backpressure (message
still delivered) — sending an unwanted SIGTERM and skipping the child's
graceful cleanup.
Symbol.asyncDispose delegates to the now-idempotent close(), which is
deprecated in favor of `await using`.
engines.node now requires >=24.11.0.
…25.5.0 Also unifies devDependencies/peerDependencies on the same version — they previously diverged (25.3.0 / 25.2.1), which produced a yarn peer dependency warning on every install.
Both delegate to the existing (idempotent) flush(), so buffered rows are never lost when a caller writes `await using sheet = ...` instead of calling flush() explicitly. engines.node now requires >=24.11.0.
compareStreams()'s error handler rejected without destroying either stream, so the stream that didn't error stayed open. compareFiles() (the only caller, which creates both streams) now also wraps the call in try/finally so cleanup runs regardless of which stream failed. engines.node now requires >=24.11.0.
…at 25.5.0 Also unifies devDependencies/peerDependencies on the same version — they previously diverged (25.3.0 / 25.2.1), which produced a yarn peer dependency warning on every install.
The per-URL worker created its ChildProcessManager and closed it with an explicit call at the end of the function body — a statement that never runs if ready(), each(), or the caller's each() callback throws. That left the child process (and the Chromium instance it launched) running after the batch reported an error. Switching to `await using` guarantees disposal on every exit path, including exceptions. engines.node now requires >=24.11 and puppeteer is aligned to 25.5.0 (devDependencies/peerDependencies previously diverged at 25.3.0/25.2.1).
…edError runCli() switches its Lanes instance to `using`, replacing the try/finally wrapper that manually called lanes.close() — cleanup now covers every exit path without the extra nesting. Error lines route through unwrapSuppressedError() so a disposal failure that races with a using-scoped body error surfaces its real cause instead of a generic "suppressed" message. cli.spec.ts's three mkdtemp/rm blocks switch to @d-zero/shared's mkdtempDisposable(). engines.node now requires >=24.11.0. @d-zero/cli-core is now a dependency.
engines.node now requires >=24.11.0.
… at 25.5.0 Also unifies devDependencies/peerDependencies on the same version — they previously diverged (25.3.0 / 25.2.1), which produced a yarn peer dependency warning on every install.
getAnchorList() never released the ElementHandles returned by page.$$(),
leaking a CDP remote-object reference per anchor on every scrape. Release
is deferred until the underlying work() promise actually settles (not
tied to getAnchorList()'s own return), since a timed-out run leaves
resolveAnchor() still running against those handles in the background —
disposing them earlier would break the in-flight CDP calls.
Pins tsconfig target to es2024: `@retryable` decorates a private method
(#fetchData), and vitest's esbuild transform emits invalid output for
that combination at target ESNext ("Private field '#fetchData' must be
declared in an enclosing class"). tsc itself has no issue with it — only
vitest's esbuild-based transform does — so this package stays on the
base target pending an esbuild fix, while the rest of the repo uses the
root's ESNext target.
engines.node now requires >=24.11.0.
The response listener registered per page was only removed at the end of the function body — if beforePageScan() or scrollAllOver() threw, the listener stayed attached. Since this page is reused for the next URL within the same child process, that leak accumulated one listener per failure. `using` now guarantees removal on every exit path. CLI error output is unwrapped via unwrapSuppressedError() so a disposal failure racing with a using-scoped body error surfaces its real cause. engines.node now requires >=24.11.0 and puppeteer is aligned to 25.5.0.
…essedError Browser and Page were closed with explicit calls in try/finally blocks; `await using` replaces both, and puppeteer 25.5.0 is required for the Browser/Page Symbol.asyncDispose type declarations that makes this compile (25.3.0's declarations predate their @public export). cli.ts's --out file handle is now `await using`, guaranteed to close even if runBatch() throws before the previous end()-based flush ran — process.stdout is left alone since the file handle is undefined when --out isn't given. Its onError callback unwraps SuppressedError so a dispose failure racing with a body error surfaces its real cause. engines.node now requires >=24.11.0. @d-zero/cli-core is now a dependency (used for unwrapSuppressedError).
…eer at 25.5.0 Also unifies devDependencies/peerDependencies on the same version — they previously diverged (25.3.0 / 25.2.1), which produced a yarn peer dependency warning on every install.
scenario2.ts registered a fresh page.on('console', ...) listener inside
each iteration of its selector loop without ever removing it, so
listeners accumulated once per selector on every scan. `using` with
@d-zero/shared's disposableListener() releases each one at the end of
its own iteration.
engines.node now requires >=24.11.0 and puppeteer is aligned to 25.5.0
(devDependencies/peerDependencies previously diverged at 25.3.0/25.2.1).
Delegates to the underlying SheetTable's asyncDispose (flush), so `await using reporter = ...` doesn't lose buffered rows. engines.node now requires >=24.11.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adopt TC39 Explicit Resource Management (
using/await using,Symbol.dispose/Symbol.asyncDispose,DisposableStack/AsyncDisposableStack) across the repo, now that the supported runtime is Node.js >=24.11 with native support.close()/destroy()methods are kept and marked@deprecated; newSymbol.dispose/Symbol.asyncDisposeimplementations delegate to the same internal logic (Display,Lanes,ProcTalk,ChildProcessManager).Sheet.flush()) getSymbol.asyncDisposeadded without deprecating them.@d-zero/shared/disposable-listener(wrapson/off-style listeners asDisposable) and@d-zero/shared/mkdtemp-disposable(temp directory asAsyncDisposable).@d-zero/cli-coregainsunwrapSuppressedError()to flattenSuppressedError(thrown when ausing-scoped body error and a disposal error occur together) so CLI error output doesn't hide the real cause behind a generic message."engines": { "node": ">=24.11.0" }(backlog-projectspreviously required>=22.1.0).tsconfig.jsontargetsESNextfor nativeusing/await usingemit;beholderpins its owntsconfig.jsontoes2024because vitest's esbuild transform emits invalid output when a decorator (@retryable) wraps a private method at targetESNext(tscitself has no issue with it).puppeteeris bumped to25.5.0across every package that depends on it — 25.3.0's type declarations markSymbol.asyncDispose@internal, which madeawait using browser = await launch(...)andAsyncDisposableStack.use(elementHandle)fail to typecheck; 25.5.0 exports them@public. This also fixes several packages wheredevDependencies/peerDependencieshad diverged (25.3.0vs25.2.1), which produced a yarn peer-dependency warning on every install.Bugs fixed along the way
Several of the
usingconversions fix real resource-leak or crash bugs that existed independently of this refactor:puppeteer-dealer/deal.ts: the child process (and the Chromium it launched) was closed with an explicit statement at the end of the function body, which never ran ifready(),each(), or the caller's callback threw — leaving a zombie Chromium process. Fixed viaawait using.proc-talk:close()re-armedonce('exit', ...)on every call; calling it twice on an already-exited process hung forever. Also,send()without a callback let a closed IPC channel emit an unhandled'error'event that crashed the parent process, and thesend()===falsefallback fired on backpressure too (message still delivered), sending an unwantedSIGTERMand skipping the child's graceful cleanup.dealer/deal.ts:lanes.close()only ran inside the success callback, so a thrownsetup()left theLanesdisplay's timer andSIGINThandler registered.beholder:getAnchorList()never released theElementHandles returned bypage.$$(), leaking a CDP remote-object reference per anchor on every scrape.replicator/a11y-check-scenarios: apage.on(...)response/console listener was only removed at the end of a function or outside a loop, leaking one listener per failure/iteration.filematch:compareStreams()'s error handler rejected without destroying either stream, leaving the non-erroring stream open.fs:zip()left its output stream open whenfinalize()failed; its'error'listener was also registered too late to catch failures duringfinalize().unzip()never listened for the source stream's errors (.pipe()doesn't forward them), and resolved on'finish'(input fully read) instead of'close'(all entries written), which could resolve before extraction of the last entries completed.shared/race-with-timeout.ts: the timer wasn't cleared when the raced promise rejected (only the resolve/timeout paths reachedclearTimeout).google-auth:waitForAuthCode'sserver.close()/clearTimeout()calls were duplicated across four exit paths; consolidated viaAsyncDisposableStack.Documentation
proc-talk/README.mdreferenced adispose()method that never existed onProcTalk(onlyclose()/Symbol.asyncDispose) — corrected to the actual API and updated to theawait usingpattern.dealer/README.mdtold readers to call the now-deprecatedclose()as a required step — updated to recommendusing.cli-core/README.mdandshared/README.mddocument the new public exports.Test plan
NX_WORKSPACE_ROOT_PATH=<worktree> yarn build— all 29 projectsyarn test— 160 files / 1989 tests passingyarn lint— clean (11 pre-existing warnings, unrelated to this change)/code-review medium,/qa-engineer,/product-managerreview cycles completed; findings addressed (see commit history)