fix(ws): pin the withheld ACK with a test, and correct the changeset

Two blockers from the pre-submission gate, both reproduced before fixing.

1. The changeset claimed the non-mux POST branch answers OPERATION_FAILED. The
   code says the opposite in as many words ("NOT an error response,
   deliberately"), the commit message says response codes are unchanged, and the
   test asserts the 200. It was a leftover sentence from an earlier iteration that
   would have shipped into the CHANGELOG announcing an API contract change that
   does not exist — and errorCode values are SemVer-relevant per
   docs/versioning-policy.md.

2. The WebSocket half of the fix had no test protection: reverting ws-routes.ts to
   master left all 9 tests green, while the commit message sells "plus the whole
   WebSocket path" as part of the fix. Three tests added against the real WS
   route — ACK on delivery, ACK withheld and seq re-opened when the write did not
   land, and a deduplicated frame still ACKed so the client can drop it. Verified
   the other way round: with ws-routes.ts reverted, the middle one fails.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claudia
2026-08-07 14:52:44 +02:00
parent ebfcac6ad1
commit 84132d3025
2 changed files with 54 additions and 2 deletions
+5 -2
View File
@@ -14,8 +14,11 @@ been delivered.
The bookkeeping is now rolled back on failure and the WebSocket ACK withheld, so
the client redelivers. `Session.write()` reports whether it reached a PTY at all
instead of silently swallowing the data, and the non-mux POST branch — whose
response has not gone out yet — answers `OPERATION_FAILED` rather than a cheerful 200.
instead of silently swallowing the data.
Response codes are unchanged: a session can legitimately have no PTY yet (created
but not started), so turning that into a failure status would be a contract change
of its own.
Note this does not remove the root cause: the POST still answers 200 before the
mux write is attempted, so a client that treats any 2xx as final still cannot
+49
View File
@@ -208,6 +208,55 @@ describe('ws-routes', () => {
}
});
it('ACKs a delivered input and burns its seq', async () => {
const ws = await connectWs('/ws/sessions/ws-test-session/terminal');
try {
const session = ctx._session;
ws.send(JSON.stringify({ t: 'i', d: 'ok\r', cid: 'c1', seq: 1 }));
expect(await nextMessage(ws)).toEqual({ t: 'ia', seq: 1 });
expect(session.shouldApplyInput('c1', 1)).toBe(false);
} finally {
ws.close();
}
});
it('withholds the ACK and re-opens the seq when the write did not land', async () => {
// A session whose PTY is gone swallows the write. ACKing anyway told the
// client to drop the frame from its durable queue while the seq stayed
// burnt, so the retry that reliable delivery exists for was rejected as a
// duplicate — the input was lost for good.
const ws = await connectWs('/ws/sessions/ws-test-session/terminal');
try {
const session = ctx._session;
session.failWrites = true;
ws.send(JSON.stringify({ t: 'i', d: 'lost\r', cid: 'c1', seq: 1 }));
await expect(nextMessage(ws, 600)).rejects.toThrow(/timeout/);
expect(session.shouldApplyInput('c1', 1)).toBe(true);
} finally {
ws.close();
}
});
it('still ACKs a duplicate frame the server deliberately skipped', async () => {
// Dedup must stay silent-but-acknowledged: the client has to be able to
// drop a frame it already delivered once.
const ws = await connectWs('/ws/sessions/ws-test-session/terminal');
try {
const session = ctx._session;
session.shouldApplyInput('c1', 7); // pretend seq 7 already landed
ws.send(JSON.stringify({ t: 'i', d: 'again\r', cid: 'c1', seq: 7 }));
expect(await nextMessage(ws)).toEqual({ t: 'ia', seq: 7 });
expect(session.writeBuffer).not.toContain('again\r');
} finally {
ws.close();
}
});
it('ignores input exceeding MAX_INPUT_LENGTH', async () => {
const ws = await connectWs('/ws/sessions/ws-test-session/terminal');
try {