[mstomar125]

What changed

• Up-to-date check without file size. extract_sidecar_from and kill_stale_daemon_sync compare a stamp written atomically to ~/.nodespace/bin/.<name>.source after extraction. It holds the bundled file's size, mtime and SHA-256 and the installed copy's own size and mtime. The copy is current only if the installed copy still has the recorded size/mtime (a foreign binary or an older app's overwrite is replaced) and the bundle is the recorded one; the bundle is hashed only when its size or mtime changed. Extraction also requires codesign --verify to pass. The ad-hoc re-sign making the copy smaller no longer triggers a SIGKILL, re-extract and restart on every launch. A changed bundled sidecar is still re-extracted and the stale daemon replaced; a copy with no stamp (pre-fix install) is replaced once.
• .pkg registration is authoritative. When /Library/LaunchAgents/<label>.plist exists, bring_up_daemon extracts nothing and writes no per-user plist; the stale refusal marker is cleared before the job is started (so an old incompatible-database refusal cannot end the wait at once); if the daemon does not answer, a loaded job is kickstarted (-k only when the daemon did not answer at all, i.e. a wedged job), otherwise the label is enabled and the plist bootstrapped. A failed bootstrap counts as success if the daemon then answers (concurrent launch). kill_stale_daemon_sync does nothing in that case.
• Signal targets. kill_running_daemon and kill_stale_daemon_sync signal a process only if it is owned by the current uid and its executable path equals exactly one of: ~/.nodespace/bin/<name>, /usr/local/bin/<name>, or the program of the loaded launchd job (launchctl print gui/<uid>/<label>, behind a fakeable function). Name-only matches, similarly named binaries elsewhere and other users' processes never match.

Not covered

• signal_daemon_to_stop does not exist at this base. App Quit / window close deliberately does not stop the daemon (an existing test asserts it); the tray Quit is daemon-side. So "App Quit stops the daemon the registration runs" is not changed here.
• Issue item 4 (deleting other users' per-user plists) is not implemented.
• ADR-084 text is a separate docs change.
• No live two-launch PID check on a real .pkg install was done.

Testing

• cargo test -p nodespace-app-lib --lib daemon_setup: 98 passed, including machine_wide_start_tests through a faked launchctl (enable then bootstrap, kickstart with and without -k, bootstrap race, marker cleared first). New: sidecar_source_stamp_tests (smaller copy of an unchanged source is current; changed source, a copy replaced behind the stamp's back, missing stamp or copy is stale; an unchanged size/mtime is not rehashed; real codesign test with signed /bin/sleep: re-sign changes size, second extraction is a no-op, a different binary is replaced) and daemon_signal_target_tests (both install locations, job program, look-alike paths, other uid, unreadable process, launchctl print parsing). No daemon is spawned or signalled.
• cargo clippy -p nodespace-app-lib --all-targets -- -D warnings and cargo fmt: clean.

Refs #3835

🤖 Generated with Claude Code

[mstomar125]

Review (initial) - #3853

Verdict: REQUEST CHANGES

Reviewed against #3835 (Refs, the PR lists what remains). cargo test -p nodespace-app-lib --lib daemon_setup through the slot: 88 passed, 0 failed. Findings come from code reading of packages/desktop-app/app-lib/src/daemon_setup.rs; no live two-launch or launchctl run was done (the PR says the same).

Summary

The stamp replaces the size test cleanly, the crash ordering is right, and the signal matcher is conservative (uid and exact path, fail-safe on unreadable). Two real problems: the .pkg path skips clear_stale_marker, which breaks the issue's own incompatible-database scenario, and the stamp does not describe the installed copy, so a copy replaced by something else is trusted forever.

Findings

• Blocking daemon_setup.rs:1163-1200 (bring_up_machine_wide_daemon, reached from :1024-1026): the early return skips crate::incompatible_database::clear_stale_marker() (:1117). start_attempt stops waiting as soon as refusal_recorded is true (:970), and clear_stale_marker's doc says that is exactly why it must run before every start. On a .pkg install after a refusal (the case the issue names, and the Reset/Retry flow that restarts via ensure_daemon_running), the stale marker survives, the wait ends at once and the UI reports incompatible_database for a daemon that has just been restarted successfully. Call clear_stale_marker() before kickstart/bootstrap (only when the daemon is not already healthy).

• Blocking :1497-1560 / :1477-1493 (stamp vs copy): the stamp records only the bundled source, never the installed copy. If something else replaces ~/.nodespace/bin/<name> without touching the stamp (an older app build that still uses the size test and its own rename, a manual copy, a dev cp), the next launch sees stamp == bundle and codesign --verify passing and keeps the foreign binary; kill_stale_daemon_sync (:161-200) does the same. Downgrade-then-upgrade is the concrete case: old app overwrites the copy (size differs), new app finds the matching stamp and never restores its own. Record the installed copy's size (and mtime or hash) in the stamp and require it to match in both extract_sidecar_from and installed_matches_source.

• Non-blocking :1555-1558: ordering is right for a crash (remove stamp, rename, write stamp: a crash leaves "no stamp", re-extract once; a partial stamp does not equal the full one, same result). Two app instances with different bundles can still end with stamp A over binary B (A rename, B rename, B stamp, A stamp); same bundle is harmless (distinct .tmp-<pid> names). Narrow, and fixed by the copy-identity check above. Write the stamp via temp + rename to make it atomic.

• Non-blocking :1555: remove_file(&stamp_path) result is ignored; if it fails and the later write fails too, an old stamp survives next to a new binary. Propagate or treat as fatal.

• Non-blocking :1184: kickstart without -k does nothing to a loaded job that is running but wedged/unresponsive, so the app waits out DAEMON_START_WAIT and gives up. The per-user path already uses kickstart -k (:2005). Also no launchctl enable before bootstrap (the per-user path has it, bootstrap_launchd_agent), so a disabled label fails with only the stderr text. Concurrent launches racing bootstrap return an error (EIO 5) from the loser, which bring_up_daemon propagates as a failure; treat "already loaded" as success by re-checking the socket.

• Non-blocking :1024-1026: a per-user plist/job from earlier launches under the same label is neither removed nor bootout-ed (issue item 4, declared not covered). launchctl print gui/<uid>/<label> then describes the per-user job, so kickstart starts the ad-hoc copy and the program it reports is ~/.nodespace/bin/nodespaced. The two-registrations state persists on machines that already have it; a follow-up should be filed.

• Non-blocking :288-304, :161-200 (matcher scope): /usr/local/bin/<name> is now always an allowed target, even when no machine-wide plist exists. A daemon running from there that holds the socket (manual/brew install) with a stale app copy gets SIGKILLed by kill_stale_daemon_sync and SIGTERMed by kill_running_daemon, which did not happen before. Probably intended; state it in the PR or gate /usr/local/bin on the machine-wide plist existing.

• Non-blocking (matcher, verified by reading) :359-373: signals only when ps -o uid=,args= gives euid == geteuid() and the first whitespace token equals one of the paths exactly. Checked: another user's process (uid mismatch), look-alike paths, unreadable/gone process (empty or unparsable output returns false), case differences on APFS (no match, fail safe), relative argv0 (no match). Limits, all fail-safe or same-uid: argv0 is not the kernel's exe path (a same-uid process can set argv0, but must also hold the daemon's socket); a home directory containing a space truncates the first token so the daemon is never matched (existing behaviour, but it silently disables the stale-restart; prefer proc_pidpath or lsof -p ... txt); TOCTOU between ps and kill (PID reuse window of a few ms, same as before). Replaced-on-disk binaries keep their path in argv0, so they still match, which is wanted.

• Non-blocking :1447-1470, :161-200: kill_stale_daemon_sync runs before the gRPC client is managed and now hashes the ~75 MB daemon synchronously on every launch (and extract_sidecar_from hashes it again, plus the CLI). Release builds are fast (about 0.1-0.3 s); debug builds are not. Consider caching the size+mtime of the bundle in the stamp and hashing only when those change.

• Non-blocking dev builds / first launch: no stamp or no copy means extract once (correct). Ad-hoc/linker-signed dev sources now re-extract on every rebuild even at the same size, an improvement over the size test. Pre-fix installs are replaced and the daemon restarted once on upgrade, as the PR says. std::fs::copy keeps the source mode so the executable bit is preserved; the stamp is written with the default umask (0644 in the user's own dir), fine.

• Non-blocking (tests) the kickstart/bootstrap branch and bring_up_machine_wide_daemon have no test; the fake-able launchctl_print is only used for the matcher. A test that clear_stale_marker runs on that path would have caught the first finding.

Requirements (#3835)

• Size-only check replaced by a source stamp (extract_sidecar_from, kill_stale_daemon_sync), subject to the copy-identity finding.
• [~] No per-user plist while /Library/LaunchAgents/<label>.plist exists: implemented; the pkg start path loses the marker clear.
• [~] Quit/stale kill targets the registered binary: matcher done; signal_daemon_to_stop not in this base (PR explains).
• ADR-084 text: separate docs change. Existing per-user plists (item 4): not done.

[mstomar125]

Re-Review - #3853 (commits d6c5427..90308be; only 90308be is new since my earlier review)

Verdict: APPROVE WITH NITS

cargo test -p nodespace-app-lib --lib daemon_setup through the slot: 98 passed, 0 failed. Findings are from code reading; no live launchctl or two-launch run.

Earlier findings

1. Marker clearing (was blocking): resolved. bring_up_machine_wide_daemon (daemon_setup.rs:1184) returns at :1189-1192 when the daemon is Healthy, before start_machine_wide_job runs clear_marker() (:1232), so a healthy running daemon is never touched. In every other case the marker is cleared before print/kickstart/enable/bootstrap. start_attempt then waits through answering_daemon, whose refused() check only fires after a first socket look; a daemon that restarts and answers returns ThisApp first (:839-847), so incompatible_database is no longer reported for a daemon that restarted fine. A daemon that restarts and refuses again rewrites the marker and the wait ends correctly. The path is also behind evict_other_product_daemon, unchanged.
2. Stamp identifies the installed copy (was blocking): resolved. The stamp holds the source size/mtime/sha and the installed size/mtime, and installed_matches_source (:1607) requires the installed copy's size+mtime to equal the recorded pair, in both extract_sidecar_from and kill_stale_daemon_sync. The downgrade-then-upgrade case is covered: an older app's rename gives the copy a new mtime, so the stamp mismatches and the copy is restored. Stamp ordering is right: it is written after resign_binary/clear_quarantine on the temp file and after the rename, and it stats dest (the renamed file keeps the temp file's mtime), so codesign cannot invalidate it. Later clear_quarantine(dest) is an xattr change and does not touch mtime. Clock changes do not matter (mtime is compared for equality only, never against "now"); a coarse-mtime filesystem is stable across re-reads. A tool that rewrites the mtime (cp -p, touch, restore from backup) causes one re-extract plus one daemon restart, then settles. Spotlight and Time Machine do not change mtime.
3. Hash skip: works as described. The bundle is hashed only when its size or mtime differs from the stamp (:1623-1627). A source replaced with identical size AND mtime but different content is trusted. That is a documented-limit case (a build tool or installer preserving mtime and size); worth one sentence in the doc comment of installed_matches_source.
4. launchctl logic: correct. Domain is gui/<uid> with target gui/<uid>/<label> (:1228-1229, uid from get_uid()). -k is used only when wedged (status != NotRunning after the existing 5 s Starting retry, :1200), i.e. the socket exists and accepts nothing (connect timeout); a stale socket file with no listener is NotRunning, so no -k. A Healthy daemon returns before this function, so no path kills a healthy daemon. Unloaded: enable (failure only logged), then bootstrap; a failed bootstrap is success only if wait_for_daemon(..., 5 s) sees Healthy, otherwise it bails with stderr. All waits are bounded, no unbounded loops (wait_for_daemon has a deadline and a 250 ms poll). Tests cover enable-then-bootstrap, kickstart with/without -k, bootstrap race success/failure, and marker-before-start.
5. Process matcher: unchanged. The only change under it is a comment on daemon_program_paths. may_signal (:346) still requires euid == process uid and argv0 exactly equal to an allowed path; /usr/local/bin/<name> is allowed as an install location and the process must still hold this user's socket.

New findings

• Non-blocking :1623-1627 (installed_matches_source): when the bundle's mtime changed but its hash is unchanged (typical after an app update that ships the same daemon), the function returns true without rewriting the stamp, so the ~75 MB bundle is re-hashed on every launch (twice: kill_stale_daemon_sync and extract_sidecar_from) until the daemon next changes. Refresh the stamp's source mtime on a hash match (the file is tiny; use the same temp+rename write).
• Non-blocking :1652-1658 (write_sidecar_stamp): if std::fs::write(&tmp, ..) fails part way, the .tmp-<pid> file is not removed (only the rename failure path cleans up). A crash between write and rename also leaves one. Harmless (never read), but it is the leak the code comments elsewhere try to avoid.
• Non-blocking :1200: wedged is true for any Starting result after 5 s, which on a loaded system could be a busy daemon with a full accept backlog rather than a wedged one; kickstart -k would then kill it. Low likelihood, and the old behaviour was to give up, so this is a trade-off to state in the comment, not a defect.
• Non-blocking (tests): start_machine_wide_job is tested, but bring_up_machine_wide_daemon itself (the Healthy early return that must not clear the marker or call launchctl) is not; the guarantee in item 1 rests on code reading. A small test taking the launchctl/clear functions as injected would pin it.
• Carried over, still open, non-blocking: per-user plists left by earlier launches under the same label are not removed (issue item 4, declared out of scope); /usr/local/bin always allowed in the matcher (now documented at :293); home directory with a space truncates argv0 in the matcher.

Requirements (#3835)

• Size-only check replaced by a source+installed-copy stamp.
• No per-user plist while the machine-wide plist exists; the pkg start path clears the stale marker and hardens kickstart/bootstrap.
• Stale kill targets only same-user, exact-path daemons.
• Existing per-user plists and the ADR text: follow-ups, as the PR states.