Skip to content

test(integration): harden pre-existing shard-5/6 CI flakes#1504

Closed
dawsontoth wants to merge 4 commits into
mainfrom
claude/fix-ci-5of6
Closed

test(integration): harden pre-existing shard-5/6 CI flakes#1504
dawsontoth wants to merge 4 commits into
mainfrom
claude/fix-ci-5of6

Conversation

@dawsontoth

Copy link
Copy Markdown
Contributor

Status: Draft — test-only hardening for two pre-existing CI failures in Integration shard 5/6. Independent of (and not stacked on) the RFC PR #1503.

Context

Integration Tests 5/6 is red on main itself — e.g. run on HEAD 572640741 fails 5/6 on Bun + Node v22/v24/v26. Neither failure is a product bug; both are test-robustness issues. This PR addresses them without touching any storage-engine code.

Changes

1. early-hints — guard the restart-deploy (fixes the Windows 5/6 failure)

The before hook called deploy_component({ restart: true }) unguarded. restart: true makes Harper restart right after accepting the deploy, so on a slow runner the HTTP response is delayed or dropped — surfacing as a fetch HeadersTimeoutError that failed the entire suite (every early-hints test) in before. The readiness polling immediately after is the real success check (it waits for /hints + seed data). Fix: tolerate a missing deploy response and let the poll be authoritative; still assert the response shape when one arrives, and the poll still fails loudly (with its own timeout) if the deploy genuinely didn't take.

2. terminology — extend the job-timeout mitigation to Node (fixes the Linux 5/6 cascade)

The async-job tests used a 30s completion timeout on Node. Under job-processor starvation — already documented in this file for Bun (bumped to 120s, "HNSW flush starves the job processor, so jobs sit IN_PROGRESS past 30s") — the csv_* / delete_* / export_* jobs on the Node shard sit IN_PROGRESS past 30s and ~10 fail in a single cascade. Extend the same mitigation to Node runtimes (90s).

The drop_database assertion ahead of those jobs (the actual RocksDB No locks available lock error) is left intact, so the genuine root-cause bug still surfaces — this only removes the downstream timeout cascade and the Windows suite-level flake.

Honesty about validation

I could not reproduce these locally: the integration harness requires ~32 sudo-configured loopback aliases (127.0.0.2127.0.0.33), which weren't available. Validated here by parse + format checks; CI is the real validator. If the Node 5/6 job tests still fail after this, that tells us the starvation is a hard hang (a server-state consequence of the failed drop_database) rather than slowness — useful signal either way.

Out of scope (flagged for the team)

The drop_database → RocksDB No locks available / "lock held by current process" error in resources/Table.ts dropTable is a real, separate root-cause bug in delicate multi-worker concurrency code. It needs a loopback-enabled local repro to fix safely and is intentionally not touched here.

🤖 Generated with Claude Code

… timeouts)

Two pre-existing shard-5/6 failures (red on main HEAD 5726407 across all
runtimes), neither a product bug — both test-robustness issues:

- early-hints: the `before` hook called deploy_component(restart: true)
  unguarded. On a slow runner the restart delays/drops the HTTP response,
  surfacing as a fetch HeadersTimeoutError that failed the whole suite — even
  though the readiness polling right after it is the real success check. Tolerate
  a missing deploy response and let the poll be authoritative.

- terminology: the async-job tests used a 30s completion timeout on Node. Under
  job-processor starvation (already documented for Bun at 120s) the csv/delete/
  export jobs sit IN_PROGRESS past 30s and ~10 fail in a cascade. Extend the same
  mitigation to Node runtimes (90s). The drop_database assertion ahead of them is
  left intact, so the separate RocksDB-lock root cause still surfaces — this only
  removes the downstream timeout cascade.

Could not reproduce locally (the integration harness requires ~32 sudo-configured
loopback aliases); validated by parse/format checks and to be confirmed in CI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request increases the job timeout limit in terminology.test.mjs from 30 to 90 seconds for non-Bun runtimes to prevent test failures caused by job-processor starvation. Additionally, in early-hints.test.ts, the component deployment operation is wrapped in a try-catch block to tolerate missing or timed-out HTTP responses during a restart, relying instead on the subsequent readiness poll. Feedback was provided to log the full error object in the catch block rather than just the error message to preserve critical debugging information.

Comment thread integrationTests/components/early-hints.test.ts
@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

