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

* fix(connectors): stop rendering running and crashed syncs as successes The sync-history row derived its state from status literals the engine never writes — 'running', 'syncing', 'error' are connector statuses, not sync-log statuses. The in-progress branch was therefore false for every row that will ever exist, so a live sync rendered as a green success tick for its whole duration and a killed run left a permanent fake success reading "No changes". - Derive the row state from the three statuses the engine actually writes - Narrow the sync-log status contract from z.string() to an enum, and make the render switch exhaustive, so producer drift fails response validation and consumer drift fails the build - Add an interrupted state for a started row older than the stale-lock TTL: it is a crashed run, not a live one, and rendering it as a permanent spinner would trade one false signal for another * docs(connectors): record why the stale TTL is a hard ceiling for the interrupted state
2026-10-30 19:32:46 +08:00 · 2026-08-20 17:34:03 -07:00
parent 451d2ccbde
commit 040f40cd61
3 changed files with 238 additions and 9 deletions
@@ -0,0 +1,175 @@
+/**
+ * @vitest-environment jsdom
+ */
+import type { ReactNode, SVGProps } from 'react'
+import { act } from 'react'
+import { createRoot, type Root } from 'react-dom/client'
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import type { SyncLogData } from '@/lib/api/contracts/knowledge/connectors'
+import { CONNECTOR_SYNC_STALE_LOCK_TTL_MS } from '@/lib/knowledge/connectors/sync-limits'
+
+const { icon } = vi.hoisted(() => ({
+  icon: (name: string) => (props: SVGProps<SVGSVGElement>) => (
+    <svg data-testid={`icon-${name}`} className={props.className} />
+  ),
+}))
+
+vi.mock('@sim/emcn/icons', () => ({
+  ChevronDown: icon('chevron-down'),
+  CircleAlert: icon('circle-alert'),
+  CircleCheck: icon('circle-check'),
+  CircleX: icon('circle-x'),
+  Loader: icon('loader'),
+  Pause: icon('pause'),
+  Play: icon('play'),
+  RefreshCw: icon('refresh-cw'),
+  Settings: icon('settings'),
+  Trash: icon('trash'),
+  TriangleAlert: icon('triangle-alert'),
+}))
+
+vi.mock('@sim/emcn', () => ({
+  Badge: ({ children }: { children?: ReactNode }) => <span>{children}</span>,
+  Button: ({ children }: { children?: ReactNode }) => <button type='button'>{children}</button>,
+  Checkbox: () => <input type='checkbox' />,
+  ChipConfirmModal: () => null,
+  cn: (...classes: unknown[]) => classes.filter(Boolean).join(' '),
+  DropdownMenu: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
+  DropdownMenuContent: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
+  DropdownMenuItem: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
+  DropdownMenuTrigger: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
+  Tooltip: ({ children }: { children?: ReactNode }) => <div>{children}</div>,
+}))
+
+vi.mock('@/lib/credentials/client-state', () => ({
+  consumeOAuthReturnContext: vi.fn(),
+  writeOAuthReturnContext: vi.fn(),
+}))
+vi.mock('@/lib/oauth', () => ({
+  getCanonicalScopesForProvider: vi.fn(() => []),
+  getProviderIdFromServiceId: vi.fn(() => undefined),
+}))
+vi.mock('@/lib/oauth/utils', () => ({ getMissingRequiredScopes: vi.fn(() => []) }))
+vi.mock('@/app/workspace/[workspaceId]/components/connect-oauth-modal', () => ({
+  ConnectOAuthModal: () => null,
+}))
+vi.mock(
+  '@/app/workspace/[workspaceId]/knowledge/[id]/components/edit-connector-modal/edit-connector-modal',
+  () => ({ EditConnectorModal: () => null })
+)
+vi.mock('@/blocks', () => ({ getBlock: vi.fn(() => undefined) }))
+vi.mock('@/blocks/icon-color', () => ({ getTileIconColorClass: vi.fn(() => '') }))
+vi.mock('@/connectors/registry', () => ({ CONNECTOR_META_REGISTRY: {} }))
+vi.mock('@/hooks/queries/kb/connectors', () => ({
+  useConnectorDetail: vi.fn(() => ({ data: undefined, isLoading: false })),
+  useDeleteConnector: vi.fn(() => ({ mutate: vi.fn(), isPending: false })),
+  useTriggerSync: vi.fn(() => ({ mutate: vi.fn() })),
+  useUpdateConnector: vi.fn(() => ({ mutate: vi.fn() })),
+}))
+vi.mock('@/hooks/queries/oauth/oauth-credentials', () => ({
+  useOAuthCredentials: vi.fn(() => ({ data: [] })),
+}))
+vi.mock('@/hooks/use-credential-refresh-triggers', () => ({
+  useCredentialRefreshTriggers: vi.fn(),
+}))
+
+import { SyncHistory } from '@/app/workspace/[workspaceId]/knowledge/[id]/components/connectors-section/connectors-section'
+
+let root: Root | null = null
+
+function makeLog(overrides: Partial<SyncLogData> & Pick<SyncLogData, 'status'>): SyncLogData {
+  return {
+    id: 'log-1',
+    connectorId: 'connector-1',
+    startedAt: new Date().toISOString(),
+    completedAt: null,
+    docsAdded: 0,
+    docsUpdated: 0,
+    docsDeleted: 0,
+    docsUnchanged: 0,
+    docsFailed: 0,
+    errorMessage: null,
+    ...overrides,
+  }
+}
+
+function render(log: SyncLogData) {
+  ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
+  const container = document.createElement('div')
+  document.body.appendChild(container)
+  root = createRoot(container)
+  act(() => root?.render(<SyncHistory logs={[log]} isLoading={false} />))
+  return container
+}
+
+function icons(container: HTMLElement) {
+  return Array.from(container.querySelectorAll('[data-testid^="icon-"]')).map((node) =>
+    node.getAttribute('data-testid')
+  )
+}
+
+afterEach(() => {
+  act(() => root?.unmount())
+  root = null
+  document.body.innerHTML = ''
+  vi.clearAllMocks()
+})
+
+describe('SyncHistory', () => {
+  it('renders a fresh "started" row as in progress, not as a success', () => {
+    const container = render(makeLog({ status: 'started' }))
+
+    expect(icons(container)).toEqual(['icon-loader'])
+    expect(icons(container)).not.toContain('icon-circle-check')
+    expect(container.textContent).toContain('In progress…')
+    expect(container.textContent).not.toContain('No changes')
+  })
+
+  it('renders a "completed" row as a success with its change counts', () => {
+    const container = render(makeLog({ status: 'completed', docsAdded: 3 }))
+
+    expect(icons(container)).toEqual(['icon-circle-check'])
+    expect(container.textContent).toContain('+3')
+    expect(container.textContent).not.toContain('In progress…')
+  })
+
+  it('renders a "completed" row with no changes as "No changes"', () => {
+    const container = render(makeLog({ status: 'completed' }))
+
+    expect(icons(container)).toEqual(['icon-circle-check'])
+    expect(container.textContent).toContain('No changes')
+  })
+
+  it('renders a "failed" row as an error with its message', () => {
+    const container = render(makeLog({ status: 'failed', errorMessage: 'token expired' }))
+
+    expect(icons(container)).toEqual(['icon-circle-x'])
+    expect(container.textContent).toContain('token expired')
+    expect(container.textContent).not.toContain('No changes')
+  })
+
+  describe('stale-lock boundary', () => {
+    it('still reads as in progress just inside the stale-lock TTL', () => {
+      const startedAt = new Date(
+        Date.now() - CONNECTOR_SYNC_STALE_LOCK_TTL_MS + 60_000
+      ).toISOString()
+      const container = render(makeLog({ status: 'started', startedAt }))
+
+      expect(icons(container)).toEqual(['icon-loader'])
+      expect(container.textContent).toContain('In progress…')
+      expect(container.textContent).not.toContain('Interrupted')
+    })
+
+    it('reads as interrupted once past the stale-lock TTL', () => {
+      const startedAt = new Date(
+        Date.now() - CONNECTOR_SYNC_STALE_LOCK_TTL_MS - 60_000
+      ).toISOString()
+      const container = render(makeLog({ status: 'started', startedAt }))
+
+      expect(icons(container)).toEqual(['icon-triangle-alert'])
+      expect(container.textContent).toContain('Interrupted')
+      expect(container.textContent).not.toContain('In progress…')
+      expect(container.textContent).not.toContain('No changes')
+    })
+  })
+})
@@ -30,6 +30,7 @@ import {
 import { createLogger } from '@sim/logger'
 import { format, formatDistanceToNow, isPast } from 'date-fns'
 import { consumeOAuthReturnContext, writeOAuthReturnContext } from '@/lib/credentials/client-state'
+import { CONNECTOR_SYNC_STALE_LOCK_TTL_MS } from '@/lib/knowledge/connectors/sync-limits'
 import { getCanonicalScopesForProvider, getProviderIdFromServiceId } from '@/lib/oauth'
 import { getMissingRequiredScopes } from '@/lib/oauth/utils'
 import { ConnectOAuthModal } from '@/app/workspace/[workspaceId]/components/connect-oauth-modal'
@@ -667,12 +668,47 @@ function ConnectorCard({
  )
 }

+/**
+ * How a sync-log row should read to a user.
+ *
+ * `interrupted` has no status of its own: the scheduler reclaims a stale
+ * `syncing` lock by flipping the *connector* to `error`, and never rewrites the
+ * log row, so a run killed mid-flight (deploy, OOM) stays `started` forever.
+ * Past the stale-lock TTL that row is a crashed run, not a live one.
+ *
+ * Treating the TTL as a hard ceiling is a deliberate policy choice, not an
+ * assumption that no run can outlive it. The in-process fallback sync runs
+ * unawaited in the web process with no duration limit, so it genuinely can —
+ * but nothing writes to the row between lock acquisition and completion, so a
+ * live run past the TTL and a dead one are byte-identical to this component.
+ * Rendering it as still running is the failure this state exists to fix; the
+ * same TTL already governs the reclaim that takes its lock away.
+ */
+type SyncLogState = 'running' | 'interrupted' | 'failed' | 'completed'
+
+function getSyncLogState(log: SyncLogData, now: number): SyncLogState {
+  switch (log.status) {
+    case 'completed':
+      return 'completed'
+    case 'failed':
+      return 'failed'
+    case 'started': {
+      const ageMs = now - new Date(log.startedAt).getTime()
+      return ageMs > CONNECTOR_SYNC_STALE_LOCK_TTL_MS ? 'interrupted' : 'running'
+    }
+    default: {
+      const exhaustive: never = log.status
+      return exhaustive
+    }
+  }
+}
+
 interface SyncHistoryProps {
  logs: SyncLogData[]
  isLoading: boolean
 }

-function SyncHistory({ logs, isLoading }: SyncHistoryProps) {
+export function SyncHistory({ logs, isLoading }: SyncHistoryProps) {
  if (isLoading) {
    return (
      <div className='flex items-center gap-2 rounded-md bg-[var(--surface-3)] px-2 py-2 text-[var(--text-muted)] text-xs'>
@@ -690,20 +726,23 @@ function SyncHistory({ logs, isLoading }: SyncHistoryProps) {
    )
  }

+  const now = Date.now()
+
  return (
    <div className='flex flex-col gap-0.5'>
      {logs.map((log) => {
-        const isError = log.status === 'error' || log.status === 'failed'
-        const isRunning = log.status === 'running' || log.status === 'syncing'
+        const state = getSyncLogState(log, now)
        const totalChanges =
          log.docsAdded + log.docsUpdated + log.docsDeleted + (log.docsFailed ?? 0)

        return (
          <div key={log.id} className='flex items-start gap-2 rounded-md px-2 py-1.5 text-xs'>
            <div className='mt-[1px] flex-shrink-0'>
-              {isRunning ? (
+              {state === 'running' ? (
                <Loader className='size-3 text-[var(--text-muted)]' animate />
-              ) : isError ? (
+              ) : state === 'interrupted' ? (
+                <TriangleAlert className='size-3 text-[var(--caution)]' />
+              ) : state === 'failed' ? (
                <CircleX className='size-3 text-[var(--text-error)]' />
              ) : (
                <CircleCheck className='size-3 text-[var(--success)]' />
@@ -715,7 +754,7 @@ function SyncHistory({ logs, isLoading }: SyncHistoryProps) {
                <span className='text-[var(--text-muted)]'>
                  {format(new Date(log.startedAt), 'MMM d, h:mm a')}
                </span>
-                {!isRunning && !isError && (
+                {state === 'completed' && (
                  <span className='text-[var(--text-muted)]'>
                    {totalChanges > 0 ? (
                      <>
@@ -747,10 +786,15 @@ function SyncHistory({ logs, isLoading }: SyncHistoryProps) {
                    )}
                  </span>
                )}
-                {isRunning && <span className='text-[var(--text-muted)]'>In progress…</span>}
+                {state === 'running' && (
+                  <span className='text-[var(--text-muted)]'>In progress…</span>
+                )}
+                {state === 'interrupted' && (
+                  <span className='text-[var(--caution)]'>Interrupted</span>
+                )}
              </div>

-              {isError && log.errorMessage && (
+              {state === 'failed' && log.errorMessage && (
                <span className='truncate text-[var(--text-error)]'>{log.errorMessage}</span>
              )}
            </div>
@@ -72,11 +72,21 @@ export const connectorDataSchema = z
  .passthrough()
 export type ConnectorData = z.output<typeof connectorDataSchema>

+/**
+ * The complete set of sync-log statuses the sync engine writes: `started` on
+ * insert, then exactly one of `completed` / `failed` on exit. Deliberately an
+ * enum rather than a free string — a connector's own `status` values
+ * (`syncing`, `error`, …) are a different vocabulary, and typing this as
+ * `z.string()` is what let the UI branch on literals no producer ever wrote.
+ */
+export const syncLogStatusSchema = z.enum(['started', 'completed', 'failed'])
+export type SyncLogStatus = z.output<typeof syncLogStatusSchema>
+
 export const syncLogDataSchema = z
  .object({
    id: z.string(),
    connectorId: z.string(),
-    status: z.string(),
+    status: syncLogStatusSchema,
    startedAt: z.string(),
    completedAt: z.string().nullable(),
    docsAdded: z.number(),