fix(server): preserve non-404 stage Airflow failures - #378
Conversation
kstonekuan
left a comment
There was a problem hiding this comment.
LGTM, merging. Welcome, and this is unusually complete for a first PR.
All three suppressed call sites are held in both directions, which is the property that matters: the point of #312 was to stop swallowing real failures without losing the degradation the suppression existed for.
call site non-404 must refuse 404 must still degrade
stage task instances ..._stage_task_failure_without_status_maps_to_502 ..._tolerates_missing_task_detail_for_a_matched_stage_run
stage run listing ..._remote_stage_listing_failure_is_a_sanitized_502 ..._tolerates_unregistered_stage_sub_dags
recent stage runs test_runs_maps_stage_rate_limit_to_502 test_runs_tolerates_an_unregistered_stage_dag
Each of those six is the only failure when its side is reverted. Two of the three degradation tests are pre-existing, which is the better kind of evidence: the tests that defined the old behaviour are what constrain the new one.
DoD 4 is specifically covered rather than incidentally. error.status == 404 is False for None, so a status-less error takes the 502 path, and treating None as a 404 fails a test that exists for exactly that:
status-less errors treated as 404 -> test_run_graph_stage_task_failure_without_status_maps_to_502
The sanitization test is the one I would have written. Seeding the environment with a real-looking internal URL and token, putting both plus a traceback into the upstream error message, and then asserting the response text contains none of them:
assert "airflow.internal.corp" not in response.text
assert "minted-token" not in response.text
assert "Traceback" not in response.textThat is DoD 5 as a test rather than an inspection, and it checks the whole response rather than just the detail field, so a leak through some other key would still fail it.
Threading source into stage_task_instances rather than reaching for a module-level value keeps the refusal using the same RuntimeSource the rest of the route already resolved, so the local and remote details stay distinguishable.
Gate clean with current main merged in: ruff check, ruff format --check, ty check, 1440 passed / 6 skipped.
Closes #312. If you want another, #311 covers the pagination on the other side of the same client and is close to this in shape, though there is an open PR against it right now, so #310's neighbours in the advanced list are the safer pick: https://github.com/Hebbian-Robotics/hflow/issues?q=is%3Aissue+is%3Aopen+label%3Aadvanced
Summary
AirflowClientErrorfailures, including errors without an HTTP status.Fixes #312
Why
Stage run surfaces previously converted authentication, rate-limit, upstream 5xx, and status-less Airflow failures into empty lanes or recent-run strips. Restricting degradation to 404 keeps the optional-stage behavior while surfacing real upstream failures through the same sanitized 502 path already used for master DAG failures.
Validation
uv run ruff check --fix— passed.uv run ruff format— passed; 208 files unchanged.uv run ty check— passed.uv run pytest -q packages/hflow-server/tests/test_server_runtime.py packages/hflow-server/tests/test_server_run_graph.py— 35 passed.uv run pytest -q— 1440 passed, 6 skipped after installing the documented ffmpeg/ffprobe prerequisite.Checklist
uv run ruff check --fix,uv run ruff format, anduv run ty check.