Skip to content

Operator#installShutdownHook silently skips installation when leader election is enabled #3376

Closed

Description

@Dennis-Mircea

Summary

When leader election is enabled, Operator#installShutdownHook(Duration) does not install any JVM shutdown hook. It only logs a warning and returns. This silently drops a behavior the caller explicitly opted into, and the rationale for doing so is neither documented in the code nor in the related commit history. On bare-JVM deployments this leaves the leader lease unreleased on SIGTERM, forcing standbys to wait for lease expiry before they can take over.

Additionally, the method's Duration gracefulShutdownTimeout parameter has been dead since PR #2479. It is accepted but never honored at runtime, yet the JavaDoc still describes it as the timeout used during shutdown.

Code references

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java

// lines 143-150
@SuppressWarnings("unused")
public void installShutdownHook(Duration gracefulShutdownTimeout) {
  if (!leaderElectionManager.isLeaderElectionEnabled()) {
    Runtime.getRuntime().addShutdownHook(new Thread(this::stop));
  } else {
    log.warn("Leader election is on, shutdown hook will not be installed.");
  }
}

And the stop() method already handles leader election correctly:

// lines 191-214 (relevant excerpt)
@Override
public void stop() throws OperatorException {
  ...
  controllerManager.stop();
  configurationService.getExecutorServiceManager().stop(reconciliationTerminationTimeout);
  leaderElectionManager.stop();              // <-- already releases the lease
  if (configurationService.closeClientOnStop()) {
    getKubernetesClient().close();
  }
  ...
}

Observations

1. The asymmetry between installShutdownHook and stop

stop() is fully aware of leader election: it calls leaderElectionManager.stop(). If the shutdown hook simply invoked this::stop (which it does in the non-leader-election branch), the lease would be released cleanly on JVM shutdown. There is no apparent technical reason the hook can't be installed when leader election is on as the cleanup path is already there.

The current behavior means that:

  • The user explicitly calls installShutdownHook(...) to request graceful shutdown on SIGTERM.
  • JOSDK silently declines to honor that request (only a log.warn).
  • On SIGTERM, the JVM exits without invoking stop().
  • leaderElectionManager.stop() never runs.
  • The lease held by the dying leader remains in etcd until its leaseDuration elapses.
  • Standby replicas wait out the lease before electing themselves leader.

This is the opposite of what most users would expect from "install a graceful shutdown hook on a leader-elected operator".

2. Dead gracefulShutdownTimeout parameter

Originally installShutdownHook(Duration) called stop(gracefulShutdownTimeout), honoring the per-call value. Commit 9390d89e ("feat: support for graceful shutdown based on configuration", PR #2479) re-routed the timeout to come from ConfigurationService.reconciliationTerminationTimeout() and changed the hook to call the no-arg this::stop. The Duration parameter was kept in the signature (probably for binary compatibility) but is no longer used, hence the @SuppressWarnings("unused"). The JavaDoc still says:

@param gracefulShutdownTimeout timeout to wait for executor threads to complete actual reconciliations

…which is misleading. Callers can pass any value and it will have no effect.

3. No documented rationale for the skip

I searched the commit history for installShutdownHook and the warn-and-skip behavior, and could not find a commit message or PR description explaining why the hook is skipped when leader election is on. The closest hypothesis I can construct is that "frameworks like Spring/Quarkus install their own shutdown hooks, so JOSDK should not double-register", but that is never stated in the code, and it also leaves bare-JVM users with broken graceful shutdown.

Open Questions

  1. Is the skip-on-leader-election behavior intentional? If so, what's the underlying concern (double-hook registration with a framework, race with the lease renewer, something else)?
  2. Should installShutdownHook(...) install the hook regardless of leader election, and rely on stop() (which already handles leaderElectionManager.stop()) to do the right thing?
  3. The Duration parameter on installShutdownHook(Duration) is unused. Should it be deprecated in favor of a no-arg overload, with the JavaDoc updated to point at ConfigurationServiceOverrider#withReconciliationTerminationTimeout(...) as the real configuration knob?

Possible directions

Depending on maintainer guidance, any of these are reasonable PR scopes (single PR or split):

  • Docs-only: update the JavaDoc and the log.warn message to explain the current behavior and point users at the right pattern for framework-managed shutdown. No behavior change.
  • Behavior fix: install the hook regardless of leader-election state, since stop() is already leader-election-aware.
  • API cleanup: deprecate the Duration parameter; introduce installShutdownHook() (no-arg) and update the JavaDoc to reference withReconciliationTerminationTimeout(...).

Activity

  1. csviri commented on May 26, 2026

    @csviri
    Collaborator

    Thank you @Dennis-Mircea for reporting.

    @xstefank @metacosm if you have the bandwidth , I'm quite budy with other stuff this week and next week I'm off.

  2. metacosm commented on May 27, 2026

    @metacosm
    Collaborator

    @Dennis-Mircea skipping the hook registration when leader election is enabled apparently was added in #1618 to address a deadlock on shutdown. I'm actually not sure why this was done this way as I don't recall the details. I haven't been too involved with leader election but I think that it would certainly deserve an API clean-up because I see potential problems for sure, in particular with the registration of callbacks, which isn't particularly documented and might lead to issues since the framework needs to do its own processing in this situation. Since what the framework needs isn't documented, it's possible the user-registered callbacks might conflict with them.

  3. Dennis-Mircea commented on May 27, 2026

    @Dennis-Mircea
    ContributorAuthor

    Thanks for the context, @metacosm! Pulled up #1618 and the diff confirms the skip was added to avoid a shutdown deadlock, but neither the PR description nor the merge commit explains what specifically deadlocked. @csviri, do you happen to remember (or have a reproducer for) the original symptom? If the underlying issue has since been addressed in the lifecycle code (e.g. by the executor service / leader manager refactors), we may now be able to re-enable the hook safely.

    Separately, I'd appreciate guidance on what operators currently relying on installShutdownHook(...) in an HA setup should do today. For example, Flink Kubernetes Operator has only this in place at startup:

    operator.installShutdownHook(
        baseConfig.get(KubernetesOperatorConfigOptions.OPERATOR_TERMINATION_TIMEOUT));

    With leader election enabled (the whole point of HA), this line is effectively a no-op: the hook is silently not installed, the Duration argument is ignored (dead since #2479), and on SIGTERM the JVM exits without releasing the lease. Is the intended pattern that HA users call operator.stop() from their own application lifecycle instead of from a JVM hook? If so, that's worth surfacing in the JavaDoc and probably in a short note in the leader-election docs as well.

    Either way, agreed the surface area deserves a clean-up. I can start with a docs-only PR (JavaDoc + log message + deprecation of the unused Duration parameter on installShutdownHook), leaving any behavior change for a separate PR once we know more about the original deadlock and the recommended HA pattern.

  4. csviri commented on May 27, 2026

    @csviri
    Collaborator

    Hi @Dennis-Mircea thank you to pointing those out. I think the point 2. was not cleaned up when that PR was merged, I would deprecate that method an marked for removal and document that reconciliationTerminationTimeout should be used instead.

    will come back regarding 1. and 3., that we should document for sure; will check why we are not adding the shutdown hook in that case.

  5. csviri commented on May 27, 2026

    @csviri
    Collaborator

    So afaik this is the issue:
    if there is a leader election in place, a pod is either a leader or not. If it is the leader but would lost lead it calls a hard System.exit(...) , the shutdown hook should not be present at this point, since we want graceful stop.

    This had the deadlock issue in the past:
    #1614

    If it's not the leader, and receives a SIGTERM, the event processing is not running, so we don't want to have a shutdown hook either.

    However this is not entirely right this way, since if a pod is a leader and receives a SIGTERM but does not loose lead we still might want to have gracefull termination.

  6. added this to the 5.4 milestone on May 27, 2026
  7. Dennis-Mircea commented on May 27, 2026

    @Dennis-Mircea
    ContributorAuthor

    Thanks @csviri, this clarifies things a lot. I pulled up #1614 to confirm the deadlock shape and the stack trace is very specific:

    LeaderElectionManager.stopLeading(...)
      -> System.exit(...)                        <- blocked on java.lang.Shutdown lock
    LeaderCallbacks.onStopLeading(...)
    LeaderElectionManager.stop(...)              <- future cancellation
    Operator.stop(...)
      <- called from the JVM shutdown hook thread
    

    So the deadlock is not really "leader election + shutdown" in general. It's the specific path:

    1. JVM shutdown hook calls Operator.stop()
    2. Operator.stop() calls leaderElectionManager.stop(), which cancels the leader-election future
    3. Cancelling the future fires the onStopLeading callback
    4. The callback (in LeaderElectionManager.stopLeading) calls System.exit(...)
    5. System.exit tries to acquire java.lang.Shutdown's lock, which is already held by the shutdown hook thread we're running on -> deadlock

    The cure for the deadlock is therefore narrower than "skip the shutdown hook under leader election". It's: don't invoke System.exit when stop() has already been called, because at that point we are intentionally tearing down rather than reacting to a lost lease. With this small improvement, the shutdown hook can be re-enabled unconditionally and case 3 (leader receives SIGTERM, still holds lead) is fixed for free.

    Concretely, the fix looks like a single boolean flag on LeaderElectionManager:

    private final AtomicBoolean stoppingGracefully = new AtomicBoolean(false);
    
    public void stop() {
      stoppingGracefully.set(true);
      // existing cancellation logic
    }
    
    private void stopLeading() {  // the onStopLeading callback
      if (stoppingGracefully.get()) {
        return;  // graceful shutdown: lease release goes through the future cancellation, no exit
      }
      // existing loss-of-lead path
      System.exit(...);
    }

    That preserves the "restart on lost lead" behavior (case 1) and unblocks both case 2 and case 3, without introducing a second leader-election cleanup code path.

    Regarding the leader vs non-leader distinction you mentioned: as far as I can tell, leaderElectionManager.stop() is already safe to call on a non-leader (no lease to release, the underlying Fabric8 elector handles it). So I don't think we need an explicit if (isLeader) branch in Operator.stop(). Installing the shutdown hook unconditionally and letting the existing stop() flow run for both leaders and non-leaders looks like the simplest correct behavior.

    Proposed scope for the follow-up PR (separate from the deprecation cleanup we already agreed on for point 2):

    1. Skip System.exit(...) in LeaderElectionManager.stopLeading when stop() has already been called.
    2. Remove the leader-election conditional in Operator.installShutdownHook() so the hook is registered for every operator.
    3. Drop the "Leader election is on, shutdown hook will not be installed" log line and update the JavaDoc to reflect the new behavior.
    4. Add a regression test that reproduces LeaderElectionManager#stopLeading maybe deadlock with Operator#installShutdownHook #1614's deadlock scenario and verifies graceful shutdown completes.

    Does this direction work for you? Do you want to make the deprecation part and the actual fix in the same PR or do you want to have separate PRs for both?

  8. csviri commented on May 27, 2026

    @csviri
    Collaborator

    sounds good! let's see the code :) thank you!

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions