fix(dfm-search): acquire commit.lock on reader to prevent heap corruption from concurrent writer commit - #392
Conversation
Reviewer's GuidePrevents 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 commitsequenceDiagram
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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
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>a226e81 to
e1f7d1d
Compare
…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.
405ff17 to
4ff1ff2
Compare
deepin pr auto review🤖 AI 代码审查报告📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价: 优秀 ✅ 通过 潜在问题:
建议: 在 catch(...) 块中添加 qWarning() 日志记录,便于后续排查 isCurrent() 失败的根因 2. 代码质量 ✅评价: 良好 ✅ 通过 潜在问题:
建议: 将 getOrCreateReader() 抽取为共享工具函数或模板方法,避免两处策略类中的代码重复。可考虑提取到 utils 目录下的 ReaderCache 工具类中 3. 代码性能 ✅评价: 优秀 ✅ 通过 潜在问题:
建议: 可考虑使用读写锁(QReadWriteLock)替代 QMutex,使快速路径的 isCurrent() 检查使用读锁,减少线程间竞争 4. 代码安全 🔒评价: 优秀 ✅ 通过
安全漏洞详情: 建议: 无安全漏洞。代码不涉及用户输入处理,无注入风险。锁获取失败时优雅降级(返回 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 代码审查工具自动生成 |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
ed93326
into
linuxdeepin:develop/meagle-20260526
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:
Enhancements:
Chores: