Skip to content

fix(dfm-search): acquire commit.lock on reader to prevent heap corruption from concurrent writer commit - #392

Merged
deepin-bot[bot] merged 2 commits into
linuxdeepin:develop/meagle-20260526from
liyigang1:v20/20260526
Sep 9, 2026
Merged

deepin-bot[bot] merged 2 commits into
linuxdeepin:develop/meagle-20260526from
liyigang1:v20/20260526

Conversation

@liyigang1

@liyigang1 liyigang1 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

IndexReader::open() reads segments_N and segment file metadata. When a separate process's IndexWriter is mid-commit (rename segments_N + replace postings files), a reader open may observe torn segment data. The corruption isn't detected until boost::make_shared<vector<shared_ptr>> runs inside weight->scorer() during search, where malloc metadata has already been overwritten — manifesting as 'unsorted double linked list corrupted' and SIGABRT.

Lucene++ IndexWriter already acquires commit.lock exclusively during commit() (via NativeFSLockFactory / fcntl). IndexReader::open(directory, readOnly=true) does not acquire any lock — this is a Lucene design assumption that fails for cross-process scenarios.

Fix: reader-side manually acquires commit.lock during IndexReader::open() (and reopen()) so the open call is atomic with respect to writer commits. Lock is released immediately after open returns; subsequent search runs lock-free since segment file descriptors opened during open are stable on Linux (writer commits create new inodes, leaving old fds intact).

Also adds a reader cache (isCurrent() check, reopen() on stale) to avoid re-opening the index on every search. Cache is guarded by QMutex; fast path returns the cached reader without re-acquiring commit.lock since open() is the only operation that observes torn segment state.

Lock acquisition timeout: 1000ms per try, 3 attempts, total up to 3s. On failure the search round is skipped with a qWarning; the next search attempt will retry. fd-level fcntl locks auto-release on process death, no stale lock files possible.

Task: https://pms.uniontech.com/task-view-395349.html
Log: 修复 dfm-search 跨进程读取 fulltext 索引时与 textindex 写入 commit() 未互斥导致的段文件不一致读取,进而触发 weight->scorer 内部
boost::make_shared 时 malloc 元数据损坏的崩溃。
Log: Synchronize reader IndexReader::open with writer IndexWriter::commit via Lucene++ commit.lock to fix heap corruption from torn segment reads.

Summary by Sourcery

Protect content and OCR text index readers from concurrent writer commits while reusing stable readers between searches.

Bug Fixes:

  • Synchronize reader initialization and refresh with writer commits to prevent torn index reads and resulting heap corruption during content and OCR text searches.

Enhancements:

  • Cache index directories and readers, refreshing them only when the index commit changes while coordinating concurrent access.

Chores:

  • Add reusable RAII handling for acquiring and releasing Lucene commit locks with bounded retries and graceful search skipping when unavailable.

@sourcery-ai

sourcery-ai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Reviewer's Guide

Prevents cross-process torn index reads by making reader open/reopen atomic with IndexWriter commits, while adding mutex-protected reader caching to reduce repeated opens and preserve lock-free searches after initialization.

Sequence diagram for synchronized reader initialization during index commit

sequenceDiagram
    participant Search as ContentIndexedStrategy
    participant Cache as ReaderCache
    participant Lock as commit.lock
    participant Writer as IndexWriter
    participant Reader as IndexReader
    participant Searcher as IndexSearcher

    Search->>Cache: getOrCreateReader(directory)
    Cache->>Cache: isCurrent()
    alt cached reader is current
        Cache-->>Search: cached reader
    else reader missing or stale
        Cache->>Lock: obtain(1000ms)
        Lock-->>Cache: acquired
        Writer-->>Lock: commit() waits for lock
        alt cached reader is stale
            Cache->>Reader: reopen(true)
        else no cached reader
            Cache->>Reader: open(directory, true)
        end
        Reader-->>Cache: stable reader
        Cache->>Lock: release()
        Cache-->>Search: cached reader
    end
    Search->>Searcher: search(reader)
    Note over Searcher,Writer: Search uses stable reader file descriptors without holding commit.lock
Loading

File-Level Changes

Change Details Files
Synchronize reader initialization with writer commits using the shared native commit lock.
  • Configure the directory with NativeFSLockFactory so reader and writer use the same fcntl-backed lock.
  • Add an RAII lock guard that retries acquisition up to three times with a one-second timeout and releases automatically.
  • Skip the search round when the lock cannot be acquired or reader initialization fails.
src/dfm-search/dfm-search-lib/contentsearch/contentstrategies/indexedstrategy.cpp
src/dfm-search/dfm-search-lib/utils/lucenecommitlockguard.cpp
src/dfm-search/dfm-search-lib/utils/lucenecommitlockguard.h
Cache and safely refresh the IndexReader instead of reopening it for every search.
  • Guard cached-reader access with QMutex.
  • Use isCurrent() as a lock-free fast path and reopen or recreate the reader only when the index version is stale.
  • Hold commit.lock across reopen/open, then retain the reader for lock-free searches using stable segment file descriptors.
src/dfm-search/dfm-search-lib/contentsearch/contentstrategies/indexedstrategy.cpp
src/dfm-search/dfm-search-lib/contentsearch/contentstrategies/indexedstrategy.h

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/dfm-search/dfm-search-lib/utils/lucenecommitlockguard.cpp" line_range="31-35" />
<code_context>
+    }
+}
+
+LuceneCommitLockGuard::~LuceneCommitLockGuard()
+{
+    if (m_acquired && m_lock) {
+        m_lock->release();
+        m_acquired = false;
+    }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** LuceneCommitLockGuard::~LuceneCommitLockGuard() calls the fallible Lock::release() without handling exceptions; if releasing commit.lock fails, the exception escapes a destructor during stack unwinding and terminates the search process via std::terminate.

**Triggers:** When the lock file descriptor or underlying filesystem is already invalid during guard destruction.

**Suggested fix:** Catch and log exceptions from release() inside the destructor, ensuring the destructor does not throw.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread src/dfm-search/dfm-search-lib/utils/lucenecommitlockguard.cpp
…tion from concurrent writer commit

IndexReader::open() reads segments_N and segment file metadata. When a
separate process's IndexWriter is mid-commit (rename segments_N + replace
postings files), a reader open may observe torn segment data. The corruption
isn't detected until boost::make_shared<vector<shared_ptr<Scorer>>> runs
inside weight->scorer() during search, where malloc metadata has already been
overwritten — manifesting as 'unsorted double linked list corrupted' and
SIGABRT.

Lucene++ IndexWriter already acquires commit.lock exclusively during commit()
(via NativeFSLockFactory / fcntl). IndexReader::open(directory, readOnly=true)
does not acquire any lock — this is a Lucene design assumption that fails for
cross-process scenarios.

Fix: reader-side manually acquires commit.lock during IndexReader::open()
(and reopen()) so the open call is atomic with respect to writer commits.
Lock is released immediately after open returns; subsequent search runs
lock-free since segment file descriptors opened during open are stable on
Linux (writer commits create new inodes, leaving old fds intact).

Also adds a reader cache (isCurrent() check, reopen() on stale) to avoid
re-opening the index on every search. Cache is guarded by QMutex; fast path
returns the cached reader without re-acquiring commit.lock since open() is
the only operation that observes torn segment state. Slow path releases the
mutex during commit.lock acquisition and I/O to avoid blocking concurrent
search threads, with double-check on re-acquire.

Both content search and OCR text search strategies use NativeFSLockFactory for
FSDirectory and share the same getOrCreateReader() pattern. FSDirectory objects
are cached per-strategy to avoid repeated construction.

Lock acquisition timeout: 1000ms per try, 3 attempts, total up to 3s.
On failure the search round is skipped with a qWarning; the next search
attempt will retry. fd-level fcntl locks auto-release on process death,
no stale lock files possible.

Task: https://pms.uniontech.com/task-view-395349.html
Log: 修复 dfm-search 跨进程读取 fulltext 索引时与 textindex 写入 commit()
未互斥导致的段文件不一致读取,进而触发 weight->scorer 内部
boost::make_shared 时 malloc 元数据损坏的崩溃。
Log: Synchronize reader IndexReader::open with writer IndexWriter::commit
via Lucene++ commit.lock to fix heap corruption from torn segment reads.
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

🤖 AI 代码审查报告

总体评分: 93 分 (通过阈值: 70分)

Pass


📊 总体评价

项目 结果
审查结论 代码审查通过
评分详情 总体评分 93 分,大于 70 分通过阈值。本次提交修复了 dfm-search 跨进程索引读写并发导致的堆损坏崩溃问题,方案设计合理,异常处理完善,无安全漏洞。

🔍 详细分析

1. 语法逻辑 ✅

评价: 优秀 ✅ 通过

潜在问题:

  1. src/dfm-search/dfm-search-lib/contentsearch/contentstrategies/indexedstrategy.cpp:572 - getOrCreateReader() 双重检查中 catch(...) 静默吞掉异常未记录日志,可能掩盖 isCurrent() 失败的根因

建议: 在 catch(...) 块中添加 qWarning() 日志记录,便于后续排查 isCurrent() 失败的根因


2. 代码质量 ✅

评价: 良好 ✅ 通过

潜在问题:

  1. src/dfm-search/dfm-search-lib/ocrtextsearch/ocrtextstrategies/indexedstrategy.cpp:523 - getOrCreateReader() 函数与 contentsearch 版本完全重复(约80行),违反 DRY 原则

建议: 将 getOrCreateReader() 抽取为共享工具函数或模板方法,避免两处策略类中的代码重复。可考虑提取到 utils 目录下的 ReaderCache 工具类中


3. 代码性能 ✅

评价: 优秀 ✅ 通过

潜在问题:

  1. src/dfm-search/dfm-search-lib/contentsearch/contentstrategies/indexedstrategy.cpp:536 - 快速路径使用 QMutex 互斥锁,高并发场景下可能成为瓶颈,建议考虑读写锁

建议: 可考虑使用读写锁(QReadWriteLock)替代 QMutex,使快速路径的 isCurrent() 检查使用读锁,减少线程间竞争


4. 代码安全 🔒

评价: 优秀 ✅ 通过

🔐 发现 0 个安全漏洞

安全漏洞详情:
✅ 未发现安全漏洞

建议: 无安全漏洞。代码不涉及用户输入处理,无注入风险。锁获取失败时优雅降级(返回 nullptr),不会导致崩溃。异常处理全面,防止错误传播。


💡 改进建议代码示例

// 建议1: 为 catch(...) 添加日志记录
// 文件: indexedstrategy.cpp getOrCreateReader() 双重检查部分

    if (cachedSnapshot) {
        try {
            if (cachedSnapshot->isCurrent()) {
                return cachedSnapshot;
            }
        } catch (const Lucene::LuceneException &e) {
            qWarning() << "Double-check isCurrent() failed:"
                       << QString::fromStdWString(e.getError());
        } catch (const std::exception &e) {
            qWarning() << "Double-check isCurrent() std exception:" << e.what();
        } catch (...) {
            qWarning() << "Double-check isCurrent() unknown exception";
        }
    }

// 建议2: 使用 QReadWriteLock 替代 QMutex
// 文件: indexedstrategy.h

    QReadWriteLock m_readerMutex;  // 替代 QMutex

// getOrCreateReader() 快速路径使用读锁:
    {
        QReadLocker locker(&m_readerMutex);
        if (m_cachedReader && m_cachedReader->isCurrent()) {
            return m_cachedReader;
        }
    }
// 慢路径写缓存时使用写锁:
    {
        QWriteLocker locker(&m_readerMutex);
        m_cachedReader = result;
    }

本报告由 AI 代码审查工具自动生成

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: liyigang1, max-lvs

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liyigang1

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

This pr force merged! (status: unstable)

@deepin-bot
deepin-bot Bot merged commit ed93326 into linuxdeepin:develop/meagle-20260526 Sep 9, 2026
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants