test: measure streaming working set via forced GC (Node 26 fix)#160
Merged
Conversation
The streaming-memory test asserts s3proxy doesn't buffer the full body. It measured process RSS, which is a GC-lag-driven high-water mark that V8 does not return to the OS after collection — so the ceiling drifted between Node versions and failed on Node 26 (~69MB vs the 50MB bound) despite streaming working correctly. Rework the test to measure the actual retained working set instead: - Force a full GC every ~1MB of chunks and sample external Buffer memory (arrayBuffers), which drops the instant freed buffers are collected. This reflects what s3proxy holds (~2MB under backpressure), not transient garbage, and is stable across Node versions. A buffering regression keeps the body referenced, so forced GC can't reclaim it and the delta climbs toward the 100MB payload. Bound tightened from 80MB to 8MB accordingly. - Acquire the GC trigger programmatically via v8.setFlagsFromString rather than relying on --expose-gc. The prior vitest.config.ts `forks.execArgv` entry never took effect (pool options must live under poolOptions, and even there vitest did not forward the flag to worker forks), so global.gc was always undefined and the original test's GC call was a silent no-op. Drop the dead config entry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3RYg3wG5UhVTCx58596iW
The audit, smoke, validation, performance, and package-verification jobs, plus the release and manual-release workflows, pinned Node 22. Move them to Node 24 (Active LTS) so the fixed build/publish runners track the current LTS. The test matrix still includes 22 to keep the >=22.13.0 engines floor exercised. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3RYg3wG5UhVTCx58596iW
Package verification exercises version-sensitive behavior (ESM resolution, install/import) and needs no AWS credentials, so run it across [22, 24, 26] rather than a single version. The other single-pinned jobs (audit, smoke, validation, performance) stay on Node 24. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F3RYg3wG5UhVTCx58596iW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
The Node CI matrix was expanded to
[22, 24, 26]in #159. That run went red on Node 26 only:test/streaming-memory.test.tsfailed withexpected 72089600 to be less than 52428800. Node 24 built and tested cleanly (its job was auto-cancelled by fail-fast once 26 failed). This PR fixes the failure so all three legs pass.Root cause
The streaming test asserts s3proxy streams rather than buffering the full body. It measured process RSS, which is a GC-lag-driven high-water mark that V8 does not return to the OS after collection — so the ceiling drifted between Node versions and tripped on Node 26 (~69 MB peak vs. the 50 MB bound) even though streaming works correctly.
Investigating also surfaced a latent config bug: the intended
--expose-gc(viavitest.config.tsforks.execArgv) never took effect. Pool options must live underpoolOptions, and even there vitest v4 did not forward the flag to worker forks — soglobal.gcwas alwaysundefinedand the test's baselineglobal.gc()call was a silent no-op. The test was measuring uncollected garbage the whole time.Change
Rework the test to measure the actual retained working set instead of an RSS high-water mark:
v8.setFlagsFromString('--expose-gc')+vm.runInNewContext('gc'), independent of launch flags. Falls back to skipping the strict bound if unavailable, so it never false-fails.arrayBuffers, not RSS — the payload lives in external Buffer memory, andarrayBuffersdrops the instant freed buffers are collected. Forcing a full GC every ~1 MB of chunks makes the peak reflect what s3proxy actually holds (~2 MB under backpressure), stable across Node versions.forks.execArgventry fromvitest.config.ts.The assertion is now stronger (8 MB vs. 50 MB) and version-stable, because the GC cadence is forced rather than left to the runtime.
Verification
Locally on Node 22: observed peak 2.06 MB streaming a 100 MB body.
lint,type-check,build, andtest:unitall pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01F3RYg3wG5UhVTCx58596iW
Generated by Claude Code