Skip to content

Wait only for the index passes that can move what a write resolves - #6206

Merged
habdelra merged 4 commits into
mainfrom
cs-13024-the-write-paths-indexing-drain-is-realm-wide-so-one-cards
Sep 21, 2026
Merged

habdelra merged 4 commits into
mainfrom
cs-13024-the-write-paths-indexing-drain-is-realm-wide-so-one-cards

Conversation

@habdelra

@habdelra habdelra commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

A card write waits for the indexing already in flight that could change what it resolves, instead of for every index pass running anywhere in the realm.

What the wait is, and where it sits

Writing a card to a realm is two jobs stitched together. First the realm has to turn the JSON document the client sent into the bytes it will store — it resolves the card's definition (the class the card adopts from, its fields, which of them are computed, which link to other cards) and serializes against it. That step is called staging. Then, separately, the realm indexes what it wrote: it recomputes the card's row in the index, and then walks outward to every other card whose row depended on it and recomputes those too. That outward walk is the fan-out, and it is the expensive half. Editing a leaf card invalidates almost nothing; editing a hub card that hundreds of others link to invalidates all of them.

Before a write stages, it waits for indexing that is already running. There are two places that wait happens — the realm's own commit, and the batch coordinator that runs ahead of it — and both call the same gate. The gate's stated reason is definition freshness: if somebody wrote a module a moment ago and its index pass has not landed, a card staging against that module's type should not resolve a stale version of it.

The problem is what the gate covers. It waits for every in-flight incremental and copy pass for the realm, whatever that pass touched. So a write of card B, which now holds only card B's own file lock, still parks behind card A's entire fan-out — and card A's fan-out is instance rows, none of which B has any interest in. One person editing a hub card gates every other person writing in that realm, for as long as the fan-out takes.

Why an instance-only pass can be skipped

The thing the gate is protecting — resolving a definition while staging — does not read the index at all. A card's type resolves through lookupDefinition, which reads the module off disk, and a commit that rewrites a module drops that module's cached definition as the bytes land. So the freshness a staging write needs is a property of the file system and the definition cache, not of index rows.

That accounts for definitions. There is one other thing a staging write resolves out of the index rather than off disk: the realm's own settings. realmConfig() reads realm.json from disk and then overlays the indexed row of the realm's config document on top — an overlay that replaces the settings map wholesale and wins, deliberately, so it can clear what the file assigned. A program reading a setting is therefore reading the index, and a pass touching that document moves what the batch resolves exactly as a module write does, while carrying no executable extension.

So the passes worth waiting for are the ones that touched an executable module — a .gts, .gjs, .ts or .js file — or the realm's config document. A pass that touched neither only rewrote instance rows, which a staging write reads nothing from, so waiting for it buys the writer nothing and costs it the whole of somebody else's fan-out.

Each index pass now records, when it is enqueued, whether it touched either, and the write path waits on that slice alone. A removal counts the same as a write: a module that is gone changes what resolves just as surely as one whose bytes moved. The config document is matched by URL against the realm root rather than by filename, so a card stored at nested/realm.json remains an ordinary instance.

Two membership decisions, and why each went the way it did

A realm copy declares itself module-touching. A copy indexes an entire source realm, so its change set is everything that realm holds, modules included. It is declared rather than computed because a copy never enumerates its changes up front — there is no list to test.

A from-scratch reindex stays outside the gate. This one deserves its reasoning spelled out rather than inherited, because the same list of job types answers two different questions differently, and reusing one answer for the other question has gone wrong before. "Should a writer wait for this pass?" and "can the index this pass produced be trusted?" are not the same question, and a from-scratch is near the top of the list for the second while being near the bottom for the first.

Asked as a wait: a from-scratch re-derives index rows from bytes that are already sitting on disk. It changes no file. Its production rows stay live and internally consistent the whole time it runs, because it builds into a working table and swaps at the end. So there is nothing in it that a staging write reads. Meanwhile the cost of waiting is extreme — a from-scratch can sit queued behind a fleet-wide reindex, and parking every writer in the realm behind one would stall writes for hours. Both halves point the same way, so it is excluded, on its own merits.

What this does not touch

The write still waits for its own index pass before it answers, exactly as before — that is a separate await further down, and it is where a write's time actually goes today.

The read path is untouched. Read-your-writes already works differently and already works correctly: a card or file read waits on the indexing the requesting user themselves started, on a bounded budget, rather than on whatever happens to be running. Nothing here changes that.

The two endpoints that genuinely need to see the whole index settled — the publishability report and the indexing-error report — still wait on every in-flight pass, since both scan index rows across the realm and a stale snapshot would make them report the wrong thing.

One real behaviour change

A card-operations program can read a card's indexed values (its computed fields, and the fields of cards it links to) while staging. That read now sees the index as it stands rather than the index after some other card's pending fan-out has landed. A row that is absent, or not yet caught up, is reported as such rather than waited for.

Serving a slightly stale view of a card someone else is in the middle of editing is correct rather than a compromise, so the general case is the intended trade. The case worth naming is a writer with two of their own writes overlapping, where the second reads indexed values the first has not finished producing — that can happen whenever two saves are in flight at once, not only on the paths that opt out of waiting for their own indexing.

That is deliberately not solved by making the write wait, for two reasons. Read-your-writes, where it actually matters, is a property of reads, and the read path already handles it — a card or file read waits on the indexing the requesting user themselves started, scoped and bounded. And a wait would only ever half-close the gap: a row left behind by another user's fan-out is stale in exactly the same way, and no wait scoped to the writer covers that. A program that needs a value to be current is better served by saying so than by every write in the realm paying for the possibility.

Honest framing: this is prophylactic

This is argued from the code, not from a measured regression, and the PR should not be read as fixing an observed slowdown.

The write path emits a stage breakdown per request, including the time spent at this gate. Across a 24-hour staging window, 99 card writes: the gate cost 0 ms on 94 of them and 1 ms on the other five. Zero is the honest number today. The write's own indexing pass, by contrast, ran a median of 3.2 s and as long as 16.2 s.

Two reasons that zero is not evidence the gate is harmless. First, it is a sampling artefact — the scenario this addresses needs two writers editing two different cards in one realm at the same time, and neither the benchmark runs nor that staging window produced it. Every write in the window was either repeated edits to a single card (which contend on the file lock, a different mechanism) or a single writer alone in a realm. Second, and more to the point, the gate reads zero across replicas only because the set of pending passes it consults is an in-memory map — one replica simply cannot see another's pending job. The moment that becomes a shared, cross-replica view, the gate starts costing what it has always logically cost, and a hub card's fan-out gates the realm cluster-wide. Narrowing it by kind is what keeps that from happening.

Where the changes live

  • packages/runtime-common/realm-index-updater.ts — each in-flight pass records whether its change set includes a module; a new gate exposes that slice, alongside the existing full and per-user gates.
  • packages/runtime-common/realm.ts — the realm's own accessor for the new gate, and the two write-path waits repointed at it.
  • packages/runtime-common/card-operations/coordinator.ts — comments only; the drain call site is unchanged.
  • packages/runtime-common/paths.ts and index-runner.ts — the realm config document's URL is resolved by one shared helper rather than a copy in each place, so the gate and the indexer cannot drift on which document it is.

Testing

Ten unit tests on the gate itself, driving the updater with a stub queue so each pass settles on demand rather than racing a real worker: an instance-only pass is outside the gate while the full gate still sees it (so the test proves narrowing, not absence); a module write holds it and releases on settle; a module removal holds it; a change set mixing a module with instances holds it; a copy holds it; a write to the realm's config document holds it, and so does a removal of it; a card merely named realm.json deeper in the realm does not; a realm URL spelled without a trailing slash still recognises its own config document; and — the sharp one — with a module pass and an instance pass both in flight, settling only the module pass releases the write path while the instance fan-out is still running.

