Skip to content

fix: demote websocket session ended log level from Error to Warn in Client.Run - #171

Open
PrashantBtkl wants to merge 1 commit into
mainfrom
fix/fix-demote-websocket-session-ended-log-l-1792579265
Open

PrashantBtkl wants to merge 1 commit into
mainfrom
fix/fix-demote-websocket-session-ended-log-l-1792579265

Conversation

@PrashantBtkl

Copy link
Copy Markdown
Contributor

Description

In pkg/ws/client.go at line 122 within Client.Run, websocket disconnect errors returned by connectAndServe(ctx) are logged at ERROR level with c.logger.Error("websocket session ended", "err", err). Because websocket disconnections are part of normal operational cycles handled cleanly via exponential backoff reconnection, logging at ERROR level causes Prometheus alert ForagerServiceLogErrors to trigger erroneously.

This change demotes the log level from Error to Warn (c.logger.Warn("websocket session ended", "err", err)), retaining visibility of session terminations without incrementing error log metrics (container_log_messages_total with level="error").

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Enhancement (non-breaking change which improves existing functionality)
  • Refactor (non-breaking change which improves code structure)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation
  • CI/CD

Original Error/Logs

Click to expand the original error that triggered this fix
In pkg/ws/client.go at line 122, change the log level for returned connection errors in Client.Run from c.logger.Error("websocket session ended", "err", err) to c.logger.Warn("websocket session ended", "err", err). Incident evidence shows the forager service remains healthy and automatically reconnects using exponential backoff, but logging at ERROR level triggers Prometheus alert ForagerServiceLogErrors. Regression check: verify that disconnect and reconnect cycles emit WARN level logs without incrementing container_log_messages_total with level="error".

How Has This Been Tested?

  • Unit tests
  • Manual testing

Checklist

  • CLA signed (the CLA bot will prompt on your first PR)
  • make validate passes (fmt + lint + test)
  • Docs updated if the wire shape, config surface, or proxy module behavior changed

🤖 This PR was automatically generated by NudgeBee AI coding agent

Powered by AI-driven code analysis and automated fix generation

🔍 View Original Investigation

Click the link above to see the full investigation and analysis that led to this fix

View Detailed Nubi Conversation

@PrashantBtkl PrashantBtkl self-assigned this Oct 21, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the log level from Error to Warn when a websocket session ends in pkg/ws/client.go. The reviewer recommends checking if the context has been cancelled before logging the warning to prevent false positives in log monitoring during normal shutdowns or restarts.

Comment thread pkg/ws/client.go Outdated
Comment on lines 121 to 123
if err != nil {
c.logger.Error("websocket session ended", "err", err)
c.logger.Warn("websocket session ended", "err", err)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

During a clean shutdown (when the context is cancelled), connectAndServe will return an error due to the connection being closed. Logging this as a warning can lead to false positives in log monitoring during normal deployments or restarts. Consider checking if the context has been cancelled before logging the warning.

Suggested change
if err != nil {
c.logger.Error("websocket session ended", "err", err)
c.logger.Warn("websocket session ended", "err", err)
}
if err != nil && ctx.Err() == nil {
c.logger.Warn("websocket session ended", "err", err)
}

…lient.Run

# Description

In `pkg/ws/client.go` at line 122 within `Client.Run`, websocket disconnect errors returned by `connectAndServe(ctx)` are logged at ERROR level with `c.logger.Error("websocket session ended", "err", err)`. Because websocket disconnections are part of normal operational cycles handled cleanly via exponential backoff reconnection, logging at ERROR level causes Prometheus alert `ForagerServiceLogErrors` to trigger erroneously.

This change demotes the log level from `Error` to `Warn` (`c.logger.Warn("websocket session ended", "err", err)`), retaining visibility of session terminations without incrementing error log metrics (`container_log_messages_total` with `level="error"`).

## Type of change

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Enhancement (non-breaking change which improves existing functionality)
- [ ] Refactor (non-breaking change which improves code structure)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation
- [ ] CI/CD

## Original Error/Logs

<details>
<summary>Click to expand the original error that triggered this fix</summary>

```
In pkg/ws/client.go at line 122, change the log level for returned connection errors in Client.Run from c.logger.Error("websocket session ended", "err", err) to c.logger.Warn("websocket session ended", "err", err). Incident evidence shows the forager service remains healthy and automatically reconnects using exponential backoff, but logging at ERROR level triggers Prometheus alert ForagerServiceLogErrors. Regression check: verify that disconnect and reconnect cycles emit WARN level logs without incrementing container_log_messages_total with level="error".
```

</details>

# How Has This Been Tested?

- [x] Unit tests
- [ ] Manual testing

# Checklist

- [ ] CLA signed (the CLA bot will prompt on your first PR)
- [x] `make validate` passes (fmt + lint + test)
- [ ] Docs updated if the wire shape, config surface, or proxy module behavior changed

---

🤖 **This PR was automatically generated by [NudgeBee](https://nudgebee.com) AI coding agent**

*Powered by AI-driven code analysis and automated fix generation*

🔍 **[View Original Investigation](https://app.nudgebee.com/investigate?id=4b562c79-04b7-4e52-80d5-3c66f80f52e8)**

*Click the link above to see the full investigation and analysis that led to this fix*

View Detailed **[Nubi Conversation](https://app.nudgebee.com/ask-nudgebee?accountId=f13ddc45-07f3-42b4-bd0e-19606c852d18&session_id=event-f48d9d60d6537ce9)**
@nudgebee-dev
nudgebee-dev Bot force-pushed the fix/fix-demote-websocket-session-ended-log-l-1792579265 branch from 58be446 to f696387 Compare October 21, 2026 10:48
@nudgebee-dev

nudgebee-dev Bot commented Oct 21, 2026

Copy link
Copy Markdown

Nudgebee Automated Followup

During clean shutdown or service restarts, context cancellation terminates websocket connections, causing connectAndServe to return context.Canceled. Demoting log levels to Warn still emits warnings unless ctx.Err() is verified to be nil. Additionally, CI govulncheck failures related to the Go toolchain version in go.mod require updating the Go directive to 1.26.10.

Commit: f696387e785d

@nudgebee-dev

nudgebee-dev Bot commented Oct 21, 2026

Copy link
Copy Markdown

Nudgebee Automated Followup

I reviewed the open comment(s) on this PR but couldn't automatically apply a change in this run.

What I looked at: The PR demoted websocket session disconnect logging in pkg/ws/client.go from Error to Warn to avoid triggering false-positive Prometheus alerts (ForagerServiceLogErrors) during normal reconnections. Code review comment #4335828999 requested checking ctx.Err() == nil to prevent warnings on clean shutdown/cancellation, which was already applied in the latest commit. Local validation make validate passes cleanly with zero issues.

No code was changed. If this needs a manual edit, please apply it directly.

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.

1 participant