[wahh3b-lgtm]

Summary

This PR implements 5 targeted performance optimizations for PasarGuard Node, designed for deployment on small 1-Core / 2GB RAM VPS instances where CPU spikes, GC pressure, and lock contention degrade performance under high traffic.

Changes

1. Non-blocking bandwidth sampling (pkg/sysstats/sys.go)

• Removed the 1-second time.Sleep inside getBandwidthSpeed() that blocked the stats collection goroutine
• Replaced with persistent cumulative counters + elapsed-time delta computation
• Eliminates CPU wait-states; reduces effective stats cycle from ~2.5s to 1.5s

2. Lock-free config serialization (backend/xray/config.go)

• ToBytes() no longer holds read locks on ALL inbound mutexes during JSON marshaling
• Each inbound is snapshotted individually under a microsecond lock/unlock
• Heavy json.Marshal runs on a detached copy with zero locks held
• Prevents concurrent SyncUser API calls from being blocked by serialization

3. sync.Pool for scanner buffers (backend/xray/log.go)

• Added sync.Pool that recycles the 64KB bufio.Scanner buffer across Xray restarts
• Eliminates one 64KB heap allocation per Start() invocation
• Reduces GC pressure on memory-constrained instances

4. Conditional orphan process scan (backend/xray/core.go)

• Added cleanExit flag to Core struct
• Skips the expensive /proc or ps-based global process scan when Xray exited cleanly
• Scan only runs after unexpected crashes, avoiding CPU spikes on normal restarts

5. Eliminated unused context allocation (controller/controller.go)

• Removed redundant context.WithCancel() in New() that was immediately discarded
• Added nil-guard on cancelFunc in Disconnect() for safe pre-Connect teardown

Test Results

• go build ./... compiles successfully (exit code 0)
• 74 unit tests pass across xray/api, wireguard, controller, pkg/stats
• 3 pre-existing test failures (missing Xray binary + TLS certs on dev machine) are unrelated to this PR

Impact

Metric	Before	After
Stats collection cycle	~2.5s (1.5s interval + 1s sleep)	~1.5s (non-blocking)
Config serialization lock hold	Duration of json.Marshal	Microsecond snapshot
Scanner buffer allocation per Start()	64KB heap alloc	Reused via sync.Pool
Orphan process scan	Every Start()	Only on abnormal exit

Summary by CodeRabbit

• Bug Fixes
  • Fixed potential crash when disconnecting before establishing a connection.
  • Improved process exit handling by distinguishing expected vs. unexpected exits.
  • Updated Xray configuration generation to prevent unintended changes during serialization.
• Performance
  • Improved log processing efficiency with pooled buffering and non-blocking log delivery.
  • Refined bandwidth speed calculation to use a stateful sampler, avoiding blocking delays.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 45e5fe6d-1571-4886-8c17-20ac97b23c1e

---

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

This PR makes five independent improvements: refactors Xray config serialization to use read-locked snapshots instead of mutating live Inbound state; adds a cleanExit flag to Core that gates orphan-process cleanup; switches scanner buffers to a sync.Pool and fixes startup log routing; adds a nil guard to Controller.Disconnect; and replaces the blocking 1-second bandwidth sampler with a stateful non-blocking one.

Changes

Backend and System Improvements

Layer / File(s)	Summary
Non-mutating Xray config serialization backend/xray/config.go	Adds snapshotInboundForSerialization to copy Inbound fields under read lock, and rewrites ToBytes() to marshal a temporary snapshot config, avoiding mutation of live inbound Settings.
cleanExit flag for orphan-process cleanup gating backend/xray/core.go	Adds cleanExit bool to Core; Start skips cleanupOrphanedProcesses() and resets the flag when it is true; handleProcessExit sets it on expected exits.
Scanner buffer pooling and startup log routing fix backend/xray/log.go	Introduces a sync.Pool of byte slices for scanner buffers in captureProcessLogs; fixes captureStartupLogLine to non-blockingly send to logsChan and then call detectLogType, consistent with the runtime log path.
Controller Disconnect nil-guard controller/controller.go	Removes pre-initialization of cancelFunc in New and adds a nil check in Disconnect to prevent a nil dereference before Connect is called.
Stateful non-blocking bandwidth sampler pkg/sysstats/sys.go	Replaces the blocking 1-second two-sample getBandwidthSpeed() with a stateful sampler using package-level locked state; returns 0, 0 on first call and byte-per-second rates on subsequent calls; GetSystemStats drops error handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main purpose of the changeset: implementing 5 targeted performance optimizations across multiple files (config serialization, bandwidth sampling, buffer pooling, orphan process cleanup, and context allocation).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{🐇 No more blocking for a second just to measure the byte,
A snapshot keeps the live state safe from JSON's might.
Clean exits skip the orphan hunt with a simple bool,
Pooled buffers share their memory — efficiency's rule!
And nil guards shield the disconnect from crashing in the night. ✨}

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

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

backend/xray/config.go (1)

327-330: 💤 Low value

Shallow copy of Settings map shares nested references.

The for-range copy creates a new map but values remain shared references. If any value is a mutable type (nested map/slice) that gets modified concurrently elsewhere, the snapshot could observe partial updates. This appears acceptable given that Settings values are typically primitives or read-only config data, but worth noting if the data model evolves.

🤖 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 `@backend/xray/config.go` around lines 327 - 330, The for-range copy of
inbound.Settings creates a new map but leaves values as shared references, which
could cause issues if any value is a mutable type (nested map/slice) that gets
modified concurrently. While this appears acceptable since Settings values are
typically primitives or read-only config data, add a comment above the
snap.Settings map copy loop in the range assignment block explaining this
shallow copy behavior and the conditions under which it would need to be changed
to a deep copy (i.e., if the data model evolves to include mutable nested
types).

🤖 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.

Inline comments:
In `@backend/xray/config.go`:
- Around line 425-428: Add a nil check for snapConfig.LogConfig before
attempting to access the AccessLog and ErrorLog fields on lines 425-428. Wrap
the code block that accesses snapConfig.LogConfig.AccessLog and
snapConfig.LogConfig.ErrorLog with a guard condition that verifies
snapConfig.LogConfig is not nil first. This prevents a nil dereference panic
when the parsed config JSON lacks a log section.
- Around line 425-433: The snapConfig.LogConfig pointer references the live
c.LogConfig object, causing mutations during serialization to race with
concurrent readers and creating a nil dereference risk if c.LogConfig is not
initialized. Replace the approach of temporarily blanking the AccessLog and
ErrorLog fields by instead creating a copy of the LogConfig struct within the
snapshot with the log fields set to empty strings. Add a nil guard to check if
c.LogConfig exists before attempting to copy it, returning appropriate handling
(such as using an empty LogConfig struct or skipping the copy) when c.LogConfig
is nil. This eliminates both the race condition and the potential panic from
dereferencing a nil LogConfig pointer.

In `@controller/controller.go`:
- Around line 68-70: The `Disconnect()` method reads `c.cancelFunc` without
holding the lock `c.mu`, while the `Connect()` method writes to `c.cancelFunc`
under lock synchronization. This creates a data race on concurrent access. Wrap
the read of `c.cancelFunc` in the `Disconnect()` method (the nil check and the
call to `c.cancelFunc()`) by acquiring `c.mu` lock before the access and
releasing it after, matching the synchronization pattern used in `Connect()` to
ensure thread-safe access to this shared field.

---

Nitpick comments:
In `@backend/xray/config.go`:
- Around line 327-330: The for-range copy of inbound.Settings creates a new map
but leaves values as shared references, which could cause issues if any value is
a mutable type (nested map/slice) that gets modified concurrently. While this
appears acceptable since Settings values are typically primitives or read-only
config data, add a comment above the snap.Settings map copy loop in the range
assignment block explaining this shallow copy behavior and the conditions under
which it would need to be changed to a deep copy (i.e., if the data model
evolves to include mutable nested types).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7c115af5-bfc3-4639-b127-91462ae04ae8

📥 Commits

Reviewing files that changed from the base of the PR and between e0031a2 and 3117956.

📒 Files selected for processing (5)

