From 2442f695ce998e08c2ee546a5ee01d08e2dd6a85 Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 23 Jul 2026 14:37:12 -0400 Subject: [PATCH] PROD-2310: count content/page prep failures instead of silently skipping Two classes of content/page sync failures never reached the exit-code check PR #182 wired up, because they were counted as skips rather than failures: - content-batch-processor.ts: items that fail during payload prep (missing model/container mapping, deleted target container, unresolved content reference) only incremented skippedCount, which never feeds totalFailed/totalSyncFailures. - push-pages.ts: a channel-level exception from processSitemap set status="error" (display-only) without incrementing failed, so a whole channel's worth of page failures was invisible to the exit code. Both now count as real failures with failureDetails entries, so they fail the sync exit code like batch-level API failures already do. Co-authored-by: Claude Sonnet 5 --- .../content-pusher/content-batch-processor.ts | 29 +++++++++++++++---- .../tests/content-batch-processor.test.ts | 24 +++++++++++++-- src/lib/pushers/page-pusher/push-pages.ts | 11 +++++++ 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/lib/pushers/content-pusher/content-batch-processor.ts b/src/lib/pushers/content-pusher/content-batch-processor.ts index 7ec98d75..cbd4c48b 100644 --- a/src/lib/pushers/content-pusher/content-batch-processor.ts +++ b/src/lib/pushers/content-pusher/content-batch-processor.ts @@ -133,10 +133,16 @@ export class ContentBatchProcessor { payloads: contentPayloads, skippedCount: batchSkippedCount, includedItems, + failedItems: prepFailedItems, } = await this.prepareContentPayloads(contentBatch, this.config.sourceGuid, this.config.targetGuid); // Track skipped items from this batch totalSkippedCount += batchSkippedCount; + // PROD-2310: prep failures (missing mapping, deleted container, unresolved + // reference) are real failures — fold them into the same counters batch-level + // API failures use, so they reach content-pusher.ts's totalFailed. + totalFailureCount += prepFailedItems.length; + allFailedItems.push(...prepFailedItems); // Execute bulk upload using saveContentItems API with returnBatchID flag const batchIDResult = await this.config.apiClient.contentMethods.saveContentItems( @@ -296,10 +302,16 @@ export class ContentBatchProcessor { contentBatch: mgmtApi.ContentItem[], sourceGuid: string, targetGuid: string - ): Promise<{ payloads: any[]; skippedCount: number; includedItems: mgmtApi.ContentItem[] }> { + ): Promise<{ + payloads: any[]; + skippedCount: number; + includedItems: mgmtApi.ContentItem[]; + failedItems: BatchFailedItem[]; + }> { const payloads: any[] = []; const includedItems: mgmtApi.ContentItem[] = []; let skippedCount = 0; + const failedItems: BatchFailedItem[] = []; // No imports needed - using reference mapper directly const modelMapper = new ModelMapper(sourceGuid, targetGuid); @@ -511,19 +523,24 @@ export class ContentBatchProcessor { includedItems.push(contentItem); } catch (error: any) { console.error( - ansiColors.yellow( - `✗ Skipping content item ${contentItem.contentID} (${contentItem.properties?.referenceName ?? "unknown"}) - ${error.message || "payload preparation failed"}` + ansiColors.red( + `✗ Failed to prepare content item ${contentItem.contentID} (${contentItem.properties?.referenceName ?? "unknown"}) - ${error.message || "payload preparation failed"}` ) ); - // Track skipped item and continue with the rest of the batch - skippedCount++; + // PROD-2310: this item will never reach the target — it's a failure, not a + // benign skip. Counting it only as `skippedCount` meant it never affected + // totalFailed/the sync exit code, even though the item didn't sync. + failedItems.push({ + originalContent: contentItem, + error: error.message || "payload preparation failed", + }); continue; } } } - return { payloads, skippedCount, includedItems }; + return { payloads, skippedCount, includedItems, failedItems }; } /** diff --git a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts index bd84ed8a..cd04ed59 100644 --- a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts +++ b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts @@ -160,6 +160,7 @@ describe("ContentBatchProcessor.processBatches — batch-level API failure", () payloads: [{}, {}], skippedCount: 0, includedItems: [makeContentItem(1), makeContentItem(2)], + failedItems: [], }); const result = await processor.processBatches([makeContentItem(1), makeContentItem(2)], makeLogger(), "Test"); @@ -181,6 +182,7 @@ describe("ContentBatchProcessor.processBatches — batch-level API failure", () payloads: [{}], skippedCount: 0, includedItems: [makeContentItem(1)], + failedItems: [], }); await processor.processBatches([makeContentItem(1)], makeLogger(), "Test"); @@ -200,6 +202,7 @@ describe("ContentBatchProcessor.processBatches — successful batch", () => { payloads: [{}, {}], skippedCount: 0, includedItems: [item1, item2], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -226,6 +229,7 @@ describe("ContentBatchProcessor.processBatches — successful batch", () => { payloads: [{}, {}], skippedCount: 0, includedItems: [item1, item2], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -253,6 +257,7 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { payloads: [{}, {}], skippedCount: 0, includedItems: [publishedItem, stagingItem], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -280,6 +285,7 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { payloads: [{}], skippedCount: 0, includedItems: [stagingItem], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -302,6 +308,7 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { payloads: [{}], skippedCount: 0, includedItems: [stagingItem], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -322,6 +329,7 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { payloads: [{}, {}], skippedCount: 0, includedItems: stagingItems, + failedItems: [], }); mockExtract.mockReturnValue({ @@ -350,6 +358,7 @@ describe("ContentBatchProcessor.processBatches — onBatchComplete", () => { payloads: [{}], skippedCount: 0, includedItems: [item], + failedItems: [], }); mockExtract.mockReturnValue({ successfulItems: [], failedItems: [] }); @@ -368,6 +377,7 @@ describe("ContentBatchProcessor.processBatches — onBatchComplete", () => { payloads: [{}], skippedCount: 0, includedItems: [item], + failedItems: [], }); mockExtract.mockReturnValue({ successfulItems: [], failedItems: [] }); @@ -387,6 +397,7 @@ describe("ContentBatchProcessor.processBatches — batch splitting", () => { payloads: [], skippedCount: 0, includedItems: [], + failedItems: [], }); mockExtract.mockReturnValue({ successfulItems: [], failedItems: [] }); @@ -405,6 +416,7 @@ describe("ContentBatchProcessor.processBatches — batch splitting", () => { payloads: [], skippedCount: 1, // 1 skip per batch includedItems: [], + failedItems: [], }); mockExtract.mockReturnValue({ successfulItems: [], failedItems: [] }); @@ -431,6 +443,7 @@ describe("ContentBatchProcessor — referenceMapper.addMapping side effect", () payloads: [{}], skippedCount: 0, includedItems: [item], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -453,6 +466,7 @@ describe("ContentBatchProcessor — referenceMapper.addMapping side effect", () payloads: [{}], skippedCount: 0, includedItems: [item], + failedItems: [], }); mockExtract.mockReturnValue({ successfulItems: [], failedItems: [] }); @@ -475,6 +489,7 @@ describe("ContentBatchProcessor.processBatches — failed item logging", () => { payloads: [{}], skippedCount: 0, includedItems: [item], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -496,6 +511,7 @@ describe("ContentBatchProcessor.processBatches — failed item logging", () => { payloads: [{}], skippedCount: 0, includedItems: [item], + failedItems: [], }); mockExtract.mockReturnValue({ @@ -575,7 +591,7 @@ describe("ContentBatchProcessor.prepareContentPayloads — stale container mappi // simulates the container having been deleted on the target since the last sync. } - it("skips the item and does not ship a payload when the target container is gone", async () => { + it("fails the item (not a skip) and does not ship a payload when the target container is gone", async () => { const sourceGuid = "pcp-stale-src"; const targetGuid = "pcp-stale-tgt"; seedModelAndContainerMapping(tmpDir, sourceGuid, targetGuid, { createTargetContainerFile: false }); @@ -591,7 +607,11 @@ describe("ContentBatchProcessor.prepareContentPayloads — stale container mappi expect(result.payloads).toHaveLength(0); expect(result.includedItems).toHaveLength(0); - expect(result.skippedCount).toBe(1); + // PROD-2310: this item never reaches the target, so it must count as a failure — + // not a skip — so it fails the sync exit code. + expect(result.skippedCount).toBe(0); + expect(result.failedItems).toHaveLength(1); + expect(result.failedItems[0].error).toEqual(expect.stringContaining("no longer exists on the target")); expect(consoleErrorSpy).toHaveBeenCalledWith( expect.stringContaining("no longer exists on the target") ); diff --git a/src/lib/pushers/page-pusher/push-pages.ts b/src/lib/pushers/page-pusher/push-pages.ts index 1a745091..c5e5510a 100644 --- a/src/lib/pushers/page-pusher/push-pages.ts +++ b/src/lib/pushers/page-pusher/push-pages.ts @@ -97,6 +97,17 @@ export async function pushPages(sourceData: mgmtApi.PageItem[], locale: string): targetGuid[0] ); status = "error"; + // PROD-2310: a channel-level throw means every page under it failed to sync. + // `status` alone is display-only (never read by the exit-code path) — record a + // failure so this isn't silently invisible to totalSyncFailures/the exit code. + failed++; + failureDetails.push({ + name: `Channel ${channel}`, + error: errorMessage, + type: "page", + guid: sourceGuid[0], + locale, + }); } }