mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
* fix(tables): retry transient DB/Redis failures in cell execution and surface error causes Workflow-group-cell runs intermittently failed on trivial DB reads/writes under heavy fan-out, stranding cells in `running`. Investigation showed the PlanetScale and ElastiCache backends were healthy at the time — the failures are transient connection-level faults that the cell (maxAttempts: 1) had no tolerance for, and the real cause was never logged (Drizzle wraps it as "Failed query: ..." and the driver cause lives in error.cause). Resilience: - Add retryTransient (lib/table/retry-transient.ts): retries only transient infra errors (reuses isRetryableInfrastructureError; adds an ioredis command-timeout match) with jittered backoff, then rethrows. Fail-fast for everything else. - Wrap the cell's getTableById/getRowById reads, the terminal write (cell-write updateRow — idempotent via the executionId guard), and the Redis cascade-lock acquire. Diagnostics: - Add describeError (lib/core/errors/retryable-infrastructure.ts): walks the .cause chain and always returns the underlying driver cause (code/errno/ syscall + causeChain), including for unclassified errors like AbortError. - Log `cause` + a `retryable` flag (and aborted/timedOut in the cell's main catch) across the cell + finalization error paths, mirroring the existing schedule-execution pattern. Logging-only; no behavior change. This lets the next recurrence reveal the real cause and whether the retry applies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tables): address review feedback on cell retry resilience - retryTransient: re-check the abort signal after the backoff sleep so a cancellation during sleep stops the next attempt (don't run/return work for an already-cancelled task). - isRetryableRedisError: walk the .cause chain (mirroring the infra classifier) so wrapped Redis timeouts are recognized; drop "Connection is in subscriber mode" — that's a connection-state programming error, not a transient drop, and would just fail identically every retry. - cascade-lock: stop wrapping acquireLock in retryTransient. acquireLock is a non-idempotent SET NX, so retrying after a timed-out-but-applied first SET returns false (key already ours) and yields a false `contended` that skips the cascade. A transient Redis blip here just fails the run before pickup (no stranded cell); the dispatcher re-drives it. - Tests: cause-chain Redis match, subscriber-mode exclusion, abort-during-sleep. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(tables): drop out-of-scope abort/timeout fields from cell catch The main catch logged `aborted`/`timedOut` from `abortSignal`/`timeoutController`, but those are declared inside the outer try block (the inner try around executeWorkflow is try/finally, so this catch belongs to the outer try) and are not in scope in the catch — `next build`'s type-check failed with "Cannot find name 'abortSignal'". Local incremental `tsc --noEmit` had skipped the file and falsely passed; the Cursor/Greptile reviewers flagged this correctly. Removed the two fields. Abort/timeout is still surfaced via `cause: describeError(err)` (an aborted run shows `name: 'AbortError'` / the timeout message), so no diagnostic signal is lost. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(tables): drop in-process retry, keep cause diagnostics only In-process retry is the wrong layer for this path: the cell task is maxAttempts:1 by design, retrying on a possibly-degraded worker may not help, and it masks the very transient-failure signal we're trying to capture before we understand the root cause. Removed retryTransient entirely (file + all wrapping in cell-write, the cascade reads, and the lock acquire) and kept only the diagnostic logging. - Deleted lib/table/retry-transient.ts (+ test); cell-write and the cascade reads call getTableById/getRowById/updateRow directly again, fail-fast. - Kept describeError + `cause`/`retryable` fields across the cell + finalization catch blocks; the cell-path `retryable` flag now sources from isRetryableInfrastructureError (the canonical classifier) for consistency. Diagnostics-first: surface the real driver cause on the next recurrence, then decide the actual fix (e.g. task-level maxAttempts, or addressing the worker- side cause) from evidence rather than a speculative in-process retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(schedules): log error cause on scheduled-execution failure paths The scheduled-job failure paths logged the raw error (.message/stack only) — its `.cause` (the real driver error behind a Drizzle "Failed query: ..." wrapper) was never recorded, and the classified-only `describeRetryableInfrastructureError` returns undefined for unrecognized errors. A real failed run (same incident window as the cell failures) failed in `applyScheduleUpdate` with exactly this unrecorded cause. Added `cause: describeError(error)` (always-on, walks the cause chain) to the applyScheduleUpdate catch, the early-failure catch, and the unhandled-error catch — passed as a second arg so the existing message+stack still emit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(errors): move describeError to @sim/utils/errors `describeError` is a general-purpose error/cause-chain helper — it didn't belong in `lib/core/errors/retryable-infrastructure.ts` (that module is specifically about classifying retryable infra errors, and the name read wrong for a generic diagnostic). Moved it to `@sim/utils/errors` alongside `toError`/ `getErrorMessage`/`getPostgresErrorCode`, with its own cycle-safe cause walk. - Added describeError + DescribedError + tests to packages/utils/src/errors.ts. - Reverted the describeError addition from retryable-infrastructure.ts (it keeps only isRetryableInfrastructureError / describeRetryableInfrastructureError, which are accurately named and still used by the schedule retry path). - Re-pointed all consumers (cell, logging-session, pause-persistence, schedule) to import describeError from @sim/utils/errors. The `retryable` classification flag still sources from isRetryableInfrastructureError where used. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
130 lines
4.2 KiB
TypeScript
130 lines
4.2 KiB
TypeScript
/**
|
|
* @vitest-environment node
|
|
*/
|
|
import { describe, expect, it } from 'vitest'
|
|
import { describeError, getPostgresErrorCode, toError } from './errors.js'
|
|
|
|
describe('toError', () => {
|
|
it('returns the same Error when given an Error', () => {
|
|
const err = new Error('test')
|
|
expect(toError(err)).toBe(err)
|
|
})
|
|
|
|
it('wraps a string into an Error', () => {
|
|
const err = toError('msg')
|
|
expect(err).toBeInstanceOf(Error)
|
|
expect(err.message).toBe('msg')
|
|
})
|
|
|
|
it('wraps a number into an Error', () => {
|
|
const err = toError(42)
|
|
expect(err).toBeInstanceOf(Error)
|
|
expect(err.message).toBe('42')
|
|
})
|
|
|
|
it('wraps null into an Error', () => {
|
|
const err = toError(null)
|
|
expect(err).toBeInstanceOf(Error)
|
|
expect(err.message).toBe('null')
|
|
})
|
|
|
|
it('wraps undefined into an Error', () => {
|
|
const err = toError(undefined)
|
|
expect(err).toBeInstanceOf(Error)
|
|
expect(err.message).toBe('undefined')
|
|
})
|
|
})
|
|
|
|
describe('getPostgresErrorCode', () => {
|
|
it('reads code from Error.code', () => {
|
|
const err = new Error('fail') as Error & { code: string }
|
|
err.code = '23505'
|
|
expect(getPostgresErrorCode(err)).toBe('23505')
|
|
})
|
|
|
|
it('reads code from plain object', () => {
|
|
expect(getPostgresErrorCode({ code: '23505' })).toBe('23505')
|
|
})
|
|
|
|
it('reads code from Error.cause', () => {
|
|
const err = new Error('fail', { cause: { code: '23505' } })
|
|
expect(getPostgresErrorCode(err)).toBe('23505')
|
|
})
|
|
|
|
it('walks nested Error causes', () => {
|
|
const pgErr = new Error('unique_violation') as Error & { code: string }
|
|
pgErr.code = '23505'
|
|
const err = new Error('outer', { cause: new Error('inner', { cause: pgErr }) })
|
|
expect(getPostgresErrorCode(err)).toBe('23505')
|
|
})
|
|
|
|
it('returns undefined for non-errors', () => {
|
|
expect(getPostgresErrorCode(undefined)).toBeUndefined()
|
|
expect(getPostgresErrorCode(null)).toBeUndefined()
|
|
expect(getPostgresErrorCode('23505')).toBeUndefined()
|
|
})
|
|
|
|
it('returns undefined when no code is present', () => {
|
|
expect(getPostgresErrorCode(new Error('no code'))).toBeUndefined()
|
|
})
|
|
|
|
it('does not loop forever on circular cause chains', () => {
|
|
const err1 = new Error('a')
|
|
const err2 = new Error('b', { cause: err1 })
|
|
// Create circular reference
|
|
;(err1 as { cause?: unknown }).cause = err2
|
|
expect(getPostgresErrorCode(err1)).toBeUndefined()
|
|
})
|
|
})
|
|
|
|
describe('describeError', () => {
|
|
it('reports name and message for a plain error, omitting causeChain', () => {
|
|
const described = describeError(new Error('boom'))
|
|
expect(described).toEqual({ name: 'Error', message: 'boom' })
|
|
expect(described.causeChain).toBeUndefined()
|
|
})
|
|
|
|
it('surfaces the deepest cause for a wrapped driver error', () => {
|
|
const driver = Object.assign(new Error('read ECONNRESET'), {
|
|
code: 'ECONNRESET',
|
|
errno: 'ECONNRESET',
|
|
syscall: 'read',
|
|
})
|
|
const wrapped = new Error('Failed query: select ...', { cause: driver })
|
|
const described = describeError(wrapped)
|
|
expect(described.message).toBe('read ECONNRESET')
|
|
expect(described.code).toBe('ECONNRESET')
|
|
expect(described.errno).toBe('ECONNRESET')
|
|
expect(described.syscall).toBe('read')
|
|
expect(described.causeChain).toEqual([
|
|
'Error: Failed query: select ...',
|
|
'Error: read ECONNRESET',
|
|
])
|
|
})
|
|
|
|
it('always returns the cause for unclassified errors (AbortError)', () => {
|
|
const aborted = Object.assign(new Error('The operation was aborted'), { name: 'AbortError' })
|
|
expect(describeError(aborted)).toEqual({
|
|
name: 'AbortError',
|
|
message: 'The operation was aborted',
|
|
})
|
|
})
|
|
|
|
it('falls back to a populated description for non-Error input without throwing', () => {
|
|
expect(describeError('just a string')).toEqual({ name: 'Error', message: 'just a string' })
|
|
expect(() => describeError({ weird: true })).not.toThrow()
|
|
})
|
|
|
|
it('stops at depth 10 and does not loop on a cyclic cause', () => {
|
|
const a = new Error('a')
|
|
const b = new Error('b')
|
|
;(a as { cause?: unknown }).cause = b
|
|
;(b as { cause?: unknown }).cause = a
|
|
let described: ReturnType<typeof describeError> | undefined
|
|
expect(() => {
|
|
described = describeError(a)
|
|
}).not.toThrow()
|
|
expect(described?.causeChain?.length).toBeLessThanOrEqual(10)
|
|
})
|
|
})
|