Skip to content

Re-enable leak detection in the AddressSanitizer test script - #38

Merged
dmccoystephenson merged 1 commit into
masterfrom
feature/reenable-asan-leak-detection
Aug 23, 2026
Merged

Re-enable leak detection in the AddressSanitizer test script#38
dmccoystephenson merged 1 commit into
masterfrom
feature/reenable-asan-leak-detection

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

  • Leak detection is re-enabled in run_tests_asan.sh (ASAN_OPTIONS=detect_leaks=1, stated explicitly rather than relying on the platform default).
  • The stale comment justifying the suppression is replaced. That comment attributed the suppression to a leak in the throw-by-pointer exception pattern, which no longer exists.
  • No source files are touched by this change.

Evidence the suppressed leak is gone

  • A grep of every throw site in src/ shows all four construct the exception by value (src/grid.cpp:30, src/grid.cpp:116, src/environment.cpp:67, src/environment.cpp:86). No throw new remains anywhere in the tree.
  • The only heap allocation in the library is Environment::grid (src/environment.cpp:10), which is paired with delete grid in ~Environment (src/environment.cpp:14).
  • No new, malloc, or free appears in src/tests.cpp; every Entity, Grid, and Environment used by the suite is stack-allocated, so each Environment destructor runs and releases its grid.

Test plan

  • make — clean compile.
  • bash run_tests_asan.shNOT RUN. See the verification gap below.
  • bash run_tests.shNOT RUN. See the verification gap below.

Verification gap — this PR is UNVERIFIED and is not proposed for automatic merge

The change was authored in a sandbox where binary execution is denied. ./tests_executable, bash run_tests.sh, bash run_tests_asan.sh, and a direct g++ -fsanitize=address ... invocation were each attempted and each was refused; only make is permitted, and it succeeded. The evidence above is therefore static analysis of every allocation and throw site in the tree, not an observed clean AddressSanitizer run.

The confirmation step requested by #33 — running the script and observing a clean pass — has consequently not been performed. Merging is asked to be gated on a maintainer running bash run_tests_asan.sh on a machine with an unrestricted toolchain and confirming a clean, leak-free pass. Should a leak surface that this static review did not predict (LeakSanitizer occasionally attributes allocations to runtime internals), the correct response would be to suppress that specific allocation rather than to restore the blanket detect_leaks=0.

Deferred backlog

#33 was the only issue open at triage time, so nothing was deferred for this cycle.

Closes #33

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

The suppression was added for a leak in the throw-by-pointer exception
pattern, which no longer exists: every throw site now constructs the
exception by value, and the library's only heap allocation
(Environment::grid) is paired with a delete in ~Environment.

Closes #33

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric

  • Scope: PASSgit diff --name-only origin/master...HEAD lists exactly one file, run_tests_asan.sh, which is the file named by run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33. No formatting, rename, or comment churn elsewhere. Net diff is 2 insertions / 3 deletions, far under the scope ceiling.
  • Tests-new: N/A — no public method or function is added by this diff.
  • Tests-fix: N/A, with a caveatrun_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33 is a tooling/documentation-drift fix, not a behavioral bug fix, so the stash-and-run empirical check has no production code to revert. The caveat is that the script itself is the test artifact here, and it could not be executed (see below).
  • Sibling structure: PASS — no new file is created.
  • Sibling renames: PASS — no identifier is renamed.
  • Docs: PASS — the ## Classes table in README.md still lists exactly Entity, Environment, Grid, and Location, each matching the class declared in the corresponding src/header/*.h. The only documentation surface inside this diff is the script's own comment, which is rewritten to match the new setting.
  • Issue resolution: PASS — the detect_leaks=0 suppression named by run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33 is removed and its stale justification replaced; nothing in run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33 is left partially addressed.
  • Header/definition parity: N/A — no header is modified.
  • Include guards: N/A — no header is added.
  • Test registration: N/A — no test*() function is added to src/tests.cpp.
  • Local test suite: UNVERIFIED — blocking for merge. ./tests_executable, bash run_tests.sh, bash run_tests_asan.sh, and a direct g++ -fsanitize=address -g src/*.cpp -o tests_executable_asan were each attempted in the authoring sandbox and each was refused. Only make is permitted there, and it completed cleanly. No AddressSanitizer run was therefore observed, and the clean pass requested by run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33 has not been demonstrated.

Findings

  • run_tests_asan.sh:8 — leak detection is now pinned with an explicit detect_leaks=1 rather than by deleting the variable. The upside is that the intent is self-documenting and portable to platforms where LeakSanitizer is not on by default. The trade-off worth flagging is that an inline assignment overrides the caller's environment, so a developer wanting to temporarily triage with ASAN_OPTIONS=detect_leaks=0 bash run_tests_asan.sh will find their setting ignored. That limitation existed before this change too, and is left as-is to keep the diff scoped to run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed #33.
  • run_tests_asan.sh:2rm ./tests_executable_asan has no -f, so the script reports an error on a fresh clone where the binary has never been built. The same pattern appears at run_tests.sh:2. Pre-existing and out of scope for this PR; filed separately.

Merge recommendation

Merge is not recommended from this session. The static evidence that the previously suppressed leak is gone is strong and is laid out in the PR description, but the confirmation step #33 explicitly asks for is an executed clean run, and that could not be produced under the sandbox's execution restrictions. A maintainer running bash run_tests_asan.sh on an unrestricted toolchain and observing a leak-free pass is what should gate this merge.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit 48cbccf into master Aug 23, 2026
@dmccoystephenson
dmccoystephenson deleted the feature/reenable-asan-leak-detection branch August 23, 2026 18:32
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.

run_tests_asan.sh disables leak detection for a leak that PR #32 already fixed

1 participant