diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index 9df7285..9c11410 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -50,6 +50,7 @@ ## Fixed +- **[issue-162] Provider refresh page cleanup** — Provider comparison refreshes now close their temporary browser page even when Ancestry navigation or scraping fails, preventing failed retries from accumulating pages in the shared browser. - Nominatim geocoding requests now time out after 15 seconds and are cancelled when their map-stream client disconnects, preventing stalled upstream sockets from blocking the shared geocoding queue. - AI discovery now rejects unsafe batch settings, limits background runs to one per family database, and lets an active run be cancelled without leaving provider work behind. - **[issue-158] Unknown API routes now return JSON errors** — Requests to unrecognized `/api` paths receive a stable 404 error envelope instead of the browser app's HTML, while client-side navigation continues to use the SPA fallback. diff --git a/server/src/services/multi-platform-comparison.service.ts b/server/src/services/multi-platform-comparison.service.ts index 23af51d..3fdcf46 100644 --- a/server/src/services/multi-platform-comparison.service.ts +++ b/server/src/services/multi-platform-comparison.service.ts @@ -733,19 +733,23 @@ export const multiPlatformComparisonService = { const personUrl = platformRef.url || `https://www.${provider}.com`; logger.browser('compare', `Scraping ${provider} from URL: ${personUrl}`); const page = await browserService.createPage(); + let scrapedData: ScrapedPersonData | null = null; - if (provider === 'ancestry' && platformRef.url) { - await page.goto(platformRef.url, { waitUntil: 'domcontentloaded' }); - await page.waitForTimeout(2000); - } - - const scrapedData = await scraper.scrapePersonById(page, externalId).catch(err => { - logger.error('compare', `Failed to scrape ${provider}/${externalId}: ${err.message}`); - return null; - }); + try { + if (provider === 'ancestry' && platformRef.url) { + await page.goto(platformRef.url, { waitUntil: 'domcontentloaded' }); + await page.waitForTimeout(2000); + } - // Close the page to free resources - await page.close().catch(() => {}); + scrapedData = await scraper.scrapePersonById(page, externalId).catch(err => { + logger.error('compare', `Failed to scrape ${provider}/${externalId}: ${err.message}`); + return null; + }); + } finally { + await page.close().catch(err => { + logger.warn('compare', `Failed to close scrape page for ${provider}/${externalId}: ${err.message}`); + }); + } if (!scrapedData) { return null; diff --git a/tests/unit/services/multiPlatformComparison.spec.ts b/tests/unit/services/multiPlatformComparison.spec.ts new file mode 100644 index 0000000..3c48198 --- /dev/null +++ b/tests/unit/services/multiPlatformComparison.spec.ts @@ -0,0 +1,185 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mocks = vi.hoisted(() => ({ + page: { + goto: vi.fn(), + waitForTimeout: vi.fn(), + close: vi.fn(), + }, + scraper: { + scrapePersonById: vi.fn(), + }, + browserService: { + isConnected: vi.fn(), + createPage: vi.fn(), + }, + augmentationService: { + getAugmentation: vi.fn(), + saveAugmentation: vi.fn(), + addPlatform: vi.fn(), + }, + idMappingService: { + resolveId: vi.fn(), + getExternalId: vi.fn(), + registerExternalId: vi.fn(), + }, + databaseService: { + getPerson: vi.fn(), + }, + logger: { + browser: vi.fn(), + data: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + }, + fs: { + existsSync: vi.fn(), + readFileSync: vi.fn(), + statSync: vi.fn(), + writeFileSync: vi.fn(), + unlinkSync: vi.fn(), + }, + downloadImage: vi.fn(), + ensureDir: vi.fn(), +})); + +vi.mock('fs', () => ({ default: mocks.fs })); + +vi.mock('../../../server/src/services/browser.service.js', () => ({ + browserService: mocks.browserService, +})); + +vi.mock('../../../server/src/services/scrapers/index.js', () => ({ + getScraper: vi.fn(() => mocks.scraper), +})); + +vi.mock('../../../server/src/services/augmentation.service.js', () => ({ + augmentationService: mocks.augmentationService, +})); + +vi.mock('../../../server/src/services/id-mapping.service.js', () => ({ + idMappingService: mocks.idMappingService, +})); + +vi.mock('../../../server/src/services/database.service.js', () => ({ + databaseService: mocks.databaseService, +})); + +vi.mock('../../../server/src/db/sqlite.service.js', () => ({ + sqliteService: {}, +})); + +vi.mock('../../../server/src/services/familysearch-refresh.service.js', () => ({ + familySearchRefreshService: {}, +})); + +vi.mock('../../../server/src/lib/familysearch/index.js', () => ({ + json2person: vi.fn(), +})); + +vi.mock('../../../server/src/lib/logger.js', () => ({ + logger: mocks.logger, +})); + +vi.mock('../../../server/src/services/local-override.service.js', () => ({ + localOverrideService: {}, +})); + +vi.mock('../../../server/src/utils/applyOverrides.js', () => ({ + applyLocalOverrides: vi.fn(), +})); + +vi.mock('../../../server/src/utils/paths.js', () => ({ + PHOTOS_DIR: '/tmp/sparsetree-test-photos', + PROVIDER_CACHE_DIR: '/tmp/sparsetree-test-provider-cache', + ensureDir: mocks.ensureDir, +})); + +vi.mock('../../../server/src/utils/downloadImage.js', () => ({ + downloadImage: mocks.downloadImage, +})); + +vi.mock('../../../server/src/utils/providerCache.js', () => ({ + getPhotoSuffix: vi.fn(() => '-ancestry'), + getCachedProviderData: vi.fn(() => null), +})); + +vi.mock('../../../server/src/utils/normalizePhotoUrl.js', () => ({ + normalizePhotoUrl: vi.fn((url: string) => url), +})); + +const { multiPlatformComparisonService } = await import( + '../../../server/src/services/multi-platform-comparison.service.js' +); + +const getProviderData = () => + multiPlatformComparisonService.getProviderData('person-1', 'ancestry', true, 'db-1'); + +function expectNoPartialMutation(): void { + expect(mocks.fs.writeFileSync).not.toHaveBeenCalled(); + expect(mocks.fs.unlinkSync).not.toHaveBeenCalled(); + expect(mocks.downloadImage).not.toHaveBeenCalled(); + expect(mocks.augmentationService.saveAugmentation).not.toHaveBeenCalled(); + expect(mocks.augmentationService.addPlatform).not.toHaveBeenCalled(); + expect(mocks.idMappingService.registerExternalId).not.toHaveBeenCalled(); +} + +describe('multiPlatformComparisonService.getProviderData', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.browserService.isConnected.mockReturnValue(true); + mocks.browserService.createPage.mockResolvedValue(mocks.page); + mocks.augmentationService.getAugmentation.mockReturnValue({ + personId: 'person-1', + platforms: [ + { + platform: 'ancestry', + externalId: 'ancestry-1', + url: 'https://www.ancestry.com/family-tree/person/tree/123/person/456/facts', + }, + ], + photos: [], + }); + mocks.idMappingService.resolveId.mockReturnValue(null); + mocks.page.goto.mockResolvedValue(undefined); + mocks.page.waitForTimeout.mockResolvedValue(undefined); + mocks.page.close.mockResolvedValue(undefined); + mocks.scraper.scrapePersonById.mockResolvedValue({ + externalId: 'ancestry-1', + provider: 'ancestry', + name: 'Example Person', + scrapedAt: '2026-08-30T00:00:00.000Z', + }); + }); + + it('closes the page and preserves the navigation error when Ancestry setup fails', async () => { + const navigationError = new Error('navigation unavailable'); + mocks.page.goto.mockRejectedValue(navigationError); + mocks.page.close.mockRejectedValue(new Error('browser disconnected')); + + await expect(getProviderData()).rejects.toBe(navigationError); + + expect(mocks.scraper.scrapePersonById).not.toHaveBeenCalled(); + expect(mocks.page.close).toHaveBeenCalledTimes(1); + expect(mocks.logger.warn).toHaveBeenCalledWith( + 'compare', + 'Failed to close scrape page for ancestry/ancestry-1: browser disconnected' + ); + expectNoPartialMutation(); + }); + + it('closes the page and returns null when the scraper rejects', async () => { + mocks.scraper.scrapePersonById.mockRejectedValue(new Error('scrape failed')); + + await expect(getProviderData()).resolves.toBeNull(); + + expect(mocks.page.goto).toHaveBeenCalledTimes(1); + expect(mocks.page.waitForTimeout).toHaveBeenCalledTimes(1); + expect(mocks.page.close).toHaveBeenCalledTimes(1); + expect(mocks.logger.error).toHaveBeenCalledWith( + 'compare', + 'Failed to scrape ancestry/ancestry-1: scrape failed' + ); + expectNoPartialMutation(); + }); +});