Every one of them was checked against a control that reddens it, and the controls differ by test rather than being one control for all eight. The two instance-only tests fail against a gate that delegates to the un-narrowed one. The module, removal, mixed-change-set and copy tests fail against a gate that holds for nothing. The config test fails against a gate that checks only for executables, the nested/realm.json test fails against a gate that matches the config document by filename instead of by URL, and the trailing-slash test fails against a gate that resolves the document's name relatively instead of through RealmPaths — a form that would hold for a URL no change set ever contains, protecting nothing while every other test stayed green. Each control reddens its intended test and leaves the rest green.

One caveat on the local run, stated because it would otherwise look like a gap. Eighteen tests in the card-operations commit suite fail locally, all in its transform section and all for the same reason: the fixture realm's ExternalReport definition never reaches the definition cache, so every case in that section dies in its setup. They fail identically with these changes reverted — same tests, same count, same error — so they are environmental here rather than something this introduces. Everything else in the write-path blast radius passes: the card-save suite, the read-path drain suite, the updater suite, atomic batch indexing, the bulk-write render hold, target-first index ordering, and the indexing suite.

At the HTTP level, the card-save suite gains a PATCH that proves the write does not consult the wide gate: every in-flight pass is stubbed to a promise that never settles, the module-touching gate is left real with nothing pending, and the write is expected to complete and read its card back out of the index. Its counterpart in the same file — a PATCH parked on a controllable module-touching gate and released on demand — is the positive control that the remaining wait is real.

🤖 Generated with Claude Code

habdelra and others added 2 commits September 18, 2026 11:25
The pre-staging gate on the write path awaited every in-flight incremental
and copy pass for the realm, whatever it touched. A card's serialization
resolves its type off disk, so a pass that rewrote only instance rows moves
nothing that gate stands in front of — yet a hub card's fan-out held every
other writer in the realm behind it.

Each incremental deferred now records whether its change set includes an
executable module, and the write path waits on that slice alone. A copy
declares itself module-touching: it indexes a whole realm and never
enumerates its changes. From-scratch passes stay outside, re-derived from
the wait question rather than inherited — one re-derives rows from bytes
already on disk while its production rows stay live, so it moves nothing a
staging write reads, and waiting for one would park every writer for the
length of a full reindex.

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] This review went after the premise the narrowing rests on — that nothing a staging write resolves comes out of the index. The method was to enumerate every BatchCore member the coordinator reaches after the drain and trace each one to its backing store, then check the tag against the URL shapes that actually reach enqueueChanges, and re-derive the from-scratch exclusion rather than take it on the comment's word. The realm-server lane was not run, so the HTTP-level additions in card-save-skip-index-wait-test.ts are read rather than executed.

The premise holds for definitions — lookupDefinition reaches the modules table and the prerenderer, never boxel_index — but it does not cover realmConfig(), which is a second index read the batch makes after the drain and which an instance-only realm.json pass does move. That needs a decision before this merges; the rest is comment accuracy.

  1. Decide what a realm.json pass should do to the gate — thread on indexedCardValues in card-operations/coordinator.ts.
  2. Say which discriminator keeps the intermediate flush in scope, and whether the flush is still load-bearing at all — thread on WriteOptions.waitForIndex in realm.ts.
  3. Name the render-hold release in the incrementalIndexing() enumeration — thread on that doc block in realm-index-updater.ts.
  4. Description: "Each was confirmed to fail against the un-narrowed gate before being kept" holds for two of the six gate tests. Running the suite with incrementalIndexingOfExecutables delegating to incrementalIndexing fails exactly the instance-only pair; the four that assert a module write, a module removal, a mixed change set and a copy hold the gate pass against it, and fail instead when the gate is stubbed to hold for nothing. They are coverage against the tag never being set — real, but a different control, and worth saying so before someone edits them trusting the stronger claim.

Adjacent, not asked of this PR: isIgnored is realm-index-updater state refreshed by index passes too, so it carries the same eventual-consistency shape as the two reads above. It only reaches the batch through assertRemovalsAllowed, and a removal-only batch already skips the drain, so nothing turns on it today — worth a line from whoever widens that next.

Comment on lines +142 to +149
// This is the one read a batch makes of the index, and it is not a network
// capability: the engine is the realm's own, handed down narrowed to the
// single row a program's reads are layered from. Called only inside the
// lock, after the drain, so what it reports is the realm as the batch is
// about to change it.
// single row a program's reads are layered from. Called inside the lock, so
// no other writer of these files can move the row underneath the batch —
// but the row is the index as it stands, not as some other card's pending
// fan-out will leave it. A program reads indexed values eventually
// consistently, and a row that is absent or not yet caught up is reported
// as such rather than waited for.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] realmConfig() is a second index read this batch makes after the drain, and it is one an instance-only pass moves.

core.realmConfigRealm#getRealmConfig#parsedRealmInfoparseRealmInfo, which reads realm.json off disk and then overlays the indexed RealmConfig row (#realmIndexQueryEngine.instance(<realm>/realm)) on top. assignRealmConfig replaces the settings map wholesale, and the indexed overlay runs second and wins — deliberately, so the second overlay can clear what the first assigned. realm.json carries no executable extension, so its incremental pass is tagged touchesExecutable: false, and neither drainIndexing() nor the realm's own gate waits for it. In the window between one writer's realm.json landing on disk and its index swap, another writer's batch reads the replacement settings off disk and then has them overwritten by the superseded index row.

That window is the one the comment above realmConfig in commitBatch says the drain exists to close: "A realm.json write still indexing when this batch arrives is exactly the case — the drain waits for it, and a snapshot taken earlier would stage realmConfig() values the realm has already replaced." That sentence no longer describes the code.

Pick one:

  • Widen the tag so a change set containing the realm's config card holds the gate. It is not an executable, but it is the one non-executable document a staging batch resolves out of the index, so the membership question the gate asks has two answers rather than one.
  • Accept it as a second eventually-consistent read — and then correct the commitBatch comment, drop "This is the one read a batch makes of the index" from this block, and say in the description that a program reading realmConfig("x") can see a setting its owner has already replaced.

Regression, introduced here. Blocking on the decision rather than on a particular fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Confirmed and fixed in 2504b0b — I took the first option, widening the tag.

I verified the mechanism rather than taking the report on trust: parseRealmInfo reads realm.json off disk and then overlays the indexed row via #realmIndexQueryEngine.instance(realmConfigCardURL), and assignRealmConfig replaces the settings map wholesale on that second pass, so the index row wins. The document carries no executable extension, so it was tagged as not affecting staging and neither gate waited for it. Real regression, mine.

Widening rather than accepting, for two reasons. The gate's question is "can this pass move what a staging write resolves", and the config document demonstrably can — so accepting would have meant the gate no longer answers its own question. And the cost is close to nothing: a realm's config is written rarely, so holding for its pass is a bounded wait nobody pays in steady state, whereas accepting it would have left a program reading realmConfig("x") able to see a setting its owner had already replaced.

What changed: the tag is now affectsStaging and the gate is incrementalIndexingAffectingStaging() — named for the question rather than for one of its two answers. The config document is matched by URL against the realm root, not by name, so a card stored at nested/realm.json stays an ordinary instance; there is a test for that which fails if the match is loosened to endsWith. The comment you quoted is true again as written, so I left it alone, and I corrected the adjacent "the one read a batch makes of the index" to name both reads.

Thanks — this is the finding the PR needed.

