Skip to content

Fix Monitor command blocking on ReadTimeout#3716

Open
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-monitor-command-timeout
Open

Fix Monitor command blocking on ReadTimeout#3716
Copilot wants to merge 7 commits intomasterfrom
copilot/fix-monitor-command-timeout

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 20, 2026

The Monitor command fails when ReadTimeout is reached, leaving channel listeners permanently blocked. The timeout error is caught but not propagated, and the channel is never closed.

Changes

  • Timeout handling: Monitor now detects and ignores timeout errors using isTimeoutError(), treating them as expected behavior for long-running monitoring operations
  • Channel lifecycle: Added closeChannel() helper with closed flag to safely close the monitor channel exactly once when:
    • Context is cancelled
    • Stop() is called
    • Non-timeout errors occur (EOF, connection errors, etc.)
  • Context awareness: Added explicit context cancellation checks in the read loop

Example

// Monitor now works with short ReadTimeout
client := redis.NewClient(&redis.Options{
    Addr:        "127.0.0.1:6379",
    ReadTimeout: 100 * time.Millisecond,  // Previously would break
})

stream := make(chan string, 1000)
monitor := client.Monitor(ctx, stream)
monitor.Start()

// Continues receiving messages despite timeouts
for msg := range stream {
    fmt.Println(msg)
}

The monitor continues operation across timeout boundaries while remaining stoppable via Stop() or context cancellation.

Original prompt

This section details on the original issue you should resolve

<issue_title>monitor command breaks due to ReadTimeout</issue_title>
<issue_description>## Expected Behavior

Monitor command should work.

Current Behavior

If you do not set a large ReadTimeout on the client when using the monitor command then the socket timeout is hit but that error is never propagated back in any way so the channel listener is left in a blocked state.

Possible Solution

Either:

  1. Update docs to inform users that they are responsible for detecting timeouts when listening to the channel
  2. A ReadTimeout (or any other error) should be propagated back to user via closing the monitor channel

Steps to Reproduce

  1. Start client
	client := redis.NewClient(&redis.Options{
		Addr: "127.0.0.1:6379",
		// ReadTimeout: 1 * time.Minute,
	})

	stream := make(chan string, 1000)
	monitorCmd := client.Monitor(ctx, stream)
	monitorCmd.Start()

	for {
		var msg string

		select {
		case <-ctx.Done():
			fmt.Println("ctx done")
		case msg = <-stream:
			fmt.Println("message")
		}

		fmt.Println(msg)
	}
  1. Wait a second and then via cli perform any redis operation
  2. Check output from client
    </issue_description>

<agent_instructions>monitor command should ignore timeout, but allow user to cancel/stop</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@ndyakov @stlava is this still a problem with the latest versions?

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.


Note

Medium Risk
Touches long-running goroutine/channel coordination in Monitor, so regressions could cause leaks or premature channel closure, but changes are localized and covered by new tests.

Overview
Fixes Monitor to be resilient to socket ReadTimeout by treating timeout errors from Peek/ReadString as non-fatal and continuing the read loop.

Improves lifecycle handling by adding a closed flag plus closeChannel() to ensure the monitor output channel is closed exactly once on Stop(), context cancellation, or non-timeout errors, preventing callers from blocking indefinitely. Adds opt-in (env-gated) tests covering timeout behavior and context cancellation cleanup.

Reviewed by Cursor Bugbot for commit 2c7e9a7. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI and others added 3 commits February 20, 2026 16:33
Co-authored-by: ndyakov <1547186+ndyakov@users.noreply.github.com>
Co-authored-by: ndyakov <1547186+ndyakov@users.noreply.github.com>
Co-authored-by: ndyakov <1547186+ndyakov@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix monitor command breaks due to ReadTimeout Fix Monitor command blocking on ReadTimeout Feb 20, 2026
Copilot AI requested a review from ndyakov February 20, 2026 16:41
@ndyakov ndyakov requested a review from ofekshenawa February 20, 2026 17:25
@ndyakov ndyakov marked this pull request as ready for review February 20, 2026 17:46
@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Feb 26, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Comment thread command.go
Comment thread monitor_test.go
Comment thread command.go
}
cmd.ch <- line
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tight CPU-spinning loop when monitor status is idle

Low Severity

The original code called rd.Peek(1) unconditionally on every loop iteration regardless of status, which naturally blocked on I/O and prevented CPU spinning. The new code moves the Peek(1) call inside the if st == monitorStatusStart block. When status is monitorStatusIdle (between Monitor() creating the goroutine and the user calling Start()), the loop contains no blocking operation — it just checks context, reads status, and loops back immediately, consuming 100% of a CPU core.

Fix in Cursor Fix in Web

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.

monitor command breaks due to ReadTimeout

2 participants