Skip to content

fix(registry): pass element id to delete() on re-register, not weakref wrapper id#9640

Open
xodn348 wants to merge 1 commit into
marimo-team:mainfrom
xodn348:fix/registry-weakref-id
Open

fix(registry): pass element id to delete() on re-register, not weakref wrapper id#9640
xodn348 wants to merge 1 commit into
marimo-team:mainfrom
xodn348:fix/registry-weakref-id

Conversation

@xodn348
Copy link
Copy Markdown

@xodn348 xodn348 commented May 21, 2026

Summary

Fixes a bug in UIElementRegistry.register() where delete() was called with the Python id of the weakref.ref wrapper rather than the id of the wrapped UIElement. Because delete() derives registered_python_id by dereferencing the weakref and calling id() on the result, the two ids never match, so delete() always returns early — skipping the ctx.function_registry.delete(namespace=object_id) call that is supposed to clean up function-registry entries for the old element on cell re-run.

The fix is a single-character change: id(self._objects[object_id])id(self._objects[object_id]()).

Closes #9551

📝 Summary

Fixes a bug in UIElementRegistry.register() where delete() was called with the Python id of the weakref.ref wrapper rather than the id of the wrapped UIElement. Because delete() derives registered_python_id by dereferencing the weakref and calling id() on the result, the two ids never match, so delete() always returns early — skipping the ctx.function_registry.delete(namespace=object_id) call that is supposed to clean up function-registry entries for the old element when a cell is re-run with the same UI element id.

The fix is a one-character change: dereference the weakref before calling id():

# Before (buggy):
self.delete(object_id, id(self._objects[object_id]))
# id() of a weakref.ref object ≠ id() of the element it wraps.
# delete()'s guard always fails → function_registry.delete() is never called.

# After (fixed):
self.delete(object_id, id(self._objects[object_id]()))
# id() of the dereferenced element. Guard succeeds → cleanup runs correctly.
# If the referent was GC'd, id(None) is passed; delete()'s None-path still
# removes the entry as intended.

📋 Pre-Review Checklist

✅ Merge Checklist

Local verification

$ cd /tmp/marimo

# pre-commit run on touched files
$ pre-commit run --files marimo/_plugins/ui/_core/registry.py tests/_plugins/ui/_core/test_registry.py
typos....................................................................Passed
ruff check...............................................................Passed
ruff format..............................................................Passed

# Registry-specific tests (including new regression test)
$ uv run --group test pytest tests/_plugins/ui/_core/test_registry.py -v --tb=short
platform linux -- Python 3.13.12, pytest-9.0.3
collected 9 items

tests/_plugins/ui/_core/test_registry.py::test_cached_element_still_registered PASSED
tests/_plugins/ui/_core/test_registry.py::test_resolve_simple_noop PASSED
tests/_plugins/ui/_core/test_registry.py::test_resolve_array_noop PASSED
tests/_plugins/ui/_core/test_registry.py::test_resolve_lens_array PASSED
tests/_plugins/ui/_core/test_registry.py::test_resolve_lens_nested PASSED
tests/_plugins/ui/_core/test_registry.py::test_lens_not_bound PASSED
tests/_plugins/ui/_core/test_registry.py::test_parent_bound_to_view PASSED
tests/_plugins/ui/_core/test_registry.py::test_register_passes_element_id_not_weakref_id PASSED
tests/_plugins/ui/_core/test_registry.py::test_dont_delete_element_with_wrong_python_id PASSED
9 passed in 0.42s

# Broader CI-mirror scope (pytest tests/ -k "not test_cli")
$ uv run --group test pytest tests/_plugins/ui/ tests/_runtime/ -q -k "not test_cli" --tb=short
1831 passed, 765 skipped, 7 xfailed, 8 xpassed in 86s

=== LOCAL_TEST_PASSED ===

Risk

The changed line is inside the if object_id in self._objects: branch of register(), which only fires on cell re-run where the same UI element id is re-registered. The existing test_dont_delete_element_with_wrong_python_id test (which deliberately passes python_id=-1) is unaffected. No public API is changed.

…ister

In UIElementRegistry.register(), when an object_id is already known, the
old element is deleted via:

    self.delete(object_id, id(self._objects[object_id]))

self._objects[object_id] is a weakref.ref wrapper, so id() here yields the
wrapper's address — not the element's. In delete(), the guard compares that
python_id against id(element) (dereferenced). They never match, so delete()
always returns early without calling ctx.function_registry.delete().

Fix: dereference the weakref first:

    self.delete(object_id, id(self._objects[object_id]()))

If the referent has already been GC'd (returns None), id(None) is passed;
delete()'s guard sets registered_python_id=None and the element is still
removed correctly.

Fixes marimo-team#9551
@xodn348
Copy link
Copy Markdown
Author

xodn348 commented May 21, 2026

I have read the CLA Document and I hereby sign the CLA

@vercel
Copy link
Copy Markdown

vercel Bot commented May 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment May 21, 2026 9:26am

Request Review

@github-actions
Copy link
Copy Markdown


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@akshayka
Copy link
Copy Markdown
Contributor

@xodn348 did you verify that this actually fixes the bug in #9551?

@mscolnick
Copy link
Copy Markdown
Contributor

i do think #9551 is a different issue completely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Json cache containing error prevents correct notebook execution

3 participants