Skip to content

fix(celery): catch the asset-probe timeout instead of hard-killing the worker - #3017

Merged
vpetersson merged 2 commits into
masterfrom
fix/revalidate-soft-time-limit
Jun 7, 2026
Merged

fix(celery): catch the asset-probe timeout instead of hard-killing the worker#3017
vpetersson merged 2 commits into
masterfrom
fix/revalidate-soft-time-limit

Conversation

@vpetersson

Copy link
Copy Markdown
Contributor

Issues Fixed

Sentry: ANTHIAS-A (Hard time limit (30s) exceeded for revalidate_asset_url), ANTHIAS-9 (TimeLimitExceeded), ANTHIAS-B (ForkPoolWorker exited with signal 9 (SIGKILL)) — all three are the same hard-kill.

Description

revalidate_asset_url's 30s hard time limit was reachable by a legitimately slow probe: url_fails can burn a hanging getaddrinfo against a broken resolver (no timeout knob exists for it), then an HTTP HEAD (10s) plus the GET fallback (10s). Tripping the hard limit SIGKILLs the pool child, which surfaces as three separate Sentry issues per occurrence.

  • soft_time_limit=60 / time_limit=90 on the on-demand probe — the soft limit raises SoftTimeLimitExceeded inside the task, which now records the same verdict an HTTP timeout gets (unreachable, last_reachability_check stamped) instead of dying
  • The periodic sweep gets the same treatment: it aborts cleanly one minute before its 30-min hard limit, releasing its Redis singleton lock so the next beat tick starts fresh
  • The hard limits stay as the backstop for a probe stuck in C code where the soft signal can't be delivered

Checklist

  • I have performed a self-review of my own code.
  • New and existing unit tests pass locally and on CI with my changes.
  • I have done an end-to-end test for Raspberry Pi devices.
  • I have tested my changes for x86 devices.
  • I added a documentation for the changes I have made (when necessary).

🤖 Generated with Claude Code

…e worker

- revalidate_asset_url's 30s hard limit was reachable by a legitimate
  probe (DNS stall + HEAD 10s + GET 10s), and tripping it SIGKILLs the
  pool child — three Sentry issues per occurrence (ANTHIAS-A,
  ANTHIAS-9, ANTHIAS-B)
- Add soft_time_limit=60 / time_limit=90: the soft limit raises inside
  the task, which records the verdict an HTTP timeout gets
  (unreachable) instead of dying
- Give the periodic sweep the same treatment: abort cleanly a minute
  before its hard limit, releasing the singleton lock
- Add regression tests for limits and soft-timeout behaviour

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@vpetersson
vpetersson requested a review from a team as a code owner June 7, 2026 11:12
@vpetersson vpetersson self-assigned this Jun 7, 2026
@vpetersson
vpetersson requested a review from Copilot June 7, 2026 11:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts Celery task time limits and handling for asset reachability probes so that slow/hung probes are handled via soft time limits (caught in-task) instead of reaching the hard time limit that SIGKILLs the pool worker process, reducing the related Sentry noise and improving operational stability.

Changes:

  • Introduces explicit soft/hard time-limit constants for the on-demand asset probe and the periodic sweep.
  • Catches SoftTimeLimitExceeded inside both tasks to abort/record outcomes cleanly (instead of worker SIGKILL).
  • Adds unit tests asserting time-limit configuration and verifying soft-limit behavior (unreachable verdict / sweep abort + lock release).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/anthias_server/celery_tasks.py Adds soft/hard time limits and catches SoftTimeLimitExceeded in asset revalidation tasks.
tests/test_celery_tasks.py Adds tests validating time-limit configuration and soft-limit handling behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_celery_tasks.py Outdated
Comment thread src/anthias_server/celery_tasks.py Outdated
Comment thread src/anthias_server/celery_tasks.py Outdated
… catch

- The soft signal is delivered asynchronously, so it can land during
  the row UPDATE as well as the probe; cover the whole task body in
  both the on-demand recheck and the sweep
- Re-raise SoftTimeLimitExceeded past the sweep's blanket per-asset
  handler so the outer abort path sees it
- Satisfy strict mypy on the Optional time-limit comparisons; reword
  a misleading test comment

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@sonarqubecloud

sonarqubecloud Bot commented Jun 7, 2026

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@vpetersson
vpetersson merged commit 633b41e into master Jun 7, 2026
10 checks passed
@vpetersson vpetersson mentioned this pull request Jun 9, 2026
5 tasks
vpetersson added a commit that referenced this pull request Jun 9, 2026
- CalVer (YYYY.0M.MICRO); still June 2026, micro 2 -> 3
- Gives Sentry a real release boundary: every build since 2026.6.2
  reported the same base version (only the +git-hash differed), so
  resolved-in-next-release never stuck and fixed issues kept
  reopening on the next event. A version bump lets the deployed
  fixes actually clear from the board.
- Ships the crash/noise fixes merged since 2026.6.2: SQLite WAL +
  busy timeout (#3015), celery migration-gate (#3016) and
  asset-probe soft limits (#3017), transient-redis/CancelledError
  Sentry filtering + redis healthcheck (#3018/#3028), GitHub
  update-check log level (#3019), webview respawn on D-Bus death at
  setup and mid-play (#3020/#3031), resilient static-file scan
  (#3026), Wayland-socket wait (#3030), and Sentry release/board
  triage tags (#3021/#3025)

Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
vpetersson added a commit that referenced this pull request Jun 11, 2026
* fix(celery): soft-limit the display-power and telemetry pokes

- ANTHIAS-A/9/B group by the worker-SIGKILL signature, not the task,
  so they survived #3017 — which only soft-limited the asset probe
- get_display_power and send_telemetry_task still ran under a bare
  time_limit=30; a wedged CEC query or a getaddrinfo stall (requests'
  timeout doesn't cover DNS) tripped the hard limit and SIGKILLed the
  pool child
- give both the #3017 treatment: soft_time_limit raises inside the
  task so it logs and skips the tick; the hard limit stays as the
  C-code backstop
- add regression tests for the limits and the soft-timeout skip

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* fix(celery): write display_power value and TTL atomically

- a soft-limit signal between SET and EXPIRE could leave display_power
  without a TTL (stale value that never expires)
- use a single SET with ex= so the write and TTL are atomic

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

* fix(telemetry): write cooldown key and TTL atomically

- send_telemetry_task now runs under a soft time limit, so a
  SoftTimeLimitExceeded landing between the cooldown SET and EXPIRE
  would leave telemetry-cooldown without a TTL — silencing telemetry
  permanently (same class as the display_power fix in f5e9466)
- collapse the SET + EXPIRE into one SET … ex= so the value and TTL
  are written atomically
- update test_telemetry to assert the atomic SET and accept ex= in
  the fake redis client

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
vpetersson-bot added a commit to vpetersson-bot/Anthias that referenced this pull request Aug 13, 2026
Two of Copilot's four suppressed comments were right.

The System Info card can report 'Mixed' — PowerStatus.UNKNOWN, which
lib/cec returns when two attached displays disagree or one is
mid-transition. The docs listed every other reading and omitted that
one, so an operator meeting it would have no way to tell whether it was
a fault. Replaced the prose with the full table of readings.

apply_power's comment said "Blank first" while the call sends unblank
on the way up. Reworded to name both directions.

The other two (parse_hhmm's warning-level logs are potential noise) are
left as they are: the schedule silently does nothing when it cannot
parse a time, so that warning is the only signal an operator gets, and
WARNING does not file a Sentry event the way the runaway ERROR pattern
in Screenly#3017/Screenly#3063 did.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
vpetersson pushed a commit that referenced this pull request Aug 14, 2026
…a display schedule (#3277)

* feat(cec): rewrite HDMI-CEC on the kernel uABI, add a display schedule

Replaces libcec with the kernel CEC API (/dev/cec*), fixes device
passthrough, and adds a scheduled display on/off.

Mechanism
- New lib/cec.py talks to /dev/cec* via ioctl from pure stdlib: no
  subprocess, no native library, no core-dump workaround.
- libcec cannot select an adapter — it enumerates the kernel adapter as
  com port 'Linux' and treats /dev/cecN as a Pulse-Eight serial port, so
  `cec-client ... /dev/cec1` fails to open at all (11.07s of retries,
  measured). The kernel API addresses each node directly.
- Every operation fans out across all adapters with a live link, so a
  device with two monitors attached does not leave the second one lit.
- Drops cec==0.2.8, libcec7, cec-utils and libcec-dev.

Passthrough
- Which /dev/cec* exist is a host property, not a board property. The
  board-cased sed in upgrade_containers.sh left pi2/pi3/pi3-64/pi4-64 on
  a useless /dev/vchiq; enumerate the host instead and emit a generated
  docker-compose.cec.override.yml.
- Removes the now-dead vchiq mounts and their per-board strip logic.

Schedule
- New lib/display_power.py plus a per-minute, edge-triggered beat task.
- Falls back to the viewer's blank/unblank when no CEC display answers,
  so it works on plain monitors too.

Notable fixes found on hardware
- A failed logical-address claim surfaced as 'no peer', reintroducing the
  exact conflation of GH #3267; failures are now a distinct ERROR state.
- The kernel returns EBUSY when configuring an already-configured
  adapter, and libcec leaves every adapter configured after it exits, so
  the claim now always clears first. Without this, CEC would have failed
  on every device upgrading from the libcec build.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* fix(cec): address self-review findings

- enable_ssl.sh / disable_ssl.sh recreate anthias-server with their own
  -f list, which omitted the CEC override and so silently brought the
  container back with no /dev/cec* passthrough.
- Scheduled power now sends CEC *and* the local blank. A device can have
  a CEC TV on one output and a plain monitor on another; CEC alone
  reported success and left the monitor lit. A monitor whose EDID
  advertises no CEC is not even counted in `attempted`, so no count
  could have detected it.
- Disabling the schedule while the display was off left the screen black
  with no way back: the manual controls are CEC-only and hidden entirely
  on a device with no adapter. The tick now restores the display and
  clears the stored state.
- The per-minute task no longer lets arbitrary failures escape; it logs
  a warning and retries next tick, so a persistent fault cannot file a
  Sentry event every 60s.
- Cover the day-checkbox int-membership render, which would silently
  render everything unchecked if the context handed over strings.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* fix(cec): address code-review findings

- Claim the right logical address. CEC_LOG_ADDR_TYPE_PLAYBACK is 3 and
  CEC_OP_ALL_DEVTYPE_PLAYBACK is 0x10; the audio-system values (4 and
  0x08) were used, so the adapter claimed LA 5 (Audio System) and
  advertised an audio system. A TV that sees one appear commonly enables
  System Audio Control/ARC and mutes its own speakers, which would
  silence a signage player every time the 5-minutely probe ran. Verified
  on the Pi 5: now claims LA 4, mask 0x0010, matching cec-ctl --playback.
- Serialise CEC access with a redis lock. The claim must clear the
  adapter first, and a clear on one fd unconfigures an operation in
  flight on another; the two beat tasks coincide every 5 minutes and the
  server drives the same nodes from a request thread. A contended bus
  now raises CecBusyError so the scheduler retries instead of latching.
- Expire the scheduler's state key. The viewer's blanked flag is
  in-process and lost on restart while redis is persisted, so a viewer
  that restarted mid-off-period would stay lit until morning.
- Map PowerStatus.TRANSITIONING instead of falling through to
  'CEC error' — a TV that is merely warming up is not a fault.
- Parse the schedule's day list with parse_days in the template context;
  the inline version had the opposite empty-input behaviour, showing a
  schedule as running on no days while it ran daily.
- Accept HH:MM:SS (any <input type="time"> with a sub-minute step posts
  it) and surface an error instead of silently discarding the edit while
  reporting success.
- Skip the lock entirely when no adapter has a live link, which is most
  of the fleet (0.3-1.4s of redis round-trip measured per transition).

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* refactor(cec): clear the SonarCloud findings

- TimeoutError is an OSError subclass (PEP 3151), so naming both in an
  except tuple is redundant. Replaced the five sites with a documented
  CEC_ERRORS tuple so the subtlety is stated once.
- Extract the duplicated 'CEC error' literal.
- Use [[ ]] for the new conditionals in upgrade_containers.sh.
- Annotate the BaseException catch in _run_bounded: it is stashed and
  re-raised on the calling thread, which Sonar cannot see across the
  thread boundary.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* style(cec): fix the NOSONAR suppression syntax

The parenthesised rule-key form is not valid for Sonar's Python
analyzer, which flagged it as a malformed suppression (S7632) and left
the original finding open.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* feat(cec): run CEC in the viewer so balena is supported

Balena support is an acceptance criterion, and device passthrough cannot
deliver it: the balena compose file is baked into the release from a
workstation, nothing on-device can enumerate the host's /dev/cec*, and a
statically listed node that turns out to be absent stops the container
from starting.

The viewer is `privileged: true` in all three compose templates, so it
already sees every CEC node on every board and every deployment. Move
the hardware access there and have anthias-server/anthias-celery ask it
over the existing Redis request-reply bus — the same mechanism the v1
current_asset_id endpoint uses.

- New lib/cec_client.py: power_status() / set_power() over the bus, plus
  available() read from a Redis fact the viewer publishes at startup so
  gating a settings render stays a single GET rather than a round trip.
- Viewer gains display_power_status / display_on / display_off handlers.
- diagnostics and the schedule now go through the client.
- The compose override generator, and the SSL scripts' handling of it,
  are deleted. No `devices:` entries are needed for server/celery on any
  board, so the OTA upgrade risk this PR carried is gone entirely.
- The CEC bus lock drops from a Redis lock to an in-process mutex, since
  exactly one process now drives the hardware.

Verified end to end on the Pi 4, where anthias-server has no /dev/cec*
at all: cec_available() True, get_display_power() 'No CEC display
detected' in 79ms. And on the Pi 5 with a live link: real transmits at
430ms, matching direct device access, so the bus hop costs nothing. The
Pi 2 — which could never have worked through the server — shows a live
1.0.0.0 physical address from the viewer.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* feat(api,ui): expose the display schedule in v2 and align the UI

API — the HTML form and DeviceSettingsViewV2 are documented mirrors of
each other, but the schedule was only settable from the form.

- GET returns the four schedule fields; PATCH accepts them.
- Serializer validators normalise 'HH:MM' (accepting 'HH:MM:SS', which
  an <input type="time"> with a sub-minute step posts) and the weekday
  list, so the beat can never read a value it cannot parse.
- Unlike the form, a malformed value is a 400 rather than a silent
  keep-previous: an API client that explicitly sent a field should be
  told it was wrong, not have it reinterpreted.
- _isolated_settings_conf moves to the root conftest so the API tests
  can use it instead of duplicating the fixture.

UI — the section was built from ad-hoc utility classes that appeared
nowhere else on the page, which is exactly how a bolt-on looks.

- Reuse .weekday-picker / .weekday, the pill chips the asset modal and
  bulk-edit modal already use for choosing days, so both places in the
  product that pick weekdays look and behave identically.
- Add x-cloak so the fields do not flash open before Alpine initialises,
  matching the modal's collapsible sections.
- Copy: lead with the observable outcome ("A TV that supports HDMI-CEC
  powers down; any other display goes black") rather than naming the
  mechanism, and drop the toggle label's stutter against the section
  heading. The overnight-schedule note now gives a concrete example.
- Document why type="time" is right here: flatpickr is initialised in
  home.ts, which the settings page does not load.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* fix(cec): derive the reply budget from the viewer's worst case

The client could give up while the viewer was still working, reporting a
fault that was not one — the same "we could not ask" vs "the display did
not answer" conflation this feature exists to remove.

  viewer power_status worst case  8s lock + 2 x 5s guard = 18s
  client QUERY_TIMEOUT_MS                                  12s
  viewer set_power worst case              2 x 5s guard = 10s
  client COMMAND_TIMEOUT_MS                                 8s

Both budgets were below the thing they were waiting for. Now the viewer
exports MAX_OPERATION_S and the client derives its budget from it, so
tuning either guard keeps the two in step. The bus lock also drops to 2s
— the viewer dispatches commands sequentially from a single subscriber
loop, so it is never contended in practice and only exists as a backstop
if that invariant changes.

Tests pin both ends of the chain: viewer worst case < client budget <
celery soft limit.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* fix(ui): make the weekday chips keyboard- and screen-reader-reachable

`.weekday input { display: none }` dropped the checkbox out of the tab
order and the accessibility tree, so the day chips could only be operated
with a pointer. Clip the input instead of hiding it, and draw the focus
ring on the chip since the input itself is invisible.

Pre-existing, and fixed at the shared rule rather than in the new
markup, so the asset modal and bulk-edit modal get it too — all three
places that pick weekdays. Appearance is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* docs(website): document the display schedule

The feature was shipped with no user-facing write-up, which is the one
checklist box the PR left unticked.

Covers what actually reaches the screen (CEC *and* local blanking, so a
plain monitor is not left lit), the field reference with defaults, the
overnight-window semantics — an on-period belongs to the day it starts,
which is the part an operator will otherwise get wrong — the v2 API
path, and the ~10-minute re-assertion that overrides a manual toggle.

Flags the weekday-numbering mismatch against asset scheduling (Mon=0
here, Mon=1 there): the UI hides it behind identical day chips, but an
API client hits it directly.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* docs: correct the cec_available comment and note the empty-day rule

cec_available() stopped probing /dev/cec* when CEC moved into the
viewer — it now reads a single Redis key the viewer publishes at
startup. The page_context comment still described the device probe and
quoted its per-adapter timing, which is the wrong cost model for anyone
deciding whether this is safe to call on every render.

Also document, on the website page, that deselecting every weekday
means *every* day rather than *no* day. That is the one behaviour in
the schedule an operator can trip over without any feedback.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* docs(cec): correct the get_display_power short-circuit comment

The comment still described the pre-viewer design: it claimed the
availability check globs device nodes in this container and that the
boards taking the branch would start reporting real state "once the
device passthrough hands them their /dev/cec* nodes". There is no
device passthrough any more — cec_available() is a redis GET of the
fact the viewer publishes, and the query it guards is a bus round trip
rather than a subprocess, so the cost the comment used to justify is
gone too.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* fix(cec): validate the seconds token before dropping it

parse_hhmm accepted any three-part value and read only the first two,
so '07:30:xx', '07:30:' and '07:30:60' all became a valid 07:30. The
v2 serializer builds its 400-on-malformed contract on this helper, so
the API was silently reinterpreting exactly the input it promises to
reject — and the HTML form recorded the edit as saved instead of
toasting.

Seconds are still dropped: the schedule has minute resolution and an
<input type="time"> with a sub-minute step legitimately posts them.
They are now parsed and range-checked first.

Found by Copilot.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

* docs: document the Mixed power reading, fix the blank-order comment

Two of Copilot's four suppressed comments were right.

The System Info card can report 'Mixed' — PowerStatus.UNKNOWN, which
lib/cec returns when two attached displays disagree or one is
mid-transition. The docs listed every other reading and omitted that
one, so an operator meeting it would have no way to tell whether it was
a fault. Replaced the prose with the full table of readings.

apply_power's comment said "Blank first" while the call sends unblank
on the way up. Reworded to name both directions.

The other two (parse_hhmm's warning-level logs are potential noise) are
left as they are: the schedule silently does nothing when it cannot
parse a time, so that warning is the only signal an operator gets, and
WARNING does not file a Sentry event the way the runaway ERROR pattern
in #3017/#3063 did.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

---------

Co-authored-by: vpetersson-bot <[email protected]>
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
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.

2 participants