External-PHP loader ownership is acquired but never released — a discarded buffer serves phantom text after did_close

[mikebronner]

Summary

Implements #365.

did_open/did_change stamp ExternalPhpText::PushedByClient so the backing-class loader never reads disk over an unsaved edit. Nothing ever handed that ownership back. Close a buffer with its changes discarded and the editor reverts in memory without writing to disk, so no did_change_watched_files event fires either. The stamp survived both, and the loader went on serving text that existed neither on disk nor in any open buffer — until the file happened to be reopened.

did_close now releases the closed path, and the release is counted: did_open counts its buffer in, did_close counts it out, and ownership goes back to the loader only when the last buffer for that path is gone. The next resolution then re-reads disk.

The release evicts nothing. RemoveFile is not called: the SourceFile input, the per-file caches, and the resolved magic-member entries all survive. That keeps the ownership edge separate from the eviction question did_close deliberately declines to reopen.

Changes

• salsa_impl.rs — SalsaRequest::ReleaseExternalPhpOwnership and its AcquireExternalPhpOwnership counterpart, both SalsaHandle methods, and the two actor arms. SalsaActor::release_external_php_ownership drops the path from external_php_text once its buffer count runs out; acquire_external_php_ownership is the count going up. Nothing else is touched.
• salsa_impl.rs — external_php_open_buffers, the per-path count of open editor buffers. Absent means zero.
• main.rs — did_open acquires before its update_file push; did_close releases, and its comment now states the ownership-release rule beside the existing eviction rationale.
• salsa_impl.rs — the ExternalPhpText::PushedByClient doc said the pusher owns invalidation, without saying how long. It now names the release edge and the discarded-buffer case that needs one.
• tests/external_php_ownership_release.rs — seven tests, driving the real did_open and did_close handlers.

Round 2 — the close/reopen race

Round 1's release was path-keyed and unconditional, and that could strip a live buffer's ownership. tower-lsp does not serialize notification handlers (DEFAULT_MAX_CONCURRENCY = 4 in 0.20.0; this server takes the default), so the didOpen of a reopened buffer can reach the Salsa actor before the didClose that preceded it at the client. The actor stamps PushedByClient for the new buffer and the late release removes it — the very state the release exists to prevent, reached from the other side. It is a write, not a stale read: the loader's next pass takes the reload arm and set_texts the shared SourceFile every per-file query reads, so a $this->member hover in a Blade view could silently revert the reopened PHP buffer's text to disk.

The property established: the release cannot drop an ownership stamp installed by a later open of the same path.

Counting is what establishes it. Increments and decrements commute, so a close/reopen pair settles at one buffer whichever order it reaches the actor in. A flag, or any "is this the buffer I closed?" test, has to answer a question about ordering; a count does not.

Both of the expressions suggested in review were tried first, and both leave the property unproven:

• Gate on the closed document's text or version. A reopen that carries the same unsaved text — Zed's multibuffer lifecycle, a tab restored with its edits — pushes bytes identical to the closed buffer's, and the version can match too if the client keeps its buffer's counter across the close. The guard then passes in exactly the case that matters: a dirty live buffer, clobbered with disk.
• Gate on the path's absence from self.documents at actor time. did_close removes the URI from documents before it sends the release, so when the reopen's insert lands first, that removal erases the live entry and the check answers "not open" for a buffer that is. It closes one interleaving and not the other.

The acquire goes before the push, not after. A concurrent didClose can land anywhere in did_open's sequence. Acquire-first is safe at every landing point: before the acquire it releases the earlier buffer's stamp, which the push then re-installs; between or after, it decrements to a non-zero count and drops nothing. Push-first opens one window — the close landing between the stamp and the count — where a live buffer is left unowned.

Honest limit: the mutation table below pins what the count decides, and a sequential handler test cannot pin the ordering of two sends inside did_open. Moving the acquire after the push leaves every test green. That ordering rests on the argument above and on the comment at the call site, not on coverage.

Other decisions worth naming

A path with no count still releases. That is the state a didChangeWatchedFiles push leaves behind — ownership with no buffer to close it — and it is the safe direction for the original defect: every divergence falls toward consulting disk, never toward serving a buffer nobody holds. Pinned by a_close_releases_a_path_no_open_ever_counted.

RemoveFile deliberately does not clear the count. Deleting a path on disk does not close its buffer, so the buffer's claim outlives the file — and its own didClose still balances the books.

The release is fallible where mark_pushed_by_client is not, which is the shape #361 was bounced for. It differs here. The call fails only if the Salsa actor is unreachable, and ensure_external_php_source_loaded — the reader the release exists to unblock — runs inside that same actor. A failed release cannot leave phantom text for anything to serve, because no live reader is left to serve it. The acquire logs the same way, at the level did_open already uses for its own Salsa push.

Sweep

The defect class is an acquire with no matching release. mark_pushed_by_client has three call sites: handle_update_file, and two in ensure_blade_source_registered. All three acquire on behalf of a client push, and did_close is the release edge for every one — it fires for the closed path whether that path is a .php backing class or a .blade.php template.

The round-2 fix adds its own pair, so the same question applies to it: did_open is the only handler that opens a buffer, did_close the only one that closes one (it holds the sole documents.remove in the server), and both are guarded by the same to_file_path() check. One acquire site, one release site, no third.

One acquirer holds ownership permanently and correctly: did_change_watched_files pushes the watcher's own fresh read for files that are not open. The watcher never goes away and pushes again on every change, so that ownership needs no release. The enum doc says so explicitly.

external_php_text has exactly one reader (ensure_external_php_source_loaded), and it is unchanged by this PR.

Acceptance Criteria

• did_close relinquishes ExternalPhpText::PushedByClient for the closed path, so the next ensure_external_php_source_loaded re-reads disk.
• RemoveFile is not called on close — the magic-member entries did_close's existing comment protects survive, with a test asserting they do (closing_a_document_keeps_its_resolved_magic_members).
• A regression test drives the real sequence (a_discarded_buffer_stops_answering_once_its_document_closes): disk declares increment(), an unsaved buffer renames it to decrement(), did_close arrives with no disk write, and the disk method resolves again while the buffer's does not.
• The did_close comment states the ownership-release rule alongside the existing eviction rationale.

Test Plan

• Full suite green — 3602 tests, 0 failures.
• cargo clippy --all-targets clean; cargo fmt --check clean.
• Every new test mutation-verified — each reddens for a different defect, and no test is redundant:

