fix(terminal): merge-time fixes for dropped-output recovery (#470)

- The TERMINAL DROP crash-trail line moves behind the scheduler's debounce
  guard, so it is written once per window rather than once per dropped
  frame. At the server's 8ms batching, one second of drops evicted the whole
  50-entry trail, including the recovery lines that explain it.
- A refresh that failed at the capture fetch deadline now returns
  'deadline', and the scheduler does not retry it: that is a stalled link,
  not contention, and each retry was another ?full=1 capture waiting out a
  deadline of up to two minutes. The early-return retries are unchanged.
  CLAUDE.md and the code comments no longer claim every skip reason is
  transient contention.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
2026-09-23 11:40:03 +02:00
parent 7a30a31430
commit f4d1ee8027
4 changed files with 77 additions and 20 deletions
+23 -10
View File
@@ -2068,9 +2068,11 @@ class CodemanApp {
+ (this._terminalWriteInFlightBytes || 0);
if (queued + data.data.length > 131072) { // 128KB — drop to prevent accumulation
// The bytes are gone from the stream now, so the recovery is the only
// thing that puts this terminal back in step with the PTY.
_crashDiag.log(`TERMINAL DROP: ${(queued / 1024).toFixed(0)}KB queued`);
this._scheduleDroppedOutputRecovery(data.id);
// thing that puts this terminal back in step with the PTY. It also
// writes the crash-trail line, once per debounce window: logged here,
// one line per dropped frame evicted the whole 50-entry trail in
// under a second.
this._scheduleDroppedOutputRecovery(data.id, 0, queued);
return;
}
@@ -2091,25 +2093,34 @@ class CodemanApp {
*
* Debounced by the same 2s as before, so a sustained burst still collapses
* into one attempt rather than hammering the API; bounded by
* `DROP_RECOVERY_MAX_ATTEMPTS`, because every reason the refresh can be
* skipped is transient contention. Giving up after the cap leaves exactly
* what the old code left, so the floor is no worse.
* `DROP_RECOVERY_MAX_ATTEMPTS`, because the early returns it retries past
* are transient contention. A refresh that died at the fetch DEADLINE is
* not retried: that is a stalled link, not contention, and each retry would
* be another `?full=1` capture waiting out a deadline of up to two minutes.
* Giving up leaves exactly what the old code left, so the floor is no worse.
*
* @param {string} sessionId - the session whose output was dropped
* @param {number} [attempt] - zero-based, for the bound
* @param {number} [queuedBytes] - render-queue bytes at the drop, for the crash trail
*/
_scheduleDroppedOutputRecovery(sessionId, attempt = 0) {
_scheduleDroppedOutputRecovery(sessionId, attempt = 0, queuedBytes) {
if (!sessionId || this._clientDropRecoveryTimer) return;
// Behind the debounce guard: one line per window, not per dropped frame.
if (Number.isFinite(queuedBytes)) _crashDiag.log(`TERMINAL DROP: ${(queuedBytes / 1024).toFixed(0)}KB queued`);
this._clientDropRecoveryTimer = setTimeout(async () => {
this._clientDropRecoveryTimer = null;
let repainted = false;
let timedOut = false;
try {
repainted = (await this._onSessionNeedsRefresh({ id: sessionId })) === true;
const result = await this._onSessionNeedsRefresh({ id: sessionId });
repainted = result === true;
timedOut = result === 'deadline';
} catch {
// Treated as "did not repaint" — retrying is the entire point of this.
}
const retry = window.CodemanDroppedOutput.shouldRetryDroppedOutputRecovery({
repainted,
timedOut,
attempt,
stillActive: this.activeSessionId === sessionId,
});
@@ -2757,7 +2768,9 @@ class CodemanApp {
* "recovered" silently loses the recovery; `_scheduleDroppedOutputRecovery`
* is the one that cannot afford to.
*
* @returns {Promise<boolean>} true only once a response has been applied.
* @returns {Promise<boolean|'deadline'>} true only once a response has been
* applied; 'deadline' when the capture fetch hit its deadline (a stalled
* link, which the dropped-output scheduler does not retry); false otherwise.
*/
async _onSessionNeedsRefresh(event = {}) {
// Server sends this after SSE backpressure clears — terminal data was dropped,
@@ -2846,7 +2859,7 @@ class CodemanApp {
return true;
} catch (err) {
console.error('needsRefresh reload failed:', err);
return false;
return err?.name === 'AbortError' ? 'deadline' : false;
} finally {
if (this._terminalRefreshOwner === refreshOwner) this._terminalRefreshOwner = null;
}
+11 -6
View File
@@ -1713,27 +1713,32 @@ function sanitizeDiagEntry(msg) {
// so a skipped refresh lost the recovery silently and the dropped bytes were
// never replayed.
//
// Bounded, because every reason the refresh can be skipped is transient
// contention that clears in seconds, and a permanently failing refresh must not
// become a forever-loop against the API. Giving up after the cap leaves exactly
// the garbled frames the old code left, so the floor is no worse than before.
// Bounded, because the early returns it retries past are transient contention
// that clears in seconds, and a permanently failing refresh must not become a
// forever-loop against the API. A refresh that hit the capture fetch DEADLINE
// is not contention but a stalled link, and is not retried at all: each retry
// would be another `?full=1` capture waiting out a deadline of up to two
// minutes, where the old code cost exactly one. Giving up after the cap leaves
// exactly the garbled frames the old code left, so the floor is no worse.
const DROP_RECOVERY_DELAY_MS = 2000;
const DROP_RECOVERY_MAX_ATTEMPTS = 5;
/**
* Should a dropped-output recovery run again?
*
* @param {{repainted: boolean, attempt: number, stillActive: boolean}} state
* @param {{repainted: boolean, timedOut?: boolean, attempt: number, stillActive: boolean}} state
* `repainted` — whether `_onSessionNeedsRefresh` actually rewrote the buffer.
* `timedOut` - whether it failed at the capture fetch deadline.
* `attempt` — how many have already run, zero-based.
* `stillActive` — whether the dropped session is still the one on screen.
* @returns {boolean}
*/
function shouldRetryDroppedOutputRecovery({ repainted, attempt, stillActive }) {
function shouldRetryDroppedOutputRecovery({ repainted, timedOut = false, attempt, stillActive }) {
// Switched away: `selectSession` repaints from the server on its own, so a
// retry here would be a second replay of a buffer that is about to be written.
if (!stillActive) return false;
if (repainted) return false;
if (timedOut) return false;
return attempt + 1 < DROP_RECOVERY_MAX_ATTEMPTS;
}