Skip to content

flaky test fix: Track card-api and file-api deps directly in the render route#4875

Closed
habdelra wants to merge 5 commits into
mainfrom
worktree-fix-prerender-file-deps-flake
Closed

flaky test fix: Track card-api and file-api deps directly in the render route#4875
habdelra wants to merge 5 commits into
mainfrom
worktree-fix-prerender-file-deps-flake

Conversation

@habdelra
Copy link
Copy Markdown
Contributor

@habdelra habdelra commented May 18, 2026

Problem

Realm Server Tests on PR #4872 failed at:

not ok 70 prerendering-test.ts > prerender - non-mutating tests > runner behavior > file prerender returns extracted metadata
  message: deps include base file-api module
  severity: failed
  actual  : false
  expected: true

The file extract's returned deps is supposed to include https://cardstack.com/base/file-api — that's the public URL the indexer references when deciding whether to invalidate a previously-extracted file. The assertion was relying on the runtime-dependency tracker to surface that URL, but the path that actually placed it there was incidental: matrix-service.fileAPIModule = importResource(() => 'file-api') happens to call loader.import('file-api') during matrix-service construction, and trackRuntimeModuleDependency runs synchronously inside that call. Whether it landed inside the route's active tracking-session window depended on microtask scheduling — fine on a fresh page, flaky after a BrowserManager.restartBrowser() (prior test 68's recovery render) or any other timing perturbation.

The render route doesn't actually need matrix-service for anything; it only needed the dep recorded.

Solution

In RenderRoute.#buildModel, await Promise.all of this.loaderService.loader.import for both card-api and file-api. This records each as a runtime dep of the current render against the active tracking session. The loader caches the modules so subsequent renders re-track without re-evaluating, and putting the await inside #buildModel keeps the promise stored in #modelPromises covering the imports so concurrent same-key model() calls dedup correctly.

Files

  • packages/host/app/routes/render.ts

Test plan

The base FileDef class is implemented in card-api but consumers import it
from file-api, which is also the URL the indexer references when
invalidating file extracts. Previously file-api only entered the dep set
via the runtime tracker — matrix-service's fileAPIModule importResource
calls loader.import('file-api') during service init, which records the
dep against whatever tracking session is active when the task body runs.

The route lifecycle doesn't guarantee that import lands inside the active
session window: beforeModel triggers lazy realm-service/matrix-service
injection, the import task queues, and then model() opens the tracking
session. On a fresh page right after a BrowserManager.restartBrowser()
the import body usually slips inside the session; on a reused page the
module is already cached, the task never reruns, and file-api silently
drops from the dep set on subsequent renders.

Promote file-api to a deterministic static dep on every file-extract,
and warn (with both the extractor's static deps and the tracker's
snapshot) when the runtime tracker missed it — so a future regression in
the tracking-session lifecycle is debuggable from the prerender log
without re-deriving the race.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@habdelra habdelra requested a review from Copilot May 18, 2026 23:22
@habdelra habdelra changed the title Stabilize file-extract deps: pin file-api as static dep flaky test fix - Stabilize file-extract deps: pin file-api as static dep May 18, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Preview deployments

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 stabilizes file-extract dependency reporting by ensuring the base file-api module is always included as a dependency, instead of relying on runtime dependency tracking timing.

Changes:

  • Adds baseRealm and a route-specific logger.
  • Pins ${baseRealm.url}file-api into merged file-extract deps.
  • Logs a warning when both runtime tracking and extractor deps missed file-api.

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

Comment thread packages/host/app/routes/render/file-extract.ts Outdated
The previous commit pinned file-api as a static dep to mask the race
between matrix-service's `fileAPIModule = importResource(() => 'file-api')`
import and the render route's tracking-session window. That covered the
symptom for the file-extract path but left the underlying problem in
place — and the same race could drop other matrix-service-loaded modules
from any render's dep set.

Address the race directly:

- Stamp `__boxelRenderContext = true` at the start of beforeModel,
  BEFORE `restoreSessionsFromStorage()`. RealmResource.tokenRefresher
  reads this flag synchronously on its first run; without the stamp it
  proceeds to its delayed loginTask, which lazily injects matrix-service
  outside any tracking session and lands its loader.import('file-api')
  in a no-session void.
- After `beginRuntimeDependencyTrackingSession()` in model(), await
  `this.matrixService.ready`. This is both the first access to
  matrix-service in the render lifecycle (now guaranteed to be inside
  the session window) and the await on its existing public readiness
  promise — its loader.import calls inside loadSDK record their deps
  against THIS render's session.

Revert the file-extract static-dep workaround; with matrix-service init
deferred to inside the session, the runtime tracker observes file-api
deterministically on every render.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@habdelra habdelra marked this pull request as draft May 18, 2026 23:46
… coupling

In RenderRoute.beforeModel, stamp __boxelRenderContext before touching
the realm service so RealmResource.tokenRefresher short-circuits and
does not lazily inject matrix-service in the prerender Chrome.

In RenderRoute.model, immediately after opening the runtime-dep tracking
session, await Promise.all of loader.import for card-api and file-api.
The loader caches the modules so subsequent renders are a cheap re-track,
and the deps are now recorded deterministically against THIS render's
session without dragging in matrix-service (and its matrix SDK + client
+ event-binding setup that the prerender never uses).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@habdelra habdelra changed the title flaky test fix - Stabilize file-extract deps: pin file-api as static dep Track card-api and file-api deps directly in the render route May 18, 2026
@habdelra habdelra marked this pull request as ready for review May 18, 2026 23:54
@habdelra habdelra requested a review from Copilot May 18, 2026 23:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread packages/host/app/routes/render.ts Outdated
Move the card-api / file-api loader.import await out of model() and into
#buildModel so the cached entry in #modelPromises covers the imports.
Concurrent same-key model() calls (the render -> child-route transition
flow that intentionally relies on #modelPromises for dedup) no longer
race past the cache check while the imports are in flight.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@habdelra habdelra changed the title Track card-api and file-api deps directly in the render route flaky test fix: Track card-api and file-api deps directly in the render route May 19, 2026
@habdelra habdelra requested a review from Copilot May 19, 2026 00:03
Comment thread packages/host/app/routes/render.ts Outdated
Restore the original beforeModel ordering. With card-api / file-api
tracked deterministically by the loader.import await in #buildModel,
the early stamp is no longer needed to keep file-api in the dep set —
tokenRefresher's lazy matrix-service injection (if it ever fires) no
longer affects render correctness.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

packages/host/app/routes/render.ts:199

  • Setting __boxelRenderContext before restoreSessionsFromStorage() but registering the cleanup only afterward means any exception during session restore (for example, malformed JSON in the sessions localStorage entry) leaves the global render-context flag set after the transition aborts. Register cleanup or use a try/finally before calling code that can throw so later host behavior is not incorrectly treated as prerender context.
    this.#authGuard.register();

@habdelra habdelra requested a review from a team May 19, 2026 00:17
@habdelra
Copy link
Copy Markdown
Contributor Author

this approach seems to have introduced a lot of instability. backing off on it

@habdelra habdelra closed this May 19, 2026
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