DELIVERY: two gates from Carom 2.0, both silent passes - #16
Conversation
Both are silent passes rather than failures: a config field that never reached three call sites because they unpack by hand, and a pressure test that asserted before any pressure existed and hid a wrong exception type behind a green run.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe documentation adds two delivery lessons. One covers configuration fields lost during manual unpacking. The other covers timeout tests that assert before queued background work starts. ChangesDelivery lessons
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change documents two delivery gates without changing product or runtime behavior, so no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 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 |
1 participant
Two gates from the Carom 2.0 release. Both are silent passes, which the document says deserve more attention than failures.
A field added to a config is not a field that arrives. Three extension packages accepted a config struct and unpacked selected fields by hand, so two fields added in that same release never reached them: a 50 ms retry-delay cap took 5,615 ms through an extension against 170 ms through the core, and a timeout that threw on the core path was accepted in silence on all three extension paths. 948 tests were green; the review bot caught it.
The uncomfortable part is that the release existed to fix this exact shape, a rule living in three copies and enforced in one, and reproduced it within itself. The fix was to pass the whole object rather than add the two missing fields, so the next field cannot drift.
Queueing work is not running it. A timeout test queued
ProcessorCount * 4blocking work items to saturate the thread pool, then asserted immediately without waiting for any worker to start. It passed on every run without ever creating the condition it was written to create. Making the workers signal a countdown turned it red at once, and what it exposed was not the intermittent failure it was written for but a wrong exception type: under real saturation the pool cancels the queued task before the action starts, and the strategy surfaced a cancellation exception where its contract promised a timeout.Both cite specific incidents, per the bar in "Keeping this document honest".
Note there are now six open PRs against this file, all adding gates and none merged. Worth a batch merge rather than six reviews.
Summary by CodeRabbit