Skip to content

feat(ssh): crash-safe orcad activation, rollback and recovery (#16741 T6-2) - #24423

Merged
OrcaWin merged 1 commit into
mainfrom
OrcaWin/node-rt-p3-t6-2
Oct 1, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
OrcaWin/node-rt-p3-t6-2

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​899 $\color{#cf222e}{\Huge{\mathbf{−}}}$​63 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​836
Prod 20 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​2263 $\color{#cf222e}{\Huge{\mathbf{−}}}$​483 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1780

ELI5

Updating or rolling back the Orca server on a remote machine takes several steps: stop the old server, back up its data, start the new one, and record which version is live. If the connection drops halfway, the machine could be left with no server running, or with data the old version can't read. With this change, the machine keeps a short journal of the step it reached. A later recovery run reads that journal and either finishes the change or undoes it cleanly. Nothing calls this code yet.

What Changed

Before: deployOrcad and rollbackOrcad ran their stop, snapshot, start and record steps with no record of progress. If the connection was lost partway, nothing on the host said what had happened. A second client could also start a deploy at the same moment.

After:

  • Activation fence and journal. Every activation and rollback runs under one fence per host: .orca-remote/.orcad-activation-transaction/ (orcad-activation-lock.ts). Before the first change to the host, it writes a journal, transaction.json, that records:

    • the activation record before and after the change;
    • the phase reached;
    • which snapshot holds the state to put back.

    The journal moves through these phases:

    • Activate: prepared → incumbent-stopped → snapshot-captured → candidate-ready.
    • Rollback: prepared → incumbent-stopped → rescue-captured → rollback-state-restored → target-ready.

    The activation record stays the commit point.

  • Recovery (recoverInterruptedOrcadActivation). It compares the journal with the current activation record and picks one of these:

    Situation Recovery action
    The record already matches the "after" side Make sure that slot is serving
    The new slot passed its health check, but the record write was lost Finish writing the record
    The record still matches the "before" side Stop whatever the run launched, restore the saved state, and bring the previous slot back
    Anything else Refuse, and keep the fence for an operator
  • Fence takeover. Normal runs never take over a fence because it is old: a retained fence means nobody can yet prove who owns the data. Recovery takes over a fence only when the install lock's existing stale or reboot check allows it. To support this, acquireInstallLock gains two options, allowStaleTakeover and waitTimeoutMs, plus a typed RemoteInstallLockBusyError. Existing callers keep their current behavior.

  • Rollback rescue. Rollback now saves a rescue copy of the newer state before restoring the pre-activation snapshot. A failed restore or a failed target can then put the newer build back, where before the host was left stopped.

  • Snapshot restore. The restore now unpacks into a staging directory first. An unreadable archive therefore fails before any live state is removed, and an interrupted restore can safely be run again. clearOrcadStateSnapshotMembersCommand, parseOrcadSnapshotPresence and orcadRollbackRescueDirName are added.

  • D7 (new orcad-daemon-protocol-crossing.ts). Updates and rollbacks are now checked against the protocol of the live terminal daemon:

    • OrcadTerminalCensus carries daemonProtocolVersion.
    • planOrcadUpdate defers any update whose candidate cannot attach to the live daemon's protocol (orcad_update_strands_live_terminals), even when forced. A forced update with an unknown protocol also defers.
    • assessOrcadRollback keeps terminals only when the rollback target speaks the live daemon's protocol or lists it as a previous version. Otherwise it refuses with orcad_rollback_strands_live_terminals or orcad_rollback_daemon_protocol_unverifiable.
    • It also no longer reads a lost snapshot probe as "snapshot missing". That case returns the new orcad_rollback_snapshot_unverifiable.
  • Re-derived from feat(orcad): consolidate Bun runtime and SSH/Remote migration #16741 against main's pinned Node runtime, not replayed:

    • The slot runtime is chosen by main's selectOrcadSlotRuntimeCommand, with no Bun fallback.
    • Stop follows main's nodePath / justLaunched contract.
    • The initial-activation admission probe runs on the candidate slot's own runtime.
    • The branch's orcad-rollback-recovery.ts is folded into orcad-rollback-transition.ts and orcad-incumbent-recovery.ts.
    • orcad-remote-deploy-stop.ts is removed: its stop and changed-state checks now live in the shared recovery path.
  • Not included: the decommission transaction (T6-4). An older client reads an unknown journal operation as unreadable, so it keeps the fence.

Why

Termination verdicts. Every step uses the execution host's verdict vocabulary: live, unverifiable or exited.

  • Loss of contact is never treated as exit, and never as an empty record or a missing snapshot.
  • An unconfirmed SSH termination keeps the fence.
  • No path removes a version slot or a snapshot. The only deletions are the fence itself and the restore staging directory, which is rebuilt from the intact archive.

Exactly one active slot. The guarantee rests on two rules. First, a slot that may have changed shared state is stopped, with proof of exit, before that state is replaced. Second, the slot being restored must be proven not running, or must already be serving.

  • Missing PID file. This is not treated as proof of exit: an SSH drop can kill the launcher after nohup starts the process but before its PID is written. In that case the data root's owner records decide.
  • Changed state. A slot that started serving may have accepted new terminals. Recovery therefore restores state that slot changed only after a fresh census callback (terminalsStartedSince) reports zero new terminals. This is the same rule main's deploy already applied to a rejected candidate.

Alternatives considered:

  • Undoing an activation that had already passed its health check. That would stop a healthy build and need a census; finishing the commit needs neither.
  • Taking over a retained fence by age. That would let a later deploy replace state that nobody has proven is safe to replace.

Linked Issue

Part of the Node runtime plan (Phase 3), T6 exit-gate track, porting #16741. Stacked on #24419 (T6-1).

Visual Proof

N/A. There's no UI change, and this code has no production caller. It is inert: deployOrcad and rollbackOrcad are still unused, and every command is POSIX-only (Windows hosts are refused, as in T6-1).

Testing

  • New orcad-activation-crash-recovery.test.ts. Runs against a stateful fake host that models processes, PID files, profile state, snapshots, the record, the journal and the fence. For three scenarios (an update over an incumbent, a first activation, and a rollback), it drops the connection at every host mutation, both before and after the host applied it, then runs recovery. Each run must end with:

    • exactly the recorded slot serving, on state that slot can read;
    • no journal and no fence left behind;
    • no slot or snapshot removed.

    It also checks the refusals:

    • terminals started since the change mean a live refusal, with the fence kept;
    • a missing or null census keeps changed state;
    • an unverifiable incumbent is never treated as exited;
    • a fresh fence is reported as pending;
    • an unreadable (newer) journal keeps the fence;
    • a fence left without its journal is dropped.
  • New orcad-activation-transaction.test.ts. Covers the journal round trip and its unreadable shapes, the recovery plan for every phase, and the fence semantics:

    • no takeover by age;
    • the journal is removed before the lock;
    • the fence is kept on unconfirmed termination, on retainOnError and on retain;
    • a recovered failure releases the fence.
  • New config/scripts/orcad-daemon-protocol-crossing.test.mjs (D7). Checks orcad's update and rollback planning against config/scripts/daemon-protocol-facts.mjs (canAttach) and the working tree's real protocol facts:

    • rollback to the release before the latest protocol bump is refused while terminals are live;
    • rollback is allowed when the target lists the live daemon's protocol;
    • a forced update over an unattachable daemon defers.
  • New orcad-state-snapshot-shell.test.ts. Runs capture, restore and clear on a real /bin/sh and tar:

    • the daemon directory is left untouched;
    • a corrupt archive keeps live state;
    • an interrupted restore can be run again.
  • Updated: the deploy, rollback, update-plan, state-snapshot and install-lock tests.

  • Regression: every src/main/ssh/*.test.ts and src/main/orcad/*.test.ts, plus the two daemon-protocol .mjs tests. 260 files: 249 passed, 11 skipped. 3367 tests passed, 54 skipped.

    • One relay test, ssh-relay-incumbent-process.test.ts, timed out once under full parallel load and passes on its own. It doesn't touch this change.
  • Static checks: node config/scripts/check-changed-code-quality.mjs origin/OrcaWin/node-rt-p3-t6-1 is clean, and so is oxlint. A scoped tsc --noEmit over the changed files is also clean; the full typecheck was not run.

  • I manually tested these changes locally

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

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Known gaps, left for later T6 slices:
    • Orcad GC does not yet read the journal. During a transaction, it pins only the recorded slots.
    • A deploy that finds a leftover journal under its own fresh lock keeps the fence. Recovery can then take it over only after the install lock's 20-minute stale window.
  • Remote wire: no client–host wire or protocol change. The journal is a new host-side file under .orca-remote/, and older clients never read it.

@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 0/5

[High risk] Adds crash-recovery and rollback logic to the deployment system.

The PR does not appear safe to merge while relay reset can leave a drained daemon holding its endpoint and four earlier Orcad recovery findings remain unresolved.

Findings

  1. P1 Candidate mistaken for exited ▶
  2. P1 First activation leaves changed state ▶
  3. P1 Live holder loses fence ▶
  4. P1 Record commits before service check ▶

Summary

The PR adds journaled Orcad activation and rollback recovery. Changes since the previous review also add SSH lifecycle tracking, relay owner-reset preparation, runtime-environment sidecar storage, and structured-session cancellation and logging updates.

  • Orcad recovery retains a fence when it cannot establish safe host state.
  • Relay owner-reset preparation introduces a shutdown state that can remain drained after a lost response.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Owner reset admitted] --> B[Drain work and dispose PTYs]
  B --> C{Preparation response settles?}
  C -->|Yes| D[Finish shutdown and release endpoint]
  C -->|Lost| E[Relay remains prepared and drained]
  E --> F[Explicit prepared-reset recovery required]
Loading

Reviews (3) · Last reviewed commit: "feat(ssh): crash-safe orcad activation, ..."

Comment on lines +163 to +175
if (!exited && stopped === 'no-pid' && remoteDir !== null) {
const admission = parseInitialOrcadActivationAdmission(
await execOrcadRemote(
options,
initialOrcadActivationAdmissionCommand(
options.host,
options.userDataDir,
remoteDir,
options.nodePath
)
)
)
exited = admission.decision === 'proceed'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Candidate mistaken for exited If SSH drops after nohup starts the candidate but before its PID file is written, the candidate may still be starting when the owner-record probe returns CLEAR. Recovery treats that answer as proof of exit, allowing it to restore shared profile state and start the incumbent while the candidate may still acquire the data root. This can leave two processes competing over the same state.

Knowledge Base Used: Remote connectivity and execution

Comment on lines +68 to +81
// With no incumbent there is no older reader to protect; the record names nothing to serve.
if (quiescence === 'exited' && input.restoreState && input.incumbent) {
const decision = input.launchedVersion
? await decideChangedStateRestore(options, input.transactionStartedAt, input.restoreState)
: 'restore'
if (decision !== 'restore' && decision !== 'unchanged') {
return decision
}
if (decision === 'restore') {
await restoreState(options, input.restoreState)
}
}
if (!input.incumbent) {
return { outcome: 'restored', readiness: null }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 First activation leaves changed state If the first activation snapshots an existing profile and the candidate changes it before being rejected or interrupted, there is no incumbent. This guard skips both snapshot restoration and the fresh terminal census, then reports recovery complete. The host has no recorded service, but its original profile state has not been restored.

Knowledge Base Used: Remote connectivity and execution

Comment on lines +108 to +112
): Promise<T> {
const lockRoot = orcadActivationTransactionRoot(options.host, options.remoteHome)
await acquireInstallLock(options.conn, lockRoot, options.host, {
signal: options.signal,
relayGcClaim: false,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Live holder loses fence If an activation or rollback is still working after the lock’s 20-minute stale window, recovery can take its fence solely because of its age. Both runs can then change the same host, and the original run’s unconditional release can delete recovery’s replacement fence and journal. A slow but live holder must not be treated as finished.

Knowledge Base Used: Remote connectivity and execution

Comment on lines +72 to +84
return {
outcome: 'refused',
verdict: 'unverifiable',
code: 'orcad_recovery_unverifiable',
reason:
`The interrupted activation could not be reconciled safely: ${errorMessage(error)} ` +
'The host remains fenced.'
}
}
}

/** Runs under a held fence. Throws when a step is unverifiable. */
export async function reconcileOrcadTransaction(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Record commits before service check During finish-commit, recovery writes the activation record before checking the installed slot’s identity and readiness. If that formerly ready slot is now missing or cannot be verified, recovery keeps the fence but has already committed a record naming a service it cannot establish.

Knowledge Base Used: Remote connectivity and execution

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Minor suggestions only.

Reviewed changes

Reviewed the full PR (29 files, one commit) in a single pass: the per-host activation fence and transaction.json journal, the crash-recovery reconciler and shared incumbent-recovery path, the snapshot staging/restore changes, the D7 daemon-protocol crossing checks, and the install-lock allowStaleTakeover / waitTimeoutMs additions.

  • Fence + journal: activations and rollbacks now run under one fence per host with a durable phase journal, and the activation record stays the commit point.
  • Recovery: recoverInterruptedOrcadActivation finishes, undoes, refuses, or reports pending, using the execution host's live / unverifiable / exited vocabulary.
  • Rollback rescue: stop → rescue-capture → restore → start, with the newer state put back when a restore or target fails.
  • Snapshot restore: extraction into a staging dir before live members are removed, plus presence parsing and the empty-root clear.
  • D7: update and rollback planning now check the live daemon's protocol; a lost snapshot probe is unverifiable, never "missing".

I traced the commit-point logic, the fence release semantics, and the first-activation undo path against the write ordering and found no path that serves the wrong build, releases a fence over unresolved state, or destroys live state.

ℹ️ A failed recovery attempt parks the next retry for the stale window

withStaleOrcadActivationRecoveryLock has no try/catch, so when reconcileOrcadTransaction throws — an unverifiable readiness probe, a transient SSH error — the lock it just stole stays on the host. Because a stale takeover recreates the lock, its age resets to now, so the next recoverInterruptedOrcadActivation acquires with waitTimeoutMs: 0 and gets RemoteInstallLockBusyError → pending for up to INSTALL_LOCK_STALE_MS (20 minutes) instead of retrying reconciliation. The PR body documents this window for a deploy finding a leftover journal, but not for a recovery's own throw; worth confirming that a transient recovery failure buying a 20-minute lockout is the intended UX. No safety impact — the fence is correctly kept.

Technical details
# A thrown recovery resets the fence's stale clock

## Affected sites
- `src/main/ssh/orcad-activation-lock.ts:105-123` — `withStaleOrcadActivationRecoveryLock` releases only on the normal-return path (`if (!retain) await releaseActivationFence(...)`); a throw unwinds before that line.
- `src/main/ssh/ssh-relay-install-lock.ts:150-188` — the stale-takeover branch calls `tryStealInstallLockCommand` with `INSTALL_LOCK_STALE_SECONDS`; the freshly recreated lock fails that check.
- `src/main/ssh/orcad-activation-recovery.ts:62-70` — the resulting `RemoteInstallLockBusyError` is reported as `outcome: 'pending'` / `orcad_recovery_transaction_still_fresh`.

## Required outcome
Confirm whether a recovery that throws before proving one serving slot should force the retry through the full install-lock stale window, or release only the lock (keeping the journal) so an immediate retry can reattempt reconciliation.

## Open questions for the human
- If the intended contract is "a throw keeps the fence", is the 20-minute backoff an acceptable operator cost for a transient SSH blip during recovery? If not, is there a way to drop the lock while preserving `transaction.json` (today `releaseActivationFence` removes the journal first and the tree second)?

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +188 to +190
const restarted = incumbent
? ` The incumbent was stopped before snapshotting; ${await restartAfterSnapshotFailure(options, incumbent, lock)}`
: ''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there is no incumbent, a failed snapshot capture leaves the activation fence retained even though nothing was stopped, snapshotted, or launched: lock.retainOnError() is set before the capture and only the incumbent branch reaches a path that can release it. The host then blocks deployOrcad and recoverInterruptedOrcadActivation for up to the 20-minute install-lock stale window, and a retry deploy surfaces a raw RemoteInstallLockBusyError. Calling lock.recovered() in the else branch (the host is provably unchanged) lets the normal-release path drop the fence.

Technical details
# First activation whose snapshot capture fails needlessly retains the fence

## Affected sites
- `src/main/ssh/orcad-installed-activation.ts:187-195` — when `incumbent` is null, `restarted` is empty, no recovery/restart runs, and the function throws with `retainOnError` still set from line 153.
- `src/main/ssh/orcad-activation-lock.ts:91-100` — the catch keeps the fence whenever `retainOnError` is true, regardless of whether any host state changed.

## Required outcome
A first activation that fails before the candidate is stopped or started (no incumbent, snapshot capture returned `failed`) should not fence the host: nothing outside the journal changed, so release is provably safe and matches the `recovered()` contract used by `restartAfterSnapshotFailure`.

## Suggested approach (optional)
In the `else` branch, call `lock.recovered()` before throwing so the normal-release path drops the journal and lock. This is only reachable for a confirmed capture failure (an unconfirmed SSH termination is rethrown earlier at line 181-183), so `recovered()` will not race an in-flight command.

@OrcaWin
OrcaWin force-pushed the OrcaWin/node-rt-p3-t6-2 branch from 32ad186 to 50fb1a1 Compare October 1, 2026 18:09
@OrcaWin
OrcaWin requested a review from brennanb2025 as a code owner October 1, 2026 18:09
@OrcaWin
OrcaWin changed the base branch from OrcaWin/node-rt-p3-t6-1 to main October 1, 2026 18:09
@OrcaWin OrcaWin closed this Oct 1, 2026
@OrcaWin OrcaWin reopened this Oct 1, 2026
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

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: d3a461ec-0210-40d2-bc94-7bf8f38d9377

📥 Commits

Reviewing files that changed from the base of the PR and between 50fb1a1a299a9191baf1c192f8fce4b3183a9c01 and 526a0a2.

📒 Files selected for processing (1)
  • src/main/ssh/orcad-activation-transaction-schema.ts

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


📝 Walkthrough

Walkthrough

The changes add a transaction journal and activation fence for Orcad deployment and rollback. They add recovery paths for interrupted operations, checks for daemon protocol compatibility with live terminals, and staged snapshot restoration. Deployment and rollback now use shared slot and state recovery operations. Tests cover protocol decisions, snapshot behavior, and simulated crashes during activation and rollback.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to 526a0

A failed snapshot during first activation blocks subsequent deployments until stale recovery becomes available, despite leaving runtime state unchanged. Resolve or explicitly accept this retry blockage before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 526a0

The change strengthens interrupted-update recovery, but recovery can stop processes and replace shared persisted state. Production integration and the guarantees around long-running transactions still need confirmation. No introduced security vulnerability was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The observed authority is remote process and profile-state control through the supplied SSH connection. The fence is scoped to a remote-home transaction root, while state operations target the configured user-data directory. Production account privileges and wider tenant or environment exposure remain unestablished.

Trust Boundaries and Controls

  • observed — Persisted journal data passes through bounded reading, schema parsing, nested activation-record validation, and before/after record matching before recovery actions. Malformed or mismatched state causes refusal rather than being interpreted as an absent transaction.
  • observed — Remote journal writes shell-escape content and paths, create a uniquely named partial file under umask 077, and rename it into place. This establishes restrictive file creation and atomic replacement, but not parent-directory ownership or power-loss durability.

Resilience and Maintainability Implications

  • observed — Ordinary activation cannot take over an aged fence. Explicit recovery permits stale takeover and retains the fence on unsuccessful reconciliation. Cleanup removes the journal before the transaction root, preventing interrupted cleanup from leaving a journal without its fence.
  • observed — With no incumbent, recovery can report restoration without restoring captured preexisting state. Base deployment likewise returned without state recovery when no version was active, and did not capture an initial-activation snapshot. The omission therefore predates this PR; the new snapshot improves available recovery evidence without establishing complete restoration of unmanaged state.

Hardening Proposals

  • proposed — Before production integration, establish a whole-transaction lifetime or live-owner fencing guarantee for stale recovery. The five-minute readiness limit alone does not demonstrate that snapshot, restore, and all other work finish before the 20-minute takeover threshold.
  • proposed — Define an explicit recovery policy for nonempty unmanaged state with no incumbent: either restore the captured preactivation state when safe, or distinguish intentional data preservation from complete state restoration.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 85 functions across 28 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: crash-safe Orcad activation, rollback, and recovery over SSH.
Description check ✅ Passed The description is detailed and covers the main required sections, including the user impact, implementation, rationale, linked work, testing, visual proof, scope boundaries, and known gaps. The AI Di…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 77860468-697d-4f79-bb84-98a7216d48ae

📥 Commits

Reviewing files that changed from the base of the PR and between 3fbdaba and 50fb1a1a299a9191baf1c192f8fce4b3183a9c01.

📒 Files selected for processing (29)
  • config/scripts/orcad-daemon-protocol-crossing.test.mjs
  • src/main/ssh/orcad-activation-crash-recovery.test.ts
  • src/main/ssh/orcad-activation-host-test-harness.ts
  • src/main/ssh/orcad-activation-lock.ts
  • src/main/ssh/orcad-activation-recovery.ts
  • src/main/ssh/orcad-activation-transaction-schema.ts
  • src/main/ssh/orcad-activation-transaction-store.ts
  • src/main/ssh/orcad-activation-transaction-transitions.ts
  • src/main/ssh/orcad-activation-transaction.test.ts
  • src/main/ssh/orcad-activation-transaction.ts
  • src/main/ssh/orcad-daemon-protocol-crossing.ts
  • src/main/ssh/orcad-incumbent-recovery.ts
  • src/main/ssh/orcad-initial-activation-admission.ts
  • src/main/ssh/orcad-installed-activation.ts
  • src/main/ssh/orcad-recovery-slot.ts
  • src/main/ssh/orcad-remote-deploy-stop.ts
  • src/main/ssh/orcad-remote-deploy.test.ts
  • src/main/ssh/orcad-remote-deploy.ts
  • src/main/ssh/orcad-remote-rollback.test.ts
  • src/main/ssh/orcad-remote-rollback.ts
  • src/main/ssh/orcad-rollback-transition.ts
  • src/main/ssh/orcad-state-snapshot-shell.test.ts
  • src/main/ssh/orcad-state-snapshot.test.ts
  • src/main/ssh/orcad-state-snapshot.ts
  • src/main/ssh/orcad-update-plan.test.ts
  • src/main/ssh/orcad-update-plan.ts
  • src/main/ssh/ssh-relay-install-lock.test.ts
  • src/main/ssh/ssh-relay-install-lock.ts
  • src/shared/runtime-bootstrap.ts
💤 Files with no reviewable changes (1)
  • src/main/ssh/orcad-remote-deploy-stop.ts

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

Comment on lines +105 to +122
export async function withStaleOrcadActivationRecoveryLock<T>(
options: OrcadActivationLockOptions,
run: (control: Pick<OrcadActivationLockControl, 'retain'>) => Promise<T>
): Promise<T> {
const lockRoot = orcadActivationTransactionRoot(options.host, options.remoteHome)
await acquireInstallLock(options.conn, lockRoot, options.host, {
signal: options.signal,
relayGcClaim: false,
allowStaleTakeover: true,
waitTimeoutMs: 0
})
let retain = false
// Any throw keeps the fence: recovery failed to prove one serving slot.
const result = await run({ retain: () => (retain = true) })
if (!retain) {
await releaseActivationFence(options, lockRoot)
}
return result

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
fd ssh-relay-install-lock-commands.ts --exec ast-grep outline {} --items all
rg -nP -A30 'export function tryStealInstallLockCommand' -g 'ssh-relay-install-lock-commands.ts'
rg -nP -A20 'export function tryCreateInstallLockCommand' -g 'ssh-relay-install-lock-commands.ts'

Repository: stablyai/orca

Length of output: 6686


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- lock command implementation ---'
sed -n '1,190p' src/main/ssh/ssh-relay-install-lock-commands.ts
printf '%s\n' '--- lock command remainder ---'
sed -n '190,310p' src/main/ssh/ssh-relay-install-lock-commands.ts
printf '%s\n' '--- activation lock and acquire callers ---'
sed -n '1,220p' src/main/ssh/orcad-activation-lock.ts
rg -n -P -A45 -B10 'acquireInstallLock|tryStealInstallLockCommand|allowStaleTakeover' src/main/ssh

Repository: stablyai/orca

Length of output: 45667


🏁 Script executed:

sed -n '1,190p' src/main/ssh/ssh-relay-install-lock-commands.ts
sed -n '190,310p' src/main/ssh/ssh-relay-install-lock-commands.ts
sed -n '1,220p' src/main/ssh/orcad-activation-lock.ts
rg -n -P -A45 -B10 'acquireInstallLock|tryStealInstallLockCommand|allowStaleTakeover' src/main/ssh

Repository: stablyai/orca

Length of output: 45667


🏁 Script executed:

set -e
sed -n '75,190p' src/main/ssh/ssh-relay-install-lock-commands.ts
sed -n '1,220p' src/main/ssh/orcad-activation-lock.ts
rg -n -P -A55 -B12 'function acquireInstallLock|acquireInstallLock\(|allowStaleTakeover|tryStealInstallLockCommand' src/main/ssh

Repository: stablyai/orca

Length of output: 42273


Preserve the original stale-lock age during recovery.

tryStealInstallLockCommand measures staleness from the lock directory timestamp. It then moves the stale directory to a tombstone and creates a new directory. Therefore, every recovery that calls retain() starts a new INSTALL_LOCK_STALE_MS window. A refused recovery can make the next retry wait for another stale window, and a deployment can remain blocked for INSTALL_LOCK_TIMEOUT_MS.

Preserve the original lock timestamp when recovery retains the fence, or allow a retained fence marked as no longer active to bypass the age check.

Comment on lines +187 to +195
if (capture === 'failed') {
const restarted = incumbent
? ` The incumbent was stopped before snapshotting; ${await restartAfterSnapshotFailure(options, incumbent, lock)}`
: ''
throw new Error(
`Could not snapshot ${options.userDataDir} before activating ${fullVersion}. Orca's ` +
'persisted state carries no schema version, so without a snapshot a rollback has no ' +
`way back. Refusing to activate.${restarted}`
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Release the fence when a first activation's snapshot capture fails.

lock.retainOnError() runs at Line 153, before the capture. When incumbent is null, restarted is empty and the function throws. No path calls lock.recovered(), so withOrcadActivationLock keeps the journal and the lock. On this path nothing was stopped or launched. A confirmed capture failure is the only way to reach this branch, because Lines 181-183 rethrow an unconfirmed termination. The host state outside the journal is therefore unchanged. The kept fence still blocks later deploys for the full lock timeout and fails them with RemoteInstallLockBusyError. Recovery cannot take the fence over until the 20-minute stale window passes.

🐛 Proposed fix
   if (capture === 'failed') {
-    const restarted = incumbent
-      ? ` The incumbent was stopped before snapshotting; ${await restartAfterSnapshotFailure(options, incumbent, lock)}`
-      : ''
+    let restarted = ''
+    if (incumbent) {
+      restarted = ` The incumbent was stopped before snapshotting; ${await restartAfterSnapshotFailure(options, incumbent, lock)}`
+    } else {
+      // Nothing was stopped or launched, so the host is provably unchanged.
+      lock.recovered()
+    }
     throw new Error(
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (capture === 'failed') {
const restarted = incumbent
? ` The incumbent was stopped before snapshotting; ${await restartAfterSnapshotFailure(options, incumbent, lock)}`
: ''
throw new Error(
`Could not snapshot ${options.userDataDir} before activating ${fullVersion}. Orca's ` +
'persisted state carries no schema version, so without a snapshot a rollback has no ' +
`way back. Refusing to activate.${restarted}`
)
if (capture === 'failed') {
let restarted = ''
if (incumbent) {
restarted = ` The incumbent was stopped before snapshotting; ${await restartAfterSnapshotFailure(options, incumbent, lock)}`
} else {
// Nothing was stopped or launched, so the host is provably unchanged.
lock.recovered()
}
throw new Error(
`Could not snapshot ${options.userDataDir} before activating ${fullVersion}. Orca's ` +
'persisted state carries no schema version, so without a snapshot a rollback has no ' +
`way back. Refusing to activate.${restarted}`
)

…T6-2)

Journal every orcad activation and rollback under a host fence so an interrupted one recovers to exactly the slot the activation record names. D7: planOrcadUpdate and assessOrcadRollback refuse a restart whose incoming build cannot attach the live terminal daemon's protocol. POSIX-only and inert: no production caller.
@OrcaWin
OrcaWin force-pushed the OrcaWin/node-rt-p3-t6-2 branch from 50fb1a1 to 526a0a2 Compare October 1, 2026 18:46
@OrcaWin
OrcaWin merged commit 1a9ac0e into main Oct 1, 2026
92 of 94 checks passed
OrcaWin added a commit that referenced this pull request Oct 2, 2026
* Revert "feat(orcad): source-side dormant export of a relay-hosted SSH target (#16741 T6-8) (#24519)"

This reverts commit 783101b.

* Revert "feat(ssh): update, roll back, recover and stop a managed orcad server (#16741 T6-5 follow-up) (#24463)"

This reverts commit 38c2d1d.

* Revert "feat(ssh): deploy and pair an empty managed orcad server over SSH (#16741 T6-5) (#24453)"

This reverts commit 8b76683.

* Revert "fix(ssh): orcad GC honors the activation journal; readiness requires proven daemon coverage (#16741 T6 follow-up) (#24451)"

This reverts commit d3f8c50.

* Revert "feat(ssh): remote orcad stop by request file and journaled decommission (#16741 T6-4) (#24449)"

This reverts commit 43d9b43.

* Revert "feat(orcad): supervisable server: stop requests, managed stop receipts and a lifetime that keeps its lock on failed teardown (#16741 T6-3) (#24433)"

This reverts commit b093d3a.

* Revert "feat(ssh): crash-safe orcad activation, rollback and recovery (#16741 T6-2) (#24423)"

This reverts commit 1a9ac0e.

* Revert "feat(runtime): SSH access links for paired servers in a downgrade-safe sidecar (#16741 T5-1+T5-2) (#24420)"

This reverts commit 99db2bf.

* Revert "feat(relay): capability-gated owner reset with a durable preparation journal (#16741 T3 R1) (#24418)"

This reverts commit 34a582b.

* Revert "feat(ssh): track connection-manager drains, test probes and provider continuations (#16741 T2 P3+P8a) (#24407)"

This reverts commit d53063d.

* Revert "feat(daemon): idle retirement, session census and recovery-only provider (#16741 T2 P4b) (#24409)"

This reverts commit ff212db.

* Revert "feat(ssh): add pty.resumeClient and split SSH PTY process listing (#16741 T2 P5+P6) (#24414)"

This reverts commit 92cb717.

* Revert "feat(relay): await owned watcher and agent children on shutdown (#16741 T2 P1) (#24400)"

This reverts commit 6b36e4f.

* Revert "feat(session): retry failed renderer session writes and verify local folder PTYs (#16741 T2 P9) (#24406)"

This reverts commit d23ecef.

* Revert "feat(ssh): remote orcad primitives on the pinned Node runtime (#16741 T6-1) (#24419)"

This reverts commit dd87ae5.

* Revert "fix(runtime): fence runtime-environment subscriptions and status probes by identity (#16741 T5-3) (#24421)"

This reverts commit ece9e4d.

* Revert "feat(orcad): migration manifest and dormant-state contracts (#16741 T6-7) (#24422)"

This reverts commit 3fbdaba.

* Revert "feat(ssh): wire SshConnection through the work and transport close ledgers (#16741 T2 P2) (#24401)"

This reverts commit 4e8edc8.

* Revert "feat(profiles): carry markdown frontmatter visibility in project transfers (#16741 T2 P7) (#24405)"

This reverts commit 60c9326.

* Revert "fix(runtime): project the PTY incarnation onto mobile session tabs (#24413)"

This reverts commit 99e0303.

* Revert "feat(daemon): tag daemon stream data with the PTY incarnation id (#16741 T2 P4a) (#24402)"

This reverts commit 817af76.

* Revert "feat(ssh): port the SSH connection work ledger and transport close ledger (#16741 T2) (#24210)"

This reverts commit c991893.

* Revert "feat(relay): fence and drain file and git response streams on shutdown (#24185)"

This reverts commit dc08ffe.

* Revert "refactor(runtime-rpc): extract the Node WebSocket lifecycle; opt-in pinned port (#24186)"

This reverts commit a789233.

* Revert "feat(relay): route relay handlers through work admission; producer publication drain (#24181)"

This reverts commit 0b812bd.

* Revert "feat(relay): land the #16741 T1 seam (work drain, publication drain, release gate) (#24156)"

This reverts commit 3aa2d3a.

---------

Co-authored-by: m4air <m4air@Mac.localdomain>
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