Review PR hardening retry delay timing assertions in Elixir tests
Session from pi-symphony-sessions on Hugging Face by thomasmustier · MIT · condensed by Coders Talk
4msession
0interventions
GOAL
Review PR #78 which replaces flaky fixed-sleep retry-delay assertions with polling helpers and a scheduling-window comparison against main.
- +0Prompt · +0Review PR #78 against main: replace fixed 50ms sleeps in retry-delay assertions with polling helpers and assert due_at_ms against the observed scheduling window instead of the clock after an arbitrary sleep.
- +0mAgent did · +0mLocated and read core_test.exs around the retry-delay assertions to understand the diff context.
- +1mAgent did · +1mSearched lib code for due_at_ms and retry scheduling logic, read retry.ex to understand how due_at_ms is computed.
- +2mAgent did · +2mRead orchestrator.ex around handle_agent_task_down and complete_issue/maybe_schedule_continuation to trace how retries get scheduled after a worker exit.
- +3mAgent did · +3mGrepped test file for all uses of assert_due_in_range, assert_due_scheduled_between and the new wait_for_retry_attempt helper to check consistency across the test suite.
- +4mOutcome · +4mReview flagged a P2 issue: assert_due_scheduled_between compares due_at_ms against a broad window rather than the actual observation time, which could let a wrongly-scheduled retry delay pass unnoticed.
VERDICT
Next time I'd ask the agent to also check whether the new assertion helper actually tightens the bound relative to the scheduling latency, not just whether it removes the sleep. The trace through retry.ex and orchestrator.ex to see how due_at_ms gets computed was what actually surfaced the real gap in the PR's fix.