fix(celery): catch the asset-probe timeout instead of hard-killing the worker - #3017
Merged
Conversation
…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]>
There was a problem hiding this comment.
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
SoftTimeLimitExceededinside 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.
… 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]>
|
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]>
5 tasks
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]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



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_failscan burn a hanginggetaddrinfoagainst 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=90on the on-demand probe — the soft limit raisesSoftTimeLimitExceededinside the task, which now records the same verdict an HTTP timeout gets (unreachable,last_reachability_checkstamped) instead of dyingChecklist
🤖 Generated with Claude Code