Conversation
Add picofuzz_repeat parameter to the reusable workflow and set it to 1 for jampy to speed up debugging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe socket closing mechanism is converted from synchronous to asynchronous. The Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Socket as Socket Instance
participant EventListener as Event Listener
participant Timeout as Timeout Handler
Caller->>Socket: await close()
activate Socket
Socket->>Socket: Create completion Promise
Socket->>Timeout: Set 5-second timeout
activate Timeout
Socket->>EventListener: Register 'close' event listener
activate EventListener
Socket->>Socket: Call end() on socket
alt Socket closes within 5 seconds
Socket-->>EventListener: 'close' event emitted
EventListener->>Socket: Resolve completion Promise
Socket->>Timeout: Clear timeout
deactivate Timeout
else Timeout expires
Timeout->>Socket: Force destroy socket
Socket->>Socket: Resolve completion Promise
end
deactivate EventListener
deactivate Socket
Caller->>Caller: Continue after close completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@dakk AFAICT, picofuzz sends I'll patch it on our side to destroy the socket after some timeout, but might be something to look into in jampy as well. |
End the socket first, then wait up to 5 seconds for it to close before forcefully destroying it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ok thank you for the debugging and for the workaround; I've also opened the issue on jampy |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
picofuzz/index.ts (1)
54-58:⚠️ Potential issue | 🟠 Major
process.exit(1)incatchbypasses thefinallyblock's async cleanup.At line 56,
process.exit(1)terminates the process immediately without waiting for theawait socket.close()at line 58. Per Node.js documentation,process.exit()does not allow pending async operations infinallyblocks to complete.🔧 Suggested fix
} catch (error) { console.error("Error:", error); - process.exit(1); + process.exitCode = 1; } finally { await socket.close(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@picofuzz/index.ts` around lines 54 - 58, The catch block currently calls process.exit(1), which prevents the async finally block (await socket.close()) from running; change the behavior to set process.exitCode = 1 instead of calling process.exit(1) (or alternatively await socket.close() inside the catch before exiting) so that the finally block can complete its async cleanup; update the catch surrounding code that references process.exit/process.exitCode and ensure socket.close() is still awaited in the finally block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@picofuzz/socket.ts`:
- Around line 97-109: In the async close() method, add a fast-path check for an
already destroyed socket by inspecting this.socket.destroyed (or
socket.destroyed after assigning const socket = this.socket) and return
immediately if true; otherwise proceed with the current logic but avoid calling
socket.end() when destroyed so you won't hang waiting for the 'close' event and
forced 5s timeout.
---
Outside diff comments:
In `@picofuzz/index.ts`:
- Around line 54-58: The catch block currently calls process.exit(1), which
prevents the async finally block (await socket.close()) from running; change the
behavior to set process.exitCode = 1 instead of calling process.exit(1) (or
alternatively await socket.close() inside the catch before exiting) so that the
finally block can complete its async cleanup; update the catch surrounding code
that references process.exit/process.exitCode and ensure socket.close() is still
awaited in the finally block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e99da258-3e44-4492-8767-6b17f65153fa
📒 Files selected for processing (2)
picofuzz/index.tspicofuzz/socket.ts
This reverts commit d7d8d68.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
picofuzz/socket.ts (1)
97-119:⚠️ Potential issue | 🟡 MinorAdd a fast-path for already-destroyed sockets to avoid an unnecessary 5s wait.
At Line 97,
close()can stall for the full timeout ifsocket.destroyedis alreadytrue, because a new'close'event is not expected in that state.🔧 Proposed fix
async close() { const socket = this.socket; + if (socket.destroyed) { + return; + } await new Promise<void>((resolve) => { let timeout: ReturnType<typeof setTimeout> | null = null; // resolve promise when socket is fully closed. socket.once("close", () => { if (timeout !== null) { clearTimeout(timeout); } resolve(); }); // send FIN to the socket socket.end(); // when the other end does not terminate as well // we forcefully destroy timeout = setTimeout(() => { socket.destroy(); resolve(); }, 5000); }); }For Node.js net.Socket: if socket.destroyed is already true, does calling socket.end() emit a new 'close' event, or can it result in ERR_STREAM_DESTROYED / no new close event?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@picofuzz/socket.ts` around lines 97 - 119, The close() method can hang waiting for a 'close' event when this.socket.destroyed is already true; add a fast-path at the start of close() that checks if socket.destroyed and immediately return (resolve) without calling socket.end() or installing the timeout/once handler. Otherwise only call socket.end(), attach the socket.once('close') handler and set the timeout as before; reference the close() function, this.socket, socket.destroyed, socket.end(), socket.once('close') and the timeout logic to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@picofuzz/socket.ts`:
- Around line 97-119: The close() method can hang waiting for a 'close' event
when this.socket.destroyed is already true; add a fast-path at the start of
close() that checks if socket.destroyed and immediately return (resolve) without
calling socket.end() or installing the timeout/once handler. Otherwise only call
socket.end(), attach the socket.once('close') handler and set the timeout as
before; reference the close() function, this.socket, socket.destroyed,
socket.end(), socket.once('close') and the timeout logic to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b67c76f-a4f8-4b23-9e81-046d8baf8e22
📒 Files selected for processing (1)
picofuzz/socket.ts
tomusdrw
changed the title
2 participants
Summary
picofuzz_repeatinput parameter to the reusable picofuzz workflow (default: 10, preserving existing behavior)picofuzz_repeat: 1for the jampy workflow to speed up debuggingPICOFUZZ_REPEATenv var to override the default repeat countTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit