fix(runtime): cap polling sleeps to timeout - #80
Conversation
|
👋 Hi @HostX0 — thank you so much for your first contribution to HFlow! A maintainer will review your pull request as soon as possible. In the meantime:
💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game. We are excited to have you here and appreciate your help making the project better! 🙌 |
kstonekuan
left a comment
There was a problem hiding this comment.
Thank you @HostX0, and welcome! You found this one yourself, described the root cause precisely, and pinned it with tests that fail for the right reason. That is a good first contribution.
What I validated locally:
- Full quality gate is clean (
ruff check,ruff format --check,ty check) and all 308 tests pass. - Confirmed both new assertions fail against pre-fix sources: the clock lands on
10.0where the budget was0.3, exactly the overshoot you describe. - Traced both loops. Each was
while time.monotonic() < deadlinefollowed by an unconditionaltime.sleep(poll_interval_s), so the deadline was only ever observed at the top of the next iteration.min(poll_interval_s, remaining_s)is the right shape, and it preserves the polling and error-reporting behavior. - Checked that the
remaining_s <= 0guard is load-bearing rather than defensive:time.sleepraisesValueErroron a negative argument, so computing a remaining budget without that check would have traded an overshoot for a crash. Good instinct.
Worth noting for scope, since your description is careful not to overclaim: no in-repo caller overrides poll_interval_s, so on the shipped hflow up path the overshoot was bounded by the 3 second default. But AirflowClient is public API (exported from hflow.runtime), so anyone calling wait_until_healthy with a longer interval could blow well past their own timeout. That is the case this fixes.
I like the fake-clock approach. Replacing the module reference and advancing time inside sleep makes the assertion exact instead of timing-dependent, which is why these tests are worth having rather than merely passing.
One small thing I would have done differently, not worth a round trip: in test_runtime_client.py you retargeted an existing test's interval from 0.1 to 10.0, which strengthens the deadline assertion but drops it from roughly three poll iterations to one. Both properties matter, so a separate test would have kept both. Not blocking.
Merging now. If you want another, the open good first issues have plenty left (issues with an assignee are taken; everything else is fair game), though given that you went and found this on your own, the more valuable thing is more of exactly this. We hang out on Discord.
|
One process note: this is still marked as a draft, so I cannot merge it. The review above stands and it is approved, so whenever you are ready, click "Ready for review" and I will squash it straight in. If you left it as a draft deliberately because you wanted to keep working on it, take your time and ignore me. |
Summary
Root cause
Both loops checked the deadline only before polling, then always slept for the full
poll_interval_s. When the configured interval exceeded the remaining budget,hflow upcould exceed its timeout by nearly one full polling interval before raising.The fix preserves the polling and error-reporting behavior while sleeping for
min(poll_interval_s, remaining_s).Verification
uv run pytest -q— 308 passed, 3 skippeduv run ruff check src/hflow/runtime/_client.py src/hflow/runtime/_lifecycle.py tests/test_runtime_client.py tests/test_runtime_cli.pyuv run ruff format --check src/hflow/runtime/_client.py src/hflow/runtime/_lifecycle.py tests/test_runtime_client.py tests/test_runtime_cli.pyuv run ty check