Skip to content

Commit f07e97d

Browse files
FUDCoclaude
andauthored
fix(ocap-kernel): use length-prefixed framing for remote messages (#957)
## Summary - Remote messages larger than `@libp2p/webrtc`'s 16 KB `MAX_MESSAGE_SIZE` were being split into multiple datachannel sends, but the kernel was reading from a `byteStream` that doesn't preserve message boundaries. The reader would wake on the first chunk and call `JSON.parse` on a truncated string; the parse error was caught silently, no ACK was sent, and the sender's `RemoteHandle` retried until `MAX_RETRIES` was hit and it gave up. - Switched to `lpStream` (length-prefixed framing) so each `write()` produces exactly one `read()` regardless of how the underlying webRTC transport chunks the bytes on the wire. - Set `maxDataLength = DEFAULT_MAX_MESSAGE_SIZE_BYTES` (1 MB) on the receiver so the new framing's size cap matches the sender-side `validateMessageSize` policy. - Translated `InvalidDataLengthError` / `InvalidDataLengthLengthError` from `lpStream` into a `ResourceLimitError` with `limitType: 'messageSize'`, so size-related failures look the same whether they tripped on the sender or the receiver. - Caught `UnexpectedEOFError` in the read loop and treated it as a graceful stream end (the byteStream's old `null`-return path is gone with lpStream). ## Test plan - [x] `yarn workspace @MetaMask/ocap-kernel test:dev:quiet` — all 2341 unit tests pass - [x] `yarn workspace @MetaMask/ocap-kernel build` — clean - [x] `yarn workspace @MetaMask/ocap-kernel lint:fix` — clean - [x] End-to-end: ran the orchestration demo through to the Sales phase, with the schematic-generation service (~17 KB SVG payload) and pcb-layout service (~15 KB SVG) both delivering successfully where they were timing out before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 26528cf commit f07e97d

9 files changed

Lines changed: 172 additions & 27 deletions

File tree

packages/ocap-kernel/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4141
- Regenerate `incarnationId` when `resetStorage=true` clears the rest of kernel state, completing the #948 peer-restart detection on browser/extension kernel reloads ([#950](https://github.com/MetaMask/ocap-kernel/pull/950))
4242
- The previous except-list preserved `incarnationId` across `resetStorage` wipes, so a restarted sender signalled the same incarnation it had before the wipe and the matching receiver's handshake decided "no restart" — leaving stale `highestReceivedSeq` in place and silently dropping the sender's fresh `seq=1` messages
4343
- Register a new vat with its subcluster before awaiting `runVat`, so a garbage-collection pass during bundle load cannot delete the still-empty subcluster out from under the in-progress vat creation ([#952](https://github.com/MetaMask/ocap-kernel/pull/952))
44+
- Use length-prefixed framing for remote messages so payloads larger than the underlying transport's per-frame cutoff (e.g. `@libp2p/webrtc`'s 16 KB datachannel limit) are reassembled correctly on the receiver ([#957](https://github.com/MetaMask/ocap-kernel/pull/957))
45+
- Replace `byteStream` with `lpStream` on every remote channel; the byte-oriented stream did not preserve `write()` boundaries, so any message the transport split into multiple frames was parsed from the first frame only, silently dropped without acknowledgement, and the sender retried until giving up after `MAX_RETRIES`
46+
- Surface receiver-side framing-cap violations (`InvalidDataLengthError`, `InvalidDataLengthLengthError`) as `ResourceLimitError` with `limitType: 'messageSize'` so size errors look the same whether they tripped on the sender's `validateMessageSize` or the receiver's framing decoder
4447

4548
## [0.7.0]
4649

packages/ocap-kernel/src/remotes/platform/connection-factory.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ function makeTestMultiaddr(protoNames: string[], host: string) {
220220
};
221221
}
222222

223-
// Simple ByteStream mock
223+
// Simple LengthPrefixedStream mock
224224
type MockByteStream = {
225225
write: (chunk: Uint8Array) => Promise<void>;
226226
read: () => Promise<Uint8Array | undefined>;
@@ -229,7 +229,7 @@ type MockByteStream = {
229229

230230
const streamMap = new WeakMap<object, MockByteStream>();
231231
vi.mock('@libp2p/utils', () => ({
232-
byteStream: (stream: object) => {
232+
lpStream: (stream: object) => {
233233
const bs: MockByteStream = {
234234
writes: [],
235235
async write(chunk: Uint8Array) {
@@ -243,6 +243,9 @@ vi.mock('@libp2p/utils', () => ({
243243
return bs;
244244
},
245245
getByteStreamFor: (stream: object) => streamMap.get(stream),
246+
InvalidDataLengthError: class InvalidDataLengthError extends Error {},
247+
InvalidDataLengthLengthError: class InvalidDataLengthLengthError extends Error {},
248+
UnexpectedEOFError: class UnexpectedEOFError extends Error {},
246249
}));
247250

248251
const createLibp2p = vi.fn();

packages/ocap-kernel/src/remotes/platform/connection-factory.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
} from '@libp2p/interface';
1111
import type { PrivateKey, Libp2p } from '@libp2p/interface';
1212
import { ping } from '@libp2p/ping';
13-
import { byteStream } from '@libp2p/utils';
13+
import { lpStream } from '@libp2p/utils';
1414
import { webRTC } from '@libp2p/webrtc';
1515
import { webSockets } from '@libp2p/websockets';
1616
import { webTransport } from '@libp2p/webtransport';
@@ -26,6 +26,7 @@ import type { Multiaddr } from '@multiformats/multiaddr';
2626
import { createLibp2p } from 'libp2p';
2727

2828
import {
29+
DEFAULT_MAX_MESSAGE_SIZE_BYTES,
2930
RELAY_RECONNECT_BASE_DELAY_MS,
3031
RELAY_RECONNECT_MAX_DELAY_MS,
3132
RELAY_RECONNECT_MAX_ATTEMPTS,
@@ -59,6 +60,8 @@ export class ConnectionFactory {
5960

6061
readonly #maxRetryAttempts: number;
6162

63+
readonly #maxDataLength: number;
64+
6265
readonly #directTransports: DirectTransport[];
6366

6467
readonly #allowedWsHosts: string[];
@@ -87,6 +90,7 @@ export class ConnectionFactory {
8790
* @param options.logger - The logger to use for the libp2p node.
8891
* @param options.signal - The signal to use for the libp2p node.
8992
* @param options.maxRetryAttempts - Maximum number of reconnection attempts. 0 = infinite (default).
93+
* @param options.maxMessageSizeBytes - Maximum inbound message size in bytes, used as `maxDataLength` on every `lpStream`. Defaults to 1 MB.
9094
* @param options.directTransports - Optional direct transports (e.g. QUIC, TCP) with listen addresses.
9195
* @param options.allowedWsHosts - Hostnames/IPs allowed for plain ws:// connections beyond private ranges.
9296
*/
@@ -97,6 +101,8 @@ export class ConnectionFactory {
97101
this.#logger = options.logger;
98102
this.#signal = options.signal;
99103
this.#maxRetryAttempts = options.maxRetryAttempts ?? 0;
104+
this.#maxDataLength =
105+
options.maxMessageSizeBytes ?? DEFAULT_MAX_MESSAGE_SIZE_BYTES;
100106
this.#directTransports = options.directTransports ?? [];
101107
const explicitHosts = options.allowedWsHosts ?? [];
102108
const relayHosts: string[] = [];
@@ -137,6 +143,7 @@ export class ConnectionFactory {
137143
* @param options.logger - The logger to use for the libp2p node.
138144
* @param options.signal - The signal to use for the libp2p node.
139145
* @param options.maxRetryAttempts - Maximum number of reconnection attempts. 0 = infinite (default).
146+
* @param options.maxMessageSizeBytes - Maximum inbound message size in bytes, used as `maxDataLength` on every `lpStream`. Defaults to 1 MB.
140147
* @param options.directTransports - Optional direct transports (e.g. QUIC, TCP) with listen addresses.
141148
* @param options.allowedWsHosts - Hostnames/IPs allowed for plain ws:// connections beyond private ranges.
142149
* @returns A promise for the new ConnectionFactory instance.
@@ -217,7 +224,9 @@ export class ConnectionFactory {
217224

218225
// Set up inbound handler
219226
await this.#libp2p.handle('whatever', async (stream, connection) => {
220-
const msgStream = byteStream(stream);
227+
const msgStream = lpStream(stream, {
228+
maxDataLength: this.#maxDataLength,
229+
});
221230
const remotePeerId = connection.remotePeer.toString();
222231
const connType = connection.direct ? 'direct' : 'relayed';
223232
this.#logger.log(
@@ -401,7 +410,9 @@ export class ConnectionFactory {
401410
this.#logger.log(
402411
`successfully connected to ${peerId} via ${addressString}`,
403412
);
404-
const msgStream = byteStream(stream);
413+
const msgStream = lpStream(stream, {
414+
maxDataLength: this.#maxDataLength,
415+
});
405416
const channel: Channel = { msgStream, stream, peerId };
406417
this.#logger.log(`opened channel to ${peerId}`);
407418
return channel;

packages/ocap-kernel/src/remotes/platform/handshake.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { UnexpectedEOFError } from '@libp2p/utils';
12
import { Logger } from '@metamask/logger';
23
import { describe, it, expect, beforeEach, vi } from 'vitest';
34

@@ -298,11 +299,11 @@ describe('handshake', () => {
298299
it('throws when channel closes during read', async () => {
299300
vi.spyOn(mockChannel.msgStream, 'read')
300301
.mockImplementation()
301-
.mockResolvedValueOnce(undefined);
302+
.mockRejectedValueOnce(new UnexpectedEOFError('stream closed'));
302303

303304
await expect(
304305
performInboundHandshake(mockChannel, mockDeps),
305-
).rejects.toThrow('Channel closed during handshake');
306+
).rejects.toThrow(UnexpectedEOFError);
306307
});
307308
});
308309
});

packages/ocap-kernel/src/remotes/platform/handshake.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,6 @@ async function readWithTimeout(
8989

9090
const readPromise = (async () => {
9191
const readBuf = await channel.msgStream.read();
92-
if (!readBuf) {
93-
throw new Error('Channel closed during handshake');
94-
}
9592
return bufToString(readBuf.subarray());
9693
})();
9794

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
import type { Stream } from '@libp2p/interface';
2+
import { lpStream, streamPair } from '@libp2p/utils';
3+
import { fromString, toString as bufToString } from 'uint8arrays';
4+
import { describe, expect, it } from 'vitest';
5+
6+
/**
7+
* Regression test for the framing bug that motivated switching from
8+
* `byteStream` to `lpStream`: when the underlying transport (e.g.
9+
* `@libp2p/webrtc`) splits a single write across multiple frames,
10+
* `byteStream`'s reader would wake on the first chunk and return a
11+
* truncated payload. `lpStream` adds a length-prefix so the reader
12+
* waits until the full message is present and returns it intact.
13+
*
14+
* Each test forces chunking by capping the underlying stream's
15+
* `maxMessageSize` well below the payload size; the abstract stream
16+
* splits the write into several `message` events on the receiver side.
17+
* `lpStream.read()` should still return exactly one complete payload
18+
* per `lpStream.write()`.
19+
*/
20+
describe('lpStream framing over a chunked transport', () => {
21+
/**
22+
* Build a connected pair of message streams whose underlying
23+
* AbstractStream splits any write larger than `chunkSize` bytes into
24+
* multiple `message` events on the receiving end.
25+
*
26+
* @param chunkSize - Per-frame cap to apply to both ends.
27+
* @returns The outbound/inbound paired streams.
28+
*/
29+
async function chunkedStreamPair(
30+
chunkSize: number,
31+
): Promise<[Stream, Stream]> {
32+
return streamPair({
33+
outbound: { maxMessageSize: chunkSize },
34+
inbound: { maxMessageSize: chunkSize },
35+
});
36+
}
37+
38+
it('reassembles a single payload that the transport splits into many frames', async () => {
39+
const [outbound, inbound] = await chunkedStreamPair(1024);
40+
const sender = lpStream(outbound, { maxDataLength: 1024 * 1024 });
41+
const receiver = lpStream(inbound, { maxDataLength: 1024 * 1024 });
42+
43+
// 20 KB payload — well above the 1 KB per-frame cap, so the transport
44+
// will split it into ~20 frames on the receiver side.
45+
const payload = 'A'.repeat(20_000);
46+
await sender.write(fromString(payload));
47+
48+
const received = await receiver.read();
49+
expect(received.byteLength).toBe(20_000);
50+
expect(bufToString(received.subarray())).toBe(payload);
51+
});
52+
53+
it('preserves message boundaries when several large payloads are sent back-to-back', async () => {
54+
const [outbound, inbound] = await chunkedStreamPair(2048);
55+
const sender = lpStream(outbound, { maxDataLength: 1024 * 1024 });
56+
const receiver = lpStream(inbound, { maxDataLength: 1024 * 1024 });
57+
58+
const payloads = ['alpha', 'bravo', 'charlie'].map(
59+
(label) => `${label}:${'x'.repeat(8_000)}`,
60+
);
61+
for (const payload of payloads) {
62+
await sender.write(fromString(payload));
63+
}
64+
65+
for (const expected of payloads) {
66+
const received = await receiver.read();
67+
expect(bufToString(received.subarray())).toBe(expected);
68+
}
69+
});
70+
71+
it('rejects an inbound message that announces a payload larger than maxDataLength', async () => {
72+
const [outbound, inbound] = await chunkedStreamPair(1024);
73+
const sender = lpStream(outbound, { maxDataLength: 1024 * 1024 });
74+
// Receiver caps inbound at 8 KB to exercise the cross-machine
75+
// mismatch sirtimid called out: a sender that allows larger messages
76+
// than the receiver does should produce a clean InvalidDataLengthError
77+
// on the receiver, not a silent reassembly stall.
78+
const receiver = lpStream(inbound, { maxDataLength: 8 * 1024 });
79+
80+
const oversized = fromString('B'.repeat(16_000));
81+
await sender.write(oversized);
82+
83+
await expect(receiver.read()).rejects.toThrow(
84+
/Message length too long|InvalidDataLength/u,
85+
);
86+
});
87+
});

packages/ocap-kernel/src/remotes/platform/transport.test.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { UnexpectedEOFError } from '@libp2p/utils';
12
import { AbortError } from '@metamask/kernel-errors';
23
import { makeAbortSignalMock } from '@ocap/repo-tools/test-utils';
34
import {
@@ -288,6 +289,7 @@ describe('transport.initTransport', () => {
288289
logger: expect.any(Object),
289290
signal: expect.any(AbortSignal),
290291
maxRetryAttempts: undefined,
292+
maxMessageSizeBytes: 1024 * 1024,
291293
directTransports: undefined,
292294
});
293295
});
@@ -305,6 +307,7 @@ describe('transport.initTransport', () => {
305307
logger: expect.any(Object),
306308
signal: expect.any(AbortSignal),
307309
maxRetryAttempts,
310+
maxMessageSizeBytes: 1024 * 1024,
308311
directTransports: undefined,
309312
});
310313
});
@@ -333,6 +336,7 @@ describe('transport.initTransport', () => {
333336
logger: expect.any(Object),
334337
signal: expect.any(AbortSignal),
335338
maxRetryAttempts: undefined,
339+
maxMessageSizeBytes: 1024 * 1024,
336340
directTransports,
337341
});
338342
});
@@ -736,7 +740,7 @@ describe('transport.initTransport', () => {
736740
});
737741
});
738742

739-
it('exits read loop when readBuf is undefined (stream ended)', async () => {
743+
it('treats UnexpectedEOFError as connection loss and triggers reconnection', async () => {
740744
let inboundHandler: ((channel: MockChannel) => void) | undefined;
741745
mockConnectionFactory.onInboundConnection.mockImplementation(
742746
(handler) => {
@@ -748,16 +752,23 @@ describe('transport.initTransport', () => {
748752
await initTransport('0x1234', {}, remoteHandler);
749753

750754
const mockChannel = createMockChannel('peer-1');
751-
// First read returns undefined, which means stream ended - loop should break
752-
mockChannel.msgStream.read.mockResolvedValueOnce(undefined);
755+
// lpStream throws UnexpectedEOFError on EOF — both for clean
756+
// end-of-stream between messages and for a partial-message drop.
757+
// The read loop can't distinguish the two, so it must treat any
758+
// EOF as a potential mid-message drop and trigger reconnection
759+
// (rather than silently exiting and leaving the sender to
760+
// retransmit into a dead channel).
761+
mockChannel.msgStream.read.mockRejectedValueOnce(
762+
new UnexpectedEOFError('stream closed'),
763+
);
753764

754765
inboundHandler?.(mockChannel);
755766

756767
await vi.waitFor(() => {
757-
// Stream ended, so no messages should be processed
758768
expect(remoteHandler).not.toHaveBeenCalled();
759-
// Should log that stream ended
760-
expect(mockLogger.log).toHaveBeenCalledWith('peer-1:: stream ended');
769+
expect(mockReconnectionManager.startReconnection).toHaveBeenCalledWith(
770+
'peer-1',
771+
);
761772
});
762773
});
763774

packages/ocap-kernel/src/remotes/platform/transport.ts

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import { StreamResetError } from '@libp2p/interface';
22
import type { StreamCloseEvent } from '@libp2p/interface';
3+
import {
4+
InvalidDataLengthError,
5+
InvalidDataLengthLengthError,
6+
} from '@libp2p/utils';
37
import {
48
AbortError,
59
IntentionalCloseError,
@@ -171,6 +175,7 @@ export async function initTransport(
171175
logger,
172176
signal,
173177
maxRetryAttempts,
178+
maxMessageSizeBytes,
174179
directTransports,
175180
allowedWsHosts,
176181
});
@@ -463,6 +468,31 @@ export async function initTransport(
463468
try {
464469
readBuf = await channel.msgStream.read();
465470
} catch (problem) {
471+
if (
472+
problem instanceof InvalidDataLengthError ||
473+
problem instanceof InvalidDataLengthLengthError
474+
) {
475+
// Peer announced a payload larger than maxMessageSizeBytes. The
476+
// length-prefixed framing is now poisoned (subsequent bytes are
477+
// not on a message boundary), so we cannot continue on this
478+
// stream. Surface a uniform "message too long" error to match the
479+
// sender-side validator.
480+
const sizeError = new ResourceLimitError(
481+
`Inbound message exceeds size limit: ${problem.message}`,
482+
{
483+
cause: problem,
484+
data: { limitType: 'messageSize' },
485+
},
486+
);
487+
outputError(
488+
channel.peerId,
489+
`reading message from ${channel.peerId}`,
490+
sizeError,
491+
);
492+
handleConnectionLoss(channel.peerId);
493+
logger.log(`closed channel to ${channel.peerId}`);
494+
throw sizeError;
495+
}
466496
if (problem instanceof StreamResetError) {
467497
// Remote-initiated stream reset: treat as connection loss and
468498
// reconnect. Do NOT mark as intentionally closed — a malicious
@@ -487,15 +517,9 @@ export async function initTransport(
487517
logger.log(`closed channel to ${channel.peerId}`);
488518
throw problem;
489519
}
490-
if (readBuf) {
491-
reconnectionManager.resetBackoff(channel.peerId); // successful inbound traffic
492-
peerStateManager.updateConnectionTime(channel.peerId);
493-
await receiveMessage(channel.peerId, bufToString(readBuf.subarray()));
494-
} else {
495-
// Stream ended (returned undefined), exit the read loop
496-
logger.log(`${channel.peerId}:: stream ended`);
497-
break;
498-
}
520+
reconnectionManager.resetBackoff(channel.peerId); // successful inbound traffic
521+
peerStateManager.updateConnectionTime(channel.peerId);
522+
await receiveMessage(channel.peerId, bufToString(readBuf.subarray()));
499523
}
500524
} finally {
501525
// Always remove the channel when readChannel exits to prevent stale channels

packages/ocap-kernel/src/remotes/types.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { Stream } from '@libp2p/interface';
2-
import type { ByteStream } from '@libp2p/utils';
2+
import type { LengthPrefixedStream } from '@libp2p/utils';
33
import type { Logger } from '@metamask/logger';
44

55
import type { KRef } from '../types.ts';
@@ -11,7 +11,7 @@ export type InboundConnectionHandler = (
1111
export type PeerDisconnectHandler = (peerId: string) => void;
1212

1313
export type Channel = {
14-
msgStream: ByteStream<Stream>;
14+
msgStream: LengthPrefixedStream<Stream>;
1515
stream: Stream;
1616
peerId: string;
1717
};
@@ -211,6 +211,14 @@ export type ConnectionFactoryOptions = {
211211
logger: Logger;
212212
signal: AbortSignal;
213213
maxRetryAttempts?: number | undefined;
214+
/**
215+
* Maximum inbound message payload size in bytes. Used as `maxDataLength`
216+
* on every `lpStream` constructed for a channel — must match the
217+
* sender-side validator's limit (`maxMessageSizeBytes` on
218+
* `RemoteCommsOptions`) so that a deployment which raises one also raises
219+
* the other. Defaults to `DEFAULT_MAX_MESSAGE_SIZE_BYTES` (1 MB).
220+
*/
221+
maxMessageSizeBytes?: number | undefined;
214222
directTransports?: DirectTransport[] | undefined;
215223
allowedWsHosts?: string[] | undefined;
216224
};

0 commit comments

Comments
 (0)