[buongarzoni]

Description of the change

BufferedSender, the default sender, only queues payloads in send(). A daemon thread sends them every 5 seconds. When the JVM exits, that daemon thread is killed, so any occurrence still queued, or being sent at that moment, is lost. Short lived processes (CLIs, batch jobs) are hit hardest: they can report an error and exit before the first send.

This PR adds a JVM shutdown hook that sends those occurrences before the process exits.

How it works

• New config: flushOnShutdown (default true) and shutdownTimeoutMillis (default 2000). When enabled, Rollbar registers the hook on construction, and Rollbar.close(...) unregisters it.

• The sender is looked up when the hook runs, not when it is registered, so a sender replaced through configure() is the one flushed.

• The flush runs on a separate daemon thread and is bounded by shutdownTimeoutMillis. The sender has no socket timeout of its own, so a hanging endpoint can never stall JVM shutdown. Errors in the hook are caught and logged, never propagated.

• A BufferedSender is flushed without being closed. The JVM runs shutdown hooks concurrently, so the application may keep logging after this hook finishes, e.g. while Spring Boot drains in flight requests with server.shutdown=graceful. The hook therefore:

  1. waits for a send already in progress (it may have taken payloads off the queue that it is still sending),
  2. sends what is queued,
  3. from then on sends each new payload right away, instead of on the next 5s tick.

  The background thread is a daemon, so keeping it running never keeps the JVM alive. Other senders are closed with close(true).

• Retries are still queued for the next scheduled run, so a network outage during shutdown does not use up every retry attempt back to back.

• BufferedSender.close(true) now waits for a send already in progress before draining the queue. It also stops draining once a run sends nothing, instead of spinning while sending is suspended by the failure strategy.

• Android: disabled (flushOnShutdown(false)), since Android apps are not shut down through JVM shutdown hooks.

API surface

The only new public API is the two config options and SenderShutdownHooks.create(...), a factory that returns a plain Thread. The hook itself and the BufferedSender method it relies on are package private in com.rollbar.notifier.sender, so no flush/shutdown method is exposed on BufferedSender or Sender.

Behavior change

The hook is enabled by default, so upgrading changes behavior without any configuration:

• Occurrences that used to be lost on exit are now sent.
• Shutdown may take up to 2 seconds longer while queued occurrences are sent.

It can be turned off with ConfigBuilder.flushOnShutdown(false), or bounded differently with shutdownTimeoutMillis(long). Users who configure Rollbar through logback, log4j2 or RollbarFilter can do the same through a ConfigProvider (configProviderClassName).

Known limitations

