fix(tmux): stabilize server cwd and tolerate async cd on pane cwd verify - #3098
Merged
Conversation
Consolidates the tmux poisoned-cwd spawn failure fix (issue #2775 family). - execRunner.Run now pins cmd.Dir to os.TempDir(), so the tmux CLI's first invocation (which auto-starts the persistent tmux server) never inherits a daemon cwd that a later Squirrel/ShipIt auto-update can delete. - verifyPaneWorkingDirectory now retries up to 5 times, 50ms apart, honoring ctx cancellation via select. Measured live: #{pane_current_path} sampled immediately after new-session was stale, and the same probe sampled 50ms later was already correct, because buildLaunchCommand's cd '<workspace>' || exit; guard only corrects the pane once its shell actually runs. The prior single-shot check lost that race every time and turned a spawn that was going to succeed into a hard failure. Reuses the existing sameDirectory helper rather than adding a parallel one. - gitworktree: a registered worktree whose directory no longer exists on disk (registration and DB row survive, directory does not) is now detected via worktreeDirMissing, its stale git registration pruned, and a fresh worktree materialized before the runtime launches, in both the Create-reuse and Restore paths. - session service toAPIError maps the new ports.ErrRuntimeWorkspaceCwdMismatch sentinel to a typed WORKSPACE_CWD_MISMATCH apierr naming both the observed and wanted paths, instead of falling through to an opaque 500. Supersedes #2984 and #2776 (both authored by this contributor, being closed in favor of this PR). Credits #3027, which independently reached the same cmd.Dir pin plus a retry loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 25, 2026
harshitsinghbhandari
requested review from
illegalcall and
neversettle17-101
and removed request for
illegalcall
July 25, 2026 13:57
- workspace.go Restore: recreate a stale worktree registration on the registration's OWN branch, not cfg.Branch. The recreate path fell through to validateBranch/addWorktree with cfg.Branch even though rec (holding the registration's real branch) was still in scope, so a session sitting on a child branch (e.g. ao/<id>/gh-pages-landing) was silently checked out on cfg.Branch (typically root) instead, looking like lost work. This is the exact agent-orchestrator-78 case. Added TestRestoreRecreatesOnRegisteredBranchNotCfgBranch. - tmux.go verifyPaneWorkingDirectory: make the cwd-mismatch sentinel error sticky across retries. A later transient probe failure could overwrite an already-observed mismatch, losing the classifiable ports.ErrRuntimeWorkspaceCwdMismatch error and regressing Fix 4 back to a bare, unclassifiable 500. Added TestVerifyPaneWorkingDirectoryKeepsMismatchErrorAfterLaterProbeFailure. - gitworktree: renamed worktreeDirMissing to pruneIfWorktreeDirMissing to signal that it mutates (calls git worktree prune), and documented that the prune is repo-wide and why that is an accepted, self-healing tradeoff here. - tmux.go: replaced time.NewTimer + Stop with time.After in the retry select; Go 1.23+ collects unreferenced timers so Stop bought nothing. - tmux_test.go: extracted a countCalls helper used by the three tests that were each hand-rolling the same call-counting loop. - Added TestExecRunnerRunsFromStableDir, a direct test of execRunner.Run against the real exec.Cmd (not the fakeRunner test seam), asserting its cwd resolves to os.TempDir(). This was previously untested: the unit suite never exercises execRunner.Run, and the PR body's coverage claim for this line was inaccurate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
illegalcall
reviewed
Jul 25, 2026
illegalcall
reviewed
Jul 25, 2026
…tion Addresses two confirmed findings from review on #3098, both against real git. - pruneIfWorktreeDirMissing called the repo-wide `git worktree prune` to clear a stale registration for one path. Verified against real git: two worktrees on distinct branches, both directories deleted, ONE `git worktree prune` removes BOTH registrations. Restoring session A then silently wiped session B's registration too, so B's later Restore found no record and fell back to cfg.Branch instead of its own recorded branch, reintroducing the exact wrong-branch bug recreateBranch was added to prevent, this time for a sibling session that was never touched. Switched to a target-specific `git worktree remove --force <path>` (already available as worktreeForceRemoveArgs), which only removes the one registration; verified this succeeds on a missing directory the same way prune did. Added TestWorkspaceIntegrationRestoreRecreatesSiblingsIndependently (real git): two sessions, both directories deleted, restored sequentially, each recreated on its own recorded branch. - pruneIfWorktreeDirMissing did not check the registration's `locked` attribute (parsed by parseWorktreePorcelain but previously unused). Verified against real git: `git worktree lock` plus a deleted directory survives `git worktree prune` (locked registrations are never pruned), and both `git worktree add` and `git worktree remove --force` at that path then fail with an opaque "missing but locked worktree" error. Now checks rec.Locked before touching the registration and returns the new typed ports.ErrWorkspaceLocked (adapter-local alias ErrWorktreeLocked, following the existing ports.ErrWorkspace* pattern) instead of attempting recovery, mapped in toAPIError to WORKSPACE_LOCKED. Added TestWorkspaceIntegrationRestoreLockedMissingWorktreeIsTypedError (real git): a locked worktree whose directory is gone, asserting the typed error and that worktree add was never attempted. Both new tests were confirmed to fail against the pre-fix code before this commit, then pass after it, to rule out passing for the wrong reason. Also updated the existing fake-git tests (TestCreatePrunesMissingRegistered..., TestRestorePrunesMissingRegistered..., TestRestoreRecreatesOnRegisteredBranch...) to stub the new target-specific remove call instead of the old repo-wide prune, and assert prune is no longer used on this path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
illegalcall
reviewed
Jul 26, 2026
Stale-registration recovery used `git worktree remove --force <path>`, a check-then-delete against the earlier os.Stat. Two concurrent restores of the same session (RestoreWithMode has no per-session guard) can both observe the registered directory as missing; once the first recreates the worktree, the second's stale --force decision deletes it, uncommitted agent work included. Recover with `git worktree add --force` instead, git's own documented override for "<path> is a missing but already registered worktree", so nothing is removed or pruned first. Verified against git 2.54: --force re-registers a missing-but-registered path, refuses outright when the directory exists and is non-empty (so a lost race fails loudly instead of destroying work), leaves every sibling registration untouched, and still refuses a missing-but-locked registration, which keeps ErrWorkspaceLocked reachable. It is never passed twice. pruneIfWorktreeDirMissing becomes the pure observation registeredWorktreeDirMissing, and the two `worktree add` stale-registration fallbacks now retry with --force rather than the repo-wide `git worktree prune` that would also drop sibling sessions' registrations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harshitsinghbhandari
added a commit
to harshitsinghbhandari/agent-orchestrator
that referenced
this pull request
Jul 26, 2026
…tion Addresses two confirmed findings from review on AgentWrapper#3098, both against real git. - pruneIfWorktreeDirMissing called the repo-wide `git worktree prune` to clear a stale registration for one path. Verified against real git: two worktrees on distinct branches, both directories deleted, ONE `git worktree prune` removes BOTH registrations. Restoring session A then silently wiped session B's registration too, so B's later Restore found no record and fell back to cfg.Branch instead of its own recorded branch, reintroducing the exact wrong-branch bug recreateBranch was added to prevent, this time for a sibling session that was never touched. Switched to a target-specific `git worktree remove --force <path>` (already available as worktreeForceRemoveArgs), which only removes the one registration; verified this succeeds on a missing directory the same way prune did. Added TestWorkspaceIntegrationRestoreRecreatesSiblingsIndependently (real git): two sessions, both directories deleted, restored sequentially, each recreated on its own recorded branch. - pruneIfWorktreeDirMissing did not check the registration's `locked` attribute (parsed by parseWorktreePorcelain but previously unused). Verified against real git: `git worktree lock` plus a deleted directory survives `git worktree prune` (locked registrations are never pruned), and both `git worktree add` and `git worktree remove --force` at that path then fail with an opaque "missing but locked worktree" error. Now checks rec.Locked before touching the registration and returns the new typed ports.ErrWorkspaceLocked (adapter-local alias ErrWorktreeLocked, following the existing ports.ErrWorkspace* pattern) instead of attempting recovery, mapped in toAPIError to WORKSPACE_LOCKED. Added TestWorkspaceIntegrationRestoreLockedMissingWorktreeIsTypedError (real git): a locked worktree whose directory is gone, asserting the typed error and that worktree add was never attempted. Both new tests were confirmed to fail against the pre-fix code before this commit, then pass after it, to rule out passing for the wrong reason. Also updated the existing fake-git tests (TestCreatePrunesMissingRegistered..., TestRestorePrunesMissingRegistered..., TestRestoreRecreatesOnRegisteredBranch...) to stub the new target-specific remove call instead of the old repo-wide prune, and assert prune is no longer used on this path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
illegalcall
reviewed
Jul 26, 2026
…h form `git worktree add -b <branch> <path> <base>` creates refs/heads/<branch> before it validates the target path, so an attempt that fails on a stale registration still leaves the branch behind. Retrying the same -b form with --force then failed with "a branch named ... already exists", so workspace-project stale-registration recovery never actually recovered. Retry on the existing-branch form `worktree add --force <path> <branch>` when the ref is present, which checks out the branch the failed attempt just created at the same base. createWorkspaceProjectRepo also gains the same up-front stale-registration check addWorktree already had, so the ordinary case is handled by the first add and never creates a stray ref at all.
Real-git regression (TestWorkspaceIntegrationAddNewBranchRecoversStaleRegistration) for the stale-registration retry, plus a fake test that now models the failed `-b` add's branch-creation side effect instead of treating it as side-effect-free, which is what let the broken retry form pass before.
Conflict in gitworktree/workspace_test.go: main replaced the sh-based `exit 1` fixtures with the Windows-safe exitStatusOne helper while this branch rewrote the workspace-project stale-registration tests. Kept this branch's tests and adopted exitStatusOne in them.
…ailed Two review findings on #3098. - execRunner.Run pinned cmd.Dir to os.TempDir(), which returns $TMPDIR verbatim without checking it exists. A stale or bogus TMPDIR then failed EVERY tmux command with "chdir <dir>: no such file or directory" (measured), taking the whole runtime down for the same dead-cwd reason #2775 did, just moved. stableRunDir stats the candidates and degrades: os.TempDir(), then the home directory, then the empty string, which leaves cmd.Dir unset and inherits the daemon's cwd. That last case is the pre-fix behavior and only risks the poisoned-server race, which verifyPaneWorkingDirectory's retry already tolerates, so it is strictly better than not running tmux at all. - addNewBranchWorktree discarded the retry's error and returned the original, which names the condition recovery was FOR ("is a missing but already registered worktree"), not why recovery failed. The two differ in the cases that matter: "'<path>' already exists" means a concurrent restore materialized the worktree first and this one lost the race, and "missing but locked" means the registration acquired a lock after the pre-check. Join both, so errors.Is keeps working for each. Also documents why Create's stale-registration recreate uses cfg.Branch while Restore's uses the registration's own branch: Restore re-attaches to a live session whose branch may have moved past what AO recorded, Create materializes a new session where a registration at that path is a leftover from a prior one. Both new tests were confirmed to fail against the pre-fix code first: - TestExecRunnerFallsBackWhenTempDirMissing (chdir error) - TestAddNewBranchWorktreeRecoveryFailureReportsBothErrors (drops "already exists") go build, go vet, gofmt clean; tmux, gitworktree and session packages ok. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
illegalcall
approved these changes
Jul 27, 2026
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
Consolidated fix for the tmux poisoned-cwd spawn failure (issue #2775 family): a running tmux server can have its cwd pinned inside a Squirrel/ShipIt auto-update staging directory that the update then deletes, and pane cwd verification could fail a spawn that was actually recovering fine.
Evidence, measured live on a real poisoned server (2026-07-25):
tmux new-session -c <a directory that definitely exists>was silently ignored; the pane landed in the dead ShipIt directory.-ccorrectly, confirming the poison lives in the server's own cwd, not tmux's-chandling in general.buildLaunchCommand'scd '<workspace>' || exit;guard DOES correct the pane, but only once the shell actually runs it:#{pane_current_path}sampled at t+0s (immediately afternew-session) was stale, and the same probe sampled at t+0.05s was already correct.verifyPaneWorkingDirectory(added by fix: stabilize daemon spawn working directories #2871) sampled#{pane_current_path}exactly once, immediately afternew-session, so it lost that race 100% of the time on a poisoned server and every spawn failed with an opaque HTTP 500 with no message.The poisoned server alone is harmless: the
cdguard recovers the pane every time. The single-shot verification is what converted a working, self-correcting spawn into a hard failure.Changes
Pin the tmux CLI cwd (
backend/internal/adapters/runtime/tmux/tmux.go,execRunner.Run): setscmd.Dir = os.TempDir()before running. The first tmux CLI call auto-starts the persistent tmux server, which inherits this process's cwd for its entire lifetime;os.TempDir()outlives app bundle swaps and ShipIt staging dirs, so the server can never be pinned to a path an update will delete.Retry pane-cwd verification (same file,
verifyPaneWorkingDirectory): retries up to 5 times, 50ms apart, with aselectonctx.Done()so cancellation is honored, returning the last error if every attempt fails. Extends the existing function and reuses the existingsameDirectoryhelper (already resolves symlinks and handles empty strings) rather than introducing a parallel implementation. The final error names both the observed pane path and the wanted workspace, and says the worktree may be missing or the tmux server may be pinned to a stale directory. The cwd-mismatch error is sticky across retries: a later transient probe failure (a one-off tmux CLI error, not a mismatch) never overwrites an already-observed mismatch, so the typed sentinel from Fix 4 always survives to the caller.Recreate missing registered worktrees (
backend/internal/adapters/workspace/gitworktree/workspace.go):registeredWorktreeDirMissingdetects a git worktree registration whose directory no longer exists on disk and lets the caller materialize a fresh worktree at the same path. Applied to both theCreate-reuse path (existingWorktree, matching fix(workspace): recreate missing registered worktrees #2776's approach) and theRestorepath, since both had the identical bug shape:if rec, ok := findWorktree(records, path); ok { return ... }without ever checking the directory actually exists. The real observed case (sessionagent-orchestrator-78) still had its branches and worktree registration plus its DB row, but no worktree directory, socd '<workspace>' || exitexited instantly and restore failed silently, which is exactly theRestorecode path. On the recreate path,Restorechecks out the registration's own branch (notcfg.Branch): AO workers routinely sit on a child branch (ao/<id>/<feature>), not the root branch AO would otherwise pass throughcfg.Branch, and recreating on the wrong branch would silently strand the session's work.git worktree add --force <path> <branch>, git's own documented override for "<path>is a missing but already registered worktree" (that failure's own hint is "use 'add -f' to override"), soregisteredWorktreeDirMissingis a pure observation (stat plus theLockedcheck) with no mutation. Measured against real git 2.54:--forcere-registers a path whose registration outlived its directory and leaves every sibling registration untouched; on a path that exists and is non-empty it refuses outright (fatal: '<path>' already exists) even with-f -f, leaving the existing files intact; and a single--forcestill refuses a missing-but-locked registration. It is never passed twice.git worktree pruneremoves sibling registrations (verified against real git: with two worktrees on distinct branches both missing their directories, one prune removed BOTH, so restoring one session could silently wipe a sibling's registration and reset it to the wrong branch on its own next restore), and the target-specificgit worktree remove --forceis check-then-delete against the earlieros.Stat, so with two concurrent restores of the same session (RestoreWithModehas no per-session guard) the second one's stale decision deletes the live worktree the first just recreated, uncommitted agent work included.add --forcehas neither problem: it touches only this path's registration, and a restore that loses the race fails loudly instead of destroying work.worktree addstale-registration fallbacks (inaddWorktreeandcreateWorkspaceProjectRepo) that recovered via the repo-widegit worktree prune: they now retry with--force, so the sibling-registration problem cannot recur through those paths either.git worktree lock) is not treated as recoverable. Verified against real git:git worktree pruneleaves a locked-but-missing registration in place, andgit worktree add(with or without a single--force) andgit worktree remove --forceat that path all fail with an opaque "missing but locked worktree" git error.registeredWorktreeDirMissingchecks the registration's already-parsedLockedattribute first and returns the new typedports.ErrWorkspaceLocked(adapter-local aliasErrWorktreeLocked, following the existingports.ErrWorkspace*pattern) instead of attempting recovery. Locking is an explicit operator signal not to touch a worktree, so-f -fis deliberately never used.Stop masking this as a bare 500 (
backend/internal/service/session/service.go,toAPIError): adds sentinelsports.ErrRuntimeWorkspaceCwdMismatchandports.ErrWorkspaceLocked(following the existingports.ErrWorkspace*pattern) and maps them to typedWORKSPACE_CWD_MISMATCH/WORKSPACE_LOCKEDconflict apierrs carrying the real message, instead of falling through to an opaque 500INTERNAL_ERRORwith no message.Prior art, reuse and credit
cmd.Dirchange and useful tests; its verification half is superseded by merged fix: stabilize daemon spawn working directories #2871, so that half was dropped here. Closing in favor of this PR.Createpath, ported here and extended toRestore. Closing in favor of this PR.cmd.Dirpin plus a retry loop. Credit due; this PR supersedes it with the additional worktree-recreation and typed-error fixes.os.Stat-then-worktree remove --forcecheck-then-delete destroying a worktree recreated by a concurrent restore. All three are fixed above and each has a real-git regression test.Fixes #2775. Related: #2871, #2984, #2776, #3027.
Tests
New/updated, all passing:
TestExecRunnerRunsFromStableDir: direct test ofexecRunner.Run(the real one, not the fakeRunner test seam every other tmux test in the package uses) against a realexec.Cmd, asserting its cwd resolves toos.TempDir(). This is the only test that runs the actualcmd.Dirline and would catch a regression there.TestVerifyPaneWorkingDirectoryRetriesUntilMatch: pins the retry behavior, verification succeeds when the first sample is stale and a later sample matches.TestVerifyPaneWorkingDirectoryHonorsCancellation: the retry loop'sctx.Done()select actually aborts a pending retry.TestVerifyPaneWorkingDirectoryKeepsMismatchErrorAfterLaterProbeFailure: a mismatch on an earlier attempt survives a probe failure on a later attempt, so the caller still gets the typed sentinel.TestCreateDestroysAndReturnsErrorWhenPaneCWDDoesNotMatch: updated for the retry sequence (5 stale samples), asserts the wrapped sentinel and exactlypaneCwdVerifyAttemptsverification calls.TestCreateRecreatesMissingRegisteredWorktreeWithForce/TestRestoreRecreatesMissingRegisteredWorktreeWithForce: stale-registration recovery on theCreateandRestorepaths (fake git), asserting theworktree add --forcecall and, via a sharedassertNoDestructiveRegistrationCleanuphelper, that neitherworktree prunenorworktree removeis used to recover.TestCreateWorkspaceProjectRepoRetriesStaleRegisteredWorktreeWithForce: the workspace-project fallback retries with--forceinstead of the repo-wide prune it used to run.TestCommandArgs: newadd existing forced/add new forcedcases pin that--forcelands in the right position and is passed exactly once.TestRestoreRecreatesOnRegisteredBranchNotCfgBranch: regression test for the branch bug in Fix 3, a stale registration recorded on a non-root branch is recreated on THAT branch, not oncfg.Branch.TestWorkspaceIntegrationRestoreRecreatesSiblingsIndependently(real git): two sessions in the same repo, both worktree directories deleted out of band, restored sequentially; each is recreated on its own recorded branch and the second's registration survives the first's restore. Confirmed to fail against the pre-fix repo-wide-prune code before landing this fix.TestWorkspaceIntegrationRestoreLockedMissingWorktreeIsTypedError(real git): a locked worktree whose directory is deleted; assertsports.ErrWorkspaceLockedand thatgit worktree addwas never attempted. Confirmed to fail against the pre-fix code (surfaces the raw git "missing but locked worktree" error instead) before landing this fix.TestWorkspaceIntegrationRestoreDoesNotDestroyWorktreeRecreatedMidRecovery(real git): the concurrent-restore finding, forced deterministically instead of raced. The hooked runner recreates the worktree at the same path and writes an uncommitted sentinel file immediately before the first registration-mutating git commandRestoreissues, which is exactly the window between the stat and the cleanup; the test asserts the sentinel survives and that a restore that lost the race never reports success for a worktree it did not materialize. Confirmed to fail against the previous commit (uncommitted work in the concurrently recreated worktree did not survive stale-registration recovery) before landing this fix.TestToAPIErrorMapsWorkspaceBranchSentinels: new cases forWORKSPACE_CWD_MISMATCHandWORKSPACE_LOCKED.Verification run from
backend/:go build ./...: cleango vet ./...: cleangofmt -l: clean on all touched filesgo test -race ./internal/adapters/runtime/tmux/... ./internal/adapters/workspace/gitworktree/... ./internal/service/session/... ./internal/session_manager/... ./internal/ports/...: all packagesok. Note: running these five packages together under-racewith default package parallelism can intermittently flake one pre-existing, untouched real-tmux integration test (TestRuntimeIntegrationSupervisedExitKeepsInteractiveShell, fixed 5s wall-clock deadlines under race-instrumented CPU contention from the other packages running concurrently); it passes reliably alone, in the full non-race suite, and with-race -p 1(sequential package execution) across all five packages.go test -count=1 ./...: all 99 packagesok.golangci-lint run ./internal/adapters/workspace/gitworktree/...: 0 issues.