fix(spp_attachment_av_scan): never swallow a database error when queueing a scan - #384
Merged
Merged
Conversation
…eing a scan
The create/write hooks wrapped the scan-queue call in a bare `except Exception`
and logged whatever it caught. A database error, however, leaves the transaction
unusable -- so swallowing one lets execution continue on a dead cursor, and the
next statement to touch the database fails instead, in unrelated code.
Observed on a DSWD dev instance: a routine attachment write during a module
upgrade hit a transient
ERROR: could not serialize access due to concurrent update
on ir_attachment. This module caught it, logged "Failed to queue malware scan for
updated attachment ID 504", and carried on. The next statement was an unrelated
`env.ref("stock.menu_stock_root")` inside spp_base_common's menu-icon refresh,
which raised InFailedSqlTransaction -- and that was the only error the operator
ever saw. EVERY module upgrade failed this way, with four different modules
blamed in turn before the real cause was found in a stray ERROR log line.
The swallow also defeated the recovery Odoo already provides.
`odoo.service.model.retrying` retries a request on IntegrityError /
OperationalError / ConcurrencyError, and SerializationFailure reaches that tuple
via TransactionRollbackError -> OperationalError. Left alone the conflict would
have been retried transparently; caught, it became a hard failure attributed to
the wrong subsystem.
Re-raise those classes ahead of the existing broad catch. Queueing a scan stays
best-effort for everything else -- a misconfigured queue raising ValueError must
not block an attachment write -- so the fix is a split, not a removal.
Scope: only the two hooks on the create/write request path. The module's seven
other `except Exception` blocks (_scan_for_malware, _quarantine,
_notify_security_admins, and three action_* methods) are reached via queue jobs
or explicit buttons, where a poisoned transaction is confined to that job rather
than a user request. Same latent hazard, different blast radius; left for a
follow-up rather than widening this change.
Tests assert both sides of the contract: a DB error propagates from create and
from write, a non-DB error is still swallowed and still logged (and the
attachment is still written), and SerializationFailure remains a subclass of what
`retrying` recovers from, so a future refactor cannot silently revive the
incident by re-raising something wrapped.
Signed-off-by: Red <redickbutay02@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Patch bump + HISTORY fragment for the DB-error re-raise fix, per repo convention. Also corrects the _MUST_NOT_SWALLOW docstring: psycopg2.Error is a superset of what retrying recovers from, not an exact match.
…nned generator output)
Assert SerializationFailure against Odoo's exported PG_CONCURRENCY_EXCEPTIONS_TO_RETRY instead of only a hardcoded tuple, so an upstream change to the retry set fails the test. Add a propagation test for ConcurrencyError, the second member of _MUST_NOT_SWALLOW.
Member
Adversarial review summaryRan a staff-engineer adversarial review on the full diff ( Attacks that failed
Added on the branch during review
Follow-upThe seven remaining CI is fully green on the final commit. Ready for human review. |
gonzalesedwin1123
approved these changes
Aug 3, 2026
This was referenced Aug 27, 2026
gonzalesedwin1123
pushed a commit
that referenced
this pull request
Aug 27, 2026
…ads (#464) #384 made the create/write scan-queue hooks re-raise database errors instead of swallowing them, so a transient serialization conflict reaches `odoo.service.model.retrying` and is retried. That contract is right inside a request. It is a trap during registry construction, which has no retry wrapper: a re-raised `could not serialize access due to concurrent update` there aborts the whole load and the server never boots. Registry load writes binary attachments routinely. Module data may declare `ir.attachment` records outright, and every `ir.ui.menu` carrying a `web_icon` recomputes `web_icon_data` -- a `Binary(attachment=True)` field -- whenever its XML is loaded, which lands as an `ir.attachment` create or write. Enqueueing there inserts a `queue.job` row, and that insert flushes the deferred `ir_attachment` UPDATE inside the re-raising hook, against a peer instance holding the same rows on a shared database. So gate both hooks on `env.registry.ready`. While it is False, nothing is enqueued. New attachments keep the `scan_status` default ("pending"), so nothing is left looking scanned; and `write` still performs the status reset before bailing out, because skipping it would leave changed bytes wearing the previous scan's `clean` verdict. That reset is a cache-level write on a row the transaction has already dirtied -- no extra SQL, no extra conflict window. Attachments written during load therefore stay `pending` until a rescan; no cron sweeps them today, which is the honest state and strictly safer than the alternative. Runtime uploads are untouched. Tests assert all four sides so the guard cannot regress in either direction: create and write enqueue nothing while the registry loads (and the write path still resets the status), and both still enqueue at runtime.
gonzalesedwin1123
pushed a commit
that referenced
this pull request
Aug 28, 2026
…can (#470) * feat(spp_attachment_av_scan): sweep attachments stranded at pending Queueing a malware scan is best-effort by design (#384): a non-database enqueue failure is logged and the attachment is still written. Nothing came back for the records that left behind — written, at the scan_status default, indistinguishable in the UI from a file still waiting its turn in a deep queue, retained unscanned indefinitely, and reachable only by a human clicking Rescan. #464 added a second, benign source of the same state. An hourly ir.cron re-queues them, active by default and bounded on both axes so it is safe on an existing database: pending_sweep_batch_size caps one run, pending_sweep_max_attempts caps the attempts per record, and pending_sweep_min_age_minutes keeps a fresh upload that is merely waiting in a deep queue from being double-queued. Bumping the attempt counter refreshes write_date, so the age threshold doubles as a flat backoff, and a broken queue is reported once per run at WARNING rather than once per record per tick. Scope is user content: quarantined files (matching action_rescan), forensic download copies, attachments with no res_model, and the system models that store their own source-controlled binaries. A blank res_model is not a proxy for that last set — Binary(attachment=True) storage records the owning model, so menu icons arrive as res_model='ir.ui.menu' — hence the explicit denylist. The sweep also has to search under skip_res_field_check, because ir.attachment._search silently hides every attachment backing a binary field, which would have dropped user-uploaded image_1920 content from the sweep while appearing to cover it. The attempt counter is bumped for every record the batch picks up, before the readability check: an attachment whose filestore file is lost has file_size > 0 but no readable bytes, and skipping it without moving write_date would park it at the head of every run forever. * fix(spp_attachment_av_scan): mark sweep cron and config defaults noupdate Without noupdate every module upgrade silently reset admin-tuned sweep values (batch size, age threshold, a deliberately disabled cron) back to the shipped defaults. * fix(spp_attachment_av_scan): evict swept payloads from the ORM cache The sweep reads each attachment's bytes only to prove they are readable; the queued job re-reads them in its own transaction. Binary fields are never evicted on their own, so a full batch would otherwise hold every payload in one cron transaction's cache simultaneously.
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
2 participants
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.
What
The
create/writehooks inspp_attachment_av_scanwrap the scan-queue call in a bareexcept Exceptionand log whatever they catch. A database error is different in kind: it leaves the transaction unusable. Swallowing one lets execution continue on a dead cursor, so the next statement to touch the database fails instead — in unrelated code, with no trace of the real cause.The incident
On a DSWD dev instance, a routine attachment write during a module upgrade hit a transient conflict:
This module caught it and moved on:
The very next statement was an unrelated XML-id lookup in
spp_base_common's menu-icon refresh:and that was the only error the operator ever saw:
Every module upgrade failed this way. Four different modules were investigated and blamed in turn — including two wrong root-cause diagnoses — before the real trigger was found as a stray
ERRORline in the server log.InFailedSqlTransactionis emitted by Postgres only when an earlier statement already failed, so the reported line is always a victim; the traceback structurally cannot name its own cause.It also defeated the recovery Odoo already provides
This is the part that makes it more than a logging annoyance.
odoo.service.model.retryingretries a request on(IntegrityError, OperationalError, ConcurrencyError)— up toMAX_TRIES_ON_CONCURRENCY_FAILURE, rolling back in between.SerializationFailurereaches that tuple throughSerializationFailure -> TransactionRollbackError -> OperationalError.So left alone, this conflict would have been retried transparently and nobody would have noticed. Caught here, it became a hard, permanent failure attributed to the wrong subsystem.
retryingappears in every one of the incident tracebacks (service/model.py:188) — it was on the stack, ready to handle exactly this, and never got the chance.(Nuance:
retryingwraps RPC/HTTP dispatch. A CLIodoo -uupgrade gets no retry — on that path the fix's value is that the upgrade fails loudly with the true cause instead of a misleading victim traceback.)The fix
Re-raise the retryable classes ahead of the existing broad catch, in both hooks:
A split, not a removal — deliberately. Queueing a scan genuinely is best-effort: a misconfigured queue channel raising
ValueErrormust not block attachment creation across the platform. Only database errors, which cannot be safely ignored, now propagate.Scope: 2 sites of 9
The file has nine
except Exceptionblocks. This changes only the two on thecreate/writerequest path.The other seven (
_scan_for_malware,_quarantine,_notify_security_admins,action_restore_quarantined,action_download_quarantined_for_analysis,action_rescan) are reached via queue jobs or explicit buttons, where a poisoned transaction is confined to that job rather than corrupting a user request. Same latent hazard, different blast radius — worth a follow-up (#385), deliberately not widened here.Tests
spp_attachment_av_scan/tests/test_scan_queue_error_handling.py— the module's suite goes 40 → 45 tests, 0 failed, 0 errors. That the pre-existing 40 still pass matters: re-raising from a hook that previously never raised is exactly the change that could break unrelated attachment tests.Both sides of the contract are asserted, because only one side is obvious:
..._propagates_on_create/..._propagates_on_writeSerializationFailurepropagates..._is_still_swallowed_on_create/..._on_writeValueErroris still logged and the attachment is still writtentest_the_retry_machinery_can_see_the_error_class_we_re_raiseSerializationFailureremains a subclass of whatretryingcatchesThe two "still swallowed" tests are the anti-vacuity guard: re-raising everything would pass the propagation tests while breaking best-effort queueing platform-wide. The last test guards the reason the fix works — a future refactor that re-raised some wrapped exception would no longer be retried, reviving the incident in a new disguise, and would fail here.
Negative control: with both re-raise clauses removed (restoring the exact pre-fix code), 2 of 45 fail — precisely the two propagation tests, while both "still swallowed" tests and the subclass check still pass. The tests detect the behaviour, not the presence of the code.
Verification notes
pre-commit run --filespasses on the changed files, exceptbandit, which fails withpyproject.toml : toml parser not available, reinstall with toml extra. That is a pre-existing environment fault in the hook, not this change: it fails identically onspp_attachment_av_scan/models/av_scanner_backend.py, which this PR does not touch.Operator note
For an instance already hitting this, the immediate workaround is to stop the job worker during a module upgrade so no
_scan_for_malwarejob races the attachment write. With this fix deployed that is unnecessary — the conflict returns to being retried invisibly.