diff --git a/packages/host/app/resources/file.ts b/packages/host/app/resources/file.ts index 45756e190a..f764604f43 100644 --- a/packages/host/app/resources/file.ts +++ b/packages/host/app/resources/file.ts @@ -5,7 +5,7 @@ import { service } from '@ember/service'; import { tracked } from '@glimmer/tracking'; import { parse } from 'date-fns'; -import { restartableTask } from 'ember-concurrency'; +import { didCancel, restartableTask } from 'ember-concurrency'; import { Resource } from 'ember-modify-based-class-resource'; import { @@ -20,6 +20,7 @@ import type CardService from '@cardstack/host/services/card-service'; import type { SaveType } from '@cardstack/host/services/card-service'; import type OperatorModeStateService from '@cardstack/host/services/operator-mode-state-service'; +import type RealmService from '@cardstack/host/services/realm'; import type RecentFilesService from '@cardstack/host/services/recent-files-service'; import type StoreService from '@cardstack/host/services/store'; @@ -143,6 +144,7 @@ class _FileResource extends Resource { @service declare private cardService: CardService; @service declare private recentFilesService: RecentFilesService; @service declare private operatorModeStateService: OperatorModeStateService; + @service declare private realm: RealmService; @service declare private store: StoreService; constructor(owner: Owner) { @@ -178,6 +180,28 @@ class _FileResource extends Resource { this._url = url; this.onStateChange = onStateChange; this.onRedirect = onRedirect; + + // Subscribe to realm events BEFORE the first fetch so a 404 result + // (e.g. the AI assistant navigates code-submode to a file it just + // created, before realm indexing has caught up) can still be recovered + // when the realm subsequently broadcasts an `index/incremental` event + // for this URL. Without this, the success-branch `setSubscription` + // below at the end of `read` is never reached on the 404 path, leaving + // the resource permanently in `not-found` despite the realm having + // since delivered the file. + let realmId = this.realm.realmOf(rri(url)); + if (realmId) { + this.setSubscription(realmId, this.onRealmInvalidation); + } else { + // No early subscription possible — the realm service hasn't yet + // discovered the realm that owns this URL. Recovery from an initial + // 404 then depends on the success-branch `setSubscription` inside + // `read`, which only fires if the fetch eventually succeeds. + log.debug( + `FileResource: no known realm for ${url} at modify-time; deferring subscription to read-success branch`, + ); + } + this.read.perform(); } @@ -219,6 +243,17 @@ class _FileResource extends Resource { return; } } catch (err: any) { + // `read` is restartable: when an invalidation event arrives while + // a read is in flight, `read.perform({force: true})` cancels this + // instance and starts a fresh one. ember-concurrency surfaces the + // cancel as a TaskCancelation thrown at the awaited fetch. The + // fresh task's `updateState({ state: 'ready' })` can land before + // the cancelled task's awaited promise resolves; treating the + // cancellation as a real fetch failure would then overwrite that + // ready state with `not-found`. + if (didCancel(err)) { + return; + } log.error(`Could not get file ${this._url}, err: ${err.message}`); this.updateState({ state: 'not-found', url: rri(this._url) }); return; @@ -285,86 +320,112 @@ class _FileResource extends Resource { }, }); - this.setSubscription(realmURL, (event: RealmEventContent) => { - if ( - event.eventName !== 'index' || - // we wait specifically for the index complete event ("incremental") so - // that the subsequent index read retrieves the latest contents of the file - event.indexType !== 'incremental' || - !Array.isArray(event.invalidations) - ) { - return; - } + this.setSubscription(realmURL, this.onRealmInvalidation); + }); - let { invalidations } = event as { invalidations: string[] }; - let normalizedURL = this.url.endsWith('.json') - ? this.url.replace(/\.json$/, '') - : this.url; + private onRealmInvalidation = (event: RealmEventContent): void => { + if ( + event.eventName !== 'index' || + // we wait specifically for the index complete event ("incremental") so + // that the subsequent index read retrieves the latest contents of the file + event.indexType !== 'incremental' || + !Array.isArray(event.invalidations) + ) { + return; + } - if (invalidations.includes(normalizedURL)) { - realmEventsLogger.trace( - `file resource ${normalizedURL} processing invalidation`, - event, - ); + let { invalidations } = event as { invalidations: string[] }; + // Match invalidations against both the currently-requested URL + // (`this._url`, kept current by `modify`) and the URL the resource + // most recently loaded into `innerState`. Both are necessary + // because: + // - `innerState.url` may be a realm-canonicalized form of `_url` + // (e.g. `experiments/author` redirects to `experiments/author.gts`) + // and the realm emits invalidations for the canonical form. + // - During a transition (modify called with a new URL while + // innerState still holds a prior file), `innerState.url` is + // stale; only `_url` reflects what the caller is asking for — + // dropping the event here would orphan the new file. + let normalize = (raw: string) => + raw.endsWith('.json') ? raw.replace(/\.json$/, '') : raw; + let candidates = new Set([normalize(this._url)]); + if (this.innerState.state !== 'loading') { + candidates.add(normalize(this.innerState.url)); + } + let normalizedURL = invalidations.find((inv) => candidates.has(inv)); - let clientRequestId = event.clientRequestId; - let reloadFile = false; + if (normalizedURL) { + realmEventsLogger.trace( + `file resource ${normalizedURL} processing invalidation`, + event, + ); - if (!clientRequestId || clientRequestId.startsWith('instance:')) { - reloadFile = true; + let clientRequestId = event.clientRequestId; + let reloadFile = false; + + if (!clientRequestId || clientRequestId.startsWith('instance:')) { + reloadFile = true; + realmEventsLogger.debug( + `reloading file resource ${normalizedURL} because realm event has ${!clientRequestId ? 'no clientRequestId' : 'clientRequestId from instance editor'}`, + ); + } else if ( + clientRequestId.startsWith('editor:') || + clientRequestId.startsWith('editor-with-instance:') + ) { + if (this.cardService.clientRequestIds.has(clientRequestId)) { realmEventsLogger.debug( - `reloading file resource ${normalizedURL} because realm event has ${!clientRequestId ? 'no clientRequestId' : 'clientRequestId from instance editor'}`, + `ignoring because request id is contained in known clientRequestIds`, + event.clientRequestId, ); - } else if ( - clientRequestId.startsWith('editor:') || - clientRequestId.startsWith('editor-with-instance:') - ) { - if (this.cardService.clientRequestIds.has(clientRequestId)) { - realmEventsLogger.debug( - `ignoring because request id is contained in known clientRequestIds`, - event.clientRequestId, - ); - } else { - reloadFile = true; - realmEventsLogger.debug( - `reloading file resource ${normalizedURL} because request id is ${clientRequestId}, not contained within known clientRequestIds`, - Object.keys(this.cardService.clientRequestIds), - ); - } - } else if (clientRequestId.startsWith('bot-patch:')) { + } else { reloadFile = true; realmEventsLogger.debug( - `reloading file resource ${normalizedURL} because request id is ${clientRequestId}`, + `reloading file resource ${normalizedURL} because request id is ${clientRequestId}, not contained within known clientRequestIds`, + Object.keys(this.cardService.clientRequestIds), ); } + } else if ( + clientRequestId.startsWith('bot-patch:') || + // create-file writes originate from this host (cardService.saveSource + // with saveType 'create-file' — the path WriteTextFileCommand uses) + // but the FileResource may not yet have any content because its first + // fetch raced indexing and 404'd. The clientRequestId being in + // cardService.clientRequestIds does NOT imply we already have the + // content (unlike the editor: case), so we still need to reload. + clientRequestId.startsWith('create-file:') + ) { + reloadFile = true; + realmEventsLogger.debug( + `reloading file resource ${normalizedURL} because request id is ${clientRequestId}`, + ); + } - if (reloadFile) { - // Mirrors the store's invalidation path: only reset the loader when - // the rewritten module has actually been imported (which includes - // entries cached as `state: 'broken'`). Resetting unconditionally - // would clone the whole loader on every external write — including - // boxel-cli writes for modules the host never loaded — and drop - // unrelated loaded modules. clearFetchCache is required because - // the module endpoint's ETag is keyed on unix-second-granularity - // `lastModified`; without it, a write landing in the same second - // as the prior fetch can be served as a 304 with the old broken - // body. The store only covers realms it subscribed to (i.e. ones - // it loaded a card instance from), so code-mode-only browsing of - // a .gts whose realm has no loaded instance relies on this path. - if ( - hasExecutableExtension(normalizedURL) && - this.loaderService.loader.isModuleLoaded(normalizedURL) - ) { - this.loaderService.resetLoader({ - clearFetchCache: true, - reason: 'file-resource-external-invalidation', - }); - } - this.read.perform({ force: true }); + if (reloadFile) { + // Mirrors the store's invalidation path: only reset the loader when + // the rewritten module has actually been imported (which includes + // entries cached as `state: 'broken'`). Resetting unconditionally + // would clone the whole loader on every external write — including + // boxel-cli writes for modules the host never loaded — and drop + // unrelated loaded modules. clearFetchCache is required because + // the module endpoint's ETag is keyed on unix-second-granularity + // `lastModified`; without it, a write landing in the same second + // as the prior fetch can be served as a 304 with the old broken + // body. The store only covers realms it subscribed to (i.e. ones + // it loaded a card instance from), so code-mode-only browsing of + // a .gts whose realm has no loaded instance relies on this path. + if ( + hasExecutableExtension(normalizedURL) && + this.loaderService.loader.isModuleLoaded(normalizedURL) + ) { + this.loaderService.resetLoader({ + clearFetchCache: true, + reason: 'file-resource-external-invalidation', + }); } + this.read.perform({ force: true }); } - }); - }); + } + }; writeTask = restartableTask( async ( diff --git a/packages/host/tests/acceptance/code-submode/create-file-test.gts b/packages/host/tests/acceptance/code-submode/create-file-test.gts index 31b459c072..18cfcba3dd 100644 --- a/packages/host/tests/acceptance/code-submode/create-file-test.gts +++ b/packages/host/tests/acceptance/code-submode/create-file-test.gts @@ -1,12 +1,26 @@ -import { click, fillIn, waitFor, waitUntil } from '@ember/test-helpers'; +import { + click, + fillIn, + settled, + waitFor, + waitUntil, +} from '@ember/test-helpers'; import { getService } from '@universal-ember/test-support'; import QUnit, { module, test } from 'qunit'; -import { baseRealm, rri, baseRRI, Deferred } from '@cardstack/runtime-common'; +import { + baseRealm, + rri, + baseRRI, + Deferred, + SupportedMimeType, +} from '@cardstack/runtime-common'; import type FileUploadService from '@cardstack/host/services/file-upload'; +import type { RealmEventContent } from 'https://cardstack.com/base/matrix-event'; + import { percySnapshot, setupLocalIndexing, @@ -1545,4 +1559,241 @@ export class TestCard extends Animal { }); }, ); + + // When the AI assistant (or any external writer) creates a new .gts and + // then updates the code-submode codePath to the just-written URL, the + // host's FileResource (packages/host/app/resources/file.ts) can lose the + // race against the realm's index pipeline. The first authedFetch returns + // 404 and `read` transitions into `state: 'not-found'`. The realm later + // broadcasts `index/incremental` for the new URL, and the FileResource + // must react to that event and recover — otherwise the URL bar stays + // stuck on "This resource does not exist" until the user re-navigates. + // + // This test simulates the external write by navigating to a non-existent + // URL, confirming the URL bar shows the not-found error, then performing + // the write via the realm directly (mirroring what the realm-server does + // when a card+source PUT lands). After the realm broadcasts the matching + // `index/incremental` event, the URL bar must recover. + module('when an external write creates a new file', function (hooks) { + hooks.beforeEach(function () { + setRealmPermissions({ + [baseRealm.url]: ['read'], + [testRealmURL]: ['read', 'write'], + }); + }); + + test('code submode recovers when a newly-created file arrives via a realm index/incremental event', async function (assert) { + let newFilePath = 'ai-created-card.gts'; + let newFileUrl = `${testRealmURL}${newFilePath}`; + let newFileSource = ` + import { CardDef } from 'https://cardstack.com/base/card-api'; + export default class AiCreatedCard extends CardDef { + static displayName = 'Ai Created Card'; + } + `; + + // Simulate the AI assistant updating the codePath to a file that does + // not yet exist in the realm. The host has not seen this URL before, + // so FileResource.read will hit 404. + await visitOperatorMode(newFileUrl); + + await waitFor('[data-test-card-url-bar-error]'); + assert + .dom('[data-test-card-url-bar-error]') + .containsText( + 'This resource does not exist', + 'URL bar surfaces the not-found error on initial 404', + ); + + // The realm broadcasts the incremental invalidation event over matrix + // once indexing of the newly-written file completes. Subscribe so we + // can await its arrival deterministically before asserting recovery. + let incrementalEvent = new Deferred(); + let unsubscribe = getService('message-service').subscribe( + testRealmURL, + (ev: RealmEventContent) => { + if ( + ev.eventName === 'index' && + ev.indexType === 'incremental' && + Array.isArray(ev.invalidations) && + (ev.invalidations as string[]).includes(newFileUrl) + ) { + unsubscribe(); + incrementalEvent.fulfill(); + } + }, + ); + + // Mirror WriteTextFileCommand exactly. `cardService.saveSource` with + // saveType 'create-file' POSTs the new source to the realm and tags + // the request with `X-Boxel-Client-Request-Id: create-file:`, + // which the realm echoes back in the `index/incremental` event. + // This shape — saveType 'create-file' and that clientRequestId + // prefix — is what the AI assistant produces and what the + // invalidation handler must treat as reload-worthy even though the + // id is in `cardService.clientRequestIds`. + let cardService = getService('card-service'); + await cardService.saveSource( + new URL(newFileUrl), + newFileSource, + 'create-file', + ); + await incrementalEvent.promise; + await settled(); + await waitFor('[data-test-code-mode][data-test-save-idle]'); + + assert + .dom('[data-test-card-url-bar-error]') + .doesNotExist( + 'URL bar error clears after the realm broadcasts the index/incremental event for the new file', + ); + assert + .dom('[data-test-card-url-bar-input]') + .hasValue( + newFileUrl, + 'code submode stays on the new file URL after recovery', + ); + assert.ok( + getMonacoContent().includes('AiCreatedCard'), + 'monaco loads the recovered file body, not a stale buffer', + ); + }); + + // When `updateCodePath` runs `FileResource.modify` for a URL whose + // file is about to be created, the modify-driven read can still be + // in flight when the realm broadcasts the matching `index/incremental` + // event. The handler then calls `read.perform({force: true})`, which + // restartableTask treats as cancel-and-restart. The cancelled task's + // awaited fetch eventually resolves, raising TaskCancelation at the + // `await`. If the catch block treated cancellation as a real fetch + // failure, it would call `updateState({ state: 'not-found' })` + // AFTER the restart already landed `state: 'ready'`, leaving the + // URL bar permanently stuck on the not-found error. + // + // This test gates the two fetches independently so the cancelled + // (older) read can be released AFTER the fresh read has already set + // state to ready — pinning the ordering that requires the + // `didCancel` guard in `file.ts` to survive. + test('cancelled read does not overwrite a fresh ready state with not-found', async function (assert) { + let newFilePath = 'cancellation-race-card.gts'; + let newFileUrl = `${testRealmURL}${newFilePath}`; + let newFileSource = ` + import { CardDef } from 'https://cardstack.com/base/card-api'; + export default class CancellationRaceCard extends CardDef { + static displayName = 'Cancellation Race Card'; + } + `; + + // Land on an EXISTING file first so FileResource is in state + // 'ready' with a realm subscription already established. This + // matches Buck's flow (he was viewing a prior file when he + // clicked New Card Definition) and is required for the race — + // the bug only fires when modify is called on an already-subscribed + // FileResource and the indexing event lands during the read. + await visitOperatorMode(`${testRealmURL}index.json`); + await waitFor('[data-test-code-mode][data-test-save-idle]'); + + // Per-call gating: each request for newFileUrl awaits its own + // Deferred so the test can release the cancelled read AFTER the + // fresh read has already updated state to 'ready'. A single + // shared gate would force both reads to resolve in mount order, + // which doesn't reproduce the race. + let pendingReads: Deferred[] = []; + let network = getService('network'); + let readGate = async (request: Request) => { + if ( + request.method === 'GET' && + request.url === newFileUrl && + request.headers.get('Accept') === SupportedMimeType.CardSource + ) { + let gate = new Deferred(); + pendingReads.push(gate); + await gate.promise; + } + return null; + }; + network.virtualNetwork.mount(readGate, { prepend: true }); + + try { + // Write the file BEFORE navigating so the realm broadcasts the + // incremental event during the modify-driven read. The handler + // matches the in-flight URL and triggers read.perform({force:true}), + // which cancels the in-flight task and starts a fresh one. + let incrementalEvent = new Deferred(); + let unsubscribe = getService('message-service').subscribe( + testRealmURL, + (ev: RealmEventContent) => { + if ( + ev.eventName === 'index' && + ev.indexType === 'incremental' && + Array.isArray(ev.invalidations) && + (ev.invalidations as string[]).includes(newFileUrl) + ) { + unsubscribe(); + incrementalEvent.fulfill(); + } + }, + ); + + let cardService = getService('card-service'); + let opState = getService('operator-mode-state-service'); + + // Fire updateCodePath without awaiting so the test can observe + // the modify-driven read sitting at the gate. Awaiting would + // block until the read resolves, defeating the point of the + // gate. + let codePathChange = opState.updateCodePath(new URL(newFileUrl)); + + // Wait until the modify-driven read (read#1) is parked behind + // the gate before triggering the write. Without this barrier + // the saveSource POST and its incremental event could land + // before modify fires — the handler would then run with the + // PRIOR `_url` and drop the event, never reaching the + // restart-driven cancellation that this test exists to pin. + await waitUntil(() => pendingReads.length >= 1); + + await cardService.saveSource( + new URL(newFileUrl), + newFileSource, + 'create-file', + ); + await incrementalEvent.promise; + + // Wait until the restart spawned by the incremental event has + // also parked at the gate; only with both reads in flight can + // we choose the release order that reproduces the race. + await waitUntil(() => pendingReads.length >= 2); + + // Release the FRESH read first. Its 200 response lands and + // sets state to 'ready'. Then release the ORIGINAL (now + // cancelled) read so its TaskCancelation throws at its `await` + // — without the didCancel guard, the catch block overwrites + // the ready state with 'not-found'. + pendingReads[1].fulfill(); + await settled(); + pendingReads[0].fulfill(); + await codePathChange; + await settled(); + await waitFor('[data-test-code-mode][data-test-save-idle]'); + + assert + .dom('[data-test-card-url-bar-error]') + .doesNotExist( + 'cancelled read must not overwrite the fresh ready state with not-found', + ); + assert + .dom('[data-test-card-url-bar-input]') + .hasValue( + newFileUrl, + 'code submode stays on the new file URL after the cancellation race', + ); + assert.ok( + getMonacoContent().includes('CancellationRaceCard'), + 'monaco shows the new file body once the fresh read wins', + ); + } finally { + network.virtualNetwork.unmount(readGate); + } + }); + }); });