Skip to content

Conversation

yuri1969
Copy link
Contributor

@yuri1969 yuri1969 commented Oct 9, 2025

What changes are being made and why?

The @PreDestroy hook is triggered very late in the lifecycle.

Started the worker's graceful cleaning process using the global ShutdownEvent which fires way earlier.

I was on the fence with removing the worker's @PreDestroy hook.

closes #11817


How the changes have been QAed?

watch -n 1 'curl http://localhost:8081/prometheus'

Running the standalone server, executing the following flow and then quitting the server:

id: crane_325035
namespace: company.team

tasks:
  - id: sleep
    type: io.kestra.plugin.scripts.shell.Script
    script: |
      trap 'echo "nope"' INT
      sleep 60

Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the @PreDestroy annotation be removed from the Worker?
Anyway close() is safe to be used multiple time but as we move the closing of the Worker here it may be a good idea to make it clear (and also add a comment inside the Worker itself).

@fhussonnois WDYT?

Copy link
Member

@fhussonnois fhussonnois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @yuri1969, thank you for your contribution. I'm OK with the use of an ApplicationEventListener to manage this issue. However, the proposed solution is not ideal for me. We should not explicitly reference the worker, because it will only work for standalone. In addition, I think the HTTP server should be keep up for any services.

So maybe we could just delay the shutdown of the embedded netty server while all services are not closed, with something like this:

=> This needs to be tested:

@Singleton
@Slf4j
@Order(Ordered.LOWEST_PRECEDENCE)
@Requires(property = "kestra.server-type")
public class GracefulEmbeddedServiceShutdownListener implements ApplicationEventListener<ServerShutdownEvent> {
   
   @Inject
   ServiceRegistry serviceRegistry;
   
   @Override
   public void onApplicationEvent(ServerShutdownEvent event) {
       List<LocalServiceState> states = serviceRegistry.all();
       if (states.isEmpty()) {
           return;
       }
       
       List<CompletableFuture<Void>> futures = states.stream()
           .map(state -> CompletableFuture.runAsync(() -> closeService(state), ForkJoinPool.commonPool()))
           .toList();

       // Wait for all services to close, before shutting down the embbeded server
       CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join();
   }
   
   private void closeService(LocalServiceState state) {
       final Service service = state.service();
       try {
           service.unwrap().close();
       } catch (Exception e) {
           log.error("[Service id={}, type={}] Unexpected error on close", service.getId(), service.getType(), e);
       }
   }
}

Then, it's OK to keep @PreDestroy on services because the close method must be safe if called twice.

The `@PreDestroy` hook is triggered very late in the lifecycle.

Started the graceful clean using the global `ShutdownEvent`.
@yuri1969
Copy link
Contributor Author

@fhussonnois Thanks, I've tested the ServiceRegistry-based version of yours on both standalone and separated instances.

It does not work as intended when listening to the ServerShutdownEvent event (fired when the EmbeddedServer shuts down) but it does with the ShutdownEvent event (fired prior to starting shutdown sequence).

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

Labels

None yet

Projects

Status: To review

Development

Successfully merging this pull request may close these issues.

Keep metrics endpoint up during graceful long shutdown

3 participants