Comment thread packages/runtime-common/realm.ts Outdated
Comment on lines +1445 to +1451
// Note: in a mixed-batch `writeMany` call where a module is followed by
// an instance, the *intermediate* index flush that fileSerialization
// depends on is still awaited inline regardless of this flag — without
// it, the next instance's serialization would fail. This flag governs
// it, the next instance's serialization would fail. That flush is the
// batch's own module landing, not another writer's, which is why the
// pre-staging gate can ignore instance-only passes and this one cannot be
// skipped. This flag governs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] This names the wrong discriminator, and it stacks a new justification on one that contradicts the change.

"the batch's own module landing, not another writer's" reads as if ownership is what keeps the flush in scope. The gate does not distinguish own from other — it distinguishes module-touching from instance-only, and another writer's module pass is waited for too. Please say module-vs-instance here; as written the next reader can conclude the opposite.

The larger question is whether the flush is load-bearing at all. Its own comment at the module→instance transition in #commitBatchUnlockedInner justifies it with "fileSerialization calls lookupDefinition, which needs dependent modules to be indexed". Nothing on that path reads boxel_index: lookupDefinitionWithContextloadDefinitionCacheEntry reads the modules table and, on a miss, populates through prerenderModule, which imports out of the loader — off disk; buildLookupContext consults only the realm list and its visibility. And the module leg of this same loop deletes the module's modules row before the next entry stages, so the following lookupDefinition is a guaranteed miss that re-derives from the bytes just written.

So either that comment is stale and the intermediate flush is a per-batch index pass every mixed /_atomic write pays for nothing, or it buys something neither comment names. Which is it? If it is stale, say so here rather than adding a reason on top of it — removing the flush is a follow-up, not this PR.

Separately, the reflow left "This flag governs" orphaned on its own line.

Pre-existing, now load-bearing — this PR adds a new justification over the old one. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] You're right on both counts, and the sentence is gone in 2504b0b rather than reworded.

On the discriminator: naming ownership was simply wrong. The gate distinguishes passes that can move what staging resolves from passes that cannot, and another writer's module pass is waited for exactly as your own is. Nothing about that comment needed my addition to explain the flush, so stacking one on top was the mistake — I removed it and left the original justification standing alone.

On whether the flush is load-bearing: I am not asserting either way, because I have not verified it to the standard that would justify a claim in a comment. What I did verify is the half you traced that bears on it — #commitBatchUnlockedInner does await this.#definitionLookup.invalidate(url.href) per module as the bytes land (realm.ts:3723), inside the same loop and before the next entry stages. So the following lookupDefinition is a guaranteed cache miss re-derived from the bytes just written, which is consistent with the flush being vestigial. What I have not established is whether anything else in that window depends on the module's index row existing — the invalidation fan-out ordering is the part I would want to rule out before calling it dead, and that is a bigger question than this PR.

So: not stale on my say-so, and not defended either. Left exactly as it was, with my addition withdrawn, and worth its own ticket rather than a claim here.

The orphaned "This flag governs" is fixed in the same commit.

Comment on lines +219 to +224
// This is the widest gate a request waits on, and it is for readers of the
// index as a whole — the publishability report, the indexing-error report,
// and the cheap "is anything pending at all" check the read-your-writes
// drain starts from. A write about to stage wants
// `incrementalIndexingOfExecutables()` instead; waiting here would make one
// card's fan-out gate every other card's write.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] The enumeration leaves out a write-path caller of this gate. Realm#_commitBatchUnlocked calls incrementalIndexing() to decide when to release the render hold. That is a write-path caller and not a reader of the index as a whole, and it has to stay wide — the hold must outlive every pass the commit spawned, whatever that pass touched — which is exactly why it belongs in a list whose next sentence is "a write about to stage wants incrementalIndexingOfExecutables() instead". As it reads, someone narrowing call sites against this doc takes the render-hold release with them and collapses the merge window a bulk import depends on.

Please name it alongside the three readers, with the clause that says why it is not narrowable.

Regression in the comment only. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in 2504b0b. The render-hold release is now named alongside the three readers, with the reason it cannot be narrowed: the hold has to outlive every pass the commit spawned whatever that pass touched, so narrowing it would free the render lane early and collapse the merge window a bulk import depends on.

Your framing of the risk is the part worth keeping — the danger was not the omission itself but that the next sentence sends a reader to narrow call sites against an incomplete list, which would have taken the render hold with it. A doc that enumerates consumers has to be exhaustive or it is worse than one that enumerates none.

A staging write resolves two things out of shared state, not one. Alongside
the executable module a card's type is built from, the realm's settings are
read off disk and then overlaid with the indexed row of the realm's config
document — an overlay that replaces the settings map wholesale and wins. A
pass touching that document therefore moves what a batch resolves just as a
module write does, while carrying no executable extension.

The gate's tag now records whether a pass touched either, and its name says
the question it answers rather than one of its two answers. The config
document is matched by URL against the realm root, so a card stored under
that name deeper in the realm stays an ordinary instance.

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

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Second pass, scoped to the config-document widening and the rename that came with it. I went at the URL match, the deletion case, the rename sweep, whether the two new tests can fail, whether a third index read is still unaccounted for, and whether holding the gate for a config write can self-wait. I did not re-review the narrowing itself, and CI was still running when this was written.

No blocking issues. The widened tag is correct on every realm shape I could put it in, and both new tests fail against their own controls and pass for the right reason. The three findings below are all comment- and naming-accuracy asks.

Dispositions of the open threads

  • The realmConfig() second-read thread is resolved. #isRealmConfigDocument matches the change-set spelling exactly: every production change set is built from paths.fileURL(...), which is the .json-suffixed file URL, and that is what the predicate compares against — the .json-stripped card URL is only the overlay's index-row key, so the two conventions do not collide here. The gate also releases late enough to matter: clearRealmIndexCachesAndBroadcast() runs inside onInvalidation, which is awaited before indexingDeferred.fulfill(), so a batch released by this gate cannot then read the memoized pre-swap parse.
  • The WriteOptions discriminator thread is resolved — the sentence is gone and what is left reads correctly on its own.
  • The incrementalIndexing() enumeration thread is resolved, and the added claim checks out: _commitBatchUnlocked does time releaseHold off incrementalIndexing(), and narrowing it would hand an instance-only bulk pass undefined and release the lane immediately, which is the collapse the comment describes.

Recommendations

  1. Build the config-document URL through RealmPaths, or reuse the existing realmConfigHrefFor — see the thread on #isRealmConfigDocument.
  2. Correct the "anything else … reads from the stored bytes" sentence for isIgnored — see the thread on that line.
  3. Sweep the remaining module-only prose out of the card-save suite — see the thread on a PATCH does not wait for indexing that touched no module.
  4. Nothing pins a config-document removal, though the module case gets its own test and the tag's comment directly above the config branch says a removal counts the same. I drove enqueueUpdate([<realm>realm.json], { delete: true }) against the updater and the gate does hold, so this is symmetry rather than a gap in behaviour — worth one more test, or a word on why the module case needs one and this does not.

On the "two answers" framing

I re-enumerated BatchCore independently and it is complete for the index: indexedCardValues and realmConfig are the only members that reach boxel_index. serializeCard resolves through lookupDefinition, which is backed by the modules table and moved by executable invalidation — already inside the gate. codeRefKey, resolveModuleId, storedLink and resolvedLink are identifier resolution over the virtual network. assertWriteSize reads constructor-set ceilings. fileExists, openSourceBytes and readSourceFile are the adapter. isIgnored is the one member that is neither, which is the thread above.

On self-wait

A batch writing the config document does not wait on its own pass. Both consults — the coordinator's drain inside the lock and the realm's own gate in #commitBatchUnlockedInner — happen before enqueueChanges registers the deferred, and the mixed-batch intermediate flush awaits its pass directly rather than through the gate. Two config writes in a row serialize on the file lock they already share, so the widening adds no serialization there. What it does add is that a config write now parks every other writer in the realm for the length of its pass, which by the PR's own numbers is seconds rather than the zero the gate measures today. That reads as the right trade given the alternative is staging against settings the realm has replaced — flagging it only so the cost is stated somewhere, since the description's behaviour-change section covers the read side but not this one.

Adjacent, out of scope

realm.ts carries three copies of this.paths.fileURL('realm.json').href.replace(/\.json$/, '') for the same card URL, in the config overlay, the host-routing map and the screenshot opt-in. Not this change's problem, but it is the same decision in three more places for whoever consolidates it next.

Comment on lines +297 to +302
// The realm's own config document, whose index row the realm's settings are
// overlaid from. Compared by URL rather than by name so a card that merely
// ends in `realm.json` somewhere below the root is not mistaken for it.
#isRealmConfigDocument(url: URL): boolean {
return url.href === new URL('realm.json', this.realmURL).href;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] This predicate is correct only while the base carries a trailing slash, and nothing in this file enforces that. Given a slashless base, new URL('realm.json', base) resolves against the origin rather than the realm: http://h/r yields http://h/realm.json, where new RealmPaths(base).fileURL('realm.json') yields http://h/r/realm.json. The gate would then match nothing for any realm, with no symptom other than the wait quietly not happening. It holds today only because Realm#url is RealmPaths.url, i.e. ensureTrailingSlash(...) — a hop away and invisible from here.

The decision also already has a home: realmConfigHrefFor(realmURL) in index-runner.ts computes this exact href through RealmPaths for the visit-class ordering, and realm.ts spells the card-URL form (.json stripped) three more times for the overlay, the routing map and the screenshot opt-in. Please build this one the same way — new RealmPaths(this.realmURL).fileURL('realm.json').href, or export and reuse realmConfigHrefFor — so the two copies deciding the same thing cannot diverge on base-URL handling.

Non-blocking, and no live defect: I drove the gate across every shape a realm can be mounted at (subpath, origin root, percent-encoded segment) plus the nested and query-string near-misses, and every production change set is built from paths.fileURL(...), so the two spellings agree on today's inputs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in bdfa289, and I went further than stating the invariant because the failure mode you describe is the dangerous kind — it holds for a URL no change set ever contains, so the gate silently protects nothing while every test stays green.

Both sites now resolve the document through one exported helper on RealmPaths (realmConfigHrefFor), which is the one you found in index-runner.ts; its local copy is gone. That is the right shape for a second reason beyond the trailing slash: the indexer uses it to rank this document first in a visit, so the gate and the indexer are answering the same question, and two copies of the rule would be free to drift apart.

There is now a test that pins the spelling — a realm whose URL carries no trailing slash still recognises its config document. I checked it catches the thing rather than just passing: reverted to new URL('realm.json', this.realmURL) it is the only test that reddens, 34/35.

Comment on lines +274 to +275
// Anything else a batch touches it reads from the stored bytes, which the
// write lock already makes exclusive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] isIgnored is a counterexample to this sentence, and the reason it is safely outside the gate is a different one. BatchCore.isIgnoredRealm#isIgnoredRealmIndexUpdater#isIgnored answers out of #ignoreData — not stored bytes, and not something the write lock makes exclusive. assertRemovalsAllowed in the coordinator consults it for every removal in a batch.

It genuinely does not need the gate, but only because #ignoreData moves solely on a from-scratch pass: discoverInvalidations is the one place that reads .gitignore and writes into that map, and IndexRunner reaches it from fromScratch alone — an incremental hands back the ignoreData it was given, unchanged. So the exclusion is the same one the next paragraph already argues for from-scratch, not "reads from the stored bytes".

Please say that instead. As written, someone who later makes an incremental recompute ignore rules has nothing pointing them back at this gate. Non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Correct — the sentence was too strong, and it is rewritten in bdfa289 to give the reason that actually holds rather than one that happens to reach the same conclusion.

I verified your tracing rather than taking it: discoverInvalidations is the only writer of #ignoreData, IndexRunner reaches it from the from-scratch path alone, and an incremental echoes back the ignoreData it was handed. So no pass this gate could wait for moves the ignore rules, which means a gate over incremental and copy passes is the wrong instrument for them rather than one that forgot them. The comment says that now.

This is the more useful half of the finding, for what it is worth. A claim that is true by accident reads identically to one that is true by construction, and the next person to widen this gate would have been reasoning from the accident.

Comment on lines +237 to +242
test('a PATCH does not wait for indexing that touched no module', async function (assert) {
assert.timeout(15000);
// Every in-flight pass, module-touching or not, parked forever. A write
// that waited on this set would never answer — which is what a hub
// card's instance-only fan-out is to every other writer in the realm.
// The module-touching gate is left real, and has nothing pending.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] The rename swept the identifiers in this file but not the prose, so the suite still names the module as the discriminator. A config pass touches no module and is waited for, so "does not wait for indexing that touched no module" now asserts something false about the gate. What the test pins is narrower and still true: the write does not consult the wide gate, which is the stub it installs.

Four spots: this test's name, the two "module-touching" comment lines under it, "the passes that touched an executable module" in the file header above incrementalIndexingAffectingStaging(), and "rather than on the module-touching ones" in the header's last paragraph. Suggest naming the gate rather than one of its answers — e.g. a PATCH does not wait for the realm-wide indexing gate.

Non-blocking, but this file is where a reader goes to find out what the gate means, and it is the one place left saying the old thing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[Claude Code 🤖] Fixed in bdfa289 — all four spots, and you were right that the test name was the sharpest of them. a PATCH does not wait for indexing that touched no module asserted something the gate no longer does: a config pass touches no module and is waited for, so the name would have sent someone looking for a bug that isn't there, or worse, "fixing" the gate to match it.

Renamed to a PATCH does not wait for indexing that cannot move what it resolves, with its sibling now by default a PATCH waits for in-flight indexing that can move what it resolves. The file header and the two remaining module-touching lines are reworded the same way, and I added a grep assertion to the edit so a leftover would have failed loudly rather than shipping.

The general lesson I am taking from this pass: a rename that sweeps identifiers is only half a rename, and test names are the worst place to leave the other half, because they read as specification rather than as commentary.

…nore rules are outside the gate

The gate's match for the realm config document is the same question the
indexer answers when it ranks that document first in a visit, so both now
resolve it through one helper on RealmPaths rather than keeping a copy each.
That also removes a way for the gate to be quietly wrong: resolving the name
relatively against a base with no trailing slash lands a path segment up, at
a URL no change set ever contains, and the gate would hold for nothing with
every test still green. There is now a test for that spelling.

The claim that everything else a batch resolves comes from stored bytes was
too strong: the realm's ignore rules are neither bytes nor under the write
lock. They are safely outside the gate for a different reason — only a
from-scratch pass's discovery step writes them, and an incremental echoes
back the set it was handed — so the comment states that reason instead.

The card-save suite still named the module as the gate's discriminator in
its prose and in two test names, one of which asserted something the gate no
longer does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@habdelra
habdelra requested a lite review from Copilot September 18, 2026 19:24

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@habdelra
habdelra requested a review from a team September 18, 2026 19:24
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files      1 suites   2h 37m 25s ⏱️
4 914 tests 4 900 ✅ 14 💤 0 ❌
4 929 runs  4 915 ✅ 14 💤 0 ❌

Results for commit bdfa289.

Realm Server Test Results

    1 files    244 suites   1h 28m 17s ⏱️
3 629 tests 3 629 ✅ 0 💤 0 ❌
3 680 runs  3 680 ✅ 0 💤 0 ❌

Results for commit bdfa289.

@habdelra
habdelra merged commit a66d685 into main Sep 21, 2026
66 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