Mutation	Test that reddens
Remove the release call from did_close	a_discarded_buffer_stops_answering_once_its_document_closes
Release clears the whole map instead of one path	closing_one_document_leaves_another_buffers_ownership_intact
did_close calls remove_file instead of releasing	closing_a_document_keeps_its_resolved_magic_members
did_close unwraps to_file_path()	closing_a_document_with_no_file_path_is_a_no_op
Release ignores the count (round 1's unconditional drop)	a_reopen_that_overtakes_its_close_keeps_the_buffers_ownership
did_open stops acquiring	a_reopen_that_overtakes_its_close_keeps_the_buffers_ownership
Release never drops a counted path	the_reopened_buffer_releases_when_it_closes_in_turn (and the round-1 regression)
Release requires a count before dropping	a_close_releases_a_path_no_open_ever_counted

The regression tests carry seed assertions before the close — the buffer's method resolves and the saved one does not. Without that pair, the post-close assertions could not tell a released stamp from a buffer that was never installed. The race tests use three distinct method names (increment on disk, decrement in the closed buffer, multiply in the reopened one) so "the reopened buffer answers" cannot pass as "the first push's text was never replaced".

Fixes #365

[mr-sherlock-holmes]

🔄 Changes Requested

All four acceptance criteria are met, and the craft here is genuinely high — this is a well-shaped fix with unusually honest tests. One blocker, and it lives in the new edge itself.

Issues Found

🔴 The release can clobber a live buffer's ownership on a close→reopen race — main.rs:24173

did_close now sends ReleaseExternalPhpOwnership to the Salsa actor. If a didOpen for the same URI follows closely (reopen / revert / Zed's multibuffer lifecycle), the reopen's UpdateFile can reach the actor's mpsc before the close's release. The actor then does the right things in the wrong order: handle_update_file stamps PushedByClient (salsa_impl.rs:9830), and the late release removes it (salsa_impl.rs:9917) — leaving a live, open buffer unowned.

That is the exact state this PR exists to prevent, arrived at from the other side.

Why the reordering is reachable — verified, not assumed. tower-lsp 0.20 does not serialize notification handlers:

• transport.rs:22 — const DEFAULT_MAX_CONCURRENCY: usize = 4;
• transport.rs:142 — service.call(req) only constructs the handler future and pushes it unawaited onto server_tasks_tx.
• transport.rs:117 — .buffer_unordered(self.max_concurrency) drives up to 4 of them concurrently; they complete in await-resolution order, not arrival order.
• main.rs:28428 — Server::new(...).serve(service) takes that default; no .concurrency_level(1).

The Salsa actor is strictly FIFO by enqueue order, so whichever handler reaches .send() first wins. did_close awaits three locks before its Salsa call (main.rs:24134, 24138, 24141); did_open awaits one (main.rs:23927) before update_file (main.rs:23940). The shorter path can win.

Why the blast radius is not contained. ensure_external_php_source_loaded does not merely serve a cross-file query — with the stamp gone it takes the _ => true arm (salsa_impl.rs:10104-10109) and mutates the shared input: existing.set_text(&mut self.db).to(text) (salsa_impl.rs:10112-10121). That is the one SourceFile every other handler reads via self.files.get(path) — loop-blocks (9925), document-symbols (10157), php-assignments (10136), patterns (10212). Your own comment at salsa_impl.rs:10108-10110 says it plainly: this write "replaces the text every per-file cache was populated from," which would otherwise "answer goto and completion out of the previous text."

So the trigger is a hover in one file and the corruption surfaces in another: an ordinary $this->member hover in any co-open Blade view backed by this class (main.rs:20442 → 20104) silently reverts the reopened buffer's text to disk. It self-heals only on the next did_change for that path — not the one that triggered it.

The property to establish (not prescribing the expression — your call which is cleanest): the release must not be able to drop an ownership stamp installed by a later open of the same path. Gating on the closed document's version/text, or on the path's absence from self.documents at actor time, both get there. Please pin it with a regression test in the same style as the existing four.

Honest calibration: the chain is compound — it needs a reopen whose text diverges from disk and a cross-file query inside the window before the next keystroke. Both links are ordinary; the chain is probabilistic rather than guaranteed. I'm blocking rather than tracking it because the defect is in the edge this PR adds (it was unreachable before — did_close previously made no Salsa call touching external_php_text), and it is the same acquire/release asymmetry the issue was filed to close.

What's Good

• The design is right. Downgrade-without-eviction keeps the ownership question and the eviction question separate, which is exactly what #365 asked for and what makes it compatible with the existing RemoveFile rationale.
• The tests are honest, and rare in being so. They drive the real did_close handler on the LspService::new harness and assert through locate_in_backing_class_files — the actual production goto/hover path (main.rs:11491, 24802), not a proxy. The buffer renames increment→decrement so the disk/buffer distinction cannot pass vacuously, and closing_a_document_keeps_its_resolved_magic_members pins the magic-member survival AC #2 asks for, including the dependent's resolution through it.
• The unconditional drop is reasoned, not lazy (salsa_impl.rs:9896-9902) — "a LoadedFromDisk guard would be a branch no didClose can reach, and therefore one no test can honestly pin" is the right instinct. The race above is the one case that reasoning doesn't cover.
• Path-keyed release covers all three acquire sites uniformly — the buffer push (9830) and both Blade live-text stamps (10055, 10063).
• Error handling matches convention, and the comment justifies it correctly: the only failure is an unreachable actor, and the reader lives inside that actor.
• The comments are excellent and, checked claim by claim, accurate. CI is green on all three platforms.

📋 Non-blocking follow-ups

• None. (#364's path_within_root containment gap on this same function is correctly left out of scope — verified untouched by this diff.)

Please address the race and re-request review.

[dr-john-h-watson]

Blocker fixed — the release is now counted, not path-keyed and unconditional.

did_open counts its buffer in before it pushes; did_close counts it out, and ownership goes back to the loader only when the count runs out. Increments and decrements commute, so a close/reopen pair settles at one buffer whichever order it reaches the actor in. The property you named holds without a test for ordering: the release cannot drop a stamp installed by a later open of the same path.

I did not use either suggested expression, and the PR body says why in full. In short: a text/version gate passes in the one case that matters, because a reopen can carry the same unsaved bytes (and the same version, if the client keeps its buffer's counter across the close). A documents-absence check at actor time closes one interleaving and not the other, because did_close removes the URI before it sends the release — when the reopen's insert lands first, that removal erases the live entry and the check answers "not open" for a buffer that is.

Four mutations run, each reddening its own test: the count guard removed (round 1's behaviour), did_open no longer acquiring, a counted path never released, and a release that demands a count. One thing the tests do not pin, stated in the body too: moving the acquire after the push leaves every test green, since a sequential handler test cannot order two sends inside did_open. That ordering rests on the argument and the call-site comment.

No non-blocking follow-ups were listed. Full suite green — 3602 tests; clippy and fmt clean.

[dr-john-h-watson]

⚠️ The Rust CI matrix has not run on this round, and I could not make it run.

What happened. Two pushes — 14e278f (the fix) and dde352d (an empty commit, pushed only to fire a fresh pull_request event). Both times Code Quality: PR #367 (CodeQL) started and passed within a minute, and the CI workflow never appeared. The rollup for this head lists two checks, both green, neither of them the Rust matrix.

What I ruled out. The workflow is active, not disabled (GET /actions/workflows). It triggers on plain pull_request with no branch or path filter, and it ran twice on this same branch earlier today (17:44, 17:57) and on PR #366 at 19:03. Nothing in this diff touches .github/. The last CI dispatch anywhere in the repo was 19:03; every attempt after that has produced CodeQL only, which is the shape of exhausted Actions minutes rather than a per-PR fault — CodeQL bills separately from the macOS and Windows runners this matrix needs.

What green looks like locally, on 14e278f, the same three commands the matrix runs:

• cargo test — 3602 passed, 0 failed
• cargo clippy --all-targets — clean
• cargo fmt --check — clean

One platform, not three. The matrix exists because Windows and macOS diverge, so treat this as unverified there.

I am leaving the item in In Progress rather than moving it to review: the work is finished and pushed, but the gate did not run, and that is not mine to declare green. The next dispatch resumes on this branch and re-checks; if the cap is the cause, it clears on its own.

dde352d is empty and exists only for the re-trigger. Squash it or drop it at merge.