Repository navigation
quest: plan the receive-credit flake and Python test timeouts - #7807
Conversation
m0: javascript_partial_frame_gives_way_qmux completed an oversized frame on moq-transport-18 in 1 of 4 local runs during #7729's iterate; no CI run has failed it. m2: bound every py/ test with one pytest-timeout setting, so a hung read fails instead of hanging the suite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 2388706.
- P2: Bound a native hang outside the test process (
quest/m2/py-test-timeouts.md:35–40). The plan says pytest-timeout'sthreadmethod can stop a native call that holds the GIL. Its watchdog is a Pythonthreading.Timer: the callback that prints stacks and callsos._exitmust acquire that same GIL, so an indefinitely held GIL prevents it from firing, just as it prevents the Python signal handler from running. Selecting either plugin method therefore cannot meet the plan's stated native-hang bound. If that case stays in scope, require an independently scheduled process or native watchdog and a regression that deliberately holds the GIL; otherwise explicitly narrow the goal.
No additional actionable issue found in the receive-credit plan. Verification: GitHub-only static inspection of the plans, test harness and pytest-timeout implementation; I did not run tests or reproduce the flake.
(Written by review (OpenAI))
There was a problem hiding this comment.
Important
The Python timeout plan's native-hang claim is wrong: pytest-timeout's thread timer is itself a Python thread that needs the GIL, so it cannot interrupt a native call that holds the GIL any better than signal. The plan should narrow that case or bound it outside the test process.
Reviewed changes
- Receive-credit flake quest: Adds
quest/m0/test-flakes-2/receive-credit-qmux.mdand links it from the test-flakes epic, recording the local-onlyjavascript_partial_frame_gives_way_qmuxfailure onmoq-transport-18with a reproduce/fix plan. - Python test timeouts quest: Adds
quest/m2/py-test-timeouts.mdand links it from m2, proposing onepytest-timeoutbound in the rootpyproject.tomlinstead of editing about 30 call sites.
The earlier OpenAI review at this head already reported the native-hang issue at quest/m2/py-test-timeouts.md:37-40; this run verified it against the plugin source and it remains unaddressed.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
pytest-timeout's thread method also needs the GIL, but moq-ffi's ctypes calls release it: signal stops awaited hangs, thread also stops a blocking native call, and neither stops code that holds the GIL. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior pullfrog review is one bullet in the Python timeout quest, rewritten to fix the earlier native-hang finding.
- Rewrote the
timeout_methodbullet:signalstops an awaited hang;threadadditionally stops a blocking synchronous moq-ffi call because uniffi loads the library withctypes.cdll(CDLL releases the GIL for each call), at the cost of ending the run; a hang that holds the GIL is explicitly out of scope. Verified againstpy/moq-ffi/moq_ffi/__init__.py,py/moq-rs/moq/testing.py, and the uniffi_UniffiLib. The prior thread onquest/m2/py-test-timeouts.md:37-40is resolved by this change.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 1a2eb55acebfe80f6dc35d3364d02028825c23b4, following 23887065d9c855cf90cf110d17a5072fc7794746.
The earlier GIL finding is addressed: the plan no longer claims a Python watchdog can interrupt code holding the GIL, explicitly excludes that case, and distinguishes it from ctypes calls that release the GIL. It also asks for a deliberate hang to validate the chosen method. No additional actionable finding in this update.
Verification: static GitHub inspection of the changed plan and bindings; I did not run pytest or reproduce a native hang.
(Written by review (OpenAI))
…quests Pullfrog on #7748: moq-e2ee-retire.md duplicates main's release/e2ee-retire.md, and smoke-moxygen-cert.md duplicates main's smoke-libressl.md (both from #7747). Remove this PR's copies with quest rm and move their extra evidence into main's. A sweep against #7747, #7795, and #7807 found no other duplicates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Plans two follow-ups from moq-dev/moq#7729. The maintainer approved both and their milestones on 2026-11-04, so
/quest-planran without an interview.Quests
[S]quest/m0/test-flakes-2/receive-credit-qmux.md, under More tests under load:receive_credit::javascript_partial_frame_gives_way_qmuxreported "moq-transport-18: a frame larger than the window completed" in 1 of 4 localjust test receive-creditruns during feat(net)!: authored tracks default to a zero max age #7729's iterate. The evidence is local only. The test is#[ignore]d, so merge-queue runs skip it, and only Interop's "Receive credit recovery" step runs it. None of the 40 most recent Interop runs failed it. The 12 failed scheduled runs from 2026-10-23 to 2026-11-03 failed other steps, and their logs show this test passing (for example run 41668668824). No release-gate run failed either, so there is no--log-failedoutput to quote.[XS]quest/m2/py-test-timeouts.md: 33 test functions and helpers inpy/moq-rs/tests/loop or await a read with nowait_forortimeoutin their bodies, mostly intest_local.pyandtest_server.py. The quest recommendspytest-timeoutwith onetimeoutin the existing[tool.pytest.ini_options], rather than editing about 30 call sites to add deadlines. It keeps the 89 existingwait_for/timeoutcalls, and picks thetimeout_methodfrom how the suite can hang. moq-ffi's ctypes calls release the GIL, sosignalstops awaited hangs andthreadalso stops a blocking native call. Neither stops code that holds the GIL, and moq-ffi calls don't.Overlap search
I searched
origin/main, the open planning PRs moq-dev/moq#7748 and moq-dev/moq#7801, and deleted quests in git history. Neither item is already a quest. #7748'snightly/receive-windows-evicted.mdcovers a differentreceive_credittest (benchmark_receive_windows). The macOSreceive_creditquest was deleted on 2026-10-25 by moq-dev/moq#6732.Merge order
Any order works. #7748 also appends to
quest/m0/test-flakes-2/README.mdandquest/m2/README.md, so whichever lands second gets link-list conflicts. A local merge withqueston PATH resolves them.Suggested follow-ups
quest/m0/nightly/README.mdstill says the macOSreceive_credittimeout belongs to test flakes, but notest-flakes-2child owns it since #6732 deleted that quest. A/quest-auditshould check whether the failure still happens and re-point or drop that line.Checks
quest check --base origin/main(596 documents ok) andsh/markdown.sh checkpass. Nothing was built.Review
Round 2:
quest reviewwith Codex (gpt-6.1-sol, medium effort) on head1a2eb55acebfe80f6dc35d3364d02028825c23b4, base90081ff1f6. It reported no findings. This round reviewed the fix for pullfrog's thread onquest/m2/py-test-timeouts.md:40, which is resolved.threadmethod needs the GIL as much assignal. That's true for code that holds the GIL. But moq-ffi loads throughctypes.cdll, which releases the GIL for each call, sothreaddoes stop a blocking native call. The bullet now states both limits and asks for a deliberately hung test to prove the chosen method.Round 1 on
23887065d9: no findings.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code