From aebb01e714ca9af54150f5ea56890c2d88ecd9c8 Mon Sep 17 00:00:00 2001 From: Henry Date: Sun, 13 Sep 2026 06:00:53 +0800 Subject: [PATCH] feat(executors): detect and tear down dead SSH session transports (#3581) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- apps/backend/go.mod | 12 +- apps/backend/go.sum | 28 +- .../runtime/lifecycle/executor_backend.go | 1 + .../runtime/lifecycle/executor_docker.go | 6 +- .../runtime/lifecycle/executor_sprites.go | 12 +- .../agent/runtime/lifecycle/executor_ssh.go | 312 ++++- .../lifecycle/executor_ssh_connection.go | 49 +- .../lifecycle/executor_ssh_keepalive.go | 456 +++++++ .../lifecycle/executor_ssh_keepalive_test.go | 1069 +++++++++++++++++ .../lifecycle/executor_ssh_lifecycle_test.go | 5 + .../runtime/lifecycle/executor_ssh_scripts.go | 10 +- .../executor_ssh_stop_resume_test.go | 43 + .../executor_ssh_transport_teardown_test.go | 169 +++ .../agent/runtime/lifecycle/goleak_test.go | 9 + .../runtime/lifecycle/manager_startup.go | 2 +- .../runtime/lifecycle/ssh_fake_server_test.go | 29 + docs/plans/ssh-transport-liveness/plan.md | 112 ++ .../task-01-ssh-transport-watchdog.md | 152 +++ docs/specs/executors/README.md | 31 + .../requirements/ssh-transport-liveness.md | 328 +++++ .../system-design/ssh-transport-liveness.md | 567 +++++++++ 21 files changed, 3312 insertions(+), 90 deletions(-) create mode 100644 apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive.go create mode 100644 apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive_test.go create mode 100644 apps/backend/internal/agent/runtime/lifecycle/executor_ssh_transport_teardown_test.go create mode 100644 docs/plans/ssh-transport-liveness/plan.md create mode 100644 docs/plans/ssh-transport-liveness/task-01-ssh-transport-watchdog.md create mode 100644 docs/specs/executors/requirements/ssh-transport-liveness.md create mode 100644 docs/specs/executors/system-design/ssh-transport-liveness.md diff --git a/apps/backend/go.mod b/apps/backend/go.mod index fab57c6faf..05a617b018 100644 --- a/apps/backend/go.mod +++ b/apps/backend/go.mod @@ -39,11 +39,11 @@ require ( go.opentelemetry.io/otel/trace v1.44.0 go.uber.org/goleak v1.3.0 go.uber.org/zap v1.26.0 - golang.org/x/crypto v0.52.0 - golang.org/x/net v0.55.0 - golang.org/x/sync v0.20.0 - golang.org/x/sys v0.45.0 - golang.org/x/text v0.37.0 + golang.org/x/crypto v0.57.0 + golang.org/x/net v0.58.0 + golang.org/x/sync v0.23.0 + golang.org/x/sys v0.48.0 + golang.org/x/text v0.42.0 google.golang.org/grpc v1.83.1 google.golang.org/protobuf v1.36.11 gopkg.in/natefinch/lumberjack.v2 v2.2.1 @@ -133,7 +133,7 @@ require ( go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/arch v0.3.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect - golang.org/x/term v0.43.0 // indirect + golang.org/x/term v0.46.0 // indirect golang.org/x/time v0.11.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect diff --git a/apps/backend/go.sum b/apps/backend/go.sum index 754b04da00..77f30c35c2 100644 --- a/apps/backend/go.sum +++ b/apps/backend/go.sum @@ -427,8 +427,8 @@ golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8U golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20210421170649-83a5a9bb288b/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4= golang.org/x/crypto v0.0.0-20220722155217-630584e8d5aa/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= -golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= -golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= +golang.org/x/crypto v0.57.0 h1:3ZVCjf8Ggz7zneR/EHRVx68Ctf+2pmIMP2UFhh9cC6M= +golang.org/x/crypto v0.57.0/go.mod h1:Fdz0i5U6CoizGwLda9DttjSk6qlZo25zYNtR+ycvuZA= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= @@ -494,8 +494,8 @@ golang.org/x/net v0.0.0-20201209123823-ac852fbbde11/go.mod h1:m0MpNAwzfU5UDzcl9v golang.org/x/net v0.0.0-20201224014010-6772e930b67b/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -517,8 +517,8 @@ golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= -golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= +golang.org/x/sync v0.23.0/go.mod h1:sUUOizhqBxiL6pEWpqNLUiaJn1ShEbZ6BBqskPbjZm0= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190312061237-fead79001313/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= @@ -562,11 +562,11 @@ golang.org/x/sys v0.0.0-20220704084225-05e143d24a9e/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= -golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= +golang.org/x/sys v0.48.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= -golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= -golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= +golang.org/x/term v0.46.0 h1:3+OXuTbaKDgwk8jTi3aSLHRlmWqHEUDUtxnbFigO4YE= +golang.org/x/term v0.46.0/go.mod h1:+K02xbkittuwc0Am4abfA3Fc+XRGXkvBXNO88NCXPoc= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -575,8 +575,8 @@ golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.4/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= -golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= -golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= +golang.org/x/text v0.42.0 h1:JbOZXgfeCPU9gacVtYliJqOhD+zhrEqK4LfdpmlUZqI= +golang.org/x/text v0.42.0/go.mod h1:ojzP1Z+2QtioaF8DTtO8K5q7JWVVYwZKenzujK0Zd0E= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= @@ -629,8 +629,8 @@ golang.org/x/tools v0.0.0-20201208233053-a543418bbed2/go.mod h1:emZCQorbCU4vsT4f golang.org/x/tools v0.0.0-20210105154028-b0ab187a4818/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.0.0-20210108195828-e2f9c7f1fc8e/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0= -golang.org/x/tools v0.44.0 h1:UP4ajHPIcuMjT1GqzDWRlalUEoY+uzoZKnhOjbIPD2c= -golang.org/x/tools v0.44.0/go.mod h1:KA0AfVErSdxRZIsOVipbv3rQhVXTnlU6UhKxHd1seDI= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go index de37573009..0345ced837 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go @@ -203,6 +203,7 @@ const ( MetadataKeySpriteState = "sprite_state" MetadataKeySpriteCreatedAt = "sprite_created_at" MetadataKeyLocalPort = "local_port" + MetadataKeyReuseExistingProcess = "reuse_existing_process" // Kubernetes executor connection metadata. These names mirror the persisted // executor config parsed by internal/agent/kubernetes. diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go b/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go index 3d706cbdc3..5a2cf44b4c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_docker.go @@ -373,9 +373,9 @@ func (r *DockerExecutor) reconnectToContainer(ctx context.Context, dockerClient ContainerIP: containerIP, WorkspacePath: dockerWorkspacePath, Metadata: map[string]interface{}{ - MetadataKeyIsRemote: true, - MetadataKeyContainerID: info.ID, - "reuse_existing_process": conn.reusingProcess, + MetadataKeyIsRemote: true, + MetadataKeyContainerID: info.ID, + MetadataKeyReuseExistingProcess: conn.reusingProcess, }, AuthToken: refreshedAuthToken, }, nil diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_sprites.go b/apps/backend/internal/agent/runtime/lifecycle/executor_sprites.go index 7d9468b6be..32a706700d 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_sprites.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_sprites.go @@ -577,12 +577,12 @@ func (r *SpritesExecutor) buildInstanceResult( agentctl.WithSessionID(req.SessionID)), WorkspacePath: spritesWorkspacePath, Metadata: map[string]interface{}{ - MetadataKeySpriteName: spriteName, - MetadataKeySpriteState: strings.TrimSpace(sprite.Status), - MetadataKeySpriteCreatedAt: sprite.CreatedAt, - MetadataKeyLocalPort: localPort, - "reuse_existing_process": reusingExisting, - MetadataKeyIsRemote: true, + MetadataKeySpriteName: spriteName, + MetadataKeySpriteState: strings.TrimSpace(sprite.Status), + MetadataKeySpriteCreatedAt: sprite.CreatedAt, + MetadataKeyLocalPort: localPort, + MetadataKeyReuseExistingProcess: reusingExisting, + MetadataKeyIsRemote: true, }, } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go index 0e05b76aae..7114b0afe2 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go @@ -24,16 +24,22 @@ import ( ) const ( - sshAgentctlHealthTimeout = 20 * time.Second - sshAgentctlCleanupTimeout = 20 * time.Second - sshAgentctlLoopbackHost = "127.0.0.1" + sshAgentctlHealthTimeout = 20 * time.Second + sshAgentctlLoopbackHost = "127.0.0.1" sshStatusRunning = "running" sshStatusUnknown = "unknown" sshStatusDisconnected = "disconnected" sshStatusAgentctlDown = "agentctl-down" + + sshTransportLostMessage = "ssh session transport lost" ) +// sshAgentctlCleanupTimeout bounds the remote controller stop a disposal +// path issues. A package-level variable, not a constant, so a backstop test +// need not wait out real seconds (see executor_ssh_keepalive.go). +var sshAgentctlCleanupTimeout = 20 * time.Second + // sshSessionState tracks per-session resources we need to clean up later: // the SSH target (so we can dial again on Stop), the per-session SSH client // (no shared pool — the Go SSH client's direct-tcpip mux gets unreliable @@ -53,6 +59,34 @@ type sshSessionState struct { prepareEnv map[string]string platform SSHRemotePlatform runtimeAPITunnel *sshRuntimeAPITunnel + + // port is the remote agentctl instance's port (the one the local forward + // targets) and workdirRoot is the resolved workdir root used to launch + // it. Both are set once at session creation and read back by a caller + // that loses the create/resume race (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1): + // its own request never produced this session, so its instance metadata + // must come from here, not from re-deriving values the surviving session + // may not actually have. + port int + workdirRoot string + + // Transport liveness (docs/specs/executors/requirements/ssh-transport-liveness.md). + // watchdog, transportLost and lastProbeReply are guarded by the owning + // SSHExecutor's mutex, the same lock that guards the session map. + watchdog *sshKeepaliveWatchdog + transportLost bool + lastProbeReply time.Time + + // Independent once-guards for the two handles teardown and every + // disposal path share: whichever reaches a handle first closes it + // exactly once, and the other observes the same result. + forwarderCloseOnce sync.Once + forwarderCloseErr error + clientCloseOnce sync.Once + clientCloseErr error + // teardownOnce guards transport teardown as a whole, so a second + // declaration logs no further warning. + teardownOnce sync.Once } // SSHExecutor implements ExecutorBackend for SSH-reachable Linux and macOS hosts. @@ -120,13 +154,11 @@ func (r *SSHExecutor) Close() error { } r.mu.Unlock() for _, s := range states { + r.stopWatchdogAndWait(s) + _ = r.closeForwarderOnce(s) + _ = r.closeClientOnce(s) _ = s.runtimeAPITunnel.Close() - if s.forwarder != nil { - _ = s.forwarder.Close() - } - if s.client != nil { - _ = r.closeSSHClient(s.client) - } + r.awaitProbeExit(s) } return nil } @@ -203,6 +235,12 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq } resumed, ok := r.resumedStateForCreate(req) if ok { + r.mu.Lock() + lost := resumed.transportLost + r.mu.Unlock() + if lost { + return nil, fmt.Errorf("ssh: reused session unavailable: %w", ErrSSHTransportLost) + } return r.buildResumedInstance(req, resumed), nil } if _, err := validateRemoteContributions(req.RemoteContributions); err != nil { @@ -224,8 +262,8 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq released := false defer func() { if !released { - _ = runtimeAPITunnel.Close() _ = client.Close() + _ = runtimeAPITunnel.Close() } }() r.report(req.OnProgress, "Connecting to SSH host", PrepareStepCompleted, "") @@ -286,7 +324,24 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq } r.mu.Lock() - r.sessions[req.InstanceID] = &sshSessionState{ + if existing, ok := r.sessions[req.InstanceID]; ok { + // AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1: another caller already + // recorded a session for this instance while we were dialing and + // preparing. Leave it and its watchdog alone, close what we just + // built instead of orphaning it, and proceed with the existing + // session on the same terms as any other reuse. + lost := existing.transportLost + r.mu.Unlock() + _ = fwd.Close() + _ = client.Close() + _ = runtimeAPITunnel.Close() + released = true + if lost { + return nil, fmt.Errorf("ssh: reused session unavailable: %w", ErrSSHTransportLost) + } + return r.buildInstanceForLostRace(req, existing), nil + } + state := &sshSessionState{ target: target, client: client, forwarder: fwd, @@ -298,13 +353,61 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq prepareEnv: sshRemoteContributionEnv(req, agentctlBin), platform: platform, runtimeAPITunnel: runtimeAPITunnel, + port: port, + workdirRoot: workdir, } + r.sessions[req.InstanceID] = state + r.startWatchdogLocked(req.InstanceID, state) r.mu.Unlock() released = true // ownership transferred to session state; released on StopInstance return r.buildInstance(req, target, fwd, taskDir, sessionDir, port, pid, workdir, authToken), nil } +// buildInstanceForLostRace builds an ExecutorInstance for a caller that lost +// the create/resume race in the executor's tracked-sessions map: another +// concurrent caller already recorded (and started a watchdog for) the +// session this caller was about to create. It reads only from the existing +// session state, never from this caller's request, since the request's +// remote-agentctl metadata describes a session this caller never created. +func (r *SSHExecutor) buildInstanceForLostRace(req *ExecutorCreateRequest, existing *sshSessionState) *ExecutorInstance { + client := existing.agentctlClient + if client == nil { + client = agentctl.NewClient(sshAgentctlLoopbackHost, existing.forwarder.LocalPort(), r.logger, + agentctl.WithExecutionID(req.InstanceID), + agentctl.WithSessionID(req.SessionID), agentctl.WithAuthToken(existing.authToken)) + } + metadata := map[string]interface{}{ + MetadataKeySSHHost: existing.target.Host, + MetadataKeySSHPort: strconv.Itoa(existing.target.Port), + MetadataKeySSHUser: existing.target.User, + MetadataKeySSHHostFingerprint: existing.target.PinnedFingerprint, + MetadataKeySSHRemoteTaskDir: existing.remoteTaskDir, + MetadataKeySSHRemoteSessionDir: existing.remoteDir, + MetadataKeySSHRemoteAgentctlPort: strconv.Itoa(existing.port), + MetadataKeySSHRemoteAgentctlPID: strconv.Itoa(existing.pid), + MetadataKeySSHLocalForwardPort: strconv.Itoa(existing.forwarder.LocalPort()), + MetadataKeySSHWorkdirRoot: existing.workdirRoot, + MetadataKeyIsRemote: true, + MetadataKeyReuseExistingProcess: existing.reusingProcess, + } + // existing.metadata is the winning session's own cloned request metadata + // (cloneSSHMetadata(req.Metadata), captured after openSSHRuntimeAPITunnelForRequest + // ran for the winner), so it carries the winner's still-open tunnel's keys — + // never this caller's own, since this caller's tunnel was just closed above. + copySSHRuntimeAPIMetadata(metadata, existing.metadata) + return &ExecutorInstance{ + InstanceID: req.InstanceID, + TaskID: req.TaskID, + SessionID: req.SessionID, + RuntimeName: r.Name(), + Client: client, + WorkspacePath: existing.remoteTaskDir, + AuthToken: existing.authToken, + Metadata: metadata, + } +} + func reuseRequiredRemoteTaskDir(req *ExecutorCreateRequest) (string, error) { if req == nil { return "", fmt.Errorf("%w: missing remote task directory", models.ErrWorkspaceReuseUnsafe) @@ -602,7 +705,7 @@ func (r *SSHExecutor) buildResumedInstance(req *ExecutorCreateRequest, state *ss MetadataKeySSHLocalForwardPort: strconv.Itoa(state.forwarder.LocalPort()), MetadataKeySSHWorkdirRoot: workdir, MetadataKeyIsRemote: true, - "reuse_existing_process": state.reusingProcess, + MetadataKeyReuseExistingProcess: state.reusingProcess, } copySSHRuntimeAPIMetadata(metadata, req.Metadata) return &ExecutorInstance{ @@ -641,18 +744,31 @@ func (r *SSHExecutor) StopInstance(ctx context.Context, instance *ExecutorInstan } r.mu.Lock() state := r.sessions[instance.InstanceID] - delete(r.sessions, instance.InstanceID) + var classification sshTransportClassification + if state != nil { + // AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8: the single disposal + // reading, taken in the same critical section as the removal, before + // any remote command on this path. + classification = classifySSHTransportLocked(state) + delete(r.sessions, instance.InstanceID) + } r.mu.Unlock() if state == nil { return r.stopPersistedRemoteAgentctl(ctx, instance, force) } - _ = state.runtimeAPITunnel.Close() - if state.forwarder != nil { - _ = state.forwarder.Close() - } - if shouldRunExecutorCleanup(instance.StopReason) && state.client != nil && r.cleanupScript != nil { + + // Step 2: signal the watchdog loop and wait for it — only it — to exit. + r.stopWatchdogAndWait(state) + // Step 3: close the local port forward through its once-guard. + _ = r.closeForwarderOnce(state) + + // Step 4: this path's own remote commands, skipped entirely when the + // reading said the transport is lost or unresponsive. + if classification == sshTransportAnswering && shouldRunExecutorCleanup(instance.StopReason) && state.client != nil && r.cleanupScript != nil { metadata := mergeSSHMetadata(state.metadata, instance.Metadata) - if err := r.cleanupScript(ctx, state.client, state.remoteTaskDir, metadata, state.prepareEnv, state.platform, instance.InstanceID, instance.StopReason); err != nil { + if err := r.runWithTransportBackstop(state, sshCleanupTimeout, func() error { + return r.cleanupScript(ctx, state.client, state.remoteTaskDir, metadata, state.prepareEnv, state.platform, instance.InstanceID, instance.StopReason) + }); err != nil { r.logger.Warn("failed to run SSH cleanup script", zap.String("instance_id", instance.InstanceID), zap.Error(err)) } } @@ -660,25 +776,34 @@ func (r *SSHExecutor) StopInstance(ctx context.Context, instance *ExecutorInstan r.logger.Info("preserving SSH agentctl after agent stop", zap.String("instance_id", instance.InstanceID), zap.String("stop_reason", instance.StopReason)) - if state.client != nil { - _ = r.closeSSHClient(state.client) - } + // Step 5 and step 6: close the client and wait for the prober exit + // even on this early-return path, so no watchdog activity survives + // the call. + _ = r.closeClientOnce(state) + _ = state.runtimeAPITunnel.Close() + r.awaitProbeExit(state) return nil } // Use the same SSH client we used for CreateInstance — if it's still // alive we can kill the remote agentctl gracefully; otherwise just drop // the connection on the floor. - if state.client != nil { - cleanupCtx, cancel := sshRemoteCleanupContext(ctx) + if classification == sshTransportAnswering && state.client != nil { stopRemote := r.stopRemote if stopRemote == nil { stopRemote = stopRemoteAgentctl } - _ = stopRemote(cleanupCtx, state.client, state.remoteDir, state.pid) - cancel() - _ = r.closeSSHClient(state.client) + _ = r.runWithTransportBackstop(state, sshAgentctlCleanupTimeout, func() error { + cleanupCtx, cancel := sshRemoteCleanupContext(ctx) + defer cancel() + return stopRemote(cleanupCtx, state.client, state.remoteDir, state.pid) + }) } + // Step 5: close the session SSH client, through its once-guard. + _ = r.closeClientOnce(state) + _ = state.runtimeAPITunnel.Close() + // Step 6: wait for the prober to exit. + r.awaitProbeExit(state) return nil } @@ -871,14 +996,14 @@ func (r *SSHExecutor) ResumeRemoteInstance(ctx context.Context, req *ExecutorCre remotePort, _ := strconv.Atoi(portStr) fwd, err := StartPortForward(client, remotePort, r.logger) if err != nil { - _ = runtimeAPITunnel.Close() _ = client.Close() + _ = runtimeAPITunnel.Close() return fmt.Errorf("ssh resume: port forward: %w", err) } if err := waitAgentctlHealthy(ctx, fwd.LocalPort(), sshAgentctlHealthTimeout); err != nil { - _ = runtimeAPITunnel.Close() _ = fwd.Close() _ = client.Close() + _ = runtimeAPITunnel.Close() return fmt.Errorf("ssh resume: agentctl health: %w", err) } instanceClient, reusingProcess := r.newResumedAgentctlClient( @@ -886,7 +1011,18 @@ func (r *SSHExecutor) ResumeRemoteInstance(ctx context.Context, req *ExecutorCre ) r.mu.Lock() - r.sessions[req.InstanceID] = &sshSessionState{ + if _, ok := r.sessions[req.InstanceID]; ok { + // AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1: a concurrent resume or + // create already recorded a session for this instance while we were + // dialing. Leave it and its watchdog alone and close what we just + // built instead of orphaning it. + r.mu.Unlock() + _ = fwd.Close() + _ = client.Close() + _ = runtimeAPITunnel.Close() + return nil + } + state := &sshSessionState{ target: target, client: client, forwarder: fwd, @@ -899,7 +1035,11 @@ func (r *SSHExecutor) ResumeRemoteInstance(ctx context.Context, req *ExecutorCre metadata: cloneSSHMetadata(req.Metadata), prepareEnv: sshRemoteContributionEnv(req, agentctlBin), runtimeAPITunnel: runtimeAPITunnel, + port: remotePort, + workdirRoot: r.workdirRoot(req.Metadata), } + r.sessions[req.InstanceID] = state + r.startWatchdogLocked(req.InstanceID, state) r.mu.Unlock() // Refresh transient metadata so downstream consumers see the new local @@ -988,6 +1128,13 @@ func (r *SSHExecutor) resetManagedBrokerResume( return nil } +// resetTrackedManagedBrokerResume disposes of a tracked session as part of +// resetting a credential-broker-backed resume (the third of the three +// disposal paths, AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4). Unlike the stop +// and close paths, its own reading is taken in its own, earlier critical +// section: the mutex is taken to read (and to re-check the session is still +// tracked, closing the race F36 identifies before the preflight's remote +// round trip), released for the preflight, then re-taken for the removal. func (r *SSHExecutor) resetTrackedManagedBrokerResume( ctx context.Context, req *ExecutorCreateRequest, @@ -995,25 +1142,71 @@ func (r *SSHExecutor) resetTrackedManagedBrokerResume( brokerPreflight func(context.Context, *ssh.Client, *ExecutorCreateRequest, SSHRemotePlatform) error, stopRemote func(context.Context, *ssh.Client, string, int) error, ) error { - if err := brokerPreflight(ctx, state.client, req, SSHRemotePlatform{}); err != nil { - return err - } r.mu.Lock() - if r.sessions[req.InstanceID] == state { + tracked := r.sessions[req.InstanceID] == state + classification := classifySSHTransportLocked(state) + r.mu.Unlock() + if !tracked { + // A concurrent disposal already removed this session: return success + // immediately, closing no handle and not waiting for the other + // call's watchdog (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4). + return nil + } + + if classification == sshTransportAnswering { + err := r.runWithTransportBackstop(state, sshBrokerPreflightTimeout, func() error { + preflightCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), sshBrokerPreflightTimeout) + defer cancel() + return brokerPreflight(preflightCtx, state.client, req, SSHRemotePlatform{}) + }) + if err != nil { + if r.isTransportLost(state) { + return ErrSSHTransportLost + } + return err + } + } + + r.mu.Lock() + tracked = r.sessions[req.InstanceID] == state + if tracked { delete(r.sessions, req.InstanceID) } r.mu.Unlock() - if state.forwarder != nil { - _ = state.forwarder.Close() + if !tracked { + return nil } - if err := stopRemote(ctx, state.client, state.remoteDir, state.pid); err != nil { - if state.client != nil { - _ = r.closeSSHClient(state.client) + + // Step 2: signal the watchdog loop and wait for it — only it — to exit. + r.stopWatchdogAndWait(state) + // Step 3: close the local port forward through its once-guard. + _ = r.closeForwarderOnce(state) + + if classification != sshTransportAnswering { + // AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.15: skip the remote + // controller stop, leave resume metadata unchanged, and report + // transport loss. + _ = r.closeClientOnce(state) + _ = state.runtimeAPITunnel.Close() + r.awaitProbeExit(state) + return ErrSSHTransportLost + } + + stopErr := r.runWithTransportBackstop(state, sshAgentctlCleanupTimeout, func() error { + cleanupCtx, cancel := sshRemoteCleanupContext(ctx) + defer cancel() + return stopRemote(cleanupCtx, state.client, state.remoteDir, state.pid) + }) + // Step 5: close the session SSH client, through its once-guard. + _ = r.closeClientOnce(state) + _ = state.runtimeAPITunnel.Close() + // Step 6: wait for the prober to exit. + r.awaitProbeExit(state) + if stopErr != nil { + if r.isTransportLost(state) { + return ErrSSHTransportLost } - return fmt.Errorf("ssh resume: stop stale broker-backed agentctl: %w", err) - } - if state.client != nil { - _ = r.closeSSHClient(state.client) + return fmt.Errorf("ssh resume: stop stale broker-backed agentctl: %w", stopErr) } clearSSHResumeRuntimeMetadata(req.Metadata) return nil @@ -1039,8 +1232,19 @@ func (r *SSHExecutor) GetRemoteStatus(ctx context.Context, instance *ExecutorIns if instance == nil { return nil, errors.New("instance is nil") } + // AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7: read the transport-lost + // marker and the client handle in one critical section and decide there, + // so a teardown can't close the client between the read and the probe. + // Only the decision needs the mutex; the healthy-session probe below + // runs after it is released. r.mu.Lock() state := r.sessions[instance.InstanceID] + var transportLost bool + var client *ssh.Client + if state != nil { + transportLost = state.transportLost + client = state.client + } r.mu.Unlock() now := time.Now() status := &RemoteStatus{ @@ -1059,13 +1263,27 @@ func (r *SSHExecutor) GetRemoteStatus(ctx context.Context, instance *ExecutorIns status.Details["user"] = state.target.User status.Details["fingerprint"] = state.target.PinnedFingerprint - probeCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - if state.client == nil { + if transportLost { + status.State = sshStatusDisconnected + status.ErrorMessage = sshTransportLostMessage + return status, nil + } + if client == nil { status.State = sshStatusDisconnected return status, nil } - if !isRemoteAgentctlAlive(probeCtx, state.client, state.pid) { + probeCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + if !isRemoteAgentctlAlive(probeCtx, client, state.pid) { + // A concurrent teardown may have closed client after the read above + // but before (or during) this probe, so a failed probe alone doesn't + // distinguish "agentctl not answering" from "transport lost mid-probe" + // (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7). Recheck the marker. + if r.isTransportLost(state) { + status.State = sshStatusDisconnected + status.ErrorMessage = sshTransportLostMessage + return status, nil + } status.State = sshStatusAgentctlDown return status, nil } diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_connection.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_connection.go index 39e52564c8..2344b0bcea 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_connection.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_connection.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "time" "github.com/kevinburke/ssh_config" @@ -502,6 +503,33 @@ func dialDirect(ctx context.Context, addr string, cfg *ssh.ClientConfig) (*ssh.C return ssh.NewClient(sshConn, chans, reqs), nil } +// sshProxyJumpConn owns both legs of a ProxyJump connection. Closing the +// bastion transport first releases any read blocked in the inner SSH client. +// The inner channel close then completes without relying on a remote reply. +type sshProxyJumpConn struct { + net.Conn + bastion interface{ Close() error } + once sync.Once + closeErr error +} + +func (c *sshProxyJumpConn) Close() error { + if c == nil { + return nil + } + c.once.Do(func() { + var bastionErr, tunnelErr error + if c.bastion != nil { + bastionErr = c.bastion.Close() + } + if c.Conn != nil { + tunnelErr = c.Conn.Close() + } + c.closeErr = errors.Join(bastionErr, tunnelErr) + }) + return c.closeErr +} + // dialViaJump implements ProxyJump as a single bastion hop. The bastion is // resolved from its own ~/.ssh/config Host block, defaulting to the same // identity source as the target (passes the user's agent / key through). @@ -539,17 +567,16 @@ func dialViaJump(ctx context.Context, target *SSHTarget, finalAddr string, final _ = bastionClient.Close() return nil, fmt.Errorf("ssh: bastion tunnel to %s: %w", finalAddr, err) } - sshConn, chans, reqs, err := ssh.NewClientConn(tunnel, finalAddr, finalCfg) + jumpConn := &sshProxyJumpConn{Conn: tunnel, bastion: bastionClient} + sshConn, chans, reqs, err := ssh.NewClientConn(jumpConn, finalAddr, finalCfg) if err != nil { - _ = tunnel.Close() - _ = bastionClient.Close() + _ = jumpConn.Close() return nil, fmt.Errorf("ssh: handshake with %s via %s: %w", finalAddr, target.ProxyJump, err) } - // Attach the bastion close to the final client lifetime. final := ssh.NewClient(sshConn, chans, reqs) go func() { _ = final.Wait() - _ = bastionClient.Close() + _ = jumpConn.Close() }() return final, nil } @@ -713,7 +740,11 @@ func parsePortString(s string) (int, bool) { } // SSH connections are owned per-session (no shared pool): a session's client -// lives on sshSessionState and is Close()d by StopInstance. The earlier -// pool/refcount/keepalive plumbing was dead code — the executor never used it -// in production, and the orphaned keepalive goroutines surfaced under e2e -// fault-injection. See PR #927 for the removal rationale. +// lives on sshSessionState and is Close()d by StopInstance. An earlier +// pool/refcount/keepalive design was removed as dead code in PR #927 — the +// executor never used it in production, and its background goroutines +// outlived their connections under e2e fault injection. The per-session +// transport-liveness watchdog in executor_ssh_keepalive.go restores a +// keepalive, but owned by the session it watches and stopped by the same +// disposal paths that close the client, so it cannot repeat that leak: see +// docs/specs/executors/requirements/ssh-transport-liveness.md. diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive.go new file mode 100644 index 0000000000..5cf5f2731e --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive.go @@ -0,0 +1,456 @@ +package lifecycle + +import ( + "errors" + "fmt" + "sync" + "time" + + "go.uber.org/zap" + "golang.org/x/crypto/ssh" +) + +// docs/specs/executors/requirements/ssh-transport-liveness.md +// AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.10 +const ( + defaultSSHKeepaliveInterval = 15 * time.Second + defaultSSHKeepaliveDeadline = 45 * time.Second +) + +// Tuning values for SSH session transport liveness. Package-level variables, +// not constants, so tests can shorten them (see goleak_test.go's TestMain, +// which zeroes the keepalive pair for this package's test binary — a +// non-positive value disables the watchdog per sshKeepaliveTuningValid). +var ( + sshKeepaliveInterval = defaultSSHKeepaliveInterval + sshKeepaliveDeadline = defaultSSHKeepaliveDeadline +) + +// sshBrokerPreflightTimeout bounds the credential-broker reachability +// preflight a broker-backed resume reset issues before it stops the stale +// remote controller. The preflight runs on the raw caller context today and +// is otherwise unbounded; 20s mirrors sshAgentctlCleanupTimeout, since both +// are reachability-scale checks, not long-running work. +var sshBrokerPreflightTimeout = 20 * time.Second + +// ErrSSHTransportLost identifies a disposal or reuse decision made because a +// session's SSH transport was declared lost (deadline/probe-error teardown) +// or has become unresponsive, distinguishing that cause from an ordinary +// remote failure or a host that refused the call. +var ErrSSHTransportLost = errors.New("ssh: session transport lost") + +const sshKeepaliveRequestType = "keepalive@openssh.com" + +const ( + sshTransportLostReasonProbeError = "probe error" + sshTransportLostReasonDeadline = "liveness deadline exceeded" +) + +// sshTransportClassification is the outcome of the single disposal reading +// AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8 requires: it governs every +// remote command a disposal path issues. +type sshTransportClassification int + +const ( + sshTransportAnswering sshTransportClassification = iota + sshTransportUnresponsive + sshTransportLost +) + +// sshKeepaliveTuningValid reports whether interval/deadline are usable +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.9): both positive, and the +// deadline strictly greater than twice the interval. +func sshKeepaliveTuningValid(interval, deadline time.Duration) bool { + return interval > 0 && deadline > 0 && deadline > 2*interval +} + +// sshKeepaliveWatchdog runs the two goroutines described in the system +// design's Control flow section for exactly one session SSH client: a +// prober that sends transport liveness probes one at a time, and a loop that +// decides when the transport is lost. It exposes start (via +// startSSHKeepaliveWatchdog), stop-deciding (stopAndAwaitLoop) and +// await-probe-exit (awaitProbeExit) — the first and last steps of a +// disposal, always in that order. +type sshKeepaliveWatchdog struct { + interval time.Duration + deadline time.Duration + client *ssh.Client + + stopCh chan struct{} + stopOnce sync.Once + + loopDone chan struct{} + proberDone chan struct{} + + recordReply func(time.Time) + onLost func(reason string, silence time.Duration) +} + +// startSSHKeepaliveWatchdog starts a watchdog for client. Callers must have +// already validated interval/deadline with sshKeepaliveTuningValid. +// startedAt is the watchdog's start time, used as the silence interval's +// baseline until the first probe reply completes. +func startSSHKeepaliveWatchdog( + client *ssh.Client, + interval, deadline time.Duration, + startedAt time.Time, + recordReply func(time.Time), + onLost func(reason string, silence time.Duration), +) *sshKeepaliveWatchdog { + w := &sshKeepaliveWatchdog{ + interval: interval, + deadline: deadline, + client: client, + stopCh: make(chan struct{}), + loopDone: make(chan struct{}), + proberDone: make(chan struct{}), + recordReply: recordReply, + onLost: onLost, + } + // replies is capacity-1 and lossy by design: a coalesced reply is + // harmless because another follows within one interval. errs is also + // capacity-1 but is written at most once ever (the prober returns + // immediately after), so the buffered send always succeeds instantly and + // is never dropped — unlike a reply, the loop must always observe it. + replies := make(chan time.Time, 1) + errs := make(chan error, 1) + go w.runProber(replies, errs) + go w.runLoop(replies, errs, startedAt) + return w +} + +// runProber sends one transport liveness probe at a time, spaced by the +// interval (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.2). It has exactly two +// exits, both unconditional: a probe error (published on the guaranteed- +// delivery errs channel before the goroutine returns, so the loop always +// observes it) and the stop signal observed while idle between probes. It +// always terminates once the client is closed, since SendRequest then errors +// immediately. +func (w *sshKeepaliveWatchdog) runProber(replies chan<- time.Time, errs chan<- error) { + defer close(w.proberDone) + for { + _, _, err := w.client.SendRequest(sshKeepaliveRequestType, true, nil) + now := time.Now() + if err != nil { + errs <- err + return + } + select { + case replies <- now: + default: + } + select { + case <-w.stopCh: + return + case <-time.After(w.interval): + } + } +} + +// runLoop decides when the transport is lost. Two precedence rules apply, +// because a select with several ready cases picks at random: a pending +// completed reply or probe error beats an expired deadline timer (handled by +// the non-blocking re-check inside the timer.C case below), and the stop +// signal beats both. The top-of-loop non-blocking check only catches a stop +// already signaled before that iteration's blocking select begins — it +// cannot catch a stop delivered concurrently with a probe error or an +// expired deadline, since the blocking select itself picks at random among +// whichever cases are ready together. So every path that is about to +// declare loss re-checks the stop signal immediately first and yields to it. +func (w *sshKeepaliveWatchdog) runLoop(replies <-chan time.Time, errs <-chan error, startedAt time.Time) { + defer close(w.loopDone) + lastReply := startedAt + timer := time.NewTimer(w.deadline) + defer timer.Stop() + for { + select { + case <-w.stopCh: + return + default: + } + select { + case <-w.stopCh: + return + case at := <-replies: + w.handleReply(at, &lastReply, timer) + case <-errs: + if w.stopRequested() { + return + } + w.declareLost(sshTransportLostReasonProbeError, time.Since(lastReply)) + return + case <-timer.C: + // A completed reply or a probe error may have arrived in the + // same instant the deadline fired. Either wins over the timer + // alone. + select { + case at := <-replies: + w.handleReply(at, &lastReply, timer) + case err := <-errs: + _ = err + if w.stopRequested() { + return + } + w.declareLost(sshTransportLostReasonProbeError, time.Since(lastReply)) + return + default: + if w.stopRequested() { + return + } + if w.handleDeadline(lastReply, timer) { + return + } + } + } + } +} + +// stopRequested reports whether the stop signal has been raised, without +// blocking. Used immediately before every action that would declare +// transport loss, so a stop delivered in the same instant as a probe error +// or an expired deadline always wins. +func (w *sshKeepaliveWatchdog) stopRequested() bool { + select { + case <-w.stopCh: + return true + default: + return false + } +} + +func (w *sshKeepaliveWatchdog) handleReply(at time.Time, lastReply *time.Time, timer *time.Timer) { + *lastReply = at + if w.recordReply != nil { + w.recordReply(*lastReply) + } + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + timer.Reset(w.deadline) +} + +func (w *sshKeepaliveWatchdog) handleDeadline(lastReply time.Time, timer *time.Timer) bool { + silence := time.Since(lastReply) + if silence > w.deadline { + w.declareLost(sshTransportLostReasonDeadline, silence) + return true + } + // Spurious wake (the timer fired just as the deadline's boundary was + // reached but the strictly-greater check above didn't trip): reset for + // the remaining time rather than looping busily. + remaining := w.deadline - silence + if remaining <= 0 { + remaining = time.Millisecond + } + timer.Reset(remaining) + return false +} + +func (w *sshKeepaliveWatchdog) declareLost(reason string, silence time.Duration) { + if w.onLost != nil { + w.onLost(reason, silence) + } +} + +// stopAndAwaitLoop signals the watchdog to stop and waits for the watchdog +// loop — not the prober — to exit. Bounded: the loop is either parked in its +// select, already returned, or performing a teardown (two local handle +// closes and one log line), so this never touches the network. Idempotent +// and safe on a nil-free zero watchdog that never started. Callers must not +// hold the executor mutex while waiting here: a mid-teardown loop acquires +// that mutex before closing loopDone, so holding it would deadlock. +func (w *sshKeepaliveWatchdog) stopAndAwaitLoop() { + w.stopOnce.Do(func() { close(w.stopCh) }) + <-w.loopDone +} + +// awaitProbeExit waits for the prober goroutine to exit. Call after +// stopAndAwaitLoop and after the session SSH client has been closed: the +// close is what unblocks a prober mid-SendRequest, whether idle or wedged. +func (w *sshKeepaliveWatchdog) awaitProbeExit() { + <-w.proberDone +} + +// classifySSHTransportLocked reads state's transport-lost marker and, when a +// watchdog is running, its silence interval, and classifies the transport +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8). Callers must hold the +// executor mutex — the marker and the last-probe-reply timestamp it reads +// are guarded by it. +func classifySSHTransportLocked(state *sshSessionState) sshTransportClassification { + if state.transportLost { + return sshTransportLost + } + if state.watchdog == nil { + return sshTransportAnswering + } + silence := time.Since(state.lastProbeReply) + if silence > 2*state.watchdog.interval { + return sshTransportUnresponsive + } + return sshTransportAnswering +} + +// closeForwarderOnce closes state's local port forward through its +// once-guard (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.7): whichever of +// transport teardown or a disposal path reaches it first performs the +// close, and the other observes the same result. +func (r *SSHExecutor) closeForwarderOnce(state *sshSessionState) error { + state.forwarderCloseOnce.Do(func() { + if state.forwarder != nil { + state.forwarderCloseErr = state.forwarder.Close() + } + }) + return state.forwarderCloseErr +} + +// closeClientOnce closes state's session SSH client through its own +// once-guard, independent of the forwarder's. +func (r *SSHExecutor) closeClientOnce(state *sshSessionState) error { + state.clientCloseOnce.Do(func() { + if state.client != nil { + state.clientCloseErr = r.closeSSHClient(state.client) + } + }) + return state.clientCloseErr +} + +// markTransportLost sets state's transport-lost marker without performing a +// full transport teardown. Used by the disposal backstop +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8): it closes only the client +// through its once-guard and must not log the teardown warning, so it is not +// "transport teardown" under the spec's Terminology. +func (r *SSHExecutor) markTransportLost(state *sshSessionState) { + r.mu.Lock() + state.transportLost = true + r.mu.Unlock() +} + +// isTransportLost reports state's transport-lost marker under the executor +// mutex. +func (r *SSHExecutor) isTransportLost(state *sshSessionState) bool { + r.mu.Lock() + defer r.mu.Unlock() + return state.transportLost +} + +// transportTeardown performs transport teardown for state +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5): set the marker, close the +// forward then the client (each through its once-guard), close the optional +// runtime API tunnel after the client, and log one warning +// naming the executor instance, remote host, and silence interval. Runs at +// most once per session (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.5); it does +// not remove the session from the tracked-sessions map +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.6). +func (r *SSHExecutor) transportTeardown(instanceID string, state *sshSessionState, reason string, silence time.Duration) { + state.teardownOnce.Do(func() { + r.mu.Lock() + state.transportLost = true + r.mu.Unlock() + forwarderErr := r.closeForwarderOnce(state) + clientErr := r.closeClientOnce(state) + _ = state.runtimeAPITunnel.Close() + r.logTransportLoss(instanceID, state, reason, silence, forwarderErr, clientErr) + }) +} + +func (r *SSHExecutor) logTransportLoss(instanceID string, state *sshSessionState, reason string, silence time.Duration, forwarderErr, clientErr error) { + host := "" + if state.target != nil { + host = state.target.Host + } + fields := []zap.Field{ + zap.String("instance_id", instanceID), + zap.String("host", host), + zap.Duration("silence_interval", silence), + zap.String("reason", reason), + } + if forwarderErr != nil { + fields = append(fields, zap.NamedError("forwarder_close_error", forwarderErr)) + } + if clientErr != nil { + fields = append(fields, zap.NamedError("client_close_error", clientErr)) + } + r.logger.Warn(sshTransportLostMessage, fields...) +} + +// startWatchdogLocked starts a watchdog for state's session SSH client, in +// the same critical section that recorded state in r.sessions +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1). Callers must hold r.mu. A +// non-positive interval/deadline, or a deadline not greater than twice the +// interval, leaves state.watchdog nil and every existing disposal path +// unchanged (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.9). +func (r *SSHExecutor) startWatchdogLocked(instanceID string, state *sshSessionState) { + interval, deadline := sshKeepaliveInterval, sshKeepaliveDeadline + if !sshKeepaliveTuningValid(interval, deadline) { + r.logger.Debug("ssh: not starting transport watchdog", zap.String("instance_id", instanceID)) + return + } + startedAt := time.Now() + state.lastProbeReply = startedAt + state.watchdog = startSSHKeepaliveWatchdog(state.client, interval, deadline, startedAt, + func(at time.Time) { + r.mu.Lock() + state.lastProbeReply = at + r.mu.Unlock() + }, + func(reason string, silence time.Duration) { + r.transportTeardown(instanceID, state, reason, silence) + }, + ) +} + +// stopWatchdogAndWait signals state's watchdog to stop and waits for its +// loop to exit. A no-op on a session that never started one. +func (r *SSHExecutor) stopWatchdogAndWait(state *sshSessionState) { + if state.watchdog != nil { + state.watchdog.stopAndAwaitLoop() + } +} + +// awaitProbeExit waits for state's watchdog's prober to exit. A no-op on a +// session that never started one. +func (r *SSHExecutor) awaitProbeExit(state *sshSessionState) { + if state.watchdog != nil { + state.watchdog.awaitProbeExit() + } +} + +// runWithTransportBackstop runs fn — a remote command issued over state's +// session SSH client — on its own goroutine and races it against timeout +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8). fn is expected to carry its +// own bound (typically a context.WithTimeout of the same duration) for the +// ordinary case where it is already executing; the backstop exists for the +// harder case where fn is blocked opening its SSH channel, which no context +// bounds. On timeout it closes the client through its once-guard — the only +// thing that releases a call blocked in Client.NewSession — marks the client +// transport-lost, and waits for fn's goroutine to actually exit before +// returning, so the abandoned command never outlives this call. +func (r *SSHExecutor) runWithTransportBackstop(state *sshSessionState, timeout time.Duration, fn func() error) error { + done := make(chan error, 1) + go func() { done <- fn() }() + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case err := <-done: + return err + case <-timer.C: + // fn may have completed in the same instant the timer fired — a + // select with both cases ready picks at random. Recheck done + // before treating this as a timeout, so a command that actually + // finished is never reported as abandoned. + select { + case err := <-done: + return err + default: + } + r.markTransportLost(state) + _ = r.closeClientOnce(state) + if err := <-done; err != nil { + return err + } + return fmt.Errorf("ssh: remote command exceeded %s and was abandoned", timeout) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive_test.go new file mode 100644 index 0000000000..2afff3d409 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_keepalive_test.go @@ -0,0 +1,1069 @@ +package lifecycle + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "runtime" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" + "github.com/kandev/kandev/internal/common/logger" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + "golang.org/x/crypto/ssh" +) + +// Tests in this file exercise SSH session transport liveness +// (docs/specs/executors/requirements/ssh-transport-liveness.md). None use +// t.Parallel(): sshKeepaliveInterval, sshKeepaliveDeadline, sshCleanupTimeout, +// sshAgentctlCleanupTimeout and sshBrokerPreflightTimeout are package-level +// and mutable, and goleak_test.go's TestMain zeroes the keepalive pair for +// every other test in this package. + +// withSSHKeepaliveTuning sets the keepalive interval/deadline pair for the +// duration of one test and restores the package defaults (zero, per +// TestMain) afterward. +func withSSHKeepaliveTuning(t *testing.T, interval, deadline time.Duration) { + t.Helper() + prevInterval, prevDeadline := sshKeepaliveInterval, sshKeepaliveDeadline + sshKeepaliveInterval, sshKeepaliveDeadline = interval, deadline + t.Cleanup(func() { sshKeepaliveInterval, sshKeepaliveDeadline = prevInterval, prevDeadline }) +} + +// withSSHRemoteCleanupTimeouts sets the three remote-cleanup timeouts for the +// duration of one test and restores their real defaults afterward. +func withSSHRemoteCleanupTimeouts(t *testing.T, cleanup, agentctl, brokerPreflight time.Duration) { + t.Helper() + prevCleanup, prevAgentctl, prevPreflight := sshCleanupTimeout, sshAgentctlCleanupTimeout, sshBrokerPreflightTimeout + sshCleanupTimeout, sshAgentctlCleanupTimeout, sshBrokerPreflightTimeout = cleanup, agentctl, brokerPreflight + t.Cleanup(func() { + sshCleanupTimeout, sshAgentctlCleanupTimeout, sshBrokerPreflightTimeout = prevCleanup, prevAgentctl, prevPreflight + }) +} + +// newTrackedSSHSession inserts a session and starts its watchdog exactly as +// CreateInstance/ResumeRemoteInstance do: under the executor mutex, in the +// same critical section as the record. +func newTrackedSSHSession(exec *SSHExecutor, instanceID string, client *ssh.Client) *sshSessionState { + state := &sshSessionState{ + target: &SSHTarget{Host: "build.example", User: "deploy", PinnedFingerprint: "SHA256:x"}, + client: client, + pid: 4242, + remoteDir: "/remote/session", + remoteTaskDir: "/remote/task", + } + exec.mu.Lock() + exec.sessions[instanceID] = state + exec.startWatchdogLocked(instanceID, state) + exec.mu.Unlock() + return state +} + +// newObservedSSHExecutor builds an SSHExecutor whose logger records every +// Warn-and-above entry, so a test can assert on the transport-loss warning +// AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.9 requires instead of only on the +// in-memory transportLost marker. +func newObservedSSHExecutor(t *testing.T) (*SSHExecutor, *observer.ObservedLogs) { + t.Helper() + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("observer logger: %v", err) + } + return NewSSHExecutor(nil, nil, nil, log), logs +} + +// newTrackedSSHSessionWithForwarder inserts a session with both a real +// session SSH client and a real local port forward — the same pair +// CreateInstance/ResumeRemoteInstance record — and starts its watchdog in the +// same critical section as the record. Unlike newTrackedSSHSession, this lets +// a test observe the forwarder half of transport teardown +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5/-001.6). +func newTrackedSSHSessionWithForwarder(exec *SSHExecutor, instanceID string, client *ssh.Client, fwd *SSHPortForwarder) *sshSessionState { + state := &sshSessionState{ + target: &SSHTarget{Host: "build.example", User: "deploy", PinnedFingerprint: "SHA256:x"}, + client: client, + forwarder: fwd, + pid: 4242, + remoteDir: "/remote/session", + remoteTaskDir: "/remote/task", + } + exec.mu.Lock() + exec.sessions[instanceID] = state + exec.startWatchdogLocked(instanceID, state) + exec.mu.Unlock() + return state +} + +// dialForwarder reports whether a new TCP connection to fwd's local port is +// currently accepted, closing it immediately if so. +func dialForwarder(fwd *SSHPortForwarder) bool { + conn, err := net.DialTimeout("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(fwd.LocalPort())), time.Second) + if err != nil { + return false + } + _ = conn.Close() + return true +} + +// waitClosedWithin blocks until ch is closed, failing the test if timeout +// elapses first. +func waitClosedWithin(t *testing.T, ch <-chan struct{}, timeout time.Duration, what string) { + t.Helper() + select { + case <-ch: + case <-time.After(timeout): + t.Fatalf("%s did not happen within %s", what, timeout) + } +} + +// waitForFreshProbeReply blocks until at least minElapsed has passed since +// this call started and state's last recorded probe reply (read under the +// same executor mutex classifySSHTransportLocked reads it under) is younger +// than margin, so a caller can act on an observed-fresh reading instead of +// sleeping a fixed duration and hoping scheduling jitter didn't eat the +// unresponsive margin before its own read happens. minElapsed still forces +// several real probe cycles to have had a chance to run, so a regression +// that stops recording replies (leaving the reading frozen at session start) +// times out and fails loudly instead of passing on the first, trivially +// fresh check. +func waitForFreshProbeReply(t *testing.T, exec *SSHExecutor, state *sshSessionState, minElapsed, margin, timeout time.Duration) { + t.Helper() + start := time.Now() + deadline := start.Add(timeout) + for { + now := time.Now() + exec.mu.Lock() + age := now.Sub(state.lastProbeReply) + exec.mu.Unlock() + if now.Sub(start) >= minElapsed && age < margin { + return + } + if now.After(deadline) { + t.Fatalf("no probe reply younger than %s observed after waiting %s (last reply age %s)", margin, timeout, age) + } + time.Sleep(time.Millisecond) + } +} + +func TestSSHKeepaliveTuningValid(t *testing.T) { + tests := []struct { + name string + interval, deadline time.Duration + want bool + }{ + {"positive pair with deadline > 2x interval", 10 * time.Millisecond, 25 * time.Millisecond, true}, + {"deadline exactly 2x interval is invalid", 10 * time.Millisecond, 20 * time.Millisecond, false}, + {"zero interval", 0, 45 * time.Second, false}, + {"zero deadline", 15 * time.Second, 0, false}, + {"negative interval", -1, 45 * time.Second, false}, + {"deadline less than interval", 30 * time.Millisecond, 10 * time.Millisecond, false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := sshKeepaliveTuningValid(tc.interval, tc.deadline); got != tc.want { + t.Fatalf("sshKeepaliveTuningValid(%v, %v) = %v, want %v", tc.interval, tc.deadline, got, tc.want) + } + }) + } +} + +// TestSSHKeepaliveProductionDefaultsMatchSpec pins the package vars' +// zero-test-override values against the named constants they initialize +// from, so a change to either constant is a deliberate, visible edit here +// rather than a silent drift from the spec's mandated 15s/45s pair — nothing +// else in this suite asserts the untouched production defaults, since +// TestMain zeroes sshKeepaliveInterval/sshKeepaliveDeadline for every other +// test in this package's binary. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.10 +func TestSSHKeepaliveProductionDefaultsMatchSpec(t *testing.T) { + if defaultSSHKeepaliveInterval != 15*time.Second { + t.Fatalf("defaultSSHKeepaliveInterval = %v, want 15s", defaultSSHKeepaliveInterval) + } + if defaultSSHKeepaliveDeadline != 45*time.Second { + t.Fatalf("defaultSSHKeepaliveDeadline = %v, want 45s", defaultSSHKeepaliveDeadline) + } + if !sshKeepaliveTuningValid(defaultSSHKeepaliveInterval, defaultSSHKeepaliveDeadline) { + t.Fatal("the production defaults must themselves satisfy sshKeepaliveTuningValid") + } +} + +// TestSSHKeepaliveWatchdogDeclaresLossOnDeadline exercises the raw watchdog's +// own decision timing in isolation from SSHExecutor — its onLost callback +// here is a test channel, not the production transportTeardown, so it does +// not exercise AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.9's warning log (see +// TestSSHKeepaliveDeadlineTearsDownTrackedSessionWithinBoundedTime for that, +// wired through the real executor). +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3 +func TestSSHKeepaliveWatchdogDeclaresLossOnDeadline(t *testing.T) { + server := newFakeSSHServer(t, nil) + client := server.dial(t) + server.setSilent(true) // every probe goes unanswered from the start + + lost := make(chan struct{}, 1) + var reason string + var silence time.Duration + w := startSSHKeepaliveWatchdog(client, 5*time.Millisecond, 20*time.Millisecond, time.Now(), nil, + func(r string, s time.Duration) { + reason, silence = r, s + lost <- struct{}{} + }) + select { + case <-lost: + case <-time.After(2 * time.Second): + t.Fatal("watchdog did not declare transport loss within the deadline") + } + if reason != sshTransportLostReasonDeadline { + t.Fatalf("reason = %q, want %q", reason, sshTransportLostReasonDeadline) + } + if silence <= 20*time.Millisecond { + t.Fatalf("silence = %s, want strictly greater than the 20ms deadline", silence) + } + // The prober is still blocked in its SendRequest; only closing the + // client releases it. + _ = client.Close() + w.stopAndAwaitLoop() + w.awaitProbeExit() +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.4 +func TestSSHKeepaliveWatchdogDeclaresLossOnProbeError(t *testing.T) { + server := newFakeSSHServer(t, nil) + client := server.dial(t) + + lost := make(chan struct{}, 1) + var reason string + w := startSSHKeepaliveWatchdog(client, 5*time.Millisecond, 500*time.Millisecond, time.Now(), nil, + func(r string, _ time.Duration) { + reason = r + lost <- struct{}{} + }) + time.Sleep(15 * time.Millisecond) // let at least one probe complete normally + _ = client.Close() + + select { + case <-lost: + case <-time.After(2 * time.Second): + t.Fatal("watchdog did not declare transport loss after the client closed") + } + if reason != sshTransportLostReasonProbeError { + t.Fatalf("reason = %q, want %q", reason, sshTransportLostReasonProbeError) + } + w.stopAndAwaitLoop() + w.awaitProbeExit() +} + +// TestSSHKeepaliveWatchdogStopSignalBeatsACoincidingProbeError drives +// runLoop's stop-vs-event select directly, forcing a genuine tie between its +// stopCh case and its errs case rather than an arbitrary real-time race +// between them. With GOMAXPROCS pinned to 1, runLoop is deterministically +// parked in its blocking select (nothing runnable to preempt it) by the time +// this test closes stopCh and sends the probe error back to back with no +// intervening scheduling point — so runLoop resumes to find both cases +// simultaneously ready, and Go's select picks between them at random. +// AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.10 forbids declaring loss for a +// session stopped while its transport is still alive, so even on the +// iterations where the random pick lands on the errs case, runLoop's recheck +// of the already-closed stopCh must still stop it from declaring loss. +// Repeated so the random pick lands on the errs case many times, not just +// once. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.10 +func TestSSHKeepaliveWatchdogStopSignalBeatsACoincidingProbeError(t *testing.T) { + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1)) + + const iterations = 300 + for i := 0; i < iterations; i++ { + w := &sshKeepaliveWatchdog{ + interval: time.Hour, + deadline: time.Hour, // never fires on its own; isolates stop vs. errs + stopCh: make(chan struct{}), + loopDone: make(chan struct{}), + } + var lostCalled atomic.Bool + w.onLost = func(string, time.Duration) { lostCalled.Store(true) } + + replies := make(chan time.Time, 1) + errs := make(chan error, 1) + go w.runLoop(replies, errs, time.Now()) + // With GOMAXPROCS(1) this test goroutine keeps running until it + // blocks or explicitly yields, so runLoop cannot have reached its + // select yet — Gosched hands off to it, and since nothing else is + // ready it parks there, definitely blocked, before control returns + // here. + runtime.Gosched() + + errs <- errors.New("probe error") + close(w.stopCh) + + select { + case <-w.loopDone: + case <-time.After(2 * time.Second): + t.Fatalf("iteration %d: runLoop did not exit after a coinciding stop+probe-error", i) + } + if lostCalled.Load() { + t.Fatalf("iteration %d: a stop signal coinciding with a probe error still declared transport loss", i) + } + } +} + +// TestSSHKeepaliveWatchdogReplyBeatsACoincidingDeadlineExpiry drives runLoop's +// reply-vs-deadline select directly. The reply is buffered before runLoop's +// goroutine has run at all (a buffered send needs no receiver), and the +// deadline is non-positive so its timer fires as soon as the runtime gets to +// it — so by the time runLoop's blocking select first evaluates, both cases +// are frequently ready together and Go's random pick exercises the timer.C +// branch as often as the direct replies case. AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3 +// requires a completed reply not yet accounted for to be accounted for first +// even when a deadline expiry is also observable at the same moment: the +// timer.C branch's own inner select must still find and consume the pending +// reply rather than falling through to declare loss. onLost.recordReply +// raises the deadline once the injected reply is accounted for (whichever +// path got there), so a real, unrelated second expiry can't also fire during +// the observation window. Repeated so the random pick lands on the timer.C +// branch many times, not just once. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3 +func TestSSHKeepaliveWatchdogReplyBeatsACoincidingDeadlineExpiry(t *testing.T) { + defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(1)) + + const iterations = 300 + for i := 0; i < iterations; i++ { + w := &sshKeepaliveWatchdog{ + interval: time.Hour, + deadline: -1, // fires as soon as the runtime processes it + stopCh: make(chan struct{}), + loopDone: make(chan struct{}), + } + var lostCalled atomic.Bool + w.onLost = func(string, time.Duration) { lostCalled.Store(true) } + w.recordReply = func(time.Time) { + // Accounted for: stop the deadline from legitimately re-expiring + // on its own so this test can observe "no loss declared" without + // racing a second, unrelated timer. + w.deadline = time.Hour + } + + replies := make(chan time.Time, 1) + errs := make(chan error, 1) + replies <- time.Now() // buffered before runLoop's goroutine exists + go w.runLoop(replies, errs, time.Now()) + + select { + case <-w.loopDone: + t.Fatalf("iteration %d: runLoop exited instead of accounting for the pending reply", i) + case <-time.After(5 * time.Millisecond): + // Still running, having accounted for the reply rather than + // declaring loss on the coinciding deadline expiry — required. + } + close(w.stopCh) + waitClosedWithin(t, w.loopDone, 2*time.Second, "runLoop exiting after stop") + if lostCalled.Load() { + t.Fatalf("iteration %d: a reply that beat (or tied) a coinciding deadline expiry still declared transport loss", i) + } + } +} + +// TestSSHKeepaliveDeadlineTearsDownTrackedSessionWithinBoundedTime is the +// capability this card exists for: a session whose transport falls silent is +// torn down automatically, within a bounded time, with no disposal call +// (StopInstance/Close) driving it. It ends without stopping the session — per +// the system design's Testing section — so goleak.VerifyTestMain proves the +// prober's own error exit (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.12) rather +// than a disposal call retiring it first. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.6 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.9 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.12 +func TestSSHKeepaliveDeadlineTearsDownTrackedSessionWithinBoundedTime(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Millisecond, 20*time.Millisecond) + + exec, logs := newObservedSSHExecutor(t) + server := newFakeSSHServer(t, nil) + client := server.dial(t) + fwd, err := StartPortForward(client, 41234, newTestLogger()) + if err != nil { + t.Fatalf("StartPortForward: %v", err) + } + t.Cleanup(func() { _ = fwd.Close() }) + state := newTrackedSSHSessionWithForwarder(exec, "instance-1", client, fwd) + + if !dialForwarder(fwd) { + t.Fatal("forwarder port must accept connections before teardown") + } + + server.setSilent(true) // every probe goes unanswered from here on + + waitClosedWithin(t, state.watchdog.loopDone, 2*time.Second, "the watchdog loop declaring transport loss") + waitClosedWithin(t, state.watchdog.proberDone, 2*time.Second, "the prober exiting after teardown closed the client") + + if !exec.isTransportLost(state) { + t.Fatal("isTransportLost = false, want true after an internally-triggered teardown") + } + if dialForwarder(fwd) { + t.Fatal("forwarder port still accepts connections after teardown, want it refused") + } + + var warnings []observer.LoggedEntry + for _, entry := range logs.All() { + if entry.Message == sshTransportLostMessage { + warnings = append(warnings, entry) + } + } + if len(warnings) != 1 { + t.Fatalf("transport-loss warnings = %d, want exactly 1 (entries: %v)", len(warnings), logs.All()) + } + fields := warnings[0].ContextMap() + if fields["instance_id"] != "instance-1" { + t.Fatalf("warning instance_id = %v, want instance-1", fields["instance_id"]) + } + if fields["host"] != "build.example" { + t.Fatalf("warning host = %v, want build.example", fields["host"]) + } + if _, ok := fields["silence_interval"]; !ok { + t.Fatal("warning missing silence_interval field") + } + if _, ok := fields["forwarder_close_error"]; ok { + t.Fatalf("warning unexpectedly named a forwarder_close_error: %v", fields) + } + if _, ok := fields["client_close_error"]; ok { + t.Fatalf("warning unexpectedly named a client_close_error: %v", fields) + } +} + +// TestSSHKeepaliveTeardownNamesAForwarderCloseError covers the presence side +// TestSSHKeepaliveDeadlineTearsDownTrackedSessionWithinBoundedTime +// deliberately only asserts the absence of: when closing the forwarder +// itself fails, transportTeardown's warning must name that error, and the +// client close must still happen regardless +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5). The forwarder's once-guard is +// pre-spent with a synthetic error here, exactly as a real forwarder.Close() +// failure would leave it, rather than forcing an actual close failure. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5 +func TestSSHKeepaliveTeardownNamesAForwarderCloseError(t *testing.T) { + exec, logs := newObservedSSHExecutor(t) + server := newFakeSSHServer(t, nil) + client := server.dial(t) + t.Cleanup(func() { _ = client.Close() }) + + synthetic := errors.New("synthetic forwarder close error") + state := &sshSessionState{ + target: &SSHTarget{Host: "build.example", User: "deploy", PinnedFingerprint: "SHA256:x"}, + client: client, + } + state.forwarderCloseOnce.Do(func() { state.forwarderCloseErr = synthetic }) + + exec.transportTeardown("instance-1", state, sshTransportLostReasonDeadline, 90*time.Second) + + if _, err := client.NewSession(); err == nil { + t.Fatal("client must still be closed despite the forwarder close error") + } + + var warnings []observer.LoggedEntry + for _, entry := range logs.All() { + if entry.Message == sshTransportLostMessage { + warnings = append(warnings, entry) + } + } + if len(warnings) != 1 { + t.Fatalf("transport-loss warnings = %d, want exactly 1 (entries: %v)", len(warnings), logs.All()) + } + fields := warnings[0].ContextMap() + got, ok := fields["forwarder_close_error"] + if !ok { + t.Fatalf("warning missing forwarder_close_error field: %v", fields) + } + if fmt.Sprint(got) != synthetic.Error() { + t.Fatalf("warning forwarder_close_error = %v, want %v", got, synthetic) + } + if _, ok := fields["client_close_error"]; ok { + t.Fatalf("warning unexpectedly named a client_close_error: %v", fields) + } +} + +// TestSSHKeepaliveStopInstanceAfterInternalTeardownIsANoOp covers what +// TestSSHKeepaliveDeadlineTearsDownTrackedSessionWithinBoundedTime +// deliberately does not: a disposal call (StopInstance) arriving after an +// internally-triggered teardown has already run. Transport teardown must not +// remove the session from the tracked sessions (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.6) +// — removal stays the disposal path's job — so the later stop still finds and +// removes it, skips its remote commands because the reading classifies the +// transport as lost (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8), and performs +// no second teardown (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.5): teardown +// runs at most once whether or not the session is subsequently stopped +// (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.12). +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.5 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.6 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.12 +func TestSSHKeepaliveStopInstanceAfterInternalTeardownIsANoOp(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Millisecond, 20*time.Millisecond) + + exec, logs := newObservedSSHExecutor(t) + cleanupCalls, stopCalls := 0, 0 + exec.cleanupScript = func(context.Context, *ssh.Client, string, map[string]interface{}, map[string]string, SSHRemotePlatform, string, string) error { + cleanupCalls++ + return nil + } + exec.stopRemote = func(context.Context, *ssh.Client, string, int) error { + stopCalls++ + return nil + } + server := newFakeSSHServer(t, nil) + client := server.dial(t) + fwd, err := StartPortForward(client, 41234, newTestLogger()) + if err != nil { + t.Fatalf("StartPortForward: %v", err) + } + t.Cleanup(func() { _ = fwd.Close() }) + state := newTrackedSSHSessionWithForwarder(exec, "instance-1", client, fwd) + + server.setSilent(true) // every probe goes unanswered from here on + waitClosedWithin(t, state.watchdog.loopDone, 2*time.Second, "the watchdog loop declaring transport loss") + waitClosedWithin(t, state.watchdog.proberDone, 2*time.Second, "the prober exiting after teardown closed the client") + + exec.mu.Lock() + tracked := exec.sessions["instance-1"] == state + exec.mu.Unlock() + if !tracked { + t.Fatal("transport teardown must not remove the session from the tracked sessions — that stays the disposal path's job") + } + + if err := exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "instance-1", + StopReason: StopReasonTaskDeleted, // would trigger the cleanup script on a healthy transport + }, false); err != nil { + t.Fatalf("StopInstance after an internal teardown: %v", err) + } + if cleanupCalls != 0 || stopCalls != 0 { + t.Fatalf("cleanup calls = %d, stop calls = %d, want both 0 — a lost transport must skip every remote command", cleanupCalls, stopCalls) + } + + var warnings int + for _, entry := range logs.All() { + if entry.Message == sshTransportLostMessage { + warnings++ + } + } + if warnings != 1 { + t.Fatalf("transport-loss warnings = %d, want exactly 1 — a disposal after teardown must not run a second teardown", warnings) + } +} + +// TestSSHKeepaliveProbeErrorTearsDownTrackedSessionWithinBoundedTime exercises +// AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.4's real teardown path — the +// session's transport terminating on its own — through the full production +// wiring (a real SSHExecutor and forwarder), not just the raw watchdog struct +// TestSSHKeepaliveWatchdogDeclaresLossOnProbeError exercises in isolation. It +// also ends without stopping the session. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.4 +func TestSSHKeepaliveProbeErrorTearsDownTrackedSessionWithinBoundedTime(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Millisecond, 2*time.Second) // deadline kept out of reach: only a probe error should trip this + + exec, logs := newObservedSSHExecutor(t) + server := newFakeSSHServer(t, nil) + client := server.dial(t) + fwd, err := StartPortForward(client, 41234, newTestLogger()) + if err != nil { + t.Fatalf("StartPortForward: %v", err) + } + t.Cleanup(func() { _ = fwd.Close() }) + state := newTrackedSSHSessionWithForwarder(exec, "instance-1", client, fwd) + + time.Sleep(15 * time.Millisecond) // let at least one probe complete normally + _ = client.Close() // simulate the transport terminating on its own + + waitClosedWithin(t, state.watchdog.loopDone, 2*time.Second, "the watchdog loop declaring transport loss") + waitClosedWithin(t, state.watchdog.proberDone, 2*time.Second, "the prober exiting after the probe error") + + if !exec.isTransportLost(state) { + t.Fatal("isTransportLost = false, want true after a probe-error teardown") + } + if dialForwarder(fwd) { + t.Fatal("forwarder port still accepts connections after teardown, want it refused") + } + found := false + for _, entry := range logs.All() { + if entry.Message == sshTransportLostMessage && entry.ContextMap()["reason"] == sshTransportLostReasonProbeError { + found = true + } + } + if !found { + t.Fatalf("no transport-loss warning recorded with reason=%q, entries: %v", sshTransportLostReasonProbeError, logs.All()) + } +} + +// TestSSHKeepaliveHealthyTransportKeepsAnsweringPastTwiceTheInterval pins +// reply accounting: state.lastProbeReply only advances because +// startWatchdogLocked's recordReply callback runs on every completed probe +// reply (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.2/-001.3). A regression that +// stops recording replies leaves lastProbeReply frozen at session start, +// which classifySSHTransportLocked misreads as "unresponsive" once twice the +// probe interval has elapsed — even though the transport is healthy — and +// StopInstance would then skip its remote cleanup/stop calls entirely. This +// test sleeps well past that threshold on an answering server and asserts +// the remote calls still ran, so it fails under that regression even though +// isTransportLost alone would not catch it (classification, not the +// transport-lost marker, is what a regression here corrupts). +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.2 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.3 +func TestSSHKeepaliveHealthyTransportKeepsAnsweringPastTwiceTheInterval(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Millisecond, 200*time.Millisecond) // unresponsive threshold 10ms, deadline far out of reach + + server := newFakeSSHServer(t, nil) + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + cleanupCalls, stopCalls := 0, 0 + exec.cleanupScript = func(context.Context, *ssh.Client, string, map[string]interface{}, map[string]string, SSHRemotePlatform, string, string) error { + cleanupCalls++ + return nil + } + exec.stopRemote = func(context.Context, *ssh.Client, string, int) error { + stopCalls++ + return nil + } + client := server.dial(t) + state := newTrackedSSHSession(exec, "instance-1", client) + + // Wait past several probe cycles for an observed-fresh reply, then act on + // it immediately, rather than sleeping a fixed duration and hoping the + // 10ms unresponsive margin survives scheduling jitter (the fixed sleep + // this replaced was intermittently flaky under load for exactly that + // reason). + waitForFreshProbeReply(t, exec, state, 30*time.Millisecond, 3*time.Millisecond, 2*time.Second) + + if err := exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "instance-1", + StopReason: StopReasonTaskDeleted, // triggers the cleanup script + }, false); err != nil { + t.Fatalf("StopInstance: %v", err) + } + if cleanupCalls != 1 || stopCalls != 1 { + t.Fatalf("cleanup calls = %d, stop calls = %d, want both 1 — a healthy transport must still classify as answering", cleanupCalls, stopCalls) + } + if exec.isTransportLost(state) { + t.Fatal("a deliberate stop on a healthy transport must not declare transport loss") + } +} + +func TestSSHExecutorCreateInstanceRefusesATransportLostSession(t *testing.T) { + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + exec.sessions["instance-1"] = &sshSessionState{ + authToken: "resumed-token", + transportLost: true, + } + req := &ExecutorCreateRequest{ + InstanceID: "instance-1", + Metadata: map[string]interface{}{ + MetadataKeySSHRemoteTaskDir: "/remote/task", + }, + } + _, err := exec.CreateInstance(context.Background(), req) + if !errors.Is(err, ErrSSHTransportLost) { + t.Fatalf("error = %v, want ErrSSHTransportLost", err) + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1 +func TestSSHExecutorBuildInstanceForLostRaceReturnsCompleteResumeMetadata(t *testing.T) { + server := newFakeSSHServer(t, nil) + fwd, err := StartPortForward(server.dial(t), 41234, newTestLogger()) + if err != nil { + t.Fatalf("StartPortForward: %v", err) + } + t.Cleanup(func() { _ = fwd.Close() }) + + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + existing := &sshSessionState{ + target: &SSHTarget{Host: "build.example", Port: 22, User: "deploy", PinnedFingerprint: "SHA256:x"}, + forwarder: fwd, + pid: 4242, + remoteDir: "/remote/session", + remoteTaskDir: "/remote/task", + authToken: "winner-token", + port: 41234, + workdirRoot: "/custom/workdir", + // The winner's own cloned request metadata, as set by + // openSSHRuntimeAPITunnelForRequest when runtime API tunneling is + // enabled — must survive into the loser's returned instance rather + // than the loser's own (by-then-closed) tunnel's values. + metadata: map[string]interface{}{ + MetadataKeySSHRuntimeAPILocalURL: "http://127.0.0.1:9000", + MetadataKeySSHRuntimeAPIRemotePort: "50000", + }, + } + req := &ExecutorCreateRequest{InstanceID: "instance-1", TaskID: "task-1", SessionID: "session-1"} + + instance := exec.buildInstanceForLostRace(req, existing) + + // A losing caller never created this session — its returned metadata + // must be exactly as complete as the winner's, or a later + // ResumeRemoteInstance (which keys off MetadataKeySSHRemoteAgentctlPort + // being non-empty) treats it as "not a resume" and orphans the still- + // running remote agentctl this race was meant to preserve. + want := map[string]interface{}{ + MetadataKeySSHHost: "build.example", + MetadataKeySSHPort: "22", + MetadataKeySSHUser: "deploy", + MetadataKeySSHHostFingerprint: "SHA256:x", + MetadataKeySSHRemoteTaskDir: "/remote/task", + MetadataKeySSHRemoteSessionDir: "/remote/session", + MetadataKeySSHRemoteAgentctlPort: "41234", + MetadataKeySSHRemoteAgentctlPID: "4242", + MetadataKeySSHLocalForwardPort: strconv.Itoa(fwd.LocalPort()), + MetadataKeySSHWorkdirRoot: "/custom/workdir", + MetadataKeyIsRemote: true, + MetadataKeyReuseExistingProcess: false, + MetadataKeySSHRuntimeAPILocalURL: "http://127.0.0.1:9000", + MetadataKeySSHRuntimeAPIRemotePort: "50000", + } + for key, wantVal := range want { + if got := instance.Metadata[key]; got != wantVal { + t.Fatalf("Metadata[%q] = %v, want %v", key, got, wantVal) + } + } + if len(instance.Metadata) != len(want) { + t.Fatalf("Metadata = %+v, want exactly %d keys matching %+v", instance.Metadata, len(want), want) + } +} + +// TestSSHExecutorBuildInstanceForLostRaceReusesAResumedWinnersProcess covers +// the race shape the test above does not: a CreateInstance caller losing +// specifically to a ResumeRemoteInstance winner whose remote agent process is +// already running (agentctlClient/reusingProcess are only ever set by +// ResumeRemoteInstance). Without reusing the winner's client and reporting +// reuse_existing_process, the loser's execution would skip reconnecting to +// that process and manager_startup.go would spawn a second, conflicting one +// against the same remote agentctl instance instead. +// +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1 +func TestSSHExecutorBuildInstanceForLostRaceReusesAResumedWinnersProcess(t *testing.T) { + server := newFakeSSHServer(t, nil) + fwd, err := StartPortForward(server.dial(t), 41234, newTestLogger()) + if err != nil { + t.Fatalf("StartPortForward: %v", err) + } + t.Cleanup(func() { _ = fwd.Close() }) + resumedClient := agentctl.NewClient(sshAgentctlLoopbackHost, fwd.LocalPort(), newTestLogger(), + agentctl.WithExecutionID("previous-instance"), agentctl.WithAuthToken("resumed-token")) + + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + existing := &sshSessionState{ + target: &SSHTarget{Host: "build.example", Port: 22, User: "deploy", PinnedFingerprint: "SHA256:x"}, + forwarder: fwd, + agentctlClient: resumedClient, + pid: 4242, + remoteDir: "/remote/session", + remoteTaskDir: "/remote/task", + authToken: "resumed-token", + reusingProcess: true, + port: 41234, + workdirRoot: "/custom/workdir", + } + req := &ExecutorCreateRequest{InstanceID: "instance-1", TaskID: "task-1", SessionID: "session-1"} + + instance := exec.buildInstanceForLostRace(req, existing) + + if instance.Client != resumedClient { + t.Fatal("Client must be the resumed winner's own agentctl client, not a freshly constructed one") + } + if got := instance.Metadata[MetadataKeyReuseExistingProcess]; got != true { + t.Fatalf("Metadata[MetadataKeyReuseExistingProcess] = %v, want true — otherwise the loser's startup spawns a second agent subprocess against the winner's already-running remote process", got) + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.1 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1 +func TestSSHExecutorResumeRemoteInstanceInsertIfAbsentUnderRace(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Second, 20*time.Second) + control := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/health" { + _, _ = io.WriteString(w, "ok") + return + } + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(control.Close) + + server := newFakeSSHServer(t, func(command, _ string) sshExecResult { + if strings.Contains(command, "kill -0") { + return sshOK + } + return sshOut("/home/kandev") + }) + server.forwardTo(strings.TrimPrefix(control.URL, "http://")) + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + t.Cleanup(func() { _ = exec.Close() }) + + metadata := sshConnectionMetadata(t, server) + metadata[MetadataKeySSHRemoteAgentctlPID] = "4242" + metadata[MetadataKeySSHRemoteAgentctlPort] = "41234" + metadata[MetadataKeySSHRemoteSessionDir] = "/remote/session" + metadata[MetadataKeySSHRemoteTaskDir] = "/remote/task" + + var wg sync.WaitGroup + errs := make([]error, 2) + for i := range errs { + wg.Add(1) + go func(i int) { + defer wg.Done() + req := &ExecutorCreateRequest{ + InstanceID: "instance-1", + AuthToken: "persisted-token", + Metadata: cloneSSHMetadata(metadata), + } + errs[i] = exec.ResumeRemoteInstance(context.Background(), req) + }(i) + } + wg.Wait() + for i, err := range errs { + if err != nil { + t.Fatalf("ResumeRemoteInstance[%d]: %v", i, err) + } + } + + exec.mu.Lock() + count := len(exec.sessions) + survivor := exec.sessions["instance-1"] + exec.mu.Unlock() + if count != 1 { + t.Fatalf("tracked sessions = %d, want exactly one to survive the race", count) + } + if survivor != nil && survivor.watchdog == nil { + t.Fatal("the surviving session must have a transport-liveness watchdog started, even when reached via the race path") + } + // goleak's TestMain verifies the losing caller's dialed client and + // forwarder were actually closed rather than orphaned. +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4 +func TestSSHExecutorResetTrackedManagedBrokerResumeLosesRaceReturnsSuccess(t *testing.T) { + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + preflightCalls, stopCalls := 0, 0 + brokerPreflight := func(context.Context, *ssh.Client, *ExecutorCreateRequest, SSHRemotePlatform) error { + preflightCalls++ + return nil + } + stopRemote := func(context.Context, *ssh.Client, string, int) error { + stopCalls++ + return nil + } + // state is deliberately not tracked in exec.sessions, simulating another + // concurrent disposal having already removed it before this call's + // reading (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4's "the one that + // does not remove it finds no tracked session"). + state := &sshSessionState{client: &ssh.Client{}} + req := &ExecutorCreateRequest{InstanceID: "instance-1"} + + err := exec.resetTrackedManagedBrokerResume(context.Background(), req, state, brokerPreflight, stopRemote) + if err != nil { + t.Fatalf("resetTrackedManagedBrokerResume: %v", err) + } + if preflightCalls != 0 || stopCalls != 0 { + t.Fatalf("preflight calls = %d, stop calls = %d, want both 0 — a losing disposal must not pay for a remote round trip", + preflightCalls, stopCalls) + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.15 +func TestSSHExecutorResetTrackedManagedBrokerResumeAbandonsAWedgedPreflight(t *testing.T) { + withSSHRemoteCleanupTimeouts(t, sshCleanupTimeout, sshAgentctlCleanupTimeout, 20*time.Millisecond) + + server := newFakeSSHServer(t, nil) + client := server.dial(t) + server.setSilent(true) + + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + state := &sshSessionState{client: client, remoteDir: "/remote/session", pid: 4242} + exec.sessions["instance-1"] = state + + brokerPreflight := func(ctx context.Context, client *ssh.Client, _ *ExecutorCreateRequest, _ SSHRemotePlatform) error { + _, _, err := runSSHCommand(ctx, client, "true") + return err + } + stopCalls := 0 + stopRemote := func(context.Context, *ssh.Client, string, int) error { stopCalls++; return nil } + + req := &ExecutorCreateRequest{InstanceID: "instance-1"} + done := make(chan error, 1) + go func() { + done <- exec.resetTrackedManagedBrokerResume(context.Background(), req, state, brokerPreflight, stopRemote) + }() + select { + case err := <-done: + if !errors.Is(err, ErrSSHTransportLost) { + t.Fatalf("error = %v, want ErrSSHTransportLost", err) + } + case <-time.After(2 * time.Second): + t.Fatal("resetTrackedManagedBrokerResume did not return after its preflight wedged") + } + if stopCalls != 0 { + t.Fatalf("stop calls = %d, want 0 — the reset must not run its remote stop once the preflight abandoned the client", stopCalls) + } + if !exec.isTransportLost(state) { + t.Fatal("a preflight abandoned by its own timeout must mark the client transport-lost") + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.10 +func TestSSHExecutorStopInstanceOnAHealthyTransportDoesNotDeclareLoss(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Millisecond, 200*time.Millisecond) + + server := newFakeSSHServer(t, nil) + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + client := server.dial(t) + state := newTrackedSSHSession(exec, "instance-1", client) + + time.Sleep(20 * time.Millisecond) // let a few healthy probes complete + + if err := exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "instance-1", + StopReason: "stopped via API", + }, false); err != nil { + t.Fatalf("StopInstance: %v", err) + } + if exec.isTransportLost(state) { + t.Fatal("a deliberate stop on a healthy transport must not declare transport loss") + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8 +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.11 +func TestSSHExecutorStopInstanceSkipsRemoteCommandsWhenTransportUnresponsive(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Millisecond, 200*time.Millisecond) // unresponsive threshold 10ms, well under the 200ms deadline + + server := newFakeSSHServer(t, nil) + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + cleanupCalls, stopCalls := 0, 0 + exec.cleanupScript = func(context.Context, *ssh.Client, string, map[string]interface{}, map[string]string, SSHRemotePlatform, string, string) error { + cleanupCalls++ + return nil + } + exec.stopRemote = func(context.Context, *ssh.Client, string, int) error { + stopCalls++ + return nil + } + client := server.dial(t) + newTrackedSSHSession(exec, "instance-1", client) + + server.setSilent(true) + time.Sleep(30 * time.Millisecond) // let silence exceed the 10ms unresponsive threshold, well under the 200ms deadline + + done := make(chan error, 1) + go func() { + done <- exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "instance-1", + StopReason: StopReasonTaskDeleted, // triggers the cleanup script + }, false) + }() + select { + case err := <-done: + if err != nil { + t.Fatalf("StopInstance: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("StopInstance did not return while the transport was unresponsive — a skip must have been missed") + } + if cleanupCalls != 0 || stopCalls != 0 { + t.Fatalf("cleanup calls = %d, stop calls = %d, want both skipped on an unresponsive transport", cleanupCalls, stopCalls) + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8 +func TestSSHExecutorStopInstanceAbandonsARemoteCommandThatWedgesAfterTheReading(t *testing.T) { + withSSHKeepaliveTuning(t, 0, 0) // no watchdog: exercises the backstop alone, not the reading-based skip + withSSHRemoteCleanupTimeouts(t, 20*time.Millisecond, 20*time.Millisecond, sshBrokerPreflightTimeout) + + server := newFakeSSHServer(t, nil) + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + client := server.dial(t) + server.setSilent(true) // the transport wedges; with no watchdog, the reading alone can't know that (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.8's last sentence) + + state := &sshSessionState{client: client, remoteDir: "/remote/session", remoteTaskDir: "/remote/task", pid: 4242} + exec.sessions["instance-1"] = state + exec.cleanupScript = func(ctx context.Context, client *ssh.Client, _ string, _ map[string]interface{}, _ map[string]string, _ SSHRemotePlatform, _ string, _ string) error { + _, _, err := runSSHCommand(ctx, client, "true") + return err + } + exec.stopRemote = func(ctx context.Context, client *ssh.Client, _ string, _ int) error { + _, _, err := runSSHCommand(ctx, client, "true") + return err + } + + done := make(chan error, 1) + go func() { + done <- exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "instance-1", + StopReason: StopReasonTaskDeleted, + }, false) + }() + select { + case err := <-done: + if err != nil { + t.Fatalf("StopInstance: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("StopInstance did not return after its remote command wedged — the backstop must have been missed") + } + if !exec.isTransportLost(state) { + t.Fatal("a command abandoned by its own timeout must mark the client transport-lost") + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.13 +func TestSSHExecutorCloseDoesNotWaitForTheNextProbe(t *testing.T) { + withSSHKeepaliveTuning(t, 500*time.Millisecond, 5*time.Second) + + server := newFakeSSHServer(t, nil) + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + client := server.dial(t) + newTrackedSSHSession(exec, "instance-1", client) + + time.Sleep(50 * time.Millisecond) // let the first probe complete + + start := time.Now() + if err := exec.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if elapsed := time.Since(start); elapsed >= 500*time.Millisecond { + t.Fatalf("Close took %s, want well under one probe interval (500ms)", elapsed) + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7 +func TestSSHExecutorGetRemoteStatusReportsTransportLoss(t *testing.T) { + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + exec.sessions["i"] = &sshSessionState{ + target: &SSHTarget{Host: "build.example"}, + pid: 4242, + transportLost: true, + } + status, err := exec.GetRemoteStatus(context.Background(), &ExecutorInstance{InstanceID: "i"}) + if err != nil { + t.Fatalf("GetRemoteStatus: %v", err) + } + if status.State != sshStatusDisconnected { + t.Fatalf("State = %q, want %q", status.State, sshStatusDisconnected) + } + if !strings.Contains(status.ErrorMessage, "transport lost") { + t.Fatalf("ErrorMessage = %q, want it to identify transport loss", status.ErrorMessage) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_lifecycle_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_lifecycle_test.go index 8f0cb19c35..ee6633e67e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_lifecycle_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_lifecycle_test.go @@ -14,6 +14,7 @@ import ( "strings" "sync" "testing" + "time" "golang.org/x/crypto/ssh" @@ -322,6 +323,7 @@ func (h *sshLaunchHarness) createInstanceBodies() []string { } func TestSSHExecutorCreateInstanceProvisionsAndTracksTheSession(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Second, 20*time.Second) harness := newSSHLaunchHarness(t, "4242") exec := NewSSHExecutor(nil, nil, NewAgentctlResolver(newTestLogger()), newTestLogger()) t.Cleanup(func() { _ = exec.Close() }) @@ -432,6 +434,9 @@ func TestSSHExecutorCreateInstanceProvisionsAndTracksTheSession(t *testing.T) { if state.platform.GOOS != "linux" || state.platform.GOARCH != "amd64" { t.Fatalf("session platform = %+v", state.platform) } + if state.watchdog == nil { + t.Fatal("CreateInstance must start a transport-liveness watchdog for the session (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1)") + } // StopInstance releases the forwarder and SSH client and forgets the state. if err := exec.StopInstance(context.Background(), instance, true); err != nil { diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_scripts.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_scripts.go index ca76dbeefd..46783c31de 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_scripts.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_scripts.go @@ -15,10 +15,12 @@ import ( "github.com/kandev/kandev/internal/scriptengine" ) -const ( - sshCleanupTimeout = 60 * time.Second - sshPrepareOutputMaxLines = 20 -) +const sshPrepareOutputMaxLines = 20 + +// sshCleanupTimeout bounds the remote cleanup script a session stop issues. +// A package-level variable, not a constant, so a backstop test need not wait +// out real seconds (see executor_ssh_keepalive.go). +var sshCleanupTimeout = 60 * time.Second func cloneSSHMetadata(metadata map[string]interface{}) map[string]interface{} { if len(metadata) == 0 { diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go index 290d5ebdba..7cc1ccd493 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" "testing" + "time" "golang.org/x/crypto/ssh" @@ -90,6 +91,44 @@ func TestSSHExecutorGetRemoteStatus(t *testing.T) { t.Fatalf("State = %q, want %q", status.State, sshStatusAgentctlDown) } }) + + t.Run("transport lost during the probe reports disconnected, not agentctl-down", func(t *testing.T) { + // AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.7: GetRemoteStatus reads the + // transport-lost marker and the client handle in one critical section, + // then releases the mutex before probing. A teardown racing in after + // that read (marker still false, client still open at read time) but + // before the probe completes must still surface as `disconnected`, + // not `agentctl-down` — a failed probe alone can't tell the two apart. + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + var state *sshSessionState + server := newFakeSSHServer(t, func(string, string) sshExecResult { + // Simulate a concurrent watchdog-driven teardown landing exactly + // between GetRemoteStatus's marker read and this probe: flip the + // marker under the same mutex the real teardown uses, then fail + // the probe the way a torn-down client would. + exec.mu.Lock() + state.transportLost = true + exec.mu.Unlock() + return sshFail("no such process") + }) + state = &sshSessionState{ + target: &SSHTarget{Host: "build.example"}, + client: server.dial(t), + pid: 4242, + } + exec.sessions["i"] = state + + status, err := exec.GetRemoteStatus(context.Background(), &ExecutorInstance{InstanceID: "i"}) + if err != nil { + t.Fatalf("GetRemoteStatus: %v", err) + } + if status.State != sshStatusDisconnected { + t.Fatalf("State = %q, want %q", status.State, sshStatusDisconnected) + } + if status.ErrorMessage != sshTransportLostMessage { + t.Fatalf("ErrorMessage = %q", status.ErrorMessage) + } + }) } func TestSSHShouldStopRemoteAgentctl(t *testing.T) { @@ -219,6 +258,7 @@ func TestSSHExecutorResumeRemoteInstance(t *testing.T) { }) t.Run("re-attaches to a live remote agentctl", func(t *testing.T) { + withSSHKeepaliveTuning(t, 5*time.Second, 20*time.Second) harness := newSSHLaunchHarness(t, "4242") exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) t.Cleanup(func() { _ = exec.Close() }) @@ -255,6 +295,9 @@ func TestSSHExecutorResumeRemoteInstance(t *testing.T) { t.Fatalf("metadata forward port = %v, want the new local port %d", forwardPort, state.forwarder.LocalPort()) } + if state.watchdog == nil { + t.Fatal("ResumeRemoteInstance must start a transport-liveness watchdog for the session (AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.1)") + } }) // @covers AC-EXECUTORS-SSH-EXECUTOR-001.9 diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_transport_teardown_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_transport_teardown_test.go new file mode 100644 index 0000000000..d5eeca91b4 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_transport_teardown_test.go @@ -0,0 +1,169 @@ +package lifecycle + +import ( + "context" + "io" + "net" + "sync" + "testing" + "time" + + "golang.org/x/crypto/ssh" +) + +type clientReleaseListener struct { + clientClosed <-chan struct{} + closed chan struct{} + once sync.Once +} + +func (l *clientReleaseListener) Accept() (net.Conn, error) { + <-l.closed + return nil, net.ErrClosed +} + +func (l *clientReleaseListener) Close() error { + <-l.clientClosed + l.once.Do(func() { close(l.closed) }) + return nil +} + +func (l *clientReleaseListener) Addr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)} +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-002.4 +func TestSSHExecutorStopInstanceClosesClientBeforeRuntimeAPITunnel(t *testing.T) { + clientClosed := make(chan struct{}) + listenerClosed := make(chan struct{}) + listener := &clientReleaseListener{clientClosed: clientClosed, closed: listenerClosed} + + exec := NewSSHExecutor(nil, nil, nil, newTestLogger()) + exec.stopRemote = func(context.Context, *ssh.Client, string, int) error { return nil } + exec.closeClient = func(*ssh.Client) error { + close(clientClosed) + return nil + } + exec.sessions["instance-1"] = &sshSessionState{ + client: &ssh.Client{}, + runtimeAPITunnel: &sshRuntimeAPITunnel{listener: listener}, + remoteDir: "/remote/session", + pid: 4242, + } + + done := make(chan error, 1) + go func() { + done <- exec.StopInstance(context.Background(), &ExecutorInstance{ + InstanceID: "instance-1", + StopReason: StopReasonTaskDeleted, + }, false) + }() + select { + case err := <-done: + if err != nil { + t.Fatalf("StopInstance: %v", err) + } + case <-time.After(time.Second): + t.Fatal("StopInstance waited for the runtime API tunnel before closing the SSH client") + } + select { + case <-listenerClosed: + case <-time.After(time.Second): + t.Fatal("runtime API tunnel did not close") + } +} + +// @covers AC-EXECUTORS-SSH-TRANSPORT-LIVENESS-001.5 +func TestSSHKeepaliveTeardownClosesRuntimeAPITunnelAfterClient(t *testing.T) { + exec, _ := newObservedSSHExecutor(t) + server := newFakeSSHServer(t, nil) + client := server.dial(t) + + clientClosed := make(chan struct{}) + listenerClosed := make(chan struct{}) + state := &sshSessionState{ + target: &SSHTarget{Host: "build.example", User: "deploy", PinnedFingerprint: "SHA256:x"}, + client: client, + runtimeAPITunnel: &sshRuntimeAPITunnel{listener: &clientReleaseListener{ + clientClosed: clientClosed, + closed: listenerClosed, + }}, + } + exec.closeClient = func(c *ssh.Client) error { + close(clientClosed) + return c.Close() + } + + exec.transportTeardown("instance-1", state, sshTransportLostReasonDeadline, time.Second) + select { + case <-listenerClosed: + case <-time.After(time.Second): + t.Fatal("transport teardown did not close the runtime API tunnel") + } +} + +type orderedCloseConn struct { + bastionClosed <-chan struct{} + closed chan struct{} + once sync.Once +} + +func (c *orderedCloseConn) Read([]byte) (int, error) { return 0, io.EOF } + +func (c *orderedCloseConn) Write(p []byte) (int, error) { return len(p), nil } + +func (c *orderedCloseConn) Close() error { + <-c.bastionClosed + c.once.Do(func() { close(c.closed) }) + return nil +} + +func (c *orderedCloseConn) LocalAddr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)} +} + +func (c *orderedCloseConn) RemoteAddr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)} +} + +func (c *orderedCloseConn) SetDeadline(time.Time) error { return nil } +func (c *orderedCloseConn) SetReadDeadline(time.Time) error { return nil } +func (c *orderedCloseConn) SetWriteDeadline(time.Time) error { return nil } + +type closeSignal struct { + closed chan struct{} + once sync.Once +} + +func (s *closeSignal) Close() error { + s.once.Do(func() { close(s.closed) }) + return nil +} + +func TestSSHProxyJumpConnClosesBastionBeforeTunnel(t *testing.T) { + bastionClosed := make(chan struct{}) + tunnelClosed := make(chan struct{}) + conn := &sshProxyJumpConn{ + Conn: &orderedCloseConn{bastionClosed: bastionClosed, closed: tunnelClosed}, + bastion: &closeSignal{closed: bastionClosed}, + } + + done := make(chan error, 1) + go func() { done <- conn.Close() }() + select { + case err := <-done: + if err != nil { + t.Fatalf("sshProxyJumpConn.Close: %v", err) + } + case <-time.After(time.Second): + t.Fatal("sshProxyJumpConn.Close waited for the tunnel before closing the bastion") + } + select { + case <-tunnelClosed: + case <-time.After(time.Second): + t.Fatal("sshProxyJumpConn did not close the tunnel") + } + if err := conn.Close(); err != nil { + t.Fatalf("second sshProxyJumpConn.Close: %v", err) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/goleak_test.go b/apps/backend/internal/agent/runtime/lifecycle/goleak_test.go index b97179a67b..32ee90e92f 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/goleak_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/goleak_test.go @@ -16,6 +16,15 @@ import ( // only when `OTEL_EXPORTER_OTLP_ENDPOINT` is set — in tests it isn't, so // the noop provider is used and no batchSpanProcessor / OTLP retry // goroutines are spawned. No IgnoreTopFunction is needed here. +// +// sshKeepaliveInterval/sshKeepaliveDeadline are zeroed for this test binary +// so the fifteen CreateInstance/ResumeRemoteInstance call sites across the +// package don't each acquire a real 15s SSH transport watchdog under this +// leak check. A non-positive value disables the watchdog +// (sshKeepaliveTuningValid); tests that exercise it set millisecond values +// of their own and must not run with t.Parallel(). func TestMain(m *testing.M) { + sshKeepaliveInterval = 0 + sshKeepaliveDeadline = 0 goleak.VerifyTestMain(m) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go b/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go index 4b5ac0d1c7..31da4efc06 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_startup.go @@ -154,7 +154,7 @@ func (m *Manager) startAgentProcess(ctx context.Context, executionID string) (re // Check if we're reconnecting to an existing running agent process. // When the existing process is still alive inside a remote executor (e.g., Sprites), // we skip subprocess launch and go directly to ACP session initialization. - reuseExisting := execution.metadataBool("reuse_existing_process") + reuseExisting := execution.metadataBool(MetadataKeyReuseExistingProcess) if !reuseExisting && execution.AgentCommand == "" { return fmt.Errorf("execution %q has no agent command configured", executionID) diff --git a/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go b/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go index ab3ac67c08..c4f73b2d6a 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go @@ -9,6 +9,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "testing" "github.com/pkg/sftp" @@ -66,8 +67,22 @@ type fakeSSHServer struct { // tcpTarget maps a requested direct-tcpip port onto a real local address. // Returning "" rejects the channel. tcpTarget func(port uint32) string + + // silent, when set, makes the server accept a global request or a new + // channel open and answer neither — so SendRequest and Client.NewSession + // both block, as on a wedged link. Toggled live so a test can run a + // session healthy, then flip the server silent mid-test. + silent atomic.Bool } +// setSilent switches the server's silent mode on or off. While silent, every +// global request (including keepalive@openssh.com) and every new channel +// open (including a session for a remote command) goes unanswered, so the +// requester's SendRequest or Client.NewSession blocks exactly as it would on +// a transport that has stopped carrying traffic. Existing, already-open +// channels are unaffected. +func (s *fakeSSHServer) setSilent(v bool) { s.silent.Store(v) } + // newFakeSSHServer starts a listener on 127.0.0.1 and serves SSH until the // test finishes. handler may be nil, in which case every command succeeds with // empty output. @@ -200,6 +215,13 @@ func (s *fakeSSHServer) serveConn(conn net.Conn) { go s.serveGlobalRequests(serverConn, reqs) for newChan := range chans { + if s.silent.Load() { + // Stall the channel open: neither Accept nor Reject, so the + // client's OpenChannel blocks until the connection itself tears + // down. No goroutine needed — the pending request resolves on + // its own when the mux beneath it closes. + continue + } switch newChan.ChannelType() { case "session": s.wg.Add(1) @@ -213,9 +235,16 @@ func (s *fakeSSHServer) serveConn(conn net.Conn) { } } +// serveGlobalRequests answers every global request (replying false to one +// that wants a reply, mirroring ssh.DiscardRequests) unless the server is +// silent, in which case it drains the request without replying — so a +// wantReply sender's SendRequest blocks until the connection tears down. func (s *fakeSSHServer) serveGlobalRequests(conn *ssh.ServerConn, requests <-chan *ssh.Request) { defer s.wg.Done() for req := range requests { + if s.silent.Load() { + continue + } switch req.Type { case "tcpip-forward": s.handleTCPIPForward(conn, req) diff --git a/docs/plans/ssh-transport-liveness/plan.md b/docs/plans/ssh-transport-liveness/plan.md new file mode 100644 index 0000000000..5cf495a0f2 --- /dev/null +++ b/docs/plans/ssh-transport-liveness/plan.md @@ -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) diff --git a/docs/plans/ssh-transport-liveness/task-01-ssh-transport-watchdog.md b/docs/plans/ssh-transport-liveness/task-01-ssh-transport-watchdog.md new file mode 100644 index 0000000000..7150b8f8b4 --- /dev/null +++ b/docs/plans/ssh-transport-liveness/task-01-ssh-transport-watchdog.md @@ -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. diff --git a/docs/specs/executors/README.md b/docs/specs/executors/README.md index 93f1631f27..b587c12cbe 100644 --- a/docs/specs/executors/README.md +++ b/docs/specs/executors/README.md @@ -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 diff --git a/docs/specs/executors/requirements/ssh-transport-liveness.md b/docs/specs/executors/requirements/ssh-transport-liveness.md new file mode 100644 index 0000000000..4380e525aa --- /dev/null +++ b/docs/specs/executors/requirements/ssh-transport-liveness.md @@ -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). diff --git a/docs/specs/executors/system-design/ssh-transport-liveness.md b/docs/specs/executors/system-design/ssh-transport-liveness.md new file mode 100644 index 0000000000..bb9535b2e2 --- /dev/null +++ b/docs/specs/executors/system-design/ssh-transport-liveness.md @@ -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.