diff --git a/.dev.vars.example b/.dev.vars.example index 06e4d999..063a60b9 100644 --- a/.dev.vars.example +++ b/.dev.vars.example @@ -11,6 +11,9 @@ NUXT_OG_IMAGE_SECRET= NUXT_IMAGE_PROXY_KEY= NUXT_AI_READY_RUNTIME_SYNC_SECRET= GITHUB_TOKEN= +# Public Artifact builds read GitHub with this token when it is set. Use a token +# from another GitHub account, so runs keep a quota the registry sync cannot spend. +ARTIFACT_GITHUB_TOKEN= ARTIFACT_PRIVATE_ACCESS_ENABLED=false GITHUB_APP_ID= GITHUB_APP_CLIENT_ID= diff --git a/docs/adr/0012-resolution-poll-allowance.md b/docs/adr/0012-resolution-poll-allowance.md new file mode 100644 index 00000000..6ef929e1 --- /dev/null +++ b/docs/adr/0012-resolution-poll-allowance.md @@ -0,0 +1,32 @@ +# ADR-0012: Resolution poll allowance + +Date: 2026-10-07 + +Amends [ADR-0008](0008-api-rate-limits.md). + +## Context + +ADR-0008 gives every v1 operation one shared allowance. +A guest gets 60 requests per 60 seconds, keyed by `CF-Connecting-IP`. + +`skilld run` polls `GET /api/v1/resolutions/{id}` at the rate the server names in `pollAfterMs`. +The server names 1000 ms. The CLI waits up to 60 seconds for a Resolution. +One build that takes a minute therefore costs about 60 polls. +That spends the whole guest allowance, so the run fails with `RATE_LIMITED`. +A second run from the same network fails the same way. + +## Decision + +A poll of one Resolution draws from a separate binding, `API_RESOLUTION_POLL_RATE_LIMIT`. +It allows 600 requests per 60 seconds. +It uses the ADR-0008 key: the verified account ID, or the guest IP. +Ten runs from one network can poll through a one-minute build. + +A poll is a `GET` of `/api/v1/resolutions/{id}`. +Every other v1 request keeps the ADR-0008 allowance, including the `POST` that starts a Resolution. +An exhausted poll allowance answers like an exhausted shared one: HTTP 429, `RATE_LIMITED`, and `Retry-After: 60`. + +## Consequences + +A poll is one indexed D1 read. 600 polls per minute is 10 reads per second for one key. +The starting `POST` stays in the shared allowance, so the number of builds one network can start does not change. diff --git a/docs/arch/README.md b/docs/arch/README.md index 8d5ce47d..96bc3b7d 100644 --- a/docs/arch/README.md +++ b/docs/arch/README.md @@ -45,7 +45,7 @@ descriptor (skilld-dev/skilld: packages/sdk/src/contract) └─ parity test test/unit/api-v1-parity.test.ts: one route file per operation ``` -- **Operation**: one `namespace.verb` entry in the contract, such as `skills.get`. Its ID never changes. Public handlers never receive caller identity. Rate-limit middleware verifies credentials only to select an allowance. Account operations reuse that identity. V1 responses bypass Workers Cache so every network request reaches the limiter. See [ADR-0008](../adr/0008-api-rate-limits.md). +- **Operation**: one `namespace.verb` entry in the contract, such as `skills.get`. Its ID never changes. Public handlers never receive caller identity. Rate-limit middleware verifies credentials only to select an allowance. Account operations reuse that identity. V1 responses bypass Workers Cache so every network request reaches the limiter. See [ADR-0008](../adr/0008-api-rate-limits.md). Resolution polls draw from their own allowance: [ADR-0012](../adr/0012-resolution-poll-allowance.md). - **defineApiOperation**: the shape of a `/api/v1` route: `{ operation, handler }`. The descriptor parses the input, checks the credential, and sets the status and cache headers. The handler only loads data. The answer passes the strict producer schema, so an unnamed field fails closed with a 500. - **Route file**: lives in the layer that owns the data, at the operation's own path: `GET /api/v1/skills/{owner}/{repository}/{name}` is `layers/registry/server/api/v1/skills/[owner]/[repository]/[name].get.ts`. It calls that layer's utilities, or reads the internal route in process. It never duplicates SQL. - **v1 presenter**: a pure function in `layers//server/presenters/-v1.ts`, typed `OperationResult`. diff --git a/layers/artifact-delivery/server/utils/build.ts b/layers/artifact-delivery/server/utils/build.ts index 2ee950fb..92ff37d0 100644 --- a/layers/artifact-delivery/server/utils/build.ts +++ b/layers/artifact-delivery/server/utils/build.ts @@ -662,6 +662,13 @@ async function failWithRejection( ) } +/** + * The wait a retryable failure names when its cause named none. GitHub asks + * for at least a minute after a secondary rate limit, and the build queue + * gives up only after several minutes of failed attempts. + */ +export const DEFAULT_RETRY_AFTER_SECONDS = 60 + export async function failResolution( dependencies: Pick, row: NonNullable>>, @@ -675,9 +682,13 @@ export async function failResolution( // The upstream reset header is an absolute epoch; error_retry_after means a // relative delay in seconds, like HTTP Retry-After. Store the delay a reader // can add to now, and never let a clock skew store zero or a negative one. - const errorRetryAfter = retryAtEpochSeconds === undefined + // A retryable failure whose cause named no time still names one, so the + // CLI never prints "may be retried" without saying when. + const errorRetryAfter = !retryable ? undefined - : Math.max(1, retryAtEpochSeconds - now) + : retryAtEpochSeconds === undefined + ? DEFAULT_RETRY_AFTER_SECONDS + : Math.max(1, retryAtEpochSeconds - now) const advanced = await transitionResolution(dependencies.db, row, 'failed', { errorCode: code, errorRetryable: retryable, diff --git a/layers/artifact-delivery/server/utils/github-source.ts b/layers/artifact-delivery/server/utils/github-source.ts index 70f4d79a..f463451c 100644 --- a/layers/artifact-delivery/server/utils/github-source.ts +++ b/layers/artifact-delivery/server/utils/github-source.ts @@ -166,6 +166,8 @@ interface GithubClientOptions { onReadFailure?: (failure: GithubReadFailure) => void sleep?: (milliseconds: number) => Promise random?: () => number + /** Unix seconds. A Retry-After delay becomes an absolute retry time against it. */ + now?: () => number } export function createPublicGithubSourceClient(options: GithubClientOptions): PublicGithubSourceClient { @@ -174,6 +176,7 @@ export function createPublicGithubSourceClient(options: GithubClientOptions): Pu export function createGithubSourceClient(options: GithubClientOptions): PublicGithubSourceClient { const expectedVisibility = options.visibility ?? 'public' + const now = options.now ?? (() => Math.floor(Date.now() / 1000)) const readRejection = (outcome: FailedReadOutcome) => outcome._tag === 'rate-limited' ? rateLimitRejection(outcome.resetAt) @@ -214,15 +217,15 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const response = fetched.response if (response.status === 404) return { _tag: 'settled', value: { _tag: 'not-found' } } - if (response.status === 401 || response.status === 403) { - // A spent quota is a fact about this minute, not about the Repository. - // It used to throw, which wrote nothing: the Resolution sat in its - // current state through the 60, 120, 240, 480 second delivery ladder and - // then failed as SERVICE_UNAVAILABLE with no reason on it. - if (response.status === 403 && response.headers.get('x-ratelimit-remaining') === '0') - return { _tag: 'settled', value: { _tag: 'rate-limited', resetAt: epochHeader(response.headers, 'x-ratelimit-reset') } } + // A spent quota is a fact about this minute, not about the Repository. + // It used to throw, which wrote nothing: the Resolution sat in its + // current state through the delivery retry ladder and then failed as + // SERVICE_UNAVAILABLE with no reason on it. + const limited = githubRateLimit(response, now()) + if (limited) + return { _tag: 'settled', value: limited } + if (response.status === 401 || response.status === 403) return { _tag: 'settled', value: { _tag: 'access-denied' } } - } if (!response.ok) { await response.body?.cancel().catch(() => { // The status already decided the outcome. A body that will not close changes nothing. @@ -552,6 +555,10 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi const extracted = await loadFromTarball(source, selected.entries) if (extracted._tag === 'extracted') return { _tag: 'loaded', value: { source, files: extracted.files, omitted } } + // The blobs API reads one file per request against the same limit. + // Falling back would spend the most quota exactly when none is left. + if (extracted._tag === 'rate-limited') + return rateLimitRejection(extracted.resetAt) // The tarball is an optimisation, never an authority. Anything it got // wrong, including a file `.gitattributes export-ignore` removed from // the archive, falls through to the blobs API, which serves every blob @@ -573,7 +580,7 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi async function loadFromTarball( source: ResolvedSource, entries: Array, - ): Promise { + ): Promise { const headers = new Headers({ 'Accept': 'application/vnd.github+json', 'User-Agent': 'skilld.dev', @@ -594,6 +601,11 @@ export function createGithubSourceClient(options: GithubClientOptions): PublicGi }) if ('_tag' in response) return { _tag: 'unusable', reason: 'unavailable', findings: [response.message] } + const limited = githubRateLimit(response, now()) + if (limited) { + await response.body?.cancel() + return limited + } if (!response.ok || !response.body) { await response.body?.cancel() return { _tag: 'unusable', reason: 'unavailable', findings: [`GitHub returned ${response.status}`] } @@ -1011,6 +1023,31 @@ function epochHeader(headers: Headers, name: string): number | null { return Number.isSafeInteger(value) && value > 0 ? value : null } +interface GithubRateLimit { + _tag: 'rate-limited' + /** Unix seconds GitHub allows the next request, when it said so. */ + resetAt: number | null +} + +/** + * GitHub's rate limit answer, or null for any other response. + * + * GitHub answers a spent primary quota with 403 or 429 and no remaining + * requests, and a secondary limit with 403 or 429 and Retry-After. A 429 is + * always a limit. A 403 with neither header denies access. + * https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api + */ +export function githubRateLimit(response: Pick, now: number): GithubRateLimit | null { + if (response.status !== 403 && response.status !== 429) + return null + const retryAfter = epochHeader(response.headers, 'retry-after') + if (retryAfter !== null) + return { _tag: 'rate-limited', resetAt: now + retryAfter } + if (response.headers.get('x-ratelimit-remaining') === '0') + return { _tag: 'rate-limited', resetAt: epochHeader(response.headers, 'x-ratelimit-reset') } + return response.status === 429 ? { _tag: 'rate-limited', resetAt: null } : null +} + function rateLimitRejection(resetAt: number | null): SourceRejection { const rejection = reject('RATE_LIMITED', 'GitHub refused the read: its rate limit is spent.', []) return resetAt === null ? rejection : { ...rejection, retryAfterSeconds: resetAt } diff --git a/layers/artifact-delivery/server/utils/queue.ts b/layers/artifact-delivery/server/utils/queue.ts index 7c2466f0..2fe4417f 100644 --- a/layers/artifact-delivery/server/utils/queue.ts +++ b/layers/artifact-delivery/server/utils/queue.ts @@ -25,6 +25,17 @@ const artifactBuildMessageSchema = z.object({ resolutionId: z.string().uuid(), }).strict() +/** + * The wait before each retry of a build attempt that threw. A fifth failed + * attempt fails the Resolution as SERVICE_UNAVAILABLE. + * + * The CLI waits 60 seconds for a Resolution, so the first retry comes within + * that window: one transient GitHub or signer error no longer fails the run. + * It used to wait 60 seconds. The later waits still ride out a long GitHub + * network fault, such as the one on 2026-09-30. + */ +export const ARTIFACT_BUILD_RETRY_DELAYS_SECONDS: readonly number[] = [5, 30, 120, 480] + export async function enqueueArtifactBuild(env: Cloudflare.Env, resolutionId: string): Promise { await env.ARTIFACT_BUILD_QUEUE.send({ version: 1, resolutionId }) } @@ -59,8 +70,9 @@ export async function consumeArtifactBuildBatch( attempt: message.attempts, error: outcome.error instanceof Error ? outcome.error.message : String(outcome.error), })) - if (message.attempts < 5) { - message.retry({ delaySeconds: Math.min(3600, 60 * 2 ** Math.max(0, message.attempts - 1)) }) + const delaySeconds = ARTIFACT_BUILD_RETRY_DELAYS_SECONDS[message.attempts - 1] + if (delaySeconds !== undefined) { + message.retry({ delaySeconds }) continue } const row = await getResolution(dependencies.db, parsed.data.resolutionId) @@ -86,6 +98,19 @@ export function reportGithubReadFailure(failure: GithubReadFailure): void { })) } +/** + * The token public Artifact builds read GitHub with. + * + * GitHub counts a personal token's quota per account, and the registry sync + * spends `GITHUB_TOKEN` to zero before each hourly reset. Every run that needed + * GitHub in those minutes failed RATE_LIMITED. `ARTIFACT_GITHUB_TOKEN`, from + * another account, keeps a quota for runs. Until it is set, builds share + * `GITHUB_TOKEN`. + */ +export function artifactGithubToken(env: Partial>): string | undefined { + return env.ARTIFACT_GITHUB_TOKEN || env.GITHUB_TOKEN +} + export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBuildDependencies { const runtimeFetch = globalThis.fetch.bind(globalThis) const privateDependencies = privateArtifactAccessEnabled(env) @@ -93,7 +118,7 @@ export function createArtifactBuildDependencies(env: Cloudflare.Env): ArtifactBu : {} return { db: env.DB, - github: createPublicGithubSourceClient({ fetch: runtimeFetch, token: env.GITHUB_TOKEN, onReadFailure: reportGithubReadFailure }), + github: createPublicGithubSourceClient({ fetch: runtimeFetch, token: artifactGithubToken(env), onReadFailure: reportGithubReadFailure }), bucket: env.PUBLIC_ARTIFACTS, signer: createArtifactSigner(env.ARTIFACT_SIGNER), trustedRoot: parseTrustedRoot(env.ARTIFACT_TRUSTED_ROOT_JSON, Math.floor(Date.now() / 1000)), diff --git a/shared/server/api-rate-limit.ts b/shared/server/api-rate-limit.ts index 72051f19..ebc8b464 100644 --- a/shared/server/api-rate-limit.ts +++ b/shared/server/api-rate-limit.ts @@ -13,7 +13,10 @@ declare module 'h3' { } } -/** One allowance across v1 operations, keyed by verified account or Cloudflare's client IP. */ +/** + * One allowance across v1 operations, keyed by verified account or Cloudflare's client IP. + * Resolution polls draw from a separate allowance under the same key. + */ export function createApiRateLimitHandler(resolveUser: (event: H3Event) => Promise) { return defineEventHandler(async (event) => { const path = event.path.split('?')[0]! @@ -30,7 +33,9 @@ export function createApiRateLimitHandler(resolveUser: (event: H3Event) => Promi const { env, requestId } = event.context.platform // Do not trust X-Forwarded-For. Missing Cloudflare identity shares a conservative bucket. const key = user ? `account:${user.id}` : `guest:${getHeader(event, 'cf-connecting-ip') ?? 'unknown'}` - const limiter = user ? env.API_ACCOUNT_RATE_LIMIT : env.API_GUEST_RATE_LIMIT + const limiter = isResolutionPoll(event.method, path) + ? env.API_RESOLUTION_POLL_RATE_LIMIT + : user ? env.API_ACCOUNT_RATE_LIMIT : env.API_GUEST_RATE_LIMIT const { success } = await limiter.limit({ key }) if (success) return @@ -52,3 +57,16 @@ export function createApiRateLimitHandler(resolveUser: (event: H3Event) => Promi }) }) } + +const RESOLUTION_POLL_PATH = /^\/api\/v1\/resolutions\/[^/]+$/ + +/** + * A read of one Resolution while its build runs. + * + * The server sets the poll rate through `pollAfterMs`, so a poll is not a + * request the caller chose to make. A one-minute build costs sixty polls, + * which used to spend the whole guest allowance (ADR-0012). + */ +function isResolutionPoll(method: string, path: string): boolean { + return method === 'GET' && RESOLUTION_POLL_PATH.test(path) +} diff --git a/test/unit/api-rate-limit.test.ts b/test/unit/api-rate-limit.test.ts index 6741032b..7afa6f3c 100644 --- a/test/unit/api-rate-limit.test.ts +++ b/test/unit/api-rate-limit.test.ts @@ -5,12 +5,14 @@ import { createApiRateLimitHandler } from '../../shared/server/api-rate-limit' const guest = vi.fn() const account = vi.fn() +const poll = vi.fn() const resolveUser = vi.fn() +const RESOLUTION_ID = '0d6f6c2e-5b1a-4c3e-9f1d-2a7b8c9d0e1f' function serve() { const app = createApp() app.use(eventHandler((event) => { - event.context.platform = { requestId: 'request-test', env: { API_GUEST_RATE_LIMIT: { limit: guest }, API_ACCOUNT_RATE_LIMIT: { limit: account } } } as never + event.context.platform = { requestId: 'request-test', env: { API_GUEST_RATE_LIMIT: { limit: guest }, API_ACCOUNT_RATE_LIMIT: { limit: account }, API_RESOLUTION_POLL_RATE_LIMIT: { limit: poll } } } as never })) app.use(createApiRateLimitHandler(resolveUser)) app.use(eventHandler(() => ({ ok: true }))) @@ -22,6 +24,7 @@ describe('api rate limits', () => { beforeEach(() => { guest.mockReset().mockResolvedValue({ success: true }) account.mockReset().mockResolvedValue({ success: true }) + poll.mockReset().mockResolvedValue({ success: true }) resolveUser.mockReset().mockResolvedValue(null) }) @@ -93,4 +96,35 @@ describe('api rate limits', () => { expect(await response.json()).toMatchObject({ code: 'RATE_LIMITED' }) expect(guest).not.toHaveBeenCalled() }) + + // A run polls its Resolution once a second while it builds. Sixty polls used + // to spend the whole guest allowance, so one slow build failed the run with + // RATE_LIMITED, and so did a second run from the same network. + it('counts Resolution polls against their own allowance, not the shared one', async () => { + const fetch = serve() + for (let attempt = 0; attempt < 3; attempt++) + await fetch(`/api/v1/resolutions/${RESOLUTION_ID}`, { 'CF-Connecting-IP': '203.0.113.8' }) + expect(poll.mock.calls).toEqual(Array.from({ length: 3 }, () => [{ key: 'guest:203.0.113.8' }])) + expect(guest).not.toHaveBeenCalled() + }) + + it('keys an account poll by the account', async () => { + resolveUser.mockResolvedValue({ id: 7, login: 'octo' }) + await serve()(`/api/v1/resolutions/${RESOLUTION_ID}`, { Authorization: 'Bearer valid' }) + expect(poll).toHaveBeenCalledWith({ key: 'account:7' }) + expect(account).not.toHaveBeenCalled() + }) + + it('still counts the request that starts a Resolution against the shared allowance', async () => { + await serve()('/api/v1/resolutions', { 'CF-Connecting-IP': '203.0.113.8' }, 'POST') + expect(guest).toHaveBeenCalledWith({ key: 'guest:203.0.113.8' }) + expect(poll).not.toHaveBeenCalled() + }) + + it('limits polls once their own allowance is spent', async () => { + poll.mockResolvedValue({ success: false }) + const response = await serve()(`/api/v1/resolutions/${RESOLUTION_ID}`, { 'CF-Connecting-IP': '203.0.113.8' }) + expect(response.status).toBe(429) + expect(await response.json()).toMatchObject({ code: 'RATE_LIMITED' }) + }) }) diff --git a/test/unit/artifact-rate-limit.test.ts b/test/unit/artifact-rate-limit.test.ts index 824295be..4d7437ed 100644 --- a/test/unit/artifact-rate-limit.test.ts +++ b/test/unit/artifact-rate-limit.test.ts @@ -1,6 +1,7 @@ import type { SourceRequest } from '../../layers/artifact-delivery/server/schemas/contracts' import type { ArtifactSigner } from '../../layers/artifact-delivery/server/utils/attestation' import type { ArtifactBuildDependencies } from '../../layers/artifact-delivery/server/utils/build' +import type { PublicGithubSourceClient } from '../../layers/artifact-delivery/server/utils/github-source' import type { TrustedRoot } from '../../layers/artifact-delivery/server/utils/trusted-root' import { describe, expect, it, vi } from 'vitest' import { processArtifactBuild } from '../../layers/artifact-delivery/server/utils/build' @@ -8,7 +9,8 @@ import { createPublicGithubSourceClient, isRetryableProblem, } from '../../layers/artifact-delivery/server/utils/github-source' -import { createResolution, resolutionRequestIdentity } from '../../layers/artifact-delivery/server/utils/state' +import { ARTIFACT_BUILD_QUEUE_NAME, artifactGithubToken, consumeArtifactBuildBatch } from '../../layers/artifact-delivery/server/utils/queue' +import { createResolution, getResolution, presentResolution, resolutionRequestIdentity } from '../../layers/artifact-delivery/server/utils/state' import { createSqliteD1 } from './helpers/d1-sqlite' const commitSha = '0123456789abcdef0123456789abcdef01234567' @@ -123,6 +125,111 @@ describe('gitHub rate limits as values', () => { }) }) +// GitHub answers a secondary rate limit with 403 or 429 and a Retry-After +// header, and a spent primary quota with 403 or 429 and no remaining requests. +// A 403 with Retry-After used to fail the run as SOURCE_ACCESS_DENIED, which +// nobody retries. A 429 used to throw, so the build sat through the queue +// retry ladder and spent quota on every attempt. +describe('gitHub secondary rate limits as values', () => { + const request: SourceRequest = { + provider: 'github', + owner: 'skilld-dev', + repository: 'skills', + selector: { type: 'path', path: 'skills/demo' }, + } + + it.each([403, 429])('rejects a %i with Retry-After as RATE_LIMITED after the named delay', async (status) => { + const client = createPublicGithubSourceClient({ + fetch: vi.fn(async () => new Response('{}', { + status, + headers: { 'retry-after': '30', 'x-ratelimit-remaining': '4321', 'content-type': 'application/json' }, + })) as unknown as typeof fetch, + now: () => NOW, + }) + + expect(await client.resolve(request)).toMatchObject({ + _tag: 'rejected', + code: 'RATE_LIMITED', + retryAfterSeconds: NOW + 30, + }) + }) + + it('rejects a 429 spent quota with its reset time', async () => { + const client = createPublicGithubSourceClient({ + fetch: vi.fn(async () => new Response('{}', { + status: 429, + headers: { 'x-ratelimit-remaining': '0', 'x-ratelimit-reset': String(RESET_AT), 'content-type': 'application/json' }, + })) as unknown as typeof fetch, + }) + + expect(await client.resolve(request)).toMatchObject({ + _tag: 'rejected', + code: 'RATE_LIMITED', + retryAfterSeconds: RESET_AT, + }) + }) + + it('stops at a rate-limited tarball instead of reading every blob', async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input) + if (url.includes('/tarball/')) { + return new Response('{}', { + status: 403, + headers: { 'retry-after': '60', 'content-type': 'application/json' }, + }) + } + if (url.includes('/git/blobs/')) + return json({ sha: 'd'.repeat(40), size: 12, encoding: 'base64', content: btoa('# Demo skill') }) + if (url.endsWith('/repos/skilld-dev/skills')) + return json({ id: 123, name: 'skills', owner: { login: 'skilld-dev' }, private: false, default_branch: 'main' }) + if (url.endsWith('/git/trees/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa')) + return json({ sha: 'a'.repeat(40), tree: [{ path: 'skills', mode: '040000', type: 'tree', sha: 'b'.repeat(40) }] }) + if (url.endsWith('/git/trees/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb')) + return json({ sha: 'b'.repeat(40), tree: [{ path: 'demo', mode: '040000', type: 'tree', sha: 'c'.repeat(40) }] }) + if (url.endsWith('/git/trees/cccccccccccccccccccccccccccccccccccccccc?recursive=1')) { + return json({ + sha: 'c'.repeat(40), + truncated: false, + tree: [{ path: 'SKILL.md', mode: '100644', type: 'blob', sha: 'd'.repeat(40), size: 12 }], + }) + } + return json({}, 404) + }) + const client = createPublicGithubSourceClient({ + fetch: fetchMock as unknown as typeof fetch, + now: () => NOW, + }) + + const result = await client.load({ + provider: 'github', + repositoryId: 123, + owner: 'skilld-dev', + repository: 'skills', + visibility: 'public', + commitSha, + treeSha: 'a'.repeat(40), + skillPath: 'skills/demo', + }) + + expect(result).toMatchObject({ _tag: 'rejected', code: 'RATE_LIMITED', retryAfterSeconds: NOW + 60 }) + expect(fetchMock.mock.calls.some(([input]) => String(input).includes('/git/blobs/'))).toBe(false) + }) +}) + +// The hourly registry sync spends the site token to zero before each GitHub +// reset. On 2026-10-06, 13 runs failed RATE_LIMITED in those minutes. A token +// from another GitHub account carries its own quota, kept for runs. +describe('the token Artifact builds read GitHub with', () => { + it('prefers the token kept for Artifact builds', () => { + expect(artifactGithubToken({ ARTIFACT_GITHUB_TOKEN: 'runs', GITHUB_TOKEN: 'site' })).toBe('runs') + }) + + it('uses the site token while no Artifact token is set', () => { + expect(artifactGithubToken({ ARTIFACT_GITHUB_TOKEN: '', GITHUB_TOKEN: 'site' })).toBe('site') + expect(artifactGithubToken({ GITHUB_TOKEN: 'site' })).toBe('site') + }) +}) + describe('which problems a caller may retry', () => { it.each([ ['RATE_LIMITED', true], @@ -170,6 +277,86 @@ describe('a spent quota persists a delay, not the reset epoch', () => { }) }) +describe('every retryable failure names its wait', () => { + it('names a wait when GitHub sent no reset', async () => { + const harness = await requestedResolution('rate-limit-no-reset-0001') + const dependencies = buildDependencies(harness.sqlite.db, createPublicGithubSourceClient({ + fetch: vi.fn(async () => new Response('{}', { status: 429 })) as unknown as typeof fetch, + })) + + await processArtifactBuild(dependencies, harness.id) + + const row = await getResolution(harness.sqlite.db, harness.id) + expect(presentResolution(row!)).toMatchObject({ state: 'failed', code: 'RATE_LIMITED', retryable: true, retryAfterSeconds: 60 }) + harness.sqlite.close() + }) + + it('names a wait when the build queue gives up', async () => { + const harness = await requestedResolution('queue-gives-up-0001') + const message = queueMessage(harness.id, 5) + + await consumeArtifactBuildBatch({} as Cloudflare.Env, queueBatch([message]), () => buildDependencies(harness.sqlite.db, githubDown())) + + const row = await getResolution(harness.sqlite.db, harness.id) + expect(presentResolution(row!)).toMatchObject({ state: 'failed', code: 'SERVICE_UNAVAILABLE', retryable: true, retryAfterSeconds: 60 }) + harness.sqlite.close() + }) +}) + +// The CLI waits 60 seconds for a Resolution. The first queue retry used to +// wait 60 seconds too, so one transient GitHub or signer error failed every +// run that met it, although the retry then built the Artifact. +describe('a failed build attempt retries inside the run deadline', () => { + it('retries the first failed attempt within ten seconds', async () => { + const harness = await requestedResolution('queue-first-retry-0001') + const message = queueMessage(harness.id, 1) + + await consumeArtifactBuildBatch({} as Cloudflare.Env, queueBatch([message]), () => buildDependencies(harness.sqlite.db, githubDown())) + + expect(message.retry).toHaveBeenCalledOnce() + expect(message.retry.mock.calls[0]![0].delaySeconds).toBeLessThanOrEqual(10) + harness.sqlite.close() + }) +}) + +async function requestedResolution(idempotencyKey: string) { + const sqlite = createSqliteD1(ARTIFACT_MIGRATIONS) + const identity = await resolutionRequestIdentity(resolutionRequest, idempotencyKey) + const created = await createResolution(sqlite.db, resolutionRequest, identity, NOW) + if (created._tag === 'idempotency-conflict') + throw new Error('Test Resolution conflicted') + return { sqlite, id: created.row.id } +} + +function buildDependencies(db: D1Database, github: PublicGithubSourceClient): ArtifactBuildDependencies { + return { + db, + github, + bucket: {} as R2Bucket, + signer: {} as ArtifactSigner, + trustedRoot: {} as TrustedRoot, + now: () => NOW, + } +} + +function githubDown(): PublicGithubSourceClient { + const fault = async () => Promise.reject(new Error('The operation was aborted due to timeout')) + return { resolve: vi.fn(fault), load: vi.fn(fault) } +} + +function queueMessage(resolutionId: string, attempts: number) { + return { + body: { version: 1, resolutionId }, + attempts, + ack: vi.fn(), + retry: vi.fn<(options: { delaySeconds: number }) => void>(), + } +} + +function queueBatch(messages: Array>) { + return { queue: ARTIFACT_BUILD_QUEUE_NAME, messages } as unknown as Parameters[1] +} + function rateLimited(): Response { return rateLimitedAt(RESET_AT) } diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 051b5bc3..dd96a883 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: 50fee44c1595f78f46cc0523374267df) +// Generated by Wrangler by running `wrangler types --config=wrangler.jsonc --env-file=.dev.vars.example` (hash: 44fdc49aa25ec9befef6b0cb85d9da7e) // Runtime types generated with workerd@1.20260828.1 2026-08-20 nodejs_compat interface __BaseEnv_Env { KV_CACHE: KVNamespace; @@ -17,6 +17,7 @@ interface __BaseEnv_Env { ARTIFACT_BUILD_QUEUE: Queue; API_GUEST_RATE_LIMIT: RateLimit; API_ACCOUNT_RATE_LIMIT: RateLimit; + API_RESOLUTION_POLL_RATE_LIMIT: RateLimit; TASK_SEARCH_RATE_LIMIT: RateLimit; AI: Ai; CF_VERSION_METADATA: WorkerVersionMetadata; @@ -36,6 +37,7 @@ interface __BaseEnv_Env { NUXT_IMAGE_PROXY_KEY: string; NUXT_AI_READY_RUNTIME_SYNC_SECRET: string; GITHUB_TOKEN: string; + ARTIFACT_GITHUB_TOKEN: string; ARTIFACT_PRIVATE_ACCESS_ENABLED: string; GITHUB_APP_ID: string; GITHUB_APP_CLIENT_ID: string; @@ -63,7 +65,7 @@ type StringifyValues> = { [Binding in keyof EnvType]: EnvType[Binding] extends string ? EnvType[Binding] : string; }; declare namespace NodeJS { - interface ProcessEnv extends StringifyValues> {} + interface ProcessEnv extends StringifyValues> {} } // Begin runtime types diff --git a/wrangler.jsonc b/wrangler.jsonc index 521662a5..619fb32e 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -13,6 +13,10 @@ "ratelimits": [ { "name": "API_GUEST_RATE_LIMIT", "namespace_id": "2401001", "simple": { "limit": 60, "period": 60 } }, { "name": "API_ACCOUNT_RATE_LIMIT", "namespace_id": "2401002", "simple": { "limit": 600, "period": 60 } }, + // A run polls its Resolution once a second while it builds, at the rate the + // server names. Polls draw from this allowance, keyed like the two above, + // so ten runs from one network can wait out a one-minute build (ADR-0012). + { "name": "API_RESOLUTION_POLL_RATE_LIMIT", "namespace_id": "2401004", "simple": { "limit": 600, "period": 60 } }, // Task search runs a paid model. The longest period a rate limit binding // takes is 60 seconds; the daily ceiling is the spend budget in // runtimeConfig.taskSearch, across every visitor.