Fix watcher reconciliation regressions - #124
Open
Shengyu Fu (shengyfu) wants to merge 12 commits into
Open
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… |
 |
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… |
What changed in this PR
Fixes watcher reconciliation regressions involving incomplete traversal, Git index changes, and unnecessary tombstones.
Changes:
- Preserves subscriptions during incomplete directory traversal.
- Polls tracked-file exemptions and reconciles forced Git additions/removals.
- Avoids tombstones without evidence and clarifies platform behavior.
| File | Description |
|---|---|
tgrep-core/src/reader.rs |
Adds efficient reader path membership checks. |
tgrep-core/src/hybrid.rs |
Exposes reader membership through HybridIndex. |
tgrep-core/src/gitignore.rs |
Tracks exemption membership fingerprints. |
tgrep-core/src/git_index.rs |
Adds fingerprints and linked-worktree handling. |
tgrep-cli/src/serve.rs |
Updates polling, reconciliation, subscriptions, and tombstones. |
tgrep-cli/tests/watcher_watch_registration.rs |
Tests Git index reconciliation. |
README.md |
Documents platform-specific watcher behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View comment |
|
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… View comment |
Suppressed comments (1)
tgrep-core/src/git_index.rs:159
- Linked worktrees can enable
extensions.worktreeConfigand overridecore.ignoreCasein$GIT_DIR/config.worktree. Reading only the common config therefore applies the shared value even when this worktree's effective value differs, activating or disabling the case-insensitive matcher and index polling incorrectly. Please apply Git's common-config-then-worktree-config precedence here.
let Ok(config) = std::fs::read_to_string(common_git_dir(&git_dir).join("config")) else {
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 772a4aa3-4cba-4ac4-aeca-dd184614127a
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
tgrep-core/src/gitignore.rs — This eagerly reads the complete Git index and matches every tracked path whenever a… |
|
tgrep-core/src/builder.rs — Treat None as the caller's supplied snapshot here. Falling back to walk_dir reconstructs… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View comment |
|
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… View comment |
Route RPC reloads through coordinated tracked-membership snapshots while preserving live public matcher behavior. Stage and atomically publish reload generations with rollback and cache invalidation guards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 772a4aa3-4cba-4ac4-aeca-dd184614127a
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
tgrep-core/src/gitignore.rs — The polling cache retains the newly parsed TrackedFiles even though this path only returns its… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View comment |
|
tgrep-core/src/builder.rs — Treat None as the caller's supplied snapshot here. Falling back to walk_dir reconstructs… View comment |
|
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
tgrep-core/src/gitignore.rs — This eagerly reads the complete Git index and matches every tracked path whenever a… View resolved comment |
Validate stable owned reads before publishing stamps across full builds, stale deltas, startup recovery, reload, and watcher updates. Retry incomplete startup reconciliation and preserve public builder APIs and stamp compatibility. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 772a4aa3-4cba-4ac4-aeca-dd184614127a
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 4
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
|
tgrep-core/src/builder.rs — --no-max-filesize allows a file larger than the 64 MiB batch budget, but this now reserves the… |
|
tgrep-cli/src/serve.rs — On Windows, rename cannot replace an existing destination. During rollback the target still… |
|
tgrep-core/src/builder.rs — stamp.mtime has only whole-second precision. On Windows, created_nanos does not change for an… |
|
tgrep-cli/src/serve.rs — These stability checks compare only FileStamp (whole-second mtime plus size). A same-size write… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View comment |
|
tgrep-core/src/gitignore.rs — The polling cache retains the newly parsed TrackedFiles even though this path only returns its… View comment |
|
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
tgrep-core/src/builder.rs — Treat None as the caller's supplied snapshot here. Falling back to walk_dir reconstructs… View resolved comment |
Suppressed comments (1)
tgrep-core/src/git_index.rs:159
- Reading only the common config does not yield the effective value for linked worktrees when
extensions.worktreeConfigis enabled:<git-dir>/config.worktreecan overridecore.ignorecase. If common config saystrueand this worktree overrides it tofalse, this function still enables case-insensitive filtering and tracked exemptions incorrectly. Apply the worktree config after the common config (or use Git-compatible config resolution).
let Ok(config) = std::fs::read_to_string(common_git_dir(&git_dir).join("config")) else {
Restore the mmap-backed builder and bounded batch accounting, then repair reload and startup publication through deferred event replay, no-watch catch-up, and stable containment-safe concrete event reads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 772a4aa3-4cba-4ac4-aeca-dd184614127a
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 4
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — tracked_membership_changed updates the observed baseline before this branch. If the no-watcher… |
|
tgrep-cli/src/serve.rs — For every missing deferred path, this clones every reader path into a HashSet and every overlay… |
Pre-existing issues (7)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — These stability checks compare only FileStamp (whole-second mtime plus size). A same-size write… View comment |
|
tgrep-core/src/builder.rs — stamp.mtime has only whole-second precision. On Windows, created_nanos does not change for an… View comment |
|
tgrep-cli/src/serve.rs — On Windows, rename cannot replace an existing destination. During rollback the target still… View comment |
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View comment |
|
tgrep-core/src/builder.rs — --no-max-filesize allows a file larger than the 64 MiB batch budget, but this now reserves the… View comment |
|
tgrep-core/src/gitignore.rs — The polling cache retains the newly parsed TrackedFiles even though this path only returns its… View comment |
|
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… View comment |
Suppressed comments (1)
tgrep-core/src/git_index.rs:159
- Reading only the common config does not yield the effective
core.ignorecasefor repositories usingextensions.worktreeConfig:$GIT_DIR/config.worktreecan override the common value for this linked worktree. That can enable or disable the tracked-file exemption and index polling incorrectly for a worktree. Apply the worktree config after the common config (including Git's normal precedence) before deciding this value.
let Ok(config) = std::fs::read_to_string(common_git_dir(&git_dir).join("config")) else {
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 2
New issues introduced by this change (4)
| Severity | Finding |
|---|---|
|
tgrep-core/src/gitignore.rs — The semantic fingerprint misses tracked paths whose final file match is a whitelist but whose… |
|
tgrep-cli/src/serve.rs — When this no-watch catch-up fails, no later retry is actually scheduled: the periodic reconcile… |
 |
tgrep-cli/src/serve.rs — For an incomplete traversal, desired.dirs is only the proven subset; sync deliberately… |
|
tgrep-core/src/builder.rs — This is only a per-file cap, not the claimed batch-wide owned-buffer bound. Files above… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View comment |
Issues resolved since last review (8)
| Severity | Finding |
|---|---|
|
tgrep-cli/src/serve.rs — For every missing deferred path, this clones every reader path into a HashSet and every overlay… View resolved comment |
|
tgrep-cli/src/serve.rs — tracked_membership_changed updates the observed baseline before this branch. If the no-watcher… View resolved comment |
|
tgrep-cli/src/serve.rs — These stability checks compare only FileStamp (whole-second mtime plus size). A same-size write… View resolved comment |
|
tgrep-core/src/builder.rs — stamp.mtime has only whole-second precision. On Windows, created_nanos does not change for an… View resolved comment |
|
tgrep-cli/src/serve.rs — On Windows, rename cannot replace an existing destination. During rollback the target still… View resolved comment |
|
tgrep-core/src/builder.rs — --no-max-filesize allows a file larger than the 64 MiB batch budget, but this now reserves the… View resolved comment |
|
tgrep-core/src/gitignore.rs — The polling cache retains the newly parsed TrackedFiles even though this path only returns its… View resolved comment |
|
tgrep-core/src/git_index.rs — This reads only the common config, but linked worktrees can enable extensions.worktreeConfig and… View resolved comment |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96b9f8ae-c5ec-450c-a003-cb9ade61d9df
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
tgrep-core/src/builder.rs — BuildOptions::max_file_size documents that None indexes files of any size, but this introduces… |
Issues resolved since last review (5)
| Severity | Finding |
|---|---|
|
tgrep-core/src/builder.rs — This is only a per-file cap, not the claimed batch-wide owned-buffer bound. Files above… View resolved comment |
|
tgrep-cli/src/serve.rs — For an incomplete traversal, desired.dirs is only the proven subset; sync deliberately… View resolved comment |
|
tgrep-cli/src/serve.rs — When this no-watch catch-up fails, no later retry is actually scheduled: the periodic reconcile… View resolved comment |
|
tgrep-core/src/gitignore.rs — The semantic fingerprint misses tracked paths whose final file match is a whitelist but whose… View resolved comment |
|
tgrep-cli/src/serve.rs — An incomplete pass still returns true here, so sync force-re-registers every proven,… View resolved comment |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96b9f8ae-c5ec-450c-a003-cb9ade61d9df
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96b9f8ae-c5ec-450c-a003-cb9ade61d9df
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
tgrep-core/src/builder.rs — BuildOptions::max_file_size documents that None indexes files of any size, but this introduces… View resolved comment |
Suppressed comments (2)
tgrep-core/src/builder.rs:467
- The configured file-size cap is checked only after the successful-mmap return. If a file was below the cap during the walk but grows past it before extraction,
sizereflects the larger file and a successful mmap indexes it anyway. Move the existing limit check ahead of the mmap attempt soBuildOptions::max_file_sizeremains authoritative during this race.
let mapped =
std::fs::File::open(path).and_then(|file| unsafe { memmap2::Mmap::map(&file) });
tgrep-core/src/builder.rs:475
- The owned-buffer budget is bypassed for every file larger than 64 MiB:
oversizedsuppresses the permit, whileowned_limitdefaults tosize, so an mmap failure under--no-max-filesizereads the entire file into an unchargedVec. A multi-gigabyte file can therefore exhaust memory despite this budget. Cap or reject the owned fallback atMAX_OWNED_FILE_BYTESwhile continuing to permit larger files when mmap succeeds.
if size > owned_limit {
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidData,
| path: &Path, | ||
| size: u64, | ||
| owned_budget: &std::sync::Arc<OwnedReadBudget>, | ||
| configured_limit: Option<u64>, |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 96b9f8ae-c5ec-450c-a003-cb9ade61d9df
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #105 and targets
shengyfu-watcher-respect-ignore-files.git add -fandgit rm --cached.Validation:
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace