feat: seedphrase account system + bulk-delete offline clients#406
Open
Ryanmello07 wants to merge 51 commits into
Open
feat: seedphrase account system + bulk-delete offline clients#406Ryanmello07 wants to merge 51 commits into
Ryanmello07 wants to merge 51 commits into
Conversation
…tions Adds three tables: - network_user_auth_seedphrase: BIP39 seedphrase auth storage - network_name_reclaim: 24h cooldown pool for changed network names - network_create_attempt: 5-per-IP-per-day seedphrase signup rate limit
…rate limit - seedphrase_controller: errors returned in result struct (not fmt.Errorf) - auth_controller: RemoveAuth errors returned in result struct - account_controller: accept both network_name and new_name, return name - auth_model: seedphrase login errors returned in result, not Go error - network_model: seedphrase path skips old auth rate limit, uses new one - db_migrations: add network_create_attempt table - network_create_rate_limit: new file with 5/ip/day limit for seedphrase
… name Seedphrase-only users (or seedphrase + wallet only) can't call claim-name or change-name until they bind an email, phone, or social login. Returns a clear error message guiding them.
requireEmailOrSsoBound now checks verified=true on network_user_auth_password instead of just existence. SSO auths (Apple/Google) are inherently verified by the provider so they still count without a verified column.
Mirrors urnetwork#399 but redesigned for production scale: a sync path for small requests (<=10,000 ids, applied inline) and an async path for larger ones, handed off to a single background task that self-chunks in bounded batches and reschedules its own remainder rather than looping unboundedly or requiring the caller to chunk requests themselves. Also fixes: - missing deactivate_time on the bulk path, which would have let the reap job's 30-day grace period be skipped for bulk-deleted clients - an unbounded per-request size with no batching, which doesn't hold up against real per-network client counts Adds task.ScheduleTaskInTxIfAbsent (task/task.go) for atomic schedule-if-not-already-pending semantics, since the existing RunOnce merge-on-conflict path only updates timing/priority on an existing pending row, not its args -- reusing it naively for duplicate-request rejection would silently drop a racing duplicate's client_ids while still reporting success.
…t gap Two independent review passes (DeepSeek, Gemini 3.1 Pro) confirmed no critical issues; this addresses their medium/low findings: - guard against a nil session.ByJwt in RemoveNetworkClientsTask rather than panicking on a nil dereference inside a MaintenanceTx - clarify why RemoveNetworkClientsTaskPost's reschedule uses plain ScheduleTaskInTx rather than the IfAbsent variant (no possible conflict: the finishing task's own row is deleted in the same tx) - lower MaxRemoveNetworkClientsCount 5M -> 2M: still 2x headroom over known ~1M real-world usage, materially smaller worst-case args_json payload on the request path - assert deactivate_time is stamped in the real-worker lifecycle test, the only test exercising multiple task invocations
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ling - nil session.ByJwt in RemoveNetworkClientsTask now has coverage (the guard added in the prior commit had none) - boundary tests at exactly RemoveNetworkClientsBatchCount (still sync) and exactly MaxRemoveNetworkClientsCount (still accepted) -- previously only the "+1 over" side of each boundary was tested - a fast, isolated test of RemoveNetworkClientsTaskPost's reschedule logic (same run_once key, different network unaffected), as a complement to the slow 205k-row real-worker lifecycle test
…d, index, reduced cap, guest rejection - Fix #1 (DoS): Add http.MaxBytesReader(100MB) to body read + reject guest-mode callers - Fix #3 (abuse): Switch to WrapWithInputRequireAuthNoGuest - Fix #4 (perf): Add network_client_network_id_client_id index migration - Fix #5 (correctness): restored original panic-based error propagation (by convention) - Fix urnetwork#6 (quality): Deduplicate client IDs in RemoveNetworkClients input - Fix urnetwork#7 (scale): Lower MaxRemoveNetworkClientsCount from 2M to 1M - Copilot suggestion 1: Return zero server.Id{} when ScheduleTaskInTxIfAbsent is not scheduled - Copilot suggestion 2: Add nil-ByJwt guard to RemoveNetworkClientsTaskPost
router_test.go still referenced WrapRequireAuthNoGuest/WrapWithInputRequireAuthNoGuest and guest-mode JWTs, which no longer exist after guest mode removal. Strip the guest-specific test routes and assertions. network_client_handlers.go's RemoveNetworkClients also called the removed WrapWithInputRequireAuthNoGuest wrapper; switch to WrapWithInputRequireAuth.
Removes stale references to model.UpgradeGuestArgs/UpgradeGuestResult and model.UpgradeGuestExistingArgs (types removed with guest mode). Adds registry entries for the new seedphrase and account-management endpoints: /auth/add-auth, /auth/remove-auth, /auth/regenerate-seedphrase, /auth/generate-seedphrase, /account/change-name, /account/claim-name. Note: the connect/api/bringyour.yml OpenAPI spec (separate repo) still lists the removed /auth/upgrade-guest* routes and doesn't yet list the new seedphrase/account routes. That spec update is out of scope for this PR and should be coordinated separately in the connect repo.
These result/error types (UpgradeGuestNetwork, UpgradeGuestResultVerification, UpgradeGuestError, UpgradeGuestExisting*) had no remaining callers after the UpgradeGuest/UpgradeFromGuestExisting functions were removed. Keeping Network.GuestUpgradeNetworkId and Testing_CreateGuestNetwork, which still reflect real DB state for existing (un-migrated) guest accounts.
AddAuth() was swallowing errors from addUserAuth(), addSsoAuth(), and
addWalletAuth(). If a user tried to add a second email (same auth_type),
SSO provider, or wallet, the function silently returned {} with no
error message despite the DB correctly rejecting the duplicate via PK
constraint. Now each path checks the returned error and surfaces it
as a proper JSON error response.
Contributor
Author
🐛 Fix: Error propagation in AddAuth (cherry-picked 2025-07-17)When a user tried to add a second email (same auth_type), SSO provider, or wallet to an account, the DB-level PK constraint correctly rejected the duplicate — but the error was silently swallowed and the API returned {} with no error message. Fixed by capturing errors from addUserAuth(), addSsoAuth(), and addWalletAuth() inside AddAuth() and returning them as proper JSON error responses. This applies to all auth types: email, phone, Google, Apple, wallet. |
RemoveAuth('solana') only deleted the network_user_auth_wallet row, but the
network_user.wallet_address and wallet_blockchain columns were left set,
preventing that wallet from creating a new account. Now they are cleared so
the wallet can be reused for a fresh signup.
Contributor
Author
🐛 Fix: Wallet can be reused after being removed
|
RemoveAuth only deleted auth rows but never updated the denormalized auth_type column on network_user. After removing wallet auth, the app still showed 'sign in with wallet' because auth_type remained 'solana'. Now every RemoveAuth path re-derives auth_type from whichever auth tables still have rows, in priority order: password > apple > google > solana > seedphrase.
NetworkUser struct now includes: - SeedphraseAuths: list of linked seedphrase auths (mirrors WalletAuths) - AuthTypes: flat string array of all linked auth methods e.g. ['solana', 'seedphrase'] or ['solana', 'seedphrase', 'email'] The app can use this directly instead of guessing from auth_type.
…pe panic countAuthMethods previously ran outside the deletion transaction, so two concurrent RemoveAuth calls for different auth types could both read the same pre-deletion count, both pass the "keep at least one method" guard, and both commit against different rows in different auth tables (no conflict under RepeatableRead) -- leaving the account with zero auth methods and no recovery path short of DB intervention. Move the count query inside the deletion tx behind a per-user `FOR UPDATE` row lock so a concurrent call blocks and retries (via server.Tx's existing serialization-failure retry) against the reduced count instead. Also replace the panic on an unrecognized auth_type with a returned error, matching the existing "last auth method" validation pattern -- user-controlled input reaching the default case previously crashed the request with a 500 and a stack-trace log entry instead of a clean error.
AuthLogin pre-inserts a success=false user_auth_attempt row and relies on each login branch to flip it to success afterward. The password and SSO branches did this; the seedphrase and wallet branches did not. Since seedphrase/wallet logins have a nil userAuth, UserAuthAttempt's rate limiter counts their failures per client IP only (5 in 5 minutes). With success never recorded, 5 successful seedphrase or wallet logins from one IP -- trivial on any shared NAT (household, office, mobile carrier CGNAT) -- exhausts that budget and locks out all subsequent auth from that IP, including unrelated users' SSO/password logins, for the rate-limit window. Seedphrase is now the primary login path (guest mode was removed), so this was reachable in ordinary use, not just adversarially.
jwt.NewByJwt panics on a zero-value networkId. If a seedphrase auth row ever outlives its network_user/network (this branch's RemoveNetwork already deletes it, but the defensive gap remains for any other path that could orphan the row), a subsequent login would pass the hash/salt check and then panic on the network join instead of failing cleanly, turning an edge case into a 500 on a public endpoint.
RemoveNetwork's batch delete covers network_user, network_user_auth_wallet, network_user_auth_password, and network_user_auth_sso, but not network_user_auth_seedphrase -- so deleting a network left the seedphrase auth row (hash, salt, lookup) behind. A later login attempt with that seedphrase would pass the argon2 check, then find no matching network_user on the join and panic on a zero-value networkId in jwt.NewByJwt (guarded separately in LoginWithSeedphrase). Also permanently blocked reusing that seedphrase for a new account since the lookup column is unique. (This fix already existed on the fork's feat/seedphrase-account-system branch -- ported by hand here since a mechanical cherry-pick would have been a no-op against this branch's history.)
…tests 58 assert.Equal/assert.NotEqual call sites had no import for github.com/go-playground/assert/v2, so the whole model test package failed to compile. Once fixed, TestRemoveNetworkClientsRejectsOversizedRequest and TestRemoveNetworkClientsExactlyAtCapIsAccepted would still fail: both seeded their client-id slice with make([]server.Id, N), which is all zero-value UUIDs. RemoveNetworkClients deduplicates ids before checking the length cap, so N identical zero ids collapse to length 1 and never exercise the boundary being tested. Seed with distinct server.NewId() values instead, matching the existing correct idiom in TestRemoveNetworkClientsUUIDArrayBinding.
The second half of TestNetworkUser used bare assert.Equal/assert.NotEqual with no import (undefined: assert) -- this file otherwise uses connect.AssertEqual throughout. It also still asserted guest/seedphrase-shaped values (UserAuth == nil, Verified == false, AuthType == AuthTypeSeedphrase) against a user created by Testing_CreateNetwork, which unconditionally creates a password user (UserAuth set, Verified: true, AuthType: AuthTypePassword). The block was mechanically migrated from a guest-network helper without updating its assertions, so it could never pass. No seedphrase-creating test helper exists to restore the original intent, so this switches to asserting what Testing_CreateNetwork actually produces, matching the pattern already used earlier in the same test.
The cherry-picked fix used connect.AssertEqual, matching the fork branch's convention where this file already imported "connect". This branch's copy of the file was already migrated to go-playground/assert/v2 throughout and never imports "connect" -- adjusting to match.
networkCreateSeedphrase's only caller (NetworkCreate) guards entry on networkCreate.AuthJwt == nil, so the "if AuthJwt provided, bind SSO too" branch inside it could never execute -- dead code that also silently swallowed a bind error via glog.Infof had it ever run, which would have been a latent trap if the outer guard were ever loosened later.
changeNetworkName's reclaim-cooldown INSERT fired whenever an old name existed, with no check that the new name actually differs from it. A no-op rename (idempotent retry, resend, a flaky client re-POST with the same name) would put the user's own current name into network_name_reclaim, and the cooldown lookup has no owner exemption -- so the user would then be locked out of "renaming" back to the name they still hold for 24h. Guard the insert on the name actually changing.
Same class of pre-existing test-compile break found and fixed on the fork's feat/seedphrase-account-system branch, applied to the subset that also exists here (this branch's network_model_test.go, router_test.go, and spec_conformance_test.go were already clean of guest-mode test scaffolding, so only these two needed the fix): - controller/network_controller_test.go: three NetworkCreateArgs struct literals still set a GuestMode field that no longer exists on that type (model/peer_model_test.go:403 also sets GuestMode, but on jwt.ByJwt, which still has that field -- not a compile error, left as-is). - task/task_test.go: a block of assert.Equal calls had no import for github.com/go-playground/assert/v2.
…Auth
The mirroring UPDATE added to sync network_user.wallet_address used
RaisePgResult, which panics on any error -- unlike the sibling INSERT a
few lines above, which returns a clean error. network_user.wallet_address
has a global (not per-user) unique index, so this UPDATE can legitimately
conflict: a legacy account whose network_user.wallet_address was set
before network_user_auth_wallet existed, and never cleared, collides
when a different user links that same address.
Since constraint violations are classified as transient, server.Tx would
retry the whole transaction for up to a minute against the same
permanent conflict before finally re-panicking, surfacing as an opaque
500 instead of the structured AddAuthMethodResult{Error:...} every other
AddAuth failure path returns. Catch it the same way the INSERT does.
A signed wallet challenge proves control of that identity the same way a verified email or SSO login does, so it should count toward requireVerifiedIdentityBound (formerly requireEmailOrSsoBound) just like the other two methods.
…d hung instead of erroring RefreshToken re-signed a new JWT using session.ByJwt.NetworkName -- whatever name was baked into the JWT being refreshed -- instead of re-reading the current name from the DB. A rename via change-name/claim-name would never be picked up until the client happened to get a truly fresh JWT some other way (e.g. logging back in). addWalletAuth attempted the INSERT directly and let a duplicate-wallet unique-constraint violation abort the transaction, then returned the error cleanly (no panic). But server.Tx's default retry options retry any failed COMMIT for up to 60s regardless of whether the underlying cause is retryable -- committing an aborted transaction always fails the same way on every retry, so this stalled every "wallet already taken" request for up to a minute before ever surfacing an error. Added a pre-check SELECT before the INSERT, mirroring the validateUserAuthAvailability pattern addUserAuthInTx already uses for email/phone auth, so the common case fails fast with a clean error instead of ever touching the DB constraint.
16 tasks
…at/seedphrase-to-upstream
Resolved 5 conflicting files (go.mod auto-merged clean on this branch,
unlike beta/self-contained-env, since this curated branch lacks the
CORS/extra-deps additions):
- go.sum: regenerated via go mod tidy.
- db_migrations.go: both sides only appended new migrations; kept both.
- model/network_model.go: took ours (empty) -- same UpgradeGuest
deletion as beta/self-contained-env, though upstream's deleted-region
boundary was larger here (this branch's own cleanup already removed
the UpgradeGuest* types too, not just the function).
- model/device_association_model.go: identical conflict/resolution to
beta/self-contained-env -- took upstream's `adopted = true` close,
dropped our leftover isGuestMode/isPro lines (isPro already computed
earlier post-rewrite; authType == AuthTypeGuest is unreachable now).
- model/peer_model_test.go (the hard one): two real conflicts, not one
mechanical diff:
- TestNetworkPeerRegistryFlushRecovery: adopted upstream's improved
design (re-add a *different* peer during recovery to prove the
resync carries fresh data, not stale accumulator state, plus an
intermediate zero-count assertion), translated to this branch's
assert.Equal/NotEqual style instead of connect.AssertEqual.
- End of TestNetworkPeerTopLevelClientLimit: not a real conflict --
upstream added two entirely new tests (TestNetworkTopLevelClient-
LimitDisabled, TestNetworkProviderConnectionExemptFromLimit) right
after a line we'd also touched (assert-style only). Added both,
translated to assert.Equal/NotEqual.
- Separately (found while resolving, confirmed pre-existing via a
clean-worktree go vet on the unmodified branch, unrelated to this
merge): every NewNetworkPeerListener call in this file was already
missing its second argument (fullReadEvery) against the real,
unchanged 2-arg signature -- a pre-existing compile break on this
branch. Fixed all 10 call sites to match upstream's real signature.
Verified: go build . ./model/... ./controller/... ./cli/api and
go vet on the same, both fully clean -- including peer_model_test.go,
which was the one file genuinely at risk of a bad manual merge.
Add/remove auth method, generate/regenerate seedphrase, and change/claim
network name were previously unlimited, unlike account creation (which
already has a 5/day IP-based limit via network_create_rate_limit.go).
New model/account_action_rate_limit.go mirrors that pattern but keys by
userId instead of client_address_hash, since these are authenticated
account actions rather than pre-auth ones:
- add_auth: 5/day, counted on success only
- remove_auth: 5/day, counted on success only
- generate_seedphrase / regenerate_seedphrase: 5/day each, counted on
success only
- change_network_name (shared by ChangeNetworkName and ClaimNetworkName):
2/day, counted on every real attempt (after format/availability
validation passes), matching the "strict" limit requested for this
action
Success-only counting for add/remove/generate/regenerate avoids the kind
of self-lockout bug already found elsewhere in this port (rejected
attempts don't burn budget). Name change counts attempts rather than
successes since it's explicitly the strict one, but only after cheap
validation so malformed/unavailable-name requests don't cost budget.
One shared network_user_action_attempt table (user_id, action, create_time)
backs all five limits instead of five dedicated tables, since they share
the exact same shape and query pattern.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Covers: limit enforcement (allows up to N, blocks the N+1th), independent counters per (user, action), unrecorded checks never burning budget (the success-only semantics AddAuth/RemoveAuth/generate/regenerate rely on), the atomic check-and-record path used by name change, window expiry, and the maintenance cleanup function. Run against a real local Postgres (server/local/run-local.sh); all 6 pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
…ame budget bundling Two real bugs surfaced by an Opus review of the rate-limiting feature: 1. maxAccountActionAttemptsError() returned a "429 " prefix intended for RaiseHttpError's top-level-error convention, but every call site here embeds the message into a soft JSON Error field with a nil top-level error, so the literal string "429 ..." was shipping to clients verbatim. Removed the prefix and gave each action a human-readable name instead, so a client can tell which of the five different limits it hit. 2. ClaimNetworkName and ChangeNetworkName shared one 2/day counter. Claim is a first-time, once-per-account onboarding action, so it could consume half of a brand-new user's same-day rename budget before they've ever picked a real name -- a normal claim-then-rename-once flow would hit the limit on day one. Gave claim its own action constant and counter (still 2/day), selected via the existing reclaimCooldown flag that already distinguishes the two call paths. Added tests: action constants are pairwise distinct and each produces a specific (non-fallback) error message, and claim/change now have independent counters. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Investigation (with upstream) found validateClientIdentityArgs had regressed from upstream: it checked session.ByJwt.GuestMode to decide whether a caller may assign explicit Roles/Principal to a new client via /network/auth-client or /auth/code-create, but that check is only reliable until a legacy guest account's first token refresh -- which unconditionally re-signs with GuestMode=false regardless of real account state (a separate, accepted-as-is behavior). After that, a bare guest account (auth_type='guest', zero rows in any auth table) could assign arbitrary roles/principal indistinguishably from a real account. Per decision: guest signup is retired and silent upgrade-to-normal-user treatment for legacy guests is acceptable, but this one check should still reflect whether the account has actually gained a real auth method, not the stale JWT claim. Replaced the check with a new HasAnyAuthMethod(ctx, userId) that counts rows across the four auth tables live. This blocks a still-bare guest correctly, and self-heals the moment that guest adds any real method via AddAuth -- no refresh or auth_type backfill required (AddAuth never updates network_user.auth_type, so that column stays 'guest' forever even after upgrade; only a live table check is accurate). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QtgqtCmKJRXdsQ5ktiqwkg
Two deployment-wide guards on RemoveNetworkClients (POST /network/remove-clients), an admin-facing endpoint with no client-app caller, per user decision: cap total blast radius regardless of who triggers it, rather than per-account/network. - A global hourly ceiling (MaxBulkClientRemovalsPerHour = 1,000,000, BulkClientRemovalWindow) on total clients admitted through the bulk API across all networks combined, tracked in a new bulk_client_removal_quota ledger table and checked+recorded atomically before a request is processed (sync or async). The single-client removal API is unaffected -- it has no comparable blast radius. - A global concurrency cap (MaxConcurrentBulkClientRemovalRuns = 10) on how many networks may have a background bulk-delete run in flight or queued at once, using a new task.CountPendingByFunctionName helper against pending_task, so many networks can't all load the task queue/database at the same time. Both are soft admission checks the caller can retry after (mirroring AlreadyInProgress), not hard invariants -- consistent with this codebase's existing account-action rate limiter, which accepts the same small race window under RepeatableRead isolation.
countRecentAccountActionAttempts and CheckAccountActionRateLimit compared `now() - INTERVAL ... <= create_time` in SQL, mixing a timestamptz expression (now()) against a naive `timestamp` column (create_time, holding UTC wall-clock values via server.NowUtc()). Postgres resolves that comparison by casting through the session's TimeZone setting, so on a non-UTC session the effective window silently shifts by the zone offset. Found while adding an hourly bulk-delete quota with the same pattern: on this environment's session timezone (BST, UTC+1), a 1-hour window was fully masked -- every check saw 0 recent attempts regardless of what was actually recorded, because the ~1 hour skew was as large as the window itself. This account-action limiter's 24h windows are wide enough that the same skew goes unnoticed in practice, but the underlying comparison was still wrong. Fix: compute the cutoff in Go (server.NowUtc().Add(-window)) and bind it as a plain parameter, removing the now()/naive-timestamp cast entirely. Same pattern already used by RemoveExpired* cleanup functions in this file. Note: the identical now()-vs-naive-timestamp pattern also appears in model/network_create_rate_limit.go, wallet_auth_challenge_attempt.go, auth_model.go, and auth_model_attempt.go (all pre-existing, not part of this session's work) -- worth a follow-up look, not fixed here.
Both independent Opus reviews of the abuse-limit commit caught the same real bug: RemoveNetworkClients charged the global hourly quota before checking the concurrency cap and attempting the run_once schedule, so a request rejected by TooManyConcurrentRuns or AlreadyInProgress still burned shared budget for zero removals. Since AlreadyInProgress explicitly tells the caller to retry, a single network's ordinary retries of one large request could exhaust the whole deployment's hourly ceiling for everyone else within a couple of attempts -- the opposite of what an abuse limit is for. Fix: charge quota only once a request is truly admitted. The sync path already always runs once reached, so charging immediately before it is unchanged. The async path now checks the concurrency cap and attempts the schedule first; only after ScheduleTaskIfAbsent actually inserts a task row does it charge quota, and if the quota check then rejects, it cancels the just-scheduled task via task.RemovePendingTask so the run_once key frees up immediately rather than staying held by a request that was ultimately rejected. Added tests covering both fronts: the existing AlreadyInProgress and TooManyConcurrentRuns tests now assert the ledger wasn't charged, and a new test drives the quota-rejects-after-schedule path directly, confirming the task is cancelled and the run_once key is free afterward.
…g them Per product decision: a request that can't fit the current hour's deployment-wide budget should wait for a later hour rather than fail outright, with a hard daily cap (24 x the hourly limit = 24,000,000) as the only case that's actually shown to the caller as an error. Replaces the rolling trailing-hour window with fixed, non-overlapping hourly buckets (UTC) -- a rolling window has no discrete "next hour" to wait for, so fixed buckets are what make queueing well-defined. ReserveBulkClientRemovalSlot walks forward from the current hour, placing a request in the earliest bucket with room; if none of the next 24 hours has room, that's the daily cap, and the request is rejected. A single request can never itself exceed a bucket's ceiling (both are capped at 1,000,000), so daily-cap rejection only happens under genuine contention from other reservations. RemoveNetworkClients now determines *when* a request will run (now vs. a future bucket) before deciding *how* (synchronous vs. background task): only a request whose reserved bucket is the current hour AND is small enough still runs synchronously, unchanged from before. Any other admitted request -- large, or deferred to a future hour -- goes through RemoveNetworkClientsTask with RunAt set to its bucket's start, so even a handful of client ids can end up "scheduled for 3pm" if the current hour is full. The response now carries scheduled_for so a caller (or an operator) can tell "running now" from "queued". Per product decision, a network's single-flight lock (AlreadyInProgress) is held for the whole queued+running duration, same as today -- one logical bulk-delete per network at a time, whether it's waiting its turn or actually executing. Since the bucket has to be known before scheduling (it becomes the task's RunAt), reservation now happens before the run_once admission check, the reverse of the previous ordering; if that check then fails (AlreadyInProgress), the reservation is released via CancelBulkClientRemovalReservation rather than left charged against a request that never ran. Also fixes a concurrency-cap self-deadlock this design would otherwise hit: the cap counted every pending bulk-delete task regardless of run_at, so a backlog of merely-queued (not yet running) requests would fill the cap and block admission of brand new ones. Renamed to CountAvailableByFunctionName and scoped to run_at <= now, so only actually-running/runnable work counts against the concurrency limit. Tests cover: filling a bucket then rolling into the next hour, exhausting the full daily lookahead to trigger real rejection, releasing a cancelled reservation, expiring old buckets without touching ones still in the lookahead window, a small request queueing when the current hour is full, and the reservation being released when AlreadyInProgress fires after it was made.
…ency cap to async work only Two independent Opus reviews of the queueing redesign converged on one substantive finding: CountAvailableByFunctionName (which excludes future-run_at tasks so a queue backlog doesn't self-deadlock admission) also means the concurrency cap can't see how large that backlog is. Every request deferred to the same future hour got RunAt set to exactly that hour's boundary, so a sustained period of heavy bulk-delete traffic could queue many networks' tasks against the same hour and have them all become eligible at the identical instant -- defeating the concurrency cap's purpose the moment that hour arrives. Fix: a deferred request's actual run time is now jittered randomly across its target hour (RunAt = bucketStart + rand[0, bucket duration)), spreading a backlog's execution instead of bursting it. A request landing in the current hour is unaffected -- it still runs essentially immediately. Also addresses a related but separate finding: the concurrency cap was being checked before the sync/async branch point, so a tiny request that ends up running synchronously (current hour has room, size fits) could be rejected by an unrelated backlog of large background runs, even though it was never going to touch the task system. The cap now only applies once a request is determined to need scheduling (large, or deferred to a future hour), restoring the sync path's original independence from it. New tests: sync-path immunity to the concurrency cap even when it's fully occupied, ScheduledFor is correctly set on the immediate (not just deferred) async path, and the sharpest coverage gap both reviewers flagged -- a network's first request genuinely deferred to a future hour (not a directly-injected fake), then a second request for the same network before that hour arrives, confirming the run_once lock is held for the whole queued+running duration even though the deferred task doesn't count against the concurrency cap while waiting.
The Opus reviews of the thundering-herd fix both noted the existing tests would pass even if the jitter line were reverted (they only check ScheduledFor falls in a range or differs from the current bucket, both true with or without jitter). Verified this empirically by disabling the jitter and confirming the new test fails without it, then restoring it and confirming the full suite passes. Drives 5 independently deferred requests (distinct networks, same target hour) and asserts their ScheduledFor values aren't all identical -- a real signal that jitter is spreading execution across the hour, not just landing everything on the boundary.
Ryanmello07
marked this pull request as ready for review
July 22, 2026 23:13
…stream # Conflicts: # db_migrations.go
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.
Summary
Two independent features, cherry-picked cleanly from a beta fork branch onto current
main:1. Seedphrase account system (replaces guest mode)
POST /auth/network-createwith no auth fieldsPOST /auth/login(seedphrasefield)POST /auth/generate-seedphrase,POST /auth/regenerate-seedphrase)POST /auth/add-auth,POST /auth/remove-auth) — cannot remove your last remaining methodPOST /account/change-name(24h cooldown on the old name) andPOST /account/claim-name(no cooldown, for first-time custom names) — both require a verified email or an SSO login bound to the account (wallet-only auth does not qualify, since it can't be used to recover the account)network_create_attempttable), independent of the existing email/password auth-attempt rate limiterguest_modeon/auth/network-create,/auth/upgrade-guest,/auth/upgrade-guest-existing) is removed. Existing guest rows/JWTs are left untouched for backward compatibility (GuestModestays onjwt.ByJwt, alwaysfalsefor new tokens;Network.GuestUpgradeNetworkIdcolumn/field kept).2. Bulk delete for offline network clients
RemoveNetworkClientscalls: small requests (≤10k ids) apply synchronously; larger requests are handed off to a background task (RemoveNetworkClientsTask) that processes up to 1M ids in bounded batches, so a single call can clear a network with hundreds of thousands of offline clients without a long-running request-path transactionrun_oncekey), with reschedule/continuation support for partial batchesnetwork_client_network_id_client_id) to support the batched deactivation query at scaleNew tables
network_user_auth_seedphrase— SHA-256 lookup + Argon2id hash for seedphrase auth (never stores the raw mnemonic)network_name_reclaim— 24h cooldown pool for changed network namesnetwork_create_attempt— 5-per-IP-per-day seedphrase signup rate limitNew endpoints
/auth/network-create/auth/loginseedphrasefield added alongside existing methods)/auth/add-auth/auth/remove-auth/auth/generate-seedphrase/auth/regenerate-seedphrase/account/change-name/account/claim-nameRemoved endpoints
POST /auth/upgrade-guestPOST /auth/upgrade-guest-existingNotes for reviewers
api/spec_conformance_test.gois updated to match the new/removed routes. The OpenAPI spec in the separateconnectrepo (connect/api/bringyour.yml) still lists the removed/auth/upgrade-guest*routes and doesn't yet list the new seedphrase/account routes — that update is out of scope here and should be coordinated in a follow-up PR againstconnect..superpowers/planning docs, BETA.md/beta-setup scripts, and any CI/workflow changes — those are fork/beta-specific and not relevant upstream.Verification
go build ./...passesgo vet ./api(spec conformance package) passes clean