test: suppress QDBus watcher QThread leak in lsan - #7589
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change documents a lazily spawned, unjoined watcher thread created by Estimated code review effort: 1 (Trivial) | ~2 minutes Mergeability Score: 🔵 Low · up to This change suppresses the QDBus watcher leak but may also hide unrelated application-created QThread leaks in the Qt test binary. The PR is mergeable with explicit owner awareness or follow-up to narrow the suppression. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit e30887c) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b77f36fd64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # QDBusConnectionManager lazily spawns a watcher QThread that is never joined | ||
| # before process exit; the manager itself is suppressed above but the thread | ||
| # object's allocation carries only the QThread constructor frame. | ||
| leak:QThread::QThread |
There was a problem hiding this comment.
In the linux64_asan Qt test, test_dash-qt exercises application shutdown while constructing product-owned threads such as ClientModel::m_thread and ClientFeeds::m_thread. LSan matches this template against every allocation stack, so leak:QThread::QThread also suppresses constructor-owned allocations for all those application QThreads; a future shutdown leak in that state would therefore pass the sanitizer job. Avoid the constructor-wide rule and prevent or isolate the QDBus watcher initialization instead.
AGENTS.md reference: AGENTS.md:L178-L180
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'd suggest to do bitcoin#35937 instead of QThread - seems as leaks are forever-friends for Qt Apps
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The added suppression should silence the dependency-owned QDBus watcher leak and unblock the sanitizer job, but its constructor-level match also reduces LeakSanitizer coverage for application-owned QThreads created by the Qt tests. The suppression should be isolated to the QDBus initialization path rather than matching every allocation stack containing QThread::QThread.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/sanitizer_suppressions/lsan`:
- [SUGGESTION] test/sanitizer_suppressions/lsan:9: Scope the suppression to the QDBus watcher
LeakSanitizer suppression templates match any allocation stack containing the specified symbol, so this rule also suppresses leaked constructor-owned allocations for application QThreads. The Qt tests instantiate `ClientModel`, which constructs its own `QThread` and a `ClientFeeds` instance with another `QThread`; other application paths construct additional threads. A future application leak whose allocation stack contains `QThread::QThread` would therefore be hidden by the sanitizer job. Prevent or isolate QDBus watcher initialization during the test, or use another mechanism whose identity is specific to the dependency allocation, instead of suppressing the shared constructor symbol.
| # QDBusConnectionManager lazily spawns a watcher QThread that is never joined | ||
| # before process exit; the manager itself is suppressed above but the thread | ||
| # object's allocation carries only the QThread constructor frame. | ||
| leak:QThread::QThread |
There was a problem hiding this comment.
🟡 Suggestion: Scope the suppression to the QDBus watcher
LeakSanitizer suppression templates match any allocation stack containing the specified symbol, so this rule also suppresses leaked constructor-owned allocations for application QThreads. The Qt tests instantiate ClientModel, which constructs its own QThread and a ClientFeeds instance with another QThread; other application paths construct additional threads. A future application leak whose allocation stack contains QThread::QThread would therefore be hidden by the sanitizer job. Prevent or isolate QDBus watcher initialization during the test, or use another mechanism whose identity is specific to the dependency allocation, instead of suppressing the shared constructor symbol.
source: ['codex']
The extended Qt suppressions from dashpay#7576 cover QDBusConnectionPrivate, QDBusConnectionManager and QLayoutPrivate, but the connection manager also lazily spawns a watcher QThread whose 120-byte allocation reports with only the QThread constructor frame, still failing test_dash-qt under asan on develop.
b77f36f to
e30887c
Compare
3 participants
Issue being fixed or feature implemented
The asan CI job's
test_dash-qtstill fails on develop after #7576's extended Qt suppressions: QDBusConnectionManager lazily spawns a watcher QThread whose 120-byte allocation reports with only theQThread::QThreadconstructor frame (the QDBus caller frames don't symbolize), so it escapes the existingQDBusConnectionManagertemplate. Seen on develop's own CI and on every PR based on it.Split out of #7586 per review feedback so the build fix there isn't held up by the suppression discussion.
What was done?
Added
leak:QThread::QThreadtotest/sanitizer_suppressions/lsanwith a comment tying it to the QDBus watcher.Known limitation, flagged during #7586 review and left open for discussion here: LSan suppression templates match any allocation stack containing the named symbol, so this also masks leaks of application-constructed QThreads (e.g.
src/qt/intro.cpp) in the Qt test binary. The stack cannot be scoped tighter by symbol because the QDBus frames are unsymbolized. Alternatives raised by reviewers: adopt the upstream approach from bitcoin#35937 instead of a symbol suppression. Happy to switch this PR to that shape if preferred — opening with the minimal one-liner so the pipeline unblocks while that's decided.How Has This Been Tested?
The failing configuration is the
linux64_asanCI job'stest_dash-qtrun; this PR's CI is the authoritative check. The suppression file is loaded via the commonLSAN_OPTIONS.Breaking Changes
None.
Checklist: