fix(connectors): count hard-kill failures and cap deletion blast radius by waleedlatif1 · Pull Request #6909 · simstudioai/sim

waleedlatif1 · 2026-08-20T23:59:52Z

Two coupled fixes from a pipeline audit. Both address failures that are currently invisible: one makes a crashing connector self-report, the other stops a healthy-looking listing from destroying a corpus.

Why a crashing connector never backs off

An OOM is a SIGKILL, so executeSync's catch and finally never run. The out-of-process stale-lock reaper is the only survivor — and it cleared the lock without ever incrementing consecutiveFailures. MAX_CONSECUTIVE_FAILURES was therefore structurally unreachable for hard kills, the exact failure mode that most needs it, and the reaper's flat 10-minute retry is shorter than any healthy sync interval. A connector that dies hard is retried more aggressively than a working one, forever, without ever recording a failure.

The observed ~2-hour crash cadence in production is the stale-lock TTL, not any connector's schedule.

The failure threshold and backoff ladder move to sync-limits.ts. Two writers of one policy with no shared constant is exactly how the drift arose.
The reaper now increments, backs off, and disables in the same statement that clears the lock. A read-then-write loses increments when two cron ticks overlap. Shape follows the existing schedule-execution.ts precedent.
Sync-log rows left started by a killed run are now swept. Nothing else ever reconciles them, and loadPreviousListingObservation reads only completed rows — so a never-closed run silently ages out the observation it should have provided.
The sweep keys off each row's own startedAt, not this tick's reclaims. A row orphaned before this ships belongs to a connector already flipped out of syncing and would never appear in a future reclaim batch. Scoped to the tick, the fix could not heal the backlog that already exists.
The finally block is deleted — every exit path set its flag, and a SIGKILL skips finally regardless, so it read as a crash safety net that could never fire.
A lock-contended run now reports as skipped rather than as a successful zero-document sync.
lastSyncAt is no longer stamped on the failure path; it should mean last successful sync, and lastSyncError already carries the other meaning.

Why a partial outage deletes half a corpus

Every existing guard asks whether a listing looks broken. The collapse heuristic only fires below ~10% of the corpus, which leaves the entire 10–100% band unguarded. A source returning half its documents — one shard down, one folder's permissions changed — produces a listing that looks perfectly healthy: sync one tombstones the missing half, sync two hard-deletes it with embeddings and tags. Two intervals, no guard involvement.

The same is true of any change to a connector's externalId derivation: a complete, correct listing of entirely new keys, under which every stored document is "absent."

A pass whose deletions exceed a share of the corpus is held, all-or-nothing. Deleting up to a cap still destroys data and leaves a state no operator asked for. fullSync remains the documented escape hatch.
A userExcluded document is never deletion-eligible. Guarded at deletion eligibility rather than at the select: filtering it out of the tombstoned read would also withhold resurrection, and since the connector listing and the restore mutation both require deletedAt IS NULL, that would strand the row permanently — invisible, unrestorable, and undeletable.
Listed documents are counted over the same population as the owned count. seenExternalIds is populated before the excluded short-circuit, so comparing it against a denominator that excludes those rows inflated the ratio and weakened the collapse guard.
resolvePreviousOwnedCount judges the previous run against a corpus at least as large as the one present. lastSyncDocCount counts only visible documents, so after a tombstoning pass it collapsed toward zero and corroboration became impossible.

Ordering note for reviewers: that last item un-jams the two-strike purge, which is currently stuck shut by accident. It ships in the same commit as the cap deliberately — landing it alone would re-open the destruction path the cap exists to close.

Verification

type-check clean · check:audits 32/32 · 1003 connector tests · 8 route tests (the scheduler route had no test file before this).

72 mutations applied, all red on their intended test, none inert. Including: re-coupling the sync-log sweep to the tick's reclaims, reinstating the flat 10-minute retry, reverting the numerator asymmetry, restoring the select-level exclusion filter, and swapping the backoff SQL's multiplication for addition, which would retry every connector at a flat 31 minutes forever.

One existing test asserted the pre-fix behavior (touches no sync-log rows when nothing was reclaimed) and was replaced with its inverse rather than left in place.

Known and deliberate

Root cause behind three of these findings: the in-process fallback path is unbounded. When Trigger.dev is unavailable, dispatchSync runs executeSync fire-and-forget in the web process with no maxDuration, so such a run can outlive the 2h stale-lock TTL and still hold write authority after the reaper has reclaimed the connector and dispatched a replacement. The guards added here — status = 'started' on completeSyncLog, status = 'syncing' on both terminal connector writes — make the reclaim authoritative so a superseded run cannot corrupt state. They do not stop two runs reconciling the same corpus concurrently. Bounding or heartbeating the fallback path is the real fix and is deliberately out of scope here.

A connector paused mid-sync is no longer flipped back to active by the completing run. That falls out of the same status = 'syncing' guard and is intended: the pause write does not check current status, so today a completing run silently un-pauses it.

A running sync heartbeats its lock, so the TTL means "nobody is working on this" rather than "this started a long time ago". The reclaim increments consecutiveFailures, and a reclaimed run's terminal write — including its consecutiveFailures reset — is rejected. For the in-process fallback path, which has no duration cap, that made the reaper a one-way ratchet: a large self-hosted sync that legitimately outran the 2h TTL was counted as a failure it could never clear, and ten of them disabled a connector whose every sync had actually succeeded. A live run now refreshes updatedAt every 5 minutes so the reclaim predicate never matches it. The beat is guarded on the run's own sync_lock_token, so it doubles as an ownership probe: a run that has lost its lock aborts instead of doing hours more work and writing documents alongside its replacement. A run that stops beating is genuinely dead or wedged, and reclaiming it stays correct.

The deletion cap gates the two generations separately. Summing soft and hard deletes against one cap double-counted: hard deletes are the previous generation's soft deletes, already gated by the same cap once. On a connector with steady churn that ratcheted shut — 1,000 documents at 15% churn against a cap of 250 applied 150 soft on sync 1, then requested 150 soft + 150 hard on sync 2, exceeded the cap, and the all-or-nothing hold blocked the very hard deletes that would have drained the backlog, which then grew forever. Each generation is now capped against the same ceiling, which keeps the per-sync blast radius bounded without the deadlock. The outage and externalId-change shapes are unaffected: both are a single oversized soft generation.

Terminal writes are guarded on an ownership token, not just on status. status = 'syncing' asserts that a run holds the lock, not that this run does. Once the reaper reclaims a stale lock and the scheduler dispatches a replacement, the replacement sets syncing again — so the original run matched, overwrote the replacement's in-flight state and the reclaim's bookkeeping, and then had the replacement's own write rejected as superseded. The dead run won and the live one lost, which is worse than the unguarded last-write-wins it replaced. knowledge_connector.sync_lock_token is written in the same CAS that takes the lock (and reused as the sync-log row id, so the connector points at the run holding it), matched by both terminal writes, and cleared on release. The migration is additive, nullable, and unbackfilled: existing rows read NULL, which no in-flight run can match, so a sync spanning the deploy simply has its terminal write skipped and is re-run by the scheduler.

Rejected updatedAt as an optimistic-concurrency token: performUpdateKnowledgeConnector stamps updatedAt on every connector update with no status guard, so a user editing config mid-sync would poison the token, the terminal write would be rejected, and the connector would sit syncing until the reaper cleared it two hours later — worse than the bug being fixed.

Both terminal writes go through a single writeTerminalConnectorState, which applies the stillHoldsSyncLock WHERE internally. Callers pass only their SET values and never build a WHERE clause, so there is exactly one place the guard can be removed from — and a unit test on that function's emitted WHERE catches its removal. A terminal path added later cannot forget the guard, because there is no unguarded way to write the row. The pre-lock "knowledge base deleted" write is deliberately outside it: that run never acquired the lock, so a status = 'syncing' guard would silently discard the error it needs to record.

A superseded run's document writes still persist. Documents it added, updated, resurrected, or deleted are already committed and are not rolled back — only the connector-level bookkeeping (status, lastSyncAt, failure counter, nextSyncAt) is discarded, in favour of whoever reclaimed the row. An operator sees the connector carrying the reaper's verdict, the sync-log row failed, and the task run reporting sync_superseded, while the corpus reflects the work that run actually did.

The failure path deliberately does not get the sync_superseded outcome. result.error there already carries the real failure cause and the task wrapper already reports the run as unsuccessful, so overwriting it would destroy the diagnostic without changing the reported outcome; the supersession is carried by a warn log instead.

Two tests in route.test.ts were inert and were rewritten rather than left: one asserted node.column === …connectorId, but eq() builds {left, right} and only inArray() builds {column}, so an eq-scoped sweep passed it; the other asserted only the bookends of the disable expression, leaving the comparison itself unchecked, so + 2 >=, + 1 >, and an inverted + 1 <= all passed — the last disabling a connector on its first hard kill. Both now pin the whole expression and are mutation-checked against those specific mutants.

A second test-quality pass replaced two assertions that could not fail. The backoff-ladder test recomputed Math.min(failures * step, cap) from the SQL's own binds and compared it against connectorFailureBackoffMinutes — both sides derived from the same two constants, so it held for any values and any shape. It now pins the SQL text whole, pins the binds to the shared constants, and pins concrete minutes on both sides (1 → 30, 3 → 90, 9 → 270, 48 → 1440, 49 → 1440), so the SQL↔JS equivalence is asserted rather than assumed. The hold-notice test used three independent toContain calls on distinct digit strings, which passed with the first two interpolations swapped — an inverted, actively misleading operator message — and now asserts the message whole.

The in-process failure path is extracted as buildSyncFailureUpdate, mirroring buildSyncSuccessUpdate, so the auto-disable threshold, the backoff, and the token release are unit-tested on the path the breaker actually runs through. Previously only the reaper's SQL equivalent was covered.

Ceiling on what the WHERE-clause assertions prove: packages/testing/src/mocks/schema.mock.ts maps every column to its bare name, so knowledgeConnector.status and knowledgeConnectorSyncLog.status are both the string 'status'. Assertions of the form node.left === schemaMock.<table>.<col> are therefore table-blind — a guard on the wrong table would still pass. No test here depends on that distinction, and the shared mock is deliberately not churned in this PR, but the next person should know the limit.

loadPreviousListingObservation reconstructs the previous listing from stored counters, and excluded documents land in docsUnchanged, so that historical figure stays inflated. Not recoverable without a schema change; errs toward blocking deletions, so it fails safe — and the new blast-radius cap is now a second line of defense behind it. Closing it as a documented limitation rather than carrying it as an open item.

If completeSyncLog(…, 'completed') succeeds and a later step throws, the catch path's failed write now no-ops rather than flipping the row. A behavior change beyond the reported bug, and an improvement: the row's counters do describe a completed listing-and-reconciliation pass, and loadPreviousListingObservation reads only completed rows, so it now receives an observation it previously lost.

sync-engine.test.ts moved from a local vi.mock('drizzle-orm') stub to the shared drizzleOrmMock. The new guard assertions need real condition nodes rather than bare vi.fn()s, and the local stub was also incomplete — it omitted six operators the module imports and only worked because no test reached them. All 113 tests pass against the shared mock.

vercel · 2026-08-20T23:59:59Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Aug 21, 2026 4:19pm

cursor · 2026-08-21T00:00:06Z

PR Summary

High Risk
Touches connector lock ownership, failure/disable policy, and reconciliation deletes that can drop embeddings. A bug here can disable live syncs or destroy a corpus.

Overview
Makes hard-killed connector syncs (OOM/SIGKILL) actually count as failures, and stops a healthy-looking partial listing from tombstoning or purging a large share of the corpus.

The stale-lock reaper now increments consecutiveFailures, applies the shared backoff ladder, and can disable the connector in the same SQL statement that clears the lock. Reclaim uses a dedicated syncLockLeaseAt (COALESCE to updatedAt for unbackfilled rows). Live runs heartbeat that lease; terminal writes go through writeTerminalConnectorState and only land if this run still holds syncLockToken while status is syncing. Orphaned started sync-log rows are closed independently of this tick’s reclaims, but spared if the lock is still being heartbeated.

Reconciliation holds a generation whose deletes exceed a per-generation cap (ratio of owned docs, with a small-corpus floor). Soft and hard deletes are gated separately so steady churn does not deadlock. User-excluded docs are never deletion-eligible; listed vs owned counts use the same non-excluded population. fullSync remains the escape hatch. Long purges hard-delete in heartbeat-separated chunks.

Document processing dispatch is centralized in dispatchDocumentProcessing so a failed fire-and-forget queue marks the row failed instead of leaving it pending. Tests also pin drizzle mock columns as table.column.

^{Reviewed by Cursor Bugbot for commit 712b2c4. Configure here.}

greptile-apps · 2026-08-21T00:03:03Z

Greptile Summary

This PR hardens connector synchronization against hard-killed or superseded runs, caps deletion reconciliation, and makes document-processing dispatch and retries safer.

Adds token-and-lease-based connector lock ownership, heartbeat, stale-lock failure accounting, and orphaned sync-log reconciliation.
Holds oversized soft- or hard-deletion generations and reports the hold through connector state.
Adds bounded processing retries, guarded document claims, and durable handling for processing-dispatch failures.
Adds the corresponding schema migration and extensive scheduler, connector, and document-processing coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/lib/knowledge/connectors/sync-engine.ts	Adds lease ownership, guarded terminal state, heartbeat probes, capped reconciliation, and bounded stuck-document recovery without leaving an eligible prior-thread defect.
apps/sim/app/api/knowledge/connectors/sync/route.ts	Makes stale-lock recovery count failures and safely closes orphaned sync logs while sparing live heartbeated runs.
apps/sim/lib/knowledge/documents/service.ts	Adds guarded processing claims, attempt accounting, and race-safe retry behavior.
apps/sim/lib/knowledge/documents/processing-dispatch.ts	Centralizes fire-and-forget dispatch failure recording so documents are not silently stranded pending.
packages/db/schema.ts	Adds connector lease/token and document-processing attempt fields used by the new concurrency controls.
packages/db/migrations/0298_nosy_ken_ellis.sql	Applies the additive database fields and indexes required by the connector and processing changes.

Sequence Diagram

