[devswha]

Problem

Updating from 0.4.13 in the app (Settings > Updates) leaves 0.4.14 without its Rust PTY host. The
update is built by the updater that is running, 0.4.13's, and that one has no
scripts/pty-sidecar.ts install step (0.4.14's updater added it). The new release's own supervisor
takes over after the install, but nothing fetches the host for the release it runs.

Found by the 0.4.13 -> 0.4.14 upgrade check, on Linux (a clean ubuntu:24.04 container) and on macOS
(the test MacBook Air, in a fresh /tmp HOME with its own herdr). Both runs installed 0.4.13 with its
one-line installer, then clicked Update and restart in Settings:

• the release dir (~/.config/herdr-web-ui/updates/<hash>/release-*) has no bin/;
• server.log: Rust PTY host unavailable (.../release-*/crates/pty-sidecar/target/release/ovis-pty-sidecar: ENOENT)
  and PTY host: Node;
• each terminal attach runs ~/.local/bin/node .../release-*/server/pty/pty-host.mjs. That works
  only because 0.4.13's installer put a private Node in ~/.local/share/herdr-web-ui/node;
• with that Node moved aside and the plugin restarted: PTY host: none (no Rust host and no Node for the sidecar), so panes are mirrored, and /api/health answers terminal_attach: false.

Everything else in that update passed: the new release's supervisor takes over under the 0.4.13
launcher, HERDR_WEB_TOKEN and HERDR_WEB_TELEMETRY keep working, and rerunning the one-line
installer moves the plugin to devswha.ovis and installs a checked host.

Change

server/supervisor.ts: once the supervisor runs a saved release that has
scripts/pty-sidecar.ts but no host record (bin/ovis-pty-sidecar.json), it runs that release's
scripts/pty-sidecar.ts install in the background, with the updater's own bounded runCommand and
the same 120 s limit as the update step. The install keeps every check it already makes: it takes
the manifest of the release named by package.json, compares the SHA-256, and checks that the
binary runs. A bridge on the Node sidecar picks the host up at its next attach, since
rustSidecar() looks again for each session. A bridge that started with no PTY host at all picks
it up at its next start. Windows is skipped, because it has no Rust host.

The supervisor writes bin/ovis-pty-sidecar.attempt before each try. The next start tries again
only 6 hours after that mark, the same wait as the unshallow fetch (#1252). So a PC that cannot get
the host (offline, a release without one, a check that keeps failing) does not download again at
every restart. A supervisor that starts while an earlier try is still running leaves bin/ to that
try. The install also gets an AbortSignal that the supervisor's shutdown aborts, so runCommand
sends SIGTERM to the installer's process group and the installer does not outlive the supervisor.

This helps anyone who updates in the app from 0.4.13 or older straight to the release with this
fix. An install already on 0.4.14 gets the host when it installs the next release, since 0.4.14's
updater fetches it. For anyone on 0.4.14 now, rerunning the one-line installer fixes it at once.

Test

server/legacy-env.test.ts gets three cases. Each runs the real runSupervisor (and the real
runCommand) the way the 0.4.13 launcher starts it, on the 0.4.13-shaped fixture the neighbouring
tests use. The release's scripts/pty-sidecar.ts is a stand-in that records each run.

• "fetches the Rust PTY host that the 0.4.13 updater left out of the release": exactly one run, in
  the release, with install.
• "tries the Rust PTY host again only after its retry wait": a second start within the wait does not
  try. Once the mark is 7 hours old, the next start tries again.
• "stops its Rust PTY host install when the supervisor stops": the stand-in waits for SIGTERM, and
  SIGTERM to the supervisor reaches it.

All 11 tests in the file pass. Mutations: with the retry-wait check removed, the retry case fails.
With the abort on shutdown removed, the stop case fails with the install's stop never happened.
With the install call removed, the first case fails.

Real surface (Linux container, the release that 0.4.13's updater installed). This ran on the first
revision, before the retry mark and the abort were added; the tests above cover those two. With that
supervisor.ts dropped into the release and the plugin restarted, server.log shows
Ovis: installed the Rust PTY host of release v0.4.14 (sha256 ea09de03...). That hash matches the
release's SHA256SUMS entry for ovis-pty-sidecar-linux-x64. The next terminal attach in the
browser ran release-*/bin/ovis-pty-sidecar on the same bridge, with no restart. With Node then
moved aside and the plugin restarted, the bridge reported PTY host: Rust and terminal_attach: true. In the browser, typing, a resize (stty 37x145 -> 26x86), seq 1 200000 and a chat
round-trip all worked.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: devswha/ovis
• Branch: fix/rust-host-after-old-update
• File: .coderabbit.yaml
• Commit: e2732f6
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: a306f492-ad94-4da3-abd8-8054ec40a6e5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: devswha/ovis
• Branch: fix/rust-host-after-old-update
• File: .coderabbit.yaml
• Commit: ea9ab87
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: dc07c759-6d4e-463a-bcd8-45cf411e439f

Commits

Reviewing files that changed from the base of the PR and between ae1b0f2 and ea9ab87.

Files selected for processing: 3

• changelog.d/rust-host-after-old-update.md
• server/legacy-env.test.ts
• server/supervisor.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Summary

Summary by CodeRabbit

• New Features
  • When updating from version 0.4.13 or earlier, the app now installs the Rust terminal host in the background. Once available, new terminals use it.
• Bug Fixes
  • Background installation retries are spaced out, and installation shuts down cleanly when the app stops. If installation fails, terminals can continue using the existing host.

For eligible active releases, the supervisor checks for a missing Rust PTY host and runs its installer in the background. It records attempts, retries after six hours, and aborts and waits for the installation during shutdown.

Changes: Legacy Rust PTY host installation

Layer / File(s)	Summary
Detect and install the missing host server/supervisor.ts, server/legacy-env.test.ts, changelog.d/rust-host-after-old-update.md	The supervisor runs the installer for eligible releases that lack the host, records attempts, and skips attempts made within the prior six hours. Tests cover installation and retry timing. The changelog describes the legacy update behavior.
Stop and await installation server/supervisor.ts, server/legacy-env.test.ts	Shutdown aborts the installation and waits for it and the bridge before releasing the lock. Tests cover termination and child-process cleanup.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

• devswha/ovis#1909: Adds Rust PTY host support to the release and updater; this PR handles the legacy-release case where the host is absent.
• devswha/ovis#1859: Adds the installer invoked here and the host assets it installs.
• devswha/ovis#1908: Uses the installed Rust PTY host by default on Linux and macOS; this PR installs the host for legacy releases that lack it.

Merge Risk: Minimal · up to ea9ab

No actionable merge-blocking risk is established for legacy Rust PTY host installation or supervisor shutdown.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly identifies the primary change: fetching the Rust PTY host omitted by updates from version 0.4.13.
Description check		The description clearly explains the problem, implementation, behavior, and validation results. It uses ## Problem and ## Test instead of the template's ## Validation heading, but the required informa…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description clearly explains the problem, implementation, behavior, and validation results. It uses ## Problem and ## Test instead of the template's ## Validation heading, but the required information is present and detailed.

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[devswha]

Merge pass gate review of 582ed09 (another model; programming + remove-ai-slops applied directly): two restart-safety blockers. The happy path works.

1. A failed install repeats on every supervisor start (server/supervisor.ts:24-30, :162)

• The trigger checks only that the script exists and the record is absent. It keeps no attempt count and no retry deadline.
• Probing the real supervisor with the record absent gave 3 install calls from 3 starts.
• On a PC that can't get the host, every start tries again for up to 120 s: offline, a release without a host, or a hash or run check that keeps failing. A restart loop then means a loop of installs.
• Fix: keep a small marker beside the release, e.g. bin/ovis-pty-sidecar.attempt with the time and count. Retry only after a backoff, or once per release.

2. A restart can orphan an installer and start another (server/supervisor.ts:27, :140-146, server/updater.ts:78-105)

• The install runs detached, gets no cancel signal, and isn't stopped on shutdown.
• With the real runCommand, the installer was still alive after its parent exited. The 120 s limit was enforced by that parent, so it went too.
• After a restart the next supervisor can start a second install in the same bin/.
• Fix: tie the child to the supervisor's lifetime: an AbortSignal on shutdown, or a kill of the process group. Or guard bin/ with a lock file holding the pid, so a second installer waits or skips while the first is alive.

What holds

• Every feat(pty): ship the Rust PTY host with each release and install it checked #1859 check is kept: manifest, crate tree, SHA-256, a binary that runs, and the atomic publish.
• 9 legacy-env and 14 installer tests pass, and so do both typechecks.
• With the install call removed, the new test fails as it should.

The merge pass rechecks the fix (changed range only) with the same gate and then queues it first.

[devswha]

Both blockers are fixed in 54e0ca9.

1. A failed install repeated on every start. Before each try, the supervisor now writes bin/ovis-pty-sidecar.attempt in the release. A later start tries again only once that mark is 6 hours old (RUST_HOST_RETRY_MS, the same wait as the unshallow fetch, #1252). A start that comes while an earlier try may still run finds a fresh mark and leaves bin/ to that try. This also covers an installer left behind by a supervisor that was SIGKILLed. Its own downloads end within INSTALL_DEADLINE_MS (90 s) in scripts/pty-sidecar.ts even after its parent is gone.

2. An installer outlived the supervisor. The install now gets an AbortSignal, and shutdown() aborts it before it stops the bridge. runCommand's abort handler then sends SIGTERM to the installer's process group, and does so synchronously, before process.exit.

New tests in server/legacy-env.test.ts. Both run the real runSupervisor and runCommand; the release's scripts/pty-sidecar.ts is a stand-in.

• "tries the Rust PTY host again only after its retry wait": a second start does not rewrite a fresh mark, and no second install runs. With the mark backdated 7 hours, the next start runs the install again. Without the retry-wait check, this test fails (Expected "kept\n").
• "stops its Rust PTY host install when the supervisor stops": the stand-in install waits for SIGTERM, and the stand-in supervisor is sent SIGTERM once the install has started. Without rustHostInstall.abort(), this test fails with the install's stop never happened.

Results: bun test server/legacy-env.test.ts 11 pass. bun run check fast passes: typecheck, build, and 6546 unit tests passed, 0 failed.

[devswha]

Merge pass gate re-check of 54e0ca9 (the changed range only): the retry storm is fixed. One blocker remains: shutdown.

Fixed: with the host record absent, three supervisor starts now give one install try. The 6 h marker is kept per release, and a corrupt marker is tolerated because only its mtime counts. Both new tests fail against in-memory mutants of their fixes. 65 tests pass and the typecheck passes.

Still blocking: the supervisor exits before the installer group is dead (server/supervisor.ts:161-167, server/updater.ts:78-111)

• On shutdown the command is aborted, but the supervisor awaits only the bridge and then exits. That drops the pending SIGKILL escalation and the 120 s timer.
• In the probe, an installer and its child that ignore SIGTERM both survived the supervisor.
• Once the marker expired, a second supervisor started another installer in the same bin/ while the first was still alive.
• Fix: keep the command's promise and await its termination before the supervisor exits. If needed, have the SIGKILL escalation run in place of the timer during shutdown, so the whole process group is dead before the supervisor lets go. A test with a child that ignores SIGTERM would pin it; today's test only proves cooperative SIGTERM.

Non-blocking notes: the marker write isn't exclusive, and its timing depends on the clock. Neither is a problem once the installer can't outlive the supervisor.

[devswha]

Merge pass gate re-check of ea9ab87 (changed range only): nearly there. One bound is missing.

What holds: an installer and its child that both ignore SIGTERM are now both gone before the supervisor exits. Shutdown took 2.0 s. An immediate restart, a restart after the marker expired, and a start racing a shutdown never overlapped installers; the ownership lock refused the concurrent one. Retry backoff, #1859's checks and the fallback are unchanged. 66 tests pass, and so does the typecheck.

The remaining gap: shutdown can wait forever (server/supervisor.ts:169, server/updater.ts:90-111)

• await Promise.all([stopChild(), rustHostTry]) waits on a promise that settles only on the child's error or close.
• close needs every stdio stream closed. A descendant that left the process group, or any process holding the pipe, keeps it open after SIGKILL. A process stuck in D state never closes at all.
• In a controlled probe both deadlines fired, yet shutdown stayed pending. The plugin then can't restart.

Policy: this is the merge pass's own correction. Our earlier brief asked for both "nothing outlives the supervisor" and "shutdown is bounded", and no code can guarantee both for a process the kernel won't kill. The bound wins:

• SIGTERM the group, then SIGKILL the group (as now).
• Then wait for the direct child's exit (not close), raced against a short deadline, such as 5 s after SIGKILL.
• If the deadline passes, log a warning naming the pid and exit anyway.

Test (non-blocking but worth it): the new test's child has no readiness handshake, and it was already gone in the reverted run, so it doesn't reliably pin a resistant descendant. Having the child signal "ready, ignoring TERM" before shutdown starts, and asserting event order, would pin it.

The merge pass rechecks the next commit, changed range only, and queues it.

[devswha]

The shutdown blocker is fixed in ea9ab87, and fd2df96 adds the gaps a review found.

Fix (ea9ab87): installMissingRustHost now returns the try's promise. It never rejects, and it settles only once runCommand's close has run, which is after the group SIGKILL for a cancelled command. shutdown keeps that promise and awaits Promise.all([stopChild(), rustHostTry]) before lock.release() and process.exit(). The abort's SIGTERM, the 2 s grace and the SIGKILL of the whole group therefore all happen while the supervisor is alive, and no 120 s timer is involved.

Test (server/legacy-env.test.ts): "kills a Rust PTY host install that ignores SIGTERM, and its children, before the supervisor exits".

• The stand-in install and a child it spawns both ignore SIGTERM and both hold the write end of a FIFO. The child holds none of runCommand's pipes, only the FIFO as fd 3.
• The child writes the started mark once its own SIGTERM handler is installed. The supervisor is then sent SIGTERM.
• The test passes only when the FIFO's reader sees EOF, meaning both are dead, within 15 s of the supervisor's exit.
• On 54e0ca9 it fails with "the install or its child outlived the supervisor". With the fix it passes in about 2.3 s (the grace). All 12 tests in the file pass.

Review (Codex gpt-6.1-sol, xhigh) of ea9ab87, all fixed in fd2df96:

• P2: a SIGTERM during the startup health check let shutdown capture rustHostTry === null, and startup could then still launch a try. A try now starts only while !stopping, with no await between that check and the assignment.
• P3: the child signalled ready before its SIGTERM handler existed. It now writes the started mark itself, after installing the handler.
• P3: a run that failed early left the FIFO reader and its timer pending. Cleanup now clears the timer, frees a reader still blocked in open (a non-blocking writer opens and leaves), and destroys it. gone can no longer reject unhandled.

bun run check fast passed before each push. As you noted, the marker write isn't exclusive and its timing depends on the clock; both are left as they are, since the installer can no longer outlive its supervisor.

[devswha]

Merge pass: fd2df96 is fine; the stop guard and the readiness handshake both help. The point in my 21:34Z comment is still open. runCommand settles on the child's close, and close fires only once every holder of the stdio pipes is gone. A descendant that left the process group, or a process the kernel can't kill (D state), keeps it pending after the group SIGKILL, so shutdown never reaches process.exit. Please race the try against a short deadline after SIGKILL, settling on the direct child's exit or after about 5 s, and log the pid if the deadline wins. With that, shutdown is bounded even in that case.

[devswha]

Shutdown is bounded now, in e2732f6, which answers your 21:34Z and 21:37Z comments.

What it does (server/supervisor.ts):

• shutdown aborts the try as before: SIGTERM to the group, then SIGKILL to the group after runCommand's 2 s grace.
• It then waits for the try through boundedStop, which races the try against RUST_HOST_STOP_MS = 7_000 (the 2 s grace plus 5 s after the SIGKILL).
• If the deadline wins, it logs Rust PTY host install in <release dir> did not end 7 s after it was stopped; exiting anyway. A process of it may still run. and the supervisor exits.

Two differences from what you asked:

• exit vs close: the race settles on the try's existing promise, which runCommand settles on close, rather than on the child's exit. For a direct child that exits but leaves a descendant holding the pipe, shutdown takes the full 7 s instead of returning at once. It stays bounded either way, and runCommand (six callers) is left unchanged.
• The warning names the release directory, not the pid: runCommand doesn't expose the child, and exposing it for this one log line meant changing that shared signature. The try also keeps its own release directory, so a handover (which replaces active first) still names the right one.

Test: "exits even when a process of its Rust PTY host install escaped the group and holds its output open".

• The stand-in install spawns a detached: true child (a group of its own) that inherits the install's stdout pipe and ignores SIGTERM. The child writes the started mark only after its handler is installed. The supervisor is then sent SIGTERM.
• On fd2df96 the supervisor never exits: the test's own 15 s guard kills it (exit 137). With the fix it exits 0 after about 7.3 s.
• All 13 tests in server/legacy-env.test.ts pass, the typecheck is clean, and bun run check fast passed before the push.

Review: Codex gpt-6.1-sol (xhigh) reviewed this commit and found no P1 or P2 problems. Its one P3, that the warning named the new release after a handover, is fixed in this commit.

[devswha]

Merge pass review-work report. Overall: PASSED. This change touches the install path at supervisor start, so it went through the gate four times. CodeRabbit was rate-limited, and the gate reviews stand in for it.

Round	Head	Verdict
1	582ed09	REJECT: a failed install repeated on every supervisor start, and the installer was detached and could outlive the supervisor
2	54e0ca9	REJECT: the retry storm was fixed (3 starts, 1 try, 6 h per-release marker), but shutdown exited before the installer group was dead
3	ea9ab87	REJECT: the group died before exit (a TERM-resistant installer and child, 2.0 s), but the wait on close had no bound
4	fd2df96 + e2732f6	Fixed. No try starts once a stop has begun, the test child sends a readiness handshake, and boundedStop waits at most 7 s (2 s grace, then 5 s after the group SIGKILL)

Round 4 evidence

• E6 (an installer that never closes): exit at 7.006 s, with the warning.
• E4 (a resistant installer and child): exit at 2.010 s, both gone.
• SIGKILL lands before the deadline.
• Mutants of both new regressions fail. The 67 focused tests pass, and so does the typecheck. Only server/supervisor.ts and server/legacy-env.test.ts changed.

One policy change, made by the merge pass: round 4 still rejected the deadline warning, because it names the release directory rather than a pid, as the merge pass's own policy comment had asked. The merge pass accepts the directory instead. It identifies the try, runCommand exposes no pid, and the warning appears only after SIGKILL failed. Logging the pid is a follow-up.

Kept throughout: #1859's checks (the manifest for the package.json version, the crate tree, SHA-256, and a binary that runs, published atomically), the fallback to Node or a mirror on failure, and the ownership lock that refuses a second installer.

CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it.