Add handleRouteError utility and improve error handling#1010
Add handleRouteError utility and improve error handling#1010philiplehmann wants to merge 4 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes HTTP route error handling with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant RouteHandler
participant ChildProc as ChildProcess
participant StdoutBuf as StdoutBuffer
participant StderrAgg as StderrAggregator
participant ErrorHelper as handleRouteError
Client->>Server: POST /pdftk/... (stream/file)
Server->>RouteHandler: dispatch request
RouteHandler->>ChildProc: spawn pdftk, write stdin (streamInputToWriteable)
ChildProc->>StdoutBuf: stream stdout -> accumulate
ChildProc->>StderrAgg: stream stderr -> collect & forward to process.stderr
par wait for completion
ChildProc-->>RouteHandler: exit(code)
StdoutBuf-->>RouteHandler: stdout buffer ready
end
alt exit code == 0
RouteHandler->>Client: respond 200 with stdout buffer
else non-zero exit
RouteHandler->>ErrorHelper: handleRouteError(res, Error(with stderr), "pdftk failed")
ErrorHelper->>Client: destroy connection or send 500 with message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/stream/src/lib/stream-child-process-to-buffer.ts (1)
11-15: Consider avoiding unnecessary Buffer copy.The
dataevent on stderr emitsBufferby default forChildProcessWithoutNullStreams. UsingBuffer.from(chunk)creates a copy. You can pushchunkdirectly if it's guaranteed to be a Buffer, or useBuffer.isBuffer(chunk)to conditionally copy.Suggested change
child.stderr.on('data', (chunk) => { - const stderrChunk = Buffer.from(chunk); + const stderrChunk = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); stderrChunks.push(stderrChunk); process.stderr.write(stderrChunk); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/stream/src/lib/stream-child-process-to-buffer.ts` around lines 11 - 15, The stderr 'data' handler currently copies every chunk via Buffer.from(chunk) which is unnecessary for ChildProcessWithoutNullStreams where chunks are Buffers; update the handler for child.stderr.on('data', (chunk) => { ... }) to avoid the copy by pushing chunk directly into stderrChunks and writing chunk to process.stderr when Buffer.isBuffer(chunk) is true, and only call Buffer.from(chunk) as a fallback when it's not a Buffer—this keeps the symbols stderrChunks, the child.stderr 'data' listener, and process.stderr.write intact while removing the redundant Buffer allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts`:
- Around line 38-45: The exit listener is being attached too late causing
exitPromise to possibly never resolve; create and attach the exit listener
immediately after the child process is spawned (i.e., set up exitPromise via
child.on('exit') before calling streamInputToWriteable or awaiting
streamToBuffer(child.stdout)), so move the exitPromise/child.on('exit') setup
above the stdin-writing/streamToBuffer calls (and keep using the same
exitPromise variable); ensure you still await exitPromise later and optionally
remove listeners/handle 'error' if needed for cleanup.
---
Nitpick comments:
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts`:
- Around line 11-15: The stderr 'data' handler currently copies every chunk via
Buffer.from(chunk) which is unnecessary for ChildProcessWithoutNullStreams where
chunks are Buffers; update the handler for child.stderr.on('data', (chunk) => {
... }) to avoid the copy by pushing chunk directly into stderrChunks and writing
chunk to process.stderr when Buffer.isBuffer(chunk) is true, and only call
Buffer.from(chunk) as a fallback when it's not a Buffer—this keeps the symbols
stderrChunks, the child.stderr 'data' listener, and process.stderr.write intact
while removing the redundant Buffer allocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d44bb9b9-5131-4ddf-87d4-39d57b5ddcd9
📒 Files selected for processing (7)
apps/pdftk/src/main.tsapps/pdftk/src/test/pdftk.spec.tsapps/tesseract/src/main.tslibs/http/error/src/index.tslibs/http/error/src/lib/handle-route-error.tslibs/stream/src/lib/stream-child-process-to-buffer.tslibs/test/request/src/lib/test-request.ts
philiplehmann
force-pushed
the
bugfix/ci-improvement
branch
from
5785212 to
c1b0fa9
Compare
- Introduce handleRouteError for consistent HTTP error responses - Refactor pdftk and tesseract endpoints to use handleRouteError - Enhance streamChildProcessToBuffer with better stderr capture and exit code checks - Improve test-request utilities with timeout and error handling - Update tests to assert status codes consistently
philiplehmann
force-pushed
the
bugfix/ci-improvement
branch
from
c1b0fa9 to
05a80db
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/stream/src/lib/stream-child-process-to-buffer.ts (1)
21-45:⚠️ Potential issue | 🔴 CriticalUnresolved: attach child completion listeners before the first
await.The window is real here because
streamInputToWriteable()returns immediately for string/Buffer input. A fast-failing child can exit between Line 27 and Line 38, especially after Line 23 kills it on stdin errors, which leavesexitPromisepending forever. Move the completion promise above the stdin write and let it settle onchild.once('error')too.🔧 Example fix
child.stderr.on('error', (error) => { console.error(error); }); + const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => { + child.once('error', reject); + child.once('exit', (code, signal) => { + resolve({ code, signal }); + }); + }); + child.stdin.on('error', (error) => { console.error(error); child.kill(); }); @@ - const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { - child.on('exit', (code, signal) => { - resolve({ code, signal }); - }); - }); - const output = await streamToBuffer(child.stdout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/stream/src/lib/stream-child-process-to-buffer.ts` around lines 21 - 45, The child-process exit listener (exitPromise) is created after awaiting streamInputToWriteable, which can return immediately and allow the child to exit before the promise exists; move creation of the exit/termination promise (currently named exitPromise) to before calling streamInputToWriteable and make it listen for both 'exit' and 'error' (use child.once('exit', ...) and child.once('error', ...) so fast failures or stdin errors settle the promise), then await that promise later after calling streamToBuffer(child.stdout); ensure existing logic still kills the child on stdin errors and that stderrChunks are read as before.
🧹 Nitpick comments (1)
apps/pdftk/src/test/pdftk.spec.ts (1)
81-90: Re-assert the/encryptoutput itself.Lines 81, 107, and 133 now feed the buffered
/encryptresponse straight into/decrypt, so these cases only prove the round-trip. They no longer prove that/encryptactually produced a protected PDF. Add one assertion onencryptedPdfitself, or a wrong-password failure case, so a no-op/encryptimplementation cannot slip through here.💡 Example follow-up
const encryptedPdf = await streamToBuffer(encryptResponse); + expect(encryptedPdf.toString('latin1').includes('/Encrypt')).toBeTruthy(); const [decryptResponse, text] = await testRequest({Also applies to: 107-116, 133-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/pdftk/src/test/pdftk.spec.ts` around lines 81 - 90, The test currently only verifies round-trip by piping encryptedPdf into /decrypt; instead add an assertion that the /encrypt output itself is actually protected: after creating encryptedPdf (from streamToBuffer(encryptResponse)) assert either that encryptedPdf buffer contains PDF encryption indicators (e.g. the bytes/string "/Encrypt" or "/Encrypt" token) or perform an additional testRequest to POST the same encryptedPdf to /decrypt without or with a wrong password and assert it fails (non-200), referencing the existing variables and helpers encryptedPdf, streamToBuffer, testRequest and the /decrypt endpoint so a no-op /encrypt cannot pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts`:
- Around line 21-45: The child-process exit listener (exitPromise) is created
after awaiting streamInputToWriteable, which can return immediately and allow
the child to exit before the promise exists; move creation of the
exit/termination promise (currently named exitPromise) to before calling
streamInputToWriteable and make it listen for both 'exit' and 'error' (use
child.once('exit', ...) and child.once('error', ...) so fast failures or stdin
errors settle the promise), then await that promise later after calling
streamToBuffer(child.stdout); ensure existing logic still kills the child on
stdin errors and that stderrChunks are read as before.
---
Nitpick comments:
In `@apps/pdftk/src/test/pdftk.spec.ts`:
- Around line 81-90: The test currently only verifies round-trip by piping
encryptedPdf into /decrypt; instead add an assertion that the /encrypt output
itself is actually protected: after creating encryptedPdf (from
streamToBuffer(encryptResponse)) assert either that encryptedPdf buffer contains
PDF encryption indicators (e.g. the bytes/string "/Encrypt" or "/Encrypt" token)
or perform an additional testRequest to POST the same encryptedPdf to /decrypt
without or with a wrong password and assert it fails (non-200), referencing the
existing variables and helpers encryptedPdf, streamToBuffer, testRequest and the
/decrypt endpoint so a no-op /encrypt cannot pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fee16f6e-4c53-4fa2-be7e-4ecae37de395
📒 Files selected for processing (7)
apps/pdftk/src/main.tsapps/pdftk/src/test/pdftk.spec.tsapps/tesseract/src/main.tslibs/http/error/src/index.tslibs/http/error/src/lib/handle-route-error.tslibs/stream/src/lib/stream-child-process-to-buffer.tslibs/test/request/src/lib/test-request.ts
✅ Files skipped from review due to trivial changes (2)
- apps/tesseract/src/main.ts
- libs/http/error/src/lib/handle-route-error.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/http/error/src/index.ts
- libs/test/request/src/lib/test-request.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts`:
- Around line 21-24: The stdin 'error' handler on the child (in
stream-child-process-to-buffer.ts) currently logs every error and kills the
child, which reintroduces benign EPIPE failures into the main flow; change the
handler used in the child.stdin.on('error', ...) callback to ignore EPIPE by
checking (error as NodeJS.ErrnoException).code !== 'EPIPE' before logging, but
still call child.kill() afterwards, and add a separate child.on('error', ...)
listener to catch spawn/process creation failures so they don't become unhandled
or leave promises unresolved; ensure this complements the existing
streamInputToWriteable()/finished() error handling and does not rethrow benign
EPIPEs.
- Around line 26-38: The exitPromise currently only listens for child.on('exit')
so a spawn failure emitting 'error' can leave the lifecycle unresolved; change
the exitPromise (the Promise assigned to exitPromise that watches child) to also
listen for child.on('error') and reject (or resolve with an error state) when
that event fires, and remove both listeners after either event to avoid leaks;
specifically update the Promise that creates exitPromise (which references
child) to attach both 'exit' and 'error' handlers and ensure proper cleanup so
Promise.allSettled([inputPromise, outputPromise, exitPromise]) cannot hang when
spawn() fails.
In `@libs/stream/src/lib/stream-child-process.ts`:
- Around line 43-47: The child stdin error handler currently treats EPIPE as
fatal and calls child.kill() and the surrounding catch rethrows EPIPE, which
prevents the later exit-code check (after finished(input.pipe(...))) from
determining real success; update the handler(s) that attach to child.stdin/error
and the catch that rethrows to only suppress EPIPE (do not call child.kill() or
rethrow for EPIPE) so the child is allowed to terminate normally and the
existing exit-code check (the code that inspects child.exitCode / 'exit' event)
can decide success or failure; specifically change the anonymous error listener
on child.stdin and the catch path around finished(input.pipe(...)) to
ignore/error-log EPIPE but otherwise propagate other errors as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b88aef0-0250-4ee9-850b-579912866a56
📒 Files selected for processing (8)
apps/pdftk/src/main.tsapps/pdftk/src/test/pdftk.spec.tsapps/tesseract/src/main.tslibs/http/error/src/index.tslibs/http/error/src/lib/handle-route-error.tslibs/stream/src/lib/stream-child-process-to-buffer.tslibs/stream/src/lib/stream-child-process.tslibs/test/request/src/lib/test-request.ts
✅ Files skipped from review due to trivial changes (1)
- apps/tesseract/src/main.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/http/error/src/index.ts
- libs/http/error/src/lib/handle-route-error.ts
- libs/test/request/src/lib/test-request.ts
| child.stdin.on('error', (error) => { | ||
| console.error(error); | ||
| child.kill(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/stream/src/lib/stream-child-process-to-buffer.ts | head -60Repository: philiplehmann/container
Length of output: 2409
🏁 Script executed:
fd -t f "stream-child-process-to-buffer" | head -5Repository: philiplehmann/container
Length of output: 120
🏁 Script executed:
rg "streamInputToWriteable" --type ts -A 10Repository: philiplehmann/container
Length of output: 5544
🏁 Script executed:
fd -t f "pdftk.spec.ts" | head -5Repository: philiplehmann/container
Length of output: 100
🏁 Script executed:
cat -n libs/stream/src/lib/stream-child-process.ts | head -80Repository: philiplehmann/container
Length of output: 2741
🏁 Script executed:
cat -n apps/pdftk/src/test/pdftk.spec.ts | sed -n '185,200p'Repository: philiplehmann/container
Length of output: 679
🏁 Script executed:
rg "child\.on\('error'" --type tsRepository: philiplehmann/container
Length of output: 49
🏁 Script executed:
rg "streamChildProcessToBuffer\|streamChildProcess" --type ts | grep -v node_modules | head -20Repository: philiplehmann/container
Length of output: 49
🏁 Script executed:
git log --oneline --all -20 -- libs/stream/src/lib/stream-child-process-to-buffer.ts 2>/dev/null | head -10Repository: philiplehmann/container
Length of output: 283
🏁 Script executed:
cat -n libs/stream/src/lib/stream-child-process-to-buffer.ts | sed -n '60,70p'Repository: philiplehmann/container
Length of output: 476
stdin error handler lacks EPIPE filtering—converts benign stream closure into hard failure.
The new code (lines 21-24) removed the EPIPE filter that the previous implementation had. When streamInputToWriteable() calls finished() on a piped stream, it rejects on any stream error including EPIPE. Combined with the .catch() at lines 32-35 and the immediate throw at line 42, a benign early stdin close (EPIPE) will fail the entire operation even if the child process exits with code 0. Preserve the EPIPE filter:
if ((error as NodeJS.ErrnoException).code !== 'EPIPE') {
console.error(error);
}
child.kill();
Also, add a child-level 'error' listener (lines 26-30) to handle spawn failures, otherwise unhandled errors or unresolved promises can occur.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts` around lines 21 - 24,
The stdin 'error' handler on the child (in stream-child-process-to-buffer.ts)
currently logs every error and kills the child, which reintroduces benign EPIPE
failures into the main flow; change the handler used in the
child.stdin.on('error', ...) callback to ignore EPIPE by checking (error as
NodeJS.ErrnoException).code !== 'EPIPE' before logging, but still call
child.kill() afterwards, and add a separate child.on('error', ...) listener to
catch spawn/process creation failures so they don't become unhandled or leave
promises unresolved; ensure this complements the existing
streamInputToWriteable()/finished() error handling and does not rethrow benign
EPIPEs.
| const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { | ||
| child.on('exit', (code, signal) => { | ||
| resolve({ code, signal }); | ||
| }); | ||
| }); | ||
|
|
||
| child.stderr.pipe(process.stderr).on('error', (error) => { | ||
| console.error(error); | ||
| const inputPromise = streamInputToWriteable(input, child.stdin, { end: true }).catch((error) => { | ||
| child.kill(); | ||
| throw error; | ||
| }); | ||
| return streamToBuffer(child.stdout); | ||
| const outputPromise = streamToBuffer(child.stdout); | ||
|
|
||
| const [inputResult, outputResult, exitResult] = await Promise.allSettled([inputPromise, outputPromise, exitPromise]); |
There was a problem hiding this comment.
Handle child-process 'error' as part of the lifecycle.
exitPromise only listens for 'exit'. If spawn() fails, Node emits 'error' instead, which is currently unhandled and can leave Promise.allSettled() waiting on exitPromise. Reject the lifecycle promise on 'error' too.
Suggested fix
- const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => {
- child.on('exit', (code, signal) => {
- resolve({ code, signal });
- });
- });
+ const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => {
+ child.once('error', reject);
+ child.once('exit', (code, signal) => {
+ resolve({ code, signal });
+ });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { | |
| child.on('exit', (code, signal) => { | |
| resolve({ code, signal }); | |
| }); | |
| }); | |
| child.stderr.pipe(process.stderr).on('error', (error) => { | |
| console.error(error); | |
| const inputPromise = streamInputToWriteable(input, child.stdin, { end: true }).catch((error) => { | |
| child.kill(); | |
| throw error; | |
| }); | |
| return streamToBuffer(child.stdout); | |
| const outputPromise = streamToBuffer(child.stdout); | |
| const [inputResult, outputResult, exitResult] = await Promise.allSettled([inputPromise, outputPromise, exitPromise]); | |
| const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => { | |
| child.once('error', reject); | |
| child.once('exit', (code, signal) => { | |
| resolve({ code, signal }); | |
| }); | |
| }); | |
| const inputPromise = streamInputToWriteable(input, child.stdin, { end: true }).catch((error) => { | |
| child.kill(); | |
| throw error; | |
| }); | |
| const outputPromise = streamToBuffer(child.stdout); | |
| const [inputResult, outputResult, exitResult] = await Promise.allSettled([inputPromise, outputPromise, exitPromise]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts` around lines 26 - 38,
The exitPromise currently only listens for child.on('exit') so a spawn failure
emitting 'error' can leave the lifecycle unresolved; change the exitPromise (the
Promise assigned to exitPromise that watches child) to also listen for
child.on('error') and reject (or resolve with an error state) when that event
fires, and remove both listeners after either event to avoid leaks; specifically
update the Promise that creates exitPromise (which references child) to attach
both 'exit' and 'error' handlers and ensure proper cleanup so
Promise.allSettled([inputPromise, outputPromise, exitPromise]) cannot hang when
spawn() fails.
0ae42bf to
daee334
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/stream/src/lib/stream-child-process-to-buffer.ts (1)
27-31:⚠️ Potential issue | 🔴 CriticalReject the lifecycle promise on
childerrors.
exitPromisestill only settles on'exit'. If the process emits'error'instead, Line 42 can wait forever becauseexitResultnever resolves. Attach a paired'error'handler and clean both listeners up after the first event.🛠️ Suggested fix
- const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve) => { - child.on('exit', (code, signal) => { - resolve({ code, signal }); - }); - }); + const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => { + const onError = (error: Error) => { + child.off('exit', onExit); + reject(error); + }; + const onExit = (code: number | null, signal: NodeJS.Signals | null) => { + child.off('error', onError); + resolve({ code, signal }); + }; + child.once('error', onError); + child.once('exit', onExit); + });In Node.js child_process, can a ChildProcess emit 'error' without emitting 'exit' when spawn() or kill() fails, and should completion code listen for both events to avoid hanging?Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/stream/src/lib/stream-child-process-to-buffer.ts` around lines 27 - 31, The Promise exitPromise created for the ChildProcess only resolves on the 'exit' event and ignores 'error', which can leave awaiters (e.g., code awaiting exitResult) hanging if the process emits 'error' but not 'exit'; update the logic that creates exitPromise for the child to listen for both 'exit' and 'error' events, have the 'error' handler reject the promise with the error, and ensure both listeners are removed after either event fires (use a single cleanup function or once handlers) so neither listener leaks or causes a double-settlement; refer to the existing variables/functions child and exitPromise to locate where to add the 'error' handler and cleanup.
🧹 Nitpick comments (1)
apps/pdftk/src/test/pdftk.spec.ts (1)
49-71: Consider not leaving the parallel compression test permanently skipped.Keeping this skipped removes CI coverage for the concurrency path. Prefer moving it to an opt-in suite/job so it still runs regularly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/pdftk/src/test/pdftk.spec.ts` around lines 49 - 71, The skipped parallel compression test (describe.skip('compress parallel')) removes CI coverage for concurrency; modify the test to run conditionally (not permanently skipped) by checking an opt-in flag (e.g., process.env.RUN_LONG_TESTS or RUN_PARALLEL_COMPRESS) at the top of the suite and only call describe.skip when the flag is absent, or move the suite into a separate test file/job that CI can opt into; ensure the logic references the existing suite name 'compress parallel' and uses the same test helpers (streamRequest, streamLength, expectStatusOk, and setup.port) so the behavior and assertions remain unchanged when the opt-in is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/pdftk/src/test/pdftk.spec.ts`:
- Around line 105-117: The test currently decrypts whatever /encrypt returned
without verifying the encrypted payload was actually transformed; before calling
/decrypt assert that the encrypted buffer (encryptedPdf from
streamToBuffer(encryptResponse)) is different from the original plaintext buffer
and does not contain the original PDF plaintext marker (e.g., assert
Buffer.compare(encryptedPdf, originalPdfBuffer) !== 0 and encryptedPdf does not
include the original plaintext string), then proceed to call testRequest for
/decrypt; apply the same added assertion in the other two encrypt scenarios that
create encryptResponse/encryptedPdf so each encrypt test proves the payload was
altered by /encrypt.
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts`:
- Around line 11-19: The stderr data handler populates stderrChunks but the code
only waits on Promise.allSettled([inputPromise, outputPromise, exitPromise]) so
stderr data that arrives after 'exit' can be missed and stderr stream errors are
only console.error'd; add a promise for the stderr stream completion (e.g.,
create stderrPromise that resolves on child.stderr 'end' and rejects on 'error'
and include it in the allSettled array), or replace waiting on exitPromise with
listening for child.on('close') which ensures stdio are drained; also change the
child.stderr 'error' listener to propagate the error (reject stderrPromise)
instead of merely logging so failures are surfaced.
---
Duplicate comments:
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts`:
- Around line 27-31: The Promise exitPromise created for the ChildProcess only
resolves on the 'exit' event and ignores 'error', which can leave awaiters
(e.g., code awaiting exitResult) hanging if the process emits 'error' but not
'exit'; update the logic that creates exitPromise for the child to listen for
both 'exit' and 'error' events, have the 'error' handler reject the promise with
the error, and ensure both listeners are removed after either event fires (use a
single cleanup function or once handlers) so neither listener leaks or causes a
double-settlement; refer to the existing variables/functions child and
exitPromise to locate where to add the 'error' handler and cleanup.
---
Nitpick comments:
In `@apps/pdftk/src/test/pdftk.spec.ts`:
- Around line 49-71: The skipped parallel compression test
(describe.skip('compress parallel')) removes CI coverage for concurrency; modify
the test to run conditionally (not permanently skipped) by checking an opt-in
flag (e.g., process.env.RUN_LONG_TESTS or RUN_PARALLEL_COMPRESS) at the top of
the suite and only call describe.skip when the flag is absent, or move the suite
into a separate test file/job that CI can opt into; ensure the logic references
the existing suite name 'compress parallel' and uses the same test helpers
(streamRequest, streamLength, expectStatusOk, and setup.port) so the behavior
and assertions remain unchanged when the opt-in is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df6ff03b-200a-47cd-8c9c-9c2f8641a0c2
📒 Files selected for processing (5)
apps/pdftk/src/test/pdftk.spec.tslibs/stream/src/lib/stream-child-process-to-buffer.tslibs/stream/src/lib/stream-child-process.spec.tslibs/stream/src/lib/stream-child-process.tslibs/test/server/src/lib/test-container.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/stream/src/lib/stream-child-process.ts
| const encryptedPdf = await streamToBuffer(encryptResponse); | ||
| const [decryptResponse, text] = await testRequest({ | ||
| method: 'POST', | ||
| host: 'localhost', | ||
| port: setup.port, | ||
| path: '/decrypt?password=1234', | ||
| headers: { 'Content-Type': 'application/pdf' }, | ||
| body: encryptedPdf, | ||
| }); | ||
| expectStatusOk(decryptResponse.statusCode); | ||
|
|
||
| expect(text.includes('/Encrypt')).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Encryption tests no longer prove /encrypt actually encrypted the payload.
Right now, these can still pass if /encrypt returns plaintext and /decrypt behaves like a pass-through. Add an assertion on the encrypted buffer before decrypting.
Suggested test hardening
const encryptedPdf = await streamToBuffer(encryptResponse);
+ expect(encryptedPdf.includes(Buffer.from('/Encrypt'))).toBeTruthy();
const [decryptResponse, text] = await testRequest({
method: 'POST',
host: 'localhost',
port: setup.port,
path: '/decrypt?password=1234',Apply the same assertion in all three encrypt scenarios.
Also applies to: 131-143, 157-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/pdftk/src/test/pdftk.spec.ts` around lines 105 - 117, The test currently
decrypts whatever /encrypt returned without verifying the encrypted payload was
actually transformed; before calling /decrypt assert that the encrypted buffer
(encryptedPdf from streamToBuffer(encryptResponse)) is different from the
original plaintext buffer and does not contain the original PDF plaintext marker
(e.g., assert Buffer.compare(encryptedPdf, originalPdfBuffer) !== 0 and
encryptedPdf does not include the original plaintext string), then proceed to
call testRequest for /decrypt; apply the same added assertion in the other two
encrypt scenarios that create encryptResponse/encryptedPdf so each encrypt test
proves the payload was altered by /encrypt.
| child.stderr.on('data', (chunk) => { | ||
| const stderrChunk = Buffer.from(chunk); | ||
| stderrChunks.push(stderrChunk); | ||
| process.stderr.write(stderrChunk); | ||
| }); | ||
|
|
||
| child.stderr.on('error', (error) => { | ||
| console.error(error); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process, can the 'exit' event fire before stderr/stdout streams are fully closed, and should code wait for 'close' or the individual stream completions to guarantee all stderr has been read?
💡 Result:
Yes, the 'exit' event can fire before stderr/stdout streams are fully closed. Official Node.js documentation explicitly states: "When the 'exit' event is triggered, child process stdio streams might still be open." To guarantee all stderr (and stdout) has been read, wait for the 'close' event on the ChildProcess instance, which is emitted after the stdio streams are closed. Alternatively, listen for the 'end' events on the individual stdout and stderr streams (after collecting data chunks from 'data' events), as these signal no more data is available from each stream. The 'close' event is emitted once all stdio streams have ended. In practice, for spawn with default piped stdio: - Listen to child.stdout.on('data') and child.stderr.on('data') to collect output. - Use child.on('close', ...) for confirmation that all streams are done and process has terminated. The 'exit' event only confirms process termination, not stream completion.
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://stackoverflow.com/questions/37522010/difference-between-childprocess-close-exit-events
- 3: https://nodejs.org/docs/latest/api/child_process.html
- 4: https://stackoverflow.com/questions/47769997/can-i-detect-whether-a-child-process-has-finished-io-processing-on-exit
🏁 Script executed:
wc -l libs/stream/src/lib/stream-child-process-to-buffer.tsRepository: philiplehmann/container
Length of output: 123
🏁 Script executed:
cat -n libs/stream/src/lib/stream-child-process-to-buffer.tsRepository: philiplehmann/container
Length of output: 2968
Add stderr stream completion to the settled set.
At line 42, stderrChunks is populated via a 'data' event listener but the code waits on Promise.allSettled([inputPromise, outputPromise, exitPromise]). The 'exit' event fires before stderr/stdout streams are fully drained in Node.js, so stderr data arriving after the process exits will be missed. Additionally, stderr stream errors at line 17 only log to console instead of surfacing, losing error context.
Add a promise for stderr stream completion (e.g., listening for the 'end' event on child.stderr) to the allSettled array, or switch to waiting on child.on('close') which fires after all stdio streams close. Also consider propagating stderr stream errors instead of silently logging them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/stream/src/lib/stream-child-process-to-buffer.ts` around lines 11 - 19,
The stderr data handler populates stderrChunks but the code only waits on
Promise.allSettled([inputPromise, outputPromise, exitPromise]) so stderr data
that arrives after 'exit' can be missed and stderr stream errors are only
console.error'd; add a promise for the stderr stream completion (e.g., create
stderrPromise that resolves on child.stderr 'end' and rejects on 'error' and
include it in the allSettled array), or replace waiting on exitPromise with
listening for child.on('close') which ensures stdio are drained; also change the
child.stderr 'error' listener to propagate the error (reject stderrPromise)
instead of merely logging so failures are surfaced.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/test/server/src/lib/test-container.ts (1)
21-31:⚠️ Potential issue | 🟠 MajorValidate
memorySizeMBbefore converting to bytes.On Line 21 / Line 31, invalid values (
<= 0,NaN, non-integer) can produce an invalid shared memory config and fail container startup at runtime.Proposed fix
export const testContainer = async ({ image, containerPort, healthPath = '/health/readiness', healthPort = containerPort, healthStatusCode = 200, memorySizeMB = 128, env, hook, }: TestContainerProps): Promise<[StartedTestContainer, number]> => { + if (!Number.isInteger(memorySizeMB) || memorySizeMB <= 0) { + throw new Error('memorySizeMB must be a positive integer'); + } + let genericContainer = new GenericContainer(image) .withEnvironment({ ...(env || {}), PORT: String(containerPort) }) .withExposedPorts(containerPort) .withUser('1000:1000') .withLogConsumer((stream) => stream.pipe(process.stdout)) .withWaitStrategy(Wait.forHttp(healthPath, healthPort).forStatusCode(healthStatusCode)) .withSharedMemorySize(memorySizeMB * 1024 * 1024);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/test/server/src/lib/test-container.ts` around lines 21 - 31, Validate the memorySizeMB value from TestContainerProps before using it to compute bytes: ensure memorySizeMB is a finite integer > 0 (reject NaN, <=0, non-integers) and either throw a clear error or coerce to a safe default, then pass memorySizeMB * 1024 * 1024 into genericContainer.withSharedMemorySize; add the check near the start of the function that constructs genericContainer (reference memorySizeMB, TestContainerProps, genericContainer, and withSharedMemorySize) so invalid inputs never produce an invalid shared memory configuration.
🧹 Nitpick comments (1)
apps/pdftk/src/test/pdftk.spec.ts (1)
49-71: Skipped parallel test has a subtle stream consumption issue.When the test is enabled, consuming response streams sequentially in the
forloop (lines 65-69) while all 20 requests complete concurrently may cause backpressure. Early responses could timeout or buffer excessively while waiting for their turn to be consumed.Consider consuming streams concurrently:
♻️ Suggested refactor for concurrent stream consumption
- for (const response of responses) { - expectStatusOk(response.statusCode); - const size1 = await streamLength(response); - expect(stats.size > size1).toBeTruthy(); - } + await Promise.all( + responses.map(async (response) => { + expectStatusOk(response.statusCode); + const size = await streamLength(response); + expect(stats.size > size).toBeTruthy(); + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/pdftk/src/test/pdftk.spec.ts` around lines 49 - 71, The sequential consumption of response streams in the for loop causes backpressure; update the test in pdftk.spec.ts to consume streams concurrently by replacing the for-loop over responses with a Promise.all that maps each response to an async handler which calls expectStatusOk(response.statusCode), awaits streamLength(response) and asserts the compressed size (use the same stats.size check); references: streamRequest (produces responses), streamLength (consumes response stream), expectStatusOk (status assertion). Ensure the concurrent handler still throws/asserts so Promise.all fails the test on any error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@libs/test/server/src/lib/test-container.ts`:
- Around line 21-31: Validate the memorySizeMB value from TestContainerProps
before using it to compute bytes: ensure memorySizeMB is a finite integer > 0
(reject NaN, <=0, non-integers) and either throw a clear error or coerce to a
safe default, then pass memorySizeMB * 1024 * 1024 into
genericContainer.withSharedMemorySize; add the check near the start of the
function that constructs genericContainer (reference memorySizeMB,
TestContainerProps, genericContainer, and withSharedMemorySize) so invalid
inputs never produce an invalid shared memory configuration.
---
Nitpick comments:
In `@apps/pdftk/src/test/pdftk.spec.ts`:
- Around line 49-71: The sequential consumption of response streams in the for
loop causes backpressure; update the test in pdftk.spec.ts to consume streams
concurrently by replacing the for-loop over responses with a Promise.all that
maps each response to an async handler which calls
expectStatusOk(response.statusCode), awaits streamLength(response) and asserts
the compressed size (use the same stats.size check); references: streamRequest
(produces responses), streamLength (consumes response stream), expectStatusOk
(status assertion). Ensure the concurrent handler still throws/asserts so
Promise.all fails the test on any error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91c442ef-ab55-4337-bd5e-3d23cead82ca
📒 Files selected for processing (5)
apps/pdftk/src/test/pdftk.spec.tslibs/stream/src/lib/stream-child-process-to-buffer.tslibs/stream/src/lib/stream-child-process.spec.tslibs/stream/src/lib/stream-child-process.tslibs/test/server/src/lib/test-container.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/stream/src/lib/stream-child-process.spec.ts
- libs/stream/src/lib/stream-child-process.ts
- libs/stream/src/lib/stream-child-process-to-buffer.ts
- Add wait between requests in pdftk tests
philiplehmann
force-pushed
the
bugfix/ci-improvement
branch
from
7e7d51c to
51800e9
Compare
1 participant
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Refactor