fix(cards): use time.monotonic() for async-process timeout#3225
Conversation
) `CardProcessManager` measured async card-rendering subprocess lifetime with `time.time()`, a wall-clock that can step backward under NTP correction, container clock skew, or manual `settimeofday`. Two failure modes both bite long-running Card jobs on Kubernetes / Batch: 1. Backward jump → elapsed delta becomes small/negative, timeout never fires, card subprocess leaks indefinitely in `CardProcessManager.async_card_processes`. 2. Forward jump → elapsed delta inflates, subprocess killed early and user sees a missing/incomplete card. Replace the three call sites in `card_creator.py` (one writer, two readers) with `time.monotonic()`, the documented stdlib API for interval measurement. Adds `test/unit/test_card_creator_monotonic.py` with three tests: - registry stores monotonic timestamp, not wall-clock - backward wall-clock jump still triggers timeout-driven reap - in-window monotonic elapsed leaves a running process alone Fixes Netflix#3224
Greptile SummaryThis PR fixes a latent timer bug in
Confidence Score: 5/5Safe to merge — the change is minimal, mechanically correct, and all three call sites are consistently updated. The fix is a straightforward swap of time.time() for time.monotonic() at every read and write site for the elapsed-time value. The stored timestamp is opaque and never crosses a process or serialization boundary, so the change is fully self-contained. The four new regression tests validate the registration path, the timeout-trigger path under a backward clock jump, the within-window no-kill path, and the _run_command replacement path — giving good coverage of the changed logic. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "test(cards): clean up card creator monot..." | Re-trigger Greptile |
The class-level schema comment on CardProcessManager.async_card_processes still documented time.time() after the implementation moved to time.monotonic(). A future reader relying on the comment could incorrectly revert the fix. Bring the comment in line with the implementation. No behavior change.
|
Addressed the P2 finding from Greptile: updated the schema comment on Pushed as |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3225 +/- ##
=========================================
Coverage ? 28.19%
=========================================
Files ? 381
Lines ? 52373
Branches ? 9247
=========================================
Hits ? 14767
Misses ? 36657
Partials ? 949 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Fixes #3224
What
metaflow/plugins/cards/card_creator.pymeasured async card-rendering subprocess lifetime withtime.time(). Replaced three call sites withtime.monotonic()and added a regression unit test.Why
time.time()returns the wall-clock and can step backward under NTP correction, container clock-skew rebase, or manualsettimeofday. Two failure modes both bite long-running Card jobs:time.time() - _async_startedbecomes small or negative → timeout check never fires → the card subprocess leaks indefinitely inCardProcessManager.async_card_processes.time.monotonic()is the documented stdlib API for interval measurement; it cannot move backward and is unaffected by system-clock adjustments.How
Three-line change in
metaflow/plugins/cards/card_creator.py:CardProcessManager._register_card_process()now storestime.monotonic()instead oftime.time()._wait_for_async_processes_to_finish()compares withtime.monotonic()._run_command()async branch compares withtime.monotonic().The stored value is opaque to all readers, so swapping the source is safe — there is no API change, no on-disk format change, and no cross-process value that needs to remain compatible.
Tests
New file
test/unit/test_card_creator_monotonic.pywith three tests:test_register_uses_monotonic_not_wall_clock— registry stores the monotonic timestamp, nottime.time().test_timeout_check_immune_to_wall_clock_backward_jump— simulates a 1-day wall-clock backward jump while monotonic advances 600 s; verifies the subprocess is still reaped at the 60 s timeout.test_timeout_check_not_triggered_when_within_window— sanity check that processes still inside the timeout window are not killed.Local run:
Checklist
AI tool disclosure
Bug surfaced by an audit grep for
time.time()used in interval contexts (an R5 rule in my local PR-audit toolkit). I authored the fix and tests, verified the call sites read as written, ran the tests locally, and can defend the monotonic-vs-wall-clock semantics without reference to an AI tool.