fix: Prevent exponential custom-option id growth on re-customization

[SimonHeybrock]

Description

Each time .opts() is applied to an object that already carries a custom-option id, StoreOptions.create_custom_trees relocates the object's existing option trees to fresh ids so the new customization does not mutate shared trees. The new ids were derived from the value of the old id (old_id + offset + 1, where offset is one above the store's current maximum id) instead of from a fresh contiguous range. Because ids are opaque keys into Store._custom_options, their absolute value carries no meaning, and computing the new id from it makes the maximum id roughly double on every re-customization of a retained object.

The trigger, in one sentence: on every update, the same retained element is re-customized by an option that matches it. The element then holds the store's largest id, so its new id old_id + offset + 1 is ~2 × max, and because it is retained it compounds — the maximum id grows as 2^n. The number of live entries stays bounded (superseded entries are reclaimed by the weakref finalizer), but the integer magnitude explodes. Over a long-running session this pins a CPU core in bignum arithmetic and, on Python 3.11+, eventually raises ValueError: Exceeds the limit (4300 digits) for integer string conversion from any repr()/str() of the object. Interactive "customize once, then display" usage never triggers it, which is why it goes unnoticed in notebooks but bites long-running streaming servers.

Reproducer 1 — a streaming overlay with jittered/partial updates (no static element required). A dashboard overlays several curves and re-styles the overlay on each update, but only a subset of the curves receives fresh data each cycle (e.g. message-arrival jitter). Curves that skip an update are the same retained objects carrying their previous ids, so they double every cycle:

import holoviews as hv
from holoviews import opts
hv.extension('bokeh')
from holoviews.core.options import Store

names = list('abc')
curves = {k: hv.Curve([(0, 0)], label=k) for k in names}
for cycle in range(15):
    k = names[cycle % len(names)]                              # only one curve gets new data
    curves[k] = hv.Curve([(x, x + cycle) for x in range(cycle + 2)], label=k)
    hv.Overlay(list(curves.values())).opts(opts.Curve(line_width=2))   # restyle the overlay
    print(cycle, max(Store._custom_options['bokeh']))          # 0,2,6,13,26,50,...,11423

Reproducer 2 — a retained reference element overlaid with fresh data. Keeping any element around and overlaying it with newly-built data each update, with styling that matches it, is enough — the kept element need not even be pre-styled:

reference = hv.Curve([1, 2, 3], label='ref')      # kept around by the app
def callback(i):
    data = hv.Curve(range(i + 2), label='data')   # a new curve every update
    return (reference * data).opts(opts.Curve(line_width=2))   # styling matches both curves
# driving this on a Stream gives reference.id = 2, 14, 126, 2046, 65534, ...

What does not trigger it (all stay linear/polynomial): building a fresh element every update; applying .opts() to the DynamicMap once and then streaming; updating every curve in the overlay each cycle (no retention); or styling that does not match the retained element (e.g. an Overlay-level option, or a retained VLine with a Curve-level option).

Fix. Allocate the relocated ids from a process-wide monotonic counter (lock-guarded), handing each customization a fresh contiguous block instead of deriving ids from the old id's value. This satisfies the only two properties the relocation requires — new ids disjoint from existing keys, and a bijection over the object's own id set — and adds two more that the old offset = max(store)+1 scheme lacked:

• Never reused. The counter only increases, so the maximum id grows linearly with the number of re-customizations rather than exponentially. A 5-curve overlay restyled with jittered partial updates grows at ~2 ids/cycle (measured, empirical order 1.00), so even a multi-day run at 1 Hz leaves the id a six-digit integer — far from the 4300-digit limit. The CPU blow-up and the ValueError are gone. (The id still increases monotonically; it is linear, not bounded — truly bounding it would require recycling freed slots, unnecessary at these magnitudes.)
• Concurrency-safe. Concurrent customizations (e.g. a live multi-threaded dashboard) reserve disjoint id blocks instead of colliding on a recomputed offset. A naive max(store)+1 offset is not safe here: two threads compute the same offset, one thread's freshly stored tree is reclaimed by the other, and the id is gone when it is propagated (New option id ... does not match any option trees). The commit history shows this dead end — a simpler contiguous-block attempt, the thread-safety test that exposes it, its revert, and this replacement. Note that id allocation was never formally thread-safe, including before this PR; the previous exponential ids just made a damaging collision astronomically unlikely (sparse, per-object-unique values), so it was effectively immune in practice rather than correct. This change replaces that accident with an actual lock.

The same root cause could also alias sibling elements onto a single id: when a container is re-customized in a different backend (so the children's ids are absent from the target backend's store), the previous logic mapped all of them to offset, collapsing their per-backend options together. Distinct ids per element resolve this too. Regression tests cover the streaming-overlay growth, the cross-backend collision, thread-safety, and the single-object case.

AI Disclosure

• Tool & Model: Claude Code + Opus 4.8

• Usage: Confirmed the defect against main, ran the reproducer, traced the mechanism through create_custom_trees/update_backends, and drafted the tests, fix, and this description. All reviewed and verified locally by the author.

• I have tested all AI-generated content in my PR.

• I take responsibility for all AI-generated content in my PR.

Checklist

• Pull request title follows the conventional format
• Tests added and are passing

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.30%. Comparing base (93e05ed) to head (c69b057).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6904      +/-   ##
==========================================
+ Coverage   89.26%   89.30%   +0.04%     
==========================================
  Files         344      344              
  Lines       74870    74993     +123     
==========================================
+ Hits        66829    66974     +145     
+ Misses       8041     8019      -22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SimonHeybrock]

This runs ~7s or so, should it be marked as slow/skipped somehow?

[hoxbro]

I assume this is to test the threading.Lock in reserve_ids? I think we can make it more focused.

        n_workers = 8
        barrier = threading.Barrier(n_workers)
        def worker(w):
            barrier.wait()
            items = []
            for i in range(120):
                start_id = hv.StoreOptions.reserve_ids(i)
                items.extend(range(start_id, start_id + i))
            return items

        with ThreadPoolExecutor(max_workers=n_workers) as executor:
            total_items = list(executor.map(worker, range(n_workers)))

        a = sum(total_items, [])  # noqa: RUF017
        assert len(a) == len(set(a))

With that being said, I cannot get this or the current implementation to fail when I change the Lock to nullcontext. Is the lock only needed on Python Freethreading? if so we should do something like this:

diff --git a/holoviews/core/options.py b/holoviews/core/options.py
index 304bb6b0b..d2ea01098 100644
--- a/holoviews/core/options.py
+++ b/holoviews/core/options.py
@@ -37,11 +37,12 @@ from __future__ import annotations
 import difflib
 import inspect
 import pickle
+import sysconfig
 import threading
 import traceback
 import typing as t
 from collections import defaultdict
-from contextlib import contextmanager
+from contextlib import contextmanager, nullcontext
 
 import numpy as np
 import param
@@ -56,6 +57,9 @@ if t.TYPE_CHECKING:
     from ..util.settings import OutputSettings
 
 
+_PYTHON_FREETHREADING = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))
+
+
 def __getattr__(name):
     if name == "Opts":
         from ..util.warnings import deprecated
@@ -1218,7 +1222,7 @@ class Store:
     # its allocation. Ids are never reused, so concurrent customizations
     # receive disjoint id blocks instead of colliding on a recomputed offset.
     _id_counter = 0
-    _id_lock = threading.Lock()
+    _id_lock = threading.Lock() if _PYTHON_FREETHREADING else nullcontext()
 
     # Backend option caches
     _lookup_cache = {}

[SimonHeybrock]

Is the lock only needed on Python Freethreading?

I am using regular Python 3.11.

I had a different fix initially (see 4th commit in the branch, a375344). This lead to exceptions about unknown IDs (sporadic, not always) in our dashboard (background thread, potentially multiple connected sessions). 7b3015e was the answer to this, but I have not tested it extensively without the lock.

[SimonHeybrock]

