Skip to content

fix(log-rotate): reopen logs after partial rotation - #13375

Merged
AlinsRan merged 3 commits into
masterfrom
fix/log-rotate-partial-reopen
Jul 8, 2026
Merged

fix(log-rotate): reopen logs after partial rotation#13375
AlinsRan merged 3 commits into
masterfrom
fix/log-rotate-partial-reopen

Conversation

@juzhiyuan

Copy link
Copy Markdown
Member

Description

Fixes #13368.

This updates log-rotate so a partial rotation failure does not prevent Nginx from reopening the log files that were rotated successfully. The regression test covers the reported behavior where a custom plugin writes through core.log.info after rotation and access logs should continue landing in the current access.log.

Root cause

rotate_file() iterates over the configured log files in apisix/plugins/log-rotate.lua. Before this patch, if rename_file() returned nil for any configured file, rotate_file() returned immediately from the loop before reaching the USR1 reopen step.

That means this sequence could happen:

  1. error.log is renamed to a timestamped file.
  2. access.log is missing, so rename_file() returns nil.
  3. rotate_file() returns before sending USR1 to the master process.
  4. The master keeps writing through stale file descriptors, so later plugin logs and access logs do not appear at the canonical current log paths.

Fix

  • Continue rotating the remaining configured files when one file is missing or cannot be renamed.
  • Track only files that actually have a rotated target.
  • Send USR1 if at least one log file was rotated.
  • Keep the degenerate both-files-missing case as a no-op by skipping USR1 when there are no rotated files.

Tests

  • Added a regression test in t/plugin/log-rotate.t using three pipelined requests:
    • /setup: configure a log-phase serverless-post-function, remove access.log, wait for partial rotation, then disable log-rotate to stabilize assertions.
    • /hello: trigger both plugin error-log output and access-log output.
    • /verify: assert the current error.log contains the plugin marker and current access.log contains the request.
  • The test explicitly avoids a single-request assertion shape because log-phase plugins run after the response has been sent.

Validation run:

PATH=/usr/local/openresty/bin:/usr/local/openresty/nginx/sbin:/usr/local/openresty/luajit/bin:$PATH \
FLUSH_ETCD=1 prove -Itest-nginx/lib -I. t/plugin/log-rotate.t

Result after the fix:

t/plugin/log-rotate.t .. ok
All tests successful.
Files=1, Tests=23, Result: PASS

Additional manual check:

  • Temporarily changed the regression setup to remove error.log while leaving access.log present; the same command passed all 23 tests after the fix.

Comment thread t/plugin/log-rotate.t
Comment thread t/plugin/log-rotate.t Outdated
Address review feedback on the regression test:
- Remove nginx_config error_log_level/access_log_buffer from both the
  block preprocessor and the /setup payload. The test framework builds
  nginx.conf via t/APISIX.pm (hardcoded log_level(info) and unbuffered
  access_log), so these config.yaml keys are never read and only read as
  load-bearing.
- Make /setup wait until the log-rotate timer has provably stopped before
  returning. plugins/reload only posts an event; the privileged agent
  processes it asynchronously, so a late rotation tick could otherwise
  fire between /hello and /verify and rotate the freshly written logs
  away, making the assertions flaky.
@AlinsRan
AlinsRan marked this pull request as ready for review July 7, 2026 09:15
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Jul 7, 2026

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@AlinsRan
AlinsRan merged commit 7f67c20 into master Jul 8, 2026
30 checks passed
@AlinsRan
AlinsRan deleted the fix/log-rotate-partial-reopen branch July 8, 2026 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: The logrotate plugin causes custom plugin logs not to be printed

5 participants