[malpern]

Problem

On 2026-11-03, keypath-cli uninstall --yes --reset-permissions (1.0.2, MacBook Air) reported success and trashed the app while two session workers (PIDs 13284, 29722) kept running from the trashed bundle. Both were --session-capabilities probes whose --session-owner (30378) no longer existed, and both had to be killed by hand.

Three causes:

1. Probes never watched their owner. Only the runtime worker's 5 ms tick checks --session-owner, and that tick exists only after start() completes. A capability probe that hung lived forever.
2. The parent signalled an abandoned probe only on timeout. A cancelled Task.sleep threw past the SIGTERM.
3. Uninstall's stop step stopped only the worker the live app tracks. Orphans of earlier app processes were never considered, and success was reported without checking.

Fix

Worker side

• SessionOwnerWatch polls the owner on a background queue in both modes. A probe calls _exit(0) when its owner dies or after 15 s. A runtime worker hands off to an orderly finish on its run loop once it may hold keys or a Caps mapping.
• SessionShutdownFallback bounds the case where that run loop never runs: _exit(1) after 30 s. It is cancelled as soon as an orderly shutdown begins, so it never cuts off a Caps restore.

Parent side

• awaitCapabilityReport (extracted from the probe wait) signals the probe on every abandoned path: timeout, cancellation, error.
• sessionLaunchArguments builds the worker argv in one place, and a test pins it to the sweep's parser.

Uninstall

• SessionWorkerSweep reads this UID's process table via sysctl (no pgrep), with a p_comm prefilter and one shared argument buffer. It covers workers of the bundle being removed and orphans of any bundle.
• An owner counts as alive only if that PID is a KeyPath app running the worker's own executable, so a reused PID cannot hide an orphan.
• It SIGTERMs workers and re-signals late arrivals. Only capability probes are SIGKILLed; a runtime worker is never killed, because that could strand a Caps mapping.
• An unreadable table (retried once, zombies skipped) or a cancelled sweep is unverified, never "none running".
• SessionUninstallCoordinator sweeps after prepare (on failure nothing is removed, and it resumes) and again before reporting success, per the AGENTS.md postcondition rule. "Still running" and "could not check" get separate messages, and every sweep is logged.

Tests

swift test --filter 'SessionOwnerWatchTests|SessionShutdownFallbackTests|SessionWorkerSweepTests|SessionUninstallCoordinatorTests|SessionCapabilityProbe|SessionRuntimeWorkerBackgroundTests'

50 tests pass. They use injected seams only. The one live test reads this test process's own table entry and argv, and signals nothing.

Review gate: ./Scripts/review-gate.sh passed on 854853f2, after three rounds of fixes (each confirmed finding addressed in its own commit).

Not verified

• No end-to-end run against a real orphaned worker. The local attempt was blocked because it sends real signals, and vm-lab is unavailable while the mini is offline.
• The root cause of the original probe hang is unknown, because uninstall deleted the log. A hung probe now self-terminates.

Follow-ups (from the final review, all PLAUSIBLE)

• A runtime worker whose main loop is wedged ignores SIGTERM. Uninstall then refuses with "Restart your Mac". The app's own stop path escalates to SIGKILL under acquireTerminationGuard; the sweep could reuse that.
• The 30 s fallback bounds reaching an orderly shutdown, not completing one; the HID join stays unbounded by design.
• If the final sweep fails after removal, the uninstall dialog still shows "Try Again", which then reports the wrong cause.
• stopAll does not re-read the table after its last signal round, so a worker exiting at the grace boundary is reported as remaining. This errs toward refusing.
• The probe deadline is armed after the setActivationPolicy main-actor hop; it could be armed before it.
• Pre-existing orphans are reaped only by uninstall; a sweep at launch would cover them.
• Missing tests: SessionWorkerSweep.live propagating nil, and the worker-side finish → shutdownBegins() wiring.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-05T04:43:38.677987Z	854853f	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Review (diff truncated at 60KB; the tail of the test files was not seen)

Overall the design is sound (owner watch, shutdown fallback, uninstall sweep with pre/post checks). Findings:

1. Runtime worker hard-exit window (SessionRuntimeWorker.swift): Before ownerWatch.setHandler is installed, the startup handler calls _exit(0) for runtime workers too. If the owner dies during currentProcessPermissionCapabilities() or early setup, that is fine (nothing is held yet). But setHandler is called just before worker.start(...), while the experimental raw-capability calls above it are awaited. Those awaits are fine, but confirm no output/Caps state is acquired in SessionRuntimeWorker.init before the handler swap, since _exit(0) would skip the Caps journal restore.

