Repository navigation
Conversation
gc, completion and deferred parent cleanup removed a shared workspace_path as soon as one of its tasks went terminal.
alt-glitch
added
type/bug
| return True | ||
| if not key: | ||
| return False | ||
| rows = conn.execute( |
There was a problem hiding this comment.
[Bug] (blocking) This query checks only the board database behind conn, but a task on another board can point to the same workspace_path. I reproduced that case on this exact head with an archived task on the default board and a ready task on a named board sharing one scratch directory. _cleanup_workspace() deleted the file that the ready task still needed. The new tests put both tasks in one connection, so they do not cover this boundary. Please make the ownership check cover every board database, or fail closed when global ownership cannot be established.
The live-task check only read the board database it was given. A ready task on a named board can point at the same directory.
|
_workspace_in_use_by_other now reads every other board database on disk, not only the connection it was given. If that set cannot be read, cleanup is skipped. pytest tests/hermes_cli/test_kanban_gc_retention.py -q On 2ace1a9 the cross-board case deleted the scratch file. On 00334af the ready task on the named board keeps it. |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES
The new head fixes the previously reported cross-board lookup gap. The added regression fails on 2ace1a93 and passes on 00334af8, and all 11 focused retention tests pass. The change also merges cleanly with current upstream/main at cf9dbe59, where the same 11 tests pass.
One cross-board identity case can still delete a live task's shared workspace. Task IDs are unique only within each board database. The inline comment includes the reproduction and impact.
No hosted check runs or commit statuses are reported for this head.
| other = sqlite3.connect(uri, uri=True, timeout=1.0) | ||
| try: | ||
| other.row_factory = sqlite3.Row | ||
| return _conn_uses_path(other, task_id, key) |
There was a problem hiding this comment.
[Bug] (blocking) A sibling board must not reuse the current board's task ID as an exclusion. Task IDs are generated independently and are unique only inside each board database. I reproduced this on the current head by creating t_same_id on both boards, archiving the default-board task, and leaving the named-board task ready with the same workspace_path. _other_board_uses_path() passed t_same_id into WHERE id != ?, filtered out the live row, and _cleanup_workspace() deleted note.txt. The same failure remains after composing this head with current upstream/main. Please query sibling boards without excluding the current board's task ID and add a matching-ID regression.
There was a problem hiding this comment.
Task ids are only unique inside one board, so the sibling lookup no longer drops a row with the same id.
Pushed cbfb444: a matching-id regression keeps the scratch file.
uv run --group dev python -m pytest tests/hermes_cli/test_kanban_gc_retention.py -q
12 passed
Task ids are unique only inside one board database. Excluding the id being cleaned up hid a live task on a sibling board that reused it.
The sibling-board case was pasted twice. Pytest only collects one.
gc, completion/archive cleanup and deferred parent cleanup rmtree'd a task's workspace_path as soon as that task went terminal, even when another non-terminal task (on this board or a sibling board) pointed at the same directory, destroying its in-progress work. Before removing, compare the resolved path against every live task on this board and on every other board's kanban.db; if any still uses it, skip the delete, log, and record a workspace_cleanup_deferred_shared event. When the sibling-board set cannot be read, refuse the delete. Salvaged from NousResearch#126513 (squash of its commits). Fixes NousResearch#126508.
gc, completion/archive cleanup and deferred parent cleanup rmtree'd a task's workspace_path as soon as that task went terminal, even when another non-terminal task (on this board or a sibling board) pointed at the same directory, destroying its in-progress work. Before removing, compare the resolved path against every live task on this board and on every other board's kanban.db; if any still uses it, skip the delete, log, and record a workspace_cleanup_deferred_shared event. When the sibling-board set cannot be read, refuse the delete. Salvaged from #126513 (squash of its commits). Fixes #126508.
3 participants
What does this PR do?
kanban gc, task completion and the deferred parent cleanup deleted a scratch directory as soon as one task that pointed at it went terminal. Decompose children can share thatworkspace_path. The other task, stillreadyor running, then lost its files.Before
rmtreeorgit worktree remove, the three sites now skip the path when another task that is notdone,archived,failedorcancelledresolves to the same directory. The skip is logged and recorded asworkspace_cleanup_deferred_shared. The last task to go terminal still removes the directory. A scratch dir that nobody else uses is still collected.Related Issue
Fixes #126508
Type of Change
Changes Made
hermes_cli/kanban_db_workspace.py:_workspace_in_use_by_other()compares resolved paths. Completion and parent cleanup use it.hermes_cli/kanban_ops.py:kanban gcuses the same guard for scratch dirs and worktrees.tests/hermes_cli/test_kanban_gc_retention.py: a live task sharing the dir keepsnote.txtthrough bothgcand_cleanup_workspace.How to Test
10 passed. Without the guard,
test_cmd_gc_keeps_a_scratch_dir_a_live_task_still_usesfailed: gc printed1 workspace(s) removedandnote.txtwas gone.ruff checkon the three files was clean. The fullpytest tests/ -qsuite was not run.This does not change decompose so children get their own directory, and it does not add
kanban gc --dry-run.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A