sequenceDiagram
  participant Scheduler
  participant Connector
  participant SyncRun
  participant SyncLog
  participant Corpus
  SyncRun->>Connector: Acquire lock with run token and lease
  loop During long-running work
    SyncRun->>Connector: Heartbeat matching token
    SyncRun->>Corpus: List and process documents
  end
  alt Lease becomes stale
    Scheduler->>Connector: Reclaim lock, increment failures, back off or disable
    Scheduler->>SyncLog: Mark orphaned started row failed
    SyncRun-->>Connector: Later ownership probe is rejected
  else Run still owns lock
    SyncRun->>Corpus: Reconcile within deletion cap
    SyncRun->>SyncLog: Complete while token still matches
    SyncRun->>Connector: Publish terminal state and release token
  end

_{Reviews (8): Last reviewed commit: "fix(knowledge): guard the completed sync..." | Re-trigger Greptile}

…ligible rows Review round 1 on #6909. - A held pass reported an ordinary successful sync: the cap logged an error and the success update then cleared lastSyncError and reset consecutiveFailures, so source-removed documents stayed indexed with no operator signal. The notice is now threaded into the success update itself — writing it at the hold site would have been clobbered by that same update ~300 lines later, in the same run. status stays active and the failure counter still resets: a held pass is a healthy sync that declined to delete. - The cap denominator counted excluded tombstones, which partitionSyncReconciliation can never delete, inflating a budget against rows that cannot be spent. Both sides are now counted over the deletion-eligible population, matching the numerator. - completeSyncLog now only writes a row still marked started, so a late-finishing in-process run cannot overwrite a row the stale sweep already closed. The TTL is documented as a hard ceiling for both dispatch paths.

waleedlatif1 · 2026-08-21T00:20:29Z

@greptile

waleedlatif1 · 2026-08-21T00:20:30Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit de457b4. Configure here.}

…ligible rows Review round 1 on #6909. - A held pass reported an ordinary successful sync: the cap logged an error and the success update then cleared lastSyncError and reset consecutiveFailures, so source-removed documents stayed indexed with no operator signal. The notice is now threaded into the success update itself — writing it at the hold site would have been clobbered by that same update ~300 lines later, in the same run. status stays active and the failure counter still resets: a held pass is a healthy sync that declined to delete. - The cap denominator counted excluded tombstones, which partitionSyncReconciliation can never delete, inflating a budget against rows that cannot be spent. Both sides are now counted over the deletion-eligible population, matching the numerator. - completeSyncLog now only writes a row still marked started, so a late-finishing in-process run cannot overwrite a row the stale sweep already closed. The TTL is documented as a hard ceiling for both dispatch paths.

…erdict Review round 2 on #6909. The terminal connector writes were unguarded, so a run that outlived the stale lock could still land its result after the reaper reclaimed it: flipping status back to active, zeroing consecutiveFailures, and erasing a backoff or an auto-disable the breaker had just applied. Both terminal paths now go through a single writer that applies the still-holds-the-lock guard itself, so no future terminal path can be added without it. The knowledge-base-deleted write stays outside deliberately — it runs before the lock is acquired, so the guard would silently discard it. A superseded success is reported as an error rather than a clean sync, matching the treatment lock contention already gets. The failure path deliberately keeps its real error message instead: it already reports failure, so overwriting the cause would lose the diagnostic and gain nothing. Falls out of the same guard: a connector paused mid-sync is no longer flipped back to active by the completing run.

waleedlatif1 · 2026-08-21T01:03:00Z

@greptile

waleedlatif1 · 2026-08-21T01:03:01Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit aff50cb. Configure here.}

…ligible rows Review round 1 on #6909. - A held pass reported an ordinary successful sync: the cap logged an error and the success update then cleared lastSyncError and reset consecutiveFailures, so source-removed documents stayed indexed with no operator signal. The notice is now threaded into the success update itself — writing it at the hold site would have been clobbered by that same update ~300 lines later, in the same run. status stays active and the failure counter still resets: a held pass is a healthy sync that declined to delete. - The cap denominator counted excluded tombstones, which partitionSyncReconciliation can never delete, inflating a budget against rows that cannot be spent. Both sides are now counted over the deletion-eligible population, matching the numerator. - completeSyncLog now only writes a row still marked started, so a late-finishing in-process run cannot overwrite a row the stale sweep already closed. The TTL is documented as a hard ceiling for both dispatch paths.

…erdict Review round 2 on #6909. The terminal connector writes were unguarded, so a run that outlived the stale lock could still land its result after the reaper reclaimed it: flipping status back to active, zeroing consecutiveFailures, and erasing a backoff or an auto-disable the breaker had just applied. Both terminal paths now go through a single writer that applies the still-holds-the-lock guard itself, so no future terminal path can be added without it. The knowledge-base-deleted write stays outside deliberately — it runs before the lock is acquired, so the guard would silently discard it. A superseded success is reported as an error rather than a clean sync, matching the treatment lock contention already gets. The failure path deliberately keeps its real error message instead: it already reports failure, so overwriting the cause would lose the diagnostic and gain nothing. Falls out of the same guard: a connector paused mid-sync is no longer flipped back to active by the completing run.

