Skip to content

fix(bridge): resync document tracker on external file changes#223

Merged
bug-ops merged 3 commits into
mainfrom
102-document-tracker-resync
Jul 24, 2026
Merged

fix(bridge): resync document tracker on external file changes#223
bug-ops merged 3 commits into
mainfrom
102-document-tracker-resync

Conversation

@bug-ops

@bug-ops bug-ops commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

DocumentTracker::ensure_open read a file from disk exactly once per session and never noticed later edits made outside mcpls — git checkout/stash, formatters, code generators, or the MCP host's own Edit/Write tools. Every per-file tool (get_hover, get_definition, get_diagnostics, get_document_symbols, etc.) kept serving stale results until the process restarted.

ensure_open now stats the file on every call:

  • An unchanged, mtime-settled snapshot fast-paths without touching the file's bytes.
  • A snapshot that's changed, or not yet "settled" (mtime younger than the filesystem's granularity margin), triggers a content re-read — debounced to at most once per 250ms on the racy path, but the stat itself is never debounced, so a genuine change is always caught on the next call.
  • If the re-read content actually differs, mcpls resyncs the LSP server via a single full-replacement textDocument/didChange (not a didClose+didOpen pair), so the push-diagnostics cache is never transiently wiped mid-resync.

Scope is intentionally limited to this stat-on-access mechanism (issue #102's "Option A"). A filesystem-watcher based approach for workspace/didChangeWatchedFiles (Option C) and a manual reload_workspace escape-hatch tool (Option B) are explicitly out of scope and can be filed separately if wanted.

Design process

This went through two rounds of architecture design + adversarial critique before implementation (see .local/handoff/ on this branch for the full trail). Notably, the critique caught and closed the same class of bug that sank the earlier, abandoned PR #103: a same-size file rewrite completing within the filesystem's mtime tick granularity (1s on ext4/HFS+) would otherwise go undetected. The chosen fix uses a "racily-clean" mtime rule (closed-form, no hashing) plus an exact content comparison as the tie-breaker, using content mcpls needs to read anyway.

Test plan

  • 11 new tests in crates/mcpls-core/src/bridge/state.rs, including a fake dual-process LSP transport that captures real wire traffic (not just tracker-internal state), so the tests prove the LSP server actually receives the notification, not just that the tracker noticed the change.
  • Covers: unchanged-file fast path, resync on content change, the PR fix(#102): handle external file changes via stat-resync, didChangeWatchedFiles, and a populated diagnostics cache #103-class same-tick regression (and its documented residual limitation), the debounce-never-gates-the-stat regression, debounce-gates-only-the-reread, deleted file, oversized file, resync at document-count capacity, and self-heal when the first-open didOpen notification fails.
  • cargo +nightly fmt --all -- --check, cargo clippy --all-targets --all-features --workspace -- -D warnings, cargo nextest run --workspace --all-features --lib --bins (415 passed), and the rustdoc gate all pass.

Documentation

Closes #102

DocumentTracker::ensure_open read a file from disk exactly once per
session and never noticed later edits made outside mcpls (git
checkout/stash, formatters, the MCP host's own Edit/Write tools),
serving stale hover/diagnostics/completion results until restart.

ensure_open now stats the file on every call. An unchanged, settled
snapshot fast-paths without touching the file's bytes; a changed or
not-yet-settled snapshot triggers a content re-read (debounced to at
most once per 250ms on the racy path) and, if the content actually
differs, resyncs the LSP server via a single full-replacement
textDocument/didChange instead of a didClose+didOpen pair, so the
push-diagnostics cache is never transiently wiped.

Closes #102
@github-actions github-actions Bot added documentation Improvements or additions to documentation rust Rust code changes mcpls-core mcpls-core crate changes labels Jul 24, 2026
@bug-ops
bug-ops enabled auto-merge (squash) July 24, 2026 21:24
The set_mtime test helper opened files read-only via File::open, then
called set_modified on that handle. Windows' SetFileTime requires
write access on the handle; Unix's utimensat-based implementation does
not, so this only failed in CI on windows-latest with PermissionDenied
(OS error 5) across every ensure_open test that backdates an mtime.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates mcpls’ bridge document tracking so DocumentTracker::ensure_open detects external on-disk edits and resynchronizes the LSP server (via a full-replacement textDocument/didChange) instead of serving stale analysis for the remainder of the session.

Changes:

  • Add stat-on-access + debounced re-read logic in DocumentTracker::ensure_open, including disk snapshot tracking (DiskSync) and resync via didChange.
  • Add/expand tests covering unchanged fast-paths, resync behavior, debounce behavior, edge cases (deleted/oversize files), and notification failure recovery.
  • Document the new behavior and its limitations in the user troubleshooting guide and in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
docs/user-guide/troubleshooting.md Adds a new “External file changes” troubleshooting section describing behavior, limitations, and diagnostics semantics.
crates/mcpls-core/src/bridge/state.rs Implements disk snapshot tracking + resync logic in ensure_open, plus extensive tests.
CHANGELOG.md Documents the fix (#102) and notes the DocumentState breaking API change.
Comments suppressed due to low confidence (1)

crates/mcpls-core/src/bridge/state.rs:419

  • resync reads file metadata and then reads content in a separate call (fs::read_to_string). This leaves a TOCTOU window where (mtime, size) used for DiskSync can diverge from the actual bytes read and compared, which can corrupt the snapshot and (in edge cases) defeat the intended size-limit precheck. Prefer opening the file once on the verify path and using that handle for both metadata and content read.
        self.check_file_size(size)?;
        let fresh = fs::read_to_string(path).await.map_err(|e| Error::FileIo {
            path: path.to_path_buf(),
            source: e,
        })?;

Comment thread crates/mcpls-core/src/bridge/state.rs
Comment thread crates/mcpls-core/src/bridge/state.rs
Comment thread crates/mcpls-core/src/bridge/state.rs Outdated
Comment thread crates/mcpls-core/src/bridge/state.rs
Comment thread docs/user-guide/troubleshooting.md Outdated
Comment thread CHANGELOG.md Outdated
…tation

The doc comment and troubleshooting entry both implied the
restored-mtime-and-size gap only applies "within a short window" or
"the racy detection window" after a file was read. That's inaccurate:
once a DiskSync snapshot is mtime_settled, the fast path trusts a
matching (mtime, size) indefinitely, so restoring an old snapshot's
exact values is missed regardless of how much time has passed.

Also add the missing (#102) reference to the DocumentState breaking-
change changelog entry, consistent with the other Unreleased entries.
@bug-ops
bug-ops enabled auto-merge (squash) July 24, 2026 21:45
@bug-ops
bug-ops merged commit 35f65b3 into main Jul 24, 2026
27 checks passed
@bug-ops
bug-ops deleted the 102-document-tracker-resync branch July 24, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation mcpls-core mcpls-core crate changes rust Rust code changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document tracker never re-reads files after first didOpen — every external edit yields stale results

2 participants