[FIX] base_bg: reaper timeout config, duplicate timeout DM, notification escaping - #442
Open
nicomacr wants to merge 5 commits into
Open
[FIX] base_bg: reaper timeout config, duplicate timeout DM, notification escaping#442nicomacr wants to merge 5 commits into
nicomacr wants to merge 5 commits into
Conversation
Contributor
|
|
The reaper computed its cutoff as `tools.config.get("limit_time_real_cron") or 0`.
With the core default (-1, meaning "use --limit-time-real") that yields a cutoff
one second in the future, and with 0 a cutoff of now: in both cases every running
job was marked failed (and the rest of its batch canceled) on each reaper run.
Resolve the limit the way the running server does — prefork: 0/unset means no
limit and -1 delegates to --limit-time-real (PreforkServer.__init__); threaded:
the value only applies when > 0, anything else falls back to --limit-time-real
(ThreadedServer.process_limit). And when no limit applies, keep the orphan sweep
running instead of skipping the reaper entirely: a stuck running job whose
records are gone can never finish on its own and would block its batch forever.
When the reaper gave up on a timed-out job, the user got two DMs: one from
_handle_job_error -> _give_up -> fail() ("Job X failed: Job timed out") and a
second one from the reaper loop itself ("Job X timed out").
Drop the loop's re-notification and keep fail()'s single message, which also
carries the related-record links. To preserve the one thing the loop message
added — a clickable reference to the job — fail() now renders the job name as
its html link.
fail() interpolated the job name and error message with plain % into a string that _notify_user then wrapped in Markup(): an error_message with angle brackets (any traceback with an object repr) broke the message render, and a crafted job name was stored HTML injected into the user's DM (XSS). Build the failure message as Markup with escaped interpolations (the job link and the related-record links, already safe, pass through), and make _notify_user escape its input: a plain-string result is now rendered inert, while methods that mean HTML keep working by returning a Markup with its own interpolations escaped. Document the contract in the README's Usage section; the bg-enqueued methods that build HTML are aligned to it in the follow-up commits.
The background reconciliation result interpolated the statement line name and the exception text with plain % before wrapping in Markup(), injecting user data as HTML into the DM that base_bg posts. Interpolate through the Markup template instead, which escapes the values. Also align the fail() override with the new base signature (notify keyword), so a registry call passing notify= does not TypeError with both modules installed.
… import DM The background import result built its HTML with an f-string interpolating the statement name before wrapping in Markup(), injecting user data as HTML into the DM that base_bg posts. Interpolate through the Markup template instead, which escapes the values, and mark the message translatable while at it.
nicomacr
force-pushed
the
19.0-t-72869-nmr
branch
from
August 31, 2026 18:02
7558846 to
f867bf6
Compare
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.
Three pre-existing fixes in
base_bg/models/bg_job.py, found while reviewing #437 (they predate that PR). One commit per fix, plus two follow-up commits from the review round:1. The reaper failed every running job when
limit_time_real_cronis unset or 0.The cutoff was computed as
tools.config.get("limit_time_real_cron") or 0. With the core default (-1, meaning "use--limit-time-real") the cutoff lands one second in the future, and with0it lands at now: in both cases each reaper run marked every running job as failed and canceled the rest of its batch. The limit is now resolved the way the running server does — prefork:0/unset means no limit,-1delegates to--limit-time-real(PreforkServer.__init__); threaded: the value only applies when > 0, anything else falls back to--limit-time-real(ThreadedServer.process_limit). And when no limit applies, the orphan sweep keeps running instead of skipping the reaper entirely: a stuck running job whose records are gone can never finish on its own and would block its batch forever.2. A permanently timed-out job DM'd its creator twice.
_handle_job_error -> _give_up -> fail()already posts "Job X failed: Job timed out", and the reaper loop posted a second "Job X timed out" right after. The loop's re-notification is gone; to keep the clickable job reference it provided,fail()now renders the job name as its html link (which also brings that link to every failure DM).3.
name/error_messagereached the DM unescaped.fail()interpolated them with plain%and_notify_userwrapped the result inMarkup(): an error message with angle brackets (any traceback with an object repr) broke the render, and a crafted job name was stored HTML injected into the creator's DM. The failure message is now built asMarkupwith escaped interpolations, and_notify_userescapes its input — a plain-string result renders inert, while methods that mean HTML keep working by returning aMarkupwith its own interpolations escaped. The contract is documented in the README, and the two bg-enqueued methods in this repo that built HTML with plain%are aligned to it in their own commits (account_reconcile_bg, which also picks up the newfail(notify=)signature, andaccount_statement_import_sheet_file_bg).Merge together with ingadhoc/odoo-saas-adhoc#3182 (same branch name, bundled build):
ai_app_adhocreturns a plain str with HTML from a bg-enqueued method and needs theMarkuptreatment before this lands.Test plan:
--test-tags /base_bg,/account_reconcile_bgon a 19.0 database: 59 + 4 tests, 0 failed (new since the review: reaper threaded-mode fallback at0, orphan sweep with no time limit).Task: https://www.adhoc.inc/odoo/project.task/72869