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

merge: integrate main @ c14f2de (PRs #197, #196) into the Stage 1 lane 

`docs/active-work.md` was the only conflicting file. #196 added the dired
Stage 2a lane at the position this branch had used to relabel the #188
framing lane header; the resolution keeps both, changing neither side's
wording.

`src/editor_core.rs` auto-merged. Both lanes touch it, so a clean
textual merge is not evidence of a clean semantic one — the gate suite
is re-run in full on the merged tree rather than inherited from the
pre-merge head.

Resolution verified for line loss in both directions: the resolved file
differs from `main` only by this branch's own authored edits, and
differs from this branch only by additions taken from `main`.
This commit is contained in:
Levi Neuwirth 2026-07-30 10:13:37 -04:00
parent 7350cfdd8a c14f2de428
commit 12e2cff466
20 changed files with 5778 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

93
.github/workflows/ci.yml vendored
View File

@ -9,10 +9,63 @@ env:
CARGO_TERM_COLOR: always
RUSTFLAGS: "-D warnings"
# Cancel a pull request's superseded runs instead of letting them burn
# to completion. This project rebases heavily — the ledger re-conflicts
# on nearly every merge — so a branch routinely takes several pushes
# while an earlier run is still going, and each of those runs is
# obsolete the moment the next push lands. macOS minutes are the
# expensive ones and the macOS leg is the critical path, so superseded
# runs are exactly where the waste concentrates.
#
# Scoped to pull requests deliberately. `github.event.pull_request.number`
# is empty for a push to `main`, so the fallback keys those runs by SHA:
# every `main` commit gets its own group and none can cancel another.
# Cancelling a `main` run would leave the branch-protection record
# ambiguous about a commit that has already landed — the one place this
# saving is not worth having.
concurrency:
group: ci-${{ github.event.pull_request.number || github.sha }}
cancel-in-progress: ${{ github.event_name == 'pull_request' }}
# Every job carries `timeout-minutes`. Without one a job inherits
# GitHub's 360-minute default, so a single hung test burns six hours —
# times four on the test matrix — and reports nothing useful at the end
# of it.
#
# The ceilings are justified against OBSERVED EXECUTION, and the
# numbers are a reading rather than a constant, so re-measure before
# trusting them:
#
# * observed max, 25-run window: 17 min (macOS/luajit)
# * observed max, 12-run window: 15.8 min (same job)
# * every other job: under 4 min
#
# `timeout-minutes` counts EXECUTION, not queue time — a 33-minute
# wall-clock run in that window executed its longest job in 17 — so no
# run in the observed history would have been killed by these values.
#
# The exposure is the case the window does NOT contain: a COLD CACHE.
# A stable-toolchain bump invalidates Swatinem's key on every leg at
# once, and a cold macOS debug build of this workspace plus the suite is
# the plausible way a HEALTHY run exceeds its ceiling. The test job
# therefore gets 35 rather than 25 — roughly 2x its observed max — while
# everything else keeps 25 against a sub-4-minute observed max, except
# `m6-perf-gates`, which keeps its own tighter 15.
#
# DIAGNOSIS, WRITTEN BEFORE IT HAPPENS: four test legs timing out
# simultaneously, shortly after a Rust release, is a cold cache and not
# a hang. Rerun, or raise this number. A single leg timing out while its
# siblings pass is the hang case these ceilings exist to catch.
#
# This is also the gate that has to exist before the basedpyright-class
# hang can ever be armed — see `PMACS_REQUIRE_PYRIGHT`, deliberately
# never set, in the test job below. 35 still beats the 360-minute
# default by an order of magnitude.
jobs:
fmt:
name: Format
runs-on: ubuntu-latest
timeout-minutes: 25
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
@ -23,6 +76,7 @@ jobs:
clippy:
name: Lint (${{ matrix.lua }})
runs-on: ubuntu-latest
timeout-minutes: 25
strategy:
fail-fast: false
matrix:
@ -40,10 +94,17 @@ jobs:
# not pmacs), so the root-package clippy above never lints it. Lint
# it explicitly or its warnings slip through CI (audit F-001).
- run: cargo clippy -p pmacs-gpu --all-targets -- -D warnings
# pmacs-protocol is likewise never linted by the root-package
# clippy above: the workspace default member is only `pmacs`. The
# local `--workspace` gate covers it, so it passes today — CI has
# simply never checked, and a warning introduced through a
# protocol-only PR would reach `main` unseen.
- run: cargo clippy -p pmacs-protocol --all-targets -- -D warnings
gpu-render:
name: GPU Render (headless)
runs-on: ubuntu-latest
timeout-minutes: 25
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
@ -76,6 +137,12 @@ jobs:
test:
name: Test (${{ matrix.os }} / ${{ matrix.lua }})
runs-on: ${{ matrix.os }}
# 35, not 25: this is the only job whose observed max is minutes
# rather than seconds, and the only one a cold cache can plausibly
# push past a 25-minute ceiling on all four legs at once. See the
# note above `jobs:` for the measurements and the cold-cache
# diagnosis.
timeout-minutes: 35
strategy:
fail-fast: false
matrix:
@ -136,18 +203,27 @@ jobs:
# render job. Set only where the install step ran.
#
# PMACS_REQUIRE_PYRIGHT is deliberately NOT set and basedpyright
# is deliberately NOT installed: that test has no timeout and
# hangs forever (root cause is the non-interruptible reader-thread
# join in `RuntimeHandles::drop`, already a named deferral in
# `src/process.rs`). This job has no `timeout-minutes`, so arming
# it today would trade a vacuous green for a six-hour hang on four
# legs. It gets armed after the hang fix and the CI timeouts land,
# and its own variable exists so that flip is one line.
# is deliberately NOT installed. Both original reasons are now
# gone: the hang's root cause was the stdin-field drop ordering in
# `RuntimeHandles::drop` and is fixed, and this job now carries
# `timeout-minutes`, so a hang could no longer burn six hours.
# The ONE remaining reason is the plain one --- basedpyright is not
# installed here, so arming the variable would fail rather than
# test anything. Installing it (a uv + bundled-node download on
# every leg) is its own decision, not a rider on the hang fix.
#
# PMACS_REQUIRE_SETSID arms the teardown-deadlock unit test. Its
# fixture orphans a grandchild with `setsid --fork`, which is
# util-linux rather than coreutils, so the test skips when the
# binary is absent (a minimal container must not fail `--lib`
# without ever testing pmacs) and this variable is what makes the
# skip fatal where the tool is guaranteed.
- run: cargo test --all-targets --no-default-features --features ${{ matrix.lua }} -- --test-threads=1
env:
PMACS_REQUIRE_LSP: ${{ runner.os == 'Linux' && '1' || '' }}
PMACS_REQUIRE_SHELLS: ${{ runner.os == 'Linux' && '1' || '' }}
PMACS_REQUIRE_LUA: ${{ runner.os == 'Linux' && '1' || '' }}
PMACS_REQUIRE_SETSID: ${{ runner.os == 'Linux' && '1' || '' }}
- run: cargo test --doc --no-default-features --features ${{ matrix.lua }}
# The workspace default member is only the root `pmacs` package, so
# the runs above never execute pmacs-protocol's own tests — the
@ -160,6 +236,7 @@ jobs:
acceptance:
name: M1 Acceptance Gates
runs-on: ubuntu-latest
timeout-minutes: 25
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
@ -170,6 +247,7 @@ jobs:
m4-perf-gates:
name: M4 Perf Gates
runs-on: ubuntu-latest
timeout-minutes: 25
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
@ -188,6 +266,7 @@ jobs:
m5-perf-gates:
name: M5 Perf Gates
runs-on: ubuntu-latest
timeout-minutes: 25
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable

7
README.md
View File

@ -220,6 +220,13 @@ translation) are routed through trampolines that exec these tools.
shell-locator helper to find `bash` / `zsh` / `fish` for
per-shell integration tests. The M7.2 fetcher's timeout test
uses `sleep`.
- **`setsid`** (util-linux, Linux only, **optional**). The process
teardown-deadlock test uses `setsid --fork` to orphan a grandchild,
which is the only way to reproduce that deadlock without depending on
shell `&` semantics (they differ between `bash` and `dash`). The test
**skips** when `setsid` is absent, so a minimal or BusyBox environment
still runs `cargo test --lib`; set `PMACS_REQUIRE_SETSID=1` to make
that skip a failure, as CI does on Linux.
- **`git`** (added in M7.2). Required for any package operation:
the package fetcher shells out to `git` to clone, fetch, and
resolve refs, with a deterministic environment

12
TEST_IMPROVEMENT.md
View File

