From 96c0399deae8a8a11e73149758ff6aa7fa161166 Mon Sep 17 00:00:00 2001 From: vpetersson-bot Date: Sun, 25 Oct 2026 09:17:08 +0000 Subject: [PATCH] fix(backup): swap restored assets in atomically, not in place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `recover()` extracts every member straight to its final path, so an asset that already exists on the device is truncated and then refilled under its own name. Nothing stops the viewer fetching that same file over `/anthias_assets/` while the write is in flight. Django's `FileResponse` measures `Content-Length` from the file as it stands when the response is built and then streams the same open fd to EOF. Rewriting the inode underneath that reader breaks the promise two ways: a restored copy that is larger makes the body overrun the header and uvicorn raises `Response content longer than Content-Length` (Sentry ANTHIAS-37), a smaller one makes it fall short and raises `Response content shorter than Content-Length`. The 500 is the visible half; the quiet half is that the viewer was handed a frame spliced from two different files. Every other writer of the media store already stages to a sibling and renames — `processing.py`, `celery_tasks.py`, `views.py`, `integrations/ingest.py`. Restore was the one path that did not, so it does now: each media-store file is extracted under `.restore-.tmp` beside its destination and `os.replace`d into place, which the kernel observes as a single inode swap. An in-flight response keeps reading the inode it opened and still matches the length it declared, and no request can observe a partial asset. The staging name is dot-prefixed and `.tmp`-suffixed on purpose: both of `views_files.anthias_assets`' transient-file guards already refuse to serve such a name, `_exclude_from_backup` keeps it out of the next archive, and the hourly `cleanup()` sweep collects it if the process dies between the extract and the rename. Config and database members keep the plain `tar.extract` path. On an upgraded device `.screenly/screenly.db` is a back-compat symlink into `.anthias/`, and extraction is meant to write *through* it; an `os.replace` there would replace the symlink with a regular file and leave the real database untouched. Also corrects the `before_send` comment that credited the `shorter` rule with ANTHIAS-37. ANTHIAS-37 is the `longer` variant, so that rule never matched it — and a genuine overrun is a bug in whatever produced the response, not noise to drop. Co-Authored-By: Claude Opus 5 (1M context) Co-authored-by: multica-agent --- .claude/skills/anthias-release/SKILL.md | 2 + src/anthias_server/django_project/settings.py | 13 +++- src/anthias_server/lib/backup_helper.py | 66 ++++++++++++++++++- tests/test_backup_helper.py | 45 ++++++++++++- 4 files changed, 122 insertions(+), 4 deletions(-) diff --git a/.claude/skills/anthias-release/SKILL.md b/.claude/skills/anthias-release/SKILL.md index fba765355..e587459c9 100644 --- a/.claude/skills/anthias-release/SKILL.md +++ b/.claude/skills/anthias-release/SKILL.md @@ -64,6 +64,8 @@ where known; OPEN items are flagged. ``/`` are the public - **environment tag vs baggage mismatch = test/CI stack leaking into prod Sentry, not a device failure.** An event tagged `environment=production` whose Celery task headers carry `baggage: sentry-environment=test` means the *producer* ran in a test stack but the *worker* fell back to production defaults. Root cause (event in normalize_video_asset, "no such table: assets"): `anthias-celery` in docker-compose.test.yml lacked `ENVIRONMENT=test`/`ANTHIAS_TEST_DB_PATH`. settings.py defaults BOTH the DSN and the DB path off `ENVIRONMENT`, so any compose service missing the var silently becomes "production". Fixed #3013 by sharing the env block via a YAML anchor. When triaging, compare the event's `environment` tag against `sentry-environment` in the task baggage and the hostnames. - **Device context tags (PR #3021 / #3025).** Events are sent from inside containers, so Sentry's stock OS context never sees the host. #3021 tags `device_type` (build-time ENV), `kernel_release`/`kernel_machine` (containers share the host kernel), `board_model` (`/proc/device-tree/model`, absent on x86). #3025 adds `balena: true|false`. Keep `settings.py` import-light — importing anthias_common.utils there crashes django-stubs' mypy plugin in CI's slim env, so `is_balena_deploy()` is inlined. Segment webview-crash cohorts by `kernel_machine` (aarch64 → pi3-64 stream; armv7l → Qt5.15 WebEngine build). - **A late soft-timeout signal kills an idle pool child — the cause behind the ANTHIAS-B/1Q volume.** Celery arms the soft limit against the *job slot*, and `asynpool._on_soft_timeout` fires whenever the job is still in `Pool._cache`. The job only leaves that cache when the parent *drains the result off the outqueue*, which is a separate step from the child finishing the work — so on a memory-pressured board (soft limits observed firing 113s after a 30s timer was armed) the parent signals a child that is already back in `wait_for_job()`. `billiard.pool.soft_timeout_sighandler` raises unconditionally, the raise lands in the idle `workloop` where no task handler can catch it, the child exits 1, and billiard logs `Process 'ForkPoolWorker-N' exited with 'exitcode 1'` at ERROR (ANTHIAS-B; the respawn churn is ANTHIAS-1Q's "Timed out waiting for UP message"). #3352 only taught `before_send` to drop the escaping exception (ANTHIAS-45/5Y) — the child kept dying. Fixed by `lib/soft_timeout_guard.py`: honour the signal only between `task_prerun` and `task_postrun`. It must rebind `billiard.pool.soft_timeout_sighandler` before the pool forks — `Worker.after_fork` runs the pool initializer (which sends `worker_process_init`) and installs its handler *afterwards*, so a receiver on that signal is always overwritten. +- **An empty unresolved queue is not evidence of a healthy fleet — check `accepted` before triaging.** When the project's error allowance is spent, Sentry rejects everything with outcome `rate_limited` / reason `error_usage_exceeded`, every issue's `lastSeen` freezes at the day the allowance ran out, and the board reads as all-clear while devices keep failing. Verify with `GET /api/0/organizations/anthias/stats_v2/?field=sum(quantity)&groupBy=outcome&groupBy=reason&category=error&statsPeriod=14d&project=`: a run of days with `accepted` at zero and `rate_limited` still climbing means the queue is empty because nothing is getting in. The same call splits `client_discard` by reason, which is how to tell a `before_send` rule that is over-matching from one that is doing its job. +- **`shorter` and `longer than Content-Length` are different failures, and only one is noise.** uvicorn raises *shorter* when a response body ends before the declared length (the client-disconnect case) and *longer* when it overruns it (the response really did produce more bytes than it promised — a bug, which must keep reporting). PR #3182 filtered the *shorter* string while crediting ANTHIAS-37, which is the *longer* variant, so that issue was never actually suppressed. Root cause was `recover()` extracting assets straight over their final paths while the viewer was fetching them; fixed by staging + `os.replace` in `backup_helper._extract_asset_atomically`. - **Triage rule:** resolve every unresolved issue not seen on the current release (Sentry auto-reopens as regression if it recurs). Real code defects fixed in the 2026-07 sweep: PR #3180 (`reconcile_stuck_processing` bare `time_limit=300` → SIGKILL trio), PR #3181 (unguarded `await self.send()` in `asset_update` consumer, disconnect-vs-broadcast race → narrow `except RuntimeError`), PR #3182 (before_send drop for client-disconnect `Response content shorter than Content-Length` static noise). ## Telemetry diff --git a/src/anthias_server/django_project/settings.py b/src/anthias_server/django_project/settings.py index ee98fb2a6..6ed22a3d0 100644 --- a/src/anthias_server/django_project/settings.py +++ b/src/anthias_server/django_project/settings.py @@ -355,8 +355,17 @@ def _sentry_before_send(event: Event, hint: Hint) -> Event | None: short by the disconnect — a broken pipe, not a truncated file. Same client-disconnect class as ``CancelledError`` above, but it surfaces as a bare ``RuntimeError``, so it's matched by message - text to avoid swallowing unrelated RuntimeErrors (Sentry - ANTHIAS-37). + text to avoid swallowing unrelated RuntimeErrors. + + Note the message: uvicorn raises *shorter* when the body ends + early and *longer* when it overruns the header, and only the + first is the client-disconnect case described above. This rule + was originally credited with ANTHIAS-37, which is the *longer* + variant and so never matched it — a real overrun is a bug in + whatever produced the response, not noise, and must keep + reporting. ANTHIAS-37's own cause (a restore rewriting an asset + in place under a live ``FileResponse``) is fixed at the source + in ``backup_helper._extract_asset_atomically``. * ``SoftTimeLimitExceeded`` **with no Anthias frame anywhere in its traceback** — celery arms the soft-limit timer for the whole job, not just the task body, so a task that finishes a hair diff --git a/src/anthias_server/lib/backup_helper.py b/src/anthias_server/lib/backup_helper.py index 237c230ca..cfb67e0b4 100644 --- a/src/anthias_server/lib/backup_helper.py +++ b/src/anthias_server/lib/backup_helper.py @@ -8,6 +8,7 @@ import tarfile import tempfile import threading +import uuid from collections.abc import AsyncGenerator, Generator from concurrent.futures import ThreadPoolExecutor from datetime import UTC, datetime @@ -685,6 +686,61 @@ def create_backup(name: str = default_archive_name) -> str: return archive_name +def _is_asset_member(name: str) -> bool: + """True for a tar member that lives inside the media store.""" + parts = name.replace('\\', '/').strip('/').split('/') + return len(parts) > 1 and parts[0] in _ASSET_DIRS + + +def _extract_asset_atomically( + tar: tarfile.TarFile, + member: tarfile.TarInfo, + home: str, + extract_kwargs: dict[str, Any], +) -> None: + """Extract one media-store file via a sibling temp name + ``os.replace``. + + ``tar.extract`` writes straight to the member's final path, so an + asset that already exists on the device is truncated and then + refilled in place — while the viewer may be fetching that exact + file over ``/anthias_assets/``. Django's ``FileResponse`` + declares ``Content-Length`` from the file as it is at the moment + the response is built and then streams the same open fd to EOF, so + a restore that lands a *larger* copy makes the body overrun the + header (uvicorn: "Response content longer than Content-Length", + Sentry ANTHIAS-37) and a *smaller* one makes it fall short + ("Response content shorter than Content-Length"). Either way the + viewer has already been handed a spliced half-old/half-new image. + + Staging to a sibling and renaming makes the swap a single inode + change: an in-flight response keeps reading the inode it opened and + still matches the length it promised, and no request can ever + observe a half-written asset. This is the same contract every other + writer of the media store already keeps (``processing.py``, + ``celery_tasks.py``, ``views.py``, ``integrations/ingest.py``) — + restore was the one path that did not. + + The staging name is dot-prefixed *and* ``.tmp``-suffixed so both of + ``views_files.anthias_assets``' transient-file guards refuse to + serve it, ``_exclude_from_backup`` keeps it out of the next + archive, and the hourly ``cleanup()`` sweep collects it if the + process dies between the extract and the rename. + """ + directory = path.dirname(member.name) + staged_name = f'{directory}/.restore-{uuid.uuid4().hex}.tmp' + staged = member.replace(name=staged_name, deep=False) + staged_path = path.join(home, staged_name) + try: + tar.extract(staged, **extract_kwargs) + os.replace(staged_path, path.join(home, member.name)) + except BaseException: + try: + remove(staged_path) + except OSError: + pass + raise + + def recover(file_path: str) -> None: home = getenv('HOME') if not home: @@ -738,7 +794,15 @@ def recover(file_path: str) -> None: member.name, ) continue - tar.extract(member, **extract_kwargs) + if member.isfile() and _is_asset_member(member.name): + # Swapped in atomically — a live request for this + # asset must never see it half-written. See + # _extract_asset_atomically. + _extract_asset_atomically( + tar, member, home, extract_kwargs + ) + else: + tar.extract(member, **extract_kwargs) if member.isfile() and _is_db_file(member.name): # On an upgraded device ``.screenly`` and # ``screenly.db`` are back-compat symlinks to diff --git a/tests/test_backup_helper.py b/tests/test_backup_helper.py index e94803b98..d362c8fa2 100644 --- a/tests/test_backup_helper.py +++ b/tests/test_backup_helper.py @@ -495,6 +495,46 @@ def test_recover_writes_only_runtime_files(backup_home: str) -> None: assert not path.exists(path.join(a, 'evil.sh')) +def test_recover_swaps_assets_atomically(backup_home: str) -> None: + """A restore must not rewrite an asset in place. + + The viewer fetches assets over ``/anthias_assets/`` while a + restore is running, and Django's ``FileResponse`` declares + ``Content-Length`` from the file as it stands when the response is + built, then streams that same open fd to EOF. Truncating and + refilling the path under a live reader therefore both corrupts what + the viewer renders and breaks the promised length — overrunning it + when the restored copy is larger (uvicorn: "Response content longer + than Content-Length", Sentry ANTHIAS-37) and falling short when it + is smaller. Staging to a sibling and renaming makes the swap one + inode change, so an in-flight response is unaffected. + """ + asset_dir = path.join(backup_home, 'anthias_assets') + target = path.join(asset_dir, 'pic.jpg') + old_bytes = b'OLD' + new_bytes = b'NEW' * 100 # deliberately larger than what it replaces + with open(target, 'wb') as f: + f.write(old_bytes) + old_inode = os.stat(target).st_ino + + archive = path.join(backup_home, 'assets.tar.gz') + with tarfile.open(archive, 'w:gz') as tar: # NOSONAR + _stage_legit_layout(tar) + _add_file_member(tar, 'anthias_assets/pic.jpg', new_bytes) + + # Stands in for the response that is already streaming this asset. + with open(target, 'rb') as inflight: + recover(archive) + assert inflight.read() == old_bytes + assert os.fstat(inflight.fileno()).st_size == len(old_bytes) + + assert os.stat(target).st_ino != old_inode + with open(target, 'rb') as f: + assert f.read() == new_bytes + # The staging name is gone — no debris for the cleanup() sweep. + assert os.listdir(asset_dir) == ['pic.jpg'] + + def test_backup_ships_only_runtime_files(backup_home: str) -> None: """A backup captures the runtime whitelist and nothing else. Config, DB, default-asset manifest, db backups and assets are kept; the @@ -1188,7 +1228,10 @@ def test_recover_clears_the_stale_wal_when_a_later_member_fails( def extract( self: tarfile.TarFile, member: tarfile.TarInfo, **kwargs: object ) -> None: - if member.name == 'anthias_assets/after-the-db.bin': + # Matched by prefix, not by exact name: an asset is extracted + # under a staging name and renamed into place, so the member + # this sees is '.restore-.tmp', not the archived name. + if member.name.startswith('anthias_assets/'): raise OSError('disk full') real_extract(self, member, **kwargs) # type: ignore[arg-type]