Skip to content

Card reads wait only on the requester's own in-flight indexing, bounded - #6083

Merged
lukemelia merged 5 commits into
mainfrom
cs-12934-card-reads-shouldnt-block-on-unrelated-in-flight-indexing
Sep 11, 2026
Merged

Card reads wait only on the requester's own in-flight indexing, bounded#6083
lukemelia merged 5 commits into
mainfrom
cs-12934-card-reads-shouldnt-block-on-unrelated-in-flight-indexing

Conversation

@lukemelia

@lukemelia lukemelia commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Problem

getCard and the card-HTML endpoint drain all in-flight incremental indexing, with no deadline, before reading the index. When a module edit's invalidation set spans a realm's module graph (a widely-imported .gts, say), that one incremental job holds every card read on the realm until the whole reindex lands — the realm is effectively down for all of its readers, triggered by one user's ordinary write. The same hold appears when a small job merely sits queued behind other realms' work in a saturated worker pool.

The drain is a freshness courtesy, not a correctness requirement: incremental jobs write into boxel_index_working and only swap into boxel_index on completion, so a read during indexing serves the previous generation's fully consistent snapshot — never a torn one. The only reader with a read-your-writes expectation is the client whose own deferred +source POST produced the pending job.

Change

  • Jobs carry their writer. Every HTTP write route (+source POST, binary POST, card POST/PATCH, source/card DELETE, _atomic, _invalidate) tags its indexing job with the effective matrix user (post assume-user) that checkPermission records on the request context. System-originated jobs (file watcher, realm copy) and credential-less writes stay untagged — anonymous writes on a public-writable realm are not a supported configuration, and no reader waits on such a job.
  • Requests carry their identity. checkPermission records the effective user on the RequestContext (authenticatedUser), and marks requests that presented no Authorization header at all (anonymous). On the public-permission early return it opportunistically verifies a supplied token for identity only; a token that fails verification, or one accompanied by X-Boxel-Assume-User (whose indirection that path cannot validate), leaves the identity unset. Identity, not authority — no authorization decision reads it.
  • Reads gate on the requester's own jobs, bounded. drainRequestersOwnIndexing replaces the two read drains. An identified reader waits only on jobs tagged with their own principal (incrementalIndexingInitiatedBy). A provably credential-less reader skips the gate outright — anonymous writes are unsupported, so they can never be the writer — meaning public-site visitors never park behind anyone's reindex. A genuinely unknown reader (unverifiable token, assume-user on the public path, realm-internal dispatch) conservatively waits on all pending jobs. Either wait is capped by readIndexDrainBudgetMs (default 10s, a realm option so tests can shrink it), after which the read proceeds on the current index generation and the post-swap index event refreshes live clients. Prerender-originated requests skip the gate: their tab holds a render slot the awaited job may itself need.
  • Impact telemetry. Each read that arrives while incremental indexing is pending emits one realm:read-index-gate key=value line: outcome= skipped-prerender / skipped-not-writer / settled / budget-expired, with waitMs= and the principal on the waited outcomes (budget-expired at warn, the rest at info). Skipped outcomes count reads an unscoped gate would have parked; waited ones price the scoped hold. Steady-state reads emit nothing.

The write-path gates (_batchWriteUnlocked, postCard's serialization drain, _publishability, _indexing-errors) keep their existing unscoped semantics — they read back what they just wrote or deliberately report fully-settled state.

Tests

  • realm-index-updater-test.ts: the scoped gate sees only jobs tagged with that principal; untagged jobs are invisible to every scoped gate; the unscoped gate still covers everything.
  • read-index-drain-test.ts (new), at the HTTP boundary: another user's in-flight indexing never holds a read (card+json and card+html, both warmed before timing); the writer's own read waits for their job; the wait is released by the budget when the job outlives it; an authenticated writer's follow-up read sees their own deferred write indexed (end-to-end, no stubs); a wiring spy proves every HTTP write route tags its job with the writer; an anonymous reader never waits, even with every updater gate held open; an unverifiable token and an assume-user-accompanied token on the public path both take the conservative bounded hold; an identified reader on a public realm skips other writers' indexing.
  • The card-source suite's definition-change flow authenticates its write and dependent read as one user: only an identified principal has a read-your-writes claim, so a credential-less write-then-read pair has no wait to lean on.

🤖 Generated with Claude Code

…rites

A module edit whose invalidation set spans a realm's module graph was
making the realm unreadable for everyone: getCard and the card-HTML
endpoint drained ALL in-flight incremental indexing, unbounded, before
reading the index (CS-12934). The drain exists only for read-your-writes
after a deferred +source POST, and the index stays consistent throughout
(incremental jobs write to boxel_index_working and swap on completion),
so parking every reader behind any pending job was far coarser than the
guarantee requires.

Now each incremental job is tagged with the matrix user whose HTTP write
produced it, checkPermission records the effective requester identity on
the RequestContext (including an identity-only token parse on the
public-permission early return), and the read endpoints wait only on the
requester's own pending jobs, bounded by a budget (default 10s,
realm-option-tunable for tests). Readers with no verifiable identity
conservatively wait — bounded — on all pending jobs; prerender-originated
reads skip the gate entirely since their tab holds a render slot the
awaited job may itself need.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@habdelra habdelra 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.

[Claude Code 🤖] This review went after the identity plumbing end to end — every path that sets requestContext.authenticatedUser, every write handler that turns it into a job tag, and the two read gates that consume it — plus the atomic-swap claim the whole design rests on. I did not run the suite; timing behavior of the new tests is judged by reading, not by execution.

No blocking issues. The scoping and bounding are sound, and the justification holds where it matters most: Batch.done() advances realm_generations inside the same transaction as applyBatchUpdates, so a read during an incremental pass really does serve the previous generation intact. What's left is one coverage gap on the write half and two identity-edge decisions worth settling before this leaves draft.

Recommendations, in priority order:

  1. Nothing tests that the HTTP write handlers tag their jobs — the cited existing read-your-writes test runs unauthenticated and passes through the fallback. See the comment on the header of read-index-drain-test.ts.
  2. The unidentified fallback holds every anonymous read on a public realm for the full budget, which is the pathology this PR set out to remove. See the comment on drainRequestersOwnIndexing in realm.ts.
  3. On a publicly-readable-but-not-writable realm, X-Boxel-Assume-User makes a write's tag and the same client's read identity disagree, and the mismatch skips the conservative fallback. See the comment on the public-path identity capture in checkPermission.
  4. The card+html leg of the first drain test is measured cold. See the comment on that assertion.

Adjacent, out of scope for this PR:

  • _batchWriteUnlocked's own gate (await this.incrementalIndexing() when waitForIndex !== false) is still both unscoped and unbounded, so a JSON-API postCard/patchCard continues to park behind every other user's indexing — the same shape of hold, on the write side.
  • indexingErrors's comment points at realm.ts:5629 for the hazard it shares with publishability; that line reference no longer lands there.

Generated by Claude Code

Comment on lines +15 to +20
// The card read endpoints (card+json / card+html GET) gate on the
// requester's OWN in-flight incremental indexing — scoped and bounded — via
// Realm.drainRequestersOwnIndexing. These tests pin the gate's three
// behaviors at the HTTP boundary: another user's pending indexing never
// holds a read, the writer's own pending indexing does, and the hold is
// bounded by the realm's readIndexDrainBudgetMs.

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.

[Claude Code 🤖] Nothing in the PR exercises the write half of the feature — that requestContext.authenticatedUser actually reaches enqueueUpdate as initiatedBy.

These HTTP tests replace both gate methods with stubs, so the updater never sees a real job, and realm-index-updater-test.ts calls enqueueUpdate(urls, { initiatedBy }) directly. The existing coverage cited in the description doesn't close it either: can serialize a card instance correctly after card definition is changed sits in the card-source POST requestpublic writable realm module ('*': ['read', 'write']) and sends no Authorization header on either the +source POST or the follow-up GET. So the job is untagged, the GET is unidentified, and the hold comes from the conservative all-jobs fallback — that test passes with every initiatingUser: argument deleted from realm.ts.

That leaves the five initiatingUser: requestContext.authenticatedUser ?? null call sites as the one path that can silently regress read-your-writes for authenticated users: drop the tag on any one handler and its own writer's read stops waiting, with nothing turning red.

