diff --git a/mobile/src/mobile-web-shell/generation-store-host-removal-race.test.ts b/mobile/src/mobile-web-shell/generation-store-host-removal-race.test.ts index bf27caef164..87fe1644906 100644 --- a/mobile/src/mobile-web-shell/generation-store-host-removal-race.test.ts +++ b/mobile/src/mobile-web-shell/generation-store-host-removal-race.test.ts @@ -103,7 +103,7 @@ async function seedHosts(store: GenerationStore): Promise { } function indexedHostKeys(fileSystem: FakeGenerationFileSystem): string[] { - return Object.keys(JSON.parse(fileSystem.text(INDEX) ?? '{}')).sort() + return [...JSON.parse(fileSystem.text(INDEX) ?? '[]')].sort() } type Sides = { session: () => GenerationStore; removeHost: (hostId: string) => Promise } diff --git a/mobile/src/mobile-web-shell/generation-store.test.ts b/mobile/src/mobile-web-shell/generation-store.test.ts index 03ed6876a54..e7caf90315d 100644 --- a/mobile/src/mobile-web-shell/generation-store.test.ts +++ b/mobile/src/mobile-web-shell/generation-store.test.ts @@ -72,10 +72,14 @@ async function activate( await store.commitGeneration(await store.stageGeneration(hostKey, result)) } +function hostKeys(count: number): string[] { + return Array.from({ length: count }, (_, index) => deriveHostCacheKey(`host-${index}`)) +} + describe('generation store', () => { it('stages and commits exactly the manifest, with the manifest written last', async () => { const fs = createFakeFileSystem() - const store = createGenerationStore({ fileSystem: fs, now: () => 10 }) + const store = createGenerationStore({ fileSystem: fs }) await activate(store, HOST) @@ -94,7 +98,7 @@ describe('generation store', () => { const staged = fs.writes.filter((path) => path.includes('/tmp/')) expect(staged.at(-1)).toBe(`${HOST}/tmp/${build}/manifest.json`) expect(staged).toHaveLength(3) - expect(fs.text('hosts.json')).toBe(JSON.stringify({ [HOST]: 10 })) + expect(fs.text('hosts.json')).toBe(JSON.stringify([HOST])) }) it('reads back the activation it committed', async () => { @@ -154,7 +158,7 @@ describe('generation store', () => { it('treats a second commit of the same build as a no-op', async () => { const fs = createFakeFileSystem() - const store = createGenerationStore({ fileSystem: fs, now: () => 10 }) + const store = createGenerationStore({ fileSystem: fs }) await activate(store, HOST) const before = fs.paths() @@ -209,9 +213,8 @@ describe('generation store', () => { it('evicts the least recently activated host past the ceiling', async () => { const fs = createFakeFileSystem() - let clock = 0 - const store = createGenerationStore({ fileSystem: fs, now: () => (clock += 1) }) - const hosts = ['a', 'b', 'c', 'd', 'e'].map((name) => deriveHostCacheKey(name)) + const store = createGenerationStore({ fileSystem: fs }) + const hosts = hostKeys(MAX_CACHED_HOSTS + 1) for (const host of hosts) { await activate(store, host) @@ -222,25 +225,23 @@ describe('generation store', () => { for (const host of hosts.slice(1)) { expect((await store.readActiveGeneration(host))?.buildId).toBe(BUILD) } - expect(Object.keys(JSON.parse(fs.text('hosts.json') ?? '{}'))).toHaveLength(MAX_CACHED_HOSTS) + expect(fs.text('hosts.json')).toBe(JSON.stringify(hosts.slice(1))) }) it('evicts a host with no index entry before the least recently activated one', async () => { const fs = createFakeFileSystem() - let clock = 0 - const store = createGenerationStore({ fileSystem: fs, now: () => (clock += 1) }) - const oldest = deriveHostCacheKey('a') + const store = createGenerationStore({ fileSystem: fs }) + const [oldest, ...rest] = hostKeys(MAX_CACHED_HOSTS - 1) const orphan = deriveHostCacheKey('orphan') - for (const name of ['a', 'b', 'c']) { - await activate(store, deriveHostCacheKey(name)) + for (const host of [oldest, ...rest]) { + await activate(store, host) } // Activated last, so recency alone would keep it; its index entry is what goes missing. await activate(store, orphan) - const index: Record = JSON.parse(fs.text('hosts.json') ?? '{}') - delete index[orphan] - fs.seed('hosts.json', { kind: 'file', bytes: new TextEncoder().encode(JSON.stringify(index)) }) + const order = [oldest, ...rest] + fs.seed('hosts.json', { kind: 'file', bytes: new TextEncoder().encode(JSON.stringify(order)) }) - await activate(store, deriveHostCacheKey('d')) + await activate(store, deriveHostCacheKey('one-too-many')) expect(fs.paths().some((path) => path.startsWith(orphan))).toBe(false) expect((await store.readActiveGeneration(oldest))?.buildId).toBe(BUILD) @@ -248,36 +249,102 @@ describe('generation store', () => { it('counts a recommit of the build a host already has as use of that host', async () => { const fs = createFakeFileSystem() - let clock = 0 - const store = createGenerationStore({ fileSystem: fs, now: () => (clock += 1) }) - const kept = deriveHostCacheKey('a') - const evicted = deriveHostCacheKey('b') - for (const name of ['a', 'b', 'c', 'd']) { - await activate(store, deriveHostCacheKey(name)) + const store = createGenerationStore({ fileSystem: fs }) + const hosts = hostKeys(MAX_CACHED_HOSTS) + const [kept, evicted] = hosts + for (const host of hosts) { + await activate(store, host) } - // A redownload of the bundle host A already has, which takes the same-build commit path. + // A redownload of the bundle the oldest host already has, which takes the same-build path. await activate(store, kept) - await activate(store, deriveHostCacheKey('e')) + await activate(store, deriveHostCacheKey('one-too-many')) expect(fs.paths().some((path) => path.startsWith(evicted))).toBe(false) expect((await store.readActiveGeneration(kept))?.buildId).toBe(BUILD) }) + it('counts opening a cached generation as use of that host', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + const hosts = hostKeys(MAX_CACHED_HOSTS) + for (const host of hosts) { + await activate(store, host) + } + expect((await store.readActiveGeneration(hosts[0]))?.buildId).toBe(BUILD) + + await activate(store, deriveHostCacheKey('one-too-many')) + + expect((await store.readActiveGeneration(hosts[0]))?.buildId).toBe(BUILD) + expect(fs.paths().some((path) => path.startsWith(hosts[1]))).toBe(false) + }) + + it('writes nothing when a read finds no active generation', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + await activate(store, HOST) + const writesBefore = fs.writes.length + + expect(await store.readActiveGeneration(deriveHostCacheKey('never-opened'))).toBeNull() + + expect(fs.writes).toHaveLength(writesBefore) + expect(fs.text('hosts.json')).toBe(JSON.stringify([HOST])) + }) + + it('returns the generation when an open cannot write the index', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + const other = deriveHostCacheKey('other') + await activate(store, HOST) + await activate(store, other) + fs.failWritesAt('hosts.json') + + expect((await store.readActiveGeneration(HOST))?.buildId).toBe(BUILD) + expect(fs.text('hosts.json')).toBe(JSON.stringify([HOST, other])) + }) + + it('rewrites the index on an open only when the host is not already the newest', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + const [older, newest] = hostKeys(2) + await activate(store, older) + await activate(store, newest) + const before = { text: fs.text('hosts.json'), writes: fs.writes.length } + + expect((await store.readActiveGeneration(newest))?.buildId).toBe(BUILD) + expect({ text: fs.text('hosts.json'), writes: fs.writes.length }).toEqual(before) + + expect((await store.readActiveGeneration(older))?.buildId).toBe(BUILD) + expect(fs.text('hosts.json')).toBe(JSON.stringify([newest, older])) + }) + + it('reads the timestamp record earlier builds wrote as an empty order', async () => { + const fs = createFakeFileSystem() + const store = createGenerationStore({ fileSystem: fs }) + const other = deriveHostCacheKey('other') + fs.seed('hosts.json', { + kind: 'file', + bytes: new TextEncoder().encode(JSON.stringify({ [HOST]: 1, [other]: 2 })) + }) + await activate(store, other) + + expect(fs.text('hosts.json')).toBe(JSON.stringify([other])) + }) + it('never counts or evicts a host that is only mid-download', async () => { const fs = createFakeFileSystem() - let clock = 0 - const store = createGenerationStore({ fileSystem: fs, now: () => (clock += 1) }) - const oldest = deriveHostCacheKey('a') + const store = createGenerationStore({ fileSystem: fs }) + const hosts = hostKeys(MAX_CACHED_HOSTS) + const oldest = hosts[0] const downloading = deriveHostCacheKey('downloading') - for (const name of ['a', 'b', 'c', 'd']) { - await activate(store, deriveHostCacheKey(name)) + for (const host of hosts) { + await activate(store, host) } const staged = await store.stageGeneration(downloading, buildResult({})) - await activate(store, deriveHostCacheKey('e')) + await activate(store, deriveHostCacheKey('one-too-many')) - // The ceiling is four cached generations, so the fifth activation evicts the least recently + // The ceiling counts cached generations, so the activation past it evicts the least recently // activated host and leaves the download alone. expect(fs.paths().some((path) => path.startsWith(oldest))).toBe(false) expect(fs.text(`${staged.directory.slice(ROOT.length + 1)}/manifest.json`)).not.toBeNull() @@ -366,7 +433,7 @@ describe('generation store', () => { expect(await store.readActiveGeneration(HOST)).toBeNull() expect((await store.readActiveGeneration(other))?.buildId).toBe(BUILD) - expect(Object.keys(JSON.parse(fs.text('hosts.json') ?? '{}'))).toEqual([other]) + expect(fs.text('hosts.json')).toBe(JSON.stringify([other])) }) it('refuses a rename that did not carry the tree, as Android below API 26 can', async () => { @@ -400,8 +467,8 @@ describe('generation store', () => { it('activates normally when the recency index cannot be read', async () => { const fs = createFakeFileSystem() - const store = createGenerationStore({ fileSystem: fs, now: () => 10 }) - fs.seed('hosts.json', { kind: 'file', bytes: new TextEncoder().encode('{}') }) + const store = createGenerationStore({ fileSystem: fs }) + fs.seed('hosts.json', { kind: 'file', bytes: new TextEncoder().encode('[]') }) fs.failReadsAt('hosts.json') await activate(store, HOST) @@ -439,26 +506,6 @@ describe('generation store', () => { expect((await store.readActiveGeneration(HOST))?.buildId).toBe(BUILD) }) - 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/${BUILD}` - ) - expect(await store.readActiveGeneration(hosts[0])).toBeNull() - for (const host of hosts.slice(1)) { - expect((await store.readActiveGeneration(host))?.buildId).toBe(BUILD) - } - }) - it('returns the activation even when the recency index cannot be written', async () => { const fs = createFakeFileSystem() const store = createGenerationStore({ fileSystem: fs }) @@ -486,16 +533,16 @@ describe('generation store', () => { it('prunes an index entry whose host tree is gone', async () => { const fs = createFakeFileSystem() - const store = createGenerationStore({ fileSystem: fs, now: () => 10 }) + const store = createGenerationStore({ fileSystem: fs }) const stale = deriveHostCacheKey('uninstalled') fs.seed('hosts.json', { kind: 'file', - bytes: new TextEncoder().encode(JSON.stringify({ [stale]: 5 })) + bytes: new TextEncoder().encode(JSON.stringify([stale])) }) await activate(store, HOST) - expect(fs.text('hosts.json')).toBe(JSON.stringify({ [HOST]: 10 })) + expect(fs.text('hosts.json')).toBe(JSON.stringify([HOST])) }) it('refuses a staged handle it did not issue', async () => { @@ -519,8 +566,7 @@ describe('generation store', () => { it('ignores a directory under the cache root that is not a host key', async () => { const fs = createFakeFileSystem() - let clock = 0 - const store = createGenerationStore({ fileSystem: fs, now: () => (clock += 1) }) + const store = createGenerationStore({ fileSystem: fs }) // Whatever else lives under the OS cache directory is not this store's to count or delete. fs.seed('not-a-host-key/stray.txt', { kind: 'file', bytes: new Uint8Array(1) }) const hosts = ['a', 'b', 'c', 'd'].map((name) => deriveHostCacheKey(name)) diff --git a/mobile/src/mobile-web-shell/generation-store.ts b/mobile/src/mobile-web-shell/generation-store.ts index 75a315ca332..53281212294 100644 --- a/mobile/src/mobile-web-shell/generation-store.ts +++ b/mobile/src/mobile-web-shell/generation-store.ts @@ -23,8 +23,8 @@ const GENERATIONS_DIRECTORY_NAME = 'generations' const STAGING_DIRECTORY_NAME = 'tmp' const HOST_INDEX_FILE_NAME = 'hosts.json' -/** The architecture reference's cache ceiling: four hosts, least recently activated evicted. */ -export const MAX_CACHED_HOSTS = 4 +/** Least recently used host evicted past this. */ +export const MAX_CACHED_HOSTS = 6 export type ActiveGeneration = { readonly buildId: string @@ -67,15 +67,14 @@ export type GenerationStore = { forgetHostUpdateFailures(hostId: string): Promise } -/** Recency only, so anything unreadable degrades to "evict this host first". */ -const HostIndexSchema = z.record(z.string(), z.number().int().nonnegative()) +/** Host cache keys, least recently used first. Recency only, so anything unreadable degrades to + * "evict this host first"; the timestamp record earlier builds wrote reads as empty once. */ +const HostOrderSchema = z.array(z.string()) export function createGenerationStore(options: { fileSystem: GenerationFileSystem - now?: () => number }): GenerationStore { const fs = options.fileSystem - const now = options.now ?? Date.now // `StagedGeneration` is structurally typed, so any object of that shape would otherwise let // `commitGeneration` rename over, and `abortStagedGeneration` delete, a directory of the caller's // choosing. Only handles this store minted are honoured. @@ -87,25 +86,30 @@ export function createGenerationStore(options: { const stagingRoot = (hostKey: string): string => joinUri(hostRoot(hostKey), STAGING_DIRECTORY_NAME) - async function readHostIndex(): Promise> { + async function readHostOrder(): Promise { // Unreadable is treated as absent here, unlike a manifest: an index nobody can read costs // eviction order, and the next activation rewrites it whole. const text = await fs.readText(joinUri(fs.rootUri, HOST_INDEX_FILE_NAME)).catch(() => null) - const parsed = text === null ? null : HostIndexSchema.safeParse(parseJson(text)) - return new Map(Object.entries(parsed?.success === true ? parsed.data : {})) + const parsed = text === null ? null : HostOrderSchema.safeParse(parseJson(text)) + return parsed?.success === true ? parsed.data : [] } - async function writeHostIndex(index: ReadonlyMap): Promise { + async function writeHostOrder(order: readonly string[]): Promise { // 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)) - ) + .writeText(joinUri(fs.rootUri, HOST_INDEX_FILE_NAME), JSON.stringify(order)) .catch(() => undefined) } + /** Use without an eviction pass, because the host count did not change. */ + async function touchHost(hostKey: string): Promise { + const order = await readHostOrder() + if (order.at(-1) !== hostKey) { + await writeHostOrder(withHostLast(order, hostKey)) + } + } + async function listHostDirectories(): Promise { const entries = await fs.list(fs.rootUri) return entries.filter((entry) => entry.isDirectory && isHostCacheKey(entry.name)) @@ -129,26 +133,20 @@ export function createGenerationStore(options: { await fs.delete(hostRoot(hostKey)) } - async function enforceHostLimit(index: Map, activated: string): Promise { + async function enforceHostLimit(order: readonly string[]): Promise { const hosts = await listActivatedHosts() - const present = new Set(hosts) - for (const key of Array.from(index.keys())) { - if (!present.has(key)) { - index.delete(key) - } - } - // A host with no index entry sorts first: the index is recency, not truth, so a lost or + // A host missing from the order sorts first: the index is recency, not truth, so a lost or // 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 !== activated) - .sort((left, right) => (index.get(left) ?? 0) - (index.get(right) ?? 0)) - for (const host of candidates.slice(0, Math.max(0, hosts.length - MAX_CACHED_HOSTS))) { + // last, so it is never among the evicted. + const evicted = new Set( + [...hosts] + .sort((left, right) => order.indexOf(left) - order.indexOf(right)) + .slice(0, Math.max(0, hosts.length - MAX_CACHED_HOSTS)) + ) + for (const host of evicted) { await dropHostTree(host) - index.delete(host) } - await writeHostIndex(index) + await writeHostOrder(order.filter((key) => hosts.includes(key) && !evicted.has(key))) } async function readActive(hostKey: string): Promise { @@ -242,12 +240,9 @@ export function createGenerationStore(options: { existing?.isDirectory === true && (await fs.fileExists(joinUri(target, MANIFEST_FILE_NAME))) ) { - // Still an activation, so it still counts as use: without this a host that redownloads the - // bundle it already has stays the least recently activated and is evicted first. No eviction - // pass, because the host count did not change. - const index = await readHostIndex() - index.set(staged.hostKey, now()) - await writeHostIndex(index) + // Still an activation: without this a host that redownloads the bundle it already has stays + // the least recently used and is evicted first. + await touchHost(staged.hostKey) await fs.delete(staged.directory) return active } @@ -271,10 +266,17 @@ export function createGenerationStore(options: { 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 a caller: the four-host ceiling is this module's invariant. - await enforceHostLimit(index, staged.hostKey) + // Enforced here rather than left to a caller: the host ceiling is this module's invariant. + await enforceHostLimit(withHostLast(await readHostOrder(), staged.hostKey)) + return active + } + + async function openActive(hostKey: string): Promise { + const active = await readActive(hostKey) + // An open is use, so a daily host downloaded long ago is not evicted first. + if (active !== null) { + await touchHost(hostKey) + } return active } @@ -304,9 +306,9 @@ export function createGenerationStore(options: { async function deleteHost(hostKey: string): Promise { await dropHostTree(hostKey) - const index = await readHostIndex() - if (index.delete(hostKey)) { - await writeHostIndex(index) + const order = await readHostOrder() + if (order.includes(hostKey)) { + await writeHostOrder(order.filter((key) => key !== hostKey)) } } @@ -322,7 +324,7 @@ export function createGenerationStore(options: { } return { - readActiveGeneration: (hostKey) => serialize(() => readActive(hostKey)), + readActiveGeneration: (hostKey) => serialize(() => openActive(hostKey)), stageGeneration: (hostKey, result) => serialize(() => stage(hostKey, result)), commitGeneration: (staged) => serialize(() => commit(staged)), abortStagedGeneration: (staged) => @@ -339,6 +341,10 @@ export function createGenerationStore(options: { } } +function withHostLast(order: readonly string[], hostKey: string): string[] { + return [...order.filter((key) => key !== hostKey), hostKey] +} + function parseJson(text: string): unknown { try { return JSON.parse(text) diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.test.ts b/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.test.ts index 832fee81e50..abd4c300bf8 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.test.ts +++ b/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import { createFakeGenerationFileSystem } from './generation-file-system-fake' -import { createGenerationStore } from './generation-store' +import { createGenerationStore, MAX_CACHED_HOSTS } from './generation-store' import { deriveHostCacheKey } from './host-cache-key' import type { MobileWebShellUpdateFailure } from './mobile-web-shell-update-failure' import { @@ -45,11 +45,18 @@ describe('appendUpdateFailure', () => { }) it('holds a ceiling across hosts, evicting the oldest of all', () => { - const kept = appendAll(Array.from({ length: 30 }, (_, at) => failure(`host-${at}`, at))) + const kept = appendAll( + Array.from({ length: MAX_UPDATE_FAILURES + 10 }, (_, at) => failure(`host-${at}`, at)) + ) expect(kept).toHaveLength(MAX_UPDATE_FAILURES) expect(kept[0]?.at).toBe(10) }) + // Pinned here because deriving it would make the failure log import the store that imports it. + it('keeps a full per-host history for every cached host', () => { + expect(MAX_UPDATE_FAILURES).toBe(MAX_UPDATE_FAILURES_PER_HOST * MAX_CACHED_HOSTS) + }) + it('drops a build id that is not a digest rather than keep host text', () => { const [entry] = appendUpdateFailure([], { ...failure('host-1', 0), diff --git a/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.ts b/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.ts index 357ddbe831f..dbca08f5f10 100644 --- a/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.ts +++ b/mobile/src/mobile-web-shell/mobile-web-shell-update-failure-log.ts @@ -16,8 +16,8 @@ import { * download from a host that fails every launch, while the file stays a few kilobytes. */ export const MAX_UPDATE_FAILURES_PER_HOST = 5 -/** A ceiling across hosts too, because hosts are not otherwise bounded: four hosts' worth. */ -export const MAX_UPDATE_FAILURES = 20 +/** A ceiling across hosts too, because hosts are not otherwise bounded: six cached hosts' worth. */ +export const MAX_UPDATE_FAILURES = 30 const UPDATE_FAILURE_LOG_FILE_NAME = 'update-failures.json'