Skip to content
This repository was archived by the owner on Jun 8, 2026. It is now read-only.

Commit 8212516

Browse files
Andrew EyeAndrew Eye
authored andcommitted
FEA-1461: PR #258 review fixes (P1 relay flap + backfill starvation + test helper dedup)
Codex P1 (relay-flap dead-letter): - cloud-socket.sendAgentSessions returns RateLimited for BOTH server-side payload throttling AND local transport unavailability (!isRelayReady() or socket disconnected after the batch was prepared). The previous code treated both as session-payload problems and would dead-letter perfectly good sessions after 5 disconnects. handleBatchAck now re-checks isRelayReady() at ack time; when the relay is down, the rate-limit counter is NOT advanced. The session still gets a backoff (so we don't hot-loop), but its dead-letter trajectory restarts the moment the relay recovers. Log line distinguishes "server payload throttle" vs "transport unavailable" so operators can tell them apart. thadeusb (backfill starvation): - Restructure syncOnce candidate selection: if the incremental branch picks zero ready candidates (every session in rate-limit backoff), fall through to backfill instead of hitting the early-return. Before this fix the incremental queue being non-empty-but-all-backed-off blocked backfill for the entire backoff window. Hidden today because RATE_LIMIT_BACKOFF_MS == MIN_INCREMENTAL_SYNC_INTERVAL_MS (both 30s), but a future backoff tune past 30s would expose it. thadeusb (test helper duplication): - Extract createServiceTestDatabase + flushAgentSessionSync + insertSessionRow into new test/helpers/agent-session-sync-test-utils.ts so future agent-monitor schema migrations force both test suites to update in lockstep. CLAUDE.md flags this exact `tests|duplication` learned-mistake. New tests: - "rate_limited that races with a relay drop does NOT advance the dead-letter counter" — simulates the race by having sendBatch flip relayReadyValue=false right before returning RateLimited. Drives 2x the threshold under flap conditions and asserts the counter stays at 0 (proved indirectly: switching back to healthy-relay rate_limiteds then takes exactly MAX_CONSECUTIVE_RATE_LIMITED attempts to dead-letter, which only holds if the flap counter wasn't advanced). - "backfill is not starved when every incremental candidate is in rate-limit backoff" — sessions in both queues, incremental in backoff, asserts backfill still flows. Bump desktop version 0.15.99 → 0.15.101 (0.15.100 reserved for FEA-1444). Testing: - pnpm -C apps/desktop typecheck: clean - pnpm -C apps/desktop lint: clean - New rate-limit tests: 6/6 pass (was 4 before this commit) - Full suite: 1833 + 104 = 1937/1937 pass, 0 fail - Independent code review on the original FEA-1461 commit caught a Medium + Low; thadeusb's peer review on the PR caught this round of three more findings (P1 relay-flap + Medium backfill starvation + Low test dedup). All addressed in this commit. Risks: - The flap detection is heuristic (re-reads isRelayReady at ack time). A relay that goes down between sendBatch returning and handleBatchAck running's isRelayReady check would not be detected — but that window is microseconds, and the worst case (counter advances by 1) is far better than the prior worst case (dead-letter after 5 flaps). - The fallthrough to backfill in syncOnce changes priority slightly: when incremental is empty-but-blocked, backfill now runs on the same tick instead of being deferred. Increases backfill throughput by up to one tick per incremental-backoff window. Net positive for cloud sync.
1 parent e8a13cd commit 8212516

5 files changed

Lines changed: 402 additions & 220 deletions

File tree

apps/desktop/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "desktop",
3-
"version": "0.15.99",
3+
"version": "0.15.101",
44
"description": "ClosedLoop Desktop",
55
"author": "ClosedLoop AI <support@closedloop.ai>",
66
"private": true,

apps/desktop/src/main/agent-session-sync-service.ts

