Skip to content

test(record-caching): run the multi-worker suite on real workers + add cross-worker coherence anchors - #1760

Merged
kriszyp merged 5 commits into
mainfrom
kris/record-caching-multiworker-test
Jul 13, 2026
Merged

test(record-caching): run the multi-worker suite on real workers + add cross-worker coherence anchors#1760
kriszyp merged 5 commits into
mainfrom
kris/record-caching-multiworker-test

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 11, 2026

Copy link
Copy Markdown
Member

Summary

The #410 record-caching integration test's record-caching [rocksdb] 4-worker suite was actually running single-worker, so the feature's central promise — cross-worker cache invalidation via the shared VerificationTable — was never exercised by CI.

Root cause: record-caching.test.ts passed env: { HARPER_WORKER_COUNT: '4' }. Harper does not read a HARPER_WORKER_COUNT env var (it's a test-file convention that other suites read into config.threads.count, e.g. read-snapshot-consistency.test.ts), and the integration harness forces --THREADS_COUNT=1 on the CLI — so the suite ran on one worker and passed for the wrong reason.

This PR:

  • Fixes the existing suite to set worker count via config.threads.count, with a runtime assertMultiWorker guard (via system_information) so it can't silently regress to a no-op again.
  • Adds a small shared helper recordCachingWorkers.ts (env→config translation + the guard) documenting the trap in one place.
  • Adds two promoted exploratory-QA anchors, both genuine multi-worker:
    • record-caching-cross-worker.test.ts — update / delete / hot-key coherence: once a write is acked, no worker may serve a stale or ghost point-read (200-key churn, delete→recreate, 60-round hot-key monotonic-read).
    • record-caching-scan-coherence.test.ts — the cached point-read path (getEntry) must agree with the uncached scan/query path (getRange, which bypasses the cache) for the same committed record, across update, delete (both orderings), and rapid churn.
  • Shared fixture record-caching-coherence/ (indexed name so the scan path can be queried).

Test-only; no product source touched. All three suites pass on real multi-worker; lint + typecheck clean.

Where to look

  • record-caching.test.ts diff is the load-bearing correctness change (2 lines) — everything else is added coverage.
  • The two new anchors assert post-ack state only (a read is compared against a write we've already seen a 200 for), so a failure is a genuine coherence bug, not a harness race. They don't probe the pre-ack in-flight window.

No linked issue (surfaced by the exploratory-QA loop, tracked internally as F-132).

🤖 Generated by an LLM (Claude Opus 4.8). Test-only change; no cross-model review run (nothing for a correctness/security lens beyond the diff itself).

Update — additional record-cache anchors (2026-07-12)

Extended with three more genuine-multi-worker record-cache anchors (same shared recordCachingWorkers helper, all green on 4-worker RocksDB):

  • record-caching-ttl-eviction.test.ts — TTL eviction (async store.remove() sweep, distinct from removeSync) invalidates every worker's cache; no cross-worker ghost after expiry.
  • record-caching-blob.test.ts — file-backed Blob attributes reflect replace/delete cross-worker (SHA-256-verified; no stale/dangling/ENOENT read, no post-delete ghost, no orphaned blob file).
  • record-caching-invalidate.test.tsinvalidate() / audited-delete write a versioned-null root; every worker's cache reflects it.

Together with the cross-worker + scan-coherence anchors above, this covers #410's point-read cache across update/delete, scan-divergence, TTL-eviction, blob, and invalidate/null — all single-node multi-worker. (A cluster-tier replication anchor exists too but lives in harper-pro, so it's not part of this core PR.)

…d cross-worker coherence anchors

The #410 record-caching integration test's "4-worker" suite actually ran
single-worker: HARPER_WORKER_COUNT is not a Harper env var (it's a test-file
convention translated into config.threads.count elsewhere), and the harness
forces --THREADS_COUNT=1, so cross-worker cache invalidation -- the load-bearing
correctness promise of the per-worker WeakLRUCache + shared VerificationTable --
was never exercised.

Fix the existing suite to configure threads via config.threads.count with a
runtime worker-count guard (assertMultiWorker) so it can't silently regress to a
no-op, and add two promoted exploratory-QA anchors:
  - cross-worker coherence under update/delete/hot-key churn
  - cached point-read vs uncached scan/query coherence

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp
kriszyp requested a review from cb1kenobi July 11, 2026 22:28

@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 introduces comprehensive integration tests to verify cross-worker record-cache coherence and point-read vs. scan coherence under update/delete churn. It also adds a shared helper to enforce multi-worker execution, ensuring the tests are not run in a vacuous single-worker environment. The review feedback suggests improving the robustness of the environment variable parsing for worker counts and recommends limiting concurrent HTTP requests across several test suites to prevent socket exhaustion and transient network failures in CI environments.

Comment thread integrationTests/database/recordCachingWorkers.ts Outdated
Comment thread integrationTests/database/record-caching-cross-worker.test.ts Outdated
Comment thread integrationTests/database/record-caching-cross-worker.test.ts Outdated
Comment thread integrationTests/database/record-caching-cross-worker.test.ts Outdated
Comment thread integrationTests/database/record-caching-scan-coherence.test.ts Outdated
Comment thread integrationTests/database/record-caching-scan-coherence.test.ts Outdated
Comment thread integrationTests/database/record-caching-scan-coherence.test.ts Outdated
Comment thread integrationTests/database/record-caching-scan-coherence.test.ts Outdated
Comment thread integrationTests/database/record-caching-scan-coherence.test.ts Outdated
Comment thread integrationTests/database/record-caching-scan-coherence.test.ts
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

kriszyp and others added 2 commits July 11, 2026 17:07
…r-count parse

Addresses review feedback: cap the large fresh-connection fan-outs (record
creation, cache-warming, per-key churn) via a mapBounded helper so they can't
exhaust ephemeral sockets on constrained CI, and reject non-positive/non-numeric
HARPER_WORKER_COUNT explicitly. Small per-key bursts stay concurrent (that
concurrency is the cross-worker coherence check).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match the cross-worker suite's waitForTable — a silent fall-through gave a
confusing failure when Harper was slow to start. Addresses review feedback.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp
kriszyp marked this pull request as ready for review July 11, 2026 23:35
kriszyp and others added 2 commits July 12, 2026 06:13
…ss-worker anchors

Extends the multi-worker record-cache coverage to three more value-type/lifecycle
seams, all on genuine 4-worker RocksDB via the shared recordCachingWorkers helper:
  - record-caching-ttl-eviction: TTL eviction (the async store.remove() sweep path,
    distinct from the removeSync path) must still invalidate every worker's cache —
    no cross-worker ghost after a record expires + is evicted.
  - record-caching-blob: file-backed Blob attributes must reflect a replace/delete
    cross-worker (new bytes, no stale/dangling/ENOENT read, no post-delete ghost,
    no orphaned blob file), SHA-256 verified.
  - record-caching-invalidate: invalidate() / audited-delete write a real versioned
    null root; every worker's cache must reflect it (no stale cached object).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…untimes

The three added anchors passed on Node but failed under the Bun / uWS-HTTP CI
runtimes due to test methodology, not Harper behavior:
  - invalidate: a "reads hit >1 worker threadId" sanity assertion failed under
    Bun (which pins connections to one worker) even though the instance has 4
    workers and all coherence assertions pass — dropped that assertion
    (assertMultiWorker in before() remains the real guard).
  - blob: Bun's fetch drops sockets ("socket hang up") on concurrent 150KB reads
    over Connection: close; the disk-walk reconcile proves the store is intact.
    Reads now retry transport errors and only assert on successful-but-wrong
    content (stale sha / unexpected 404 / post-delete ghost); a still-failed
    transport read is inconclusive, not a divergence.
  - ttl-eviction: passed but slow (~74s in CI) -> raised the timeout and trimmed
    the workload so it stays well under budget on a loaded shard.

Verified green under Node, HARPER_RUNTIME=bun, and HARPER_UWS_HTTP=1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
let client: ReturnType<typeof createApiClient>;
let httpURL: string;
let authHeader: string;
const failures: string[] = [];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: failures is shared across S1/S2/S3 and never reset between tests

The suite-level failures array accumulates for the whole suite. Once any test pushes an entry (e.g. S1 fails), every later test's strictEqual(failures.length, 0, ...) also fails and reprints the earlier test's messages — so S2/S3 report a failure they didn't cause, obscuring which test actually found the defect. S2 also breaks on the first round because failures.length > 0 is already true from S1.

Giving each test its own local failures array (or resetting in a beforeEach) keeps a real failure attributed to the test that found it.


Generated by Barber AI

// Direct same-instant disagreement across the burst is the strongest signal:
// different workers answered differently for the identical id at the identical tick.
if (any404 && any200) {
ghostObservations.push({ id, tick, msSinceConfirm: 0 });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Low: transient cross-worker ghost signals are collected but never asserted

ghostObservations (including this same-instant any404 && any200 case, commented as "the strongest signal") and scanDivergences are only console.error'd — the test can only fail on the post-quiescent stickyGhosts / finalScanLive / confirmedCount checks. A genuine transient cross-worker incoherence during the eviction window would be logged but still pass CI, which narrows this anchor's protective value versus its stated purpose.

If that's intentional (the async sweep propagates per-worker with unavoidable lag, so a brief disagreement window would be flaky to enforce), a one-line comment saying so would stop a future reader assuming it's enforced. If not, assert ghostObservations/scanDivergences are empty (perhaps gated on msSinceConfirm past a margin).


Generated by Barber AI

@kriszyp
kriszyp merged commit ca4551c into main Jul 13, 2026
61 of 63 checks passed
@kriszyp
kriszyp deleted the kris/record-caching-multiworker-test branch July 13, 2026 21:26
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