Skip to content

Remote dataset loaders have no timeout: distillateflow() / oildoe() hang forever against a black-holing host #508

Description

@kgdunn

Summary

The remote dataset loaders call pd.read_csv(url) with no timeout, so a slow or black-holing host hangs the caller indefinitely - in a notebook, in a test, or inside an agent tool call. There is also no caching, so every call re-downloads the file.

Where

Line numbers as of ac52aab (branch claude/repo-error-audit-si62bq, PR #500).

  • src/process_improve/experiments/datasets.py:13-27 - _read_remote_csv, the shared helper
  • src/process_improve/experiments/datasets.py:22 - return pd.read_csv(url)
  • Callers include distillateflow() (:30) and oildoe() (:84); the bundled loaders (pollutant, golf, boilingpot, solar) read from _DATASETS_DIR and are unaffected.

Evidence

def _read_remote_csv(url: str) -> pd.DataFrame:
    ...
    try:
        return pd.read_csv(url)
    except (OSError, ValueError) as exc:
        raise RuntimeError(...)

pd.read_csv(<url>) goes through urllib with no timeout. The test suite already knows better - tests/test_multivariate.py fetches its remote fixture with urlopen(req, timeout=10). The library is less careful than its own tests.

Why it is wrong

distillateflow() pulls 44 640 rows and oildoe() another remote file. Against an unreachable-but-not-refusing host, the call blocks forever: no timeout, no retry policy, no way for a caller to bound it. Inside the MCP tool surface that means a tool call that never returns (the tool timeout will fire, but the worker stays blocked in a syscall). Re-downloading on every call also makes the doc builds and any dataset-marked tests needlessly slow and flaky.

Suggested fix

Fetch explicitly with a bounded timeout and hand the buffer to pandas:

with urllib.request.urlopen(url, timeout=30) as response:   # nosec - fixed https URLs
    payload = response.read()
return pd.read_csv(io.BytesIO(payload))

Keep the existing RuntimeError wrapping (add TimeoutError/URLError to the caught set) so the failure message still names the dataset and the URL.

Optionally add an on-disk cache under platformdirs.user_cache_dir keyed by URL, with an env var to bypass it. platformdirs is not currently a dependency, so check whether adding it is wanted before reaching for it - a simple cache directory under the package's existing data dir may be preferable to a new dependency.

Acceptance criteria

  • Every network read in src/ has an explicit timeout.
  • A timeout surfaces as the module's documented RuntimeError (naming the dataset), not as a bare URLError/TimeoutError.
  • Test with a monkeypatched fetch that raises TimeoutError, asserting the wrapped error message.
  • If caching is added: a second call inside one session performs no second request.

Scope / non-goals

Do not change the bundled-CSV loaders. Do not change the returned DataFrames' contents or column names.

References

Found during the repo-wide audit in PR #500.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions