[buongarzoni]

Description of the change

Please include a summary of the change and which issues are fixed.
Please also include relevant motivation and context.

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.