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.
This commit is contained in:
Brennan Benson
2026-08-19 14:08:12 -07:00
parent 3cb517abc2
commit 52f8de332b
3 changed files with 102 additions and 15 deletions
@@ -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<string, unknown>) => void = () => {}
const mux = {
request: vi
.fn()
.mockImplementationOnce(
() =>
new Promise<Record<string, unknown>>((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)
+54 -5
View File
@@ -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')
})
})
+19 -10
View File
@@ -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`. */