Skip to content

Commit 2a70fd4

Browse files
authored
Improve frontend-backend connection resiliency (#86)
* Improve frontend backend reconnect handling * Reconnect after missed stream heartbeats * Harden frontend stream failure handling * Harden backend connection lifecycle * Harden socket reconnect edge cases * Fix stale frontend connection races * Add server websocket liveness deadlines * Harden frontend socket cleanup paths * Fix socket cleanup review findings * Address AI review connection feedback * Finalize already-closing streams during clear Avoid calling close a second time when Streams.clear sees a stream that is already shutting down. This lets the existing shutdown path finalize with completed() instead of treating the normal already-closing state as a cleanup failure. Add a regression test that starts local stream shutdown before clearing the transport and verifies the command is completed without reporting a clear error. * Complete streams after clear close failures During final stream teardown, preserve command close errors for the cleared callback but still run stream completion callbacks. This releases command subscribers and clears stream slots even when the transport is already broken. Update the stream clear regression test to assert both error propagation and terminal completion after a command close failure. * Clear stale socket handler after callback failure If the connected callback throws after a stream handler is assigned, clear the cached handler and close the dialed connection before reporting failure. This prevents later get() calls from reusing a non-serving handler. Add a socket regression test that throws from connected(), verifies cleanup, and confirms the next get() opens a fresh stream.
1 parent bf9495c commit 2a70fd4

28 files changed

Lines changed: 2707 additions & 160 deletions

application/command/handler.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import (
2121
// length exceeds the read buffer.
2222
// ErrHandlerInvalidControlMessage is returned when a control frame carries zero
2323
// bytes of payload.
24+
// ErrHandlerUnknownControlSignal is returned when a control frame carries an
25+
// unsupported control signal byte.
2426
var (
2527
ErrHandlerUnknownHeaderType = errors.New(
2628
"unknown command header type")
@@ -30,6 +32,9 @@ var (
3032

3133
ErrHandlerInvalidControlMessage = errors.New(
3234
"invalid control message")
35+
36+
ErrHandlerUnknownControlSignal = errors.New(
37+
"unknown control signal")
3338
)
3439

3540
// HandlerCancelSignal is a channel that, when closed or written to, signals
@@ -247,6 +252,9 @@ func (e *Handler) handleControl(d byte, l log.Logger) error {
247252
} else {
248253
l.Debug("Repeated Resume Stream command, ignore")
249254
}
255+
256+
default:
257+
return ErrHandlerUnknownControlSignal
250258
}
251259

252260
return nil

application/command/handler_echo_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package command
66

77
import (
88
"bytes"
9+
"errors"
910
"io"
1011
"sync"
1112
"testing"
@@ -93,3 +94,33 @@ func TestHandlerHandleEcho(t *testing.T) {
9394
return
9495
}
9596
}
97+
98+
func TestHandlerRejectsUnknownControlSignal(t *testing.T) {
99+
w := dummyWriter{
100+
written: make([]byte, 0, 64),
101+
}
102+
s := []byte{
103+
byte(HeaderControl | 1),
104+
0xff,
105+
}
106+
lock := sync.Mutex{}
107+
bufferPool := NewBufferPool(4096)
108+
handler := newHandler(
109+
Configuration{},
110+
nil,
111+
rw.NewFetchReader(testDummyFetchGen(s)),
112+
&w,
113+
&lock,
114+
0,
115+
0,
116+
log.NewDitch(),
117+
NewHooks(configuration.HookSettings{}),
118+
&bufferPool,
119+
)
120+
121+
hErr := handler.Handle()
122+
123+
if !errors.Is(hErr, ErrHandlerUnknownControlSignal) {
124+
t.Fatalf("expected unknown control signal error, got %v", hErr)
125+
}
126+
}

application/commands/ssh.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -801,6 +801,7 @@ func (d *sshClient) local(
801801
if wErr != nil {
802802
remote.closer()
803803
d.l.Debug("Failed to write data to remote: %s", wErr)
804+
return wErr
804805
}
805806
}
806807

@@ -910,12 +911,13 @@ func (d *sshClient) Close() error {
910911
d.fingerprintVerifyResultReceiveClosed = true
911912
}
912913

914+
d.baseCtxCancel()
915+
913916
remote, remoteErr := d.getRemote()
914917
if remoteErr == nil {
915918
remote.closer()
916919
}
917920

918-
d.baseCtxCancel()
919921
d.remoteCloseWait.Wait()
920922

921923
return nil

application/commands/ssh_lifecycle_test.go

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@
55
package commands
66

77
import (
8+
"context"
9+
"errors"
810
"testing"
11+
"time"
912

1013
"github.com/Snuffy2/shellport/application/command"
1114
"github.com/Snuffy2/shellport/application/configuration"
1215
"github.com/Snuffy2/shellport/application/log"
16+
"golang.org/x/crypto/ssh"
1317
)
1418

1519
// TestSSHCommandKeepsBufferPoolScopedToSession verifies that SSH clients retain
@@ -38,3 +42,77 @@ func TestSSHCommandKeepsBufferPoolScopedToSession(t *testing.T) {
3842
)
3943
}
4044
}
45+
46+
// TestSSHCloseCancelsBeforeWaitingForRemote verifies Close can unblock remote
47+
// startup paths that only exit after the base context is cancelled.
48+
func TestSSHCloseCancelsBeforeWaitingForRemote(t *testing.T) {
49+
ctx, cancel := context.WithCancel(context.Background())
50+
client := &sshClient{
51+
baseCtx: ctx,
52+
baseCtxCancel: cancel,
53+
credentialReceive: make(chan []byte),
54+
fingerprintVerifyResultReceive: make(chan bool),
55+
remoteConnReceive: make(chan sshRemoteConn),
56+
credentialReceiveClosed: false,
57+
fingerprintVerifyResultReceiveClosed: false,
58+
}
59+
client.remoteCloseWait.Add(1)
60+
61+
go func() {
62+
<-ctx.Done()
63+
close(client.remoteConnReceive)
64+
client.remoteCloseWait.Done()
65+
}()
66+
67+
done := make(chan struct{})
68+
go func() {
69+
_ = client.Close()
70+
close(done)
71+
}()
72+
73+
select {
74+
case <-ctx.Done():
75+
case <-time.After(100 * time.Millisecond):
76+
t.Fatal("Close did not cancel base context before waiting for remote")
77+
}
78+
79+
select {
80+
case <-done:
81+
case <-time.After(100 * time.Millisecond):
82+
t.Fatal("Close did not return after remote shutdown")
83+
}
84+
}
85+
86+
type failingSSHWriter struct {
87+
err error
88+
}
89+
90+
func (w failingSSHWriter) Write(_ []byte) (int, error) {
91+
return 0, w.err
92+
}
93+
94+
// TestSSHLocalReturnsRemoteWriteErrors verifies stdin write failures surface to
95+
// the stream handler instead of leaving the UI in a misleading connected state.
96+
func TestSSHLocalReturnsRemoteWriteErrors(t *testing.T) {
97+
writeErr := errors.New("remote write failed")
98+
closed := false
99+
client := &sshClient{
100+
l: log.NewDitch(),
101+
remoteConn: sshRemoteConn{
102+
writer: failingSSHWriter{err: writeErr},
103+
closer: func() error { closed = true; return nil },
104+
session: &ssh.Session{},
105+
},
106+
}
107+
header := command.StreamHeader{}
108+
header.Set(SSHClientStdIn, 5)
109+
110+
err := client.local(nil, newLimitedReader([]byte("hello")), header, make([]byte, 16))
111+
112+
if !errors.Is(err, writeErr) {
113+
t.Fatalf("expected remote write error, got %v", err)
114+
}
115+
if !closed {
116+
t.Fatal("expected remote closer to run after write failure")
117+
}
118+
}

application/commands/telnet.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,19 +267,21 @@ func (d *telnetClient) client(
267267
if wErr != nil {
268268
remoteConn.Close()
269269
d.l.Debug("Failed to write data to remote: %s", wErr)
270+
return wErr
270271
}
271272
}
272273

273274
return nil
274275
}
275276

276277
func (d *telnetClient) Close() error {
278+
d.baseCtxCancel()
279+
277280
remoteConn, remoteConnErr := d.getRemote()
278281
if remoteConnErr == nil {
279282
remoteConn.Close()
280283
}
281284

282-
d.baseCtxCancel()
283285
d.closeWait.Wait()
284286
return nil
285287
}

application/commands/telnet_lifecycle_test.go

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,13 @@
55
package commands
66

77
import (
8+
"context"
9+
"errors"
10+
"io"
11+
"net"
812
"reflect"
913
"testing"
14+
"time"
1015

1116
"github.com/Snuffy2/shellport/application/command"
1217
"github.com/Snuffy2/shellport/application/configuration"
@@ -40,3 +45,100 @@ func TestTelnetCommandKeepsBufferPoolScopedToSession(t *testing.T) {
4045
)
4146
}
4247
}
48+
49+
// TestTelnetCloseCancelsBeforeWaitingForRemote verifies Close can unblock
50+
// remote startup paths that only exit after the base context is cancelled.
51+
func TestTelnetCloseCancelsBeforeWaitingForRemote(t *testing.T) {
52+
ctx, cancel := context.WithCancel(context.Background())
53+
client := &telnetClient{
54+
baseCtx: ctx,
55+
baseCtxCancel: cancel,
56+
remoteChan: make(chan net.Conn),
57+
}
58+
client.closeWait.Add(1)
59+
60+
go func() {
61+
<-ctx.Done()
62+
close(client.remoteChan)
63+
client.closeWait.Done()
64+
}()
65+
66+
done := make(chan struct{})
67+
go func() {
68+
_ = client.Close()
69+
close(done)
70+
}()
71+
72+
select {
73+
case <-ctx.Done():
74+
case <-time.After(100 * time.Millisecond):
75+
t.Fatal("Close did not cancel base context before waiting for remote")
76+
}
77+
78+
select {
79+
case <-done:
80+
case <-time.After(100 * time.Millisecond):
81+
t.Fatal("Close did not return after remote shutdown")
82+
}
83+
}
84+
85+
type failingTelnetConn struct {
86+
net.Conn
87+
writeErr error
88+
closed bool
89+
}
90+
91+
func (c *failingTelnetConn) Read(_ []byte) (int, error) {
92+
return 0, io.EOF
93+
}
94+
95+
func (c *failingTelnetConn) Write(_ []byte) (int, error) {
96+
return 0, c.writeErr
97+
}
98+
99+
func (c *failingTelnetConn) Close() error {
100+
c.closed = true
101+
return nil
102+
}
103+
104+
func (c *failingTelnetConn) LocalAddr() net.Addr {
105+
return nil
106+
}
107+
108+
func (c *failingTelnetConn) RemoteAddr() net.Addr {
109+
return nil
110+
}
111+
112+
func (c *failingTelnetConn) SetDeadline(_ time.Time) error {
113+
return nil
114+
}
115+
116+
func (c *failingTelnetConn) SetReadDeadline(_ time.Time) error {
117+
return nil
118+
}
119+
120+
func (c *failingTelnetConn) SetWriteDeadline(_ time.Time) error {
121+
return nil
122+
}
123+
124+
// TestTelnetClientReturnsRemoteWriteErrors verifies stdin write failures
125+
// surface to the stream handler instead of leaving the UI connected to dead IO.
126+
func TestTelnetClientReturnsRemoteWriteErrors(t *testing.T) {
127+
writeErr := errors.New("remote write failed")
128+
remote := &failingTelnetConn{writeErr: writeErr}
129+
client := &telnetClient{l: log.NewDitch(), remoteConn: remote}
130+
131+
err := client.client(
132+
nil,
133+
newLimitedReader([]byte("hello")),
134+
command.StreamHeader{},
135+
make([]byte, 16),
136+
)
137+
138+
if !errors.Is(err, writeErr) {
139+
t.Fatalf("expected remote write error, got %v", err)
140+
}
141+
if !remote.closed {
142+
t.Fatal("expected remote connection to close after write failure")
143+
}
144+
}

0 commit comments

Comments
 (0)