Skip to content

Commit 6b0c69e

Browse files
waleedlatif1waleedlatif1
authored
improvement(ci): track flaky checks daily and fix the live flakes (#9038)
## Summary - Add a daily `ci-flakes` workflow (`scripts/report-ci-flakes.ts`): a job that failed on a CI attempt and passed on a re-run of the same commit files one `ci-flake` issue per failing test (Vitest `FAIL` lines and the E2E harnesses' `[Suite] FAIL` lines), or per job when its log names none. Occurrences are keyed by job id, so the overlapping 3-day lookback never double-counts, and a closed issue reopens when its flake recurs. Dependency-free, so the job needs no install - Fix the live flakes found in the last two weeks of re-runs: - GitHub rate-limit reset test: pin the clock; a second boundary between building the header and reading it gave 899000 - OAuth token and token-family integration tests: import the auth graph statically, so its cold load (up to ~43s on a slow runner) no longer counts against the first test's 30s timeout - stop-after E2E: wait for the sibling-pause claim until the resume request settles, not a fixed 2s that a slow `next dev` round trip outlasts - HTTP E2E readiness: 300s → 600s; healthy cold compiles take 60–185s and a degraded runner ran past 300s with every step 3–6x slower - Reconciliation-window integration test: print each scan's plan when a window statement reads past the bound, so its next failure names the cause (diagnostics only; the cause isn't confirmed yet) - Mobile e2e and usage-log flakes were already fixed by later commits, so nothing changes there - Testing rules: a short section on the flake workflow and how to fix one (no retries, longer timeouts, or skips)
1 parent 2e200d5 commit 6b0c69e

11 files changed

Lines changed: 806 additions & 89 deletions

File tree

‎.claude/rules/sim-testing.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,17 @@ screenshot. Write it to a caller-supplied path (`<SUITE>_REPORT_PATH`) and have
6565
disposable, seeds with SQL, exercises the real HTTP boundary, cleans up its fixtures, and writes
6666
the report.
6767

68+
## Flaky tests
69+
70+
A daily job (`.github/workflows/ci-flakes.yml`) files a `ci-flake` issue for every test or job
71+
that failed on a CI attempt and passed when the same commit was re-run, and reopens it if the
72+
flake recurs after it was closed. Fix the nondeterminism it names: pin the clock
73+
(`vi.useFakeTimers({ toFake: ['Date'] })` + `vi.setSystemTime`), give each test its own ids and
74+
rows, wait on the condition rather than a delay, and keep cold imports out of a test's timeout by
75+
importing statically. Never add a retry, a skip, or a longer timeout to make one pass; raise a
76+
budget only when its run history shows the budget itself is wrong, and record that evidence
77+
beside it.
78+
6879
## Unit test mechanics
6980

7081
### Structure

‎.cursor/rules/sim-testing.mdc‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,17 @@ screenshot. Write it to a caller-supplied path (`<SUITE>_REPORT_PATH`) and have
6363
disposable, seeds with SQL, exercises the real HTTP boundary, cleans up its fixtures, and writes
6464
the report.
6565

66+
## Flaky tests
67+
68+
A daily job (`.github/workflows/ci-flakes.yml`) files a `ci-flake` issue for every test or job
69+
that failed on a CI attempt and passed when the same commit was re-run, and reopens it if the
70+
flake recurs after it was closed. Fix the nondeterminism it names: pin the clock
71+
(`vi.useFakeTimers({ toFake: ['Date'] })` + `vi.setSystemTime`), give each test its own ids and
72+
rows, wait on the condition rather than a delay, and keep cold imports out of a test's timeout by
73+
importing statically. Never add a retry, a skip, or a longer timeout to make one pass; raise a
74+
budget only when its run history shows the budget itself is wrong, and record that evidence
75+
beside it.
76+
6677
## Unit test mechanics
6778

6879
### Structure

‎.github/scripts/http-e2e.sh‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@
88
# The job provides DATABASE_URL, BETTER_AUTH_SECRET and ENCRYPTION_KEY; each group sets the rest of
99
# its app's environment here. Reports and server logs land in $RUNNER_TEMP/e2e.
1010
#
11-
# Under `next dev` the first request cold-compiles the app under Turbopack, which takes 42-150s on
12-
# CI runners, so the readiness deadline only has to catch a hung boot: an exited server fails
13-
# immediately, and either way the server log tail lands in the job log.
11+
# Under `next dev` the first request cold-compiles the app under Turbopack, which takes 60-185s on
12+
# a healthy CI runner and has run past 300s on a degraded one. The readiness deadline, about three
13+
# times the healthy worst case, only has to catch a hung boot: an exited server fails immediately,
14+
# and either way the server log tail lands in the job log.
1415
#
1516
# Each app starts from an empty Turbopack dev cache: a cache written under other NEXT_PUBLIC_*
1617
# values, by a server that `next dev` SIGKILLs 100ms after SIGTERM, can panic Turbopack or wedge a
@@ -21,7 +22,7 @@ set -euo pipefail
2122

2223
group=${1:?usage: http-e2e.sh <scim|cli|stop-after|desktop-inbox|mobile>}
2324
report_dir="$RUNNER_TEMP/e2e"
24-
ready_timeout_seconds=300
25+
ready_timeout_seconds=600
2526
mkdir -p "$report_dir"
2627

2728
server_pid=''

‎.github/workflows/ci-flakes.yml‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
name: ci-flakes
2+
3+
# Files one `ci-flake` issue per CI check that failed and then passed on a re-run of the same
4+
# commit, and reopens a closed one when its flake recurs (scripts/report-ci-flakes.ts). The
5+
# three-day lookback overlaps the daily schedule on purpose: the script skips occurrences an issue
6+
# already lists, so a missed or repeated run neither drops nor double-counts one.
7+
8+
on:
9+
schedule:
10+
- cron: '23 7 * * *'
11+
workflow_dispatch:
12+
inputs:
13+
days:
14+
description: Look back this many days of CI runs (the script's default is 3).
15+
required: false
16+
dry_run:
17+
description: Print the issues it would file or update without changing them.
18+
type: boolean
19+
required: false
20+
default: false
21+
22+
permissions:
23+
actions: read
24+
contents: read
25+
issues: write
26+
27+
concurrency:
28+
group: ci-flakes
29+
cancel-in-progress: false
30+
31+
jobs:
32+
report:
33+
name: report
34+
# GitHub-hosted: a pure API client with no dependencies, so it needs no install or cache.
35+
runs-on: ubuntu-latest
36+
timeout-minutes: 15
37+
steps:
38+
- name: Checkout code
39+
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v6
40+
with:
41+
persist-credentials: false
42+
- name: Setup Bun
43+
uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
44+
with:
45+
bun-version: 1.4.2
46+
- name: Report flaky checks
47+
env:
48+
GITHUB_TOKEN: ${{ github.token }}
49+
DAYS: ${{ inputs.days }}
50+
DRY_RUN: ${{ inputs.dry_run }}
51+
run: |
52+
args=()
53+
if [ -n "$DAYS" ]; then args+=(--days "$DAYS"); fi
54+
if [ "$DRY_RUN" = true ]; then args+=(--dry-run); fi
55+
bun scripts/report-ci-flakes.ts "${args[@]}"

‎apps/sim/app/api/auth/oauth2/token/route.integration.ts‎

Lines changed: 22 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,30 @@
11
import { randomBytes } from 'node:crypto'
2+
import { db } from '@sim/db'
3+
import * as schema from '@sim/db/schema'
24
import { envFlagsMock } from '@sim/testing/mocks/env-flags.mock'
5+
import { requestUtilsMockFns } from '@sim/testing/mocks/request.mock'
36
import { generateId } from '@sim/utils/id'
7+
import { makeSignature } from 'better-auth/crypto'
8+
import { eq, inArray, like, sql } from 'drizzle-orm'
49
import { NextRequest } from 'next/server'
510
import { describe, expect, it, vi } from 'vitest'
6-
7-
/** Every request below targets this origin, so Better Auth must issue its cookies for it. */
8-
vi.hoisted(() => {
11+
import { auth } from '@/lib/auth'
12+
import * as tokenStore from '@/lib/auth/oauth-access-token'
13+
import * as provider from '@/lib/auth/oauth-provider'
14+
import { isCapabilityWithheldForUser } from '@/lib/permission-groups/user-scope.server'
15+
import { POST as revokeRoute } from '@/app/api/auth/oauth2/revoke/route'
16+
import { POST as tokenRoute } from '@/app/api/auth/oauth2/token/route'
17+
18+
/**
19+
* Every request below targets this origin, so Better Auth must issue its cookies for it, and
20+
* signs them with this secret. Both are set before the imports above evaluate: the auth graph is
21+
* imported statically so its cold load runs at collection, not against the first test's timeout.
22+
*/
23+
const authSecret = vi.hoisted(() => {
24+
const secret = 'test-secret-that-is-at-least-32-chars-long'
925
process.env.NEXT_PUBLIC_APP_URL = 'https://test.sim.ai'
26+
process.env.BETTER_AUTH_SECRET = secret
27+
return secret
1028
})
1129
/** Each case resolves its requests to its own client IP so the per-IP token bucket applies. */
1230
vi.mock('@/lib/core/utils/request', async () => (await import('@sim/testing')).requestUtilsMock)
@@ -26,27 +44,6 @@ describe('OAuth token route in PostgreSQL', () => {
2644
it.each(['http://127.0.0.1:48881/callback', 'cursor://anysphere.cursor-mcp/oauth/callback'])(
2745
'persists Search audiences through native PKCE issuance and refresh with %s',
2846
async (redirectUri) => {
29-
const authSecret = 'test-secret-that-is-at-least-32-chars-long'
30-
process.env.BETTER_AUTH_SECRET = authSecret
31-
const [
32-
{ db },
33-
schema,
34-
{ eq, like, sql },
35-
{ makeSignature },
36-
{ auth },
37-
{ POST: tokenRoute },
38-
tokenStore,
39-
{ requestUtilsMockFns },
40-
] = await Promise.all([
41-
import('@sim/db'),
42-
import('@sim/db/schema'),
43-
import('drizzle-orm'),
44-
import('better-auth/crypto'),
45-
import('@/lib/auth'),
46-
import('@/app/api/auth/oauth2/token/route'),
47-
import('@/lib/auth/oauth-access-token'),
48-
import('@sim/testing/mocks/request.mock'),
49-
])
5047
const POST = (request: NextRequest) => tokenRoute(request, {})
5148
/** A fresh TEST-NET-2 address per case, so reruns never draw on an earlier run's bucket. */
5249
const clientIp = `198.51.100.${(randomBytes(1)[0] % 254) + 1}`
@@ -264,35 +261,7 @@ describe('OAuth token route in PostgreSQL', () => {
264261
)
265262

266263
it('issues, rotates, contains replay, and revokes a real Better Auth PKCE grant', async () => {
267-
const authSecret = 'test-secret-that-is-at-least-32-chars-long'
268-
process.env.BETTER_AUTH_SECRET = authSecret
269-
270-
const [
271-
{ db },
272-
schema,
273-
{ eq, inArray, like },
274-
{ makeSignature },
275-
{ auth },
276-
{ POST: exchangeRoute },
277-
{ POST: revokeRoute },
278-
tokenStore,
279-
provider,
280-
{ requestUtilsMockFns },
281-
{ isCapabilityWithheldForUser },
282-
] = await Promise.all([
283-
import('@sim/db'),
284-
import('@sim/db/schema'),
285-
import('drizzle-orm'),
286-
import('better-auth/crypto'),
287-
import('@/lib/auth'),
288-
import('@/app/api/auth/oauth2/token/route'),
289-
import('@/app/api/auth/oauth2/revoke/route'),
290-
import('@/lib/auth/oauth-access-token'),
291-
import('@/lib/auth/oauth-provider'),
292-
import('@sim/testing/mocks/request.mock'),
293-
import('@/lib/permission-groups/user-scope.server'),
294-
])
295-
const exchangeToken = (request: NextRequest) => exchangeRoute(request, {})
264+
const exchangeToken = (request: NextRequest) => tokenRoute(request, {})
296265
const revokeToken = (request: NextRequest) => revokeRoute(request, {})
297266

298267
const testId = randomBytes(8).toString('hex')

‎apps/sim/lib/auth/oauth-token-family.integration.ts‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,22 @@
11
import { randomBytes } from 'node:crypto'
2-
import { describe, expect, it } from 'vitest'
2+
import { db } from '@sim/db'
3+
import * as schema from '@sim/db/schema'
4+
import { eq, sql } from 'drizzle-orm'
5+
import { describe, expect, it, vi } from 'vitest'
6+
import * as tokenStore from '@/lib/auth/oauth-access-token'
7+
import * as provider from '@/lib/auth/oauth-provider'
8+
import { revokeOAuthToken, rotateOAuthRefreshToken } from '@/lib/auth/oauth-token-family'
9+
10+
/**
11+
* Set before the imports above evaluate: they load the auth graph at collection, not against the
12+
* test's timeout.
13+
*/
14+
vi.hoisted(() => {
15+
process.env.BETTER_AUTH_SECRET ||= 'oauth-token-family-integration-test-secret'
16+
})
317

418
describe('OAuth token families in PostgreSQL', () => {
519
it('rotates, contains replay, revokes one login, and follows consent changes', async () => {
6-
process.env.BETTER_AUTH_SECRET ||= 'oauth-token-family-integration-test-secret'
7-
8-
const [{ db }, schema, { eq, sql }, provider, tokenStore] = await Promise.all([
9-
import('@sim/db'),
10-
import('@sim/db/schema'),
11-
import('drizzle-orm'),
12-
import('@/lib/auth/oauth-provider'),
13-
import('@/lib/auth/oauth-access-token'),
14-
])
15-
const { revokeOAuthToken, rotateOAuthRefreshToken } = await import(
16-
'@/lib/auth/oauth-token-family'
17-
)
1820
const testId = randomBytes(8).toString('hex')
1921
const userId = `oauth-family-test-user-${testId}`
2022
const sessionId = `oauth-family-test-session-${testId}`

‎apps/sim/lib/knowledge/__integration__/listing-continuation.integration.ts‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,7 +1008,10 @@ describe('durable source and member cycles in PostgreSQL', () => {
10081008
hardDelete.mockRestore()
10091009
}
10101010
}
1011-
/** Plans a window statement from freshly analyzed statistics, returning the document rows it read. */
1011+
/**
1012+
* Plans a window statement from freshly analyzed statistics, returning the document rows it
1013+
* read and the scans that read them, so a statement over the bound names the plan behind it.
1014+
*/
10121015
const explainWalk = async (query: string, params: Parameters<typeof db.$client.unsafe>[1]) => {
10131016
const [explained] = await db.$client.begin(async (tx) => {
10141017
await tx.unsafe('ANALYZE document')
@@ -1018,17 +1021,16 @@ describe('durable source and member cycles in PostgreSQL', () => {
10181021
const plan = toRecord(node)
10191022
return [plan, ...toArray(plan.Plans).flatMap(nodes)]
10201023
}
1021-
const plan = nodes(toRecord(toArray(explained['QUERY PLAN'])[0]).Plan)
1022-
return plan
1024+
const scans = nodes(toRecord(toArray(explained['QUERY PLAN'])[0]).Plan)
10231025
.filter((node) => node['Relation Name'] === 'document')
1024-
.reduce(
1025-
(total, node) =>
1026-
total +
1026+
.map((node) => ({
1027+
scan: `${node['Node Type']}${node['Index Name'] ? ` on ${node['Index Name']}` : ''}`,
1028+
read:
10271029
((toNumberOrNull(node['Actual Rows']) ?? 0) +
10281030
(toNumberOrNull(node['Rows Removed by Filter']) ?? 0)) *
1029-
(toNumberOrNull(node['Actual Loops']) ?? 1),
1030-
0
1031-
)
1031+
(toNumberOrNull(node['Actual Loops']) ?? 1),
1032+
}))
1033+
return { read: scans.reduce((total, { read }) => total + read, 0), scans }
10321034
}
10331035
/**
10341036
* Every window statement reads at most one window of documents, whichever connector index
@@ -1040,7 +1042,8 @@ describe('durable source and member cycles in PostgreSQL', () => {
10401042
) => {
10411043
expect(walked.length).toBeGreaterThan(0)
10421044
for (const { query, params } of walked) {
1043-
expect(await explainWalk(query, params), query).toBeLessThanOrEqual(
1045+
const { read, scans } = await explainWalk(query, params)
1046+
expect(read, `${query}\n${JSON.stringify(scans, null, 2)}`).toBeLessThanOrEqual(
10441047
RECONCILIATION_WINDOW_SIZE
10451048
)
10461049
}

‎apps/sim/lib/knowledge/documents/utils.test.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -398,12 +398,15 @@ describe('fetchWithRetry rate-limit handling', () => {
398398
})
399399

400400
it('does not retry GitHub before a reset beyond the operation budget', async () => {
401+
vi.useFakeTimers({ toFake: ['Date'] })
402+
vi.setSystemTime(new Date('2026-10-14T12:00:00.000Z'))
403+
const nowSeconds = Date.now() / 1000
401404
const fetchMock = vi
402405
.fn()
403406
.mockResolvedValueOnce(
404407
response(403, {
405408
'x-ratelimit-remaining': '0',
406-
'x-ratelimit-reset': String(Math.floor(Date.now() / 1000) + 900),
409+
'x-ratelimit-reset': String(nowSeconds + 900),
407410
})
408411
)
409412
.mockResolvedValueOnce(response(200))
@@ -415,8 +418,7 @@ describe('fetchWithRetry rate-limit handling', () => {
415418
)
416419

417420
expect(error?.message).toBe('HTTP 403 - upstream rate limit exceeded')
418-
expect(getRetryAfterMs(error)).toBeGreaterThan(899_000)
419-
expect(getRetryAfterMs(error)).toBeLessThanOrEqual(900_000)
421+
expect(getRetryAfterMs(error)).toBe(900_000)
420422
expect(fetchMock).toHaveBeenCalledTimes(1)
421423
})
422424

‎apps/sim/scripts/test-workflow-stop-after-e2e.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -602,16 +602,26 @@ try {
602602
)
603603
assert(siblingContext)
604604
const completion = resume(siblingPauses, paused)
605+
/**
606+
* The claim is held for as long as the resume runs, so the wait ends with the request rather
607+
* than a fixed window: under `next dev` a request can sit seconds before reaching the route.
608+
*/
609+
let resumeSettled = false
610+
completion.then(
611+
() => {
612+
resumeSettled = true
613+
},
614+
() => {
615+
resumeSettled = true
616+
}
617+
)
605618
try {
606619
let claimed = false
607-
for (let attempt = 0; attempt < 100; attempt++) {
620+
while (!claimed && !resumeSettled) {
608621
const [{ count }] =
609622
await sql`select count(*)::int as count from resume_queue where parent_execution_id = ${paused.runId} and status = 'claimed'`
610-
if (count === 1) {
611-
claimed = true
612-
break
613-
}
614-
await sleep(20)
623+
claimed = count === 1
624+
if (!claimed) await sleep(20)
615625
}
616626
assert(claimed, 'The first resume must own the run before its sibling is queued')
617627
await execute(

0 commit comments

Comments
 (0)