xiaoxue_admin
/
orca
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
orca/docs/ssh-handler-reregistration-...

12 KiB
Raw Blame History

SSH Handler Re-registration Port Forwards

Problem

Issue #2932 reported that macOS window reactivation can re-run attachMainWindowServices, which calls registerSshHandlers again (src/main/window/attach-main-window-services.ts:83).

Before this change, registerSshHandlers removed and re-added IPC handlers but also replaced the module-level connectionManager and portForwardManager (src/main/ipc/ssh.ts:438, src/main/ipc/ssh.ts:439). activeSessions remains module-global (src/main/ipc/ssh.ts:63), so live relay sessions kept references to the old port-forward manager while new IPC handlers read a fresh empty one.

The visible failure is:

  1. Connect an SSH target.
  2. Add a local port forward.
  3. Close all windows on macOS while the app process remains alive.
  4. Reactivate Orca, causing SSH handlers to register again.
  5. ssh:listPortForwards returns an empty list, ssh:removePortForward cannot remove the old forward id, ssh:addPortForward/ssh:updatePortForward can fail because the fresh connection manager has no live connection, and ssh:disconnect does not close the old SSH connection or local listener. Re-adding the same local port fails because the old server remains bound.

Root Cause

SSH handler registration mixes two lifetimes:

  • Process-lifetime session state: active SSH connections, relay sessions, port listeners, relay lost backoff, reset/connect in-flight maps.
  • Window-lifetime callback state: getMainWindow and renderer IPC handlers.

The previous re-registration path preserved activeSessions but replaced the managers that sessions and IPC handlers must share. Port-forward IPC operations use portForwardManager (src/main/ipc/ssh.ts:992, src/main/ipc/ssh.ts:1047, src/main/ipc/ssh.ts:1056), and disconnect/terminate cleanup also uses that variable (src/main/ipc/ssh.ts:750, src/main/ipc/ssh.ts:814). After replacement, those operations no longer targeted the manager that owns the live local servers. Replacing connectionManager also strands the live SshConnection objects: existing relay sessions still hold their current connection, but new IPC handlers and getSshConnectionManager() see an empty manager.

Non-goals

  • Do not change relay protocol, remote deployment, or SSH transport behavior.
  • Do not redesign port-forward persistence or enrichment.
  • Do not change renderer UI.
  • Do not introduce a second SSH service layer.
  • Do not force-dispose live SSH sessions merely because a window was recreated.

Design

  1. Preserve process-lifetime managers across handler re-registration. Instantiate SshConnectionManager and SshPortForwardManager only when absent; later registerSshHandlers calls reuse the existing instances.

  2. Refresh every live callback owner on re-registration. This is required; a plain connectionManager ??= new SshConnectionManager(callbacks) is not enough.

    • SshConnectionManager must update callbacks used by both future and existing SshConnection objects, either via explicit setCallbacks methods on manager/connection or via a stable callback proxy whose implementation is mutable.
    • Existing SshRelaySession objects must refresh getMainWindow, store, runtime, and detected-port callback references. Event handlers must call the current callback at event time; do not capture the old getMainWindow in long-lived provider callbacks.
    • The credential-request tracking set must not be per-registration if live connections can switch callbacks during an in-flight ssh:connect.

  3. Re-register IPC handlers and dependent global listeners on every call. ipcMain handlers, advertised URL refresh, credential IPC, browse handler, and power-monitor listeners are window-registration concerns and should still point at the latest window.

  4. Preserve existing explicit teardown behavior. ssh:disconnect, ssh:terminateSessions, ssh:removeTarget, reset, and double-connect cleanup must still remove forwards through the shared manager before detaching or disposing sessions.

  5. Add regression tests in src/main/ipc/ssh.test.ts. Connect a target, add a mocked port forward, call registerSshHandlers again, then assert:

    • ssh:listPortForwards still returns the original forward.
    • ssh:removePortForward can remove the original id.
    • ssh:addPortForward/ssh:updatePortForward still use the original live connection.
    • A second re-registration followed by ssh:disconnect still calls removeAllForwards and disconnect on the original shared managers.
    • State, credential, PTY, and detected-port callbacks from an existing live session publish to the newest window after re-registration.

Data Flow

  • First registration:

    • registerSshHandlers(store, getWindowA) creates store wrapper, connection manager, port-forward manager, handlers, listeners, and current callback environment.
    • ssh:connect creates a relay session with the shared port-forward manager.
    • ssh:addPortForward stores a local server in that same manager.

  • Window reactivation:

    • registerSshHandlers(store, getWindowB) removes/re-adds IPC handlers.
    • Existing managers are reused.
    • Existing connection and relay-session callback owners are refreshed to the latest store/runtime/window environment.
    • New handlers close over getWindowB and call the same managers.

  • Cleanup:

    • ssh:removePortForward and ssh:disconnect operate on the same manager that owns the live forward, then broadcast through the latest window.

