diff --git a/docs/ops/artifact-delivery-rollout.md b/docs/ops/artifact-delivery-rollout.md index 1cb2930b..a562b2d3 100644 --- a/docs/ops/artifact-delivery-rollout.md +++ b/docs/ops/artifact-delivery-rollout.md @@ -126,6 +126,9 @@ If GitHub answers 401 to the cached installation token, the read mints one new t If the App secrets are set but unusable, or a mint fails, reads use the fallback. The sync then paces itself on the fallback's quota. A failure stands for about 60 seconds in each isolate. In that time, reads make no mint, and the isolate emits one event with the reason. After a 401, the new mint runs even while a failure stands. A mint ends at the deadline of the read that waits for it. A `/gh` page read waits at most its 4 second limit. Builds and the sync wait up to the mint's own 15 second limit. A read deadline that ends a mint does not count as a failure. + +Requests in one isolate share a minted token and a failure window, never a mint in flight. workerd ties a fetch to the request that made it, so a request that waited for another request's mint could fail or hang. If several requests find no token at the same time, each mints its own. + Keep the read App apart from the `GITHUB_APP_*` secrets below. Those belong to private delivery. Changing `NUXT_TOKEN_KEY` breaks stored GitHub credentials. diff --git a/layers/registry/server/utils/github-client.ts b/layers/registry/server/utils/github-client.ts index 593fcb1d..f62ad779 100644 --- a/layers/registry/server/utils/github-client.ts +++ b/layers/registry/server/utils/github-client.ts @@ -84,6 +84,9 @@ export interface GithubBindingRuntime { * Pass the generated Cloudflare environment from the request platform or a * Nitro task. Falls back to process.env in local dev, * since `.env` populates process.env but not the Worker env binding. + * + * Resolve them inside each request, task or queue batch. The credential holds + * that request's mint in flight, which another request must never await. */ export function resolveGithubBindings( cloudflareEnv?: GithubBindingSource, diff --git a/shared/server/github-app-credential.ts b/shared/server/github-app-credential.ts index cf9cc46a..b8e690e4 100644 --- a/shared/server/github-app-credential.ts +++ b/shared/server/github-app-credential.ts @@ -81,14 +81,19 @@ interface CachedInstallationToken { } /** - * Installation tokens kept between reads in one isolate. A token lives an + * Installation tokens kept between requests in one isolate. A token lives an * hour; minting one per read would add a request and a signature to each. + * + * It holds finished values only. workerd ties a promise, a `Response` or a + * stream to the request that made it. A request that awaited another + * request's mint could throw "Cannot perform I/O on behalf of a different + * request", or wait forever once that request ended. A mint in flight stays + * with the {@link GithubCredential} of the request that started it. */ export interface InstallationTokenCache { tokens: Map /** Unix seconds until which a failure stands, by key. */ failedUntil: Map - pending: Map> } /** How one mint ended. */ @@ -100,7 +105,7 @@ type MintOutcome | { _tag: 'cut-off' } export function createInstallationTokenCache(): InstallationTokenCache { - return { tokens: new Map(), failedUntil: new Map(), pending: new Map() } + return { tokens: new Map(), failedUntil: new Map() } } let isolateTokenCache: InstallationTokenCache | null = null @@ -141,7 +146,7 @@ export interface GithubCredential { /** * The credential after GitHub answered 401 to `rejected`. The cached * installation token is dropped and a new one minted, once for every read - * that held it, even while a failed mint stands. + * through this credential that held it, even while a failed mint stands. */ renew: (rejected: GithubReadCredential, deadline?: AbortSignal) => Promise /** The token for a read GitHub denied the App, or null. */ @@ -183,6 +188,10 @@ export function parseGithubCredentialConfig( * the reason is reported, so a broken key shows up as an event instead of as * a spent quota. A failure stands for about {@link FAILED_MINT_SECONDS}: reads * in that window use the fallback with no mint and no second report. + * + * Create one credential per request, scheduled task or queue batch. Reads + * through one credential share its mint in flight. A request with no cached + * token mints its own, and never waits for another request's mint. */ export function createGithubCredential( config: GithubCredentialConfig, @@ -212,22 +221,28 @@ export function createGithubCredential( } const app = config.app const key = `${app.appId}:${app.installationId}` + // This request's mint in flight. It never enters the isolate cache. + let pending: Promise | null = null const startMint = (deadline: AbortSignal | undefined): Promise => { + const failureAtStart = cache.failedUntil.get(key) const minting = mintInstallationToken(app, runtime, deadline) .then((outcome) => { if (outcome._tag === 'minted') { cache.tokens.set(key, outcome.token) cache.failedUntil.delete(key) } - else if (outcome._tag === 'failed') { - // One event for the window, from the read that started the mint. + // One event for the window. A failure another request recorded while + // this mint ran already has its event. + else if (outcome._tag === 'failed' && !(failureStands(key) && cache.failedUntil.get(key) !== failureAtStart)) { standFailure(key) runtime.report({ outcome: 'app-token-unavailable', reason: outcome.reason, fallback: fallbackName }) } return outcome }) - .finally(() => cache.pending.delete(key)) - cache.pending.set(key, minting) + .finally(() => { + pending = null + }) + pending = minting return minting } const credentialFor = async (deadline: AbortSignal | undefined, mintThroughFailure: boolean): Promise => { @@ -236,13 +251,13 @@ export function createGithubCredential( const cached = cache.tokens.get(key) if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) return { token: cached.token, isApp: true } - let pending = cache.pending.get(key) - if (!pending) { + let minting = pending + if (!minting) { if (!mintThroughFailure && failureStands(key)) return fallback - pending = startMint(deadline) + minting = startMint(deadline) } - const outcome = await untilAborted(pending, deadline) + const outcome = await untilAborted(minting, deadline) if (outcome._tag === 'minted') return { token: outcome.token.token, isApp: true } if (outcome._tag === 'failed') diff --git a/test/unit/registry-github-app-credential.test.ts b/test/unit/registry-github-app-credential.test.ts index 9ddd3de3..9177965b 100644 --- a/test/unit/registry-github-app-credential.test.ts +++ b/test/unit/registry-github-app-credential.test.ts @@ -1,4 +1,5 @@ import type { GithubCredentialReport } from '../../shared/server/github-app-credential' +import { AsyncLocalStorage } from 'node:async_hooks' import { afterEach, describe, expect, it, vi } from 'vitest' import { getRepo, @@ -307,8 +308,8 @@ describe('a failed mint', () => { expect(github.reads.at(-1)).toBe('Bearer ghs_app_2') }) - it('costs one mint and one report for reads that meet it together and after', async () => { - const github = await stubGithub({ mintPlan: ['fail'] }) + it('costs one report for requests that meet it together, and no mint after', async () => { + const github = await stubGithub({ mintPlan: ['fail', 'fail', 'fail'] }) const reports: GithubCredentialReport[] = [] const isolate = runtime(reports) const env = await appEnv({ GITHUB_TOKEN: 'site' }) @@ -318,7 +319,7 @@ describe('a failed mint', () => { await Promise.all([read('ui'), read('nuxt'), read('image')]) await Promise.all([read('ui'), read('nuxt')]) - expect(github.mints).toBe(1) + expect(github.mints).toBe(3) expect(github.reads).toEqual(Array.from({ length: 5 }).fill('Bearer site')) expect(reports).toHaveLength(1) }) @@ -372,6 +373,89 @@ describe('a failed mint', () => { }) }) +// workerd ties a fetch to the request that made it. A request that awaited +// another request's mint could throw "Cannot perform I/O on behalf of a +// different request", or wait forever once that request ended. Requests share +// only a finished token. +describe('requests that find no token in the isolate', () => { + it('each read with a token minted in their own I/O context', async () => { + const github = await stubGithub({ mintPlan: ['held', 'held'] }) + const isolate = runtime() + const env = await appEnv({ GITHUB_TOKEN: 'site' }) + const first = workerRequest('first') + const second = workerRequest('second') + + const reads = Promise.all([ + first.run(() => getRepo('nuxt', 'ui', resolveGithubBindings(env, isolate))), + second.run(() => getRepo('nuxt', 'nuxt', resolveGithubBindings(env, isolate))), + ]) + await vi.waitFor(() => expect(github.mints).toBe(2)) + github.releaseMints() + + expect((await reads).map(repo => repo.status)).toEqual([200, 200]) + // Either request can sign its JWT first, so either can mint `ghs_app_1`. + const mintedBy = new Map(github.mintsBy.map(([request, token]) => [`Bearer ${token}`, request])) + const readWithMintOf = new Map(github.readsBy.map(([request, authorization]) => [request, mintedBy.get(authorization!)])) + expect(readWithMintOf).toEqual(new Map([['first', 'first'], ['second', 'second']])) + }) + + it('leaves a request on the App when the request whose mint it met ends', async () => { + const github = await stubGithub({ mintPlan: ['held'] }) + const reports: GithubCredentialReport[] = [] + const isolate = runtime(reports) + const env = await appEnv({ GITHUB_TOKEN: 'site' }) + const first = workerRequest('first') + const second = workerRequest('second') + + // The first request ends before GitHub answers its mint, so the answer + // never arrives. + void first.run(() => getRepo('nuxt', 'ui', resolveGithubBindings(env, isolate))) + await vi.waitFor(() => expect(github.mints).toBe(1)) + const read = second.run(() => getRepo('nuxt', 'nuxt', resolveGithubBindings(env, isolate))) + first.end() + github.releaseMints() + + const repo = await within(read, 1_000) + // The mint that never answered holds no later request either. + const later = await within(workerRequest('third').run(() => getRepo('nuxt', 'image', resolveGithubBindings(env, isolate))), 1_000) + + expect([repo.status, later.status]).toEqual([200, 200]) + expect(github.mints).toBe(2) + expect(github.readsBy).toEqual([['second', 'Bearer ghs_app_2'], ['third', 'Bearer ghs_app_2']]) + expect(reports).toEqual([]) + }) +}) + +/** + * One Worker request's I/O context. A fetch belongs to the request that made + * it: once that request ends, its answer never arrives. + */ +interface IoContext { + name: string + ended: boolean +} + +const ioContexts = new AsyncLocalStorage() + +function workerRequest(name: string) { + const context: IoContext = { name, ended: false } + return { + run: async (work: () => Promise): Promise => await ioContexts.run(context, work), + end: () => { + context.ended = true + }, + } +} + +/** The promise's value, or a rejection if it takes longer than `ms`. */ +async function within(promise: Promise, ms: number): Promise { + let timer: ReturnType | undefined + const limit = new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(`still waiting after ${ms} ms`)), ms) + }) + return await Promise.race([promise, limit]).finally(() => clearTimeout(timer)) +} + function runtime(reports: GithubCredentialReport[] = [], clock = { now: NOW }) { return { tokenCache: createInstallationTokenCache(), now: () => clock.now, report: (event: GithubCredentialReport) => reports.push(event) } } @@ -411,35 +495,55 @@ interface StubOptions { hold?: { token: string, until: Promise } } -/** `hang` answers nothing until the request is aborted. `fail` answers 500. */ -type MintAnswer = 'hang' | 'fail' +/** + * `hang` answers nothing until the request is aborted. `fail` answers 500. + * `held` answers a token once the test calls `releaseMints`. + */ +type MintAnswer = 'hang' | 'fail' | 'held' /** * GitHub as the App and a personal token see it. Each mint answers a new * installation token: `ghs_app_1`, then `ghs_app_2`. */ async function stubGithub(options: StubOptions) { - const state = { mints: 0, reads: [] as Array, mintSignals: [] as Array } + let releaseMints = () => {} + const released = new Promise((resolve) => { + releaseMints = resolve + }) + const state = { + mints: 0, + reads: [] as Array, + mintSignals: [] as Array, + /** The I/O context that made each mint, with the token GitHub answered. */ + mintsBy: [] as Array<[string | undefined, string]>, + /** The I/O context that made each read, with its authorization. */ + readsBy: [] as Array<[string | undefined, string | null]>, + releaseMints: () => releaseMints(), + } const hold = { pending: options.hold } const appQuota = { 'x-ratelimit-remaining': '4990', 'x-ratelimit-limit': '5000' } - vi.stubGlobal('fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const answer = async (input: RequestInfo | URL, init: RequestInit | undefined, context: IoContext | undefined): Promise => { const url = String(input) const headers = new Headers(init?.headers) if (url.endsWith(`/app/installations/${INSTALLATION_ID}/access_tokens`)) { - state.mints++ + const token = `ghs_app_${++state.mints}` state.mintSignals.push(init?.signal) - const answer = options.mintPlan?.shift() - if (answer === 'fail') + state.mintsBy.push([context?.name, token]) + const plan = options.mintPlan?.shift() + if (plan === 'fail') return new Response('{}', { status: 500 }) - if (answer === 'hang') { + if (plan === 'hang') { return await new Promise((_, reject) => { init?.signal?.addEventListener('abort', () => reject(init.signal!.reason), { once: true }) }) } - return Response.json({ token: `ghs_app_${state.mints}`, expires_at: new Date((NOW + 3600) * 1000).toISOString() }, { status: 201 }) + if (plan === 'held') + await released + return Response.json({ token, expires_at: new Date((NOW + 3600) * 1000).toISOString() }, { status: 201 }) } const authorization = headers.get('authorization') state.reads.push(authorization) + state.readsBy.push([context?.name, authorization]) if (hold.pending && authorization === `Bearer ${hold.pending.token}`) { const until = hold.pending.until hold.pending = undefined @@ -505,6 +609,12 @@ async function stubGithub(options: StubOptions) { if (options.deniesApp && asApp && url.includes('/neondatabase/')) return Response.json({ message: 'Resource not accessible by integration' }, { status: 403, headers: quota }) return Response.json({ name: 'repo', full_name: 'owner/repo', owner: { login: 'owner' }, default_branch: 'main' }, { status: 200, headers: quota }) + } + vi.stubGlobal('fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const context = ioContexts.getStore() + const response = await answer(input, init, context) + // workerd drops the answer to a fetch whose request has ended. + return context?.ended ? await new Promise(() => {}) : response })) return state }