test: narrow Qt and BDB LSan suppressions - #7560
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change configures LeakSanitizer for DWARF-based malloc unwinding and adds Qt DBus and Berkeley DB suppressions. The test framework parses and validates LeakSanitizer suppression summaries. Node and wallet tests check expected suppression data. Framework unit tests cover valid, absent, and malformed summaries. Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant LSAN
participant TestNode
participant StartupAssertion
LSAN->>TestNode: write suppression summary to stderr
TestNode->>TestNode: parse trailing summary
TestNode->>StartupAssertion: compare expected suppression data
StartupAssertion-->>TestNode: report mismatch before message matching
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit bd4ab64) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR adds a single line, leak:QDBusConnectionManager, to test/sanitizer_suppressions/lsan, following the exact pattern of the three existing Qt/DBus suppressions already in the file. It fixes a documented LSan false-positive in the linux64_asan-build CI job's qt/test/test_dash-qt step, carries no consensus or runtime risk, and both agent lanes as well as CodeRabbit found zero actionable issues. Source: reviewer backend gpt-5.6-sol (codex, general + dash-core-commit-history roles) and claude-sonnet-5 (dash-core-commit-history + general roles); final verifier backend claude-sonnet-5 (final-verifier role). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
d005540 to
7ddfc60
Compare
PastaPastaPasta
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/bdb.cpp`:
- Around line 94-100: Update the failed and exception paths in Open to call
UnlockDirectory for the wallet directory whenever this environment successfully
acquired the lock, including the existing !fDbEnvInit cleanup path. Ensure
cleanup occurs before returning or propagating the exception, while avoiding
unlock attempts when this environment did not acquire the lock.
- Around line 94-100: Update the initialized-close branch in the wallet
environment Close() method to reset dbenv immediately after dbenv->close(0),
preventing reuse of the closed handle and repeated close calls. Add regression
coverage verifying both consecutive Close(); Close(); and Close(); Open();
sequences.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7701d56-906e-4dde-96ee-57a5f9029d81
📒 Files selected for processing (2)
src/wallet/bdb.cpptest/sanitizer_suppressions/lsan
🚧 Files skipped from review as they are similar to previous changes (1)
- test/sanitizer_suppressions/lsan
| if (!fDbEnvInit) { | ||
| if (dbenv) { | ||
| dbenv->close(0); | ||
| dbenv.reset(); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -i 'fs_helpers\.(cpp|h)$' | xargs -r rg -n -C 8 '\b(LockDirectory|UnlockDirectory|dir_locks)\b'
rg -n -C 8 '\b(LockDirectory|UnlockDirectory|fDbEnvInit|dbenv->close|dbenv\.reset)\b' src/wallet/bdb.cppRepository: dashpay/dash
Length of output: 10677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- BerkeleyEnvironment declarations and lifecycle ---'
fd -t f -i 'bdb\.(cpp|h)$' | xargs -r ast-grep outline
rg -n -C 12 '\b(BerkeleyEnvironment::(Open|Close|Reset|~BerkeleyEnvironment)|LockDirectory|UnlockDirectory|g_dbenvs|make_unique<BerkeleyEnvironment>|BerkeleyEnvironment)' src/wallet src | head -n 500
printf '%s\n' '--- Open and Close implementation ---'
sed -n '80,220p' src/wallet/bdb.cpp
printf '%s\n' '--- Build/test references ---'
rg -n -C 5 '\b(bdb|BerkeleyEnvironment|walletlock|LockDirectory|UnlockDirectory)\b' src/wallet src/test src/Makefile* 2>/dev/null | head -n 500Repository: dashpay/dash
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Lock helper tests and ownership behavior ---'
sed -n '1110,1175p' src/test/util_tests.cpp
sed -n '45,95p' src/util/fs_helpers.cpp
printf '%s\n' '--- All Open call sites and failure handling ---'
rg -n -C 5 '\->Open\(|\.Open\(' src/wallet src/test | head -n 300
printf '%s\n' '--- Read-only control-flow check for lock cleanup ---'
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/wallet/bdb.cpp").read_text()
open_body = source[source.index("bool BerkeleyEnvironment::Open"):source.index("//! Construct an in-memory mock")]
lock_pos = open_body.index("LockDirectory")
tail = open_body[lock_pos:]
print("UnlockDirectory calls after LockDirectory in Open:",
len(re.findall(r"\bUnlockDirectory\s*\(", tail)))
print("Failure returns after LockDirectory in Open:",
len(re.findall(r"\breturn\s+false\s*;", tail)))
for match in re.finditer(r"\breturn\s+false\s*;", tail):
start = max(0, match.start() - 180)
print("---")
print(tail[start:match.end()].strip())
PYRepository: dashpay/dash
Length of output: 10243
Release .walletlock on every failed Open.
Open acquires .walletlock before dbenv->open(), but the failed-open path returns without calling UnlockDirectory. The lock remains in dir_locks and can block another process from loading the wallet. Release the lock on every partial or failed Open, including exception paths, only when this environment acquired it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/bdb.cpp` around lines 94 - 100, Update the failed and exception
paths in Open to call UnlockDirectory for the wallet directory whenever this
environment successfully acquired the lock, including the existing !fDbEnvInit
cleanup path. Ensure cleanup occurs before returning or propagating the
exception, while avoiding unlock attempts when this environment did not acquire
the lock.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 '\b(void BerkeleyEnvironment::(Close|Open)|fDbEnvInit|dbenv->close|dbenv\.reset)\b' src/wallet/bdb.cpp
rg -n -C 5 'BerkeleyEnvironment|ReloadDbEnv|MakeMock' src test --glob '*.{cpp,h}'Repository: dashpay/dash
Length of output: 38009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,475p' src/wallet/bdb.cpp
sed -n '1,130p' src/wallet/test/db_tests.cpp
rg -n -C 8 'ReloadDbEnv|\.Open\(|\.Close\(|IsInitialized|BerkeleyEnvironment\(' src/wallet/test src/test --glob '*.{cpp,h}'
sed -n '1,90p' src/wallet/bdb.hRepository: dashpay/dash
Length of output: 11555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("src/wallet/bdb.cpp").read_text()
close = re.search(r"void BerkeleyEnvironment::Close\(\)\n\{(?P<body>.*?)\n\}\n\nvoid BerkeleyEnvironment::Reset", source, re.S)
open_ = re.search(r"bool BerkeleyEnvironment::Open\(bilingual_str& err\)\n\{(?P<body>.*?)\n\}\n\n//! Construct", source, re.S)
reload = re.search(r"void BerkeleyEnvironment::ReloadDbEnv\(\)\n\{(?P<body>.*?)\n\}\n\nbool BerkeleyDatabase::Rewrite", source, re.S)
assert close and open_ and reload
close_body = close.group("body")
open_body = open_.group("body")
reload_body = reload.group("body")
# Current initialized-close path closes the handle without resetting it.
initialized_close = close_body.split("if (!fDbEnvInit)", 1)[1]
assert "int ret = dbenv->close(0);" in initialized_close
assert initialized_close.find("dbenv.reset()") == -1
# Open skips Reset() when dbenv is still non-null.
assert "if (!dbenv) {\n Reset();\n }" in open_body
# ReloadDbEnv closes, then resets, then opens.
assert reload_body.find("Flush(true);") < reload_body.find("Reset();") < reload_body.find("Open(open_err);")
# Model the relevant handle state. False means closed/null; True means open.
def close_transition(initialized, handle):
if not initialized:
return False # early branch resets the handle
return handle # current initialized branch leaves the closed handle non-null
def open_transition(initialized, handle):
if initialized:
return initialized, handle
if not handle:
handle = True # Reset() creates a fresh DbEnv
return True, handle
initialized, handle = True, True
initialized, handle = False, close_transition(initialized, handle)
assert handle is True, "initialized Close() currently leaves a stale non-null handle"
initialized, handle = open_transition(initialized, handle)
assert initialized and handle, "state model did not reach Open()"
print("PASS: source ordering and state model demonstrate that Close(); Open() reuses the stale non-null DbEnv handle.")
PYRepository: dashpay/dash
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
lines = Path("src/wallet/bdb.cpp").read_text().splitlines()
def line_index(text):
for i, line in enumerate(lines):
if text in line:
return i
raise AssertionError(f"missing: {text}")
close_start = line_index("void BerkeleyEnvironment::Close()")
reset_start = line_index("void BerkeleyEnvironment::Reset()")
open_start = line_index("bool BerkeleyEnvironment::Open(bilingual_str& err)")
reload_start = line_index("void BerkeleyEnvironment::ReloadDbEnv()")
close_lines = lines[close_start:reset_start]
initialized_lines = lines[line_index(" fDbEnvInit = false;"):reset_start]
open_lines = lines[open_start:reload_start]
reload_lines = lines[reload_start:line_index("bool BerkeleyDatabase::Rewrite")]
assert " dbenv.reset();" in close_lines, "early Close() branch must reset dbenv"
assert " int ret = dbenv->close(0);" in initialized_lines
assert " dbenv.reset();" not in initialized_lines, "initialized Close() branch currently lacks reset"
assert " if (!dbenv) {" in open_lines
assert " Reset();" in open_lines
assert reload_lines.index(" Flush(true); // This will flush and close the environment") < reload_lines.index(" Reset();") < reload_lines.index(" Open(open_err);")
# State model: (initialized, non-null handle). A closed handle is represented
# by initialized=False, handle=True.
def close_state(state, fixed=False):
initialized, handle = state
if not initialized:
return False, False # early branch closes and resets
handle = False if fixed else handle # initialized branch closes; fixed code resets
return False, handle
def open_state(state):
initialized, handle = state
if initialized:
return state
if not handle:
handle = True # Open() calls Reset() only when dbenv is null
return True, handle
current = close_state((True, True))
assert current == (False, True)
assert open_state(current) == (True, True)
assert close_state(current) == (False, False)
fixed = close_state((True, True), fixed=True)
assert fixed == (False, False)
assert open_state(fixed) == (True, True)
assert close_state(fixed) == (False, False)
print("PASS: the initialized Close() path leaves a stale non-null handle; Open() reuses it; resetting after close removes both stale-handle paths.")
PYRepository: dashpay/dash
Length of output: 290
Reset dbenv after closing it.
Close() leaves dbenv non-null after the initialized DbEnv::close(0) call. Open() can then reuse the closed handle, and a later Close() can call close() again.
Reset dbenv immediately after the initialized close. Add regression coverage for Close(); Close(); and Close(); Open();.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/bdb.cpp` around lines 94 - 100, Update the initialized-close
branch in the wallet environment Close() method to reset dbenv immediately after
dbenv->close(0), preventing reuse of the closed handle and repeated close calls.
Add regression coverage verifying both consecutive Close(); Close(); and
Close(); Open(); sequences.
There was a problem hiding this comment.
The initialized-close finding is valid and more severe than stale-handle reuse alone: DbEnv::close() clears its internal DB_ENV*, so the new !fDbEnvInit branch makes Close(); Close() crash when it invokes close() through the invalidated wrapper. Resetting dbenv after the initialized close fixes both repeated close and reopen. The failed-Open() .walletlock cleanup described in the same comment is pre-existing and is outside this PR's scope.
There was a problem hiding this comment.
@thepastaclaw, agreed. The initialized-close issue is more severe than stale-handle reuse. Close() must reset dbenv after the initialized DbEnv::close(0) call. This prevents both Close(); Close(); from calling through an invalid wrapper and Close(); Open(); from reusing it.
The failed-Open() .walletlock cleanup is pre-existing. It should not block this PR. Would you like me to open a follow-up GitHub issue for it?
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash PR: 7560
File: src/wallet/bdb.cpp:94-100
Timestamp: 2026-08-07T23:40:23.222Z
Learning: In `src/wallet/bdb.cpp`, `DbEnv::close()` clears the wrapper's internal `DB_ENV*`. `BerkeleyEnvironment::Close()` must reset `dbenv` after closing an initialized environment. Otherwise, a subsequent `Close()` can call through an invalidated `DbEnv` wrapper, and `Open()` can reuse it.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The unopened-handle cleanup addresses the reported leak, but it introduces a deterministic repeated-close crash after an initialized environment has already been closed. The generic Berkeley DB allocator suppression also hides unrelated BDB leaks, and the commit subject should acknowledge the production wallet lifecycle change.
Source: reviewer backend gpt-5.6-sol (general and dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `src/wallet/bdb.cpp`:
- [BLOCKING] src/wallet/bdb.cpp:95-97: Reset the initialized environment handle after closing it
The new uninitialized branch assumes every non-null `dbenv` is an unopened handle, but the initialized branch calls `DbEnv::close(0)` without resetting the wrapper. Berkeley DB clears the wrapper's internal `DB_ENV*` during the first close, so a subsequent `Close()` enters this new branch and calls `close()` through an invalidated handle; this deterministically crashes with Berkeley DB 4.8. Normal wallet lifecycle code can reach this sequence because `Flush(true)` calls `Close()`, while eventual environment destruction calls `Close()` again. Reset `dbenv` immediately after the initialized close. This also ensures the new `Open()` guard creates a fresh handle after `Close(); Open()`. Add regression coverage for both `Close(); Close();` and `Close(); Open()`.
In `test/sanitizer_suppressions/lsan`:
- [SUGGESTION] test/sanitizer_suppressions/lsan:6: Do not suppress Berkeley DB's generic allocator
`__os_malloc` is Berkeley DB's shared allocation wrapper, with call sites throughout its environment, database, logging, locking, and memory-pool code. LeakSanitizer matches any frame in an allocation stack, so this entry suppresses nearly every BDB-originating leak rather than only the four allocations reported by these tests. It also prevents the ASan job from verifying whether the accompanying `BerkeleyEnvironment::Close()` change actually fixes the target leak. Remove this suppression; if an unavoidable dependency leak remains after correcting the lifecycle, suppress a function specific to that allocation path.
| if (dbenv) { | ||
| dbenv->close(0); | ||
| dbenv.reset(); |
There was a problem hiding this comment.
🔴 Blocking: Reset the initialized environment handle after closing it
The new uninitialized branch assumes every non-null dbenv is an unopened handle, but the initialized branch calls DbEnv::close(0) without resetting the wrapper. Berkeley DB clears the wrapper's internal DB_ENV* during the first close, so a subsequent Close() enters this new branch and calls close() through an invalidated handle; this deterministically crashes with Berkeley DB 4.8. Normal wallet lifecycle code can reach this sequence because Flush(true) calls Close(), while eventual environment destruction calls Close() again. Reset dbenv immediately after the initialized close. This also ensures the new Open() guard creates a fresh handle after Close(); Open(). Add regression coverage for both Close(); Close(); and Close(); Open().
| if (dbenv) { | |
| dbenv->close(0); | |
| dbenv.reset(); | |
| int ret = dbenv->close(0); | |
| dbenv.reset(); | |
| if (ret != 0) | |
| LogPrintf("BerkeleyEnvironment::Close: Error %d closing database environment: %s\n", ret, DbEnv::strerror(ret)); |
source: ['coderabbit']
There was a problem hiding this comment.
Fixed in 1430201: Close() now resets dbenv after the initialized DbEnv::close(0), so repeated Close() is a no-op and Open() recreates the environment from a fresh handle. This was also the cause of the CI segfaults (DbEnv::close via ~BerkeleyEnvironment). db_tests and wallet_tests pass locally.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Reset the initialized environment handle after closing it 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.
| leak:QDBusConnectionPrivate | ||
| leak:QLayoutPrivate | ||
| leak:QDBusConnectionManager | ||
| leak:__os_malloc |
There was a problem hiding this comment.
🟡 Suggestion: Do not suppress Berkeley DB's generic allocator
__os_malloc is Berkeley DB's shared allocation wrapper, with call sites throughout its environment, database, logging, locking, and memory-pool code. LeakSanitizer matches any frame in an allocation stack, so this entry suppresses nearly every BDB-originating leak rather than only the four allocations reported by these tests. It also prevents the ASan job from verifying whether the accompanying BerkeleyEnvironment::Close() change actually fixes the target leak. Remove this suppression; if an unavoidable dependency leak remains after correcting the lifecycle, suppress a function specific to that allocation path.
source: ['codex']
There was a problem hiding this comment.
Agreed — leak:__os_malloc would mask nearly every BDB-origin leak. Removed in 1430201; the Close() lifecycle fix addresses the actual unopened-environment leak the suppression was papering over.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Do not suppress Berkeley DB's generic allocator 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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/bdb.cpp (1)
155-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve mock mode when reopening a Berkeley environment.
CreateMockWalletDatabase()constructs an in-memoryBerkeleyEnvironment.Close()dropsdbenv, andOpen()callsReset(), which clearsfMockDb. A reopened mock environment therefore uses the file-backed setup. Preserve the mock configuration across reset, or add a mock-specific recreation path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/bdb.cpp` around lines 155 - 158, Update the Open() and Reset() flow so reopening an environment created by CreateMockWalletDatabase() preserves fMockDb and recreates the in-memory BerkeleyEnvironment instead of switching to file-backed storage. Keep normal file-backed reopening behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/wallet/bdb.cpp`:
- Around line 155-158: Update the Open() and Reset() flow so reopening an
environment created by CreateMockWalletDatabase() preserves fMockDb and
recreates the in-memory BerkeleyEnvironment instead of switching to file-backed
storage. Keep normal file-backed reopening behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 349f810c-f17e-4ed0-b95e-583065900599
📒 Files selected for processing (2)
src/wallet/bdb.cpptest/sanitizer_suppressions/lsan
ce20bfd to
2498f38
Compare
PastaPastaPasta
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The production Berkeley DB lifecycle edits and broad allocator suppression from the prior revision have been removed, resolving all three prior findings. However, the ASan functional suite also runs the legacy-wallet variant of wallet_upgradetohd.py, whose equivalent rejected -usehd=0 startup still performs exact stderr matching without accepting the newly emitted __lock_open suppression summary.
Source: reviewer backend gpt-5.6-sol (general and dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/functional/wallet_upgradetohd.py`:
- [BLOCKING] test/functional/wallet_upgradetohd.py:85: Allow the BDB summary in the other HD startup test
The ASan job runs `wallet_upgradetohd.py --legacy-wallet`, which creates and upgrades a Berkeley DB wallet before invoking the same rejected `-usehd=0` startup path handled in `wallet_hd.py`. Process teardown can therefore append the suppressed `__lock_open` allocation summary after the expected startup error. This call still uses exact full-text matching without `expected_lsan_suppressions`, so the newly enabled suppression summary makes this later functional test fail. Declare the same expected row here; the descriptor-wallet variant remains compatible because the helper intentionally permits the summary to be absent.
2498f38 to
587108a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/tool_wallet.py`:
- Around line 51-53: Update the stderr parsing flow around
split_lsan_suppression_summary to pass stderr.rstrip('\n') for parsing, while
retaining the original stderr when no suppression summary is returned. Add a
regression test covering the accepted [('__lock_open', 1, 160)] summary followed
by two trailing newlines, ensuring the empty-stderr assertion still succeeds.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 122a4adc-f9f6-4e30-89e0-d8586fd067ac
📒 Files selected for processing (3)
test/functional/test_framework/test_node.pytest/functional/test_framework/util.pytest/functional/tool_wallet.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/test_framework/test_node.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The narrow dependency-frame suppressions are appropriate, but three deterministic Linux ASan functional-test failures remain: the shared parser rejects LSan's native summary boundaries, failing dash-wallet commands do not parse the permitted BDB summary, and wallet_upgradetohd.py omits the expected summary declaration.
Source: reviewer backends gpt-5.6-sol (general and dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 additional finding(s) omitted (not in diff).
🤖 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/functional/wallet_upgradetohd.py`:
- [BLOCKING] test/functional/wallet_upgradetohd.py:85: Allow the BDB summary in the other HD startup test
The ASan suite runs `wallet_upgradetohd.py --legacy-wallet`. This path creates and upgrades a Berkeley DB wallet, then starts the node with rejected `-usehd=0`; wallet loading opens the BDB environment before the HD compatibility check fails. Teardown can therefore append the suppressed `__lock_open / 1 allocation / 160 bytes` summary. Because this call does not opt into suppression parsing, `assert_start_raises_init_error()` compares the full error plus summary against the expected error and fails. Pass the same expected row used by `wallet_hd.py`; the descriptor-wallet run remains valid because the helper permits the summary to be absent.
In `test/functional/test_framework/util.py`:
- [BLOCKING] test/functional/test_framework/util.py:29-46: Accept LSan's native summary boundaries
LLVM's `PrintMatchedSuppressions()` emits the opening separator directly and terminates the final separator with exactly two newlines. This parser instead requires a newline before the opening separator and requires the final separator to be the absolute end of the string. Consequently, raw `dash-wallet` stderr containing only a native suppression summary is not recognized, and `assert_tool_output()` subsequently fails its empty-stderr assertion. Parse both summary-only and application-stderr-prefixed forms, accept either the native two-newline trailer or the intentionally stripped representation, and add a regression fixture using LLVM's exact native output.
In `test/functional/tool_wallet.py`:
- [BLOCKING] test/functional/tool_wallet.py:41-46: Allow the BDB summary on failing wallet-tool commands
Only successful dash-wallet invocations use the new suppression parser. Some expected-error commands in the legacy-wallet run open the BDB environment before returning their application error. For example, the missing-`-dumpfile` test constructs and verifies the existing `todump` database through `MakeDatabase()` before `DumpWallet()` rejects the missing argument. Process teardown can then append the permitted `__lock_open / 1 allocation / 160 bytes` summary, causing the direct `stderr.strip()` comparison to fail. Parse and validate the same exact optional row in this helper so malformed, different, and additional suppression rows still fail.
| def split_lsan_suppression_summary(stderr): | ||
| separator = '-' * 53 | ||
| marker = f'\n{separator}\nSuppressions used:\n count bytes template\n' | ||
| if marker not in stderr: | ||
| return stderr, None | ||
|
|
||
| application_stderr, summary = stderr.rsplit(marker, 1) | ||
| footer = f'\n{separator}' | ||
| if not summary.endswith(footer): | ||
| return stderr, None | ||
|
|
||
| suppressions = [] | ||
| for row in summary[:-len(footer)].splitlines(): | ||
| match = re.fullmatch(r'\s*(\d+)\s+(\d+)\s+(\S+)', row) | ||
| if match is None: | ||
| return stderr, None | ||
| suppressions.append((match[3], int(match[1]), int(match[2]))) | ||
| return application_stderr, suppressions |
There was a problem hiding this comment.
🔴 Blocking: Accept LSan's native summary boundaries
LLVM's PrintMatchedSuppressions() emits the opening separator directly and terminates the final separator with exactly two newlines. This parser instead requires a newline before the opening separator and requires the final separator to be the absolute end of the string. Consequently, raw dash-wallet stderr containing only a native suppression summary is not recognized, and assert_tool_output() subsequently fails its empty-stderr assertion. Parse both summary-only and application-stderr-prefixed forms, accept either the native two-newline trailer or the intentionally stripped representation, and add a regression fixture using LLVM's exact native output.
| def split_lsan_suppression_summary(stderr): | |
| separator = '-' * 53 | |
| marker = f'\n{separator}\nSuppressions used:\n count bytes template\n' | |
| if marker not in stderr: | |
| return stderr, None | |
| application_stderr, summary = stderr.rsplit(marker, 1) | |
| footer = f'\n{separator}' | |
| if not summary.endswith(footer): | |
| return stderr, None | |
| suppressions = [] | |
| for row in summary[:-len(footer)].splitlines(): | |
| match = re.fullmatch(r'\s*(\d+)\s+(\d+)\s+(\S+)', row) | |
| if match is None: | |
| return stderr, None | |
| suppressions.append((match[3], int(match[1]), int(match[2]))) | |
| return application_stderr, suppressions | |
| def split_lsan_suppression_summary(stderr): | |
| separator = '-' * 53 | |
| marker = f'{separator}\nSuppressions used:\n count bytes template\n' | |
| marker_index = stderr.rfind(marker) | |
| if marker_index == -1 or (marker_index > 0 and stderr[marker_index - 1] != '\n'): | |
| return stderr, None | |
| application_stderr = stderr[:marker_index] | |
| if application_stderr.endswith('\n'): | |
| application_stderr = application_stderr[:-1] | |
| summary = stderr[marker_index + len(marker):] | |
| footer = f'\n{separator}' | |
| if summary.endswith(footer + '\n\n'): | |
| summary = summary[:-2] | |
| elif not summary.endswith(footer): | |
| return stderr, None | |
| suppressions = [] | |
| for row in summary[:-len(footer)].splitlines(): | |
| match = re.fullmatch(r'\s*(\d+)\s+(\d+)\s+(\S+)', row) | |
| if match is None: | |
| return stderr, None | |
| suppressions.append((match[3], int(match[1]), int(match[2]))) | |
| return application_stderr, suppressions |
source: ['codex', 'coderabbit']
There was a problem hiding this comment.
Resolved in this update — Accept LSan's native summary boundaries 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.
587108a to
3c4c11e
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The shared parser now correctly handles native and stripped LeakSanitizer summary boundaries, resolving the prior parser failure. Two legacy-wallet error paths still compare stderr without opting into the new suppression-summary handling, so the Linux ASan functional suite can fail when Berkeley DB emits the permitted __lock_open summary.
Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
2 additional finding(s) omitted (not in diff).
🤖 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/functional/wallet_upgradetohd.py`:
- [BLOCKING] test/functional/wallet_upgradetohd.py:85: Allow the BDB summary in the other HD startup test
The ASan suite runs this test with `--legacy-wallet`. The test upgrades a Berkeley DB wallet to HD and then attempts the rejected `-usehd=0` startup. `CWallet::Create()` loads and verifies the database before performing the HD compatibility check at `wallet.cpp:3256-3260`, so Berkeley DB's environment is opened and teardown can emit the suppressed `__lock_open / 1 allocation / 160 bytes` summary. This call does not pass `expected_lsan_suppressions`, leaving the summary attached to stderr and causing the full-text comparison to fail. Declare the same exact row as `wallet_hd.py`; the descriptor run remains valid because the helper allows the summary to be absent.
In `test/functional/tool_wallet.py`:
- [BLOCKING] test/functional/tool_wallet.py:41-46: Allow the BDB summary on failing wallet-tool commands
Only successful `dash-wallet` invocations pass stderr through `split_lsan_suppression_summary()`. In the legacy-wallet run, expected-error commands can open Berkeley DB before reporting their application error. For example, the missing-`-dumpfile` case calls `MakeDatabase()` with verification enabled; `MakeBerkeleyDatabase()` invokes `BerkeleyDatabase::Verify()`, which opens the environment before `DumpWallet()` rejects the missing argument. Teardown can then append the permitted `__lock_open / 1 allocation / 160 bytes` summary, causing the raw `stderr.strip()` comparison to fail. Parse and validate the same exact optional row in this helper so different, additional, or malformed summaries still fail.
3c4c11e to
b61c7c9
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The strict LSan summary parser and the failing dash-wallet command path are correctly handled at the current head. One legacy-wallet startup assertion in wallet_upgradetohd.py still omits the expected Berkeley DB suppression row, so the Linux ASan functional suite can fail despite receiving the expected initialization error.
Source: reviewer backend gpt-5.6-sol (general and dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/functional/wallet_upgradetohd.py`:
- [BLOCKING] test/functional/wallet_upgradetohd.py:85: Allow the BDB summary in the other HD startup test
The ASan suite runs this test with `--legacy-wallet`. After the test upgrades the Berkeley DB wallet to HD, this rejected `-usehd=0` startup verifies and loads the database before reaching the compatibility check at `src/wallet/wallet.cpp:3256-3260`. Berkeley DB teardown can therefore append the suppressed `__lock_open / 1 allocation / 160 bytes` summary. Because this call does not pass `expected_lsan_suppressions`, `assert_start_raises_init_error()` leaves that summary in stderr and the full-text comparison fails. Declare the same exact suppression row used by `wallet_hd.py`; the descriptor-wallet run remains valid because the helper permits the summary to be absent.
b61c7c9 to
bd4ab64
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The narrow suppression rules and strict parser changes are coherent, and the parser unit test passes. However, the legacy-wallet variant of wallet_upgradetohd.py still performs exact stderr matching without accepting the newly suppressed Berkeley DB summary, leaving a Linux ASan functional-test blocker.
Source: reviewers gpt-5.6-sol (Codex general and dash-core-commit-history); final verifier gpt-5.6-sol (Codex). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 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/functional/wallet_upgradetohd.py`:
- [BLOCKING] test/functional/wallet_upgradetohd.py:85: Allow the BDB summary in the other HD startup test
The ASan functional suite runs `wallet_upgradetohd.py --legacy-wallet`. This test upgrades a Berkeley DB wallet to HD and then starts the node with `-usehd=0`; startup verifies the wallet through `BerkeleyDatabase::Verify()`, opens the DB_PRIVATE environment, loads the wallet, and only then rejects the option in `CWallet::Create()`. Teardown can therefore append the suppressed `__lock_open / 1 allocation / 160 bytes` summary. Because this call does not pass `expected_lsan_suppressions`, `assert_start_raises_init_error()` retains the summary in stderr and its full-text comparison fails. Declare the same exact row used by `wallet_hd.py`; the descriptor-wallet run remains valid because the helper permits the summary to be absent.
2 participants
Issue being fixed or feature implemented
The Linux ASan job exposed two independent dependency-owned lifetime reports:
QDBusConnectionManager::executeConnectionRequestreport appears on unrelateddevelop-based PRs.The previous branch suppressed Berkeley DB's shared
__os_mallocallocator and changed Dash wallet environment lifecycle code. The allocator rule was too broad, and the production changes were not the cause of either retained allocation. LeakSanitizer also appends a suppression summary to subprocess stderr, breaking exact stderr checks inwallet_hd.pyandtool_wallet.py.What was done?
QDBusConnectionManager,__lock_open, and__memp_fopen; removed the shared__os_mallocrule.assert_start_raises_init_error()parameter for startup-error tests and madewallet_hd.pydeclare its exact__lock_open / 1 allocation / 160 bytesentry.dash-walletsubprocess checks accept only that same exact BDB summary when present. Application stderr, malformed summaries, and different suppression rows still fail.How Has This Been Tested?
On current
developwith the configured macOS depends tree:make -j13./src/test/test_dash --run_test=wallet_testsQT_QPA_PLATFORM=cocoa ./src/qt/test/test_dash-qt(full app and wallet cases, no skips)test/functional/test_runner.py wallet_hd.py(descriptor and legacy modes)python3 -m unittest test_framework.test_nodeUsing the exact
linux64_asanartifact from failed job 93167272068 in an x86 CI-faithful container:__lock_open.__memp_fopen.wallet_hd.pypasses in descriptor and legacy modes with the final framework code and suppression file mounted in.The next ASan cycle additionally demonstrated that both
wallet_hd.pymodes pass and thattool_wallet.pyreceives exactly one 160-byte__lock_opensummary; commit 587108a applies the same strict parsing to that subprocess path.No generic allocator suppression, disabled leak detection, skipped affected test, diagnostic instrumentation, or production wallet behavior change remains.
Breaking Changes
None.
Checklist: