feat(mcp): improve concurrent workspace execution - #864
Draft
skevetter wants to merge 17 commits into
Draft
Conversation
…th a semaphore Add an in-process opSemaphore gating workspace_exec, workspace_create, and workspace_start so an orchestrator driving one devsy mcp serve process can't overwhelm the local Docker/Kubernetes backend with an unbounded burst of calls. workspace_list/status/stop/delete and provider_* tools stay ungated since gating deletes could deadlock a caller trying to free resources while all slots are held by stuck creates. New flag --mcp-max-concurrent-ops (default 8) controls the limit.
…s unset --ide-launch=skip only suppressed the host-side IDE launch; the container still downloaded and installed an IDE server binary (e.g. openvscode-server) because installIDE is gated solely on ide.Name, which never sees IDELaunch. validate() now mirrors RunHeadless's existing pairing and defaults IDE to none when launch is skipped and no explicit --ide was passed, while respecting an explicit --ide choice.
…unch The prior e2e assertion checked test -d /root/.openvscode-server inside the container, but this fixture's devcontainer sets remoteUser=vscode, so openvscode-server actually installs under /home/vscode/.openvscode-server. The assertion printed "absent" whether or not the IDE server was installed, giving zero regression protection. Assert instead on the host-side workspace config's ws.IDE.Name, the exact value applySkipLaunchIDEDefault sets and installIDE gates on. Verified via a live Docker round-trip: reverting the fix makes the test fail (ide.Name resolves to "openvscode" and openvscode-server actually installs); restoring the fix makes it pass (ide.Name is "none", no install log line at all).
…kspace_exec Adds a reusable MCPClient helper (e2e/framework/mcp.go) that drives a real devsy mcp serve subprocess over stdio JSON-RPC, and a new e2e/tests/mcp package exercising workspace_list and workspace_exec through the actual MCP transport instead of the SDK's in-memory transport used by unit tests.
skip_launch_no_install.go calls setupBrowserIDE which is only available on non-Windows platforms (defined in browser_returns.go with //go:build !windows). Adding the same build tag ensures cross-platform build compatibility.
…verage - Drive workspace_exec through a real MCP client in TestServer_WorkspaceExecRespectsSemaphore so it proves the tool handler itself is gated by the semaphore, not just the primitive. - Assert errors.Is(err, context.DeadlineExceeded) in TestOpSemaphore_AcquireRespectsContextCancel instead of a bare non-nil check. - Validate workspace_create's source before acquiring the op semaphore so malformed requests fail fast without consuming a scarce slot. - Extend TestExecOneShot_UnlocksAfterSuccessfulLock to exercise Unlock and assert unlockCalls, matching what the test name promises.
Records the design plan for this branch's readiness-gap fixes and ignores .superpowers/ (per-plan SDD scratch state) so it never lands in a commit.
❌ Deploy Preview for devsydev failed.
|
📝 WalkthroughWalkthroughThe change adds bounded MCP concurrency, workspace execution locking, IDE skip-launch normalization, MCP stdio end-to-end tests, and POSIX atomic-write directory synchronization. It also adds supporting tests, documentation, and repository ignore configuration. ChangesMCP operation concurrency
Workspace execution locking
IDE launch normalization
MCP stdio end-to-end coverage
Atomic provider-write durability
Supporting repository changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCPTest
participant MCPClient
participant DevsyMCPServer
participant Workspace
MCPTest->>MCPClient: StartMCPServer
MCPClient->>DevsyMCPServer: initialize over stdio
MCPTest->>MCPClient: CallTool workspace_exec
MCPClient->>DevsyMCPServer: JSON-RPC tool request
DevsyMCPServer->>Workspace: execute workspace command
Workspace-->>DevsyMCPServer: command result
DevsyMCPServer-->>MCPClient: JSON-RPC response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pkg/workspace/exec_test.go (1)
200-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the production lock lifecycle.
These tests call
acquireExecLockdirectly. They do not executeresolveExecTargetorExecOneShot.They will pass if
resolvedExecTarget.unlockis not populated, if a post-lock resolution failure leaks the lock, or ifExecOneShotno longer defersresolved.unlock().Add an injectable workspace and runtime seam. Assert one unlock after successful execution and after each failure that occurs after lock acquisition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/workspace/exec_test.go` around lines 200 - 243, Replace the direct acquireExecLock tests with production-path tests that exercise resolveExecTarget and ExecOneShot through injectable workspace and runtime seams. Verify resolvedExecTarget.unlock is populated, ExecOneShot unlocks exactly once after successful execution, and every failure after lock acquisition also unlocks exactly once; retain the lock-failure assertion that no unlock occurs when acquisition fails.pkg/provider/atomic_test.go (1)
70-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test to match what it proves.
The test reads the file after
WriteFileAtomicreturns. This verifies content visibility, but it does not prove crash durability or exercise a failedsyncDir. It also passes on Windows, wheresyncDiris a no-op.At minimum, use a name such as
TestWriteFileAtomic_SucceedsAndPreservesData. Add separate platform-specific or failure-injection coverage if crash durability requires direct regression testing.Proposed rename
-func TestWriteFileAtomic_SucceedsAndDataIsDurable(t *testing.T) { +func TestWriteFileAtomic_SucceedsAndPreservesData(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/atomic_test.go` around lines 70 - 85, Rename TestWriteFileAtomic_SucceedsAndDataIsDurable to reflect that it verifies successful writing and preserved file content, such as TestWriteFileAtomic_SucceedsAndPreservesData. Do not describe this test as proving crash durability; leave platform-specific or failure-injection coverage outside this change.e2e/tests/mcp/helper.go (1)
10-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegister the temp-dir cleanup before the provider setup.
If
framework.SetupDockerProviderfails at line 17, the function returns before line 21. The temporary directory created at line 11 then stays on disk for the whole CI run. Move theCleanupTempDirregistration directly afterCopyToTempDir.♻️ Proposed refactor
tempDir, err := framework.CopyToTempDir(testdataPath) if err != nil { return "", nil, err } + ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir) f, err := framework.SetupDockerProvider(initialDir+"/bin", "docker") if err != nil { return "", nil, err } - ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir) ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, tempDir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/mcp/helper.go` around lines 10 - 25, Move the ginkgo.DeferCleanup registration for framework.CleanupTempDir immediately after the successful framework.CopyToTempDir call in setupWorkspace, before framework.SetupDockerProvider runs; keep the provider cleanup registration after successful setup.e2e/framework/mcp.go (2)
45-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture the server stderr for diagnostics.
cmd.Stderrstays nil, so the subprocess stderr goes to/dev/null. Ifdevsy mcp servefails to start or panics, the test reports only an EOF fromreadResponse. Attach a buffer orginkgo.GinkgoWriterand include its content in handshake errors.♻️ Proposed refactor
cmd := exec.CommandContext(ctx, filepath.Join(f.DevsyBinDir, f.DevsyBinName), "mcp", "serve") + var stderr bytes.Buffer + cmd.Stderr = &stderr stdinPipe, err := cmd.StdinPipe()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/framework/mcp.go` around lines 45 - 58, Update StartMCPServer to attach the subprocess stderr to a diagnostic buffer or GinkgoWriter before cmd.Start. Include the captured stderr content when reporting handshake/readResponse failures, while preserving the existing startup error handling.
100-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCorrelate the response ID with the request ID.
readResponsereturns the next line on stdout. The code assumes that line is the response for this request. If the server writes a notification or any other JSON-RPC message,CallTooldecodes the wrong payload and the test fails with a confusing message. Compareresp.IDwith the sent ID, or skip lines whoseidis 0.♻️ Proposed refactor
- if err := c.send(jsonRPCRequest{ + id := c.nextID.Add(1) + if err := c.send(jsonRPCRequest{ JSONRPC: jsonRPCVersion, - ID: c.nextID.Add(1), + ID: id, Method: "tools/call", Params: map[string]any{"name": name, "arguments": args}, }); err != nil { return nil, false, err } resp, err := c.readResponse() if err != nil { return nil, false, err } + if resp.ID != id { + return nil, false, fmt.Errorf("response id mismatch: want %d, got %d", id, resp.ID) + } if resp.Error != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/framework/mcp.go` around lines 100 - 114, Update CallTool to retain the request ID generated for the tools/call request and verify that the response returned by readResponse has the same ID before processing resp.Error or the result. Skip notification or unrelated JSON-RPC messages, including responses with ID 0, and continue reading until the matching response is received.e2e/tests/mcp/mcp.go (1)
30-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the fixture workspace appears in the list.
The current check only requires a non-empty list. Any workspace left over from another spec or from the developer machine satisfies it. The spec title states that it lists a running workspace, so assert that the fixture workspace is present in the result.
♻️ Proposed refactor
workspaces, ok := listResult["workspaces"].([]any) gomega.Expect(ok).To(gomega.BeTrue()) gomega.Expect(workspaces).NotTo(gomega.BeEmpty()) + names := []string{} + for _, w := range workspaces { + entry, isMap := w.(map[string]any) + gomega.Expect(isMap).To(gomega.BeTrue()) + name, _ := entry["name"].(string) + names = append(names, name) + } + gomega.Expect(names).To(gomega.ContainElement(filepath.Base(tempDir)))Add the
path/filepathimport. Adjust the expected value ifworkspace_execresolves names differently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/mcp/mcp.go` around lines 30 - 35, Update the workspace_list assertions in the MCP test to verify that the fixture workspace is present, rather than only requiring a non-empty result. Derive the expected workspace name or path from the existing fixture configuration, using filepath as needed, and assert that one entry matches it while preserving the existing tool error checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/mcp/semaphore.go`:
- Around line 27-32: Update acquire in cmd/mcp/semaphore.go#L27-L32 to check
ctx.Err() before selecting, and recheck after acquiring a slot; if cancellation
is detected, release the token and return the context error. Add a test in
cmd/mcp/semaphore_test.go#L82-L96 covering a pre-canceled context with available
semaphore capacity, verifying no permit remains consumed.
In `@cmd/mcp/serve_test.go`:
- Around line 62-132: Reduce the cyclomatic complexity of
TestServer_WorkspaceExecRespectsSemaphore below the configured limit by
extracting either the MCP server/client setup or the res.IsError content
inspection into a focused helper. Preserve the test’s semaphore assertions and
existing failure messages, and keep the helper anchored to the setup or
result-validation logic rather than changing behavior.
In `@e2e/framework/mcp.go`:
- Around line 70-93: Update StartMCPServer’s handshake error paths to invoke
closeFn before returning after cmd.Start succeeds, ensuring the subprocess and
pipe goroutines are cleaned up. Capture the initialize response from
readResponse and validate its JSON-RPC error field before sending
notifications/initialized; return a descriptive error when initialization fails,
while preserving successful handshake behavior.
---
Nitpick comments:
In `@e2e/framework/mcp.go`:
- Around line 45-58: Update StartMCPServer to attach the subprocess stderr to a
diagnostic buffer or GinkgoWriter before cmd.Start. Include the captured stderr
content when reporting handshake/readResponse failures, while preserving the
existing startup error handling.
- Around line 100-114: Update CallTool to retain the request ID generated for
the tools/call request and verify that the response returned by readResponse has
the same ID before processing resp.Error or the result. Skip notification or
unrelated JSON-RPC messages, including responses with ID 0, and continue reading
until the matching response is received.
In `@e2e/tests/mcp/helper.go`:
- Around line 10-25: Move the ginkgo.DeferCleanup registration for
framework.CleanupTempDir immediately after the successful
framework.CopyToTempDir call in setupWorkspace, before
framework.SetupDockerProvider runs; keep the provider cleanup registration after
successful setup.
In `@e2e/tests/mcp/mcp.go`:
- Around line 30-35: Update the workspace_list assertions in the MCP test to
verify that the fixture workspace is present, rather than only requiring a
non-empty result. Derive the expected workspace name or path from the existing
fixture configuration, using filepath as needed, and assert that one entry
matches it while preserving the existing tool error checks.
In `@pkg/provider/atomic_test.go`:
- Around line 70-85: Rename TestWriteFileAtomic_SucceedsAndDataIsDurable to
reflect that it verifies successful writing and preserved file content, such as
TestWriteFileAtomic_SucceedsAndPreservesData. Do not describe this test as
proving crash durability; leave platform-specific or failure-injection coverage
outside this change.
In `@pkg/workspace/exec_test.go`:
- Around line 200-243: Replace the direct acquireExecLock tests with
production-path tests that exercise resolveExecTarget and ExecOneShot through
injectable workspace and runtime seams. Verify resolvedExecTarget.unlock is
populated, ExecOneShot unlocks exactly once after successful execution, and
every failure after lock acquisition also unlocks exactly once; retain the
lock-failure assertion that no unlock occurs when acquisition fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 158a6d82-9b08-4581-bae1-7882041c9076
📒 Files selected for processing (23)
.gitignorecmd/mcp/semaphore.gocmd/mcp/semaphore_test.gocmd/mcp/serve.gocmd/mcp/serve_test.gocmd/mcp/tools_exec.gocmd/mcp/tools_workspace.gocmd/workspace/up/up_test.gocmd/workspace/up/up_validate.godocs/superpowers/plans/2026-08-03-devsy-mcp-readiness-gaps.mde2e/e2e_suite_test.goe2e/framework/mcp.goe2e/tests/ide/skip_launch_no_install.goe2e/tests/mcp/helper.goe2e/tests/mcp/mcp.goe2e/tests/mcp/testdata/basic/.devcontainer/devcontainer.jsonpkg/flags/names/names.gopkg/provider/atomic.gopkg/provider/atomic_posix.gopkg/provider/atomic_test.gopkg/provider/atomic_windows.gopkg/workspace/exec.gopkg/workspace/exec_test.go
Comment on lines
+27
to
+32
| select { | ||
| case s.slots <- struct{}{}: | ||
| return func() { <-s.slots }, nil | ||
| case <-ctx.Done(): | ||
| return nil, fmt.Errorf("waiting for a free operation slot: %w", ctx.Err()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject an already-canceled context before acquiring a slot.
When ctx is 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/mcp/semaphore.go` around lines 27 - 32, Update acquire in
cmd/mcp/semaphore.go#L27-L32 to check ctx.Err() before selecting, and recheck
after acquiring a slot; if cancellation is detected, release the token and
return the context error. Add a test in cmd/mcp/semaphore_test.go#L82-L96
covering a pre-canceled context with available semaphore capacity, verifying no
permit remains consumed.
Comment on lines
+70
to
+93
| if err := c.send(jsonRPCRequest{ | ||
| JSONRPC: jsonRPCVersion, | ||
| ID: c.nextID.Add(1), | ||
| Method: "initialize", | ||
| Params: map[string]any{ | ||
| "protocolVersion": "2024-11-05", | ||
| "capabilities": map[string]any{}, | ||
| "clientInfo": map[string]any{"name": "devsy-e2e", "version": "0.1"}, | ||
| }, | ||
| }); err != nil { | ||
| return nil, err | ||
| } | ||
| if _, err := c.readResponse(); err != nil { | ||
| return nil, fmt.Errorf("initialize handshake: %w", err) | ||
| } | ||
| if err := c.send(jsonRPCRequest{ | ||
| JSONRPC: jsonRPCVersion, | ||
| Method: "notifications/initialized", | ||
| }); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return c, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up the subprocess when the handshake fails, and check the initialize error.
Two problems exist in this block:
- Each early return at lines 80, 83, and 89 happens after
cmd.Start()succeeded. The function never callscloseFn, so stdin stays open andcmd.Waitis never called. The subprocess and its pipe goroutines stay alive until the test context ends. - The code discards the initialize response body. If the server returns a JSON-RPC error for
initialize,StartMCPServerreports success. The failure then appears later in an unrelatedCallToolassertion.
🔧 Proposed fix
- if err := c.send(jsonRPCRequest{
+ fail := func(err error) (*MCPClient, error) {
+ _ = c.Close()
+ return nil, err
+ }
+
+ if err := c.send(jsonRPCRequest{
JSONRPC: jsonRPCVersion,
ID: c.nextID.Add(1),
Method: "initialize",
Params: map[string]any{
"protocolVersion": "2024-11-05",
"capabilities": map[string]any{},
"clientInfo": map[string]any{"name": "devsy-e2e", "version": "0.1"},
},
}); err != nil {
- return nil, err
+ return fail(err)
}
- if _, err := c.readResponse(); err != nil {
- return nil, fmt.Errorf("initialize handshake: %w", err)
+ resp, err := c.readResponse()
+ if err != nil {
+ return fail(fmt.Errorf("initialize handshake: %w", err))
+ }
+ if resp.Error != nil {
+ return fail(fmt.Errorf(
+ "initialize handshake: jsonrpc error %d: %s", resp.Error.Code, resp.Error.Message))
}
if err := c.send(jsonRPCRequest{
JSONRPC: jsonRPCVersion,
Method: "notifications/initialized",
}); err != nil {
- return nil, err
+ return fail(err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := c.send(jsonRPCRequest{ | |
| JSONRPC: jsonRPCVersion, | |
| ID: c.nextID.Add(1), | |
| Method: "initialize", | |
| Params: map[string]any{ | |
| "protocolVersion": "2024-11-05", | |
| "capabilities": map[string]any{}, | |
| "clientInfo": map[string]any{"name": "devsy-e2e", "version": "0.1"}, | |
| }, | |
| }); err != nil { | |
| return nil, err | |
| } | |
| if _, err := c.readResponse(); err != nil { | |
| return nil, fmt.Errorf("initialize handshake: %w", err) | |
| } | |
| if err := c.send(jsonRPCRequest{ | |
| JSONRPC: jsonRPCVersion, | |
| Method: "notifications/initialized", | |
| }); err != nil { | |
| return nil, err | |
| } | |
| return c, nil | |
| } | |
| fail := func(err error) (*MCPClient, error) { | |
| _ = c.Close() | |
| return nil, err | |
| } | |
| if err := c.send(jsonRPCRequest{ | |
| JSONRPC: jsonRPCVersion, | |
| ID: c.nextID.Add(1), | |
| Method: "initialize", | |
| Params: map[string]any{ | |
| "protocolVersion": "2024-11-05", | |
| "capabilities": map[string]any{}, | |
| "clientInfo": map[string]any{"name": "devsy-e2e", "version": "0.1"}, | |
| }, | |
| }); err != nil { | |
| return fail(err) | |
| } | |
| resp, err := c.readResponse() | |
| if err != nil { | |
| return fail(fmt.Errorf("initialize handshake: %w", err)) | |
| } | |
| if resp.Error != nil { | |
| return fail(fmt.Errorf( | |
| "initialize handshake: jsonrpc error %d: %s", resp.Error.Code, resp.Error.Message)) | |
| } | |
| if err := c.send(jsonRPCRequest{ | |
| JSONRPC: jsonRPCVersion, | |
| Method: "notifications/initialized", | |
| }); err != nil { | |
| return fail(err) | |
| } | |
| return c, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/framework/mcp.go` around lines 70 - 93, Update StartMCPServer’s handshake
error paths to invoke closeFn before returning after cmd.Start succeeds,
ensuring the subprocess and pipe goroutines are cleaned up. Capture the
initialize response from readResponse and validate its JSON-RPC error field
before sending notifications/initialized; return a descriptive error when
initialization fails, while preserving successful handshake behavior.
TestServer_WorkspaceExecRespectsSemaphore scored 9 against the repo's cyclop limit of 8 after the CodeRabbit fix wave rewrote it into a full MCP round-trip test. CI's Lint check caught it since the local pre-push check only covers new-from-rev at the branch's own base, not main's current tip.
Comments across this branch had drifted toward restating behavior already clear from names/signatures, or explaining implementation-plan mechanics (e.g. "Step 3") with no meaning outside that planning session. Cut to one-liners covering only genuinely non-obvious rationale (asymmetric lock timeouts, POSIX/Windows fsync split, the IDELaunch/IDE-name gating mismatch). Also removes docs/superpowers/plans/2026-08-03-devsy-mcp-readiness-gaps.md — planning artifacts from the superpowers skill workflow are local working files, not project documentation, and should never land in a shipped diff.
e2e/framework/mcp.go's MCPClient.readResponse read exactly one line after each request, assuming it was that request's response. Tool calls that stream log progress (workspace_create/workspace_start) interleave id-less notifications on the same stdout stream, so a future spec exercising those would have decoded a notification as the response. Now reads until a response with the matching id arrives. Also: CallTool wasn't safe for concurrent use (shared stdin/stdout with no synchronization), and a handshake failure in StartMCPServer leaked the subprocess instead of killing it. Both fixed. opSemaphore.acquire had a narrow race: a context that's cancelled between winning the select and returning would hand out a slot anyway. Checked before and after acquiring. Found by a second CodeRabbit CLI pass after the initial fix wave.
skevetter
marked this pull request as draft
August 3, 2026 15:35
CallTool accepted a context but readResponseFor's blocking ReadBytes ignored it, so a hung devsy mcp serve process would block a caller past its own timeout — and since c.mu stays held for the call's duration, every later CallTool on that client would also hang forever behind the same unreleased lock. readResponseForCtx now races the read against ctx.Done(). bufio.Reader isn't safe for concurrent use, so a cancellation that fires mid-read can't simply abandon that goroutine and let a later call start a second read on the same stream — instead the client is marked poisoned and every subsequent call fails fast. Verified against a real devsy mcp serve subprocess: an already-expired context aborts in ~85µs (not a multi-second hang), and the next call on the poisoned client fails immediately rather than reading state left by the abandoned goroutine. Found by a third CodeRabbit CLI pass.
main merged a lint-config change (0eeed9a, #868) enabling revive.nested-structs while this branch was in flight, so CI's Lint job — which resolves against main's current tip — flagged the anonymous struct in jsonRPCResponse.Error that this branch's own .golangci.yaml doesn't yet know about. Same fix either way: named type, no behavior change.
skevetter
changed the title
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
1 participant
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.
Summary
Fixes 5 gaps found during a hands-on readiness evaluation of devsy's CLI/MCP surface for external agents orchestrating many devcontainer workspaces (deploy, exec, teardown), plus a cross-platform build fix and CodeRabbit review findings surfaced along the way.
workspace.ExecOneShotnow takes the existing per-workspace flock before running, closing a race where a concurrentworkspace_delete/workspace_createcould interleave with an in-flightworkspace_execon the same workspace.--mcp-max-concurrent-ops) gatingworkspace_exec/workspace_create/workspace_start, so an orchestrator driving many workspaces through onedevsy mcp serveprocess can't overwhelm the local Docker/Kubernetes backend.--ide-launch=skipstill triggered an IDE install: now also defaults--idetononewhen left unset, so headless/agent callers don't pay for an unwanted IDE server binary download.MCPClient(real stdio JSON-RPC against a realdevsy mcp servesubprocess) and e2e specs coveringworkspace_list/workspace_execand the unknown-workspace error path.WriteFileAtomicnow fsyncs the parent directory after the atomic rename on POSIX (Windows unaffected, still a no-op).Also fixes a missing
!windowsbuild tag discovered along the way, and addresses all 5 findings from a CodeRabbit review of the full diff (semaphore-gating test depth, source validation ordering, lock/unlock test assertions).Test plan
go build ./...and the full non-e2e suite (go test ./... -race, excluding/testand/e2e) pass, 101 packages, zero failures.golangci-lint runclean at every commit.ginkgo --focus "devsy mcp serve"and theideskip-launch spec) run against a real Docker/Colima backend, independently verified.Summary by CodeRabbit
New Features
--ide-launch=skipnow defaults to no IDE when none is specified.Bug Fixes
Tests