Skip to content

Bound FITS-header fetch so a dead peer can't stall the frame - #765

Merged
thusser merged 2 commits into
developfrom
feature/fits-header-fetch-timeout
Aug 16, 2026
Merged

Bound FITS-header fetch so a dead peer can't stall the frame#765
thusser merged 2 commits into
developfrom
feature/fits-header-fetch-timeout

Conversation

@thusser

@thusser thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member

Summary

FitsHeaderMixin.add_requested_fits_headers() awaited every requested header future with no timeout. The futures are created before the exposure and awaited after readout, so a peer that never answers its IQ stalls frame finalization for the full XMPP IQ timeout (~120s — slixmpp's Iq.send default).

Trigger (issue #764): admin was a pyobs-gui on a laptop sent to sleep without closing the client. Its session lingered on ejabberd, and fli230 spent ~120s per exposure fetching headers from it before giving up.

Change

  • Bound the whole collection to a single asyncio.wait(..., timeout=...) deadline; cancel + gather anything still pending, then skip it with a timed out warning (mirrors the existing RemoteError path).
  • New fits_header_timeout: float = 15.0 kwarg on FitsHeaderMixin, stored as _fitsheadermixin_header_timeout.

A @timeout decorator was considered and rejected: it works by having the server reply with a method_timeout IQ before running, and a dead peer never replies at all.

Test plan

  • ruff check / black --check clean; pyrefly check clean on fitsheader.py.
  • pytest tests/mixins/test_fitsheader.py: 43 passed, including two new tests (dead peer times out and is cancelled; live+dead mix adds live headers and skips dead).

See specs/plans/2026-08-16-fits-header-fetch-timeout.md for the plan.

Note: this branch also contains commit 1e2b81d5 ("Mark core-tier test baseline plan implemented"), which the graphify automation committed to the branch on checkout — unrelated to this change.

@thusser

thusser commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Found and fixed two bugs while reviewing this (commit caa5621):

  1. Crash on empty futures. asyncio.wait(futures.values(), timeout=...) raises ValueError: Set of Tasks/Futures is empty. whenever futures is {} — which request_fits_headers() returns for any module with no comm, or with comm but no peer implementing IFitsHeaderBefore/IFitsHeaderAfter (dev/test setups, single-module deployments). Previously a harmless no-op loop, now crashes every exposure on that path. None of the existing tests (old or new) exercised an empty dict, and the one test that runs a real grab_data() (test_aborted_exposure_raises_aborted_error) aborts before reaching header assembly, so this shipped uncaught. Fixed by skipping the asyncio.wait call when futures is empty.

  2. fits_header_timeout never reaches BaseCamera/BaseVideo. Both call ImageFitsHeaderMixin.__init__ with an explicit keyword list rather than forwarding **kwargs (unlike basespectrograph.py, which does), so a configured fits_header_timeout silently fell into Module/Object's catch-all kwargs and was dropped — defeating the PR's stated purpose for fli230 (a BaseCamera), the exact module in the motivating incident. Fixed by adding an explicit fits_header_timeout param to both and threading it through.

Added regression tests for both, plus a fits_header_timeout-reaches-mixin test for BaseSpectrograph (that path was already correct via **kwargs, just untested). Full tests/mixins/ + tests/modules/camera/ suite (134 tests) passes; ruff/black/pyrefly clean.

@thusser thusser left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on head caa5621: pytest (fitsheader + basecamera/basespectrograph/basevideo) 87 passed, ruff/black/pyrefly clean.

Fix is correct and a real improvement (concurrent await under one deadline vs the old sequential awaits). Two non-blocking notes:

  1. future.result() only guards RemoteError. A peer that errors with something else (or a task cancelled externally) still propagates. Same as the old await, so no regression, but worth a follow-up if peers ever fail with non-RemoteError.

  2. 1e2b81d5 (test-baseline plan 'implemented') rides along here and touches specs/. Drop it if you want a clean single-purpose PR; otherwise harmless doc churn.

LGTM.

add_requested_fits_headers() awaited every header future with no timeout, so
a peer that never answers its IQ (e.g. a laptop put to sleep without closing
its client) stalled frame finalization for the full ~120s XMPP IQ timeout.
Bound the collection to a single asyncio.wait() deadline and skip anything
still pending. New fits_header_timeout kwarg (default 15s) on FitsHeaderMixin.
Fixes #764.
…g BaseCamera/BaseVideo

asyncio.wait() raised ValueError on an empty futures dict (any module with no
comm, or no peer implementing IFitsHeaderBefore/After), crashing every
exposure. BaseCamera and BaseVideo also passed an explicit keyword list to
ImageFitsHeaderMixin.__init__ instead of forwarding **kwargs, so a configured
fits_header_timeout was silently dropped for exactly the module (fli230,
BaseCamera) that motivated this PR.

Adds regression tests for both, plus a fits_header_timeout-reaches-mixin
guard for BaseSpectrograph (already correct via **kwargs, previously
untested).
@thusser
thusser force-pushed the feature/fits-header-fetch-timeout branch from caa5621 to d6aa40e Compare August 16, 2026 19:32
@thusser
thusser merged commit a35487e into develop Aug 16, 2026
3 checks passed
@thusser
thusser deleted the feature/fits-header-fetch-timeout branch August 16, 2026 19:39
thusser added a commit that referenced this pull request Aug 16, 2026
PR #765 merged. Records the two bugs found and fixed during review
(empty-futures crash, fits_header_timeout not reaching BaseCamera/BaseVideo)
and the follow-up filed as issue #767 for the narrow RemoteError catch.
thusser added a commit that referenced this pull request Sep 2, 2026
* Bound FITS-header fetch so a dead peer can't stall the frame

add_requested_fits_headers() awaited every header future with no timeout, so
a peer that never answers its IQ (e.g. a laptop put to sleep without closing
its client) stalled frame finalization for the full ~120s XMPP IQ timeout.
Bound the collection to a single asyncio.wait() deadline and skip anything
still pending. New fits_header_timeout kwarg (default 15s) on FitsHeaderMixin.
Fixes #764.

* Fix crash on empty header futures and fits_header_timeout not reaching BaseCamera/BaseVideo

asyncio.wait() raised ValueError on an empty futures dict (any module with no
comm, or no peer implementing IFitsHeaderBefore/After), crashing every
exposure. BaseCamera and BaseVideo also passed an explicit keyword list to
ImageFitsHeaderMixin.__init__ instead of forwarding **kwargs, so a configured
fits_header_timeout was silently dropped for exactly the module (fli230,
BaseCamera) that motivated this PR.

Adds regression tests for both, plus a fits_header_timeout-reaches-mixin
guard for BaseSpectrograph (already correct via **kwargs, previously
untested).
thusser added a commit that referenced this pull request Sep 2, 2026
PR #765 merged. Records the two bugs found and fixed during review
(empty-futures crash, fits_header_timeout not reaching BaseCamera/BaseVideo)
and the follow-up filed as issue #767 for the narrow RemoteError catch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant