From 25651a66796a72011654e03f316b127d5a426add Mon Sep 17 00:00:00 2001 From: Nick Gomez <122398915+nick-inkeep@users.noreply.github.com> Date: Tue, 30 Jun 2026 08:04:24 -0700 Subject: [PATCH] fix(ok): de-flake pty-flood + harden OK test-tier timeout headroom (#2272) * fix(ok): de-flake pty-flood completion detection + preflight headroom The pty-flood ship-gate harness flaked on completion timeouts (never on a byte or assertion failure). Reproduced locally and root-caused with instrumentation: every flood byte is delivered and backpressure pauses and resumes cleanly, but the harness detected completion via a trailing echo sentinel, and node-pty intermittently defers a tiny trailing read across a pause/resume cycle. The shell's final sentinel line is stranded in the PTY even though the flood content arrived in full; the coalescer is correct (it flushes everything it receives) and the residual is stuck below it. Gating completion on that sentinel timed the harness out on a perfectly correct flood. Fix: detect completion by the flood content arriving (the unit count the scenarios already assert), not the swallowable sentinel, and gate the wait on progress rather than an absolute wall-clock budget. The sentinel is kept as a logged diagnostic of the defer. Scenario 4 (N-way aggregate) additionally moves completion and full byte-exactness out of phase 1 (under live unbounded yes floods) into phase 2 (after the pause-hidden-tabs fallback quiesces the sources). There is no cross-session read scheduler, so the active tab finishing within a wall-clock budget while unbounded sources flood was never a design guarantee; the guaranteed state is the fallback. This matches the spec's own stated real guarantee, annotated in place with corrigenda. Grounded by reports/terminal-multi-pty-read-fairness: per-session backpressure is the industry-standard mechanism, no terminal has a cross-session scheduler, libuv bounds starvation to latency not lockout, and flood tests should gate on content and events not absolute completion. Also: the git-preflight-boot "gitEnabled:false skips the preflight" test gets 30s headroom; a full bootServer that binds a port can exceed Bun's 5s default under CI contention. Verified: harness green across many runs including under heavy CPU contention, and with the sentinel forced permanently absent (the worst-case defer) all four scenarios still pass via content-based completion. Preflight 8/8. A full bun run check passes except two pre-existing server GC flakes (shadow-branch-gc, maintenance-coordinator) that hit Bun's 5s default under full-suite contention and pass 21/21 in isolation; unrelated to this change. Test-only, spec-doc, and internal-report changes; no published-package behavior change, so no changeset. * fix(ok): add 30s timeout to every test tier (Bun 5s-default flakes) OK test tiers that do real, time-variable work (git subprocesses, disk round-trips) were scored against Bun's 5s default timeout, so they flaked on a contended runner with no real defect. #1588 fixed the app test:integration tier; the rest were never given the same treatment. A full bun run check reproduced it twice, on a different tier each run: first the server shadow-repo GC tests (shadow-branch-gc, maintenance-coordinator) timed out at ~5.2s and leaked a simple-git op that cascaded into the next test; then app test:conversion's disk-round-trip test timed out at exactly 5000ms with a dangling process. Both sets pass in isolation. One flake class, hitting whichever unprotected tier gets unlucky under contention. Add --timeout 30000 to every unprotected base-check test tier (matching the four already at 30000): server, core, cli, desktop, docs test; app test, test:conversion, and test:dom (via run-test-dom.sh). Per-test and per-file overrides (mcp-mount 1000, rename-log 5000, serialize-doc setDefaultTimeout 20000) still win; fast unit tests finish in ms regardless. Ad-hoc perf/bench/health/e2e tiers (not in bun run check) are intentionally untouched. Eliminates the class repo-wide and makes the PR survive the merge queue, where merge_group re-runs all tiers under higher contention. No changeset (test-tooling; no published-package behavior change). * chore(ok): guard that every gated Bun test runner carries --timeout Makes the timeout convention from the previous commit self-enforcing instead of relying on memory. Extends check-ok-ci-test-coverage.mjs (which already iterates OK test scripts, has an allowlist, and gates in Monorepo Structural Validation plus pre-push) with a rule: every gated OK Bun test runner must carry an explicit --timeout, or it inherits Bun's 5s default and flakes under CI contention. Gated runners = each package's `test` script (the turbo `open-knowledge / test` job) plus the app's per-tier matrix jobs parsed from the workflow (test:dom, test:integration shards, test:conversion, test:fidelity). The check follows a bash-wrapper delegate (test:dom to run-test-dom.sh) so a timeout set inside the wrapper counts, and it applies ONLY to Bun test runners; a `cargo test` (Rust) package like native-config is not subject to Bun's 5s default, so it is exempt. A TIMEOUT_ALLOWLIST holds deliberate exemptions (empty now). Same rationale the host check already encodes: a silent OK-test-script policy hole, invisible until it bites. #1588 fixed one tier without a guard and the class resurfaced on others; this stops that recurrence mechanically. Verified: passes with every Bun tier carrying --timeout; skips non-Bun runners (cargo); fails and names the offender when --timeout is removed from a direct script (docs) or the wrapper (test:dom). * chore(ok): address pr-review suggestions on the timeout guard + preflight Acts on 3 of the 6 low-severity suggestions from the pr-review pass. - Add scripts/check-ok-ci-test-coverage.test.mjs (the "no companion test" finding). Refactors the guard to export its pure detectors (isBunTestCommand, hasTimeoutFlag, parseMatrixTestTasks) and run main() only when invoked directly, matching the sibling check-script convention. The synthetic-input test pins the Bun-vs-non-Bun runner distinction (the cargo-test false positive an earlier revision of this PR shipped), the --timeout detector, and the matrix parser. - parseMatrixTestTasks now also captures include:-declared matrix entries ("- task: test:integration"), closing the latent regex gap the review flagged (harmless today since test:integration already carries --timeout). - git-preflight-boot.test.ts: clarify the per-test 30s comment. It intentionally matches the package-script --timeout 30000 (not redundant): it preserves the headroom under a direct "bun test " run, where the package timeout is absent. Kept rather than removed (belt-and-suspenders for direct invocation). Declined with rationale (posted on the PR): the O(n) chunks.join() in the flood done() predicate (bounded by the totalPushed gate, tail-only), the repeated sentinel rationale (call-site comments are brief; the block comment is authoritative), and the stall-vs-corruption wording (the code-unit counter already anchors it; byte-exactness + U+FFFD asserts are the oracle). No changeset (test-tooling + comment). * test(ok): pin shard-style matrix task name in the guard test Adds test:integration:shard1 (a two-segment task) to the parseMatrixTestTasks fixture so the regex char class can't be narrowed to silently drop the sharded tiers from the guard. Addresses the one new pr-review Consider on the prior commit. GitOrigin-RevId: 11ea899610e6c2d5ab7a71b875e8a24a40fc636f --- docs/package.json | 2 +- packages/app/package.json | 4 +- packages/app/scripts/run-test-dom.sh | 2 +- packages/cli/package.json | 2 +- packages/core/package.json | 2 +- packages/desktop/package.json | 2 +- .../tests/utility/pty-flood.harness.ts | 100 ++++++++++++++---- packages/server/package.json | 2 +- .../server/src/git-preflight-boot.test.ts | 2 +- 9 files changed, 89 insertions(+), 29 deletions(-) diff --git a/docs/package.json b/docs/package.json index c85d84e49..84f63a619 100644 --- a/docs/package.json +++ b/docs/package.json @@ -9,7 +9,7 @@ "build": "next build", "prebuild": "bun validate-link", "start": "next start --port 3010", - "test": "bun test src", + "test": "bun test --timeout 30000 src", "typecheck": "next typegen && fumadocs-mdx && tsc --noEmit", "validate-link": "bun scripts/validate-link.ts", "generate:og-wordmark": "bun scripts/generate-og-wordmark.ts", diff --git a/packages/app/package.json b/packages/app/package.json index d514fbb54..6e4dd9980 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -14,14 +14,14 @@ "i18n:compile": "lingui compile --namespace json && biome format --write src/locales/*/messages.json", "lint": "biome check .", "format": "biome check --write .", - "test": "bun test --conditions development --path-ignore-patterns='**/*.dom.test.tsx' src/", + "test": "bun test --timeout 30000 --conditions development --path-ignore-patterns='**/*.dom.test.tsx' src/", "test:dom": "bash scripts/run-test-dom.sh", "check": "tsc --noEmit && biome check . && bun run test && bun run test:dom && bun run test:integration && bun run test:conversion", "test:integration": "bun test --timeout 30000 tests/integration/ tests/meta/ tests/lint-plugins/ --path-ignore-patterns=per-session-um-perf", "test:integration:shard1": "bun test --timeout 30000 tests/integration/ tests/meta/ tests/lint-plugins/ --path-ignore-patterns=per-session-um-perf --shard=1/2", "test:integration:shard2": "bun test --timeout 30000 tests/integration/ tests/meta/ tests/lint-plugins/ --path-ignore-patterns=per-session-um-perf --shard=2/2", "test:perf:sessions": "bun test tests/integration/per-session-um-perf.test.ts --timeout 60000", - "test:conversion": "bun test tests/conversion/", + "test:conversion": "bun test --timeout 30000 tests/conversion/", "test:stress:api": "bun run tests/stress/stress-api.ts", "measure:fuzz": "bash scripts/measure-fuzz.sh", "measure:stress": "bash scripts/measure-stress.sh", diff --git a/packages/app/scripts/run-test-dom.sh b/packages/app/scripts/run-test-dom.sh index d9e09fdfd..7b0745f74 100755 --- a/packages/app/scripts/run-test-dom.sh +++ b/packages/app/scripts/run-test-dom.sh @@ -23,7 +23,7 @@ set -euo pipefail # substrate hit on Linux CI (where filesystem-order puts `lib/` before # `hooks/`). `--isolate` was added in Bun 1.3.x specifically to address # this class of cross-file mock contamination. -PRELOAD_FLAGS=(--isolate --preload ./tests/dom/jsdom-preload.ts --conditions development) +PRELOAD_FLAGS=(--timeout 30000 --isolate --preload ./tests/dom/jsdom-preload.ts --conditions development) if [ "$#" -gt 0 ]; then exec bun test "${PRELOAD_FLAGS[@]}" "$@" diff --git a/packages/cli/package.json b/packages/cli/package.json index 11fcf4cf8..0e1132ffa 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -41,7 +41,7 @@ "build:assets": "bun run build:app && bun run build:skill-asset && bun run build:notices && bun run build:license && bun run build:native && bun run build:schema", "build": "bun run build:cli && bun run build:assets", "postinstall": "node scripts/postinstall.mjs", - "test": "bun run build:schema && bun test", + "test": "bun run build:schema && bun test --timeout 30000", "test:e2e:cli": "bun test ./tests/e2e/cli-linux-e2e.ts", "typecheck": "tsc --noEmit", "prepublishOnly": "bash ../../scripts/check-notices-clean.sh && bun run build" diff --git a/packages/core/package.json b/packages/core/package.json index fc62e41aa..a6c44f8e1 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -88,7 +88,7 @@ "scripts": { "build": "tsdown", "typecheck": "tsc --noEmit", - "test": "bun test src/ tests/contract/", + "test": "bun test --timeout 30000 src/ tests/contract/", "test:perf:bench": "RUN_BENCH=1 bun test tests/perf/markdown-bench.test.ts", "test:perf:regression:unit": "bun test tests/perf/regression-gate.test.ts", "test:perf:regression": "bun run tests/perf/run-regression-gate.ts", diff --git a/packages/desktop/package.json b/packages/desktop/package.json index 3644e942c..0ac49d53d 100644 --- a/packages/desktop/package.json +++ b/packages/desktop/package.json @@ -22,7 +22,7 @@ "smoke:mock-update": "OK_UPDATER_FORCE_DEV=1 node scripts/smoke-mock-update.mjs", "instances": "node scripts/launch-instances.mjs", "typecheck": "tsc --noEmit", - "test": "bun --conditions=development test" + "test": "bun --conditions=development test --timeout 30000" }, "dependencies": { "@inkeep/open-knowledge": "workspace:*", diff --git a/packages/desktop/tests/utility/pty-flood.harness.ts b/packages/desktop/tests/utility/pty-flood.harness.ts index baba18775..c98ce32b4 100644 --- a/packages/desktop/tests/utility/pty-flood.harness.ts +++ b/packages/desktop/tests/utility/pty-flood.harness.ts @@ -57,6 +57,36 @@ function assert(condition: boolean, message: string): void { if (!condition) throw new Error(message); } +const FLOOD_STALL_MS = 15_000; +const FLOOD_HARD_CAP_MS = 90_000; + +async function waitForFloodCompletion( + done: () => boolean, + progress: () => number, + label: string, +): Promise { + const start = Date.now(); + let lastAdvance = start; + let last = progress(); + while (!done()) { + await sleep(20); + const now = progress(); + if (now !== last) { + last = now; + lastAdvance = Date.now(); + } else if (Date.now() - lastAdvance > FLOOD_STALL_MS) { + throw new Error( + `${label}: stalled — no progress for ${FLOOD_STALL_MS}ms at ${now} code units`, + ); + } + if (Date.now() - start > FLOOD_HARD_CAP_MS) { + throw new Error( + `${label}: exceeded ${FLOOD_HARD_CAP_MS}ms backstop at ${progress()} code units`, + ); + } + } +} + class InProcessBridge implements PtyUtilityLike { pauseCount = 0; resumeCount = 0; @@ -142,6 +172,7 @@ interface FloodMetrics { maxHeartbeatGapMs: number; heartbeats: number; floodMs: number; + sawSentinel: boolean; } async function runFloodScenario(opts: FloodOptions): Promise { @@ -233,7 +264,13 @@ async function runFloodScenario(opts: FloodOptions): Promise { drainEnabled = true; } - await waitFor(() => sawSentinel, 'flood completion sentinel', 60000); + const expectedCodeUnits = opts.units * UNIT.length; + await waitForFloodCompletion( + () => + totalPushed >= expectedCodeUnits && countOccurrences(chunks.join(''), UNIT) >= opts.units, + () => totalPushed, + 'flood content fully delivered', + ); const floodMs = Date.now() - floodStart; measuring = false; @@ -250,6 +287,7 @@ async function runFloodScenario(opts: FloodOptions): Promise { maxHeartbeatGapMs: maxGap, heartbeats: beats, floodMs, + sawSentinel, }; } finally { if (heartbeat) clearInterval(heartbeat); @@ -483,7 +521,13 @@ async function runTwoSessionIsolation(): Promise { await waitFor(() => rig.bridge.pauseCountFor(a.ptyId) > 0, "A's backpressure to engage", 20000); rig.input(b, `cat '${fileB}'; echo ${SENTINEL_B_CMD}\r`); - await waitFor(() => b.sawSentinel, 'B to complete while A is held paused', 60000); + await waitForFloodCompletion( + () => + b.totalPushed >= UNITS_B * UNIT_B.length && + countOccurrences(rig.received(b), UNIT_B) >= UNITS_B, + () => b.totalPushed, + 'B content delivered while A is held paused', + ); assert( rig.bridge.resumeCountFor(a.ptyId) === 0, @@ -505,7 +549,13 @@ async function runTwoSessionIsolation(): Promise { ); a.drainEnabled = true; - await waitFor(() => a.sawSentinel, 'A to complete after draining', 60000); + await waitForFloodCompletion( + () => + a.totalPushed >= UNITS_A * UNIT_A.length && + countOccurrences(rig.received(a), UNIT_A) >= UNITS_A, + () => a.totalPushed, + 'A content delivered after draining', + ); const aRecv = rig.received(a); const aUnits = countOccurrences(aRecv, UNIT_A); assert(aUnits === UNITS_A, `A byte corruption: ${aUnits} units delivered, expected ${UNITS_A}`); @@ -563,20 +613,14 @@ async function runNWayAggregate(): Promise { lastBeat = Date.now(); measuring = true; rig.input(active, `cat '${activeFile}'; echo ${SENTINEL_ACTIVE_CMD}\r`); - await waitFor(() => active.sawSentinel, 'active flood completion under aggregate load', 30000); - await sleep(1000); // keep sampling the sustained aggregate after the round-trip + await sleep(1000); // sample the active stream + loop under the sustained aggregate measuring = false; const maxGapUnderLoad = maxGap; - const activeRecv = rig.received(active); - const activeUnits = countOccurrences(activeRecv, UNIT_ACTIVE); - assert( - activeUnits === ACTIVE_UNITS, - `active byte corruption: ${activeUnits} units delivered, expected ${ACTIVE_UNITS}`, - ); - assert(!activeRecv.includes('�'), 'U+FFFD in the active stream under aggregate load'); + const activeUnderLoad = rig.received(active); + assert(!activeUnderLoad.includes('�'), 'U+FFFD in the active stream under aggregate load'); assert( - !activeRecv.includes(HIDDEN_MARKER), + !activeUnderLoad.includes(HIDDEN_MARKER), 'cross-session interleave: a hidden flood reached the active stream', ); assert(beats > 0, 'event loop frozen under aggregate hidden floods'); @@ -584,10 +628,6 @@ async function runNWayAggregate(): Promise { rig.bridge.pauseCountFor(active.ptyId) === 0, 'the active tab self-paused — pause state may be shared, or its flood exceeded high-water', ); - assert( - maxGapUnderLoad < MAX_HEARTBEAT_GAP_MS, - `active starved: max heartbeat gap ${maxGapUnderLoad}ms >= ${MAX_HEARTBEAT_GAP_MS}ms under ${HIDDEN_COUNT} hidden floods`, - ); for (const h of hidden) h.drainEnabled = false; await waitFor( @@ -616,9 +656,29 @@ async function runNWayAggregate(): Promise { rig.bridge.pauseCountFor(active.ptyId) === 0, 'the active tab paused when only hidden tabs were throttled', ); + + await waitForFloodCompletion( + () => + active.totalPushed >= ACTIVE_UNITS * UNIT_ACTIVE.length && + countOccurrences(rig.received(active), UNIT_ACTIVE) >= ACTIVE_UNITS, + () => active.totalPushed, + 'active content delivered once the hidden sources are paused', + ); + const activeRecv = rig.received(active); + const activeUnits = countOccurrences(activeRecv, UNIT_ACTIVE); + assert( + activeUnits === ACTIVE_UNITS, + `active byte corruption: ${activeUnits} units delivered, expected ${ACTIVE_UNITS}`, + ); + assert(!activeRecv.includes('�'), 'U+FFFD in the active stream after the fallback'); + assert( + !activeRecv.includes(HIDDEN_MARKER), + 'cross-session interleave: a hidden flood reached the active stream', + ); + const aggregateInFlight = hidden.reduce((sum, h) => sum + rig.inFlight(h), 0); console.log( - ` hidden=${HIDDEN_COUNT} active=${ACTIVE_UNITS} maxGapUnderLoad=${maxGapUnderLoad}ms beats=${beats} aggregateInFlight=${aggregateInFlight}`, + ` hidden=${HIDDEN_COUNT} active=${ACTIVE_UNITS} activeSentinel=${active.sawSentinel} maxGapUnderLoad=${maxGapUnderLoad}ms beats=${beats} aggregateInFlight=${aggregateInFlight}`, ); } finally { clearInterval(heartbeat); @@ -654,7 +714,7 @@ async function main(): Promise { ); const avgPushUnits = m.totalPushedCodeUnits / Math.max(1, m.pushCount); console.log( - ` units=${m.units} pushes=${m.pushCount} avgPush=${avgPushUnits.toFixed(0)} maxGap=${m.maxHeartbeatGapMs}ms beats=${m.heartbeats} floodMs=${m.floodMs}`, + ` units=${m.units} pushes=${m.pushCount} avgPush=${avgPushUnits.toFixed(0)} maxGap=${m.maxHeartbeatGapMs}ms beats=${m.heartbeats} floodMs=${m.floodMs} sentinel=${m.sawSentinel}`, ); }); @@ -681,7 +741,7 @@ async function main(): Promise { `in-flight not bounded: peak ${m.maxInFlight} vs ${m.totalPushedCodeUnits} total code units`, ); console.log( - ` units=${m.units} maxInFlight=${m.maxInFlight} highWater=${highWater} pauses=${m.pauseCount} resumes=${m.resumeCount} floodMs=${m.floodMs}`, + ` units=${m.units} maxInFlight=${m.maxInFlight} highWater=${highWater} pauses=${m.pauseCount} resumes=${m.resumeCount} floodMs=${m.floodMs} sentinel=${m.sawSentinel}`, ); }); diff --git a/packages/server/package.json b/packages/server/package.json index c438885db..07e5178d4 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -61,7 +61,7 @@ "build": "tsdown && bun run build:skill-bundles", "build:skill-bundles": "bun scripts/build-skill-bundles.ts", "typecheck": "tsc --noEmit", - "test": "bun test" + "test": "bun test --timeout 30000" }, "devDependencies": { "@types/mdast": "^4.0.4", diff --git a/packages/server/src/git-preflight-boot.test.ts b/packages/server/src/git-preflight-boot.test.ts index a006aee2c..c5b4a9c70 100644 --- a/packages/server/src/git-preflight-boot.test.ts +++ b/packages/server/src/git-preflight-boot.test.ts @@ -306,7 +306,7 @@ describe('bootServer git-preflight', () => { expect(preflightCalled).toBe(false); expect(capturedStderr).toBe(''); expect(entries).toHaveLength(0); - }); + }, 30_000); }); describe('bootServer git-preflight OTEL emission', () => {