Repository navigation
test: bound the footprint check by reads on both sides of the capture - #16141
Merged
Merged
Conversation
testSummaryPayloadIncludesPhysicalFootprintMemoryBytes compared the snapshot's memory_bytes with a footprint read taken before the capture. Since #13014 the capture is an async request to the app's shared snapshot service, which may wait behind the host's own census users while the host keeps allocating and freeing. The capture's own footprint read therefore lands some time after that reference. In #15488's validation run the host footprint moved 112 MiB (7 x 16 MiB) in that gap, past the 20% tolerance. The test now reads the footprint again after the capture and requires memory_bytes to be within the same tolerance of the range the two reads span. With no drift the accepted range is unchanged. It also requires memory_source_fallback_pids to be empty, which shows the value came from ri_phys_footprint rather than the resident-size fallback. Refs #15488 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe physical-footprint summary test now brackets snapshot capture with two readings, checks the snapshot value against that range with a size-based tolerance, and asserts that no PIDs used a memory-source fallback. ChangesSnapshot Memory Validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~5 minutes Change: Other Merge Risk: ⚪ Minimal · up to This is a bounded test-only change, and source inspection found no merge-blocking issue. The test has not been run locally, so normal CI validation remains useful. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
Pre-merge review (correctness first, subagent): approve, no defects.
Evidence update: the first red run (36758205468) failed iteration 1 as expected: |
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
1 participant
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.
CmuxTopSnapshotScopeTests.testSummaryPayloadIncludesPhysicalFootprintMemoryBytescompared the snapshot'smemory_byteswith a footprint read taken before the capture. The capture reads the footprint later, and the app host's footprint moves in between. In the #15488 validation run (shard 3 of run 36749306412) the two reads were 112 MiB apart (exactly 7 × 16 MiB), past the 20% tolerance. Part of #15488.Why the reads drift
Since #13014 the capture is an async request to the app's shared snapshot service. It runs off the main thread and can wait behind the host's own users of that service, while the host keeps allocating and freeing. Before #13014 the test was synchronous and the capture ran inline, so nothing ran between the two reads. The product is right:
capture()asks for a snapshot taken after the request, so it never reuses an older one.memory_bytescomes fromri_phys_footprint, the same field the test reads.The test passed in 104 of 105 runs scanned in main and PR logs since 2026-09-20. This duration ranged from 0.026 s to 8.1 s.
Change
The test reads the footprint again after the capture. It requires
memory_bytesto be within the same tolerance,max(16 MiB, 20%), of the range the two reads span. With no drift the accepted range is the same as before. It also requiresmemory_source_fallback_pidsto be empty, which shows the value came fromri_phys_footprint. The helper'sproc_pid_rusagecall moves onto one line to keep the file within its length budget.Evidence
Each run holds 256 MiB of dirty memory from just after the first footprint read, through the capture, on diag refs over main's 4df2a40 app products (run 36748094366), with
app-host-test-rerun.yml, 5 iterations each. The second pair leaks the buffer so every iteration dirties fresh pages:abs(memoryBytes - expectedFootprintBytes)256–345 MiB over tolerance268714176against a55017760tolerance)In the freed pair, iterations 2–5 reallocate pages the footprint still counts, so only iteration 1 drifts.
This PR's own CI also ran the fixed test in its shard, with no injected drift, and it passed.
No local build or test run; this Mac does not compile cmux.
Changelog
none
🤖 Generated with Claude Code
Summary by cubic
Stops
testSummaryPayloadIncludesPhysicalFootprintMemoryBytesfrom flaking when the app host's footprint moves between the test's reference read and the async capture.memory_bytesto fall within the same tolerance of the range the two reads span; with no drift the accepted range is unchanged.memory_source_fallback_pidsis empty to prove the value came fromri_phys_footprint.proc_pid_rusagecall onto one line to keep the file within its length budget.Written for commit 3877b95. Summary will update on new commits.
Summary by CodeRabbit