Worth one test in a permissioned module — essentially the rename test above, moved behind auth: POST +source with a user's JWT, then GET card+json with the same JWT, asserting the post-rename serialization. Without the tag that GET returns the old schema immediately, so the assertion fails rather than hangs.

Follow-up-shaped but in this PR's scope, non-blocking.


Generated by Claude Code

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 🤖] Covered in 92924afa39 and 7e76658d3e. Two layers: an authenticated end-to-end rename test (+source POST with a user's JWT, follow-up GET as the same user asserts the post-rename serialization — it fails, not hangs, if the tag is dropped), and the spy test you sketched: enqueueUpdate is shadowed to capture initiatedBy while delegating to the prototype, and each HTTP write route (+source POST, card POST, card PATCH, _invalidate, _atomic, card DELETE, source DELETE) is driven once as the user, asserting each captured tag.

Comment thread packages/runtime-common/realm.ts Outdated
Comment on lines +6590 to +6594
let user = requestContext.authenticatedUser;
let pending =
user !== undefined
? this.#realmIndexUpdater.incrementalIndexingInitiatedBy(user)
: this.incrementalIndexing();

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.

[Claude Code 🤖] The unidentified fallback keeps the pathology this PR removes, for the reader population most exposed to it. On a '*': ['read'] realm an anonymous GET has no identity, so it takes incrementalIndexing() and holds for the full readIndexDrainBudgetMs — 10s by default — on every read for as long as a wide module reindex is in flight. Published realms, og:image fetches, and crawlers are all in that bucket, and each expired hold also emits a warn, so a long reindex becomes 10s plus a log line per anonymous request.

For a request that carries no Authorization header at all, the conservative branch isn't buying anything. Every tagged job comes from an authenticated write, so a credential-less caller provably isn't the writer; and the untagged jobs (file watcher, realm copy) have no reader with a read-your-writes claim on them either. The genuinely ambiguous cases are narrower: a token that was sent but failed verification, and the isLocal dispatch that skips checkPermission entirely.

Suggest splitting them — skip the gate when no Authorization header was presented and the request isn't a local dispatch, keep the conservative wait for token-present-but-unverified and for isLocal. That needs internalHandle's isLocal recorded on the RequestContext alongside authenticatedUser, since the handler can't currently tell the two "no identity" cases apart.

This would flip the an anonymous reader conservatively waits test, so it's a decision rather than a fix — non-blocking, but worth settling before this leaves draft.


Generated by Claude Code

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 🤖] Resolved in 7e76658d3e via the sentinel form suggested in the sibling thread here, which goes further than the header-based split: a credential-less write tags its job with the shared ANONYMOUS_REQUESTER principal and a credential-less read waits only on jobs so tagged. Anonymous reads never park behind an identified user's reindex, anonymous write-then-read on a public-writable realm stays consistent, and untagged system jobs hold no scoped reader. The conservative all-jobs hold now applies only to an unverifiable token, an assume-user indirection the public path cannot validate, or a realm-internal dispatch.

Comment thread packages/runtime-common/realm.ts Outdated
Comment on lines +5022 to +5024
// authorized, so they simply leave the identity unset. The
// `X-Boxel-Assume-User` indirection is not honored on this path —
// honoring it requires the assume-user permission check the main path

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.

[Claude Code 🤖] What keeps the identity symmetric between a client's write and its own read on a realm that is publicly readable but not publicly writable?

Tracing that case: the write carries requiredPermission: 'write', misses this branch (no '*': ['write']), runs the main path, and is tagged with the assumed user. The read carries requiredPermission: 'read', hits this branch, and is identified as the token bearer. The reader then looks identified, so incrementalIndexingInitiatedBy(bearer) finds nothing — and because identity is set, it does not fall back to the conservative wait either. Read-your-writes is off for that caller, with no signal.

X-Boxel-Assume-User isn't rare traffic: worker.ts's job fetcher and the realm's own _fetch both set it unconditionally to the realm owner, so any cross-realm request out of those paths carries it.

If nothing holds the symmetry, the cheap version needs no matrix round-trip: when the request carries X-Boxel-Assume-User and identity was captured on this path, leave authenticatedUser unset rather than recording the bearer — falling back to the conservative gate is correct precisely because we don't know who the requester is.

Non-blocking.


Generated by Claude Code

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 92924afa39: on the public-permission path a request carrying X-Boxel-Assume-User records no identity at all, so it lands in the conservative bounded hold rather than being mis-identified as the bearer — the write's tag (the assumed user, via the main path) and the read's identity can no longer disagree silently. Covered by the 'token accompanied by X-Boxel-Assume-User is not trusted for identity on the public path' test.

Comment on lines +134 to +149
startedAt = Date.now();
let htmlResponse = await getPersonAs(
'john',
['read'],
'application/vnd.card+html',
);
elapsed = Date.now() - startedAt;
assert.strictEqual(
htmlResponse.status,
200,
`card+html HTTP 200: ${htmlResponse.text}`,
);
assert.true(
elapsed < NO_WAIT_CEILING_MS,
`card+html read returned without waiting on the writer's job (took ${elapsed}ms)`,
);

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.

[Claude Code 🤖] The warm-up at the top of the test uses getPersonAs's default accept (card+json), so this is the first card+html request on the realm and is timed cold against NO_WAIT_CEILING_MS — exactly the cold assembly the warm-up exists to keep out of the measurement. Add a card+html warm-up alongside the json one. Non-blocking; flake risk on a loaded CI box.


Generated by Claude Code

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 7e76658d3e — the test warms the card+html endpoint alongside card+json before any timed request.

@backspace backspace 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.

[Claude Code 🤖] Reviewed the read gate end to end: which checkPermission path yields an identity, whether every HTTP write reaches the updater tagged, what the unidentified fallback actually waits on, and what the prerender skip is reachable from. Not covered: the write-path gates the description scopes out, and how the per-process deferreds behave across realm-server replicas.

Bottom line: no blocking issues. Two non-blocking asks:

  1. The unidentified fallback still parks anonymous readers of public-read-only realms behind other users' tagged jobs; tag anonymous writes with a sentinel instead of falling back to the unscoped gate — see the comment on drainRequestersOwnIndexing in realm.ts.
  2. Nothing executable pins the handler → initiatedBy wiring; an enqueueUpdate spy driven across the write routes closes it — see the comment at the top of read-index-drain-test.ts.

Comment thread packages/runtime-common/realm.ts Outdated
Comment on lines +6590 to +6594
let user = requestContext.authenticatedUser;
let pending =
user !== undefined
? this.#realmIndexUpdater.incrementalIndexingInitiatedBy(user)
: this.incrementalIndexing();

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.

[Claude Code 🤖] An unidentified reader waits on every pending job here, other users' tagged ones included. On a realm that is public-readable but not public-writable that requester provably isn't the writer — every HTTP write there was authorized on the token path and is tagged, so the only untagged jobs are system-originated — yet anonymous card reads still park behind an identified user's module reindex for the full budget. That is the hold this change sets out to remove, bounded rather than gone, for the anonymous surface (e.g. anonymous API reads of the catalog realm while an admin's module edit reindexes).

Tag the anonymous case rather than falling back to the unscoped gate: an HTTP write with no authenticatedUser passes a sentinel (initiatingUser: requestContext.authenticatedUser ?? ANONYMOUS_REQUESTER, best via one helper the seven handlers share), and this gate calls incrementalIndexingInitiatedBy(user ?? ANONYMOUS_REQUESTER). Untagged jobs then hold no reader, anonymous read-your-writes on public-writable realms still works, and the anonymous +source POST → GET rename test in card-source-endpoints-test.ts keeps passing. Branching on permissions from here doesn't work: createRequestContext rewrites a read's permissions to '*': ['read'] on world-readable realms, so '*': write isn't visible at this point.

Design gap in the new gate rather than a regression against main, which held these readers unbounded. Non-blocking, but small enough to land in this PR; the "anonymous reader conservatively waits" test would flip to asserting it waits only on anonymous-tagged jobs.

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 🤖] Adopted in 7e76658d3e, exactly as proposed: ANONYMOUS_REQUESTER is the shared read-your-writes principal, derived on both sides by one requesterPrincipal helper (the seven handlers pass it as initiatingUser; the gate calls incrementalIndexingInitiatedBy with it). Untagged jobs hold no scoped reader, anonymous read-your-writes on public-writable realms still works (the anonymous rename flow keeps passing), and the public-realm tests now assert both directions — anonymous skips identified users' jobs, anonymous waits on anonymous-initiated jobs.

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.

