Skip to content

fix: skip writes and still shut down after failed DB manager startup - #4246

Open
arimu1 wants to merge 4 commits into
apache:2.xfrom
arimu1:fix/4241-abstract-db-manager-failed-startup
Open

fix: skip writes and still shut down after failed DB manager startup#4246
arimu1 wants to merge 4 commits into
apache:2.xfrom
arimu1:fix/4241-abstract-db-manager-failed-startup

Conversation

@arimu1

@arimu1 arimu1 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #4241 and #4242

Description

AbstractDatabaseManager checks isRunning() on the shutdown path but not on the write path.

After startupInternal() throws, startup() logs the cause and leaves running = false. Then:

  • write() did not check it — kept accepting events and dereferencing state startup never assigned (NPE per event, burying the original cause).
  • shutdown() only ran shutdownInternal() when running — so resources acquired during a failed startup (or in the manager factory) were never released. Returns true anyway.

This change:

  1. Makes write() return early when !isRunning(), logging a single status warning instead of one failure per event.
  2. Makes shutdown() always invoke shutdownInternal() once (tracked independently of running), so partial startup state can be cleaned up.
  3. Makes CassandraManager.shutdownInternal() null-safe when session was never created (motivating failure mode from the issue / related log4j-cassandra: failed appender startup leaks the DataStax Cluster, so the JVM never exits #4242).

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (the build instructions)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests are provided

Testing

  • JDK 17 (branch enforcer requires [17,18)) — JBR 17.0.14
  • ./mvnw test -pl :log4j-core-test -am -Dtest=AbstractDatabaseManagerTest -Dsurefire.failIfNoSpecifiedTests=false11/11 pass
    • New: testFailedStartupSkipsWrite, testFailedStartupStillShutsDown, testShutdownWithoutStartupStillRunsShutdownInternal
  • Spotless check clean on :log4j-core, :log4j-core-test, :log4j-cassandra

When startupInternal() fails, AbstractDatabaseManager left running=false
but write() still accepted events (often NPEing on unassigned state) and
shutdown() skipped shutdownInternal(), leaking resources acquired during
startup. Guard write() with isRunning() (log once) and always invoke
shutdownInternal() once. Make CassandraManager.shutdownInternal null-safe
for a missing session after failed connect.

Fixes apache#4241

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@vy
vy enabled auto-merge (squash) August 27, 2026 09:50
@vy

vy commented Aug 27, 2026

Copy link
Copy Markdown
Member

@arimu1, could you take care of the build failures, please?

@arimu1

arimu1 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @vy — CI should be green now.

AbstractDatabaseAppenderTest.testAppend was failing because the Mockito mock’s isRunning() is final and defaults to false, so write() returned early after the production guard. The test now uses a spy of a stub manager and calls startup() so the happy path still exercises writeThrough().

Failed-startup behavior remains covered by AbstractDatabaseManagerTest (testFailedStartupSkipsWrite, testFailedStartupStillShutsDown, etc.).

testAppend used a Mockito mock whose final isRunning() stayed false after
the write() guard change. Use a spy of a stub manager and call startup()
so the happy path still exercises writeThrough; failed-startup coverage
remains in AbstractDatabaseManagerTest.

Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
auto-merge was automatically disabled August 28, 2026 06:25

Head branch was pushed to by a user without write access

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

AbstractDatabaseManager: a manager whose startup failed still receives writes, and is never shut down

2 participants