neuwirth
/
pmacs
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(lsp): read the glob form from the pattern, and stop a cancelled scan emitting 

Review of the #233 implementation found two correctness defects. Both
are cases where the FIRST fix for #233 was itself wrong, which is worth
naming: this lane repaired absolute globs and, in the same change, broke
a case that had worked since May.

P1 --- `resolve_watcher` returned "absolute" for EVERY string, so the
form was carried but derived from the union arm rather than from the
pattern. A bare `*.txt` is a valid relative pattern (LSP 3.17 defines
`Pattern` relative to a base path; VS Code treats string watchers as
applying across workspace folders), and classifying it absolute matched
it against `<base>/foo.txt`, which `^[^/]*%.txt$` can never match. A
leading `/` is what makes a pattern absolute. The `filewatchflat` test
could not catch this: it sends the RelativePattern OBJECT form, so it
constrains the object arm, never the string arm the regression lived in
--- the framing's own F1 finding, repeating inside the lane that named
it.

P2 --- `scan_tree` awaits `read_dir` once per directory, so the watcher
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 rechecked
after the scan.

Both tests are mutation-checked and each bite fails only its own defect.
Reverting P1 fails the bare-string test alone --- the absolute test
still passes, so the two readings are independent. Deleting P2's recheck
reproduces the defect verbatim: `.received = "1 file:///…/foo.txt"`,
a batch from a watcher that was already cancelled.

P2's witness needs a seam. The race is a cancel landing during one of
the scan's suspensions, which no arrangement of real timing produces on
demand --- the same situation, and the same device, as `git.lua`'s
`_deliver_status`. `pmacs.lsp._after_scan_for_tests` is nil in
production and is handed the SCAN RESULT deliberately: a test that
cancels on any other scan passes with the recheck deleted, because the
loop would break at the post-sleep check and emit nothing anyway.

Gates: 9/9 green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
Levi Neuwirth 2026-08-11 09:00:15 +02:00
parent 0723017754
commit 2a16e0eed5
No known key found for this signature in database
5 changed files with 304 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
builtin/runtime/lsp.lua
View File

@ -2081,6 +2081,29 @@ local function start_file_watcher(sid, base, glob, kind_mask, record)
if record.cancelled or not server_is_live(sid) then break end
local cur = scan_tree(base, matches)
-- The seam that makes the recheck below WITNESSABLE. `scan_tree`
-- suspends on `read_dir` once per directory, and the race is a
-- cancel arriving during one of those suspensions --- which no
-- arrangement of real timing can be made to happen on demand.
-- Same reason `git.lua` exposes `_deliver_status`: the contract is
-- about an interleaving the caller does not choose. Unset in
-- production, so this costs one nil test per tick.
-- `cur` is handed over so a test can cancel on THE SCAN THAT
-- OBSERVED a given change. Cancelling on any other scan is not a
-- witness: the loop would break at the post-sleep check on the
-- next iteration and emit nothing anyway, so the assertion would
-- pass with the recheck below deleted.
if pmacs.lsp._after_scan_for_tests then
pcall(pmacs.lsp._after_scan_for_tests, record, cur)
end
-- RECHECKED AFTER THE SCAN, not only after the sleep (review P2).
-- The coroutine is suspended for most of a tick with `_sleep`
-- already cleared, so a cancel landing there sets `cancelled` and
-- has no sleep to interrupt. Without this line the resumed scan
-- runs on to `did_change_watched_files` below and a SUPERSEDED
-- watcher emits one last batch under its OLD pattern. One batch is
-- enough: it is a wrong-pattern notification the server acts on.
if record.cancelled or not server_is_live(sid) then break end
local changes = {}
for rel, sig in pairs(cur) do
local was = prev[rel]
@ -2110,11 +2133,20 @@ end
-- Resolve a GlobPattern (string | { baseUri, pattern }) to
-- (base_dir, pattern, form). The form must travel with the pair: a
-- RelativePattern's pattern is relative to its baseUri, but a bare
-- string matches the file's ABSOLUTE path (real servers send absolute
-- globs), and dropping the distinction here is what made those globs
-- unable to match anything. A bare string with no base falls back to
-- the directory of an attached file on `sid` (best effort).
-- RelativePattern's pattern is relative to its baseUri, and dropping
-- that distinction is what made absolute server globs unable to match
-- anything. A bare string with no base falls back to the directory of
-- an attached file on `sid` (best effort).
--
-- THE FORM COMES FROM THE PATTERN, NOT FROM THE UNION ARM (review P1).
-- The first fix for #233 returned `"absolute"` for every string, which
-- is a different bug wearing the same shape: LSP 3.17 defines `Pattern`
-- relative to a base path, and VS Code treats a string watcher as
-- applying across workspace folders, so a bare `*.txt` is a VALID
-- relative pattern. Classifying it absolute matched it against
-- `<base>/foo.txt`, which `^[^/]*%.txt$` can never match --- so that
-- fix silently broke a case that worked before it. A leading `/` is
-- what makes a pattern absolute; the arm it arrived in is not.
local function resolve_watcher(sid, gp)
if type(gp) == "table" and gp.baseUri then
return pmacs.lsp.path_for_uri(gp.baseUri), gp.pattern or "**", "relative"
@ -2124,7 +2156,9 @@ local function resolve_watcher(sid, gp)
if rec.server == sid and rec.uri then
local p = pmacs.lsp.path_for_uri(rec.uri)
local dir = p and p:match("^(.*)/[^/]*$")
if dir then return dir, gp, "absolute" end
if dir then
return dir, gp, (gp:sub(1, 1) == "/") and "absolute" or "relative"
end
end
end
end