Review round 3 on #6909. Guarding terminal writes on status='syncing' proved only that *a* run held the lock, not that this one did. After a stale-lock reclaim dispatched a replacement, the original run's guard matched the replacement's own lock and clobbered both its state and the reaper's bookkeeping — and the live run's write was then rejected. The dead run won and the live one was discarded, which is worse than the last-write-wins behavior the guard replaced. A nullable sync_lock_token is stamped in the same statement that claims the lock, so ownership is established atomically with acquisition and matching it proves the lock is still this run's. status='syncing' stays alongside it as defence in depth and to keep a connector paused mid-sync from being flipped back to active. Rejected using updatedAt as an optimistic-concurrency token: connector updates bump it unconditionally with no status guard, so a user editing config mid-sync would strand the connector in syncing until the reaper cleared it two hours later. Migration is additive, nullable, no backfill. Existing rows read NULL, which no in-flight run can match, so a sync spanning the deploy loses only its terminal write and is re-run by the scheduler.

…ligible rows Review round 1 on #6909. - A held pass reported an ordinary successful sync: the cap logged an error and the success update then cleared lastSyncError and reset consecutiveFailures, so source-removed documents stayed indexed with no operator signal. The notice is now threaded into the success update itself — writing it at the hold site would have been clobbered by that same update ~300 lines later, in the same run. status stays active and the failure counter still resets: a held pass is a healthy sync that declined to delete. - The cap denominator counted excluded tombstones, which partitionSyncReconciliation can never delete, inflating a budget against rows that cannot be spent. Both sides are now counted over the deletion-eligible population, matching the numerator. - completeSyncLog now only writes a row still marked started, so a late-finishing in-process run cannot overwrite a row the stale sweep already closed. The TTL is documented as a hard ceiling for both dispatch paths.

…erdict Review round 2 on #6909. The terminal connector writes were unguarded, so a run that outlived the stale lock could still land its result after the reaper reclaimed it: flipping status back to active, zeroing consecutiveFailures, and erasing a backoff or an auto-disable the breaker had just applied. Both terminal paths now go through a single writer that applies the still-holds-the-lock guard itself, so no future terminal path can be added without it. The knowledge-base-deleted write stays outside deliberately — it runs before the lock is acquired, so the guard would silently discard it. A superseded success is reported as an error rather than a clean sync, matching the treatment lock contention already gets. The failure path deliberately keeps its real error message instead: it already reports failure, so overwriting the cause would lose the diagnostic and gain nothing. Falls out of the same guard: a connector paused mid-sync is no longer flipped back to active by the completing run.

Review round 3 on #6909. Guarding terminal writes on status='syncing' proved only that *a* run held the lock, not that this one did. After a stale-lock reclaim dispatched a replacement, the original run's guard matched the replacement's own lock and clobbered both its state and the reaper's bookkeeping — and the live run's write was then rejected. The dead run won and the live one was discarded, which is worse than the last-write-wins behavior the guard replaced. A nullable sync_lock_token is stamped in the same statement that claims the lock, so ownership is established atomically with acquisition and matching it proves the lock is still this run's. status='syncing' stays alongside it as defence in depth and to keep a connector paused mid-sync from being flipped back to active. Rejected using updatedAt as an optimistic-concurrency token: connector updates bump it unconditionally with no status guard, so a user editing config mid-sync would strand the connector in syncing until the reaper cleared it two hours later. Migration is additive, nullable, no backfill. Existing rows read NULL, which no in-flight run can match, so a sync spanning the deploy loses only its terminal write and is re-run by the scheduler.

…s apart Review round 4 on #6909. Two regressions the guards introduced, plus two tests that could not fail. Counting a stale-lock reclaim as a failure turned the reaper into a one-way ratchet for any sync that legitimately outran the TTL: the in-process path has no duration cap, so a long self-hosted sync was reclaimed, its successful terminal write then failed the ownership guard and was discarded, and its failure counter never reset. Ten of those and a working connector was disabled telling the user to reconnect. A running sync now refreshes updatedAt every five minutes, so the reaper's staleness predicate means "nobody is working on this" rather than "this started a long time ago". The beat is guarded on the run's own lock, so it doubles as an ownership probe: a run whose lock was reclaimed abandons immediately instead of working for hours and then discarding the result. The deletion cap summed soft and hard deletes against one ceiling sized for a single generation, so a connector with steady churn deadlocked from its second sync onward and got monotonically worse — the all-or-nothing hold blocked the very hard deletes that would have drained the tombstone backlog. Hard deletes are confirmations of removals already gated when they were soft-deleted, so each generation now caps independently. Both guard tests for the reaper asserted only the bookends of the rendered SQL, leaving the comparison itself unasserted: an inverted threshold that disabled a connector on its first hard kill passed. Both now assert the whole expression.

…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.

