deepmodeling / deepmodeling/deepmd-kit

[Code scan] Close merge_lmdb source environments through the LMDB cache

Open
#5,635 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Python
Stars
2k
Forks
649
Avg merge
6d 18h
Merged PRs (30d)
15

Description

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: https://github.com/deepmodeling/deepmd-kit/blob/73de44b1f94471b2e3bdb6b11f57b34d7bc791bb/deepmd/dpmodel/utils/lmdb_data.py#L51-L74
- `_close_lmdb()` is the matching refcount-aware close path: https://github.com/deepmodeling/deepmd-kit/blob/73de44b1f94471b2e3bdb6b11f57b34d7bc791bb/deepmd/dpmodel/utils/lmdb_data.py#L77-L91
- `merge_lmdb()` calls `_open_lmdb(src_path)` for every source: https://github.com/deepmodeling/deepmd-kit/blob/73de44b1f94471b2e3bdb6b11f57b34d7bc791bb/deepmd/dpmodel/utils/lmdb_data.py#L1767-L1769
- It later calls `src_env.close()` directly: https://github.com/deepmodeling/deepmd-kit/blob/73de44b1f94471b2e3bdb6b11f57b34d7bc791bb/deepmd/dpmodel/utils/lmdb_data.py#L1816-L1822

## 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.