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.

Project	Deployment	Actions	Updated (UTC)
docs	Ready	Preview	Aug 21, 2026 2:10am

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

PR Summary

High Risk
Touches connector lock ownership, failure/disable bookkeeping, and document deletion reconciliation—wrong predicates can disable healthy connectors or leave/destroy indexed documents.

Overview
Makes hard-killed connector syncs actually count as failures, and stops healthy-looking listings from mass-deleting a corpus.

Stale-lock reclaim now increments consecutiveFailures, applies the shared backoff ladder, and can disable—in one SQL statement—then clears syncLockToken. Orphaned started sync-log rows are closed independently of this tick’s reclaims, but spared if a live run still holds a heartbeated lock.

Lock ownership is a new nullable sync_lock_token (same id as the sync-log row). Terminal writes and heartbeats go through stillHoldsSyncLock / writeTerminalConnectorState. Runs refresh updatedAt every 5 minutes during listing, batches, and stuck-retry chunks, and abort as sync_superseded if the lock is gone. The old finally reset is removed.

Deletion safety: capReconciliationDeletions holds a generation that exceeds 25% of eligible docs (floor 25), per generation so churn does not deadlock. User-excluded docs are never deletion-eligible but remain resurrectable. Suspect-listing counts use the same non-excluded population; resolvePreviousOwnedCount un-jams two-strike corroboration after tombstoning. Held passes stay active and surface a notice on lastSyncError. fullSync still bypasses the cap.

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

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

Greptile Summary

The PR adds ownership-token and heartbeat handling for connector syncs, records stale-lock failures, and caps reconciliation deletions. Two lifecycle gaps remain:

A reclaimed run can perform reconciliation writes before probing ownership again.
A held-deletion warning can disappear on the next incremental success while withheld documents remain indexed.

Confidence Score: 3/5

The PR is not yet safe to merge because superseded runs can still mutate the corpus and held-deletion warnings can disappear while stale documents remain indexed.

Ownership is probed only after reconciliation writes, allowing a reclaimed run to race its replacement, and the success path replaces the hold notice with null on a later incremental pass without applying the withheld deletions.

Files Needing Attention: apps/sim/lib/knowledge/connectors/sync-engine.ts

Important Files Changed

Filename	Overview
apps/sim/lib/knowledge/connectors/sync-engine.ts	Adds deletion caps, hold reporting, lock tokens, and heartbeats, but permits post-reclaim document writes and prematurely clears persistent hold state.
apps/sim/app/api/knowledge/connectors/sync/route.ts	Reclaims stale connector locks, increments failure state atomically, and sweeps orphaned sync logs while sparing currently heartbeated owners.
apps/sim/lib/knowledge/connectors/sync-limits.ts	Centralizes connector failure thresholds, retry backoff, stale-lock TTL, and heartbeat interval.
packages/db/schema.ts	Adds the nullable connector sync ownership token used by acquisition, heartbeat, and terminal-write guards.
packages/db/migrations/0297_gray_kitty_pryde.sql	Adds the nullable `sync_lock_token` column required by the new ownership protocol.

Sequence Diagram

sequenceDiagram
  participant A as Original sync
  participant DB as Connector/Documents
  participant R as Stale-lock reaper
  participant B as Replacement sync
  A->>DB: Heartbeat while owning token A
  A->>A: Await long operation
  R->>DB: Reclaim lock and clear token A
  B->>DB: Acquire lock with token B
  A->>DB: Reconcile document mutations
  A->>DB: Heartbeat / ownership probe
  DB-->>A: Token no longer matches
  A-->>A: Report superseded after writes landed

_{Reviews (6): Last reviewed commit: "fix(connectors): heartbeat every unbound..." | 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.

waleedlatif1 · 2026-08-21T01:38:43Z

@greptile

waleedlatif1 · 2026-08-21T01:38:44Z

@cursor review

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-21T01:45:22Z

@greptile

waleedlatif1 · 2026-08-21T01:45:23Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

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

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

waleedlatif1 · 2026-08-21T01:58:56Z

@greptile

waleedlatif1 · 2026-08-21T01:58:57Z

@cursor review

greptile-apps · 2026-08-21T02:02:07Z

      result.docsDeleted += await hardDeleteDocuments(safeHardDeleteIds, syncLogId, connectorId)
    }

+    await beatIfDue()


Reconciliation precedes ownership probe

If a listing, hydration, processing, reconciliation, or deletion operation remains pending until the stale-lock reaper reclaims the connector, the original run performs its reconciliation writes before this ownership probe. It can therefore soft-delete, resurrect, or hard-delete documents after a replacement owns the connector, leaving corpus state determined by the racing runs.

Knowledge Base Used: Knowledge (RAG) Module

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

greptile-apps · 2026-08-21T02:02:07Z

+      buildSyncSuccessUpdate(
+        now,
+        actualDocCount,
+        calculateNextSyncTime(connector.syncIntervalMinutes),
+        reconciliationHoldNotice
      )


Incremental success clears hold warning

When a capped full-listing pass withholds an oversized soft-delete generation, it creates no tombstones, so the next successful incremental pass supplies a null hold notice here. That clears lastSyncError while the withheld source deletions remain unapplied and the removed documents remain indexed.

Knowledge Base Used: Knowledge (RAG) Module

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

cursor

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

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

cursor · 2026-08-21T02:03:40Z

+
          await processDocumentsWithQueue(
-            retryDocs.map((doc) => ({
+            retryDocs.slice(i, i + STUCK_RETRY_DISPATCH_CHUNK_SIZE).map((doc) => ({


Lock loss swallowed in stuck retry

Medium Severity

beatIfDue was added inside the stuck-retry try whose catch treats every error as a failed enqueue. A SyncLockLostException is therefore logged as an enqueue warning and execution continues into completeSyncLog('completed') instead of taking the outer abandon path that writes nothing and returns superseded. That races the sync-log sweep and can record a reclaimed or paused run as a completed observation.

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

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.

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

cursor Bot reviewed Aug 21, 2026

View reviewed changes

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

vercel Bot temporarily deployed to Preview August 21, 2026 00:19 Inactive

greptile-apps Bot reviewed Aug 21, 2026

View reviewed changes

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

cursor Bot reviewed Aug 21, 2026

View reviewed changes

waleedlatif1 force-pushed the fix/sync-core branch from de457b4 to aff50cb Compare August 21, 2026 00:41

vercel Bot temporarily deployed to Preview August 21, 2026 00:41 Inactive

greptile-apps Bot reviewed Aug 21, 2026

View reviewed changes

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

cursor Bot reviewed Aug 21, 2026

View reviewed changes

waleedlatif1 force-pushed the fix/sync-core branch from aff50cb to 8befa49 Compare August 21, 2026 01:20

vercel Bot deployed to Preview August 21, 2026 01:25 View deployment

waleedlatif1 force-pushed the fix/sync-core branch from 8befa49 to 3530024 Compare August 21, 2026 01:31

vercel Bot temporarily deployed to Preview August 21, 2026 01:37 Inactive

greptile-apps Bot reviewed Aug 21, 2026

View reviewed changes

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

cursor Bot reviewed Aug 21, 2026

View reviewed changes

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

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

vercel Bot temporarily deployed to Preview August 21, 2026 01:44 Inactive

vercel Bot temporarily deployed to Preview August 21, 2026 01:49 Inactive

cursor Bot reviewed Aug 21, 2026

View reviewed changes

vercel Bot temporarily deployed to Preview August 21, 2026 01:58 Inactive

greptile-apps Bot reviewed Aug 21, 2026

View reviewed changes

waleedlatif1 added 6 commits August 20, 2026 19:03

cursor Bot reviewed Aug 21, 2026

View reviewed changes

waleedlatif1 added 5 commits August 20, 2026 19:03

waleedlatif1 force-pushed the fix/sync-core branch from b68731c to 6c7c056 Compare August 21, 2026 02:05

vercel Bot deployed to Preview August 21, 2026 02:10 View deployment

Uh oh!

Conversation

waleedlatif1 commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why a crashing connector never backs off

Why a partial outage deletes half a corpus

Verification

Known and deliberate

Uh oh!

vercel Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

greptile-apps Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

waleedlatif1 commented Aug 21, 2026

Uh oh!

greptile-apps Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Aug 21, 2026

Choose a reason for hiding this comment

Lock loss swallowed in stuck retry

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

waleedlatif1 commented Aug 20, 2026 •

edited

Loading

vercel Bot commented Aug 20, 2026 •

edited

Loading

cursor Bot commented Aug 21, 2026 •

edited

Loading

greptile-apps Bot commented Aug 21, 2026 •

edited

Loading