Skip to content

[FIX] base_bg: do not let one job break the whole reaper - #437

Closed
nicomacr wants to merge 1 commit into
ingadhoc:19.0from
adhoc-dev:19.0-t-72579-nmr
Closed

[FIX] base_bg: do not let one job break the whole reaper#437
nicomacr wants to merge 1 commit into
ingadhoc:19.0from
adhoc-dev:19.0-t-72579-nmr

Conversation

@nicomacr

@nicomacr nicomacr commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Problema

Un job cuyos registros objetivo ya no existen revienta al ser dado por vencido: fail() browsea esos ids para linkearlos en la notificación y, como browsear un id borrado devuelve un recordset truthy, leer su display_name levanta MissingError. La excepción aborta el loop entero de _cron_check_running_jobs: la transacción se rollbackea, el job envenenado queda en running y ningún otro job colgado se procesa, en esa corrida ni en las siguientes.

El mismo fail() corre desde run()_handle_job_error_give_up, así que un job con su target borrado también aborta el cron del runner.

Cambio

  • Causa raíz: fail() chequea existencia real (_get_records().exists()) antes de linkear los registros en la notificación. Aplica a jobs de cualquier modelo.
  • Hardening del reaper: cada job se maneja en su propio savepoint. Error transitorio de PG (_is_transient_error) → warning y se deja para la próxima corrida; cualquier otra cosa (típicamente un override del modelo que revienta) → el job se marca failed con las implementaciones base, sin notificar, y se cancela el resto del batch. Ese camino de recuperación corre en su propio savepoint, así que si también revienta el reaper sigue con los demás jobs vencidos.

Tests

base_bg/tests/test_bg_job.py:

  • test_fail_notifies_when_records_were_deleted: fail() sobre un job cuyos registros se borraron → queda failed sin levantar MissingError.
  • test_cron_check_running_jobs_skips_poisoned_job: un override de fail() que revienta + un job sano → el envenenado queda failed con su batch cancelado y el sano se vence igual.
  • test_cron_check_running_jobs_defers_transient_error: SerializationFailure → el job queda running para la próxima corrida.

Tarea: https://www.adhoc.inc/odoo/project.task/72579

@roboadhoc

Copy link
Copy Markdown
Contributor

Pull request status dashboard

@nicomacr
nicomacr requested a review from JoelZilli August 19, 2026 16:26
@nicomacr nicomacr changed the title [FIX] base_bg: keep the reaper going when a job cannot be timed out [FIX] base_bg: do not let one job break the whole reaper Aug 19, 2026
@nicomacr
nicomacr requested a review from joa-adhoc August 20, 2026 12:12
Failing a job whose target records were deleted raised MissingError: fail()
browsed those ids to link them in the notification, and browsing a dropped id is
truthy. That aborted the whole _cron_check_running_jobs loop, so the poisoned job
stayed running forever and none of the other stuck jobs were ever handled.

Check real existence before linking the records, and handle each job -- and the
recovery path itself -- in its own savepoint, so an override that raises no
longer takes the reaper down with it.

@JoelZilli JoelZilli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-revisado después del amend. LGTM, sin bloqueos — dejo esto como comentario y no como Approve formal por ahora, pero de contenido está aprobado de mi lado.

Verificado

  • fail() con _get_records().exists(): la raíz queda cerrada y el camino que me preocupaba converge — run()except_handle_job_error_give_upfail ya no levanta, así que _cron_run_enqueued_jobs no aborta más. Aplica a jobs de cualquier modelo, como dice el body.
  • Fallback en su propio savepoint + try/except: tenés razón en la corrección, el savepoint solo no alcanzaba porque la excepción se propagaba igual y mataba el loop.
  • Afuera el invalidate_all(flush=False), con el comentario que explica por qué no hace falta.
  • Mi observación sobre mapped() estaba mal y tu corrección es correcta: en Odoo 19, mapped(func) sobre un recordset vacío llama func(self) una vez con el recordset entero (orm/models.py:6174-6183), así que el r and sí cubría el caso vacío — lo que no cubría era el id borrado, que es el bug. La conclusión no cambia y if records: + comprehension explícita es mejor que sacar el lambda a secas.
  • El cambio del test envenenado es acertado: con el fix de fail(), borrar el partner ya no envenena nada, así que el test viejo hubiera pasado incluso sin el savepoint. Chequeé que patch.object(type(self.BgJob), "fail", ...) no toca el BgJob.fail del fallback (clase de módulo, distinta de la del registry) y que base_fail se captura antes del patch: el test ejercita de verdad el camino de recuperación.

Nits

Tres comentarios inline, ninguno bloquea. Nada más de mi lado.

No corrí los tests localmente (habría que cambiar de rama en mi working tree); me apoyo en el CI. Verifiqué mapped() y la semántica de _FlushingSavepoint leyendo el core 19.

Comment thread base_bg/models/bg_job.py
Comment thread base_bg/models/bg_job.py
Comment thread base_bg/models/bg_job.py
@nicomacr

Copy link
Copy Markdown
Contributor Author

@roboadhoc r+ nobump

roboadhoc pushed a commit that referenced this pull request Aug 21, 2026
Failing a job whose target records were deleted raised MissingError: fail()
browsed those ids to link them in the notification, and browsing a dropped id is
truthy. That aborted the whole _cron_check_running_jobs loop, so the poisoned job
stayed running forever and none of the other stuck jobs were ever handled.

Check real existence before linking the records, and handle each job -- and the
recovery path itself -- in its own savepoint, so an override that raises no
longer takes the reaper down with it.

closes #437

Signed-off-by: Nicolas Mac Rouillon (ADV) <nmr@adhoc.inc>
@roboadhoc roboadhoc closed this in 91beebb Aug 21, 2026
@roboadhoc
roboadhoc deleted the 19.0-t-72579-nmr branch August 21, 2026 15:47
@roboadhoc roboadhoc added the 18.1 label Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants