Conversation
…SH spawn path
expandRemotePath() returns an already-quoted shell fragment
("$HOME"'/path'), but three call sites wrapped its output in shq()
again: the withRemoteUpdateMutex python argv, the reservation/lock/
owner_file assignments in buildSpawnCommand, and the identity values in
buildOwnedStaleTerminationCommand.
The remote shell strips only one quoting layer, so python received a
mutex path with literal quote characters in it (creating a directory
literally named ' in $HOME), and the payload's mkdir "$reservation"
loop spun on a path that can never exist. Every Desktop SSH backend
spawn hung until the connect timeout, retried, and left an orphaned
flock queue behind; stale-owner cleanup always printed REFUSED for the
same reason.
The regression test parses the composed command with a real sh — the
same parse the remote login shell performs — and requires the mutex
path and the payload's reservation paths to come out fully expanded.
alt-glitch
added
type/bug
…and stale-identity sites independently; this branch now carries only the remaining withRemoteUpdateMutex shq(mutexPath) fix and the real-sh-parse regression test
|
Rebased via merge after beb212d landed — that commit independently fixed two of the three double-quoting sites this PR covered (the payload's reservation/lock/owner_file assignments and the stale-termination identity values). The remaining delta here is the third site: |
|
Heads-up on overlap: this fixes the same Whichever one lands fixes the quoting fault, so no argument from me either way — but for whoever reviews: #96260 covers this same one-line mutex change plus two adjacent correctness defects in the same spawn chain that the one-line fix alone leaves behind:
It also pins the contract with argv-level regression tests (asserting the exact argv the remote python receives, under dash), which is the gap that let this site regress silently in the first place — the existing Happy to rebase/adjust if a maintainer prefers to land a minimal one first. |
alt-glitch
added
area/install-update
Re-triaged after the rebase: no longer marking this a duplicate of #96084. The remaining |
|
Salvaged into #104206 — your commit Thread points, addressed:
The competing #99189 / #96260 fix the same line; #96260's separate lockfile-pid concern stays open there. Auto-merge (rebase) is armed. Thanks for finding it first. |
`apps/desktop/'` is not a real path. It is a literal single-quote directory
holding a full absolute path as nested subdirectories:
apps/desktop/'/var/folders/5h/.../hermes-update-mutex-LMF9y5/home/.hermes-update-in-progress.mutex'
Both files are 0 bytes, nothing in the tree references them, and the leading
and trailing `'` are part of the filenames. They are the fingerprint of the
double-quoted mutex path in `withRemoteUpdateMutex()`: the path reaches Python
with its shell quotes still attached, so it is treated as CWD-relative and
`os.makedirs()` materialises the whole absolute path under whatever directory
the process happened to be in. Running
`apps/desktop/electron/remote-lifecycle.test.ts` from `apps/desktop`
reproduces it on the spot.
They were swept in by a `git add .` in 3662057, an unrelated menu-label
commit.
This only removes the committed artifact. The generator is a separate concern
already covered by open PRs (#99189, #96187, #96260) and issues (#99133,
#96212, #96188); those repair the quoting but none of them deletes these two
files, so the litter would survive whichever one lands.
Review follow-ups on the #96187 cherry-pick: skip on win32 like the sibling tests that shell out; reuse the file's `exec` helper; one temp root so a failing second mkdtemp cannot leak the shim dir; quote the shim's redirect target; guard the payload-prefix sentinel so a renamed loop marker can never make the test execute the real spawn payload; drop the lockMetadata fields the assertions never read. Move the mutexPath contract into withRemoteUpdateMutex's doc comment instead of a third inline restatement.
4 participants
Symptom
Since the managed SSH remote update engine landed, every Desktop SSH backend spawn on a POSIX remote hangs ~20s and dies, in a loop: the Bots view shows "Could not check 's Bot Chat registry (Timed out connecting to profile …)", and each retry strands an orphaned flock-queue python on the remote. A telltale artifact is a directory literally named
'in the remote $HOME, containing a quote-mangled copy of the.hermes-update-in-progress.mutexpath.Root cause
expandRemotePath()returns an already-quoted shell fragment ("$HOME"'/path'), meant for direct interpolation into the remote command string. Three call sites wrapped that fragment inshq()a second time:withRemoteUpdateMutex—shq(mutexPath)in thepython3 -cargvbuildSpawnCommand— thereservation=/lock=/owner_file=payload assignmentsbuildOwnedStaleTerminationCommand— the identity values (path=/home=/token=)The remote shell strips only one layer, so python receives a mutex path with literal quote characters (→ the
'shadow directory), the payload'swhile ! mkdir "$reservation"spins forever on a literal"$HOME"'…'path (→ the 20s hang + orphan queue), and the stale-owner identity check can never match live argv (→ cleanup always REFUSED, so the orphans accumulate).Fix
Interpolate the fragments raw at those three sites, exactly as the neighboring
mkdir -p "$(dirname …)"/rm -fcall sites already do. No behavior change beyond correct quoting.Regression test
The new test parses the composed spawn command with a real
sh(python3 shimmed on PATH to capture argv) — the same single parse the remote login shell performs — then evaluates the payload's assignment prefix and requires the mutex path and reservation/lock/owner paths to come out fully expanded. It fails on the previous code with:and passes with the fix.