Lines changed: 36 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -296,35 +296,41 @@ export class AgentSessionSyncService {
296296

297297
const nowMs = Date.now();
298298
let candidateIds: string[] = [];
299+
// FEA-1461: try incremental first. `pickReadyCandidates` may filter
300+
// out every queued session (all in rate-limit backoff). When that
301+
// happens, fall through to backfill so it does not get starved
302+
// for the whole backoff window — previously the early-return
303+
// below would skip backfill entirely on every tick.
299304
if (
300305
this.incrementalQueue.length > 0 &&
301306
nowMs - this.lastIncrementalBatchAttemptedAtMs >=
302307
MIN_INCREMENTAL_SYNC_INTERVAL_MS
303308
) {
304-
syncMode = "incremental";
305-
// FEA-1461: iterate rather than slice so a session under
306-
// rate-limit backoff at the head of the queue does not
307-
// head-of-line-block siblings behind it.
308309
candidateIds = this.pickReadyCandidates(
309310
this.incrementalQueue,
310311
INCREMENTAL_SESSION_BATCH_SIZE,
311312
nowMs,
312313
);
313-
// FEA-1461: only stamp the throttle timestamp if we actually
314-
// selected at least one ready candidate. Stamping when every
315-
// candidate was filtered by backoff would unnecessarily delay
316-
// a session added to the queue moments later by the full
317-
// MIN_INCREMENTAL_SYNC_INTERVAL_MS window.
318314
if (candidateIds.length > 0) {
315+
syncMode = "incremental";
316+
// Only stamp the throttle timestamp if we actually selected
317+
// at least one ready candidate. Stamping when every candidate
318+
// was filtered by backoff would unnecessarily delay a session
319+
// added to the queue moments later by the full
320+
// MIN_INCREMENTAL_SYNC_INTERVAL_MS window.
319321
this.lastIncrementalBatchAttemptedAtMs = nowMs;
320322
}
321-
} else if (this.backfillQueue.length > 0) {
322-
syncMode = "backfill";
323-
candidateIds = this.pickReadyCandidates(
323+
}
324+
if (candidateIds.length === 0 && this.backfillQueue.length > 0) {
325+
const backfillCandidates = this.pickReadyCandidates(
324326
this.backfillQueue,
325327
BACKFILL_SESSION_BATCH_SIZE,
326328
nowMs,
327329
);
330+
if (backfillCandidates.length > 0) {
331+
syncMode = "backfill";
332+
candidateIds = backfillCandidates;
333+
}
328334
}
329335

330336
if (!syncMode || candidateIds.length === 0) {
@@ -644,12 +650,23 @@ export class AgentSessionSyncService {
644650
// log only, no counter, no dead-letter, no dequeue, no backoff. For an
645651
// oversized session that's permanently throttled, that produced an
646652
// infinite retry loop (re-chunking + log spam every 5s).
653+
//
654+
// FEA-1461 review fix (PR #258, Codex P1): `cloud-socket.sendAgentSessions`
655+
// returns `RateLimited` for BOTH server-side payload throttling AND
656+
// local transport unavailability (`!isRelayReady()` or socket
657+
// disconnected after the batch was prepared). Treating a relay flap
658+
// as a session-payload problem would dead-letter perfectly good
659+
// sessions after 5 disconnects. Re-check relay readiness here: if the
660+
// relay is down right now, the ack came from the transport layer —
661+
// defer with backoff but do NOT increment the dead-letter counter.
662+
const relayHealthy = this.options.isRelayReady();
647663
const deadLettered: string[] = [];
648664
const deferred: string[] = [];
649665
const retryDeadline = Date.now() + RATE_LIMIT_BACKOFF_MS;
650666
for (const id of ids) {
651-
const count = (this.rateLimitedCountById.get(id) ?? 0) + 1;
652-
if (count >= MAX_CONSECUTIVE_RATE_LIMITED) {
667+
const previousCount = this.rateLimitedCountById.get(id) ?? 0;
668+
const count = relayHealthy ? previousCount + 1 : previousCount;
669+
if (relayHealthy && count >= MAX_CONSECUTIVE_RATE_LIMITED) {
653670
deadLettered.push(id);
654671
this.rateLimitedCountById.delete(id);
655672
this.nextRetryAfterMs.delete(id);
@@ -658,7 +675,9 @@ export class AgentSessionSyncService {
658675
this.timeoutCountById.delete(id);
659676
this.deadLetteredIds.add(id);
660677
} else {
661-
this.rateLimitedCountById.set(id, count);
678+
if (relayHealthy) {
679+
this.rateLimitedCountById.set(id, count);
680+
}
662681
this.nextRetryAfterMs.set(id, retryDeadline);
663682
deferred.push(id);
664683
}
@@ -677,7 +696,8 @@ export class AgentSessionSyncService {
677696
const attempt = this.rateLimitedCountById.get(sampleId) ?? 0;
678697
gatewayLog.info(
679698
TAG,
680-
`agent-session batch (${syncMode}, ~${formatBytes(payloadBytes)}) rate_limited by server; ` +
699+
`agent-session batch (${syncMode}, ~${formatBytes(payloadBytes)}) rate_limited ` +
700+
`(${relayHealthy ? "server payload throttle" : "transport unavailable"}); ` +
681701
`deferring ${deferred.length} session(s) for ${Math.round(RATE_LIMIT_BACKOFF_MS / 1000)}s ` +
682702
`(attempt ${attempt}/${MAX_CONSECUTIVE_RATE_LIMITED}); batch left queued for retry`,
683703
);

0 commit comments

Comments
 (0)