fix(compile): make overlay teardown atomic
This commit is contained in:
parent
b6e44f21d6
commit
7562d83198
|
|
@ -1,7 +1,18 @@
|
||||||
# Compile-mode — framing (Arc 5 stage 1, terminal)
|
# Compile-mode — framing (Arc 5 stage 1, terminal)
|
||||||
|
|
||||||
**Revision 13 — 2026-07-14. Status: implemented on branch
|
**Revision 14 — 2026-07-14. Status: implemented on branch
|
||||||
`compile-mode` (PR #113); revisions 7–13 fold in PR rounds 1–7.**
|
`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
|
Revision 13 (PR #113 round 7, findings 1–2): overlay handle
|
||||||
attachment is validated — `attach_style_overlay` rejects a 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 store identity, and same-buffer splits copy the render view to
|
||||||
the new pane (Revision 12). Attachment validates the handle:
|
the new pane (Revision 12). Attachment validates the handle:
|
||||||
wrong-buffer and disposed handles are rejected with messages
|
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`
|
`add`, `clear`, `clear_before`, `spans`, and idempotent `dispose`
|
||||||
(teardown of the translator + every window render view; the
|
(teardown of the translator + every window render view; the
|
||||||
translator detach rides the always-registered registry, not the
|
translator detach rides the always-registered registry, not the
|
||||||
optional editor core; one handle per buffer incarnation needs no
|
optional editor core; teardown preflights both borrows so re-entrant
|
||||||
disposal).
|
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
|
- **Buffer-switch hooks**: `buffer.after-switch` exists and fires on
|
||||||
the ordinary switch paths (recentf subscribes,
|
the ordinary switch paths (recentf subscribes,
|
||||||
`builtin/runtime/recentf.lua:54`). **`pmacs.editor.jump_back` does
|
`builtin/runtime/recentf.lua:54`). **`pmacs.editor.jump_back` does
|
||||||
|
|
|
||||||
|
|
@ -1101,6 +1101,16 @@ pub enum BindingError {
|
||||||
after the edit completes"
|
after the edit completes"
|
||||||
)]
|
)]
|
||||||
Reentrant,
|
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
|
// repeated creation on a long-lived buffer. Safe to call
|
||||||
// twice; safe after the buffer is gone.
|
// twice; safe after the buffer is gone.
|
||||||
methods.add_method("dispose", |lua, this, ()| {
|
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
|
this.disposed
|
||||||
.store(true, std::sync::atomic::Ordering::Relaxed);
|
.store(true, std::sync::atomic::Ordering::Relaxed);
|
||||||
let id = crate::overlay::style_store_identity(&this.spans);
|
let id = crate::overlay::style_store_identity(&this.spans);
|
||||||
// Window cleanup needs the editor core, which is
|
// Window cleanup needs the editor core, which is
|
||||||
// optional app data...
|
// optional app data...
|
||||||
if let Some(core) = lua.app_data_ref::<SharedCore>() {
|
if let Some(core) = core.as_mut() {
|
||||||
let mut core = core.borrow_mut();
|
|
||||||
for win in core.windows.values_mut() {
|
for win in core.windows.values_mut() {
|
||||||
win.overlays.retain(|v| v.overlay_identity() != Some(id));
|
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
|
// WITHOUT a core, and returning success while the
|
||||||
// translator stays attached would leak per-edit work for
|
// translator stays attached would leak per-edit work for
|
||||||
// the buffer's lifetime (round-7 finding 2).
|
// the buffer's lifetime (round-7 finding 2).
|
||||||
if let Some(registry) = lua.app_data_ref::<SharedRegistry>() {
|
if let Ok(buf) = registry.get_mut(this.buffer) {
|
||||||
let mut r = registry.borrow_mut();
|
buf.detach_view(this.translator);
|
||||||
if let Ok(buf) = r.get_mut(this.buffer) {
|
|
||||||
buf.detach_view(this.translator);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
});
|
});
|
||||||
|
|
@ -3029,6 +3055,16 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result<T
|
||||||
handle.buffer, id.0
|
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);
|
attach_style_overlay_to_visible_windows(lua, id.0, &handle.spans);
|
||||||
Ok(())
|
Ok(())
|
||||||
},
|
},
|
||||||
|
|
|
||||||
|
|
@ -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]
|
#[test]
|
||||||
fn r5f3_tracked_line_start_matches_the_scan_across_transitions() {
|
fn r5f3_tracked_line_start_matches_the_scan_across_transitions() {
|
||||||
// Round-5 finding 3 is a performance fix — the per-CR/BS/erase
|
// Round-5 finding 3 is a performance fix — the per-CR/BS/erase
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue