From 7562d8319831b4dfcf8a3daf2a8721b038bb0864 Mon Sep 17 00:00:00 2001
From: Levi Neuwirth <ln@levineuwirth.org>
Date: Tue, 14 Jul 2026 10:57:14 +0100
Subject: [PATCH] fix(compile): make overlay teardown atomic

---
 docs/compile-mode-framing.md     |  23 ++++--
 src/lua_bindings/mod.rs          |  50 +++++++++++--
 tests/compile_mode_acceptance.rs | 124 +++++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+), 12 deletions(-)

diff --git a/docs/compile-mode-framing.md b/docs/compile-mode-framing.md
index 3cd2d2b..5d2addc 100644
--- a/docs/compile-mode-framing.md
+++ b/docs/compile-mode-framing.md
@@ -1,7 +1,18 @@
 # Compile-mode — framing (Arc 5 stage 1, terminal)
 
-**Revision 13 — 2026-07-14. Status: implemented on branch
-`compile-mode` (PR #113); revisions 7–13 fold in PR rounds 1–7.**
+**Revision 14 — 2026-07-14. Status: implemented on branch
+`compile-mode` (PR #113); revisions 7–14 fold in PR rounds 1–8.**
+
+Revision 14 (PR #113 round 8, direct review fixes): overlay disposal
+now preflights both the optional editor-core borrow and the required
+registry borrow before changing the shared disposed flag or removing
+either view. A re-entrant callback therefore receives a pointed,
+retryable error instead of a `RefCell` panic or partial teardown; the
+acceptance bite holds each borrow in turn, proves the handle remains
+fully live, and retries successfully. `attach_style_overlay` also
+resolves the recorded owner in the registry after its identity checks,
+so a handle whose buffer (and translator) has died is rejected as
+stale rather than reporting a successful no-op.
 
 Revision 13 (PR #113 round 7, findings 1–2): overlay handle
 attachment is validated — `attach_style_overlay` rejects a handle
@@ -287,12 +298,14 @@ Everything below was verified by reading the code, not the roadmap.
   the store identity, and same-buffer splits copy the render view to
   the new pane (Revision 12). Attachment validates the handle:
   wrong-buffer and disposed handles are rejected with messages
-  pointing at `add_style_overlay` (Revision 13). The handle has
+  pointing at `add_style_overlay`; a handle whose recorded owner has
+  died is rejected as stale (Revisions 13–14). The handle has
   `add`, `clear`, `clear_before`, `spans`, and idempotent `dispose`
   (teardown of the translator + every window render view; the
   translator detach rides the always-registered registry, not the
-  optional editor core; one handle per buffer incarnation needs no
-  disposal).
+  optional editor core; teardown preflights both borrows so re-entrant
+  calls fail atomically and can be retried; one handle per buffer
+  incarnation needs no disposal).
 - **Buffer-switch hooks**: `buffer.after-switch` exists and fires on
   the ordinary switch paths (recentf subscribes,
   `builtin/runtime/recentf.lua:54`). **`pmacs.editor.jump_back` does
diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs
index 279c374..0ad2752 100644
--- a/src/lua_bindings/mod.rs
+++ b/src/lua_bindings/mod.rs
@@ -1101,6 +1101,16 @@ pub enum BindingError {
          after the edit completes"
     )]
     Reentrant,
+
+    /// Style-overlay teardown was requested from a callback that is
+    /// still running under an editor-core or buffer-registry borrow.
+    /// Disposal touches both stores, so it must acquire both before
+    /// changing the shared disposed flag or removing either view.
+    #[error(
+        "style overlay disposal cannot run while editor state is borrowed; defer dispose() until \
+         after the current callback completes"
+    )]
+    StyleOverlayDisposeReentrant,
 }
 
 // ---------------------------------------------------------------------------
