Opened 3 weeks ago

Closed 3 weeks ago

#37315 closed Bug (wontfix)

Backport #36487 to 5.2/6.0, or document partial callback crash with robust=True

Reported by: Bona Fide IT GmbH Owned by:
Component: Database layer (models, ORM) Version: 5.2
Severity: Normal Keywords:
Cc: Bona Fide IT GmbH Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is a follow-up to ticket #36487, which fixed an issue where transaction.on_commit error logging fails when using functools.partial with robust=True.

The fix landed in Django 6.1, but we ran into this issue on Django 5.2 LTS, and I'd like to propose either a backport or a documentation fix for the stable/5.2.x and stable/6.0.x branches.

Currently in 5.2 and 6.0, robust=True does the opposite of what it promises for partial callbacks: the except clause that is supposed to swallow the error raises an AttributeError of its own ('functools.partial' object has no attribute '__qualname__'), turning a handled error into an unhandled one.

from functools import partial
from django.db import transaction

def boom(**kwargs):
    raise OSError("broker is down")

transaction.on_commit(partial(boom, pk=7), robust=True)
# AttributeError: 'functools.partial' object has no attribute '__qualname__'

This is particularly painful because it stays invisible until a failure actually happens in production, and the official 5.2/6.0 documentation explicitly recommends this exact combination.

The docs for on_commit() recommend:

"Callbacks will not be passed any arguments, but you can bind them with functools.partial()"

And for robust=True they promise:

"All errors derived from Python's Exception class are caught and logged..."

Because 5.2 is an LTS release supported until April 2028, readers following the documentation will continue to hit this crash. Therefore, I'd like to propose two options for the triage team to consider:

Option 1: Code Backport (Preferred)
Backport the 2-line fix from #36487 to stable/5.2.x and stable/6.0.x. I understand the strict backport policy for extended support LTS releases (security/data-loss only). However, because the current behavior crashes a natively documented feature and turns handled errors into unhandled exceptions, an exception might be warranted.

Option 2: Documentation Warning (Fallback)
If a code backport is rejected under the backport policy, I propose adding a warning block to the 5.2 and 6.0 documentation for on_commit. The warning would clarify that robust=True crashes with functools.partial in these versions, and suggest the workaround (using a closure or manually setting __qualname__ on the partial).

I am happy to prepare the patch for whichever option the team prefers!

Change History (2)

comment:1 by Yassin Bahri, 3 weeks ago

I reproduced this using the same callback against the current stable/5.2.x, stable/6.0.x, and main branches.

from functools import partial

from django.db import transaction


def boom():
    raise OSError("broker is down")


transaction.on_commit(partial(boom), robust=True)

Results:

  • stable/5.2.x raises AttributeError: 'functools.partial' object has no attribute '__qualname__'.
  • stable/6.0.x raises the same AttributeError.
  • main catches and logs the callback exception as expected.

The behavior dates back to 4a1150b41d, which originally added the robust argument, so this isn't a regression in Django 5.2 or 6.0. It was fixed on main by 7a2f35b1b7 for #36487.

Both Django 5.2 and 6.0 are currently in extended support, during which only security and data-loss fixes are eligible for backporting. Documentation fixes are generally backported only to the latest stable release, where this issue is already fixed.

Therefore, although the reported behavior is real, I don't believe either proposed change currently qualifies for a backport under the supported versions policy. A merger would need to decide whether the fact that the 6.0 backport was missed during its mainstream support period warrants an exception.

I will keep things as is until someone else check this out too for a different opinion.

comment:2 by Jacob Walls, 3 weeks ago

Resolution: → wontfix
Status: new → closed

I know this is not the answer you are hoping for, but we prefer to stay very conservative with extended support versions. It will raise eyebrows if we start merging non data-loss/security fixes into extended support versions. Regarding the documentation idea, we prefer not to document bugs. This issue ticket should discoverable enough.

Going forward, the new annual release cycle in which every release is an LTS will ideally reduce the number of orgs hanging out on extended support versions. In other words, for future situations like this, you will be able to upgrade to the version with the fix with the comfort of that version also being an LTS.

Note: See TracTickets for help on using tickets.
Back to Top