Review round 6 on #6909, plus a consistency pass over the whole branch. The sweep keys on the log row's startedAt, and the heartbeat added earlier in this PR refreshes the connector's updatedAt — the log table has no equivalent, so nothing refreshed what the sweep reads. A legitimately long in-process sync kept its connector lock exactly as designed while its log row was closed as failed at the TTL, and completeSyncLog's started guard then no-opped when the run finished. A successful sync was recorded permanently as a failure and its counters were lost to the listing-safety check. Neither round was wrong alone; the combination was. The sweep now spares a row whose id is still the connector's lock token, reusing the ownership mechanism rather than adding another. Every orphan still drains: a reclaimed run's token is cleared, a replaced run's token belongs to its successor, and rows predating the column have none. Five documentation claims that later rounds falsified are corrected, including the sweep's own rationale, which still argued the platform kills every run at the duration ceiling — the reasoning the heartbeat exists because it does not hold for the in-process path. applySupersededOutcome's boolean parameter was vestigial: both call sites passed false and a test asserted the dead branch. Simplified.

waleedlatif1 · 2026-08-21T15:38:19Z

@greptile

waleedlatif1 · 2026-08-21T15:38:21Z

@cursor review

An OOM is a SIGKILL, so executeSync's catch and finally never run. The out-of-process stale-lock reaper was the only survivor, and it cleared the lock without ever incrementing consecutiveFailures — so MAX_CONSECUTIVE_FAILURES was unreachable for hard kills and a crashing connector looped indefinitely on a flat 10-minute retry, faster than any healthy interval. - Move the failure threshold and backoff ladder into sync-limits.ts so the two writers cannot drift; the reaper now increments, backs off, and disables in the same statement that clears the lock - Sweep sync-log rows left `started` by a killed run, keyed off the row's own startedAt so the sweep is self-healing and drains the existing backlog - Delete the unreachable finally block; report a lock-contended run as skipped rather than as a successful zero-document sync - Stop stamping lastSyncAt on the failure path Deletion reconciliation only questioned listings that looked broken, leaving every partial-outage shape between 10% and 100% unguarded: a source serving half its documents tombstones the other half and hard-deletes it on the next pass. - Hold a reconciliation pass whose deletions exceed a share of the corpus, all-or-nothing; fullSync remains the documented escape hatch - Never make a user-excluded document deletion-eligible, guarded at deletion eligibility rather than at the select so resurrection still works - Count listed documents over the same population as the owned count - Judge the previous run against a corpus at least as large as the one present, which un-jams the two-strike purge — shipped with the cap, never before it

…ligible rows Review round 1 on #6909. - A held pass reported an ordinary successful sync: the cap logged an error and the success update then cleared lastSyncError and reset consecutiveFailures, so source-removed documents stayed indexed with no operator signal. The notice is now threaded into the success update itself — writing it at the hold site would have been clobbered by that same update ~300 lines later, in the same run. status stays active and the failure counter still resets: a held pass is a healthy sync that declined to delete. - The cap denominator counted excluded tombstones, which partitionSyncReconciliation can never delete, inflating a budget against rows that cannot be spent. Both sides are now counted over the deletion-eligible population, matching the numerator. - completeSyncLog now only writes a row still marked started, so a late-finishing in-process run cannot overwrite a row the stale sweep already closed. The TTL is documented as a hard ceiling for both dispatch paths.

…erdict Review round 2 on #6909. The terminal connector writes were unguarded, so a run that outlived the stale lock could still land its result after the reaper reclaimed it: flipping status back to active, zeroing consecutiveFailures, and erasing a backoff or an auto-disable the breaker had just applied. Both terminal paths now go through a single writer that applies the still-holds-the-lock guard itself, so no future terminal path can be added without it. The knowledge-base-deleted write stays outside deliberately — it runs before the lock is acquired, so the guard would silently discard it. A superseded success is reported as an error rather than a clean sync, matching the treatment lock contention already gets. The failure path deliberately keeps its real error message instead: it already reports failure, so overwriting the cause would lose the diagnostic and gain nothing. Falls out of the same guard: a connector paused mid-sync is no longer flipped back to active by the completing run.

Review round 3 on #6909. Guarding terminal writes on status='syncing' proved only that *a* run held the lock, not that this one did. After a stale-lock reclaim dispatched a replacement, the original run's guard matched the replacement's own lock and clobbered both its state and the reaper's bookkeeping — and the live run's write was then rejected. The dead run won and the live one was discarded, which is worse than the last-write-wins behavior the guard replaced. A nullable sync_lock_token is stamped in the same statement that claims the lock, so ownership is established atomically with acquisition and matching it proves the lock is still this run's. status='syncing' stays alongside it as defence in depth and to keep a connector paused mid-sync from being flipped back to active. Rejected using updatedAt as an optimistic-concurrency token: connector updates bump it unconditionally with no status guard, so a user editing config mid-sync would strand the connector in syncing until the reaper cleared it two hours later. Migration is additive, nullable, no backfill. Existing rows read NULL, which no in-flight run can match, so a sync spanning the deploy loses only its terminal write and is re-run by the scheduler.

