Merge pull request #28571 from backstage/rugvip/conflict

catalog-backend: ignore stitching db conflicts

.changeset/eleven-mice-sleep.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-backend': patch
+---
+
+Ignore benign database conflict errors during stitching, now logged with debug level instead.

plugins/catalog-backend/src/database/operations/stitcher/performStitching.test.ts

@@ -294,4 +294,73 @@ describe('performStitching', () => {
       );
     },
   );
+
+  it.each(databases.eachSupportedId())(
+    'handles conflicts with past stitches %p',
+    async databaseId => {
+      if (databaseId === 'MYSQL_8') {
+        // MySQL doesn't handle conflicts in the same way as the other two, most
+        // likely due to the conflict probably being handled with a merged even
+        // if it's for the entity_ref. This probably means that MySQL will have
+        // inconsistencies in the final_entities table in some cases, but they
+        // should heal fairly quickly.
+        return;
+      }
+      const knex = await databases.init(databaseId);
+      await applyDatabaseMigrations(knex);
+
+      // Drop the foreign key constraint so we can insert a conflicting entity
+      // where the ID is inconsistent with the entity ref across refresh_state
+      // and final_entities
+      await knex.schema.alterTable('final_entities', table => {
+        table.dropForeign('entity_id');
+      });
+
+      await knex<DbRefreshStateRow>('refresh_state').insert([
+        {
+          entity_id: 'other-id',
+          entity_ref: 'k:ns/n',
+          unprocessed_entity: JSON.stringify({}),
+          processed_entity: JSON.stringify({
+            apiVersion: 'a',
+            kind: 'k',
+            metadata: {
+              name: 'n',
+              namespace: 'ns',
+            },
+            spec: {
+              k: 'v',
+            },
+          }),
+          errors: '[]',
+          next_update_at: knex.fn.now(),
+          last_discovery_at: knex.fn.now(),
+        },
+      ]);
+      await knex<DbFinalEntitiesRow>('final_entities').insert([
+        {
+          entity_id: 'my-id',
+          entity_ref: 'k:ns/n',
+          hash: '',
+          stitch_ticket: 'old-ticket',
+          final_entity: JSON.stringify({}),
+        },
+      ]);
+
+      const stitchLogger = mockServices.logger.mock();
+      await expect(
+        performStitching({
+          knex,
+          logger: stitchLogger,
+          strategy: { mode: 'immediate' },
+          entityRef: 'k:ns/n',
+        }),
+      ).resolves.toBe('abandoned');
+
+      expect(stitchLogger.debug).toHaveBeenCalledWith(
+        'Skipping stitching of k:ns/n, conflict',
+        expect.anything(),
+      );
+    },
+  );
 });

plugins/catalog-backend/src/database/operations/stitcher/performStitching.ts

@@ -33,7 +33,10 @@ import {
 import { buildEntitySearch } from './buildEntitySearch';
 import { markDeferredStitchCompleted } from './markDeferredStitchCompleted';
 import { BATCH_SIZE, generateStableHash } from './util';
-import { LoggerService } from '@backstage/backend-plugin-api';
+import {
+  LoggerService,
+  isDatabaseConflictError,
+} from '@backstage/backend-plugin-api';
 
 // See https://github.com/facebook/react/blob/f0cf832e1d0c8544c36aa8b310960885a11a847c/packages/react-dom-bindings/src/shared/sanitizeURL.js
 const scriptProtocolPattern =
@@ -71,15 +74,29 @@ export async function performStitching(options: {
     }
 
     // Insert stitching ticket that will be compared before inserting the final entity.
-    await knex<DbFinalEntitiesRow>('final_entities')
-      .insert({
-        entity_id: entityResult[0].entity_id,
-        hash: '',
-        entity_ref: entityRef,
-        stitch_ticket: stitchTicket,
-      })
-      .onConflict('entity_id')
-      .merge(['stitch_ticket']);
+    try {
+      await knex<DbFinalEntitiesRow>('final_entities')
+        .insert({
+          entity_id: entityResult[0].entity_id,
+          hash: '',
+          entity_ref: entityRef,
+          stitch_ticket: stitchTicket,
+        })
+        .onConflict('entity_id')
+        .merge(['stitch_ticket']);
+    } catch (error) {
+      // It's possible to hit a race where a refresh_state table delete + insert
+      // is done just after we read the entity_id from it. This conflict is safe
+      // to ignore because the current stitching operation will be triggered by
+      // the old entry, and the new entry will trigger it's own stitching that
+      // will update the entity.
+      if (isDatabaseConflictError(error)) {
+        logger.debug(`Skipping stitching of ${entityRef}, conflict`, error);
+        return 'abandoned';
+      }
+
+      throw error;
+    }
 
     // Selecting from refresh_state and final_entities should yield exactly
     // one row (except in abnormal cases where the stitch was invoked for