Repository navigation
fix(kanban): lease active task worktrees - #95500
solovision24 wants to merge 2 commits into
Conversation
c5c61ee to
cb509d3
Compare
The ownership model is well thought through: non-process-scoped lock reasons, preserve-on-unknown everywhere, repo-scoped lifecycle lock serializing Hermes lock transitions, and an adversarial test that simulates a relock between owned-unlock and remove to prove git's own guard is the final backstop. Three issues:
No blocking issues beyond these. |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head cb509d37883f10c4935ec3632398be75e655b4da against live main@d0351e32309bd68a1012c302157947b955323fca, including the full four-file diff, the worktree add/re-entry/removal paths, current cleanup semantics inherited from #88129, exact-head workflow state, the existing automated comment, and the overlapping #90529/#95501 branches. The branch is 1 commit ahead / 28 behind merge base 98e87ac886a0b53e2ff80ac767df5283bf7d17ed; those 28 main commits are path-disjoint from this PR's four files, so the stale base is evidence debt rather than a newly observed semantic collision.
There is good work here. Exact lock reasons, preserve-on-unknown/foreign ownership, one repo-scoped lifecycle lock for Hermes transitions, and retaining Git's no-force dirty/relock guard are the right primitives. This also preserves the important #88129/#88051 lineage: @Cossackx's original worktree-reaper semantics and the later kill-before-remove/no---force TOCTOU hardening are not weakened.
I do have one P1 defect class with two concrete manifestations: the worktree becomes visible before its durable ownership proof exists.
P1 — lifecycle ownership is installed after publication, and existing live trees are never adopted
hermes_cli/kanban_db.py::_ensure_git_worktree() performs git worktree add ..., waits for it to return, and only then calls _lock_task_worktree(). _lock_task_worktree() acquires hermes-worktree-lifecycle.lock, but the materialization itself is outside that critical section.
That means the exact race this PR is intended to close still exists for every newly created task tree: after git worktree add has registered a clean worktree but before the Git lock is installed, any cleanup path that does not already participate in this new file lock sees a normal unlocked worktree.
I forced the submitted ordering with real Git:
after_add_exists=yes
after_competing_remove_exists=no
lock_rc=128
lock_output=fatal: '../worktrees/t_gap' is not a working tree
The sequence was simply: add task worktree → competing git worktree remove → attempt the PR's lock. The remover wins because there is no ownership proof yet; the later lock cannot recover the checkout. This is not hypothetical scheduling machinery—the two commands are exactly the boundary this patch separates.
The same publication gap exists across upgrade. Worktrees created by the pre-#95500 runtime carry no active Hermes Kanban task ... lock. The new code locks an existing tree only when that task later passes through _ensure_git_worktree() / the resolver again. An already-running pre-upgrade worker does not necessarily traverse that path, so the first version containing this PR can still expose the exact in-flight trees that motivated the incident until each one is re-entered. The existing automated review noticed this compatibility hole; I reproduced the mechanism and consider it part of the same blocking ownership transition, not a polish item.
Required repair:
- Acquire the repo-scoped lifecycle lock before publishing a task worktree and hold it across
git worktree addand the Git-nativeworktree lock. Refactor the helper so this is one non-reentrant critical section rather thanaddfollowed by a helper that obtains the lock afterward. - Add an adoption path for already-current Kanban worktrees before any Hermes cleanup/GC surface is allowed to reap them after upgrade. Adoption should prove the task/worktree/branch (and preferably current run/claim generation), preserve unknown/foreign locks, then install the canonical lock under the same lifecycle serialization.
- Add deterministic regressions where a competing remover is released immediately after
worktree addbut before lock installation and must be unable to remove the tree, plus an upgrade fixture where a pre-lock current task is adopted before cleanup can touch it.
One adjacent design point is worth carrying into that repair: the lock reason is currently task/branch-scoped, not run/generation-scoped. That is sufficient for the narrow completed/archived cleanup shape inherited today, but #95501's retention work is adding delayed destructive mutation and already needs a consume-time task/run-generation recheck. Please keep one lifecycle authority rather than letting #95500 own Git locks while #95501 invents a second deletion-generation model.
Exact-object boundary: the coding-router half is not in this PR
The body says the installed router counterpart now uses the same lifecycle lock and cites python3 -m unittest test_coding_worker plus a bounded local router run. None of that counterpart code is in this repository diff. That evidence is useful for the author's machine, but it cannot certify the merge object for other installs. If the coding router is intentionally out-of-repo, please link its authoritative version/receipt and state the compatibility contract; otherwise prove the repo-side change is safe against the unmodified installed router. Do not treat a local installed counterpart as a shipped repository invariant.
Prior comment disposition
I am not carrying the automated comment's common.parent concern as a finding. _git_common_dir() explicitly runs git rev-parse --path-format=absolute --git-common-dir; a real linked-worktree probe returns the primary checkout's common .git directory, not .git/worktrees/<name>, so common.parent is the repository toplevel in the supported linked-worktree shape.
Graph / merge order / credit
- #90529 (
@roycepersonalassistant) is complementary, not duplicate, and collides with all four files in this PR. It owns exact linked-anchor identity, Git-common-dir repository selection, exact toplevel/branch reuse, and canonical-checkout preservation. It supersedes closed #89734, which in turn preserved the #89727 / #70585 resolver lineage. #95500 must either restack over the current disposition of that work or explicitly document which parts supersede it; do not resolve those four files textually and lose the anchor ownership tests. - #95501 (
@arcominion) is the delayed stale-workspace deletion owner and collides inhermes_cli/kanban_db.pyplustest_kanban_worktree_teardown.py. Its current review already identifies the other side of this shape: deletion must revalidate current task/run authority at mutation time. Preferred composition is one lifecycle/generation primitive shared by #95500 and #95501, then restack the retention work after the active-tree owner is settled. - #88129 (merged, @teknium1 salvaging #88051/@Cossackx) is historical teardown provenance, not competing ownership. This PR correctly keeps its no-force Git revalidation and foreign-work preservation semantics.
Exact-head hosted acceptance is still absent: CI 32963270002, Docker 32963269284, and Nix 32963269361 all concluded action_required; the CI run has zero jobs. The local 73-test + 16-test + bounded-router evidence is useful but does not substitute for an executed exact-head cross-platform run.
Once publication and adoption are inside the same ownership transaction, this is a strong direction. The preservation rules are conservative, the Git-native backstop is appropriate, and the incident is worth closing at the mechanism rather than with another cleanup exception. 🚀
|
Follow-up at exact head
Verification on this exact head:
|
Orion independent review — approved for upstream consideration
No merge or deployment was performed. Local runtime activation remains separately gated on upstream merge and post-upgrade safety verification. |
|
PR #95500 watchdog tick (2026-09-13): OPEN, no code changes needed. Hard expiration 2026-11-15. |
Summary
Incident
A bounded coding-router run used a registered Kanban task worktree as its source. During provider execution, both that canonical source checkout and the router's isolated result checkout disappeared before verification, producing
verification_failed, empty changed files, and ENOENT errors. The installed router counterpart now uses the same lifecycle-lock protocol and leaves result worktrees locked until explicit cleanup.Verification
assert False)scripts/run_tests.sh tests/hermes_cli/test_kanban_worktree_teardown.py tests/hermes_cli/test_kanban_worktree_isolation.py tests/cli/test_worktree.py -q— 73 passedpython3 -m unittest test_coding_worker— 16 passedrun-20260826T072107-81dd4084reachedverified_no_commit; source and isolated result both remain present and lockedgit diff --checkpython -m compileallon changed Python files