feat(subscription): configure client routing and automatic server groups - #759
feat(subscription): configure client routing and automatic server groups#759Rerowros wants to merge 20 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds validated Xray and Sing-box subscription profiles, profile delivery and preview endpoints, host classification metadata, multi-worker NATS coordination, WireGuard reconciliation changes, administrator group-access checks, dashboard editing, documentation, and tests. ChangesSubscription profile flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 maps each profile pool, Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces opt-in “subscription profiles” that generate a single full client configuration (Xray or Sing-box) by grouping all eligible user endpoints into pools/countries with automatic selection logic, while keeping legacy subscription generation unchanged. It also adds backend validation and new public/admin endpoints plus dashboard UI to create templates, classify hosts, and preview per-user profile output.
Changes:
- Add backend profile schema/validation and generators for Xray/Sing-box, plus public download and admin preview endpoints.
- Extend host subscription data to include stable host identity and profile classification fields used for grouping.
- Add dashboard support for new template types, host classification fields, and a per-user profile preview modal (with request abort support).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_subscription_profiles.py |
Adds unit tests and optional executable-backed validators for generated profiles. |
docs/subscription-profiles.md |
Documents profile creation, endpoint classification, and output behavior/limitations. |
dashboard/src/service/http.ts |
Adds AbortSignal plumbing to orvalFetcher for cancellable requests. |
dashboard/src/service/api/index.ts |
Extends generated API types and adds profile preview/download API hooks. |
dashboard/src/features/users/dialogs/user-subscription-profile-preview-modal.tsx |
New UI modal to preview generated per-user profile JSON. |
dashboard/src/features/users/components/action-buttons.tsx |
Adds action to open the profile preview modal (RBAC-gated). |
dashboard/src/features/templates/forms/client-template-form.ts |
Allows new profile template types and adds default starter JSON for both. |
dashboard/src/features/templates/dialogs/client-template-modal.tsx |
Adds labels for new template types. |
dashboard/src/features/templates/components/use-client-templates-list-columns.tsx |
Displays new template types in templates list. |
dashboard/src/features/hosts/forms/host-form.ts |
Adds form schema for host profile classification fields. |
dashboard/src/features/hosts/dialogs/host-modal.tsx |
Adds UI inputs for host profile classification and preserves them when clearing Xray template. |
dashboard/src/features/hosts/components/hosts-list.tsx |
Maps subscription_templates.profile into form defaults. |
dashboard/public/statics/locales/{en,zh,ru,fa}.json |
Adds i18n strings for the profile preview modal. |
app/subscription/share.py |
Adds generate_subscription_profile() entrypoint (legacy generation remains separate). |
app/subscription/profiles.py |
Implements profile parsing, endpoint tagging/grouping, and Xray/Sing-box profile builders. |
app/routers/user.py |
Adds admin endpoint to preview a user-specific generated profile. |
app/routers/subscription.py |
Adds public endpoint to download a profile by token/profile ID. |
app/operation/subscription.py |
Adds profile template lookup + profile generation path for public/admin endpoints. |
app/operation/client_template.py |
Validates profile template content and relaxes “cannot delete last template” for non-legacy types. |
app/models/subscription.py |
Adds host_id, is_disabled, and profile_classification to inbound model. |
app/models/subscription_profile.py |
New Pydantic schema for machine-readable profile definitions. |
app/models/host.py |
Adds HostProfileClassification and includes it in SubscriptionTemplates. |
app/models/client_template.py |
Adds xray_profile / singbox_profile template types. |
app/db/crud/client_template.py |
Ensures only legacy template types become “system” on first creation. |
app/core/hosts.py |
Populates host_id, is_disabled, and profile_classification into subscription inbounds. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
app/models/host.py (1)
577-583: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one machine-identifier pattern between the two models.
The regex at line 581 duplicates
PROFILE_ID_PATTERNinapp/models/subscription_profile.pyline 16. The two patterns must stay identical, because a hostpoolvalue must match a profileProfilePool.idvalue for_grouped_endpointsinapp/subscription/profiles.pyto place the endpoint. Import the shared constant instead of restating the expression.🤖 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 `@app/models/host.py` around lines 577 - 583, Update validate_pool in the host model to import and reuse PROFILE_ID_PATTERN from subscription_profile.py instead of defining a duplicate regex, preserving the existing normalization and validation behavior so pool values match ProfilePool.id.
🤖 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 `@app/models/host.py`:
- Around line 557-559: Fix the undefined annotation in
SubscriptionTemplates.profile by defining HostProfileClassification before
SubscriptionTemplates, or by converting the annotation to a quoted forward
reference. Ensure app.models.host imports successfully without changing the
field’s type or behavior.
- Line 573: Update the country field validation and validate_country flow so
whitespace-only or padded inputs are stripped and converted to None before
min_length/max_length constraints run. Ensure non-empty values are normalized
before ISO-country validation, while preserving the optional None behavior and
two-character country-code requirements.
In `@app/models/subscription_profile.py`:
- Around line 56-58: Update the profile-building flow in
app/subscription/profiles.py so client and happ_deeplink are either consumed to
produce client-specific output and behavior or removed from the accepted profile
contract. Replace engine-agnostic routing_rules handling with per-engine
validation and mapping: validate rule shapes in _validate_template_content based
on the template type, and ensure build_xray_profile and build_singbox_profile
emit only schemas accepted by their respective engines.
In `@app/routers/user.py`:
- Around line 461-469: Add a server-side dependency requiring
client_templates/read_simple to get_user_subscription_profile_preview, while
preserving the existing users/read requirement and endpoint behavior. Add a
regression test verifying that an administrator with only users/read cannot
retrieve the rendered profile preview.
In `@app/subscription/profiles.py`:
- Around line 236-246: Update app/subscription/profiles.py lines 236-246 to use
profile.health_check.idle_timeout for the Sing-box urltest idle_timeout field.
In app/models/subscription_profile.py lines 26-31, rename timeout to
idle_timeout, default it to 30m, and allow the h unit in validation. In
dashboard/src/features/templates/forms/client-template-form.ts lines 193-224,
replace timeout: '5s' with idle_timeout: '30m' in both xray_profile and
singbox_profile defaults.
- Around line 256-262: Remove the legacy {"type": "block", "tag": "block"}
outbound from the list built by the profile generation flow around
outbounds.extend. Keep the selector and direct outbounds unchanged, and do not
add a replacement outbound since no route rule references block.
- Around line 100-111: Update _grouped_endpoints to require only the default
pool to have eligible endpoints, omit empty enabled pools from the returned
groups, and preserve the default-pool validation. Change both callers to consume
the returned grouping instead of recomputing enabled pools and filtering
endpoints. In build_xray_profile, guard the pool_tags[pool.fallback_pool][0]
lookup so an empty fallback pool cannot be indexed.
- Around line 127-131: Update the tags mapping in _xray_outbounds so dsdialer
uses a distinct pg-dsdialer-{...} prefix instead of pg-dialer-{...}, while
preserving the existing suffix removal. Ensure this prefix remains outside the
obseryatory.subjectSelector ["pg-proxy-"] match and leaves the regular dialer
tag unchanged.
In `@app/subscription/share.py`:
- Around line 157-161: Update the sing_box branch in the profile serialization
function to pass the same default=str option to json.dumps as the xray branch,
ensuring both build_singbox_profile and build_xray_profile outputs use
consistent serialization behavior.
In `@dashboard/src/features/hosts/dialogs/host-modal.tsx`:
- Around line 1035-1074: The new client profile classification UI in the host
modal contains hard-coded labels and helper text. Update the visible text around
the FormLabel and exclusion description to use the dashboard’s existing
translation hook/key conventions, including the section title, Pool, Country,
Exclude from automatic groups, and its explanatory text, while preserving the
current form fields and controls.
- Around line 1035-1081: Expose the missing
subscription_templates.profile.priority field in the “Client profile
classification” panel alongside pool and country, using the existing
FormField/FormItem pattern and form control. Ensure administrators can view and
edit the profile priority value while preserving the existing fields and layout.
In `@dashboard/src/features/templates/forms/client-template-form.ts`:
- Around line 193-224: Update the default configurations for
ClientTemplateType.xray_profile and ClientTemplateType.singbox_profile to avoid
declaring an unused fallback pool: use a single primary pool and remove its
fallback_pool reference, while preserving the existing health_check,
routing_rules, and client settings.
In `@docs/subscription-profiles.md`:
- Around line 53-55: Update the fenced path block in the subscription profile
documentation to declare the `text` language on its opening fence, preserving
the path content unchanged.
---
Nitpick comments:
In `@app/models/host.py`:
- Around line 577-583: Update validate_pool in the host model to import and
reuse PROFILE_ID_PATTERN from subscription_profile.py instead of defining a
duplicate regex, preserving the existing normalization and validation behavior
so pool values match ProfilePool.id.
🪄 Autofix
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 Plus
Run ID: ff94b4f9-4266-408c-a6de-4ff1789a885f
📒 Files selected for processing (28)
app/core/hosts.pyapp/db/crud/client_template.pyapp/models/client_template.pyapp/models/host.pyapp/models/subscription.pyapp/models/subscription_profile.pyapp/operation/client_template.pyapp/operation/subscription.pyapp/routers/subscription.pyapp/routers/user.pyapp/subscription/profiles.pyapp/subscription/share.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/features/hosts/components/hosts-list.tsxdashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/hosts/forms/host-form.tsdashboard/src/features/templates/components/use-client-templates-list-columns.tsxdashboard/src/features/templates/dialogs/client-template-modal.tsxdashboard/src/features/templates/forms/client-template-form.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/features/users/dialogs/user-subscription-profile-preview-modal.tsxdashboard/src/service/api/index.tsdashboard/src/service/http.tsdocs/subscription-profiles.mdtests/test_subscription_profiles.py
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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
`@dashboard/src/features/subscriptions/components/subscription-rule-advanced-sheet.tsx`:
- Around line 81-98: Give the profile ID Input in
subscription-rule-advanced-sheet.tsx an accessible name by associating it with
the visible label or adding an aria-label. In subscription-profile-editor.tsx,
add an accessible name such as “Remove routing rule” to the icon-only button.
In `@dashboard/src/features/templates/components/subscription-profile-editor.tsx`:
- Around line 33-42: Update SubscriptionProfileEditor and ClientTemplateModal so
the routing-rule draft validation state is propagated to the modal and saving is
rejected while the draft contains invalid JSON or a non-object rule. Preserve
the draft display and existing valid-object update behavior, and clear the
invalid state once parsing succeeds.
In `@dashboard/src/features/templates/forms/subscription-profile-form.ts`:
- Around line 34-61: The subscription profile schema must enforce the backend
Happ deeplink contract. In the superRefine callback of
parseSubscriptionProfileContent, require any present happ_deeplink to use an
accepted happ://routing/add/ or happ://routing/onadd/ prefix and require client
to equal 'happ', adding validation issues for invalid URIs or non-Happ clients.
In dashboard/src/features/templates/forms/subscription-profile-form.ts lines
34-61, implement these checks; in
dashboard/src/features/templates/forms/subscription-profile-form.test.ts lines
24-35, replace happ://profile with a valid routing URI and add rejection cases
for an invalid URI and a deeplink with a non-Happ client.
- Around line 96-115: Update the profile validation before the mapping in the
subscription profile normalization flow to verify every entry in `profile.pools`
is a non-null object. Reject any invalid pool entry with the existing
unsuccessful validation response so malformed values such as null reach Raw JSON
repair instead of causing `pool.enabled` access in the pools mapping to throw.
In `@docs/subscription-profiles.md`:
- Around line 64-72: Update docs/subscription-profiles.md lines 64-72 to remove
the claim that Xray generates pg-select-<pool> choices, and state that selecting
pools or countries requires routing rules. Also update
docs/subscription-profiles.md lines 86-90 to state that Sing-box creates
selectors only for nonempty pools and creates urltest groups only when automatic
endpoints exist.
🪄 Autofix
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 Plus
Run ID: 119002b0-10f7-4a38-ae43-4817a525727d
📒 Files selected for processing (18)
app/models/settings.pyapp/models/subscription_profile.pyapp/operation/subscription.pyapp/subscription/profiles.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/ru.jsondashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/subscriptions/components/sortable-subscription-rule.tsxdashboard/src/features/subscriptions/components/subscription-rule-advanced-sheet.tsxdashboard/src/features/subscriptions/components/subscription-settings-schema.tsdashboard/src/features/templates/components/subscription-profile-editor.tsxdashboard/src/features/templates/dialogs/client-template-modal.tsxdashboard/src/features/templates/forms/subscription-profile-form.test.tsdashboard/src/features/templates/forms/subscription-profile-form.tsdashboard/src/service/api/index.tsdocs/subscription-profiles.mdtests/api/test_subscription_profile_api.pytests/test_subscription_profiles.py
🚧 Files skipped from review as they are similar to previous changes (5)
- dashboard/src/features/hosts/dialogs/host-modal.tsx
- app/models/subscription_profile.py
- dashboard/src/features/templates/dialogs/client-template-modal.tsx
- app/subscription/profiles.py
- dashboard/src/service/api/index.ts
7d90a09 to
4ecfead
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
app/app_factory.py (1)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the truncated comment.
The comment on Line 81 ends with a semicolon and no conclusion. State why non-node roles register the ignore handler.
🤖 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 `@app/app_factory.py` around lines 76 - 82, Complete the comment above _ignore_worker_sync_message to explain that non-node roles still subscribe to worker_sync messages but intentionally ignore them through the registered handler.tests/test_nats_leader_steal.py (1)
10-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the leader module globals after each test.
These tests set
leader._is_leaderandleader._tokendirectly and never restore them.leader._is_leaderstaysTrueafter the first test, so any later test in the same session that callsleader.is_job_leader()observes leaked state.tests/test_nats_leader_heartbeat.pyprotects itself with an autouse fixture; add the same guard here.♻️ Proposed fixture
from app.nats import leader from role import Role +@pytest.fixture(autouse=True) +def _reset_leader_state(): + yield + leader._is_leader = False + leader._token = None + leader._kv = None + + `@pytest.mark.asyncio`Also applies to: 44-59
🤖 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 `@tests/test_nats_leader_steal.py` around lines 10 - 30, Add an autouse fixture in the test module that snapshots and restores leader._is_leader and leader._token around every test, matching the cleanup pattern used by the heartbeat tests. Apply it to both leader-steal tests so try_become_leader state cannot leak between tests.app/db/crud/wireguard.py (1)
286-307: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd
.distinct()to the accessible-tags statement.A user who reaches the same inbound tag through several enabled groups produces one row per group. The callers deduplicate in Python, so the extra rows only add transfer and parsing cost. The tag-filtered variant now runs over all users, so the row count matters more than before.
♻️ Proposed change
.join(ProxyInbound, ProxyInbound.id == inbounds_groups_association.c.inbound_id) + .distinct() )🤖 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 `@app/db/crud/wireguard.py` around lines 286 - 307, Update _accessible_tags_stmt to apply distinct() to the constructed select before returning it, including both unfiltered and conditionally filtered variants, so each user/tag pair is returned only once.app/nats/__init__.py (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
is_multi_workerin the router.
app/nats/router.pylines 15-17 recompute the same expression inline in_router_enabled. Importis_multi_workerthere so the multi-worker rule stays defined in one place.🤖 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 `@app/nats/__init__.py` around lines 14 - 16, Update `_router_enabled` in `app/nats/router.py` to import and call `is_multi_worker()` from `app.nats` instead of recomputing `runtime_settings.role.requires_nats or server_settings.workers > 1` inline. Keep the existing router behavior unchanged while centralizing the multi-worker rule in `is_multi_worker`.app/nats/leader.py (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not rely on a module-level
assertfor the timing invariant.Python removes
assertstatements when it runs with-O. The invariant then disappears at runtime. The testtest_heartbeat_retry_budget_fits_in_leasealready covers this constant relationship, so either keep the check only in tests or raise an explicit error.♻️ Proposed change
-# Worst-case time from last successful renew to concede must stay under the lease. -assert HEARTBEAT_INTERVAL + (HEARTBEAT_MAX_RETRIES - 1) * HEARTBEAT_RETRY_DELAY < DEFAULT_LEASE_SECONDS +# Worst-case time from last successful renew to concede must stay under the lease. +if HEARTBEAT_INTERVAL + (HEARTBEAT_MAX_RETRIES - 1) * HEARTBEAT_RETRY_DELAY >= DEFAULT_LEASE_SECONDS: + raise RuntimeError("Heartbeat retry budget exceeds the leader lease duration")🤖 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 `@app/nats/leader.py` around lines 29 - 30, Replace the module-level assert guarding the heartbeat timing invariant in app/nats/leader.py with an explicit startup-time validation that raises an appropriate error, or remove it from production code and rely on test_heartbeat_retry_budget_fits_in_lease. Do not use assert for this invariant, since it must remain enforced when Python runs with optimization enabled.tests/test_nats_leader_heartbeat.py (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePatching
asyncio.sleepchanges the stdlib module globally.
leader.asynciois the realasynciomodule, so this replacesasyncio.sleepfor the whole process while the test runs.monkeypatchrestores it afterwards, so the blast radius is limited, but any other coroutine scheduled during this test also receives the no-op stub. Consider recording the delays through a wrapper that still yields control, or assert on the delay values with a module-local sleep indirection inapp/nats/leader.py.🤖 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 `@tests/test_nats_leader_heartbeat.py` around lines 99 - 109, Update the heartbeat test around _heartbeat_loop and its sleep patch so it does not replace the shared asyncio.sleep function; use a module-local sleep indirection in app/nats/leader.py and patch that symbol, or wrap the real sleep while recording delays and still yielding control. Preserve the existing assertion that the recorded delays are [5.0, 1.0, 1.0].tests/test_nats_node_memory.py (1)
76-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test double with real NATS KV error behavior.
These tests run only against
MemoryCasKv.MemoryCasKv.createraisesKeyWrongLastSequenceErroron an existing key, while nats-pyKeyValue.createraisesKeyAlreadyExistsError. The suite therefore cannot detect the create-conflict gap flagged inapp/nats/kv_cas.pyat Lines 39-49. After you resolve that gap, make the double raise the same exception type as the real client so this coverage stays meaningful.🤖 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 `@tests/test_nats_node_memory.py` around lines 76 - 93, Update MemoryCasKv.create to raise nats-py’s KeyAlreadyExistsError when the key already exists, matching the real KeyValue.create behavior instead of KeyWrongLastSequenceError. Preserve successful creation semantics and ensure tests covering NatsUserSyncStore claim/create conflicts exercise the production exception path.app/nats/kv_cas.py (1)
61-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass the prefix to
kv.keys()instead of listing the whole bucket.
kv_list_keysignores thefiltersparameter declared inCasKv.keys. It fetches every key in the bucket and filters in Python.NatsUserSyncStore.claim_userscalls this on each claim, so cost grows with total pending users across all nodes, not with one node. Use the server-side filter and keep the client-side prefix check as a guard.♻️ Proposed refactor
async def kv_list_keys(kv: CasKv, prefix: str) -> list[str]: try: - keys = await kv.keys() + keys = await kv.keys(filters=[f"{prefix}>"]) except nats_js_errors.NoKeysError: return [] return [key for key in keys if key.startswith(prefix)]Confirm the subject-filter syntax accepted by the installed nats-py
KeyValue.keys, and alignMemoryCasKv.keysmatching with it.🤖 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 `@app/nats/kv_cas.py` around lines 61 - 66, Update kv_list_keys to pass prefix as the server-side filter when calling CasKv.keys, while retaining the client-side startswith check as a guard. Confirm the installed nats-py KeyValue.keys subject-filter syntax, and update MemoryCasKv.keys to apply matching semantics consistently.app/node/__init__.py (1)
49-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed shutdown error.
_shutdown_nodediscards every exception.remove_noderuns it in a detached task, so a failure to stop a remote core leaves no trace. Log at debug level to keep diagnosis possible.Also keep a reference to the task created at Line 82. An unreferenced task can be garbage collected before it completes.
♻️ Proposed refactor
async def _shutdown_node(self, node: PasarGuardNode | None, *, remote_stop: bool = True): if node is None: return try: await node.set_health(Health.INVALID) if remote_stop: await node.stop() - except Exception: - pass + except Exception as exc: + self.logger.debug("Node shutdown failed (remote_stop=%s): %s", remote_stop, exc)For the detached task, store it in a set owned by
NodeManagerand discard it on completion:- asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + task = asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + self._pending_shutdowns.add(task) + task.add_done_callback(self._pending_shutdowns.discard)Add
self._pending_shutdowns: set[asyncio.Task] = set()in__init__.🤖 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 `@app/node/__init__.py` around lines 49 - 58, Update NodeManager’s _shutdown_node to catch the exception as err and log it at debug level instead of silently swallowing it. In remove_node, retain the detached shutdown task in a NodeManager-owned _pending_shutdowns set, and register completion cleanup so finished tasks are discarded while shutdown continues safely.app/node/nats_memory.py (1)
119-128: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNote the per-user round-trip cost of the KV queue.
enqueue_usersperforms onekv_put_jsonper user, and eachkv_put_jsonperforms at least onegetplus onecreate/update. A full user sync of N users therefore costs at least 2N KV round trips.claim_usersadds a_requeue_expired_claimsscan plus agetper pending key on every call.This path runs on node user synchronization, so the cost scales with panel size. Consider batching per node into bounded chunks, or skipping the
getinkv_put_jsonby attemptingcreatefirst. The list-scan part shares a root cause withkv_list_keysinapp/nats/kv_cas.py.Also applies to: 149-193
🤖 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 `@app/node/nats_memory.py` around lines 119 - 128, Reduce KV round trips in enqueue_users by updating kv_put_json to attempt create first and only perform an update when the key already exists, preserving overwrite semantics. Also optimize claim_users and _requeue_expired_claims to avoid fetching each key individually where the KV API supports batched operations, and apply the same bounded/list-scan improvement to kv_list_keys.
🤖 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 `@app/db/crud/wireguard.py`:
- Around line 333-344: Update _peer_ips_present_clause to use dialect-correct
JSON array emptiness checks instead of comparing peer_ips.as_string() with "[]".
Preserve the non-null requirement and ensure _user_ids_with_peer_ips returns
only users whose wireguard.peer_ips array contains at least one entry across
PostgreSQL, MySQL, and SQLite.
In `@app/nats/leader.py`:
- Around line 60-71: Update _ensure_kv and the start_job_leader renewal/reclaim
flow to detect a closed or disconnected _nc before returning the cached _kv.
When the connection is unavailable, fail the current renewal attempt and trigger
the existing NATS reconnect path instead of calling _concede_leadership("renewal
exhausted") or returning a stale KV handle; continue retrying until recovery or
lease expiration.
In `@app/node/nats_memory.py`:
- Around line 436-444: Update shutdown_bridge_memory so all module globals are
reset even when _nc.close() raises. Suppress the close error using the same
handling pattern already used by ensure_bridge_memory, while ensuring _nc,
_user_sync_kv, _lifecycle_kv, _user_sync_store, and _lifecycle_coordinator are
cleared.
---
Nitpick comments:
In `@app/app_factory.py`:
- Around line 76-82: Complete the comment above _ignore_worker_sync_message to
explain that non-node roles still subscribe to worker_sync messages but
intentionally ignore them through the registered handler.
In `@app/db/crud/wireguard.py`:
- Around line 286-307: Update _accessible_tags_stmt to apply distinct() to the
constructed select before returning it, including both unfiltered and
conditionally filtered variants, so each user/tag pair is returned only once.
In `@app/nats/__init__.py`:
- Around line 14-16: Update `_router_enabled` in `app/nats/router.py` to import
and call `is_multi_worker()` from `app.nats` instead of recomputing
`runtime_settings.role.requires_nats or server_settings.workers > 1` inline.
Keep the existing router behavior unchanged while centralizing the multi-worker
rule in `is_multi_worker`.
In `@app/nats/kv_cas.py`:
- Around line 61-66: Update kv_list_keys to pass prefix as the server-side
filter when calling CasKv.keys, while retaining the client-side startswith check
as a guard. Confirm the installed nats-py KeyValue.keys subject-filter syntax,
and update MemoryCasKv.keys to apply matching semantics consistently.
In `@app/nats/leader.py`:
- Around line 29-30: Replace the module-level assert guarding the heartbeat
timing invariant in app/nats/leader.py with an explicit startup-time validation
that raises an appropriate error, or remove it from production code and rely on
test_heartbeat_retry_budget_fits_in_lease. Do not use assert for this invariant,
since it must remain enforced when Python runs with optimization enabled.
In `@app/node/__init__.py`:
- Around line 49-58: Update NodeManager’s _shutdown_node to catch the exception
as err and log it at debug level instead of silently swallowing it. In
remove_node, retain the detached shutdown task in a NodeManager-owned
_pending_shutdowns set, and register completion cleanup so finished tasks are
discarded while shutdown continues safely.
In `@app/node/nats_memory.py`:
- Around line 119-128: Reduce KV round trips in enqueue_users by updating
kv_put_json to attempt create first and only perform an update when the key
already exists, preserving overwrite semantics. Also optimize claim_users and
_requeue_expired_claims to avoid fetching each key individually where the KV API
supports batched operations, and apply the same bounded/list-scan improvement to
kv_list_keys.
In `@tests/test_nats_leader_heartbeat.py`:
- Around line 99-109: Update the heartbeat test around _heartbeat_loop and its
sleep patch so it does not replace the shared asyncio.sleep function; use a
module-local sleep indirection in app/nats/leader.py and patch that symbol, or
wrap the real sleep while recording delays and still yielding control. Preserve
the existing assertion that the recorded delays are [5.0, 1.0, 1.0].
In `@tests/test_nats_leader_steal.py`:
- Around line 10-30: Add an autouse fixture in the test module that snapshots
and restores leader._is_leader and leader._token around every test, matching the
cleanup pattern used by the heartbeat tests. Apply it to both leader-steal tests
so try_become_leader state cannot leak between tests.
In `@tests/test_nats_node_memory.py`:
- Around line 76-93: Update MemoryCasKv.create to raise nats-py’s
KeyAlreadyExistsError when the key already exists, matching the real
KeyValue.create behavior instead of KeyWrongLastSequenceError. Preserve
successful creation semantics and ensure tests covering NatsUserSyncStore
claim/create conflicts exercise the production exception path.
🪄 Autofix
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 Plus
Run ID: 92dd3ca5-b33b-4339-a249-ede0df480c98
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.env.example.github/workflows/api-codeql.yml.github/workflows/build-dev.yml.github/workflows/build.yml.github/workflows/frontend-codeql.yml.github/workflows/test-database-migrations.ymlapp/app_factory.pyapp/core/hosts.pyapp/core/manager.pyapp/db/crud/wireguard.pyapp/jobs/node_checker.pyapp/jobs/record_usages.pyapp/nats/__init__.pyapp/nats/kv_cas.pyapp/nats/leader.pyapp/nats/message.pyapp/nats/router.pyapp/node/__init__.pyapp/node/manager_sync.pyapp/node/nats_memory.pyapp/operation/__init__.pyapp/operation/core.pyapp/operation/group.pyapp/operation/node.pyapp/operation/subscription.pyapp/operation/user.pyapp/operation/user_template.pyapp/routers/group.pyapp/subscription/share.pyconfig.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/ui/variables-popover.tsxdashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/subscriptions/components/subscription-general-settings-section.tsxdashboard/src/features/subscriptions/components/subscription-settings-schema.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/pages/_dashboard.settings.general.tsxpyproject.tomlrole.pytests/api/test_core.pytests/api/test_user.pytests/test_connect_concurrency.pytests/test_create_app_nats_guard.pytests/test_group_access_unit.pytests/test_nats_leader_heartbeat.pytests/test_nats_leader_steal.pytests/test_nats_node_memory.pytests/test_node_manager_sync.py
🚧 Files skipped from review as they are similar to previous changes (7)
- dashboard/public/statics/locales/zh.json
- dashboard/public/statics/locales/en.json
- dashboard/public/statics/locales/fa.json
- dashboard/public/statics/locales/ru.json
- dashboard/src/features/subscriptions/components/subscription-settings-schema.ts
- app/operation/subscription.py
- dashboard/src/features/users/components/action-buttons.tsx
There was a problem hiding this comment.
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: 3
🧹 Nitpick comments (10)
app/app_factory.py (1)
76-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the truncated comment.
The comment on Line 81 ends with a semicolon and no conclusion. State why non-node roles register the ignore handler.
🤖 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 `@app/app_factory.py` around lines 76 - 82, Complete the comment above _ignore_worker_sync_message to explain that non-node roles still subscribe to worker_sync messages but intentionally ignore them through the registered handler.tests/test_nats_leader_steal.py (1)
10-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the leader module globals after each test.
These tests set
leader._is_leaderandleader._tokendirectly and never restore them.leader._is_leaderstaysTrueafter the first test, so any later test in the same session that callsleader.is_job_leader()observes leaked state.tests/test_nats_leader_heartbeat.pyprotects itself with an autouse fixture; add the same guard here.♻️ Proposed fixture
from app.nats import leader from role import Role +@pytest.fixture(autouse=True) +def _reset_leader_state(): + yield + leader._is_leader = False + leader._token = None + leader._kv = None + + `@pytest.mark.asyncio`Also applies to: 44-59
🤖 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 `@tests/test_nats_leader_steal.py` around lines 10 - 30, Add an autouse fixture in the test module that snapshots and restores leader._is_leader and leader._token around every test, matching the cleanup pattern used by the heartbeat tests. Apply it to both leader-steal tests so try_become_leader state cannot leak between tests.app/db/crud/wireguard.py (1)
286-307: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd
.distinct()to the accessible-tags statement.A user who reaches the same inbound tag through several enabled groups produces one row per group. The callers deduplicate in Python, so the extra rows only add transfer and parsing cost. The tag-filtered variant now runs over all users, so the row count matters more than before.
♻️ Proposed change
.join(ProxyInbound, ProxyInbound.id == inbounds_groups_association.c.inbound_id) + .distinct() )🤖 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 `@app/db/crud/wireguard.py` around lines 286 - 307, Update _accessible_tags_stmt to apply distinct() to the constructed select before returning it, including both unfiltered and conditionally filtered variants, so each user/tag pair is returned only once.app/nats/__init__.py (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
is_multi_workerin the router.
app/nats/router.pylines 15-17 recompute the same expression inline in_router_enabled. Importis_multi_workerthere so the multi-worker rule stays defined in one place.🤖 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 `@app/nats/__init__.py` around lines 14 - 16, Update `_router_enabled` in `app/nats/router.py` to import and call `is_multi_worker()` from `app.nats` instead of recomputing `runtime_settings.role.requires_nats or server_settings.workers > 1` inline. Keep the existing router behavior unchanged while centralizing the multi-worker rule in `is_multi_worker`.app/nats/leader.py (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDo not rely on a module-level
assertfor the timing invariant.Python removes
assertstatements when it runs with-O. The invariant then disappears at runtime. The testtest_heartbeat_retry_budget_fits_in_leasealready covers this constant relationship, so either keep the check only in tests or raise an explicit error.♻️ Proposed change
-# Worst-case time from last successful renew to concede must stay under the lease. -assert HEARTBEAT_INTERVAL + (HEARTBEAT_MAX_RETRIES - 1) * HEARTBEAT_RETRY_DELAY < DEFAULT_LEASE_SECONDS +# Worst-case time from last successful renew to concede must stay under the lease. +if HEARTBEAT_INTERVAL + (HEARTBEAT_MAX_RETRIES - 1) * HEARTBEAT_RETRY_DELAY >= DEFAULT_LEASE_SECONDS: + raise RuntimeError("Heartbeat retry budget exceeds the leader lease duration")🤖 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 `@app/nats/leader.py` around lines 29 - 30, Replace the module-level assert guarding the heartbeat timing invariant in app/nats/leader.py with an explicit startup-time validation that raises an appropriate error, or remove it from production code and rely on test_heartbeat_retry_budget_fits_in_lease. Do not use assert for this invariant, since it must remain enforced when Python runs with optimization enabled.tests/test_nats_leader_heartbeat.py (1)
99-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePatching
asyncio.sleepchanges the stdlib module globally.
leader.asynciois the realasynciomodule, so this replacesasyncio.sleepfor the whole process while the test runs.monkeypatchrestores it afterwards, so the blast radius is limited, but any other coroutine scheduled during this test also receives the no-op stub. Consider recording the delays through a wrapper that still yields control, or assert on the delay values with a module-local sleep indirection inapp/nats/leader.py.🤖 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 `@tests/test_nats_leader_heartbeat.py` around lines 99 - 109, Update the heartbeat test around _heartbeat_loop and its sleep patch so it does not replace the shared asyncio.sleep function; use a module-local sleep indirection in app/nats/leader.py and patch that symbol, or wrap the real sleep while recording delays and still yielding control. Preserve the existing assertion that the recorded delays are [5.0, 1.0, 1.0].tests/test_nats_node_memory.py (1)
76-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the test double with real NATS KV error behavior.
These tests run only against
MemoryCasKv.MemoryCasKv.createraisesKeyWrongLastSequenceErroron an existing key, while nats-pyKeyValue.createraisesKeyAlreadyExistsError. The suite therefore cannot detect the create-conflict gap flagged inapp/nats/kv_cas.pyat Lines 39-49. After you resolve that gap, make the double raise the same exception type as the real client so this coverage stays meaningful.🤖 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 `@tests/test_nats_node_memory.py` around lines 76 - 93, Update MemoryCasKv.create to raise nats-py’s KeyAlreadyExistsError when the key already exists, matching the real KeyValue.create behavior instead of KeyWrongLastSequenceError. Preserve successful creation semantics and ensure tests covering NatsUserSyncStore claim/create conflicts exercise the production exception path.app/nats/kv_cas.py (1)
61-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPass the prefix to
kv.keys()instead of listing the whole bucket.
kv_list_keysignores thefiltersparameter declared inCasKv.keys. It fetches every key in the bucket and filters in Python.NatsUserSyncStore.claim_userscalls this on each claim, so cost grows with total pending users across all nodes, not with one node. Use the server-side filter and keep the client-side prefix check as a guard.♻️ Proposed refactor
async def kv_list_keys(kv: CasKv, prefix: str) -> list[str]: try: - keys = await kv.keys() + keys = await kv.keys(filters=[f"{prefix}>"]) except nats_js_errors.NoKeysError: return [] return [key for key in keys if key.startswith(prefix)]Confirm the subject-filter syntax accepted by the installed nats-py
KeyValue.keys, and alignMemoryCasKv.keysmatching with it.🤖 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 `@app/nats/kv_cas.py` around lines 61 - 66, Update kv_list_keys to pass prefix as the server-side filter when calling CasKv.keys, while retaining the client-side startswith check as a guard. Confirm the installed nats-py KeyValue.keys subject-filter syntax, and update MemoryCasKv.keys to apply matching semantics consistently.app/node/__init__.py (1)
49-58: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed shutdown error.
_shutdown_nodediscards every exception.remove_noderuns it in a detached task, so a failure to stop a remote core leaves no trace. Log at debug level to keep diagnosis possible.Also keep a reference to the task created at Line 82. An unreferenced task can be garbage collected before it completes.
♻️ Proposed refactor
async def _shutdown_node(self, node: PasarGuardNode | None, *, remote_stop: bool = True): if node is None: return try: await node.set_health(Health.INVALID) if remote_stop: await node.stop() - except Exception: - pass + except Exception as exc: + self.logger.debug("Node shutdown failed (remote_stop=%s): %s", remote_stop, exc)For the detached task, store it in a set owned by
NodeManagerand discard it on completion:- asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + task = asyncio.create_task(self._shutdown_node(old_node, remote_stop=remote_stop)) + self._pending_shutdowns.add(task) + task.add_done_callback(self._pending_shutdowns.discard)Add
self._pending_shutdowns: set[asyncio.Task] = set()in__init__.🤖 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 `@app/node/__init__.py` around lines 49 - 58, Update NodeManager’s _shutdown_node to catch the exception as err and log it at debug level instead of silently swallowing it. In remove_node, retain the detached shutdown task in a NodeManager-owned _pending_shutdowns set, and register completion cleanup so finished tasks are discarded while shutdown continues safely.app/node/nats_memory.py (1)
119-128: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNote the per-user round-trip cost of the KV queue.
enqueue_usersperforms onekv_put_jsonper user, and eachkv_put_jsonperforms at least onegetplus onecreate/update. A full user sync of N users therefore costs at least 2N KV round trips.claim_usersadds a_requeue_expired_claimsscan plus agetper pending key on every call.This path runs on node user synchronization, so the cost scales with panel size. Consider batching per node into bounded chunks, or skipping the
getinkv_put_jsonby attemptingcreatefirst. The list-scan part shares a root cause withkv_list_keysinapp/nats/kv_cas.py.Also applies to: 149-193
🤖 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 `@app/node/nats_memory.py` around lines 119 - 128, Reduce KV round trips in enqueue_users by updating kv_put_json to attempt create first and only perform an update when the key already exists, preserving overwrite semantics. Also optimize claim_users and _requeue_expired_claims to avoid fetching each key individually where the KV API supports batched operations, and apply the same bounded/list-scan improvement to kv_list_keys.
🤖 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 `@app/db/crud/wireguard.py`:
- Around line 333-344: Update _peer_ips_present_clause to use dialect-correct
JSON array emptiness checks instead of comparing peer_ips.as_string() with "[]".
Preserve the non-null requirement and ensure _user_ids_with_peer_ips returns
only users whose wireguard.peer_ips array contains at least one entry across
PostgreSQL, MySQL, and SQLite.
In `@app/nats/leader.py`:
- Around line 60-71: Update _ensure_kv and the start_job_leader renewal/reclaim
flow to detect a closed or disconnected _nc before returning the cached _kv.
When the connection is unavailable, fail the current renewal attempt and trigger
the existing NATS reconnect path instead of calling _concede_leadership("renewal
exhausted") or returning a stale KV handle; continue retrying until recovery or
lease expiration.
In `@app/node/nats_memory.py`:
- Around line 436-444: Update shutdown_bridge_memory so all module globals are
reset even when _nc.close() raises. Suppress the close error using the same
handling pattern already used by ensure_bridge_memory, while ensuring _nc,
_user_sync_kv, _lifecycle_kv, _user_sync_store, and _lifecycle_coordinator are
cleared.
---
Nitpick comments:
In `@app/app_factory.py`:
- Around line 76-82: Complete the comment above _ignore_worker_sync_message to
explain that non-node roles still subscribe to worker_sync messages but
intentionally ignore them through the registered handler.
In `@app/db/crud/wireguard.py`:
- Around line 286-307: Update _accessible_tags_stmt to apply distinct() to the
constructed select before returning it, including both unfiltered and
conditionally filtered variants, so each user/tag pair is returned only once.
In `@app/nats/__init__.py`:
- Around line 14-16: Update `_router_enabled` in `app/nats/router.py` to import
and call `is_multi_worker()` from `app.nats` instead of recomputing
`runtime_settings.role.requires_nats or server_settings.workers > 1` inline.
Keep the existing router behavior unchanged while centralizing the multi-worker
rule in `is_multi_worker`.
In `@app/nats/kv_cas.py`:
- Around line 61-66: Update kv_list_keys to pass prefix as the server-side
filter when calling CasKv.keys, while retaining the client-side startswith check
as a guard. Confirm the installed nats-py KeyValue.keys subject-filter syntax,
and update MemoryCasKv.keys to apply matching semantics consistently.
In `@app/nats/leader.py`:
- Around line 29-30: Replace the module-level assert guarding the heartbeat
timing invariant in app/nats/leader.py with an explicit startup-time validation
that raises an appropriate error, or remove it from production code and rely on
test_heartbeat_retry_budget_fits_in_lease. Do not use assert for this invariant,
since it must remain enforced when Python runs with optimization enabled.
In `@app/node/__init__.py`:
- Around line 49-58: Update NodeManager’s _shutdown_node to catch the exception
as err and log it at debug level instead of silently swallowing it. In
remove_node, retain the detached shutdown task in a NodeManager-owned
_pending_shutdowns set, and register completion cleanup so finished tasks are
discarded while shutdown continues safely.
In `@app/node/nats_memory.py`:
- Around line 119-128: Reduce KV round trips in enqueue_users by updating
kv_put_json to attempt create first and only perform an update when the key
already exists, preserving overwrite semantics. Also optimize claim_users and
_requeue_expired_claims to avoid fetching each key individually where the KV API
supports batched operations, and apply the same bounded/list-scan improvement to
kv_list_keys.
In `@tests/test_nats_leader_heartbeat.py`:
- Around line 99-109: Update the heartbeat test around _heartbeat_loop and its
sleep patch so it does not replace the shared asyncio.sleep function; use a
module-local sleep indirection in app/nats/leader.py and patch that symbol, or
wrap the real sleep while recording delays and still yielding control. Preserve
the existing assertion that the recorded delays are [5.0, 1.0, 1.0].
In `@tests/test_nats_leader_steal.py`:
- Around line 10-30: Add an autouse fixture in the test module that snapshots
and restores leader._is_leader and leader._token around every test, matching the
cleanup pattern used by the heartbeat tests. Apply it to both leader-steal tests
so try_become_leader state cannot leak between tests.
In `@tests/test_nats_node_memory.py`:
- Around line 76-93: Update MemoryCasKv.create to raise nats-py’s
KeyAlreadyExistsError when the key already exists, matching the real
KeyValue.create behavior instead of KeyWrongLastSequenceError. Preserve
successful creation semantics and ensure tests covering NatsUserSyncStore
claim/create conflicts exercise the production exception path.
🪄 Autofix
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 Plus
Run ID: 92dd3ca5-b33b-4339-a249-ede0df480c98
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
.env.example.github/workflows/api-codeql.yml.github/workflows/build-dev.yml.github/workflows/build.yml.github/workflows/frontend-codeql.yml.github/workflows/test-database-migrations.ymlapp/app_factory.pyapp/core/hosts.pyapp/core/manager.pyapp/db/crud/wireguard.pyapp/jobs/node_checker.pyapp/jobs/record_usages.pyapp/nats/__init__.pyapp/nats/kv_cas.pyapp/nats/leader.pyapp/nats/message.pyapp/nats/router.pyapp/node/__init__.pyapp/node/manager_sync.pyapp/node/nats_memory.pyapp/operation/__init__.pyapp/operation/core.pyapp/operation/group.pyapp/operation/node.pyapp/operation/subscription.pyapp/operation/user.pyapp/operation/user_template.pyapp/routers/group.pyapp/subscription/share.pyconfig.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/ui/variables-popover.tsxdashboard/src/features/hosts/dialogs/host-modal.tsxdashboard/src/features/subscriptions/components/subscription-general-settings-section.tsxdashboard/src/features/subscriptions/components/subscription-settings-schema.tsdashboard/src/features/users/components/action-buttons.tsxdashboard/src/pages/_dashboard.settings.general.tsxpyproject.tomlrole.pytests/api/test_core.pytests/api/test_user.pytests/test_connect_concurrency.pytests/test_create_app_nats_guard.pytests/test_group_access_unit.pytests/test_nats_leader_heartbeat.pytests/test_nats_leader_steal.pytests/test_nats_node_memory.pytests/test_node_manager_sync.py
🚧 Files skipped from review as they are similar to previous changes (7)
- dashboard/public/statics/locales/zh.json
- dashboard/public/statics/locales/en.json
- dashboard/public/statics/locales/fa.json
- dashboard/public/statics/locales/ru.json
- dashboard/src/features/subscriptions/components/subscription-settings-schema.ts
- app/operation/subscription.py
- dashboard/src/features/users/components/action-buttons.tsx
🛑 Comments failed to post (3)
app/db/crud/wireguard.py (1)
333-344: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find existing JSON path comparisons in the CRUD layer to check for a dialect-safe precedent. rg -nP --type=py -C3 '(as_string\(\)|json_extract|JSON_LENGTH|jsonb_array_length)'Repository: PasarGuard/panel
Length of output: 154
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## Locate wireguard.py and related files" fd -a 'wireguard\.py$' . || true echo echo "## Relevant slice of app/db/crud/wireguard.py" if [ -f app/db/crud/wireguard.py ]; then wc -l app/db/crud/wireguard.py sed -n '300,370p' app/db/crud/wireguard.py echo sed -n '1,80p' app/db/crud/wireguard.py fi echo "## Repository config files" git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|setup\.py|poetry\.lock|uv\.lock|Pipfile\.lock)$|app/.*(models|db|crud).*\.py$' || trueRepository: PasarGuard/panel
Length of output: 17052
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## pyproject dependencies" sed -n '1,220p' pyproject.toml | rg -n 'sqlalchemy|alembic|aiosqlite|psycopg|mysql|asyncmy|mariadb|pymysql|pydantic|fastapi|uvicorn|python-multipart|redis|celery|celery|databases|aiomysql|redis' -C 2 || true echo echo "## locate User model and proxy_settings definition" rg -n "class User|proxy_settings|JSONB|JSON" app/db app/models app/db/migrations/versions/3b59f3680c90_add_groups_table_and_add_proxy_settings_column_to_users_table.py.py app/db/migrations/versions/3c1a7e5b9d20_add_wireguard_subnets_pool_table.py -C 2 || true echo echo "## search dialect-specific JSON length helpers already in repo" rg -n -i 'json_|json_length|jsonb|json_extract|asstring|as_string' app pyproject.toml uv.lock || trueRepository: PasarGuard/panel
Length of output: 50373
🌐 Web query:
SQLAlchemy PostgreSQL JSON as_string literal string [] JSONPath💡 Result:
In SQLAlchemy, specifically when working with the PostgreSQL dialect, interacting with JSON structures and path-based queries involves distinct approaches for indexing and casting. JSON Indexing and Path Access - Bracket Notation: You can access JSON keys or array indices using standard Python bracket syntax on a JSON column (e.g., column['key'] or column['index']). By default, these operations return a JSON/JSONB expression [1][2]. - Path Access: For multi-level access or specific JSON paths, you can pass a tuple to the bracket operator, such as column[('key1', 'key2', 'index')]. SQLAlchemy translates this into the appropriate PostgreSQL path operator [2]. Retrieving as String or Text - astext: To perform comparisons against strings (e.g., in a WHERE clause), you must use the.astext attribute [3][2]. This translates the PostgreSQL JSON operator
->>(which returns text) rather than->(which returns JSON/JSONB) [3][4]. - as_string: This method is available on JSON comparators [5][1]. While it often applies a VARCHAR cast in the PostgreSQL dialect, it should be used for retrieving elements as their scalar type rather than as a general-purpose CAST for type cross-casting [6]. Casting to Specific Types - To cast a JSON element to a specific SQL type (e.g., Integer, Boolean), use.astext followed by.cast(Type) [3]. Example: session.query(Model).filter(Model.json_col['id'].astext.cast(Integer) == 1) [3] JSONPATH Type - For advanced SQL/JSON path queries, such as those used withjsonb_path_query_arrayorjsonb_path_exists, SQLAlchemy provides a specificJSONPATHtype [7]. This allows you to cast a literal string path into the correct PostgreSQL type for path processing [7]. Example: stmt = select(func.jsonb_path_query_array(table.c.jsonb_col, cast("$.address.id", JSONPATH))) [7] Important Considerations - Do not rely on.as_string or.as_int as universal cast functions [6]. If you need to perform true type conversion (e.g., converting a string-encoded JSON value into an integer), it is best practice to use the explicitcast()function on the astext-returned value [6][3]. - PostgreSQL path expressions (e.g., $.key) are typically handled as string literals in SQL; ensure any necessary escaping is followed according to PostgreSQL requirements when constructing these strings manually [8].Citations:
- 1: https://stackoverflow.com/questions/29974143/how-to-query-a-json-element
- 2: https://stackoverflow.com/questions/31804378/how-to-query-on-a-json-type-field-with-sqlalchemy
- 3: https://stackoverflow.com/questions/53264047/sqlalchemy-filter-by-json-field
- 4: https://stackoverflow.com/questions/34682263/how-to-create-an-index-on-a-nested-key-of-a-json-postgresql-column-in-sqlalchemy
- 5: http://docs.sqlalchemy.org/en/latest/core/type_basics.html
- 6: sqlalchemy/sqlalchemy#11065
- 7: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/postgresql/json.py
- 8: https://www.postgresql.org/docs/current/datatype-json.html
🌐 Web query:
SQLAlchemy MySQL JSON extract JSON_EXTRACT empty array literal [] string comparison💡 Result:
When querying MySQL JSON columns using SQLAlchemy, comparing the result of JSON_EXTRACT (accessed via the bracket operator, e.g., table.c.data['key']) against an empty array literal '[]' as a string often fails to return the expected results [1]. This occurs because JSON_EXTRACT returns a JSON-formatted value, and direct string comparison with '[]' may not behave as intended due to type mismatches [1]. To correctly identify records where a JSON path contains an empty array, you should use one of the following approaches: 1. Use JSON_ARRAY: Instead of comparing against the string '[]', compare the extracted value against the MySQL function JSON_ARRAY [1]. Example: session.query(MyTable).filter(MyTable.data['codes'] == func.json_array) 2. Use JSON_LENGTH: This is often the most reliable and performant method for checking if an array is empty [1]. Example: session.query(MyTable).filter(func.json_length(MyTable.data['codes']) == 0) Important considerations: - JSON_EXTRACT Behavior: SQLAlchemy maps subscription syntax (e.g., table.c.data['path']) to the MySQL JSON_EXTRACT function [2][3]. - Type Casting: If you need to perform specific string comparisons for non-array values, use the.as_string method on the JSON element (e.g., table.c.data['key'].as_string == 'value') to ensure correct type handling by SQLAlchemy across different backends [4][5]. - JSON vs. SQL NULL: Remember that JSON_EXTRACT returning a JSON null is distinct from an SQL NULL [6]. Use JSON_TYPE or JSON_VALUE (if using MySQL 8.0.21+) for reliable null checks on JSON properties [6][7].
Citations:
- 1: https://medium.com/@felipegirotti/mysql-select-empty-json-array-53c1194e5efa
- 2: https://docs.sqlalchemy.org/en/21/dialects/mysql.html
- 3: sqlalchemy/sqlalchemy#4739
- 4: sqlalchemy/sqlalchemy#9480
- 5: sqlalchemy/sqlalchemy#11065
- 6: https://bugs.mysql.com/bug.php?id=112082
- 7: https://stackoverflow.com/questions/63581165/how-to-null-check-mysql-json-column-property
🌐 Web query:
SQLAlchemy SQLite JSON json_extract empty array output []💡 Result:
In SQLite, the behavior of json_extract when extracting an array is that it returns a JSON-formatted string representation of that array [1][2][3]. If the extracted result is an empty array, json_extract will return the string '[]' [1]. When using SQLAlchemy's SQLite JSON type, this behavior is managed by wrapping the database-level json_extract function in JSON_QUOTE [4][5]. This wrapping ensures that the extracted values are consistently treated as JSON string values [4][6][5]. Key points regarding this behavior: 1. SQLite's native json_extract function: When a single path argument is provided and it references a JSON array or object, the function returns that value as a JSON string [1]. Thus, an empty array in the JSON source results in the string '[]' [1]. 2. SQLAlchemy integration: SQLAlchemy's dialect for SQLite further guarantees that these results are quoted as JSON strings [4][6][7]. This helps maintain consistency in the output format, ensuring that you receive a JSON string like '[]' rather than potentially confusing native SQLite types in certain scenarios [4][5]. 3. Alternative usage: If you are using newer versions of SQLite (3.38.0+), you can also use the ->> operator, which returns the "unwrapped" SQL value [2][3]. However, for empty arrays, the distinction often remains that the value is represented as a string or a JSON-formatted text block depending on your exact access method [1][2]. If you are seeing '[]' and expected a Python list object, remember that you are receiving a string from the database; you will typically need to use json.loads in your application layer if you want to convert that string representation back into a native Python list [4].
Citations:
- 1: https://sqlite.org/json1.html
- 2: https://jsonic.io/guides/sqlite-json
- 3: https://www.dbpro.app/learn/sqlite/sqlite-json-extract-function
- 4: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 5: https://docs.sqlalchemy.org/en/21/dialects/sqlite.html
- 6: https://fossies.org/linux/sqlalchemy/lib/sqlalchemy/dialects/sqlite/json.py
- 7: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "## User proxy_settings model definition" rg -n -C 4 'class User|proxy_settings' app/db/models.py app/models/user.py || true echo echo "## SQLAlchemy package metadata if vendored/installer not available" if [ -f uv.lock ]; then rg -n -C 2 'name = "sqlalchemy"|name = "greenlet"|name = "aiosqlite"|name = "asyncpg"|name = "asyncmy"' uv.lock || true fiRepository: PasarGuard/panel
Length of output: 19647
🌐 Web query:
site:github.com/sqlalchemy/sqlalchemy "json_extract" "JSON_QUOTE" SQLite JSON as_string SQLAlchemy💡 Result:
In SQLite, the json_extract function retrieves a value from a JSON document, but it behaves differently depending on the data type—for instance, it may return a raw scalar (like 1 for true) rather than a quoted JSON string [1]. The json_quote function is often used in conjunction with json_extract to ensure that the extracted result is treated as a valid JSON-formatted string, which helps normalize behavior across different database backends [2]. In SQLAlchemy, when working with JSON columns in SQLite, you may encounter generated SQL that includes JSON_QUOTE(JSON_EXTRACT(...)) [3][4][5]. This is an internal implementation detail used by the SQLAlchemy SQLite dialect to attempt to unify behavior with other databases like MySQL [2]. To handle JSON data effectively in SQLAlchemy in a platform-agnostic way, you should use the.as_string method on the JSON object subscript [3]. This method instructs SQLAlchemy to treat the retrieved JSON element as a string, providing a consistent API across different database dialects [3][6]. For example, to filter a query by a JSON key's value: # Recommended, platform-agnostic approach query = session.query(MyTable).filter( MyTable.data["key"].as_string == "my_value") Avoid relying on internal behaviors like JSON_QUOTE directly. If you face issues where types are not matching as expected (e.g., comparing a JSON element to a literal), using.as_string (or.as_integer, etc.) ensures that SQLAlchemy handles the database-specific casting correctly [3][6]. If you need to perform true cross-type casting (e.g., forcing a JSON string to an integer), use the standard sqlalchemy.cast function instead [6].
Citations:
- 1: sqlalchemy/sqlalchemy#4969
- 2: sqlalchemy/sqlalchemy#3850
- 3: sqlalchemy/sqlalchemy#9480
- 4: sqlalchemy/sqlalchemy#4437
- 5: sqlalchemy/sqlalchemy#5156
- 6: sqlalchemy/sqlalchemy#11065
🌐 Web query:
SQLAlchemy 2.0 MySQL JSON __getitem__ as_string JSON_QUOTE💡 Result:
In SQLAlchemy 2.0, when working with a MySQL JSON column, the
getitemoperator (e.g.,column['key']) is compiled to the MySQLJSON_EXTRACTfunction [1][2][3]. Regarding theas_string()method andJSON_QUOTEbehavior: 1. Behavior: In some dialects or configurations, usingas_string()on a JSON element may result in the expression being wrapped inJSON_QUOTEat the database level [4]. This occurs because SQLAlchemy's JSON element methods are intended to signal that the data should be handled as a specific scalar type rather than as a nested JSON object; however, this implementation has been noted as potentially misleading because it is not always equivalent to a direct SQLCAST[4]. 2. As-String vs. Casting: Theas_string()method is designed to retrieve JSON elements as their scalar type, assuming the JSON value already matches that type [4]. If you require a true SQL type conversion (e.g., forcing a value to be treated as a string regardless of its JSON type), you should use the explicitcast()function [4]: # Using as_string (may involve JSON_QUOTE or JSON_EXTRACT) table.c.json_col['key'].as_string # Explicit cast for true type conversion cast(table.c.json_col['key'], String) 3. Recommendation: If you are encountering issues whereas_string()producesJSON_QUOTEand interferes with your query logic, prefer usingcast(..., String)to ensure the SQL compiler produces a standardCAST(... AS CHAR)or equivalent operation [4][5]. For complex MySQL JSON operations that require specific path handling or type coercion, ensure you are using thesqlalchemy.dialects.mysql.JSONtype, which is automatically utilized when you use the standardsqlalchemy.types.JSONtype against a MySQL backend [6][7].Citations:
- 1: sqlalchemy/sqlalchemy#3619
- 2: sqlalchemy/sqlalchemy#4739
- 3: sqlalchemy/sqlalchemy#3547
- 4: sqlalchemy/sqlalchemy#11065
- 5: https://stackoverflow.com/questions/66799742/sqlalchemy-json-configured-varchar-how-to-filter
- 6: https://dokk.org/documentation/sqlalchemy/rel_2_0_23/dialects/mysql/
- 7: https://docs.sqlalchemy.org/en/21/dialects/mysql.html
🌐 Web query:
sqlalchemy.dialects.mysql JSON as_string JSON_QUOTE source💡 Result:
In SQLAlchemy, the
as_string()method is a standard, backend-agnostic tool used to cast the result of a JSON index or path operation to a string for comparison purposes [1][2][3]. WhileJSON_QUOTEis a MySQL function often seen in generated SQL when performing JSON operations, it is not a primary component that you interact with directly through theJSONtype API [1][4]. Key takeaways regarding your query: 1. Use as_string: When working with JSON column elements (e.g.,model.column['key']), SQLAlchemy may not automatically know how to compare the returned JSON fragment to a standard Python string [1]. Calling.as_string()on the JSON expression tells SQLAlchemy to treat the element as a string, ensuring correct behavior across different database dialects (including MySQL and SQLite) [1][2][3]. 2. Handling JSON_QUOTE: Issues involvingJSON_QUOTEoften arise when users try to manually compare JSON values against database functions or when SQLAlchemy generates specific SQL to handle type casting [1][5]. If you encounter unexpectedJSON_QUOTEusage in your generated SQL, it is frequently a sign that the expression is being compared as a JSON object rather than a string [1]. Adding.as_string()usually resolves these issues by properly casting the value [1][5]. 3. MySQL Specifics: Thesqlalchemy.dialects.mysql.JSONtype is used automatically when using the basesqlalchemy.types.JSONtype with a MySQL or MariaDB backend [4][6]. It handles the mapping to the native MySQLJSONtype (orLONGTEXTin MariaDB) and adapts index operations to useJSON_EXTRACT[4][7]. For most use cases, you should rely oncolumn['key'].as_string() == 'value'rather than attempting to manually call or bypassJSON_QUOTE[1].Citations:
- 1: sqlalchemy/sqlalchemy#9480
- 2: http://docs.sqlalchemy.org/en/latest/core/type_basics.html
- 3: https://docs.sqlalchemy.org/en/13/core/type_basics.html?highlight=json
- 4: https://docs.sqlalchemy.org/en/21/dialects/mysql.html
- 5: https://stackoverflow.com/questions/75730699/how-to-turn-off-sqlite-json-query-with-json-quote-there-are-different-between-s
- 6: http://docs.sqlalchemy.org/en/latest/dialects/mysql.html
- 7: https://fossies.org/linux/sqlalchemy/lib/sqlalchemy/dialects/mysql/json.py
🌐 Web query:
PostgreSQL empty array is_not null "->" operator PostgreSQL jsonb_path_exists empty array []💡 Result:
In PostgreSQL, an empty array (
{}or[]in JSONB) is distinct fromNULL[1][2][3]. 1. Empty Array vs. NULL: An empty array is a non-null, zero-length container [1]. To check if an array column is an empty array, you must compare it explicitly to the empty literal (e.g.,WHERE col = '{}'), not usingIS NOT NULL[2]. 2. jsonb_path_exists Behavior: Thejsonb_path_existsfunction (and the@?operator) returnstrueif the path query finds at least one item, andfalseotherwise [4][5]. - If you target a path that points to an empty array (e.g.,$.datawheredatais[]),jsonb_path_existswill returntruebecause the path exists and resolves to the empty array value [4][5]. - If you want to check if the array is empty specifically, you should verify the path exists and check that it contains no elements, often by using a filter or checking the size of the array [5]. For example,jsonb_path_exists(data, '$.a[*] ? (@.type() == "array" && $.size() == 0)')or similar logic can be used depending on your specific requirements [5].Citations:
- 1: https://stackoverflow.com/questions/67177169/do-empty-arrays-equal-null-in-postgresql-or-is-it-an-array-with-null-inside
- 2: https://thelinuxcode.com/access-array-data-postgresql/
- 3: https://www.postgresql.org/message-id/4FDA6268.2010209%40pinpointresearch.com
- 4: https://www.postgresql.org/docs/current/functions-json.html
- 5: https://justatheory.com/2023/10/sql-jsonpath-operators/
Use dialect-specific JSON emptiness checks.
_peer_ips_present_clausecompares an extracted JSON value to the literal text"[]". PostgreSQL path access returns a typed JSON array, not a text"[]", sopeer_ips.as_string() != "[]"can change the user set. MySQL and SQLite use JSON-extract-style extraction here too, so use one of: per-dialect JSON length checks;jsonb_path_exists/SQL JSON path for PostgreSQL; or parse the JSON value in Python after loading the user set.🤖 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 `@app/db/crud/wireguard.py` around lines 333 - 344, Update _peer_ips_present_clause to use dialect-correct JSON array emptiness checks instead of comparing peer_ips.as_string() with "[]". Preserve the non-null requirement and ensure _user_ids_with_peer_ips returns only users whose wireguard.peer_ips array contains at least one entry across PostgreSQL, MySQL, and SQLite.app/nats/leader.py (1)
60-71: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect how the shared NATS client is created and whether reconnect options are set. fd -t f 'client.py' -p app/nats --exec cat -n {}Repository: PasarGuard/panel
Length of output: 4057
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" fd -t f 'leader.py|client.py|settings.py|.*\.py$' app 2>/dev/null | sed -n '1,80p' echo echo "== app/nats/leader.py outline ==" ast-grep outline app/nats/leader.py 2>/dev/null || true echo echo "== relevant leader.py sections ==" cat -n app/nats/leader.py | sed -n '1,260p' echo echo "== references to _ensure_kv / _nc / _kv / try_become_leader / start_job_leader / _concede_leadership ==" rg -n "_ensure_kv|_nc|_kv|try_become_leader|start_job_leader|_concede_leadership|create_nats_client|connect\\(" app/nats leader.py \ 2>/dev/null || true echo echo "== nats connect API docs in package if available ==" python3 - <<'PY' import inspect, importlib try: import nats import asyncio print("nats", getattr(nats, "__version__", "unknown")) print(inspect.signature(nats.connect)) print(inspect.getdoc(nats.connect)) except Exception as e: print(type(e).__name__ + ":", e) PYRepository: PasarGuard/panel
Length of output: 20498
🌐 Web query:
nats-py nats.connect default reconnect reconnect_wait reconnect_jitter reconnect_jitter_to reconnect failed reconnect options💡 Result:
In the nats-py library, reconnect behavior is controlled during the initialization of the connection via nats.connect [1][2][3]. The primary options for managing reconnections are as follows: allow_reconnect: A boolean that enables or disables automatic reconnection logic [1][2][3]. It defaults to True [1][2][3]. If disabled, the connection closes immediately upon the first disconnect [4][5]. max_reconnect_attempts: An integer that sets the maximum number of reconnection attempts per server [1][2][3]. The default is typically 60 [4][5], though some migrations or newer versions may adjust this to 10 [6]. Setting this value to -1 enables unlimited reconnection attempts, which is recommended for long-lived services to prevent the client from entering a closed state during extended outages [4][5]. reconnect_time_wait: An integer (representing seconds) that sets the interval to wait between reconnection attempts [1][3][7]. Regarding jitter and advanced tuning, while standard nats-py (often found in nats/aio/client.py) emphasizes the options above, newer or different NATS core implementations may introduce additional fine-tuning parameters such as reconnect_jitter or reconnect_timeout [6]. Users should check the specific version of nats-py they are using, as the library has undergone transitions (e.g., migrating to nats-core) which may change parameter names (e.g., max_reconnect_attempts becoming reconnect_max_attempts) [6]. To implement these, pass the parameters directly to the nats.connect method: await nc.connect( servers=["nats://localhost:4222"], allow_reconnect=True, max_reconnect_attempts=-1, # Infinite retries reconnect_time_wait=2 # 2 seconds between attempts) For official, up-to-date documentation on all available parameters for your specific version, refer to the nats-py GitHub repository source code [1][3] or the official NATS documentation on resilient clients [4][5].
Citations:
- 1: https://github.com/nats-io/nats.py/blob/main/nats/src/nats/aio/client.py
- 2: nats-io/nats.py#600
- 3: https://github.com/nats-io/nats.py/blob/master/nats/aio/client.py
- 4: https://docs.nats.io/learn/resilient-clients/reconnection.md
- 5: https://docs.nats.io/learn/resilient-clients/reconnection
- 6: https://github.com/nats-io/nats.py/blob/e537164138a73f84cf67b4bfa624efa4092ee374/nats-core/MIGRATION.md
- 7: https://docs.nats.io/using-nats/developer/connecting/reconnect/wait
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import ast from pathlib import Path path = Path("app/nats/client.py") text = path.read_text() tree = ast.parse(text) for node in ast.walk(tree): if isinstance(node, ast.Assign): for target in node.targets: if isinstance(target, ast.Name) and target.id == "_kv": print("cached_global_kv_assignment", ast.unparse(node)) if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in {"_ensure_kv", "stop_job_leader"}: print(f"function {node.name} has global_kv=", any(isinstance(stmt, ast.Global) and any(g.arg == "_kv" for g in stmt.targets) for stmt in node.body)) print("\ncreate_nats_client body:") for node in tree.body: if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)) and node.name == "create_nats_client": for stmt in node.body: print(" ", ast.unparse(stmt)) PY echo echo "== exact _concede/heartbeat/start lifecycle ==" sed -n '173,230p' app/nats/leader.py | cat -nRepository: PasarGuard/panel
Length of output: 3182
Let the NATS client reconnect instead of yielding leadership permanently.
start_job_leadercalls through to_ensure_kvafter_concede_leadership("renewal exhausted"), but_ensure_kvonly creates a client when_kv is None. Since_kvstays cached,_nccan leaveis_closedmode and_require_reconnect()still pending while the reclaim path returns a stale KV handle. Fail the current renewal attempt instead of conceding leadership when the connection is disconnected, then retry until recovery or a new lease expires.🤖 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 `@app/nats/leader.py` around lines 60 - 71, Update _ensure_kv and the start_job_leader renewal/reclaim flow to detect a closed or disconnected _nc before returning the cached _kv. When the connection is unavailable, fail the current renewal attempt and trigger the existing NATS reconnect path instead of calling _concede_leadership("renewal exhausted") or returning a stale KV handle; continue retrying until recovery or lease expiration.app/node/nats_memory.py (1)
436-444: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reset the module globals even if
close()fails.If
_nc.close()raises, the assignments below it do not run._user_sync_storeand_lifecycle_coordinatorthen stay set, soensure_bridge_memoryreturns stale objects bound to a dead connection. Suppress the close error, asensure_bridge_memoryalready does at Lines 418-420.🛡️ Proposed fix
async def shutdown_bridge_memory() -> None: global _nc, _user_sync_kv, _lifecycle_kv, _user_sync_store, _lifecycle_coordinator - if _nc is not None: - await _nc.close() + if _nc is not None: + with contextlib.suppress(Exception): + await _nc.close() _nc = None _user_sync_kv = None _lifecycle_kv = None _user_sync_store = None _lifecycle_coordinator = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async def shutdown_bridge_memory() -> None: global _nc, _user_sync_kv, _lifecycle_kv, _user_sync_store, _lifecycle_coordinator if _nc is not None: with contextlib.suppress(Exception): await _nc.close() _nc = None _user_sync_kv = None _lifecycle_kv = None _user_sync_store = None _lifecycle_coordinator = None🤖 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 `@app/node/nats_memory.py` around lines 436 - 444, Update shutdown_bridge_memory so all module globals are reset even when _nc.close() raises. Suppress the close error using the same handling pattern already used by ensure_bridge_memory, while ensuring _nc, _user_sync_kv, _lifecycle_kv, _user_sync_store, and _lifecycle_coordinator are cleared.
4b73b1a to
5f69f0c
Compare
Two ways the profile route returned a 500 on host configurations that the
legacy subscription formats handle without complaint.
random_user_agent hosts: the profile builders construct their own bare
XrayConfiguration/SingBoxConfiguration, so they never received the user-agent
templates and ended up calling random.choice([]) -> IndexError on ws,
httpupgrade, xhttp, raw/tcp, h2 and grpc. Only the user-agent templates are
threaded through; feeding in the real subscription template would leak its
direct/block outbounds into the profile and break balancer validation.
xhttp download settings: generate_subscription_profile never materialised
transport_config.download_settings, unlike process_inbounds_and_tags. The raw
cache entry reached the writer with candidate lists and an unformatted
{SERVER_IP}, producing "address": ["{SERVER_IP}"] and "port": [8443] where Xray
requires a single string and a scalar. The shared step is now factored out as
_apply_download_settings and used by both generators.
Also inlines require_config_eligible so this no longer depends on PasarGuard#756.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Xray has no selector outbound, so a single config cannot offer the user a choice of group: only a routing rule can point at a balancer. That left every automatic group invisible in the client, and the generated config carried no `remarks` at all, so it showed up unnamed. Clients do let the user switch between whole configs, and the legacy Xray subscription already ships a JSON array, so each automatic group is now published as its own entry: "Auto (primary)", "Auto (DE)", "Auto (ES)". Pools can override the label via the new optional `ProfilePool.title`; `id` stays machine-readable because routing rules address it. Sing-box is untouched: its selector/urltest groups already render as a picker inside one config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the `title` field to the profile schema and the pool editor, so the label a client shows for an automatic group can be set without hand-editing JSON, and documents the array output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerated with orval 8.22.0 from the schema produced by scripts/export_openapi.py, adding `useUserSubscriptionProfile` and `useGetUserSubscriptionProfilePreview`. Purely additive: +291 lines, no deletions. Also excludes `*.test.ts` from the app tsconfig. These run under `bun test`, which transpiles them itself, so the only thing tsc did with them was fail to resolve `bun:test`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5f69f0c to
5243742
Compare
…able Automatic groups alone do not let a user pick one specific server, so every endpoint is now published as its own entry alongside them. These carry no balancer or observatory: the catch-all rule names the endpoint's outbound directly. Three settings that were previously hardcoded: - `domain_strategy` (default AsIs, unchanged). This one matters: under AsIs Xray never resolves a domain, so IP-based rules such as `geoip:*` only match traffic that already arrived as an IP. Rulesets mixing domain and IP matching need IPIfNonMatch. - `balancer_strategy` (default random, unchanged) — random/roundRobin/ leastPing/leastLoad. - `health_check.burst`, emitting `burstObservatory` with a `pingConfig` instead of `observatory`. leastPing and leastLoad need measured latency, which plain observatory does not collect; it only tracks alive/dead for fallbackTag. Endpoint ordering moves into a shared `_ordered_endpoints` helper so tag assignment cannot drift between the group configs and the per-endpoint ones: `_endpoint_tags` keys on object identity over the pool-filtered set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds domain strategy, balancer strategy, per-server publishing and latency probing to the profile editor, with the caveats that matter inline: AsIs never resolves domains so geoip rules silently never match, and leastPing/leastLoad do nothing without burst probing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orts one The profile could declare `incy` or `v2rayn` as its client, but neither did anything: only `happ` ever produced a header, so the other two were dead enum values promising support that did not exist. Happ, INCY and v2rayTun all read a header named `routing` and all three disagree on its value, so the payload is now validated against the declared client instead of being assumed to be a Happ deeplink: - happ happ://routing/add|onadd/<b64>, or happ://routing/off - incy the same Happ profile, scheme optional or absent, bare base64 too - v2raytun bare base64 of an Xray routing object; a deeplink there is inert - generic rejects a payload outright, since no header would be sent `v2rayn` is dropped rather than implemented: v2rayNG, v2rayN and Streisand have no mechanism for importing routing rules from a subscription, so offering the option only misleads. Happ additionally gains `routing_enabled: false`, which sends `routing-enable: 0`. `happ_deeplink` stays as a validation alias for the renamed `routing_payload`, so profiles written before this keep parsing. The editor now explains the split the documentation makes: rules inside the config are what a client applies when it receives full JSON — Happ ignores its own routing rules in that mode — while the header only matters for clients still being served share links. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Sing-box client shows an outbound's tag verbatim, so the group tags are what appears in its picker. They were machine-readable, meaning the user chose between `pg-select-primary`, `pg-country-de` and `pg-auto-country-de`. Pools now use their title (falling back to the id) and countries their code, so the picker reads `Fastest`, `Fastest · Auto`, `DE`, `DE · Auto`. Two pools may legitimately share a title, and unlike a cosmetic clash a duplicate tag makes the config invalid, so duplicates get a numeric suffix. This mirrors the `remarks` labelling already applied to the Xray configs; Xray keeps machine-readable balancer tags because there the tag is never displayed and operator-authored routing rules address it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rerowros
changed the title
Rerowros
changed the title
Summary
Configure routing, DNS and optional automatic server selection for an application in one place. Open Applications, choose Happ, Mihomo / Clash, Sing-box or Xray, edit the configuration, preview the actual subscription for a user, and save the document together with its delivery rule.
This PR contains the complete workflow and supersedes #887. It targets
devwithout requiring #756, #758, #458 or #880. No database migration or node upgrade is needed. Partially addresses #635.Behavior
happ://routing/onadd/...line alongside plaintext server links. The JSONNameremains independent of the library label. Optional header delivery rejects values exceeding 2048 bytes and conflicts with manual routing headers. Full Xray mode uses its own document.urltestgroups. Empty optional groups are omitted. Sing-box does not promise strict fallback; Xray fallback names a concrete available endpoint.Compatibility and correctness
client_templates; delivery assignments use existing subscription rules. Nativetemplate_id, generatorprofile_idand managedhapp_routingare mutually exclusive sources. The first matching User-Agent rule retains control of delivery.409for concurrent edits. Referenced documents cannot be deleted singly or in bulk; reference checks use current locking reads.Validation
644a4342is green: SQLite, PostgreSQL, TimescaleDB, MySQL and MariaDB migration/test jobs; dashboard tests/build; isolated MySQL reference regressions (2 passed); Ruff; Python and TypeScript CodeQL. Database matrix, client settings checks.Actual device import/update and traffic routing in Happ and the other applications still require device-level acceptance; core validation and preview do not establish those results.
Review guide
The inherited FinalMask test fixture is aligned with the array-packet schema already on
dev; this is test-only.