EmbedStore.__getitem__ reads the index and the data unlocked, so a concurrent reader can fail with "Error while getting the slice" #690

Summary

EmbedStore.__getitem__ reads the key index and the node bytes as two separately-synchronised operations, with no lock spanning them. A reader concurrent with a writer can obtain a freshly-synced index entry whose (offset, length) its own stale view of the backing schunk does not yet cover, and the slice fails:

RuntimeError: Error while getting the slice   (blosc2/blosc2_ext.pyx:2232)

Affects EmbedStore, and by inheritance DictStore and TreeStore. Seen on 4.10.1.dev0.

The asymmetry

The write path is properly locked — __setitem__ (embed_store.py:263) runs the whole mutation inside _write_bracket(), which holds the exclusive frame lock:

with self._write_bracket():                    # holding_lock() + _sync_metadata()
    ...
    offset = self._current_offset
    self._store[offset : offset + data_len] = serialized_data
    self._current_offset += data_len
    self._embed_map[key] = {"offset": offset, "length": data_len}
    self._save_metadata()                      # index published, still under the lock

The read path takes no lock at all — __getitem__ (embed_store.py:289):

self._sync_metadata()                          # (1) refresh index from vlmeta
...
offset = node_info["offset"]
length = node_info["length"]
serialized_data = bytes(self._store[offset : offset + length])   # (2) read bytes -- RAISES

Steps (1) and (2) are independent. _sync_metadata() polls vlmeta and re-syncs C-level state including change_tick; the subsequent get_slice bounds-checks against something that poll does not appear to refresh. _schunk_store defaults to True, so _backing_schunk is _store — this is not two handles drifting apart, it is one handle whose cached extent disagrees with the index it just re-read.

What is already guaranteed

Because __setitem__ writes the bytes before publishing the map entry, and does both under the lock, a visible index entry implies the bytes are on disk. The reader is not observing a genuinely absent node; it is observing a stale view of a node that is really there. Any fix therefore only has to make the reader look at current state — it does not have to introduce ordering.

Also, since data is append-only and offsets only grow, a stale extent is always too short, so this raises rather than silently returning another node's bytes.

Traceback

node = estore.get(key)
  src/blosc2/embed_store.py:311  in get          -> return self[key]
  src/blosc2/embed_store.py:301  in __getitem__  -> bytes(self._store[offset : offset + length])
  src/blosc2/schunk.py:1241      in __getitem__  -> self.get_slice(item.start, item.stop)
  src/blosc2/schunk.py:1185      in get_slice    -> super().get_slice(start, stop, out)
  blosc2/blosc2_ext.pyx:2232                     -> RuntimeError: Error while getting the slice

Reproduction

tests/test_locking.py::test_embed_store_cross_process_writers — two writer subprocesses appending to a locking=True store while the main process lists keys and reads them. Intermittent; needs a loaded box to hit the window.

It surfaced when the test suite moved to running in parallel (2 xdist workers -> 4 on a 4-core CI runner). Failing CI job, ubuntu-latest / Python 3.12: run 31008426106, job 92314070183.

Scope

Triggered only when all three hold:

• _shared is true — i.e. locking=True (or BLOSC_LOCKING set and mmap_mode is None) and urlpath is not None (_set_shared, embed_store.py:194)
• reads are genuinely concurrent with writes from another process — sequential use with two handles never hits the window
• the node is embedded, so the read goes through EmbedStore.__getitem__

Note that _shared is not an edge case here, it is the cross-process feature itself: with _shared false, _sync_metadata() returns immediately and a second handle never observes another process's writes at all. The defect sits in the mode whose purpose is coherent concurrent access.

DictStore.__getitem__ inherits this directly via its return self._estore[key] fallback for embedded values. Its external-leaf path re-syncs under holding_lock() in _sync_store() but then opens the file after releasing the lock — possibly the same shape, not traced. TreeStore(DictStore) inherits both. Plain SChunk/NDArray reads are unaffected: they have no separate index in vlmeta to fall out of step with the data.

Possible fixes

1. Lock the read, mirroring the write. Wrap the index lookup and the slice in self._backing_schunk.holding_lock(), keeping from_cframe() outside the block since the bytes are already copied. Correct regardless of which cached field is stale, and symmetric with _write_bracket(). Cost: holding_lock() is exclusive by its own docstring ("including plain reads through other locked handles"), so reads serialise against each other, not only against writers.

2. Fix the staleness at the source. Work out why the vlmeta poll refreshes change_tick and the metadata but not the extent get_slice validates against, and refresh that too. Then reads need neither a lock nor a retry, because the data-before-index ordering already makes them safe. This is the better fix; it needs someone familiar with that layer.

A bounded retry on RuntimeError would also work, since the bytes are genuinely present — but it hides the staleness rather than fixing it, and converts a correctness bug into a latency bug that returns under load.

Note for whoever fixes this

test_embed_store_cross_process_writers and test_dict_store_cross_process_writers currently swallow this error to keep CI green (commit 96096b0):

except RuntimeError:
    continue

Those guards are scoped to the concurrent loop only — every key is still verified strictly for presence and contents once the writers exit — but they are scaffolding. They should be removed as part of the fix, otherwise the tests will no longer catch a regression of exactly this bug. They are the reproduction case.