[BUG] transport review findings: shutdown deadlock, nil-listener panic, websocket leak, and reconnect lifecycle issues

[AlexStocks]

Summary

I reviewed the current master branch of AlexStocks/getty locally and found several source-grounded lifecycle / concurrency / stability issues in transport/ that look worth fixing.

The findings below are intentionally limited to issues with concrete code paths and clear impact, rather than broad speculation.

---

1. Client shutdown can deadlock because client.stop() closes sessions while holding c.Lock(), and session close can synchronously re-enter client reconnect logic

Files:

• transport/client.go
• transport/session.go

Relevant code path

client.stop():

c.Lock()
for s := range c.ssMap {
    s.RemoveAttribute(sessionClientKey)
    s.RemoveAttribute(ignoreReconnectKey)
    s.Close()
}
c.Unlock()

session.stop():

clt, cltFound := s.GetAttribute(sessionClientKey).(*client)
ignoreReconnect, flagFound := s.GetAttribute(ignoreReconnectKey).(bool)
if cltFound && flagFound && !ignoreReconnect {
    clt.reConnect()
}

reConnect() calls methods that need the same client lock again (for example through sessionNum() / connect()).

Why this matters

This creates a shutdown-time lock re-entry path:

• client shutdown holds c.Lock()
• session close runs synchronously under that lock
• session close can call back into clt.reConnect()
• reconnect path needs the same client lock again

That is a real deadlock risk during client shutdown.

Suggested fix

• never call s.Close() while holding the client map lock
• first snapshot sessions under lock, then release the lock, then close them
• more generally, avoid synchronous reconnect work inside the session teardown path

---

2. Session teardown performs reconnect synchronously inside session.stop(), which makes close/shutdown paths unexpectedly heavy and fragile

File: transport/session.go

Relevant code path

if cltFound && flagFound && !ignoreReconnect {
    clt.reConnect()
}

reConnect() itself can:

• loop,
• dial,
• sleep/backoff,
• and inspect/update client session state.

Why this matters

A session close path is expected to be teardown-only, but here it can synchronously perform reconnection logic.

That means:

• Close() is no longer a small/local operation,
• teardown can block inside reconnect backoff/dial logic,
• and shutdown ordering becomes harder to reason about.

This also amplifies the deadlock problem above.

Suggested fix

• decouple reconnect scheduling from session.stop()
• move reconnect decisions to a dedicated client lifecycle controller / background worker
• make teardown publish state and return, rather than performing reconnection inline

---

3. Deferred error path in handlePackage() can panic because of an incorrect nil check

File: transport/session.go

Relevant code path

if err != nil {
    ...
    if s != nil || s.listener != nil {
        s.listener.OnError(s, err)
    }
}

The guard uses || instead of &&.

Why this matters

If s != nil but s.listener == nil, the condition still passes and s.listener.OnError(...) dereferences a nil listener.

That means a normal I/O / decode / shutdown error in the deferred cleanup path can escalate into a panic.

Suggested fix

• replace the guard with if s != nil && s.listener != nil { ... }
• add a regression test for the error-cleanup path with a nil listener

---

4. WebSocket self-connect rejection leaks the upgraded connection

File: transport/server.go

Relevant code path

conn, err := s.upgrader.Upgrade(w, r, nil)
...
if conn.RemoteAddr().String() == conn.LocalAddr().String() {
    log.Warnf(...)
    return
}

After the websocket upgrade succeeds, the self-connect branch returns directly without closing conn.

Why this matters

At that point the connection has already been upgraded and allocated. Returning without closing it leaks the socket/resource for that request path.

Suggested fix

• explicitly close conn before returning in the self-connect rejection branch
• add a test or at least a small harness covering this branch if possible

---

5. WSS event loop panics on normal server shutdown/error return paths

File: transport/server.go

Relevant code path

err = server.Serve(tls.NewListener(s.streamListener, config))
if err != nil {
    log.Errorf(...)
    panic(err)
}

The non-TLS WS loop logs serve errors and returns, but the WSS loop panics on any non-nil Serve(...) result.

Why this matters

For long-running servers, Serve(...) returning is not always an exceptional condition. During normal shutdown, listener closure often causes Serve to return.

Treating that path as unconditional panic makes graceful shutdown brittle and can convert expected shutdown into process crash.

Suggested fix

• distinguish expected shutdown errors from unexpected serve failures
• handle normal listener/server close without panic
• keep panic only for genuinely impossible internal invariants, if any

---

6. UDP receive buffer sizing contains a dead branch / likely logic bug

File: transport/session.go

Relevant code path

maxBufLen = int(s.maxMsgLen + maxReadBufLen)
if int(s.maxMsgLen<<1) < bufLen {
    maxBufLen = int(s.maxMsgLen << 1)
}

At this point bufLen has not yet been populated from conn.recv(buf) and is still zero.

Why this matters

The condition is effectively dead on entry and does not do what it appears to intend.

This looks like a real logic bug in UDP buffer sizing, and it likely means the branch never contributes to runtime behavior.

Suggested fix

• re-check the intended sizing rule
• if the intent was to cap by 2 * maxMsgLen, compare against the right value instead of uninitialized bufLen
• add a focused test around UDP buffer sizing / message length guard behavior

---

7. Reconnect attempt accounting appears monotonic across successes, which may permanently disable reconnect after enough intermittent failures

File: transport/client.go

Relevant code path

var reconnectAttempts int
...
for {
    ...
    if connPoolSize <= sessionNum || maxReconnectAttempts < reconnectAttempts {
        break
    }
    c.connect()
    reconnectAttempts++
    ...
}

reconnectAttempts increases each loop, but I did not see it reset after a successful reconnect.

Why this matters

If intermittent disconnect/reconnect cycles accumulate over process lifetime, the client may eventually stop reconnecting entirely once it crosses maxReconnectAttempts, even if previous reconnects actually succeeded.

Suggested fix

• make retry accounting track consecutive reconnect failures rather than total lifetime attempts in the loop
• reset the counter when pool health is restored / a reconnect succeeds
• add a regression test for repeated disconnect-success-disconnect cycles

---

Notes

I did not try to turn every suspicious pattern into an issue here. The items above are the ones that looked most actionable and had the clearest code evidence.

If helpful, I can also add a follow-up comment grouping these into:

• directly actionable fixes,
• and items that should first get a reproducer / stress test.