diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 679d573..d23409b 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1439,12 +1439,23 @@ local function apply_workspace_edit(ops) if #plan == 0 then return 0, 0, 0 end -- G1 — capture the origin BUFFER, not its path. A path captured here -- is a plain Lua local, and no amount of reconciliation can reach an - -- already-captured local: when the batch renames the active file, the - -- old path no longer resolves, `find_or_open` hits - -- `resolve_target_buffer`'s NotFound arm, and that arm CREATES an - -- empty path-backed buffer and selects it. The user was returned to a - -- phantom file that never existed. The handle follows the rename for - -- free, because the buffer is what moved. + -- already-captured local: once the batch renames or deletes the active + -- file, that string names something that is no longer there. The + -- handle follows a rename for free, because the buffer is what moved. + -- + -- The framing's G1 described the failure as a "phantom buffer" created + -- by `resolve_target_buffer`'s NotFound arm. **That is not what + -- happens on this path, and the wrong explanation is recorded here + -- rather than left to be rediscovered.** `pmacs.buffer.find_or_open` + -- calls `crate::file_io::load_file` directly and maps the error, so a + -- missing path RAISES; the NotFound arm belongs to + -- `EditorCore::resolve_target_buffer`, which serves + -- `pmacs.window.display_file` and the startup/daemon target, not this + -- binding. The real defect is quieter: `restore_origin` runs under a + -- `pcall`, so the raise is swallowed and the user is left in whatever + -- buffer the last applied op made active. And when the old path DOES + -- still resolve -- a batch that deletes and then recreates it -- the + -- fallback silently opens a file the user asked to delete. local origin_buf = pmacs.window.buffer() local edit_total, files, res_ops = 0, 0, 0 -- Plan items fully applied before a failure. Q#RD3 permits partial @@ -1458,9 +1469,9 @@ local function apply_workspace_edit(ops) -- -- **No path fallback (G1).** If the origin buffer is gone — the batch -- deleted its file and reconciliation killed it — restore NOTHING. - -- The old code's path fallback is exactly what fabricated a phantom - -- buffer; "return the user somewhere plausible" is not worth inventing - -- a file that does not exist. + -- "Return the user somewhere plausible" is not worth re-opening a path + -- the batch just destroyed, and when that path has been recreated the + -- fallback would drop the user into a file they asked to delete. local function restore_origin() if not origin_buf then return end pcall(pmacs.window.switch_buffer, origin_buf) @@ -2931,30 +2942,95 @@ local function attachments_under(path) return out end +-- How many attributed failures one status line spells out before +-- collapsing the rest into a count. +local RESOURCE_REPORT_LIMIT = 2 + +-- A failure collector for a reconciliation fan-out. +-- +-- **Why this exists rather than a bare `pcall` per step.** Every step +-- below is fallible for reasons outside this file's control -- a stale +-- server id makes `forget_uri` raise, a stopped server makes `did_close` +-- raise -- and an IGNORED `pcall` makes the hook callback RETURN +-- SUCCESSFULLY. `resource.renamed` and `resource.deleted` are +-- `all-must-succeed`, so the registry's error logger is the mechanism +-- that surfaces a failing subscriber; a callback that swallows its own +-- failures gives that logger nothing to log, and the concrete outcome is +-- silent: `forget_uri` fails, the callback carries on, and the old +-- stores, routes and `documents` entry stay live under a URI the editor +-- no longer holds. +-- +-- It must NOT abort the loop. One unreachable server must not leave +-- every other attachment unreconciled, so failures accumulate and are +-- raised once, after every attachment has been processed. +local function failure_sink(hook_name) + local sink = { hook = hook_name, items = {} } + + -- Run `fn(...)`, and on a raise record it attributed to `what`. + -- Returns `ok, value` like `pcall`, so a caller can branch. + function sink:step(what, fn, ...) + local ok, value = pcall(fn, ...) + if not ok then + self.items[#self.items + 1] = string.format("%s: %s", what, tostring(value)) + end + return ok, value + end + + -- Report everything collected, on BOTH channels, and raise. + -- + -- The raise is what the `all-must-succeed` logger needs in order to + -- write an attributed record to *errors*; the status line is what the + -- user actually sees, because stale LSP state looks like the editor + -- quietly breaking. `pmacs.error` is deliberately not used: it is + -- defined only by a test stub, so writing there would reproduce the + -- silence this replaces. + function sink:finish() + if #self.items == 0 then return end + local shown, n = {}, #self.items + for i = 1, math.min(n, RESOURCE_REPORT_LIMIT) do shown[i] = self.items[i] end + local summary = table.concat(shown, "; ") + if n > #shown then + summary = summary .. string.format("; and %d more", n - #shown) + end + pcall(pmacs.editor.set_status, + string.format("LSP %s: %d reconciliation failure%s -- %s", + self.hook, n, (n == 1 and "" or "s"), summary)) + error(string.format("%s: %s", self.hook, table.concat(self.items, "; ")), 0) + end + + return sink +end + pmacs.hook.add("resource.renamed", function(old_path, new_path) if type(old_path) ~= "string" or type(new_path) ~= "string" then return end + local sink = failure_sink("resource.renamed") for _, hit in ipairs(attachments_under(old_path)) do local key, rec, old_uri = hit.key, hit.rec, hit.rec.uri -- The buffer's own path was rebound before this hook fired, so ask -- it rather than reconstructing the tail ourselves. A buffer that -- somehow lost its path (killed, unbound) cannot be re-opened, and - -- falls through to the teardown-only path below. + -- falls through to the teardown-only path below. Not routed through + -- the sink: a pathless buffer is a legitimate state here, not a + -- reconciliation failure. local ok_path, new_buf_path = pcall(function() return rec.buffer:path() end) local new_uri = (ok_path and new_buf_path) and file_uri_for(new_buf_path) or nil -- 1. Flush any pending didChange for the OLD uri, so the server is -- not left holding an edit it can no longer attribute. - flush_did_change_for(rec) + sink:step("flush didChange for " .. old_uri, flush_did_change_for, rec) pending_did_change[key] = nil -- 2. didClose the old uri — this removes the open-document -- registration and nothing else. - pcall(pmacs.lsp.did_close, rec.server, old_uri) + sink:step("didClose " .. old_uri, pmacs.lsp.did_close, rec.server, old_uri) -- 3. Purge the routes, drain their awaiters, and clear all fourteen -- stores plus `documents` for the old key. Runs against the OLD -- server, which matters when step 4 picks a different one. - pcall(pmacs.lsp.forget_uri, rec.server, old_uri) + -- A failure here is the one that most needs reporting: the + -- callback would otherwise continue with the old stores, routes + -- and `documents` entry all still live. + sink:step("forget_uri " .. old_uri, pmacs.lsp.forget_uri, rec.server, old_uri) if not new_uri then attachments[key] = nil @@ -2964,8 +3040,9 @@ pmacs.hook.add("resource.renamed", function(old_path, new_path) -- 4. Re-run ensure_server. Server affinity keys on the detected -- project root, so a rename ACROSS roots needs a different -- server; a same-root rename reuses the existing one. - local sid = ensure_server(rec.language, new_buf_path) - if not sid then + local ok_sid, sid = sink:step("ensure_server for " .. new_buf_path, + ensure_server, rec.language, new_buf_path) + if not (ok_sid and sid) then attachments[key] = nil styled_buffers[key] = nil diag_viewed_buffers[key] = nil @@ -2976,36 +3053,45 @@ pmacs.hook.add("resource.renamed", function(old_path, new_path) rec.server = sid rec.uri = new_uri rec.version = 1 - local ok_text, text = pcall(buffer_text, rec.buffer) - pcall(pmacs.lsp.did_open, sid, new_uri, rec.version, - ok_text and text or "") + local ok_text, text = sink:step("read " .. new_uri, buffer_text, rec.buffer) + sink:step("didOpen " .. new_uri, pmacs.lsp.did_open, + sid, new_uri, rec.version, ok_text and text or "") -- 6. Re-root the diagnostic overlays. `DiagnosticView.uri` is -- set once at construction and is private, so this is the -- only way to move it — and the sweep reaches PASSIVE -- windows, which the attach path cannot, while preserving -- each overlay's position in the composition order. - pcall(pmacs.diag._rename_resource, old_uri, new_uri) + sink:step("re-root diagnostics to " .. new_uri, + pmacs.diag._rename_resource, old_uri, new_uri) end end end + -- Raised only after EVERY attachment has been processed: one + -- unreachable server must not leave the rest unreconciled. + sink:finish() end) pmacs.hook.add("resource.deleted", function(path) if type(path) ~= "string" then return end + local sink = failure_sink("resource.deleted") for _, hit in ipairs(attachments_under(path)) do local key, rec = hit.key, hit.rec -- No flush: the document is gone, and shipping a didChange for a -- file the server can no longer read buys nothing. pending_did_change[key] = nil - pcall(pmacs.lsp.did_close, rec.server, rec.uri) - pcall(pmacs.lsp.forget_uri, rec.server, rec.uri) - -- Drop the record unconditionally. The buffer may be gone entirely - -- (an unmodified visited file is killed), in which case a retained - -- record is a dangling handle that `repull_for_attachments` would - -- iterate; and a modified buffer kept alive has no file to analyze - -- until it is saved, which re-attaches through the ordinary path. + sink:step("didClose " .. rec.uri, pmacs.lsp.did_close, rec.server, rec.uri) + sink:step("forget_uri " .. rec.uri, pmacs.lsp.forget_uri, rec.server, rec.uri) + -- Drop the record unconditionally, INCLUDING after a failure above. + -- The buffer may be gone entirely (an unmodified visited file is + -- killed), in which case a retained record is a dangling handle that + -- `repull_for_attachments` would iterate; and a modified buffer kept + -- alive has no file to analyze until it is saved, which re-attaches + -- through the ordinary path. Keeping a record whose teardown failed + -- would be strictly worse than dropping it: the failure is reported + -- either way, and a retained one is re-swept every refresh. attachments[key] = nil styled_buffers[key] = nil diag_viewed_buffers[key] = nil end + sink:finish() end) diff --git a/src/editor_core.rs b/src/editor_core.rs index f902418..9b8b5d3 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -4998,7 +4998,21 @@ impl EditorCore { } match self.kill_buffer(id) { Ok(()) => out.killed.push(id), - Err(message) => out.refused.push((id, message)), + // Named, because the reason alone is not actionable: + // `kill_buffer`'s "cannot kill the last remaining + // buffer" says nothing about *which* buffer is now + // bound to a path whose file is gone, and that buffer's + // name is what the user needs in order to save it + // somewhere else. + Err(message) => { + let name = self + .registry + .borrow() + .get(id) + .map_or_else(|_| format!("{id:?}"), |b| b.name().to_owned()); + out.refused + .push((id, format!("buffer {name:?}: {message}"))); + } } } out diff --git a/src/lsp.rs b/src/lsp.rs index ba32fb2..f5630b6 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -1699,15 +1699,35 @@ impl LspManager { ); } for rid in abandoned_rids { - self.pending_routes.remove(&(sid, rid)); - if let Some(client) = self.clients.get_mut(&sid) { - client.pending.remove(&rid); - client.cancelled_rids.insert(rid); - } - self.send_cancel_request(sid, rid); + self.abandon_request(sid, rid); } } + /// Abandon one in-flight request: drop its response route, drop the + /// client's `pending` entry, record the rid so a late reply is + /// dropped silently rather than surfacing as an unmatched response, + /// and ask the server to stop working on it. + /// + /// Extracted from [`Self::drain_cancelled_externals`] by dired Stage + /// 2a so [`Self::forget_uri`] reuses it instead of being a second, + /// incomplete copy. **All four steps are load-bearing together.** + /// Removing only the route and the awaiter — which is what + /// `forget_uri` originally did — leaves `client.pending` holding the + /// rid forever when the server never replies, and leaves + /// `cancelled_rids` without it, so a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename that is worse than a leak: the old server keeps the entry + /// and no attachment drains it afterwards, so the entries + /// accumulate. + fn abandon_request(&mut self, sid: LspServerId, rid: u64) { + self.pending_routes.remove(&(sid, rid)); + if let Some(client) = self.clients.get_mut(&sid) { + client.pending.remove(&rid); + client.cancelled_rids.insert(rid); + } + self.send_cancel_request(sid, rid); + } + /// Send `$/cancelRequest { id }` to `sid`, best-effort. A server /// that is not accepting writes (stopped / crashed) is skipped by /// [`Self::send_notification`]'s state guard; the `Err` is @@ -3123,7 +3143,11 @@ impl LspManager { /// respect to another manager tick, but putting the gate first /// means every later call observes the forgotten state even if a /// future refactor introduces an early return. - /// 2. **Purge `pending_routes`** whose route carries this URI. + /// 2. **Abandon every in-flight request scoped to this URI**, through + /// [`Self::abandon_request`] — the same path the per-tick + /// cancellation sweep uses, so the route, the client's `pending` + /// entry, the `cancelled_rids` record and `$/cancelRequest` all + /// happen together rather than only the first of the four. /// `WorkspaceSymbol` is retained unconditionally: it carries no /// URI at all — its query stands in for the doc URI in the /// supersede key — and a workspace-symbol query is not scoped to @@ -3182,24 +3206,26 @@ impl LspManager { // Step 1 — the gate, first. self.forgotten_documents.insert((sid, uri.to_owned())); - // Step 2 — collect the rids this URI owns, then purge. + // Steps 2 and 3 — collect the rids this URI owns, settle their + // awaiters cancelled, then abandon each request through the + // SAME path the per-tick sweep uses + // ([`Self::abandon_request`]): route, `client.pending`, + // `cancelled_rids`, `$/cancelRequest`. Purging the route alone + // would leave the request live in the client and a late reply + // unrecognised. let doomed_rids: Vec = self .pending_routes .iter() .filter(|((s, _), route)| *s == sid && route.scoped_uri() == Some(uri)) .map(|((_, rid), _)| *rid) .collect(); - for rid in &doomed_rids { - self.pending_routes.remove(&(sid, *rid)); - } - - // Step 3 — settle the awaiters joined to those rids cancelled. for rid in &doomed_rids { if let Some(p) = self.pending_external.remove(&(sid, *rid)) { for a in &p.awaiters { self.runtime.complete_external_cancelled(a.job_id); } } + self.abandon_request(sid, *rid); } // Step 4 — the fourteen stores plus `documents`. @@ -4556,6 +4582,67 @@ mod resource_reconciliation_tests { assert!(mgr.pending_external.contains_key(&(a, 2))); } + /// Review round 1 — `forget_uri` must abandon the request in the + /// **client**, not only in the route table. + /// + /// `pending_routes` and `pending_external` are two of four places an + /// in-flight request lives. `LspClient.pending` (written by + /// `send_request`) and `cancelled_rids` are the other two, and + /// dropping only the first two leaves the entry live forever when the + /// server never replies, while a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename the old server keeps those entries and no attachment + /// drains it afterwards, so they accumulate. + /// + /// Bite: fails against a `forget_uri` that purges routes and + /// awaiters without going through `abandon_request`. + #[test] + fn forget_uri_abandons_the_request_in_the_client_too() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + + for (rid, uri) in [(11u64, old), (12u64, other)] { + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + let client = mgr.clients.get_mut(&a).expect("client a"); + client.pending.insert(rid, "textDocument/hover".to_owned()); + } + let client = mgr.clients.get(&a).expect("client a"); + assert!(client.pending.contains_key(&11), "precondition"); + assert!(client.pending.contains_key(&12), "precondition"); + assert!( + client.cancelled_rids.is_empty(), + "precondition: nothing abandoned yet" + ); + + mgr.forget_uri(a, old).expect("forget"); + + let client = mgr.clients.get(&a).expect("client a"); + assert!( + !client.pending.contains_key(&11), + "the purged request must leave `client.pending`, or it leaks \ + for the lifetime of a server that never replies" + ); + assert!( + client.cancelled_rids.contains(&11), + "and must be recorded, or a late reply surfaces as a generic \ + unrouted response instead of being dropped" + ); + assert!( + client.pending.contains_key(&12), + "an unrelated document's request must survive" + ); + assert!( + !client.cancelled_rids.contains(&12), + "and must not be marked abandoned" + ); + } + /// Acceptance 31c — the error contract, both arms. The second is the /// one that matters: the subscriber runs per attachment, an /// attachment need not have any pending route or populated result, diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 212e962..a8a130b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1797,17 +1797,109 @@ fn reconcile_delete_and_fire( for id in &outcome.killed { after_buffer_removed(lua, *id); } - let mut args = mlua::MultiValue::new(); - args.push_back(mlua::Value::String( - match lua.create_string(normalized.as_os_str().as_encoded_bytes()) { - Ok(s) => s, - Err(_) => return outcome, - }, - )); - run_hook_if_defined(lua, "resource.deleted", args); + if let Ok(path_arg) = lua.create_string(normalized.as_os_str().as_encoded_bytes()) { + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String(path_arg)); + run_hook_if_defined(lua, "resource.deleted", args); + } + // Reported AFTER the fan-out, deliberately: a subscriber may set its + // own status, and this message must be the last word because it is + // the data-loss-adjacent one. Unconditional, so a path that cannot + // cross into Lua still gets its refusal reported rather than losing + // both the hook and the report. + report_delete_reconcile(lua, &normalized, &outcome); outcome } +/// Cap on how many buffer names one status line spells out before +/// collapsing the rest into a count. A directory delete can reach +/// dozens; a status line that scrolls off is a message nobody reads. +const DELETE_REPORT_NAMED_LIMIT: usize = 3; + +/// Render the buffers a delete could not reconcile, and put it on the +/// status channel. +/// +/// **Silence here is the defect this exists to close.** Both outcomes +/// leave a buffer alive and still bound to a path whose file is gone, so +/// the next `C-x C-s` recreates the file the user just deleted. That is +/// recoverable only if the user knows it happened: +/// +/// * `kept_modified` — a modified buffer, kept on purpose. On the +/// synchronous path #190 refuses before disk so this cannot arise, but +/// `pmacs.fs.remove` dispatches a worker, and a buffer modified in the +/// interval between the caller's check and the syscall reaches here. +/// * `refused` — could not be removed at all: the last remaining buffer +/// (`kill_buffer` refuses to empty the registry), or a buffer that was +/// mid-edit when the reconciliation ran. +/// +/// The channel is `EditorCore::status`, which is what +/// `pmacs.editor.set_status` writes. **Not `pmacs.error`** — that +/// channel is defined only by a test stub, so all fifteen of its guarded +/// call sites are dead, and a report written there would be exactly the +/// silence being fixed. +/// +/// Lives inside the shared seam rather than at its two call sites, for +/// the same reason the reconciliation does: a caller that has to +/// remember to report is a caller that will forget. The first version of +/// this function's callers both discarded the outcome. +fn report_delete_reconcile( + lua: &Lua, + path: &std::path::Path, + outcome: &crate::editor_core::DeleteReconcile, +) { + if outcome.kept_modified.is_empty() && outcome.refused.is_empty() { + return; + } + let name_of = |p: &std::path::Path| { + p.file_name() + .map_or_else(|| p.display().to_string(), |n| n.to_string_lossy().into()) + }; + let mut parts: Vec = Vec::new(); + if !outcome.kept_modified.is_empty() { + let n = outcome.kept_modified.len(); + let named: Vec<&str> = outcome + .kept_modified + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, name)| name.as_str()) + .collect(); + parts.push(format!( + "{n} buffer{} with unsaved changes kept ({}{}) — saving {} will RECREATE the deleted file", + if n == 1 { "" } else { "s" }, + named.join(", "), + if n > named.len() { + format!(", and {} more", n - named.len()) + } else { + String::new() + }, + if n == 1 { "it" } else { "them" }, + )); + } + if !outcome.refused.is_empty() { + let n = outcome.refused.len(); + let named: Vec = outcome + .refused + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, why)| why.clone()) + .collect(); + parts.push(format!( + "{n} buffer{} could not be closed ({}{})", + if n == 1 { "" } else { "s" }, + named.join("; "), + if n > named.len() { + format!("; and {} more", n - named.len()) + } else { + String::new() + }, + )); + } + let message = format!("deleted {}: {}", name_of(path), parts.join("; ")); + if let Some(core) = lua.app_data_ref::() { + core.borrow_mut().status = message; + } +} + /// Drive [`crate::async_runtime::TickOutcome::resources`] through /// reconciliation, one settled mutation at a time (dired Stage 2a, /// Q#DR29). diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs index f1ba6f9..63870a0 100644 --- a/tests/resource_reconciliation_acceptance.rs +++ b/tests/resource_reconciliation_acceptance.rs @@ -147,6 +147,10 @@ fn buffer_name(state: &EditorState, global: &str) -> Option { ) } +fn status(state: &EditorState) -> String { + state.core.borrow().status.clone() +} + fn buffer_is_valid(state: &EditorState, global: &str) -> bool { eval( state, @@ -903,6 +907,21 @@ fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { "the last remaining buffer cannot be killed, so it survives the \ deletion of its file" ); + // **Reported, not silent.** Survival alone is not the criterion: the + // buffer is still bound to a path whose file is gone, so the next + // `C-x C-s` recreates the file the user deleted. That is recoverable + // only if the user is told. + let said = status(&state); + assert!( + said.contains("could not be closed"), + "the refusal must reach the status channel; status was {said:?}" + ); + assert!( + said.contains("only.txt"), + "and must name the buffer, because `cannot kill the last \ + remaining buffer` alone does not say WHICH buffer is now bound \ + to a deleted path; status was {said:?}" + ); // Half two: a directory of buffers where one refuses removal. The // rest must still reconcile. @@ -933,6 +952,26 @@ fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { "and C still reconciled afterwards — one refusal must not abort \ the rest" ); + // The kept-modified case reports too, and says what the consequence + // is. This is the asynchronous race the framing's H1 leaves open: + // #190 refuses before disk on the synchronous path, but + // `pmacs.fs.remove` dispatches a worker, so a buffer modified in the + // interval reaches the drain with its file already gone. + let said2 = status(&state2); + assert!( + said2.contains("unsaved changes kept"), + "a modified buffer kept alive over a deleted file must be \ + reported; status was {said2:?}" + ); + assert!( + said2.contains("RECREATE"), + "and the report must state the consequence — saving it puts the \ + deleted file back; status was {said2:?}" + ); + assert!( + said2.contains("b.txt"), + "naming the buffer; status was {said2:?}" + ); } // --------------------------------------------------------------------------- @@ -1082,6 +1121,16 @@ fn acc53b_a_mid_edit_refusal_leaves_window_side_and_round_trip_state_untouched() core.registry.borrow().contains(doomed_id), "and the buffer itself is still in the registry" ); + drop(core); + // And the refusal is REPORTED. Leaving state untouched is only half + // the contract: the file is gone, so a user who is not told keeps a + // buffer bound to a path that no longer exists. + let said = status(&state); + assert!( + said.contains("mid-edit") && said.contains("could not be closed"), + "a mid-edit refusal must reach the status channel; status was \ + {said:?}" + ); } // --------------------------------------------------------------------------- @@ -1771,6 +1820,22 @@ fn acc34_renaming_the_active_file_through_the_applier_returns_the_same_buffer() /// Acceptance 35. When the origin buffer is **gone** after the edit, the /// applier restores **nothing** rather than falling back to the old /// path. +/// +/// **The plan deletes the origin's file and then RECREATES it, and that +/// is what makes the row bite at all.** With a plain delete the forbidden +/// fallback is unobservable: `find_or_open` on a path that no longer +/// exists raises straight out of `file_io::load_file`, the surrounding +/// `pcall` swallows it, and nothing happens — so "no buffer at the old +/// path" and "the active buffer is live" both hold with the fallback +/// present. Recreating the path gives the fallback something to open, and +/// it is not a contrived shape: a `documentChanges` batch that deletes +/// and recreates a file is ordinary LSP refactoring output. +/// +/// Bite: the applier restoring by path instead of by buffer handle. The +/// handle is invalid (reconciliation killed the buffer) so a +/// handle-based restore does nothing; a path-based one loads the +/// recreated file into a NEW buffer and switches the user into it — +/// dropping them, silently, into a file they asked to delete. #[test] fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { let fx = Fixture::new(); @@ -1783,10 +1848,18 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { &state, &fx.root, &serde_json::json!({ - "documentChanges": [{ - "kind": "delete", - "uri": file_uri(&doomed), - }], + "documentChanges": [ + { + "kind": "delete", + "uri": file_uri(&doomed), + }, + { + // Recreates the path, so a path-based restore has a + // file to open and the fallback becomes observable. + "kind": "create", + "uri": file_uri(&doomed), + }, + ], }), ); // `other` keeps the registry non-empty so the delete's kill is not @@ -1806,12 +1879,17 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { ); settle_a_while(&mut state); - assert!(!doomed.exists(), "the file is gone"); + assert!( + doomed.exists(), + "precondition for the bite: the batch recreated the path, so a \ + path-based restore CAN open it" + ); assert!( !buffer_is_valid(&state, "B"), - "and its clean buffer was reconciled away" + "the origin buffer was reconciled away by the delete" ); - let phantom: bool = eval( + + let reopened: bool = eval( &state, &format!( "for _, b in ipairs(pmacs.buffer.list()) do @@ -1822,10 +1900,20 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { ), ); assert!( - !phantom, - "the applier must restore NOTHING rather than re-opening the path \ - it just deleted — a path fallback would recreate it as an empty \ - buffer, and the next C-x C-s would resurrect the file" + !reopened, + "the applier must restore NOTHING. A path-based restore loads the \ + recreated file into a fresh buffer, which is the editor silently \ + re-opening a file the user asked to delete" + ); + + let active_path: Option = eval( + &state, + "local b = pmacs.window.buffer(); return b and b:path() or nil", + ); + assert_ne!( + active_path.as_deref(), + Some(doomed.to_str().unwrap()), + "and the user must not be sitting in it either" ); let active_valid = { let core = state.core.borrow(); @@ -1834,6 +1922,114 @@ fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { }; assert!( active_valid, - "and the window it left behind must sit on a live buffer" + "the window it left behind must still sit on a live buffer" + ); +} + +/// Review round 1 — a reconciliation failure inside the LSP subscriber +/// must be **reported and attributed**, and must not stop the remaining +/// attachments from reconciling. +/// +/// The scenario is the reviewer's: a **stale server id**. The attachment +/// record still names a server the manager has forgotten, so +/// `did_close` and `forget_uri` both raise. With ignored `pcall`s the +/// callback returned successfully, so the `all-must-succeed` hook logger +/// had nothing to log, and the old stores, routes and `documents` entry +/// stayed live under a URI the editor no longer held — silently. +/// +/// Two packages under one parent directory give two servers, and only +/// one is staled out, so the row can assert both halves at once: the +/// failure is surfaced, **and** the healthy attachment still moves. +/// +/// Bite: fails against ignored `pcall`s (nothing on either channel), and +/// against a fix that lets the first failure `error()` out of the loop +/// (the healthy attachment would never reconcile). +#[test] +fn a_subscriber_reconciliation_failure_is_reported_and_the_rest_still_reconcile() { + let fx = Fixture::new(); + fx.write("w/a/Cargo.toml", "[package]\nname = \"a\"\n"); + fx.write("w/b/Cargo.toml", "[package]\nname = \"b\"\n"); + let file_a = fx.write("w/a/src/main.rs", "fn main() {}\n"); + let file_b = fx.write("w/b/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "A", &file_a); + settle_until(&mut state, "server for package a", |s| server_count(s) == 1); + open_as(&state, "B", &file_b); + settle_until(&mut state, "server for package b", |s| server_count(s) == 2); + + // Stale out the server serving package `a` only: stop it, then forget + // it, leaving `attachments[a].server` naming a server the manager no + // longer holds. `forget_uri` raises for an unknown server id, which + // is exactly the failure mode under test. + let root_a = file_uri(&fx.at("w/a")); + exec( + &state, + &format!( + "local victim + for _, row in ipairs(pmacs.lsp.list()) do + if row.root_uri == \"{root_a}\" then victim = row.id end + end + assert(victim, 'no server rooted at package a') + pcall(pmacs.lsp.stop, victim) + _G.VICTIM = victim" + ), + ); + settle_until(&mut state, "the victim is forgotten", |s| { + let gone: bool = eval( + s, + "pcall(pmacs.lsp.forget, _G.VICTIM) + for _, row in ipairs(pmacs.lsp.list()) do + if row.id == _G.VICTIM then return false end + end + return true", + ); + gone + }); + exec(&state, "pmacs.editor.set_status('')"); + + // Rename the parent, so BOTH attachments are in the fan-out. + let new_uri_b = file_uri(&fx.at("w2/b/src/main.rs")); + rename_fire_and_forget(&mut state, &fx.at("w"), &fx.at("w2")); + settle_a_while(&mut state); + + // Half one: the failure is surfaced, on both channels, attributed to + // the operation that failed. + let said = status(&state); + assert!( + said.contains("resource.renamed") && said.contains("reconciliation failure"), + "the failure must reach the status channel; status was {said:?}" + ); + assert!( + said.contains("forget_uri"), + "and must name WHICH step failed — an unattributed count does not \ + tell anyone that the URI-keyed stores were left live; status was \ + {said:?}" + ); + + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("resource.renamed") && errors.contains("forget_uri"), + "the callback must RAISE, so the all-must-succeed hook logger has \ + something to record; *errors* held {errors:?}" + ); + + // Half two: the healthy attachment still reconciled. The fake + // republishes diagnostics on every `didOpen`, so diagnostics under + // the NEW uri prove the whole ordered teardown ran for package b + // after package a's failed. + settle_until(&mut state, "package b reattached at its new uri", |s| { + diag_count(s, &new_uri_b) > 0 + }); + assert!( + diag_count(&state, &new_uri_b) > 0, + "one unreachable server must not leave every other attachment \ + unreconciled — the raise has to come after the loop, not inside it" ); }