Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/cli/script/generate-api-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,22 @@
* Also reads SDK function names from the installed @sentry/api package to
* map operationIds to their TypeScript SDK function names.
*
* Also reads the `@sentry/api/operations` catalog to list the non-safe
* operations Sentry marks idempotent (`x-sentry-idempotent`), which the
* authenticated fetch retries on transient failures.
*
* Usage:
* tsx script/generate-api-schema.ts
*
* Output:
* src/generated/api-schema.json
* src/generated/idempotent-operations.json
*/

import { mkdir, readFile, writeFile } from "node:fs/promises";
import { dirname, resolve } from "node:path";
import { fileURLToPath } from "node:url";
import { operations } from "@sentry/api/operations";

/** Resolve the root directory of @sentry/api by finding its main entry point. */
function sentryApiDir(): string {
Expand All @@ -32,6 +38,7 @@ function sentryApiDir(): string {
}

const OUTPUT_PATH = "src/generated/api-schema.json";
const IDEMPOTENT_OUTPUT_PATH = "src/generated/idempotent-operations.json";

/**
* Build the OpenAPI spec URL from the installed @sentry/api version.
Expand All @@ -53,6 +60,8 @@ const PATH_PARAM_PATTERN = /\{(\w+)\}/g;
export type { ApiEndpoint } from "../src/lib/api-schema.js";

import type { ApiEndpoint } from "../src/lib/api-schema.js";
import type { IdempotentOperation } from "../src/lib/idempotent-operations.js";
import { isRetryableMethod } from "../src/lib/retry-policy.js";

// ---------------------------------------------------------------------------
// OpenAPI Types (minimal subset we need)
Expand Down Expand Up @@ -231,3 +240,21 @@ await writeFile(OUTPUT_PATH, JSON.stringify(endpoints, null, 2));
console.log(
`Generated ${OUTPUT_PATH} (${endpoints.length} endpoints, ${Math.round(JSON.stringify(endpoints).length / 1024)}KB)`,
);

// Safe methods already retry, so only writes need listing; that keeps the
// file the CLI loads on every request tiny.
const idempotentOperations: IdempotentOperation[] = operations
.filter((op) => op.idempotent && !isRetryableMethod(op.method))
.map((op) => ({ method: op.method, path: op.path }))
.sort(
(a, b) => a.path.localeCompare(b.path) || a.method.localeCompare(b.method),
);

await writeFile(
IDEMPOTENT_OUTPUT_PATH,
`${JSON.stringify(idempotentOperations, null, 2)}\n`,
);

console.log(
`Generated ${IDEMPOTENT_OUTPUT_PATH} (${idempotentOperations.length} operations)`,
);
14 changes: 14 additions & 0 deletions packages/cli/src/generated/idempotent-operations.json
Comment thread
sentry-junior[bot] marked this conversation as resolved.
Resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[
{
"method": "POST",
"path": "/api/0/organizations/{organization_id_or_slug}/artifactbundle/assemble/"
},
{
"method": "POST",
"path": "/api/0/organizations/{organization_id_or_slug}/chunk-upload/"
},
{
"method": "POST",
"path": "/api/0/projects/{organization_id_or_slug}/{project_id_or_slug}/files/difs/assemble/"
}
]
10 changes: 4 additions & 6 deletions packages/cli/src/lib/api/chunk-upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,8 @@
}

const limit = pLimit(serverOptions.concurrency);
// Chunks are stored by SHA-1, so a replayed upload is a no-op.
const { fetch: authFetch } = getSdkConfig(regionUrl, { idempotent: true });
// Sentry marks chunk-upload idempotent, so the fetch retries it.
const { fetch: authFetch } = getSdkConfig(regionUrl);

Check warning on line 399 in packages/cli/src/lib/api/chunk-upload.ts

View check run for this annotation

@sentry/warden / warden: code-review

Chunk uploads lose retries when server URL is not the catalog path

Removing `idempotent: true` means chunk POSTs only retry when `serverOptions.url` matches the OpenAPI template, but the CLI posts to the server-provided URL, which can be a non-catalog path like `/chunks/`. Keep the opt-in (or match on the request regardless of path) so content-addressed chunk uploads still retry.
Comment thread
sentry-junior[bot] marked this conversation as resolved.
Resolved

await Promise.all(
missingChunks.map((chunk) =>
Expand Down Expand Up @@ -488,8 +488,8 @@
}

const limit = pLimit(serverOptions.concurrency);
// Chunks are stored by SHA-1, so a replayed upload is a no-op.
const { fetch: authFetch } = getSdkConfig(regionUrl, { idempotent: true });
// Sentry marks chunk-upload idempotent, so the fetch retries it.
const { fetch: authFetch } = getSdkConfig(regionUrl);

await Promise.all(
missingChunks.map((chunk) =>
Expand Down Expand Up @@ -552,8 +552,6 @@
method: "POST",
body,
schema,
// Assembly state is keyed by checksum; re-sending only polls it.
idempotent: true,
},
);

Expand Down
3 changes: 0 additions & 3 deletions packages/cli/src/lib/api/dart-symbols.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,6 @@ export async function uploadDartSymbolMap(
method: "POST",
body: assembleBody,
schema: DifAssembleResponseSchema,
// Assembly state is keyed by checksum; re-sending only polls it.
idempotent: true,
},
);

Expand Down Expand Up @@ -196,7 +194,6 @@ export async function uploadDartSymbolMap(
method: "POST",
body: assembleBody,
schema: DifAssembleResponseSchema,
idempotent: true,
},
);

Expand Down
2 changes: 0 additions & 2 deletions packages/cli/src/lib/api/debug-files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -272,15 +272,13 @@ async function postAssemble(
endpoint: string,
body: unknown,
): Promise<DifAssembleResponse> {
// Assembly state is keyed by checksum; re-sending only polls it.
const { data } = await apiRequestToRegion<DifAssembleResponse>(
regionUrl,
endpoint,
{
method: "POST",
body,
schema: DifAssembleResponseSchema,
idempotent: true,
},
);
return data;
Expand Down
5 changes: 3 additions & 2 deletions packages/cli/src/lib/api/infrastructure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,9 @@ export type ApiRequestOptions<T = unknown> = {
/** Manually validate each redirect before forwarding credentials. */
validatedRedirects?: boolean;
/**
* Retry transient failures even for writes. Only for requests the server
* dedupes (see `idempotent` in sentry-client.ts).
* Retry transient failures even for writes the server does not mark
* idempotent. Only for requests the server dedupes (see `idempotent` in
* sentry-client.ts).
*/
idempotent?: boolean;
Comment thread
BYK marked this conversation as resolved.
Resolved
};
Expand Down
3 changes: 0 additions & 3 deletions packages/cli/src/lib/api/proguard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,6 @@ export async function uploadProguardMappings(
method: "POST",
body: assembleBody,
schema: DifAssembleResponseSchema,
// Assembly state is keyed by checksum; re-sending only polls it.
idempotent: true,
},
);

Expand Down Expand Up @@ -237,7 +235,6 @@ async function pollDifAssembly(params: {
method: "POST",
body,
schema: DifAssembleResponseSchema,
idempotent: true,
},
);

Expand Down
2 changes: 0 additions & 2 deletions packages/cli/src/lib/api/sourcemaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,6 @@ async function uploadArtifactBundle(opts: {
method: "POST",
body: assembleBody,
schema: AssembleResponseSchema,
// Assembly state is keyed by checksum; re-sending only polls it.
idempotent: true,
},
);

Expand Down
67 changes: 67 additions & 0 deletions packages/cli/src/lib/idempotent-operations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* Writes Sentry declares safe to replay.
*
* `script/generate-api-schema.ts` copies the non-safe operations that the
* `@sentry/api` catalog marks `idempotent` (from Sentry's
* `x-sentry-idempotent` OpenAPI extension). Endpoints opt in on the server,
* so the CLI no longer hard-codes which assemble and upload calls may retry.
* The generated file is committed and refreshed by `generate:schema`; commit
* it again after bumping `@sentry/api`.
*/

import generated from "../generated/idempotent-operations.json";

/** A non-safe operation Sentry marks idempotent. */
export type IdempotentOperation = {
/** Uppercase HTTP method */
method: string;
/** OpenAPI path template, such as `/api/0/organizations/{org}/chunk-upload/` */
path: string;
};

type Matcher = { method: string; pattern: RegExp };

const TEMPLATE_SEGMENT = /^\{[^/]+\}$/;
const REGEXP_SPECIAL = /[.*+?^${}()|[\]\\]/g;

function toMatcher({ method, path }: IdempotentOperation): Matcher {
const source = path
.split("/")
.map((segment) =>
TEMPLATE_SEGMENT.test(segment)
? "[^/]+"
: segment.replace(REGEXP_SPECIAL, "\\$&"),
)
.join("/");
return { method: method.toUpperCase(), pattern: new RegExp(`^${source}$`) };
}

let matchers: Matcher[] | undefined;

/**
* Whether Sentry marks this request's operation idempotent.
*
* Matches from the `/api/` segment on, so a self-hosted install served under
* a path prefix still matches.
*
* @param method - HTTP method (any case)
* @param url - Absolute request URL
*/
export function isIdempotentOperation(method: string, url: string): boolean {
if (!URL.canParse(url)) {
return false;
}
const pathname = new URL(url).pathname;
const apiIndex = pathname.indexOf("/api/");
if (apiIndex === -1) {
return false;
}
const apiPath = pathname.slice(apiIndex);
const normalized = apiPath.endsWith("/") ? apiPath : `${apiPath}/`;
const upper = method.toUpperCase();

matchers ??= (generated satisfies IdempotentOperation[]).map(toMatcher);
return matchers.some(
(matcher) => matcher.method === upper && matcher.pattern.test(normalized),
);
}
6 changes: 4 additions & 2 deletions packages/cli/src/lib/retry-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
* server already applied it, so replaying could duplicate the write.
* PUT and DELETE are excluded too: Sentry uses PUT for partial updates with
* side effects (issue merges, activity, notifications), and a replayed
* DELETE turns a successful delete into a 404. Callers that know a write is
* safe to replay (content-addressed uploads) opt in per request.
* DELETE turns a successful delete into a 404. Writes Sentry marks
* idempotent (`x-sentry-idempotent`, see idempotent-operations.ts) replay,
* and callers that know an unmarked write is safe to replay opt in per
* request.
*
* A 429 is the exception and retries for any method: Sentry's rate-limit
* middleware rejects the request before the view runs, so nothing was
Expand Down
14 changes: 10 additions & 4 deletions packages/cli/src/lib/sentry-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
TimeoutError,
} from "./errors.js";
import { extractInputUrl } from "./fetch-input.js";
import { isIdempotentOperation } from "./idempotent-operations.js";
import { logger } from "./logger.js";
import {
type CacheLookup,
Expand Down Expand Up @@ -707,7 +708,10 @@ async function fetchWithRetry({
}
const attemptFactory = await buildAttemptFactory(input, init);
const timeoutMs = resolveTimeoutMs(fullUrl);
const methodRetryable = isRetryableMethod(method, idempotent);
const methodRetryable = isRetryableMethod(
method,
idempotent || isIdempotentOperation(method, fullUrl),
);
const urlPath = extractUrlPath(fullUrl);

for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) {
Expand Down Expand Up @@ -763,7 +767,8 @@ async function fetchWithRetry({
* - Auth token injection (Bearer token)
* - Request timeout via AbortController
* - Automatic retry on transient HTTP errors (408, 429, 5xx), network errors,
* and timeouts — for GET/HEAD/OPTIONS, or any method with `idempotent`
* and timeouts — for GET/HEAD/OPTIONS, operations Sentry marks idempotent
* (see idempotent-operations.ts), or any method with `idempotent`
* - 429 handling (any method; rejected before processing): waits for the
* server's rate-limit reset when it fits the cap, otherwise throws
* {@link RateLimitError}
Expand All @@ -784,8 +789,9 @@ type AuthenticatedFetchOptions = {
bufferResponseBody?: boolean;
/**
* Caller asserts that replaying the request is harmless, so transient
* failures retry for any method. Only for writes the server dedupes, such
* as content-addressed chunk uploads and checksum-keyed assemble calls.
* failures retry for any method. Operations Sentry marks idempotent retry
* without this; set it only for writes the server dedupes but does not
* mark, such as preprod assemble polling.
*/
idempotent?: boolean;
};
Expand Down
81 changes: 81 additions & 0 deletions packages/cli/test/lib/idempotent-operations.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { describe, expect, test } from "vitest";
import { isIdempotentOperation } from "../../src/lib/idempotent-operations.js";

const REGION_URL = "https://us.sentry.io";

describe("isIdempotentOperation", () => {
test.each([
"/api/0/organizations/acme/chunk-upload/",
"/api/0/organizations/acme/artifactbundle/assemble/",
"/api/0/projects/acme/web/files/difs/assemble/",
])("matches POST %s", (path) => {
expect(isIdempotentOperation("POST", `${REGION_URL}${path}`)).toBe(true);
});

test("ignores method case and a missing trailing slash", () => {
expect(
isIdempotentOperation(
"post",
`${REGION_URL}/api/0/organizations/acme/chunk-upload`,
),
).toBe(true);
});

test("matches under a self-hosted path prefix", () => {
expect(
isIdempotentOperation(
"POST",
"https://sentry.example.com/sentry/api/0/organizations/acme/chunk-upload/",
),
).toBe(true);
});

test("ignores the query string", () => {
expect(
isIdempotentOperation(
"POST",
`${REGION_URL}/api/0/organizations/acme/chunk-upload/?foo=bar`,
),
).toBe(true);
});

test("does not match other methods on a marked path", () => {
expect(
isIdempotentOperation(
"PUT",
`${REGION_URL}/api/0/organizations/acme/chunk-upload/`,
),
).toBe(false);
});

test("does not match writes Sentry leaves unmarked", () => {
expect(
isIdempotentOperation(
"POST",
`${REGION_URL}/api/0/projects/acme/web/files/preprodartifacts/assemble/`,
),
).toBe(false);
expect(
isIdempotentOperation(
"POST",
`${REGION_URL}/api/0/organizations/acme/issues/1/autofix/`,
),
).toBe(false);
});

test("does not let a template segment span several path segments", () => {
expect(
isIdempotentOperation(
"POST",
`${REGION_URL}/api/0/organizations/acme/extra/chunk-upload/`,
),
).toBe(false);
});

test("returns false for URLs it cannot parse or that are not API paths", () => {
expect(isIdempotentOperation("POST", "not a url")).toBe(false);
expect(
isIdempotentOperation("POST", `${REGION_URL}/organizations/acme/`),
).toBe(false);
});
});
Loading
Loading