feat(ingest): bound the default http transport in time, in front of the pinned library (S2.4)
The hole: `read_http`'s default transport is the library's `urllib_get`, which invokes the
stdlib opener with no `timeout=`. urllib's documented fallback is then the process-wide default
socket timeout — `None` out of the box — so an http source that accepts a connection and never
answers hangs a run indefinitely. That contradicts the invariant that nothing runs unbounded.
The spec text for S2.4 ("a timeout parameter on `_urllib_get`") could NOT be followed literally:
that function is UPSTREAM library code (`llm_ingestion_okf.connectors`, pinned v0.3.1, pull-only),
signature `(url, credential) -> str` — measured, not assumed. Same failure class as S2.2's
"implement it in `ingest.py`": spec text that says "change X" has to be checked against whether
X is ours at all.
So the fix goes in FRONT of the library: `timeout_get` scopes `socket.setdefaulttimeout` around
a delegate call to the library's own `urllib_get`, and `materialize` now hands the library that
wrapped transport instead of letting it resolve its own untimed default. This meets S2.4's own
verification criterion — a bound WITHOUT a second socket path — and avoids duplicating the
credential-header logic. An explicitly injected `http_get` is passed through UNWRAPPED: a
caller-owned transport (MCP fronts a subprocess with its own `timeout_seconds`) keeps its own
policy, and a process-global side effect is not ours to impose on it.
Honest limit, carried in the code comment, the test docstring and `docs/extending.md`, not just
in the commit: the default socket timeout is PROCESS-global. Under `concurrency=k` the runner is
asyncio on one thread, so the scoping holds; driving `read_http` from a thread-pool executor
would make it unsafe.
Half of S2.4's scope was already delivered upstream — transport failures are categorised as
`SourceError(code="http_transport")`. Coarser than the plan envisaged, but not ours to rewrite.
Two pre-existing guards went red on the first pass, both on PROSE only: `ingest.py` must not
contain "urlopen" (no forked connector) or "ingest_mcp" (AST-guarded mcp-free). No code violated
either — my docstrings merely named them. The guards were left exactly as strict as they were and
the prose was reworded; weakening a real guard to save a comment is the trade this repo refuses.
578 -> 583 tests. Five mutations MEASURED red (restored from scratchpad + `shasum -c` each time,
never `git checkout`):
1. remove the timeout scoping entirely -> RED
2. apply the bound AFTER the delegate call -> RED
3. set the bound but never restore it (no finally)-> RED (the unconditional control)
4. hand the library a bare None again (pre-S2.4) -> RED (the wiring)
5. make the wrapping unconditional -> RED (the conditional control)
Mutations 1 and 2 take ~10s to fail rather than failing instantly: that is the loopback test's
join deadline expiring. It is the measurement that the bound actually BITES — a black-hole
listener on 127.0.0.1 that completes the handshake and never answers, run on a daemon thread so
a detached seam fails an assertion instead of hanging the suite forever. Every other assertion
here only proves we set a global; that one proves the global does something.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdLGwd33vqhkToh98Ym34P
This commit is contained in:
parent
ddd6338f02
commit
8910a673ea
3 changed files with 292 additions and 6 deletions
|
|
@ -33,6 +33,7 @@ dependencies and imports no ``agent_framework`` / ``mcp``. Guarded by
|
|||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
import socket
|
||||
from pathlib import Path
|
||||
|
||||
from llm_ingestion_okf import (
|
||||
|
|
@ -64,10 +65,10 @@ from llm_ingestion_okf.render import render_fenced_block, render_table
|
|||
|
||||
# Two historical PRIVATE names the repo's existing tests bind to, re-exported so those bindings
|
||||
# survive the adoption unchanged. `_urllib_get` backs an identity assertion that the default http
|
||||
# transport is the real socket path rather than a stub (tests/test_ingest_http.py);
|
||||
# `_sql_value_to_text` is exercised directly for the §5 bool/BLOB refusals
|
||||
# (tests/test_ingest_sql.py). Not in `__all__` — they are not part of this module's contract.
|
||||
from llm_ingestion_okf.connectors import urllib_get as _urllib_get # noqa: F401
|
||||
# transport is the real socket path rather than a stub (tests/test_ingest_http.py), and is the
|
||||
# delegate `timeout_get` wraps below; `_sql_value_to_text` is exercised directly for the §5
|
||||
# bool/BLOB refusals (tests/test_ingest_sql.py). Not in `__all__` — not part of this contract.
|
||||
from llm_ingestion_okf.connectors import urllib_get as _urllib_get
|
||||
from llm_ingestion_okf.render import sql_value_to_text as _sql_value_to_text # noqa: F401
|
||||
|
||||
#: The repo's historical name for the §4 top-level model. The library calls it ``Manifest`` and
|
||||
|
|
@ -75,7 +76,15 @@ from llm_ingestion_okf.render import sql_value_to_text as _sql_value_to_text #
|
|||
#: repo-local name stable for callers that bound to it before the library adoption.
|
||||
ManifestV1 = Manifest
|
||||
|
||||
#: The wall-clock bound applied to the default http transport (S2.4). A source that accepts a
|
||||
#: connection and then never answers must not be able to hang a run: the repo requires stop
|
||||
#: criteria and budget caps at startup precisely so nothing runs unbounded, and an untimed socket
|
||||
#: is the same failure wearing a different hat. Chosen to match the MCP transport's own
|
||||
#: `timeout_seconds` default so both transports in the `http` family fail on the same clock.
|
||||
HTTP_TIMEOUT_SECONDS = 30.0
|
||||
|
||||
__all__ = [
|
||||
"HTTP_TIMEOUT_SECONDS",
|
||||
"Extraction",
|
||||
"FileSource",
|
||||
"HttpGet",
|
||||
|
|
@ -99,9 +108,41 @@ __all__ = [
|
|||
"read_sql",
|
||||
"render_fenced_block",
|
||||
"render_table",
|
||||
"timeout_get",
|
||||
]
|
||||
|
||||
|
||||
def timeout_get(get: HttpGet, *, timeout: float = HTTP_TIMEOUT_SECONDS) -> HttpGet:
|
||||
"""Wrap an http transport so its socket operations are time-bounded (S2.4).
|
||||
|
||||
The library's ``urllib_get`` is the only socket path, and it invokes the stdlib opener with no
|
||||
``timeout=`` argument. urllib's documented fallback in that case is the process-wide default
|
||||
socket timeout — ``None`` unless set — so an unanswered connection blocks forever. Scoping
|
||||
``socket.setdefaulttimeout`` around the delegate call therefore bounds the real transport
|
||||
WITHOUT opening a second socket path and WITHOUT re-implementing the credential header, which
|
||||
is what S2.4 asks for: the fix belongs in front of the pinned library, not inside it.
|
||||
|
||||
The previous default is restored in a ``finally``, so a transport failure cannot leak the
|
||||
bound onto unrelated sockets.
|
||||
|
||||
KNOWN CAVEAT — the default socket timeout is PROCESS-global, not per-call. Under
|
||||
``concurrency=k`` the runner is asyncio on a single thread, so this scoping holds. If
|
||||
``read_http`` is ever driven from a thread-pool executor, this is NOT thread-safe and the
|
||||
bound has to move to a per-call timeout argument on the opener — i.e. to owning a socket
|
||||
path in this module, which is exactly what this wrapper exists to avoid.
|
||||
"""
|
||||
|
||||
def bounded(url: str, credential: str | None) -> str:
|
||||
previous = socket.getdefaulttimeout()
|
||||
socket.setdefaulttimeout(timeout)
|
||||
try:
|
||||
return get(url, credential)
|
||||
finally:
|
||||
socket.setdefaulttimeout(previous)
|
||||
|
||||
return bounded
|
||||
|
||||
|
||||
def load_manifest(path: str | Path) -> tuple[Manifest, str]:
|
||||
"""Validate a manifest fail-fast, BEFORE any source access (§4, §9).
|
||||
|
||||
|
|
@ -141,13 +182,21 @@ def materialize(
|
|||
``http`` source is refused fail-fast unless it is set, so the manifest itself can never
|
||||
grant network access. ``http_get`` optionally injects the transport seam; both are ignored
|
||||
for ``file``/``sql`` sources. Call ``materialize_bundle`` directly when the stamp is
|
||||
wanted alongside the paths."""
|
||||
wanted alongside the paths.
|
||||
|
||||
The DEFAULT transport is time-bounded (S2.4): rather than letting the library resolve its own
|
||||
untimed ``urllib_get``, this passes that same socket path wrapped in :func:`timeout_get`, so
|
||||
an unanswered http source fails typed instead of hanging the run. An explicitly injected
|
||||
``http_get`` is passed through UNWRAPPED — a caller-supplied transport (the MCP one fronts a
|
||||
subprocess with its own ``timeout_seconds``; test stubs touch nothing) owns its own timeout
|
||||
policy, and a process-global socket bound is not ours to impose on it."""
|
||||
transport = http_get if http_get is not None else timeout_get(_urllib_get)
|
||||
return list(
|
||||
materialize_bundle(
|
||||
Path(manifest_path),
|
||||
Path(bundle_dir),
|
||||
ingested_at,
|
||||
allow_network=allow_network,
|
||||
http_get=http_get,
|
||||
http_get=transport,
|
||||
).written
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue