Respawn the session when the transport is poisoned - #12
Conversation
5034088 to
8ddc053
Compare
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/frama-c/transport.rs">
<violation number="1" location="src/frama-c/transport.rs:125">
P3: When a peer dies during `recv_frame`, later calls report an incomplete frame write even though no write failed. Use a direction-neutral poison message so read-side failures remain diagnosable.</violation>
</file>
<file name="src/mcp/server.rs">
<violation number="1" location="src/mcp/server.rs:3036">
P2: When another request is failing concurrently, this one-time poison snapshot can be false. `ensure_main_spawned` then chooses in-place reload, which observes the newly poisoned transport, returns an error, and requires a second call to respawn. Serialize the poison decision with the transport request or retry through the respawn path when the reload observes a newly poisoned client.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| s.poisoned | ||
| || s.with_rte != new_rte | ||
| || s.project_options != new_project_options | ||
| || client_lock.as_ref().is_some_and(|c| c.is_poisoned()) |
There was a problem hiding this comment.
P2: When another request is failing concurrently, this one-time poison snapshot can be false. ensure_main_spawned then chooses in-place reload, which observes the newly poisoned transport, returns an error, and requires a second call to respawn. Serialize the poison decision with the transport request or retry through the respawn path when the reload observes a newly poisoned client.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server.rs, line 3036:
<comment>When another request is failing concurrently, this one-time poison snapshot can be false. `ensure_main_spawned` then chooses in-place reload, which observes the newly poisoned transport, returns an error, and requires a second call to respawn. Serialize the poison decision with the transport request or retry through the respawn path when the reload observes a newly poisoned client.</comment>
<file context>
@@ -3023,7 +3027,13 @@ impl FramaCMcpServer {
+ s.poisoned
+ || s.with_rte != new_rte
+ || s.project_options != new_project_options
+ || client_lock.as_ref().is_some_and(|c| c.is_poisoned())
}
};
</file context>
| fn poisoned_transport() -> FramaCError { | ||
| FramaCError::Io(std::io::Error::new( | ||
| std::io::ErrorKind::BrokenPipe, | ||
| "transport poisoned by an incomplete frame write", |
There was a problem hiding this comment.
P3: When a peer dies during recv_frame, later calls report an incomplete frame write even though no write failed. Use a direction-neutral poison message so read-side failures remain diagnosable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/frama-c/transport.rs, line 125:
<comment>When a peer dies during `recv_frame`, later calls report an incomplete frame write even though no write failed. Use a direction-neutral poison message so read-side failures remain diagnosable.</comment>
<file context>
@@ -56,4 +98,30 @@ impl Transport {
+fn poisoned_transport() -> FramaCError {
+ FramaCError::Io(std::io::Error::new(
+ std::io::ErrorKind::BrokenPipe,
+ "transport poisoned by an incomplete frame write",
+ ))
}
</file context>
| "transport poisoned by an incomplete frame write", | |
| "transport poisoned; the connection must be replaced", |
jserv
left a comment
There was a problem hiding this comment.
Rebase latest main branch and resolve conflicts.
8ddc053 to
c7b1e57
Compare
|
Thank @Suzu1Dev for contributing! |
Depends on #10 (this branch contains its commits until that lands; rebased onto main after #11 merged, and will rebase again once #10 lands).
Problem
#10 made an incomplete frame write poison the transport, so later calls fail
fast instead of corrupting the protocol. But the session cannot recover
cleanly from that state:
ensure_main_spawneddecides in-place vs respawn fromMainFramaCStatealone and never inspects the transport. The first
reload_projectwithexplicit files still attempts an in-place reload on the dead transport,
fails with BrokenPipe, and only then marks the session poisoned — recovery
takes two calls.
reload_projectnever reachesensure_main_spawnedat all: itsfile resolution calls
kernel.ast.getFileson the dead transport first(
src/mcp/project.rs), so the most natural retry fails forever.checkwithfiles=Nonehits the same wall through its internalreload_projectcall.retryable: falseerror that names noremedy, even though a respawn would fix it.
mid-computation is observed by the in-flight READ (EOF), and
recv_framedid not set the poison flag there, so the common crashshape deferred recovery by yet another call.
Reproduction
Regression tests in
tests/test-transport-poison-recovery.rsrun a realFrama-C 33.0, SIGKILL the main instance, and let one
getFilesturn theresulting EPIPE into a poisoned transport (deterministic — no timeouts, no
sleeps). Against the parent commit with only
src/reverted, both testsfail with the bug's exact signature:
a single explicit reload must recover the session: I/O error: transport poisoned by an incomplete frame writeThe read-side half is pinned the same way: with
src/reverted,a_peer_death_mid_read_poisons_every_later_framefails because the EOFarm never set the flag.
On this branch everything passes: one explicit
reload_projectrespawns(the pid changes), a no-arg reload recovers through the cached file list,
and a peer death observed mid-read poisons the transport immediately.
Fix
Transport.poisonedbecomes anArc<AtomicBool>shared with the owningFramaCClient, sois_poisoned()is a lock-free load that never touchesthe request mutex.
ensure_main_spawned's respawn decision gains that flag as its lastdisjunct: a poisoned transport now respawns on the first reload instead
of failing in place to mark the session poisoned.
reload_projectgates onis_poisoned()and resolves filesfrom
MainFramaCState.files— the last successfully loaded list — insteadof asking the dead transport. An empty cache returns an error that names
the remedy (
kind: TransportPoisoned,retryable: true, suggests passingfilesexplicitly). Healthy-transport behavior is unchanged.recv_frame's EOF and read-error arms poison the transport too,mirroring
send_frame: after EOF the peer is gone for good, and a readerror leaves the stream state unknowable. The read-timeout arm
deliberately does NOT poison — the poll loop times out routinely on
healthy servers.
Deliberately not in this PR: sandbox clients still have no respawn path
(delete + recreate is their recovery), and capping the poll loop's POLL/Kill
writes at
min(remaining, WRITE_TIMEOUT)is left for later — it onlyshortens error latency in a session that is already being torn down.
Testing
tests/unit/frama-c-transport.rs(poison mechanics via peer-close:write-side, read-side, and a timeout-must-not-poison guard; all run in
<1 s) and
tests/test-transport-poison-recovery.rs(2 session-levelregression tests, ~6 s)
539 tests pass (534 baseline + 5 new)
cargo clippy --all-targetsclean