2. Orphan scoping in belongsToUninstall: A worker whose owner PID is dead but whose executable lives in a different, live KeyPath bundle (for example a dev build) is treated as an orphan and killed. That is probably intended, but a runtime worker in that case gets SIGTERM only, and a probe gets SIGKILL after 2s. Consider a test pinning this behavior.

3. SessionShutdownFallback.arm semantics: A second call after arming returns true, so the caller's CFRunLoopPerformBlock runs retire again. retire guards on finished, but before finished is set a duplicate block is queued. It is harmless but redundant. A false-on-already-armed return, with a separate "shutdown began" signal, would be cleaner.

4. stopAll loop edge: If remaining is empty after the first iteration's signals (only possible when the initial list is non-empty and everything vanished), the loop breaks correctly. But if elapsed >= runtimeGrace while runtime workers remain, there is no final re-read after the last SIGTERM. The remaining list is from the previous poll, which is fine, but the sleep happens before the read, so the staleness is at most one poll interval.

5. Test coverage: There are no tests for currentUserKeyPathProcesses ENOMEM retry or zombie handling. These are hard to unit test, so just a note. SessionOwnerWatchTests use real Thread.sleep(0.1), which conflicts with the repo's "no Thread.sleep" guideline. The inverted-expectation approach would avoid it.

No blocking correctness bugs found in the visible portion.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 854853f28f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Synchronize handler replacement with verdict delivery

If the owner exits during runtime startup, the timer can copy the initial _exit(0) handler, release the lock, and be descheduled before invoking it. Meanwhile, runIfRequested() can complete setHandler() and enter worker.start(), which may acquire a Caps mapping. When the timer resumes, it still invokes the old hard-exit handler, bypassing finish() and leaving the mapping unrestored. Make the transition to orderly shutdown synchronize with in-flight handler delivery, rather than protecting only the handler property.

Useful? React with 👍 / 👎.

[malpern]

Verdicts are now delivered while holding the lock, and setHandler waits for one in flight, so the startup hard-exit handler can't run after the orderly one is installed. Covered by testSetHandlerWaitsForAVerdictInFlight.

🤖 Addressed by Claude Code

[chatgpt-codex-connector]

Replace the test's real sleep with controlled time

The new owner-watch test uses Thread.sleep to wait for possible duplicate callbacks, contrary to the explicit AGENTS.md testing rule: “Do not use Thread.sleep. Use Date overrides or mock clocks.” This adds a wall-clock delay while still making the negative assertion depend on background-queue scheduling. Inject a controllable scheduling seam and advance or drain it deterministically before checking the callback count.

AGENTS.md reference: AGENTS.md:L380-L381

Useful? React with 👍 / 👎.

[malpern]

Removed the Thread.sleep: the test injects a serial queue and drains it with queue.sync {} before checking the callback count. The no-sleep lint now passes.

🤖 Addressed by Claude Code

[claude]

Review (diff was truncated at 60 KB, so the last uncovered test file was not reviewed).

No blocking correctness issues found. The owner-watch, shutdown-fallback and sweep logic reads consistently. Notes:

1. Test convention (minor): SessionOwnerWatchTests.testWatchFiresOnceWhenOwnerDiesWithoutTheMainThread uses Thread.sleep(forTimeInterval: 0.1). AGENTS.md says not to use Thread.sleep. An inverted expectation or a second fulfill counter would do the same job.
2. PID-reuse window in stopAll (low): SIGTERM/SIGKILL go to PIDs from the last listing. kill is not rechecked against the process start time, so a worker that exits and has its PID reused between read() and signal could signal an unrelated same-UID process. The window is about 100 ms and it is the same UID, so the risk is small. Re-listing right before the SIGKILL step would narrow it.
3. Late arrivals (low): once remaining is empty the loop exits without a final re-read. A worker spawned in that gap is only caught by the coordinator's post-removal sweep, which reports failure after the app is already trashed. That is acceptable, but the failure text could say it is safe to retry.
4. SessionShutdownFallback.arm: finish() sets finished = true before calling shutdownBegins(). retire() checks finished and arm() checks shutdownBegan, so there is a tiny window where the owner-exit handler arms _exit(1) after finish has started but before it cancels. shutdownBegins() cancels the armed item right after, so this is benign. It would be tidier to call shutdownBegins() first.

Test coverage is good: the argv parser is pinned to the real launch arguments, and both cancellation and unverified sweep paths are tested.

[claude]

Review of PR #1361. The diff was truncated at 60000 bytes, so I did not see the end of SessionUninstallCoordinatorTests.

No blocking issues found. The owner-watch, shutdown-fallback and worker-sweep logic holds up. The cancellation, unreadable-table and runtime-never-SIGKILL paths are handled and tested.

Minor notes, none blocking:

• Owner liveness: SessionOwnerWatch relies on SystemStateProvider.isProcessAlive(pid:). If that is kill(pid, 0), a zombie owner or a reused owner PID looks alive. The probe deadline covers probes. A runtime worker with a reused owner PID would only be caught by the run-loop tick, and only if that tick checks the same thing. Consider checking process identity there too, as belongsToUninstall already does.
• Test fragility: testLiveListingNeverIncludesThisProcess compares KERN_PROCARGS2 argv with ProcessInfo.arguments under xctest. That can differ under some runners and may flake.
• Test setup: the Locked helper is duplicated as a private class in two test files. Sharing one would be tidier.
• Failure messaging: if the final sweep reports a worker after the app has been trashed, the result is success: false with a clear message and the backup path. That is correct. Make sure the UI doesn't also tell the user to retry the uninstall, since nothing is left to uninstall.

[claude]

Review of PR #1361. The supplied diff was truncated at 60 KB, so the tail of SessionUninstallCoordinatorTests was not reviewed.

I found no blocking correctness issues in what I could see. The owner-watch handler swap, the shutdown-fallback cancellation, the sweep's SIGTERM-only handling for runtime workers, and the unverified-on-cancel path all look consistent. I have two minor notes:

1. SessionWorkerProcess.belongsToUninstall compares executable.hasPrefix(app.standardizedFileURL.path + "/"). The sweep passes Bundle.main.bundleURL, and the kernel reports the resolved executable path. If the install path involves a symlink (for example /Applications reached through a firmlink or alias), the prefix check could miss the app's own workers. They would then be treated as owned by a live separate install and left running. Consider resolving symlinks on app (resolvingSymlinksInPath()) before comparing.
2. SessionWorkerSweep.stopAll signals by raw PID taken from an earlier listing, so a PID reused between the listing and the SIGKILL could hit an unrelated process. The window is about 100 ms and only affects capability probes, so this is low risk. Re-checking the executable before SIGKILL would close it.

The new tests cover the main paths: the cancelled-wait signal, the timeout, handler-swap ordering, the sweep's kill and grace behavior, and the unverified results.

[claude]

Review of PR 1361 (diff truncated at 60KB; the tail of SessionUninstallCoordinatorTests was not reviewed).

Findings

1. Probe mode keeps the owner-watch at 0.25s but runtime mode drops to 2s, and the _exit(0) startup handler can fire while the runtime is mid-start. In SessionRuntimeWorker, the watch is created before worker.start(...) and its handler is only swapped after currentProcessPermissionCapabilities() and other awaits. If the owner dies in that window, _exit(0) runs. This is fine while nothing is held, but check that no output or Caps mapping is acquired earlier than the setHandler call, such as inside the #if experimental block or the awaited capability calls.

2. SessionShutdownFallback.arm returns true when already armed, so a second owner verdict re-posts retire to the run loop. This is harmless because retire guards on finished, but the doc says "once". Consider returning early to avoid redundant work.

3. Sweep belongsToUninstall treats a worker as an orphan whenever the owner's executable differs from the worker's. If the owner app was replaced in place, for example by a Sparkle update that re-signs the same path, the executable path still matches, so this is OK. A running dev build with a different path whose worker was launched by a different-path owner is handled correctly. No change needed, but this relies on KeyPath's p_comm being exactly "KeyPath" (command(of:) == "KeyPath"). p_comm is truncated to 16 chars and would break if the executable were ever renamed.

4. Final sweep failure after trashing returns success: false with no resume() and after the app is gone. The message is accurate, but callers that treat success == false as "nothing changed" may offer a retry that cannot work. Confirm the wizard UI shows failureReason verbatim and does not re-enable the uninstall button as if the removal had not happened.

5. Test coverage: FakeProcesses.signals is read in tests without the lock (fake.signals.isEmpty). It is a data race under strict concurrency, though it only runs after the sweep completes. Locked is also declared private separately in two test files, which is fine but duplicated.

No blocking correctness bugs found in the supplied portion.

[claude]

Review (diff truncated at 60KB; the tail of SessionUninstallCoordinatorTests was not reviewed).

Findings:

1. SessionOwnerWatch.swift – retain cycle, deliberate but worth confirming. The handler runs under lock and calls worker?.retire/arm only; fine. But setHandler called from the main thread blocks if a verdict is mid-delivery. The old handler is _exit(0), so it never returns and the lock is never released. That means setHandler would block forever, but the process is exiting anyway, so it's harmless.

2. SessionRuntimeWorker – _exit(1) fallback after 30s. If retire runs but finish is slow (for example, a Caps restore takes more than 30s), shutdownBegins() cancels the fallback, so this is fine. If the run loop is wedged, the Caps journal is left for recovery, as documented. No change needed.

3. SessionWorkerSweep.stopAll – SIGTERM'd runtime workers are not re-signalled and PIDs can be reused. remaining is a PID list from the process table. If a worker exits and its PID is reused by another KeyPath worker, termed suppresses the SIGTERM. The window is tiny, so this is low severity.

4. currentUserKeyPathProcesses – p_comm match is limited to 16 chars and is "KeyPath". This is OK for the app binary, but a renamed bundle executable would be missed (hasSuffix(".app/Contents/MacOS/KeyPath") assumes the same name). Acceptable.

5. Uninstall final sweep returns success: false after the app is already trashed. Callers that treat success: false as 'nothing changed' (the UI wording aside) may offer a retry that cannot work, because the app is gone. Make sure the UI path handles "KeyPath was removed, but ..." without re-running.

6. Test coverage. Coverage is good (parsing, scoping, cancellation, retry, in-flight handler swap). Missing: a test that SessionRuntimeWorker actually installs the owner watch and replaces the handler (it is only exercised manually), and the live sysctl ENOMEM retry loop.

No blocking correctness issues found in the reviewed portion.

[claude]

Review of PR #1361 (the diff was truncated at 60 KB, so the tail of SessionUninstallCoordinatorTests was not reviewed.)

I found no blocking correctness issues. Both the owner-watch hard exit and the uninstall sweep look sound. Specifics I checked:

• awaitCapabilityReport abandons the probe on timeout, cancellation and thrown error. It skips the signal only after a report was read.
• SessionOwnerWatch.setHandler takes the same lock the verdict is delivered under, so the old _exit(0) handler cannot run after the swap.
• The sweep never SIGKILLs a runtime worker, it retries one unreadable process listing, and it returns .unverified on cancellation.
• Zombies are skipped and the kern.argmax buffer is bounded.

Minor points, none blocking:

1. Final-sweep failure after removal. In SessionUninstallCoordinator, when the post-trash sweep is not .noneRunning, the result is success: false even though the app and config are already gone. A caller or UI that treats success == false as "retry" or "nothing changed" would mislead the user. The message does say KeyPath was removed. Check that the wizard UI doesn't offer a retry that can't work, or consider a distinct result/log flag.
2. Default stopSessionWorkers. The init default { .noneRunning } silently skips the new safety check for any caller that doesn't pass it. Only configureWizardDependencies passes it today. Consider making it non-defaulted, with tests passing it explicitly.
3. Process-name filter. command(of:) == "KeyPath" relies on p_comm. This is fine for the current executable name but silently finds no workers if the executable is ever renamed. A comment, or a test pinning the name to the bundle's CFBundleExecutable, would guard it.
4. Orphan-kill window. ownerExitShutdownGrace is 30 s, and the fallback _exit(1) skips the Caps restore. The comment says the journal survives for recovery. A test that the journal is recovered after the hard exit would be worth adding if none exists.

Test coverage for the new paths (watch verdicts, handler swap, sweep scope, PID reuse, malformed argv) looks good.

[claude]

Review (diff truncated at 60KB; later test file portion not reviewed)

Overall the change looks solid. A few actionable notes:

1. SessionWorkerSweep.stopAll – SIGTERM-only runtime grace vs. owner-exit fallback: runtime workers get 10s to exit, but the worker's own owner-exit fallback grace is 30s (ownerExitShutdownGrace). A slow-but-healthy Caps restore can legitimately exceed 10s and uninstall will report stillRunning and abort (safe, just a UX failure). Consider aligning or documenting.

2. PID reuse window in belongsToUninstall: an owner PID reused by a different KeyPath process running the same executable path would make an orphan look owned and be skipped. Narrow, but the check is path-only; comparing process start time would close it.

3. SessionOwnerWatch handler lock: setHandler blocks on lock while the handler runs. Current handlers return immediately (_exit / run-loop post), so fine, but _exit(0) while holding the lock is OK only because the process dies; worth a comment if handlers ever grow.

4. Probe deadline 15s vs. kill: _exit(0) on deadline means a probe could exit with no report; the parent already treats that as nil, so fine.

5. Tests: FakeProcesses.arrivals/clock are mutated without the lock in places (arrivals set before use, so OK). No test covers the final post-trash sweep returning .stillRunning/.unverified in the visible portion (may be in the truncated part).

No blocking issues found.