Skip to content

desktop: fix two managed-SSH-spawn bugs that break every fresh remote backend spawn - #96061

Closed
SmelterLabs wants to merge 1 commit into
NousResearch:mainfrom
SmelterLabs:fix/remote-spawn-quoting-posix
Closed

desktop: fix two managed-SSH-spawn bugs that break every fresh remote backend spawn#96061
SmelterLabs wants to merge 1 commit into
NousResearch:mainfrom
SmelterLabs:fix/remote-spawn-quoting-posix

Conversation

@SmelterLabs

Copy link
Copy Markdown
Contributor

What

Two bugs in the managed SSH spawn engine (apps/desktop/electron/remote-lifecycle.ts) that together break every fresh remote backend spawn on a stock Ubuntu host. Found while running the current tip against a real remote (Ubuntu Server 24.04, dash as /bin/sh); the lockfile-reuse path masks both bugs for already-running backends, which is likely why they went unnoticed.

Bug 1 — double shell-quoting in the spawn payload (and the stale reaper)

expandRemotePath() already returns a shell-quoted fragment like "$HOME"'/.hermes/...'. buildSpawnCommand() wrapped that output in shq() again, so the payload's reservation / lock / owner_file variables store the quote characters literally. Every mkdir "$reservation" then fails forever, and the reservation loop spins ~5 minutes per attempt while holding the box-global update mutex — with parallel spawn attempts queueing behind it on the flock. Observed live: 20+ payloads stacked on the mutex, every desktop connect timing out at 20s while the abandoned remote payloads kept spinning.

The same double-quoting sits in buildOwnedStaleTerminationCommand()'s identity guards, so every stale-backend reap REFUSEs on path mismatch.

Fix: embed expandRemotePath() output raw (it is already quoted), with comments marking the contract.

Bug 2 — bashism in lockfile publication

The payload runs under plain sh (dash on Ubuntu), but published the lockfile via ${var//__PID__/$child} — bash-only substitution. dash aborts the whole script on it after the serve child was spawned; the client sees an unknown failure, runs its error cleanup which deletes the session token file, and the just-booting serve dies on the missing token — orphaning one serve process per attempt.

Fix: POSIX sed substitution.

Tests

Two regression tests added to remote-lifecycle.test.ts:

  • payload variables must keep $HOME expandable (no re-quoting after expandRemotePath())
  • lockfile publication must be POSIX sh (no ${var//}; pid substituted via sed)

Both fail against the previous code; all 89 remote-lifecycle tests pass with the fix. Live-verified end to end against an Ubuntu 24.04 remote: fresh backend spawn in ~3s with correct backend.lock.json publication, and kill-recovery (dispatch probe -> retire -> respawn) in 12s.

🤖 Generated with Claude Code

… backend

1. Quoting: the spawn payload wrapped expandRemotePath() output -- already
   a shell-quoted fragment like "$HOME"'/...' -- in shq() again, so the
   reservation/lock/owner_file variables hold the quote characters
   literally and every mkdir "$reservation" fails forever (~5 min per
   attempt spinning in the reservation loop while holding the box-global
   update mutex; queued spawns starve behind it). The same double quoting
   sits in the stale-reaper identity guards, making every reap REFUSE.
   The lockfile-reuse path masks the bug for existing backends, so it
   only bites on fresh spawns.
2. Bashism: lockfile publication used ${var//__PID__/$child} -- bash-only
   substitution in a payload run under plain sh (dash on Ubuntu), which
   aborts the script AFTER the serve was spawned. The client then saw an
   unknown failure, ran its error cleanup (deleting the token file), and
   the just-booted serve died on the missing token -- orphaning one serve
   per attempt. Replaced with a POSIX sed substitution.

Adds two regression tests: payload variables must keep $HOME expandable
(no re-quoting), and the pid substitution must be POSIX sh. Both fail
against the previous code; all 89 remote-lifecycle tests pass with the
fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #96084 (rebase merge, commit pending confirmation below) — your commit cherry-picked onto current main with your authorship preserved. Both diagnoses were exactly right, and the turnaround was outstanding: the fresh-spawn path shipped in #95942 a few hours earlier with precisely the gap you hit (the engine suites drive scripted transports; the reuse path masks both bugs; only a live fresh spawn against a real remote exposes them — the test you ran). We added a dash-level E2E on top: the real buildSpawnCommand payload now executes under dash through the full reservation/mutex/publication sequence, and the pre-fix payload provably aborts. Third landed contribution of yours in this campaign — thank you.

@teknium1 teknium1 closed this Aug 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Final merge SHA: beb212d (#96084, admin-merged on the pre-authorized re-green — the rebase was conflict-only after a GitHub rate-limit window stalled the PR head sync; content identical to the CI-green 542736a, which passed 29 checks plus our dash E2E and your Ubuntu round-trip).

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