Skip to content

fix(dpmodel): release merged LMDB cache references#5797

Open
njzjz wants to merge 2 commits into
deepmodeling:masterfrom
njzjz:codex/code-scan-5635
Open

fix(dpmodel): release merged LMDB cache references#5797
njzjz wants to merge 2 commits into
deepmodeling:masterfrom
njzjz:codex/code-scan-5635

Conversation

@njzjz

@njzjz njzjz commented Jul 14, 2026

Copy link
Copy Markdown
Member

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.

Balance each source environment acquired by merge_lmdb through the refcount-aware cache close path, including exceptions. Add a regression proving merge preserves shared readers and leaves the cache reference count unchanged.

Coding-Agent: Codex
Codex-Version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
Copilot AI review requested due to automatic review settings July 14, 2026 19:31
@dosubot dosubot Bot added the bug label Jul 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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

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

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

Comment thread source/tests/pt/test_lmdb_dataloader.py Fixed
Comment thread source/tests/pt/test_lmdb_dataloader.py Fixed
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

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.

Release the retained readers with del so finalizers run without assignments that CodeQL flags as unused.

Coding-Agent: Codex
Codex-Version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants