Skip to content

fix: scan status race - #504

Merged
KoblerS merged 4 commits into
cap-js:mainfrom
ansgarlichter:fix/scanning-race
Jul 30, 2026
Merged

fix: scan status race#504
KoblerS merged 4 commits into
cap-js:mainfrom
ansgarlichter:fix/scanning-race

Conversation

@ansgarlichter

@ansgarlichter ansgarlichter commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Closes #432
Related to #481

Problem

Clean attachments intermittently returned a 202/stuck-in-Scanning state on download, because the download-triggered re-scan in rescan() (lib/generic-handlers.js) wrote the status to Scanning in a non-awaited cds.tx(...) before spawning the scan:

// Set status to Scanning and commit before emitting event to prevent race conditions
cds.tx(async () => await malwareScanner.updateStatus(target, keys, "Scanning"))  // NOT awaited
cds.spawn(async () => await malwareScanner.emit("ScanAttachmentsFile", { target, keys }))
throw cds.error({ status: 202, code: "UnableToDownloadAttachmentScanStatusExpired" })

The comment promised "commit before emitting", but the cds.tx is not awaited, so there is no happens-before guarantee. The spawned scan (_scanAttachmentsFile) also sets Scanning as its first step and later writes Clean. Under load the un-awaited Scanning commit is starved on the DB connection pool and lands after the scan's Clean commit → last-writer-wins → row stuck in Scanning (visible in logs as Updated scan status to Scanning for undefined, since rescan passes req.target.name as a string).

Fix

Remove the request-path Scanning write entirely (both the composition and inline-attachment branches), leaving only the cds.spawn that triggers the scan.

This is correct because the write was redundant, not load-bearing: _scanAttachmentsFile already sets the status to Scanning as its first committed step. Removing it:

  • eliminates the duplicate/late write that clobbered Clean (no stuck-in-Scanning), and
  • avoids holding a DB connection across the 202 throw, which contended with the detached scan transaction for the pool.

I reverted my earlier attempt (await cds.tx(...)). I first tried making the write safe by awaiting the cds.tx(...). That fixed the ordering but made things worse in a different way: the awaited commit holds a DB connection across the 202 throw and collides with the detached cds.spawn scan transaction. On a single-connection database (local SQLite dev server + test harness) this drained the pool (Pool is draining / database is not open) and hung the reques — reproducible on a UI-triggered rescan in local dev if you have additional things happening afterwards like audit log writes. Removing the write (rather than awaiting it) avoids both failure modes.

Relationship to #442

Draft PR #442 takes the same removal approach. This PR arrives at the same conclusion independently, with the added data point that the await-the-tx alternative is not viable (pool drain on single-connection DBs). Happy to consolidate with #442 if you prefer that.

Out of scope

#481 (attachment stuck in Scanning after a crash between the Scanning and Clean writes) is a separate crash-recovery concern and is not addressed here.

@ansgarlichter

Copy link
Copy Markdown
Contributor Author

FAIL tests/integration/attachments-non-draft.test.js
● Test suite failed to run
TypeError: Cannot set properties of undefined (setting 'messageType')

I guess that the tests failures related to these errors, are not related to my changes. On main the same test runs seem to fail as well.

@eric-pSAP eric-pSAP mentioned this pull request Jul 29, 2026
1 task
KoblerS
KoblerS previously approved these changes Jul 30, 2026

@KoblerS KoblerS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGMT, thank you!

@eric-pSAP
eric-pSAP enabled auto-merge (squash) July 30, 2026 09:11
@KoblerS
KoblerS disabled auto-merge July 30, 2026 13:58
@KoblerS
KoblerS enabled auto-merge (squash) July 30, 2026 13:58
@KoblerS
KoblerS disabled auto-merge July 30, 2026 13:59
@KoblerS
KoblerS merged commit 68beb38 into cap-js:main Jul 30, 2026
42 of 58 checks passed
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.

[Version: 3.12.0] Files stuck in status 'Scanning' after malware scanner found the file 'Clean'

3 participants