From 52f8de332b1bc408e4c9e2a26e5f2c3cb21c4e18 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:08:12 -0700 Subject: [PATCH] test(remote): pin the ranged-read cap to real control-queue headroom The cap comment claimed a full-cap window stays in the control lane "unconditionally" and the guard test only asserted one frame fits the lane, so a raise to 384-768 KiB stayed green while two concurrent full-cap responses would already overflow the shared control queue -- which for a response closes the client. Pin the two-deep headroom and state the real bound, including that widening the cap is a wire change against a host still advertising rangedReadVersion 1. Also cover the two behaviours the suite claimed but did not exercise: a regular file answers a full-cap read in one syscall, so the fill loop was untested (both mutations of readFullStreamChunk stayed green), and the merged capability document made the abort-does-not-evict guard load-bearing without any test reaching it. --- ...h-filesystem-provider-capabilities.test.ts | 29 +++++++++ src/relay/fs-handler-file-range.test.ts | 59 +++++++++++++++++-- src/shared/file-range-read.ts | 29 +++++---- 3 files changed, 102 insertions(+), 15 deletions(-) diff --git a/src/main/providers/ssh-filesystem-provider-capabilities.test.ts b/src/main/providers/ssh-filesystem-provider-capabilities.test.ts index 2a1f574449f..5a34b2a0c1f 100644 --- a/src/main/providers/ssh-filesystem-provider-capabilities.test.ts +++ b/src/main/providers/ssh-filesystem-provider-capabilities.test.ts @@ -70,6 +70,35 @@ describe('SSH filesystem capability document', () => { expect(mux.request).toHaveBeenCalledTimes(2) }) + // One document now backs every feature, so a caller walking away from its own + // probe must not evict the fetch a second feature is still waiting on -- + // otherwise an aborted quick-open probe costs the ranged-read probe a round + // trip, and a caller that keeps aborting re-fetches forever. + it('keeps the in-flight document when one caller aborts its own probe', async () => { + let settle: (value: Record) => void = () => {} + const mux = { + request: vi + .fn() + .mockImplementationOnce( + () => + new Promise>((resolve) => { + settle = resolve + }) + ) + .mockResolvedValue({ rangedReadVersion: 1 }) + } + const controller = new AbortController() + const abandoned = probeSshQuickOpenSearchCapability(mux as never, controller.signal) + const patient = probeSshRangedReadCapability(mux as never) + controller.abort() + await expect(abandoned).rejects.toThrow('client_disconnected') + + settle({ rangedReadVersion: 1 }) + await expect(patient).resolves.toBe(true) + await expect(probeSshRangedReadCapability(mux as never)).resolves.toBe(true) + expect(mux.request).toHaveBeenCalledTimes(1) + }) + it('treats a non-object response as no capabilities', async () => { const mux = { request: vi.fn().mockResolvedValue('yes') } await expect(probeSshRangedReadCapability(mux as never)).resolves.toBe(false) diff --git a/src/relay/fs-handler-file-range.test.ts b/src/relay/fs-handler-file-range.test.ts index 2ebe0bcab59..4690b6a14c7 100644 --- a/src/relay/fs-handler-file-range.test.ts +++ b/src/relay/fs-handler-file-range.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it } from 'vitest' import { readRelayFileRange } from './fs-handler-file-range' +import { readFullStreamChunk } from './fs-handler-file-read' import { FileRangeReadRequestError, MAX_FILE_RANGE_READ_BYTES } from '../shared/file-range-read' import { HEADER_LENGTH, prepareJsonRpcPayload } from './protocol' import { DISPATCHER_CONTROL_QUEUE_MAX_BYTES } from './dispatcher-writer-admission' @@ -62,8 +63,9 @@ describe('readRelayFileRange', () => { expect(result.bytesRead).toBe(0) }) - // The window is one fixed allocation filled across however many syscalls the - // kernel takes, so every partial read has to land at the right offset in it. + // A regular file answers a full-cap read in one syscall, so this pins offset + // arithmetic over a many-page window, not the fill loop -- that is covered + // against a stub reader in `readFullStreamChunk` below. it('assembles a many-page window without misplacing any byte', async () => { const size = MAX_FILE_RANGE_READ_BYTES const contents = Buffer.allocUnsafe(size) @@ -97,9 +99,13 @@ describe('readRelayFileRange', () => { // The cap is a TRANSPORT budget, not just a memory one. A response frame over // DISPATCHER_CONTROL_QUEUE_MAX_BYTES is demoted to the `legacy-response` lane, // which the writer refuses once the producer queue is busy -- so an over-wide - // cap fails with ResponseOverCapacity depending on unrelated load. A full-cap - // window of incompressible bytes must stay inside the control lane. - it('encodes a full-cap window into a frame the control lane always admits', async () => { + // cap fails with ResponseOverCapacity depending on unrelated load. + // + // Fitting the lane once is not enough: the control queue is a SHARED budget + // and overflowing it closes the client, so a full-cap frame has to leave room + // for a second one. Anything wider lets two pipelined tail reads -- or one + // read racing an unrelated response -- kill the connection. + it('leaves control-queue headroom for a second full-cap window', async () => { const contents = Buffer.allocUnsafe(MAX_FILE_RANGE_READ_BYTES) for (let i = 0; i < contents.length; i++) { contents[i] = (i * 37) % 256 @@ -110,6 +116,7 @@ describe('readRelayFileRange', () => { const frameBytes = HEADER_LENGTH + prepareJsonRpcPayload({ jsonrpc: '2.0', id: 4294967295, result }).byteLength expect(frameBytes).toBeLessThanOrEqual(DISPATCHER_CONTROL_QUEUE_MAX_BYTES) + expect(frameBytes * 2).toBeLessThanOrEqual(DISPATCHER_CONTROL_QUEUE_MAX_BYTES) }) it('rejects a missing file', async () => { @@ -165,3 +172,45 @@ describe('readRelayFileRange', () => { }) }) }) + +// The fill loop is what makes "short result == EOF" true. A regular file hands +// back a whole window in one syscall, so the loop only shows up against a +// reader that answers partially -- a pipe, a network filesystem, a large read +// interrupted by a signal. +describe('readFullStreamChunk', () => { + function partialReader(source: Buffer, perCall: number) { + const calls: { offset: number; length: number; position: number }[] = [] + return { + calls, + read(buffer: Buffer, offset: number, length: number, position: number) { + calls.push({ offset, length, position }) + const slice = source.subarray(position, position + Math.min(length, perCall)) + slice.copy(buffer, offset) + return Promise.resolve({ bytesRead: slice.length }) + } + } + } + + it('fills the window across partial reads, each landing at its own offset', async () => { + const source = Buffer.from('abcdefghij') + const reader = partialReader(source, 3) + const buffer = Buffer.alloc(6) + const bytesRead = await readFullStreamChunk(reader, buffer, 6, 2) + expect(bytesRead).toBe(6) + expect(buffer.toString('utf8')).toBe('cdefgh') + expect(reader.calls).toEqual([ + { offset: 0, length: 6, position: 2 }, + { offset: 3, length: 3, position: 5 } + ]) + }) + + // Without the zero check the loop would spin forever on a reader at EOF. + it('stops at the first empty read and reports only what it filled', async () => { + const source = Buffer.from('abcd') + const reader = partialReader(source, 3) + const buffer = Buffer.alloc(16) + const bytesRead = await readFullStreamChunk(reader, buffer, 16, 0) + expect(bytesRead).toBe(4) + expect(buffer.subarray(0, bytesRead).toString('utf8')).toBe('abcd') + }) +}) diff --git a/src/shared/file-range-read.ts b/src/shared/file-range-read.ts index 70cf278525c..6af436fcf48 100644 --- a/src/shared/file-range-read.ts +++ b/src/shared/file-range-read.ts @@ -1,14 +1,23 @@ -/** Largest window one `fs.readFileRange` response may carry: `STREAM_CHUNK_SIZE`, - * the house per-frame budget for file bytes. +/** Largest window one `fs.readFileRange` response may carry. Derived from the + * relay writer's admission budget, NOT from the 16 MiB frame cap. * - * The binding constraint is the relay writer's admission budget, NOT the 16 MiB - * frame cap. A response frame over `DISPATCHER_CONTROL_QUEUE_MAX_BYTES` (1 MiB) - * is demoted to the `legacy-response` lane, which is refused once the producer - * queue passes `DEFAULT_PRODUCER_QUEUE_MAX_BYTES` (2 MiB) -- so a wide window - * fails with an opaque `ResponseOverCapacity` that depends on what else is - * queued at the time. 256 KiB of raw bytes is ~350 KB of base64, which stays in - * the control lane unconditionally. Bigger transfers belong on the ack-paced - * bulk lane (`fs.readFileStream`), not on a wider cap here. + * A response frame over `DISPATCHER_CONTROL_QUEUE_MAX_BYTES` (1 MiB) is demoted + * to the `legacy-response` lane, which is refused outright once the producer + * queue passes `DEFAULT_PRODUCER_QUEUE_MAX_BYTES` (2 MiB) -- an opaque + * `ResponseOverCapacity` that depends on unrelated load. 256 KiB of raw bytes + * frames to ~350 KB, so a full-cap response takes the control lane instead. + * + * The control lane is a shared budget, not a per-frame one: two full-cap + * responses fit alongside each other, and the third overflows -- which for a + * response is fatal, it closes the client. That is the same exposure every + * control-lane response already carries (`fs.readFile` frames any sub-1 MiB + * file the same way), and the two-deep headroom is pinned by a test. Widening + * the cap spends that headroom, so bigger transfers belong on the ack-paced + * bulk lane (`fs.readFileStream`) rather than on a wider window here. + * + * Raising this value is a wire change even though it is a constant: an older + * host still advertising `rangedReadVersion: 1` refuses a longer window with an + * opaque error, so a wider cap needs a version bump to negotiate against. * * Requests above it are REJECTED, never clamped: a clamped read is * indistinguishable from EOF, and callers page by advancing `position`. */