diff --git a/docs/active-work.md b/docs/active-work.md index c2b19b3..c325c73 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -210,6 +210,63 @@ hazard in a shape that looks committed. **A documented error message that never appears is worse than no documentation**, because the reader waits for a signal that is not coming. +## LSP file watcher (issue #233) — branch OPEN, framing revision 1 AWAITING REVIEW + +**Issue #233** — https://github.com/levineuwirth/pmacs/issues/233. +**URGENT by user ruling 2026-08-10: PR #227 is held unmerged until this +is resolved**, even though #227 is green and cleared. + +- **Branch `lsp-file-watcher`**, base `githubsucks/main` @ `0e4c58d`. + **`githubsucks/lsp-file-watcher` is the authoritative tip** — the ref, + not a SHA. Recover with + `git fetch githubsucks && git checkout lsp-file-watcher`. +- **Framing `docs/lsp-file-watcher-framing.md`, revision 1, DRAFT.** + Committed at the branch's first commit so it is portable during + review — the standing lesson from the GUI arc framing, which spent two + review rounds as an untracked file in one worktree. +- **NO IMPLEMENTATION MAY START** until revision 1 is approved *and* the + open ruling below is answered. + +**Not a #232 regression.** The statusline activity indicator is correct; +it renders real in-flight jobs. What changed on 2026-08-09 is +**visibility**. The polling watcher has been there since `1c25730` +(2026-05-19). Quieting the indicator would delete the only instrument +that found this, and is explicitly not the fix. + +- **Scope: D1 and D2 only** — plain-string globs are matched against + relative paths so they never match (rust-analyzer sees **no** file + changes at all; gopls sees `go.mod` but never `.go`), and + re-registering the same id leaks the previous coroutines + (rust-analyzer registers twice under one id → 12 pollers, 6 + permanently uncancellable). +- **Two findings from framing that the issue does not carry**, both of + which change the fix: + 1. **The existing test cannot discriminate this fix in either + direction.** The fake LSP's `**/*.txt` compiles to `^.-[^/]*%.txt$` + and `.-` spans `/`, so it matches relative *and* absolute subjects. + `m4_24` passes whether D1 is fixed or broken. New coverage must use + a pattern whose two readings disagree. + 2. **"Match the absolute path" alone would break `RelativePattern`.** + Measured: `*.txt` matches `a.txt` but not `/base/a.txt`. + `resolve_watcher` returns `(base, pattern)` and discards which form + it came from, so its contract has to change — not just the match + subject. +- **OPEN RULING, blocking the start of implementation:** D1 and D2 fix + correctness and the leak but **do not stop the walking** — `walk` + recurses unconditionally and `matches` gates only recording. After + this lane rust-analyzer still walks the whole tree every 250 ms, six + times per tick instead of twelve. If the acceptance bar is "the + modeline stops flipping", this lane does not meet it and **D3 must be + framed first**. +- **D3 deferred** with what was checked: there is **no `notify`/inotify + dependency in the tree**, so a real filesystem-notification primitive + is a new crate plus a Rust primitive plus its binding; and there is + **no ignore-list infrastructure** to reuse (`src/project.rs` knows + `.git` as a marker, not as something to skip). D3 is a + `COHERENCE.md` §9 concern and needs its own framing. +- **Gates:** `./scripts/gate --acceptance m4_acceptance` plus the + touched LSP acceptance suites. No `--protocol` — no wire change. + ## `scripts/gate` — PR #225 OPEN (build tooling) **PR #225** — https://github.com/levineuwirth/pmacs/pull/225. Written diff --git a/docs/lsp-file-watcher-framing.md b/docs/lsp-file-watcher-framing.md new file mode 100644 index 0000000..04eae7c --- /dev/null +++ b/docs/lsp-file-watcher-framing.md @@ -0,0 +1,197 @@ +# LSP file watcher — framing + +**Status: revision 1 — DRAFT, awaiting review. No implementation may +begin from this document.** + +Answers issue #233. **Scope is D1 and D2 only** — the two bug-shaped +defects. D3 (the polling cost) is named here, deferred with reasons, and +gets its own framing. + +## What is and is not a regression + +**#232 is not at fault and nothing about it should be reverted.** The +statusline activity indicator it added is *correct*: it renders real +in-flight jobs from `AsyncRuntime::activity_summary`, and the jobs it +names (`sleep 250ms`, `read_dir `) are real. What changed on +2026-08-09 is **visibility**, not behaviour. + +The behaviour has been there since `1c25730` (2026-05-19). So the user- +facing report — "the modeline flips several times a second" — is a +three-month-old defect that became observable last week, and the fix +belongs to the watcher, not the indicator. + +Recorded plainly because the tempting move is to quiet the indicator, +and that would delete the only instrument that found this. + +## Verified against the tree at `0e4c58d` + +Every claim below was read or executed this session, not carried from +the issue. + +- `FILE_WATCH_INTERVAL_MS = 250` (`lsp.lua:1924`); each watcher is one + `pmacs.async` coroutine looping sleep → `scan_tree` + (`lsp.lua:2060-2097`). +- `scan_tree` builds `rel` from an empty prefix and calls + `matches(rel)` — **relative** paths (`lsp.lua:2035-2056`). +- **`walk` recurses into every directory unconditionally.** `matches` + gates only whether an entry is *recorded*. A watcher that can never + match still walks the whole tree every tick. +- `resolve_watcher`'s string branch returns the pattern **unchanged** + with the base guessed from an attached file's directory + (`lsp.lua:2102-2116`). +- `register_file_watchers` ends `file_watchers[skey][reg.id] = recs` + with no cancellation of the outgoing list (`lsp.lua:2132`). +- Job purposes are `format!("sleep {}ms", …)` (`async_runtime.rs:1027`) + and `format!("read_dir {}", …)` (`:1178`). +- The fake LSP registers **one** watcher, a `RelativePattern` + `{ baseUri, pattern: "**/*.txt" }`, id `watch-1` + (`pmacs_fake_lsp.rs:312-331`). + +### The glob table, reproduced + +Ran the tree's own `expand_braces` / `glob_one_to_pattern` / +`glob_matcher` under LuaJIT. Output matches the issue exactly, compiled +patterns included: + +| glob | compiled | `main.go` | `go.mod` | absolute | +|---|---|---|---|---| +| `**/*.{mod,work}` | `^.-[^/]*%.mod$` | false | **true** | true | +| `/goproj/**/*.{go,…}` | `^/tmp/goproj/.-[^/]*%.go$` | false | false | true | +| `/rsproj/**/*.rs` | `^/tmp/rsproj/.-[^/]*%.rs$` | false | false | true | + +## Two findings the issue does not carry, both of which shape the fix + +### F1 — the existing test cannot discriminate this fix, in either direction + +`**/*.txt` compiles to `^.-[^/]*%.txt$`, and `.-` spans `/`. Measured: +it matches `a.txt`, `sub/a.txt`, `/base/a.txt` **and** +`/base/sub/a.txt`. So `m4_24_workspace_did_change_watched_files` passes +whether the matching subject is relative or absolute. + +The issue says the tested path and the exercised path are disjoint. The +sharper statement is that the existing test is **insensitive**: it +cannot fail for D1 and it cannot confirm D1's fix. New coverage must use +a pattern whose two readings disagree, or it will inherit the same +blindness. + +### F2 — the fix cannot simply "match absolute"; the form must be carried + +Per LSP, a plain-string glob matches the **absolute** path while a +`RelativePattern`'s pattern is relative to **its base**. Matching +everything absolutely breaks the second. Measured on `*.txt`: + +| subject | matches | +|---|---| +| `a.txt` (relative, correct for RelativePattern) | **true** | +| `/base/a.txt` (absolute) | **false** | + +`resolve_watcher` returns `(base, pattern)` and **discards which form it +came from**, so both callers below it are already unable to tell. The +fix therefore changes that function's contract — a third return value or +an explicit record field — rather than only changing the subject string +at the match site. A fix that ignores this trades rust-analyzer's six +broken globs for every `RelativePattern` whose pattern does not begin +`**/`. + +## D1 — plain-string globs never match + +**Consequences, as measured in the issue and confirmed by the table +above:** rust-analyzer is never told about any file change (all six +globs absolute); gopls is told about `go.mod`/`go.work` but never `.go` +sources (only its relative glob matches). + +**Fix:** match a plain-string glob against `base .. "/" .. rel`; keep a +`RelativePattern` matched against `rel`. `resolve_watcher` gains the +form in its return, and the record carries it. + +The leading `**/` in gopls' relative glob compiles to `.-`, which spans +`/`, so that glob keeps matching under the absolute subject — which is +why one server's working case does not regress. + +## D2 — re-registration leaks the previous coroutines + +`file_watchers[skey][reg.id] = recs` replaces the record list without +setting `cancelled` or cancelling the in-flight `_sleep`. The old +coroutines poll until the server dies and are unreachable by +`unregister_file_watchers`, which can only see what the table now holds. + +**Reachable today**: rust-analyzer registers +`workspace/didChangeWatchedFiles` **twice under the same id**, six +watchers each, with no intervening unregister — 12 concurrent +coroutines, six permanently uncancellable. The issue's 44.1/s dir-open +rate against a ~270 ms period implies 12 watchers, so the leak is +measured from outside the process, not only read from the source. + +**Fix:** cancel the outgoing list before replacing it, with the same +treatment `unregister_file_watchers` already applies. + +## What this lane does NOT fix, stated so the report is not mistaken for closed + +**The poll cost survives both fixes.** D1 makes matching correct and D2 +halves rust-analyzer's watcher count; neither stops the walk. After this +lane, rust-analyzer still walks the entire tree every 250 ms — six times +per tick instead of twelve — including `.git`, `target` and +`node_modules`, at one async job per directory. + +So the modeline will still show activity, at roughly half the rate. **If +the acceptance bar for this lane is "the flipping stops", this lane does +not meet it** and should not be started until D3 is framed. That is a +ruling for the user, not an assumption to make quietly. + +## D3 — deferred, with what was checked + +Options named in the issue: coalesce a server's watchers into one scan; +root the scan at the workspace rather than an attached file's directory; +an ignore list; back off when nothing changes; or a real +filesystem-notification primitive. + +Checked while framing: **there is no `notify`/inotify dependency in the +tree**, so the last option is a new crate *and* a new Rust primitive +plus its Lua binding — not a small change. There is also **no existing +ignore-list infrastructure** to reuse; `src/project.rs` knows `.git` as +a *marker* name, not as something to skip. + +D3 is a `COHERENCE.md` §9 concern — background work with no ownership +model — and §9's own Stage 1 is the indicator that surfaced it. + +## Verification + +The suite must fail without each fix, which the existing suite cannot +(F1). Planned: + +- **A fake-LSP mode registering a plain-string ABSOLUTE glob**, with a + pattern whose relative and absolute readings **disagree** — so the + test fails today and passes after D1. +- **A fake-LSP mode registering a `RelativePattern` whose pattern does + not begin `**/`** (e.g. `*.txt` at the base). This is F2's guard: it + passes today, and fails against a fix that matches everything + absolutely. Without it, the obvious wrong fix is green. +- **A re-registration mode**: the same id twice, no unregister. The + witness is that the superseded watchers **stop**, asserted on + observable polling rather than on internal table shape, since the + defect is precisely that the old records are unreachable. +- Existing `m4_24` kept and expected **unchanged** — it covers the + working branch and its insensitivity is now recorded rather than + mistaken for coverage. + +Each new test is mutation-tested against the fix it names. + +## Coherence impact (§20) + +- **Journey steps**: none added; step 5's editing surface is affected + only in that a correct watcher makes servers see edits they currently + miss. +- **Interaction islands**: none. +- **Config registry**: no new setting. The interval stays a module + constant; making it configurable would offer the user a knob for a + defect rather than a preference, and D3 may remove the poll entirely. +- **Background-work attribution (§9)**: this lane *reduces* unattributed + background work but does not model it. D3 owns that, and the honest + statement is that the indicator worked — it made three months of + invisible churn visible on its first week. + +## Gates + +`./scripts/gate --acceptance m4_acceptance` plus the touched LSP +acceptance suites; no `--protocol` (no wire change, no +`PROTOCOL_VERSION` bump).