fix(logging): shutdown hook cleanup reaches close0 when hook removal throws (#7686)

[konlue]

Fixes #7686.

Modifications

AbstractLogConsumeClient.close() removed its own shutdown hook before any exception handling. When close() runs from the hook thread during JVM shutdown, Runtime.removeShutdownHook throws IllegalStateException: Shutdown in progress, so close0() was never reached on the shutdown path and the backend was not flushed or cleaned up.

Hook removal now tolerates IllegalStateException: during shutdown the removal is both impossible and unnecessary, and cleanup proceeds to close0(). The single call into Runtime is isolated in a protected removeHook method so the shutdown-time failure can be reproduced deterministically instead of depending on real JVM shutdown. isStarted is flipped with compareAndSet(true, false), so close0() runs exactly once across explicit close, re-initialization and the shutdown hook; previously a concurrent close could run cleanup twice.

Verifying this change

• New testClose0StillRunsWhenHookRemovalFails: throws IllegalStateException("Shutdown in progress") from the hook-removal seam and asserts close0() still executes. Errors on the pre-fix code, green after.

• New testCloseIsIdempotent: repeated close runs close0() exactly once.

• Targeted tests pass:

  ./mvnw -B -ntp -pl shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common -am test -Dmaven.javadoc.skip=true

  62/62 green.

Notes

• The ElasticSearch, Kafka and RocketMQ log clients extend the base class but do not override close(), so no downstream change is needed.

[Aias00]

Summary

Fixes #7686: when close() is invoked by the registered shutdown-hook thread itself during JVM shutdown, Runtime.removeShutdownHook() throws IllegalStateException("Shutdown in progress"). In the previous code this exception propagated out of close() before close0() was reached, so the real resource cleanup was silently skipped exactly at JVM-shutdown time — the worst possible moment.

What the change does

AbstractLogConsumeClient.java:

• New tryRemoveShutdownHook(): reads closeThread.get(), and wraps the new protected removeHook(hook) in a try/catch (IllegalStateException) that logs at DEBUG. Cleanup now always proceeds to close0().
• New protected removeHook(hook): the single seam that calls Runtime.getRuntime().removeShutdownHook(hook), so tests can simulate the JVM-shutdown throw.
• close() now calls tryRemoveShutdownHook() first, then uses isStarted.compareAndSet(true, false) (atomic, idempotent) before invoking close0() — so close0() runs exactly once even under concurrent/re-entrant close.

Verification

• Master confirmation: master close() is exactly the buggy form — direct removeShutdownHook(closeThread.get()) with no try/catch, followed by if (isStarted.get()) { isStarted.set(false); close0(); }. The PR correctly isolates and fixes it.
• closeThread scope: closeThread is referenced only inside the parent class (field declaration, initClient set + addShutdownHook, and tryRemoveShutdownHook get). No subclass overrides or touches it → refactor is safe.
• Idempotency / race safety: the old isStarted.set(false) after a get() check was effectively idempotent single-threaded; the new compareAndSet additionally makes concurrent close() calls safe (two threads can no longer both run close0()). Strict improvement, no regression. tryRemoveShutdownHook() remains unconditionally invoked, so the non-shutdown path is unchanged.
• Tests are valid, not trivial:
  • testClose0StillRunsWhenHookRemovalFails uses ThrowingHookLogConsumeClient whose removeHook throws. Confirmed from source that the parent initClient sets closeThread to a registered hook thread (line 79-80), so tryRemoveShutdownHook() reads it and the override is genuinely invoked — the test really exercises the IllegalStateException path and asserts close0() ran once.
  • testCloseIsIdempotent: three close() calls → close0() runs once.
  • Local: mvn -pl shenyu-plugin/shenyu-plugin-logging/shenyu-plugin-logging-common test -Dtest=AbstractLogConsumeClientTest → 5 tests, 0 failures, BUILD SUCCESS.
• CI: all relevant checks pass — pr_build, Analyze (java), check-license-header, rat_check, scope, sql-matrix, registry_dependencies, build (shenyu-integrated-test-http), build (shenyu-integrated-test-k8s-gateway-api-http), agent_gateway_migrations. The only reds are e2e (4s aggregator) and e2e-case (e2e-logging-rocketmq-compose). These are attributed to the known RocketMQ logging-compose e2e flake — orthogonal to this PR (which only changes close() + a unit test and never touches the rocketmq e2e test or the consume path); this exact job has flaked on unrelated PRs (#7666 / #7661 / #7669).

Notes (non-blocking)

• In testClose0StillRunsWhenHookRemovalFails, the finally block removes client.getRegisteredHook() — a thread created in initClient0 that is not the one actually registered via addShutdownHook(closeThread.get()) (parent line 80). The real shutdown hook therefore leaks for the rest of the JVM. Because close() is now idempotent (compareAndSet), the leak is benign (no double close0() on actual shutdown), but for test hygiene it would be cleaner to remove closeThread.get() in the finally. Not a correctness issue.

Conclusion

Correct, well-scoped fix for a real shutdown-time cleanup gap, with valid regression tests and green relevant CI. APPROVE.