test(connectors): pin the failure ladder on both sides and cover the disable path

Review round 5 on #6909.

The nextSyncAt test recomputed its expected interval from the SQL's own binds and
compared against the helper using those same constants, so both sides derived
from one source and the assertion held for any values. It pinned the rendered SQL
text but nothing about SQL-JS equivalence: a consistent refactor of both, or SQL
whose text was right but whose semantics diverged, would have passed. Both sides
now assert concrete values, so neither can move alone.

The hold notice was checked with independent substring matches on distinct
digits, so swapping the withheld count and the cap produced an inverted,
misleading operator message that still passed. Now pinned whole, plus an
assertion that the two orderings differ.

Extracted buildSyncFailureUpdate to mirror the success path, covering the
in-process ladder, a null counter treated as a first failure, the disable firing
exactly at the threshold rather than one early, and the ownership token released
on both outcomes. That is the path the disable ratchet runs through and it was
previously covered only on the reaper's SQL side.

Author: waleedlatif1

apps/sim/app/api/knowledge/connectors/sync/route.test.ts

@@ -17,6 +17,8 @@ import {
 import type { NextRequest } from 'next/server'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import {
+  CONNECTOR_FAILURE_BACKOFF_CAP_MINUTES,
+  CONNECTOR_FAILURE_BACKOFF_STEP_MINUTES,
   connectorFailureBackoffMinutes,
   MAX_CONSECUTIVE_FAILURES,
 } from '@/lib/knowledge/connectors/sync-limits'
@@ -133,12 +135,47 @@ describe('connector sync scheduler stale-lock reaper', () => {
 
     const [threshold, step, cap] = numericBinds(nextSyncAt)
     expect(threshold).toBe(MAX_CONSECUTIVE_FAILURES)
+    expect(step).toBe(CONNECTOR_FAILURE_BACKOFF_STEP_MINUTES)
+    expect(cap).toBe(CONNECTOR_FAILURE_BACKOFF_CAP_MINUTES)
 
-    // Recomputing the ladder from the binds the SQL actually carries makes this
-    // fail the moment the route and `connectorFailureBackoffMinutes` drift apart.
-    for (const failures of [1, 2, 5, 10, 47, 48, 100]) {
-      expect(Math.min(failures * step, cap)).toBe(connectorFailureBackoffMinutes(failures))
-    }
+    /**
+     * Pinned to literals, not recomputed from the binds. Comparing
+     * `Math.min(failures * step, cap)` against `connectorFailureBackoffMinutes`
+     * derived both sides from the same two constants, so it held for any values
+     * AND any shape — swapping the SQL's `*` for `+` left every substring and
+     * every bind untouched. The shape is pinned by the string assertion above;
+     * these pin the magnitudes independently of both the SQL and the helper.
+     */
+    expect(step).toBe(30)
+    expect(cap).toBe(1440)
+  })
+
+  it('applies the same minutes in SQL that the shared helper computes in JS', async () => {
+    /**
+     * The equivalence the ladder test above only appeared to establish. The SQL
+     * encodes `LEAST((failures) * 30, 1440)`; these fix what the JS helper
+     * returns for the same inputs, so the two cannot drift without one of the
+     * two assertions failing.
+     */
+    expect(connectorFailureBackoffMinutes(1)).toBe(30)
+    expect(connectorFailureBackoffMinutes(2)).toBe(60)
+    expect(connectorFailureBackoffMinutes(3)).toBe(90)
+    expect(connectorFailureBackoffMinutes(9)).toBe(270)
+    // 48 * 30 is exactly the cap; either side of it must clamp, not overshoot.
+    expect(connectorFailureBackoffMinutes(47)).toBe(1410)
+    expect(connectorFailureBackoffMinutes(48)).toBe(1440)
+    expect(connectorFailureBackoffMinutes(49)).toBe(1440)
+    expect(connectorFailureBackoffMinutes(100)).toBe(1440)
+  })
+
+  it('releases the reclaimed run ownership token', async () => {
+    await runTickRecovering(['connector-1'])
+
+    /**
+     * Without this the reclaimed run's token still matches its own terminal
+     * write, so it can overwrite the verdict this reclaim just recorded.
+     */
+    expect(setPayloadForUpdate(0).syncLockToken).toBeNull()
   })
 
   it('does not stamp lastSyncAt when reclaiming a stale lock', async () => {

apps/sim/lib/knowledge/connectors/sync-engine.test.ts

@@ -1059,15 +1059,83 @@ describe('countDeletionEligibleOwned', () => {
 })
 
 describe('buildReconciliationHoldNotice', () => {
-  it('names the counts and the full-sync remedy', async () => {
+  it('places each count in its own role', async () => {
     const { buildReconciliationHoldNotice } = await import('@/lib/knowledge/connectors/sync-engine')
 
-    const notice = buildReconciliationHoldNotice(500, 250, 1000)
+    /**
+     * Asserted whole rather than by three independent `toContain` checks on
+     * distinct digit strings: those passed even with the first two arguments
+     * swapped, which inverts the message into "withheld 250 — more than the 500
+     * allowed" and misleads the operator it exists to inform.
+     */
+    expect(buildReconciliationHoldNotice(500, 250, 1000)).toBe(
+      'Withheld 500 document removal(s) — more than the 250 allowed in one sync ' +
+        'of 1000 documents. Documents deleted at the source are still indexed. ' +
+        'Check the source is returning its full contents, then run a full sync to apply the removals.'
+    )
+  })
+
+  it('cannot be satisfied by swapping the withheld and cap counts', async () => {
+    const { buildReconciliationHoldNotice } = await import('@/lib/knowledge/connectors/sync-engine')
+
+    expect(buildReconciliationHoldNotice(500, 250, 1000)).not.toBe(
+      buildReconciliationHoldNotice(250, 500, 1000)
+    )
+  })
+})
+
+describe('buildSyncFailureUpdate', () => {
+  const now = new Date('2026-08-20T00:00:00.000Z')
+  const minutesAfter = (mins: number) => new Date(now.getTime() + mins * 60 * 1000)
+
+  it('backs off on the shared ladder below the threshold', async () => {
+    const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine')
+
+    const first = buildSyncFailureUpdate(now, 0, 'boom')
+    expect(first.status).toBe('error')
+    expect(first.consecutiveFailures).toBe(1)
+    expect(first.lastSyncError).toBe('boom')
+    expect(first.nextSyncAt).toEqual(minutesAfter(30))
+
+    const third = buildSyncFailureUpdate(now, 2, 'boom')
+    expect(third.consecutiveFailures).toBe(3)
+    expect(third.nextSyncAt).toEqual(minutesAfter(90))
+  })
+
+  it('treats a null counter as a first failure', async () => {
+    const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine')
+
+    expect(buildSyncFailureUpdate(now, null, 'boom').consecutiveFailures).toBe(1)
+    expect(buildSyncFailureUpdate(now, undefined, 'boom').nextSyncAt).toEqual(minutesAfter(30))
+  })
+
+  it('disables exactly at the threshold, not before it', async () => {
+    const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine')
+    const { MAX_CONSECUTIVE_FAILURES } = await import('@/lib/knowledge/connectors/sync-limits')
+
+    /**
+     * The path the auto-disable breaker actually runs through in-process. Only
+     * the reaper's SQL equivalent was covered before, so an off-by-one here —
+     * disabling a connector one failure early — was invisible.
+     */
+    const below = buildSyncFailureUpdate(now, MAX_CONSECUTIVE_FAILURES - 2, 'boom')
+    expect(below.status).toBe('error')
+    expect(below.consecutiveFailures).toBe(MAX_CONSECUTIVE_FAILURES - 1)
+    expect(below.nextSyncAt).not.toBeNull()
+
+    const at = buildSyncFailureUpdate(now, MAX_CONSECUTIVE_FAILURES - 1, 'boom')
+    expect(at.status).toBe('disabled')
+    expect(at.consecutiveFailures).toBe(MAX_CONSECUTIVE_FAILURES)
+    expect(at.nextSyncAt).toBeNull()
+    expect(at.lastSyncError).toContain('reconnect')
+  })
+
+  it('releases the ownership token on both outcomes', async () => {
+    const { buildSyncFailureUpdate } = await import('@/lib/knowledge/connectors/sync-engine')
+    const { MAX_CONSECUTIVE_FAILURES } = await import('@/lib/knowledge/connectors/sync-limits')
 
-    expect(notice).toContain('500')
-    expect(notice).toContain('250')
-    expect(notice).toContain('1000')
-    expect(notice).toContain('full sync')
+    expect(buildSyncFailureUpdate(now, 0, 'boom').syncLockToken).toBeNull()
+    expect(buildSyncFailureUpdate(now, MAX_CONSECUTIVE_FAILURES, 'boom').syncLockToken).toBeNull()
   })
 })
 

apps/sim/lib/knowledge/connectors/sync-engine.ts

@@ -659,6 +659,39 @@ export function buildReconciliationHoldNotice(
   )
 }
 
+/**
+ * The connector row a failed sync writes.
+ *
+ * Extracted for the same reason as {@link buildSyncSuccessUpdate}: this is the
+ * path the auto-disable breaker runs through, so the threshold and the backoff
+ * it applies need to be assertable without standing up the whole sync. The
+ * in-process ladder here and the reaper's SQL ladder must agree — they are two
+ * writers of one policy, both sourced from
+ * {@link connectorFailureBackoffMinutes}.
+ */
+export function buildSyncFailureUpdate(
+  now: Date,
+  previousFailures: number | null | undefined,
+  errorMessage: string
+) {
+  const failures = (previousFailures ?? 0) + 1
+  const disabled = failures >= MAX_CONSECUTIVE_FAILURES
+
+  return {
+    status: (disabled ? 'disabled' : 'error') as 'disabled' | 'error',
+    lastSyncError: disabled
+      ? 'Connector disabled after repeated sync failures. Please reconnect.'
+      : errorMessage,
+    nextSyncAt: disabled
+      ? null
+      : new Date(now.getTime() + connectorFailureBackoffMinutes(failures) * 60 * 1000),
+    consecutiveFailures: failures,
+    // Releases the lock so a stale token can never match a later run.
+    syncLockToken: null,
+    updatedAt: now,
+  }
+}
+
 /**
  * The connector row a successful sync writes.
  *
@@ -2055,29 +2088,24 @@ export async function executeSync(
     try {
       await completeSyncLog(syncLogId, 'failed', result, errorMessage)
 
-      const now = new Date()
-      const failures = (connector.consecutiveFailures ?? 0) + 1
-      const disabled = failures >= MAX_CONSECUTIVE_FAILURES
-      const backoffMinutes = connectorFailureBackoffMinutes(failures)
-      const nextSync = disabled ? null : new Date(now.getTime() + backoffMinutes * 60 * 1000)
+      const failureUpdate = buildSyncFailureUpdate(
+        new Date(),
+        connector.consecutiveFailures,
+        errorMessage
+      )
 
-      if (disabled) {
+      if (failureUpdate.status === 'disabled') {
         logger.warn('Connector disabled after repeated failures', {
           connectorId,
-          consecutiveFailures: failures,
+          consecutiveFailures: failureUpdate.consecutiveFailures,
         })
       }
 
-      const failureWriteLanded = await writeTerminalConnectorState(connectorId, syncLogId, {
-        status: disabled ? 'disabled' : 'error',
-        lastSyncError: disabled
-          ? 'Connector disabled after repeated sync failures. Please reconnect.'
-          : errorMessage,
-        nextSyncAt: nextSync,
-        consecutiveFailures: failures,
-        syncLockToken: null,
-        updatedAt: now,
-      })
+      const failureWriteLanded = await writeTerminalConnectorState(
+        connectorId,
+        syncLogId,
+        failureUpdate
+      )
 
       /**
        * Deliberately does NOT get {@link applySupersededOutcome}. `result.error`