Skip to content

Let the callback say whether it still has work - #189

Merged
timkpaine merged 1 commit into
mainfrom
tkp/settle-pending-tasks
Aug 23, 2026
Merged

Let the callback say whether it still has work#189
timkpaine merged 1 commit into
mainfrom
tkp/settle-pending-tasks

Conversation

@timkpaine

Copy link
Copy Markdown
Member

Builds were still failing intermittently — roughly one run in eight. They were easy to miss because the failures were hidden behind re-runs: gh run list reports the final conclusion, so a run that failed on attempt 1 and passed on attempt 2 shows as green. Checking run_attempt surfaced run 32660537412, which failed on its first attempt with the same symptom as before:

FAILED raydar/tests/test_task_tracker.py::TestRayTaskTracker::test_construction_and_dataframe
AssertionError: tracker recorded no finished tasks

Tracker created at 19:13:56, assertion at 19:15:58 — the full 120s wait, so nothing ever arrived.

Why #183 didn't fix it

The settle loop I added there never ran. It asked the tracker whether anything was pending via a separate has_pending_tasks() call. AsyncMetadataTracker is an async actor, so that query can be answered before the callback it was meant to observe has even started.

Measured on an actor of the same shape — submitting callback, callback, then the query from one caller, recording execution order:

max concurrent callbacks: 1
['query-ran', 'callback-a-start', 'callback-a-end', 'callback-b-start', 'callback-b-end']

The query runs first. It sees an empty pending list, reports no work, and the loop exits immediately without retrying anything — leaving the last batch stranded exactly as before. The probe also shows callbacks are serialized, so there was no data race on pending_tasks; ordering was the whole problem.

The fix

callback now returns whether it still holds tasks, and the caller waits on that same call. There is nothing left to race: the answer describes work the callback has already finished doing. has_pending_tasks() is gone.

Verification

test_callback_reports_tasks_the_gcs_has_not_published pins the contract. Dropping the return reproduces the regression:

E  AssertionError: callback must report whether it still has work

53 tests pass locally; lint and checks clean.

Same caveat as #183: this only reproduces under CI timing — the suite passes on macOS with or without the change — so CI is the real judge. Given the ~1-in-8 rate, a couple of green runs would not be meaningful; I'll re-run this branch several times before marking it ready.

The settle loop I added in #183 never ran. It asked the tracker whether
anything was pending through a separate call, and the tracker is an
async actor, so that call can be answered before the callback it was
meant to observe has started. Measured on an actor of the same shape,
submitting callback, callback, query and recording execution order:

    ['query-ran', 'callback-a-start', 'callback-a-end',
     'callback-b-start', 'callback-b-end']

The query runs first, sees an empty pending list, reports no work, and
the loop exits without retrying anything. The last batch is stranded
exactly as it was before, which is why builds still failed roughly one
run in eight, always with an empty dataframe after the full 120s wait.

Have the callback return whether it still holds tasks instead. The
caller waits on that same call, so there is nothing left to race: the
answer describes work the callback has already done.

Still only reproduces under CI timing; the suite passes here either way.

Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
@github-actions

github-actions Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Test Results

53 tests  +1   53 ✅ +1   35s ⏱️ +5s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 481851d. ± Comparison against base commit 0e9e837.

♻️ This comment has been updated with latest results.

@timkpaine
timkpaine marked this pull request as ready for review August 23, 2026 21:20
@timkpaine
timkpaine merged commit 52708d3 into main Aug 23, 2026
11 checks passed
@timkpaine
timkpaine deleted the tkp/settle-pending-tasks branch August 23, 2026 21:20
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