Fix concurrent map writes in codespace port forwarding#13313
Open
williammartin wants to merge 5 commits into
Open
Fix concurrent map writes in codespace port forwarding#13313williammartin wants to merge 5 commits into
williammartin wants to merge 5 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Instead of copying CodespaceConnection by value and using a *sync.Mutex pointer to share the mutex across copies, store a *CodespaceConnection pointer in CodespacesPortForwarder. This makes all forwarders naturally share the same connection (and its mutex) without pointer tricks. Also extract the mutex-guarded section in ForwardPort into a helper method (createTunnelPort) that uses defer to unlock, replacing fragile manual Unlock calls on each error path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a race in gh cs ports forward where concurrent port forwards can trigger fatal error: concurrent map writes due to goroutine-unsafe mutations inside the dev-tunnels TunnelManager/Tunnel objects.
Changes:
- Switch
CodespacesPortForwarder.connectionto store*connection.CodespaceConnection(avoids shallow-copying shared pointers). - Add
ManagerMu sync.MutextoCodespaceConnectionand use it to serializeTunnelManageroperations (create/get/list/delete ports). - Add a concurrent forwarding test intended to reproduce the race under
go test -race.
Show a summary per file
| File | Description |
|---|---|
| internal/codespaces/portforwarder/port_forwarder.go | Avoids connection shallow-copying and serializes tunnel-manager operations with a mutex. |
| internal/codespaces/portforwarder/port_forwarder_test.go | Adds/updates tests, including a new concurrent-forwarding regression test. |
| internal/codespaces/connection/connection.go | Introduces ManagerMu on CodespaceConnection to coordinate manager access across forwarders. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
Wrap GetTunnelPort and DeleteTunnelPort calls in anonymous functions that use defer for the mutex unlock, making them robust against future edits that add early returns between Lock and Unlock. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Hold ManagerMu across the entire get/check/delete sequence in UpdatePortVisibility to eliminate a time-of-check-time-of-use gap - Remove IIFE pattern in favor of explicit lock/unlock - Revert signal.NotifyContext change in cmd.go (out of scope) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13399
When forwarding multiple ports concurrently with
gh cs ports forward, the dev-tunnelsTunnelManagermutates shared state on theTunnelobject inbuildUri(), which is not goroutine-safe. This causes afatal error: concurrent map writespanic.Root cause
ForwardPortsruns all port forwards concurrently viaerrgroup, sharing a singleCodespaceConnection. The shallow copy inNewPortForwarder(*codespaceConnectiondereference) means all forwarders share the sameTunnelManagerandTunnelpointers, so concurrent calls toGetTunnelPort/CreateTunnelPortrace on the shared map.Fix
CodespacesPortForwarder.connectionfromconnection.CodespaceConnection(value) to*connection.CodespaceConnection(pointer), eliminating the unnecessary shallow copy. Since all the important fields (TunnelManager,TunnelClient,Tunnel,Options) were already pointers, the copy was pointless and created this class of bug.sync.Mutex(ManagerMu) toCodespaceConnectionto serializeTunnelManageroperations that mutate shared state on theTunnelobject. Because the connection is now shared by pointer, a plainsync.Mutex(not a pointer) works naturally.createTunnelPorthelper fromForwardPortso the mutex-guarded critical section usesdeferto unlock, replacing fragile manualUnlock()calls on each error path.ConnectandRefreshPortsalready have their own synchronization and are left outside the lock.Test
Added
TestConcurrentForwardPortDoesNotRacewhich forwards 10 ports concurrently from a shared connection, reproducing the race detected bygo test -race.Performance
The mutex serializes
TunnelManagerAPI calls that were previously concurrent. Benchmarking with a real codespace (5 ports, 5 runs each) shows no measurable impact:The serialization overhead is negligible because the bottleneck is network round-trips to the dev-tunnels API and SSH tunnel setup, not CPU time in the critical section.