diff --git a/mobile/src/mobile-web-shell/generation-store.test.ts b/mobile/src/mobile-web-shell/generation-store.test.ts index 93b713bb71a..c7548e8c12f 100644 --- a/mobile/src/mobile-web-shell/generation-store.test.ts +++ b/mobile/src/mobile-web-shell/generation-store.test.ts @@ -229,15 +229,18 @@ describe('generation store', () => { expect(await store.readActiveGeneration(HOST)).toBeNull() }) - it('leaves no generation and no tmp when a download is interrupted before commit', async () => { + it('leaves no generation and no tmp for any host when a download is interrupted', async () => { const fs = createFakeFileSystem() const store = createGenerationStore({ fileSystem: fs }) + const other = deriveHostCacheKey('host-b') await store.stageGeneration(HOST, buildResult({})) + await store.stageGeneration(other, buildResult({})) await store.sweepStagedGenerations() expect(fs.paths().some((path) => path.includes('/tmp'))).toBe(false) expect(await store.readActiveGeneration(HOST)).toBeNull() + expect(await store.readActiveGeneration(other)).toBeNull() }) it('treats a second commit of the same build as a no-op', async () => { @@ -337,17 +340,39 @@ describe('generation store', () => { it('serializes two stage calls for one host and build', async () => { const fs = createFakeFileSystem() const store = createGenerationStore({ fileSystem: fs }) + // One build id cannot really carry two asset lists; differing ones are what make an interleaved + // pair visible, because unserialized both trees land in the one staged directory. + const staging = `${HOST}/tmp/${'a'.repeat(64)}` + const earlier = buildResult({ assets: [{ path: 'assets/earlier.js', byteLength: 2 }] }) + const later = buildResult({ assets: [{ path: 'assets/later.js', byteLength: 3 }] }) const [first, second] = await Promise.all([ - store.stageGeneration(HOST, buildResult({})), - store.stageGeneration(HOST, buildResult({})) + store.stageGeneration(HOST, earlier), + store.stageGeneration(HOST, later) ]) expect(first.directory).toBe(second.directory) - // Two interleaved stages would write one tree twice over; serialized, the second one starts by - // dropping the first's tree, so the write log is exactly two whole stagings. - expect(fs.writes).toHaveLength(6) - expect(fs.writes.at(-1)).toBe(`${HOST}/tmp/${'a'.repeat(64)}/manifest.json`) + // Each staging is a contiguous run ending in its manifest; interleaved they would alternate. + expect(fs.writes).toEqual([ + `${staging}/assets/earlier.js`, + `${staging}/manifest.json`, + `${staging}/assets/later.js`, + `${staging}/manifest.json` + ]) + expect(fs.paths().filter((path) => path.startsWith(`${staging}/assets/`))).toEqual([ + `${staging}/assets/later.js` + ]) + }) + + it('drops residue from an earlier attempt instead of staging over it', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + const staging = `${HOST}/tmp/${'a'.repeat(64)}` + fs.seed(`${staging}/assets/orphan.js`, { kind: 'file', bytes: new Uint8Array(1) }) + + await store.stageGeneration(HOST, buildResult({})) + + expect(fs.paths().some((path) => path.endsWith('orphan.js'))).toBe(false) }) it('refuses a path that escapes the staged tree, and a host key that is not one', async () => { @@ -358,7 +383,8 @@ describe('generation store', () => { 'assets/../../outside.js', '/etc/passwd', 'assets//app.js', - 'manifest.json' + 'manifest.json', + 'Manifest.JSON' ] for (const path of escapes) { @@ -409,6 +435,65 @@ describe('generation store', () => { expect((await store.readActiveGeneration(HOST))?.buildId).toBe('a'.repeat(64)) }) + it('keeps the host it just activated when the clock jumps backward', async () => { + const fs = createFakeFileSystem() + const times = [100, 200, 300, 400, 1] + let tick = 0 + const store = createGenerationStore({ fileSystem: fs, now: () => times[tick++] ?? 0 }) + const hosts = ['a', 'b', 'c', 'd', 'e'].map((name) => deriveHostCacheKey(name)) + + for (const host of hosts) { + await activate(store, host) + } + + expect((await store.readActiveGeneration(hosts[4]))?.directory).toBe( + `${ROOT}/${hosts[4]}/generations/${'a'.repeat(64)}` + ) + expect(await store.readActiveGeneration(hosts[0])).toBeNull() + for (const host of hosts.slice(1)) { + expect((await store.readActiveGeneration(host))?.buildId).toBe('a'.repeat(64)) + } + }) + + it('prunes an index entry whose host tree is gone', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs, now: () => 10 }) + const stale = deriveHostCacheKey('uninstalled') + fs.seed('hosts.json', { + kind: 'file', + bytes: new TextEncoder().encode(JSON.stringify({ [stale]: 5 })) + }) + + await activate(store, HOST) + + expect(fs.text('hosts.json')).toBe(JSON.stringify({ [HOST]: 10 })) + }) + + it('returns the activation even when the recency index cannot be written', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + fs.failWritesAt('hosts.json') + + const staged = await store.stageGeneration(HOST, buildResult({})) + const active = await store.commitGeneration(staged) + + expect(active.buildId).toBe('a'.repeat(64)) + expect((await store.readActiveGeneration(HOST))?.buildId).toBe('a'.repeat(64)) + expect(fs.text('hosts.json')).toBeNull() + }) + + it('refuses a handle whose staged tree is gone without touching the activation', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + await activate(store, HOST) + + const staged = await store.stageGeneration(HOST, buildResult({ buildId: 'b'.repeat(64) })) + await store.abortStagedGeneration(staged) + + await expect(store.commitGeneration(staged)).rejects.toThrow('no longer on disk') + expect((await store.readActiveGeneration(HOST))?.buildId).toBe('a'.repeat(64)) + }) + it('keeps the adapter aligned with the port', () => { expect(adapterSatisfiesPort).toBe(true) }) diff --git a/mobile/src/mobile-web-shell/generation-store.ts b/mobile/src/mobile-web-shell/generation-store.ts index 3ad980ea137..51aa2f4301e 100644 --- a/mobile/src/mobile-web-shell/generation-store.ts +++ b/mobile/src/mobile-web-shell/generation-store.ts @@ -38,7 +38,6 @@ export type GenerationStore = { abortStagedGeneration(staged: StagedGeneration): Promise sweepStagedGenerations(): Promise deleteHostCache(hostKey: string): Promise - evictHostsBeyond(limit?: number): Promise } /** Recency only, so anything unreadable degrades to "evict this host first". */ @@ -64,11 +63,14 @@ export function createGenerationStore(options: { } async function writeHostIndex(index: ReadonlyMap): Promise { - await fs.createDirectory(fs.rootUri) - await fs.writeText( - joinUri(fs.rootUri, HOST_INDEX_FILE_NAME), - JSON.stringify(Object.fromEntries(index)) - ) + // Recency, not truth: a full disk here must not turn an activation that is already on disk + // into a thrown commit, and the next activation rewrites the whole index anyway. + await fs + .writeText( + joinUri(fs.rootUri, HOST_INDEX_FILE_NAME), + JSON.stringify(Object.fromEntries(index)) + ) + .catch(() => undefined) } async function listHostDirectories(): Promise { @@ -80,7 +82,7 @@ export function createGenerationStore(options: { await fs.delete(hostRoot(hostKey)) } - async function enforceHostLimit(limit: number, index: Map): Promise { + async function enforceHostLimit(index: Map, activated: string): Promise { const hosts = await listHostDirectories() const present = new Set(hosts.map((host) => host.name)) for (const key of Array.from(index.keys())) { @@ -89,11 +91,13 @@ export function createGenerationStore(options: { } } // A host with no index entry sorts first: the index is recency, not truth, so a lost or - // truncated one costs eviction order rather than a generation. - const ordered = [...hosts].sort( - (left, right) => (index.get(left.name) ?? 0) - (index.get(right.name) ?? 0) - ) - for (const host of ordered.slice(0, Math.max(0, ordered.length - limit))) { + // truncated one costs eviction order rather than a generation. The host just activated is + // never a candidate, because `now()` is a wall clock: one backward jump would otherwise make + // the newest entry the oldest and evict the tree the caller is about to open. + const candidates = hosts + .filter((host) => host.name !== activated) + .sort((left, right) => (index.get(left.name) ?? 0) - (index.get(right.name) ?? 0)) + for (const host of candidates.slice(0, Math.max(0, hosts.length - MAX_CACHED_HOSTS))) { await dropHostTree(host.name) index.delete(host.name) } @@ -140,7 +144,6 @@ export function createGenerationStore(options: { // plus a fresh write is not a generation either side verified. await fs.delete(directory) try { - await fs.createDirectory(directory) for (const asset of assets) { await fs.writeBytes(asset.uri, asset.bytes) } @@ -169,6 +172,11 @@ export function createGenerationStore(options: { await fs.delete(staged.directory) return active } + // Before any delete: an aborted or swept handle must not cost the live generation, and a tree + // that is no longer on disk cannot be renamed into one either. + if (!(await fs.fileExists(joinUri(staged.directory, MANIFEST_FILE_NAME)))) { + throw new Error(`staged generation ${staged.buildId} is no longer on disk`) + } // Every other generation goes before the rename, never after. A crash between the two leaves // zero generations, which the runbook's redownload rule already covers; the other order can // leave two directories under `generations/` with nothing to say which one is the activation. @@ -178,16 +186,16 @@ export function createGenerationStore(options: { await fs.createDirectory(generations) await fs.moveDirectory(staged.directory, target) // Android below API 26 implements a directory move as a non-recursive copy plus a delete - // (expo-file-system android FileSystemPath.kt:158-173), which can land an empty directory. + // (expo-file-system android FileSystemPath.kt:158-173), which can land an empty directory. Its + // `delete()` then fails on the non-empty source, so the tmp tree survives for the next sweep. if (!(await fs.fileExists(joinUri(target, MANIFEST_FILE_NAME)))) { await fs.delete(target) throw new Error(`generation ${staged.buildId} did not carry its manifest through the rename`) } const index = await readHostIndex() index.set(staged.hostKey, now()) - // Enforced here rather than left to the caller: the four-host ceiling is this module's - // invariant, and `evictHostsBeyond` exists for the launch path, not as its only enforcement. - await enforceHostLimit(MAX_CACHED_HOSTS, index) + // Enforced here rather than left to a caller: the four-host ceiling is this module's invariant. + await enforceHostLimit(index, staged.hostKey) return active } @@ -208,7 +216,7 @@ export function createGenerationStore(options: { } // One queue for the whole store rather than one per host: every operation is a short burst of - // cache I/O, and a single order answers the stage/commit/sweep/evict interleavings at once. A + // cache I/O, and a single order answers the stage/commit/sweep/delete interleavings at once. A // second `stageGeneration` for the same host and build waits for the first rather than writing // into the tree it is still filling. let tail: Promise = Promise.resolve() @@ -224,11 +232,7 @@ export function createGenerationStore(options: { commitGeneration: (staged) => serialize(() => commit(staged)), abortStagedGeneration: (staged) => serialize(() => fs.delete(staged.directory)), sweepStagedGenerations: () => serialize(sweep), - deleteHostCache: (hostKey) => serialize(() => deleteHost(hostKey)), - evictHostsBeyond: (limit = MAX_CACHED_HOSTS) => - serialize(async () => { - await enforceHostLimit(limit, await readHostIndex()) - }) + deleteHostCache: (hostKey) => serialize(() => deleteHost(hostKey)) } } @@ -267,12 +271,13 @@ function requireBuildId(buildId: string): string { } /** The manifest schema bans traversal already, but this is the last code between a manifest and a - * write, and `manifest.json` is the store's own name rather than an asset's to take. */ + * write, and `manifest.json` is the store's own name rather than an asset's to take — folded, + * because APFS and NTFS are case-insensitive and `Manifest.JSON` would land on the same file. */ function requireStorablePath(path: string): string { const segments = path.split('/') const storable = path.length > 0 && - path !== MANIFEST_FILE_NAME && + path.toLowerCase() !== MANIFEST_FILE_NAME && !path.includes('\\') && segments.every((segment) => segment !== '' && segment !== '.' && segment !== '..') if (!storable) {