• A send still in flight at the exact moment the JVM exits is lost, as is everything on SIGKILL. Avoiding that would mean blocking JVM exit.
• If the failure strategy has suspended sending (network down), the queue is not sent unless it is a durable queue.
• A sender that wraps a BufferedSender is not recognized as one, so it gets close(true) instead.
• SyncSender has no connect/read timeouts. The hook is bounded, but an explicit close(true) (e.g. from a logging appender's stop()) is not. This predates this PR and is better addressed separately.
• RollbarFilter.destroy() does not close the notifier, so on webapp redeploys the hook keeps it reachable until JVM shutdown. The sender's background thread already kept it alive before this PR.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

Shortcut stories and GitHub issues (delete irrelevant)

• Fix [SC-]
• Fix Remove GSON Dependency #1

Checklists

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6050a31abc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[rollbar-circleci-machine]

AI Agent Review (openai, openai-astra)

Overall: The change is sound. I read every changed file and its collaborators in the checkout and found no correctness defect in production code. I did not run the tests, so I can't say whether they pass.

What I checked:

• Hook lifecycle: Rollbar registers the hook only when flushOnShutdown() is true and there is a sender (Rollbar.java:51-53,66-87). close(boolean) removes it before closing the sender (Rollbar.java:753-757). An IllegalStateException (JVM already shutting down) and a SecurityException are both caught on add and on remove (Rollbar.java:79-86,130-137).
• Current sender at run time: CurrentSenderProvider reads the live config under configReadLock (Rollbar.java:94-104). This matches how RollbarBase.configure swaps the config under the write lock (RollbarBase.java:141-153).
• Timeout: SenderShutdownHook flushes on a daemon thread and waits for it with join(timeoutMillis). It deliberately avoids join(0), which would wait forever (SenderShutdownHook.java:52-74).
• BufferedSender.close(true): it now shuts down the executor and waits for any in-flight run before draining the queue (BufferedSender.java:129-147). That closes the real gap where a run had already taken payloads off the queue while they were still being sent. A periodic task is not rescheduled after shutdown(), so the wait ends once the current run finishes.
• Config: both new settings are carried through withConfig and ConfigImpl (ConfigBuilder.java:169-170,785-786).
• Android: it opts out because DiskQueue already persists payloads (rollbar-android/.../Rollbar.java:489-493).
• Style: the new Javadoc follows the existing <p> style that google_checks.xml already accepts, and the new lines are within the 100-character limit.

Notes outside the findings (these are design or out-of-diff observations, not defects):

• Un-closed notifiers are kept alive: the registered hook holds a non-static inner CurrentSenderProvider, which holds a reference to the whole Rollbar (Rollbar.java:71,94). Any notifier never passed to close(boolean) is therefore kept until JVM exit. With the default config this adds little, because each un-closed BufferedSender already leaves a live executor thread behind. Apps that create many Rollbar instances sharing one custom sender would now accumulate hooks, though. Consider saying in the flushOnShutdown Javadoc that notifiers should be closed.
• The 2 s bound applies only to this PR's own hook: an explicit close(true) is still unbounded (BufferedSender.java:141 plus SyncSender, which sets no socket timeouts). The logback and log4j2 appenders call it from stop() (rollbar-logback/.../RollbarAppender.java:140, rollbar-log4j2/.../RollbarAppender.java:145; neither file is in this diff). This was already true before the PR.
• Busy loop while sending is suspended: flushQueue spins when a SenderFailureStrategy has sending suspended (BufferedSender.java:155-158,320-321). This was already true before the PR. The only implementation is Android's, and Android opts out of the hook.
• RollbarFilter.destroy() never closes the notifier (rollbar-web/.../RollbarFilter.java:92-95; not in this diff).

[waltjones]

Required

1. The hook shuts the sender down as soon as shutdown starts, so errors logged during a graceful shutdown are dropped.

SenderShutdownHook.java:84 calls BufferedSender.close(true), which stops the background sending thread (BufferedSender.java:140). The JVM runs all shutdown hooks at the same time, so Rollbar's hook finishes within milliseconds of SIGTERM. Meanwhile, Spring Boot's hook may still be letting in-flight requests finish, for up to 30s with server.shutdown=graceful. From then on, send() (BufferedSender.java:100) only adds to the queue, and nothing ever sends what's queued.

• Before this PR: the background sending thread kept running while the hooks ran, sending every 5s. Only the last ≤5s of errors were lost.
• After this PR: everything logged after the hook's first flush is lost, which can be the whole shutdown window. That includes exceptions from requests failing while the server shuts down, which are exactly the errors people want during a deploy.
• Who is partly covered: logback/log4j2 appender users, because the appender's stop() calls close(true), which sends the queue again, as long as logging stops after the app does.
• Who is not covered: anyone using new Rollbar(...) or Rollbar.init(...) directly. That includes the usual Spring @bean setup: Rollbar has no no-argument close(), so Spring never closes it.
• Suggested fix: have the hook send what's queued without shutting the sender down.
  • Give BufferedSender a shutdown mode that sends what's queued, waits for the current send, and then sends each new payload right away (executorService.execute(sendTask)) instead of waiting for the next 5s tick.
  • Leave the background thread running; the JVM stops it when it exits. The 2s limit still applies to the first flush.
  • Add a test that runs the hook, then sends a payload, and checks that the payload is actually sent.