Fix toggle race and replace pkill with proper PID handling - #4
Conversation
- mining_controller now derives state from the Popen handle instead of flipping sender.state / xmrig_status before xmrig actually starts, so the menu no longer lies when xmrig dies immediately on bad config. - Add a short startup grace window; if xmrig exits within it, the menu stays off and a macOS notification tells the user to check the log. - Replace 'pkill xmrig' (fire-and-forget, name-based) with terminate() on the stored Popen handle, then wait with a SIGKILL fallback. Applies to toggle-off, the Quit button, and the global exception handler. - Run xmrig with start_new_session=True so its process group is isolated from the menu bar app's signals.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…ation Fixes the 5 issues Greptile flagged on PR numycode#4: P1: stop no longer blocks the macOS main thread (up to 5s). The @rumps.clicked callbacks and Quit handler now dispatch _stop_xmrig to a daemon background thread. The thread hops back to the main run loop via AppHelper.callAfter for the menu state update, so the UI stays responsive and Quit exits immediately. P1: TimeoutExpired is already caught inside _stop_xmrig's try block, so the uncaught-exception path on Quit is closed. P1: stale checkmark after unexpected xmrig death is reconciled. mining_controller now checks _is_running() before honoring sender.state, and a rumps.Timer polls the process during the startup grace window. If the process dies inside the grace window (or at any point while the toggle is on), the timer callback clears the stale checkmark and surfaces a notification. The stop-while-grace-timer-running case is also handled: the daemon thread sets xmrig_process = None before the timer callback next fires, and the timer stops itself. P2: the original time.sleep(0.25) on the main thread is gone. Popen returns quickly; the grace period is now driven by a rumps.Timer on the main run loop, so the menu can dismiss immediately. P2: only OSError is caught, so the redundant FileNotFoundError clause is already gone.
|
Addressed all 5 Greptile review items in a follow-up commit on this branch. P1 fixes
P2 fixes
Bonus: the global exception handler in Still unverified at runtime — sandbox is Linux, rumps is macOS-only. Re-requesting @greptile-apps review on the new commit. |
…final-tick crash Re-review found 5 new P1 issues introduced (or not fully closed) by the previous commit. All five are addressed here. P1 (daemon-thread orphan on Quit, line 105): _stop_xmrig_async on Quit spawned a daemon thread that the Python interpreter kills during Cocoa app teardown, so xmrig was orphaned. on_quit now calls _stop_xmrig_sync with SHUTDOWN_* timeouts so SIGTERM/SIGKILL actually land before rumps.quit_application() tears down the run loop. SHUTDOWN timeouts reduced from 2s+1s to 1s+0.5s to keep the blocking wait well under macOS's 5s unresponsiveness threshold. try/except around the sync stop ensures rumps.quit_application() is still called if anything unexpected blows up. P1 (grace-expiry swallows crash on final tick, line 174): the timer callback's grace-expiry early return was checked before poll(), so a crash on the very last tick of the grace window was silently ignored and the toggle stayed on. Reordered: poll() crash check runs first, with an explicit return; grace-expiry branch moved to the end. P1 (grace timer not cancelled on intentional stop, line 211): if the user clicked Toggle during the 250ms startup grace window, the timer kept firing and saw the process exiting via SIGTERM, posting a spurious 'XMRig stopped unexpectedly' notification. _stop_xmrig_async now calls _startup_check_timer.stop() before snapshotting the handle. P1 (TimeoutExpired from SIGKILL wait, line 233): the outer except in _stop_xmrig_sync only covered (OSError, ProcessLookupError). If a hung xmrig did not die after SIGKILL, the second proc.wait() raised subprocess.TimeoutExpired, propagated out of the exception handler in __main__, and skipped both the 'Force exiting...' log and os._exit(0). Added subprocess.TimeoutExpired to the outer except tuple.
|
Second-round Greptile review surfaced 5 new P1s. All addressed in a new commit on this branch ( Quit path — Grace timer race — Spurious crash notification — TimeoutExpired leak — the outer Diff: |
Greptile's third review caught that the TimeoutExpired fix I applied to _stop_xmrig_sync was missing from the parallel inner _shutdown inside _stop_xmrig_async. The inner target.wait(timeout=TOGGLE_SIGKILL_TIMEOUT) on line 216 is not wrapped in its own try/except, so a hung xmrig that somehow survives SIGKILL raises TimeoutExpired, the outer except on line 217 only covers (OSError, ProcessLookupError), the exception escapes the function, and the daemon thread dies silently while xmrig stays running as an orphan. The finally still clears the handle and flips the menu, which makes the bug hard to notice — menu says 'off' but xmrig is still mining. Add subprocess.TimeoutExpired to the outer except tuple to match the sync path.
|
Thanks Greptile — good catch. The fix I applied to the sync path was missing the parallel in the inner of . The on line 216 isn't in its own try/except, and the outer except only covered , so a hung xmrig that survived SIGKILL would have raised , the function's would still run (clearing the handle and flipping the menu to off), but the exception would then escape the daemon thread silently and xmrig would stay running. New commit |
Two cleanups after a final once-over: 1. __main__ exception handler now wraps _stop_xmrig_sync in its own try/except so a stop failure (e.g. an unexpected exception escaping the inner try blocks) cannot prevent 'Force exiting...' from logging or os._exit(0) from running. An orphaned xmrig is the lesser evil compared to a process that won't quit at all. 2. _set_toggle_state docstring was misleading: it said 'from any thread' but NSMenuItem state must be set on the main thread. The function is safe in practice because all current call sites dispatch to the main thread (rumps click handlers, NSTimer callback, AppHelper.callAfter from the background _shutdown). Updated the docstring to make that contract explicit.
Two related races that both boil down to: a callback acting on self.xmrig_process without verifying it is still the process it was tracking. 1) Greptile T-Rex P1: the AppHelper.callAfter(self._set_toggle_state, False) in the async _shutdown was queued unconditionally, while the handle-clear was gated by 'if self.xmrig_process is target'. So if the user toggled off, the daemon finished, and then the user toggled on again before the queued callAfter ran, the new miner's check mark was silently cleared. Moved the callAfter (and the 'Stopped XMRig' log) inside the same gate. The new miner's _start_xmrig set state=True; the stale callAfter would have clobbered it. 2) Same shape bug, smaller window: the grace timer callback acted on whatever self.xmrig_process currently was. If the user reconciled a crashed startup by clicking the toggle, the grace timer's next tick would still see the dead handle and post a duplicate 'XMRig stopped unexpectedly' notification. Added self._grace_target, set in _start_xmrig right before arming the timer, checked at the top of _check_xmrig_startup — mismatch means the user has already acted, so the timer bails. The reconciliation path itself also clears xmrig_process, stops the timer, and clears _grace_target so any already-dequeued tick bails too.
|
Caught — good T-Rex repro. The handle-clear was gated but the While I was in there I also fixed the same shape of bug in the grace timer (smaller window, ~0.25s, but a real duplicate-notification case I had missed). The grace callback now tracks Commit |
Both actions now run on Node 24, clearing the deprecation warning
GitHub emitted on run 28975020358 ('Node.js 20 is deprecated. The
following actions target Node.js 20 but are being forced to run on
Node.js 24: actions/setup-python@v5, actions/upload-artifact@v4').
v5/v6 of these actions require Actions Runner v2.327.1+, which
macos-latest already satisfies. No other workflow changes needed;
the action inputs (python-version, artifact name/path) are
unchanged across these majors.
|
Also bumped the two Node 20 actions in the macOS build workflow (commit
Both v6/v5 of these actions require Actions Runner v2.327.1+, which |
Latest action majors + a few quality-of-life wins. All changes are backward-compatible drop-ins — same inputs to actions, same default behaviors for our usage. Actions: - actions/checkout v5 → v7 (latest, June 2026). v7 blocks fork PR checkouts under pull_request_target / workflow_run by default, which we don't use, so this is a free security default. - actions/setup-python stays on v6 (already on v6 from prior commit); tracking @v6 picks up future patches. - actions/upload-artifact v5 → v7 (latest, April 2026). v7's only breaking change is the new opt-in `archive: false` flag for direct single-file uploads; our directory upload with the default `archive: true` is identical to v5. Hardening / perf: - cache: 'pip' on setup-python, keyed on a hash of requirements.txt, pyproject.toml, and setup.py. Reuses pip's download cache between runs; shaves 30–60s on every non-cold build. - permissions: contents: read at the top level. The workflow never pushes back, so the GITHUB_TOKEN should not be granted write scope. - concurrency: cancel-in-progress on pull_request only. Saves CI minutes when multiple commits land on a PR in quick succession. main pushes still run to completion so a release build is never cut off by a follow-up. - timeout-minutes: 20 on the job. py2app is ~5–8 min cold, ~2–3 min warm; 20 min is a generous ceiling that prevents a hung run from burning the whole 6-hour Actions budget.
|
While I was in the workflow, did a full pass (commit Latest action majors:
Quality-of-life wins:
What I deliberately did not do:
|
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Lint job: ruff check + format check, parallel to the build, runs on every PR. Continue-on-error for now so it surfaces style issues without blocking builds; tighten to blocking once the existing code is ruff-clean. Build job: package the py2app output as a DMG via hdiutil (built into macOS, no install needed) and upload with upload-artifact@v7's 'archive: false' option. The user gets a single XMManager.dmg to double-click and drag to Applications, instead of a zipped dist/ folder that they have to unzip just to get to the .app. Also adds a build summary in the Actions UI (app size, DMG size, main binary arch) and a status badge at the top of the README.
CI: DMG distribution + lint jobNew commit What changed in
After this lands, artifact download will be: User double-clicks the DMG, gets Known follow-ups (intentionally skipped here):
I can tackle any of those as a follow-up if you want — they each deserve their own PR so this one stays scoped to "make the build work better". I also cloned |
Replaces the old adjacent_to_app() helper (which assumed xmrig sat next
to the .app) with find_xmrig() that checks standard macOS install
locations in priority order:
1. $XMRIG_PATH environment variable
2. /opt/homebrew/bin/xmrig (Apple Silicon Homebrew)
3. /usr/local/bin/xmrig (Intel Homebrew)
4. /opt/local/bin/xmrig (MacPorts)
5. ~/bin/xmrig, ~/.local/bin/xmrig
6. Adjacent to the .app (legacy sidecar install)
7. Next to main.py (dev fallback)
GUI apps launched by Launch Services on macOS inherit a stripped PATH
(/usr/bin:/bin:/usr/sbin:/sbin), so 'which' isn't an option and the
known locations are checked explicitly.
If xmrig is not found anywhere, Main starts a one-shot rumps.Timer
that fires a notification ("XMRig not found ... install with brew
install xmrig or set XMRIG_PATH") and calls rumps.quit_application().
The timer is started from a run() override rather than __init__ so
the NSApp run loop is already up when notification/terminate are
called.
xmrig_path and xmrig_command moved from module-level globals to
instance attributes on Main (set from the constructor's keyword arg),
so a missing binary no longer constructs a None in the command list.
_start_xmrig has a defensive guard against the None case.
README updated with the new install instructions and XMRIG_PATH doc.
|
Pushed What changed
If nothing is found, the app shows a macOS notification and quits instead of silently sitting there with a dead toggle. README is updated with the new install steps and the Tested locally by stubbing |
XMRig defaults to ./config.json in the CWD, which is "/" for a Launch Services-launched menu bar app, so xmrig has never been able to find a config out of the box. Wire `--config=$HOME/.xmrig.json` into the launch command. README updated to point users at the new location and explain why it has to live outside the .app bundle.
Summary
mining_controller. Stop using a separatexmrig_statusglobal and flippingsender.statebefore we know xmrig actually started. State is now derived from the livePopenhandle, and a short startup grace window (0.25s) catches "xmrig exited immediately" cases so the menu no longer lies about whether mining is on.pkill xmrigwith real PID handling. Store thePopenhandle, sendSIGTERMto that specific child, wait up to 3s, and fall back toSIGKILL. This applies to toggle-off, the Quit button, and the global exception handler. No more fire-and-forget pkill, and no more risk of killing a manually-startedxmrigwith the same name.xmmanager.log.start_new_session=Trueso signals received by the menu bar app don't leak into the child.Why this matters
The original
mining_controllerdidsender.state = not sender.statefollowed bysubprocess.Popen(xmrig_command)with no readiness check. If xmrig died (bad config, missing binary, EACCES onchmod +x), the checkmark stayed on and the next click tried to stop something that wasn't running.subprocess.Popen(["pkill", "xmrig"])was fire-and-forget — the "Stopped XMRig" log line ran before pkill had done anything — andpkillmatches by name, so it could kill a freshly spawned child if the user clicked quickly, or any unrelatedxmrigon the system.Testing
I could not exercise this on macOS from the sandbox, so this PR is unverified at runtime. Manual plan on a real Mac:
pgrep -fl xmrigshows the child.config.jsonso xmrig dies on startup, click Toggle → menu stays off, notification, log showsxmrig exited immediately with code N.kill <xmmanager-pid>from another shell → xmrig is left running (this matches the original behavior because ofstart_new_session=True, but is now documented; a SIGTERM handler on the parent is a follow-up).AI disclosure
This PR was drafted with AI assistance (gorkie). The bug analysis, the design decisions, and the testing plan are mine; the AI helped structure the fix, catch missing error paths, and write this description. The README's
## AI Noticehas been updated to call out this contribution, per Hack Club Horizons' "no AI slop" rule.Not in this PR (intentionally)
adjacent_to_app(still has its own TODO from the original author).xmmanager_config.json(the file is currently dead — referencing it would be a behavior change).SIGTERM/SIGINThandler on the menu bar app process (see test step 6).