fix(backend): check the account limit only when add_account creates an account - #8041
Open
yhabib wants to merge 1 commit into
Open
fix(backend): check the account limit only when add_account creates an account#8041yhabib wants to merge 1 commit into
yhabib wants to merge 1 commit into
Conversation
…n account add_account called assert_account_limit before it looked the caller up. At the limit the call trapped for a caller that already had an account, and it trapped before the one-off backfill that sets the principal on a legacy account. A legacy user then lost access to stored data, because get_account keeps returning None until the backfill runs. Move the check into the branch that creates a new account. A new principal still traps at the limit, which is the intended DoS protection. Pass the limit as a parameter to add_account_with_limit so a test can reach it without 330,000 accounts. Fix the stale panic text: the PRE_MIGRATION_LIMIT name went away, but the message still said "Pre migration".
|
✅ No security or compliance issues detected. Reviewed everything up to 165bb42. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟢 Approved
The change is small, directly addresses the stated failure mode, and is covered by targeted new tests exercising both limit and legacy-backfill scenarios.
Pull request overview
This PR adjusts AccountsStore::add_account so the account-limit check only runs when a new account would be created, preventing traps for existing accounts (including legacy accounts that need principal backfill) when the store is at its limit.
Changes:
- Refactored
add_accountto delegate to a newadd_account_with_limithelper and moved the limit assertion into the “create new account” branch. - Updated the account-limit panic message to “Account limit exceeded …”.
- Added focused unit tests covering: trap-at-limit for new accounts, non-trap behavior for existing accounts, legacy principal backfill at limit, and successful create below limit.
File summaries
| File | Description |
|---|---|
rs/backend/src/accounts_store.rs |
Moves the limit check to only apply when inserting a new account; introduces add_account_with_limit for testability; updates panic message. |
rs/backend/src/accounts_store/tests.rs |
Adds unit tests validating limit behavior for new/existing/legacy accounts and below-limit creation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Motivation
add_accountchecked the account limit before it looked up the caller. At the limit, the call trapped for every caller, even one that already had an account. A trap rolls back state, so the one-off backfill that setsprincipalon a legacy account never ran. A legacy user then lost access to stored data, becauseget_accountkept returningNoneuntil the backfill ran, and the frontend retriedadd_accounton every sign-in and failed every time.Two callers reach
add_account: theadd_accountupdate endpoint (the only production caller), and a toy-data helper used only undertestortoy_data_gen. The fix affects only the production endpoint.Mainnet held 308,885 accounts against a limit of 330,000 on 2026-09-04, so the trap is not theoretical.
Changes
add_account_with_limit, a private function that takes the limit as a parameter, soadd_accountcan still trap a new caller atACCOUNT_LIMIT.Tests
Added four tests in
rs/backend/src/accounts_store/tests.rs, each against a small store with a passed-in limit:falseat the limit and does not trap.principal: Nonegets the backfill at the limit and does not trap.true.Ran
./scripts/lint-rs,cargo test, andcargo spellcheck -- --code 1. All pass.Todos
ACCOUNT_LIMITand the Candid interface are unchanged, and mainnet has not hit the limit yet.