mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-03 05:59:43 +02:00
fix(api,ws): an input whose delivery fails can be retried instead of being lost
Both input paths recorded the (clientId, seq) pair as applied and acknowledged the frame BEFORE knowing whether the write had landed: the POST route because its mux write is fire-and-forget so the response never waits on a tmux child, the WebSocket handler because it ACKed unconditionally. When the write then failed, the client dropped the frame from its durable queue and the server rejected the retry as a duplicate. The reliable-delivery layer was guaranteeing exactly-once delivery of something that had never been delivered — and `Session.write()` returned void, so a session whose PTY was gone swallowed the data with no signal at all. - `forgetInputSeq()` rolls the bookkeeping back on failure, but only when that seq is still the newest one; a later input has superseded it and must not re-open. - The WebSocket handler withholds its ACK when the write did not land, so the client redelivers. - `Session.write()` reports whether it reached a PTY. Response codes are unchanged, deliberately: a session can legitimately have no PTY yet, and turning that into a failure status would be a contract change of its own. What this does NOT do: remove the root cause. The POST still answers 200 before the mux write is attempted, so a client that treats any 2xx as final cannot learn about that failure. What closes is the narrower window — the write failed AND the ACK never reached the client — plus the whole WebSocket path. Closing the rest would mean awaiting the tmux child inside the request. 9 tests. They drive the HTTP route, not only the Session primitives: with the rollback removed from the route, 2 of them fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -879,28 +879,41 @@ export function registerSessionRoutes(
|
||||
// Reliable delivery (POST fallback when the WebSocket is down): a 2xx IS the
|
||||
// client's ACK, so a tagged duplicate redelivery must still return 200 but
|
||||
// skip the write. Untagged requests (curl/legacy) always apply.
|
||||
if (typeof clientId === 'string' && typeof seq === 'number' && !session.shouldApplyInput(clientId, seq)) {
|
||||
const tagged = typeof clientId === 'string' && typeof seq === 'number';
|
||||
if (tagged && !session.shouldApplyInput(clientId as string, seq as number)) {
|
||||
return {};
|
||||
}
|
||||
|
||||
// Write input to PTY. Direct write is synchronous; writeViaMux
|
||||
// (tmux send-keys) is fire-and-forget to avoid blocking the HTTP response.
|
||||
if (useMux) {
|
||||
// Fire-and-forget: don't block HTTP response on tmux child process.
|
||||
// Fallback to direct write on failure.
|
||||
// Fire-and-forget: don't block the HTTP response on a tmux child process.
|
||||
// Fallback to a direct write on failure.
|
||||
//
|
||||
// Because the response has already been sent by then, a failure here is the
|
||||
// one case the caller can never learn about — so the dedup bookkeeping is
|
||||
// rolled back. Otherwise the seq stays recorded as applied and a retry, the
|
||||
// very mechanism reliable delivery exists for, is rejected as a duplicate.
|
||||
const undoOnFailure = () => {
|
||||
if (tagged) session.forgetInputSeq(clientId as string, seq as number);
|
||||
};
|
||||
session
|
||||
.writeViaMux(inputStr)
|
||||
.then((ok) => {
|
||||
if (!ok) {
|
||||
console.warn(`[Server] writeViaMux failed for session ${id}, falling back to direct write`);
|
||||
session.write(inputStr);
|
||||
}
|
||||
if (ok) return;
|
||||
console.warn(`[Server] writeViaMux failed for session ${id}, falling back to direct write`);
|
||||
if (!session.write(inputStr)) undoOnFailure();
|
||||
})
|
||||
.catch(() => {
|
||||
session.write(inputStr);
|
||||
if (!session.write(inputStr)) undoOnFailure();
|
||||
});
|
||||
} else {
|
||||
session.write(inputStr);
|
||||
// Same rollback. NOT an error response, deliberately: a session can
|
||||
// legitimately have no PTY yet (created but not started), and callers have
|
||||
// always been able to write to one without a 4xx.
|
||||
if (!session.write(inputStr) && tagged) {
|
||||
session.forgetInputSeq(clientId as string, seq as number);
|
||||
}
|
||||
}
|
||||
return {};
|
||||
});
|
||||
|
||||
@@ -180,13 +180,19 @@ export function registerWsRoutes(app: FastifyInstance, ctx: SessionPort, getHost
|
||||
const cid = typeof msg.cid === 'string' ? msg.cid : null;
|
||||
const seq = Number.isInteger(msg.seq) ? (msg.seq as number) : null;
|
||||
const apply = cid && seq !== null ? session.shouldApplyInput(cid, seq) : true;
|
||||
let delivered = true;
|
||||
if (apply) {
|
||||
// Typed input from a claim-holding desktop keeps the claim "hot"
|
||||
// and re-asserts the desktop layout after a mobile override.
|
||||
if (holdsDesktopClaim) session.noteDesktopActivity();
|
||||
session.write(msg.d);
|
||||
delivered = session.write(msg.d);
|
||||
// A session whose PTY is gone swallows the write. ACKing anyway told
|
||||
// the client to drop the frame from its durable queue and left the seq
|
||||
// burnt, so the retry that reliable delivery exists for was rejected as
|
||||
// a duplicate: the input was lost for good.
|
||||
if (!delivered && cid && seq !== null) session.forgetInputSeq(cid, seq);
|
||||
}
|
||||
if (seq !== null && socket.readyState === 1) {
|
||||
if (delivered && seq !== null && socket.readyState === 1) {
|
||||
socket.send(`{"t":"ia","seq":${seq}}`);
|
||||
}
|
||||
} else if (
|
||||
|
||||
Reference in New Issue
Block a user