Skip to content

feat(usage): add durable receipt collection and ACK clients - #22

Open
dr-hoseyn wants to merge 1 commit into
PasarGuard:mainfrom
dr-hoseyn:codex/durable-usage-receipts
Open

dr-hoseyn wants to merge 1 commit into
PasarGuard:mainfrom
dr-hoseyn:codex/durable-usage-receipts

Conversation

@dr-hoseyn

@dr-hoseyn dr-hoseyn commented Sep 26, 2026 •

Copy link
Copy Markdown

Adds collect_usage and acknowledge_usage to both REST and gRPC clients. The node owns a stable receipt ID and retains the receipt until the caller durably stores it and explicitly acknowledges it. Repeating collection or ACK is safe; this client never falls back to destructive GetStats(reset=True) and never acknowledges automatically.

The protobuf contract and generated bindings add UsageRequest, UsageReceipt, UsageAck, CollectUsage, and AcknowledgeUsage. Existing stats APIs remain unchanged. An empty receipt ID with no stats means there is no new traffic and requires no ACK.

Companion panel: PasarGuard/panel#946. Requires the companion node receipt protocol: PasarGuard/node#94. Merge/release the node and Bridge before activating receipt accounting in the panel; drain legacy collectors before activation. The Bridge release containing these methods is a runtime prerequisite of the panel change.

Validation: existing suite 46 passed plus 5 subtests; Ruff passed. External local tests exercised both transports against the actual Go controller handlers over TLS, killed/restarted the server between collection and ACK, checked identical replay, repeated ACK, empty polls, and rejection of legacy resets. Both transport scenarios passed. Two additional real Go → Bridge → panel/SQLite recovery scenarios also passed: all 1,111 bytes were recovered once after a panel staging failure and node process restart.

No test, documentation, or benchmark files are included. This is a reliability protocol addition, not a claim of faster RPC throughput.

Upstream CI has status action_required and requires repository-maintainer approval before it can run: https://github.com/PasarGuard/node_bridge_py/actions/runs/36254487456 . It is not reported as passed; the local suite and real transport results above are the available validation. The companion node current-head Linux CI passed, including WireGuard integration.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 25e2a2d9-3dba-41b9-bbac-8c969e5fb12f

📥 Commits

Reviewing files that changed from the base of the PR and between bec9cb2 and b5e6d88.

📒 Files selected for processing (7)
  • PasarGuardNodeBridge/abstract_node.py
  • PasarGuardNodeBridge/common/service.proto
  • PasarGuardNodeBridge/common/service_grpc.py
  • PasarGuardNodeBridge/common/service_pb2.py
  • PasarGuardNodeBridge/common/service_pb2.pyi
  • PasarGuardNodeBridge/grpclib.py
  • PasarGuardNodeBridge/rest.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The bridge adds usage receipt collection and acknowledgment operations. It defines protobuf messages and RPCs, registers gRPC service methods, and adds REST and gRPC client wrappers. Receipt collection is documented as non-resetting, and acknowledgment releases a receipt after durable storage.

Changes

Usage Receipt API

Layer / File(s) Summary
Usage contracts and messages
PasarGuardNodeBridge/abstract_node.py, PasarGuardNodeBridge/common/service.proto, PasarGuardNodeBridge/common/service_pb2.py, PasarGuardNodeBridge/common/service_pb2.pyi
Adds abstract collection and acknowledgment methods and protobuf contracts for usage requests, receipts, acknowledgments, and RPCs.
gRPC service registration
PasarGuardNodeBridge/common/service_grpc.py
Registers the collection and acknowledgment methods in the service base, handler mapping, and client stub.
REST and gRPC client methods
PasarGuardNodeBridge/grpclib.py, PasarGuardNodeBridge/rest.py
Adds client methods for collecting receipts and acknowledging them. Reformats lifecycle lease release calls without changing their arguments.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Suggested reviewers: m03ed

Merge Risk: ⚪ Minimal · up to b5e6d

The receipt APIs appear ready to merge after normal checks; no actionable issue is established by the supplied evidence.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to b5e6d

The new client calls use the existing authenticated connections, and no new authentication bypass was found. Receipt accounting nevertheless depends on compatible node behavior, durable panel storage, and a coordinated activation sequence that this client change cannot enforce.

Retained concerns

  • Medium · architecture · inferred: Receipt accounting is conditional on a compatible node retaining and validating receipts, a panel committing them before ACK, and legacy collectors being drained. The Bridge exposes the calls but cannot enforce that cross-component activation sequence; early or mixed-version activation could interrupt or undermine accounting.
Security review details

Security Blast Radius

  • inferred — A caller able to use a configured Bridge node client can request usage and submit a receipt ID for ACK through that client's existing credentials. The visible exposure is its configured node connection; node-side authorization, tenant isolation, and the scope of retained receipts cannot be determined here.

Security Findings and Attack Paths

  • inferred — No PR-introduced authentication bypass or verified receipt-clearing attack is established by the client forwarding code. Whether an incorrect, stale, or repeated ACK can release node-owned state is determined by the unavailable authoritative handler, not by the generated server declaration.

Trust Boundaries and Controls

  • observed — REST sends the new requests through the existing HTTPS client with an x-api-key header. gRPC uses the existing TLS channel and passes the same form of API-key metadata through its common request handler. These client controls do not establish what the remote handler checks before releasing a receipt.

Resilience and Maintainability Implications

  • inferred — Accounting integrity across failed staging, lost responses, repetition, concurrent callers, and restart relies on persistent receipt identity, commit-before-ACK ordering, and idempotent node transitions. The Bridge preserves an explicit ACK boundary but does not enforce those external invariants.

Hardening Proposals

  • proposed — Before activating receipt accounting, verify against the deployed node and panel that receipt ownership survives restart, concurrent and repeated collection replays the same pending receipt, stale ACKs cannot clear newer state, panel storage commits before ACK, and mixed-version or rollback states cannot resume destructive legacy collection.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 6 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding usage receipt collection and acknowledgment clients with durable receipt handling.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 6 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

A rabbit checks the receipt by moonlit glow,
The counters stay where they were, steady below.
A message carries stats along the way,
An ack releases it when stored to stay.
Then off the rabbit hops through clover green.

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

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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