From 0f86ebfc9daadeeb8c1d2adb31bd051f8fc4fc5f Mon Sep 17 00:00:00 2001 From: Quentin Ligier Date: Fri, 14 Aug 2026 11:31:36 +0200 Subject: [PATCH] Isolate db writes to MB_INSTALLED_STRUCT_DEF during migration --- docs/changelog.md | 1 + .../src/app/igs/igs.component.ts | 4 +- .../data/INpmPackageVersionResourceDao.java | 2 +- .../MbInstalledStructureDefinitionEntity.java | 2 +- .../matchbox/config/MatchboxJpaConfig.java | 7 +- .../packages/MatchboxJpaPackageCache.java | 45 +++-- ...InstalledStructureDefinitionMigration.java | 162 +++++++++++++----- 7 files changed, 163 insertions(+), 60 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 881ad58fa4..f67acbbe70 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -4,6 +4,7 @@ - Align `spring-core` and `spring-websocket`, which HAPI FHIR pulls in as 6.2.18, with the Spring Framework version already used elsewhere (6.2.19), fixing CVE-2026-41838 in `spring-websocket` and CVE-2026-41848 in `spring-core` - Force `spring-retry` to 2.0.13, which HAPI FHIR pulls in as 2.0.10, to fix CVE-2026-41710 - Fix the GUI not displaying markdown content (#555) +- Isolate each database write during the migration of `MB_INSTALLED_STRUCT_DEF` 2026/08/11 Release 4.1.12 diff --git a/matchbox-frontend/src/app/igs/igs.component.ts b/matchbox-frontend/src/app/igs/igs.component.ts index 2a6d6d7bff..a7762f2528 100644 --- a/matchbox-frontend/src/app/igs/igs.component.ts +++ b/matchbox-frontend/src/app/igs/igs.component.ts @@ -38,13 +38,13 @@ export class IgsComponent { }; constructor( - private data: FhirConfigService, + data: FhirConfigService, private fhirPathService: FhirPathService ) { this.client = data.getFhirClient(); this.addPackageId = new UntypedFormControl('', [Validators.required, Validators.minLength(1)]); this.addVersion = new UntypedFormControl('current', [Validators.required, Validators.minLength(1)]); - this.addUrl = new UntypedFormControl('url'); + this.addUrl = new UntypedFormControl(''); this.search(); } diff --git a/matchbox-server/src/main/java/ca/uhn/fhir/jpa/dao/data/INpmPackageVersionResourceDao.java b/matchbox-server/src/main/java/ca/uhn/fhir/jpa/dao/data/INpmPackageVersionResourceDao.java index 9247afff7a..02339759b1 100644 --- a/matchbox-server/src/main/java/ca/uhn/fhir/jpa/dao/data/INpmPackageVersionResourceDao.java +++ b/matchbox-server/src/main/java/ca/uhn/fhir/jpa/dao/data/INpmPackageVersionResourceDao.java @@ -79,7 +79,7 @@ Slice findByResourceType(Pageable thePage, @Param("resourceType") String theResourceType); // MATCHBOX: added for our needs - @Query("SELECT e FROM NpmPackageVersionResourceEntity e WHERE e.myResourceType = :resourceType ORDER BY e.myCanonicalUrl ASC") + @Query("SELECT e FROM NpmPackageVersionResourceEntity e WHERE e.myResourceType = :resourceType ORDER BY e.myResourcePid ASC") Slice findByResourceTypeOrdered(Pageable thePage, @Param("resourceType") String theResourceType); diff --git a/matchbox-server/src/main/java/ca/uhn/fhir/jpa/model/entity/MbInstalledStructureDefinitionEntity.java b/matchbox-server/src/main/java/ca/uhn/fhir/jpa/model/entity/MbInstalledStructureDefinitionEntity.java index a82129dd35..6dda85f666 100644 --- a/matchbox-server/src/main/java/ca/uhn/fhir/jpa/model/entity/MbInstalledStructureDefinitionEntity.java +++ b/matchbox-server/src/main/java/ca/uhn/fhir/jpa/model/entity/MbInstalledStructureDefinitionEntity.java @@ -30,7 +30,7 @@ public class MbInstalledStructureDefinitionEntity implements Serializable { * A primary key for the table. */ @Id - @SequenceGenerator(name = "SEQ_MB_INSTSTRUCTDEF", sequenceName = "SEQ_MB_INSTSTRUCTDEF") + @SequenceGenerator(name = "SEQ_MB_INSTSTRUCTDEF", sequenceName = "SEQ_MB_INSTSTRUCTDEF", allocationSize = 1) @GeneratedValue(strategy = GenerationType.AUTO, generator = "SEQ_MB_INSTSTRUCTDEF") @Column(name = "PID") private Long id; diff --git a/matchbox-server/src/main/java/ch/ahdis/matchbox/config/MatchboxJpaConfig.java b/matchbox-server/src/main/java/ch/ahdis/matchbox/config/MatchboxJpaConfig.java index 82ef41048a..68d0a39c98 100644 --- a/matchbox-server/src/main/java/ch/ahdis/matchbox/config/MatchboxJpaConfig.java +++ b/matchbox-server/src/main/java/ch/ahdis/matchbox/config/MatchboxJpaConfig.java @@ -80,6 +80,7 @@ import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; import org.springframework.data.domain.Page; +import org.springframework.transaction.PlatformTransactionManager; import javax.annotation.Nullable; @@ -629,12 +630,14 @@ public MbInstalledStructureDefinitionMigration mbInstalledStructureDefinitionMig final MatchboxJpaPackageCache matchboxJpaPackageCache, final INpmPackageVersionResourceDao myPackageVersionResourceDao, final IBinaryStorageSvc myBinaryStorageSvc, - final DaoRegistry myDaoRegistry) { + final DaoRegistry myDaoRegistry, + final PlatformTransactionManager txManager) { return new MbInstalledStructureDefinitionMigration(installedStructureDefinitionRepository, matchboxJpaPackageCache, myPackageVersionResourceDao, myBinaryStorageSvc, - myDaoRegistry); + myDaoRegistry, + txManager); } private static void registerOptionalProvider(final MatchboxRestfulServer fhirServer, diff --git a/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MatchboxJpaPackageCache.java b/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MatchboxJpaPackageCache.java index aa8e903fdf..ac80ba1f48 100644 --- a/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MatchboxJpaPackageCache.java +++ b/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MatchboxJpaPackageCache.java @@ -58,15 +58,34 @@ public void interceptEntityBeforeSaving(final NpmPackageVersionResourceEntity en */ public void interceptEntityAfterSaving(final NpmPackageVersionResourceEntity entity, final IBaseResource res) { - switch (res) { + final var installedEntity = this.buildInstalledStructureDefinitionEntity(entity, res); + if (installedEntity != null) { + this.installedStructureDefinitionRepository.save(installedEntity); + } + } + + /** + * Builds (without saving) the {@link MbInstalledStructureDefinitionEntity} for a StructureDefinition that was + * just persisted, or {@code null} if {@code res} isn't a StructureDefinition. + *

+ * Split out from {@link #interceptEntityAfterSaving} so callers that need to isolate the save in its own + * transaction can build the entity first and save it independently - see + * {@link MbInstalledStructureDefinitionMigration}, where each row's save is isolated in its own transaction so + * a single bad row (e.g. a constraint violation) is simply skipped instead of aborting the whole backfill. + */ + @Nullable + public MbInstalledStructureDefinitionEntity buildInstalledStructureDefinitionEntity( + final NpmPackageVersionResourceEntity npmPackageVersionResourceEntity, + final IBaseResource res) { + return switch (res) { case org.hl7.fhir.r4.model.StructureDefinition sdR4 -> - this.interceptStructureDefinition(entity, sdR4, null, null); + this.toInstalledStructureDefinitionEntity(npmPackageVersionResourceEntity, sdR4, null, null); case org.hl7.fhir.r4b.model.StructureDefinition sdR4b -> - this.interceptStructureDefinition(entity, null, sdR4b, null); + this.toInstalledStructureDefinitionEntity(npmPackageVersionResourceEntity, null, sdR4b, null); case org.hl7.fhir.r5.model.StructureDefinition sdR5 -> - this.interceptStructureDefinition(entity, null, null, sdR5); - default -> { /* do nothing */ } - } + this.toInstalledStructureDefinitionEntity(npmPackageVersionResourceEntity, null, null, sdR5); + default -> null; + }; } /** @@ -79,13 +98,13 @@ private void updateStructureDefinition(final NpmPackageVersionResourceEntity npm } /** - * Intercept a StructureDefinition right after it got persisted in the database. Create our - * MbInstalledStructureDefinitionEntity to store it in an optimized way. + * Builds the MbInstalledStructureDefinitionEntity for a StructureDefinition, without saving it. */ - private void interceptStructureDefinition(final NpmPackageVersionResourceEntity npmPackageVersionResourceEntity, - final org.hl7.fhir.r4.model.@Nullable StructureDefinition sdR4, - final org.hl7.fhir.r4b.model.@Nullable StructureDefinition sdR4b, - final org.hl7.fhir.r5.model.@Nullable StructureDefinition sdR5) { + private MbInstalledStructureDefinitionEntity toInstalledStructureDefinitionEntity( + final NpmPackageVersionResourceEntity npmPackageVersionResourceEntity, + final org.hl7.fhir.r4.model.@Nullable StructureDefinition sdR4, + final org.hl7.fhir.r4b.model.@Nullable StructureDefinition sdR4b, + final org.hl7.fhir.r5.model.@Nullable StructureDefinition sdR5) { // 1. Extract interesting info final var terser = new FhirTerserWrapper(sdR4, sdR4b, sdR5); var title = terser.getSinglePrimitiveValueOrNull("title"); @@ -109,7 +128,7 @@ private void interceptStructureDefinition(final NpmPackageVersionResourceEntity entity.setKind(kind); entity.setValidatable(isValidatable); entity.setNpmPackageVersionResourceEntity(npmPackageVersionResourceEntity); - this.installedStructureDefinitionRepository.save(entity); + return entity; } /** diff --git a/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MbInstalledStructureDefinitionMigration.java b/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MbInstalledStructureDefinitionMigration.java index b0e5119d7d..38bf113b23 100644 --- a/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MbInstalledStructureDefinitionMigration.java +++ b/matchbox-server/src/main/java/ch/ahdis/matchbox/packages/MbInstalledStructureDefinitionMigration.java @@ -6,6 +6,7 @@ import ca.uhn.fhir.jpa.binary.api.IBinaryStorageSvc; import ca.uhn.fhir.jpa.dao.data.INpmPackageVersionResourceDao; import ca.uhn.fhir.jpa.dao.data.MbInstalledStructureDefinitionRepository; +import ca.uhn.fhir.jpa.model.entity.MbInstalledStructureDefinitionEntity; import ca.uhn.fhir.jpa.model.entity.NpmPackageVersionResourceEntity; import ch.ahdis.matchbox.config.MatchboxJpaConfig; import ch.ahdis.matchbox.spring.MatchboxEventListener; @@ -18,11 +19,13 @@ import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Slice; -import org.springframework.transaction.annotation.Transactional; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; +import org.springframework.transaction.support.TransactionTemplate; -import jakarta.persistence.EntityManager; -import jakarta.persistence.PersistenceContext; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; import static java.util.Objects.requireNonNull; @@ -41,35 +44,51 @@ *

* The bean is registered with prototype scope (see {@link MatchboxJpaConfig}) since it only does one-time startup * work: there won't be any reference kept to it, and it'll be collected by GC during the app lifecycle. + *

+ * Each page is processed in its own short transaction (see {@link #backfillPage}), rather than the whole backfill + * running as one long transaction: the embedded web server starts accepting HTTP requests during context refresh, + * i.e. before this ApplicationRunner even starts, so a single transaction spanning every installed + * StructureDefinition (potentially thousands, across many pages) would hold locks on NPM_PACKAGE_VER_RES / + * MB_INSTALLED_STRUCT_DEF for a long time while real traffic can already be hitting the same tables - a deadlock + * waiting to happen, and it did. * * @author Quentin Ligier **/ public class MbInstalledStructureDefinitionMigration implements ApplicationRunner { private static final Logger LOG = LoggerFactory.getLogger(MbInstalledStructureDefinitionMigration.class); + private static final int PAGE_SIZE = 250; private final MbInstalledStructureDefinitionRepository installedStructureDefinitionRepository; private final MatchboxJpaPackageCache matchboxJpaPackageCache; private final INpmPackageVersionResourceDao myPackageVersionResourceDao; private final IBinaryStorageSvc myBinaryStorageSvc; private final IFhirResourceDao binaryDao; - - @PersistenceContext - private EntityManager entityManager; + private final TransactionTemplate pageTxTemplate; + private final TransactionTemplate entityTxTemplate; public MbInstalledStructureDefinitionMigration(final MbInstalledStructureDefinitionRepository installedStructureDefinitionRepository, final MatchboxJpaPackageCache matchboxJpaPackageCache, final INpmPackageVersionResourceDao myPackageVersionResourceDao, final IBinaryStorageSvc myBinaryStorageSvc, - final DaoRegistry myDaoRegistry) { + final DaoRegistry myDaoRegistry, + final PlatformTransactionManager txManager) { this.installedStructureDefinitionRepository = requireNonNull(installedStructureDefinitionRepository); this.matchboxJpaPackageCache = requireNonNull(matchboxJpaPackageCache); this.myPackageVersionResourceDao = requireNonNull(myPackageVersionResourceDao); this.myBinaryStorageSvc = requireNonNull(myBinaryStorageSvc); this.binaryDao = (IFhirResourceDao) myDaoRegistry.getResourceDao("Binary"); + this.pageTxTemplate = new TransactionTemplate(requireNonNull(txManager)); + // A separate, REQUIRES_NEW template, only used as a fallback when a page's batch save fails: see + // #saveEntities for why. + this.entityTxTemplate = new TransactionTemplate(txManager); + this.entityTxTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); } + /** + * Deliberately not {@code @Transactional}: each page gets its own transaction via {@link #pageTxTemplate} in + * {@link #backfillPage}, see the class-level javadoc for why. + */ @Override - @Transactional public void run(final ApplicationArguments args) { if (this.installedStructureDefinitionRepository.count() > 0) { // Either this is not the first startup after the upgrade, or packages have already been installed @@ -77,44 +96,105 @@ public void run(final ApplicationArguments args) { return; } LOG.info("MB_INSTALLED_STRUCT_DEF is empty, backfilling it from the already-installed StructureDefinitions"); - this.backfillInstalledStructureDefinitions(); + + Pageable page = PageRequest.of(0, PAGE_SIZE); + boolean hasNext; + do { + LOG.trace("Backfilling MB_INSTALLED_STRUCT_DEF page {} (size {})", page.getPageNumber(), page.getPageSize()); + hasNext = this.backfillPage(page); + page = page.next(); + } while (hasNext); + LOG.info("MB_INSTALLED_STRUCT_DEF migration complete"); } /** - * Backfill for the MB_INSTALLED_STRUCT_DEF table on trigger - *

- * That table is populated by {@link MatchboxJpaPackageCache#interceptEntityAfterSaving} whenever a - * StructureDefinition is freshly installed. On an upgrade from a version predating that table, existing - * StructureDefinitions were never processed by that hook, so this replays it for every StructureDefinition - * already persisted in NPM_PACKAGE_VER_RES. + * Backfills one page of StructureDefinitions: reads it and rebuilds the entities in one transaction + * ({@link #buildPage}), then saves them ({@link #saveEntities}). + * + * @return whether there is a next page to process. + */ + private boolean backfillPage(final Pageable page) { + final PageBuild build = requireNonNull(this.pageTxTemplate.execute(status -> this.buildPage(page))); + if (!build.entities().isEmpty()) { + this.saveEntities(build.entities()); + } + return build.hasNext(); + } + + /** + * Reads one page of StructureDefinitions and rebuilds the MB_INSTALLED_STRUCT_DEF entity for each one (see + * {@link MatchboxJpaPackageCache#buildInstalledStructureDefinitionEntity}), without saving anything yet. Runs + * inside the page-level transaction opened by {@link #backfillPage}. + */ + private PageBuild buildPage(final Pageable page) { + final Slice slice = + this.myPackageVersionResourceDao.findByResourceTypeOrdered(page, "StructureDefinition"); + final List entities = new ArrayList<>(slice.getNumberOfElements()); + for (final NpmPackageVersionResourceEntity entity : slice.getContent()) { + final MbInstalledStructureDefinitionEntity installedEntity = this.buildEntity(entity); + if (installedEntity != null) { + entities.add(installedEntity); + } + } + return new PageBuild(entities, slice.hasNext()); + } + + /** + * Reads and parses one StructureDefinition's binary content and rebuilds its MB_INSTALLED_STRUCT_DEF entity, + * or returns {@code null} (logging the error) if that fails. + */ + private MbInstalledStructureDefinitionEntity buildEntity(final NpmPackageVersionResourceEntity entity) { + try { + // Yes, that's a SQL N+1 query here, but we can live with it. + final IBaseBinary binary = this.binaryDao.readByPid(entity.getResourceBinary().getId()); + final byte[] content = this.myBinaryStorageSvc.fetchDataByteArrayFromBinary(binary); + final IBaseResource resource = FhirContext.forCached(entity.getFhirVersion()) + .newJsonParser() + .parseResource(new String(content, StandardCharsets.UTF_8)); + return this.matchboxJpaPackageCache.buildInstalledStructureDefinitionEntity(entity, resource); + } catch (final Exception e) { + LOG.error( + "MATCHBOX: failed to backfill MB_INSTALLED_STRUCT_DEF for NpmPackageVersionResourceEntity#{}", + entity.getId(), e); + return null; + } + } + + /** + * Saves a page's worth of rows. *

- * Reprocesses every installed StructureDefinition unconditionally, so callers should only invoke this once, - * when MB_INSTALLED_STRUCT_DEF is empty (see {@link MbInstalledStructureDefinitionMigration}). + * Fast path: the whole page is saved as a single batch, in a single transaction/commit - this is what almost + * every page will hit, since bad rows should be rare. Only if that batch fails does this fall back to + * retrying each row individually, each in its own {@code REQUIRES_NEW} transaction: on Postgres, a single + * failed statement poisons every subsequent statement on the same connection/transaction until it's rolled + * back, so re-running the page one row at a time - each on its own connection - is the only way to identify + * and skip just the bad row(s) without aborting the rest of the page. Paying that per-row transaction + * overhead (a fresh connection plus a full commit round-trip each) only on the rare page that actually + * contains a bad row, instead of on every row, is what keeps the common case fast. */ - private void backfillInstalledStructureDefinitions() { - Pageable page = PageRequest.of(0, 50); - Slice slice; - do { - slice = this.myPackageVersionResourceDao.findByResourceTypeOrdered(page, "StructureDefinition"); - for (final NpmPackageVersionResourceEntity entity : slice.getContent()) { - try { - // Yes, that's a SQL N+1 query here, but we can live with it. - final IBaseBinary binary = this.binaryDao.readByPid(entity.getResourceBinary().getId()); - final byte[] content = this.myBinaryStorageSvc.fetchDataByteArrayFromBinary(binary); - final IBaseResource resource = FhirContext.forCached(entity.getFhirVersion()) - .newJsonParser() - .parseResource(new String(content, StandardCharsets.UTF_8)); - this.matchboxJpaPackageCache.interceptEntityAfterSaving(entity, resource); - } catch (final Exception e) { - LOG.error( - "MATCHBOX: failed to backfill MB_INSTALLED_STRUCT_DEF for NpmPackageVersionResourceEntity#{}", - entity.getId(), e); - } + private void saveEntities(final List entities) { + try { + this.pageTxTemplate.executeWithoutResult( + status -> this.installedStructureDefinitionRepository.saveAll(entities)); + return; + } catch (final Exception e) { + LOG.debug("MATCHBOX: batch save of {} MB_INSTALLED_STRUCT_DEF rows failed, retrying them individually", + entities.size(), e); + } + for (final MbInstalledStructureDefinitionEntity entity : entities) { + try { + this.entityTxTemplate.executeWithoutResult( + status -> this.installedStructureDefinitionRepository.save(entity)); + } catch (final Exception e) { + LOG.error("MATCHBOX: failed to save MB_INSTALLED_STRUCT_DEF row for canonical URL '{}'", + entity.getCanonicalUrl(), e); } - // Flush the pending entity inserts and clear the persistence context before loading the next page - this.entityManager.flush(); - this.entityManager.clear(); - page = page.next(); - } while (slice.hasNext()); + } + } + + /** + * The entities rebuilt from one page, ready to be saved, and whether there is a next page to process. + */ + private record PageBuild(List entities, boolean hasNext) { } }