• backend/xray/config.go
• backend/xray/core.go
• backend/xray/log.go
• controller/controller.go
• pkg/sysstats/sys.go

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@controller/controller.go`:
- Around line 68-84: The Disconnect() method has duplicate code blocks that
cause compilation errors. Remove the second backend variable declaration block
starting at line 81 (the duplicate c.mu.Lock(), backend := c.backend,
c.mu.Unlock() sequence) to eliminate the duplicate `:=` assignment of the
backend variable in the same scope and complete the Disconnect() function
properly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: da52e867-2f31-426c-a11c-5a9bad2d1be9

📥 Commits

Reviewing files that changed from the base of the PR and between 3117956 and a8e64b3.

📒 Files selected for processing (1)

• controller/controller.go

[coderabbitai]

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@controller/controller.go`:
- Around line 68-84: The Disconnect() method has duplicate code blocks that
cause compilation errors. Remove the second backend variable declaration block
starting at line 81 (the duplicate c.mu.Lock(), backend := c.backend,
c.mu.Unlock() sequence) to eliminate the duplicate `:=` assignment of the
backend variable in the same scope and complete the Disconnect() function
properly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: da52e867-2f31-426c-a11c-5a9bad2d1be9

📥 Commits

Reviewing files that changed from the base of the PR and between 3117956 and a8e64b3.

📒 Files selected for processing (1)

• controller/controller.go

🛑 Comments failed to post (1)

controller/controller.go (1)

68-84: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Remove duplicated Disconnect() block — current code does not compile.

At Line 68, Disconnect() is declared a second time inside itself, which causes the parser error reported by typecheck. Also, Line 83 re-declares backend with := in the same scope as Line 79, which is another compile-time failure. Keep only one function header and one backend snapshot block.

Proposed minimal fix

 func (c *Controller) Disconnect() {
-func (c *Controller) Disconnect() {
 	c.mu.Lock()
 	cancel := c.cancelFunc
 	c.cancelFunc = nil
 	c.mu.Unlock()

 	if cancel != nil {
 		cancel()
 	}

 	c.mu.Lock()
 	backend := c.backend
 	c.mu.Unlock()
-
-	c.mu.Lock()
-	backend := c.backend
-	c.mu.Unlock()

 	// Shutdown backend outside of lock to avoid deadlock
 	// Shutdown() will wait for process termination to complete
 	if backend != nil {
 		backend.Shutdown()

🧰 Tools

🪛 golangci-lint (2.12.2)

[error] 68-68: : # github.com/pasarguard/node/controller [github.com/pasarguard/node/controller.test]
controller/controller.go:68:35: syntax error: unexpected { at end of statement

(typecheck)

---

[error] 68-68: expected ';', found '{'

(typecheck)

🤖 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/controller.go` around lines 68 - 84, The Disconnect() method has
duplicate code blocks that cause compilation errors. Remove the second backend
variable declaration block starting at line 81 (the duplicate c.mu.Lock(),
backend := c.backend, c.mu.Unlock() sequence) to eliminate the duplicate `:=`
assignment of the backend variable in the same scope and complete the
Disconnect() function properly.

Source: Linters/SAST tools

[T3ST3ST3R0N]

Thanks for this, @wahh3b-lgtm! dev had moved a lot since you opened it, so we finished it on your branch:

• Kept the ToBytes snapshot. Besides less locking, it fixes a real crash risk: a user sync clones the live config while a health-check restart serializes it, and dev's ToBytes wrote the very settings maps the clone reads (Go can abort with "concurrent map read and map write"). Two fixes in it: excluded inbounds are serialized unchanged again (the snapshot replaced their hand-written clients with an empty list), and Xray WireGuard inbounds, added on dev since, get their users as peers. Added tests for both, a -race test for serializing while cloning, and a test that fails if a new Inbound field is not copied into the snapshot.
• Kept the scanner buffer pool.
• Carried your idea of never subtracting a larger counter from a smaller one over to dev's bandwidthDelta, which still wrapped around when an interface was recreated between samples; it now counts the traffic since the reset (with a test).
• Left out the sysstats and Disconnect changes, which dev already covers, and the clean-exit skip of the orphan scan: the exit handler of a replaced process can run after the next start, so the flag could end up describing the wrong process, and the scan is cheap.

Thanks again!