From f03f3899872c4f469c6fab05b867cd968ac6a86e Mon Sep 17 00:00:00 2001 From: Prasenjit Sarkar Date: Wed, 22 Jul 2026 23:11:01 +0100 Subject: [PATCH] Fix /api/map timing out to a 500 with an empty list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `www.sonarsource.com` (and other sites) returned `500 "Discovery timeout after 30000ms"` with zero URLs, while `docs.sonarsource.com` returned thousands. Two bugs combined: 1. map.controller wrapped discovery in `Promise.race([discovery, reject- after-timeout])`. The instant the deadline passed, the race REJECTED and every URL already discovered was thrown away -> 500 empty. A slow but successful crawl looked identical to a hard failure. 2. In discoverUrls, the internal AbortController was dead code (its signal was never passed to any fetch), and the slow phases — subdomain sitemap expansion and browser crawl — were unbounded `await`s. For a seed like www. with includeSubdomains, subdomain expansion pulls each sibling's sitemap, and a big `docs.` sitemap alone blows the whole budget. Fix — treat timeoutMs as a SOFT deadline and always return what was found: - New pure `discovery-deadline.ts`: `withDeadline(p, ms, fallback)` resolves to `fallback` instead of rejecting; `makeDeadline(totalMs, now?)` tracks the remaining budget (injectable clock). Unit-tested. - discoverUrls: Phase 1 methods now collect URLs as each settles (a hung sitemap can't erase the others), and are awaited only up to the deadline. Subdomain expansion and browser crawl run only while budget remains and are each bounded by `withDeadline`. The method always resolves with the accumulated, filtered URLs and sets `partial: true` when truncated. - Controller drops the reject-race and surfaces `partial` in the response. - Partial (timed-out) results are not cached, so a truncated list can't poison the cache for the full TTL. - Default timeoutMs 30s -> 60s: the deadline is only a ceiling (fast sites still return in 1-3s), and this lets legitimately large sites finish before going partial. Verified in Docker against the live sites: www.sonarsource.com now returns 200 with 100 URLs (was 500-empty); docs.sonarsource.com still returns its big list; fuel-finder.uk unaffected; a forced 2.5s timeout returns 200 + partial:true (not 500) and is not cached. tsc, eslint, 186 tests (incl. 6 new), and openapi:check all pass. Co-Authored-By: Claude Opus 4.8 --- src/api/controllers/map.controller.ts | 21 ++--- src/api/schemas/index.ts | 2 +- src/services/discovery-deadline.spec.ts | 47 ++++++++++ src/services/discovery-deadline.ts | 54 +++++++++++ src/services/url-discovery.service.ts | 115 +++++++++++++++--------- src/types/discovery.ts | 4 + swagger.yaml | 2 +- 7 files changed, 189 insertions(+), 56 deletions(-) create mode 100644 src/services/discovery-deadline.spec.ts create mode 100644 src/services/discovery-deadline.ts diff --git a/src/api/controllers/map.controller.ts b/src/api/controllers/map.controller.ts index d7affd4..fbb89a7 100644 --- a/src/api/controllers/map.controller.ts +++ b/src/api/controllers/map.controller.ts @@ -73,17 +73,12 @@ export class MapController { crawlOptions }; - // Run URL discovery with timeout wrapper to prevent hanging - const discoveryTimeoutMs = timeoutMs ?? 60000; // Use user-provided timeout or default to 60 seconds - - const timeoutPromise = new Promise((_, reject) => - setTimeout(() => reject(new Error(`Discovery timeout after ${discoveryTimeoutMs}ms`)), discoveryTimeoutMs) - ); - - const discoveryResult = await Promise.race([ - this.discoveryService.discoverUrls(discoveryOptions), - timeoutPromise - ]); + // The discovery service now enforces `timeoutMs` internally as a SOFT + // deadline and always resolves with whatever it found (see + // url-discovery.service.ts). No outer reject-race here — that used to throw + // away every URL already discovered the moment the deadline passed, turning + // a slow-but-successful crawl into a 500 with an empty list. + const discoveryResult = await this.discoveryService.discoverUrls(discoveryOptions); // Prepare response const response: MapResponse = { @@ -93,7 +88,9 @@ export class MapController { url, includeSubdomains, maxUrls, - timestamp: new Date().toISOString() + timestamp: new Date().toISOString(), + // Signal a deadline-truncated result so callers can widen timeoutMs. + partial: discoveryResult.partial ?? false } }; diff --git a/src/api/schemas/index.ts b/src/api/schemas/index.ts index 5adf7b2..fd67433 100644 --- a/src/api/schemas/index.ts +++ b/src/api/schemas/index.ts @@ -175,7 +175,7 @@ export const mapRequestSchema = z.object({ skipSitemaps: z.boolean().optional().default(false), sitemapsOnly: z.boolean().optional().default(false), useUrlIndex: z.boolean().optional().default(true), - timeoutMs: z.number().int().min(1000).max(300000).optional().default(30000), + timeoutMs: z.number().int().min(1000).max(300000).optional().default(60000), includePatterns: z.array(z.string()).optional(), excludePatterns: z.array(z.string()).optional(), }); diff --git a/src/services/discovery-deadline.spec.ts b/src/services/discovery-deadline.spec.ts new file mode 100644 index 0000000..94fa611 --- /dev/null +++ b/src/services/discovery-deadline.spec.ts @@ -0,0 +1,47 @@ +import { withDeadline, makeDeadline } from './discovery-deadline'; + +const slow = (v: T, ms: number) => new Promise((r) => setTimeout(() => r(v), ms)); +const slowReject = (ms: number) => new Promise((_r, rej) => setTimeout(() => rej(new Error('boom')), ms)); + +describe('withDeadline', () => { + it('returns the promise value when it settles in time', async () => { + await expect(withDeadline(slow('ok', 5), 200, 'fallback')).resolves.toBe('ok'); + }); + + it('returns the fallback when the promise is too slow', async () => { + await expect(withDeadline(slow('ok', 200), 20, 'fallback')).resolves.toBe('fallback'); + }); + + it('returns the fallback (never rejects) when the promise rejects', async () => { + await expect(withDeadline(slowReject(5), 200, 'fallback')).resolves.toBe('fallback'); + }); + + it('returns the fallback immediately when ms <= 0', async () => { + await expect(withDeadline(slow('ok', 5), 0, 'fallback')).resolves.toBe('fallback'); + await expect(withDeadline(slow('ok', 5), -100, 'fallback')).resolves.toBe('fallback'); + }); + + it('does not lose the value if it resolves at nearly the same time (first settle wins)', async () => { + // Whatever wins the race, the result is one of the two valid outcomes and it never throws. + const r = await withDeadline(slow('ok', 20), 20, 'fallback'); + expect(['ok', 'fallback']).toContain(r); + }); +}); + +describe('makeDeadline', () => { + it('reports remaining budget and expiry against an injected clock', () => { + let t = 1000; + const d = makeDeadline(500, () => t); // ends at 1500 + expect(d.remaining()).toBe(500); + expect(d.expired()).toBe(false); + t = 1300; + expect(d.remaining()).toBe(200); + expect(d.expired()).toBe(false); + t = 1500; + expect(d.remaining()).toBe(0); + expect(d.expired()).toBe(true); + t = 1800; + expect(d.remaining()).toBe(0); // clamped, never negative + expect(d.expired()).toBe(true); + }); +}); diff --git a/src/services/discovery-deadline.ts b/src/services/discovery-deadline.ts new file mode 100644 index 0000000..f4b47a8 --- /dev/null +++ b/src/services/discovery-deadline.ts @@ -0,0 +1,54 @@ +/** + * Small deadline helpers for URL discovery. + * + * The map endpoint runs several discovery methods (sitemap, robots, common + * paths, subdomain sitemaps, browser crawl). Some are slow and unbounded — a + * site with a huge `docs.` sitemap can take far longer than the request's + * budget. These helpers let discovery treat `timeoutMs` as a SOFT deadline: + * return whatever has been found so far instead of throwing everything away. + * + * Kept as a pure, dependency-free module so the behaviour is unit-tested without + * standing up the browser pool / sitemap parser. + */ + +/** + * Resolve to `fallback` if `p` does not settle within `ms`. Never rejects — a + * rejection from `p` also resolves to `fallback`. The underlying work is not + * cancelled (there is no cross-cutting abort signal wired through discovery yet), + * it is just no longer awaited, so a slow method can't block the response. + */ +export function withDeadline(p: Promise, ms: number, fallback: T): Promise { + if (ms <= 0) return Promise.resolve(fallback); + return new Promise((resolve) => { + let settled = false; + const finish = (v: T) => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(v); + }; + const timer = setTimeout(() => finish(fallback), ms); + // Don't let the timer keep the event loop alive on its own. + if (typeof timer.unref === 'function') timer.unref(); + p.then((v) => finish(v), () => finish(fallback)); + }); +} + +export interface Deadline { + /** Milliseconds left before the deadline (never negative). */ + remaining(): number; + /** True once the deadline has passed. */ + expired(): boolean; +} + +/** + * A monotonic-ish budget of `totalMs` from "now". `now` is injectable so the + * arithmetic is testable without real time. + */ +export function makeDeadline(totalMs: number, now: () => number = Date.now): Deadline { + const end = now() + totalMs; + return { + remaining: () => Math.max(0, end - now()), + expired: () => now() >= end, + }; +} diff --git a/src/services/url-discovery.service.ts b/src/services/url-discovery.service.ts index e55e382..630cfc1 100644 --- a/src/services/url-discovery.service.ts +++ b/src/services/url-discovery.service.ts @@ -2,6 +2,7 @@ import { SitemapParserService } from './sitemap-parser.service'; import { URLValidationUtils } from '../utils/url-validation.utils'; import { logger } from '../utils/logger'; import { assertPublicUrl, ssrfSafeRequestConfig } from '../utils/ssrf-guard'; +import { withDeadline, makeDeadline } from './discovery-deadline'; import { extractChildLinks } from '../scraper/crawl-links'; import { redisClient } from './redis.service'; import { PlaywrightService } from './playwright.service'; @@ -146,9 +147,11 @@ export class URLDiscoveryService { return cached; } - // Set up abort controller for timeout - const controller = new AbortController(); - const timeoutId = setTimeout(() => controller.abort(), timeoutMs); + // Treat timeoutMs as a SOFT deadline: each phase is bounded by the time left, + // and whatever has been discovered when the budget runs out is returned rather + // than thrown away. `timedOut` records that the result may be incomplete. + const deadline = makeDeadline(timeoutMs); + let timedOut = false; try { // Run discovery methods strategically: fast methods first, then supplement with crawling if needed @@ -177,25 +180,39 @@ export class URLDiscoveryService { sitemapPromises.push(this.runDocumentDiscovery(url, rateLimiting)); } - // Run fast discovery methods in parallel - const sitemapResults = await Promise.allSettled(sitemapPromises); - - // Process sitemap results - sitemapResults.forEach((result) => { - if (result.status === 'fulfilled') { - const methodResult = result.value; - allUrls.push(...methodResult.urls); - methodCounts[methodResult.method] += methodResult.urls.length; - } - }); + // Collect each method's URLs the moment it settles, then wait for the batch + // only up to the deadline. A method still running when the budget expires is + // dropped, but everything already found is kept — one hung sitemap can no + // longer erase the URLs the other methods returned. + const collect = (r: DiscoveryMethodResult) => { + allUrls.push(...r.urls); + methodCounts[r.method] += r.urls.length; + }; + const phase1 = Promise.allSettled( + sitemapPromises.map((p) => p.then(collect, () => { /* ignore per-method failure */ })) + ).then(() => true); + if ((await withDeadline(phase1, deadline.remaining(), false)) === false) { + timedOut = true; + } // Phase 1.5: Subdomain expansion — when includeSubdomains is set, actively - // discover sibling subdomains (docs., blog., …) and pull each sitemap. + // discover sibling subdomains (docs., blog., …) and pull each sitemap. This + // is the slow part (a big docs. site can have a huge sitemap), so it only + // runs while there's a meaningful budget left and is bounded by it. if (includeSubdomains && !skipSitemaps) { - const subResult = await this.runSubdomainDiscovery(url, maxUrls); - allUrls.push(...subResult.urls); - methodCounts.sitemap += subResult.urls.length; - logger.info(`Subdomain discovery added ${subResult.urls.length} URLs`, { url }); + if (deadline.remaining() > 2000) { + const subResult = await withDeadline( + this.runSubdomainDiscovery(url, maxUrls), + deadline.remaining(), + { urls: [], method: 'sitemap' as const, timeTaken: 0 } + ); + allUrls.push(...subResult.urls); + methodCounts.sitemap += subResult.urls.length; + logger.info(`Subdomain discovery added ${subResult.urls.length} URLs`, { url }); + } else { + logger.info('Skipping subdomain discovery: time budget exhausted', { url }); + } + if (deadline.expired()) timedOut = true; } // Phase 2: Browser crawling only if we haven't found enough URLs @@ -210,25 +227,36 @@ export class URLDiscoveryService { // would just add latency without finding meaningfully more. const sparseThreshold = Math.min(maxUrls, 30); if (!sitemapsOnly && currentUrlCount < sparseThreshold) { - logger.info('Running supplemental browser crawling', { - url, - currentUrls: currentUrlCount, - targetUrls: maxUrls - }); - - const crawlResult = await this.runBrowserDiscovery( - url, - Math.floor(maxUrls * 0.3), - rateLimiting, - { - ...crawlOptions, - maxCrawlDepth: Math.min(crawlOptions?.maxCrawlDepth || 2, 2), // Limit depth for speed - enableDeepCrawling: currentUrlCount < maxUrls * 0.5 // Only deep crawl if we really need more URLs - } - ); + // Browser crawling is the slowest method; skip it if the budget is spent. + if (deadline.remaining() > 3000) { + logger.info('Running supplemental browser crawling', { + url, + currentUrls: currentUrlCount, + targetUrls: maxUrls + }); - allUrls.push(...crawlResult.urls); - methodCounts.crawling += crawlResult.urls.length; + const crawlResult = await withDeadline( + this.runBrowserDiscovery( + url, + Math.floor(maxUrls * 0.3), + rateLimiting, + { + ...crawlOptions, + maxCrawlDepth: Math.min(crawlOptions?.maxCrawlDepth || 2, 2), // Limit depth for speed + enableDeepCrawling: currentUrlCount < maxUrls * 0.5 // Only deep crawl if we really need more URLs + } + ), + deadline.remaining(), + { urls: [], method: 'crawling' as const, timeTaken: 0 } + ); + + allUrls.push(...crawlResult.urls); + methodCounts.crawling += crawlResult.urls.length; + if (deadline.expired()) timedOut = true; + } else { + logger.info('Skipping browser crawling: time budget exhausted', { url, currentUrls: currentUrlCount }); + timedOut = true; + } } @@ -253,11 +281,16 @@ export class URLDiscoveryService { discoveryMethods: methodCounts, timeTaken, fromCache: false, - searchQuery + searchQuery, + // The soft deadline was hit: these results are usable but may be incomplete. + partial: timedOut || undefined }; - // Cache the result - await this.cacheResult(cacheKey, discoveryResult); + // Only cache complete results — a partial (timed-out) run shouldn't poison + // the cache with a truncated URL list for the full TTL. + if (!timedOut) { + await this.cacheResult(cacheKey, discoveryResult); + } logger.info('URL discovery completed', { url, @@ -275,8 +308,6 @@ export class URLDiscoveryService { timeTaken: Date.now() - startTime }); throw error; - } finally { - clearTimeout(timeoutId); } } diff --git a/src/types/discovery.ts b/src/types/discovery.ts index dd12150..c6a243a 100644 --- a/src/types/discovery.ts +++ b/src/types/discovery.ts @@ -48,6 +48,8 @@ export interface DiscoveryResult { timeTaken: number; fromCache: boolean; searchQuery?: string; + /** True when the soft deadline was hit and the URL list may be incomplete. */ + partial?: boolean; } export interface DiscoveryMethodResult { @@ -101,6 +103,8 @@ export interface MapResponse { includeSubdomains: boolean; maxUrls: number; timestamp: string; + /** True when discovery hit the soft deadline; widen timeoutMs for more. */ + partial?: boolean; }; error?: string; } diff --git a/swagger.yaml b/swagger.yaml index 9229c19..410bd3d 100644 --- a/swagger.yaml +++ b/swagger.yaml @@ -1428,7 +1428,7 @@ paths: type: integer minimum: 1000 maximum: 300000 - default: 30000 + default: 60000 includePatterns: type: array items: