From ebfdb0dd5eb5114a1e75a3ac07ca312686726ae2 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 13:57:21 +1100 Subject: [PATCH 1/3] fix(github): bound the App mint by the read deadline, cache failures A mint ignored the 4 second page read limit, so a hung GitHub held /gh pages for the mint's own 15 seconds on each read. A failed mint was not remembered, so every read tried a new one and reported it, and the sync paced on the fallback token the whole time. A read now passes its deadline to the mint it waits for. A failed mint stands for about 60 seconds per isolate with one event. A deadline that ends a mint is no failure, and a 401 on a minted token still re-mints once inside a window. --- docs/ops/artifact-delivery-rollout.md | 4 +- .../artifact-delivery/server/utils/queue.ts | 1 + layers/registry/server/utils/github-client.ts | 4 + shared/server/github-app-credential.ts | 159 +++++++++++++---- test/unit/artifact-github-read-app.test.ts | 1 + .../registry-github-app-credential.test.ts | 166 +++++++++++++++++- 6 files changed, 294 insertions(+), 41 deletions(-) diff --git a/docs/ops/artifact-delivery-rollout.md b/docs/ops/artifact-delivery-rollout.md index 2a381ef6..1cb2930b 100644 --- a/docs/ops/artifact-delivery-rollout.md +++ b/docs/ops/artifact-delivery-rollout.md @@ -123,7 +123,9 @@ Each denial emits an `app-denied` event: `github-credential` for registry reads If GitHub answers 401 to the cached installation token, the read mints one new token and repeats. If GitHub rejects the new token too, the read uses the fallback and emits an `app-token-rejected` event. -If the App secrets are set but unusable, or GitHub refuses to mint a token, every read uses the fallback and emits an event with the reason. The sync then paces itself on the fallback's quota. +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. 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/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 71481590..e8d5cdcb 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -188,6 +188,7 @@ function defaultGithubSourceRuntime(): GithubCredentialRuntime { // and the registry reads in this isolate share it. tokenCache: isolateInstallationTokenCache(), report: reportGithubCredential, + random: Math.random, } } diff --git a/layers/registry/server/utils/github-client.ts b/layers/registry/server/utils/github-client.ts index e801ef51..593fcb1d 100644 --- a/layers/registry/server/utils/github-client.ts +++ b/layers/registry/server/utils/github-client.ts @@ -100,6 +100,7 @@ export function resolveGithubBindings( now: runtime.now ?? (() => Math.floor(Date.now() / 1000)), tokenCache: runtime.tokenCache ?? isolateInstallationTokenCache(), report, + random: Math.random, }), reportCredential: report, } @@ -122,6 +123,7 @@ function readAsApp( bindings: GithubBindings, read: { label: string + deadline?: AbortSignal send: (token: string | undefined) => Promise refusal: (answer: A) => Promise fallbackOnDenial: boolean @@ -277,6 +279,8 @@ async function ghRequest( const read = await readAsApp(bindings, { label: new URL(url).pathname, + // A page read's limit covers the mint it waits for, too. + deadline: init?.signal ?? undefined, send: token => fetch(url, { ...init, headers: withAuthorization(headers, token) }), refusal: githubAppRefusal, fallbackOnDenial: true, diff --git a/shared/server/github-app-credential.ts b/shared/server/github-app-credential.ts index ef1e30d8..cf9cc46a 100644 --- a/shared/server/github-app-credential.ts +++ b/shared/server/github-app-credential.ts @@ -21,6 +21,19 @@ const MAX_TOKEN_RESPONSE_BYTES = 64 * 1024 */ export const INSTALLATION_TOKEN_RENEW_SECONDS = 5 * 60 +/** + * How long a failed mint stands. Reads in that time use the fallback without + * a mint, so a broken App costs one mint and one event a minute per isolate, + * not one per read. + */ +export const FAILED_MINT_SECONDS = 60 + +/** The spread around {@link FAILED_MINT_SECONDS}, so isolates do not mint again together. */ +const FAILED_MINT_JITTER_SECONDS = 10 + +/** The failure window key for App secrets that do not parse. */ +const MISCONFIGURED_KEY = 'misconfigured' + /** The read App: metadata read only, installed on skilld-dev. */ export interface ReadAppConfig { appId: number @@ -73,11 +86,21 @@ interface CachedInstallationToken { */ export interface InstallationTokenCache { tokens: Map - pending: Map> + /** Unix seconds until which a failure stands, by key. */ + failedUntil: Map + pending: Map> } +/** How one mint ended. */ +type MintOutcome + = | { _tag: 'minted', token: CachedInstallationToken } + /** GitHub refused, failed or outlasted the mint's own limit. */ + | { _tag: 'failed', reason: string } + /** The deadline of the read that started the mint ended it. It says nothing about the App. */ + | { _tag: 'cut-off' } + export function createInstallationTokenCache(): InstallationTokenCache { - return { tokens: new Map(), pending: new Map() } + return { tokens: new Map(), failedUntil: new Map(), pending: new Map() } } let isolateTokenCache: InstallationTokenCache | null = null @@ -97,6 +120,8 @@ export interface GithubCredentialRuntime { now: () => number tokenCache: InstallationTokenCache report: (event: GithubCredentialReport) => void + /** A number in [0, 1), for the failed mint window's jitter. */ + random: () => number } /** The credential for one GitHub read. An undefined token reads anonymously. */ @@ -107,13 +132,18 @@ export interface GithubReadCredential { } export interface GithubCredential { - current: () => Promise + /** + * The credential for one read. A mint the read waits for ends at + * `deadline`, and the read then rejects with the deadline's reason, as its + * own request would. A read with no deadline waits for the mint's own limit. + */ + current: (deadline?: AbortSignal) => Promise /** * 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. + * that held it, even while a failed mint stands. */ - renew: (rejected: GithubReadCredential) => Promise + renew: (rejected: GithubReadCredential, deadline?: AbortSignal) => Promise /** The token for a read GitHub denied the App, or null. */ fallback: FallbackToken | null } @@ -151,7 +181,8 @@ export function parseGithubCredentialConfig( * * When the App cannot give a token, the read still runs on the fallback and * the reason is reported, so a broken key shows up as an event instead of as - * a spent quota. + * 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. */ export function createGithubCredential( config: GithubCredentialConfig, @@ -163,44 +194,93 @@ export function createGithubCredential( return staticGithubCredential(config.token.token) const fallbackName = config.fallback?.name ?? 'anonymous' const fallback = { token: config.fallback?.token, isApp: false } + const cache = runtime.tokenCache + const failureStands = (key: string) => (cache.failedUntil.get(key) ?? 0) > runtime.now() + const standFailure = (key: string) => cache.failedUntil.set( + key, + runtime.now() + FAILED_MINT_SECONDS + (runtime.random() - 0.5) * FAILED_MINT_JITTER_SECONDS, + ) if (config._tag === 'app-misconfigured') { const current = async (): Promise => { - runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) + if (!failureStands(MISCONFIGURED_KEY)) { + standFailure(MISCONFIGURED_KEY) + runtime.report({ outcome: 'app-misconfigured', reason: config.reason, fallback: fallbackName }) + } return fallback } return { current, renew: current, fallback: null } } const app = config.app const key = `${app.appId}:${app.installationId}` - const current = async (): Promise => { - const cached = runtime.tokenCache.tokens.get(key) - if (cached && cached.expiresAt - INSTALLATION_TOKEN_RENEW_SECONDS > runtime.now()) - return { token: cached.token, isApp: true } - let pending = runtime.tokenCache.pending.get(key) - if (!pending) { - pending = mintInstallationToken(app, runtime).finally(() => runtime.tokenCache.pending.delete(key)) - runtime.tokenCache.pending.set(key, pending) - } - const minted = await pending - if ('token' in minted) { - runtime.tokenCache.tokens.set(key, minted) - return { token: minted.token, isApp: true } + const startMint = (deadline: AbortSignal | undefined): Promise => { + 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. + 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) + return minting + } + const credentialFor = async (deadline: AbortSignal | undefined, mintThroughFailure: boolean): Promise => { + while (true) { + deadline?.throwIfAborted() + 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) { + if (!mintThroughFailure && failureStands(key)) + return fallback + pending = startMint(deadline) + } + const outcome = await untilAborted(pending, deadline) + if (outcome._tag === 'minted') + return { token: outcome.token.token, isApp: true } + if (outcome._tag === 'failed') + return fallback + // Another read's deadline ended that mint. This read's deadline has not + // ended, so it mints again. } - runtime.report({ outcome: 'app-token-unavailable', reason: minted.reason, fallback: fallbackName }) - return fallback } return { - current, - renew: async (rejected) => { + current: async deadline => await credentialFor(deadline, false), + renew: async (rejected, deadline) => { // Another read that held the same token may have renewed it already. - if (runtime.tokenCache.tokens.get(key)?.token === rejected.token) - runtime.tokenCache.tokens.delete(key) - return await current() + // The read that drops the token mints a new one, even while a failed + // mint stands: the token worked, so the App may work again. + const drops = cache.tokens.get(key)?.token === rejected.token + if (drops) + cache.tokens.delete(key) + return await credentialFor(deadline, drops) }, fallback: config.fallback, } } +/** The promise's value, or the deadline's reason if the deadline ends first. */ +async function untilAborted(promise: Promise, deadline: AbortSignal | undefined): Promise { + if (!deadline) + return await promise + return await new Promise((resolve, reject) => { + const abort = () => reject(deadline.reason) + if (deadline.aborted) { + abort() + return + } + deadline.addEventListener('abort', abort, { once: true }) + void promise.then(resolve, reject).finally(() => deadline.removeEventListener('abort', abort)) + }) +} + /** A credential of one token, or anonymous reads, with no App and no fallback. */ export function staticGithubCredential(token: string | undefined): GithubCredential { const current = async (): Promise => ({ token, isApp: false }) @@ -259,6 +339,8 @@ export async function readWithGithubCredential( read: { /** The Repository or API path, for the report. Never a token or a query string. */ label: string + /** The read's own deadline, which also ends a mint the read waits for. */ + deadline?: AbortSignal send: (token: string | undefined) => Promise /** How GitHub refused a read made as the App, or null when it did not. */ refusal: (answer: A) => Promise @@ -267,11 +349,11 @@ export async function readWithGithubCredential( fallbackOnDenial: boolean }, ): Promise> { - let current = await credential.current() + let current = await credential.current(read.deadline) let answer = await read.send(current.token) let refusal = current.isApp ? await read.refusal(answer) : null if (refusal === 'unauthorized') { - current = await credential.renew(current) + current = await credential.renew(current, read.deadline) answer = await read.send(current.token) refusal = current.isApp ? await read.refusal(answer) : null } @@ -313,7 +395,10 @@ export async function requestInstallationToken(input: { jwt: string installationId: number body: { repository_ids?: number[], permissions: Record } + /** The caller's deadline. The request also ends at its own limit. */ + deadline?: AbortSignal }): Promise { + const limit = AbortSignal.timeout(GITHUB_REQUEST_TIMEOUT_MS) const response = await input.fetch(`${GITHUB_API}/app/installations/${input.installationId}/access_tokens`, { method: 'POST', headers: { @@ -326,7 +411,7 @@ export async function requestInstallationToken(input: { body: JSON.stringify(input.body), // workerd accepts only `follow` and `manual`. A redirect here is an error. redirect: 'manual', - signal: AbortSignal.timeout(GITHUB_REQUEST_TIMEOUT_MS), + signal: input.deadline ? AbortSignal.any([input.deadline, limit]) : limit, }) if (response.status >= 300 && response.status < 400) { await response.body?.cancel() @@ -383,23 +468,25 @@ export async function signGithubAppJwt(input: { async function mintInstallationToken( app: ReadAppConfig, runtime: Pick, -): Promise { + deadline: AbortSignal | undefined, +): Promise { const minted = await signGithubAppJwt({ issuer: String(app.appId), privateKeyPkcs8: app.privateKeyPkcs8, now: runtime.now() }) .then(jwt => requestInstallationToken({ fetch: runtime.fetch, jwt, installationId: app.installationId, body: { permissions: { metadata: 'read' } }, + deadline, })) - // A network fault, a 5xx or a key WebCrypto rejects: the read uses the - // fallback token, and the reason reaches the report. + // A network fault, a 5xx, a timeout or a key WebCrypto rejects: the read + // uses the fallback token, and the reason reaches the report. .catch((error: unknown) => ({ _tag: 'failed' as const, reason: error instanceof Error ? error.message : String(error) })) if (minted._tag === 'failed') - return { _tag: 'unavailable', reason: minted.reason.slice(0, 200) } + return deadline?.aborted ? { _tag: 'cut-off' } : { _tag: 'failed', reason: minted.reason.slice(0, 200) } if (minted._tag === 'refused') - return { _tag: 'unavailable', reason: `GitHub refused the installation token (${minted.status})` } + return { _tag: 'failed', reason: `GitHub refused the installation token (${minted.status})` } const expiresAt = Math.floor(Date.parse(minted.expiresAt) / 1000) - return { token: minted.token, expiresAt } + return { _tag: 'minted', token: { token: minted.token, expiresAt } } } function firstToken(env: GithubCredentialEnv, names: ReadonlyArray): FallbackToken | null { diff --git a/test/unit/artifact-github-read-app.test.ts b/test/unit/artifact-github-read-app.test.ts index d712d9d5..ca1dba8c 100644 --- a/test/unit/artifact-github-read-app.test.ts +++ b/test/unit/artifact-github-read-app.test.ts @@ -275,6 +275,7 @@ function runtime( now: overrides.now ?? (() => NOW), tokenCache: overrides.cache ?? createInstallationTokenCache(), report: overrides.report ?? (() => {}), + random: () => 0.5, } } diff --git a/test/unit/registry-github-app-credential.test.ts b/test/unit/registry-github-app-credential.test.ts index 0455a82f..9ddd3de3 100644 --- a/test/unit/registry-github-app-credential.test.ts +++ b/test/unit/registry-github-app-credential.test.ts @@ -7,7 +7,7 @@ import { resolveGithubBindings, } from '../../layers/registry/server/utils/github-client' import { githubRateLimited } from '../../layers/registry/server/utils/github-rate-limited' -import { createInstallationTokenCache } from '../../shared/server/github-app-credential' +import { createInstallationTokenCache, INSTALLATION_TOKEN_RENEW_SECONDS } from '../../shared/server/github-app-credential' // Every Worker GitHub read used `GITHUB_TOKEN`, a personal token whose quota // the owner's own tools and CI runners also spend. The read App's @@ -236,8 +236,144 @@ describe('a read GitHub answered 401 for the cached installation token', () => { }) }) -function runtime(reports: GithubCredentialReport[] = []) { - return { tokenCache: createInstallationTokenCache(), now: () => NOW, report: (event: GithubCredentialReport) => reports.push(event) } +// A mint GitHub never answered held a page read for the mint's own 15 second +// limit, not the page's 4 second read limit. +describe('a mint for a read with a deadline', () => { + it('ends at the read deadline, not at the mint limit', async () => { + const github = await stubGithub({ mintPlan: ['hang'] }) + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) + const started = Date.now() + + await expect(getRepo('nuxt', 'ui', bindings, { timeoutMs: 50 })).rejects.toMatchObject({ name: 'TimeoutError' }) + + expect(Date.now() - started).toBeLessThan(2_000) + expect(github.mintSignals[0]?.aborted).toBe(true) + expect(github.reads).toEqual([]) + }) + + // A page can start a read with little of its budget left. That says + // nothing about the App, so the next read mints again. + it('leaves the next read on the App when only the deadline ended the mint', async () => { + const github = await stubGithub({ mintPlan: ['hang'] }) + const reports: GithubCredentialReport[] = [] + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime(reports)) + await expect(getRepo('nuxt', 'ui', bindings, { timeoutMs: 50 })).rejects.toMatchObject({ name: 'TimeoutError' }) + + const repo = await getRepo('nuxt', 'ui', bindings) + + expect(repo.status).toBe(200) + expect(github.mints).toBe(2) + expect(github.reads).toEqual(['Bearer ghs_app_2']) + expect(reports).toEqual([]) + }) + + it('holds a read with no deadline to no page deadline', async () => { + const github = await stubGithub({ mintPlan: ['hang'] }) + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime()) + + const [page, sync] = await Promise.allSettled([ + getRepo('nuxt', 'ui', bindings, { timeoutMs: 50 }), + getRepo('nuxt', 'nuxt', bindings), + ]) + + expect(page).toMatchObject({ status: 'rejected', reason: { name: 'TimeoutError' } }) + expect(sync).toMatchObject({ status: 'fulfilled', value: { status: 200 } }) + expect(github.mints).toBe(2) + expect(github.reads).toEqual(['Bearer ghs_app_2']) + }) +}) + +// Every read tried a new mint after one failed, and reported it. A broken App +// cost one mint and one event per read, and the sync paced on GITHUB_TOKEN. +describe('a failed mint', () => { + it('stands for about a minute, then the next read mints again', async () => { + const github = await stubGithub({ mintPlan: ['fail'] }) + const reports: GithubCredentialReport[] = [] + const clock = { now: NOW } + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime(reports, clock)) + + await getRepo('nuxt', 'ui', bindings) + clock.now = NOW + 54 + await getRepo('nuxt', 'ui', bindings) + + expect(github.mints).toBe(1) + expect(github.reads).toEqual(['Bearer site', 'Bearer site']) + expect(reports).toEqual([{ outcome: 'app-token-unavailable', reason: 'GitHub App request returned 500', fallback: 'GITHUB_TOKEN' }]) + + clock.now = NOW + 65 + await getRepo('nuxt', 'ui', bindings) + + expect(github.mints).toBe(2) + 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'] }) + const reports: GithubCredentialReport[] = [] + const isolate = runtime(reports) + const env = await appEnv({ GITHUB_TOKEN: 'site' }) + // Each request resolves its own bindings over the one isolate cache. + const read = (repo: string) => getRepo('nuxt', repo, resolveGithubBindings(env, isolate)) + + await Promise.all([read('ui'), read('nuxt'), read('image')]) + await Promise.all([read('ui'), read('nuxt')]) + + expect(github.mints).toBe(1) + expect(github.reads).toEqual(Array.from({ length: 5 }).fill('Bearer site')) + expect(reports).toHaveLength(1) + }) + + it('never stops the one new mint after GitHub answered 401 for a minted token', async () => { + const rejects: string[] = [] + const mintPlan: MintAnswer[] = [] + let release = () => {} + const until = new Promise((resolve) => { + release = resolve + }) + const github = await stubGithub({ rejects, mintPlan, hold: { token: 'ghs_app_1', until } }) + const reports: GithubCredentialReport[] = [] + const clock = { now: NOW } + const bindings = resolveGithubBindings(await appEnv({ GITHUB_TOKEN: 'site' }), runtime(reports, clock)) + + // This read holds the first token while the next mint fails. + const held = getRepo('nuxt', 'ui', bindings) + await vi.waitFor(() => expect(github.reads).toEqual(['Bearer ghs_app_1'])) + clock.now = NOW + 3600 - INSTALLATION_TOKEN_RENEW_SECONDS + mintPlan.push('fail') + await getRepo('nuxt', 'nuxt', bindings) + expect(github.reads.at(-1)).toBe('Bearer site') + + rejects.push('ghs_app_1') + release() + const repo = await held + + expect(repo.status).toBe(200) + expect(github.mints).toBe(3) + expect(github.reads.at(-1)).toBe('Bearer ghs_app_3') + }) + + it('reports unusable App secrets once a window, not once a read', async () => { + const github = await stubGithub({}) + const reports: GithubCredentialReport[] = [] + const clock = { now: NOW } + const env = { ...(await appEnv({ GITHUB_TOKEN: 'site' })), SKILLD_READ_APP_ID: 'not-an-id' } + const bindings = resolveGithubBindings(env, runtime(reports, clock)) + + await getRepo('nuxt', 'ui', bindings) + await getRepo('nuxt', 'nuxt', bindings) + expect(reports).toHaveLength(1) + + clock.now = NOW + 65 + await getRepo('nuxt', 'ui', bindings) + + expect(github.mints).toBe(0) + expect(reports).toHaveLength(2) + expect(reports[1]).toEqual({ outcome: 'app-misconfigured', reason: 'SKILLD_READ_APP_ID is not a GitHub App ID', fallback: 'GITHUB_TOKEN' }) + }) +}) + +function runtime(reports: GithubCredentialReport[] = [], clock = { now: NOW }) { + return { tokenCache: createInstallationTokenCache(), now: () => clock.now, report: (event: GithubCredentialReport) => reports.push(event) } } async function appEnv(tokens: { GITHUB_TOKEN?: string }) { @@ -269,24 +405,46 @@ interface StubOptions { fallbackAnswers?: 'rate-limited' | 'unauthorized' | 'low-quota' /** Installation tokens GitHub answers 401, as for a revoked token. */ rejects?: string[] + /** How GitHub answers each mint, in order. A mint past the end succeeds. */ + mintPlan?: MintAnswer[] + /** The first read with this token waits for `until` before GitHub answers it. */ + hold?: { token: string, until: Promise } } +/** `hang` answers nothing until the request is aborted. `fail` answers 500. */ +type MintAnswer = 'hang' | 'fail' + /** * 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 } + const state = { mints: 0, reads: [] as Array, mintSignals: [] as Array } + 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 url = String(input) const headers = new Headers(init?.headers) if (url.endsWith(`/app/installations/${INSTALLATION_ID}/access_tokens`)) { state.mints++ + state.mintSignals.push(init?.signal) + const answer = options.mintPlan?.shift() + if (answer === 'fail') + return new Response('{}', { status: 500 }) + if (answer === '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 }) } const authorization = headers.get('authorization') state.reads.push(authorization) + if (hold.pending && authorization === `Bearer ${hold.pending.token}`) { + const until = hold.pending.until + hold.pending = undefined + await until + } const asApp = authorization?.startsWith('Bearer ghs_app_') === true const graphql = url.endsWith('/graphql') if (asApp && options.rejects?.includes(authorization!.slice('Bearer '.length))) From c665aec5872d4f40c21940c9e53e49d5b738cf33 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 14:29:13 +1100 Subject: [PATCH 2/3] fix(github): keep each App mint inside the request that started it The isolate token cache shared one mint promise across requests. workerd ties a fetch to the request that made it. When that request answered before its mint finished, workerd dropped the mint, and the shared promise never settled. Every later request in the isolate that found no token waited on it: in local workerd, the next two requests got no answer in 40 and 10 seconds. The isolate cache now holds finished values only: a token with its expiry, and the failure window. A mint in flight stays with the credential of the request that started it. Reads in one request still share one mint. Two requests that find no token each mint their own, and a failure that one request records while the other mints keeps the window to one event. --- docs/ops/artifact-delivery-rollout.md | 3 + layers/registry/server/utils/github-client.ts | 3 + shared/server/github-app-credential.ts | 39 ++++-- .../registry-github-app-credential.test.ts | 132 ++++++++++++++++-- 4 files changed, 153 insertions(+), 24 deletions(-) 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..1412b006 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,87 @@ 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]) + expect(new Map(github.mintsBy)).toEqual(new Map([['first', 'ghs_app_1'], ['second', 'ghs_app_2']])) + expect(new Map(github.readsBy)).toEqual(new Map([['first', 'Bearer ghs_app_1'], ['second', 'Bearer ghs_app_2']])) + }) + + 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 +493,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 +607,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 } From faddc4af043421c9507f003de27cded65f2bc9fe Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 7 Oct 2026 14:40:00 +1100 Subject: [PATCH 3/3] test(github): match each read to its own mint, in any order Either request can sign its JWT first, so either can mint the first token. The test now checks that each request read with the token its own request minted. --- test/unit/registry-github-app-credential.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unit/registry-github-app-credential.test.ts b/test/unit/registry-github-app-credential.test.ts index 1412b006..9177965b 100644 --- a/test/unit/registry-github-app-credential.test.ts +++ b/test/unit/registry-github-app-credential.test.ts @@ -393,8 +393,10 @@ describe('requests that find no token in the isolate', () => { github.releaseMints() expect((await reads).map(repo => repo.status)).toEqual([200, 200]) - expect(new Map(github.mintsBy)).toEqual(new Map([['first', 'ghs_app_1'], ['second', 'ghs_app_2']])) - expect(new Map(github.readsBy)).toEqual(new Map([['first', 'Bearer ghs_app_1'], ['second', 'Bearer ghs_app_2']])) + // 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 () => {