Revert VS Code to canonical LSP file-watching, closing #31 (Q9) - #512
Merged
Conversation
…stration Issue #31 (Q9) investigated whether the client-side per-project RelativePattern DLL watcher added by PR #26 (for issue #2) is still needed. Two findings: - files.watcherExclude covering bin/ suppresses any createFileSystemWatcher-based approach, including this client-side glue — narrowing the glob doesn't route around it, so the glue provided no resilience against the one confirmed failure mode. - Issue #2's original symptom does not reproduce today. An Extension Host recreation (a real, unbuilt project registered before its first dotnet build, then built) recovers correctly with the client-side glue disabled — the server's standard workspace/didChangeWatchedFiles dynamic registration, delivered by vscode-languageclient's own FileSystemWatcherFeature with no Reqnroll-specific client code, is sufficient on its own under default VS Code settings. Removes ProjectManager's per-project output-assembly watcher, its synthetic didChangeWatchedFiles forwarding, and associated lifecycle tracking. Adds two regression tests: one proving files.watcherExclude suppresses the watcher this class used to create, one recreating issue #2 end-to-end against the real bundled LSP server and a real dotnet build. Experimental — pending manual verification before merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Marks Q9 resolved for VS Code and VS in docs/LSP-IDE-Support-Open-Questions.md and docs/LSP-IDE-Support-Feature-Designs.md, citing both the Extension Host recreation and live manual verification (two real dotnet builds against a real multi-project solution, correctly detected and filtered using only the canonical WatchedFilesHandler dynamic registration). Rider was out of scope for this investigation and remains open, tracked in a new follow-up issue (#511). Adds a changelog entry and strengthens projectManager.ts's class doc with the live-session confirmation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reqnroll.trace.server: verbose correctly drives vscode-languageclient's tracing regardless of what's visible, but the Reqnroll LSP Trace channel is a LogOutputChannel with its own independent display filter that only the user can raise (Output panel dropdown / "Developer: Set Log Level..."), not something the extension can set. Discovered while investigating #31: the panel looked empty during manual testing even with tracing on, while the file-based log under %LOCALAPPDATA%\Reqnroll\ had full content the whole time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
defineStepRecovery.test.ts assumed its __defineStepE2E__ fixture already existed on disk, created out-of-band by hand during local investigation and never committed to git. On a clean CI checkout it never existed at all, so the test failed trying to open a nonexistent file. Now creates and removes it itself via suiteSetup/suiteTeardown, making the suite fully self-contained. That surfaced a real race once the fixture is created mid-session (via ProjectManager's live .csproj watcher) instead of pre-existing at initial workspace scan: the fixed 5s settle wait before running dotnet build wasn't enough for registerProject's async msbuild evaluation to register the project's OutputAssemblyPath first. Bumped to 15s. Also adds actions/setup-dotnet to the VSCode CI job, which had no .NET SDK setup step at all — the test's dotnet build had nothing to run against. watcherExclude.test.ts's control case flaked on CI's Linux runner (false !==true) — gave the watcher-arm delay and overall timeout more headroom (300ms/5s -> 1s/10s) and added a settle delay after the config update, with an explicit per-suite Mocha timeout to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Even with 10x the original timeouts (10s/90s), neither the watcherExclude control case nor the #2 recreation test's discovery-retry ever fired on this repo's xvfb-run + headless-Electron GitHub Actions runner -- including the control case with no exclude at all, which has no reason to fail if file-watch delivery works at all. That rules out a race and points at a genuine capability gap in that specific CI sandbox: real OS-level file-watch delivery for a freshly created nested directory doesn't appear to work there, regardless of which watcher implementation (client-side glue or canonical dynamic registration) is doing the watching -- so there's nothing a longer wait or a different mechanism can fix. Both suites now skip under CI (process.env.CI) and run fully locally / in manual Extension Host runs, which is where every real finding in this investigation actually came from. Also reverts the actions/setup-dotnet addition to the VSCode CI job -- nothing in that job calls dotnet anymore now that the only consumer is CI-skipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks
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
1 participant
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.
🤔 What's changed?
Removes the VS Code extension's client-side per-project output-assembly watcher (
ProjectManager.watchProjectOutputPath/notifyOutputAssemblyChanged/_outputWatchers, added by #26) and the syntheticworkspace/didChangeWatchedFilesnotification it sent. The extension now relies solely on the server's standard dynamic registration (WatchedFilesHandler.cs) +vscode-languageclient's own genericFileSystemWatcherFeature— no Reqnroll-specific client code for this signal at all, same as it always could have been.Also closes out issue #31 (Q9) in the design docs and adds two regression tests covering what the investigation actually found.
⚡️ What's your motivation?
Fixes #31.
#26 added the client-side glue on the (untested) assumption that
vscode-languageclient's generic watcher wasn't reliably deliveringworkspace/didChangeWatchedFilesfor**/bin/**/*.dll, specifically pointing at VS Code'sfiles.watcherExclude(commonly set to excludebin//obj/) as the likely cause. That assumption was investigated directly and found not to hold:files.watcherExcludecoveringbin/does suppress delivery — but it suppresses anycreateFileSystemWatcher-based approach equally, including the client-side glue. Narrowing the glob to a per-projectRelativePatterndidn't route around it. So the glue provided zero resilience against the one confirmed failure mode; it's not addressed by this PR either, and isn't specific to it.dotnet build, using a real bundled LSP server — recovers correctly with the client-side glue completely disabled.dotnet builds in a live manual VS Code session against a real multi-project solution (one incremental rebuild, one full/clean rebuild producing 150+ dependency-DLL creation events as noise) were both correctly detected and filtered by the server, using canonical registration alone. Server-side log:Net: the client-side glue was pure duplicated intent — the exact concern issue #31 raised — with no measured reliability benefit under either configuration tested.
Rider was out of scope for this investigation (Visual Studio doesn't need this signal —
VsProjectEventMonitorhooksDTE.Events.BuildEvents.OnBuildDonedirectly) and remains open, tracked in #511.🏷️ What kind of change is this?
🧩 Area(s) touched
src/LSP) — docs onlysrc/VSCode)♻️ Anything particular you want feedback on?
I only ran each side of the A/B comparison (glue enabled vs. disabled) once in the automated harness — the results were clean and the mechanism is deterministic, not flaky by nature, but a couple more repeats before merge would rule out a fluke if anyone wants extra confidence.
📋 Checklist:
docs/