From 9747ede41f5ab765127aa4e3bd977b25d80ead28 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Jul 2026 15:51:22 -0400 Subject: [PATCH 1/2] fix(gc): clean up tracking entities after operation completes --- .../io/cryostat/diagnostic/Diagnostics.java | 15 ++- .../diagnostic/DiagnosticsHelper.java | 20 ++++ .../db/migration/V4.2.1__cryostat.sql | 49 ++++++++ .../diagnostics/GarbageCollectionTest.java | 109 +++++++++++++----- 4 files changed, 161 insertions(+), 32 deletions(-) diff --git a/src/main/java/io/cryostat/diagnostic/Diagnostics.java b/src/main/java/io/cryostat/diagnostic/Diagnostics.java index 32ccdcc7a..427bb0e6c 100644 --- a/src/main/java/io/cryostat/diagnostic/Diagnostics.java +++ b/src/main/java/io/cryostat/diagnostic/Diagnostics.java @@ -298,7 +298,6 @@ public RestResponse handleThreadDumpsStorageDownload( @Path("targets/{targetId}/gc") @RolesAllowed("write") @Blocking - @Transactional @POST @Operation( summary = "Initiate a garbage collection on the specified target", @@ -308,14 +307,22 @@ public RestResponse handleThreadDumpsStorageDownload( request. This is generally equivalent to a System.gc() call made within the target JVM. """) public void gc(@RestPath long targetId) { - Target target = Target.getTargetById(targetId); + Target target = + QuarkusTransaction.requiringNew().call(() -> Target.getTargetById(targetId)); + GarbageCollection gc = + QuarkusTransaction.requiringNew() + .call( + () -> { + GarbageCollection entity = GarbageCollection.of(target); + entity.persist(); + return entity; + }); targetConnectionManager.executeConnectedTask( target, conn -> conn.invokeMBeanOperation( "java.lang:type=Memory", "gc", null, null, Void.class)); - - GarbageCollection.of(target).persist(); + QuarkusTransaction.requiringNew().run(() -> GarbageCollection.deleteById(gc.id)); } @Path("fs/heapdumps") diff --git a/src/main/java/io/cryostat/diagnostic/DiagnosticsHelper.java b/src/main/java/io/cryostat/diagnostic/DiagnosticsHelper.java index 17df564c5..477f1ad30 100644 --- a/src/main/java/io/cryostat/diagnostic/DiagnosticsHelper.java +++ b/src/main/java/io/cryostat/diagnostic/DiagnosticsHelper.java @@ -32,18 +32,22 @@ import io.cryostat.ConfigProperties; import io.cryostat.Producers; import io.cryostat.StorageBuckets; +import io.cryostat.asyncprofiler.AsyncProfilerRecording; import io.cryostat.diagnostic.Diagnostics.HeapDump; import io.cryostat.diagnostic.Diagnostics.ThreadDump; import io.cryostat.diagnostic.HeapDumpsMetadataService.StorageMode; import io.cryostat.libcryostat.sys.Clock; import io.cryostat.recordings.ActiveRecordings.Metadata; import io.cryostat.targets.Target; +import io.cryostat.targets.Target.EventKind; +import io.cryostat.targets.Target.TargetDiscovery; import io.cryostat.targets.TargetConnectionManager; import io.cryostat.ws.MessagingServer; import io.cryostat.ws.Notification; import io.quarkus.narayana.jta.QuarkusTransaction; import io.quarkus.runtime.StartupEvent; +import io.quarkus.vertx.ConsumeEvent; import io.smallrye.common.annotation.Identifier; import io.smallrye.mutiny.Uni; import io.smallrye.mutiny.infrastructure.Infrastructure; @@ -139,6 +143,22 @@ void onStart(@Observes StartupEvent evt) { buckets.createIfNecessary(heapDumpReportBucket); } + @ConsumeEvent(value = Target.TARGET_JVM_DISCOVERY, blocking = true) + void onTargetLost(TargetDiscovery event) { + if (!EventKind.LOST.equals(event.kind())) { + return; + } + Target target = event.serviceRef(); + QuarkusTransaction.requiringNew() + .run( + () -> { + AsyncProfilerRecording.delete("target", target); + io.cryostat.diagnostic.HeapDump.delete("target", target); + io.cryostat.diagnostic.ThreadDump.delete("target", target); + io.cryostat.diagnostic.GarbageCollection.delete("target", target); + }); + } + public void dumpHeap(Target target, String requestId) { log.tracev( "Heap Dump request received for Target: {0} with jobId {1}", target.id, requestId); diff --git a/src/main/resources/db/migration/V4.2.1__cryostat.sql b/src/main/resources/db/migration/V4.2.1__cryostat.sql index 3897c947e..614e635fd 100644 --- a/src/main/resources/db/migration/V4.2.1__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.1__cryostat.sql @@ -112,3 +112,52 @@ BEGIN RAISE NOTICE 'Added plugin-id labels to existing CryostatAgent target nodes'; END $$; + +-- Migrate existing GarbageCollection rows into _AUD as INSERT+DELETE revision pairs, +-- then truncate the primary table. Going forward, the gc() handler will persist and +-- immediately delete each entity so only _AUD retains the durable record. +DO $$ +DECLARE + migration_rev INTEGER; + migration_ts BIGINT; + gc RECORD; + insert_rev INTEGER; +BEGIN + migration_ts := EXTRACT(EPOCH FROM now()) * 1000; + + -- Create one synthetic REVINFO row for this migration event. + -- REVINFO.REV has no DEFAULT; use nextval() explicitly (Hibernate manages the sequence). + INSERT INTO REVINFO (REV, REVTSTMP, username) + VALUES (nextval('REVINFO_SEQ'), migration_ts, 'migration') + RETURNING REV INTO migration_rev; + + -- For each existing GarbageCollection row that has a matching INSERT revision in _AUD, + -- add a DELETE revision and back-fill REVEND on the INSERT revision. + FOR gc IN SELECT id FROM GarbageCollection LOOP + + -- Find the INSERT revision for this entity + SELECT REV INTO insert_rev + FROM GarbageCollection_AUD + WHERE id = gc.id AND REVTYPE = 0 + ORDER BY REV DESC + LIMIT 1; + + IF insert_rev IS NOT NULL THEN + -- Back-fill REVEND on the INSERT row (ValidityAuditStrategy requirement) + UPDATE GarbageCollection_AUD + SET REVEND = migration_rev, REVEND_TSTMP = migration_ts + WHERE id = gc.id AND REV = insert_rev; + + -- Insert the DELETE revision row + INSERT INTO GarbageCollection_AUD (id, REV, REVTYPE, REVEND, REVEND_TSTMP, target_id, triggeredAt) + SELECT gc.id, migration_rev, 2, NULL, NULL, target_id, triggeredAt + FROM GarbageCollection + WHERE id = gc.id; + END IF; + END LOOP; + + -- Primary table is now fully mirrored in _AUD; clear it + TRUNCATE TABLE GarbageCollection; + + RAISE NOTICE 'GarbageCollection migration complete: primary table truncated, % revision created', migration_rev; +END $$; diff --git a/src/test/java/io/cryostat/diagnostics/GarbageCollectionTest.java b/src/test/java/io/cryostat/diagnostics/GarbageCollectionTest.java index e0830e9c1..2c57d2138 100644 --- a/src/test/java/io/cryostat/diagnostics/GarbageCollectionTest.java +++ b/src/test/java/io/cryostat/diagnostics/GarbageCollectionTest.java @@ -17,6 +17,8 @@ import static io.restassured.RestAssured.given; +import java.util.List; + import io.cryostat.audit.AuditTestBase; import io.cryostat.diagnostic.Diagnostics; import io.cryostat.diagnostic.GarbageCollection; @@ -55,8 +57,22 @@ public void testGcTriggerCreatesAuditEntity() { .assertThat() .statusCode(204); - long count = GarbageCollection.count("target.id = ?1", Long.valueOf(targetId)); - Assertions.assertEquals(1, count, "Expected one GarbageCollection entity to be created"); + // Primary table must be empty — the gc() handler deletes the row after persisting. + long primaryCount = GarbageCollection.count("target.id = ?1", Long.valueOf(targetId)); + Assertions.assertEquals( + 0, primaryCount, "GarbageCollection primary table should be empty after gc()"); + + // _AUD table must have exactly 2 revisions (INSERT + DELETE) for the entity. + AuditReader auditReader = AuditReaderFactory.get(em); + List gcRevisions = + auditReader + .createQuery() + .forRevisionsOfEntity(GarbageCollection.class, false, true) + .getResultList(); + Assertions.assertEquals( + 2, + gcRevisions.size(), + "GarbageCollection_AUD should have 2 revisions (INSERT + DELETE)"); } @Test @@ -74,16 +90,24 @@ public void testGcTriggerCreatesAuditLog() { .assertThat() .statusCode(204); + // Primary row is gone; query the AUD table instead. AuditReader auditReader = AuditReaderFactory.get(em); - var gcEntity = - GarbageCollection.find("target.id", Long.valueOf(targetId)) - .firstResult(); - Assertions.assertNotNull(gcEntity, "GarbageCollection entity should exist"); - - var revisions = auditReader.getRevisions(GarbageCollection.class, gcEntity.id); - Assertions.assertTrue(!revisions.isEmpty(), "Should have at least one audit revision"); + List auditResults = + auditReader + .createQuery() + .forRevisionsOfEntity(GarbageCollection.class, true, true) + .getResultList(); + Assertions.assertFalse(auditResults.isEmpty(), "Should have at least one audit revision"); + + // Retrieve the entity state from the last known revision (includes deleted entities). + List allRevisions = + auditReader + .createQuery() + .forRevisionsOfEntity(GarbageCollection.class, false, true) + .getResultList(); Assertions.assertTrue( - revisions.size() >= 1, "Should have at least one audit revision for creation"); + allRevisions.size() >= 2, + "Should have at least 2 audit revisions (INSERT and DELETE)"); } @Test @@ -104,15 +128,22 @@ public void testGcEntityHasCorrectTimestamp() { long afterTrigger = System.currentTimeMillis(); - var gcEntity = - GarbageCollection.find("target.id", Long.valueOf(targetId)) - .firstResult(); - Assertions.assertNotNull(gcEntity); + // Read triggeredAt from the INSERT revision in _AUD (primary row is deleted). + AuditReader auditReader = AuditReaderFactory.get(em); + List results = + auditReader + .createQuery() + .forRevisionsOfEntity(GarbageCollection.class, true, true) + .getResultList(); + Assertions.assertFalse(results.isEmpty(), "Should have at least one audit revision"); + + GarbageCollection audited = (GarbageCollection) results.get(0); + Assertions.assertNotNull(audited); Assertions.assertTrue( - gcEntity.triggeredAt >= beforeTrigger, + audited.triggeredAt >= beforeTrigger, "Triggered timestamp should be after or equal to before trigger time"); Assertions.assertTrue( - gcEntity.triggeredAt <= afterTrigger, + audited.triggeredAt <= afterTrigger, "Triggered timestamp should be before or equal to after trigger time"); } @@ -131,12 +162,19 @@ public void testGcEntityHasCorrectTargetReference() { .assertThat() .statusCode(204); - var gcEntity = - GarbageCollection.find("target.id", Long.valueOf(targetId)) - .firstResult(); - Assertions.assertNotNull(gcEntity); - Assertions.assertNotNull(gcEntity.target); - Assertions.assertEquals(Long.valueOf(targetId), gcEntity.target.id); + // Read target reference from the INSERT revision in _AUD (primary row is deleted). + AuditReader auditReader = AuditReaderFactory.get(em); + List results = + auditReader + .createQuery() + .forRevisionsOfEntity(GarbageCollection.class, true, true) + .getResultList(); + Assertions.assertFalse(results.isEmpty()); + + GarbageCollection audited = (GarbageCollection) results.get(0); + Assertions.assertNotNull(audited); + Assertions.assertNotNull(audited.target); + Assertions.assertEquals(Long.valueOf(targetId), audited.target.id); } @Test @@ -156,9 +194,24 @@ public void testMultipleGcTriggersCreateMultipleEntities() { .statusCode(204); } - long count = GarbageCollection.count("target.id = ?1", Long.valueOf(targetId)); + // Primary table must be empty — each gc() call deletes its row. + long primaryCount = GarbageCollection.count("target.id = ?1", Long.valueOf(targetId)); + Assertions.assertEquals( + 0, + primaryCount, + "GarbageCollection primary table should be empty after 3 gc() calls"); + + // _AUD table must have 6 revisions: 2 (INSERT + DELETE) per trigger. + AuditReader auditReader = AuditReaderFactory.get(em); + List allRevisions = + auditReader + .createQuery() + .forRevisionsOfEntity(GarbageCollection.class, false, true) + .getResultList(); Assertions.assertEquals( - 3, count, "Expected three GarbageCollection entities to be created"); + 6, + allRevisions.size(), + "Should have 6 audit revisions (2 per trigger × 3 triggers)"); } @Test @@ -215,6 +268,8 @@ public void testGcAuditQueryIntegration() { long endTime = System.currentTimeMillis(); + // The DELETE transaction creates its own REVINFO row; query the latest revision + // in the time window — it should be the DELETE revision (revtype == 2). Integer revisionNumber = given().basePath("/") .log() @@ -235,6 +290,7 @@ public void testGcAuditQueryIntegration() { Assertions.assertNotNull(revisionNumber, "Should have at least one revision"); + // The most recent revision in the window is the DELETE revision (revtype == 2). given().basePath("/") .log() .all() @@ -251,9 +307,6 @@ public void testGcAuditQueryIntegration() { .body( "entities.GarbageCollection", org.hamcrest.Matchers.instanceOf(java.util.List.class)) - .body( - "entities.GarbageCollection[0].triggeredAt", - org.hamcrest.Matchers.notNullValue()) - .body("entities.GarbageCollection[0].revtype", org.hamcrest.Matchers.equalTo(0)); + .body("entities.GarbageCollection[0].revtype", org.hamcrest.Matchers.equalTo(2)); } } From efb0e18a80ee426e118813e36aaec74f8b632196 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Tue, 21 Jul 2026 16:13:16 -0400 Subject: [PATCH 2/2] only create GC migration REVINFO if there were entities to migrate --- .../db/migration/V4.2.1__cryostat.sql | 78 ++++++++++--------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/src/main/resources/db/migration/V4.2.1__cryostat.sql b/src/main/resources/db/migration/V4.2.1__cryostat.sql index 614e635fd..ad1d3c707 100644 --- a/src/main/resources/db/migration/V4.2.1__cryostat.sql +++ b/src/main/resources/db/migration/V4.2.1__cryostat.sql @@ -119,45 +119,51 @@ END $$; DO $$ DECLARE migration_rev INTEGER; - migration_ts BIGINT; gc RECORD; insert_rev INTEGER; + gc_count INTEGER; BEGIN - migration_ts := EXTRACT(EPOCH FROM now()) * 1000; - - -- Create one synthetic REVINFO row for this migration event. - -- REVINFO.REV has no DEFAULT; use nextval() explicitly (Hibernate manages the sequence). - INSERT INTO REVINFO (REV, REVTSTMP, username) - VALUES (nextval('REVINFO_SEQ'), migration_ts, 'migration') - RETURNING REV INTO migration_rev; - - -- For each existing GarbageCollection row that has a matching INSERT revision in _AUD, - -- add a DELETE revision and back-fill REVEND on the INSERT revision. - FOR gc IN SELECT id FROM GarbageCollection LOOP - - -- Find the INSERT revision for this entity - SELECT REV INTO insert_rev - FROM GarbageCollection_AUD - WHERE id = gc.id AND REVTYPE = 0 - ORDER BY REV DESC - LIMIT 1; - - IF insert_rev IS NOT NULL THEN - -- Back-fill REVEND on the INSERT row (ValidityAuditStrategy requirement) - UPDATE GarbageCollection_AUD - SET REVEND = migration_rev, REVEND_TSTMP = migration_ts - WHERE id = gc.id AND REV = insert_rev; - - -- Insert the DELETE revision row - INSERT INTO GarbageCollection_AUD (id, REV, REVTYPE, REVEND, REVEND_TSTMP, target_id, triggeredAt) - SELECT gc.id, migration_rev, 2, NULL, NULL, target_id, triggeredAt - FROM GarbageCollection - WHERE id = gc.id; - END IF; - END LOOP; + SELECT COUNT(*) INTO gc_count FROM GarbageCollection; + + IF gc_count > 0 THEN + -- Create one synthetic REVINFO row for this migration event only when there are rows + -- to migrate. Using timestamp 0 places this revision in the same "seed" epoch as the + -- initial revision, keeping it out of any recent time-range queries. + -- REVINFO.REV has no DEFAULT; use nextval() explicitly (Hibernate manages the sequence). + INSERT INTO REVINFO (REV, REVTSTMP, username) + VALUES (nextval('REVINFO_SEQ'), 0, 'migration') + RETURNING REV INTO migration_rev; + + -- For each existing GarbageCollection row that has a matching INSERT revision in _AUD, + -- add a DELETE revision and back-fill REVEND on the INSERT revision. + FOR gc IN SELECT id FROM GarbageCollection LOOP + + -- Find the INSERT revision for this entity + SELECT REV INTO insert_rev + FROM GarbageCollection_AUD + WHERE id = gc.id AND REVTYPE = 0 + ORDER BY REV DESC + LIMIT 1; + + IF insert_rev IS NOT NULL THEN + -- Back-fill REVEND on the INSERT row (ValidityAuditStrategy requirement) + UPDATE GarbageCollection_AUD + SET REVEND = migration_rev, REVEND_TSTMP = 0 + WHERE id = gc.id AND REV = insert_rev; + + -- Insert the DELETE revision row + INSERT INTO GarbageCollection_AUD (id, REV, REVTYPE, REVEND, REVEND_TSTMP, target_id, triggeredAt) + SELECT gc.id, migration_rev, 2, NULL, NULL, target_id, triggeredAt + FROM GarbageCollection + WHERE id = gc.id; + END IF; + END LOOP; - -- Primary table is now fully mirrored in _AUD; clear it - TRUNCATE TABLE GarbageCollection; + RAISE NOTICE 'GarbageCollection migration complete: primary table truncated, % revision created', migration_rev; + ELSE + RAISE NOTICE 'GarbageCollection migration: no existing rows, skipping revision creation'; + END IF; - RAISE NOTICE 'GarbageCollection migration complete: primary table truncated, % revision created', migration_rev; + -- Primary table is now fully mirrored in _AUD; clear it (no-op if already empty) + TRUNCATE TABLE GarbageCollection; END $$;