DictStore.__getitem__ opens an external leaf after releasing the store lock, so a concurrent overwrite or delete can race it

[FrancescAlted]

Split out from #690, which noted this path but did not trace it. #690 was closed by #691, which fixed the EmbedStore half only.

Summary

DictStore.__getitem__ resolves an external leaf's path under the store lock, but opens the file after releasing it. A concurrent overwrite or delete of that key from another process removes and rewrites the file at that same path in the meantime, so the reader can open a file that is missing or only partially written.

Unlike #690 this is not a stale-cache problem — it is a plain time-of-check/time-of-use gap around the file open.

What is already correct

Worth stating, because it rules out the obvious reading. Both mutation paths run inside _mutation_bracket() (which holds the embed store's exclusive frame lock) and order the file write before publishing the map entry — __setitem__, dict_store.py:549-579:

# ... write the value to dest_path (save() / to_cframe() / copy2) ...
rel_path = os.path.relpath(dest_path, self.working_dir)
rel_path = rel_path.replace(os.sep, "/")
self.map_tree[key] = rel_path          # published only after the bytes are on disk
self._bump_store_tick()

So for a newly added key, a reader that sees the entry will find a complete file. That half is fine.

Where the gap is

__getitem__ (dict_store.py:586):

self._sync_store()                     # takes holding_lock() internally, then releases
if key in self.map_tree:
    filepath = self.map_tree[key]
    if filepath in self.offsets:
        ...
    else:
        urlpath = os.path.join(self.working_dir, filepath)
        if os.path.exists(urlpath):    # ← check
            return self._annotate_external_value(
                key, blosc2.open(urlpath, ...)   # ← use, no lock held
            )
        else:
            raise KeyError(...)

_sync_store() does the right thing internally — it re-syncs map_tree under holding_lock() — but it has released the lock by the time the path is resolved and opened.

Meanwhile __setitem__ overwriting an existing key does, under the lock (dict_store.py:525-529):

if key in self.map_tree:
    old_filepath = self.map_tree.pop(key)
    old_full_path = os.path.join(self.working_dir, old_filepath)
    if os.path.exists(old_full_path):
        os.remove(old_full_path)

and then rewrites the value. dest_path is derived deterministically from the key (rel_key + ext), so for an overwrite that keeps the same extension the remove and the rewrite hit the same path the reader is about to open. __delitem__ (dict_store.py:631) removes it outright.

So a reader interleaved with a concurrent overwrite of the same key can, between its os.path.exists() and its blosc2.open():

• find the file gone -> FileNotFoundError, or the os.path.exists check fails and it raises KeyError for a key that does exist
• find the file present but only partially rewritten -> a truncated or malformed cframe

Not reproduced

I have not got this to fire. tests/test_locking.py::test_dict_store_cross_process_writers only ever writes disjoint keys (/{tag}/ext{i}), so it never exercises a concurrent overwrite or delete of a key another process is reading — which is presumably why it has stayed green. A test that has one process rewriting or deleting the same key a reader is fetching in a loop would be the reproduction.

The .b2z sub-path (filepath in self.offsets -> blosc2_ext.open(self.b2z_path, offset=...)) may have a similar exposure if a store can be repacked while a reader holds an offset, but packing is a whole-store operation and I have not looked at whether that can overlap a read.

Possible fix

Mirror what #691 did for EmbedStore: resolve the path and open the file inside the same lock, and do any expensive work outside it. _sync_store() already knows how to take holding_lock(), so the shape is available. The same caveat from #691 applies — holding_lock() is exclusive, so this serialises reads against each other, and the block should stay short.

Scope

Same preconditions as #690: _shared true (locking=True, or BLOSC_LOCKING with mmap_mode is None, plus a real urlpath), and reads genuinely concurrent with writes from another process. Additionally this one needs the concurrent write to be an overwrite or delete of the same key, not just an append of new keys — which makes it rarer than #690 but not unreachable. TreeStore inherits it.