Good call on the focused test -- I've added essentially your version as test_concurrent_id_reservation_yields_disjoint_blocks, exercising reserve_ids directly. For now I kept the slower end-to-end test as well, since that's the one that can fail on main (the focused one needs reserve_ids, which doesn't exist pre-fix); happy for it to be dropped or skipped later if the ~8s isn't worth it.

On whether the lock is needed off free-threading: I'd keep it unconditional rather than gate it on Py_GIL_DISABLED.

reserve_ids is a read-modify-write that spans several bytecodes:

base = max(Store._id_counter, cls.id_offset())  # load + call id_offset()
Store._id_counter = base + n                     # separate store

The GIL only makes a single bytecode atomic. id_offset() is a function call that iterates backends and builds lists -- guaranteed thread-switch points -- and the store of _id_counter is a separate op from the load. So two threads can read the same counter, compute the same base, and both write base + n, handing out overlapping id blocks. That's a plain lost update; the GIL does not prevent it.

The reason it's hard to reproduce is timing, not absence: a normal opts() call tends to finish within one thread-scheduling quantum (~5 ms default switch interval), so in practice the read→write rarely gets interrupted at the wrong moment. Force the scheduler to switch and it shows up. The end-to-end concurrency test demonstrates this on unfixed code: at 08006b8fd (the test refactor sitting on top of the revert, before the fix is reapplied) test_concurrent_recustomization_keeps_styles_distinct fails on a regular GIL build. That commit reverts the whole fix, so it shows the bug exists, not specifically that the lock is the cure.

To isolate the lock itself, test_concurrent_id_reservation_yields_disjoint_blocks drives reserve_ids directly with sys.setswitchinterval(1e-6): with the lock it's always zero duplicates, and swapping only Lock→nullcontext on the fixed code reliably yields tens of thousands of duplicate ids. That no-lock state isn't a commit (the test needs reserve_ids, which doesn't exist pre-fix), but it's a one-line local change to check. This all matches the crashes we saw in a threaded dashboard on a regular GIL build.

So nullcontext on GIL builds wouldn't be an optimization -- it would reintroduce a real, if rare, race. An uncontended threading.Lock is a few tens of ns here, probably nothing to worry about?

[hoxbro]

I have left some comments.

For the tests, I think we make the test as self explaining as possible. When I see a comment spanning multiple lines I tend to not read it 🙃, and potentially it can also get out of sync with the actual code. Maybe add a link to this PR in the class docstring if people wants to know more.

Have you verified that these tests fails on main?

[hoxbro]

We should change this back in teardown method. Though, I can see this is not done for the other ones in the file either.

    def setup_method(self):
        self._backend = hv.Store.current_backend
        hv.Store.set_current_backend("matplotlib")

    def teardown_method(self):
        hv.Store.current_backend = self._backend

[hoxbro]

I assume this is to test the threading.Lock in reserve_ids? I think we can make it more focused.

        n_workers = 8
        barrier = threading.Barrier(n_workers)
        def worker(w):
            barrier.wait()
            items = []
            for i in range(120):
                start_id = hv.StoreOptions.reserve_ids(i)
                items.extend(range(start_id, start_id + i))
            return items

        with ThreadPoolExecutor(max_workers=n_workers) as executor:
            total_items = list(executor.map(worker, range(n_workers)))

        a = sum(total_items, [])  # noqa: RUF017
        assert len(a) == len(set(a))

With that being said, I cannot get this or the current implementation to fail when I change the Lock to nullcontext. Is the lock only needed on Python Freethreading? if so we should do something like this:

diff --git a/holoviews/core/options.py b/holoviews/core/options.py
index 304bb6b0b..d2ea01098 100644
--- a/holoviews/core/options.py
+++ b/holoviews/core/options.py
@@ -37,11 +37,12 @@ from __future__ import annotations
 import difflib
 import inspect
 import pickle
+import sysconfig
 import threading
 import traceback
 import typing as t
 from collections import defaultdict
-from contextlib import contextmanager
+from contextlib import contextmanager, nullcontext
 
 import numpy as np
 import param
@@ -56,6 +57,9 @@ if t.TYPE_CHECKING:
     from ..util.settings import OutputSettings
 
 
+_PYTHON_FREETHREADING = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))
+
+
 def __getattr__(name):
     if name == "Opts":
         from ..util.warnings import deprecated
@@ -1218,7 +1222,7 @@ class Store:
     # its allocation. Ids are never reused, so concurrent customizations
     # receive disjoint id blocks instead of colliding on a recomputed offset.
     _id_counter = 0
-    _id_lock = threading.Lock()
+    _id_lock = threading.Lock() if _PYTHON_FREETHREADING else nullcontext()
 
     # Backend option caches
     _lookup_cache = {}

[hoxbro]

Move these to top-level

[hoxbro]

Suggested change

	# Monotonic high-water mark for custom-option ids and the lock guarding
	# its allocation. Ids are never reused, so concurrent customizations
	# receive disjoint id blocks instead of colliding on a recomputed offset.

I don't think this comment is needed. Maybe it could be limited to one line.

[SimonHeybrock]

Agreed -- trimmed to a single line pointing at reserve_ids, where the monotonic, disjoint allocation is documented. I kept a one-line breadcrumb rather than dropping it entirely so the two bare class attributes (_id_counter, _id_lock) still hint at where the logic lives.

[hoxbro]

Suggested change

	# Relocate the object's trees to a fresh contiguous block above the
	# offset. Ids are opaque keys into Store._custom_options; the only
	# requirements are that the new ids are disjoint from existing keys
	# (guaranteed by the offset) and that distinct old ids map to distinct
	# new ids. Indexing by position keeps the maximum id linear in the
	# number of live customizations; deriving it from the old id's value
	# (tree_id + offset + 1) doubles the maximum on every re-customization.
	for new_id, tree_id in enumerate(obj_ids, start=offset):

[SimonHeybrock]

Done, removed. The rationale it carried -- why ids are relocated by position (enumerate) rather than by old-id value, and how the old tree_id + offset + 1 doubled the maximum on every re-customization -- moved into the commit message for the cleanup commit, since that's change history rather than something the remaining code needs to explain.

[SimonHeybrock]

Have you verified that these tests fails on main?

Yes, that is why the tests are separate commits before the fixing commit. You can also see from the commit history that the initial fix (which did not use a lock) was not sufficient (lead to crashes in our threaded dashboard).

[hoxbro]

Pushed some small updates in c69b057. Which mainly just reduced down the number of items and thread in the concurrent tests. Verified they fail by changing the thread lock to a null context.

[hoxbro]

Thank you for the PR