…s apart Review round 4 on #6909. Two regressions the guards introduced, plus two tests that could not fail. Counting a stale-lock reclaim as a failure turned the reaper into a one-way ratchet for any sync that legitimately outran the TTL: the in-process path has no duration cap, so a long self-hosted sync was reclaimed, its successful terminal write then failed the ownership guard and was discarded, and its failure counter never reset. Ten of those and a working connector was disabled telling the user to reconnect. A running sync now refreshes updatedAt every five minutes, so the reaper's staleness predicate means "nobody is working on this" rather than "this started a long time ago". The beat is guarded on the run's own lock, so it doubles as an ownership probe: a run whose lock was reclaimed abandons immediately instead of working for hours and then discarding the result. The deletion cap summed soft and hard deletes against one ceiling sized for a single generation, so a connector with steady churn deadlocked from its second sync onward and got monotonically worse — the all-or-nothing hold blocked the very hard deletes that would have drained the tombstone backlog. Hard deletes are confirmations of removals already gated when they were soft-deleted, so each generation now caps independently. Both guard tests for the reaper asserted only the bookends of the rendered SQL, leaving the comparison itself unasserted: an inverted threshold that disabled a connector on its first hard kill passed. Both now assert the whole expression.

…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.

The migration was hand-written, which left it inconsistent with every other migration in the repo and, more importantly, without a schema snapshot. Drizzle diffs against the latest snapshot to decide what a migration needs to contain, so the next generate would have seen the column as still missing and emitted it a second time. Regenerated properly: drizzle-kit now owns the SQL, the journal entry, and 0297_snapshot.json. The emitted statement matches the house pattern for an additive nullable column, and check:migrations still reports backward-compatible.

Review round 6 on #6909, plus a consistency pass over the whole branch. The sweep keys on the log row's startedAt, and the heartbeat added earlier in this PR refreshes the connector's updatedAt — the log table has no equivalent, so nothing refreshed what the sweep reads. A legitimately long in-process sync kept its connector lock exactly as designed while its log row was closed as failed at the TTL, and completeSyncLog's started guard then no-opped when the run finished. A successful sync was recorded permanently as a failure and its counters were lost to the listing-safety check. Neither round was wrong alone; the combination was. The sweep now spares a row whose id is still the connector's lock token, reusing the ownership mechanism rather than adding another. Every orphan still drains: a reclaimed run's token is cleared, a replaced run's token belongs to its successor, and rows predating the column have none. Five documentation claims that later rounds falsified are corrected, including the sweep's own rationale, which still argued the platform kills every run at the duration ceiling — the reasoning the heartbeat exists because it does not hold for the in-process path. applySupersededOutcome's boolean parameter was vestigial: both call sites passed false and a test asserted the dead branch. Simplified.

…fore sparing its log row The previous round spared a sync-log row whose id was still the connector's lock token. That is necessary but not sufficient: the reaper's reclaim filters out archived and soft-deleted connectors, so their rows keep status syncing with an intact token indefinitely and would have been spared forever. It also spared a run that died without ever being reclaimed. Keying on connector liveness alone has the opposite gap — it cannot tell which run holds the lock, so an orphan on a connector that is immediately re-locked by a replacement is spared until a tick happens to catch the connector idle. Sparing now requires all three: the connector is locked, this row's run is the holder, and that lock is being heartbeated. An orphan can satisfy at most two, so no timing window strands one. This is a per-row liveness predicate, not a restriction of the candidate set — every stale row is still evaluated. A backlog row predating the token column fails on two counts and still drains, so the sweep remains self-healing.

…loop The heartbeat was added where we happened to be looking. The pagination loop — where a large source spends most of its wall clock, since the batch loop does not start until every page is fetched — never beat at all, so a long listing on the uncapped in-process path was still reclaimed as a hard failure. That is the exact ratchet the heartbeat exists to prevent. Auditing the remaining phases found something worse in the stuck-document retry: on the in-process path it handed the entire backlog to a single await that fully parses, embeds and indexes every document before returning, so no beat placement could interrupt it. That dispatch is now chunked, with a beat per chunk. All four call sites share one beatIfDue closure; the two pre-existing inline blocks were collapsed onto it rather than left as copies. An await longer than the TTL — one pathological listing page, or a very large hard delete — is still not covered, and no inline beat can cover it. Closing that needs a concurrent interval, which is a second mechanism and a separate decision. Adds the first test that drives executeSync itself, reaching the pagination loop through the real lock acquisition rather than testing helpers in isolation.

#6921 merged first and took 0297 for an unrelated column on a different table, so this branch's migration is regenerated rather than renamed. Renaming would leave the snapshot describing the wrong ordinal, and drizzle diffs against that snapshot to decide what the next migration contains. The regenerated statement is the lock token column alone — it correctly diffs against staging's 0297 snapshot rather than re-emitting the column that landed there.

