Skip to content

test(integration): de-flake computed indexed properties REST block - #1721

Merged
kriszyp merged 1 commit into
mainfrom
kris/fix-computed-indexed-flake
Jul 11, 2026
Merged

test(integration): de-flake computed indexed properties REST block#1721
kriszyp merged 1 commit into
mainfrom
kris/fix-computed-indexed-flake

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 8, 2026

Copy link
Copy Markdown
Member

What

De-flakes the Computed indexed properties REST block in integrationTests/apiTests/computed-indexed-properties.test.mjs (seen failing intermittently on Integration Tests 5/6, Node v26).

Root cause

jsTotalPrice is a @computed @indexed attribute whose resolver is registered at runtime by the component's resources.js (setComputedAttribute) as per-worker in-memory state (resources/Table.ts userResolvers) — unlike the @computed(from: …) expression attributes, whose resolvers are set at schema-load time.

The /Product/ route (from the @export'd table) starts serving as soon as the schema loads, which can be before resources.js finishes on a given http worker. Harper runs many http workers (default cpus-1) and load-balances via SO_REUSEPORT, so a request can hit a still-cold worker. Two failure modes:

  • a PUT on a cold worker computes the @indexed value with a missing resolver → jsTotalPrice is indexed as undefined, frozen at write time (never self-heals on read), so ?jsTotalPrice=119 misses the record; and
  • a GET ?select(jsTotalPrice) served by a cold worker recomputes null.

The existing route probe only proves the table is reachable on one worker — not that the resolver is registered everywhere.

Fix (test-only)

Add waitForComputedResolver, a client-side readiness gate (in the spirit of restartHttpWorkers) that seeds a throwaway probe record and reads the on-demand computed value in bursts — each burst opens fresh connections (Connection: close) so requests spread across workers via SO_REUSEPORT — until several consecutive bursts see zero cold responses, i.e. every worker has run resources.js. The suite stays multi-worker (this is a functional test, not a single-thread special case).

Also corrects the search_by_value comment: jsTotalPrice is null there because the operations API runs on the main thread, where resources.js never registered the resolver — not because of a write-time race, as the old comment claimed.

Verification

On a 19-worker box, forcing the race by deferring the resolver registration 4s (setTimeout(() => setComputedAttribute(...), 4000)):

Variant Result
Original test (no gate) ✗ ✗ ✗ — write+filter fails deterministically (race reproduced)
Gated fix ✓ ✓ ✓
Unmodified suite, default threads, no injected delay ✓ across repeated runs

Cross-model review

Reviewed via the cross-model-review skill (Codex leg + Harper-domain adjudication; Gemini leg skipped — test-only change, nothing left after the integration-test strip). Findings addressed in this PR:

  • [Significant] Probe record collided with the =119 filter assertions. The probe previously computed to 119 (same as the real assertions filter on), so a missed cleanup could reintroduce a flake in a different test. Fixed: probe now uses taxRate: 0 → computes to 100 (PROBE_COMPUTED), which can never satisfy ?jsTotalPrice=119 / ?totalPrice=119; cleanup moved into a finally.
  • [Suggestion] Seed PUT now status-checked (.expect(204)) so a seed failure fails fast with an accurate message instead of a misattributed 60s "resolver not live" timeout.
  • [Suggestion] readsPerBurst now scales with host core count (max(60, cpus*8)) so a straggler worker is reliably hit even on high-core hosts.

No blockers. Underlying product hardening tracked in #1631 (out of scope here).

Related

@github-actions
github-actions Bot requested a review from heskew July 8, 2026 21:12
@kriszyp
kriszyp requested review from kylebernhardy and removed request for cb1kenobi July 8, 2026 21:12

@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 a client-side readiness poll (waitForComputedResolver) in the computed indexed properties integration test to ensure that the JS-computed resolver is fully initialized across all HTTP workers before executing tests. It also updates documentation comments explaining the execution context of the main thread versus worker threads. The review feedback recommends wrapping the polling requests in a try-catch block to handle transient network errors gracefully and prevent unexpected test failures due to unhandled promise rejections.

Comment thread integrationTests/apiTests/computed-indexed-properties.test.mjs
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

The 'Computed indexed properties' suite exercises jsTotalPrice, a
'@computed @indexed' attribute whose resolver is registered at runtime by
resources.js (setComputedAttribute) as per-worker in-memory state
(Table.ts userResolvers), not at schema-load time. The /Product/ route (from the
@export'd table) starts serving as soon as the schema loads, which can be before
resources.js finishes on a worker. Harper runs many http workers (default
cpus-1) and load-balances via SO_REUSEPORT, so a request can hit a still-cold
worker:
  - a PUT on a cold worker indexes jsTotalPrice as undefined (frozen at write
    time), so the ?jsTotalPrice=119 filter misses the record; and
  - a GET ?select(jsTotalPrice) on a cold worker recomputes null.
Either surfaces as an intermittent failure in the REST block (seen on
Integration Tests 5/6, Node v26).

Fix: gate the suite (before all reads/writes) on the jsTotalPrice resolver being
live on every worker — poll on-read bursts (fresh connections spread across
workers via Connection: close) until several consecutive bursts see zero cold
responses. The route probe alone only proves the table is reachable on one
worker. Stays multi-worker (functional test, not a single-thread special case).

Verified on a 19-worker box by deferring the resolver registration 4s: the
ungated write+filter fails deterministically (3/3), the gated version passes
(3/3); the unmodified suite passes across repeated runs.

Also corrects the search_by_value comment: jsTotalPrice is null there because
the operations API runs on the main thread, where resources.js never registered
the resolver — not because of a write-time race.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kriszyp
kriszyp force-pushed the kris/fix-computed-indexed-flake branch from 828c842 to dce7084 Compare July 8, 2026 22:15

@Ethan-Arrowood Ethan-Arrowood left a comment

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.

Great root-cause writeup — much better than pinning the suite to one worker.

sent with Claude Fable 5

@kriszyp
kriszyp merged commit 6f9970d into main Jul 11, 2026
83 of 85 checks passed
@kriszyp
kriszyp deleted the kris/fix-computed-indexed-flake branch July 11, 2026 12:06
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