Skip to content

Commit 2734819

Browse files
committed
fix(connectors): make the sync-log sweep aware of a live run's heartbeat
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.
1 parent 4e1dc16 commit 2734819

2 files changed

Lines changed: 56 additions & 7 deletions

File tree

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010
import {
1111
dbChainMockFns,
12+
flattenMockConditions,
1213
hasMockCondition,
1314
type MockCondition,
1415
resetDbChainMock,
@@ -213,6 +214,29 @@ describe('connector sync scheduler stale-lock reaper', () => {
213214
).toBe(true)
214215
})
215216

217+
it('spares the log row of a run that still holds its connector lock', async () => {
218+
await runTickRecovering(['connector-1'])
219+
220+
/**
221+
* The sweep keys on `startedAt`, which no heartbeat refreshes, so age alone
222+
* would close a legitimately long in-process run's row and record a
223+
* successful sync as failed.
224+
*/
225+
const where = dbChainMockFns.where.mock.calls[1][0]
226+
const liveness = flattenMockConditions(where).find(
227+
(node: MockCondition) => typeof node.toSQL === 'function'
228+
)
229+
expect(liveness).toBeDefined()
230+
231+
const rendered = (liveness as unknown as MockSqlFragment).toSQL().sql
232+
expect(rendered).toContain('NOT EXISTS')
233+
expect(rendered).toContain("'syncing'")
234+
235+
const bound = (liveness as unknown as MockSqlFragment).values
236+
expect(bound).toContain(schemaMock.knowledgeConnector.syncLockToken)
237+
expect(bound).toContain(schemaMock.knowledgeConnectorSyncLog.id)
238+
})
239+
216240
it('closes stale sync-log rows even when no connector was reclaimed this tick', async () => {
217241
/**
218242
* The self-healing assertion. A row orphaned before this sweep existed —

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

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,29 @@ const DISPATCH_CONCURRENCY = 10
3131

3232
const STALE_LOCK_ERROR_MESSAGE = 'Sync timed out (stale lock recovered)'
3333

34+
/**
35+
* Excludes a sync-log row whose run still demonstrably holds its connector's
36+
* lock.
37+
*
38+
* The sweep keys on `startedAt`, and nothing refreshes that — the heartbeat
39+
* renews `knowledge_connector.updatedAt`, and the log table has no equivalent
40+
* column. So a legitimately long in-process run keeps its connector lock but
41+
* would still have its log row closed as `failed` at the TTL, recording a
42+
* successful sync as a failure and losing its counters to
43+
* `loadPreviousListingObservation`. Matching the connector's `syncLockToken`
44+
* against the row's own id is exactly "this run is still the lock holder", so a
45+
* live run is spared while every orphan — reclaimed, replaced, or predating the
46+
* token column, where the token is NULL — is still swept.
47+
*/
48+
function runNoLongerHoldsItsLock(): SQL {
49+
return sql`NOT EXISTS (
50+
SELECT 1 FROM ${knowledgeConnector}
51+
WHERE ${knowledgeConnector.id} = ${knowledgeConnectorSyncLog.connectorId}
52+
AND ${knowledgeConnector.syncLockToken} = ${knowledgeConnectorSyncLog.id}
53+
AND ${knowledgeConnector.status} = 'syncing'
54+
)`
55+
}
56+
3457
/**
3558
* The reclaimed connector's new consecutive-failure count.
3659
*
@@ -117,12 +140,13 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
117140
* stay stranded forever. Keying off the row's own `startedAt` instead makes
118141
* the sweep self-healing and lets it drain the existing backlog.
119142
*
120-
* Safe on liveness: the run ceiling is
121-
* {@link CONNECTOR_SYNC_MAX_DURATION_SECONDS} and this TTL is twice that, so
122-
* a row older than the cutoff belongs to a run the platform has already
123-
* killed and which cannot still be writing. The predicate is per-row on
124-
* `startedAt`, so a fresh run's log row can never be caught by it — even on
125-
* a connector whose previous run is being reclaimed in this same tick.
143+
* Age alone does not prove a run is dead: the in-process fallback path has
144+
* no duration cap, so a large self-hosted sync can genuinely still be
145+
* working past the TTL. `runNoLongerHoldsItsLock` is what makes this safe —
146+
* a run still holding its connector's lock is spared regardless of age.
147+
* The age predicate is also per-row on `startedAt`, so a fresh run's log row
148+
* can never be caught by it, even on a connector whose previous run is being
149+
* reclaimed in this same tick.
126150
*/
127151
const closedSyncLogs = await db
128152
.update(knowledgeConnectorSyncLog)
@@ -134,7 +158,8 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
134158
.where(
135159
and(
136160
eq(knowledgeConnectorSyncLog.status, 'started'),
137-
lte(knowledgeConnectorSyncLog.startedAt, staleCutoff)
161+
lte(knowledgeConnectorSyncLog.startedAt, staleCutoff),
162+
runNoLongerHoldsItsLock()
138163
)
139164
)
140165
.returning({ id: knowledgeConnectorSyncLog.id })

0 commit comments

Comments
 (0)