Repository navigation
fix: address code review bugs across client and server - #28
Conversation
- Fix drip start hanging when some tunnels fail to connect - Pass RemoteIP on TCP/TLS listener path for per-IP limits - Protect primary session from heartbeat removal; fix heap pool leaks - Roll back registration rate limits on failed registrations - Require authentication for data connections; improve stale group cleanup - Fix HTTP handoff connection metric leak and port allocator TOCTOU - Track tunnel activity for CleanupStale correctness - Fix pool client Close race, maxSessions enforcement, and primary data race - Close WebSocket response bodies; widen SSE disconnect handling - Validate config before save; wait for daemon exit on Unix - Pass daemon token via DRIP_TOKEN env instead of argv - Return errors instead of os.Exit in tunnel runner Co-authored-by: Gouryella <Gouryella@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262f5113f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if group.PrimaryConn != nil { | ||
| group.PrimaryConn.Close() | ||
| continue |
There was a problem hiding this comment.
When cleanupStaleGroups deletes the stale group from m.groups before this branch, group.PrimaryConn.Close() can no longer rely on the lifecycle manager's RemoveGroup call to close the group, because RemoveGroup will find no map entry and return without calling group.Close(). For stale pooled tunnels that have a primary connection, this leaves the group's stopCh open and can leak the heartbeat goroutine and any data sessions after the tunnel has been removed; close the group explicitly as well, or defer deleting it until the lifecycle cleanup runs.
Useful? React with 👍 / 👎.
When cleanupStaleGroups removed groups from the map before calling PrimaryConn.Close(), lifecycle RemoveGroup could not find the entry and skipped group.Close(), leaking heartbeat and data sessions. Keep groups in the map until cleanup completes, explicitly close the group after PrimaryConn.Close(), and add regression tests. Co-authored-by: Gouryella <Gouryella@users.noreply.github.com>
Summary
This PR fixes all issues identified in the comprehensive code review across client CLI, connection pool, and server tunnel infrastructure.
Client fixes
drip starthang: Separate startup and run phases so partial tunnel failures no longer block foreverstopChduring data session handshake; enforcemaxSessionswith pending slot trackingc.primarydata race: Guard primary session assignment with mutexresp.Bodyafterhttp.ReadResponseContent-Lengthconfig init/config setsaveDRIP_TOKENenv instead of argvos.Exit; extend shutdown wait after force-close timeouterrors.Isfor EOF /net.ErrCloseddetectionServer fixes
RemoteIPon TCP/TLS listener path (previously only WebSocket)sessionHeapPoolon all pathsPrimaryConn, then always callgroup.Close()so heartbeat/data sessions are torn down even when the map entry was already removed; defer map deletion until cleanup completesUnregisterConnection()when connection is handed offTakeListener(fixes TOCTOU)LastActiveon send, stream open, and byte countersTest plan
go test ./...go vet ./...go test -race ./internal/client/... ./internal/server/...TestCleanupStaleGroupsStopsHeartbeatandTestCleanupStaleGroupsStopsHeartbeatWithPrimaryConn