fix(agent): scope workspace content folder by UID#512
Merged
Conversation
When a workspace was deleted and recreated under a new UID, the bind-mount source path was reused verbatim. Docker Desktop's macOS file-sharing VM caches /host_mnt entries by inode, so the recycled inode under the same path surfaced as "bind source path does not exist" on the next docker run. Suffix the content folder with the workspace UID so each recreate uses a fresh path, and clear the persisted ContentFolder on UID change so the recompute always picks up the new value. Also log host environment and bind-source resolution before docker run so future cache-race failures are diagnosable from the log alone.
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughThis PR centralizes filesystem path constants and agent download URL logic from ChangesConfig Centralization, Agent API Refactor, SSH Server Rewrite, Docker Diagnostics
Sequence Diagram(s)sequenceDiagram
participant cobra as cobra.Command
participant sshServerCmd
participant parseSSHToken
participant ensureActivityFile
participant runActivityHeartbeat
participant shutdownOnCancel
participant Server
cobra->>sshServerCmd: run(ctx)
sshServerCmd->>parseSSHToken: base64 token blob
parseSSHToken-->>sshServerCmd: authorizedKeys, hostKey
alt --stdio mode
sshServerCmd->>ensureActivityFile: create activity file (if --track-activity)
sshServerCmd->>runActivityHeartbeat: start goroutine (chtimes on interval)
sshServerCmd->>shutdownOnCancel: start goroutine (ctx cancel → Shutdown)
sshServerCmd->>Server: Serve(stdioListener)
else listener mode
sshServerCmd->>sshServerCmd: check port availability → error if in use
sshServerCmd->>shutdownOnCancel: start goroutine
sshServerCmd->>Server: ListenAndServe()
end
Note over shutdownOnCancel: ctx cancelled → Server.Shutdown(bounded timeout ctx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
|
Container-side path constants (ContainerDataDir, ContainerDevsyHelperLocation, RemoteDevsyHelperLocation, ContainerActivityFile, WorkspaceBusyFile) were declared in pkg/agent next to runtime logic. They are pure paths derived from config.BinaryName, so they belong in pkg/config alongside DevContainerResultPath and SSHSignatureHelperPath. No behavior change; all call sites updated.
The function had grown to ~175 lines with five distinct phases (decode,
maybe-rerun-as-root, ensure dir, handle stale workspace, resolve content
folder + write) interleaved with a per-step "log.Errorf … return err"
pattern that doubled every failure in the log.
Extract handleStaleWorkspace and resolveContentFolder so the orchestrator
reads top-to-bottom in ~40 lines. Drop the redundant pre-return Errorf
pairs — wrapped errors already carry the cause. Preserve the
operationally meaningful INFO line ("delete old workspace: …") that ops
greps on.
No behavior change.
Tunnel had seven parameters; the four io.Reader/Writer values plus user and timeout fit naturally in an options struct. Keep ctx as the first positional arg per Go convention.
Lift AgentDownloadBaseURL and AgentLatestDownloadURL to pkg/config alongside GitHubReleasesURL — they're path constants, not runtime state. The DefaultAgentDownloadURL function stays in pkg/agent because it dispatches on env override and version (behavior, not a path).
It's a pure path resolver — picks between an env override, the latest URL, and the version-pinned URL — so it belongs alongside the URL constants in pkg/config/repo.go. The agent package no longer needs to import pkg/version. Tests move with the function.
The original comment referred to /var/lib/loft/* permissions from devsy's upstream fork. devsy doesn't use that path — the predicate is still correct (Platform-enabled runs can't address the host's LocalFolder), but the reasoning needed to match.
There was a problem hiding this comment.
Pull request overview
This PR addresses a reproducible Docker Desktop (macOS) bind-mount failure after workspace delete/recreate by ensuring the workspace “content” bind source path changes when the workspace UID changes, and by adding pre-docker run diagnostics to make host-vs-daemon path-resolution issues visible in logs.
Changes:
- Scope the workspace content folder by UID (
content-<uid>) and clear persistedContentFolderon UID-change recreation so the new UID-suffixed path is recomputed. - Centralize agent/container path constants and the default agent download URL under
pkg/config, updating call sites accordingly. - Add Docker run diagnostics: host environment logging (once) and bind source existence checks (per mount), with a focused unit test for bind-source extraction.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tunnel/services.go | Use config.ContainerDevsyHelperLocation when building the credentials-server command. |
| pkg/options/resolve.go | Switch agent path/URL defaults from agent to config helpers. |
| pkg/ide/rstudio/rstudio.go | Use config.ContainerDataDir for the RStudio download folder path. |
| pkg/ide/jetbrains/generic.go | Use config.ContainerDataDir for JetBrains server download folder path. |
| pkg/driver/docker/docker.go | Emit docker host/bind diagnostics before invoking docker run. |
| pkg/driver/docker/diagnostics.go | New helper functions to log docker host env and bind-mount source existence. |
| pkg/driver/docker/diagnostics_test.go | Unit test coverage for bind source extraction from docker args. |
| pkg/dotfiles/dotfiles.go | Use config.ContainerDevsyHelperLocation when building dotfiles agent command. |
| pkg/devcontainer/setup/setup.go | Use config.ContainerDataDir for marker file paths inside containers. |
| pkg/devcontainer/setup.go | Use config for default agent download URL and helper binary location. |
| pkg/config/repo.go | Introduce config.DefaultAgentDownloadURL() (env override + version-pinned/latest logic). |
| pkg/config/repo_test.go | New tests for URL trimming behavior in config.DefaultAgentDownloadURL(). |
| pkg/config/paths.go | Centralize container/remote helper paths and workspace busy file name in config. |
| pkg/agent/workspace.go | Update search locations and change content dir helper to require UID suffix. |
| pkg/agent/inject.go | Use config.RemoteDevsyHelperLocation and config.DefaultAgentDownloadURL(). |
| pkg/agent/delivery/remote_docker.go | Use config.ContainerDevsyHelperLocation as delivery destination path. |
| pkg/agent/delivery/local_docker.go | Use config.ContainerDevsyHelperLocation for deriving the binary name. |
| pkg/agent/delivery/legacy_shell.go | Use config helper paths and default download URL. |
| pkg/agent/delivery/kubernetes.go | Use config.ContainerDevsyHelperLocation for in-cluster binary placement. |
| pkg/agent/delivery/kubernetes_test.go | Update test expectations to use config.ContainerDevsyHelperLocation. |
| pkg/agent/agent.go | Refactor workspace info writing flow, implement UID-scoped content dir, add TunnelOptions. |
| pkg/agent/agent_test.go | Remove tests that moved to pkg/config/repo_test.go. |
| cmd/workspace/ssh.go | Use config.ContainerDevsyHelperLocation for internal agent commands. |
| cmd/internal/ssh_server.go | Use config.ContainerActivityFile for activity tracking (now has a race issue noted). |
| cmd/internal/fleet_server.go | Use config.ContainerActivityFile when touching activity file. |
| cmd/internal/container_tunnel.go | Update call site to new agent.Tunnel(ctx, agent.TunnelOptions{...}) API. |
| cmd/internal/agentworkspace/up.go | Update runner defaults + pass UID into GetAgentWorkspaceContentDir. |
| cmd/internal/agentworkspace/logs.go | Use config defaults for helper path and download URL in runner creation. |
| cmd/internal/agentcontainer/daemon.go | Use config.ContainerDataDir and config.ContainerActivityFile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
123
to
125
| go func() { | ||
| _, err = os.Stat(agent.ContainerActivityFile) | ||
| _, err = os.Stat(config.ContainerActivityFile) | ||
| if err != nil { |
Comment on lines
+58
to
+62
| func logBindSources(args []string) { | ||
| for _, src := range extractBindSources(args) { | ||
| _, err := os.Lstat(src) | ||
| log.Infof("docker bind: src=%s exists=%t", src, err == nil) | ||
| } |
… safety - Propagate context: Run takes ctx, the activity heartbeat exits on cancel instead of running until process exit. - Replace the os.Create/Close every-10s touch with os.Chtimes — same effect, no truncation, no nil-handle panic on failure. - Fix the "is in use" silent-success path: ListenAndServe now errors when the port is taken instead of logging and returning nil. - Wrap base64 decode errors instead of dropping them; uniform error formatting throughout. - Use errors.Is(err, fs.ErrNotExist) for the activity-file existence check so non-ENOENT stat errors surface. - Validate flag combinations up front: --track-activity now requires --stdio. - Extract parseSSHToken / decodeAuthorizedKeys / decodeBase64Bytes / ensureActivityFile / runActivityHeartbeat so each piece is independently testable; add unit tests covering the pure logic and ctx-cancel semantics.
- Add Shutdown(ctx) to the sshserver.Server interface; implement on both the host server and the container server by delegating to the underlying ssh.Server.Shutdown. - Wire into cmd/internal: a goroutine waits on ctx.Done and calls Shutdown with a 5s timeout, so the SSH server actually drains on SIGTERM instead of running until process exit. The shutdown context derives from Background by design — the parent ctx is already canceled by the time we get here. - Wrap the server-closed error: ssh.ErrServerClosed is the expected outcome of a graceful shutdown, not a CLI failure. - Lower SSHServerCmd to unexported sshServerCmd — nothing outside the package needed it.
skevetter
marked this pull request as ready for review
June 20, 2026 17:12
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/config/repo_test.go (2)
27-40: ⚡ Quick winAdd test coverage for dev and release version paths.
The current tests only verify trailing-slash normalization when
EnvAgentURLis set. The core version-based branching logic inDefaultAgentDownloadURL()(dev version → latest, release version → versioned URL) is untested.📋 Suggested test cases
func (s *RepoTestSuite) TestDefaultAgentDownloadURL_DevVersion() { // Ensure EnvAgentURL is not set _ = os.Unsetenv(EnvAgentURL) // Mock version.GetVersion to return version.DevVersion // (requires version package to support injection or this to be an integration test) // Expected: returns AgentLatestDownloadURL } func (s *RepoTestSuite) TestDefaultAgentDownloadURL_ReleaseVersion() { // Ensure EnvAgentURL is not set _ = os.Unsetenv(EnvAgentURL) // Mock version.GetVersion to return a release version like "v1.2.3" // Expected: returns AgentDownloadBaseURL + "v1.2.3" }Note: These tests may require refactoring
DefaultAgentDownloadURLto accept a version parameter or using test-time dependency injection forversion.GetVersion().🤖 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/config/repo_test.go` around lines 27 - 40, The test suite for DefaultAgentDownloadURL() lacks coverage for the core version-based branching logic that distinguishes between dev and release versions. Add two new test methods to the RepoTestSuite: TestDefaultAgentDownloadURL_DevVersion which ensures EnvAgentURL is unset and verifies the function returns AgentLatestDownloadURL for dev versions, and TestDefaultAgentDownloadURL_ReleaseVersion which also unsets EnvAgentURL and verifies the function returns the versioned URL (AgentDownloadBaseURL plus version) for release versions. You may need to refactor DefaultAgentDownloadURL to accept a version parameter or introduce dependency injection for the version.GetVersion() call to make these tests feasible without modifying global state.
15-25: 💤 Low valueConsider using
os.LookupEnvfor more robust environment restoration.
os.Getenvreturns""for both unset variables and variables set to empty string. IfEnvAgentURLwas originally set to""(unlikely but possible),TearDownTestwill unset it instead of restoring the empty value. Usingos.LookupEnvwould correctly distinguish the two cases.♻️ More robust cleanup using LookupEnv
type RepoTestSuite struct { suite.Suite - originalEnv string + originalEnv string + originalEnvExists bool } func (s *RepoTestSuite) SetupTest() { - s.originalEnv = os.Getenv(EnvAgentURL) + s.originalEnv, s.originalEnvExists = os.LookupEnv(EnvAgentURL) } func (s *RepoTestSuite) TearDownTest() { - if s.originalEnv != "" { + if s.originalEnvExists { _ = os.Setenv(EnvAgentURL, s.originalEnv) } else { _ = os.Unsetenv(EnvAgentURL) } }🤖 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/config/repo_test.go` around lines 15 - 25, The SetupTest method uses os.Getenv which cannot distinguish between an unset environment variable and one set to an empty string, both returning "". To fix this, modify SetupTest to use os.LookupEnv instead, which returns both the value and a boolean indicating whether the variable exists. Store both the value and existence status (the boolean) in the RepoTestSuite struct. Then update TearDownTest to use this stored boolean to correctly determine whether to set the variable back to its original value (including empty strings) or unset it completely.
🤖 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.
Nitpick comments:
In `@pkg/config/repo_test.go`:
- Around line 27-40: The test suite for DefaultAgentDownloadURL() lacks coverage
for the core version-based branching logic that distinguishes between dev and
release versions. Add two new test methods to the RepoTestSuite:
TestDefaultAgentDownloadURL_DevVersion which ensures EnvAgentURL is unset and
verifies the function returns AgentLatestDownloadURL for dev versions, and
TestDefaultAgentDownloadURL_ReleaseVersion which also unsets EnvAgentURL and
verifies the function returns the versioned URL (AgentDownloadBaseURL plus
version) for release versions. You may need to refactor DefaultAgentDownloadURL
to accept a version parameter or introduce dependency injection for the
version.GetVersion() call to make these tests feasible without modifying global
state.
- Around line 15-25: The SetupTest method uses os.Getenv which cannot
distinguish between an unset environment variable and one set to an empty
string, both returning "". To fix this, modify SetupTest to use os.LookupEnv
instead, which returns both the value and a boolean indicating whether the
variable exists. Store both the value and existence status (the boolean) in the
RepoTestSuite struct. Then update TearDownTest to use this stored boolean to
correctly determine whether to set the variable back to its original value
(including empty strings) or unset it completely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f39bb99-a984-4436-a26d-4e8f374e3017
📒 Files selected for processing (32)
cmd/internal/agentcontainer/daemon.gocmd/internal/agentworkspace/logs.gocmd/internal/agentworkspace/up.gocmd/internal/container_tunnel.gocmd/internal/fleet_server.gocmd/internal/ssh_server.gocmd/internal/ssh_server_test.gocmd/workspace/ssh.gopkg/agent/agent.gopkg/agent/agent_test.gopkg/agent/delivery/kubernetes.gopkg/agent/delivery/kubernetes_test.gopkg/agent/delivery/legacy_shell.gopkg/agent/delivery/local_docker.gopkg/agent/delivery/remote_docker.gopkg/agent/inject.gopkg/agent/workspace.gopkg/config/paths.gopkg/config/repo.gopkg/config/repo_test.gopkg/devcontainer/setup.gopkg/devcontainer/setup/setup.gopkg/dotfiles/dotfiles.gopkg/driver/docker/diagnostics.gopkg/driver/docker/diagnostics_test.gopkg/driver/docker/docker.gopkg/ide/jetbrains/generic.gopkg/ide/rstudio/rstudio.gopkg/options/resolve.gopkg/ssh/server/ssh.gopkg/ssh/server/ssh_container.gopkg/tunnel/services.go
💤 Files with no reviewable changes (1)
- pkg/agent/agent_test.go
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.
Summary
When a workspace is deleted and recreated under a new UID (e.g. on a config change), the bind-mount source path was reused verbatim. Docker Desktop's macOS file-sharing VM caches
/host_mntentries by inode, so the recycled inode under the same path surfaces asbind source path does not existon the nextdocker run— reproducible failure on the seconddevsy up, while the first run always succeeds.content-<uid>) so each recreate uses a path Docker Desktop's VM has never bound before.ContentFolderimmediately after the UID-change recreate so the recompute below always picks up the new UID-suffixed path.docker host: os=… server=… storageDriver=…(once) anddocker bind: src=… exists=…(per mount) beforedocker run, so future cache-race failures are diagnosable from the log alone — the conjunction "host=exists, daemon=missing" is the structural fingerprint.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests