Skip to content

Stop flaky atomic instance batch depending on unindexed modules#5449

Merged
habdelra merged 1 commit into
mainfrom
fix-flaky-atomic-instances-depend
Jul 9, 2026
Merged

Stop flaky atomic instance batch depending on unindexed modules#5449
habdelra merged 1 commit into
mainfrom
fix-flaky-atomic-instances-depend

Conversation

@habdelra

@habdelra habdelra commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What was flaky

atomic-endpoints-test.ts > ... > writes > can write multiple instances that depend on each other intermittently fails in CI with a 500 and FilterRefersToNonexistentTypeError for the Place type (failing run).

Root cause

The test does two separate /_atomic writes: first place.gts + country.gts (modules), then Country/Place instances that adopt from them.

/_atomic returns once writes are durable, not indexed, and _batchWriteUnlocked run with waitForIndex: false deliberately skips waiting for a prior write's pending index (so consecutive atomic writes stay responsive). So the second batch's fileSerialization can look up the Place definition while the first batch's modules are still indexing. That on-demand definition lookup then races the in-flight module index: the generation bump discards the prerender result and the lookup returns undefined, so the whole batch rolls back with a 500. Under CI load the module index lags into that window; locally it usually wins, so it only flakes in CI.

The existing module-before-instance ordering handles modules and instances within a single batch. This test splits them across two batches, so that path never applies.

Fix

The first batch is setup for the second, so it posts with ?waitForIndex=true. The modules are indexed before the instance batch runs, which removes the race. This is the documented way for a caller writing modules and then dependent instances across separate batches to stay correct.

Diagnostics kept alongside the fix

So a recurrence is debuggable without a rerun:

  • The atomic 500 handler now logs e.cause. FilterRefersToNonexistentTypeError carries the actionable detail (which module/definition was missing, or that a concurrent invalidation discarded the lookup) in its cause, not its message.
  • The instance-batch assertion prints the response body on a non-201.

atomic-endpoints-test "can write multiple instances that depend on each
other" writes place.gts + country.gts in one /_atomic batch, then writes
Country/Place instances that adopt from them in a second /_atomic batch.

/_atomic returns once writes are durable, not once they are indexed, and
_batchWriteUnlocked with waitForIndex:false deliberately skips waiting for
a prior write's pending index. So the second batch's fileSerialization can
look up the Place definition while the first batch's modules are still
indexing. That on-demand definition lookup then races the in-flight module
index — the generation bump discards the prerender result and the lookup
returns undefined — so the batch fails with FilterRefersToNonexistentType
and a 500. Under CI load the module index lags into that window; locally it
usually wins, so the test only flakes in CI.

The earlier module-before-instance ordering fix only serializes modules
and instances within a single batch; this test splits them across two
batches, so that fix does not apply.

Fix: the module batch is setup, so post it with ?waitForIndex=true. The
modules are indexed before the instance batch runs, removing the race. This
is the documented way for a caller writing modules then dependent instances
across separate batches to stay correct.

Diagnostics kept alongside the fix so a recurrence is debuggable without a
rerun:
- the atomic 500 handler now logs e.cause; FilterRefersToNonexistentType
  carries which module/definition was missing (or that a concurrent
  invalidation discarded the lookup) in its cause, not its message
- the instance-batch assertion prints the response body on non-201

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

Copilot AI 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.

Pull request overview

This PR addresses a CI flake in the realm-server atomic write tests caused by issuing two separate /_atomic batches where the second batch depends on modules from the first before those modules are indexed. It also improves server-side diagnostics so future atomic-batch 500s preserve the most actionable underlying error detail.

Changes:

  • Make the module-setup /_atomic request in atomic-endpoints-test.ts opt into waitForIndex=true so the subsequent instance batch can’t race module indexing.
  • Improve atomic-write 500 logging to include e.cause (message + stack when it’s an Error) so root causes like FilterRefersToNonexistentTypeError aren’t swallowed.
  • Enhance the test assertion to print the response body when the instance batch doesn’t return 201.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/runtime-common/realm.ts Logs e.cause details when an atomic write fails, improving debuggability of intermittent 500s.
packages/realm-server/tests/atomic-endpoints-test.ts Uses /_atomic?waitForIndex=true for module setup and prints response body on failure to eliminate and diagnose the flaky race.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@habdelra
habdelra requested a review from a team July 9, 2026 14:43
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files      1 suites   2h 47m 9s ⏱️
3 433 tests 3 418 ✅ 15 💤 0 ❌
3 452 runs  3 437 ✅ 15 💤 0 ❌

Results for commit da84d4f.

Realm Server Test Results

    1 files      1 suites   12m 17s ⏱️
1 765 tests 1 765 ✅ 0 💤 0 ❌
1 844 runs  1 844 ✅ 0 💤 0 ❌

Results for commit da84d4f.

@habdelra
habdelra merged commit c640569 into main Jul 9, 2026
94 of 95 checks passed
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.

3 participants