Repository navigation
Fix repository safety, file previews, and process cleanup from main audit - #138
Conversation
danielss-dev
left a comment
danielss-dev
left a comment
There was a problem hiding this comment.
Bugbot found 3 bugs and 1 risk in the new hard-reset collision guard and process-cleanup paths. Literal pathspec / ignore / rename / preview changes look solid on review; main gaps are Windows index path separators and post-reap kill(-pgid) on provider CLI success.
| guard_reset_tree(repo, index, &repo.find_tree(entry.id())?, root, &rel)?; | ||
| } else if metadata.is_dir() && entry.kind() != Some(git2::ObjectType::Commit) { | ||
| guard_replaced_directory(index, root, &rel)?; | ||
| } else if index.get_path(&rel, 0).is_none() { |
There was a problem hiding this comment.
bug Hard-reset collision guard misses tracked paths on Windows (backslash index lookup)
parent.join(name) / rel.join(child.file_name()) build PathBufs with \\ separators on Windows, then those paths are passed straight to index.get_path. Libgit2 index keys are always forward-slash (src/main.rs), so get_path("src\\main.rs") returns None for every nested tracked entry.
Failure mode: On Windows, a hard reset whose target tree contains any nested path is refused as an "untracked or ignored" collision even when the worktree is clean — or, for directory-replacement checks, the same mismatch mis-classifies tracked children. Root-only collisions still work, which is why Unix CI/tests would not catch this. The PR notes Windows execution of these branches as outstanding.
Suggested fix: normalize to git's path form before the lookup (and reuse in guard_replaced_directory):
fn git_index_path(path: &Path) -> String {
path.components()
.map(|c| c.as_os_str().to_string_lossy())
.collect::<Vec<_>>()
.join("/")
}
// ...
} else if index.get_path(git_index_path(&rel).as_str(), 0).is_none() {There was a problem hiding this comment.
This is a false positive for the pinned git2 0.19.0 API. Index::get_path (src/index.rs:396–397) first calls path_to_repo_path; on Windows that helper (src/util.rs:203–234) calls fixup_windows_path, which converts backslashes to forward slashes before invoking libgit2. Both reset guards therefore already normalize these PathBuf values. Adding lossy conversion here would duplicate the binding and risk corrupting valid Unix non-UTF-8 names. I am adding a cross-platform regression covering nested tracked files and tracked-directory replacement, and including it in Windows CI.
| let path = rel.join(child.file_name()); | ||
| if child.file_type()?.is_dir() { | ||
| guard_replaced_directory(index, root, &path)?; | ||
| } else if index.get_path(&path, 0).is_none() { |
There was a problem hiding this comment.
bug Same Windows separator bug in guard_replaced_directory
path is built with rel.join(child.file_name()), so on Windows it contains backslashes before index.get_path. Tracked descendants of a directory that would be replaced by a file/symlink are treated as untracked, producing false-positive hard-reset refusals (or incorrect collision paths) for any nested layout.
Use the same forward-slash normalization as in guard_reset_tree before get_path.
There was a problem hiding this comment.
This is a false positive for the pinned git2 0.19.0 API. Index::get_path (src/index.rs:396–397) first calls path_to_repo_path; on Windows that helper (src/util.rs:203–234) calls fixup_windows_path, which converts backslashes to forward slashes before invoking libgit2. Both reset guards therefore already normalize these PathBuf values. Adding lossy conversion here would duplicate the binding and risk corrupting valid Unix non-UTF-8 names. I am adding a cross-platform regression covering nested tracked files and tracked-directory replacement, and including it in Windows CI.
| }; | ||
| // A CLI can exit while a helper still owns the pipes. Drain only after | ||
| // stopping the complete owned process tree, also on natural completion. | ||
| stop(&mut child); |
There was a problem hiding this comment.
bug stop() after try_wait reaps the leader, then kill(-pgid) on a free PID
On the success path, child.try_wait() returning Ok(Some(status)) has already reaped the provider process, freeing its PID/PGID. The following stop(&mut child) does kill(-(child.id() as i32), SIGKILL) (via kill_process_tree).
Failure mode:
- Intended case (helper still alive): PGID is held by the helper, so the kill is correct and unblocks pipe joins — good.
- Common case (no helper, normal
gh/azexit): the process group is gone;kill(-pgid)is usuallyESRCH, but the PID is eligible for reuse. - Race on a multi-threaded Tauri host: another Strand spawn that calls
process_group(0)can recycle that PID as a new group leader between reap andstop(). This then SIGKILLs an unrelated process tree (e.g. a concurrent git/AI child).
The new natural_exit_stops_helpers_before_joining_provider_output test needs helper cleanup, but unconditional post-reap killpg on every successful CLI call is unsafe.
Suggested direction: record the pgid at spawn; only signal the group while members may still exist (e.g. kill before reap via waitid(..., WNOWAIT), or call stop only while stdout/stderr reader threads have not yet seen EOF). Never kill(-id) after a successful reap when the group may already be empty.
There was a problem hiding this comment.
Confirmed. Fixed in 45e4e1e: Unix completion is now observed with waitid(WEXITED | WNOHANG | WNOWAIT), then the owned group is signaled before Child::wait releases the PID. Windows still targets its owned Job Object. A regression verifies that two exit observations leave the leader waitable, and the existing helper-held-pipe test still passes. All 167 Tauri tests pass locally.
| if matches!(child.try_wait(), Ok(Some(_))) { return; } | ||
| #[cfg(windows)] | ||
| { | ||
| if matches!(child.try_wait(), Ok(Some(_))) { return; } |
There was a problem hiding this comment.
risk Windows cancel still skips tree kill once the Git leader has exited
The Unix side of this change correctly keeps signaling the owned group after the leader exits so LFS/submodule helpers cannot retain pipes. On Windows the early try_wait() == Some(_) → return was preserved, so taskkill /T never runs against that dead PID.
Failure mode: the same "leader exited, helper still owns stderr/stdout" cancel scenario this PR regresses on Unix remains on Windows: Cancel can hang until the helper finishes transferring, because descendants are not torn down once the root PID is gone. Consider a Job Object (as in the provider-command path) or another tree kill that does not require the leader to still be alive.
There was a problem hiding this comment.
Confirmed. Fixed in 30658d1: streaming Git retains an owned Windows Job Object through cancellation, so cleanup does not depend on the wrapper PID still running. Git starts suspended, is assigned to the job, then resumes; this also closes the spawn/assignment race for fast Git wrappers. The job wrapper is shared with Tauri and uses owned handles. Added a native Windows regression that reaps the leader while its helper retains stdout, then checks that cancellation closes the pipe. The module cross-compiles locally; the new regression runs in Windows CI on this PR.
| guard_reset_tree(repo, index, &repo.find_tree(entry.id())?, root, &rel)?; | ||
| } else if metadata.is_dir() && entry.kind() != Some(git2::ObjectType::Commit) { | ||
| guard_replaced_directory(index, root, &rel)?; | ||
| } else if index.get_path(&rel, 0).is_none() { |
There was a problem hiding this comment.
bug Hard-reset collision guard misses tracked paths on Windows (backslash index lookup)
parent.join(name) / rel.join(child.file_name()) build PathBufs with \\ separators on Windows, then those paths are passed straight to index.get_path. Libgit2 index keys are always forward-slash (src/main.rs), so get_path("src\\main.rs") returns None for every nested tracked entry.
Failure mode: On Windows, a hard reset whose target tree contains any nested path is refused as an "untracked or ignored" collision even when the worktree is clean — or, for directory-replacement checks, the same mismatch mis-classifies tracked children. Root-only collisions still work, which is why Unix CI/tests would not catch this. The PR notes Windows execution of these branches as outstanding.
Suggested fix: normalize to git's path form before the lookup (and reuse in guard_replaced_directory):
fn git_index_path(path: &Path) -> String {
path.components()
.map(|c| c.as_os_str().to_string_lossy())
.collect::<Vec<_>>()
.join("/")
}
// ...
} else if index.get_path(git_index_path(&rel).as_str(), 0).is_none() {There was a problem hiding this comment.
This is a false positive for the pinned git2 0.19.0 API. Index::get_path (src/index.rs:396–397) first calls path_to_repo_path; on Windows that helper (src/util.rs:203–234) calls fixup_windows_path, which converts backslashes to forward slashes before invoking libgit2. Both reset guards therefore already normalize these PathBuf values. Adding lossy conversion here would duplicate the binding and risk corrupting valid Unix non-UTF-8 names. I am adding a cross-platform regression covering nested tracked files and tracked-directory replacement, and including it in Windows CI.
| let path = rel.join(child.file_name()); | ||
| if child.file_type()?.is_dir() { | ||
| guard_replaced_directory(index, root, &path)?; | ||
| } else if index.get_path(&path, 0).is_none() { |
There was a problem hiding this comment.
bug Same Windows separator bug in guard_replaced_directory
path is built with rel.join(child.file_name()), so on Windows it contains backslashes before index.get_path. Tracked descendants of a directory that would be replaced by a file/symlink are treated as untracked, producing false-positive hard-reset refusals (or incorrect collision paths) for any nested layout.
Use the same forward-slash normalization as in guard_reset_tree before get_path.
There was a problem hiding this comment.
This is a false positive for the pinned git2 0.19.0 API. Index::get_path (src/index.rs:396–397) first calls path_to_repo_path; on Windows that helper (src/util.rs:203–234) calls fixup_windows_path, which converts backslashes to forward slashes before invoking libgit2. Both reset guards therefore already normalize these PathBuf values. Adding lossy conversion here would duplicate the binding and risk corrupting valid Unix non-UTF-8 names. I am adding a cross-platform regression covering nested tracked files and tracked-directory replacement, and including it in Windows CI.
| }; | ||
| // A CLI can exit while a helper still owns the pipes. Drain only after | ||
| // stopping the complete owned process tree, also on natural completion. | ||
| stop(&mut child); |
There was a problem hiding this comment.
bug stop() after try_wait reaps the leader, then kill(-pgid) on a free PID
On the success path, child.try_wait() returning Ok(Some(status)) has already reaped the provider process, freeing its PID/PGID. The following stop(&mut child) does kill(-(child.id() as i32), SIGKILL) (via kill_process_tree).
Failure mode:
- Intended case (helper still alive): PGID is held by the helper, so the kill is correct and unblocks pipe joins — good.
- Common case (no helper, normal
gh/azexit): the process group is gone;kill(-pgid)is usuallyESRCH, but the PID is eligible for reuse. - Race on a multi-threaded Tauri host: another Strand spawn that calls
process_group(0)can recycle that PID as a new group leader between reap andstop(). This then SIGKILLs an unrelated process tree (e.g. a concurrent git/AI child).
The new natural_exit_stops_helpers_before_joining_provider_output test needs helper cleanup, but unconditional post-reap killpg on every successful CLI call is unsafe.
Suggested direction: record the pgid at spawn; only signal the group while members may still exist (e.g. kill before reap via waitid(..., WNOWAIT), or call stop only while stdout/stderr reader threads have not yet seen EOF). Never kill(-id) after a successful reap when the group may already be empty.
There was a problem hiding this comment.
Confirmed. Fixed in 45e4e1e: Unix completion is now observed with waitid(WEXITED | WNOHANG | WNOWAIT), then the owned group is signaled before Child::wait releases the PID. Windows still targets its owned Job Object. A regression verifies that two exit observations leave the leader waitable, and the existing helper-held-pipe test still passes. All 167 Tauri tests pass locally.
| if matches!(child.try_wait(), Ok(Some(_))) { return; } | ||
| #[cfg(windows)] | ||
| { | ||
| if matches!(child.try_wait(), Ok(Some(_))) { return; } |
There was a problem hiding this comment.
risk Windows cancel still skips tree kill once the Git leader has exited
The Unix side of this change correctly keeps signaling the owned group after the leader exits so LFS/submodule helpers cannot retain pipes. On Windows the early try_wait() == Some(_) → return was preserved, so taskkill /T never runs against that dead PID.
Failure mode: the same "leader exited, helper still owns stderr/stdout" cancel scenario this PR regresses on Unix remains on Windows: Cancel can hang until the helper finishes transferring, because descendants are not torn down once the root PID is gone. Consider a Job Object (as in the provider-command path) or another tree kill that does not require the leader to still be alive.
There was a problem hiding this comment.
Confirmed. Fixed in 30658d1: streaming Git retains an owned Windows Job Object through cancellation, so cleanup does not depend on the wrapper PID still running. Git starts suspended, is assigned to the job, then resumes; this also closes the spawn/assignment race for fast Git wrappers. The job wrapper is shared with Tauri and uses owned handles. Added a native Windows regression that reaps the leader while its helper retains stdout, then checks that cancellation closes the pipe. The module cross-compiles locally; the new regression runs in Windows CI on this PR.
|
Reviewed all eight inline comments (four findings, each posted twice).
Local validation: 414 Rust tests pass, plus the focused terminal regression after the final fix; Cargo check and strict Clippy pass. macOS and frontend CI passed on 6e97c73. The final head ad63df6 has a fresh CI run pending. No merge performed. |
1 participant
Summary
Several repository operations could affect files outside the user's selection or destroy unsnapshotted data. For example, discarding
[id].tsxcould also reverti.tsx, and a hard reset could overwrite an untracked file that collides with the target commit.This PR addresses all seven findings from the main-branch audit:
.gitignoretargets and moves into Git metadata or the worktree root.Stronger regression tests also exposed process-cleanup gaps: Unix Git cancellation now kills surviving helpers after the leader exits, and provider commands terminate their owned process tree before draining pipes. Test fixtures now isolate personal signing/LFS settings and use readiness checkpoints for cancellation. Rust CI adds macOS coverage.
Changes are split into fourteen focused commits. The audit report, task list, roadmap, durable learnings, README, and affected user-guide pages record the fixes and remaining work.
Validation
cargo check -p strand-core -p strand-tauripasses.cargo clippy -p strand-core -p strand-tauri -- -D warningspasses.git diff --checkpass.Remaining validation and follow-ups
Packaged-app validation and production performance certification remain outstanding. The Windows reset, cancellation, and provider regression subsets passed on 6e97c73; the final terminal ordering fix has a fresh CI run pending. Historical blob materialization and unbounded provider output reads remain separate follow-ups. See
docs/main-audit-2026-09-29.mdfor evidence and limitations.Review follow-up
Confirmed and repaired the Unix post-reap process-group race and Windows helper cancellation after leader exit. Windows Git is assigned to its Job Object while suspended. The separator findings were false positives because git2 0.19 normalizes Windows paths internally. New nested-path regressions also caught and fixed stale-index collision checks after external Git changes. All inline findings have replies with evidence and fix commits. The Windows CI job now runs reset, cancellation, and provider regression subsets.
Linux CI also reproduced a terminal lifecycle race: Exit was published before removing the session from the registry. The final commit moves notification after removal, with a synchronous observer regression that fails on the original code and passes after the fix.