Repository navigation
fix(kanban): preserve shared workspaces across boards - #126617
JoaoMarcos44 wants to merge 2 commits into
Conversation
alt-glitch
added
type/bug
| for child in boards.iterdir() | ||
| if child.is_dir() and (child / "kanban.db").is_file() | ||
| ) | ||
| except OSError: |
There was a problem hiding this comment.
[P1] Keep this guard fail-closed when board discovery itself fails.
This exception returns a partial database list. _workspace_use_snapshot() then leaves complete=True, so completion or GC can delete a workspace without checking an unreadable sibling board. That restores the data-loss path this change is intended to close.
I reproduced this by making boards.iterdir() raise while a live task in another board referenced the archived task workspace. _cmd_gc() reported that it removed one workspace and deleted the file.
Please propagate discovery failure into the snapshot completeness result, and add a regression that proves an enumeration or read failure preserves the workspace. An active workspace path that cannot be resolved should also make the snapshot incomplete.
Trim the salvaged tests to two: gc keeps a scratch dir a live same-board task still uses (and records the deferred event), and cleanup refuses the delete when sibling boards cannot be listed while a named board's live task uses the dir (the fail-closed path NousResearch#126617 missed). The plain same-board cleanup, cross-board and same-id cases go through the same _workspace_in_use_by_other scan.
Trim the salvaged tests to two: gc keeps a scratch dir a live same-board task still uses (and records the deferred event), and cleanup refuses the delete when sibling boards cannot be listed while a named board's live task uses the dir (the fail-closed path #126617 missed). The plain same-board cleanup, cross-board and same-id cases go through the same _workspace_in_use_by_other scan.
This branch has not been deployed
3 participants
Summary
Fixes #126508 by moving scratch/worktree deletion behind a workspace-reference guard at the lifecycle owner.
The destructive paths previously checked containment and parent/child activity, but they did not prove that the same resolved
workspace_pathwas unused by another non-terminal task. Because Hermes stores named boards in separate SQLite DBs, that proof must include sibling boards as well as the current connection.What changes
hermes kanban gcworkspace_cleanup_deferred_sharedwhen deletion is deferredDifferential from #126513
I inspected #126513 before publishing to avoid a duplicate. Its helper queries the current board connection only. A live task in another board DB can therefore reference the same resolved path without being visible to that guard.
This PR is independently implemented from current
main; no commit, hunk, or code from #126513 was cherry-picked or copied. The cross-board regression is the behavioral distinction: archived task in the default board + live task in a named board + identical resolved scratch path => GC must preserve the directory.The regression is also run with
HERMES_KANBAN_DBandHERMES_KANBAN_WORKSPACES_ROOTpinned like a worker environment. Sibling-board discovery deliberately uses physical board DB files, so a process-wide DB pin cannot collapse every board lookup onto the current DB.I did not fold in the source-side decompose fix from #103303 because #103325/#103628 already cover that separate producer-side problem.
Regression coverage
Commit structure
26dbdcd95ccf243e1425977008cd9ece1c8ab645— root-cause implementation + focused regressionsa2caf0e644d28d11913f1446d99b6d4dd56ddf21— post-publication regression hardening for the pinned worker environmentThe second commit is intentionally test-only: the implementation did not need to change after the adversarial review.
Post-publication audit
I ran 10 review passes over provenance/deduplication, exact base/head, destructive-site coverage, multi-board ownership, worker env pinning, terminal-state semantics, path identity/cross-platform behavior, read-only/fail-closed behavior, GC cost, and CI/review/main-drift state.
One material improvement came out of that audit: the cross-board test now reproduces the process-wide worker pins noted above. No additional code blocker was found. #126513 remains on its original current-board-only implementation at the time of this update, and upstream
mainis stille408d363393ccb72267e67bcccf4f8954b438cd9.Validation status
The branch is based on exact upstream
maincommite408d363393ccb72267e67bcccf4f8954b438cd9.The connected environment does not expose a repository test runner, so I am not claiming local pytest/ruff results I could not execute. The first upstream Actions attempt ended in
startup_failurebefore jobs ran, and no workflow has run for the follow-up test commit yet.There is a narrow residual cross-DB TOCTOU window inherent to any check-then-delete design: a new task could acquire the same path after the ownership snapshot and before filesystem removal. Closing that fully would require a cross-board coordination/locking primitive, which is broader than this deletion-owner fix.
Related