Adds a bounded retry budget so a deterministically failing document stops being re-parsed and re-embedded on every sync, and closes the unguarded state transitions around it. - `processing_attempts` is charged in the one guarded write every dispatch passes through, cleared on success, and bounds the stuck-document sweep. - The document claim now guards on status and gates on the row it writes back, so a worker can no longer process and bill a document it never claimed. The retry, missing-context and sweep-reset writes are guarded the same way. - `STALE_PROCESSING_MINUTES` and the queue concurrency are derived from the env vars the processing task is configured with, so raising the run ceiling can no longer make the sweep reclaim live work. - Qualifies every column in the shared schema mock as `table.column`, which makes `.where()` assertions across the repo able to fail on a wrong-table predicate. Two tests were pinning nothing as a result.

A document that exhausted its processing-attempt budget while still `pending` matched no recovery path at all: the connector sweep requires an unspent budget, the timeout path requires `processing`, and retry accepted only terminal states. Since every dispatch charges an attempt, a worker killed before its claim UPDATE burned budget without ever changing status. Retry now also admits `pending` once the shared queued dispatch grace has elapsed, measured from COALESCE(queued_at, uploaded_at) exactly as the sweep measures it, so a double-click still lands inside a live dispatch's window and still matches no rows. Retry committed its reset in its own transaction and then dispatched, so a throwing dispatch stranded the row it had just reset. It now unwinds to `failed` and reports the failure instead of painting success over a dead document — and the orchestration layer stops rejecting `pending` before the guarded requeue can see it, and stops hard-coding success. Four upload paths logged total dispatch failure and walked away. Upload documents carry no connector id, so nothing sweeps them. They now share one unwinding dispatch helper. The funnel is deliberately left alone: the connector sweep wants its documents reclaimable, and the outbox handler must let the throw propagate so the relay retries. `updated_at` carried two meanings, row mtime and lock lease, so any unrelated write renewed a wedged run's lease — including the connector edit path, the only control the UI leaves enabled on one. The lease moves to its own column written solely by lock acquisition and the heartbeat, read as COALESCE(lease, updated_at) so a row already syncing at deploy stays reclaimable, and cleared by both terminal helpers and by the two knowledge-base-deleted writers that flip a possibly-locked row. Connector updates now refuse while a sync holds the lock, matching the sibling sync path. Also: the reaper reports the disabled verdict it actually wrote rather than a timeout the operator cannot wait out, reconciliation hard-deletes in heartbeat-separated chunks, and the sync-log sweep gets a partial index on the column it scans.

… lock `executeSync`'s success path closed its sync-log row before `writeTerminalConnectorState` ran its ownership check, and the close was guarded only on `status = 'started'`. That guard defers to the scheduler's sweep, but the sweep is not the only writer that strands a live run: the knowledge-base-deleted writers clear the token unconditionally, a user pausing a connector flips it out of `syncing`, and the reaper's reclaim and its log-close are two statements that can commit apart. In each case the run's connector write is refused while its log row is still `started`, so the run published a `completed` row for bookkeeping that was discarded — and `loadPreviousListingObservation` reads exactly those rows as corroboration for the next run's reconciliation. The close now takes the ownership condition itself, reusing `stillHoldsSyncLock` as an EXISTS predicate so the log row and the connector row are written under the same condition and cannot disagree. Swapping the two calls was considered and rejected: a `completeSyncLog` failure would then leave a `started` row on a connector already recorded `active`, the reaper would later mark it `failed`, and a legitimate observation would be lost silently. Only the success path is guarded — a `failed` row is never read back as evidence, and both failure paths legitimately close a run whose lock is already gone. A refused close short-circuits to the superseded result the terminal write would have produced two statements later. The `ConnectorDeletedException` handler hard-deleted leftover documents and closed its log, but wrote nothing to the connector row, leaving it `syncing` with a live token. Nothing else could clear it: the reaper requires `isNull(archivedAt)` and `isNull(deletedAt)`, so the one writer able to recover a stranded lock skips exactly the rows this path creates. It now releases token and lease and makes the transition terminal, matching the two knowledge-base-deleted writers. Guarded on ownership alone rather than `stillHoldsSyncLock`, for the same reason the heartbeat is: the connector being archived is this path's precondition, so a liveness clause would reject every write the release exists to make. A no-op when the row was hard deleted rather than archived — a user-initiated connector delete removes the row outright, leaving nothing to unwedge.

waleedlatif1 · 2026-08-21T16:20:36Z

@greptile

waleedlatif1 · 2026-08-21T16:20:37Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 712b2c4. Configure here.}

waleedlatif1 mentioned this pull request Aug 21, 2026

fix(connectors): stop rendering running and crashed syncs as successes #6910

Merged

greptile-apps Bot reviewed Aug 21, 2026

View reviewed changes

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

Comment thread apps/sim/app/api/knowledge/connectors/sync/route.ts