34
docs/active-work.md
View File

@ -222,16 +222,38 @@ is resolved**, even though #227 is green and cleared.
**`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, APPROVED
2026-08-10.** The ruling that blocked implementation is ANSWERED (see
below); D1 and D2 are implemented on the branch atop the framing
commit. Committed at the branch's first commit so it is portable
- **Framing `docs/lsp-file-watcher-framing.md`, revision 2** — design
approved 2026-08-10; revision 2 records a review round **against the
implementation** that found two correctness defects, both now fixed
(see below). 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.
- **Review round 1 on the code found two defects, and both are cases
where the first fix was itself wrong:**
1. **P1 — a bare-string `*.txt` stopped working.**
`resolve_watcher` classified **every** string as absolute, so a
valid relative pattern was matched against `<base>/foo.txt` and
could never fire. That path worked **before** this lane, so the
repair broke a live case while fixing another. The form now comes
from the pattern (a leading `/`), not from the union arm. The
flat-pattern test did not catch it because it exercises the
RelativePattern **object** arm — F1's tested-vs-exercised split,
repeating inside the lane that named it.
2. **P2 — a scan finishing after cancellation still emitted one
batch.** `scan_tree` suspends on `read_dir` per directory with
`_sleep` already cleared, so a cancel landing there had nothing to
interrupt and the resumed scan ran on to
`did_change_watched_files` under the superseded pattern.
Cancellation and liveness are rechecked after the scan.
- **A test seam was added**: `pmacs.lsp._after_scan_for_tests`, nil in
production, handed the scan result. P2's race cannot be produced by
timing; the seam is the same device as `git.lua`'s `_deliver_status`.
It takes the scan result because a test cancelling on any *other*
scan would pass with the fix deleted.
- **What the implementation is**: `resolve_watcher` returns
`(base, pattern, form)` and the watch record carries the form —
`"absolute"` (plain string) matches `base .. "/" .. rel`,
`"relative"` (RelativePattern) matches `rel`; and
`"absolute"` (pattern begins `/`) matches `base .. "/" .. rel`,
`"relative"` matches `rel`; and
`register_file_watchers` cancels the outgoing record list through the
new `cancel_watch_records` (shared with `unregister_file_watchers`)
before the table write drops the only reference to it.

46
docs/lsp-file-watcher-framing.md
View File

@ -1,9 +1,47 @@
# LSP file watcher — framing
**Status: revision 1 — APPROVED 2026-08-10.** 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".
**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

26
src/bin/pmacs_fake_lsp.rs
View File

@ -354,6 +354,32 @@ fn main() {
});
write_frame(&mut stdout, &req);
}
// Issue #233 review P1 guard: `filewatchbare` registers a
// BARE STRING with no base and no leading `/` — `*.txt`.
// The string arm and the `filewatchflat` arm below carry the
// same pattern deliberately: `flat` proves a
// RelativePattern stays relative, and this proves the
// classification is read from THE PATTERN rather than from
// the union arm it arrived in. The first fix for #233
// called every string absolute, which matched this against
// `<base>/foo.txt` and broke a case that had worked since
// May. Without this mode that regression is invisible.
("initialized", _) if mode == "filewatchbare" => {
let req = serde_json::json!({
"jsonrpc": "2.0",
"id": 9304,
"method": "client/registerCapability",
"params": { "registrations": [{
"id": "watch-bare",
"method": "workspace/didChangeWatchedFiles",
"registerOptions": { "watchers": [{
"globPattern": "*.txt",
"kind": 7
}] }
}] }
});
write_frame(&mut stdout, &req);
}
// Issue #233 F2 guard: `filewatchflat` registers a
// RelativePattern whose pattern has no leading `**/`
// (`*.txt` at the base). It matches base-level files

168
tests/m4_acceptance.rs
View File

@ -5506,6 +5506,174 @@ fn m4_24_relative_pattern_without_globstar_stays_relative() {
);
}
/// Issue #233 review P2 — a scan that completes AFTER cancellation
/// must not emit.
///
/// `scan_tree` awaits `read_dir` once per directory, so the watcher
/// coroutine spends most of a tick suspended with `_sleep` already
/// cleared. A cancel arriving there — re-registration or unregistration
/// — sets `cancelled` and has no sleep to interrupt, so before the fix
/// the resumed scan ran on and emitted one last batch under the
/// superseded pattern.
///
/// No arrangement of real timing produces that interleaving on demand,
/// so it is driven through `pmacs.lsp._after_scan_for_tests`, the same
/// device `git.lua` uses for out-of-order completions. The hook is
/// handed the scan result and cancels **only on the scan that observed
/// `foo.txt`** — cancelling on any other scan would pass with the fix
/// deleted, because the loop would break at the post-sleep check and
/// emit nothing regardless.
#[test]
fn m4_24_a_scan_finishing_after_cancellation_emits_nothing() {
use pmacs::editor::EditorState;
let dir = tempfile::tempdir().expect("tempdir");
let base = dir.path().to_path_buf();
let base_disp = base.display().to_string();
let a_path = base.join("a.rs");
std::fs::write(&a_path, b"fn a() {}\n").expect("write a");
let a_disp = a_path.display().to_string();
let received = base.join(".received");
let mut state = EditorState::new_with_roots(&crate::iso::roots());
let fake = fake_lsp_path();
state
.lua_host
.lua()
.load(format!(
"pmacs.lsp.config.rust = {{ command = '{fake}',
env = {{ PMACS_FAKE_LSP_MODE = 'filewatch',
PMACS_FAKE_LSP_WATCH_BASE = '{base_disp}' }} }}"
))
.exec()
.expect("override rust config");
state
.lua_host
.lua()
.load(format!("pmacs.buffer.find_or_open('{a_disp}')"))
.exec()
.expect("open a.rs");
assert!(
pump_lua_flag(
&mut state,
"(function() for _,r in ipairs(pmacs.lsp.list()) do \
if r.state and r.state.kind=='initialized' then return true end \
end return false end)()",
5,
),
"fake never initialized"
);
// Armed BEFORE the file exists, so the cancel cannot land early:
// the hook fires on every scan and only cancels once the scan it is
// inspecting actually contains foo.txt.
state
.lua_host
.lua()
.load(
"pmacs.lsp._after_scan_for_tests = function(record, cur)
if cur and cur['foo.txt'] then record.cancelled = true end
end",
)
.exec()
.expect("install scan hook");
let warm = Instant::now() + Duration::from_millis(900);
while Instant::now() < warm {
state.tick_processes();
state.tick_lsp();
state.tick_async();
std::thread::sleep(Duration::from_millis(15));
}
std::fs::write(base.join("foo.txt"), b"one\n").expect("write foo.txt");
let deadline = Instant::now() + Duration::from_secs(4);
while Instant::now() < deadline {
state.tick_processes();
state.tick_lsp();
state.tick_async();
std::thread::sleep(Duration::from_millis(15));
}
let got = std::fs::read_to_string(&received).unwrap_or_default();
assert!(
!got.contains("foo.txt"),
"a watcher cancelled during its scan emitted a stale batch \
anyway; .received = {got:?}"
);
}
/// Issue #233 review P1 — a BARE-STRING glob with no leading `/` is a
/// relative pattern and must stay one.
///
/// The first fix for #233 classified every string-arm pattern as
/// absolute, so `*.txt` was matched against `<base>/foo.txt` and could
/// never fire — silently breaking a case that had worked since May
/// while fixing the absolute one. `m4_24_relative_pattern_without_globstar_stays_relative`
/// does not cover it: that mode sends the `RelativePattern` OBJECT form,
/// so it constrains the object arm only. This sends the same pattern
/// through the STRING arm, which is the arm the regression lived in.
#[test]
fn m4_24_bare_string_glob_stays_relative() {
use pmacs::editor::EditorState;
let dir = tempfile::tempdir().expect("tempdir");
let base = dir.path().to_path_buf();
let base_disp = base.display().to_string();
let a_path = base.join("a.rs");
std::fs::write(&a_path, b"fn a() {}\n").expect("write a");
let a_disp = a_path.display().to_string();
let received = base.join(".received");
let foo_uri = format!("file://{}", base.join("foo.txt").display());
let mut state = EditorState::new_with_roots(&crate::iso::roots());
let fake = fake_lsp_path();
state
.lua_host
.lua()
.load(format!(
"pmacs.lsp.config.rust = {{ command = '{fake}',
env = {{ PMACS_FAKE_LSP_MODE = 'filewatchbare',
PMACS_FAKE_LSP_WATCH_BASE = '{base_disp}' }} }}"
))
.exec()
.expect("override rust config");
state
.lua_host
.lua()
.load(format!("pmacs.buffer.find_or_open('{a_disp}')"))
.exec()
.expect("open a.rs");
assert!(
pump_lua_flag(
&mut state,
"(function() for _,r in ipairs(pmacs.lsp.list()) do \
if r.state and r.state.kind=='initialized' then return true end \
end return false end)()",
5,
),
"fake never initialized"
);
let warm = Instant::now() + Duration::from_millis(900);
while Instant::now() < warm {
state.tick_processes();
state.tick_lsp();
state.tick_async();
std::thread::sleep(Duration::from_millis(15));
}
std::fs::write(base.join("foo.txt"), b"one\n").expect("write foo.txt");
assert!(
pump_until_file_contains(&mut state, &received, &format!("1 {foo_uri}"), 6),
"CREATED for foo.txt never reported under a bare-string `*.txt` \
glob — the string arm is being classified absolute again; \
.received = {:?}",
std::fs::read_to_string(&received).unwrap_or_default()
);
}
/// Issue #233 D2 — re-registering a live id supersedes it. The
/// `filewatchrereg` fake registers `watch-re` TWICE with no
/// unregister between — `**/*.old`, then `**/*.new` — exactly the