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

fix(stage2a): close review round 1 — four unreported failures 

All four findings were the same shape: a failure that left state wrong
and told nobody.

**Delete refusals reach the user.** `reconcile_delete_and_fire` returned
`kept_modified` and `refused` and both production callers discarded
them, so a last-buffer refusal or the asynchronous modified-buffer race
left the file gone and the buffer still bound to it — and the next
`C-x C-s` recreates the deleted file. Reporting now happens inside the
shared seam, not at its call sites, for the same reason the
reconciliation does: a caller that has to remember to report is a caller
that will forget. The message names the buffers (capped, with a count
for the rest) and states the consequence, and it is written to
`EditorCore::status`, not `pmacs.error` — that channel is defined only
by a test stub, so a report there would be the same silence.
`reconcile_delete` now prefixes `kill_buffer`'s reason with the buffer
name, because "cannot kill the last remaining buffer" does not say which
buffer is now bound to a deleted path.

**The LSP subscribers stop swallowing their own failures.** Ignored
`pcall`s around `did_close`, `forget_uri`, `did_open` and overlay
re-rooting made the callback return successfully, so the
`all-must-succeed` logger had nothing to log — concretely, a stale server
made `forget_uri` raise while the callback carried on with the old
stores, routes and `documents` entry all live. A shared failure sink
attributes each step, reports on both channels, and raises **after** the
loop, so one unreachable server cannot leave every other attachment
unreconciled.

**`forget_uri` abandons requests through the established path.** It
purged `pending_routes` and `pending_external` but not the same ids
`send_request` put in `LspClient.pending`, and recorded nothing in
`cancelled_rids`. The per-rid work is extracted from
`drain_cancelled_externals` as `abandon_request` and reused, rather than
a second incomplete copy: route, client pending, cancelled record and
`$/cancelRequest` now happen together.

**Acceptance 35 is pinned.** With a plain delete the forbidden fallback
was unobservable — `find_or_open` raises out of `load_file` and the
`pcall` swallows it — so both assertions passed with the fallback
present. The plan now deletes the origin's file and recreates it, which
gives the fallback something to open and makes "restores nothing"
falsifiable. The corrected G1 explanation also reaches the production
comments, which still repeated the false `resolve_target_buffer::NotFound`
story.

New pins: acceptance 53 and 53b assert the status channel; a stale-server
row asserts attribution on both channels *and* that the healthy
attachment still reconciles; an `lsp.rs` unit test asserts the client-side
abandonment with an unrelated request as its control.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T
This commit is contained in:
Levi Neuwirth 2026-07-29 21:28:16 -04:00
parent d5fadf4120
commit 44135fcd73
5 changed files with 535 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

138
builtin/runtime/lsp.lua
View File

@ -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)

16
src/editor_core.rs
View File

@ -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

113
src/lsp.rs
View File

@ -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<u64> = 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,

108
src/lua_bindings/mod.rs
View File

@ -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<String> = 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<String> = 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::<SharedCore>() {
core.borrow_mut().status = message;
}
}
/// Drive [`crate::async_runtime::TickOutcome::resources`] through
/// reconciliation, one settled mutation at a time (dired Stage 2a,
/// Q#DR29).

220
tests/resource_reconciliation_acceptance.rs
View File

@ -147,6 +147,10 @@ fn buffer_name(state: &EditorState, global: &str) -> Option<String> {
)
}
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<String> = 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"
);
}