Preserve recent node logs and fan out log streams per subscriber#55
Preserve recent node logs and fan out log streams per subscriber#55W0lfD wants to merge 1 commit into
Conversation
WalkthroughThis PR refactors backend log streaming from simple unbuffered channels to a context-aware buffered subscription model. A new ChangesLog Subscription Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/rpc/log.go (1)
15-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat closed log subscription as graceful stream completion.
Line 17 returns an error when
logChancloses, but withSubscribeLogs(stream.Context())channel closure is expected on normal cancellation and backend log buffer shutdown. This can turn valid stream termination into client-visible gRPC errors.Suggested fix
import ( - "errors" "fmt" "github.com/pasarguard/node/common" ) @@ case log, ok := <-logChan: if !ok { - return errors.New("log channel closed") + return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/rpc/log.go` around lines 15 - 18, The current handler for logs treats a closed logChan as an error; when SubscribeLogs(stream.Context()) closes the channel on normal cancellation or backend shutdown the code in the loop (checking case log, ok := <-logChan) should treat ok==false as graceful completion instead of returning an error—modify the branch to exit the function/loop and return nil (or otherwise signal successful completion) when logChan is closed so normal stream cancellation does not produce a gRPC error from this code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@controller/rpc/log.go`:
- Around line 15-18: The current handler for logs treats a closed logChan as an
error; when SubscribeLogs(stream.Context()) closes the channel on normal
cancellation or backend shutdown the code in the loop (checking case log, ok :=
<-logChan) should treat ok==false as graceful completion instead of returning an
error—modify the branch to exit the function/loop and return nil (or otherwise
signal successful completion) when logChan is closed so normal stream
cancellation does not produce a gRPC error from this code path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c74e8c9a-736b-4c82-9bad-4c0dae78e72e
📒 Files selected for processing (12)
backend/backend.gobackend/logstream/buffer.gobackend/logstream/buffer_test.gobackend/wireguard/log.gobackend/wireguard/wireguard.gobackend/wireguard/wireguard_lifecycle_test.gobackend/xray/core.gobackend/xray/log.gobackend/xray/xray.gobackend/xray/xray_test.gocontroller/rest/log.gocontroller/rpc/log.go
|
this should be in pkg directory not backend, this is not a backend |
|
|
Summary
This PR changes node log streaming from a live-only/shared-consumer model to a bounded fan-out log stream.
The node now keeps the latest log lines in an in-memory ring buffer. When a Logs client subscribes, it receives the buffered recent lines first and then continues receiving live updates. Each subscriber has its own stream, so multiple open panels no longer compete for log lines.
Problem
Before this change, the Logs page only showed lines received while the current stream was connected. This caused a few confusing and inconvenient behaviors:
This made the logs less useful exactly during the setup/debugging flow where they are most needed
Changes
logstream.BufferValidation performed:
make testSummary by CodeRabbit
Logs()method replaced with context-awareSubscribeLogs()for improved cancellation handling