Skip to content

fix: log connection errors at info level - #41

Open
shreemaan-abhishek wants to merge 1 commit into
mainfrom
fix/socket-log-errors
Open

fix: log connection errors at info level#41
shreemaan-abhishek wants to merge 1 commit into
mainfrom
fix/socket-log-errors

Conversation

@shreemaan-abhishek

@shreemaan-abhishek shreemaan-abhishek commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #39.

A failed upstream connection was written to error.log at error level even though the error is also returned to the Lua caller:

[error] recv() failed (111: Connection refused)

The level comes from op->peer.log_error = NGX_ERROR_ERR, which ngx_event_connect_peer copies into c->log_error and nginx core consults in ngx_connection_error and the SSL error paths. lua_socket_log_errors off only reaches ngx_lua cosockets, so this module kept logging; a gateway that retries and fails over as a matter of course fills the log with routine refused connections, and tests pointing at a dead upstream on purpose fail their no_error_log blocks.

This sets peer.log_error to NGX_ERROR_INFO, matching a cosocket under lua_socket_log_errors off: the caller receives the error, the log records it at info.

The two existing connect-refused tests (t/001 TEST 5, t/006 TEST 10) previously had to skip the no_error_log [error] check; both now assert it. Verified the new assertions fail against a binary built from main and pass with this change; full suite is green (15 files, 382 tests).

Summary by CodeRabbit

  • Bug Fixes

    • Reduced the severity of connection peer errors in logs, preventing expected connection failures from being reported as critical errors.
    • Connection-refused scenarios no longer generate [error] log entries.
  • Tests

    • Updated connection failure coverage to verify the revised logging behavior and ensure no unexpected error messages are emitted.

The error is already returned to the Lua caller, yet every failed
upstream connection also wrote an error-level line, ignoring the
lua_socket_log_errors off convention APISIX relies on. Log through
NGX_ERROR_INFO instead, matching a cosocket with logging disabled.

Fixes #39
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6d79f534-26f1-41db-82e8-0282cfd3f190

📥 Commits

Reviewing files that changed from the base of the PR and between 8a517c4 and c5de0d3.

📒 Files selected for processing (3)
  • src/ngx_http_ffi_client_request.c
  • t/001-http11-short-lived.t
  • t/006-stateful-object.t

📝 Walkthrough

Walkthrough

The client now logs peer connection failures at NGX_ERROR_INFO. Short-lived and stateful connection-failure tests update their assertion plans and verify that no [error] log is emitted.

Changes

Connection Error Logging

Layer / File(s) Summary
Adjust connection logging and validate tests
src/ngx_http_ffi_client_request.c, t/001-http11-short-lived.t, t/006-stateful-object.t
Peer connection failures now use NGX_ERROR_INFO. Tests allocate three assertions per block and check for the absence of error-level log entries.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Suggested reviewers: membphis

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: logging connection errors at info level.
Linked Issues check ✅ Passed The code and tests implement issue #39 by changing connection errors to info level and asserting no error-level logs.
Out of Scope Changes check ✅ Passed All changes directly support the linked issue by updating connection logging and related connect-failure tests.
E2e Test Quality Review ✅ Passed The change sets peer.log_error to NGX_ERROR_INFO; E2E tests cover buffered and stateful refused connections, verify returned errors, and assert no [error] log with matching three-assertion plans.
Security Check ✅ Passed The patch only changes peer.log_error to NGX_ERROR_INFO; returned errors are unchanged, and no credential logging, storage, authorization, ownership, TLS, or secret-reference path is added.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/socket-log-errors

Comment @coderabbitai help to get the list of available commands.

@shreemaan-abhishek shreemaan-abhishek self-assigned this Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Connection errors are logged at error level, ignoring lua_socket_log_errors off

1 participant