From 325150a938c7b3b5c3bb6c969095e950da542ba7 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 16 Oct 2026 11:39:12 -0500 Subject: [PATCH] fix: the access logger no longer keeps its instance alive The worker thread's task and the JVM shutdown hook referenced the logger (and through it the QInstance) strongly for the life of the JVM, so every instance a host or test built and dropped stayed in memory (kof22's suite ran out of heap in CI). Both now hold the logger weakly: a dropped instance can be collected and its worker stops at the next tick. - withRegisterShutdownHook(false) for hosts that close the runtime themselves (default true, unchanged behaviour). - isWorkerRunning() for hosts and tests. - README and CHANGELOG (also notes #18). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++ README.md | 3 +- .../secureddata/SecuredDataAccessLogger.java | 75 +++++++++++++++++-- .../secureddata/SecuredDataQBitConfig.java | 24 ++++++ .../SecuredDataAccessLoggerTest.java | 62 +++++++++++++++ 5 files changed, 164 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a722b0e..fe7cf18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes are documented here, following [Keep a Changelog](https://ke ## [Unreleased] +### Fixed: access-log lifecycle +- The access-log worker and JVM shutdown hook hold the logger weakly, so an instance the host drops (tests, reloads) can be collected; its worker stops at the next tick. +- `withRegisterShutdownHook(false)` for hosts that call `SecuredDataRuntime.close()` themselves; `SecuredDataAccessLogger.isWorkerRunning()`. +- Virtual (computed) fields resolve through the table classification instead of throwing (#18). +- Idle access-log ticks open no context, so the worker never validates an unfinished host instance, and a failed tick no longer stops the worker (#18). + ### Changed: UI API 1.6.0 - UI API extension secured-data 1.1.0 (requires QQQ 4.1.0-RC.15, UI API 1.6.0): operationIds, standard error responses with BasicErrorResponseV1, extension tags, and retained SecuredDataErrorV1 schema deprecated since 1.1.0. diff --git a/README.md b/README.md index 973031d..fb9f5fc 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ Policy/break-glass caches are bounded per instance and keyed by relevant user pe The extra `search_value_fingerprints` TEXT column persists JSON arrays of HMAC-SHA256 fingerprints for distinct-value detection after restarts and across nodes. Use a shared secret key supplier on multi-node deployments; the default is a random per-JVM key, so historical fingerprints from another JVM/key cannot be compared reliably. -One daemon worker drains a bounded queue as a system session. Enqueueing does not wait for persistence. Full queues and failed writes warn without values and increment `getDroppedCount()`. The app can expose this counter later. Explicitly call the instance's `SecuredDataRuntime.close()` at host shutdown; a JVM shutdown hook also drains accepted rows. An abrupt process crash can lose queued rows. +One daemon worker drains a bounded queue as a system session. Enqueueing does not wait for persistence. Full queues and failed writes warn without values and increment `getDroppedCount()`. The app can expose this counter later. Explicitly call the instance's `SecuredDataRuntime.close()` at host shutdown; a JVM shutdown hook also drains accepted rows unless `withRegisterShutdownHook(false)` turns it off (hosts that close the runtime themselves). Neither the worker nor the hook holds the instance strongly: an instance the host drops (tests, reloads) can be collected, and its worker stops at the next tick. An abrupt process crash can lose queued rows. Flagged rows raise the optional alerts producer signal `secured_data.enumeration`, deduplicated by `secured_data.enumeration::.:`. RC.5 already provides `QAlerts`; without an installed sink it logs the signal. Its catalog validator rejects underscores in type names, so this mandated type is sent through the producer without catalog registration until that core restriction is reconciled. No values or fingerprints are sent to the alert sink. @@ -144,6 +144,7 @@ The classification field picker uses CUSTOM possible values, reading `table_name | `withFingerprintKeySupplier(Supplier)` | Random 32-byte JVM key; supplied key must be at least 16 bytes | | `withAccessLogQueueCapacity(int)` | 4096 rows | | `withAccessLogFlushIntervalMillis(long)` | 250 ms | +| `withRegisterShutdownHook(boolean)` | true; JVM hook that drains the access log at shutdown | | `withExactMatchPerHour(int)` | 20; zero disables | | `withMissAlertConsecutive(int)` | 5; zero disables | | `withDistinctValueAlert(int)` | 15; zero disables | diff --git a/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLogger.java b/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLogger.java index ed77c5f..fe194dd 100644 --- a/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLogger.java +++ b/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLogger.java @@ -21,6 +21,7 @@ package com.kingsrook.qbits.secureddata; +import java.lang.ref.WeakReference; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayDeque; @@ -90,7 +91,12 @@ public class SecuredDataAccessLogger implements AutoCloseable /******************************************************************************* - ** One daemon drains this instance's queue; the JVM hook flushes on shutdown. + ** One daemon drains this instance's queue; the optional JVM hook flushes on + ** shutdown. Neither holds the logger strongly: once the host drops its + ** instance (a test, a reloaded instance), the logger, runtime and instance + ** can be collected, and the worker stops at its next tick. Hosts that close + ** the runtime themselves can turn the hook off + ** (SecuredDataQBitConfig.withRegisterShutdownHook(false)). *******************************************************************************/ public SecuredDataAccessLogger(QInstance instance, SecuredDataRuntime runtime) { @@ -103,10 +109,69 @@ public class SecuredDataAccessLogger implements AutoCloseable thread.setDaemon(true); return (thread); }); + WeakReference self = new WeakReference<>(this); long interval = runtime.config().getAccessLogFlushIntervalMillis(); - worker.scheduleWithFixedDelay(this::scheduledFlush, interval, interval, TimeUnit.MILLISECONDS); - shutdownHook = new Thread(this::close, "secured-data-access-log-shutdown"); - Runtime.getRuntime().addShutdownHook(shutdownHook); + worker.scheduleWithFixedDelay(new WeakTick(self, worker), interval, interval, TimeUnit.MILLISECONDS); + shutdownHook = runtime.config().getRegisterShutdownHook() ? new Thread(new WeakClose(self), "secured-data-access-log-shutdown") : null; + if(shutdownHook != null) + { + Runtime.getRuntime().addShutdownHook(shutdownHook); + } + } + + + + /******************************************************************************* + ** The worker's task, holding the logger weakly: when the logger has been + ** collected the worker shuts itself down. + *******************************************************************************/ + private record WeakTick(WeakReference logger, ScheduledExecutorService worker) implements Runnable + { + /******************************************************************************* + ** Drain, or stop once the logger is gone. + *******************************************************************************/ + @Override + public void run() + { + SecuredDataAccessLogger target = logger.get(); + if(target == null) + { + worker.shutdown(); + return; + } + target.scheduledFlush(); + } + } + + + + /******************************************************************************* + ** The JVM shutdown hook, holding the logger weakly. + *******************************************************************************/ + private record WeakClose(WeakReference logger) implements Runnable + { + /******************************************************************************* + ** Drain accepted rows at JVM shutdown, if the logger still exists. + *******************************************************************************/ + @Override + public void run() + { + SecuredDataAccessLogger target = logger.get(); + if(target != null) + { + target.close(); + } + } + } + + + + /******************************************************************************* + ** Whether the background worker is still running (false after close). + *******************************************************************************/ + public boolean isWorkerRunning() + { + return (!worker.isShutdown()); } @@ -488,7 +553,7 @@ public class SecuredDataAccessLogger implements AutoCloseable } worker.shutdown(); flush(); - if(Thread.currentThread() != shutdownHook) + if(shutdownHook != null && Thread.currentThread() != shutdownHook) { try { diff --git a/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataQBitConfig.java b/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataQBitConfig.java index edc18da..3647c96 100644 --- a/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataQBitConfig.java +++ b/src/main/java/com/kingsrook/qbits/secureddata/SecuredDataQBitConfig.java @@ -115,6 +115,7 @@ public class SecuredDataQBitConfig implements QBitConfig private Clock clock = Clock.systemUTC(); private int accessLogQueueCapacity = 4096; private long accessLogFlushIntervalMillis = 250; + private boolean registerShutdownHook = true; private int exactMatchPerHour = 20; private int missAlertConsecutive = 5; private int distinctValueAlert = 15; @@ -216,6 +217,29 @@ public class SecuredDataQBitConfig implements QBitConfig + /******************************************************************************* + ** Whether the access logger registers a JVM shutdown hook that drains + ** accepted rows (default true). Hosts that call SecuredDataRuntime.close() + ** from their own shutdown sequence turn it off. + *******************************************************************************/ + public boolean getRegisterShutdownHook() + { + return (registerShutdownHook); + } + + + + /******************************************************************************* + ** Configure registerShutdownHook. + *******************************************************************************/ + public SecuredDataQBitConfig withRegisterShutdownHook(boolean registerShutdownHook) + { + this.registerShutdownHook = registerShutdownHook; + return (this); + } + + + /******************************************************************************* ** Configure accessLogFlushIntervalMillis. *******************************************************************************/ diff --git a/src/test/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLoggerTest.java b/src/test/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLoggerTest.java index 76f2d30..eebd63f 100644 --- a/src/test/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLoggerTest.java +++ b/src/test/java/com/kingsrook/qbits/secureddata/SecuredDataAccessLoggerTest.java @@ -22,6 +22,7 @@ package com.kingsrook.qbits.secureddata; import java.io.Serializable; +import java.lang.ref.WeakReference; import java.time.Instant; import java.util.ArrayList; import java.util.List; @@ -387,4 +388,65 @@ class SecuredDataAccessLoggerTest idle.close(); } } + + + /******************************************************************************* + ** Neither the worker thread nor the JVM shutdown hook keeps a discarded + ** instance alive: a host or test that drops its instance without closing + ** the runtime lets it be collected. + *******************************************************************************/ + @Test + void aDiscardedInstanceCanBeCollected() throws Exception + { + close(); + WeakReference discarded = discardedInstance(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + List pressure = new ArrayList<>(); + while(discarded.get() != null && System.nanoTime() < deadline) + { + pressure.add(new byte[1 << 20]); + if(pressure.size() > 64) + { + pressure.clear(); + } + System.gc(); + Thread.sleep(20); + } + assertThat(discarded.get()).as("instance still reachable from the access logger").isNull(); + } + + + + /******************************************************************************* + ** Build, use and drop an instance whose worker ticks every 10 ms. + *******************************************************************************/ + private static WeakReference discardedInstance() throws Exception + { + QInstance instance = SecuredDataTestSupport.base(); + instance.addBackend(new com.kingsrook.qqq.backend.core.model.metadata.QBackendMetaData().withName("memory") + .withBackendType(com.kingsrook.qqq.backend.core.modules.backend.implementations.memory.MemoryBackendModule.class)); + instance.addTable(person("memory")); + new SecuredDataQBitProducer().withQBitConfig(new SecuredDataQBitConfig().withAccessLogFlushIntervalMillis(10)).produce(instance); + Thread.sleep(50); + QContext.clear(); + return (new WeakReference<>(instance)); + } + + + + /******************************************************************************* + ** Hosts that close the runtime themselves can skip the JVM hook; close + ** still drains and stops the worker. + *******************************************************************************/ + @Test + void shutdownHookIsOptional() throws Exception + { + setup(config -> config.withRegisterShutdownHook(false)); + var logger = SecuredDataRuntime.current().logger(); + assertThat(logger.isWorkerRunning()).isTrue(); + logger.onAccess(event(FieldAccessEventType.VIEW, null, null)); + logger.close(); + assertThat(logger.isWorkerRunning()).isFalse(); + assertThat(logs()).hasSize(1); + } } -- 2.54.0