Skip to content

fix(kanban): discard partial worktrees when git worktree add fails - #126020

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-126004
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-126004

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

A failed or killed git worktree add left a partial checkout behind, and the next dispatch silently reused it. Git writes the worktree's .git pointer before checking out files, so the leftover directory passed _ensure_git_worktree's common-dir reuse shortcut and workers ran inside an incomplete tree — in the reporter's data, 5 of 6 affected tasks even reported completed on a partial checkout.

This PR:

  • Discards partial worktrees on both failure paths (non-zero exit and subprocess.TimeoutExpired): best-effort git worktree remove --force, then rmtree, then git worktree prune last — the order matters, because prune only drops registrations whose directory is gone, so it must run after the directory is removed to clear admin metadata that remove could not (damaged state). A retry now performs a real, complete worktree add.
  • Raises the add timeout from a hard 60s to 600s, overridable via HERMES_KANBAN_WORKTREE_TIMEOUT. The 60s cap killed full checkouts of large monorepos mid-flight (the reporter's 41.6k-file repo under Defender real-time scanning exceeded it); worktree add is a local checkout with no network hang risk.
  • Keeps 4000 chars of failure text instead of 500 in _record_task_failure, so the real git fatal line survives into tasks.last_failure_error / task_runs.error / event payloads instead of being cut off by progress noise.

Related Issue

Fixes #126004

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/kanban_db_workspace.py: added _worktree_add_timeout() (env-overridable 600s default) and _discard_partial_worktree() (best-effort remove → rmtree → prune); _ensure_git_worktree now cleans up on both the non-zero-exit and timeout paths before raising, preserving the original error.
  • hermes_cli/kanban_db_dispatch.py: _record_task_failure caps the error text at 4000 chars instead of 500.
  • tests/hermes_cli/test_kanban_worktree_failure_cleanup.py: new coverage — partial discard + successful retry after a non-zero exit and after a timeout; timeout env override is honored; failure text keeps 4000 chars.

How to Test

  1. python -m pytest tests/hermes_cli/test_kanban_worktree_failure_cleanup.py -v — Observed result: 4 passed (partial checkout discarded on both failure paths, retry completes a real checkout, HERMES_KANBAN_WORKTREE_TIMEOUT=123 reaches the git call, last_failure_error length is 4000).
  2. python -m pytest tests/hermes_cli/test_kanban_worktree_isolation.py tests/hermes_cli/test_kanban_worktree_teardown.py tests/hermes_cli/test_kanban_worktree_unicode_path.py -q — Observed result: 19 passed (existing worktree behavior unchanged).
  3. python -m pytest tests/hermes_cli/test_kanban_worker_exit_trailer.py tests/hermes_cli/test_kanban_blocked_sticky.py tests/hermes_cli/test_kanban_review_lifecycle_complete.py -q — Observed result: 42 passed (_record_task_failure callers unaffected).
  4. ruff check on all three changed files — all checks passed.

Note: tests/hermes_cli/test_kanban_db.py::test_infrastructure_spawn_refusal_never_charges_the_card fails identically on a clean upstream/main checkout of this host (verified via git stash on the base commit) — pre-existing environment failure, unrelated to this change. All other 52 tests in that file pass.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A (HERMES_KANBAN_WORKTREE_TIMEOUT is an env knob like the existing HERMES_KANBAN_WORKSPACES_ROOT, documented in the helper's docstring)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — the reporter's failure mode is Windows/AV-specific; the fix is platform-neutral (git + stdlib), and the timeout env knob covers slower hosts
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

N/A

Screenshots / Logs

tests/hermes_cli/test_kanban_worktree_failure_cleanup.py::test_failed_add_discards_partial_and_retry_succeeds PASSED
tests/hermes_cli/test_kanban_worktree_failure_cleanup.py::test_timed_out_add_discards_partial PASSED
tests/hermes_cli/test_kanban_worktree_failure_cleanup.py::test_add_timeout_env_override PASSED
tests/hermes_cli/test_kanban_worktree_failure_cleanup.py::test_record_task_failure_keeps_4000_chars PASSED
============================== 4 passed in 1.37s ==============================

A killed or failed 'git worktree add' left a partial checkout behind
(the .git pointer lands before the files do), and the next dispatch
passed _ensure_git_worktree's common-dir reuse shortcut straight into
the incomplete tree — workers ran on a partial checkout and most such
tasks even reported completed (NousResearch#126004).

- Both failure paths (non-zero exit, subprocess timeout) now run a
  best-effort discard: worktree remove --force, rmtree, then prune last
  so an admin registration the remove could not clear is still dropped.
- The add timeout rises from a hard 60s (which killed full checkouts of
  large monorepos mid-flight under AV scanning) to 600s, overridable via
  HERMES_KANBAN_WORKTREE_TIMEOUT.
- _record_task_failure keeps 4000 chars instead of 500 so the real git
  fatal line survives into last_failure_error and task_runs.error.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kanban: git worktree add killed by 60s subprocess timeout -> partial dir silently reused on retry; stderr truncated to 500 chars

2 participants