Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. WalkthroughThe change adds journal-backed usage receipts for user and outbound statistics. WireGuard and Xray provide non-resetting snapshots with usage epochs. Controller, REST, and RPC methods collect and acknowledge receipts, while stats reset handling uses the journal for supported streams. ChangesUsage Receipt Collection
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Panel
participant NodeService
participant Controller
participant UsageStore as usage.Store
participant UsageBackend as usageBackend
Panel->>NodeService: CollectUsage request
NodeService->>Controller: CollectUsage
Controller->>UsageStore: Collect with snapshot callback
UsageStore->>Controller: Invoke snapshot callback
Controller->>UsageBackend: UsageSnapshot
UsageBackend-->>Controller: Epoch and stats
Controller-->>UsageStore: Epoch and stats
UsageStore-->>Controller: UsageReceipt
Controller-->>NodeService: UsageReceipt
NodeService-->>Panel: UsageReceipt
Panel->>NodeService: AcknowledgeUsage request
NodeService->>Controller: AcknowledgeUsage
Controller->>UsageStore: Acknowledge receipt ID
UsageStore-->>Controller: Acknowledgement result
Controller-->>NodeService: Empty response
NodeService-->>Panel: Empty response
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No confirmed issue prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The receipt design protects against repeated collection and delayed acknowledgements, but its accounting safeguards depend on preserving the journal throughout deployment and rollback. Losing or replacing that file could reopen destructive reads and lose the history needed to avoid duplicate accounting. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 13 files. (2 skipped: 2 unsupported.)
✨ 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. A rabbit checks the counter’s flow, Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
The current destructive stats RPC resets core counters before the panel can durably store its response. This PR adds non-destructive collection with a durable node-owned receipt: Xray/WireGuard counters are read without reset, and the cumulative baseline plus receipt commit together in a synchronous bbolt transaction before the response is published. Until ACK, all collectors receive the same ID and payload, including after a node process restart.
CollectUsage/AcknowledgeUsageare exposed through authenticated REST and gRPC handlers. Replay and ACK work even with a stopped backend. ACK only clears the matching pending receipt, so a delayed ACK cannot remove a newer sample. After a stream is activated, legacy destructive reads for that stream (including individual-user/outbound reads) fail with HTTP 412 / gRPC FailedPrecondition. Activation survives restart. Empty polls persist their baseline without creating receipts that would grow the panel ledger.Xray snapshots are fenced by a core-generation ID and lifecycle lock. WireGuard keeps cumulative interface traffic across observed kernel counter resets and preserves its legacy accounting baseline during activation. Invalid counter decreases without a new epoch fail closed. Disk/open/read/transaction errors do not reset the core. Each stream retains one pending payload; historical counter identities are retained to avoid charging a reappearing counter twice.
Measured write reduction
Unchanged operations now roll back the read-only transaction instead of committing bbolt metadata: replaying a pending receipt, a stale/duplicate ACK, and polling unchanged cumulative counters. Activation, changed baselines, new receipts and matching ACKs still require synchronous commits.
A controlled comparison against the preceding PR revision
093903dmeasured bbolt transaction-ID advancement for 100 calls of each of those four unchanged cases: 400 journal commits before, 0 after (100% fewer unnecessary commits). A subsequent changed counter still persisted and replayed after reopening the journal. This measures journal commits for unchanged calls, not overall throughput or writes for real new usage.Deployment and durability boundary
Companion panel: PasarGuard/panel#946. Deploy/release this node and the companion Python Bridge (PasarGuard/node_bridge_py#22) first, drain all legacy panel collectors, then activate the updated panel. Do not resume a legacy collector after activation.
USAGE_JOURNAL_PATHdefaults to/var/lib/pg-node/usage/receipts.db, covered by the existing compose volume. Keep that file on persistent local storage; deleting/restoring an old journal independently of the panel can lose deduplication history. bbolt file locking serializes access across processes, commits use fsync, and initial directory entries are synchronized on Unix. No asynchronous/NoSync mode is used.This removes the destructive reset-before-storage gap and provides replay for committed samples. It does not make the underlying Xray/kernel counters durable: unobserved traffic, or a core/host crash that destroys volatile counters before the first journal commit, still requires persistence in the traffic-producing core itself. No end-to-end guarantee against arbitrary source/hardware failure is claimed.
Validation
Current-head upstream Linux CI passed, including the regular suite and WireGuard kernel integration: https://github.com/PasarGuard/node/actions/runs/36256290794 .
go test -count=1 -p 1 ./...passed locally using a Go overlay solely to supply the Windows Xray path and external regression checks. No tracked tests were modified. Linux-only kernel integration is left to upstream CI.go vet ./...passed.Related open node PRs #58 and #78 were checked; neither implements a durable usage receipt protocol. No test, documentation, or benchmark files are included.