@ -42,6 +42,9 @@ hand before inclusion.
on `push:main` + `pull_request` only. No coverage measurement, no
scheduled runs, no branch protection on `main` (verified via API:
404, so every job is advisory).
**~~No branch protection~~ — CLOSED. Protection was enabled during
the arc; the API now reports 12 required contexts, `strict` off,
`enforce_admins` off. The 404 above was a reading at audit time.**
The suite is unusually thoughtful in places — the daemon harness's
connect-based readiness probe, the `PMACS_REQUIRE_GPU` hard-fail
@ -358,9 +361,12 @@ test) pass in CI and flake for whoever runs the documented local gate.
(Findings that change what CI *certifies*; speedups are §6.)
1. **Branch protection is off** — every job is advisory; a red run
merges as easily as a green one. Turn on required checks for the
cheap deterministic jobs at minimum (fmt, clippy, ubuntu test legs).
1. ~~**Branch protection is off**~~ — **DONE.** Every job was
advisory; a red run merged as easily as a green one. All 12 contexts
are now required, rather than the cheap-jobs-only starter suggested
here. `strict` is off (a PR need not rebase every time `main` moves,
which this repository's ledger contention makes expensive) and
`enforce_admins` is off (the maintainer retains an override).
2. **No job timeouts except m6** (15 min). Everything else inherits
360 min. The day a runner image ships any of the PATH-gated tools
(§1.2), the basedpyright-class hang burns 6 h × 4 matrix legs with

32
builtin/hooks/default.lua
View File

@ -16,6 +16,12 @@
-- format-on-save subscribe here.
-- * editor.before-quit --- short-circuit. A callback may veto quit
-- (e.g. "buffer modified --- save first?").
-- * resource.renamed --- all-must-succeed (dired Stage 2a). Fired
-- after a successful rename, with (old, new)
-- canonical absolute paths.
-- * resource.deleted --- all-must-succeed (dired Stage 2a). Fired
-- after a successful delete, with the
-- canonical absolute path.
--
-- These are *defined* here so user config can attach callbacks via
-- pmacs.hook.add. Run sites are in Rust (after-load, after-edit) and in
@ -76,6 +82,32 @@ define {
kind = "short-circuit",
}
define {
name = "resource.renamed",
description = "Fired once per SUCCESSFUL filesystem rename, with the old " ..
"and new paths as canonical absolute strings. The core " ..
"reconciles what it can reach -- buffer paths and names, the " ..
"URI-keyed LSP stores, attached diagnostic overlays -- but a " ..
"package that keys its own state by path or URI is invisible " ..
"to that, so this hook is the mechanism that scales. It " ..
"carries PATHS rather than a rebind list precisely because " ..
"dired's listing buffers are pathless: a path-keyed consumer " ..
"must be able to reconcile from (old, new) alone. Does not " ..
"fire for a rename that failed or was cancelled.",
kind = "all-must-succeed",
}
define {
name = "resource.deleted",
description = "Fired once per SUCCESSFUL filesystem delete, with the " ..
"canonical absolute path. Buffers on the path and beneath it " ..
"have already been reconciled: unmodified ones killed " ..
"through both removal phases, modified ones kept alive. " ..
"Subscribers drop their own path-keyed state. Does not fire " ..
"for a delete that failed or was cancelled.",
kind = "all-must-succeed",
}
define {
name = "editor.before-quit",
description = "Fired before the editor exits. Return false to veto.",

24
builtin/runtime/fs.lua
View File

@ -163,6 +163,30 @@ end
-- If a package needs at-most-one-pending semantics for mutations,
-- it should serialize on the package side (await each op before
-- dispatching the next). The fs primitive can't enforce that.
--
-- **And that is a CORRECTNESS precondition, not only a cancellation
-- one (dired Stage 2a, Q#DR29).** A successful `rename` or `remove`
-- reconciles the editor's path owners in the main-thread drain — buffer
-- paths and names, the URI-keyed LSP state, the `resource.renamed` /
-- `resource.deleted` hooks. That reconciliation is deliberately
-- order-INDEPENDENT: the runtime drains the reply bus with `try_recv`
-- and establishes no execution token, so a worker can finish first and
-- be descheduled before sending, and reply order therefore does not
-- recover filesystem execution order.
--
-- Independent mutations commute, so nothing is owed for them. But
-- **mutations whose source/target paths overlap must be serialized by
-- dispatching the next only after the previous handle settles.** There
-- is no static ordering rule that would substitute: rename `dir` ->
-- `newdir` racing delete `dir/child.txt` needs delete-then-rename if
-- the delete ran first on disk and rename-then-delete if the rename
-- did, and a fixed "deletes before renames" rule gets one of the two
-- wrong — the kill misses, the rename then rebinds the buffer onto a
-- path whose file is gone, and it survives pointing at nothing.
--
-- A caller that ignores this owns the residue: a buffer left bound to a
-- stale path, or killed when it should have been rebound. Recoverable
-- and visible, not data loss — but real.
function fs.rename(from, to)
if type(from) ~= "string" then

229
builtin/runtime/lsp.lua
View File

@ -1124,7 +1124,7 @@ pmacs.hook.add("buffer.after-edit", function()
-- Stale suppression must stay keystroke-accurate even though the
-- O(file) didChange send below is coalesced: render families
-- anchored to pre-edit positions are hidden from this edit on.
pcall(pmacs.lsp._mark_document_stale, rec.uri)
pcall(pmacs.lsp._mark_document_stale, rec.server, rec.uri)
-- Arc 1d: was this edit a typed character? The input-origin signal
-- (see the trigger block below).
local typed = pmacs.editor.this_command
@ -1437,19 +1437,44 @@ local function apply_workspace_edit(ops)
end
end
if #plan == 0 then return 0, 0, 0 end
local origin = active_buffer_path()
-- 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: 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
-- application, so this is what stops a caller claiming "nothing was
-- mutated" when something was.
local applied_ops = 0
-- Return the user to where they invoked from — best-effort, since
-- that path may have just been renamed or deleted. Runs on the
-- FAILURE path too (Q#RD7): previously this ran only after a
-- successful loop, so a mid-batch refusal stranded the user in
-- whatever buffer the last applied op left active.
-- Return the user to where they invoked from. Runs on the FAILURE
-- path too (Q#RD7): previously this ran only after a successful loop,
-- so a mid-batch refusal stranded the user in whatever buffer the last
-- applied op left active.
--
-- **No path fallback (G1).** If the origin buffer is gone — the batch
-- deleted its file and reconciliation killed it — restore NOTHING.
-- "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 origin then pcall(pmacs.buffer.find_or_open, origin) end
if not origin_buf then return end
pcall(pmacs.window.switch_buffer, origin_buf)
end
for _, item in ipairs(plan) do
local ok, err
@ -2882,3 +2907,191 @@ pmacs.command.define {
pmacs.keymap.bind { scope = "global", sequence = "M-g n", command = "diag.next" }
pmacs.keymap.bind { scope = "global", sequence = "M-g p", command = "diag.previous" }
-- Resource reconciliation ---------------------------------------------------
--
-- dired Stage 2a, §5. A rename or delete moves or destroys a path that
-- FOURTEEN URI-keyed store families, the `documents` mirror, the pending
-- response routes and the attached diagnostic overlays are all keyed by.
-- `EditorCore` reconciles the buffer's own path and name; these two
-- subscribers reconcile the LSP layer, which is buffer-keyed here
-- (`rec.uri` is cached per buffer and read at dozens of sites, so ONE
-- rebind reaches all of them) and URI-keyed in Rust.
--
-- These subscribers are independent of every other `resource.renamed`
-- consumer by construction: this one touches URI-keyed state, dired's
-- touches its own handle table, and neither reads what the other wrote.
-- That matters because `all-must-succeed` does NOT abort the fan-out —
-- `run_all_must_succeed` collects each callback's error and continues —
-- so a subscriber may not rely on a raising peer to stop the sequence,
-- and the ordered teardown below is ordered INTERNALLY rather than by
-- registration.
-- Every attachment whose document is `path` or lies beneath it, as
-- `{ key, rec, path }`. Resolved through `path_for_uri` and compared
-- with `paths_related`, so the comparison is component-aware and runs on
-- the same canonical form the buffer registry keys on.
local function attachments_under(path)
local out = {}
for key, rec in pairs(attachments) do
local rec_path = rec.uri and pmacs.lsp.path_for_uri(rec.uri)
if rec_path and paths_related(rec_path, path) then
out[#out + 1] = { key = key, rec = rec, path = rec_path }
end
end
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. 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.
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.
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.
-- 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
styled_buffers[key] = nil
diag_viewed_buffers[key] = nil
else
-- 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 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
else
-- 5. didOpen the new uri with the buffer's current text and a
-- fresh version. This also reclaims the tombstone for
-- exactly (server, new uri).
rec.server = sid
rec.uri = new_uri
rec.version = 1
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.
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
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)

348
docs/active-work.md
View File

@ -788,6 +788,177 @@ has **no branch and no framing yet**.
`git fetch githubsucks && git worktree add ../pmacs-rd-impl
-b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`.
## dired Stage 2a — rename/delete reconciliation — PR #196 OPEN, review round 1 closed
- Portable branch: `githubsucks/dired-stage2-impl`, worktree
`../pmacs-dired-s2`. Implements **Stage 2a only** of the framing merged
as #171 (`docs/dired-stage2-framing.md` rev 9, §5/§6/§10 — the
substrate transaction, no dired surface). Position against `main`, as
pasted command output rather than a remembered constant:
```
$ git merge-base HEAD githubsucks/main
e003b81cdd577140fc77330bd4578d3090696877
```
That base is the #190 merge, and #190 matters here specifically:
Stage 2a **adopts** its `delete_verdict` refusal rather than
reinventing one, and lifts its walk query out into
`editor_core::buffers_bound_under` so the guard and both
reconciliation seams cannot disagree about which buffers an operation
touches. **Re-measure the merge-base before relying on it** —
`main` has branch protection, all 12 checks must pass on the merging
head, and a conflicting PR builds no merge ref at all, so a green run
from before a move reads as current when it is not. **Re-measured after
round 1: `main` had not moved, so no integration was needed** — that is
a reading of the tree, not a standing fact.
- **What 2b and 2c still owe, stated so the split boundary is auditable.**
2a ships **no user-visible surface at all** and no dired code: the
`dired_acceptance` count is deliberately unchanged at **25**, and a
moved count there would mean it touched something it should not have.
2b owes the mark and operation layer (`m u U t d x D R w M`),
`pmacs.minibuffer.confirm` plus its `src/editor.rs` load-sequence line,
`pmacs.killring.push`, dired's own `resource.renamed` subscriber, and
acceptance 1–22, 33, 39–41. 2c owes `mkdir`/`copy`/`remove_dir_all`,
`JobKind` 12 → 15, `dired.recursive-deletes`, and acceptance 42–47.
- **The split boundary has not moved since rev 9.** It was re-checked
against this tree: #188 (generated-buffer immutability Stage 1) did not
convert dired's `paint`, so §3.1's coordination note is still an
obligation of that lane rather than a collision with this one, and
nothing in this diff touches `builtin/runtime/dired.lua`.
- **Two m4 rows were re-pinned, and that is a behaviour change to a
landed lane's assertions.** `rd9` and `rd14` pinned #190's deliberate
restraint on the `apply_resource_op` delete arm — descendants stay
orphaned, only the first of two duplicate path-bound buffers is
reconciled — and both doc comments gave the same reason: widening
would have routed N buffers through `remove_buffer_and_fire`, phase 2
without phase 1, leaving up to N windows on removed ids.
`EditorCore::reconcile_delete` composes both phases, so the constraint
is discharged and the old assertions became the defect. Each row now
asserts BOTH directions — reconciled away **and** no window holding a
removed id — and each direction is bite-verified.
- **One framing claim is wrong and is corrected at the test, not
silently worked around.** §5's G1 says a stale captured path
"materializes a phantom" by reaching `resolve_target_buffer`'s
`NotFound` arm. It does not: `pmacs.buffer.find_or_open` calls
`crate::file_io::load_file` directly and maps the error, so a missing
path **raises**, and the `NotFound` arm belongs to
`resolve_target_buffer`, which serves `pmacs.window.display_file` and
the startup/daemon target rather than that binding. The defect is real
and smaller: the `pcall` swallows the raise, so the user is stranded
wherever the last applied op left them. Acceptance 34 is restructured
to bite on that (its plan edits another file first, which is what makes
the restore observable at all) and the correction is recorded in the
test's own doc comment.
- **Two bites were vacuous as the framing specified them, and both
reasons are worth keeping.** Item 28's *rename* row cannot pin the
walk's containment rule: `reconcile_rename` calls
`Path::strip_prefix` to rebuild a descendant's tail, and that is
component-aware too, so a string-prefix walk is silently corrected a
second time. The row moved to the **delete** side, where the walk's
verdict IS the kill list. Item 30's composition-order assertion was a
tautology: the LSP attach leaves `diagnostic` **last** in the stack, and
moving the last element to the end is a no-op, so a remove-and-re-push
was indistinguishable from an in-place mutation; the row now pushes one
more overlay after it and asserts that precondition explicitly.
- **23 acceptance criteria are bite-verified by executed mutation**, each
labelled `OK (assertion)` — none merely `OK (COMPILE)`, and none
vacuous. Items 25, 27, 28, 29 (both directions), 30 (both mutations),
31, 31b (both gates), 31d (both halves), 34, 50 (both mutations), 51,
52, 53b, 54, 55, plus the two re-pinned m4 rows in three
configurations.
- **Review round 1 found four defects; all four are fixed, and all four
were the same shape — a failure that left state wrong and told nobody.**
Worth keeping as one lesson rather than four bugs: every one of them
was a `pcall` or a discarded return value, and each *looked* like
defensive coding.
- **P1 — delete refusals were silent.** `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
moved **inside the shared seam**, for the same reason the
reconciliation lives there: a caller that has to remember to report
is a caller that will forget. Channel is `EditorCore::status`;
**not `pmacs.error`**, which is defined only by a test stub, so a
report there would have been the same silence.
- **P2 — the LSP subscribers swallowed their own reconciliation
failures.** Ignored `pcall`s made the callback return successfully,
so the `all-must-succeed` logger had nothing to log. A shared
failure sink now attributes each step and raises **after** the loop,
because a fix that aborts on the first failure would leave every
other attachment unreconciled — that wrong fix is itself a
bite-verified mutation.
- **P2 — `forget_uri` left purged requests live in the client.** It
dropped `pending_routes` and `pending_external` but not the ids
`send_request` puts in `LspClient.pending`, and recorded nothing in
`cancelled_rids`, so a server that never replies leaked the entry and
a late reply surfaced as a generic unrouted response. The per-rid
work is now extracted from `drain_cancelled_externals` as
`abandon_request` and **reused** rather than copied.
- **P2 — acceptance 35 was unpinned even after the G1 correction.**
With a plain delete the forbidden path fallback is 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. The corrected G1 explanation also reached
the production comments, which still repeated the false
`resolve_target_buffer::NotFound` story — *a correction that stops at
the test comment has only half landed.*
- **One round-1 pin passed with its own bug restored, and the reason is
reusable.** Acceptance 53 asserted `contains("only.txt")` for the
buffer-name attribution — but the status line opens with
`deleted only.txt:`, the deleted path's **basename**, so stripping the
attribution changed nothing the assertion could see. Both halves now
assert the buffer's *own* name, which for a path-backed buffer is the
full path and which only the attribution can produce. **A pin written
to close a review finding is exactly the kind that passes with the bug
restored**, and the detector was running the bite rather than reading
the assertion.
- **31 bites now, all executed, every one labelled `OK (assertion)`** —
the original 23 plus 8 for round 1 (report call removed; refusal reason
unattributed; kept-modified name dropped; subscriber failures
swallowed; the wrong fix that aborts the loop; `forget_uri` skipping
`abandon_request`; and the forbidden path fallback restored, which must
fail acceptance 34 **and** 35 independently).
- Verification at this head, each gate run to its own file and its own
exit code checked (never through a pipe): `cargo fmt --check` clean;
`cargo clippy --workspace --all-targets -- -D warnings` clean;
`cargo test --lib` **1,876** passed / 3 ignored; `--lib --features
crdt` **2,061** / 4 ignored; the new
`resource_reconciliation_acceptance` **25** default and **25** crdt;
`dired_acceptance` **25** and **25** crdt, deliberately unmoved; the
frozen additivity gate `m8_1` **10** / `m8_2` **15** / `m8_3` **32**,
all unchanged; `m4_acceptance -- --skip basedpyright` **149** passed /
3 ignored / 1 filtered; `lsp_multi_root_acceptance` **13**;
`lsp_dispatch_seams_acceptance` **15**;
`typed_edit_chain_acceptance` **13**; `journey_acceptance` **24**
(the ratchet floor, asserted as a count rather than a colour);
`gpu_invocation_acceptance` **15** crdt — **and that number is only
real with `pmacs` and `pmacs-gpu` built first**, which is the `a37`
trap in §5: the same command reported 12 failures before the build and
15 passes after, so a red run there is not evidence of a regression
until the binaries exist; `PMACS_REQUIRE_GPU=1 cargo test -p
pmacs-gpu` **202**; isolated-`XDG_CONFIG_HOME` workspace sweep with
`--no-fail-fast` **3,559** passed across **104** suites, 19 ignored, 0
failed; `git diff --check` clean. Every one of those was run as its own
step with its own exit status checked — never `cmd | tail` inside an
`&&` chain, which returns *tail's* status and has masked a real failure
in this repo before.
- **Ownership, per the framing's own warning.** §16 says 2a must not run
concurrently with **Journey Stage 1b**, because 1b's LSP
spawn-failure reporting lands in `builtin/runtime/lsp.lua`'s
attachment lifecycle and 1b's compile/binding half touches
`src/editor_core.rs` — the same two files 2a rewrites, where the
conflicts are semantic rather than textual so a clean `git merge`
proves nothing. **1b must not be started while this PR is open.** No
other lane in flight touches them: #188 is `dired.lua`/`buffer.rs`
generated-buffer writes, and the bottom-panel and CI lanes are
elsewhere.
- Recovery from a clean checkout:
`git fetch githubsucks && git worktree add ../pmacs-dired-s2
-b dired-stage2-impl githubsucks/dired-stage2-impl`.
## Generated-buffer immutability framing lane — MERGED AS PR #188
- Portable branch: `githubsucks/generated-buffer-immutability`; worktree
@ -1087,6 +1258,183 @@ has **no branch and no framing yet**.
at 42 insertions against 88 deletions — merging it would *revert*
current documentation. The section said "whoever confirms the branch
carries nothing unique removes the section"; this is that.
## Test-improvement arc, lane 3a — CI timeouts and concurrency
- Portable branch: `githubsucks/ci-timeouts-concurrency`, worktree
`../pmacs-ci3`. Workflow only — **no product code, no tests changed.**
- **Base, measured at write time:**
```
$ git log --oneline -1 githubsucks/main
b7bf2c6 Merge pull request #194 from levineuwirth/silent-skip-arming
```
- Ships the three cheap, deterministic items of `TEST_IMPROVEMENT.md`
§5-6. The larger ones — nextest (§6.3), the serial/parallel split
(§6.2), the parallel canary leg (§5.6), the nightly cron (§5.5), and
the macOS matrix trim (§6.4) — are **deliberately not here**: each
changes what CI certifies or how it runs, and each wants its own
decision rather than riding a timeout patch.
- **`timeout-minutes` on every job (§5.2).** Measured before changing:
**7 of 8 jobs had none** and inherited GitHub's 360-minute default;
only `m6-perf-gates` had one (15). A single hung test therefore burnt
six hours, times four on the test matrix.
**This is the gate that must land before `PMACS_REQUIRE_PYRIGHT` can
ever be set** — lane 2 left basedpyright unarmed precisely because
this did not exist.
- **The ceilings are 25, and 35 for the test job — anchored on observed
execution, corrected in review.** Revision 1 cited "~14.6 min, ample
headroom", which was one reading quoted as a property. Re-measured
over two windows: **17 min** max over 25 runs and **15.8 min** over
12, both macOS/luajit; every other job under 4 min. Against 17, a
flat 25 is ~1.5x, not "ample".
- `timeout-minutes` counts **execution, not queue** — a 33-minute
wall-clock run in that window executed its longest job in 17 — so
**no run in observed history would have been killed** by either
value.
- The real exposure is what the window does *not* contain: a **cold
cache**. A stable-toolchain bump invalidates Swatinem's key on
every leg simultaneously, and a cold macOS debug build plus suite
is the plausible way a *healthy* run overruns. It would present as
four legs timing out at once, the day after a Rust release.
- So the test job takes 35 (~2x its observed max) and the rest keep
25 (~6x theirs), and **the diagnosis is written into the workflow
before the event**: simultaneous four-leg timeouts after a
toolchain release are a cold cache, not a hang; a single leg
timing out beside passing siblings is the hang case.
- **`concurrency` with `cancel-in-progress` (§6.1)**, scoped to pull
requests. `github.event.pull_request.number` is empty on a push to
`main`, so the fallback keys those by SHA and no `main` run can
cancel another — cancelling one would leave the branch-protection
record ambiguous about a commit that already landed.
- **`-p pmacs-protocol` clippy (§5.7).** Verified passing locally
*before* proposing it, so adding it cannot turn CI red on arrival.
The root-package clippy never covered it: the workspace default
member is only `pmacs`.
- **§5.1 branch protection is DONE, not deferred** — it belongs in
neither this lane's shipped list nor its deferrals, and review was
right that its absence from both was an omission. It was enabled
earlier in this session; verified against the API at review time:
```
$ gh api repos/levineuwirth/pmacs/branches/main/protection
{"enforce_admins":false,"force_push":false,"required_checks":12,"strict":false}
```
All 12 checks required; `strict` off deliberately, so a PR need not
rebase every time `main` moves (this repository's ledger contention
makes strict expensive); `enforce_admins` off so the user retains an
override. **This matters to the concurrency comment**, which
justifies exempting `main` pushes by appeal to "the
branch-protection record" — that record now exists, so the
justification is real rather than aspirational.
- **Required status checks are NAME-COUPLED to job names, and this
lane's own deferrals will break them.** A required context that no
longer exists does not fail — it leaves every PR pinned on
"Expected — waiting for status", indefinitely, which is
`main` becoming unmergeable by policy rather than by a red run.
Three deferrals above change job names or the matrix: the macOS trim
(§6.4) removes two contexts outright, and nextest (§6.3) or the
serial/parallel split (§6.2) rename or add them.
**Rule: any job rename, removal, or matrix change updates the
branch-protection required-checks list in the same motion.** Recorded
here because this is the entry that both enabled protection and named
the lanes that will invalidate it.
- Recovery from a clean checkout:
`git fetch githubsucks && git worktree add ../pmacs-ci3
-b ci-timeouts-concurrency githubsucks/ci-timeouts-concurrency`.
## Test-improvement arc, lane 4 — process teardown stdin deadlock
- Portable branch: `githubsucks/process-teardown-stdin-deadlock`,
worktree `../pmacs-hang`. Implements
`docs/process-teardown-stdin-deadlock-framing.md` (rev 3: one review
round, then a CI round that falsified the reproduction).
- **Base, measured rather than quoted:**
```
$ git log --oneline -1 githubsucks/main
e003b81 Merge pull request #190 from levineuwirth/resource-op-delete-guard-impl
```
- Recovery from a clean checkout:
`git fetch githubsucks && git worktree add ../pmacs-hang
-b process-teardown-stdin-deadlock
githubsucks/process-teardown-stdin-deadlock`.
- **The defect:** `RuntimeHandles::drop` joined its reader threads in
the `Drop` **body**, which runs before any field drops. The
`ChildStdin` sink lives in the `stdin` **field**, so it could only be
released after the join returned — and the join waited on readers
blocked in `read()` on pipes whose write ends the child still held,
because the child never got the stdin EOF that would have made it
exit. A closed cycle inside one function; teardown hung forever.
- **This is the root cause of the `m4_5_basedpyright` hang** that has
parked `--workspace` sweeps (once for 2h26m) and forced
`-- --skip basedpyright` into every gate recipe. The handoff's §3
claim that the desktop's binary was broken is **retired by this PR**:
the binary was fine. `basedpyright-langserver` is a uv console script
that runs bundled `node` via `subprocess.run` and **waits**; at
teardown `shutdown()` SIGTERMs the recorded pid (the wrapper), which
dies without forwarding, and **that** orphans node to `PPid: 1`
holding the pipes. A direct binary like `clangd` is a genuine child
whose pipes close on reap. That is the whole of the "intermittent"
story.
- **Corrected in review round 2:** rev 1–3 said the wrapper "spawns node
and exits". Wrong — and refutable from evidence already in hand, since
the initialize handshake succeeds, which a wrapper that exited at spawn
could not have done. The `PPid: 1` observation was taken *after*
`shutdown()` had killed the wrapper. **We create the orphan.** The fix
is unaffected; the parked follow-up changes from "tolerate
self-orphaning servers" to "stop orphaning them" (signal the group).
- **Diagnosis method, because reproduce-first was the instruction:**
gdb thread stacks plus `/proc` fd forensics on a live wedged process,
both pipe ends identified in both processes, reproduced 5/5. Three
earlier reproductions were vacuous — see the handoff §5 lesson; the
shipped test carries two positive controls because of it.
- Verification (each gate its own step, real exit status, no
`cmd | tail`): fmt 0; `git diff --check` 0; clippy 0; `--lib` 1864
passed; `--lib --features crdt` 2049 passed; **`m4_acceptance`
without the skip 150 passed in 2.66s with the basedpyright test
`ok`**; the **eleven** PTY/REPL/worker/panel suites of the framing's
Bet 2 all 0 (144 tests); `PMACS_REQUIRE_GPU=1 -p pmacs-gpu` 202
passed. Bite verified by revert: `ok` in 2.03s with the fix, FAILED on
timeout at 10.00s without it, both controls passing first.
- **CI round 1 falsified the reproduction, and the control is what
caught it.** Three Test legs failed on `9b1cf3d`'s predecessor: the
synthetic child used `sh -c 'cat <&0 & exit 0'`, and `<&0` does not
defeat the `/dev/null` rule it was chosen for — the rule applies
*before explicit redirections*, so fd 0 is already `/dev/null` and the
redirect duplicates it onto itself. `bash` skips the default when a
stdin redirect is present; **`dash`, which is Ubuntu's and CI's
`/bin/sh`, does not.** Local probing through `/bin/sh` could not see
it. Now `setsid --fork cat`, with no shell at all. **Lesson recorded in
the handoff §5: never probe shell behaviour through `/bin/sh` — name
the implementation.**
- **`acc28` on macos/lua54 was a flake, established not assumed.**
`bottom_panel_stage1_acceptance::acc28` failed once on that leg;
rerunning the same job on the *identical* head passed, and the suite is
46/46 locally. It is now in Bet 2's falsifier list — its absence from
rev 1 was a real gap, since it drives real child input through a PTY in
a panel and this PR changes PTY-mode teardown ordering.
- **Not fixed here, parked in the framing §5:** cancellable non-group
`read` (covers a child that ignores EOF, and one that stops draining
while `write_all` is blocked); the orphaned-server **leak** — post-fix
the server exits by cooperation, not enforcement.
- `CLAUDE.md`'s `--skip basedpyright` entry is deliberately untouched.
Dropping it is a separate proposal owed evidence of repeated green.
The timeout precondition is **already satisfied** — #195 (this PR's
base) gave every job a `timeout-minutes` — so the only remaining reason
`PMACS_REQUIRE_PYRIGHT` stays unarmed is that CI does not install
basedpyright at all; arming it would fail rather than test anything.
- Adds `PMACS_REQUIRE_SETSID`, armed on Linux. The teardown test's
fixture needs `setsid --fork`, which is util-linux rather than
coreutils, so it **skips** when absent (the standard `--lib` gate must
not hard-fail a minimal container on an undeclared tool) and the
variable makes that skip fatal where the tool is guaranteed. Both arms
verified against a PATH with `setsid` genuinely removed: unarmed skips,
armed FAILS. README's test-dependency list declares it.
## Parked lane: kill-ring browser + persistence
- Portable branch: `githubsucks/kill-ring-browser`

71
docs/agent-handoff.md
View File

@ -1289,10 +1289,32 @@ git diff --check
Machine-specific caveats — re-verify on a machine you haven't used
before trusting them:
- **basedpyright**: the DESKTOP's local binary is broken and HANGS the
`m4_5_basedpyright` tests — hence the `--skip` there. The LAPTOP has
a working basedpyright 1.39.9 (verified 2026-07-10: the m4_5 test
passes in 0.18s), so the skip is droppable on the laptop.
- **basedpyright**: the desktop binary was **never broken** — this was a
real code defect, diagnosed and fixed 2026-07-29 (see §5, "A `Drop`
body runs before its fields"). `RuntimeHandles::drop` joined its reader
threads before the `stdin` field dropped, so the server never got stdin
EOF, never exited, and kept the output pipe the readers were blocked
on. Deterministic on the desktop, invisible on the laptop and in CI,
which is why it read as a broken local binary for weeks.
**How the orphan is actually made — WE make it.** basedpyright's
console script runs bundled `node` through `subprocess.run` and
**waits** (`nodejs_wheel/executable.py:50`, verified in 1.39.6). At
teardown `shutdown()` SIGTERMs the *recorded* pid — the Python wrapper
— which dies without forwarding the signal, orphaning node to `PPid 1`
holding the pipes. An earlier revision of this entry said the wrapper
"spawns node and exits"; that was wrong, and the refutation was already
in hand, since the initialize handshake succeeds, which a
wrapper that exited at spawn could not have done. The consequence is
for the follow-up, not the fix: the orphan-management work is **stop
orphaning them** (signal the group), not tolerate self-orphaning.
The `--skip` above stays for now: it is still correct on any tree
predating the fix, and — the one live reason — **CI never installs
basedpyright at all**, so arming `PMACS_REQUIRE_PYRIGHT` would fail
rather than test anything. The two original reasons are both gone: the
hang is fixed, and #195 gave every job a `timeout-minutes`, so a hang
can no longer burn six hours. Installing basedpyright in CI (a uv plus
bundled-node download per leg) and dropping the local skip are two
separate proposals, each owed its own evidence.
- **GPU on the laptop**: AMD Radeon 780M (RADV) — native Vulkan,
`PMACS_REQUIRE_GPU=1` works without lavapipe.
- **Flaky-under-load tests — rerun isolated before treating a sweep
@ -1459,6 +1481,47 @@ round-trip cannot detect a discriminant shift.
asks you to keep. Same family as the skip-reports-`ok` lesson below
and the double-invocation traps: **the thing that summarizes a gate
must not be able to lose the gate's verdict.**
- **A reproduction is a measurement, and needs its own positive control.**
The basedpyright-hang lane wrote **four** reproductions that passed
against the *unfixed* tree, each vacuous for a different reason: the
child exited before the join; the child never read stdin at all; the
child's stdin was silently rebound to `/dev/null` (POSIX XCU §2.9.3
assigns `/dev/null` to an asynchronous list's stdin when job control is
off, so `sh -c 'cat & exit 0'` EOFs instantly); and then **the repair
for that was also wrong** — the rule applies *before explicit
redirections*, so `<&0` duplicates `/dev/null` onto itself. `bash`
skips the default when a stdin redirect is present, `dash` does not, so
`<&0` passed locally and failed in CI. The shipped test uses
`setsid --fork`, removing the shell from the reproduction entirely.
Every one of the four looked obviously right when written, and the
fourth was verified locally before it failed. Note what a narrower rule
would have missed: "check the child is still alive" catches only the
first. Only the general form catches all four — **and the ones nobody
has invented yet.** Note also which mechanism caught the fourth: not a
reviewer, but the control itself, failing loudly in CI and naming its
own cause. So: assert the precondition your reproduction
depends on, in the test, before exercising the thing under test. In
`teardown_closes_stdin_before_joining_readers` that is two controls
(the recorded child has exited; both readers are still blocked in
`read`), each with a failure message naming what its absence means —
and a `/bin/sh` that is `bash` locally and `dash` in CI is exactly the
sort of divergence no amount of local verification reaches.
This is the same rule that produced #192's bite positive control and
#194's re-read-the-artifact lesson, stated at full generality: **a
measurement you have not controlled is a claim, not evidence.**
- **A `Drop` body runs before its fields, whatever the declaration
order.** Cost a multi-week misattribution: `RuntimeHandles::drop`
joined its reader threads in the drop *body*, while the `stdin` sink it
needed to close first sat in a *field* — reachable only after that body
returned. The child never got EOF, never exited, and kept the output
pipe the readers were blocked on, so teardown hung forever. Reordering
the struct's fields cannot fix this shape; the operation has to move
into the body. Generally: **if a `Drop` body waits on anything, check
what the waited-on party needs that only a field drop will release.**
Corollary from the same investigation — `cancel`-flag style wake-outs
only work where the thread actually polls them; a thread blocked in a
raw `read` never sees one, so a flag next to a blocking syscall is
documentation, not a mechanism.
- **A test that skips on a missing precondition reports `ok`, and a gate log
cannot tell that apart from a pass.** `vterm_stage3_acceptance::a37` — the
only acceptance driving a real daemon, a real PTY and a real wgpu render

561
docs/process-teardown-stdin-deadlock-framing.md Normal file
View File

@ -0,0 +1,561 @@
# Framing — close child stdin before joining readers (process teardown deadlock)
A pipe-mode child that exits on stdin EOF can deadlock the supervisor's
teardown forever. `RuntimeHandles::drop` joins its reader threads in the
`Drop` body, which runs **before** the `stdin` field drops, so the child
never receives the EOF that would make it close the very pipe write ends
those readers are blocked on. The fix is a two-line reorder that reuses a
mechanism already present in this file.
This is the diagnosed root cause of
`m4_5_basedpyright_initializes_and_negotiates_encoding` hanging
indefinitely — the hazard that has parked `cargo test --workspace` runs
(once for 2h26m) and forced `-- --skip basedpyright` into every gate
recipe.
**Scope: `src/process.rs` only. No protocol change. No Lua surface. No
new primitive.**
---
## Revision history
- **rev 1** — initial framing. Root cause established by live diagnosis
(gdb stacks + `/proc` fd forensics on a wedged process), reproduced
5/5 deterministically at `e003b81`.
- **rev 2** — review round 1. rev 1's synthetic child was **itself
vacuous** (the third in this lane): POSIX assigns `/dev/null` to a
background job's stdin when job control is off, so `sh -c 'cat &
exit 0'` EOFs instantly and exits against the *unfixed* tree.
Q#TD6 now uses the explicit-redirect form and criterion 2 gains a
positive control. Also: Q#TD3's bound widened to cover a blocked
stdin writer (a child that read stdin but stopped draining it),
criterion 5 extended to `docs/agent-handoff.md` §3, Bet 4 marked as
lane-stopping.
- **rev 4** — review round 2. §1.7's causal account was **wrong**: the
basedpyright wrapper uses `subprocess.run` and *waits*; the orphan is
created by pmacs SIGTERMing the wrapper at shutdown, not by the wrapper
exiting at spawn. Corrected here, in the handoff and in the ledger, and
§5's P2 restated — the follow-up is "stop orphaning them", not "tolerate
self-orphaning". Also: the `setsid` dependency is now skip-unless-armed
rather than a hard assert, since it is util-linux and the standard
`--lib` gate must not fail on an undeclared tool.
- **rev 3** — CI falsified rev 2's repair. `<&0` is defeated on `dash`
(the rule applies *before* explicit redirections, so `<&0` duplicates
`/dev/null` onto itself); it passed locally only because `/bin/sh` here
is `bash`. **Control 2 caught it in CI and named its own cause** — the
fourth vacuity in this lane, and the first one a control found instead
of a reviewer. The reproduction now uses `setsid --fork cat`, removing
the shell entirely. Bet 2's falsifier list also gained
`bottom_panel_stage1_acceptance`, which holds PTY-in-panel tests and
was a genuine gap in rev 1's list.
---
## 0. Coherence impact (COHERENCE §20)
This is a defect fix, not coherence work, and it should not claim
otherwise.
- **Journey steps touched:** none directly. It protects the steps that
depend on a live language server (§2 step 6 onward) from an unbounded
teardown, but it adds no journey surface.
- **Interaction islands added:** none.
- **Config registry:** no new options.
- **Background-work attribution:** unchanged. The supervisor's process
model is untouched; only the order of two teardown operations moves.
- **Protocol:** unchanged.
The one genuine coherence connection is indirect and worth stating
plainly: the hang parks `cargo test --workspace`, which is the ratchet
every COHERENCE priority is verified against (§19, §25). A gate that can
hang forever degrades every other lane's evidence. That is the argument
for doing this now rather than parking it — not a claim that it advances
a priority.
---
## 1. Ground truth (scouted @ `e003b81`)
Line numbers are hints; symbols are authoritative.
### 1.1 The reproduction is deterministic, not intermittent
`docs/agent-handoff.md` and the test-improvement audit both describe this
hang as intermittent. On a machine where `basedpyright-langserver`
resolves to a uv-installed shim it is **completely reliable**: 5 runs, 5
hangs, via
```
cargo test --test m4_acceptance -- --exact \
m4_5_basedpyright_initializes_and_negotiates_encoding
```
§1.7 explains why it looks intermittent across machines. The practical
consequence: this defect is directly testable, and any fix has a
revert-bite.
### 1.2 The cycle, in five links
Observed stack of the wedged test thread (gdb, `sudo` required —
`ptrace_scope=1`):
```
tests/m4_acceptance.rs:1374 Rc<RefCell<ProcessSupervisor>> dropped
→ ProcessSupervisor::drop src/process.rs:1545
→ ProcessSupervisor::shutdown src/process.rs:1463
→ ProcessSupervisor::tick src/process.rs:1188
→ ProcessSupervisor::poll_one src/process.rs:1268 (drop site: :1337)
→ RuntimeHandles::drop src/process.rs:631
→ JoinHandle::join ← blocked, indefinitely
```
The links:
1. **`RuntimeHandles::drop` (`:631`)** sets `cancel`, then joins every
handle in `self.readers`.
2. **Rust runs a type's `Drop::drop` body before dropping its fields.**
`stdin: Option<StdinWriter>` is a *field* (`:505`), so it cannot drop
until the body returns. The body never returns.
3. **`StdinWriter` (`:556`)** holds the `Sender`; `StdinWriter::spawn`
(`:568`) moves the `ChildStdin` sink into its thread, which drops the
sink only once `rx.recv()` errors. Sender alive ⇒ sink alive ⇒ **the
child's stdin write end never closes.**
4. **The child therefore never sees EOF**, stays alive, and keeps the
stdout/stderr **write** ends it inherited.
5. **The readers are blocked in `read()`** at `:1886` inside
`spawn_reader` (`:1874`). `cancel` is consulted only at the loop top
(`:1883`) and around `send_timeout` — **never while `read` is
blocked.**
Verified on the live process: the test held fd 4 (child stdin, WRONLY)
and fds 5 and 7 (stdout/stderr, RDONLY); the server process held the
matching opposite ends on fds 0, 1, 2. Two reader threads sat in
`anon_pipe_read`, and the stdin-writer thread sat parked in
`Receiver::recv` at `:577` — alive, still owning the sink.
Confirmation from the other direction: when the wedged test process was
killed, its fd 4 closed, the server immediately saw stdin EOF and
exited. The cycle's load-bearing link is exactly the one the fix cuts.
### 1.3 The existing comment names the false premise
`RuntimeHandles::drop` documents its own reasoning:
> Wake any reader thread blocked in a bounded `send` — dropping the
> master closes the kernel pipe and unblocks `read`, but does nothing
> for a reader stuck on a full channel […]
The premise is true for a **PTY master** and false for **pipe mode**,
where `read` unblocks only when *every* write end closes. `cancel` was
introduced for the full-channel case and is correct for it; the comment
mistakenly treats the `read` case as already handled.
### 1.4 `shutdown()`'s SIGKILL phase is unreachable on this path
`shutdown()` (`:1463`) sends SIGTERM to all ids, then runs a bounded
grace loop (`deadline` at `:1476`) that calls `tick()`, and *then*
escalates to SIGKILL. The stack shows the deadlock occurs **inside that
grace loop's `tick()`**, because `poll_one` drops `RuntimeHandles` the
moment it observes the recorded pid exited. The SIGKILL phase is never
reached.
So "shutdown force-kills everything first" is not true of this path.
(An earlier working assumption of mine said it did; the stack refutes
it.) Even if reached, SIGKILL targets the *recorded* pid, which per
§1.7 is not the surviving process.
### 1.5 Only pipe-mode, non-group spawns are affected
`spawn_pipes` (~`:1712`) chooses per stream:
| `spec.group` | reader | cancellable mid-`read`? |
| --- | --- | --- |
| `true` | `spawn_group_reader` (`:1941`) — `O_NONBLOCK` + `poll` | **yes** |
| `false` | `spawn_reader` (`:1874`) — blocking `read` | **no** |
PTY mode (~`:1724`) also uses `spawn_reader`, but there §1.3's premise
holds: dropping the master genuinely ends the read. The `spawn_ansi_parser`
reader also lives in `readers`, and reads a channel rather than an fd, so
it is unaffected.
Non-group pipe consumers are, per `spawn_reader`'s own doc comment, the
**REPL and LSP** paths. This defect is therefore reachable by every LSP
server and every REPL — not by terminals.
### 1.6 The fix mechanism already exists in this file
`close_stdin` (`:1611`) already does precisely what is needed, and
already documents the semantics and the idempotence:
```rust
// Dropping the writer closes the pipe at the kernel
// level. `take()` is idempotent — second call sees None.
let _ = runtime.stdin.take();
```
The fix is applying an existing, already-reviewed mechanism at the one
site that is missing it. It introduces no new concept.
### 1.7 Why basedpyright wedges and clangd/gopls do not
`basedpyright-langserver` is a uv-installed **Python console script**:
```python
from basedpyright.langserver import main
sys.exit(main())
```
`main()` reaches `run_node.run`, which calls `nodejs_wheel`'s `node(...)`
— and that is **`subprocess.run`** (`nodejs_wheel/executable.py:50`). It
**waits**. Verified in the installed 1.39.6 source, not assumed.
So the wrapper does *not* exit at spawn time, and **pmacs creates the
orphan itself**:
1. The wrapper runs `node …/langserver.index.js --stdio` and blocks. Node
is a genuine grandchild; the initialize handshake completes normally.
2. At teardown, `shutdown()` sends **SIGTERM to the recorded pid** — the
Python wrapper — before entering its grace loop.
3. The wrapper dies on the default disposition and **does not forward the
signal**. Node is reparented to `PPid: 1`, still holding the inherited
pipes, idle in `ep_poll`.
4. `poll_one` then observes the recorded pid terminated, drops
`RuntimeHandles`, and enters the deadlock.
**rev 1–3 of this doc said the wrapper "spawns node and exits".** That was
wrong, and the evidence against it was already in hand: the test's
assertions all pass *before* teardown, so the handshake succeeded — which
is impossible if the wrapper had exited at spawn. The observation that
generated the claim (`PPid: 1`, wrapper gone) was taken **after**
`shutdown()` had already killed it.
This matters for the parked work, not for the fix. The follow-up is not
"tolerate servers that self-orphan" — it is **stop orphaning them**:
signal the process group rather than a wrapper pid that swallows the
signal. P2 in §5 is restated accordingly.
`clangd` and `gopls` are real binaries: genuine children, reaped
normally, write ends closed, blocking `read` returns `Ok(0)` cleanly. The
"intermittency" in the handoff is not timing — it is *which server binary
is installed how*.
### 1.8 Limits of the evidence
- The deterministic reproduction is **one machine, one server**. The
causal chain is verified there link by link; its generality to other
shim-launched servers is reasoned, not measured.
- The gdb capture is a single sample of a state that was stable across a
four-minute window and identical across two independent runs. That is
strong for a deadlock and would be weak for a race.
- Nothing here establishes how often the hang has fired in CI. CI never
installs basedpyright (`PMACS_REQUIRE_PYRIGHT` is deliberately never
set, #194), so in CI this test skips and the defect is **dark**. Every
observation is local.
---
## 2. Decisions
### Q#TD1 — the fix is a reorder inside `Drop`, not a new primitive
```rust
impl Drop for RuntimeHandles {
fn drop(&mut self) {
self.cancel.store(true, Ordering::Relaxed);
// Close the child's stdin BEFORE joining. A stdio child exits
// on EOF and closes its stdout/stderr write ends, and that —
// not `cancel` — is what unblocks a reader parked in `read`
// (`cancel` is only observed between reads and around `send`).
// The sink lives in the `stdin` field, which cannot drop until
// this body returns, so joining first deadlocks against it.
let _ = self.stdin.take();
for h in std::mem::take(&mut self.readers) {
let _ = h.join();
}
}
}
```
Rejected alternative: reordering the struct's *fields*. Field order does
not help — the explicit `Drop::drop` body runs before **all** fields
regardless of their declaration order. This is the trap that makes the
bug non-obvious, and it belongs in the comment.
### Q#TD2 — the reorder is unconditional across modes
Applying it only to pipe+non-group would require `RuntimeHandles::drop`
to learn which mode it is in, which it currently does not need to know.
Closing stdin before teardown is correct in both modes, so the reorder is
unconditional.
This is a uniformity change, and uniformity changes in this repo have
made total functions partial before. It is therefore carried as a **bet
with a named falsifier** (§3, Bet 2), not as an assumption: PTY-mode
`stdin` is the pty *writer*, and dropping it while `pair.master` and the
cloned reader still exist must not end the read early.
### Q#TD3 — the fix assumes the child drains stdin to EOF, and covers nothing outside that
Stated up front because it bounds the claim: the fix works by making the
child exit. A child that never reads stdin — or reads it and ignores EOF
— keeps its write ends open and still wedges the join.
There is a third member of that family, and it is not covered by the
wording above because such a child *did* read stdin: **the EOF is only
delivered if the writer thread reaches the end of its queue.** Its body
is a blocking `sink.write_all(&bytes)` (`:578`), so a child that has
stopped draining stdin while queued bytes remain blocks the writer
indefinitely — the sink never drops, EOF never arrives, and the join
re-wedges. This needs only a full stdin pipe buffer at teardown time, not
a misbehaving child. For LSP teardown the queue is near-empty and the
practical risk is nil, but the bound belongs in the claim: **the fix
assumes the child keeps draining stdin until EOF.** A full stdin pipe
with a non-draining child is P1's case as well.
Covering *that* case requires making the blocking `read` itself
cancellable, i.e. moving non-group readers onto `spawn_group_reader`'s
`O_NONBLOCK` + `poll` mechanism. `spawn_reader`'s doc comment already
names this as a deferral from the compile-mode framing. It stays parked
(§5, P1) rather than riding this PR, because it is a behavioural change
to every REPL and LSP ingest path and deserves its own review.
The honest claim for this PR is therefore: **it fixes the observed
deadlock for stdio children that honour EOF, which is what LSP servers
are, and narrows — not eliminates — the class.**
### Q#TD4 — queued stdin writes are not lost, and the writer is not joined
`crossbeam`'s `Receiver::recv` drains buffered items before reporting
disconnection, so dropping the `Sender` still lets the writer thread
write everything already queued. The writer thread is **not** joined
here, so there remains no guarantee the final flush completes before the
process is signalled. That is pre-existing, unchanged by this PR, and
noted rather than fixed (P3, §5).
Draining is also the mechanism by which the fix can fail to deliver EOF
at all when the child has stopped reading — see Q#TD3's third case.
### Q#TD5 — the leaked orphan server is not fixed here
After the fix, the wedge is gone but a shim-launched server is still an
orphaned grandchild that teardown's recorded pid cannot signal. It exits
here only because it honours stdin EOF — by cooperation, not by
enforcement. A server that ignores EOF leaks. Parked (§5, P2).
### Q#TD6 — the synthetic reproduction must model EOF-honouring, not sleeping, and needs an explicit stdin redirect
Two distinct traps here, and this lane has now walked into **three**
vacuous reproductions, so the reasoning is recorded rather than the
conclusion alone.
**Trap 1 — a sleeping child models the wrong defect.**
`sh -c 'sleep 300 & exit 0'` orphans a grandchild that holds the write
ends but **never reads stdin**, so closing stdin does not free it. That
reproduces a hang this fix does *not* address; it belongs to P1 (§5), not
here.
**Trap 2 — a background job does not inherit stdin.** POSIX XCU §2.9.3:
> If job control is disabled, the standard input of an asynchronous
> list, before any explicit redirections, shall be assigned to
> `/dev/null`.
Job control is off in every non-interactive `sh`, so in
`sh -c 'cat & exit 0'` the background `cat` gets **`/dev/null`**, not the
inherited pipe. It EOFs immediately and exits **against the unfixed
tree** — the test would pass either way and Bet 3's revert-bite would
report VACUOUS.
Measured on this machine (`/bin/sh` → `bash`), stdin attached to a
held-open fifo, checking the orphan's `/proc/<pid>/fd/0`:
| form | grandchild | fd 0 |
| --- | --- | --- |
| `sh -c 'cat & exit 0'` | **gone** | — (EOF'd from `/dev/null`) |
| `sh -c 'cat <&0 & exit 0'` | alive | the real pipe |
**Trap 3 — `<&0` does not repair it, and the obvious fix is wrong.** rev 2
proposed `sh -c 'cat <&0 & exit 0'`, verified on this machine. **CI
falsified it.** Re-read the rule: `/dev/null` is assigned *before any
explicit redirections*, so by the time `<&0` runs, fd 0 already **is**
`/dev/null`, and the redirect faithfully duplicates it onto itself.
`bash` happens to skip the default when a stdin redirect is present;
`dash` — Ubuntu's `/bin/sh`, and CI's — does not. Measured:
| shell | form | grandchild | fd 0 |
| --- | --- | --- | --- |
| bash | `cat & exit 0` | gone | — |
| bash | `cat <&0 & exit 0` | alive | real pipe |
| dash | `cat <&0 & exit 0` | **gone** | — (CI: control 2 failed) |
The local probe could not have caught this: `/bin/sh` here is `bash`.
**The model is therefore `setsid --fork cat`, with no shell at all.**
`setsid --fork` forks, the parent exits, and the child inherits
stdin/stdout/stderr untouched — no asynchronous list, no `/dev/null`
rule, no implementation variance. The recorded pid (`setsid`) terminates
promptly so `poll_one` reaches the teardown path, while `cat` survives
holding the inherited pipes and exits on EOF exactly as a stdio language
server does. Unfixed, this deadlocks; fixed, teardown completes.
`setsid(1)` is util-linux, which the Linux gate already assumes.
Presence is **asserted, not skipped** — a skip would reintroduce the
silent-green shape lane 2 removed.
The controls are what make this recoverable rather than a silent
regression: control 2 failed loudly in CI and named its own cause. That
is #192's lesson one level down — the bite needs a control, and so does
the reproduction.
---
## 3. Bets (falsifiable)
1. **The reorder resolves the observed hang.** Falsified if
`m4_5_basedpyright_initializes_and_negotiates_encoding` still fails to
terminate after the change.
2. **The reorder is safe for PTY mode.** Falsified by any regression in
`vterm_stage1/2/3_acceptance`, `terminal_config_acceptance`,
`terminal_copy_mode_acceptance`, `m6_4/m6_5_repl_acceptance`,
`m6_7_scrollback_acceptance`, `m6_8_multi_repl_acceptance`,
`worker_shutdown_acceptance`, or **`bottom_panel_stage1_acceptance`**
— added in rev 3: it holds PTY-in-panel tests (`acc28` drives real
child input and the `C-c` escape) and its absence from rev 1's list
was a real gap, not a judgement call.
3. **The synthetic test bites.** Falsified if the new test passes with
`let _ = self.stdin.take();` removed. This must be checked by actual
revert, per the standing rule that a new pin needs its own bite.
4. **The basedpyright test passes rather than merely terminating.** The
hang is at teardown (`m4_acceptance.rs:1374`), *after* the body's
assertions, so it should now pass outright. Falsified if it terminates
with a failure — which would mean a second, independent defect.
**If falsified, stop the lane and frame that defect separately.** Do
not paper over it: "terminates" was never the goal, and a failing
assertion here is new information, not a loose end.
---
## 4. Acceptance
1. `RuntimeHandles::drop` takes `stdin` before joining readers, with a
comment naming the drop-body-before-fields trap.
2. New unit test in `src/process.rs` (so it runs under the standard
`cargo test --lib` gate, not only an acceptance suite):
`teardown_closes_stdin_before_joining_readers`.
- Spawns `setsid --fork cat` as a **non-group pipe** process. The
choice of `setsid` over a shell background job is load-bearing
(Q#TD6) and gets a comment saying so. `setsid` presence is
**asserted, not skipped.**
- **Two positive controls, before teardown starts:** (1) the recorded
child has actually exited — while it lives it holds the output pipe
itself, so control 2 would pass for the wrong reason; (2) both
readers are still blocked in `read`, which is only true while
something still holds the write ends. Without these the test
silently degrades into modelling the wrong thing and reports green
while doing it — which is exactly what happened on `dash`, and
control 2 is what caught it.
- `#[cfg(target_os = "linux")]`: the controls read `/proc`, and
`setsid(1)` is util-linux (absent on macOS). Gate it explicitly and
say why, rather than letting it be incidentally Linux-only. (Same
reasoning as the APFS gate — `cfg(unix)` would be wrong here.)
- Performs the full reap-and-drop sequence on a helper thread and
asserts completion via `recv_timeout`, so a regression **fails**
within a bounded window instead of hanging. A test that hangs on
regression would reproduce the exact hazard this PR removes.
- Bound: 10s (default `grace_period` is 2s, `:927`).
- On the failure path the helper thread stays wedged and the `cat`
survives until the harness's fds close at process exit. That is
bounded and acceptable — but the test comment must **say so**, or a
future reviewer correctly flags a leaked thread as a defect.
3. The bite is demonstrated by revert, and the result recorded in the PR
body — pass/fail both ways, per Bet 3.
4. `cargo test --test m4_acceptance` runs **without**
`-- --skip basedpyright` and completes, locally, on the machine where
it currently hangs 5/5.
5. Docs, in **both** places the superseded cause lives — replacing it,
not appending to it:
- `docs/agent-handoff.md` §5 gains the drop-body-before-fields lesson
and the corrected cause, replacing "no timeout on the initialize
handshake".
- `docs/agent-handoff.md` §3's machine caveat currently says the
desktop's **local binary is broken and hangs**. §1.7 shows the
binary was never broken: the shim architecture plus this defect
was. Left alone, §3 keeps steering readers toward a false model —
and toward keeping the skip forever.
**Deliberately not a criterion:** removing `-- --skip basedpyright` from
`CLAUDE.md`'s standing gate list. It is a separate call that is the
user's to make, and it changes only *local* behaviour — CI skips the test
regardless (§1.8). I will propose it with evidence after the fix has been
green repeatedly, rather than fold a process change into a defect fix.
When that proposal comes it owes two things beyond the green runs: the
`docs/agent-handoff.md` §3 caveat updated (criterion 5 covers it here,
but the *skip* rationale lives with it), and an explicit note that
`PMACS_REQUIRE_PYRIGHT` stays **unarmed** in CI until the per-test
timeout lane (3a) merges — the ordering #194 established, where presence
of the variable decides execution and arming without a timeout would give
CI the same unbounded hang this PR removes locally.
---
## 5. Parked (each needs its own evidence)
- **P1 — cancellable non-group `read`.** Move `spawn_reader` onto
`spawn_group_reader`'s `O_NONBLOCK` + `poll` mechanism so `cancel` is
observed within `READER_SEND_POLL_INTERVAL` (`:421`, 50ms) even
mid-`read`. Bounds teardown unconditionally, including for children
that ignore EOF (Q#TD3) — **and** the blocked-writer case, where EOF is
never delivered because `write_all` is stuck on a full pipe. Already
named as a deferral by `spawn_reader`'s own doc comment. Tests: the
`sleep 300` shape from Q#TD6 (child never reads stdin), plus a
fill-the-pipe-then-stop-reading shape for the writer case.
- **P2 — stop orphaning wrapper-launched servers (Q#TD5).** Restated in
rev 4, because the corrected §1.7 changes the target: the orphan is not
self-inflicted by the server, it is created by **us** SIGTERMing a
wrapper that does not forward the signal. Spawn stdio servers in their
own process group and signal the group, reusing the machinery the group
path and `reap_ledger` already have. Fixes a real leak: every
basedpyright-backed session currently leaves a `node` process behind.
Note the ordering consequence — a group-directed SIGTERM would reach
node directly, so this also removes the condition the present fix works
around, rather than merely tolerating it.
- **P3 — join the stdin writer thread** so the final flush is ordered
against child termination (Q#TD4).
- **P4 — re-audit the "intermittent" label** in `docs/agent-handoff.md`
and the audit now that §1.7 explains it. Rides this PR's doc update
only insofar as criterion 5 requires; a broader sweep is separate.
---
## 6. Gates
Per `CLAUDE.md`, each as its own step with a real exit status checked
(never `cmd | tail` — a pipe returns the tail's status and has masked a
real failure here before):
- `cargo fmt --check`
- `cargo clippy --workspace --all-targets -- -D warnings`
- `cargo test --lib`
- `cargo test --lib --features crdt`
- `cargo test --test m4_acceptance` — **without** the basedpyright skip
- The PTY/REPL suites named in Bet 2
- `cargo test --test worker_shutdown_acceptance`
- `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu`
- `git diff --check`
Commit before gating, so the results describe the pushed tree.
---
## 7. Branch plan
One branch, one PR: `process-teardown-stdin-deadlock`, from `main` @
`e003b81` or later. Worktree `pmacs-hang` (already clean at that SHA).
Small diff — the reorder, one unit test, one comment, the handoff
update. P1–P4 do not ride it.
`docs/active-work.md` is integrated **late**, immediately before pushing,
to avoid the ledger-contention treadmill with the other open lanes.

247
src/async_runtime.rs
View File

@ -391,6 +391,70 @@ struct PendingJob {
/// When the job was registered. Used to compute "age" in the
/// `*workers*` buffer.
dispatched_at: Instant,
/// The filesystem mutation this job performs, retained so the
/// main-thread drain can reconcile the editor's path owners once
/// the syscall lands (dired Stage 2a, §5).
///
/// The paths have to live here because the dispatchers **move**
/// them into the worker closure and nothing else retains them, and
/// because the reply is undifferentiated — rename and remove both
/// settle as `ReplyKind::FsUnit`, so a drain cannot key on the
/// reply and must key on the pending job.
///
/// One enum field rather than a pair of `Option`s: two would admit
/// a both-`Some` state that cannot occur, which every consumer
/// would then have to rule out by hand. `COHERENCE.md` §9 is why
/// this is a field on the job and not a side map — the parse
/// job→buffer link already lives in a side map and §9 names that as
/// the defect.
resource: Option<ResourceOp>,
}
/// A settled filesystem mutation, with the paths the worker consumed
/// (dired Stage 2a, §5).
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum ResourceOp {
/// A successful `rename(from, to)`.
Rename {
/// Source path, as the caller spelled it.
from: PathBuf,
/// Destination path, as the caller spelled it.
to: PathBuf,
},
/// A successful `remove(path)`.
Remove {
/// The path that was removed.
path: PathBuf,
},
}
/// What one [`AsyncRuntime::tick`] observed.
///
/// Settle identity and resource metadata come out of **one**
/// transaction — the post-drain loop already borrows `pending` to
/// record completions — so a consumer cannot see a settle without its
/// resource, or the reverse.
#[derive(Clone, Debug, Default)]
pub struct TickOutcome {
/// Ids that transitioned from `Running` to a terminal state during
/// this tick. The Lua runtime resumes coroutines parked on these.
pub settled: Vec<JobId>,
/// Successful resource mutations, **in bus-arrival order. This is
/// not filesystem execution order.**
///
/// [`AsyncRuntime::tick`] drains the reply bus with `try_recv` and
/// the runtime establishes no execution token, so a worker can
/// complete, be descheduled before sending, and have a later
/// mutation's reply arrive first. A consumer that reads "in settle
/// order" and infers causality is wrong; reconciliation is
/// deliberately order-independent (Q#DR29), and the primitive's
/// contract is that a caller with overlapping source/target paths
/// serializes by awaiting each op before dispatching the next.
///
/// Carries **only** jobs that settled
/// [`PendingState::Complete`] — a failed or cancelled mutation
/// reconciles nothing and fires no hook.
pub resources: Vec<ResourceOp>,
}
/// Snapshot of a job's terminal state, returned by
@ -684,6 +748,18 @@ impl AsyncRuntime {
kind: JobKind,
supersede_key: Option<&str>,
stream: Option<usize>,
) -> (JobId, CancellationToken) {
self.allocate_with_resource(kind, supersede_key, stream, None)
}
/// [`Self::allocate`], plus the filesystem mutation this job
/// performs. Only the two mutating fs dispatchers pass `resource`.
fn allocate_with_resource(
&self,
kind: JobKind,
supersede_key: Option<&str>,
stream: Option<usize>,
resource: Option<ResourceOp>,
) -> (JobId, CancellationToken) {
let id = self.next_job_id.fetch_add(1, Ordering::Relaxed);
let cancel = CancellationToken::new();
@ -711,6 +787,7 @@ impl AsyncRuntime {
max_batch: stream.unwrap_or(0),
kind,
dispatched_at: Instant::now(),
resource,
},
);
(id, cancel)
@ -869,7 +946,17 @@ impl AsyncRuntime {
/// Dispatch a `rename(from, to)` job. Settles to
/// [`JobResult::Unit`] on success. T M8.1.
pub fn dispatch_fs_rename(&self, from: PathBuf, to: PathBuf, supersede: Option<&str>) -> JobId {
let (id, cancel) = self.allocate(JobKind::FsRename, supersede, None);
// The closure below MOVES both paths; the pending entry is the
// only thing that still knows them when the reply lands.
let (id, cancel) = self.allocate_with_resource(
JobKind::FsRename,
supersede,
None,
Some(ResourceOp::Rename {
from: from.clone(),
to: to.clone(),
}),
);
let bus = self.workers.clone();
self.pool.dispatch(move |_pool| {
let kind = run_fs_rename(&cancel, &from, &to);
@ -891,7 +978,12 @@ impl AsyncRuntime {
/// Dispatch a `remove(path)` job. T M8.1.
pub fn dispatch_fs_remove(&self, path: PathBuf, supersede: Option<&str>) -> JobId {
let (id, cancel) = self.allocate(JobKind::FsRemove, supersede, None);
let (id, cancel) = self.allocate_with_resource(
JobKind::FsRemove,
supersede,
None,
Some(ResourceOp::Remove { path: path.clone() }),
);
let bus = self.workers.clone();
self.pool.dispatch(move |_pool| {
let kind = run_fs_remove(&cancel, &path);
@ -996,11 +1088,16 @@ impl AsyncRuntime {
}
}
/// Drain every queued reply on the main-thread bus, update
/// pending entries, and return the list of ids that *transitioned
/// from Running to a terminal state* during this tick. The Lua
/// runtime resumes coroutines parked on these ids.
pub fn tick(&self) -> Vec<JobId> {
/// Drain every queued reply on the main-thread bus, update pending
/// entries, and report what settled.
///
/// [`TickOutcome::settled`] is the ids that transitioned from
/// `Running` to a terminal state during this tick — the Lua runtime
/// resumes coroutines parked on these.
/// [`TickOutcome::resources`] is the filesystem mutations among them
/// that **succeeded**, in **bus-arrival order** (see the field's
/// own documentation: that is not execution order).
pub fn tick(&self) -> TickOutcome {
let mut newly_settled = Vec::new();
while let Ok(env) = self.main.try_recv() {
let Ok(reply): Result<WorkerReply, _> = self.main.decode(&env) else {
@ -1071,6 +1168,7 @@ impl AsyncRuntime {
// a successor that came in mid-flight will have overwritten
// the entry already, and that successor's pending lifetime
// is what owns the slot now.
let mut resources = Vec::new();
if !newly_settled.is_empty() {
let pending = self.pending.borrow();
let mut sup = self.supersede.borrow_mut();
@ -1078,6 +1176,16 @@ impl AsyncRuntime {
let now = Instant::now();
for id in &newly_settled {
if let Some(job) = pending.get(id) {
// The harvest (§5): one more read in a loop that
// already borrows `pending` and reads `job.kind`,
// so settle identity and resource metadata come out
// of one transaction. Gated on `Complete` — a
// failed or cancelled mutation reconciles nothing.
if let Some(resource) = &job.resource
&& matches!(job.state, PendingState::Complete(_))
{
resources.push(resource.clone());
}
if let Some(key) = &job.supersede_key
&& sup.get(key) == Some(id)
{
@ -1106,7 +1214,10 @@ impl AsyncRuntime {
completed.pop_back();
}
}
newly_settled
TickOutcome {
settled: newly_settled,
resources,
}
}
/// Snapshot the runtime's job tables for the `*workers*`
@ -1749,6 +1860,126 @@ mod tests {
}
}
/// dired Stage 2a, acceptance 54 (controlled-bus layer). Allocate
/// two resource jobs **without dispatching workers**, inject their
/// successful replies in a chosen order, and assert
/// `TickOutcome.resources` reports exactly that order.
///
/// This is the honest statement of what the runtime guarantees:
/// `tick` drains the reply bus with `try_recv` and establishes no
/// execution token, so what a consumer sees is bus-arrival order.
/// The test fails against sorting by job id or kind, and against any
/// claim that the order recovers dispatch or filesystem-execution
/// order — because the injection order here is *deliberately* the
/// reverse of the allocation order in the first case.
#[test]
fn tick_reports_resources_in_bus_arrival_order_not_allocation_order() {
fn run(reverse: bool) -> Vec<ResourceOp> {
let rt = AsyncRuntime::with_pool_size(1);
let (a, _) = rt.allocate_with_resource(
JobKind::FsRename,
None,
None,
Some(ResourceOp::Rename {
from: PathBuf::from("/tmp/a-from"),
to: PathBuf::from("/tmp/a-to"),
}),
);
let (b, _) = rt.allocate_with_resource(
JobKind::FsRemove,
None,
None,
Some(ResourceOp::Remove {
path: PathBuf::from("/tmp/b-gone"),
}),
);
let order = if reverse { [b, a] } else { [a, b] };
for id in order {
rt.workers
.send(
ASYNC_REPLY_TOPIC,
&WorkerReply {
job_id: id,
kind: ReplyKind::FsUnit,
},
)
.expect("inject reply");
}
let outcome = rt.tick();
assert_eq!(outcome.settled.len(), 2, "both jobs settled");
outcome.resources
}
let a_first = ResourceOp::Rename {
from: PathBuf::from("/tmp/a-from"),
to: PathBuf::from("/tmp/a-to"),
};
let b_first = ResourceOp::Remove {
path: PathBuf::from("/tmp/b-gone"),
};
assert_eq!(
run(true),
vec![b_first.clone(), a_first.clone()],
"B injected first must be reported first, even though A was \
allocated first"
);
assert_eq!(
run(false),
vec![a_first, b_first],
"and the reverse arrival order reverses the report"
);
}
/// A failed or cancelled mutation reconciles nothing, so it must not
/// appear in `resources` at all (acceptance 37's runtime half).
#[test]
fn a_failed_or_cancelled_resource_job_is_not_harvested() {
let rt = AsyncRuntime::with_pool_size(1);
let (failed, _) = rt.allocate_with_resource(
JobKind::FsRename,
None,
None,
Some(ResourceOp::Rename {
from: PathBuf::from("/tmp/nope"),
to: PathBuf::from("/tmp/also-nope"),
}),
);
let (cancelled, _) = rt.allocate_with_resource(
JobKind::FsRemove,
None,
None,
Some(ResourceOp::Remove {
path: PathBuf::from("/tmp/never"),
}),
);
rt.workers
.send(
ASYNC_REPLY_TOPIC,
&WorkerReply {
job_id: failed,
kind: ReplyKind::Error("ENOENT".to_owned()),
},
)
.expect("inject");
rt.workers
.send(
ASYNC_REPLY_TOPIC,
&WorkerReply {
job_id: cancelled,
kind: ReplyKind::Cancelled,
},
)
.expect("inject");
let outcome = rt.tick();
assert_eq!(outcome.settled.len(), 2, "both settled");
assert!(
outcome.resources.is_empty(),
"only Complete mutations are harvested; got {:?}",
outcome.resources
);
}
#[test]
fn dispatch_sum_completes_with_correct_value() {
let rt = AsyncRuntime::with_pool_size(2);

60
src/buffer.rs
View File

@ -148,6 +148,26 @@ struct EditDescription {
inserted_len: u64,
}
/// Provenance of a [`Buffer`]'s name (dired Stage 2a, Q#DR30).
///
/// A rename must move a name that merely *renders* the file's path and
/// must leave a name the user chose alone. String inspection cannot
/// tell those apart — a user may legitimately name a buffer with a
/// string that normalizes to its own path — so the fact is recorded at
/// the moment the name is written instead of being reconstructed
/// later.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum BufferNameOrigin {
/// A caller named this buffer: `Buffer::new`/`from_bytes`, an
/// ordinary [`Buffer::set_name`], or the `pmacs.buffer.set_name`
/// binding. A rename leaves the name alone.
Explicit,
/// The name was derived from the buffer's backing path by a
/// path-backed creation site, through
/// [`Buffer::set_path_derived_name`]. A rename rewrites it.
PathDerived,
}
/// The unit of editable content: rope + identity + views + undo.
///
/// # Threading
@ -159,6 +179,14 @@ pub struct Buffer {
id: BufferId,
rope: Rope,
name: String,
/// Where [`Self::name`] came from. Recorded rather than inferred,
/// because a path-backed buffer's name is **not** reliably its
/// path: `get_or_load_buffer` takes the name from the path *as
/// given* and normalizes only the stored `file_path`, so a
/// relative open is named `foo.rs` while its path is absolute.
/// Rename reconciliation asks this bit, never the string
/// (dired Stage 2a, Q#DR30).
name_origin: BufferNameOrigin,
/// The buffer's single active major mode, if one has been selected.
major_mode: Option<String>,
is_modified: bool,
@ -247,6 +275,10 @@ impl Buffer {
id,
rope,
name: name.into(),
// Construction names a buffer explicitly. A path-backed
// creation site re-records provenance through
// `set_path_derived_name` right after binding the path.
name_origin: BufferNameOrigin::Explicit,
major_mode: None,
is_modified: false,
read_only: false,
@ -449,9 +481,35 @@ impl Buffer {
&self.name
}
/// Set the buffer's name. Used by save-as and rename operations.
/// Set the buffer's name, recording it as **explicitly chosen**
/// ([`BufferNameOrigin::Explicit`]).
///
/// This is the user-facing door — `pmacs.buffer.set_name` and
/// save-as go through it — and it is deliberately explicit even
/// when the string happens to denote the file: naming a buffer
/// `notes` for `${cwd}/notes` is still a naming operation, and a
/// later rename must not overwrite it. Path-backed creation sites
/// use [`Self::set_path_derived_name`] instead.
pub fn set_name(&mut self, name: impl Into<String>) {
self.name = name.into();
self.name_origin = BufferNameOrigin::Explicit;
}
/// Set the buffer's name **and** record that it was derived from
/// the buffer's backing path ([`BufferNameOrigin::PathDerived`]).
///
/// Every site that creates or re-binds a path-backed buffer uses
/// this door, including rename reconciliation itself — so a second
/// rename still follows the path.
pub fn set_path_derived_name(&mut self, name: impl Into<String>) {
self.name = name.into();
self.name_origin = BufferNameOrigin::PathDerived;
}
/// Where this buffer's name came from (dired Stage 2a, Q#DR30).
#[must_use]
pub fn name_origin(&self) -> BufferNameOrigin {
self.name_origin
}
/// This buffer's active major mode, if any.

70
src/diag.rs
View File

@ -266,6 +266,22 @@ impl DiagnosticStore {
*self.epochs.entry(uri.to_owned()).or_insert(0) += 1;
}
/// Drop **every** trace of `uri`, epoch included (dired Stage 2a,
/// §5 finding 4).
///
/// Distinct from [`Self::clear`] on purpose: `clear` *creates* an
/// `epochs` entry (`or_insert(0) += 1`) because a consumer caching
/// against the epoch must observe that the diagnostics went away.
/// Forgetting is the opposite intent — the editor no longer holds
/// this URI at all — so leaving the counter behind would be a
/// URI-keyed leak in the one map nothing else prunes.
pub fn forget(&mut self, uri: &str) {
self.by_uri.remove(uri);
self.severity_counts.remove(uri);
self.stale_uris.remove(uri);
self.epochs.remove(uri);
}
/// Monotonic per-URI change counter: how many times `set` /
/// `clear` ran for this URI. `0` for a URI never written.
/// Consumers cache against this to detect republishes that no
@ -489,6 +505,17 @@ impl DiagnosticView {
}
impl View for DiagnosticView {
/// Re-root this view when the buffer's file was renamed (dired
/// Stage 2a, §5). The URI field is private and `View` has no
/// downcast, so this hook is the only way an outside sweep can
/// reach it — and mutating in place preserves this overlay's
/// position in the window's composition order.
fn rename_resource(&mut self, old_uri: &str, new_uri: &str) {
if self.uri == old_uri {
new_uri.clone_into(&mut self.uri);
}
}
fn kind(&self) -> &'static str {
"diagnostic"
}
@ -745,6 +772,49 @@ mod tests {
}
}
/// dired Stage 2a §5, finding 4. `clear` *creates* an `epochs`
/// entry, because a consumer caching against the epoch has to see
/// that the diagnostics went away; nothing ever removes one. So a
/// `forget_uri` that called `clear` would leave a URI-keyed leak
/// behind in the one map nothing prunes — which is why the forget
/// path is its own store method.
#[test]
fn forget_drops_the_epoch_while_clear_deliberately_bumps_it() {
let mut store = DiagnosticStore::new();
store.set(
"file:///a.rs",
vec![diag(0, DiagnosticSeverity::Error, "boom")],
);
store.mark_stale("file:///a.rs");
assert_eq!(store.epoch_for("file:///a.rs"), 1);
store.clear("file:///a.rs");
assert_eq!(
store.epoch_for("file:///a.rs"),
2,
"clear announces the removal to epoch-keyed caches"
);
store.set(
"file:///a.rs",
vec![diag(0, DiagnosticSeverity::Error, "boom")],
);
store.mark_stale("file:///a.rs");
store.forget("file:///a.rs");
assert!(store.for_uri("file:///a.rs").is_empty(), "diagnostics");
assert!(!store.is_stale("file:///a.rs"), "stale flag");
assert_eq!(
store.severity_counts_for("file:///a.rs"),
(0, 0, 0, 0),
"severity counts"
);
assert_eq!(
store.epoch_for("file:///a.rs"),
0,
"forget leaves no trace at all, epoch included"
);
}
#[test]
fn from_lsp_value_parses_minimal_diagnostic() {
let v = json!({

294
src/editor_core.rs
View File

@ -938,11 +938,18 @@ impl EditorCore {
}
let normalized = normalize_buffer_path(path.to_path_buf());
let (bytes, meta) = crate::file_io::load_file(path)?;
// The name is the path **as given** — a relative open is named
// `foo.rs` while `file_path` below is absolute. Recording the
// provenance (Q#DR30) is what lets rename reconciliation move
// this name without having to guess from the string.
let display_name = path.display().to_string();
let id = self
.registry
.borrow_mut()
.create_from_bytes(display_name, &bytes);
.create_from_bytes(display_name.clone(), &bytes);
if let Ok(b) = self.registry.borrow_mut().get_mut(id) {
b.set_path_derived_name(display_name);
}
self.set_buffer_path(id, Some(normalized));
self.set_buffer_meta(id, Some(meta));
Ok((id, true))
@ -1001,7 +1008,12 @@ impl EditorCore {
}),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {
let display_path = path.display().to_string();
let buffer_id = self.registry.borrow_mut().create(display_path);
let buffer_id = self.registry.borrow_mut().create(display_path.clone());
// Path-backed creation site (Q#DR30): the name is the
// path, so a later rename may move it.
if let Ok(b) = self.registry.borrow_mut().get_mut(buffer_id) {
b.set_path_derived_name(display_path);
}
self.set_buffer_path(buffer_id, Some(path.to_path_buf()));
"[new file]".clone_into(&mut self.status);
Ok(ResolvedTarget::Buffer {
@ -4872,6 +4884,205 @@ impl EditorCore {
.map_err(|e| e.to_string())
}
/// Rebind every buffer affected by a successful rename of `old` to
/// `new` (dired Stage 2a, Q#DR14). Returns one
/// [`RenameRebind`] per buffer moved.
///
/// A rename is a **transaction across path owners**, not a field
/// update. This method owns the two owners that live in the buffer:
/// the stored path and — subject to the provenance rule below — the
/// name. Everything else keyed by the path (URI-keyed LSP stores,
/// diagnostic overlays, dired's pathless handles, a package's own
/// URI table) reconciles off the `resource.renamed` hook that the
/// caller fires, because no buffer-keyed rebind can reach them.
///
/// Both rename paths call this — the drain harvest for
/// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the
/// two cannot drift apart.
///
/// # The name
///
/// The name is rewritten only for a buffer whose name is
/// [`crate::buffer::BufferNameOrigin::PathDerived`]. String
/// inspection cannot substitute for that bit in either direction: a
/// relative open is named `foo.rs` (so an equality test leaves it
/// stale), and a user may name a buffer with a string that
/// normalizes to its own path (so a path-equivalence test
/// overwrites a chosen name). When it does fire, the new name is
/// the **normalized** new path — a buffer opened relatively
/// therefore acquires an absolute name, because no buffer records
/// which base its name was relative to. Reconciliation re-records
/// `PathDerived`, so a second rename still follows.
pub fn reconcile_rename(&mut self, old: &Path, new: &Path) -> Vec<RenameRebind> {
let old_n = normalize_buffer_path(old.to_path_buf());
let new_n = normalize_buffer_path(new.to_path_buf());
// A directory rename moves its whole subtree by construction,
// so descendants are always in scope here.
let affected = {
let reg = self.registry.borrow();
buffers_bound_under(&reg, &old_n, true)
};
let mut rebinds = Vec::with_capacity(affected.len());
for (id, bound) in affected {
// Rebuild the path under the new root. An exact match maps
// to `new` itself; a descendant keeps its relative tail.
let target = if bound == old_n {
new_n.clone()
} else {
match bound.strip_prefix(&old_n) {
Ok(tail) => new_n.join(tail),
// Unreachable: `buffers_bound_under` matched on
// exactly this prefix. Skip rather than guess.
Err(_) => continue,
}
};
let name_followed = {
let mut reg = self.registry.borrow_mut();
let Ok(buf) = reg.get_mut(id) else { continue };
buf.set_file_path(Some(target.clone()));
// The file behind this buffer moved, so metadata
// captured against the old path no longer describes
// it. Clearing is what `set_buffer_path`'s callers do
// via `set_buffer_meta`; leaving it would make
// external-change detection compare against a stat of
// a path that is gone.
buf.set_file_meta(None);
if buf.name_origin() == crate::buffer::BufferNameOrigin::PathDerived {
buf.set_path_derived_name(target.display().to_string());
true
} else {
false
}
};
rebinds.push(RenameRebind {
buffer_id: id,
old_path: bound,
new_path: target,
name_followed,
});
}
rebinds
}
/// Reconcile the buffers a successful delete of `path` orphaned
/// (dired Stage 2a, Q#DR18).
///
/// Walks the whole registry by normalized equality **or**
/// component-aware prefix, so descendants of a deleted directory
/// are included and a second buffer on one path is not missed.
/// Descendants are unconditionally in scope here, unlike in
/// `delete_verdict`: a recursive delete destroyed them, and a
/// non-recursive one only succeeds on an *empty* directory, so a
/// buffer still bound underneath it was already an orphan.
///
/// Policy, per buffer:
///
/// * **modified** — kept alive and reported. The buffer keeps its
/// contents; only the file is gone. This is the half of the
/// promise that is robust, because it runs at drain time against
/// whatever state exists then.
/// * **mid-edit** — skipped entirely and reported in `refused`,
/// **preflighted** rather than discovered. A refusal from
/// `BufferRegistry::remove` is *not* inert: by the time it
/// returns `ConcurrentEdit`, [`Self::kill_buffer`] has already
/// dropped the id from `round_trip_buffers`, closed any side
/// window showing the buffer, and redirected every remaining
/// window onto a fallback with cursor, selection, overlays and
/// scroll position reset. The preflight is *sound*, not merely
/// cheap: phase 1 is entirely `EditorCore`, which holds no Lua
/// handle, so nothing between the check and the removal can
/// re-enter Lua and begin an edit.
/// * otherwise — killed through the full phase 1 above.
///
/// Neither refusal aborts the rest: a directory delete reaching
/// twelve descendants must not stop at the one that is mid-edit.
///
/// # Phase 2 is the caller's
///
/// Buffer removal is two phases and the only place they are
/// composed today is a Lua binding (`pmacs.buffer.kill`). Phase 2 —
/// buffer-scoped keymaps, buffer-local config, folds, and the
/// registered `on_removed` callbacks — lives in `lua_bindings` and
/// needs `&Lua`, so this returns [`DeleteReconcile::killed`] and
/// its caller runs phase 2 over those ids. `EditorCore` does not
/// gain a Lua handle.
pub fn reconcile_delete(&mut self, path: &Path) -> DeleteReconcile {
let affected = {
let reg = self.registry.borrow();
buffers_bound_under(&reg, path, true)
};
let mut out = DeleteReconcile::default();
for (id, _bound) in affected {
let preflight = {
let reg = self.registry.borrow();
let Ok(buf) = reg.get(id) else { continue };
let name = buf.name().to_owned();
if buf.is_modified() {
Some(Err((true, name)))
} else if buf.editing_in_progress() {
Some(Err((false, name)))
} else {
Some(Ok(()))
}
};
match preflight {
Some(Ok(())) => {}
Some(Err((true, name))) => {
out.kept_modified.push((id, name));
continue;
}
Some(Err((false, name))) => {
out.refused.push((
id,
format!("buffer {name:?} is mid-edit; finish the edit first"),
));
continue;
}
None => continue,
}
match self.kill_buffer(id) {
Ok(()) => out.killed.push(id),
// 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
}
/// Re-root every URI-keyed overlay in **every** window from
/// `old_uri` to `new_uri` (dired Stage 2a, §5).
///
/// The traversal mirrors overlay disposal's
/// (`lua_bindings`'s `retain` over `overlay_identity`), with the
/// `retain` replaced by [`View::rename_resource`]. That reaches
/// passive windows as well as the active one — which the Lua attach
/// path cannot, since `pmacs.diag._attach_view` takes the active
/// window and errors otherwise — and preserves composition order,
/// because nothing is removed or re-pushed.
///
/// A window that never received the overlay still has none;
/// renaming cannot re-root an overlay that was never attached.
pub fn rename_resource_in_views(&mut self, old_uri: &str, new_uri: &str) {
for win in self.windows.values_mut() {
for overlay in &mut win.overlays {
overlay.rename_resource(old_uri, new_uri);
}
}
}
/// Switch one frontend's active window to a different buffer, allocating
/// a fresh [`TextView`] for it without changing global active state.
pub fn switch_active_buffer_for(
@ -5129,6 +5340,85 @@ fn backward_word(buf: &Buffer, mut pos: Position) -> Position {
pos
}
/// Every path-bound buffer an operation on `target` affects, paired
/// with its **normalized** stored path (dired Stage 2a; the shared walk
/// query #190 introduced for `delete_verdict`, lifted so rename
/// reconciliation and delete reconciliation cannot drift from it).
///
/// Three properties, each of which a naive lookup gets wrong:
///
/// * It scans **every** buffer.
/// [`crate::buffer_registry::BufferRegistry::find_by_path`] is
/// first-match-only, and duplicate path-bound buffers are reachable
/// from public Lua via `pmacs.buffer.from_file` — so a first match
/// can hide a second buffer on the same path, which then survives
/// pointing at a path that no longer exists.
/// * Both sides are normalized. Stored paths are normalized on write
/// (`set_buffer_path`) while an op names its target however the
/// caller spelled it, so a raw comparison misses the match entirely.
/// * Containment is **component-aware** ([`Path::starts_with`]), never
/// a string prefix: `/foo` is not an ancestor of `/foobar`.
///
/// `include_descendants` is the caller's decision because the two
/// consumers legitimately differ. A delete *guard* scopes descendants
/// to `recursive` (#190: a non-recursive delete destroys nothing
/// beneath the target, so a buffer under it must not refuse the op),
/// whereas a **rename** always moves its whole subtree and a
/// post-delete reconciliation is looking at a directory that is
/// already gone.
pub fn buffers_bound_under(
reg: &crate::buffer_registry::BufferRegistry,
target: &Path,
include_descendants: bool,
) -> Vec<(BufferId, PathBuf)> {
let target = normalize_buffer_path(target.to_path_buf());
let mut out = Vec::new();
for id in reg.ids() {
let Ok(buf) = reg.get(*id) else { continue };
let Some(bound) = buf.file_path() else {
continue;
};
let bound = normalize_buffer_path(bound.to_path_buf());
if bound == target || (include_descendants && bound.starts_with(&target)) {
out.push((*id, bound));
}
}
out
}
/// One buffer moved by [`EditorCore::reconcile_rename`].
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct RenameRebind {
/// The buffer that moved.
pub buffer_id: BufferId,
/// Its normalized path before the rename.
pub old_path: PathBuf,
/// Its normalized path after the rename.
pub new_path: PathBuf,
/// Whether the buffer's **name** followed the path, per
/// [`crate::buffer::BufferNameOrigin`]. Reported rather than
/// inferred so a consumer does not have to re-derive the
/// provenance rule.
pub name_followed: bool,
}
/// Outcome of [`EditorCore::reconcile_delete`].
///
/// Three lists rather than two, because "kept on purpose" and "could
/// not be removed" are different events: collapsing them makes a
/// failure look like a policy decision.
#[derive(Clone, Debug, Default)]
pub struct DeleteReconcile {
/// Buffers whose phase 1 (core-side removal) completed. The
/// caller **must** run phase 2 (`after_buffer_removed`) over
/// these — `EditorCore` holds no Lua handle.
pub killed: Vec<BufferId>,
/// Modified buffers kept alive deliberately, with their names.
pub kept_modified: Vec<(BufferId, String)>,
/// Buffers that could not be removed, with the reason.
pub refused: Vec<(BufferId, String)>,
}
/// Normalize a buffer path to an absolute, lexically-clean form:
///
/// 1. expand a leading `~` / `~/…` against `$HOME`,

1092
src/lsp.rs
View File

File diff suppressed because it is too large Load Diff

26
src/lua_bindings/diag.rs
View File

@ -232,6 +232,32 @@ pub fn install_diag(
)?;
}
// dired Stage 2a §5 step 6 — re-root every attached
// `DiagnosticView` from `old_uri` to `new_uri` after a rename.
//
// `DiagnosticView.uri` is set once at construction and is private,
// and `View` has no downcast, so nothing outside `diag.rs` can
// reach it; the `View::rename_resource` hook is the seam. The sweep
// walks EVERY window, which is what `_attach_view` above cannot do
// — it takes the active window and errors otherwise — so a passive
// split that already holds the overlay is re-rooted too. It mutates
// in place, so each overlay keeps its position in the window's
// composition order; a remove-and-re-push would move the underline
// to the end of the stack and pass a one-window test anyway.
{
diag_mod.set(
"_rename_resource",
lua.create_function(move |lua, (old_uri, new_uri): (String, String)| {
let Some(core) = lua.app_data_ref::<SharedCore>() else {
return Ok(false);
};
core.borrow_mut()
.rename_resource_in_views(&old_uri, &new_uri);
Ok(true)
})?,
)?;
}
pmacs.set("diag", diag_mod)?;
Ok(())
}

365
src/lua_bindings/mod.rs
View File

@ -1671,16 +1671,11 @@ fn delete_verdict(
}
};
let target = crate::editor_core::normalize_buffer_path(path.to_path_buf());
for id in reg.ids() {
let Ok(buf) = reg.get(*id) else { continue };
let Some(bound) = buf.file_path() else {
continue;
};
let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf());
if bound != target && !(scan_descendants && bound.starts_with(&target)) {
continue;
}
// The shared walk (dired Stage 2a): one enumeration, so this guard
// and the two reconciliation seams cannot disagree about which
// buffers an operation on `path` touches.
for (id, _bound) in crate::editor_core::buffers_bound_under(reg, path, scan_descendants) {
let Ok(buf) = reg.get(id) else { continue };
// "Modified" is `Buffer::is_modified()`. No new notion of
// dirtiness, and a *clean* open buffer is deliberately not
// guarded — refusing there would fail legitimate deletes for
@ -1714,6 +1709,223 @@ fn delete_verdict(
DeleteVerdict::Clear
}
/// Reconcile a successful rename and fire `resource.renamed` (dired
/// Stage 2a, §5).
///
/// Both rename paths land here — the drain harvest for
/// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the two
/// can no longer drift, which is how the raw-lookup trap survived being
/// "fixed" once already.
///
/// The hook carries the **paths**, normalized absolute, not the rebind
/// list: dired's buffers are pathless, so a path-keyed consumer must be
/// able to reconcile from `(old, new)` alone. And the Rust side is
/// structurally incapable of being complete — any package may key state
/// by URI in its own module table and the LSP manager will never know —
/// so the hook is the mechanism that scales, not a convenience.
///
/// Returns the rebinds, for a caller that wants to report.
fn reconcile_rename_and_fire(
lua: &Lua,
from: &std::path::Path,
to: &std::path::Path,
) -> Vec<crate::editor_core::RenameRebind> {
let (rebinds, old_n, new_n) = {
let Some(core) = lua.app_data_ref::<SharedCore>() else {
return Vec::new();
};
let mut core = core.borrow_mut();
let rebinds = core.reconcile_rename(from, to);
(
rebinds,
crate::editor_core::normalize_buffer_path(from.to_path_buf()),
crate::editor_core::normalize_buffer_path(to.to_path_buf()),
)
};
// The borrow is released before re-entering Lua: subscribers call
// back into the core (dired reverts a listing, the LSP subscriber
// re-attaches), and a live borrow would panic.
let mut args = mlua::MultiValue::new();
args.push_back(mlua::Value::String(
match lua.create_string(old_n.as_os_str().as_encoded_bytes()) {
Ok(s) => s,
Err(_) => return rebinds,
},
));
args.push_back(mlua::Value::String(
match lua.create_string(new_n.as_os_str().as_encoded_bytes()) {
Ok(s) => s,
Err(_) => return rebinds,
},
));
run_hook_if_defined(lua, "resource.renamed", args);
rebinds
}
/// Reconcile a successful delete and fire `resource.deleted` (dired
/// Stage 2a, §6).
///
/// Composes the **same two removal phases** `pmacs.buffer.kill`
/// composes. Phase 1 (`EditorCore::reconcile_delete`) closes side
/// windows showing a doomed buffer, redirects every other window to a
/// fallback, and removes the id from the registry; phase 2 —
/// buffer-scoped keymaps, buffer-local config, folds, and the
/// registered `on_removed` callbacks — runs here, because it needs
/// `&Lua` and `EditorCore` has no Lua handle.
///
/// `apply_resource_op`'s delete arm previously ran
/// `remove_buffer_and_fire`, i.e. phase 2 **without** phase 1, leaving
/// any window displaying that buffer pointing at a removed id. Routing
/// both paths through here is what makes that go away as a property of
/// the seam rather than as a separate patch.
fn reconcile_delete_and_fire(
lua: &Lua,
path: &std::path::Path,
) -> crate::editor_core::DeleteReconcile {
let (outcome, normalized) = {
let Some(core) = lua.app_data_ref::<SharedCore>() else {
return crate::editor_core::DeleteReconcile::default();
};
let mut core = core.borrow_mut();
let outcome = core.reconcile_delete(path);
(
outcome,
crate::editor_core::normalize_buffer_path(path.to_path_buf()),
)
};
// Phase 2, over exactly the ids phase 1 removed.
for id in &outcome.killed {
after_buffer_removed(lua, *id);
}
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).
///
/// **Each settled mutation reconciles on its own, and nothing here
/// depends on the relative order of two mutations that were in flight
/// simultaneously** — `resources` is bus-arrival order and the runtime
/// establishes no execution token. That is safe rather than merely
/// honest: independent mutations commute, and the primitive's contract
/// (`builtin/runtime/fs.lua`) requires a caller with overlapping
/// source/target paths to serialize by awaiting each op before
/// dispatching the next.
fn reconcile_settled_resources(lua: &Lua, resources: &[crate::async_runtime::ResourceOp]) {
use crate::async_runtime::ResourceOp;
for op in resources {
match op {
ResourceOp::Rename { from, to } => {
reconcile_rename_and_fire(lua, from, to);
}
ResourceOp::Remove { path } => {
reconcile_delete_and_fire(lua, path);
}
}
}
}
fn remove_buffer_and_fire(lua: &Lua, registry: &SharedRegistry, id: BufferId) -> mlua::Result<()> {
registry
.borrow_mut()
@ -3220,6 +3432,37 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
)?;
}
{
// dired Stage 2a Q#DR21 — expose the existing Rust setter,
// which already documents itself as for "save-as and rename
// operations". Dired needs it because its listing buffers are
// **pathless**: no buffer-keyed rebind can find them, so the
// only way a `*dired:<path>*` buffer can follow a renamed
// directory is for dired's own `resource.renamed` subscriber to
// rename it. The alternative — kill and recreate under the new
// name — loses window placement, the cursor, the read-only
// intercept, round-trip input and the major mode, each of which
// would have to be re-established in the right order.
//
// Uniqueness stays the CALLER's job, matching the Rust setter;
// dired reuses its existing `<2>`-variant uniquifier.
//
// This records `BufferNameOrigin::Explicit` (Q#DR30): it is a
// naming operation even when the string happens to denote the
// file, so a later rename must not overwrite it.
let reg = registry.clone();
buffer.set(
"set_name",
lua.create_function(move |_, (id, name): (BufferIdLua, String)| {
reg.borrow_mut()
.get_mut(id.0)
.map_err(mlua::Error::external)?
.set_name(name);
Ok(())
})?,
)?;
}
{
let reg = registry.clone();
buffer.set(
@ -3244,6 +3487,11 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
))
})?;
let id = reg.borrow_mut().create_from_bytes(path.clone(), &bytes);
// Path-backed creation site (Q#DR30): this name is the
// path as given, so rename reconciliation may move it.
if let Ok(b) = reg.borrow_mut().get_mut(id) {
b.set_path_derived_name(path.clone());
}
if let Some(core) = lua.app_data_ref::<SharedCore>() {
let mut core = core.borrow_mut();
core.switch_active_buffer(id)
@ -3307,6 +3555,10 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
))
})?;
let id = reg.borrow_mut().create_from_bytes(path.clone(), &bytes);
// Path-backed creation site (Q#DR30), as in `from_file`.
if let Ok(b) = reg.borrow_mut().get_mut(id) {
b.set_path_derived_name(path.clone());
}
if let Some(core) = lua.app_data_ref::<SharedCore>() {
let mut core = core.borrow_mut();
core.switch_active_buffer(id)
@ -3428,12 +3680,18 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
.map_err(|e| io_err("rename (parents)", e))?;
}
std::fs::rename(&from, &to).map_err(|e| io_err("rename", e))?;
let bid = reg.borrow().find_by_path(&from);
if let Some(id) = bid
&& let Some(core) = lua.app_data_ref::<SharedCore>()
{
core.borrow_mut().set_buffer_path(id, Some(to.clone()));
}
// dired Stage 2a: the raw, first-match,
// un-normalized `find_by_path` lookup this arm
// used is replaced by the shared transaction.
// Three defects went with it — stored paths are
// normalized on write while the op names its
// target raw, so the lookup could miss the
// buffer entirely; a directory rename has many
// affected buffers by construction and only the
// first moved; and the buffer's *name* stayed
// stale, so the statusline and buffer list kept
// the old filename.
reconcile_rename_and_fire(lua, &from, &to);
}
"delete" => {
// Four ordered phases (Q#RD2): stat/no-op
@ -3490,18 +3748,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
};
r.map_err(|e| io_err("delete", e))?;
// Phase 4 — reconcile exactly as before
// (Q#RD10): the single first exact-path match is
// removed and additional clean duplicates are
// left in place. Removing them all would route N
// Phase 4 — reconcile through the shared seam
// (dired Stage 2a, Q#DR27). #190 deliberately
// left this as the single first exact-path match
// because removing them all would have routed N
// buffers through `remove_buffer_and_fire`,
// which is phase 2 without phase 1, creating up
// to N dangling windows — the parked lifecycle
// defect this lane must not enlarge.
let bid = reg.borrow().find_by_path(&pb);
if let Some(id) = bid {
remove_buffer_and_fire(lua, &reg, id)?;
}
// which is phase 2 *without* phase 1 and would
// have created up to N dangling windows. That
// constraint is now gone: `reconcile_delete`
// composes both phases, so descendants and
// duplicate path-bound buffers can all be
// reconciled, and no window is left holding a
// removed id.
reconcile_delete_and_fire(lua, &pb);
}
other => {
return Err(mlua::Error::external(format!(
@ -7358,9 +7617,15 @@ pub fn install_async(
async_mod.set(
"_tick",
lua.create_function(move |lua, ()| {
let ids = rt.tick();
let t = lua.create_table_with_capacity(ids.len(), 0)?;
for (i, id) in ids.into_iter().enumerate() {
let outcome = rt.tick();
// Reconcile BEFORE the settled ids reach Lua. The Lua
// runtime resumes parked coroutines from the table this
// returns, so a coroutine that renamed and then
// inspects a buffer would otherwise see pre-rename
// state. Ordering here is by construction, not by luck.
reconcile_settled_resources(lua, &outcome.resources);
let t = lua.create_table_with_capacity(outcome.settled.len(), 0)?;
for (i, id) in outcome.settled.into_iter().enumerate() {
t.set(i + 1, id)?;
}
Ok(t)
@ -9992,11 +10257,18 @@ pub fn install_lsp(
// `builtin/runtime/lsp.lua` calls this per edit so stale
// suppression stays keystroke-accurate while the O(file)
// full-document notification is coalesced.
//
// **Takes the server id since dired Stage 2a.** It previously
// took the URI alone while creating URI keys in three stores
// for every server at once, which made it the second
// uncorrelated writer able to resurrect a URI `forget_uri` had
// just cleared. The sole production caller already holds
// `rec.server`.
let m = manager.clone();
lsp_mod.set(
"_mark_document_stale",
lua.create_function(move |_, uri: String| {
m.borrow().mark_document_stale(&uri);
lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| {
m.borrow().mark_document_stale(id.0, &uri);
Ok(())
})?,
)?;
@ -10520,6 +10792,35 @@ pub fn install_lsp(
)?;
}
{
// dired Stage 2a §5 — the per-document teardown the
// `resource.renamed` subscriber needs. Modelled on `forget`
// above: a closure over the shared manager that calls through
// and maps the error with `mlua::Error::external`.
//
// Error contract: **raises** for an unknown server id, matching
// `forget`'s behaviour for the same input, and **succeeds
// silently** when the URI has no state under a known server.
// The second arm is the one that matters — the subscriber runs
// per attachment, an attachment need not have any pending route
// or populated result store, and cleanup can be repeated after
// an earlier partial teardown. An over-strict binding would turn
// that ordinary idempotent case into an error inside a hook.
//
// Takes the **old** URI, so calling it after `did_open` of the
// new one is safe and order-independent.
let m = manager.clone();
lsp_mod.set(
"forget_uri",
lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| {
m.borrow_mut()
.forget_uri(id.0, &uri)
.map_err(mlua::Error::external)?;
Ok(())
})?,
)?;
}
{
let m = manager.clone();
lsp_mod.set(

191
src/process.rs
View File

@ -636,6 +636,26 @@ impl Drop for RuntimeHandles {
// channel because the consumer fell behind. Cancel flag
// unwedges that case before we join. T M6.2.
self.cancel.store(true, Ordering::Relaxed);
// Close the child's stdin BEFORE joining. `cancel` covers a
// reader stuck in `send`; it does NOT cover one stuck in
// `read`, which is only consulted between reads. What actually
// unblocks that reader is the child exiting and closing its
// output pipe --- and a stdio child exits on stdin EOF.
//
// The premise in the comment above ("dropping the master
// closes the kernel pipe") holds for a PTY master but NOT for
// pipe mode, where `read` unblocks only once *every* write end
// closes. An escaped descendant holding one (a shim-launched
// language server that orphans its real process) keeps the
// reader blocked indefinitely.
//
// The sink lives in the `stdin` FIELD, and a type's `Drop::drop`
// body runs before *all* of its fields regardless of their
// declaration order --- so reordering the struct cannot fix
// this. Joining first deadlocks against the very EOF that would
// have ended the join. `take()` is idempotent, matching
// `close_stdin`.
let _ = self.stdin.take();
for h in std::mem::take(&mut self.readers) {
let _ = h.join();
}
@ -3204,6 +3224,177 @@ mod tests {
handle.join().expect("test thread should exit cleanly");
}
/// The stdin sink lives in a *field* of [`RuntimeHandles`], so it
/// cannot drop until `Drop::drop`'s body returns --- and a type's
/// drop body runs before *all* of its fields, whatever their
/// declaration order (so reordering the struct cannot fix this).
/// Joining readers inside that body therefore deadlocks against any
/// child that exits on stdin EOF while still holding the output
/// pipe: no EOF, so no exit, so no pipe close, so a blocking
/// `spawn_reader` never returns.
///
/// This is the root cause of
/// `m4_5_basedpyright_initializes_and_negotiates_encoding` hanging
/// forever. Modelled with an orphaned grandchild, which is exactly
/// what a shim-launched language server is: the basedpyright
/// console script spawns bundled `node` and exits, leaving the real
/// server at `PPid 1` holding the inherited pipes.
///
/// `setsid --fork` is used rather than a shell background job, and
/// that choice is LOAD-BEARING. POSIX XCU 2.9.3 assigns `/dev/null`
/// to an asynchronous list's stdin when job control is off --- i.e.
/// in every non-interactive `sh` --- so `sh -c 'cat & exit 0'` reads
/// EOF immediately and exits *against the unfixed tree*, giving a
/// test that passes either way and proves nothing. The obvious
/// repair does not work either: the rule applies **before explicit
/// redirections**, so by the time `<&0` runs, fd 0 already *is*
/// `/dev/null` and the redirect faithfully duplicates it onto
/// itself. `bash` happens to skip the default when a stdin redirect
/// is present; `dash` --- Ubuntu's `/bin/sh`, and CI's --- does not,
/// so `<&0` passed locally and failed in CI.
///
/// `setsid --fork` sidesteps all of it: it forks, the parent exits,
/// and the child inherits stdin/stdout/stderr untouched by any shell.
/// No async list, no `/dev/null` rule, no implementation variance.
///
/// Linux-gated deliberately rather than incidentally: the controls
/// read `/proc`, and `setsid(1)` is util-linux (absent on macOS).
///
/// On the failure path this leaks a wedged worker thread, and `cat`
/// survives until the harness's fds close at process exit. Bounded
/// and intentional --- a test that *hung* on regression would
/// reproduce the very hazard it exists to catch.
#[cfg(target_os = "linux")]
#[test]
fn teardown_closes_stdin_before_joining_readers() {
use std::sync::mpsc;
/// `sh` becomes a zombie when it exits, because this test
/// deliberately never ticks (a tick runs `poll_one`, which is
/// the teardown path under test). `kill(pid, None)` succeeds on
/// a zombie, so liveness has to come from the process state
/// rather than from signal 0.
fn reaped_or_zombie(pid: u32) -> bool {
match std::fs::read_to_string(format!("/proc/{pid}/stat")) {
Err(_) => true,
Ok(s) => s
.rsplit_once(')')
.and_then(|(_, rest)| rest.split_whitespace().next())
.is_some_and(|state| state == "Z"),
}
}
// setsid(1) is util-linux, not coreutils, and the standard
// `cargo test --lib` gate must not hard-fail on a tool the
// README does not require --- a minimal or BusyBox container
// would fail without ever testing pmacs. So: skip when absent,
// but FAIL when `PMACS_REQUIRE_SETSID` is set, which CI sets on
// Linux. That is the arming pattern from the silent-skip lane,
// and it is what keeps this from becoming a test that reports
// `ok` having never run. Presence decides, so an empty value
// counts as unset (a `${{ cond && '1' || '' }}` expression sets
// the empty string, not nothing).
let armed = std::env::var_os("PMACS_REQUIRE_SETSID").is_some_and(|v| !v.is_empty());
if !binary_available("setsid") {
assert!(
!armed,
"PMACS_REQUIRE_SETSID is set but setsid(1) is not on PATH: \
install util-linux, or unset the variable to allow the skip"
);
eprintln!(
"setsid(1) not on PATH; skipping \
teardown_closes_stdin_before_joining_readers"
);
return;
}
let (done_tx, done_rx) = mpsc::channel();
let handle = std::thread::spawn(move || {
let mut sup = ProcessSupervisor::new();
sup.set_grace_period(Duration::from_millis(300));
let mut spec = ProcessSpec::new("orphan-holds-pipe", "setsid");
// `setsid --fork` forks and the parent exits, so the
// *recorded* pid terminates promptly (letting `poll_one`
// reach the teardown path) while `cat` survives holding the
// inherited pipes. `cat` reads stdin and exits on EOF,
// exactly as a stdio language server does.
spec.args = vec!["--fork".into(), "cat".into()];
// The default, restated because it is the whole point: with
// `StdinMode::Null` there is no sink to drop and no EOF to
// deliver.
spec.stdin = StdinMode::Piped;
let id = sup.spawn(spec).expect("spawn");
let sh_pid = sup
.processes
.get(&id)
.and_then(|p| p.runtime.as_ref())
.map(|rt| rt.pid)
.expect("runtime records the spawned pid");
// CONTROL 1: the recorded child must actually exit. Until it
// does, *it* holds the output pipe, and control 2 would pass
// for the wrong reason. (`setsid` without `--fork` may exec
// directly instead of forking, in which case there is no
// grandchild and this is the control that notices.)
let deadline = Instant::now() + Duration::from_secs(5);
while Instant::now() < deadline && !reaped_or_zombie(sh_pid) {
std::thread::sleep(Duration::from_millis(10));
}
assert!(
reaped_or_zombie(sh_pid),
"control 1 failed: the recorded child (`sh`) should exit \
promptly, leaving the grandchild orphaned. While `sh` is \
alive it holds the output pipe itself, so control 2 would \
pass without the grandchild modelling anything"
);
// CONTROL 2: both readers must still be blocked in `read`,
// which is only true while something still holds the output
// pipe's write ends. If the grandchild never inherited the
// real stdin, it has already read EOF and exited, the write
// ends are closed, the readers have finished --- and the
// deadlock is not being modelled at all. This control is
// what caught the shell form failing on dash after it
// passed on bash.
let readers = sup
.processes
.get(&id)
.and_then(|p| p.runtime.as_ref())
.map(|rt| {
(
rt.readers.len(),
rt.readers.iter().filter(|h| !h.is_finished()).count(),
)
})
.expect("runtime still present before teardown");
assert_eq!(
readers,
(2, 2),
"control 2 failed: both readers must still be blocked in \
`read`, i.e. an escaped grandchild still holds the output \
pipe. Finished readers mean `cat` read EOF and exited \
already, so it never inherited the real stdin --- check \
that `setsid --fork` still forks and passes fds 0/1/2 \
through untouched on this runner"
);
// The deadlock, if present, is here:
// shutdown -> tick -> poll_one -> RuntimeHandles::drop -> join.
drop(sup);
let _ = done_tx.send(());
});
done_rx.recv_timeout(Duration::from_secs(10)).expect(
"supervisor drop should complete within 10s --- if hung, \
`RuntimeHandles::drop` is joining its readers before dropping \
the `stdin` field, so the child never receives EOF, never \
exits, and never closes the output pipe the readers are \
blocked on",
);
handle.join().expect("test thread should exit cleanly");
}
// -----------------------------------------------------------------
// Compile-mode group lifecycle (Q#CM3; framing acceptance 34)
// -----------------------------------------------------------------

14
src/view.rs
View File

@ -310,6 +310,20 @@ pub trait View {
fn clone_for_split(&self) -> Option<Box<dyn View>> {
None
}
/// Retarget this overlay from `old_uri` to `new_uri` after a
/// resource rename (dired Stage 2a, §5). Default: no-op — a view
/// that renders nothing URI-keyed is unaffected.
///
/// Mutates **in place**, so the overlay keeps its position in the
/// window's composition order. That is the reason this is a trait
/// hook rather than a remove-and-re-push at the call site: overlays
/// are an ordered `Vec` merged in sequence, and re-pushing would
/// move a diagnostic underline to the end of the stack. It is also
/// how *passive* windows are reached at all — the Lua attach path
/// (`pmacs.diag._attach_view`) can only touch the active window,
/// while the sweep that drives this walks every window.
fn rename_resource(&mut self, _old_uri: &str, _new_uri: &str) {}
}
// ---------------------------------------------------------------------------

100
tests/m4_acceptance.rs
View File

@ -8549,17 +8549,26 @@ fn rd8_recursive_delete_refuses_for_a_modified_descendant() {
assert!(tree.exists(), "including the directory itself");
}
/// Criterion 9 — a *clean* recursive delete leaves descendant buffers
/// orphaned, not removed.
/// Criterion 9 — a *clean* recursive delete reconciles descendant
/// buffers, through both removal phases.
///
/// This pin deliberately asserts today's imperfect behaviour. Widening
/// reconciliation to the tree would route N buffers through
/// `remove_buffer_and_fire` — phase 2 without phase 1 — promoting the
/// parked dangling-window defect from exact-path to tree-wide.
/// **Rewritten by dired Stage 2a** (`docs/dired-stage2-framing.md` §6,
/// Q#RD27 / acceptance 23). This row previously pinned the opposite —
/// that the descendant buffer stayed orphaned — and gave the reason:
/// widening reconciliation would have routed N buffers through
/// `remove_buffer_and_fire`, which is phase 2 *without* phase 1, so a
/// tree delete would have left up to N windows pointing at removed ids.
/// That constraint is discharged: `EditorCore::reconcile_delete`
/// composes the same two phases `pmacs.buffer.kill` composes, and the
/// delete arm routes through it. The old assertion is not merely
/// obsolete, it is now the defect — an orphaned buffer whose next
/// `C-x C-s` recreates a file the user deleted.
///
/// Bite: fails against an implementation that widens reconciliation.
/// Bite, both directions: fails against an exact-path reconciliation
/// (the descendant survives) **and** against a widening that skips
/// phase 1 (a window keeps a removed id).
#[test]
fn rd9_clean_recursive_delete_leaves_descendants_orphaned() {
fn rd9_clean_recursive_delete_reconciles_descendants_through_both_phases() {
let dir = tempfile::tempdir().expect("tempdir");
let tree = dir.path().join("tree");
std::fs::create_dir(&tree).expect("mkdir");
@ -8568,6 +8577,13 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() {
let mut state = pmacs::editor::EditorState::new();
rd_open(&mut state, "B", &inner);
// Display it, so the phase-1 window redirect has something to do.
state
.lua_host
.lua()
.load("pmacs.window.switch_buffer(B)")
.exec()
.expect("show the descendant");
let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true");
assert!(ok, "a clean tree deletes: {err}");
@ -8580,9 +8596,23 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() {
.eval()
.expect("validity probe");
assert!(
still,
"THE BITE: reconciliation stays exact-path, so the descendant \
buffer is orphaned rather than removed"
!still,
"THE BITE: a buffer under a recursively deleted directory must be \
reconciled away, not left bound to a path whose file is gone"
);
let core = state.core.borrow();
let dangling: Vec<_> = core
.windows
.iter()
.filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id))
.map(|(id, w)| (*id, w.buffer_id))
.collect();
assert!(
dangling.is_empty(),
"THE OTHER HALF: widening the reconciliation must not promote the \
dangling-window defect from exact-path to tree-wide; dangling: \
{dangling:?}"
);
}
@ -8633,15 +8663,22 @@ fn rd10_absent_plus_ignore_does_not_destroy_a_modified_buffer() {
assert_eq!(text, "unsavedcontent\n", "the unsaved edit survives");
}
/// Criterion 14 — clean duplicates: exactly one match reconciled.
/// Criterion 14 — clean duplicates: **every** match reconciled.
///
/// Bite: fails against an implementation that removes **all** matches.
/// It pins the reconciliation half of Q#RD10 and *only* that: with both
/// buffers clean there is no verdict difference between consulting one
/// match and consulting all, so this setup cannot see validation
/// breadth. Criterion 6 is what detects incomplete validation.
/// **Rewritten by dired Stage 2a** (§6, acceptance 23). This row
/// previously pinned "exactly one", which was Q#RD10's deliberate
/// restraint: removing them all would have routed N buffers through
/// `remove_buffer_and_fire` — phase 2 without phase 1 — so the second
/// duplicate was left alive rather than have its window dangle.
/// `reconcile_delete` composes both phases, so the restraint is gone and
/// the surviving duplicate is now the defect: it is bound to a path
/// whose file no longer exists, and `find_by_path` cannot even see it.
///
/// Bite: fails against a first-match implementation (one duplicate
/// survives) and against a widening that skips phase 1 (a window keeps
/// a removed id).
#[test]
fn rd14_clean_duplicates_reconcile_exactly_one() {
fn rd14_clean_duplicates_all_reconcile() {
let dir = tempfile::tempdir().expect("tempdir");
let f = dir.path().join("twin.rs");
std::fs::write(&f, b"twin\n").expect("write");
@ -8658,6 +8695,13 @@ fn rd14_clean_duplicates_reconcile_exactly_one() {
.exec()
.expect("two clean buffers on one path");
state
.lua_host
.lua()
.load("pmacs.window.switch_buffer(SECOND)")
.exec()
.expect("show the second duplicate");
let (ok, err) = rd_delete(&mut state, &f, "");
assert!(ok, "two clean duplicates must not block: {err}");
@ -8668,9 +8712,23 @@ fn rd14_clean_duplicates_reconcile_exactly_one() {
.eval()
.expect("validity probe");
assert!(
first != second,
"THE BITE: exactly one duplicate is reconciled away, not both \
and not neither (first={first}, second={second})"
!first && !second,
"THE BITE: both buffers bound to the deleted path must be \
reconciled away; a survivor points at a file that is gone and is \
invisible to `find_by_path` (first={first}, second={second})"
);
let core = state.core.borrow();
let dangling: Vec<_> = core
.windows
.iter()
.filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id))
.map(|(id, w)| (*id, w.buffer_id))
.collect();
assert!(
dangling.is_empty(),
"THE OTHER HALF: removing every match must not leave a window on \
a removed id; dangling: {dangling:?}"
);
}

2045
tests/resource_reconciliation_acceptance.rs Normal file
View File

File diff suppressed because it is too large Load Diff