fix(app): report an exited target once instead of spamming error dialogs - #82
Merged
Conversation
Selecting a process that has already exited (or watching one exit with a tool window open) filled the screen with error dialogs until the app had to be killed from another terminal. The auto-refresh dialogs poll every 300-1000 ms and reported *every* failed poll with a modal. A modal runs a nested event loop, so the timer kept firing underneath it and the dialogs stacked - 332 of them in six seconds in a headless repro. `File -> Change Process...` fed the same loop from the other side: it closed the old handle while the Memory Map and the hex viewers were still polling through it. A failure is now reported only when it changes what the user sees: the first fetch (the window is empty) or the one that makes the poll give up after 3 s of continuous failure (the table is frozen from then on), and never twice for the same error. Failures in between heal quietly, which is what keeps a target failing on alternating ticks from stacking a modal per failure. On giving up, the poll stops and the status line says so - including after a filter keystroke rebuilds it. Changing process now tears down every dialog that held the old handle. Fixing that surfaced neighbouring teardown gaps that made the guarantee hollow: * `QDialog.done()` delivers no `QCloseEvent`, so the teardown that lived in `closeEvent` never ran for the dialogs' own Close button (`accept()`) or Esc (`reject()`). Every worker-bearing dialog kept polling, hidden and unreachable, and leaked a thread per open/close cycle. A `TearsDownOnClose` mixin now runs the teardown exactly once, whichever way a dialog is dismissed. * `CheatTable.shutdown()` joined its poller with a bounded wait and ignored the result, so a wedged poller kept reading while the handle it read through was closed and the widget owning its thread was deleted. * A detached worker (one that blew its join and was parked rather than stopped) could still be inside a read when the process handle was released - a recycled HANDLE on Windows, a deallocated Mach port on macOS. Releasing a handle now waits for those workers, and they all live in one registry instead of two, so nothing is invisible to that wait. * `application.main` released the handle the picker opened rather than the one the window ended up attached to, leaking the current target's handle after a process switch. Also documents that `get_threads` reports an exited process differently per platform (empty on Linux/Windows, `OSError` on macOS), unlike `get_memory_regions` and `get_modules`. Closes#74
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the app’s error-dialog spam when the target process exits (or its handle is closed during process switching) by making auto-refreshing dialogs report failures only when it meaningfully changes what the user sees, and by hardening dialog/worker teardown so background QThreads don’t keep polling (or outlive their owners) in unsafe ways.
Changes:
- Add an auto-refresh base dialog with a “report once + grace window + give-up” policy to prevent stacked modal dialogs.
- Introduce shared detached-worker handling and a
TearsDownOnClosemixin so teardown runs exactly once regardless of dismissal path (close/accept/reject). - Defer process-handle release until detached workers are finished (or bounded-wait at app exit), and close all worker-bearing dialogs/viewers on process switch.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/app/test_detached_worker_close.py | New tests covering detached-worker close deferral and bounded waiting behavior. |
| tests/app/test_auto_refresh_dialog.py | New regression tests for “report once”, grace window, give-up behavior, and teardown via Close/Esc/close(). |
| PyMemoryEditor/process/abstract.py | Document platform differences for get_threads() on exited processes. |
| PyMemoryEditor/app/threads_dialog.py | Add _on_polling_stopped() status message when auto-refresh gives up. |
| PyMemoryEditor/app/pointer_scan_dialog.py | Switch to TearsDownOnClose and move cleanup into _teardown(). |
| PyMemoryEditor/app/open_process_dialog.py | Switch to TearsDownOnClose and unify worker shutdown via shutdown_worker_thread(). |
| PyMemoryEditor/app/modules_dialog.py | Preserve “auto-refresh stopped” in status line and add _on_polling_stopped(). |
| PyMemoryEditor/app/memory_viewer_dialog.py | Switch to TearsDownOnClose and move timer/worker shutdown into _teardown(). |
| PyMemoryEditor/app/memory_map_dialog.py | Preserve “auto-refresh stopped” in status line and add _on_polling_stopped(). |
| PyMemoryEditor/app/main_window.py | Close all worker-bearing dialogs/viewers on process switch; defer old-handle close via detached-worker gate; move scan-worker detaching into shared registry. |
| PyMemoryEditor/app/log_console_dialog.py | Switch to TearsDownOnClose so logging handler teardown happens on accept/reject too. |
| PyMemoryEditor/app/cheat_table.py | Make shutdown idempotent; route poller shutdown through shutdown_worker_thread; guard UI-to-worker calls after shutdown. |
| PyMemoryEditor/app/application.py | On exit, bounded-wait for detached workers then close the current window process handle (not the original picker handle). |
| PyMemoryEditor/app/_widgets.py | Add shared detached-worker registry helpers and the TearsDownOnClose mixin. |
| PyMemoryEditor/app/_auto_refresh_dialog.py | Implement failure-grace and one-time reporting policy; stop polling after persistent failure; integrate TearsDownOnClose. |
| docs/app.md | Document new “single notice + auto-refresh stopped” behavior when the target exits. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
…etries Review follow-ups on the dialog teardown work. `_on_worker_finished` cleared `self._worker` and deleted it without checking which worker had actually finished. `finished` is queued, so it can arrive after the next tick already started a replacement: a worker leaves `run()` (`isRunning()` goes False, so the in-flight guard lets the next fetch through) and its signal lands a moment later, retiring the *running* replacement instead. That drops the reference the teardown needs and schedules a live QThread for deletion, which aborts the process. Both the auto-refresh dialogs and the hex viewer (whose interval goes down to 50 ms, so the two overlap easily) now retire the worker the signal came from. An explicit `refresh()` against a still-dead target reported nothing until the grace window expired, because the window already had data from before. The flag that fixes it is bound to the fetch `refresh()` starts, so it answers that request and cannot leak into an unrelated failure later. The Threads window sat at "0 thread(s)" against a dead target on Linux and Windows, where `get_threads` reports an exited process as an empty sequence rather than raising — never reporting, never giving up, while the Memory Map and Modules windows beside it said the process was gone. A live process always has at least one thread, so an empty enumeration is now treated as the target being gone. Also: nothing may start a fetch on a dismissed dialog (the teardown latch is one-way, so such a worker could never be joined again), and the process picker joins its enumeration worker briefly instead of for a second — that join runs on the click that dismisses it, waiting on a process list nobody will read.
The process picker and the pointer scan kept the shape that was fixed in the polling dialogs: retire whatever `self._worker` currently points at, regardless of which worker the queued `finished` came from. The picker is genuinely exposed — it re-enumerates every 3 s, and a worker leaves `run()` before its signal is delivered, so the in-flight guard can let a replacement start in that window and the late signal then deletes the running one. The pointer scan can't hit it today (the Scan button stays disabled until this very slot re-enables it, so no scan can be replaced mid-flight), but it now matches: re-enabling the UI or dropping the worker on behalf of a superseded scan would be wrong the moment that precondition changes.
CI hit two failures that were both in the new tests, not in the app. `tests/app/test_auto_refresh_dialog.py` left the Qt process in a state where a *later* test segfaulted inside pytest-qt's `qtbot.wait` — enough to take an xdist worker down with "node down: Not properly terminated", after which the run hung until the 6 h job timeout. Reproducible deterministically with pytest tests/app/test_auto_refresh_dialog.py tests/app/test_app_smoke.py The dialogs were parentless, so each one was owned by its Python wrapper and destroyed the instant the test's reference went away, taking its worker children with it while the event queue could still hold events for them. They now hang off a module-scoped parent widget, so Qt owns them and nothing is freed underneath a queued event. They are still closed per test — that is what exercises the teardown. `test_the_close_waits_for_a_detached_worker` also failed on Windows with "Internal C++ object (_WedgedWorker) already deleted": once the worker finishes, the detach machinery reaps it, so asserting on `isRunning()` afterwards touches a deleted object. The worker now sets a plain Python flag when its `run()` completes, and the assertions use that.
Review follow-up. The Hex Viewer is the one polling window that never gave up: against a dead target its read fails on every tick and each one logged a warning — up to 20 a second at the 50 ms interval floor — so the Log Console filled with the same line forever. That is the spam of issue #74 wearing a different hat, reported through the logger instead of a modal. It now logs the first tick of a failure streak rather than every one, and stops polling after the same grace window the other dialogs use. Stopping goes through the auto-refresh button, so the UI can't claim to be refreshing while it isn't, and re-enabling it hands the target a fresh grace window. Also guards the pending-worker snapshot in `call_when_detached_workers_finish` against a worker reaped between the listing and the check, matching `wait_for_detached_workers` and the loop just below it — the same list with the same hazard, and an escape there would abort a process switch mid-teardown.
…ssage Review follow-ups on the previous commit. Deduping the log line by message alone went quiet about a *different* range that started failing after the user moved the viewer — the point is to stop repeating the same line every tick, not to hide a new problem. The key is now (address, size, message). Also notes in `_FAILURE_GRACE_MS` that the Hex Viewer shares the budget (its interval goes down to 50 ms, so a tick count would have been an order of magnitude off), and documents the viewer's behaviour alongside the other windows in docs/app.md.
Scope trim after re-reading the whole change. Three pieces went in during review rounds rather than from evidence about issue #74, and each cost more than it bought. * The deferred process-handle release. Two helpers, an app-exit hook and a test module existed to keep a handle open until every detached worker had finished. What they protected against is a read whose result is already discarded: those fetches are read-only and their signals are disconnected before the handle goes. `_change_process` closes inline again, with the residual window written down where it happens. The cheat poller still goes through `shutdown_worker_thread`, which is not cosmetic — deleting a widget that owns a running QThread aborts the process. * `_report_next_failure`. It answered an explicit refresh right away instead of at the give-up, which only differs when the user re-opens an already-open window from the menu. A fourth piece of state in a small state machine, for a case where the status line already shows the previous failure. * The stale-`finished` guard in the pointer scan, which the scan button's own disabled state already makes unreachable. `application.main` keeps closing `window.process` rather than the handle the picker opened — that one was a real bug, not hardening.
The detach rationale still pointed at the handle-holding mechanism that the trim removed. The shared registry earns its place on its own: one place to look for threads that outlived their owner.
`worker.disconnect()` — the bare, no-argument form — raises TypeError in
PySide6 ("not enough arguments"), and the `except (RuntimeError,
TypeError): pass` around it swallowed that. Nothing was ever
disconnected: a slot still fired after the call returned. Every "a late
emit can't touch a now-closing dialog" claim in this app rested on it,
including the ones this branch added.
`QObject.disconnect(worker, None, None, None)` is the form that works.
`worker.disconnect(worker)` is not an alternative: it only drops
connections whose *receiver* is the worker, and every slot here belongs
to the owning dialog.
Two consequences of the same root cause come with it:
* `_handle_ready` / `_handle_failed` now refuse to act on a dismissed
dialog, the way `refresh` / `_start_fetch` already did. Disconnecting
cannot unqueue a delivery that was already posted, and during
`MainWindow.closeEvent` that modal would be a nested event loop in the
middle of teardown.
* The Threads window waits for two consecutive empty enumerations before
calling the target dead. An empty snapshot is legal-but-rare on a live
process (`Thread32First` can come up dry), and the first fetch is
reported the moment it fails — so a single blip announced the death of
a process that was very much alive.
Also corrects the `_change_process` comment about detached workers: the
exposure is the concurrent read itself, not the discarded result, and
that window is accepted rather than eliminated.`detach_worker` connects the reaper to `finished` after the caller has checked `isRunning()`. A worker that finishes inside that window has already emitted, so the reaper never runs and a dead worker stays parked in the registry for the life of the process. It now reaps on the spot when the thread turns out to be finished. Also gives the mixin an `_is_dismissed()` helper instead of repeating `getattr(self, "_teardown_done", False)` at four call sites: a typo in one copy would silently disable that guard, and the guards are what keep a dismissed dialog from fetching or reporting.
Several of these blocks restated the code or told the story of how the bug was found. Kept the notes that would stop a future edit from reintroducing something — why the four-argument disconnect, why the clear() after closing hex viewers, why the snapshot is cleared last — and cut the rest.
`MainWindow.closeEvent` ran `_shutdown_worker()` — which pumps the event loop for up to two seconds while it waits on the scan worker — before closing the auxiliary dialogs. The dialogs are still live during that pump, so a poll landing there reaches `_handle_failed` and pops a modal in the middle of teardown. The heartbeat was already stopped first for exactly this reason; the dialogs are the same hazard and were missed. Reproduced with Modules open past its grace window and a wedged scan worker: one modal during the pump before, none after. Three smaller ones from the same review: * The cheat poller checks the stop flag per entry rather than once per tick. It is the worker that *writes*, and a write landing on a handle closed behind it is far worse than a stale read — a recycled HANDLE on Windows sends it into an unrelated process. Blowing the join now takes a single syscall hanging for a second, not a large table. * `detach_worker` sweeps finished workers out of the registry. Qt emits `finished` before `isFinished()` flips, so the check added for that race could miss it and leave a dead worker parked for good. * `MemoryViewerDialog._on_read_result` and `PointerScanDialog._on_error` refuse to act on a dismissed dialog, like the auto-refresh handlers already did. Disconnecting cannot unqueue a delivery already posted, and `_on_error` pops a modal.
Same pass as before, on what the last two commits added.
The registry sweep was justified by Qt emitting `finished` before `isFinished()` flips. Measured on this build: 200 out of 200 samples had `isFinished()` already true inside the handler, so the claim doesn't hold and the sweep guarded nothing reproducible. The fallback that does have a demonstrated case — a worker that finished before the reaper was connected — stays.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#74.
The bug
Attaching to a process that has already exited — or watching the current target exit with a tool window open — filled the screen with error dialogs, one per poll tick, until the app had to be killed from another terminal.
Reproduced headlessly before touching anything: 332 stacked modals in 6 seconds.
The auto-refresh dialogs (Memory Map, Modules, Threads) poll every 300–1000 ms and reported every failed poll with a
QMessageBox. A modal runs a nested event loop, so the timer kept firing underneath it and each failure stacked another dialog on top of the last.File → Change Process…fed the same loop from the other side: it closed the old handle while the Memory Map and any open hex viewer were still polling through it.The fix
A failure is reported only when it changes what the user sees:
Never twice for the same error. Everything in between heals quietly — and that silence is load-bearing, not tidiness: reporting every failure re-opens the spam, because a target failing on alternating ticks stacks a fresh modal per failure (249 modals plus a
RecursionErrorin a repro of that case).On giving up, the poll stops and the status line says
auto-refresh stopped, including after a filter keystroke rebuilds that line. Changing process now tears down every dialog that held the old handle.Teardown gaps this surfaced
The fix leaned on guarantees that turned out not to hold, so these came with it:
QDialog.done()delivers noQCloseEvent. The teardown living incloseEventnever ran for the dialogs' own Close button (accept()) or Esc (reject()) — every worker-bearing dialog kept polling, hidden and unreachable, leaking a thread per open/close cycle. ATearsDownOnClosemixin now runs the teardown exactly once, whichever way a dialog is dismissed.CheatTable.shutdown()joined its poller with a bounded wait and ignored the result, so a wedged poller kept reading while the handle it read through was closed and the widget owning its thread was deleted.application.mainreleased the handle the picker opened rather than the one the window ended up attached to, leaking the current target's handle after a process switch.Also documents that
get_threadsreports an exited process differently per platform (empty on Linux/Windows,OSErroron macOS), unlikeget_memory_regionsandget_modules, which raise everywhere.Deliberate trade-offs
Verification
tests/app/test_auto_refresh_dialog.py(reporting policy, give-up, grace window, flapping target, dismissal via Close/Esc/close()) andtests/app/test_detached_worker_close.py(deferred and blocking handle release).tests/app→ 100 passed.MainWindow: wedged cheat poller, wedged scan worker, handle released at exit, hex-viewer lifecycle, Esc dispatched from Qt's C++ side.flake8clean,mypyclean, docs build clean.The 15 failures elsewhere in the suite are pre-existing macOS-only
mach_vm_read_overwritefailures in the core scan/memory tests — reproduced on a clean checkout with these changes stashed, and macOS is not in CI.