Skip to content

fix(desktop): finish POSIX SSH spawn publication - #96103

Closed
Wenfengcheng wants to merge 1 commit into
NousResearch:salv/96061-spawn-fixesfrom
Wenfengcheng:fix/96084-spawn-pid-mutex-followup
Closed

fix(desktop): finish POSIX SSH spawn publication#96103
Wenfengcheng wants to merge 1 commit into
NousResearch:salv/96061-spawn-fixesfrom
Wenfengcheng:fix/96084-spawn-pid-mutex-followup

Conversation

@Wenfengcheng

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #96084, targeting its salv/96061-spawn-fixes branch rather than main so @SmelterLabs' salvaged commit and authorship remain intact.

This closes three remaining gaps in the same managed-SSH spawn path:

  • pass the already-expanded update-mutex path directly to Python instead of shell-quoting it a second time;
  • capture only the live backend PID instead of combining the short-lived setsid wrapper PID with the backend PID;
  • preserve the old global PID-placeholder semantics by using sed .../g.

Related issue: #96024. This PR deliberately does not duplicate #96084's ownership-variable or stale-reaper changes.

Root Cause

expandRemotePath() returns a complete shell word. withRemoteUpdateMutex() still wrapped that word in shq(), so Python could receive literal quote characters instead of the expanded mutex path.

The detached launch also emitted $! twice: the outer setsid ... & echo $! reported a wrapper that had already exited, while the inner shell reported the live backend. In 20/20 WSL probes, child contained both PID lines. That breaks signal handling and lockfile publication.

Finally, #96084's POSIX replacement used first-match sed. Because lock metadata precedes the final pid field, an earlier __PID__ occurrence could be replaced while leaving pid: "__PID__". /g preserves the previous Bash global-replacement behavior.

Fix

  • remove only the second shq(mutexPath);
  • remove only the outer & echo $!, retaining the inner backend echo $!, setsid/nohup, redirections, and mutex-FD close;
  • add the g flag to PID substitution;
  • add structural regressions plus a POSIX full-spawn test that verifies mutex creation, one live PID, matching lockfile PID, global placeholder replacement, and reservation cleanup.

How to Verify

cd apps/desktop
npx vitest run --config vitest.config.ts electron/remote-lifecycle.test.ts
npm run typecheck
npx eslint electron/remote-lifecycle.ts electron/remote-lifecycle.test.ts

The three focused tests failed against parent 542736a891 and pass at 994746f5d2.

Test Plan

  • Parent RED: all three A/D/C-global regressions fail on fix(desktop): fresh managed-SSH spawns no longer wedge on dash or double-quoted paths (salvage #96061) #96084 head
  • remote-lifecycle.test.ts: 91 passed, 3 platform-skipped
  • Desktop TypeScript checks: all three projects passed
  • Changed-file ESLint passed
  • git diff --check and static secret/debug/conflict scan passed
  • WSL2 Ubuntu /bin/sh (dash) full ownership payload passed:
    • one live FULL_PID
    • FULL_LOCK_PID matched it
    • expected serve --isolated argv
    • mutex created at the expanded $HOME path
    • reservation released

Platforms tested: Windows 11/Git Bash and WSL2 Ubuntu/dash. I did not independently repeat #96084's external macOS-to-Ubuntu SSH round trip.

Risk Assessment

Medium-low — this touches process lifecycle and shell construction, but the production diff is three narrow lines. Mutex/flock ownership, descriptor closure, reservation protocol, atomic lock publication, token handling, authentication, timeout, and stale-reaper behavior are unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/ssh SSH remote execution comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants