-
Notifications
You must be signed in to change notification settings - Fork 2
feat(mcp): improve concurrent workspace execution #864
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.
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
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
da158c4
fix(workspace): lock workspace during ExecOneShot to prevent races wi…
skevetter 3fbf972
fix(workspace): wrap long t.Fatalf line in exec_test.go to satisfy ll…
skevetter 485b37d
feat(mcp): bound concurrent workspace_exec/create/start operations wi…
skevetter 5edf9cc
fix(up): --ide-launch=skip also skips IDE server install when --ide i…
skevetter 1bfa505
fix(e2e): assert resolved IDE name, not a container path, for skip-la…
skevetter f9804d4
test(e2e): add MCP stdio JSON-RPC e2e coverage for workspace_list/wor…
skevetter 373c55c
fix(provider): fsync parent directory after atomic rename for crash d…
skevetter 2902ca7
fix(e2e): add missing !windows build tag to skip_launch_no_install.go
skevetter f7a0a02
fix: address CodeRabbit findings on semaphore gating and lock test co…
skevetter d98626b
chore: add plan doc and gitignore SDD scratch workspace
skevetter 76af939
fix: extract semaphore-error check to reduce cyclop complexity
skevetter dd4f609
refactor: trim comments to non-obvious WHY, drop plan doc from PR
skevetter 9f05c58
fix: correlate MCP e2e responses by ID and harden semaphore acquire
skevetter 9e26371
fix: honor ctx cancellation in MCPClient.CallTool
skevetter 9862da4
fix(lint): extract jsonRPCError to satisfy revive nested-structs
skevetter 3d4d518
fix: trim ResolveDockerCommand comment to one line
skevetter 6b2efdb
fix: replace unicode arrow with ASCII in ResolveDockerCommand comment
skevetter 700a317
Merge branch 'main' into feat/mcp-exec-safety-and-coverage
skevetter 4fe604f
style: update comments
skevetter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package mcp | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| ) | ||
|
|
||
| type opSemaphore struct { | ||
| slots chan struct{} | ||
| } | ||
|
|
||
| func newOpSemaphore(max int) *opSemaphore { | ||
| if max <= 0 { | ||
| max = 1 | ||
| } | ||
| return &opSemaphore{slots: make(chan struct{}, max)} | ||
| } | ||
|
|
||
| func (s *opSemaphore) acquire(ctx context.Context) (func(), error) { | ||
| if err := ctx.Err(); err != nil { | ||
| return nil, fmt.Errorf("waiting for a free operation slot: %w", err) | ||
| } | ||
| select { | ||
| case s.slots <- struct{}{}: | ||
| if err := ctx.Err(); err != nil { | ||
| <-s.slots | ||
| return nil, fmt.Errorf("waiting for a free operation slot: %w", err) | ||
| } | ||
| return func() { <-s.slots }, nil | ||
| case <-ctx.Done(): | ||
| return nil, fmt.Errorf("waiting for a free operation slot: %w", ctx.Err()) | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| package mcp | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "sync/atomic" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| func TestOpSemaphore_LimitsConcurrency(t *testing.T) { | ||
| sem := newOpSemaphore(2) | ||
| var inFlight, maxInFlight atomic.Int32 | ||
|
|
||
| track := func() { | ||
| cur := inFlight.Add(1) | ||
| for { | ||
| m := maxInFlight.Load() | ||
| if cur <= m || maxInFlight.CompareAndSwap(m, cur) { | ||
| break | ||
| } | ||
| } | ||
| time.Sleep(20 * time.Millisecond) | ||
| inFlight.Add(-1) | ||
| } | ||
|
|
||
| done := make(chan struct{}, 5) | ||
| for range 5 { | ||
| go func() { | ||
| release, err := sem.acquire(context.Background()) | ||
| if err != nil { | ||
| t.Errorf("acquire failed: %v", err) | ||
| done <- struct{}{} | ||
| return | ||
| } | ||
| track() | ||
| release() | ||
| done <- struct{}{} | ||
| }() | ||
| } | ||
| for range 5 { | ||
| <-done | ||
| } | ||
|
|
||
| if got := maxInFlight.Load(); got > 2 { | ||
| t.Fatalf("max concurrent = %d, want <= 2", got) | ||
| } | ||
| } | ||
|
|
||
| func TestOpSemaphore_ReleaseAllowsNextAcquire(t *testing.T) { | ||
| sem := newOpSemaphore(1) | ||
| release1, err := sem.acquire(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("first acquire failed: %v", err) | ||
| } | ||
|
|
||
| acquired := make(chan struct{}) | ||
| go func() { | ||
| release2, err := sem.acquire(context.Background()) | ||
| if err != nil { | ||
| t.Errorf("second acquire failed: %v", err) | ||
| return | ||
| } | ||
| close(acquired) | ||
| release2() | ||
| }() | ||
|
|
||
| select { | ||
| case <-acquired: | ||
| t.Fatal("second acquire succeeded while first slot was held") | ||
| case <-time.After(50 * time.Millisecond): | ||
| } | ||
|
|
||
| release1() | ||
| select { | ||
| case <-acquired: | ||
| case <-time.After(time.Second): | ||
| t.Fatal("second acquire never succeeded after release") | ||
| } | ||
| } | ||
|
|
||
| func TestOpSemaphore_AcquireRespectsContextCancel(t *testing.T) { | ||
| sem := newOpSemaphore(1) | ||
| release, err := sem.acquire(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("first acquire failed: %v", err) | ||
| } | ||
| defer release() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond) | ||
| defer cancel() | ||
| _, err = sem.acquire(ctx) | ||
| if !errors.Is(err, context.DeadlineExceeded) { | ||
| t.Fatalf("acquire error = %v, want context deadline exceeded", err) | ||
| } | ||
| } |
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject an already-canceled context before acquiring a slot.
When
ctxis already canceled and a slot is free, both cases in Line 27 are ready. Go can select the channel send. The handler can then start a canceled workspace operation and consume a permit.cmd/mcp/semaphore.go#L27-L32: Checkctx.Err()before theselect. If cancellation is observed after the channel send, remove the token and return the context error.cmd/mcp/semaphore_test.go#L82-L96: Add a test that cancels a context before callingacquirewhile semaphore capacity is available.📍 Affects 2 files
cmd/mcp/semaphore.go#L27-L32(this comment)cmd/mcp/semaphore_test.go#L82-L96🤖 Prompt for AI Agents