feat: prevent compose output string overflow - #346
Conversation
|
Thanks for picking this up, and for shipping tests and docs with it. Reviewed at 16e4258. The approach is right: cap before appending, and leave the callback untouched so streaming consumers still see every chunk. That second part is what actually makes #257 solvable, and I'm glad it has a test. Four small things I'd like to see before merging: 1. Rename the option. 2. Document the streaming case. 3. Drop the 4. Two more tests. One for Two more things I'd like your opinion on. Happy to take them as follow-ups if you'd rather keep this PR tight: 5. Nothing on the result says output was dropped. 6. Truncation keeps the head. For One last thing, entirely optional and not for this PR: |
16e4258 to
ad15291
Compare
|
@AlexZeitler I've handled your feedbacks |
|
Thanks, that was quick. Reviewed at ad15291. The rename, the Two things came out of a closer look. The first one is on me: the trailing window was my suggestion, so the cost that comes with it is mine to report. 1. The trailing window copies the whole buffer on every chunk once the limit is reached. The result.err = output.slice(Math.max(0, output.length + tail.length - maxLength)) + tailAs soon as that offset is nonzero, V8 flattens the buffer and copies it, so each chunk costs the full window width. Plain A standalone script, no dependencies: const CHUNK = 'x'.repeat(100)
const CHUNKS = 20000
const time = (label, fn) => {
const t0 = process.hrtime.bigint()
fn()
console.log(`${label}: ${Number(process.hrtime.bigint() - t0) / 1e6} ms`)
}
time('plain +=', () => {
let err = ''
for (let i = 0; i < CHUNKS; i++) err += CHUNK
})
time('trailing window, limit 1e6', () => {
const maxLength = 1_000_000
let err = ''
for (let i = 0; i < CHUNKS; i++) {
const tail = CHUNK.slice(-maxLength)
err = err.slice(Math.max(0, err.length + tail.length - maxLength)) + tail
}
})
time('chunk ring, limit 1e6', () => {
const maxLength = 1_000_000
const parts = []
let total = 0
for (let i = 0; i < CHUNKS; i++) {
parts.push(CHUNK)
total += CHUNK.length
while (total - parts[0].length >= maxLength) total -= parts.shift().length
}
parts.join('')
})On Node 24 I get well under a millisecond for The default limit is not affected: the offset stays zero, and Keeping the chunks in an array, dropping from the front, and joining once in the 2. For release purposes this is a
|
Co-authored-by: neilime <314088+neilime@users.noreply.github.com> Signed-off-by: Emilien Escalle <emilien.escalle@escemi.com>
ad15291 to
0e015dd
Compare
|
@AlexZeitler I've handled your last feedback. Thanks |
|
Looks good at 0e015dd. The chunk ring does the job, and the eviction is more careful than what I sketched: a partially evicted chunk gets trimmed in place instead of dropped, and compacting only once half the array is dead keeps that amortized. I ran the same measurement as before, with the new I also walked the eviction loop looking for a way to spin forever or to run The new cases cover what I would have asked for anyway: partial eviction, a chunk larger than the limit, surrogate pairs, many small chunks, and output arriving after exit. Thanks for working through all of this. Happy to merge once CI is green. |
|
📦 |
Long-running or verbose Compose commands could accumulate stdout/stderr beyond JavaScript's string limit and fail with
RangeError: Invalid string length. This addsmaxOutputLengthto bound captured output independently for each stream, addressing #257.buffer.constants.MAX_STRING_LENGTH, and accepts0to disable buffering. Callbacks and logging still receive every chunk.truncated.outandtruncated.errflags are present on successful, rejected, and typed results. Commands that parse stdout reject if it was truncated.The README and API docs cover the option and result flags. The commit uses
feat:for the new public API.Validation: 42 selected buffering, executable-resolution, and parsing tests pass, including nine new cases for chunk eviction, sustained small-chunk output, and output received during the existing exit delay.
yarn buildandyarn lintpass (lint reports seven existing warnings). Docker integration tests were not run because the sandbox cannot access the Docker socket.A local Node.js 26 benchmark using 20,000 stderr chunks of 100 bytes with a 1,000,000-code-unit limit took about 4.5 ms with chunk queues, compared with 1.0–1.3 seconds for the previous implementation (two trials, excluding the fixed exit delay).