Repository navigation
Fix the flaky timer precision and Windows handle-count gates (#30, #26) - #32
Conversation
timer_precision compared the loop's median expiry lateness for a 250 us deadline against a fixed 500 us (native) / 2 ms (WASI) bound. The M1 Windows spike had already measured 275.2 us p50 on real hardware, so that bound carried under a factor of two of clear air, and a loaded CI VM crossed it with no backend regression: 756.2 us on main run 34947726746 and 594.2 us on PR #29 run 34950806076 within an hour, and 2.31 ms on WASI 0.3 during #22, whose samples spanned 1.07-6.44 ms. Each failure blocked a merge or skipped the release workflow (issue #30). Two independent requirements replace the single fixed median. Capability: the second-best expiry of twenty must still meet the same platform floor. Load only ever makes a sample later, never earlier, so this clause cannot be excused by a loaded or mismeasured host, while a wait floor or a deadline rounded up to milliseconds raises every sample and fails it. Typical case: the median must stay within the larger of that floor and twice what this host achieves with its own sleep, measured interleaved with the expiries so a load spike moves the bound as well as its subject. The calibration never touches the Driver, so no backend regression can move it. Measured on CI, it tracks the loop within a few per cent on windows-2025 (280.5-292.2 us against 285.1-298.5 us), ubuntu-24.04 (65.5 us against 69.6 us) and both WASI targets, and is 8.7x too pessimistic on macos-15, which coalesces nanosleep to 1.66 ms while the loop resolves 189.7 us -- exactly why the capability clause does not depend on it. Only a demonstrably loaded host retries, at most three times: an attempt whose own sleep stays inside half the quiet-host floor fails immediately. Every attempt reports min/2nd/p50/p90/max for both measurements and which clause it missed.
loop_drop_terminates_live_children_and_releases_their_handles sampled GetProcessHandleCount once per cycle and compared it to an exact baseline. It failed on windows-2025 in PR #24's run 34941544032 with 83 against 82, on a PR that only changed release scripts (issue #26). The outstanding handle is a WaitCompletionPacket, and it is not leaked. The backend learns a child exited through RegisterWaitForSingleObject, which the thread pool backs with such a packet. UnregisterWaitEx(INVALID_HANDLE_VALUE) is the strongest join Windows documents and it joins the callback; the pool closes its own packet afterwards, on its own threads. An instrumented 192-cycle sweep on windows-2025 (run 34954538795) saw the table oscillate 130 -> 131 -> 130, gaining and releasing exactly one WaitCompletionPacket, on 117 to 138 of the 192 rounds with eight busy threads, and move zero times on the same idle runner. So the comparison stays exact and gains a bounded deadline instead of a single sample: settled() requires the count to be exactly the baseline again within five seconds. A leaked handle never comes back and still fails, and now names itself -- handle_table walks this process's handle table with GetHandleInformation and NtQueryObject(ObjectTypeInformation), and the failure reports what the table gained and released. The two other whole-process handle comparisons in this file get the same treatment for the same reason.
|
Caution Review failedFailed to post review comments. GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. This happened while posting 2 inline comments. Use ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. ⏰ Context from checks skipped due to timeout. (30)
🧰 Additional context used🪛 LanguageTooldocs/lanes/flakes.md[style] ~74-~74: Consider an alternative for the overused word “exactly”. (EXACTLY_PRECISELY) [grammar] ~135-~135: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) [locale-violation] ~189-~189: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects. (AFTERWARDS_US) [style] ~213-~213: Consider an alternative for the overused word “exactly”. (EXACTLY_PRECISELY) 🪛 markdownlint-cli2 (0.23.2)docs/lanes/flakes.md[warning] 203-203: Fenced code blocks should have a language specified (MD040, fenced-code-language) 📝 WalkthroughWalkthroughThe pull request replaces the fixed timer median check with calibrated, bounded validation. It also adds delayed Windows handle settling and named diagnostics. Documentation records the causes, fixes, and verification results for both CI flakes. ChangesCI flake fixes
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: High Sequence Diagram(s)sequenceDiagram
participant timer_precision
participant precision_check
participant Driver
participant thread_sleep
timer_precision->>precision_check: run check
precision_check->>Driver: measure timer expiry
precision_check->>thread_sleep: measure host sleep
precision_check-->>timer_precision: return or panic after retries
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The Windows handle-count changes are separate from issue Full details: Docstring CoverageExplanation Docstring coverage is 63.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 2 files. (2 skipped: 2 unsupported.)
✨ 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 |
Fixes #30 and #26. Report with the full evidence:
docs/lanes/flakes.md.#30
timer_precision: the bound sat below the platform floorspikes/iocp/WINDOWS_RESULTS.mdhad already measured 275.2 µs p50 for this same 250 µs deadline against a fixed 500 µs bound.max(floor, 2 × the host's own interleaved thread::sleep median).Driver. It tracks the loop within a few percent on windows-2025, ubuntu-24.04 and both WASI targets, and is 8.7× too pessimistic on macos-15 (nanosleep coalescing) — which is why the capability clause does not depend on it. Retries are bounded at 3 and only for a demonstrably loaded host.#26 Windows handle count: not a leak
WaitCompletionPacket, identified by walking the handle table withNtQueryObject.RegisterWaitForSingleObjectis backed by a thread-pool packet;UnregisterWaitEx(INVALID_HANDLE_VALUE)joins the callback, then the pool closes its own packet on its own threads.NtAssociateWaitCompletionPacketon the driver's own port would remove the thread pool from this path.CI
Three fully green runs: 34956471189 and 34957283051 (consecutive, same code) and 34958380080 on the final SHA, 38/38 jobs each.
Summary by CodeRabbit
Bug Fixes
Documentation