Skip to content

fix(ssh): an SSH server update counts terminals again just before it stops the server - #30874

Merged
nwparker merged 2 commits into
mainfrom
nwparker/ssh-update-census-recount
Oct 19, 2026
Merged

nwparker merged 2 commits into
mainfrom
nwparker/ssh-update-census-recount

Conversation

@nwparker

@nwparker nwparker commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 8 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​64 $\color{#cf222e}{\Huge{\mathbf{−}}}$​26 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​38
Prod 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​40 $\color{#cf222e}{\Huge{\mathbf{−}}}$​24 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​16

ELI5

When Orca updates the server it runs on an SSH host, it first checks whether terminals are running there. On SSH, that check happened only once, before the slow upload of the new version. A terminal you opened while the upload was in progress was never counted, so the server restart could kill it.

What Changed

  • Before: an SSH update counted terminals once, before building and uploading the new server, and never again. If you opened a terminal during the upload, and the host ran terminals inside the server process (the degraded fallback that any restart ends), the restart killed that terminal without asking.
  • After: every update counts terminals again right before it stops the running server. A terminal opened during the upload now holds the update back, as it already did on WSL.

Mechanism:

  • recountCensus was an optional field of the deploy step, and the SSH path never supplied it. It is now required, so SSH, WSL and first-time deploys all have to provide it or the code doesn't compile.
  • The step that stops the running server always recounts first (orcad-installed-activation.ts).
  • If the recount can't be read, or it throws, the answer is "unverifiable". Unverifiable refuses the restart, even when the update is forced. It never reads as zero.
  • SSH's recount reuses the census call it already makes. WSL's recount is now always a function, which returns unverifiable when the running server couldn't be proven up.

Why

Making the recount a required parameter closes the whole gap: no current or future runtime can leave it out. Patching only the SSH call would have left the same trap for the next runtime.

The first count stays before the upload, outside the host's activation fence (the host-side lock an update holds while it swaps versions). That's where a server that went idle can still be woken. Under the fence, a wake attempt only reads "fenced". So if the server stops during the upload, the recount comes back unverifiable and the update defers. That is the safe direction.

Rollback and decommission also count terminals before taking that lock, but no upload sits between their count and the stop. They are left as they are.

Found by Codex review 5, P1 #1.

Linked Issue

None. Found in an internal review.

Visual Proof

N/A: no UI change. The only visible difference is that an update now shows the existing "terminals running" deferral in a case where it used to restart.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • A new test in orcad-runtime-maintenance.test.ts fails on main: the SSH update hands the deploy no recount.

  • In orcad-remote-deploy.test.ts, a recount just before the stop defers the update in four cases: live terminals; terminals running inside the server, even when forced; an unreadable count, even when forced; and a recount that throws.

  • WSL incumbent tests: an unproven incumbent's recount is unverifiable.

  • Ran locally: src/main/wsl, src/main/managed-server, src/main/ssh/orcad* and src/main/ipc/runtime-environments (2127 tests pass); pnpm tc:node; and every step of the "static analysis and typecheck" job.

  • Live-verified: nothing on a real host. Verified only in unit tests, plus the Docker managed-orcad e2e lane in CI.

Review

Codex (gpt-6.1-sol, high effort): clean on both heads.

An independent Opus review found no P0, P1 or P2 issues. Its three minor notes are handled here: a recount that throws now reads as unverifiable, the maintenance test was renamed, and the WSL test now checks the recount.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • SSH: this is the fix.
  • Mixed versions: nothing changes on the wire. The recount uses the census method the host already supports.
  • Platforms: no platform-specific code.

…stops the server

The SSH update passed the terminal census taken before the artifact upload and no recount,
so a terminal opened during the upload was missed and, on a host running terminals inside
orcad, ended by the restart. The recount is now a required deploy option, so every runtime
(SSH, WSL, first deploy) supplies one and the stop step always recounts.
@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 688d35d4-f942-45d1-bef0-27fc2fd27fbe
📥 Commits

Reviewing files that changed from the base of the PR and between 575d3b2 and 727645d.

📒 Files selected for processing: 15
  • src/main/managed-server/activation/orcad-activation-crash-recovery.test.ts
  • src/main/managed-server/host-state/orcad-state-mutation-timeout.test.ts
  • src/main/managed-server/host-state/orcad-windows-host-lane.test.ts
  • src/main/managed-server/install/orcad-remote-lifecycle-windows.test.ts
  • src/main/managed-server/lifecycle/orcad-managed-runtime-context.ts
  • src/main/ssh/hostile-host-test-support/ssh-hostile-host-managed-orcad.ts
  • src/main/ssh/orcad-installed-activation.ts
  • src/main/ssh/orcad-remote-deploy.test.ts
  • src/main/ssh/orcad-remote-deploy.ts
  • src/main/ssh/orcad-runtime-maintenance.test.ts
  • src/main/ssh/orcad-runtime-maintenance.ts
  • src/main/ssh/orcad-snapshot-room.test.ts
  • src/main/wsl/wsl-orcad-deploy.ts
  • src/main/wsl/wsl-orcad-incumbent.test.ts
  • src/main/wsl/wsl-orcad-incumbent.ts

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

📝 Walkthrough

Deployment options now require a census recount callback. Managed runtime and WSL deployment paths supply callbacks, and WSL incumbent results provide a callback when census data is unreadable. Activation recounts an incumbent when its record matches the locked record, and uses an unverifiable census when the recount fails or the records do not match. Tests cover recount outcomes and update deployment helpers to provide callbacks.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 72764

No actionable merge-blocking issue remains; the update defers when terminal activity cannot be verified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 15 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: SSH updates now recount terminals before stopping the server.
Description check Passed The description is detailed, focused, and covers the change, motivation, testing, review, visual proof, and platform impact. It does not link an issue, but it explains that the change came from an int…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description is detailed, focused, and covers the change, motivation, testing, review, visual proof, and platform impact. It does not link an issue, but it explains that the change came from an internal review; the remaining unchecked manual-test items are addressed by the testing notes.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • 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

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

@nwparker
nwparker merged commit 209e468 into main Oct 19, 2026
52 checks passed
nwparker added a commit that referenced this pull request Oct 20, 2026
…ake count

Rebased onto #30874, which made recountCensus required and gave SSH its own recount. The SSH
fence now supplies that recount, counting over the tunnel's current pairing without waking a
server under the activation lock; an unanswered recount reads unverifiable, as on main.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant