fix(transcripts): close readline interface and destroy read stream on error exit#98493
Conversation
|
@clawsweeper review This PR fixes #98467 with a focused source change in |
|
🦞🧹 I asked ClawSweeper to review this item again. |
7132f17 to
ae8fe26
Compare
|
@clawsweeper re-review PR body and branch HEAD have been updated:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
ae8fe26 to
5c8d0e3
Compare
|
Pushed
The remaining CI failures ( |
5c8d0e3 to
6874938
Compare
|
Rebased onto latest The three failing checks on the latest run are all in files this PR does not modify:
These failures persist after rebase, so they appear to be pre-existing on main. I have not expanded the PR scope to fix unrelated files; if a maintainer prefers otherwise, let me know and I can address them in a follow-up. |
|
@clawsweeper re-review Branch rebased onto |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@maintainer team — requesting maintainer override for the three pre-existing CI failures on this PR. All three failing checks are in files this PR does not touch:
I rebased onto the latest The PR scope is intentionally narrow: only
I would prefer not to expand this PR into unrelated gateway/LLM test files. If maintainers agree these are pre-existing main-branch failures, please consider landing this as-is; otherwise, let me know if a follow-up cleanup PR for those files is desired. |
|
Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 6:21 AM ET / 10:21 UTC. Summary PR surface: Source +11, Tests +108. Total +119 across 2 files. Reproducibility: yes. source inspection gives a high-confidence path: current main opens a transcript read stream in the bounded reader and can throw during JSON parsing before explicit stream cleanup. A focused Node probe also confirmed descriptor release is asynchronous unless the stream close is awaited. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land one canonical fix that closes the readline interface, destroys and awaits the read stream close, and keeps the focused Linux-gated fd regression coverage; this PR is now the better landing candidate if CI is resolved or waived. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence path: current main opens a transcript read stream in the bounded reader and can throw during JSON parsing before explicit stream cleanup. A focused Node probe also confirmed descriptor release is asynchronous unless the stream close is awaited. Is this the best way to solve the issue? Yes. The local cleanup in AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5113fbf4cc6a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +11, Tests +108. Total +119 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
6874938 to
1ecf993
Compare
|
@clawsweeper re-review Addressed the P2 test-portability finding:
Verification:
Live HEAD: |
|
🦞🧹 I asked ClawSweeper to review this item again. |
1ecf993 to
64bac61
Compare
|
Thanks @vincentkoc for the quick review and merge! |
…ng throws (#98840) The batch output file download path creates a readline interface over a Readable.fromWeb() response body stream. If JSON.parse throws on a malformed JSONL line, the for-await loop exits via exception but the readline interface and underlying Readable stream were never explicitly closed or destroyed, leaving the HTTP response body stream dangling. Extract the stream reading into , a testable helper that wraps the iteration in a try-finally so both reader.close() and inputStream.destroy() are always called, matching the pattern established in #98493 for the same class of leak.
…ng throws (openclaw#98840) The batch output file download path creates a readline interface over a Readable.fromWeb() response body stream. If JSON.parse throws on a malformed JSONL line, the for-await loop exits via exception but the readline interface and underlying Readable stream were never explicitly closed or destroyed, leaving the HTTP response body stream dangling. Extract the stream reading into , a testable helper that wraps the iteration in a try-finally so both reader.close() and inputStream.destroy() are always called, matching the pattern established in openclaw#98493 for the same class of leak.
…ng throws (openclaw#98840) The batch output file download path creates a readline interface over a Readable.fromWeb() response body stream. If JSON.parse throws on a malformed JSONL line, the for-await loop exits via exception but the readline interface and underlying Readable stream were never explicitly closed or destroyed, leaving the HTTP response body stream dangling. Extract the stream reading into , a testable helper that wraps the iteration in a try-finally so both reader.close() and inputStream.destroy() are always called, matching the pattern established in openclaw#98493 for the same class of leak.
What Problem This Solves
TranscriptsStore.readUtterancesFromDir(src/transcripts/store.ts:199-218) streamstranscript.jsonlthrough acreateReadStream+readline.createInterfacepair whenmaxUtterancesis set, but it never closes the interface or destroys the stream. IfJSON.parsethrows inside thefor awaitloop, the function propagates the error while leaving the underlying file descriptor open. Over time this leaks fds in long-running gateway processes.Changes
src/transcripts/store.ts: extract thecreateReadStreamresult into a namedstream, wrap thefor awaitloop intry/finally, and calllines.close()andstream.destroy()infinally. Whenstream.destroy()does not immediately close the fs handle, await the stream'scloseevent before returning.src/transcripts/store.test.ts: add regression tests for missing-file handling, normal JSONL reads, tail truncation, and fd-leak-free behavior on both the malformed-JSON error path and the happy path.Real behavior proof
BEFORE (current
main, no fix applied)Driving the real
TranscriptsStore.readUtterancesFromSessionDiragainst a malformed JSONL file leaks the transcript fd:NEGATIVE CONTROL
Reverting only the source change in
src/transcripts/store.tswhile keeping the new tests makes the fd-leak regression test fail:AFTER (this branch)
The same local fd-count repro and the new unit tests pass with zero leaked fds:
Evidence
TranscriptsStore.readUtterancesFromDirnow closes the readline interface and destroys the read stream (awaiting close) when the streaming JSONL parser exits, fixing a file descriptor leak on malformed lines./home/0668000666/0668000666/AI/OpenClaw/new_open_claw, branchfix/transcripts-store-fd-leak, baseopenclaw/main @ f55abc0606, live HEAD68749382de.leaked transcript.jsonl fds: 0after a malformed-line parse error; source change is 1 file, ~10 net LoC.JSON.parsethrows or when the loop exits normally.lines.close(),stream.destroy(), awaitingclose) are platform-agnostic Node.js APIs.Tests and validation
src/transcripts/store.test.tswith 5 tests covering:transcript.jsonlreturns[]maxUtteranceskeeps only the tailtry/finallycleanup makes the fd-leak test fail.Risk checklist
src/transcripts/store.ts), 1 new test file.pnpm tsgo:corepasses.pnpm tsgo:core:testfails on pre-existing errors insrc/gateway/server-cron-notifications.test.ts,src/gateway/server-cron.test.ts, andsrc/llm/providers/anthropic.test.tsthat are unrelated to this change (verified onopenclaw/main @ f55abc0606).CI status
The latest CI run on this PR shows three failing checks. All three failures are in files this PR does not touch:
check-lint:typescript(no-base-to-string)insrc/gateway/server-cron.test.ts:626,676.check-test-types: tuple-length and type mismatch errors insrc/gateway/server-cron-notifications.test.ts,src/gateway/server-cron.test.ts, andsrc/llm/providers/anthropic.test.ts.checks-node-compact-small-whole-3: assertion failure insrc/llm/providers/mistral.test.ts:298.I rebased the branch onto the latest
openclaw/main @ f55abc0606; these same failures persist on main. They appear to be pre-existing CI failures unrelated to the transcript store cleanup change, so I have not expanded this PR's scope to address them.Related