[Claude Code 🤖] Resolves it, in 7e76658d3e: requesterPrincipal derives the principal identically on the write and read sides, credential-less writes tag ANONYMOUS_REQUESTER, and untagged system jobs hold no scoped reader. Verified against the anonymous rename flow in card-source-endpoints-test.ts and the green realm-server shards on 30dfce7d67.

Comment on lines +22 to +26
// The updater's gates are stubbed per test rather than racing real jobs: a
// real incremental job settles as fast as the worker runs it, so "the read
// did not wait" could never be asserted deterministically against one.
// (RealmIndexUpdater's own tagging/filtering is covered in
// realm-index-updater-test.ts.)

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.

[Claude Code 🤖] Nothing executable pins the wiring between requestContext.authenticatedUser and the job tag. These tests stub both gates, the updater tests construct tagged jobs directly, and the existing +source POST → GET read-your-writes test in card-source-endpoints-test.ts runs anonymously on a public-writable realm, so it passes through the unscoped fallback. A new write handler that omits initiatingUser, or one of the seven existing ones losing it, silently disables read-your-writes for that route with no test going red.

The deterministic version doesn't need to race a real job: shadow realm.realmIndexUpdater.enqueueUpdate the way stubUpdaterGates shadows the gates, delegating to the prototype but capturing opts.initiatedBy (the synchronous update path calls through this.enqueueUpdate, so one spy covers both), then drive each write route once as hassan — +source POST, card+json POST, PATCH, DELETE, _atomic — and assert the captured tag. Test-coverage gap, 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 🤖] Added in 7e76658d3e as described: the spy shadows enqueueUpdate on the updater instance (delegating to the prototype, so the synchronous update path is covered by the same shadow), and one test drives +source POST, card POST, card PATCH, _invalidate, _atomic, card DELETE, and source DELETE as an authenticated user, asserting each captured initiatedBy. The authenticated end-to-end rename test from the sibling thread backs it with a black-box check.

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.

[Claude Code 🤖] Resolves it, in 7e76658d3e: the enqueueUpdate spy covers all seven HTTP write routes including _invalidate, and the authenticated end-to-end rename test fails on the old schema if the +source tag is dropped.

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.

🟡 Changes recommended

The invalidate path misses user propagation, and authenticated read-your-writes coverage is incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR bounds card-read waits to the requesting user’s in-flight indexing jobs while preserving read-your-writes behavior.

Changes:

  • Tags indexing jobs with their initiating user.
  • Adds bounded scoped drains for card JSON/HTML reads.
  • Adds updater, HTTP-boundary, and test-helper coverage.

Review findings:

  • realm.ts:1756Critical (2 votes): The /_invalidate handler does not propagate the authenticated user.
  • realm.ts:5190Moderate (1 vote): Add authenticated private-realm coverage for same-user read-your-writes behavior.
File summaries
File Description
packages/runtime-common/realm.ts Tracks request identity and applies scoped read drains.
packages/runtime-common/realm-index-updater.ts Supports user-scoped indexing gates.
packages/realm-server/tests/realm-index-updater-test.ts Tests scoped and untagged job behavior.
packages/realm-server/tests/read-index-drain-test.ts Tests bounded HTTP read behavior.
packages/realm-server/tests/helpers/index.ts Exposes the configurable drain budget.
Review details

Suppressed comments (1)

packages/runtime-common/realm.ts:5190

  • [Claude Code 🤖] The HTTP tests stub the read gate but never perform a token-authenticated deferred write; the existing card-source read-your-writes case uses an anonymous public request. A regression that drops initiatingUser here would therefore still pass, while the subsequent same-user GET would skip the untagged job. Please add an authenticated private-realm integration case that verifies the POST's job is visible only to that user's read gate.
        initiatingUser: requestContext.authenticatedUser ?? null,
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

await this.#realmIndexUpdater.update(urls, {
...(opts?.delete ? { delete: true } : {}),
clientRequestId: opts?.clientRequestId ?? null,
initiatedBy: opts?.initiatedBy ?? null,

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 🤖] Tagged in 7e76658d3einvalidateURLs now passes the requester principal like every other write route, and the wiring spy test asserts it. One correction to the failure mode described: this handler awaits the synchronous updateIndexAndCollectInvalidations, so its 204 returns only after the job settles — a follow-up read by the same caller could not race it. The tag matters for the caller's concurrent reads from other tabs and keeps the principal derivation uniform across routes.

… under assume-user; gate telemetry

Review on the draft surfaced three identity-edge decisions and a coverage
gap, all adopted:

- A caller with no Authorization header provably isn't the writer behind
  any tagged job (every tagged job comes from an authenticated write), so
  anonymous reads now skip the gate instead of taking the conservative
  full-budget hold — that hold was the pathology this change removes,
  landing on published realms, og:image fetches, and crawlers.
- On the public-permission path, a token accompanied by
  X-Boxel-Assume-User no longer records the bearer as the identity: the
  write it pairs with is tagged with the assumed user (writes always take
  the main path), so recording the bearer would silently skip
  read-your-writes. Identity stays unset and the conservative gate covers
  the ambiguity.
- The write-half of the feature (handlers tagging jobs with
  requestContext.authenticatedUser) now has an authenticated end-to-end
  test; the public-writable rename flow can't cover it since its job is
  untagged and its reader unidentified.
- The card+html leg of the no-wait test gets its own warm-up so it isn't
  timed cold.

Also adds impact telemetry Luke asked for: one realm:read-index-gate
key=value line per card read that arrives while incremental indexing is
pending (outcome= skipped-prerender/skipped-not-writer/skipped-anonymous/
settled/budget-expired, waitMs on waits; budget-expired at warn). Skipped
outcomes count reads the unscoped gate would have parked; waited ones
price the writer-scoped hold. Steady-state reads emit nothing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lukemelia
lukemelia marked this pull request as ready for review September 10, 2026 21:29
…ymous reads

Review on the previous revision surfaced that skipping the read gate for
credential-less callers broke anonymous read-your-writes (a +source POST
then GET on a public-writable realm could serve the pre-write schema),
while the conservative fallback it replaced held every anonymous reader
for the full budget during an identified user's reindex — the pathology
this branch removes.

The sentinel design serves both: a write authorized without credentials
tags its indexing job with ANONYMOUS_REQUESTER, and a credential-less
read waits (bounded) only on jobs so tagged. Untagged system jobs (file
watcher, realm copy) hold no scoped reader. The requesterPrincipal helper
derives the principal identically on the write and read sides. Also tags
the _invalidate handler's job, adds a wiring spy test covering every HTTP
write route's job tag, warms the card+html leg before timing it, and
holds the unscoped gate open in the writer-wait tests so the gate's
fast-path pending probe sees the stubbed job.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Host Test Results

    1 files  ±0      1 suites  ±0   2h 35m 56s ⏱️ + 7m 52s
4 739 tests +5  4 725 ✅ +5  14 💤 ±0  0 ❌ ±0 
4 754 runs  +5  4 740 ✅ +5  14 💤 ±0  0 ❌ ±0 

Results for commit f8281fb. ± Comparison against earlier commit 30dfce7.

Realm Server Test Results

    1 files  ±0    211 suites  ±0   1h 15m 28s ⏱️ + 5m 18s
2 775 tests  - 1  2 775 ✅  - 1  0 💤 ±0  0 ❌ ±0 
2 814 runs   - 1  2 814 ✅  - 1  0 💤 ±0  0 ❌ ±0 

Results for commit f8281fb. ± Comparison against earlier commit 30dfce7.

…ntial-less

Four atomic-endpoints tests POST /_atomic with a JWT and then GET the
written card with no Authorization header. The read gate scopes
read-your-writes to the requester's principal, and a credential-less
read acts as the shared anonymous principal — it does not wait on the
authenticated write's indexing job, so the GET raced the deferred job
and read the index before the row landed. Real clients read with the
session they wrote with; these tests mixed identities. Use the atomic
endpoint's ?waitForIndex=true so the write itself is synchronous.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lukemelia
lukemelia requested a review from a team September 10, 2026 23:57

@backspace backspace 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.

[Claude Code 🤖] Re-reviewed 30dfce7d67 for convergence: each thread's fix verified as a change against bcb7557797, the tag sweep re-run across every HTTP write site, and the anonymous and mixed-credential flows traced through checkPermissionrequesterPrincipal → the gate.

Approved: no blocking issues remain. Two non-blocking notes on the new test file, inline.

Earlier threads:

  1. Anonymous fallback held readers behind identified users' jobs — resolved in 7e76658d3e (reply in thread).
  2. Handler → tag wiring unpinned — resolved in 7e76658d3e (reply in thread).

Other reviewers' threads, as verified facts: the authenticated end-to-end and spy tests, the assume-user identity rule on the public path, the card+html warm-up, and the _invalidate tag all land as changes in 92924afa39 / 7e76658d3e, and the anonymous rename flow in card-source-endpoints-test.ts still gets read-your-writes through the shared principal. Those threads are left for their authors to resolve.

Percy is the only red check: one unreviewed visual change on a PR that touches no host code, so approve it in Percy or ignore it.

Comment on lines +224 to +229
// End-to-end (no stubs): the authenticated write handlers must tag their
// deferred indexing jobs with the writer, or this same-user
// write-then-read sees the pre-write schema. The public-writable variant
// of this flow cannot cover the tagging — an unauthenticated write
// produces an untagged job and an unidentified read takes the
// conservative all-jobs hold, so it passes with the tags removed.

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.

[Claude Code 🤖] This comment predates the shared principal: a credential-less write on a public-writable realm now tags its job ANONYMOUS_REQUESTER and a credential-less read waits on exactly those jobs, so the public-writable variant would read the pre-rename schema (non-deterministically) if a handler dropped its tag, rather than pass. Either rewrite the second sentence to the reason the authenticated variant is kept (a specific identity the spy test below can assert on, and the private-realm path is the production case) or drop it. Non-blocking.

Comment on lines +286 to +299
let readBack = await request
.get(new URL(id).pathname)
.set('Accept', 'application/vnd.card+json')
.set('Authorization', auth());
assert.strictEqual(readBack.status, 200, `HTTP 200: ${readBack.text}`);
let attributes = readBack.body.data.attributes;
assert.true(
'field2a' in attributes,
`post-rename schema is served: ${JSON.stringify(attributes)}`,
);
assert.false(
'field2' in attributes,
`pre-rename field is gone: ${JSON.stringify(attributes)}`,
);

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.

[Claude Code 🤖] This flow's pass depends on the real rename job (module rewrite plus instance re-render through the prerenderer) settling inside this module's 4 s readIndexDrainBudgetMs; past the budget the read proceeds on the old generation and the field2a assertion fails, so a slow CI box turns a correct read-your-writes into a red run. The shrunk budget exists for the budget-expiry test above, not for this one. Give this test its own module with the default (or a generous) budget, or leave it here only if you're willing to accept that flake shape. Non-blocking.

Public anonymous-writable realms are not a configuration we support, so a
provably credential-less caller can never have a write in flight — which
means they never have a read-your-writes claim on pending indexing. The
read gate now answers them with skipped-not-writer immediately instead of
scoping them to a shared anonymous principal.

That deletes the ANONYMOUS_REQUESTER concept entirely: the constant, the
requesterPrincipal helper (the write sites read
requestContext.authenticatedUser directly), the anonymous-principal
tagging of credential-less writes (their jobs are now untagged, like
system-originated ones), and the anonymous write-then-read consistency
argument in the comments. The anonymous flag on RequestContext stays: it
is what distinguishes a provably credential-less caller (skip) from one
whose identity is merely unknown (conservative bounded hold).

The two anonymous gate tests collapse into one stronger case: with every
updater gate held open, a credential-less read still returns immediately.
One existing test depended on the removed behavior — the card-source
suite's definition-change test wrote and read back credential-lessly on a
public-writable realm — and now authenticates that write/read pair as the
same user, keeping authorization on the realm's public write permission.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lukemelia
lukemelia merged commit 7063fc5 into main Sep 11, 2026
65 of 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.

4 participants