Found during a Codex global scan of deepmodeling/deepmd-kit at commit 73de44b1f94471b2e3bdb6b11f57b34d7bc791bb.
Problem
merge_lmdb() opens source environments through the shared LMDB cache but closes them directly, bypassing the cache refcount.
Evidence:
_open_lmdb() stores environments in _ENV_CACHE and increments a refcount for reused paths:
|
# Process-level cache: python-lmdb does not allow opening the same path twice |
|
# in one process. We ref-count so the Environment is closed (and freed from |
|
# the cache) once every reader that shares it is garbage-collected. |
|
_ENV_CACHE: dict[str, tuple[lmdb.Environment, int]] = {} |
|
|
|
|
|
def _open_lmdb(path: str) -> lmdb.Environment: |
|
"""Open (or reuse) a readonly LMDB environment with reference counting. |
|
|
|
The python-lmdb binding raises ``lmdb.Error`` if the same path is opened |
|
more than once in a single process. We cache by resolved absolute path |
|
and bump a reference count. Call :func:`_close_lmdb` when done to |
|
decrement the count; when it reaches zero the environment is closed and |
|
removed from the cache. |
|
""" |
|
resolved = str(Path(path).resolve()) |
|
entry = _ENV_CACHE.get(resolved) |
|
if entry is not None: |
|
env, refcount = entry |
|
_ENV_CACHE[resolved] = (env, refcount + 1) |
|
return env |
|
env = lmdb.open(path, readonly=True, lock=False, readahead=False, meminit=False) |
|
_ENV_CACHE[resolved] = (env, 1) |
|
return env |
_close_lmdb() is the matching refcount-aware close path:
|
def _close_lmdb(path: str) -> None: |
|
"""Decrement the ref-count for *path* and close the env when it hits zero.""" |
|
resolved = str(Path(path).resolve()) |
|
entry = _ENV_CACHE.get(resolved) |
|
if entry is None: |
|
return |
|
env, refcount = entry |
|
if refcount <= 1: |
|
del _ENV_CACHE[resolved] |
|
try: |
|
env.close() |
|
except Exception: |
|
pass |
|
else: |
|
_ENV_CACHE[resolved] = (env, refcount - 1) |
merge_lmdb() calls _open_lmdb(src_path) for every source:
|
for src_path in src_paths: |
|
src_env = _open_lmdb(src_path) |
|
with src_env.begin() as txn: |
- It later calls
src_env.close() directly:
|
# Update sys_id_offset for next source |
|
if src_sys_ids is not None and len(src_sys_ids) > 0: |
|
sys_id_offset += max(int(s) for s in src_sys_ids) + 1 |
|
else: |
|
sys_id_offset += 1 |
|
|
|
src_env.close() |
Impact
The cache can retain a closed environment, and an active LmdbDataReader sharing the same cached environment can be invalidated by a merge. Later readers may reuse the closed handle instead of opening a fresh environment.
Suggested Fix
Replace the direct close with _close_lmdb(src_path) in a finally block. Add a regression test that opens a reader, calls merge_lmdb() on the same path, and then verifies the existing reader and a new reader can still read frames.
Found during a Codex global scan of
deepmodeling/deepmd-kitat commit73de44b1f94471b2e3bdb6b11f57b34d7bc791bb.Problem
merge_lmdb()opens source environments through the shared LMDB cache but closes them directly, bypassing the cache refcount.Evidence:
_open_lmdb()stores environments in_ENV_CACHEand increments a refcount for reused paths:deepmd-kit/deepmd/dpmodel/utils/lmdb_data.py
Lines 51 to 74 in 73de44b
_close_lmdb()is the matching refcount-aware close path:deepmd-kit/deepmd/dpmodel/utils/lmdb_data.py
Lines 77 to 91 in 73de44b
merge_lmdb()calls_open_lmdb(src_path)for every source:deepmd-kit/deepmd/dpmodel/utils/lmdb_data.py
Lines 1767 to 1769 in 73de44b
src_env.close()directly:deepmd-kit/deepmd/dpmodel/utils/lmdb_data.py
Lines 1816 to 1822 in 73de44b
Impact
The cache can retain a closed environment, and an active
LmdbDataReadersharing the same cached environment can be invalidated by a merge. Later readers may reuse the closed handle instead of opening a fresh environment.Suggested Fix
Replace the direct close with
_close_lmdb(src_path)in afinallyblock. Add a regression test that opens a reader, callsmerge_lmdb()on the same path, and then verifies the existing reader and a new reader can still read frames.