feat: rescan attachments stuck in "Scanning" on server start - #512
Draft
cschuerings wants to merge 2 commits into
Draft
feat: rescan attachments stuck in "Scanning" on server start#512cschuerings wants to merge 2 commits into
cschuerings wants to merge 2 commits into
Conversation
If the server crashes or restarts while a malware scan is in progress,
attachment rows can get stuck in "Scanning" status permanently — the
scanner never gets a chance to write the terminal Clean/Infected/Failed
result back.
Adds a `rescanStuckAttachments()` function that runs on `served` (via
`cds.spawn` to avoid blocking startup or holding a DB connection on the
serving path). It iterates all attachment entities in the compiled model,
deduplicates by physical table (so service projections don't trigger
redundant queries), selects rows with `status = 'Scanning'`, and
re-emits `ScanAttachmentsFile` for each — reusing the existing scanner
pipeline end-to-end.
Opt-in via config flag (defaults to false):
cds.requires.attachments.rescanOnStart = true
Also respects the existing `attachments.scan` flag.
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.
Related to #481
Problem
If the server crashes or restarts while a malware scan is in progress, attachment rows can get stuck in
"Scanning"status permanently. The scanner sets a row toScanningas its first step in_scanAttachmentsFile, then fetches content, scans, and writes backClean/Infected/Failed. If the process dies between those steps, no trigger ever fires again to move the row to a terminal status — downloads of such rows return 202 forever.The only existing recovery path is the on-download expiry check in
enforceScanPolicy, but that requires someone to attempt a download. Rows with no subsequent download activity stay stuck indefinitely.This was already called out as out-of-scope in #504:
Fix
Add a
rescanStuckAttachments()function that runs once onserved, detached viacds.spawnso it never blocks startup or holds a DB connection on the serving path — consistent with the pool-contention rationale from #504.It iterates
cds.model.definitions, deduplicates by underlying physical table (so service projections don't cause redundant queries), selects all rows withstatus = 'Scanning', and re-emitsScanAttachmentsFilefor each — reusing the existing scanner pipeline end-to-end without any new scanning logic.Both entity types are handled:
statuscolumn, keysup__ID+ID)<prefix>_statuscolumn,<prefix>_urlpassed through for object-store fetch)The existing
Semaphore(maxConcurrentScans) naturally throttles any large backlog.Configuration
Opt-in via a new flag (defaults to
false):Also respects the existing
attachments.scanflag — if scanning is disabled, the sweep is skipped entirely.Out of scope
Per-tenant reconciliation in multitenancy deployments (marked with a
TODO(mt)comment). For MT, the on-download rescan path inenforceScanPolicyremains the safety net. Happy to extend if maintainers see value.Question for maintainers
Is this the right approach for crash recovery, or would you prefer a different mechanism (e.g. a periodic reconciliation job, or hooking into the outbox retry logic)? Also open to feedback on the opt-in default —
falsefelt safer for a first version buttruecould be argued since the operation is idempotent.