fix(gpu): F-008 fail-fast must actually tear down the session, not just flag it
Review follow-up on the F-008 bounded outbox. Closing the outbox on a lossless overflow set a `closed` flag but did not disconnect: the reader stayed blocked on its still-open socket clone, so no `Disconnected` fired, the daemon was never signaled, and the optimistic CRDT edit whose `send_crdt_op` failed was applied locally, logged, and forgotten. That is silent divergence — the GPU keeps showing text the daemon never received, the exact stalled-daemon case F-008 exists to handle. Keep a `shutdown_handle` socket clone and `shutdown(Both)` whenever the outbox closes — the overflow path in `send_event`, and the writer's own write-failure path. Clones share the socket's file description, so the shutdown wakes the reader (blocked in `read_message`) with EOF: it fires the existing `Disconnected` flow, which renders `(daemon disconnected)`, and the daemon sees the half-close. The fail-fast is now a real teardown → the user gets a visible disconnect (and a fresh snapshot on re-attach) instead of a silently diverged buffer. New socketpair test asserts a send against a closed outbox drives the peer to EOF. Auto-reconnect/resync remains deferred (named in the framing). Validated: fmt clean; clippy -p pmacs-gpu --all-targets clean; 52 pmacs-gpu tests pass (incl. the new shutdown test + both headless renders on the local adapter). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014TXbAwk27agwhrNNrhLi2U
This commit is contained in:
parent
be6943c49d
commit
45b02d1597
|
|
@ -136,9 +136,19 @@ Landed as framed; all three in `pmacs-gpu`, no protocol/daemon change.
|
||||||
same-kind `Viewport`/`Pointer{Drag}` (coalesce), appends everything else
|
same-kind `Viewport`/`Pointer{Drag}` (coalesce), appends everything else
|
||||||
lossless, and on a lossless append past `OUTBOX_MAX` sets `closed` +
|
lossless, and on a lossless append past `OUTBOX_MAX` sets `closed` +
|
||||||
returns `false` (fail-fast). The writer waits on the condvar, `mem::take`s
|
returns `false` (fail-fast). The writer waits on the condvar, `mem::take`s
|
||||||
the whole batch, and writes with the lock released. 5 unit tests
|
the whole batch, and writes with the lock released.
|
||||||
(coalesce-to-latest, clicks keep order, same-kind-tail only, overflow
|
**Fail-fast is a real teardown, not just a flag** (review follow-up):
|
||||||
closes, coalescing is uncapped).
|
closing the outbox alone left the reader blocked on its still-open
|
||||||
|
socket clone, so the optimistic edit whose `send_crdt_op` failed was
|
||||||
|
applied locally, logged, and forgotten — silent divergence, the exact
|
||||||
|
stalled-daemon case F-008 targets. Now a `shutdown_handle` clone is
|
||||||
|
`shutdown(Both)` whenever the outbox closes (overflow in `send_event`,
|
||||||
|
or a writer write error): the reader wakes with EOF and fires the
|
||||||
|
existing `Disconnected` path (`(daemon disconnected)` in the window),
|
||||||
|
and the daemon sees the half-close. 6 unit tests (coalesce-to-latest,
|
||||||
|
clicks keep order, same-kind-tail only, overflow closes, coalescing is
|
||||||
|
uncapped, and a socketpair test asserting a closed outbox drives the
|
||||||
|
peer to EOF).
|
||||||
- **F-007** (`main.rs`): a pure `mb_dropdown_window(n, selected, band_top)
|
- **F-007** (`main.rs`): a pure `mb_dropdown_window(n, selected, band_top)
|
||||||
-> Option<(first, count)>` clamps the row count to what fits above the
|
-> Option<(first, count)>` clamps the row count to what fits above the
|
||||||
band (hides entirely when not even one row fits, so `top_y` is never
|
band (hides entirely when not even one row fits, so `top_y` is never
|
||||||
|
|
@ -150,9 +160,11 @@ Landed as framed; all three in `pmacs-gpu`, no protocol/daemon change.
|
||||||
byte-identical to before. 1 unit test.
|
byte-identical to before. 1 unit test.
|
||||||
|
|
||||||
Validated: `cargo fmt` clean; `clippy -p pmacs-gpu --all-targets` clean;
|
Validated: `cargo fmt` clean; `clippy -p pmacs-gpu --all-targets` clean;
|
||||||
51 pmacs-gpu unit tests pass, including the two headless render tests on
|
52 pmacs-gpu unit tests pass, including the two headless render tests on
|
||||||
this box's Vulkan adapter (`PMACS_REQUIRE_GPU=1`). No divergence from the
|
this box's Vulkan adapter (`PMACS_REQUIRE_GPU=1`). Divergence from the
|
||||||
framing.
|
framing: the F-008 fail-fast needed an actual socket teardown, added as a
|
||||||
|
review follow-up (above); the framing's "clean disconnect" was otherwise
|
||||||
|
aspirational.
|
||||||
|
|
||||||
**Still needs a human eyeball before merge** (per the validation
|
**Still needs a human eyeball before merge** (per the validation
|
||||||
implication above): attach to a non-CRDT daemon → the actionable banner;
|
implication above): attach to a non-CRDT daemon → the actionable banner;
|
||||||
|
|
@ -162,11 +174,13 @@ with the selection visible; and a normal attach still renders/resizes
|
||||||
|
|
||||||
## Deferred (named)
|
## Deferred (named)
|
||||||
|
|
||||||
- **F-008 degraded banner.** Overflow-close currently surfaces only as
|
- **F-008 auto-reconnect / resync.** Overflow-close now actively tears the
|
||||||
logged send errors (the reader thread still drives the visible
|
session down and shows `(daemon disconnected)` (the review follow-up
|
||||||
`Disconnected` status). A dedicated "daemon not keeping up — reconnect"
|
above). What's still deferred is *recovery*: `pmacs-gpu` has no
|
||||||
banner + auto-reconnect is deferred to the reconnect thread
|
auto-reconnect, so the diverged optimistic replica is reconciled only by
|
||||||
([[attach_reconnect]] already exists daemon-side for the TUI).
|
a fresh `BufferSnapshot` on a manual re-attach. A "daemon not keeping up
|
||||||
|
— reconnecting…" banner + automatic re-attach belongs with the reconnect
|
||||||
|
thread ([[attach_reconnect]] already exists daemon-side for the TUI).
|
||||||
- **F-003 capability renegotiation.** We reject on missing caps; a
|
- **F-003 capability renegotiation.** We reject on missing caps; a
|
||||||
friendlier flow would offer to relaunch the daemon with `crdt`. Out of
|
friendlier flow would offer to relaunch the daemon with `crdt`. Out of
|
||||||
scope — the frontend can't manage the daemon's lifecycle.
|
scope — the frontend can't manage the daemon's lifecycle.
|
||||||
|
|
|
||||||
|
|
@ -276,6 +276,15 @@ pub fn connect(
|
||||||
// buffer state — safe to read on one clone while the other writes
|
// buffer state — safe to read on one clone while the other writes
|
||||||
// (the FD is full-duplex).
|
// (the FD is full-duplex).
|
||||||
let mut read_stream = stream.try_clone().map_err(AttachClientError::Connect)?;
|
let mut read_stream = stream.try_clone().map_err(AttachClientError::Connect)?;
|
||||||
|
// A third clone kept solely to *shut the socket down* (F-008). When the
|
||||||
|
// outbox closes — a lossless overflow, or a writer write error — we
|
||||||
|
// `shutdown(Both)` so the reader (blocked in `read_message` on its own
|
||||||
|
// clone) wakes with EOF and fires `Disconnected`. That routes the
|
||||||
|
// stall into the existing visible teardown instead of leaving the GPU
|
||||||
|
// showing locally-applied edits the daemon never received. Clones share
|
||||||
|
// the socket's file description, so a shutdown on any of them affects
|
||||||
|
// all — and it half-closes toward the daemon so it sees the departure.
|
||||||
|
let shutdown_handle = stream.try_clone().map_err(AttachClientError::Connect)?;
|
||||||
let write_stream = stream;
|
let write_stream = stream;
|
||||||
// Bounded, coalescing outbound queue (F-008) shared with the writer
|
// Bounded, coalescing outbound queue (F-008) shared with the writer
|
||||||
// thread; the `Condvar` wakes the writer when the UI thread enqueues.
|
// thread; the `Condvar` wakes the writer when the UI thread enqueues.
|
||||||
|
|
@ -336,6 +345,10 @@ pub fn connect(
|
||||||
if let Err(e) = write_message(&mut write_stream, &event) {
|
if let Err(e) = write_message(&mut write_stream, &event) {
|
||||||
eprintln!("pmacs-gpu: attach writer stopped: {e}");
|
eprintln!("pmacs-gpu: attach writer stopped: {e}");
|
||||||
lock.lock().expect("outbox lock").closed = true;
|
lock.lock().expect("outbox lock").closed = true;
|
||||||
|
// Wake the reader (and signal the daemon) so the
|
||||||
|
// session tears down visibly (F-008) rather than
|
||||||
|
// leaving the reader blocked on a dead socket.
|
||||||
|
let _ = write_stream.shutdown(std::net::Shutdown::Both);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -345,6 +358,7 @@ pub fn connect(
|
||||||
|
|
||||||
Ok(AttachClient {
|
Ok(AttachClient {
|
||||||
outbox,
|
outbox,
|
||||||
|
shutdown_handle,
|
||||||
frontend_id: hello.assigned_frontend_id,
|
frontend_id: hello.assigned_frontend_id,
|
||||||
server_protocol_version: hello.protocol_version,
|
server_protocol_version: hello.protocol_version,
|
||||||
})
|
})
|
||||||
|
|
@ -357,6 +371,10 @@ pub struct AttachClient {
|
||||||
/// thread. `send_event` locks it, applies the enqueue policy, and
|
/// thread. `send_event` locks it, applies the enqueue policy, and
|
||||||
/// wakes the writer via the paired `Condvar`.
|
/// wakes the writer via the paired `Condvar`.
|
||||||
outbox: Arc<(Mutex<Outbox>, Condvar)>,
|
outbox: Arc<(Mutex<Outbox>, Condvar)>,
|
||||||
|
/// Socket clone used only to `shutdown(Both)` when the outbox closes
|
||||||
|
/// (F-008), so the reader wakes and the session tears down visibly
|
||||||
|
/// instead of diverging silently. See [`connect`].
|
||||||
|
shutdown_handle: UnixStream,
|
||||||
/// Assigned by the daemon in the `Hello` response. Every
|
/// Assigned by the daemon in the `Hello` response. Every
|
||||||
/// `FrontendEvent` carries this so the daemon can route input back
|
/// `FrontendEvent` carries this so the daemon can route input back
|
||||||
/// to the per-session `SemanticRenderState`.
|
/// to the per-session `SemanticRenderState`.
|
||||||
|
|
@ -480,6 +498,15 @@ impl AttachClient {
|
||||||
cvar.notify_one();
|
cvar.notify_one();
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
|
// Refused because the outbox is closed — a lossless overflow
|
||||||
|
// against a stalled daemon (or an earlier writer failure).
|
||||||
|
// Tear the session down actively (F-008): shut the socket so
|
||||||
|
// the reader wakes with EOF and fires `Disconnected`, giving a
|
||||||
|
// visible "(daemon disconnected)" instead of a GPU that keeps
|
||||||
|
// showing optimistic edits the daemon never received. Idempotent
|
||||||
|
// — a second shutdown just returns `NotConnected`, ignored.
|
||||||
|
drop(ob);
|
||||||
|
let _ = self.shutdown_handle.shutdown(std::net::Shutdown::Both);
|
||||||
Err(TransportError::Io(std::io::Error::new(
|
Err(TransportError::Io(std::io::Error::new(
|
||||||
std::io::ErrorKind::BrokenPipe,
|
std::io::ErrorKind::BrokenPipe,
|
||||||
"attach writer stopped or outbound queue overflowed",
|
"attach writer stopped or outbound queue overflowed",
|
||||||
|
|
@ -667,4 +694,32 @@ mod tests {
|
||||||
assert_eq!(ob.queue.len(), 1);
|
assert_eq!(ob.queue.len(), 1);
|
||||||
assert!(!ob.closed);
|
assert!(!ob.closed);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_closed_outbox_shuts_the_socket_down_to_wake_the_reader() {
|
||||||
|
use std::io::Read;
|
||||||
|
// A socketpair stands in for the daemon connection; `a` is the peer
|
||||||
|
// a blocked reader would be reading from.
|
||||||
|
let (mut a, b) = UnixStream::pair().expect("socketpair");
|
||||||
|
// The post-overflow state: the outbox is already closed.
|
||||||
|
let mut outbox = Outbox::new();
|
||||||
|
outbox.closed = true;
|
||||||
|
let client = AttachClient {
|
||||||
|
outbox: Arc::new((Mutex::new(outbox), Condvar::new())),
|
||||||
|
shutdown_handle: b,
|
||||||
|
frontend_id: FrontendId::LOCAL,
|
||||||
|
server_protocol_version: PROTOCOL_VERSION,
|
||||||
|
};
|
||||||
|
// A send against the closed outbox fails *and* shuts the socket
|
||||||
|
// down (F-008 fail-fast is now a real teardown, not just a flag).
|
||||||
|
assert!(client.send_event(fe_key('x')).is_err());
|
||||||
|
// The peer reads EOF: a real blocked reader would wake here and
|
||||||
|
// fire Disconnected, instead of the session diverging silently.
|
||||||
|
let mut buf = [0u8; 8];
|
||||||
|
assert_eq!(
|
||||||
|
a.read(&mut buf).expect("read peer"),
|
||||||
|
0,
|
||||||
|
"peer should see EOF after the shutdown"
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue