fix(crashlog): build the crash context outside the lock by donislawdev · Pull Request #210 · donislawdev/BeanNetworkTester

donislawdev · 2026-09-28T17:18:09Z

Summary

The crash logger could deadlock the whole program. crashlog._record built the report context while holding its only lock, and the context provider is the GUI's own code (gui/crash.context). Both ways it deadlocked were reproduced before the fix:

Same thread. When a Control page field held an invalid value (a letter in Loss), the provider's read of the form raised ValueError and passed it to crashlog.note(). That call went back into record() and tried to take the lock the same thread already held. The thread hung, and every later report from every thread queued behind it, the main thread included.
Across threads (real Tk 9.0). A worker thread's provider read a tk.StringVar, which waits for the Tk main loop, while the main loop was waiting on the logger's lock.

The WinDivert handle still closes in both cases: the stop path closes it first and the 10 s stall check fires. The damage was a frozen window (STOP did nothing), and a watchdog whose own crash-log calls queue behind the wedged lock.

Changes

crashlog.py
- The duplicate check stays under the lock. The context is built outside it, and the entry is inserted under the lock with a second look. If another thread recorded the same new fault in the meantime, the occurrence merges into its record (_count_again) and nothing is written to disk twice.
- A thread-local re-entrancy guard in _collect_context: a fault the provider records itself gets the base context only (context_provider_skipped: "re-entered"). Without the guard, provider -> note -> provider would recurse even with the lock out of the way.
- The module docstring and set_context_provider now state the provider's contract: plain data, no GUI toolkit calls, no lock a recording thread may hold. A stale sentence claiming the CLI sets a provider is removed; only the GUI does.
gui/crash.py
- context reads plain data only:
  - Form: a raw copy taken by leave_breadcrumb on the main thread every tick, kept in a WeakKeyDictionary in this module. App is exactly on both class ratchets, so the copy is not stored on App.
  - Counters: app.last_snapshot, the same numbers one tick old, read without the engine lock.
  - Invalid field: the report gets settings_error plus the raw form instead of recording a fault.
- A real failure inside context is still recorded (crashlog.quiet). That is safe now, because no lock is held and the provider is not asked again.
- The per-tick copy costs about 23 us on real Tk 9.0.4 (42 fields, measured), against a tick of ~0.7 s.

Tests

New in tests/test_crashlog.py. Each test swaps in a private lock, so a regression fails that one test instead of hanging the suite.

test_a_provider_that_records_a_fault_of_its_own_does_not_wedge_the_logger
test_two_threads_can_build_their_context_at_the_same_time: a barrier inside the provider, passable only when both threads are in it at once.
test_the_same_new_fault_from_two_threads_at_once_is_one_record
test_a_gui_crash_report_never_reads_tk_off_the_main_thread: the real App on the fake Tk, an invalid field, and a worker writing a report.

Mutation proof: four new entries in tests/test_mutation_registry.py, all four caught:

re-entrancy guard off
provider under the lock again
no second look
GUI report reading Tk again

All 12 crashlog/crash: entries were run and caught. The move_to_end anchor moved into the helper, so its indentation was updated.

Run locally:

the guard selection for the changed files, plus ruff and mypy on the changed modules;
the reproduction probes before and after the fix. Before: hang after 3 s, and deadlock until the 8 s faulthandler dump. After: both finish in about 0.1 s, and the form is read on the main thread only.

Not run locally: the full suite (it runs here on Linux and Windows).

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Fixed an issue that could freeze the app when a form contained an invalid value or a background error occurred, leaving STOP unresponsive.
- Crash reports continue to include available settings and now identify invalid form values as invalid, rather than reading form values during error reporting.

The context provider is the GUI's own code, and it ran while the crash logger held its only lock. An invalid form field made the provider record its own ValueError into that lock on the same thread, and a worker's Tk read waited for a main loop that was waiting on the lock. Either way every later report from every thread queued behind it: the window froze and STOP did nothing. - crashlog: de-duplicate under the lock, build the context outside it, insert under the lock again with a second look (a concurrent record of the same new fault merges into one). A thread-local guard records a fault the provider raises itself without asking the provider again. - gui/crash: the report reads plain data only - a raw form copy taken on the main thread each tick, and the last counter snapshot. An invalid field is reported as settings_error instead of being recorded. - Four tests and four mutation entries, all caught. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-09-28T17:18:30Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Advanced

Run ID: fc23ce17-a700-4c72-bc44-8c4e945585c6

📥 Commits

Reviewing files that changed from the base of the PR and between 1382383 and 2e36b66.

📒 Files selected for processing (5)

CHANGELOG.md
beantester/crashlog.py
beantester/gui/crash.py
tests/test_crashlog.py
tests/test_mutation_registry.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout. (9)

GitHub Check: pip-audit (advisories against the pinned set)
GitHub Check: Analyze (python)
GitHub Check: mutation registry
GitHub Check: tests (ubuntu-latest, py3.14)
GitHub Check: tests (windows-latest, py3.14)
GitHub Check: mypy
GitHub Check: semgrep (ERROR, HIGH and CRITICAL block)
GitHub Check: Analyze (actions)
GitHub Check: review new dependencies

🧰 Additional context used

📓 Path-based instructions (15)

Applies to text shown to the user (labels, buttons, tooltips, placeholders, dialogs, errors, status messages, empty states, translations).