pmacs/docs/lsp-file-watcher-framing.md

# LSP file watcher — framing

**Status: revision 2 — approved design (2026-08-10), plus two
correctness findings from review OF THE IMPLEMENTATION.** The user ruled
that D1 and D2 proceed with the walking explicitly surviving this lane;
D3 gets its own framing. The acceptance bar for this lane is correctness
and the leak, not "the flipping stops".

**Revision 2 records a review round against the code, not the design.
Both findings are cases where the first fix was itself wrong**, and both
were confirmed against the tree before being acted on:

- **P1 — the form must be read from the PATTERN, not from the union
  arm.** `resolve_watcher` returned `"absolute"` for *every* string, so
  a bare `*.txt` — a valid relative pattern under LSP 3.17, and how VS
  Code treats string watchers across workspace folders — was matched
  against `<base>/foo.txt` and could never fire. **That case worked
  before this lane touched it**, so the repair for #233 silently broke a
  live path while fixing another. A leading `/` is what makes a pattern
  absolute; classification now reads the string.
- **P2 — a scan completing after cancellation still emitted.**
  `scan_tree` awaits `read_dir` once per directory, so the coroutine
  sits suspended for most of a tick with `_sleep` already cleared. A
  cancel arriving there sets `cancelled` and has no sleep to interrupt,
  and the resumed scan ran on to `did_change_watched_files` — one stale
  batch under the superseded pattern, which is a wrong-pattern
  notification the server acts on. Cancellation and liveness are now
  rechecked after the scan.

**F1's lesson repeated itself inside this lane.** The flat-pattern test
constrains the RelativePattern **object** arm, so it said nothing about
the **string** arm P1's regression lived in — the same
tested-path/exercised-path split this framing opened by naming. Both
findings now have tests, and both tests were mutation-checked: each
fails only its own defect.

**A test seam was added, and is recorded here rather than buried.**
`pmacs.lsp._after_scan_for_tests` is a production hook, nil in normal
operation, that P2's witness requires: the race is a cancel landing
during one of the scan's suspensions, which no arrangement of real
timing produces on demand. Same device and justification as `git.lua`'s
`_deliver_status`. It is handed the scan result deliberately — a test
that cancels on any *other* scan passes with the fix deleted, because
the loop would break at the post-sleep check and emit nothing anyway.

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 <path>`) 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 |
| `<abs>/goproj/**/*.{go,…}` | `^/tmp/goproj/.-[^/]*%.go$` | false | false | true |
| `<abs>/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.

**Answered 2026-08-10: the user accepted this scope.** D1 and D2
proceed; the walking is D3's problem, framed separately.

## 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).