[management] update account delete with proper proxy domain and service cleanup#5817
Merged
pascal-fischer merged 5 commits intomainfrom Apr 10, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved the application-layer bulk service deletion call from DefaultAccountManager.DeleteAccount; SqlStore.DeleteAccount now deletes Changes
Sequence Diagram(s)mermaid rect rgba(200,200,255,0.5) rect rgba(200,255,200,0.5) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
management/server/types/account.go (1)
105-105: Consider enforcing DB-level cleanup forDomainsassociation.At Line 105,
Domainsis a new account-scoped relation, but the tag does not enforce cascade deletion. Adding an explicit constraint makes account cleanup resilient even if delete paths change later.♻️ Suggested hardening
- Domains []*proxydomain.Domain `gorm:"foreignKey:AccountID;references:id"` + Domains []*proxydomain.Domain `gorm:"foreignKey:AccountID;references:id;constraint:OnDelete:CASCADE"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/account.go` at line 105, The Domains association on the Account model lacks a DB-level cascade constraint; update the Domains field tag (on the Account struct) to include a GORM constraint that enforces cascade delete (and optionally cascade update) so associated proxydomain.Domain rows are removed when an Account is deleted (e.g., add a gorm tag like `constraint:OnDelete:CASCADE,OnUpdate:CASCADE` to the Domains field).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/store/sql_store.go`:
- Around line 399-404: The transaction that removes an Account (in
DeleteAccount) currently deletes Services, Policies, and UsersG but omits
Domains, leaving orphaned Domain rows; add an explicit deletion of
account.Domains via the same tx.Select(clause.Associations).Delete(...) pattern
(filtering by account.Id or account_id) before the final tx.Delete(account) so
Domains are removed in the same transaction; update the DeleteAccount
transaction to call the deletion for account.Domains (similar to how
account.Services is deleted) and check/return result.Error as done for the other
deletes.
---
Nitpick comments:
In `@management/server/types/account.go`:
- Line 105: The Domains association on the Account model lacks a DB-level
cascade constraint; update the Domains field tag (on the Account struct) to
include a GORM constraint that enforces cascade delete (and optionally cascade
update) so associated proxydomain.Domain rows are removed when an Account is
deleted (e.g., add a gorm tag like
`constraint:OnDelete:CASCADE,OnUpdate:CASCADE` to the Domains field).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b53b58b4-cc0b-4ef8-bde6-73e8e6df0c68
📒 Files selected for processing (3)
management/server/account.gomanagement/server/store/sql_store.gomanagement/server/types/account.go
💤 Files with no reviewable changes (1)
- management/server/account.go
Comment on lines
+399
to
404
| result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id) | ||
| if result.Error != nil { | ||
| return result.Error | ||
| } | ||
|
|
||
| result = tx.Select(clause.Associations).Delete(account) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Account model associations =="
rg -n 'Services\s+\[\]\*service\.Service|Domains\s+\[\]\*proxydomain\.Domain' management/server/types/account.go
echo
echo "== Domain model delete constraint check =="
rg -n 'type Domain struct|AccountID|constraint|OnDelete' management/internals/modules/reverseproxy/domain/domain.go -A8 -B2
echo
echo "== DeleteAccount cleanup steps =="
rg -n 'func \(s \*SqlStore\) DeleteAccount|Delete\(account\.Policies|Delete\(account\.UsersG|Delete\(account\.Services|Delete\(account\.Domains|Delete\(account\)' management/server/store/sql_store.go -A30 -B5Repository: netbirdio/netbird
Length of output: 4423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full DeleteAccount function context =="
sed -n '385,419p' management/server/store/sql_store.go
echo
echo "== Checking for any domain cleanup elsewhere =="
rg -n 'Domains.*Delete|DeleteDomain|domain.*orphan|cascade.*domain' management/server/store/sql_store.go
echo
echo "== Checking Account model for Domains association details =="
sed -n '100,110p' management/server/types/account.go
echo
echo "== Checking if there are transaction hooks or other cleanup paths =="
rg -n 'func.*Delete|OnDelete|cascade' management/internals/modules/reverseproxy/domain/domain.goRepository: netbirdio/netbird
Length of output: 1117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking cascade delete behavior in other delete operations =="
rg -n 'Select\(clause\.Associations\)\.Delete' management/server/store/sql_store.go -B2 -A2
echo
echo "== Looking for account deletion tests =="
fd -t f '.*test.*\.go$' | xargs rg -l 'DeleteAccount' | head -5
echo
echo "== Checking if Domains have any cascade constraints defined =="
rg -n 'type Domain struct' management/internals/modules/reverseproxy/domain/domain.go -A30 | head -40
echo
echo "== Checking recent changes or migrations related to domains =="
fd -t f '*.go' management/server/store | xargs rg -l 'Domain' | grep -i migratRepository: netbirdio/netbird
Length of output: 2975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DeleteAccount test cases =="
rg -n 'DeleteAccount|func Test.*Delete' management/server/store/sql_store_test.go -A20 | head -80
echo
echo "== Domain-related assertions in tests =="
rg -n 'Domain.*assert|assert.*Domain|orphan' management/server/store/sql_store_test.go
echo
echo "== Check if Domains cleanup is tested =="
rg -n 'Domains|proxydomain' management/server/store/sql_store_test.go | head -10Repository: netbirdio/netbird
Length of output: 4631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if there's a test for domain orphaning (searching for domain-related test) =="
rg -n 'Domain|proxydomain' management/server/store/sql_store_test.go -B2 -A5 | grep -A5 -B5 'Delete'
echo
echo "== Looking for any domain cleanup or cascading logic =="
rg -n 'type Account struct' management/server/types/account.go -A50 | grep -E 'Domains|Services'
echo
echo "== Checking if there are database migrations or constraints for domains =="
fd -t f 'migrat' management/server/store | xargs cat 2>/dev/null | grep -i 'domain\|account_id' | head -20Repository: netbirdio/netbird
Length of output: 1384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding migration/schema files =="
find . -type f \( -name '*migrat*' -o -name '*.sql' \) | grep -v node_modules | grep -v '.git' | head -20
echo
echo "== Checking if GORM cascade delete works with the foreignKey pattern =="
rg -n 'onDelete|ON DELETE|cascade' management/server/types/ management/internals/modules/reverseproxy/domain/
echo
echo "== Looking for when Services deletion was added (search for Services delete pattern) =="
rg -n 'Delete\(account\.Services' management/server/store/sql_store.go -B10 -A5Repository: netbirdio/netbird
Length of output: 1205
🏁 Script executed:
#!/bin/bash
set -euco pipefail
echo "== Checking migration.go for domain table definition =="
cat management/server/store/migration.go | grep -A20 'type domains\|Domain table' || echo "Searching differently..."
echo
echo "== Searching for Domain table creation in migrations =="
rg -n 'CreateTable.*domain\|domains.*table' management/server/store/ -i
echo
echo "== Checking GORM migration syntax for cascade constraints =="
rg -n 'ForeignKey.*onDelete|OnDelete\|constraint' management/server/store/migration.go -B2 -A2
echo
echo "== Looking at how Services table is created vs Domains =="
rg -n 'CreateTable.*service\|CreateTable.*domain' management/server/store/migration.go -i -A10 | head -50Repository: netbirdio/netbird
Length of output: 677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking migration_up.go =="
ls -la management/cmd/migration_up.go && wc -l management/cmd/migration_up.go
echo
echo "== Searching for Domain table AutoMigrate in store code =="
rg -n 'AutoMigrate.*Domain\|Domain.*AutoMigrate' management/server/store/ -B2 -A2
echo
echo "== Looking for main store initialization =="
rg -n 'func.*New.*SqlStore\|AutoMigrate' management/server/store/sql_store.go -A30 | head -60
echo
echo "== Finding where models are migrated =="
fd -t f 'sql_store.go' management/server/store/ | xargs rg -n 'AutoMigrate' -B5 -A10Repository: netbirdio/netbird
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking how db is initialized and tables created =="
rg -n 'func.*Open.*\|gorm\.Open\|AutoMigrate\|CreateTable' management/server/store/sqlite.go -B2 -A8 | head -80
echo
echo "== Looking for all GORM model tags for Account associations =="
rg -n 'Services.*gorm\|Domains.*gorm' management/server/types/account.go -B1 -A1
echo
echo "== Checking if there are onDelete tags on the Domain or Service models =="
rg -n 'onDelete\|OnDelete' management/server/types/ management/internals/modules/reverseproxy/ -iRepository: netbirdio/netbird
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== List files in store directory =="
ls -la management/server/store/ | grep -E '\.go$|^total'
echo
echo "== Find where DB tables are created/migrated =="
rg -n 'gorm\.DB\|Migrator\|CreateTable' management/server/store/*.go | head -30
echo
echo "== Check if GORM's Select(clause.Associations) cascades on Account.Domains =="
rg -n 'Select\(clause\.Associations\)\.Delete' management/server/store/*.go -B5 -A5Repository: netbirdio/netbird
Length of output: 867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Looking at sql_store.go initialization =="
head -100 management/server/store/sql_store.go
echo
echo "== Searching for how DB handles cascade deletes in current code =="
rg -n 'Select\(clause\.Associations\)|clause\.Associations' management/server/store/sql_store.go
echo
echo "== Check the Account model associations more carefully =="
sed -n '95,115p' management/server/types/account.goRepository: netbirdio/netbird
Length of output: 6699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check constraint tags on Account associations =="
rg -n 'gorm:' management/server/types/account.go | grep -E 'Services|Domains|Policies|Users|Onboarding'
echo
echo "== Check if Domain cleanup is actually tested =="
rg -n 'GetDomain\|proxydomain\|Domains' management/server/store/sql_store_test.go | grep -i 'delete\|account'
echo
echo "== Verify if domains would be orphaned after account deletion =="
rg -n 'Domains\|domain.*delete\|orphan' management/server/store/sql_store_test.goRepository: netbirdio/netbird
Length of output: 792
Add explicit Domain deletion in DeleteAccount transaction.
Lines 399-404 clean up Services, Policies, and UsersG before deleting the account, but Domains are missing. Without the OnDelete:CASCADE constraint (unlike Onboarding), Domain rows will be orphaned when the account is deleted.
Proposed fix
result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id)
if result.Error != nil {
return result.Error
}
+ result = tx.Select(clause.Associations).Delete(account.Domains, "account_id = ?", account.Id)
+ if result.Error != nil {
+ return result.Error
+ }
+
result = tx.Select(clause.Associations).Delete(account)
if result.Error != nil {
return result.Error
}📝 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.
Suggested change
| result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id) | |
| if result.Error != nil { | |
| return result.Error | |
| } | |
| result = tx.Select(clause.Associations).Delete(account) | |
| result = tx.Select(clause.Associations).Delete(account.Services, "account_id = ?", account.Id) | |
| if result.Error != nil { | |
| return result.Error | |
| } | |
| result = tx.Select(clause.Associations).Delete(account.Domains, "account_id = ?", account.Id) | |
| if result.Error != nil { | |
| return result.Error | |
| } | |
| result = tx.Select(clause.Associations).Delete(account) | |
| if result.Error != nil { | |
| return result.Error | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/store/sql_store.go` around lines 399 - 404, The transaction
that removes an Account (in DeleteAccount) currently deletes Services, Policies,
and UsersG but omits Domains, leaving orphaned Domain rows; add an explicit
deletion of account.Domains via the same
tx.Select(clause.Associations).Delete(...) pattern (filtering by account.Id or
account_id) before the final tx.Delete(account) so Domains are removed in the
same transaction; update the DeleteAccount transaction to call the deletion for
account.Domains (similar to how account.Services is deleted) and check/return
result.Error as done for the other deletes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/store/sqlstore_bench_test.go (1)
262-270: Consider seeding at least one Domain row to exercise this schema path.Line 269 correctly adds
&domain.Domain{}to migration setup. As a follow-up, add a minimal Domain fixture and equivalence assertion so this change is behaviorally covered (not just schema-present).Suggested follow-up (example)
const ( accountID = "benchmark-account-id" + domainID = "benchmark-domain-id" numUsers = 20 ... ) ... if err := db.Create(&acc).Error; err != nil { b.Fatalf("create account: %v", err) } +dom := domain.Domain{ + ID: domainID, + AccountID: accountID, + Domain: "svc.benchmark.com", +} +if err := db.Create(&dom).Error; err != nil { + b.Fatalf("create domain: %v", err) +}And in
testAccountEquivalence(...), add a Domains equality check once domain fixtures are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sqlstore_bench_test.go` around lines 262 - 270, Add a minimal Domain fixture to the migration/seed dataset so the Domain schema path is exercised: include a populated instance (e.g. with Name/AccountID or other required fields) alongside the existing &domain.Domain{} entry in the models/seed setup, and update the testAccountEquivalence(...) assertions to compare Domains as well (add a Domains equality check or include Domain fields in the existing account equivalence check) so the new Domain row is behaviorally validated rather than only present in the schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/store/sqlstore_bench_test.go`:
- Around line 262-270: Add a minimal Domain fixture to the migration/seed
dataset so the Domain schema path is exercised: include a populated instance
(e.g. with Name/AccountID or other required fields) alongside the existing
&domain.Domain{} entry in the models/seed setup, and update the
testAccountEquivalence(...) assertions to compare Domains as well (add a Domains
equality check or include Domain fields in the existing account equivalence
check) so the new Domain row is behaviorally validated rather than only present
in the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2837277c-527f-4894-9622-764b7a1b85fd
📒 Files selected for processing (1)
management/server/store/sqlstore_bench_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
management/server/account_test.go (1)
1820-1826: Consider extending this test to assert domain pointer-field copy isolation.This fixture is a great start; adding assertions for
SupportsCustomPorts/RequireSubdomainwould catch aliasing regressions inDomain.Copy().Possible test enhancement
+ supportsCustomPorts := true + requireSubdomain := true account := &types.Account{ @@ Domains: []*domain.Domain{ { ID: "domain1", Domain: "test.com", AccountID: "account1", + SupportsCustomPorts: &supportsCustomPorts, + RequireSubdomain: &requireSubdomain, }, }, @@ accountCopy := account.Copy() @@ + *account.Domains[0].SupportsCustomPorts = false + *account.Domains[0].RequireSubdomain = false + require.True(t, *accountCopy.Domains[0].SupportsCustomPorts) + require.True(t, *accountCopy.Domains[0].RequireSubdomain)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/account_test.go` around lines 1820 - 1826, Extend the test fixture to verify Domain.Copy() creates independent pointer-field copies by: for each domain in the fixture call copied := domain.Copy(), assert copied.SupportsCustomPorts and copied.RequireSubdomain match the original values, then mutate the original domain's SupportsCustomPorts and RequireSubdomain and assert the copied values do not change (and vice versa); use the Domain.Copy() helper and the SupportsCustomPorts/RequireSubdomain fields to locate the code and ensure no aliasing between original and copied pointer fields.management/server/types/account.go (1)
916-919: Optional: preallocatedomainsslice capacity.Tiny improvement for large accounts:
Suggested micro-refactor
- domains := []*proxydomain.Domain{} + domains := make([]*proxydomain.Domain, 0, len(a.Domains)) for _, domain := range a.Domains { domains = append(domains, domain.Copy()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/account.go` around lines 916 - 919, The loop building domains currently appends into an empty slice which causes reallocations for large accounts; preallocate the slice capacity using make with length or capacity based on len(a.Domains) (e.g., initialize domains := make([]*proxydomain.Domain, 0, len(a.Domains))) and then append each domain.Copy() from a.Domains (symbols: domains, a.Domains, Domain.Copy()) so the slice grows without repeated allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/account_test.go`:
- Around line 1820-1826: Extend the test fixture to verify Domain.Copy() creates
independent pointer-field copies by: for each domain in the fixture call copied
:= domain.Copy(), assert copied.SupportsCustomPorts and copied.RequireSubdomain
match the original values, then mutate the original domain's SupportsCustomPorts
and RequireSubdomain and assert the copied values do not change (and vice
versa); use the Domain.Copy() helper and the
SupportsCustomPorts/RequireSubdomain fields to locate the code and ensure no
aliasing between original and copied pointer fields.
In `@management/server/types/account.go`:
- Around line 916-919: The loop building domains currently appends into an empty
slice which causes reallocations for large accounts; preallocate the slice
capacity using make with length or capacity based on len(a.Domains) (e.g.,
initialize domains := make([]*proxydomain.Domain, 0, len(a.Domains))) and then
append each domain.Copy() from a.Domains (symbols: domains, a.Domains,
Domain.Copy()) so the slice grows without repeated allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de4a2abf-ac4c-4eca-af83-1f83aaaef758
📒 Files selected for processing (3)
management/internals/modules/reverseproxy/domain/domain.gomanagement/server/account_test.gomanagement/server/types/account.go
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/store/sql_store_test.go (1)
446-458: Add Postgres parity for the new cleanup assertions.Line 446 onward validates domain/service/target cleanup only in
TestSqlite_DeleteAccount. Please mirror this inTestPostgresql_DeleteAccountso cross-engine delete behavior is guarded too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store_test.go` around lines 446 - 458, The Postgres test TestPostgresql_DeleteAccount is missing the cleanup assertions added to TestSqlite_DeleteAccount: after DeleteAccount call, call store.ListCustomDomains(ctx, account.Id) and assert no error and zero length, then query the underlying SqlStore db for rpservice.Service and rpservice.Target (using db.Model(&rpservice.Service{}).Find(&services, "account_id = ?", account.Id).Error and similarly for rpservice.Target) and assert no error and length 0 for each; mirror the exact checks from TestSqlite_DeleteAccount into TestPostgresql_DeleteAccount to ensure parity across engines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/store/sql_store_test.go`:
- Around line 446-458: The Postgres test TestPostgresql_DeleteAccount is missing
the cleanup assertions added to TestSqlite_DeleteAccount: after DeleteAccount
call, call store.ListCustomDomains(ctx, account.Id) and assert no error and zero
length, then query the underlying SqlStore db for rpservice.Service and
rpservice.Target (using db.Model(&rpservice.Service{}).Find(&services,
"account_id = ?", account.Id).Error and similarly for rpservice.Target) and
assert no error and length 0 for each; mirror the exact checks from
TestSqlite_DeleteAccount into TestPostgresql_DeleteAccount to ensure parity
across engines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d90f8bd-517b-40d1-990a-ed03449c447f
📒 Files selected for processing (1)
management/server/store/sql_store_test.go
bcmmbaga
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Refactor
Tests