Skip to content

Commit

Permalink
Fix flake on test_path_temporary_files_are_removed (#2059)
Browse files Browse the repository at this point in the history
I was seeing this test fail in CI (example runs below).  It turns out this test
has a race condition - the test asserts that the temp file has been deleted, but
the temp file is deleted asynchronously on a
concurrent.futures.Future.add_done_callback() callback.  There is no way to ask
a Future to wait until callbacks are finished running (without having the
callback itself perform some sort of coordination, which feels like overkill for
our needs here).

I fixed this with a quick bodge - we test if the file has been deleted, but if
it's still there we wait 200ms and try again.  It's ugly but it works.

Example failures:
- https://github.com/replicate/cog/actions/runs/11858411614/job/33048999811
- https://github.com/replicate/cog/actions/runs/11858411614/job/33049000193
- and other jobs on the same workflow run
  • Loading branch information
philandstuff authored Nov 15, 2024
1 parent d714a70 commit 746ec53
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions python/tests/server/test_http_input.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,15 @@ def test_path_temporary_files_are_removed(client, match):
},
)
temporary_path = resp.json()["output"]

# HACK: the temp file is deleted in a concurrent.futures callback, which
# isn't guaranteed to return before the future result resolves. Therefore
# the file might still exist at the point we receive the HTTP response.

if not os.path.exists(temporary_path):
return # the file is gone, we're done
# else wait and try again
time.sleep(0.2)
assert not os.path.exists(temporary_path)


Expand Down

0 comments on commit 746ec53

Please sign in to comment.