No blockers — two non-critical observations posted as inline comments: (1) the Promise.all of 30–40 concurrent vector searches may amplify CI load on an already-strained runner; a small concurrency cap would preserve most of the speedup more safely; (2) the shared-component refactor in vector-index-integrity introduces an implicit test-ordering dependency (reindex must run last) that is currently met by declaration order but is not enforced — worth a brief code comment.

Address review feedback (#1504): log the full error object in the deploy catch,
not just e.message, so a genuine deploy failure (bad package, invalid operation)
remains debuggable when the readiness poll later times out.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kriszyp

kriszyp commented Jun 28, 2026

Copy link
Copy Markdown
Member

FWIW, I thought/hoped that #1428 and #1499 would address terminology test failures, but as usual, difficult to know without some more runs.

dawsontoth and others added 2 commits June 28, 2026 16:02
The vector-index-integrity suite is a major Windows CI hotspot (~29 min for the
suite). Most of the cost is ~100–200 sequential, individually-committed HTTP
round-trips per test — tiny data (DIMS=8, N≤50), so it's round-trip + per-write
fsync overhead, not vector math. Apply correctness-neutral reductions:

- reindex backfill: bulk-insert the N pre-index seed records in one request
  instead of N sequential POSTs. There is no HNSW index during insert, so
  per-record ordering is irrelevant; the backfill builds the index afterward.
- update-churn & reindex: run the independent read-back verification searches
  concurrently (Promise.all) instead of serializing N round-trips.

Left untouched (semantically order-sensitive): the entry-point insert/delete
sequence and the churn update sequence.

Note: validated on CI, not locally — restart_service is flaky in a local
multi-loopback setup (readiness probes time out at 60s), which both fails and
dominates local timings; the churn test (the one that runs cleanly locally)
still passes with the parallelized read-back.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each test in this suite deployed its own component and restarted the HTTP
workers; the restart is the dominant per-test cost on slow runners (a test doing
3 inserts still took ~265s on Windows — almost entirely restart/setup). Deploy
all four tables in one component with a single restart in `before`, and drop the
per-test add_component/set_component_file/restart. Restarts: ~5 -> 2 (the shared
one, plus the single restart `reindex` needs for its mid-test index-add);
add_component: 4 -> 1.

Tables keep distinct names/databases so the tests stay isolated despite sharing
one component. ReindexTable is deployed without an index initially; the reindex
test redeploys the full schema with it indexed (a no-op re-assert for the other
three, which have already run — reindex is last) to trigger backfill.

CI-validated (local restart_service is flaky in a multi-loopback setup).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
// Searching for that exact vector must return it in the top-5. These are
// independent read-only searches, so run them concurrently rather than
// serializing N round-trips.
const churnResults = await Promise.all(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Launching 30 concurrent QUERY requests via Promise.all on a CI runner that's already strained enough to hit 30s job timeouts may cause a wave of simultaneous HTTP connections to the same Harper instance. On an overloaded runner this could trigger per-connection timeouts or starve other in-flight requests, producing flaky misses that look like data-integrity failures rather than infra noise. A small concurrency cap (e.g. 5 in-flight at a time) preserves most of the speedup while bounding the burst:

Suggested change
const churnResults = await Promise.all(
const CONCURRENCY = 5;
const churnResults: any[][] = [];
for (let start = 0; start < N; start += CONCURRENCY) {
const batch = await Promise.all(
Array.from({ length: Math.min(CONCURRENCY, N - start) }, (_, j) =>
vectorSearch(httpURL, headers, path, seedVector((start + j) * 100 + ROUNDS, DIMS), { limit: 5 })
)
);
churnResults.push(...batch);
}

// the bulk of this suite's wall-time. The tables use distinct names/databases so the tests stay
// isolated despite sharing one component; `reindex` runs last and is the only test that mutates
// the schema afterward, so re-asserting the others on its redeploy is a harmless no-op.
const PROJECT = 'vecintegsuite';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared component/schema state creates an implicit test-ordering dependency: reindex must run last because it redeploys SCHEMA_REINDEX_INDEXED, which adds the HNSW index to ReindexTable. node:test runs suites in declaration order in practice, but that's a convention, not a guarantee enforced by the framework. If a future contributor reorders the test(...) calls — or node:test ever runs them in parallel — ReindexTable could have an index unexpectedly present for earlier tests.

At minimum, a comment near the test declarations noting "order matters — reindex must run last" (or a runtime guard that asserting the reindex test runs after the others) would make the dependency explicit. This is low-risk given the current codebase, but worth a brief note in code.

@dawsontoth dawsontoth closed this Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants