Skip to content

relay_bridge: await the pump task instead of sleeping 50 ms - #3877

Open
jeff-hykin wants to merge 1 commit into
mainfrom
jeff/fix/relay_bridge_test_race
Open

relay_bridge: await the pump task instead of sleeping 50 ms#3877
jeff-hykin wants to merge 1 commit into
mainfrom
jeff/fix/relay_bridge_test_race

Conversation

@jeff-hykin

@jeff-hykin jeff-hykin commented Sep 1, 2026

Copy link
Copy Markdown
Member

Fix flakey test

test_pump_dies_visibly_on_encode_error failed on the 3.11 runner while the
identical tree passed everywhere else: `sleep(0.05)` then `assert
_task.done()` only holds if the pump gets scheduled inside that window, and
a loaded runner does not owe us that. Awaiting the task is what the tests
actually mean, and it still fails fast because the pump either raises or
returns immediately. test_pump_stops_cleanly_on_session_close had the same
race.
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change replaces fixed sleeps in relay pump lifecycle tests with bounded awaits of the pump task. The encode-error and clean session-close paths completed as intended in focused runs, ten repeated scheduling runs, and the complete relay WebTransport client test module.

Confidence Score: 5/5

Safe to merge: the updated lifecycle assertions directly observe the pump task's terminal outcome and passed all exercised paths.

Focused testing confirmed that encoding failures propagate as ValueError, session closure completes normally, and later offers are rejected in both cases. Repeated runs showed no timeout, hang, or scheduling flake.

Files Needing Attention: No files need further attention.

T-Rex T-Rex Logs

What T-Rex did

  • Restored the CPython 3.12 interpreter in the prepared virtual environment and ran the targeted pytest tests pump_dies_visibly_on_encode_error and pump_stops_cleanly_on_session_close; both tests passed with exit code 0. The run also captured a repository-noted non-fatal ResourceWarning that did not affect the results.
  • Ran the two lifecycle tests ten consecutive times and all executions completed without timeouts, hangs, or scheduling failures.
  • Executed the complete dimos/web/relay_bridge/test_wt_client.py module and all 21 tests passed.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "relay_bridge: await the pump task instea..." | Re-trigger Greptile

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main    #3877      +/-   ##
==========================================
- Coverage   77.64%   77.64%   -0.01%     
==========================================
  Files        1323     1323              
  Lines      125419   125416       -3     
  Branches    10911    10911              
==========================================
- Hits        97384    97377       -7     
- Misses      24890    24892       +2     
- Partials     3145     3147       +2     
Flag Coverage Δ
OS-ubuntu-24.04-arm 74.31% <100.00%> (-0.01%) ⬇️
OS-ubuntu-latest 74.85% <100.00%> (-0.01%) ⬇️
Py-3.10 74.85% <100.00%> (+<0.01%) ⬆️
Py-3.11 74.85% <100.00%> (-0.01%) ⬇️
Py-3.12 74.85% <100.00%> (-0.01%) ⬇️
Py-3.13 74.85% <100.00%> (-0.01%) ⬇️
Py-3.14 74.85% <100.00%> (-0.01%) ⬇️
Py-3.14t 74.85% <100.00%> (-0.01%) ⬇️
SelfHosted-Large 30.21% <0.00%> (-0.01%) ⬇️
SelfHosted-Linux 34.72% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
dimos/web/relay_bridge/test_wt_client.py 98.15% <100.00%> (-0.03%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant