Replace git worktree with git clone for session isolation#157
Replace git worktree with git clone for session isolation#157kirich1409 merged 1 commit intomainfrom
Conversation
Worktrees share the parent .git directory, causing lock file conflicts between concurrent sessions. Full clones give each session an independent .git — no shared state, simpler lifecycle management. - ProjectManager: add_worktree() -> clone_for_session() (local clone) - ProjectManager: remove_worktree() -> remove_session_clone() (rm -rf) - CloudSessionManager: sessions/<uuid> instead of worktrees/<uuid> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the runner’s cloud session isolation strategy by switching from git worktree to creating a full local git clone per session, aiming to eliminate shared .git state/lock contention between sessions.
Changes:
- Replace per-session
git worktree add/removewithgit clone+ directory deletion APIs inProjectManager. - Update cloud session creation to create a per-session clone directory and pass it into the container environment.
- Refresh docs/comments to reflect the new isolation approach.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| runner/src/project_manager.rs | Replaces worktree management with clone_for_session() and remove_session_clone() helpers. |
| runner/src/cloud_session_manager.rs | Creates a per-session clone directory instead of a worktree and uses it as the workspace path for the session container. |
| runner/src/main.rs | Updates init comment about the data-dir subdirectories (workspaces vs session clones). |
Comments suppressed due to low confidence (1)
runner/src/cloud_session_manager.rs:176
- The session clone path isn’t persisted anywhere (
CreateSession.work_diris left empty), which makes later cleanup/recovery impossible and leavesProjectManager::remove_session_cloneeffectively unused. Consider storingclone_dirintowork_dir(and using that field for cleanup on stop/delete/reconcile) so clones don’t accumulate on disk.
// 4. Create session record in DB (state = Starting).
let effective_image = image
.unwrap_or_else(|| self.docker.default_image())
.to_owned();
let profile_name = profile.unwrap_or("default").to_owned();
let db_session = self
.store
.create_session(store::CreateSession {
project_id: project_id.to_owned(),
branch: branch.to_owned(),
image: effective_image.clone(),
profile: profile_name.clone(),
work_dir: String::new(),
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| let data_dir = config::Config::default().data_dir; | ||
| // "workspaces" dir holds git worktrees for session isolation (legacy name, not a domain entity) | ||
| // "workspaces" dir holds session clones for isolation (legacy name, not a domain entity) |
There was a problem hiding this comment.
This comment says the top-level <data_dir>/workspaces directory “holds session clones”, but session clones are created under <data_dir>/projects/<project_id>/sessions/<uuid> in CloudSessionManager. Please update the comment (or align the clone location) so the initialization docs match runtime behavior.
| // "workspaces" dir holds session clones for isolation (legacy name, not a domain entity) | |
| // Session clones are stored under per-project paths in "projects/.../sessions/...". | |
| // Keep the top-level "workspaces" directory for legacy compatibility and other workspace data. |
| // 3. Create isolated git clone for the branch. | ||
| let project_dir = self.project_manager.projects_dir().join(project_id); | ||
| // Use a unique subdirectory per session to avoid collisions. | ||
| let session_worktree_id = uuid::Uuid::new_v4().to_string(); | ||
| let worktree_dir = project_dir.join(format!("worktrees/{session_worktree_id}")); | ||
| let session_clone_id = uuid::Uuid::new_v4().to_string(); | ||
| let clone_dir = project_dir.join(format!("sessions/{session_clone_id}")); | ||
|
|
||
| self.project_manager | ||
| .add_worktree(&project_dir, branch, &worktree_dir) | ||
| .clone_for_session(&project_dir, branch, &clone_dir) | ||
| .await?; |
There was a problem hiding this comment.
clone_dir is created as <project_dir>/sessions/<uuid>, but git clone won’t create missing intermediate directories. If <project_dir>/sessions doesn’t exist, session creation will fail with “could not create leading directories”. Consider create_dir_all(clone_dir.parent().unwrap()) (or choose a base directory that is guaranteed to exist) before calling clone_for_session.
| let output = Command::new("git") | ||
| .args([ | ||
| "-C", | ||
| &project_dir.to_string_lossy(), | ||
| "worktree", | ||
| "add", | ||
| &worktree_dir.to_string_lossy(), | ||
| "clone", | ||
| "--branch", | ||
| branch, | ||
| &project_dir.to_string_lossy(), | ||
| &clone_dir.to_string_lossy(), | ||
| ]) |
There was a problem hiding this comment.
clone_for_session invokes git clone into clone_dir, but it doesn’t ensure the parent directory exists. Since callers may pass a nested path (e.g. .../sessions/<id>), consider creating clone_dir.parent() with tokio::fs::create_dir_all first and returning a clear error if that fails.
| tokio::fs::remove_dir_all(clone_dir) | ||
| .await | ||
| .map_err(|e| RelayError::Git { | ||
| message: format!("failed to spawn git worktree remove: {e}"), | ||
| stderr: String::new(), | ||
| })?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); | ||
| return Err(RelayError::Git { | ||
| message: format!( | ||
| "git worktree remove failed (exit {:?})", | ||
| output.status.code() | ||
| "failed to remove session clone at {}: {e}", | ||
| clone_dir.display() | ||
| ), | ||
| stderr, | ||
| }); | ||
| } | ||
| stderr: String::new(), | ||
| })?; |
There was a problem hiding this comment.
remove_session_clone wraps filesystem deletion failures in RelayError::Git. Since RelayError::Git maps to HTTP 400, a host filesystem issue (permissions, IO errors) would be reported as a client error. Consider introducing a dedicated filesystem/IO error variant (or mapping this case to an internal error) so operational failures surface correctly.
Merge with main pulled in two commits that invert earlier text: - #156 (f44bbd2): first session now auto-created after clone succeeds; DELETE of last session returns 400 - #157 (1d883a7): session isolation switched from git worktree back to independent git clone per session (sessions/<uuid>) Updates: - D4: restate as 1 project = 1 clone; 1 session = 1 independent clone (+ container). Alternative becomes worktree (rejected due to shared .git lock conflicts) - Session.work_dir comment: session clone directory, not worktree - Invariant: restore "Project without sessions is invalid"; document auto-create flow, retry path, and DELETE-last-session rejection - Isolation: clone-per-session with own .git, RELAY_WORKSPACE_DIR exposes it to the container Addresses copilot round 2 on PR #131.
* Align §7 Data Model and D4 with implementation reality - Add Terminal entity (1:N from Session), added in Wave 3b - Add default_image to Project, work_dir to Session - Remove stale fields (claude_session_id, current_exec_id) - Update SessionProfile fields to match code - Document invariant: project without sessions is invalid - Document session isolation: clone (cloud) vs worktree (local) - Update D4: cloud = 1 session = 1 clone = 1 container Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Align §7 and D4 with runner store fields and worktree isolation - Project: add local_path, clone_error, deleted_at; drop generic error - Session: rename error → error_reason; add deleted_at - Invariant: relax "project must have a session" — Cloud clones in background without auto-creating a session, so an empty Project is a valid transitional state - Isolation: Cloud uses one git clone per project and one git worktree (plus one container) per session — not a full clone per session - D4: restate as "1 project = 1 clone; 1 session = 1 worktree + 1 container" and adjust alternatives accordingly Addresses copilot review comments on PR #131. * Re-align §7 and D4 with post-#156/#157 main state Merge with main pulled in two commits that invert earlier text: - #156 (f44bbd2): first session now auto-created after clone succeeds; DELETE of last session returns 400 - #157 (1d883a7): session isolation switched from git worktree back to independent git clone per session (sessions/<uuid>) Updates: - D4: restate as 1 project = 1 clone; 1 session = 1 independent clone (+ container). Alternative becomes worktree (rejected due to shared .git lock conflicts) - Session.work_dir comment: session clone directory, not worktree - Invariant: restore "Project without sessions is invalid"; document auto-create flow, retry path, and DELETE-last-session rejection - Isolation: clone-per-session with own .git, RELAY_WORKSPACE_DIR exposes it to the container Addresses copilot round 2 on PR #131. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
add_worktree()/remove_worktree()withclone_for_session()/remove_session_clone()inProjectManagergit clonefrom the project's local repo instead of a worktree — independent.gitdirectory, no shared lock filesrm -rfinstead ofgit worktree removeCloses #140
Test plan
cargo fmt --checkcleancargo clippy --all-targets --all-features -- -D warningscleancargo test— 189 tests pass🤖 Generated with Claude Code