test: suppress Qt executable leaks 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 aa6eaa8) |
![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']
There was a problem hiding this comment.
Resolved in this update — Scope the suppression to the QDBus watcher no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
b77f36f to
e30887c
Compare
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.
e30887c to
a325fdb
Compare
Adapt the approach from bitcoin#35937 so Qt's process-global leaks do not require broad shared-symbol suppressions. Hide the suppression summary in both CI entry points that load the LSan rules.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head aa6eaa8, the executable-scoped LeakSanitizer rules match the generated Qt binary names, both CI entry points use valid LSAN_OPTIONS syntax, and the stated shell and lint checks pass. The prior constructor-wide QThread suppression concern is fixed because that rule was removed and replaced with the process-level approach requested in the earlier review; no actionable findings remain.
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)
4 participants
Issue being fixed or feature implemented
Qt keeps process-global allocations alive through process exit, including the QDBus watcher observed in the
linux64_asanQt-test job. Symbol-level suppressions are brittle: the remaining watcher allocation only exposesQThread::QThread, while suppressing that shared constructor would also hide leaks from application-owned threads.What was done?
Adapted the approach from bitcoin#35937:
test_dash-qtanddash-qt.print_suppressions=0toLSAN_OPTIONSin both CI entry points that load the suppression file, avoiding suppression summaries on stderr.This keeps the Qt workaround out of non-Qt binaries without relying on shared Qt constructor symbols.
How Has This Been Tested?
bash -n ci/dash/matrix.sh ci/test/04_install.shtest/lint/lint-shell.pytest/lint/lint-whitespace.pygit diff --checkThe
linux64_asanCI job remains the authoritative behavioral check fortest_dash-qt.Breaking Changes
None.
Checklist: