fix(dpmodel): release merged LMDB cache references

[njzjz]

Summary

• release every source environment acquired by merge_lmdb() through the refcount-aware LMDB cache path
• use finally so malformed metadata or copy failures also balance the merge's temporary reference
• add a regression that checks both shared-reader usability and exact cache refcount symmetry

Root cause and fix

_open_lmdb() caches readonly environments by resolved path because python-lmdb does not permit independently opening the same path multiple times in one process. Every acquisition increments a cache refcount, and _close_lmdb() is the matching release operation.

merge_lmdb() acquired each source through _open_lmdb(), but called src_env.close() directly. If a reader already shared that cached environment, the merge invalidated its live handle while the cache still retained it. A later reader could then receive the same closed object.

The source loop now pairs _open_lmdb(src_path) with _close_lmdb(src_path) in finally. This releases only the reference owned by the merge; existing readers retain their references and environment. The finally scope covers metadata parsing, source/destination transactions, frame copying, and system-ID offset updates.

Why existing tests missed this

Existing merge tests opened readers only after the merge and did not keep a source reader alive across the operation. They therefore never exercised the cache-sharing condition that makes a direct Environment.close() unsafe.

The new regression:

1. opens a source LmdbDataReader and records its cached environment/refcount;
2. merges that same source;
3. verifies the exact same environment remains cached with an unchanged refcount;
4. verifies the existing reader still reads;
5. opens a new reader, verifies the refcount increases by exactly one, and verifies it also reads.

On the previous implementation, the existing reader fails deterministically with:

lmdb.Error: Attempt to operate on closed/deleted/dropped object.

Validation performed:

• focused old behavior reproduction: deterministic closed-environment failure
• fixed regression: passed
• complete TestMergeLmdbSystemIds group: 4 passed
• duplicate-source and malformed-metadata refcount scenarios independently checked during review
• ruff format --check .
• ruff check .
• git diff --check

Closes #5635.

Coding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

• Bug Fixes

  • Improved LMDB merging reliability when source data is accessed by multiple readers.
  • Preserved frame metadata and system identifiers during merges, including fallback handling for incomplete metadata.
  • Ensured source data remains accessible after merging and is cleaned up safely afterward.

• Tests

  • Added regression coverage for shared LMDB readers and cache lifecycle behavior.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 33e09580-15cd-4fdc-9887-8ab94bb2cc23

📥 Commits

Reviewing files that changed from the base of the PR and between 3122138 and 5fb2ce7.

📒 Files selected for processing (2)

• deepmd/dpmodel/utils/lmdb_data.py
• source/tests/pt/test_lmdb_dataloader.py

---

📝 Walkthrough

Walkthrough

merge_lmdb now closes source environments through the shared reference-counted cache. A regression test verifies existing and newly created readers remain usable after merging.

Changes

LMDB cache lifecycle

Layer / File(s)	Summary
Cache-aware merge cleanup and regression coverage deepmd/dpmodel/utils/lmdb_data.py, source/tests/pt/test_lmdb_dataloader.py	merge_lmdb releases each source environment with _close_lmdb in finally, and tests validate cache reference counts, environment identity, and reader frame access.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• deepmodeling/deepmd-kit#5283: Introduces the LMDB utility containing merge_lmdb, which this PR updates.

Suggested reviewers: iprozd, outisli

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately summarizes the main change: releasing merged LMDB cache references.
Linked Issues check	✅ Passed	The changes replace direct LMDB closes with cache-aware cleanup in finally and add the requested regression coverage.
Out of Scope Changes check	✅ Passed	The additional metadata and system-id handling stays within merge_lmdb's core copy and merge behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.31%. Comparing base (7d5ad38) to head (5fb2ce7).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
deepmd/dpmodel/utils/lmdb_data.py	94.44%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5797      +/-   ##
==========================================
- Coverage   79.85%   78.31%   -1.54%     
==========================================
  Files        1022     1049      +27     
  Lines      117351   120651    +3300     
  Branches     4313     4351      +38     
==========================================
+ Hits        93706    94490     +784     
- Misses      22101    24598    +2497     
- Partials     1544     1563      +19     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[njzjz]

Possible reviewers based on changed lines, exact file history, and exact-file review history:

• @wanghan-iapcm — 1 commits on changed files; 4 reviews on exact changed files (deepmd/dpmodel/utils/lmdb_data.py, source/tests/pt/test_lmdb_dataloader.py).
• @iProzd — 3 commits on changed files (deepmd/dpmodel/utils/lmdb_data.py, source/tests/pt/test_lmdb_dataloader.py).

No review request was made automatically.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

source/tests/pt/test_lmdb_dataloader.py:1112

• The test cleanup relies on del ... + gc.collect() to trigger __del__ finalizers, which is nondeterministic and can be flaky across Python implementations. Since LmdbDataReader exposes an idempotent .close(), prefer explicitly closing readers in finally to release the LMDB cache refs deterministically (you can keep gc.collect() as a belt-and-suspenders if desired).

        finally:
            # Drop both strong references so their finalizers release the
            # cache entries before the cleanup assertion below.
            del existing_reader, new_reader
            gc.collect()

[njzjz-bot]

Three independent full reviews were completed against this exact head. No concrete actionable findings were identified, so no artificial inline comment was added. The effective diff is a focused regression test that verifies merge-time LMDB cache leases remain balanced while active and newly created readers keep sharing the same valid environment. The PR currently has merge conflicts, which must be resolved separately.

The Codex quota is close to resetting, so I am concentrating the remaining token budget on these reviews.

Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh