ITADN

download_test_pdfs silently swallows thread exceptions, masking flaky CI failures

#3728Opencostajohnt 创建于 2026-04-11
nf-testing
C
costajohntcommented
`tests/__init__.py:128-141` calls `concurrent.futures.wait(futures)` but never iterates the returned futures or calls `.result()` on them. Any exception raised in a worker thread is silently captured and never surfaced. ### The code ```python def download_test_pdfs() -> None: pdfs = read_yaml_to_list_of_dicts(Path(__file__).parent / "example_files.yaml") with concurrent.futures.ThreadPoolExecutor(max_workers=20) as executor: futures = [ executor.submit(get_data_from_url, pdf["url"], name=pdf["local_filename"]) for pdf in pdfs ] concurrent.futures.wait(futures) ``` ### Symptom When a download fails (flaky network, rate limit, external host hiccup), `download_test_pdfs()` returns without raising. The test suite then proceeds assuming every file is local. The first failure surfaces several seconds later as a pytest `FileNotFoundError` when a test tries to open a missing file, which masks the real cause (the actual download failure from minutes earlier). ### Observed impact This surfaced in at least one Windows CI run: the cache restore missed, so `download_test_pdfs()` was triggered, and one of the fetches from `raw.githubusercontent.com` failed silently. The test that eventually opened `ASurveyofImageClassificationBasedTechniques.pdf` failed with a misleading `FileNotFoundError: ...tests/pdf_cache/ASurveyofImageClassificationBasedTechniques.pdf`. Investigating the root cause required correlating the pytest failure back to the network download step, which is more work than necessary for what is essentially "the host had a hiccup." ### Proposed fix Iterate the futures and call `.result()` on each to re-raise worker thread exceptions: ```python with concurrent.futures.ThreadPoolExecutor(max_workers=20) as executor: futures = [ executor.submit(get_data_from_url, pdf["url"], name=pdf["local_filename"]) for pdf in pdfs ] for future in concurrent.futures.as_completed(futures): future.result() # re-raises any exception from the worker ``` If the intent is to tolerate partial failures, collect exceptions and raise a summary: ```python errors = [] for future in concurrent.futures.as_completed(futures): try: future.result() except Exception as e: errors.append(e) if errors: raise RuntimeError( f"Failed to download {len(errors)} test PDF(s). First error: {errors[0]}" ) from errors[0] ``` Either way, failures should be visible at the source, not masked as downstream FileNotFoundError. ### Context Spotted while investigating a Windows CI flake on another PR. The symptom was a confusing `FileNotFoundError` on a test file that looked unrelated to the PR's changes. The actual root cause was this silent swallow plus a flaky external download, neither of which the PR could have caused.
2 条评论