feat(ocap-kernel): leverage libp2p v3 features in remote communications#914
feat(ocap-kernel): leverage libp2p v3 features in remote communications#914
Conversation
The "handles multiple simultaneous reconnections to different peers" test was flaky because the fast test backoff (10-50ms) exhausted the default connection rate limit (10/min) in <1s, blocking reconnection for ~60s — right at the edge of the redemption timeout. Fix the test with three changes: - Add maxConnectionAttemptsPerMinute: 500 to prevent rate limiting - Restart both peers in parallel (Promise.all) to halve wall-clock time - Use ackTimeoutMs: 5000 for a 20s redemption window Also add RemoteHandle.giveUp() method so the transport-level give-up (via onRemoteGiveUp) properly cleans up: stops ACK retransmission, rejects pending messages, and rejects pending URL redemptions. The previous code path only rejected redemptions, missing timer cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- StreamResetError no longer marks peer as intentionally closed — prevents a malicious peer from permanently suppressing reconnection - Add signal.aborted guard to peer:disconnect handler to avoid spurious reconnection during shutdown - Add MIN_STREAM_INACTIVITY_TIMEOUT_MS (5s) floor to prevent self-DoS via extremely small timeout values - Clean up double-invoke in #handleAckTimeout — delegate to giveUp() - Add unit test for RemoteHandle.giveUp() - Add test for stop() calling channel.stream.close() - Add test for closeChannel when abort() throws - Add streamInactivityTimeoutMs to comms-query-string round-trip test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sirtimid
changed the title
Coverage Report
File Coverage |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Wait longer than the inactivity timeout (2s + buffer). | ||
| // The stream should auto-abort due to inactivityTimeout, | ||
| // triggering connection loss handling and reconnection. | ||
| await delay(4_000); |
There was a problem hiding this comment.
E2E test delay shorter than clamped inactivity timeout
Medium Severity
The test sets streamInactivityTimeoutMs: 2_000 and waits delay(4_000), expecting the stream to have auto-aborted. However, registerChannel clamps the value via Math.max(streamInactivityTimeoutMs, MIN_STREAM_INACTIVITY_TIMEOUT_MS), which evaluates to Math.max(2000, 5000) = 5000. Since 4s < 5s, the stream hasn't timed out yet, so the test succeeds on the existing connection without ever triggering inactivity-based reconnection. The test passes but doesn't exercise the intended code path.
Additional Locations (2)
| ); | ||
| this.#rejectAllPending(`not acknowledged after ${MAX_RETRIES} retries`); | ||
| this.rejectPendingRedemptions( | ||
| `Remote connection lost after ${MAX_RETRIES} failed retries`, |
There was a problem hiding this comment.
Double giveUp() call in ACK timeout path
Low Severity
#handleAckTimeout calls this.giveUp(reason) on line 384, then this.#onGiveUp?.(this.#peerId) on line 385 triggers #handleRemoteGiveUp, which calls remote.giveUp(reason) again with a different reason string. The second invocation is a no-op since collections were already cleared, but it contradicts the comment stating "notify RemoteManager which calls giveUp()" — implying RemoteManager is the intended caller. The redundant path may confuse future maintainers.
Additional Locations (1)
1 participant
Closes #903
Summary
Adopts several libp2p v3 features to improve error handling, observability, and robustness of the remote communications layer:
async, so handler rejections automatically abort the stream instead of silently dropping errors{ error, local }properties for better diagnostics distinguishing local vs remote and clean vs error closesstreamInactivityTimeoutMs(default 120s) detects dead streams via bidirectional silence, with a 5s minimum floor to prevent self-DoSpeer:disconnectsafety net — listens for peer-level disconnect events as a fallback whenreadChannelcleanup misses a disconnectionStreamResetErrordetection — recognizes remote stream resets without permanently marking the peer as intentionally closed (prevents malicious peers from suppressing reconnection)closeChannelrewrite — useschannel.stream.close()directly instead of the fragileunwrap()pattern, with abort fallbackstop()sends FIN to remote peers so they see clean stream ends rather thanStreamResetErrorChanneltype gainsstream— direct access to stream lifecycle APIs, avoidingunwrap()TooManyOutboundProtocolStreamsErrorhandling — re-throws immediately since trying other addresses won't helpwriteWithTimeoutstream status guard — fast-fails on already-closed streams before attempting a writeAlso fixes:
maxConnectionAttemptsPerMinute: 500, parallel restarts, andackTimeoutMs: 5000RemoteHandle.giveUp()method — transport-level give-up now properly cleans up (stops ACK retransmission + rejects pending messages + rejects pending URL redemptions)#handleAckTimeoutdouble-invoke — delegates togiveUp()instead of duplicating rejection logicTest plan
New unit tests cover: stream status fast-fail (
it.eachover 4 statuses),peer:disconnecthandler (registration, forwarding, relay filtering, shutdown guard), stream close event diagnostics (4 scenarios viait.each),TooManyOutboundProtocolStreamsErrorrethrow,StreamResetErrorreconnection behavior,closeChannelgraceful/abort/double-fail paths,stop()stream closing,RemoteHandle.giveUp(), andstreamInactivityTimeoutMsquery string round-trip. New E2E test file (libp2p-v3-features.test.ts) covers stream inactivity timeout recovery end-to-end.🤖 Generated with Claude Code