feat: add bulk delete endpoint for offline network clients#2
Closed
full-bars wants to merge 4 commits into
Closed
Conversation
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
…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
There was a problem hiding this comment.
Pull request overview
Adds a scalable bulk network-client deactivation endpoint intended for very large offline-client sets, using a sync/async split plus per-network deduped task scheduling to keep request-path work bounded and avoid long transactions.
Changes:
- Added
/network/remove-clientsAPI handler and route to bulk-deactivate clients scoped by the authenticated session’snetwork_id. - Implemented bulk deactivation in
modelwith a synchronous path (≤10k IDs) and a background task path (>10k IDs) that batches work and self-reschedules. - Extended the task scheduler with an atomic “insert only if absent” helper to reject duplicate large runs per network.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| taskworker/taskworker.go | Registers the new RemoveNetworkClientsTask + post hook with the task worker. |
| task/task.go | Refactors scheduling via prepareTask and adds ScheduleTaskInTxIfAbsent / ScheduleTaskIfAbsent for atomic dedup insertion. |
| task/task_test.go | Adds coverage for the new “schedule-if-absent” semantics and arg preservation. |
| model/network_client_model.go | Implements sync/async bulk deactivation, batching, dedup keying, and task rescheduling. |
| model/network_client_model_test.go | Adds extensive tests for sync/async behavior, dedup rejection, scoping, batching, idempotency, and worker lifecycle. |
| api/handlers/network_client_handlers.go | Adds an HTTP handler for bulk client removal. |
| api/api.go | Adds the new route /network/remove-clients. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+321
to
+322
| scheduled = 0 < tag.RowsAffected() | ||
| return scheduled, p.taskId |
Owner
|
Will review and merge soon to https://github.com/Ryanmello07/server/tree/feat/seedphrase-account-system (current live testing) afterwards will move to beta then main as a PR along side the rest of the changes of the account update. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
5 tasks
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.
Mirrors urnetwork#399, redesigned for scale (target: hundreds of thousands of networks, up to ~1M offline clients per network). Network scoping (
network_id) comes from the session, never request input.Design
MaintenanceTx), self-rescheduling any remainder under the same dedup key so a crash/timeout only retries the current chunk, not the whole list.INSERT ... ON CONFLICT DO NOTHING(addedtask.ScheduleTaskInTxIfAbsent) — not silently merged, which the task system's existingRunOncewould have done (it merges timing only, not args, so a naive reuse would've dropped the second request's ids while still reporting success).What's substantially different from urnetwork#399: the original was a single unbounded transaction, no dedup protection, no scale consideration, and one weak test (just checked the call didn't error). This version adds: the sync/async split above; atomic per-network duplicate-run rejection; checkpointed self-rescheduling so a failure only costs one chunk of progress, not the whole run; a
deactivate_timefix (the original never stamped it, which the reap job needs for its 30-day grace period before hard-deleting client data — without it, an older bulk-deleted client could've been hard-deleted almost immediately instead); a nil-ByJwtguard against a corrupted/reconstructed task session; and 29 tests covering all of the above, up from 1.Reviewed: multiple independent agentic code reviews (architecture, security, code quality), all targeting the concurrency/scale claims specifically. The first pass found 7 issues (2 critical: an args-loss race in the dedup, and a permanent-stuck-state failure mode) — both fixed. Follow-up passes confirmed the fixes hold and found only minor hardening items, also applied.
Latest review round (Neko-chan, July 16): 3 subagents reviewed the PR. 4 Important + 3 Notable findings fixed in
9418e8e:RemoveNetworkClientsTaskPostScheduleTaskInTxIfAbsentis not scheduled(network_id, client_id)to support batch query at scale29 tests across
modelandtask: sync/async boundaries, duplicate rejection (atomic + per-network scoped), multi-batch task correctness, retry idempotency, nil-guard, IDOR scoping, and a full run through a realtask.TaskWorkerspanning multiple self-rescheduled invocations.