Repository navigation
MonkeyPatch records stale undo entries when delattr/setitem/delitem fails #14909
Description
While looking at #14161 and its fix in #14209, I noticed that the same undo-ordering problem still exists in MonkeyPatch.delattr, MonkeyPatch.setitem, and MonkeyPatch.delitem.
These methods register an undo entry before attempting the mutation. If the mutation raises, the target remains unchanged, but undo() later tries to restore a change that never happened.
Minimal reproduction:
from types import MappingProxyType
import pytest
def test_failed_delattr(monkeypatch):
class A:
__slots__ = ()
x = 1
a = A()
with pytest.raises(AttributeError):
monkeypatch.delattr(a, "x")
assert a.x == 1
def test_failed_setitem(monkeypatch):
mapping = MappingProxyType({"x": 1})
with pytest.raises(TypeError):
monkeypatch.setitem(mapping, "x", 2)
assert mapping["x"] == 1
def test_failed_delitem(monkeypatch):
mapping = MappingProxyType({"x": 1})
with pytest.raises(TypeError):
monkeypatch.delitem(mapping, "x")
assert mapping["x"] == 1On current main, the original exceptions are caught and all three targets remain unchanged. However, fixture teardown calls undo() and produces another error for each test because the failed operations left stale undo entries.
I think the fix would be to follow the ordering already used by MonkeyPatch.setattr after #14209:
- Save the original value.
- Attempt the mutation.
- Register the undo entry only after the mutation succeeds.
Successful operations would keep their existing behavior.
I reproduced this locally and have a small fix with regression tests ready. If maintainers agree that this is a bug and that the proposed approach is appropriate, I would be happy to submit a PR. Could this issue be assigned to me?
Environment:
- pytest
9.2.0.dev204+gc0ab40c93 - Python 3.12.13
- Red Hat Enterprise Linux 9.8
Activity
[-]MonkeyPatch delattr/setitem/delitem leave stale undo entries when mutation fails[/-][+]MonkeyPatch records stale undo entries when delattr/setitem/delitem fails[/+]sharonyao1127 commentedon Aug 19, 2026
Opened a PR applying the same "save original -> mutate -> register undo only on success" ordering (as used by
setattrafter #14209) todelattr,setitemanddelitem, with regression tests mirroring the reproduction above.fix(MonkeyPatch): register undo entry only after a successful mutatio…
fix(MonkeyPatch): register undo entry only after a successful mutatio…
fix(MonkeyPatch): register undo entry only after a successful mutatio…
Amine-LG commentedon Aug 20, 2026
For completeness, I've now committed and pushed the fix and regression tests I mentioned when opening #14909: 5d3421c.
I had already prepared and tested this patch locally before opening the issue, but held off publishing it while waiting for maintainer review and assignment guidance. Since #14910 was opened in the meantime, I wanted to make my existing implementation available for comparison.
This version follows the successful-mutation-first ordering accepted for
setattrin #14209 and adds three focused regression tests for faileddelattr,setitem, anddelitem, each explicitly verifying thatundo()remains safe. It also includes the changelog and AUTHORS updates. The targeted tests, completetesting/test_monkeypatch.pymodule, pre-commit checks, and local patch-coverage check all pass.I'm ready to open a PR if maintainers prefer this version, and otherwise I'll follow your guidance on how to handle the overlap.
Gambit-Checkmate commentedon Aug 22, 2026
Hi, I’d like to investigate this MonkeyPatch stale-undo-entry case. I can reproduce the failing delattr/setitem/delitem paths, add regression tests for each operation, and then keep the fix limited to the rollback bookkeeping. Is this still available for a community PR?
Amine-LG commentedon Aug 22, 2026
Thanks for checking first. I originally reported this after reproducing it and already had a tested local fix prepared. #14910 was subsequently opened for the same issue so there is already overlapping work and the maintainers haven't yet weighed in. It's probably best to wait for their direction before adding another implementation.
Merge pull request #14910 from sharonyao1127/fix-monkey-patch-delattr…