fix(ok): de-flake pty-flood + harden OK test-tier timeout headroom#389
Merged
Conversation
…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 <file>" 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
![inkeep-internal-ci[bot]](https://avatars.githubusercontent.com/in/2806322?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated approval from agents-private public-mirror-sync (run: https://github.com/inkeep/agents-private/actions/runs/28455230545). Source of truth is the monorepo; direct edits on inkeep/open-knowledge are overwritten on next sync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.