Uh oh!
There was an error while loading. Please reload this page.
fix(ssh): give each managed launch a fresh remote server log - #8930
fix(ssh): give each managed launch a fresh remote server log#8930marmar9615-cloud wants to merge 2 commits into
Conversation
The managed remote launch appends to server.log, and the readiness failure branch treats whatever is already in that file as this run's output: if [ -s "$LOG_FILE" ]; then tail -n 80 "$LOG_FILE" >&2 else printf 'It wrote nothing to %s, so it exited before producing any output.\n' fi The empty-log arm arrived in pingdotgg#5132 to name the case where the remote server exits without logging anything. The log is opened in append mode and never cleared, so [ -s ] is true from the first run that logs onward. After that the empty-log arm is unreachable, a server that dies silently is reported with the previous run's error, and the user is pointed at the wrong remedy. The file also grows without bound across managed restarts. Unlink rather than truncate. wait_for_pid_exit gives up after two seconds, so a previous server that ignores the kill is still holding its descriptor when the next launch runs. Unlinking leaves it writing into the old file; truncating leaves it writing into the new one, which puts the size back above zero and sends the diagnostic down the tail branch again. The new test runs the real generated script through a shell against a seeded stale log, with a fake node that picks a port, fails readiness at once, and lets the runner exit without writing a byte. It fails on the append-only script at the "It wrote nothing to" assertion.
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate Security or dismiss this notice. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The production change is a small, isolated SSH launch fix that clears stale managed-server logs before starting a new process, with an end-to-end regression test. The test additionally introduces a line-scoped Notes:
You can add or adjust custom eligibility rules. Learn more. |
The executed suite needs node:child_process, which effect(nodeBuiltinImport) rejects. The first version turned the rule off for the whole file, which also covered the thirteen tests that were already there and had never needed it. Use the next-line form instead, so the exemption reaches only the one import that requires it. Verified against a patched tsgo: removing the directive reports TS377057 at src/tunnel.test.ts(3,35), and the next-line form silences exactly that.
What changed
One line in the generated remote launch script,
packages/ssh/src/tunnel.ts:Why it should exist
The readiness failure branch tells two cases apart by file size:
That empty-log arm came from #5132, to name the case where the remote server exits without
logging anything. The launch opens the log with
>>and nothing clears it, so[ -s ]is truefrom the first run that logs onward and the arm is unreachable after that. A server that dies
silently is reported with the previous run's error, which sends you after the wrong problem. The
file also grows without bound across managed restarts.
Why unlink and not truncate
wait_for_pid_exitgives up after 20 x 0.1s (tunnel.ts:495-502), so a server that ignores thekill is still holding its descriptor when the next launch runs. Measured separately from the test,
with a survivor holding the append-mode descriptor the launch gave it:
[ -s ]takes>truncaterm -fthen>>Truncating hands the new file to the old writer. Unlinking leaves it with the old one. The test
below passes under either form, so treat the table as the reason for the choice, not as something
the test proves.
Test
One executed case in
packages/ssh/src/tunnel.test.ts. It seedsserver.logwith a marker,installs a fake
nodethat picks a port, fails readiness at once, and lets the runner exitwithout writing a byte, then runs the real
buildRemoteLaunchScript()output through a shell. Itasserts exit 1, stderr containing "It wrote nothing to", no marker in stderr, and a 0-byte log.
Against the append-only script it fails at the "It wrote nothing to" assertion. It follows the
executed-shell pattern already in
apps/desktop/src/wsl/DesktopWslEnvironment.test.ts, includingthe shell probe that skips where the tools are missing.
vp test run src/tunnel.test.ts: 1 file, 14 tests, all pass.tsgo --noEmitclean.Nothing else reads this path. The only other reference is the on-demand tail script at
tunnel.ts:649, which opens by path per SSH invocation, so unlinking strands no descriptor.Note
Low Risk
Small change to generated remote launch shell behavior and failure diagnostics only; no auth or data-path changes.
Overview
Each managed remote SSH launch now deletes the existing
server.logbefore starting the server, so readiness failures can tell a silent new run from one that actually logged output.Previously the log was only opened in append mode, so
[ -s "$LOG_FILE" ]stayed true after the first run and a server that exited without writing anything could surface stale tail output instead of the "It wrote nothing to …" message.Adds an executed test in
tunnel.test.tsthat runs the realbuildRemoteLaunchScript()output under bash/WSL with a seeded stale log and a fakenode; the suite skips when required POSIX tools are missing.Reviewed by Cursor Bugbot for commit 9c84d1d. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix stale log tailing by deleting
$LOG_FILEbeforenohupin SSH launch scriptrm -f "$LOG_FILE"immediately before starting the managed server, so readiness and output checks see only the current run's log.nodebinary and a seeded staleserver.logto verify a silent launch is reported instead of old log content.nohup,mktemp,cmp, andtailis available.Macroscope summarized 9c84d1d.