Skip to content

Fix bugs with connection switching #2957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sawka merged 3 commits into from
Feb 27, 2026
Merged

Fix bugs with connection switching #2957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61836e3
add getorcreate to syncmap
sawka Feb 26, 2026
0d6e9d9
pass rtopts to startnewjob
sawka Feb 26, 2026
29786e5
per blockid lockout for resync controller
sawka Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions pkg/blockcontroller/blockcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/wavetermdev/waveterm/pkg/jobcontroller"
"github.com/wavetermdev/waveterm/pkg/remote"
"github.com/wavetermdev/waveterm/pkg/remote/conncontroller"
"github.com/wavetermdev/waveterm/pkg/util/ds"
"github.com/wavetermdev/waveterm/pkg/util/shellutil"
"github.com/wavetermdev/waveterm/pkg/wavebase"
"github.com/wavetermdev/waveterm/pkg/waveobj"
Expand Down Expand Up @@ -75,10 +76,17 @@ type Controller interface {

// Registry for all controllers
var (
controllerRegistry = make(map[string]Controller)
registryLock sync.RWMutex
controllerRegistry = make(map[string]Controller)
registryLock sync.RWMutex
blockResyncMutexMap = ds.MakeSyncMap[*sync.Mutex]()
)

func getBlockResyncMutex(blockId string) *sync.Mutex {
return blockResyncMutexMap.GetOrCreate(blockId, func() *sync.Mutex {
return &sync.Mutex{}
})
}
Comment on lines +81 to +88
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Enumerate all uses of blockResyncMutexMap.
rg -n --type=go '\bblockResyncMutexMap\b' -C3

# Verify whether Delete is ever called for this map.
rg -n --type=go 'blockResyncMutexMap\.Delete\s*\(' || true

Repository: wavetermdev/waveterm

Length of output: 826

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find all uses of getBlockResyncMutex
rg -n --type=go 'getBlockResyncMutex' -C5

# Check for any cleanup, Reset, or Clear methods on blockResyncMutexMap
rg -n --type=go 'blockResyncMutexMap\.(Clear|Reset|Range)' || true

Repository: wavetermdev/waveterm

Length of output: 1683

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check for block deletion or removal patterns
rg -n --type=go 'DeleteBlock|RemoveBlock|block.*delete|block.*remove' -i | head -20

# Look for block lifecycle or creation patterns
rg -n --type=go 'type Block|CreateBlock|NewBlock' | head -20

Repository: wavetermdev/waveterm

Length of output: 3745

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check the DeleteBlock implementation
sed -n '153,200p' pkg/wcore/block.go

# Also check if blockcontroller is involved in block deletion
rg -n --type=go 'ResyncController|blockResync' | head -20

Repository: wavetermdev/waveterm

Length of output: 2489

blockResyncMutexMap has no cleanup for deleted blocks, causing unbounded map growth.

When blocks are deleted via DeleteBlock(), their corresponding mutex entries remain in blockResyncMutexMap indefinitely. Since blocks are frequently created and deleted throughout the application lifecycle, this creates a memory leak with monotonic map growth.

Consider either:

  • Bounded lock striping (hash blockId → fixed mutex array)
  • Safe cleanup in DeleteBlock() with ref-counted or weak references to lock entries
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/blockcontroller/blockcontroller.go` around lines 81 - 88, The
blockResyncMutexMap created via ds.MakeSyncMap used by getBlockResyncMutex grows
without bounds because entries aren't removed when blocks are deleted; update
the implementation to avoid the leak by either (A) replacing the dynamic map
with bounded lock striping (create a fixed-size slice of sync.Mutex and map
blockId → mutex via a hash function used by getBlockResyncMutex) or (B) add safe
cleanup in DeleteBlock(): when DeleteBlock(blockId) runs, remove or decrement a
refcount for the corresponding entry in blockResyncMutexMap (use a ref-counted
wrapper stored in the map or ds.MakeSyncMap.Remove/Release if available) so
mutex entries are deleted when no longer needed; modify getBlockResyncMutex and
DeleteBlock to use the chosen strategy and ensure concurrent access is safe.


// Registry operations
func getController(blockId string) Controller {
registryLock.RLock()
Expand Down Expand Up @@ -145,6 +153,10 @@ func ResyncController(ctx context.Context, tabId string, blockId string, rtOpts
return fmt.Errorf("invalid tabId or blockId passed to ResyncController")
}

mu := getBlockResyncMutex(blockId)
mu.Lock()
defer mu.Unlock()

Comment on lines +156 to +159
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show key mutation paths for a block controller lifecycle.
rg -n --type=go '\bfunc\s+ResyncController\b|\bfunc\s+DestroyBlockController\b|\bfunc\s+handleBlockCloseEvent\b' -A35 -B5 pkg/blockcontroller/blockcontroller.go

# Show where block-scoped mutex is actually acquired.
rg -n --type=go '\bgetBlockResyncMutex\s*\(' -C2 pkg/blockcontroller/blockcontroller.go

Repository: wavetermdev/waveterm

Length of output: 3247

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find all callers of DestroyBlockController
rg -n --type=go '\bDestroyBlockController\s*\(' pkg/blockcontroller/blockcontroller.go

# Check the full ResyncController function to see all call sites
sed -n '151,200p' pkg/blockcontroller/blockcontroller.go

# Check if there are any other files calling DestroyBlockController
rg -l --type=go '\bDestroyBlockController\s*\(' pkg/

Repository: wavetermdev/waveterm

Length of output: 2129

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check callers in other files
rg -n --type=go '\bDestroyBlockController\s*\(' pkg/wshrpc/wshserver/wshserver.go pkg/service/workspaceservice/workspaceservice.go -B3 -A3

# Also check the full context around DestroyBlockController definition
sed -n '295,303p' pkg/blockcontroller/blockcontroller.go

Repository: wavetermdev/waveterm

Length of output: 1274

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check getController and deleteController implementations to understand what state they protect
sed -n '1,100p' pkg/blockcontroller/blockcontroller.go | grep -A10 'func getController\|func deleteController\|blockResyncMutexMap\|var '

# Get the full beginning of the file to see global state and any other synchronization
head -100 pkg/blockcontroller/blockcontroller.go

Repository: wavetermdev/waveterm

Length of output: 3519

Per-block serialization is incomplete without locking DestroyBlockController.

ResyncController acquires the per-block mutex (line 156-158) and calls DestroyBlockController at lines 176, 185, 216, 224, and 234 while holding it. However, handleBlockCloseEvent (line 146) launches DestroyBlockController asynchronously without this lock, and external callers in other files (workspaceservice.go, wshserver.go) also bypass it. This allows concurrent mutations to the same block's controller state.

Make DestroyBlockController acquire the per-block mutex before proceeding. To avoid self-deadlock when called from ResyncController, create an internal helper that skips lock acquisition for callers that already hold it.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/blockcontroller/blockcontroller.go` around lines 156 - 159,
ResyncController currently holds the per-block mutex from getBlockResyncMutex
and calls DestroyBlockController while holding it, but other callers
(handleBlockCloseEvent and external callers in workspaceservice.go/wshserver.go)
call DestroyBlockController without that lock; to fix, refactor
DestroyBlockController by extracting the core teardown logic into an internal
helper (e.g., destroyBlockControllerUnlocked) that assumes the caller holds the
per-block mutex, then make the public DestroyBlockController acquire
getBlockResyncMutex(blockId).Lock()/Unlock() and call the helper; update
ResyncController to call the new unlocked helper directly to avoid
self-deadlock, and leave other callers using the public DestroyBlockController
so they obtain the mutex.

blockData, err := wstore.DBMustGet[*waveobj.Block](ctx, blockId)
if err != nil {
return fmt.Errorf("error getting block: %w", err)
Expand Down
7 changes: 5 additions & 2 deletions pkg/blockcontroller/durableshellcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func (dsc *DurableShellController) Start(ctx context.Context, blockMeta waveobj.

if jobId == "" {
log.Printf("block %q starting new durable shell\n", dsc.BlockId)
newJobId, err := dsc.startNewJob(ctx, blockMeta, dsc.ConnName)
newJobId, err := dsc.startNewJob(ctx, blockMeta, dsc.ConnName, rtOpts)
if err != nil {
return fmt.Errorf("failed to start new job: %w", err)
}
Expand Down Expand Up @@ -218,11 +218,14 @@ func (dsc *DurableShellController) SendInput(inputUnion *BlockInputUnion) error
return jobcontroller.SendInput(context.Background(), data)
}

func (dsc *DurableShellController) startNewJob(ctx context.Context, blockMeta waveobj.MetaMapType, connName string) (string, error) {
func (dsc *DurableShellController) startNewJob(ctx context.Context, blockMeta waveobj.MetaMapType, connName string, rtOpts *waveobj.RuntimeOpts) (string, error) {
termSize := waveobj.TermSize{
Rows: shellutil.DefaultTermRows,
Cols: shellutil.DefaultTermCols,
}
if rtOpts != nil && rtOpts.TermSize.Rows > 0 && rtOpts.TermSize.Cols > 0 {
termSize = rtOpts.TermSize
}
cmdStr := blockMeta.GetString(waveobj.MetaKey_Cmd, "")
cwd := blockMeta.GetString(waveobj.MetaKey_CmdCwd, "")
opts, err := remote.ParseOpts(connName)
Expand Down
11 changes: 11 additions & 0 deletions pkg/util/ds/syncmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,14 @@ func (sm *SyncMap[T]) TestAndSet(key string, newValue T, testFn func(T, bool) bo
}
return false
}

func (sm *SyncMap[T]) GetOrCreate(key string, createFn func() T) T {
sm.lock.Lock()
defer sm.lock.Unlock()
if v, ok := sm.m[key]; ok {
return v
}
v := createFn()
sm.m[key] = v
return v
}
Loading