AP-25788: Dump log buffer in shutdown hook if not yet initialized

[enplotz]

AP-25788 (NodeLogger may swallow buffered log messages)

[Copilot]

Pull request overview

This PR addresses AP-25788 by ensuring buffered startup log messages don’t get silently lost when the JVM shuts down before KNIME logging finishes initialization, by adding an emergency buffer dump mechanism and configuration knobs.

Changes:

• Added a structured “drain result” (BufferContents) and a drain() API to extract buffered messages plus eviction metadata.
• Installed a shutdown hook (on first pre-init log usage) that dumps buffered startup log messages to stdout/stderr with configurable minimum level.
• Introduced environment variables to control the emergency dump target stream and minimum dump level.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
org.knime.core/src/eclipse/org/knime/core/node/logging/LogBuffer.java	Adds BufferContents and a drain() method to retrieve buffered messages + eviction metadata in one shot.
org.knime.core/src/eclipse/org/knime/core/node/logging/KNIMELogger.java	Installs an emergency shutdown hook and implements dumping of buffered startup logs to a PrintStream with level filtering.
org.knime.core/src/eclipse/org/knime/core/node/KNIMEConstants.java	Adds env var constants to configure the emergency dump target and minimum level.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

drainBufferedMessagesTo introduces non-trivial behavior (level filtering, eviction notice gating, and single-shot dumping via isEmergencyBufferDumped) but there are no tests covering it. Since this module already has JUnit coverage for KNIMELogger (see org.knime.core.tests/.../KNIMELoggerTest), it would be good to add tests that (a) buffer some messages before init, (b) invoke the dump with a PrintStream backed by a ByteArrayOutputStream, and (c) assert output respects EMERGENCY_BUFFER_DUMP_MIN_LEVEL and eviction notice behavior.

[Copilot]

drain() uses m_logBuffer.drainingIterator() which is explicitly documented as not thread-safe and requiring synchronization on the parent object. Since drain() is a new entry point for draining, it should either be synchronized or have equivalent Javadoc clearly stating the required external synchronization (e.g. synchronized(BUFFER) in KNIMELogger) to prevent accidental unsafe use later.

Suggested change

	final var current = m_logBuffer.size();
	final var messages = new BufferedLogMessage[current];
	final var iter = m_logBuffer.drainingIterator();
	for (var i = 0; i < current; i++) {
	messages[i] = iter.next();
	}
	return new BufferContents(messages, m_logBuffer.getNumberOfEvictedEntries(), m_logBuffer.m_levelForEvictionMessage);
	synchronized (m_logBuffer) {
	final var current = m_logBuffer.size();
	final var messages = new BufferedLogMessage[current];
	final var iter = m_logBuffer.drainingIterator();
	for (var i = 0; i < current; i++) {
	messages[i] = iter.next();
	}
	return new BufferContents(messages, m_logBuffer.getNumberOfEvictedEntries(),
	m_logBuffer.m_levelForEvictionMessage);
	}

[Copilot]

installEmergencyBufferDumpHandlers() resets isShutdownHookInstalled to false when addShutdownHook throws. For IllegalStateException (shutdown already in progress), this will cause repeated failed attempts (and repeated stderr output) on subsequent log calls, and the buffered messages still won’t be dumped. Consider handling IllegalStateException separately by dumping the buffer immediately (or leaving the flag true to avoid retries), and only resetting the flag for SecurityException if you want to allow retry when permissions change.

Suggested change

	} catch (IllegalStateException | SecurityException ex) {
	isShutdownHookInstalled.set(false);
	EMERGENCY_BUFFER_DUMP_TARGET.println("Unable to register KNIME startup log shutdown hook:");
	} catch (IllegalStateException ex) {
	// JVM shutdown is already in progress; we cannot register a shutdown hook anymore.
	// Keep the flag set to avoid repeated failed attempts and dump buffered messages immediately.
	EMERGENCY_BUFFER_DUMP_TARGET.println(
	"Unable to register KNIME startup log shutdown hook because JVM shutdown is already in progress.");
	ex.printStackTrace(EMERGENCY_BUFFER_DUMP_TARGET);
	drainBufferedMessagesTo(EMERGENCY_BUFFER_DUMP_TARGET,
	"JVM is shutting down before KNIME logging could finish initialization (emergency dump without shutdown hook)");
	} catch (SecurityException ex) {
	// Registration was blocked by a security manager; allow future retries by resetting the flag.
	isShutdownHookInstalled.set(false);
	EMERGENCY_BUFFER_DUMP_TARGET.println(
	"Unable to register KNIME startup log shutdown hook due to security restrictions:");