@@ -1851,13 +1861,32 @@ impl UserData for StyleOverlayHandleLua {
         // repeated creation on a long-lived buffer. Safe to call
         // twice; safe after the buffer is gone.
         methods.add_method("dispose", |lua, this, ()| {
+            // Preflight every borrow before changing shared state.
+            // A callback may run while the editor core or registry is
+            // already borrowed; panicking (or removing the window
+            // views before discovering a registry conflict) would
+            // leave a partially disposed handle. Returning a pointed
+            // error keeps the operation retryable after the callback.
+            let core_handle = lua.app_data_ref::<SharedCore>();
+            let mut core = match core_handle.as_deref() {
+                Some(core) => Some(core.try_borrow_mut().map_err(|_| {
+                    mlua::Error::external(BindingError::StyleOverlayDisposeReentrant)
+                })?),
+                None => None,
+            };
+            let registry_handle = lua
+                .app_data_ref::<SharedRegistry>()
+                .ok_or_else(|| mlua::Error::external(BindingError::NoRegistry))?;
+            let mut registry = registry_handle
+                .try_borrow_mut()
+                .map_err(|_| mlua::Error::external(BindingError::StyleOverlayDisposeReentrant))?;
+
             this.disposed
                 .store(true, std::sync::atomic::Ordering::Relaxed);
             let id = crate::overlay::style_store_identity(&this.spans);
             // Window cleanup needs the editor core, which is
             // optional app data...
-            if let Some(core) = lua.app_data_ref::<SharedCore>() {
-                let mut core = core.borrow_mut();
+            if let Some(core) = core.as_mut() {
                 for win in core.windows.values_mut() {
                     win.overlays.retain(|v| v.overlay_identity() != Some(id));
                 }
@@ -1867,11 +1896,8 @@ impl UserData for StyleOverlayHandleLua {
             // WITHOUT a core, and returning success while the
             // translator stays attached would leak per-edit work for
             // the buffer's lifetime (round-7 finding 2).
-            if let Some(registry) = lua.app_data_ref::<SharedRegistry>() {
-                let mut r = registry.borrow_mut();
-                if let Ok(buf) = r.get_mut(this.buffer) {
-                    buf.detach_view(this.translator);
-                }
+            if let Ok(buf) = registry.get_mut(this.buffer) {
+                buf.detach_view(this.translator);
             }
             Ok(())
         });
@@ -3029,6 +3055,16 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
                             handle.buffer, id.0
                         )));
                     }
+                    // The recorded owner may have been removed since
+                    // the handle was created. Buffer IDs are
+                    // generational, so resolving it is the only way
+                    // to distinguish a live owner from a stale handle;
+                    // silently scanning the windows would otherwise
+                    // report a successful no-op.
+                    with_registry(lua, |r| {
+                        resolve(r, id.0)?;
+                        Ok(())
+                    })?;
                     attach_style_overlay_to_visible_windows(lua, id.0, &handle.spans);
                     Ok(())
                 },
diff --git a/tests/compile_mode_acceptance.rs b/tests/compile_mode_acceptance.rs
index d1c9799..56be46c 100644
--- a/tests/compile_mode_acceptance.rs
+++ b/tests/compile_mode_acceptance.rs
@@ -2594,6 +2594,130 @@ fn r7f2_dispose_detaches_translator_in_a_headless_host() {
     );
 }
 
