Repository navigation
fix(worker): keep FastAPI out of modules the workers import - #663
Merged
Merged
Conversation
The worker images install no FastAPI, and document_storage imported it, so backups, restores, the backup cleanup and the daily temp cleanup failed on the workers. It now raises RecordingGoneError, which the documents endpoint turns into the same 404. The daily cleanup imports storage hygiene inside its guard, so a failed import no longer skips the sweeps. Fixes: #660
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.
Pull Request
Description
The worker images install
requirements/worker.txt, which has no FastAPI.backend/utils/document_storage.pyimportedHTTPExceptionandrun_in_threadpoolfrom FastAPI at module level, and the restore runner, the restore stages and the storage hygiene scan all import it. On the workers that broke four tasks at their first import:create_backup_taskandrestore_backup_task, throughbackend.core.backupcleanup_backup_artifacts, the six-hourly backup cleanupcleanup_temp_recordings, the daily cleanup, which imported storage hygiene on its first line, so none of its sweeps ran eitherCI installs the API and worker requirements together, so no test saw it. The chain arrived with #495 and #533 on 21 Oct. No release has it.
What changed:
document_storageraisesRecordingGoneErrorwhen an upload's recording is deleted under it, anddocuments.pyturns it into the same404 Recording not found. That covers the upload endpoint andcreate_text_document, which the MCPattach_documenttool calls. The file cleanup's thread hop callsanyio.to_thread.run_sync, which is all FastAPI'srun_in_threadpooldoes, so the thread limiter and the cancellation behaviour are unchanged.test_document_storage_cancellation.pystill covers both.cleanup_temp_recordingsimports storage hygiene inside the guard that already catches a hygiene failure, so the sweeps before it run whatever happens to that import.backend/tests/test_worker_import_boundary.py. It walks every backend module a worker can reach: the Celery app and its include list, every module underbackend/worker, the GPU preflight, the diarisation subprocess script and the storage hygiene command. It follows imports inside functions, since the tasks import their work lazily. It imports all of them in a subprocess with FastAPI, Starlette, uvicorn, mcp, docker, reportlab, re2 and their dependencies blocked by a meta-path finder, and fails on any unguarded import of those packages in a worker-reachable module, inside a function or out. A further test fails when a new pin inbackend.txtalone is missing from the blocked list.docs/DEVELOPMENT.mdstates the rule under the backend conventions and lists both image-boundary tests under Test Reliability.No new dependencies.
anyiois already in every image through httpx.Fixes: #660
Decision record
DEVELOPMENT.mdalready says to raiseHTTPExceptionin endpoints. The other option was to moveinsert_document_or_remove_fileanddelete_document_filesinto the API layer, which would split one module's lifecycle rules, and its tests, across two places.calendar_serviceimportsHTTPExceptioninsidetry/except ModuleNotFoundErrorand sets it toNoneon the workers. That would have passed here too, but a worker path that ever reached theraisewould fail with aTypeError.calendar_service/__init__.py,oauth.py,sync.pyanddashboard.pykeep that guard. They import cleanly on the workers, and their raises are in API-only helpers. The new test allows a guarded import.worker-iocarries Starlette, uvicorn and mcp throughclaude-agent-sdk, butworker-cpuruns the backups and has none of them.pyannote.audiois installed by the Dockerfile and not in CI, so a module that needs it is skipped by the import check and still read by the static check.Type of change
Checks run
scripts/check.py lint format blindexcept heldpins typecheck docs alembicand the full pytest suite with a pgvector Postgres, innojoin-dev-tests:local: 3618 passed. Whitespace, file size andvalidate_docs.pyon the host.The two new tests fail on
mainand pass here. Onmainthe import check fails 13 modules,backend.core.backupamong them, all withNo module named 'fastapi'. The hygiene test fails because the task raises before its sweeps.Migration impact
Documentation impact
.github/CONTRIBUTING.md, "Documentation Ownership").Security impact
Manual verification
Imported every module a worker can reach (256, all 21 task modules among them) and
backend.core.backupin throwaway containers on the locally builtnojoin-workerandnojoin-worker-ioimages, which have no FastAPI. This branch'sbackend/was mounted read-only, with no network and no.env. Both images import everything with no failures. Withmain'sbackend/the same check fails the 13 modules above on both images.