Skip to content

fix: demote websocket session termination logging to Warn and reset backoff on sustained connection - #170

Closed
PrashantBtkl wants to merge 1 commit into
mainfrom
fix/fix-demote-websocket-session-termination-1792577693
Closed

PrashantBtkl wants to merge 1 commit into
mainfrom
fix/fix-demote-websocket-session-termination-1792577693

Conversation

@PrashantBtkl

Copy link
Copy Markdown
Contributor

Description

In pkg/ws/client.go, Client.Run logs websocket session terminations and auto-reconnect errors at Error severity (c.logger.Error("websocket session ended", "err", err)). When transient network drops occur, this triggers alert rules configured for error-level logs. In addition, the reconnect loop continually doubles delay up to maxReconnectDelay without ever resetting delay back to initialDelay when a connection succeeds and stays active.

This change:

  1. Demotes the session termination log in Client.Run from Error to Warn.
  2. Records start := time.Now() before calling c.connectAndServe(ctx) and resets delay = initialDelay if the connection was sustained for longer than maxReconnectDelay, ensuring subsequent reconnections start with the initial delay rather than the maximum backoff.

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, modify Client.Run around line 122 so that websocket session terminations and auto-reconnect errors from connectAndServe are logged at Warn or Info severity instead of Error severity, to prevent transient network drops from triggering error-log alert rules, and reset the exponential backoff delay to initialDelay upon a sustained successful connection.

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

…ackoff on sustained connection

# Description

In `pkg/ws/client.go`, `Client.Run` logs websocket session terminations and auto-reconnect errors at `Error` severity (`c.logger.Error("websocket session ended", "err", err)`). When transient network drops occur, this triggers alert rules configured for error-level logs. In addition, the reconnect loop continually doubles `delay` up to `maxReconnectDelay` without ever resetting `delay` back to `initialDelay` when a connection succeeds and stays active.

This change:
1. Demotes the session termination log in `Client.Run` from `Error` to `Warn`.
2. Records `start := time.Now()` before calling `c.connectAndServe(ctx)` and resets `delay = initialDelay` if the connection was sustained for longer than `maxReconnectDelay`, ensuring subsequent reconnections start with the initial delay rather than the maximum backoff.

## 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, modify Client.Run around line 122 so that websocket session terminations and auto-reconnect errors from connectAndServe are logged at Warn or Info severity instead of Error severity, to prevent transient network drops from triggering error-log alert rules, and reset the exponential backoff delay to initialDelay upon a sustained successful connection.
```

</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)**
@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 WebSocket client's reconnection logic to reset the exponential backoff delay if a connection is sustained longer than maxReconnectDelay, and downgrades the session-end log level from Error to Warn. The review feedback points out a critical edge case where a slow, failed connection attempt exceeding maxReconnectDelay could incorrectly reset the backoff delay, and suggests ensuring the connection was successfully established before resetting.

Comment thread pkg/ws/client.go
Comment on lines +127 to 129
if time.Since(start) > maxReconnectDelay {
delay = initialDelay
}

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

Checking time.Since(start) > maxReconnectDelay to determine if the connection was sustained has a subtle edge case: if the connection attempt fails (e.g., due to a slow DNS resolution or TCP connection timeout) but takes longer than maxReconnectDelay to return an error, the backoff delay will be incorrectly reset to initialDelay. This defeats the exponential backoff mechanism when the server is completely unreachable or slow to respond.

To fix this robustly, consider having connectAndServe return a boolean indicating whether the connection was successfully established (e.g., after the greeting is sent), and only reset the backoff if the connection was both established and sustained:

// In Client.Run:
established, err := c.connectAndServe(ctx)
...
if established && time.Since(start) > maxReconnectDelay {
    delay = initialDelay
}

@nudgebee-dev

nudgebee-dev Bot commented Oct 21, 2026

Copy link
Copy Markdown

Nudgebee Automated Followup

CI check 'vuln' failed due to a vulnerability in crypto/x509 in Go standard library 1.26.9, requiring Go >= 1.26.10. Additionally, review feedback on the PR noted that backoff delay was reset if connectAndServe blocked for longer than maxReconnectDelay even if the connection never succeeded (e.g., dial timeout or network hang during dial). Returning whether the connection was established ensures backoff only resets on sustained active sessions.

Commit: 7ad370ac1009

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