+// ---------------------------------------------------------------------------
+// PR #113 round 8 — direct review fixes
+// ---------------------------------------------------------------------------
+
+#[test]
+fn r8f1_dispose_is_atomic_and_retryable_under_reentrant_borrows() {
+    // dispose() is callable from Lua callbacks that may still be
+    // running under an EditorCore or BufferRegistry RefCell borrow.
+    // It must return a pointed error before changing the shared
+    // disposed state or removing either view, then succeed when
+    // retried after that callback completes.
+    let s = editor();
+    exec(
+        &s,
+        r#"
+        _G.r8_buf = pmacs.buffer.create("*r8-dispose*")
+        pmacs.window.switch_buffer(_G.r8_buf)
+        _G.r8_ov = pmacs.buffer.add_style_overlay(_G.r8_buf)
+        "#,
+    );
+
+    let (ok_core, err_core): (bool, String) = {
+        let _core_borrow = s.core.borrow_mut();
+        eval(
+            &s,
+            "local ok, err = pcall(_G.r8_ov.dispose, _G.r8_ov); \
+             return ok, tostring(err)",
+        )
+    };
+    assert!(!ok_core, "dispose under a core borrow must fail cleanly");
+    assert!(
+        err_core.contains("defer dispose()"),
+        "core-borrow error must name the recovery; got: {err_core}"
+    );
+    let (still_attached, can_attach): (i64, bool) = eval(
+        &s,
+        r#"
+        local n = 0
+        for _, kind in ipairs(pmacs.window._overlay_kinds()) do
+            if kind == "buffer_style_overlay" then n = n + 1 end
+        end
+        return n, pcall(pmacs.buffer.attach_style_overlay, _G.r8_buf, _G.r8_ov)
+        "#,
+    );
+    assert_eq!(
+        still_attached, 1,
+        "failed disposal must not remove render views"
+    );
+    assert!(
+        can_attach,
+        "failed disposal must not mark the handle disposed"
+    );
+
+    let registry = s.core.borrow().registry.clone();
+    let (ok_registry, err_registry): (bool, String) = {
+        let _registry_borrow = registry.borrow_mut();
+        eval(
+            &s,
+            "local ok, err = pcall(_G.r8_ov.dispose, _G.r8_ov); \
+             return ok, tostring(err)",
+        )
+    };
+    assert!(
+        !ok_registry,
+        "dispose under a registry borrow must fail cleanly"
+    );
+    assert!(
+        err_registry.contains("defer dispose()"),
+        "registry-borrow error must name the recovery; got: {err_registry}"
+    );
+    let (still_attached, can_attach): (i64, bool) = eval(
+        &s,
+        r#"
+        local n = 0
+        for _, kind in ipairs(pmacs.window._overlay_kinds()) do
+            if kind == "buffer_style_overlay" then n = n + 1 end
+        end
+        return n, pcall(pmacs.buffer.attach_style_overlay, _G.r8_buf, _G.r8_ov)
+        "#,
+    );
+    assert_eq!(
+        still_attached, 1,
+        "registry conflict must not partially dispose"
+    );
+    assert!(can_attach, "registry conflict must leave the handle live");
+
+    let (ok_final, remaining): (bool, i64) = eval(
+        &s,
+        r#"
+        local ok = pcall(_G.r8_ov.dispose, _G.r8_ov)
+        local n = 0
+        for _, kind in ipairs(pmacs.window._overlay_kinds()) do
+            if kind == "buffer_style_overlay" then n = n + 1 end
+        end
+        return ok, n
+        "#,
+    );
+    assert!(ok_final, "dispose must be retryable after the borrow ends");
+    assert_eq!(remaining, 0, "successful retry removes the render view");
+}
+
+#[test]
+fn r8f2_attach_rejects_a_handle_whose_owner_buffer_was_removed() {
+    // BufferId equality alone does not prove that the recorded owner
+    // is still live. Without a registry resolution this returned
+    // success after the buffer (and its translator) had been removed.
+    let s = editor();
+    let (ok, err): (bool, String) = eval(
+        &s,
+        r#"
+        local buf = pmacs.buffer.create("*stale-overlay-owner*")
+        local ov = pmacs.buffer.add_style_overlay(buf)
+        pmacs.buffer.remove(buf)
+        local ok, err = pcall(pmacs.buffer.attach_style_overlay, buf, ov)
+        return ok, tostring(err)
+        "#,
+    );
+    assert!(!ok, "attachment for a removed owner must be rejected");
+    assert!(
+        err.contains("stale buffer handle"),
+        "stale-owner error must identify the invalid handle; got: {err}"
+    );
+}
+
 #[test]
 fn r5f3_tracked_line_start_matches_the_scan_across_transitions() {
     // Round-5 finding 3 is a performance fix — the per-CR/BS/erase