Skip to content

quest: plan the receive-credit flake and Python test timeouts - #7807

Merged
kixelated merged 2 commits into
mainfrom
quest-plan/followups-2026-11-04
Nov 4, 2026
Merged

kixelated merged 2 commits into
mainfrom
quest-plan/followups-2026-11-04

Conversation

@kixelated

@kixelated kixelated commented Nov 4, 2026 •

Copy link
Copy Markdown
Collaborator

Plans two follow-ups from moq-dev/moq#7729. The maintainer approved both and their milestones on 2026-11-04, so /quest-plan ran without an interview.

Quests

  • m0, [S] quest/m0/test-flakes-2/receive-credit-qmux.md, under More tests under load: receive_credit::javascript_partial_frame_gives_way_qmux reported "moq-transport-18: a frame larger than the window completed" in 1 of 4 local just test receive-credit runs 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-failed output to quote.
  • m2, [XS] quest/m2/py-test-timeouts.md: 33 test functions and helpers in py/moq-rs/tests/ loop or await a read with no wait_for or timeout in their bodies, mostly in test_local.py and test_server.py. The quest recommends pytest-timeout with one timeout in the existing [tool.pytest.ini_options], rather than editing about 30 call sites to add deadlines. It keeps the 89 existing wait_for/timeout calls, and picks the timeout_method from how the suite can hang. moq-ffi's ctypes calls release the GIL, so signal stops awaited hangs and thread also 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's nightly/receive-windows-evicted.md covers a different receive_credit test (benchmark_receive_windows). The macOS receive_credit quest 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.md and quest/m2/README.md, so whichever lands second gets link-list conflicts. A local merge with quest on PATH resolves them.

Suggested follow-ups

quest/m0/nightly/README.md still says the macOS receive_credit timeout belongs to test flakes, but no test-flakes-2 child owns it since #6732 deleted that quest. A /quest-audit should check whether the failure still happens and re-point or drop that line.

Checks

quest check --base origin/main (596 documents ok) and sh/markdown.sh check pass. Nothing was built.

Review

Round 2: quest review with Codex (gpt-6.1-sol, medium effort) on head 1a2eb55acebfe80f6dc35d3364d02028825c23b4, base 90081ff1f6. It reported no findings. This round reviewed the fix for pullfrog's thread on quest/m2/py-test-timeouts.md:40, which is resolved.

  • Fixed: the thread said pytest-timeout's thread method needs the GIL as much as signal. That's true for code that holds the GIL. But moq-ffi loads through ctypes.cdll, which releases the GIL for each call, so thread does 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

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>
@coderabbitai

coderabbitai Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 64392f5b-e85d-4075-bee9-ef76672c9bb0
Commits

Reviewing files that changed from the base of the PR and between 90081ff and 1a2eb55.

Files selected for processing: 4
  • quest/m0/test-flakes-2/README.md
  • quest/m0/test-flakes-2/receive-credit-qmux.md
  • quest/m2/README.md
  • quest/m2/py-test-timeouts.md
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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's thread method can stop a native call that holds the GIL. Its watchdog is a Python threading.Timer: the callback that prints stacks and calls os._exit must 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))

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md and links it from the test-flakes epic, recording the local-only javascript_partial_frame_gives_way_qmux failure on moq-transport-18 with a reproduce/fix plan.
  • Python test timeouts quest: Adds quest/m2/py-test-timeouts.md and links it from m2, proposing one pytest-timeout bound in the root pyproject.toml instead 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Comment thread quest/m2/py-test-timeouts.md Resolved
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>
@kixelated
kixelated added this pull request to the merge queue Nov 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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_method bullet: signal stops an awaited hang; thread additionally stops a blocking synchronous moq-ffi call because uniffi loads the library with ctypes.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 against py/moq-ffi/moq_ffi/__init__.py, py/moq-rs/moq/testing.py, and the uniffi _UniffiLib. The prior thread on quest/m2/py-test-timeouts.md:37-40 is resolved by this change.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Merged via the queue into main with commit eacff68 Nov 4, 2026
7 of 9 checks passed
@kixelated
kixelated deleted the quest-plan/followups-2026-11-04 branch November 4, 2026 04:31
kixelated added a commit that referenced this pull request Nov 4, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant