Skip to content

Commit dfc2100

Browse files
authored
fix(dsn): prevent hang during DSN auto-detection in repos with test fixtures (#445)
Running `sentry issues` without explicit flags in repos containing test fixture DSNs (e.g., the CLI repo itself) caused the process to hang. PR #420 bumped scan depth from 2→3, causing `test/` directories to be scanned — finding ~86 fake DSNs that were all resolved via unbounded `Promise.all`, triggering 190+ concurrent API requests, rate limiting (429), and retry storms. ## Changes - **Skip test directories in scanner** — Add `test/`, `tests/`, `__mocks__/`, `fixtures/`, `__fixtures__/` to `ALWAYS_SKIP_DIRS`. Test directories contain fixture DSNs, not real Sentry configuration. - **Deduplicate DSNs before resolution** — Group by `(orgId, projectId)` or `publicKey`, resolving only one representative per unique pair. Reduces ~86 DSNs to ~10-20 unique API calls. - **Add concurrency limit** — `p-limit(5)` on DSN resolution API calls to prevent overwhelming the Sentry API. - **Add 15s timeout** — Race `Promise.all` against a deadline, returning partial results on timeout so the CLI never hangs indefinitely. - **In-flight dedup for `resolveOrgRegion`** — Concurrent calls for the same orgSlug share a single HTTP request via a module-level promise cache. - **Fix `getDirMtime` dynamic import** — Replace wasteful `await import("node:fs/promises")` (called 75+ times) with a static import. Relates to #420 and #414.
1 parent f451431 commit dfc2100

File tree

5 files changed

+268
-13
lines changed

5 files changed

+268
-13
lines changed

src/lib/dsn/code-scanner.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
*/
1414

1515
import type { Dirent } from "node:fs";
16-
import { readdir } from "node:fs/promises";
16+
import { readdir, stat } from "node:fs/promises";
1717
import path from "node:path";
1818
// biome-ignore lint/performance/noNamespaceImport: Sentry SDK recommends namespace import
1919
import * as Sentry from "@sentry/bun";
@@ -85,6 +85,12 @@ const ALWAYS_SKIP_DIRS = [
8585
".cursor",
8686
// Node.js
8787
"node_modules",
88+
// Test directories (contain fixture DSNs, not real configuration)
89+
"test",
90+
"tests",
91+
"__mocks__",
92+
"fixtures",
93+
"__fixtures__",
8894
// Python
8995
"__pycache__",
9096
".pytest_cache",
@@ -432,7 +438,6 @@ type CollectResult = {
432438
*/
433439
async function getDirMtime(dir: string): Promise<number> {
434440
try {
435-
const { stat } = await import("node:fs/promises");
436441
const stats = await stat(dir);
437442
return Math.floor(stats.mtimeMs);
438443
} catch {

src/lib/region.ts

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,57 @@ import { withAuthGuard } from "./errors.js";
1313
import { getSdkConfig } from "./sentry-client.js";
1414
import { getSentryBaseUrl, isSentrySaasUrl } from "./sentry-urls.js";
1515

16+
/**
17+
* Promise cache for org region resolution, keyed by orgSlug.
18+
*
19+
* When multiple DSNs share an orgId, concurrent calls to resolveOrgRegion
20+
* deduplicate into a single HTTP request. A resolved promise returns
21+
* instantly on `await`, so this also serves as a warm in-memory cache.
22+
*
23+
* Rejected promises (e.g., AuthError) are automatically evicted so that
24+
* retries after re-authentication can succeed without restarting the CLI.
25+
*/
26+
const regionCache = new Map<string, Promise<string>>();
27+
1628
/**
1729
* Resolve the region URL for an organization.
1830
*
31+
* Deduplicates concurrent calls for the same orgSlug — only one HTTP
32+
* request fires per unique org, even when many DSNs trigger resolution
33+
* in parallel.
34+
*
1935
* Resolution order:
20-
* 1. Check SQLite cache
21-
* 2. Fetch organization details via SDK to get region URL
22-
* 3. Fall back to default URL if resolution fails
36+
* 1. Return in-flight or previously resolved promise (instant)
37+
* 2. Check SQLite cache
38+
* 3. Fetch organization details via SDK to get region URL
39+
* 4. Fall back to default URL if resolution fails
2340
*
2441
* Uses the SDK directly (not api-client) to avoid circular dependency.
2542
*
2643
* @param orgSlug - The organization slug
2744
* @returns The region URL for the organization
2845
*/
29-
export async function resolveOrgRegion(orgSlug: string): Promise<string> {
30-
// 1. Check cache first
46+
export function resolveOrgRegion(orgSlug: string): Promise<string> {
47+
const existing = regionCache.get(orgSlug);
48+
if (existing) {
49+
return existing;
50+
}
51+
52+
const promise = resolveOrgRegionUncached(orgSlug);
53+
regionCache.set(orgSlug, promise);
54+
// Evict on rejection (AuthError) so retries after re-login work.
55+
// Non-auth errors already resolve to baseUrl fallback, so only
56+
// AuthError re-throws can leave a rejected promise in the cache.
57+
promise.catch(() => regionCache.delete(orgSlug));
58+
return promise;
59+
}
60+
61+
/**
62+
* Resolve org region from SQLite cache or API.
63+
* Called at most once per orgSlug per process lifetime.
64+
*/
65+
async function resolveOrgRegionUncached(orgSlug: string): Promise<string> {
66+
// 1. Check SQLite cache first
3167
const cached = await getOrgRegion(orgSlug);
3268
if (cached) {
3369
return cached;

src/lib/resolve-target.ts

Lines changed: 108 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
*/
1515

1616
import { basename } from "node:path";
17+
import pLimit from "p-limit";
1718
import {
1819
findProjectByDsnKey,
1920
findProjectsByPattern,
@@ -29,7 +30,7 @@ import {
2930
setCachedProject,
3031
setCachedProjectByDsnKey,
3132
} from "./db/project-cache.js";
32-
import type { DetectedDsn } from "./dsn/index.js";
33+
import type { DetectedDsn, DsnDetectionResult } from "./dsn/index.js";
3334
import {
3435
detectAllDsns,
3536
detectDsn,
@@ -578,11 +579,79 @@ export async function fetchProjectId(
578579
return toNumericId(projectResult.value.id);
579580
}
580581

582+
/**
583+
* Maximum concurrent DSN resolution API calls.
584+
* Prevents overwhelming the Sentry API with parallel requests when
585+
* many DSNs are detected (e.g., monorepos or repos with test fixtures).
586+
*/
587+
const DSN_RESOLVE_CONCURRENCY = 5;
588+
589+
/**
590+
* Maximum time (ms) to spend resolving DSNs before returning partial results.
591+
* Prevents indefinite hangs when the API is slow or rate-limiting.
592+
*/
593+
const DSN_RESOLVE_TIMEOUT_MS = 15_000;
594+
595+
/**
596+
* Resolve DSNs with a concurrency limit and overall timeout.
597+
*
598+
* Uses p-limit's `map` helper for concurrency control and races it
599+
* against `AbortSignal.timeout` so the CLI never hangs indefinitely.
600+
* Queued tasks check the abort signal before doing work (same pattern
601+
* as code-scanner's earlyExit flag from PR #414). In-flight tasks that
602+
* already started are abandoned on timeout — their individual HTTP
603+
* timeouts (30s in sentry-client.ts) bound them independently.
604+
*
605+
* Results are written to a shared array so that tasks completing before
606+
* the deadline are captured even when the overall operation times out.
607+
*
608+
* @param dsns - Deduplicated DSNs to resolve
609+
* @returns Array of resolved targets (null for failures/timeouts)
610+
*/
611+
async function resolveDsnsWithTimeout(
612+
dsns: DetectedDsn[]
613+
): Promise<(ResolvedTarget | null)[]> {
614+
const limit = pLimit(DSN_RESOLVE_CONCURRENCY);
615+
const signal = AbortSignal.timeout(DSN_RESOLVE_TIMEOUT_MS);
616+
617+
// Shared results array — tasks write their result as they complete,
618+
// so partial results survive timeout.
619+
const results: (ResolvedTarget | null)[] = new Array(dsns.length).fill(null);
620+
621+
const mapDone = limit.map(dsns, (dsn, i) => {
622+
if (signal.aborted) {
623+
return Promise.resolve(null);
624+
}
625+
return resolveDsnToTarget(dsn).then((target) => {
626+
results[i] = target;
627+
return target;
628+
});
629+
});
630+
631+
// Race limit.map against the abort signal so in-flight tasks
632+
// don't block the timeout.
633+
const aborted = new Promise<"timeout">((resolve) => {
634+
signal.addEventListener("abort", () => resolve("timeout"), { once: true });
635+
});
636+
const raceResult = await Promise.race([
637+
mapDone.then(() => "done" as const),
638+
aborted,
639+
]);
640+
641+
if (raceResult === "timeout") {
642+
log.warn(
643+
`DSN resolution timed out after ${DSN_RESOLVE_TIMEOUT_MS / 1000}s, returning partial results`
644+
);
645+
}
646+
647+
return results;
648+
}
649+
581650
/**
582651
* Resolve all targets for monorepo-aware commands.
583652
*
584653
* When multiple DSNs are detected, resolves all of them in parallel
585-
* and returns a footer message for display.
654+
* (with concurrency limiting) and returns a footer message for display.
586655
*
587656
* Resolution priority:
588657
* 1. Explicit org and project - returns single target
@@ -662,13 +731,46 @@ export async function resolveAllTargets(
662731
return inferFromDirectoryName(cwd);
663732
}
664733

665-
// Resolve all DSNs in parallel
666-
const resolvedTargets = await Promise.all(
667-
detection.all.map((dsn) => resolveDsnToTarget(dsn))
734+
return resolveDetectedDsns(detection);
735+
}
736+
737+
/**
738+
* Deduplicate detected DSNs and resolve them with concurrency limiting.
739+
*
740+
* Groups DSNs by (orgId, projectId) or publicKey, resolves one per unique
741+
* combination, then deduplicates resolved targets by org+project slug.
742+
*
743+
* @param detection - DSN detection result with all found DSNs
744+
* @returns Resolved targets with optional footer message
745+
*/
746+
async function resolveDetectedDsns(
747+
detection: DsnDetectionResult
748+
): Promise<ResolvedTargets> {
749+
// Deduplicate DSNs by (orgId, projectId) or publicKey before resolution.
750+
// Multiple DSNs in test fixtures or monorepos can share the same org+project
751+
// — resolving each unique pair once avoids redundant API calls.
752+
const uniqueDsnMap = new Map<string, DetectedDsn>();
753+
for (const dsn of detection.all) {
754+
const dedupeKey = dsn.orgId
755+
? `${dsn.orgId}:${dsn.projectId}`
756+
: `key:${dsn.publicKey}`;
757+
if (!uniqueDsnMap.has(dedupeKey)) {
758+
uniqueDsnMap.set(dedupeKey, dsn);
759+
}
760+
}
761+
const uniqueDsns = [...uniqueDsnMap.values()];
762+
763+
log.debug(
764+
`Resolving ${uniqueDsns.length} unique DSN targets (${detection.all.length} total detected)`
668765
);
669766

767+
// Resolve with concurrency limit to avoid overwhelming the Sentry API.
768+
// Without this, large repos can fire 100+ concurrent HTTP requests,
769+
// triggering rate limiting (429) and retry storms.
770+
const resolvedTargets = await resolveDsnsWithTimeout(uniqueDsns);
771+
670772
// Filter out failed resolutions and deduplicate by org+project
671-
// (multiple DSNs with different keys can point to same project)
773+
// (different orgId forms can resolve to the same org slug)
672774
const seen = new Set<string>();
673775
const targets = resolvedTargets.filter((t): t is ResolvedTarget => {
674776
if (t === null) {

test/lib/dsn/code-scanner.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,64 @@ describe("Code Scanner", () => {
299299
expect(result).toBeNull();
300300
});
301301

302+
test("skips test directories", async () => {
303+
// DSNs in test/ should be ignored (they contain test fixtures, not real config)
304+
mkdirSync(join(testDir, "test/lib"), { recursive: true });
305+
writeFileSync(
306+
join(testDir, "test/lib/scanner.test.ts"),
307+
'const DSN = "https://testkey@o123.ingest.sentry.io/456";'
308+
);
309+
310+
const result = await scanCodeForFirstDsn(testDir);
311+
expect(result).toBeNull();
312+
313+
// Also test tests/ directory
314+
mkdirSync(join(testDir, "tests/unit"), { recursive: true });
315+
writeFileSync(
316+
join(testDir, "tests/unit/app.test.ts"),
317+
'const DSN = "https://testkey@o999.ingest.sentry.io/789";'
318+
);
319+
320+
const allResult = await scanCodeForDsns(testDir);
321+
expect(allResult.dsns).toHaveLength(0);
322+
});
323+
324+
test("skips __mocks__ and fixtures directories", async () => {
325+
mkdirSync(join(testDir, "__mocks__"), { recursive: true });
326+
writeFileSync(
327+
join(testDir, "__mocks__/sentry.ts"),
328+
'export const DSN = "https://mock@o123.ingest.sentry.io/111";'
329+
);
330+
mkdirSync(join(testDir, "fixtures"), { recursive: true });
331+
writeFileSync(
332+
join(testDir, "fixtures/config.ts"),
333+
'export const DSN = "https://fixture@o123.ingest.sentry.io/222";'
334+
);
335+
336+
const result = await scanCodeForDsns(testDir);
337+
expect(result.dsns).toHaveLength(0);
338+
});
339+
340+
test("still finds DSNs in src/ when test/ is skipped", async () => {
341+
// Ensure test/ skipping doesn't affect real source directories
342+
mkdirSync(join(testDir, "src"), { recursive: true });
343+
writeFileSync(
344+
join(testDir, "src/config.ts"),
345+
'const DSN = "https://realkey@o123.ingest.sentry.io/456";'
346+
);
347+
mkdirSync(join(testDir, "test"), { recursive: true });
348+
writeFileSync(
349+
join(testDir, "test/fixture.ts"),
350+
'const DSN = "https://fakekey@o999.ingest.sentry.io/999";'
351+
);
352+
353+
const result = await scanCodeForDsns(testDir);
354+
expect(result.dsns).toHaveLength(1);
355+
expect(result.dsns[0].raw).toBe(
356+
"https://realkey@o123.ingest.sentry.io/456"
357+
);
358+
});
359+
302360
test("respects gitignore", async () => {
303361
writeFileSync(join(testDir, ".gitignore"), "ignored/");
304362
mkdirSync(join(testDir, "ignored"), { recursive: true });

test/lib/region.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,4 +258,58 @@ describe("resolveOrgRegion", () => {
258258
expect(await resolveOrgRegion("org-a")).toBe("https://us.sentry.io");
259259
expect(await resolveOrgRegion("org-b")).toBe("https://de.sentry.io");
260260
});
261+
262+
test("deduplicates concurrent API calls for the same org", async () => {
263+
let fetchCount = 0;
264+
265+
// Mock fetch to track call count
266+
const originalFetch = globalThis.fetch;
267+
globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => {
268+
const req = new Request(input, init);
269+
if (req.url.includes("/organizations/dedup-org/")) {
270+
fetchCount += 1;
271+
// Small delay to ensure concurrency overlap
272+
await Bun.sleep(50);
273+
return new Response(
274+
JSON.stringify({
275+
id: "789",
276+
slug: "dedup-org",
277+
name: "Dedup Organization",
278+
links: {
279+
organizationUrl: "https://us.sentry.io/organizations/dedup-org/",
280+
regionUrl: "https://us.sentry.io",
281+
},
282+
}),
283+
{
284+
status: 200,
285+
headers: { "Content-Type": "application/json" },
286+
}
287+
);
288+
}
289+
return new Response(JSON.stringify({ detail: "Not found" }), {
290+
status: 404,
291+
});
292+
};
293+
294+
try {
295+
// Fire 5 concurrent calls for the same org
296+
const results = await Promise.all([
297+
resolveOrgRegion("dedup-org"),
298+
resolveOrgRegion("dedup-org"),
299+
resolveOrgRegion("dedup-org"),
300+
resolveOrgRegion("dedup-org"),
301+
resolveOrgRegion("dedup-org"),
302+
]);
303+
304+
// All should return the same result
305+
for (const result of results) {
306+
expect(result).toBe("https://us.sentry.io");
307+
}
308+
309+
// Only one fetch should have been made (in-flight dedup)
310+
expect(fetchCount).toBe(1);
311+
} finally {
312+
globalThis.fetch = originalFetch;
313+
}
314+
});
261315
});

0 commit comments

Comments
 (0)