dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

feat(executors): detect and tear down dead SSH session transports (#3581)

Browse Source 
Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
2026-09-13 06:00:53 +08:00
committed by GitHub
parent f150dd32d1
commit aebb01e714
21 changed files with 3312 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/plans/ssh-transport-liveness/plan.md
+112
View File
@@ -0,0 +1,112 @@
---
created: 2026-09-09
status: done
requirements:
- REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-001
- REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-002
system_design:
- ../../specs/executors/system-design/ssh-transport-liveness.md
legacy_specs: []
---
# Implementation plan: SSH session transport liveness
## Overview
A remote runner's SSH tunnel can die silently over a lossy link and stay dead for
hours: the local port forward keeps accepting connections, so agentctl's WebSocket
hangs waiting on a response that will never arrive instead of erroring. Nothing in
`apps/backend/internal/agent/runtime/lifecycle` probed the transport for liveness;
the keepalive plumbing had been deliberately removed pre-merge (PR #927) after
orphaned goroutines surfaced under e2e fault injection.
This plan restores a per-session SSH keepalive watchdog, owned and bounded by the
session lifecycle so it cannot leak the way the removed version did, and wires its
failure into transport teardown so a dead tunnel becomes a fast, non-retryable
error instead of an infinite hang.
## Scope
In scope: a `keepalive@openssh.com` probe loop per SSH session, liveness
classification (probe reply, probe error, or transport end), transport teardown on
loss (mark, close forward, close client, warn once), disposal-path ownership so a
watchdog cannot outlive its session, and the `GetRemoteStatus` reclassification of
a transport lost mid-probe.
Out of scope (see the requirements doc for the full list): reconnect/re-dial, task
or agent state transitions, an operator-facing config toggle, the two downstream
diagnosability follow-ups (empty-payload-reads-as-success; unbounded agent
backoff), restructuring the serial remote-status poll loop, hardening
`SSHPortForwarder.Close` itself, shortening remote-command timeouts, other
executors, application-level health/ping above the transport, and multi-hop
ProxyJump chains.
## Technical approach
Add `executor_ssh_keepalive.go`: a watchdog goroutine pair (probe + deadline timer)
started under the executor mutex alongside the session record (insert-if-absent),
using fixed package-level tuning values (probe interval, unresponsive threshold,
disposal timeouts) with no runtime feature flag. A `keepalive@openssh.com` reply of
any kind counts as liveness and resets the deadline; a probe error or transport EOF
declares the transport lost immediately. Idempotent teardown runs through
per-handle once-guards so a stop racing a declaration cannot double-dispose.
Extend `executor_ssh.go`'s disposal sequence to close the forward and client and
mark the session transport-lost exactly once, and extend `GetRemoteStatus` to read
the transport marker and client in one critical section so a status read racing a
declaration cannot return a stale `agentctl-down` instead of `disconnected`.
Existing patterns: `executor_ssh_connection.go` for the `golang.org/x/crypto/ssh`
client/session lifecycle this watchdog attaches to, and
`executor_ssh_stop_resume_test.go` / `ssh_fake_server_test.go` for the fake SSH
server harness used to simulate a silent transport.
## Tests
All tests live in `apps/backend/internal/agent/runtime/lifecycle`, TDD'd against
the fake SSH server harness (no new Playwright E2E: no `apps/web/` change, no
schema/migration, no event-bus subscriber).
| Criteria | Evidence |
| --- | --- |
| AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1 through .4, .8 through .11 | `executor_ssh_keepalive_test.go`: watchdog start-under-mutex, probe/reply/error classification, deadline arithmetic, tuning-value production defaults |
| AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5 through .7 | `executor_ssh_transport_teardown_test.go`: mark-then-close-forward-then-close-client sequence, single warning, post-teardown remote-status reclassification |
| AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1 through .15 | `executor_ssh_keepalive_test.go` + `executor_ssh_stop_resume_test.go`: ownership, stop-vs-declare race precedence, once-guard composition, resume-metadata completeness, backstop timeout |
| Watchdog leak safety | `goleak_test.go`: no watchdog goroutine survives session disposal |
## End-to-end evidence
No new Playwright E2E. Confirmed across five spec re-checks (most recently Spec
Review round 3): the changed-file list is backend + specs only, with no frontend,
schema, or event-bus surface.
## Work orders
- [x] [Task 01: Detect and tear down dead SSH session transports](task-01-ssh-transport-watchdog.md)
Single work order; implemented sequentially, no subagents.
## Verification results
Implementation is complete across 5 Build rounds, 5 Testing rounds, and 4 Review
rounds (hard cap). Final receipts (branch head prior to this documentation
addition): `go build ./...` clean; full SSH/keepalive suite green under `-race`;
`golangci-lint` 0 new issues; `gofmt` / spec-lint / architecture-lint clean;
package coverage 79.0%. See PR #3581 for the full round-by-round history and CI
receipts.
## Risks
- The frozen spec accepts three residual internal-consistency gaps (F35/F36/F37)
with recorded readings rather than spec amendments; see the requirements doc's
cross-references for the exact boundary each reading draws.
- `resetTrackedManagedBrokerResume`'s two-critical-section shape is the frozen
design's own mandated shape (read+check before preflight, removal after), not a
branch-introduced bug; a concurrent stop can still interleave with an in-flight
preflight and surface a generic error instead of `ErrSSHTransportLost`. Recorded
as carried-forward, non-blocking test-rigor residual, not fixed by this plan.
## References
- [Requirements](../../specs/executors/requirements/ssh-transport-liveness.md)
- [System design](../../specs/executors/system-design/ssh-transport-liveness.md)
docs/plans/ssh-transport-liveness/task-01-ssh-transport-watchdog.md
+152
View File
@@ -0,0 +1,152 @@
---
id: "01-ssh-transport-watchdog"
title: "Detect and tear down dead SSH session transports"
status: done
wave: 1
depends_on: []
plan: "plan.md"
requirements:
- REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-001
- REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-002
acceptance_criteria:
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.2
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.4
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.6
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.8
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.9
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.10
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.11
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.2
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.3
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.5
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.6
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.7
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.9
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.10
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.11
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.12
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.13
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.14
- AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.15
system_design:
- ../../specs/executors/system-design/ssh-transport-liveness.md
---
# Task 01: Detect and tear down dead SSH session transports
## Summary
Restore a `keepalive@openssh.com` liveness watchdog on the per-session SSH client,
owned and torn down by the session lifecycle so it cannot leak the orphaned
goroutines that got the original plumbing removed in PR #927. Wire watchdog
failure into transport teardown so a dead tunnel closes the client, which makes
`ChannelBackendClient.RequestPayload`'s blocked wait fail fast with a clean,
non-retryable error instead of hanging indefinitely.
## In scope
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_connection.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_scripts.go`
- `apps/backend/internal/agent/runtime/lifecycle/manager_startup.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_docker.go` and
`executor_sprites.go` (shared disposal helper adjustments only)
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive_test.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_transport_teardown_test.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go`
- `apps/backend/internal/agent/runtime/lifecycle/executor_ssh_lifecycle_test.go`
- `apps/backend/internal/agent/runtime/lifecycle/goleak_test.go`
- `apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go`
## Out of scope
Reconnect/re-dial, task or agent state transitions, an operator config/runtime
toggle, the two downstream diagnosability follow-ups filed separately
(empty-payload-reads-as-success; unbounded agent backoff), restructuring the
serial remote-status poll loop, hardening `SSHPortForwarder.Close` itself,
shortening remote-command timeouts, other executors, application-level
health/ping above the transport, and multi-hop ProxyJump chains.
## Acceptance
- A session's watchdog starts in the same critical section as its record
(insert-if-absent) and probes every 15s; any reply resets the deadline.
- 45s of silence, a probe error, or transport end declares the transport lost and
runs the six-step disposal sequence: mark, skip/abandon remote commands per the
unresponsive condition, close the forward, close the client, warn once, join.
- A stop request wins a three-way race against a declaration and a reset; teardown
is idempotent under two per-handle once-guards; the record stays insert-if-absent
and is not removed by teardown itself.
- `GetRemoteStatus` reads the transport marker and client in one critical section
so a probe-time loss is reported as `disconnected`, not `agentctl-down`.
- A lost create-vs-resume race adopts the winner's full resume metadata, including
runtime API tunnel fields.
- No watchdog goroutine survives session disposal (`goleak`).
## Verification
Run from `apps/backend`:
```bash
go build ./...
go test ./internal/agent/runtime/lifecycle/... -run 'TestSSH' -race -count=1
golangci-lint run ./internal/agent/runtime/lifecycle/...
gofmt -l internal/agent/runtime/lifecycle
```
## Files likely touched
See "In scope" above.
## Dependencies
None.
## Risks
The link this watchdog defends against is lossy (12.5% packet loss observed in
production); tuning values are fixed, not configurable, per the frozen design.
## Parallelism
`sequential`
## Inputs
- [Requirements](../../specs/executors/requirements/ssh-transport-liveness.md)
- [System design](../../specs/executors/system-design/ssh-transport-liveness.md)
- [Plan](plan.md)
- PR #927 (prior keepalive removal, cited for its e2e fault-injection rationale)
## Results
Implemented the watchdog, teardown sequence, and ownership guarantees across 5
Build rounds, 5 Testing rounds, and 4 Review rounds (hard cap). Two incidental
bugs were fixed during TDD: the prober's replies/error channel split, and an
upstream `x/crypto/ssh` v0.52.0 deadlock (bumped to v0.57.0). Production bugs
found and fixed during Build/Review: `buildInstanceForLostRace` dropped, then
still didn't fully adopt, the resume winner's tunnel metadata (fixed across
rounds 3-4, mirroring `buildResumedInstance`); `runLoop`'s stop-beats-both
precedence wasn't Go-select-guaranteed (fixed with `stopRequested()`
reverification before every declare/handle site); a flaky Testing-round-4 test
raced a classification threshold against a fixed sleep (fixed with a
lock-guarded poll).
Verification (branch head prior to this documentation addition):
- `go build ./...`: clean.
- `go test ./internal/agent/runtime/lifecycle/... -race`: full SSH/keepalive
suite green; package coverage 79.0%.
- `golangci-lint run`: 0 new issues.
- `gofmt -l`, spec-lint, architecture-lint: clean.
See PR #3581 for full round-by-round receipts, the rebase reconciliation with the
concurrent SSH Runtime API Tunnel feature, and CI history.
docs/specs/executors/README.md
+31
View File
@@ -27,6 +27,37 @@ failure and recovery contracts.
- Task ownership of worktrees belongs to the [task system](../tasks/README.md).
- Desktop process supervision belongs to the [desktop system](../desktop/README.md).
## Specification map
### Requirements
- [Executor-Profile Environment Precedence](requirements/executor-profile-env-precedence.md)
- [Port collision and backend ownership safety](requirements/port-collision-safety.md)
- [SSH Executor](requirements/ssh-executor.md)
- [Remote SSH task-directory reclamation](requirements/remote-task-directory-reclamation.md)
- [Kubernetes worker presets](requirements/kubernetes-worker-presets.md)
- [Kubernetes startup timing](requirements/kubernetes-startup-timing.md)
- [Kubernetes retained compute visibility](requirements/kubernetes-retained-compute.md)
- [SSH Session Transport Liveness](requirements/ssh-transport-liveness.md)
### System design
- [Executor-Profile Environment Precedence System Design Part 1](system-design/executor-profile-env-precedence-01.md)
- [Executor-Profile Environment Precedence System Design Part 2](system-design/executor-profile-env-precedence-02.md)
- [Executor-Profile Environment Precedence System Design Part 3](system-design/executor-profile-env-precedence-03.md)
- [Executor-Profile Environment Precedence System Design Part 4](system-design/executor-profile-env-precedence-04.md)
- [Executor-Profile Environment Precedence System Design Part 5](system-design/executor-profile-env-precedence-05.md)
- [SSH Executor](system-design/ssh-executor.md)
- [Remote SSH task-directory reclamation](system-design/remote-task-directory-reclamation.md)
- [Kubernetes worker presets](system-design/kubernetes-worker-presets.md)
- [Kubernetes startup timing](system-design/kubernetes-startup-timing.md)
- [Kubernetes retained compute visibility](system-design/kubernetes-retained-compute.md)
- [SSH Session Transport Liveness](system-design/ssh-transport-liveness.md)
## Migration record
Migration remains in progress while legacy source detail is extracted from the
docs/specs/executors/requirements/ssh-transport-liveness.md
+328
View File
@@ -0,0 +1,328 @@
---
status: active
system: executors
created: 2026-09-09
owners:
- kandev
---
# SSH Session Transport Liveness Requirements
## Overview
An SSH executor session owns one SSH connection to its remote host for its whole
life. Every agent operation on that session — the HTTP calls to the remote
controller and the long-lived agent event WebSocket — travels through a local
port forward riding that one connection.
Nothing checks that the connection is still alive. When the underlying network
path dies without a TCP reset (a VPN re-key, a NAT table eviction, a lossy mesh
link), the connection becomes a zombie: the local forward keeps accepting and
the remote end never answers. Requests do not fail, they hang, and stay hung
until something else intervenes.
Measured 2026-09-09 against a runner over a VPN link with 12.5% packet loss: an
SSH-executor session stopped delivering traffic for roughly two hours while sessions on other executors, on the same backend, continued
uninterrupted. It recovered only when re-established on a new local forward
port. Nothing detected the loss and nothing reported it.
This capability makes a dead session transport observable in bounded time: it
probes the connection and closes it when it stops answering, turning an unbounded
hang into an ordinary connection error the layers above already report.
There is a second contract here: the watchdog must be owned by the session it
belongs to and gone once that session is gone. Equivalent plumbing
existed once and was removed before it ever shipped, because its background
goroutines outlived their connections under fault injection, so the ownership
rules are contract, not implementation detail.
## Terminology
- **Session SSH client:** the single SSH connection an SSH executor session
owns from launch or resume until the session is stopped.
- **Transport liveness probe:** a request sent on a session SSH client that
requires the remote host to answer, used only to learn whether the connection
carries traffic.
- **Completed probe reply:** an answer received for a probe. An answer that
declines the request still proves the connection carries traffic and counts
as completed.
- **Liveness deadline:** the longest a session SSH client may go without a
completed probe reply before its transport is declared lost.
- **Silence interval:** for a session SSH client with a running watchdog, the
time elapsed since the later of that watchdog's start and its most recent
completed probe reply. It is what the liveness deadline is measured against
and the value reported in the teardown log. A session with no running watchdog
has none.
- **Unresponsive transport:** a session SSH client whose silence interval
exceeds twice the probe interval, meaning a probe has been outstanding for
longer than a further whole interval. A transport is unresponsive for the
whole span between twice the probe interval and the liveness deadline, before
it is declared lost. The design says why.
- **Transport teardown:** closing a session's local port forward and its
session SSH client after the transport is declared lost.
- **Watchdog:** the background activity that sends probes for one session SSH
client and performs transport teardown for it.
## Prior art
**Our own prior reasoning (wiki).** Searched: collection `wiki` via the QMD MCP
server (qmd 2.8.3), whose `status` resolved a configured vault path with 459
documents — a healthy semantic query, not a silent grep fallback. Two queries: a typed
`lex`+`vec` pair on keepalive / watchdog / dead-connection terms, and a plain
query on liveness probes and background goroutine ownership. **The wiki returned
nothing useful** — the top hits do not touch connection liveness, so there is no
recorded prior position to follow or depart from.
**What others shipped (saas-kb).** Searched: `fsm-docs-server` MCP,
`search_fsm_docs` with `category: "ai_sdlc"`, on SSH-tunnel keepalive and
agent-session heartbeat phrasings, plus a Paperclip-scoped follow-up and one
`get_fsm_doc` on its sandbox-providers reference. Coverage is thin (best
relevance 0.016); two data points are real:
- **Kiro** keeps an SSH tunnel up with an out-of-process supervisor (a macOS
LaunchAgent that restarts it): supervise from outside, not probe from within.
- **Paperclip** (v2026.517.0) emits a keepalive on its streaming execution
endpoint **every 15 seconds** so proxies do not idle out, and tunes probe
timeouts to 45, 60 and 90 seconds to cover cold starts "without masking real
hangs".
**What we are doing differently.** Both keep a connection *alive*; this
capability *detects that one is dead* and closes it. Kiro's shape is wrong here:
our tunnel process is fine and the forward is still listening, so only the party
holding the connection can tell it has stopped answering.
Paperclip's numbers corroborate both constants we fix: 15 seconds is a shipped
cadence for a long-lived tunnel, and a deadline near 45 seconds a shipped choice
for "long enough not to fire on a slow link, short enough to still detect".
## Requirements
### REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-001: Session transport liveness detection
**Intent:** A session whose SSH connection has silently stopped carrying
traffic must be detected and closed within a bounded time, so that work on that
session fails with an error instead of hanging indefinitely.
**User story:** As a user running an agent on my own remote machine, I want a
session whose connection has died to fail promptly, so that I find out within a
minute instead of hours later.
#### Acceptance criteria
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1:** When an SSH executor session
takes ownership of a session SSH client, on either a new launch or a resume
of an existing remote controller, the system shall start one watchdog for
that client after the session has been recorded in the executor's tracked
sessions, in the same critical section in which that record is made, so that
no disposal path can run between the record and the start.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.2:** While a watchdog is running,
the system shall send transport liveness probes on its session SSH client one
at a time, spaced by the probe interval, and shall not start a probe while an
earlier one on the same client is still outstanding. Any completed probe reply
shall count as evidence of liveness, whether the host accepted or declined
it.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3:** When a session SSH client's
silence interval becomes strictly greater than the liveness deadline, the
system shall declare that session's transport lost and perform transport
teardown. A silence interval exactly equal to the liveness deadline shall not
declare transport loss. The comparison shall be made from the recorded time of
the most recent completed probe reply, not from the order in which a reply and
a deadline expiry became observable; a completed reply not yet accounted for
shall be accounted for first.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.4:** While a watchdog is running,
when a transport liveness probe returns an error, or the session SSH client's
transport terminates on its own, the system shall declare that session's
transport lost and tear it down without waiting for the liveness deadline. A
probe error observed after that session's watchdog has been stopped shall not
declare transport loss.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5:** Transport teardown shall close
the session's local port forward first and the session SSH client second. An
error returned by either close shall not prevent the other close, nor prevent
the teardown from completing and recording its result. A close that returned
an error shall be named in the teardown warning required by
`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.9`, so that a teardown which could
not fully release its handles is distinguishable in the log from one that did.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.6:** After transport teardown for a
session in which the local port forward's close returned no error, a new TCP
connection to that session's local forward port shall be refused rather than
accepted. When that close returned an error, teardown still completes per
`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5` and the port is left as the failed
close produced; the two never both bind.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7:** After transport teardown for a
session, remote status for that session shall report the state
`disconnected` with an error message identifying transport loss, and shall not
send any command over the closed client.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.8:** When a session SSH client
reached its host through a ProxyJump bastion, transport teardown shall also
release the bastion connection.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.9:** When transport teardown
completes, the system shall record one warning-level log naming the executor
instance identifier, the remote host, and the session SSH client's silence
interval. That is the same quantity
`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3` compares against the deadline, so
it is defined even when no probe reply ever completed.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.10:** The probe interval shall
default to 15 seconds and the liveness deadline shall default to 45 seconds.
The system shall not expose either as operator configuration and shall not add
a runtime feature toggle.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.11:** Transport teardown shall be
local to the Kandev backend: no command on the remote host, no stop of the
remote controller, and the session's persisted resume metadata left unchanged,
so a later resume can re-attach to a controller still running.
### REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-002: Watchdog ownership and termination
**Intent:** Liveness detection must not be paid for with background work that
outlives the connection it was watching. The watchdog belongs to one session,
starts once, and is gone when that session is gone.
#### Acceptance criteria
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1:** A session shall have at most
one watchdog at any time. Recording a session in the executor's tracked
sessions shall not replace an existing record for the same executor instance
identifier. When a record already exists at that point, the system shall leave
the existing record and its watchdog in place, start no additional watchdog,
and close the newly obtained port forward and session SSH client that would
have been recorded, so neither a connection nor a watchdog is left unowned. The caller that lost shall proceed with the existing session on the
same terms as any other reuse, including the refusal required by
`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.14` when that session's transport has
been declared lost.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.2:** An SSH connection not owned by
a session shall not start a watchdog. This covers the connection test used when
configuring an executor, remote task-directory reclamation, and the untracked
dial used to reset a credential-broker-backed resume.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.3:** When a session has no session
SSH client, the system shall not start a watchdog and shall not report an
error.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4:** When a session is stopped, the
SSH executor is closed, or a tracked session is discarded while resetting a
credential-broker-backed resume, the system shall stop that session's watchdog,
and no watchdog activity for it shall remain running once that stop, close, or
reset call returns. Those three are the complete set of
paths that dispose of a tracked session. Stopping a watchdog that has already
stopped, or one that was never started, shall make no further calls and shall
report success. This guarantee binds the disposal call that removed the
session from the executor's tracked sessions. When two disposal calls run
concurrently for the same session, the one that does not remove it finds no
tracked session and shall return success immediately, closing no handle and
not waiting for the other call's watchdog; the removing call remains bound by
the guarantee above.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.5:** Transport teardown for a
session shall run at most once. A second teardown for the same session shall
close nothing further and shall report success.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.6:** Transport teardown shall not
remove the session from the executor's tracked sessions; removal remains the
disposal paths' responsibility.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.7:** When transport teardown and a
session stop run concurrently for the same session, the local port forward
shall be closed exactly once, the session SSH client shall be closed exactly
once, and both operations shall complete without error.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8:** When a disposal path would
issue a remote command over a session SSH client whose transport has been
declared lost, or whose transport is unresponsive, the system shall skip that
command. For a session stop this means skipping both the remote cleanup script
and the remote controller stop, and completing the stop without error. The
skips shall be decided from a single reading covering both the transport-lost
marker and the silence interval, taken before that path issues any remote
command and no later than the session's removal from the executor's tracked
sessions; that one reading shall govern every remote command on that disposal
path, so no path can run one remote operation and skip another. A teardown
firing after that reading shall not cause a stop to return an error. Every
remote command a disposal path issues shall run under its own remote-cleanup
timeout, including one that has none today. When a disposal does issue a remote
command and that command has not completed within that timeout, the system
shall close the session SSH client without waiting for the command to finish,
mark that client transport-lost, treat the command as failed, and continue the
disposal. The disposal shall not return until the abandoned command has ended;
closing the client bounds that wait, because the close is what releases a
command blocked opening its channel. When the session has no running watchdog,
neither skip condition holds and the disposal issues every remote command, still
bounded by the timeout above.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.9:** When the probe interval or the
liveness deadline is not positive, or the deadline is not greater than twice
the interval, the system shall not start a watchdog and shall record one
debug-level log naming the executor instance identifier.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.10:** When a session is stopped
while its transport is still alive, the system shall not declare that
session's transport lost and shall not record the transport-loss warning for
it.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.11:** When a session is stopped
while its transport is unresponsive but its liveness deadline has not yet
expired, the stop shall complete without waiting for that deadline, without
waiting for any outstanding probe to be answered, and without issuing any
remote command over that session's SSH client.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.12:** When transport teardown
completes for a session, the system shall send no further transport liveness
probes on that session's SSH client, whether or not that session is
subsequently stopped.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.13:** When a session whose transport
is answering normally is disposed of between two probes, the disposal shall not
wait for the watchdog's next probe to be sent, and the time it spends waiting
for that session's watchdog activity to finish shall total less than one probe
interval. This bounds only the waiting the watchdog imposes; the disposal's own
remote commands are not bounded by it. It is observed on a disposal path with
no remote commands, where the whole call time is that waiting.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.14:** When the instance-creation
path finds an already-tracked session for the requested executor instance
identifier whose transport has been declared lost, it shall not return an
executor instance for it, and shall report an error identifying transport
loss.
- **AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.15:** When resetting a
credential-broker-backed resume discards a tracked session whose transport is
lost or unresponsive, the reset shall skip both its credential-broker
reachability preflight and the remote controller stop, leave
the session's persisted resume metadata unchanged, and report an error
identifying transport loss. It shall report that same error, rather than a
generic remote-stop or broker-unreachable failure, when it does issue its
preflight or its remote controller stop and that call fails because the session
SSH client was closed after the reading required by
`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8`, whether by a teardown or by that
criterion's own timeout. The transport-lost mark that criterion requires is what
distinguishes those two causes from a host that refused the call.
## Out of scope
Each exclusion below is a deliberate contract boundary, not an oversight.
- **Reconnecting a session whose transport was lost.** Teardown ends the
transport and does not re-dial, re-forward, or re-attach. The SSH executor is
resumable, so a later resume re-establishes the connection.
- **Task, session, and agent state transitions.** This capability does not fail,
retry, restart, or re-queue the agent's work. It closes the transport; existing
disconnect handling decides what the user sees.
- **Operator configuration and feature toggles.** The interval and deadline are
fixed internal values (`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.10`): no config
key, environment variable, or runtime flag. A link failing the deadline has been
unusable for 45 seconds; nothing to tune.
- **The two downstream diagnosability defects.** An empty response payload read
as success, and the agent's unbounded reconnect backoff, are on their own cards;
this capability neither fixes nor depends on them.
- **The shape of the remote-status polling loop.** It visits sessions one at a
time and can be delayed by one that is not answering. This capability bounds
that delay, because a lost transport is closed and short-circuits status
(`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7`), but does not restructure it.
- **Hardening `SSHPortForwarder.Close` itself.** It is idempotent for sequential
callers and races for concurrent ones. This capability serialises every close
of a session's forwarder through that session's once-guard, so no path it
introduces reaches the race, and making the forwarder internally safe would
change shared code for no behaviour this contract observes. Named because a
reader will notice it, not as deferred work.
- **Shortening remote-command timeouts.** Disposal skips its remote commands when
the transport is lost or unresponsive, and abandons one that outlives its own
timeout, both under `AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8`. Neither
shortens those timeouts: a transport answering when the reading is taken runs
its commands on the same clock as today.
- **Other executors.** Local, Docker, Kubernetes and Sprites executors have their
own transports and are unchanged.
- **Application-level liveness above the transport.** Health, ping, or heartbeat
traffic on the controller's endpoints is a separate concern from whether the
SSH connection carries bytes.
- **Ordered or persisted collections.** This capability introduces no query,
list, or stored record, so no sort order or tiebreak applies. Its only
orderings are `AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5`'s teardown sequence
and the disposal sequence in the design.
- **Multi-hop ProxyJump chains.** One bastion hop is the supported topology.
## System design
See [SSH session transport liveness](../system-design/ssh-transport-liveness.md).
docs/specs/executors/system-design/ssh-transport-liveness.md
+567
View File
@@ -0,0 +1,567 @@
---
status: current
system: executors
requirements:
- REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-001
- REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-002
---
# SSH Session Transport Liveness System Design
## Purpose and boundaries
The executor system owns the SSH connection a session runs on, so it owns the
question of whether it is still alive. Nothing above it can: the local port
forward is a listener in this process and keeps accepting whether or not the far
end is reachable, so every layer above sees an open socket and waits.
This design uses, and does not own: the agent runtime's controller client and
event stream (they react to a closed connection, not decide one is dead); the
lifecycle manager's remote-status poll (it reads status, not drives teardown);
host-key trust and target resolution (teardown changes neither).
## Requirement mapping
| Requirement | Design section |
| --- | --- |
| `REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-001` | [Probe mechanism](#probe-mechanism), [Control flow](#control-flow), [Failure and recovery](#failure-and-recovery) |
| `REQ-EXECUTORS-SSH-TRANSPORT-LIVENESS-002` | [Components and responsibilities](#components-and-responsibilities), [Ownership and termination](#ownership-and-termination) |
Acceptance criteria are cited short: `-001.4` is
`AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.4`.
## Components and responsibilities
- **Session state** (`sshSessionState`, `apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go`).
Gains the watchdog handle, a transport-lost marker, the timestamp of the last
completed probe reply, and **two independent once-guards: one for the local
port forward and one for the session SSH client**. The executor mutex guards
the first three, as it already guards the session map; the once-guards
synchronise themselves, because every close through one runs with that mutex
released.
Two guards, not one spanning both handles: disposal paths issue remote commands
over the client *between* the two closes, and a combined "close both" guard
would either close the client before those commands, stranding them, or defer
the forwarder close until after them, inverting an ordering the stop path
guarantees. Each handle gets its own guard, closing exactly once whichever of
teardown or disposal reaches it first.
- **Transport watchdog** (new file, `executor_ssh_keepalive.go`). One per session
SSH client. Owns its probe cadence, its deadline, and the decision to declare
the transport lost. Exposes start, stop-deciding, and await-probe-exit; the last
two are the first and last steps of one disposal, always in that order, with the
two handle closes and the path's own remote commands in between.
[Stop sites](#stop-sites-and-the-ordering-that-matters) says why a single `stop`
cannot work.
- **SSH executor** (`SSHExecutor`). Starts a watchdog when it records a session,
stops it on every disposal path, and short-circuits remote status for a session
whose transport is lost.
- **Port forwarder** (`SSHPortForwarder`). Unchanged, but not because its
`Close` is safe to call concurrently. That `Close` is
`select { case <-f.closed: return nil; default: close(f.closed) }` — idempotent
for *sequential* callers, racy for concurrent ones: both can take the `default`
branch and the second panics closing an already-closed channel, which is exactly
teardown-versus-stop (`-002.7`). So **every** close of a session's forwarder,
teardown and all three disposal paths, goes through that session's once-guard,
and it is the guard, not the forwarder, that makes the close exactly once.
## Data and contracts
### Probe mechanism
The probe is an SSH global request named `keepalive@openssh.com`, sent on the
session client with `wantReply` set and no payload — the OpenSSH convention,
chosen over two alternatives:
- **Not a remote command.** Running something like `kill -0` opens a session
channel, and `Client.NewSession` takes no context and blocks until that open is
answered — so on a wedged connection the probe would itself hang forever, the
exact failure this capability detects. The existing remote-controller liveness
probe has this shape, which is why a wedged session can stall the shared
remote-status poll.
- **Any reply proves liveness.** A peer that does not implement the request
answers with a failure rather than ignoring it; bytes crossed both ways, which
is the only thing asked. Hence `-001.2` counts a declined reply as completed,
and the probe works against hosts that do not recognise the request name.
A probe error is never transient: `SendRequest` fails only when the frame cannot
be written or the multiplexer has torn down, and both mean the connection is
finished. Nothing is retryable, which is why `-001.4` tears down on the first
error rather than counting failures.
`Conn.SendRequest` blocks until the reply arrives or the multiplexer tears down,
and takes no context. That single fact shapes the rest of this design: the send
must live on its own goroutine, and the deadline must be enforced by another.
### Tuning values
Package-level variables in the lifecycle package, not constants, so tests can
shorten them:
| Name | Default | Meaning |
| --- | --- | --- |
| `sshKeepaliveInterval` | 15s | Spacing between probes. |
| `sshKeepaliveDeadline` | 45s | Longest gap with no completed reply before the transport is declared lost. |
Neither is read from configuration, an environment variable, or a runtime flag
(`-001.10`). A non-positive value for either, or a deadline not greater than
twice the interval, means "do not start a watchdog", logged once at debug level
(`-002.9`).
`-002.8`'s three remote-cleanup timeouts become package-level variables on the
same terms, so a backstop test need not wait out 20 or 60 seconds of real time.
Both are new; nothing sets them today. They exist so the lifecycle package's tests
can opt out of the watchdog, and that opt-out is load-bearing: fifteen
`CreateInstance` and `ResumeRemoteInstance` calls, in
`executor_ssh_lifecycle_test.go` and `executor_ssh_stop_resume_test.go`, would
each otherwise acquire a real 15-second watchdog under `goleak.VerifyTestMain`, in
tests not about liveness.
Both are therefore zero for this package's tests by default, so no existing test
starts a watchdog and none needs editing; tests that *do* exercise it set
millisecond values, where `-002.9` binds them too — such a test's deadline must
exceed *twice* its interval or no watchdog starts at all. Retrofitting fifteen
unrelated tests with per-test teardown is rejected: it spreads knowledge of this
capability across files with no reason to know it, and a later test would silently
miss the retrofit. Hence `-002.9` makes an unusable pair a first-class "no
watchdog" outcome instead of an error.
## Control flow
### Two goroutines, and why not fewer or more
A watchdog runs exactly two goroutines:
1. **Prober.** Loops: send one probe, publish the outcome, wait one interval.
Publication is a non-blocking send on a capacity-1 channel, so the prober
never blocks on a loop that has already returned. **On a probe error it
publishes the error first and only then returns, permanently** — publishing
before returning is what lets the loop tear down on the error rather than
waiting out the deadline (`-001.4`). It also returns, without starting another
probe, when the stop signal is set, waiting its interval in a `select` on that
signal rather than sleeping, so a signal delivered while idle wakes it at once.
Those are its only two exits, both unconditional.
2. **Watchdog loop.** Selects on the stop signal, the prober's outcomes, and a
deadline timer. A completed reply resets the timer and records the reply time;
a probe error or a deadline expiry triggers teardown. After a teardown the
loop returns; it never performs a second one.
The prober's error exit is what makes an internally-triggered teardown terminate:
on a deadline expiry the loop tears down and returns, and the client close inside
that teardown unblocks the prober's outstanding `SendRequest`. Without it the
prober would take that error, wait its interval, and probe a closed client forever
— the goroutine outliving its connection that removed the earlier plumbing. So
teardown ends probing on its own (`-002.12`).
Probes never overlap (`-001.2`): the prober is sequential, so on a wedged
connection exactly one `SendRequest` is outstanding and the deadline, not a
failure count, ends the session. Spacing is end-to-start, so a slow reply delays
the next probe rather than stacking behind it — which is also what lets the
silence interval stand in for how long a probe has been outstanding in the
disposal reading.
Two precedence rules, because a `select` with several ready cases picks at
random.
**A pending completed reply beats an expired deadline timer.** The loop decides
loss from the *recorded time* of the most recent completed reply, not from which
channel it observed first: a reply that completed inside the deadline can still be
unread when the timer fires, and tearing down there would kill a connection that
had answered. When both are ready the loop accounts for the reply first,
recomputes the silence interval, and declares loss only if that interval is
*strictly greater* than the deadline (`-001.3`) — so the timer firing is never
sufficient alone.
**The stop signal beats both**, so a deliberate stop coinciding with a deadline
expiry cannot log a transport-loss warning (`-002.10`): the loop checks it before
acting on either of the others.
Fewer is not possible: with `SendRequest` uncancellable, one goroutine cannot both
wait for a reply and enforce a deadline. More is not needed: a `Client.Wait`
watcher would report a cleanly-ended connection only marginally sooner than the
prober, whose blocked `SendRequest` returns as soon as the multiplexer tears
down.
### Detection bounds
A connection that ends with an error is detected immediately when a probe is in
flight, and within one probe interval (15 s) when the prober is idle. One that
stops answering without an error is detected at the liveness deadline (45 s),
measured from the last reply.
### Transport teardown
Ordered, and the order is contract (`-001.5`):
1. Under the executor mutex, set the transport-lost marker and read the
forwarder and client handles. Release the mutex before closing anything, so
one dead session cannot stall another session's operations.
2. Close the local port forward. Its listener closes, so a new local connection
is refused rather than accepted
(`-001.6`).
3. Close the session SSH client, tearing down the multiplexer and ending every
in-flight forwarded channel — which is what turns a hung request above into an
ordinary connection error. When the client was reached through a bastion, the
executor's existing bastion-release path fires from the client's own
termination (`-001.8`).
4. Log one warning (`-001.9`).
Close runtime listener after client.
Step 2 runs through the session's forwarder once-guard and step 3 through its
client once-guard — the two separate guards described in
[Components and responsibilities](#components-and-responsibilities), which the
disposal paths share. Each handle is closed exactly once no matter which side
gets there first (`-002.5`,
`-002.7`).
Teardown issues no remote command and touches no persisted metadata (`-001.11`),
so the remote controller keeps running and its recorded pid, port and session
directory stay valid for a later resume.
Setting the marker in step 1, before any handle is closed, removes the window a
concurrent status read would see: a reader holding the mutex across both the
marker read and its decision either sees the marker and reports `disconnected`,
or sees open handles — never a closing client.
That requires a change to how remote status locks, and the change is part of this
capability rather than an assumption about existing code. `GetRemoteStatus` today
takes the executor mutex only long enough to read the session out of the map,
releases it, then reads `state.client` and probes unguarded — so a teardown can
close the client between the read and the probe. `-001.7`'s short-circuit must
read the marker and the client handle in one critical section and decide there,
returning `disconnected` without issuing any command. Only the decision needs the
mutex; the healthy-session probe runs after it is released, since holding it
across the probe would let one wedged session block status for every other.
Watchdogs share nothing: two sessions on one host hold separate clients and
watchdogs, so a loss tears down only the one that stopped answering.
Teardown does not remove the session from the executor's session map (`-002.6`);
removing it would make a later stop find no state and return early, skipping the
bookkeeping the stop still owes.
That has one consequence outside teardown. The instance-creation path reuses an
already-tracked session by reading its recorded local forward port — a value that
survives the listener being closed — so reusing a torn-down session would return a
healthy-looking instance pointing at a port that now refuses connections: the
silent dead end this capability exists to remove. The reuse path consults the
marker and refuses, naming transport loss (`-002.14`); creation already reports
errors when the dial fails, so callers handle that shape.
## Ownership and termination
### Start sites
A watchdog starts at the two places the executor takes ownership of a session SSH
client and records it: instance creation, and resume of a live remote controller.
Both start it after the session is in the map **and before releasing the executor
mutex**, in the one critical section that also writes the entry (`-001.1`).
Ordering alone is not enough: both sites today write the entry, release the mutex
and return, so a stop landing in that gap would stop a watchdog that did not exist
yet, which would then start against a client the stop had already closed and
declare loss for a session nobody owns. Starting under the mutex costs two
goroutines and no I/O, removing the window rather than narrowing it.
Starting under the mutex closes the window between record and start, but on its
own does not deliver "at most one watchdog per session"
(`-002.1`). Both sites check for an existing
session, **release the mutex to connect**, and only then re-take it to record, so
two callers for one executor instance identifier can both pass the check and both
reach the recording step. Today the second overwrites the entry, orphaning the
first caller's client and forwarder. With a watchdog attached that orphan stops
being merely wasteful: every disposal path finds a session by reading the map, so
an overwritten entry's watchdog is unreachable by all three and never exits,
probing a connection nobody owns or declaring loss against a live session — the
outlived-goroutine failure that removed the earlier plumbing.
The record is therefore **insert-if-absent**, not assignment. A caller finding an
entry present leaves it and its watchdog alone, starts none of its own, closes
the port forward and SSH client it had just obtained, and proceeds with the
existing session on the same terms as any other reuse, which for a session
already marked transport-lost means the refusal of `-002.14` rather than an
instance. Otherwise it reports no error: losing this race means another caller
already produced what this one was asked for. Re-checking under the mutex at the
point of insertion, not only before connecting, is what makes check and record
atomic.
Every other dial in the SSH paths is short-lived, already closed by its own
`defer`, and starts no watchdog (`-002.2`): the connection test used when
configuring an executor, remote task-directory reclamation, and the untracked dial
that resets a credential-broker-backed resume.
### Stop sites, and the ordering that matters
There are exactly three paths that dispose of a tracked session (`-002.4`):
instance stop, executor close, and the tracked credential-broker resume reset.
Each runs the same sequence.
That sequence has six steps, not three, for one reason: **two of the three paths
issue remote commands over the session SSH client between closing the forward and
closing the client, and those commands cannot be moved.** The stop runs the
cleanup script and the remote controller stop there; the reset the controller
stop. The reset carries one further remote call, a credential-broker reachability
preflight, which runs ahead of the removal and is gated by the same reading.
1. **Under the executor mutex, remove the session from the tracked sessions and
take the disposal reading.** The reading covers the transport-lost marker and
the silence interval together and classifies the transport as *lost*,
*unresponsive*, or *answering* (`-002.8`), and that one reading governs every
remote command on the path. A session with no running watchdog has no silence
interval and classifies as *answering*, leaving every existing stop unchanged
when the tuning values are non-positive.
**The reset takes the reading before its broker-reachability preflight**, which
runs ahead of the removal and stays there. A reading taken behind that
preflight is worthless: on a wedged transport it blocks in
`Client.NewSession`, so the disposal never reads at all and never skips
anything. Only the reading moves — the preflight keeps its position, so its
own failure path still returns before the session is untracked. `-002.8`
therefore anchors the reading no later than the removal rather than at it,
which is what makes `-002.15` reachable in the one condition it names. For the
reset the reading is therefore its own, earlier critical section: the mutex is
taken to read, released for the preflight, re-taken for the removal. The stop
and the executor close take one section, reading and removing together.
**Then release the executor mutex and do not re-take it across steps 2 to 6.**
An invariant, not a preference: teardown takes the mutex as its own first act,
and step 2 waits for a loop that may be in the middle of one, so a disposal
holding it would deadlock against the teardown it waits for. The requirement to
*start* a watchdog under the mutex (`-001.1`) invites the symmetric mistake.
2. **Signal the watchdog loop and wait for the loop — only the loop — to exit.**
Bounded: the loop is parked in its `select`, already returned, or performing a
teardown, and a teardown is two local handle closes and one log line, so the
wait ends without touching the network. It is not bounded by the prober, which
is the point of splitting the steps. Once this returns, no teardown for this
session is running and none can start.
3. **Close the local port forward**, through the forwarder once-guard.
4. **Run this path's own remote commands — unless step 1's reading said *lost*
or *unresponsive*, in which case skip them entirely.** For a stop these are
the cleanup script and then the remote controller stop, in that order; for the
reset, the remote controller stop; the executor close has none, so this step is
empty for it. The reset's preflight runs ahead of step 1, gated by the same
reading.
Step 1's reading is a snapshot, and a transport can wedge *after* it, so each
command runs under its own timeout and one still outstanding when that expires
is abandoned (`-002.8`). Without it a wedge landing between the reading and the
command hangs the disposal for ever: step 2 has already retired the only
component that would have closed the client. The three timeouts are
`sshCleanupTimeout` (60 s) for the cleanup script, `sshAgentctlCleanupTimeout`
(20 s) for the controller stop, and a new one for the reset's preflight, which
runs on the raw caller context today and is therefore unbounded.
Abandoning needs a second goroutine, because the disposal's own is inside
`Client.NewSession`. The disposal runs the command on its own goroutine and
waits on that goroutine and a timer. On the timer it closes the client
**through the client once-guard**, so step 5 finds the guard spent rather than
double-closing, marks the client transport-lost for `-002.15`, and only then
joins the command goroutine. That join is bounded by the close, not by the
command, and it keeps the abandoned command inside `-002.4`'s no-leak guarantee
rather than beside it.
Skipping is what makes `-002.11` achievable; the next section gives the two
conditions and why each is needed.
5. **Close the session SSH client**, through the client once-guard.
6. **Wait for the prober to exit.** If it was mid-probe, step 5's close unblocks
its `SendRequest` and it returns through its error exit; if it was idle, step
2's signal already woke it out of its interval wait. Both are prompt, so this
never costs a probe interval, keeping a deliberate stop fast rather than merely
finite. `-002.13` catches an implementation that waits the interval out;
`-002.11` one that never returns.
Only after step 6 does the disposal call return, which is what `-002.4` requires:
"the call" is the stop, close, or reset call, not an internal helper.
Four collapses of this sequence look simpler and each breaks a named criterion:
- **Stop before closing** (one `stop` that signals and joins both goroutines,
called before the client is closed) deadlocks on exactly the connection this
capability exists for: the prober is blocked in an uncancellable `SendRequest`,
the loop returns on the signal and never tears down, and nothing else closes the
client because that was scheduled for after `stop` returns. Returning early from
`stop` leaves the prober running past the call — the leak `-002.4` forbids.
- **Close before stopping.** The prober's in-flight `SendRequest` errors while the
loop is still selecting, and a probe error declares the transport lost
(`-001.4`), so a deliberate stop would log a transport-loss warning, which
`-002.10` forbids.
- **Close the client early, before the remote commands.** It bounds step 4 by
making every command fail fast on *every* disposal, including the vast majority
where the transport is healthy. Those commands remove the task directory and the
remote controller process, so running them over a just-closed client abandons
that process on the host every time a session stops. It also inverts an ordering
the stop path guarantees and `executor_ssh_scripts_test.go` asserts directly:
cleanup before the remote stop and before the client close.
- **Bound the remote commands with a context timeout instead of skipping.** What
hangs is not bounded by their context: each already runs under a cleanup
timeout, and it governs execution, not the channel-open preceding it. Step 4's
backstop is a different mechanism — closing the client, the only thing that
unblocks a `Client.NewSession` — and it complements the skip rather than
replacing it: the skip keeps an already-unresponsive disposal from paying that
timeout at all (`-002.11`).
The sequence needs no "was I asked to stop?" flag inside teardown. Step 2
removes the only component that can declare loss or log, so by
step 5 nobody is left to misread the resulting error — which is also why `-001.4`
is scoped to a *running* watchdog.
Every step is idempotent. A disposal of a session that never started a watchdog,
or whose watchdog already stopped, does nothing in steps 2 and 6 and succeeds. One
whose watchdog already tore the transport down needs no close of its own (the
once-guards make steps 3 and 5 no-ops) and its prober has exited, so step 6
returns at once.
**Two disposals racing each other.** All three paths remove the session in step 1,
so of two concurrent disposals exactly one gets the record. The other finds
nothing and returns success immediately, closing no handle and waiting for nothing
(`-002.4`). The no-remaining-activity guarantee therefore binds the caller that
removed the session, not both: a shared completion handle would let a backend
shutdown block on an unrelated stop's work.
Termination is provable: the loop exits on its signal, and the prober on the error
produced by a close that step 5 guarantees. The package's `goleak` check over
`TestMain` is the standing regression guard.
### Disposing of a session whose transport is lost or unresponsive
Step 1's reading classifies the transport once, and every remote command on that
disposal path obeys it (`-002.8`). Re-reading per decision would let a teardown
land between two of them and produce a stop that ran the cleanup script but
skipped the controller stop.
*Lost* and *unresponsive* reach the same decision for different reasons, and both
are needed. **Lost** means teardown has run and the client is closed, so a remote
command fails immediately and skipping saves only a pointless error.
**Unresponsive** means the client is still open and has stopped answering, so a
remote command does *not* fail — it blocks in `Client.NewSession`, which takes no
context, and never returns. That makes the skip load-bearing, and it is invisible
to the marker alone: a transport is unresponsive for the whole span between twice
the probe interval and the liveness deadline, while the watchdog has correctly not
yet declared it lost. Consulting only the marker would satisfy `-002.8` and still
hang on `-002.11`.
Deriving *unresponsive* from the silence interval costs no new signalling between
the goroutines. Probes are sent one at a time spaced by the interval, so silence
past one interval means a probe is outstanding — but on a healthy link that is
true from every interval boundary until the reply lands, so a one-interval
threshold would call each healthy session unresponsive for one round trip per
cycle and skip the remote commands of a stop with no reason to skip them. The
threshold is therefore *twice* the interval, which `-002.9` keeps inside the
deadline: a probe still unanswered a whole further interval later is not a slow
reply. The timestamp it is computed from is already on the session state for the
teardown log (`-001.9`) and is read under the same mutex, in the same critical
section, as the marker.
The reading is not a guarantee that the transport survives the disposal, and need
not be: a teardown firing just after it closes the client underneath a command
still in flight from before step 2, which then fails promptly and is logged like
any other. A command on a *closed* client fails immediately; only the
*wedged-but-open* client hangs. The single reading buys a coherent, bounded
decision, not an uninterruptible one.
**The stop and the reset differ in what they report.** A stop treats a skipped or
failed cleanup as non-fatal and completes without error (`-002.8`); its job is to
release local resources and it has done so. The reset cannot: its remote
controller stop is the *point* of the call, killing a stale broker-backed
controller before the resume metadata is cleared so a fresh create can run. If
that stop was skipped, or failed because the client was closed under it — by a
teardown, or by step 4's own backstop — the controller is still running on the
host. The same holds for the preflight, whose helper otherwise reports every
failure as broker-unreachable. The reset therefore leaves the resume metadata
unchanged and reports an error identifying transport loss (`-002.15`), and tells
that from a host that refused the call by the transport-lost mark both closers
set.
## Failure and recovery
A session whose transport is torn down does not recover in place. In-flight
requests fail with connection errors, the agent's event stream disconnects through
the path it already uses for a dropped stream, and the session's work ends with an
error instead of hanging. Recovery is a resume: re-dial, re-verify the fingerprint,
open a fresh forward, re-attach to the still-running controller.
## Persistence
None. The watchdog holds no durable state. The transport-lost marker lives on
in-memory session state for the life of the executor's session entry; persisted
resume metadata is untouched by teardown.
## Security
Probes carry no payload and no credential material, and are sent only on
connections whose host key already matched the pinned fingerprint. Teardown
neither re-pins nor clears trust; the next dial runs the existing check
unchanged.
## Observability
- One warning log at teardown naming SSH session transport loss, the executor
instance identifier, the remote host, and the silence interval
(`-001.9`). A forwarder or client close
that returned an error is named in the same line (`-001.5`).
- One debug log with the executor instance identifier when a watchdog is not
started, because a tuning value is not positive or the deadline is not greater
than twice the interval.
- Remote status reports `disconnected` with an error naming transport loss, which
the executor status badge renders (`-001.7`). It short-circuits on the marker
and issues no command, so an already-dead session cannot delay the shared
remote-status poll.
- No new `expvar` counter; the structured logs carry the instance identifier.
## Testing
The fake SSH server answers global requests through `ssh.DiscardRequests`, which
replies. This capability needs the opposite: a server that accepts a global
request, and a channel open, and answers neither — so `SendRequest` and
`Client.NewSession` both block as on a wedged link. The fake server gains a switchable mode for that, so a test can run a session healthy,
flip the server silent, and assert teardown within a shortened deadline — bounded
in milliseconds once the tuning values are lowered, with `goleak` over `TestMain`
covering termination. That silent mode makes the hardest criteria testable:
- **A stop that races a silent transport must return** — flip the server silent,
stop the session well before the deadline, assert the stop completes
(`-002.11`). A stop-then-close implementation hangs here rather than failing an
assertion, so this test needs its own bound. It also proves the step 4 skip: an
implementation issuing either remote command over the still-open wedged client
blocks in `Client.NewSession` and never reaches the assertion. The reset needs
the same shape, its preflight being the earlier of its two remote calls.
- **A command that wedges after the reading must not hang the disposal** — read
*answering*, then flip the server silent: the stop must still return and report
the command failed (`-002.8`). Millisecond-bounded once the cleanup timeouts are
lowered; without the backstop it never returns, so it needs its own bound.
- **A deliberate stop must not report transport loss** — the same setup on a
*healthy* server, asserting no transport-loss warning is recorded (`-002.10`),
which is what fails if the steps are reordered to close before signalling.
- **An ordinary disposal between two probes must not wait out the interval** — a
healthy session with a long probe interval and a longer deadline; let one probe
complete, then dispose of it and assert the call returns in well under one
interval (`-002.13`). Use the executor-close path, whose lack of remote commands
makes the whole call time the watchdog wait the criterion bounds. Without it, a
disposal that waits for the prober's interval timer passes every other criterion
while making every ordinary stop take up to fifteen seconds.
`goleak` covers the prober's error exit (`-002.12`) only if some test reaches an
internally-triggered teardown and then ends without stopping the session, so the
deadline-expiry test should do exactly that.
Because both tuning values default to zero for this package's tests (see
[Tuning values](#tuning-values)), every test above sets them explicitly and no
other test in the package starts a watchdog. Those variables are package-level and
mutable, which carries one constraint invisible in the code: **a test that sets
any of them must not call `t.Parallel()`**, and neither may any test that creates
or resumes an SSH session while such a test could run. No test in the package uses
`t.Parallel()` today; it is written down because the instinct on a wait-based
liveness test is to parallelise it, and the result is a data race surfacing under
`-race` as a flake in an unrelated change.
## Related decisions
No architecture decision record is required: this capability restores a transport
property inside an existing owned boundary and adds no public contract,
configuration key, or dependency.