#1: implement worktree and clone workspace strategies #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue/1-workspace-strategies"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #1.
What
docs/workspaces.md §4 names three workspace strategies but only
sharedwas implemented;
worktreeandclonewere refused at set time. Thisimplements the git-backed strategies end to end.
db
worktree/clonebecome valid strategies; the configuration refuses agit-backed strategy without a non-empty absolute base at set time
(§4), in both directions (setting the strategy, and clearing/relativizing
the base of a git-backed project).
ResolveWorkspacenow takes the session id and derives the per-sessionpath
<base>/.bob/wt/s<id>/<base>/.bob/clone/s<id>— parallelsessions of one project never share a mutating tree (INV-5), and a
worktree and a clone of the same session cannot collide.
toolhost
CreateWorkspace/DestroyWorkspace—git worktree add -B bob/s<id>(one branch per session;
-Bcreate-or-reset self-heals the leftover abest-effort destroy can leave),
git clone,git worktree remove/rm -rf.destroyed without force; the dirty set is reported (
git status --porcelain).a session that died without cleanup, or rebinds to the same project,
re-attaches to its private tree (resumability). The reuse check is
precise:
git worktree listmembership, so a directory that is aworktree of some other repo is refused, never adopted.
rpc / server
workspace.create/workspace.destroymethods (§11),answered by every tool host (headless host, TUI, REPL — the caller is
the workspace host for
host:"caller"projects).created — or whose pinned host is absent — fails loudly at bind, never
as a tool failure mid-turn.
refuses the delete and names what is dirty (
session rm --forcediscards). Teardown degrades to a recorded leak when the host is
unreachable or the destroy fails — a session row is never made
undeletable by its workspace.
host:"caller"case dispatches back to the same connection; a synchronous dispatch
deadlocks the read loop that must read the reply).
positive tool-call (DB) and drain ids in the response router, so a
workspace response can never be misrouted into a pending tool call.
— dirty/unreachable degrades to a recorded leak, never a refusal that
strands the session).
subagents
worktree/clonechild gets its own private workspace on its toolhost before its first turn (docs/subagents.md §6), wired via hooks from
the rpc.Server; a failed spawn rolls the workspace back.
Deviations from the normative doc (with consequences)
configuration must reject a non-git base at set time; the server has no
filesystem (INV-8), so the config-level check is base set + absolute at
set time, and the actual git check runs at workspace creation on the
host, failing the bind loudly. Consequence: a misconfigured git-backed
project is caught at bind rather than at
project set.workspace.status, orphan reporting intoolhost.list, claim/takeover(§6) are not built. They are the monitoring surface of the deferred GC
story (§13); the dirty-set reporting in
workspace.destroycovers theINV-9 need. Consequence: orphaned worktrees (crashed host, project
deleted) are recorded in the server log but not surfaced in the
monitoring UI until that lands.
session rmgains--forceto satisfy INV-9's "explicitconfirmation" — the doc does not name the mechanism.
Tests
worktree registration, dirty guards naming the file, forced discard,
reuse,
-Bself-heal, non-git/missing/occupied base refusals, sharednever destroyed).
workspace and routes tool cwd into it; delete destroys tree + branch;
bind refuses without a registered host; dirty delete refuses and names
the file, force discards;
host:"caller"bind/delete (the async readloop); subagent children get distinct private workspaces; rebind-away
destroys the old tree.
ParseSessionArgs--forceparsing.Mutation-checked: removing the INV-9 refusal, the bind-time create, the
delete-time destroy, the rebind teardown, or the subagent creation each
fails the test named after that behaviour.
make check(fmt + vet + test) andmake buildpass.Verdict: CHANGES REQUESTED
Review of #2 (implement the
worktreeandcloneworkspace strategies) against issue #1 and docs/workspaces.md §4–§5, §11. The core of the issue is implemented end-to-end and is well tested; there is one correctness defect in the new rebind path that I reproduced, plus two cheaper findings.Verified
db/projects.go,AddProject+UpdateProjectField); per-session paths<base>/.bob/wt/s<id>/<base>/.bob/clone/s<id>are derived indb.ResolveWorkspace(distinct per session and per strategy — no collision space); create at bind / destroy at delete are dispatched over the newworkspace.create/workspace.destroymethods, answered by the headless host, TUI, REPL and the caller itself forhost:"caller"; the INV-9 dirty guard withsession rm --forceworks; subagent children get private workspaces before their first turn with rollback on failed spawn.make buildandmake checkboth pass (exact output at the bottom). Full suite green, including uncached runs of the workspace suites:TestWorktreeBindCreatesWorkspace,TestWorktreeBindRequiresHost,TestSessionDeleteDirtyGuard,TestHostCallerWorktreeBind,TestSubagentWorktreeOwnWorkspace,TestRebindAwayDestroysWorktree,TestCreateDestroyWorktree,TestDestroyWorktreeDirtyGuard,TestCreateDestroyClone,TestCreateWorkspaceRejectsBadBases,TestDestroySharedRefuses,TestResolveWorkspace,TestParseSessionArgs.toolhost/workspace.goDestroyWorkspace(worktree case, theelse if len(d) > 0 && !forceguard) →TestDestroyWorktreeDirtyGuardFAILS atworkspace_test.go:121("dirty destroy without force must refuse"). Restored. (2) Disabled the bind-timeworkspace.createdispatch inrpc/server.gobindSession→TestWorktreeBindCreatesWorkspaceFAILS atworkspaces_test.go:151("workspace not created on the host") andTestHostCallerWorktreeBindFAILS atworkspaces_test.go:329. Restored. Both tests pin their behaviour.Findings, by severity
1. [Blocking]
rpc/server.go:737-751— a failed rebind destroys the old workspace first and strands the session on a nonexistent path.bindSessiondestroys the previous private workspace (lines 737–751) before creating/validating the new one (lines 752–760). If the new workspace cannot be created — host not registered, base not a git repo, base missing — the bind refuses loudly, but the old tree is already gone andSetSessionBindingnever ran, so the session keeps its old binding pointing at a destroyed directory. Every subsequent tool call then fails with a nonexistent cwd, and the model sees a corrupt binding to work around.Reproduced with a throwaway test (removed after): bind session to
wt-proj(host registered, base A), then rebind towt-proj2(worktree, base B, hostno-such-host). The rebind refused with "not registered" and the session remained bound towt-projwith the old path — butos.Stat(<baseA>/.bob/wt/s1)returned "no such file or directory". The old tree was destroyed despite the failed rebind.This contradicts the PR's own invariant ("teardown degrades to a recorded leak, never to a refusal that strands the session") — here it is the teardown itself that strands the session, and it violates the "checks for both side effects before performing the first" rule the change otherwise follows. Fix: create/validate the new workspace before destroying the old one. Nothing is lost: a failed old-tree destroy already degrades to a recorded leak by design, so with create-first you get — new create fails → old tree intact; new create succeeds, old destroy fails → logged leak and the rebind proceeds. The existing
TestRebindAwayDestroysWorktreeonly covers the success path (rebind to a shared project, which has no create step), which is why this interleaving is untested.2. [Medium]
rpc/server.go:941— delete resolves the workspace strategy from the project's current config, not the one the workspace was created under.workspaces.md §10:
set strategyapplies only to workspaces created after; existing sessions keep theirs. ButdestroySessionWorkspacereadsproj.Strategytoday: a session bound underworktreewhose project was switched tosharedleaks its private tree silently on delete (line 941 returns early, no log); the reverse — a session bound undersharedwhose project switched toworktree— makes delete refuse with a false INV-9 error on the project base itself (the "workspace" being guarded is not a private tree, andgit worktree removeon the main worktree cannot succeed anyway). The strategy should travel with the binding (or be derivable from the path's.bob/wtvs.bob/cloneprefix).3. [Low]
toolhost/workspace.go:92-95— clone reuse adopts any git repo at the path; the worktree sibling refuses foreign trees.The worktree reuse check is precise —
isWorktreeOf(toolhost/workspace.go:61-64) verifies membership in the base's worktree list, and the PR body advertises exactly that ("a directory that is a worktree of some other repo is refused, never adopted"). The clone reuse check is onlyisGitRepo(lines 92–95): any git repository sitting at<base>/.bob/clone/s<id>is silently adopted as the session's workspace. For symmetry the clone check should verify the tree is actually a clone of base (e.g. its origin points at base), or refuse any git repo that is not.4. [Note, doc-vs-code, not blocking]
db/projects.goDeleteProjectdoes not unbind bound sessions or destroy their workspaces, while workspaces.md §10 says "delete project → bound sessions are unbound and their workspaces guarded (INV-9)". Consequence: sessions bound to a deleted worktree project leak their trees, recorded only as a warning when the session is later deleted. This matches the documented "recorded leak" degradation, so it does not block — but the §10 row is unmet and should be implemented or called out in the deviations list.Exact check output
Summary
Issue #1's core — both git-backed strategies working end-to-end — is implemented correctly and is genuinely well tested (the claimed mutation checks hold for the two I re-ran). Finding 1 is a real defect in the new code on a plausible path (rebinding to a misconfigured git-backed project corrupts the session's existing binding); findings 2 and 3 are concrete and cheap. Once 1 is fixed and 2–3 are addressed, this is an accept.
Verdict: CHANGES REQUESTED
Re-review of PR #2 at head
5a5907f(matchesorigin/issue/1-workspace-strategies; working tree clean throughout — every mutation below was reverted and the tree re-verified) against the four findings of comment 4006. All four are genuinely fixed, and each is pinned by a test I re-mutated myself (exact failing lines below). The re-review found one new gap in the finding-4 fix — project delete destroys a bound session's workspace without aborting its running turn, against the very §10 row the fix implements — which keeps this from an accept.The 4 findings — verified fixed
1. Rebind order (blocking) — fixed.
bindSessioncreates/validates the NEW workspace first (rpc/server.go:742-756) and only a successful create proceeds to the old-tree destroy (rpc/server.go:758-776); a failed create returns before the destroy block and beforeSetSessionBinding, so the old binding and tree stay intact. Mutation-checked: I swapped the two blocks so create ran after destroy →TestRebindToUnbindableProjectKeepsOldWorkspaceFAILS atworkspaces_test.go:532("host not registered: old worktree destroyed by the failed rebind"). Restored; test passes, covering both unbindable targets (missing host, non-git base).2. Bind-time strategy (medium) — fixed. schemaV21 (db/schema.go:523-524) adds
sessions.workspace_strategy;SetSessionBinding(db/projects.go:430-440) writes project+path+strategy in one UPDATE (the INV-6 atomicity comment is preserved); delete (rpc/server.go:969-983), the rebind destroy (rpc/server.go:765) and the project-delete walk (rpc/server.go:1433) all readsess.WorkspaceStrategy, falling back to the project's current config only for pre-schemaV21 sessions. Both directions are tested byTestSessionDeleteUsesBindTimeStrategy: worktree→shared still destroys the private tree at delete; shared→worktree (with the base dirtied) does not raise a false INV-9 on the base. Mutation-checked: I changed rpc/server.go:969 tostrategy := proj.Strategy→ the test FAILS atworkspaces_test.go:594("worktree leaked after delete (bind-time strategy ignored)"). Restored. Rewind blocks: all six reopen-rewind sites drop the column (db/projects_test.go:420, 626, 692; db/recover_test.go:1664, 1840; db/sessions_test.go:267). The one rewind that does not drop it —TestRepairStrayToolMessages(db/recover_test.go:1232) — never reopens the DB, so the v21 ALTER cannot hit a duplicate column; see note 3.3. Clone reuse precise (low) — fixed.
isCloneOf(toolhost/workspace.go:195-211) re-attaches only when the tree'sremote.origin.url, cleaned, equals the cleaned base; a foreign git repo or a plain directory is refused with a diagnostic naming base — symmetric with the worktree sibling'sisWorktreeOfmembership check.TestCloneReuseIsPrecisecovers reuse, foreign-clone refusal and non-git refusal. Mutation-checked: I forcedisCloneOfto return false → the test FAILS atworkspace_test.go:184("reuse: ... exists but is not a clone of ..."). Restored.4. DeleteProject §10 (note) — fixed.
MethProjectDelete(rpc/server.go:1405-1454) walksSessionsByProject, destroys each bound private workspace through the project's host (INV-9: a dirty tree is left in place, never force-destroyed; an unreachable host or failed destroy degrades to a recorded leak — never a refusal that strands the delete), unbinds each session viaSetSessionProject(which clears project_id, workspace_path AND workspace_strategy — the strategy dies with the binding), then deletes the row.TestProjectDeleteUnbindsAndGuardscovers clean-destroyed / dirty-guarded / both-unbound / subsequentsession rm. Mutation-checked: I disabled the destroy loop → the test FAILS atworkspaces_test.go:680("clean workspace not destroyed by project delete"). Restored.New finding — CHANGES REQUESTED
[Medium]
rpc/server.go:1423-1453— project delete destroys bound sessions' workspaces without aborting their running turns; the §10 row the fix implements says "aborts the turn".workspaces.md §10's row for delete project is
delete project → aborts the turn | bound sessions are unbound and their workspaces guarded (INV-9)— two halves. The fix implements "unbound + guarded workspaces" but not "aborts the turn", and the destroy it adds is exactly what makes the missing abort harmful. The sibling path shows the intended order:MethSessionDeletecallsterminateSessionTree(rpc/server.go:1116) BEFOREdestroySessionWorkspace— a session is never torn down under a live turn. The project-delete walk has no equivalent: deleting a project while a bound session is mid-turn (the situation §10's table exists for) destroys that session's clean private workspace underneath the running turn — the host runsgit worktree remove/rm -rfconcurrently with the turn's in-flight commands, the turn's next tool call fails against a destroyed cwd, and the session is left unbound (project_id NULL) while its turn keeps running. Who hits it: an operator runningproject rmon a project that has a session mid-turn — an ordinary §10 path. The turn fails loudly rather than silently, and INV-9 still protects dirty trees, so this is a disruption, not data corruption — but the fix is cheap and mirrors the sibling: terminate each bound session's live activity (e.g.terminateSessionTreeper session, or a project-scoped variant) before the workspace walk at rpc/server.go:1423. I found no test covering delete-while-mid-turn.Non-blocking notes
SetSessionProjectfails for one session (rpc/server.go:1448), the delete returns an error but earlier sessions were already unbound and their trees destroyed, and the failing session's tree was destroyed before its failed unbind. A retry converges (the re-destroy on the gone tree logs a false "leak", then unbind and delete succeed), so it is recoverable, but the walk is not atomic. DB-error-only path.workspace.destroyas{path, force}; the implementation sends{strategy, base, path, force}(rpc/server.go:920). Cosmetic doc drift.TestRepairStrayToolMessages(db/recover_test.go:1232) rewinds past v15 leaving the v21 column in place while deleting its schema_migrations row; it passes only because the test never reopens. If it is ever extended to reopen, the v21 ALTER fails with "duplicate column". Consider dropping the column there for consistency.Checks (exact)
gofmt -l .andgo vet ./...are clean. The three new rpc tests (TestRebindToUnbindableProjectKeepsOldWorkspace,TestSessionDeleteUsesBindTimeStrategy,TestProjectDeleteUnbindsAndGuards) andTestCloneReuseIsPrecisepass at head.Summary
All four findings from comment 4006 are genuinely fixed, each pinned by a test that I re-mutated and confirmed fails (exact lines above). The adversarial re-review found one new gap in the finding-4 fix: the project-delete walk destroys bound sessions' workspaces without aborting their running turns, against the §10 row it implements. Terminate before destroy on that one path and this is an accept.
MethProjectDelete destroyed bound sessions' workspaces without aborting their running turns, against the §10 row it implements ("delete project → aborts the turn"): a session mid-turn lost its clean private tree under the running turn and was left unbound mid-turn. Mirror MethSessionDelete's terminate-before-destroy ordering. - rpc/server.go: terminate every bound session's activity tree (its own and its subagent descendants', transitively) before the workspace walk; TestProjectDeleteTerminatesRunningTurn pins it — with the pass removed the activity is left running under the destroyed tree. - unbind before destroy in the walk (review note 1), with the failure policy stated at the site: an unbind failure aborts with the tree intact (no dangling binding to a deleted project); a destroy failure degrades to a recorded leak (INV-9 guarded / §13 orphan), never a refusal that strands the delete. - docs/workspaces.md §11: workspace.destroy shape is {strategy, base, path, force} (was {path, force}) — the host needs strategy+base to remove the tree. - db/recover_test.go TestRepairStrayToolMessages: the rewind now drops the v21 sessions.workspace_strategy column like every other reopen-rewind site (review note 3), so extending the test to reopen would not hit a duplicate-column ALTER.Verdict: CHANGES REQUESTED
Re-review of #2 at head
3876ddb(verified: local HEAD,origin/issue/1-workspace-strategiesand the PR's head_sha all equal3876ddb25ca1e498d4ca1388a7cd1ce6d266c49f; working tree clean). Comment 4009's four items: three are fully fixed, one (the terminate-before-destroy fix) is only partially fixed — it covers subagent-kind sessions but not a root session's interactive turn. Re-review found one new blocking defect: project delete deadlocks forhost:"caller"projects (reproduced). Details below.Comment 4009 items — verified
1. Terminate-before-destroy (the 4009 finding) — PARTIALLY fixed; subagent case fixed, root-session case not. The pass at rpc/server.go:1436 runs
terminateSessionTreeper bound session before the walk, mirroring MethSessionDelete (rpc/server.go:1116). The subagent/descendant case is pinned and mutation-checked: I removed the pass (rpc/server.go:1436) andTestProjectDeleteTerminatesRunningTurnFAILED at rpc/workspaces_test.go:764 — "activity state after project delete: running, want killed" — restored, passes. But the fix only aborts sessions with a subagent-kind activity.terminateSessionTree(rpc/subagents.go:778-801) acts only whenActivityBySessionfinds a livekind='subagent'row (db/activities.go:74-76 —WHERE kind = 'subagent' AND ref_id = ?), and a root session has none. EvencancelTurncouldn't help:turnCancelis registered only inrunDrivenTurn(rpc/subagents.go:224-235); the interactive turn runs onconnCtx(rpc/server.go:1744) with no registration. So a root session mid-turn is a no-op for the pass, and the walk destroys its clean private tree under the running turn — the exact harm 4009 described — and §10's "aborts the turn" row remains unmet for the ordinary interactive case. The new test pins only the subagent shape: it explicitly creates aKind:"subagent"activity (rpc/workspaces_test.go:745). Severity: medium (the turn fails loudly against a gone cwd — a disruption, not corruption; the driven/subagent case the fix's own comment describes is genuinely fixed).2. Mid-walk partial-failure policy — fixed and sound. The policy is now stated at the site (rpc/server.go:1444-1456) and enforced by the code: unbind before destroy (SetSessionProject at 1453 precedes destroyWorkspace at 1464), unbind failure → abort with that session's tree intact (the retry walks only still-bound sessions — SessionsByProject filters
project_id, db/dao.go:290-306 — so it converges), destroy failure → recorded leak, never a refusal that strands the delete. This is consistent with the codebase's documented degradation (destroySessionWorkspace, server.go:947-989) and with the ordering rule: each failure lands in a clean, retryable state. Not test-pinned (no injectable unbind failure), but the comment carries the policy — acceptable. One residual nit from 4009 note 1: a re-destroy of an already-gone tree logs a spurious "recorded leak" (gitDirty errors on a removed dir) — harmless, pre-existing.3. §11 shape drift — fixed. docs/workspaces.md:575 now lists
workspace.destroyas{strategy, base, path, force}, matchingWorkspaceDestroyParams(rpc/protocol.go:455-464);workspace.create{strategy, base, path, ref}matchesWorkspaceCreateParams(rpc/protocol.go:434-444).4. TestRepairStrayToolMessages rewind — fixed. db/recover_test.go:1232 now drops
sessions.workspace_strategy(comment updated at 1209-1213), matching the other reopen-rewind sites (db/projects_test.go:420, 625, 692; db/recover_test.go:1665, 1841; db/sessions_test.go:267). Test passes.New finding (blocking)
[Blocking] rpc/server.go:301 —
MethProjectDeletedeadlocks forhost:"caller"projects.The async-dispatch list in serveConn covers exactly the methods that can dispatch workspace create/destroy back to the caller connection:
MethSessionCreate || MethSessionProject || MethSessionDelete(rpc/server.go:301).MethProjectDeleteis not in it, so it runs synchronously in the connection read loop. But the project-delete walk callsdestroyWorkspace(rpc/server.go:1464), which for ahost:"caller"project resolves the host to that same connection (rpc/server.go:786-806) andcallHost(rpc/server.go:852-894) writesworkspace.destroyto it and blocks onwsPending— a channel only the read loop fills. The read loop is the blocked caller: the reply is never read, the request never returns, and the connection is wedged until it disconnects. Reproduced with a throwaway test (removed; tree clean):host:"caller"worktree project, one bound session,project rmfrom the caller → no return within 10s ("DEADLOCK: project delete on host:caller project did not return within 10s"). Control with an identical setup but a named host → returned<nil>in 0.04s and destroyed the tree, isolating the caller-dispatch as the cause. Reachability: ordinary — the PR body advertiseshost:"caller"as first-class ("the caller is the workspace host for host:"caller" projects"), andproject rmis an ordinary operator action (verbs/verbs.go:1184,cl.Call(rpc.MethProjectDelete, ...)on the same connection). On the hang the sessions are already unbound (unbind precedes destroy at rpc/server.go:1453) while the project row is not yet deleted — partial state until the client disconnects, then the destroy error is logged as a leak and the delete completes. Fix: addMethProjectDeleteto the async list at rpc/server.go:301, the same shape as its three siblings. No test covers caller-hosted project delete (the two project-delete tests use a named host), which is why this is untested.Checks (exact)
gofmt -l .andgo vet ./...clean. The three project-delete tests, the caller-bind test, the rebind/unbind-time/clone-reuse tests and TestRepairStrayToolMessages all pass at head.Summary
Three of 4009's four items are fully and correctly fixed (policy, §11 shape, rewind — with the mutation evidence above). The terminate-before-destroy fix is real but partial: subagent-driven turns are aborted and pinned; a root session's interactive turn is not (§10's row still unmet for the ordinary interactive case). The re-review's one new defect is the
host:"caller"project-delete deadlock — a hang on an ordinary path, reproduced and isolated. Fix the deadlock (add MethProjectDelete to the async list) and close the root-session abort gap (register a cancel for interactive turns and terminate root sessions' live turns in the walk, or scope §10's "aborts the turn" explicitly) and this is an accept.Verdict: ACCEPT
Re-review of #2 at head
e3a86c5(verified: local HEAD,origin/issue/1-workspace-strategiesand the PR head all equale3a86c53efab31c1f83c111a2a7cf68321ee2be8; working tree clean throughout — every mutation below was reverted and the tree re-verified,git diff HEADempty).Comment 4011 finding 1 (blocking: project delete deadlocks on host:"caller") — fixed
MethProjectDeleteis now in the async-dispatch list at rpc/server.go:301. I enumerated the dispatch-to-caller set and it is closed: the only dispatch cases that can reachcallHost(rpc/server.go:854) synchronously areMethSessionCreate(bindSession at 1019),MethSessionDelete(destroySessionWorkspace at 1119),MethProjectDelete(walk destroyWorkspace at 1470) andMethSessionProject(bindSession at 1513) — all four are in the goroutine list. No other case indispatch(rpc/server.go:1001-1603) touches a workspace host;MethProjectSetonly pins hostname/user; the subagent workspace hooks (rpc/subagents.go:749,752) run in turn-driver goroutines, never the read loop. Response correlation is unambiguous: workspace ids count down from -1 (rpc/server.go:860) while tool.run ids are DB tool-call ids and drain ids are positive counters, and the serveConn router checks toolPending → drainPending → wsPending in that order (rpc/server.go:333-352), so a workspace reply can never be misrouted.Mutation-checked myself (rule 6): I removed
|| req.Method == MethProjectDeletefrom rpc/server.go:301 →TestProjectDeleteHostCallerFAILS atrpc/workspaces_test.go:1054with "DEADLOCK: project delete on host:caller project did not return within 10s" — the exact hang 4011 reproduced. Restored; test passes (0.04s). The test is a genuine deadlock probe, not a mock: the caller registers realOnRequesthandlers for workspace.create/destroy (client.go read loop dispatches them in a goroutine), binds a caller-hosted worktree session, deletes the project under a bounded wait, and asserts the workspace is destroyed, the project row gone, and a follow-upsession.deletecompletes (no dangling binding — the second-call shape).Comment 4011 finding 2 (medium: root session's interactive turn not aborted) — fixed
All three halves are present and correct:
handleChatTurnderivesturnCtx, cancel := context.WithCancel(context.WithoutCancel(connCtx))(rpc/server.go:1763) and registers the cancel ins.turnCancel[sessionID]underturnMu, deleting it afterSend/SendTurnreturns. The map slot is per-session and serialized with the driven turn's registration by the ACT-6 turn lock both sites hold (lockSessionat rpc/server.go:1684, driveSession), so the slot is never contended.SendTurnno longer re-wraps the non-driven path inWithoutCancel(agent/service.go), soprov.Chat(ctx, ...)andrunOneTool'srunCtx(derived from the turn ctx, agent/service.go:661) carry the live cancel;connRunner.Runselects onctx.Done()(rpc/server.go:1932), so an in-flight tool call is interrupted too.terminateSessionTreecallss.cancelTurn(sessionID)(rpc/subagents.go:811) even with no subagent activity row, preserving the state-first-cancel-second ordering for subagent turns and covering root sessions for both the project-delete walk and MethSessionDelete.Mutation-checked myself, two independent hunks: (a) removing the unconditional
s.cancelTurn(sessionID)at rpc/subagents.go:811 →TestProjectDeleteAbortsInteractiveTurnFAILS atrpc/workspaces_test.go:879("interactive turn was not aborted by project delete (still running after 10s)") andTestSessionDeleteAbortsInteractiveTurnFAILS atrpc/workspaces_test.go:965. (b) re-adding theif !driven { ctx = context.WithoutCancel(ctx) }re-wrap inSendTurn→TestProjectDeleteAbortsInteractiveTurnFAILS atrpc/workspaces_test.go:879. Both restored. The tests are sound:blockingProvider.Chat(rpc/subagents_test.go:1426) closesstartedon entry and blocks onctx.Done(); the test waits forstarted— which happens strictly after registration — before issuing the delete, so there is no registration race in the test itself.P2 disconnect-survival holds.
turnCtxis detached from the connection (WithoutCancel(connCtx)), so a client disconnect cancelsconnCtxbut not the turn; tool execution still dies with its host connection viaconnCtx.Done()in the runner, unchanged. Mutation-checked: I changed rpc/server.go:1763 tocontext.WithCancel(connCtx)(no detachment) →TestTurnErrorPersistsAfterClientDisconnectFAILS atrpc/subagents_test.go:1492("condition not met within 5s" — the turn died with the disconnect, so the "boom" error record never landed). Restored; the test passes at head and pins both halves (the turn must survive the disconnect and fail with "boom", not "context canceled"). The new interactive cancel is purely additive:Agent.CancelTurnis only invoked byKillSubagent(agent/subagents.go:648, which acts only on live subagent activities) and the deadline paths — pre-existing behaviors are unchanged.Whole-PR re-review (adversarial, rule 8)
sessions.workspace_strategywritten in the one-statementSetSessionBinding(rpc/server.go:779) and read at both delete sites (destroySessionWorkspace, walk at 1462-1465, pre-v21 fallback), precise clone reuse viaisCloneOf(toolhost/workspace.go:195). 4009: terminate-before-destroy pass (project delete) plus the policy comment, §11 shape at docs/workspaces.md:575, theTestRepairStrayToolMessagesrewind drops the column (db/recover_test.go:1232). 4011 note 2 (policy): unbind-before-destroy with unbind as the single refusal, destroy degrading to a recorded leak — enforced by the code I read at rpc/server.go:1447-1476.sharedstrategy never dispatches (guarded at 1466-1471); the caller-hosted bind and session-delete shapes are pinned byTestHostCallerWorktreeBind(bind + session delete through the caller connection); the caller-hosted project-delete shape byTestProjectDeleteHostCaller; the follow-upsession.deletein that test exercises the second-call shape.bob/s<id>, one branch per session, toolhost/workspace.go:262) and are created with-Bso a leftover branch self-heals. Session workspace paths<base>/.bob/{wt,clone}/s<id>are distinct per strategy and session (db.ResolveWorkspace).Checks (exact)
gofmt -l .andgo vet ./...clean. All three new tests plus the P2 test pass at head (TestProjectDeleteHostCaller,TestProjectDeleteAbortsInteractiveTurn,TestSessionDeleteAbortsInteractiveTurn,TestTurnErrorPersistsAfterClientDisconnect).Non-blocking notes (each names its consequence)
lockSession(rpc/server.go:1684) and theturnCancelregistration (rpc/server.go:1763) is not yet cancelable; if the delete walk'sterminateSessionTreeruns in that window,cancelTurnis a no-op and the destroy lands under the starting turn. Consequence: the turn fails loudly at its next boundary (resolveTurn refusal or a tool call against a destroyed cwd), the session is unbound, and the failure is recorded — the documented degradation, not silent corruption. The abort-first ordering shrinks the window to the handler's pre-registration span; closing it fully would require canceling before the turn exists, which is not representable. This is the design 4011 requested.MethSessionCreatein one call (the tests split create +session.project); the async set covers it and the deadlock shape is identical to the pinnedTestHostCallerWorktreeBind. Consequence: if the async list ever regressed forMethSessionCreateonly, the bind-on-create shape would go unpinned.turnCancelentry leak on panic. The map delete inhandleChatTurnis not deferred, so a panic inSend/SendTurnwould leave the cancel in the map. Consequence: a later turn of the same session overwrites the slot and the leaked cancel targets a dead context — harmless, and symmetric with the pre-existing driven path (rpc/subagents.go:234-236).Summary
Both 4011 findings are genuinely fixed, each pinned by a test I re-mutated myself (four mutations, exact failing lines above: workspaces_test.go:1054, 879, 965; subagents_test.go:1492). The dispatch-to-caller set is closed under all four methods that can reach it; P2 disconnect-survival is preserved and pinned. Prior findings from 4006/4009 hold at head.
make buildandmake checkpass, the uncached suite is green, the tree is clean. This is correct and complete for issue #1 — mergeable.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.