Edge Cases

  • Re-registration while a target is connected and has active port forwards.
  • Re-registration while ssh:connect, restorePortForwards, reset, reconnect, or disconnect is in flight. The operation must not split credential tracking or create a session that holds stale callbacks.
  • Re-registration while no targets are connected.
  • Re-registration after the store object changes. Either update existing relay sessions to use the new store/runtime or document and test the stronger invariant that production re-registration always passes the same process store/runtime.
  • Re-registration after the window changes. All broadcasts, credential prompts, PTY events, detected-port events, advertised URL refreshes, relay-loss state changes, and terminal relay errors must use the newest getMainWindow.
  • Disconnect after re-registration must release old local ports.
  • ssh:connect after window reactivation must be idempotent when the existing session is already ready and healthy: return the connected state without tearing down forwards. Explicit reset/reconnect or non-ready replacement paths must still await old port teardown before restoring forwards.
  • getSshConnectionManager() consumers must continue to see live connections after re-registration.
  • Test isolation must not depend on module-singleton state leaking between tests. Add explicit reset/teardown support if preserving managers makes beforeEach(registerSshHandlers) insufficient.
  • SSH and relay paths must keep working for remote targets; the fix must not assume local filesystem or local-only execution.
  • Windows/Linux remain unaffected: re-registration can still happen during development or future window lifecycles, and the fix must avoid path or platform assumptions.

Test Plan

  • Unit: pnpm vitest run --config config/vitest.config.ts src/main/ipc/ssh.test.ts
    • Add regression coverage for list/remove/disconnect after handler re-registration.
    • Add coverage that add/update after re-registration uses the still-live connection manager connection, not a fresh empty manager.
    • Add coverage that existing connection/session callbacks publish to a second mock window after re-registration.
    • Add an in-flight connect or credential-request test if callback refresh uses mutable callback objects.
    • Existing connect, disconnect, reset, relay-loss, and terminate tests cover adjacent lifecycle behavior.
  • Typecheck: pnpm typecheck.
  • Lint: pnpm lint.
  • Electron/SSH validation: use an existing SSH target such as openclaw 2 if available in the running app, add a disposable local port forward, trigger window/service re-registration by closing and reopening the main window on macOS, then verify the forward remains listed and removable. IPC/unit tests are supporting evidence only; if the golden path cannot be exercised safely, halt before PR and report the missing evidence.

UI Quality Bar

Not UI-visible. No layout, copy, or visual styling changes are expected. The only user-visible expectation is that existing SSH port-forward rows remain present and actionable after window reactivation.

Review Screenshots

  1. SSH target connected with a port forward listed before re-registration.
  2. Same SSH target after window reactivation, showing the same port forward still listed.
  3. Same SSH target after removing the port forward, showing it gone without an error.

Rollout

  1. Add the focused regression test to prove the current lifecycle bug.
  2. Change SSH handler registration to reuse process-lifetime managers.
  3. Run the focused test, then typecheck and lint.
  4. Validate in Electron against an SSH target if feasible; otherwise halt before PR if the golden-path SSH UI cannot be exercised.

Lightweight Eng Review

  • Scope: reduced to SSH IPC lifecycle only. No relay, renderer, or persistence redesign is needed because the broken boundary is manager replacement during handler re-registration.
  • Architecture/data flow: process-lifetime managers stay module-level and are reused; window-lifetime IPC handlers/listeners are refreshed; existing connection and relay-session callback owners must also be refreshed or proxied so live events target the current BrowserWindow.
  • Failure modes covered:
    • Active forwards becoming invisible after re-registration.
    • ssh:removePortForward missing the old forward id.
    • ssh:addPortForward/ssh:updatePortForward failing against a fresh empty connection manager.
    • ssh:disconnect failing to close old SSH connections and local listeners after re-registration.
    • Store/window/runtime callback refresh after re-registration.
    • Re-registration during in-flight connect/reset/reconnect.
    • No-session re-registration continuing to work.
  • Test coverage required:
    • Unit in src/main/ipc/ssh.test.ts for connect/add/list/remove across registerSshHandlers calls.
    • Unit in src/main/ipc/ssh.test.ts for disconnect cleanup after re-registration.
    • Unit in src/main/ipc/ssh.test.ts for existing live callbacks reaching the newest window after re-registration.
    • Unit in src/main/ipc/ssh.test.ts for no-session re-registration and test teardown/reset of module singletons.
    • Existing lifecycle tests for reset, terminate, relay loss, and sleep remain adjacent coverage.
  • Performance/blast radius: no material startup or IPC cost. Reusing managers avoids leaked runtime state and does not add polling, watchers, or cross-process calls. Callback refresh is O(number of live SSH connections and sessions) per registration, which should be tiny.
  • UI quality bar: not UI-visible; preserve existing SSH port-forward UI state rather than changing layout or copy.
  • Required review screenshots:
    1. Connected SSH target with active port forward before re-registration.
    2. Connected SSH target with same port forward after re-registration.
    3. Connected SSH target after removing that forward.
  • Feasibility: one-time manager creation is feasible only with callback refresh for existing SshConnection and SshRelaySession instances. If that refresh proves larger than expected, prefer a stable callback proxy over recreating managers; do not dispose live sessions just to make callback ownership easier.
  • Residual risks: Electron validation may be constrained by availability of an existing SSH target and by avoiding live-user port collisions. If the golden path cannot be exercised safely, stop before opening a PR and report the missing manual evidence.