docs: Add architecture docs for verify role reconciliation - #106
Conversation
Replaces the synchronous in-request Discord role fan-out (which times out behind API Gateway on large guilds) with an SQS self-requeue reconciler: desired state persisted + 202, per-guild job row as lease/cursor/progress, generation-based supersede for concurrent config changes, and user_links normalized out of the guild JSON blob. Designed for 50k linked members at effectively zero idle cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDUUtVMvN2EJzPDHX1Qst8
Companion to the high-level architecture doc: schemas and migrations, common-crate module layout (ReconScope merge algebra, VerifyJob lease/supersede/checkpoint SQL, keyset pagination), rewritten API handlers, the consumer worker control flow with tuning constants, UI polling, Terraform diffs, a failure matrix, and staged rollout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDUUtVMvN2EJzPDHX1Qst8
…tion Move the verify reconciliation docs into docs/high-level-architecture/ and docs/low-level-architecture/, both named verify-role-reconciliation.md (same feature name at each level, level conveyed by directory). Add docs/CLAUDE.md documenting the convention so future features follow the same structure and naming, and cross-link the two docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDUUtVMvN2EJzPDHX1Qst8
📝 WalkthroughWalkthroughImplements asynchronous verify-role reconciliation with SQS jobs, persistent leases and checkpoints, normalized user-link storage, updated API contracts, worker processing, dashboard polling, infrastructure tuning, and integration/performance validation. ChangesVerify Role Reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant VerifyAPI
participant SQS
participant Consumer
participant Database
participant Discord
Dashboard->>VerifyAPI: submit verify-role change
VerifyAPI->>Database: save desired state and job generation
VerifyAPI->>SQS: enqueue reconciliation token
SQS->>Consumer: deliver token
Consumer->>Database: lease job and scan link batch
Consumer->>Discord: reconcile roles
Consumer->>Database: checkpoint or complete job
Dashboard->>VerifyAPI: poll job status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
docs/low-level-architecture/verify-role-reconciliation.md (1)
3-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPut Status on its own one-line header.
The documented convention requires a one-line
Status; separate it from the companion-link text.Proposed fix
-Status: **Proposed** · Companion to +Status: **Proposed** + +Companion to [`high-level-architecture/verify-role-reconciliation.md`](../high-level-architecture/verify-role-reconciliation.md)As per coding guidelines, every architecture document starts with a one-line Status.
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 3 - 6, Move “Status: **Proposed**” onto its own one-line header at the start of the document, leaving the companion-link description on the following line.Source: Coding guidelines
🤖 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 `@docs/CLAUDE.md`:
- Around line 20-26: Update the fenced directory-tree block in the documentation
to specify the text language identifier, changing the opening fence to use text
while preserving the tree contents.
In `@docs/high-level-architecture/verify-role-reconciliation.md`:
- Around line 143-170: Specify a language on the pseudocode fence surrounding
verify_recon, using text or pseudocode, so the documentation passes markdownlint
MD040 without changing the pseudocode content.
In `@docs/low-level-architecture/verify-role-reconciliation.md`:
- Around line 166-177: The VerifyJob response and OpenAPI schema use different
fields and names. Align the `VerifyJob` serialization contract with the schema
by either defining an explicit response DTO for `get_job` with the documented
`updatedAt` field and intended fields, or updating the schema to exactly match
the serialized `VerifyJob`; ensure generated clients and the wire response
agree.
- Around line 425-427: Update the reconciliation flow described in the “Ordering
is deliberate” section so desired-state persistence and job-row creation occur
atomically, using a database transaction or an equivalent durable
outbox/recovery sweep. Ensure a crash after guild.save() cannot leave the new
desired state without a corresponding job, and update the duplicated guidance at
the referenced later section consistently.
- Around line 555-580: Update the reconciliation loop around reconcile_user to
track the last fully completed user separately from processed, and checkpoint
only through that user; when processed is zero, leave the cursor unchanged
rather than advancing to batch[0]. Stage each user’s count changes until
reconcile_user completes successfully, then commit them, so rate-limited retries
neither skip users nor persist duplicate counts.
- Around line 180-224: Update VerifyJob::supersede to make scope merging atomic
under concurrent calls: perform the existing-row read and merged-scope upsert
within a transaction using row locking, or retry optimistic reads and writes
against the latest generation. Ensure concurrent pending/running scopes are both
preserved while retaining terminal-job behavior, and commit before returning the
generation.
- Around line 395-413: The Guild read-modify-write flow around role updates,
per-user link handlers, and job completion must not save a stale whole-Guild
blob. Replace these full-blob saves with optimistic versioned/transactional
updates, or move mutable role-member counts to separate storage so each path
updates only its own fields; preserve concurrent role configuration and
link/count changes across the affected flows.
- Around line 207-220: Update the verify_jobs upsert in the generation-bump
conflict handler to set lease_until = NULL alongside the existing status and
cursor resets, while preserving generation-guarded lease release behavior
elsewhere.
---
Nitpick comments:
In `@docs/low-level-architecture/verify-role-reconciliation.md`:
- Around line 3-6: Move “Status: **Proposed**” onto its own one-line header at
the start of the document, leaving the companion-link description on the
following line.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 476640d2-7625-4e76-a251-38a0b4bb43ec
📒 Files selected for processing (3)
docs/CLAUDE.mddocs/high-level-architecture/verify-role-reconciliation.mddocs/low-level-architecture/verify-role-reconciliation.md
| ``` | ||
| docs/ | ||
| high-level-architecture/ | ||
| verify-role-reconciliation.md | ||
| low-level-architecture/ | ||
| verify-role-reconciliation.md ← same name, deeper detail | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the fenced tree block.
markdownlint reports MD040 here; use text (or another appropriate identifier) for the directory tree.
Proposed fix
-```
+```text📝 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.
| ``` | |
| docs/ | |
| high-level-architecture/ | |
| verify-role-reconciliation.md | |
| low-level-architecture/ | |
| verify-role-reconciliation.md ← same name, deeper detail | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/CLAUDE.md` around lines 20 - 26, Update the fenced directory-tree block
in the documentation to specify the text language identifier, changing the
opening fence to use text while preserving the tree contents.
Source: Linters/SAST tools
| ``` | ||
| on verify_recon(guild_id, generation): | ||
| job ← SELECT * FROM verify_jobs WHERE guild_id = $1 | ||
| if job is missing, or generation < job.generation, or job.status is terminal: | ||
| return Ok // stale message; the newer chain owns the work | ||
| acquire lease: | ||
| UPDATE verify_jobs SET lease_until = now() + LEASE, status = 'running' | ||
| WHERE guild_id = $1 AND generation = $2 | ||
| AND (lease_until IS NULL OR lease_until < now()) | ||
| → 0 rows updated ⇒ another invocation holds the lease ⇒ return Ok | ||
|
|
||
| roles ← guild config (desired state) | ||
| deadline ← now() + WORK_BUDGET // e.g. 60 s, well under Lambda timeout | ||
| loop while now() < deadline: | ||
| batch ← next LIMIT-N rows of guild_user_links after job.cursor | ||
| if batch is empty: mark job succeeded; return Ok | ||
| for each (user_id, links) in batch: | ||
| desired ← roles matched by link_arr_match(links, role.pattern) | ||
| issue idempotent PUT/DELETE per role in scope // paced ≤ RATE req/s | ||
| on 429 with retry_after ≤ SHORT_WAIT: sleep briefly (bounded budget) | ||
| on 429 with retry_after > SHORT_WAIT: backoff ← retry_after; break | ||
| on 403/404: count as error/skip; never fail the job | ||
| checkpoint: | ||
| UPDATE verify_jobs SET cursor=$c, processed=+n, errors=+e | ||
| WHERE guild_id=$1 AND generation=$2 | ||
| → 0 rows ⇒ superseded mid-flight ⇒ return Ok (new chain restarts) | ||
| send continuation message (DelaySeconds = backoff, else 0); release lease; return Ok | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the pseudocode fence.
markdownlint reports MD040 here; use text or pseudocode so the block is lint-compliant.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 143-143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/high-level-architecture/verify-role-reconciliation.md` around lines 143
- 170, Specify a language on the pseudocode fence surrounding verify_recon,
using text or pseudocode, so the documentation passes markdownlint MD040 without
changing the pseudocode content.
Source: Linters/SAST tools
| #[derive(Clone, Serialize, Deserialize)] | ||
| pub struct VerifyJob { | ||
| pub guild_id: Id<GuildMarker>, | ||
| pub generation: i64, | ||
| pub status: String, | ||
| pub scope: ReconScope, | ||
| pub cursor: Option<Id<UserMarker>>, | ||
| pub total: i32, | ||
| pub processed: i32, | ||
| pub errors: i32, | ||
| pub counts: HashMap<Id<RoleMarker>, u32>, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the VerifyJob response with OpenAPI.
get_job serializes the Rust VerifyJob, which includes guild_id, generation, scope, cursor, and counts, while the schema omits them and declares updatedAt, which is not present on the struct. Generated clients will not match the documented wire contract.
Define an explicit response DTO with the intended field names, or update the schema to match the actual serialized response.
Also applies to: 448-459
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 166 -
177, The VerifyJob response and OpenAPI schema use different fields and names.
Align the `VerifyJob` serialization contract with the schema by either defining
an explicit response DTO for `get_job` with the documented `updatedAt` field and
intended fields, or updating the schema to exactly match the serialized
`VerifyJob`; ensure generated clients and the wire response agree.
| **Supersede (API side).** Called by every handler that changes verify desired state. | ||
| Read-merge-upsert; the `ON CONFLICT` increment is atomic, so concurrent bumps both land — | ||
| the message chain that observes the *highest* generation wins, and both enqueued tokens | ||
| point at a row whose scope contains both changes (a lost scope-merge race between two | ||
| simultaneous admin edits degrades to `all: true`, never to a dropped operation, because | ||
| the loser's handler re-reads and re-merges before its upsert): | ||
|
|
||
| ```rust | ||
| impl VerifyJob { | ||
| /// Bump the guild's job to a new generation with `scope` folded in. | ||
| /// Returns the new generation to embed in the SQS message. | ||
| pub async fn supersede( | ||
| guild_id: Id<GuildMarker>, | ||
| scope: ReconScope, | ||
| pg_pool: &Pool<Postgres>, | ||
| ) -> Result<i64, StatusCode> { | ||
| // Merge with the existing scope only if the job hasn't finished — | ||
| // a terminal job's scope is history, not pending work. | ||
| let mut merged = scope; | ||
| if let Some(existing) = Self::from_db(guild_id, pg_pool).await? { | ||
| if existing.status == "pending" || existing.status == "running" { | ||
| let mut base = existing.scope; | ||
| base.merge(merged); | ||
| merged = base; | ||
| } | ||
| } | ||
|
|
||
| let row = sqlx::query( | ||
| "INSERT INTO verify_jobs (guild_id, generation, status, scope, total) | ||
| VALUES ($1, 1, 'pending', $2, | ||
| (SELECT count(*) FROM guild_user_links WHERE guild_id = $1)) | ||
| ON CONFLICT (guild_id) DO UPDATE SET | ||
| generation = verify_jobs.generation + 1, | ||
| status = 'pending', | ||
| scope = $2, | ||
| cursor = NULL, | ||
| processed = 0, | ||
| errors = 0, | ||
| counts = '{}', | ||
| total = EXCLUDED.total, | ||
| updated_at = CURRENT_TIMESTAMP | ||
| RETURNING generation", | ||
| ) | ||
| .bind(BigDecimal::from(guild_id.into_nonzero().get())) | ||
| .bind(serde_json::to_string(&merged).map_err(ise)?) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make scope merging atomic.
from_db + in-memory merge + ON CONFLICT only makes the generation increment atomic; concurrent handlers can still overwrite each other’s merged JSON scope. One operation can therefore be lost while both messages reference the latest generation.
Use a transaction with row locking, or optimistic read/merge/retry against the latest row.
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 180 -
224, Update VerifyJob::supersede to make scope merging atomic under concurrent
calls: perform the existing-row read and merged-scope upsert within a
transaction using row locking, or retry optimistic reads and writes against the
latest generation. Ensure concurrent pending/running scopes are both preserved
while retaining terminal-job behavior, and commit before returning the
generation.
| let row = sqlx::query( | ||
| "INSERT INTO verify_jobs (guild_id, generation, status, scope, total) | ||
| VALUES ($1, 1, 'pending', $2, | ||
| (SELECT count(*) FROM guild_user_links WHERE guild_id = $1)) | ||
| ON CONFLICT (guild_id) DO UPDATE SET | ||
| generation = verify_jobs.generation + 1, | ||
| status = 'pending', | ||
| scope = $2, | ||
| cursor = NULL, | ||
| processed = 0, | ||
| errors = 0, | ||
| counts = '{}', | ||
| total = EXCLUDED.total, | ||
| updated_at = CURRENT_TIMESTAMP |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Clear the previous lease when superseding a generation.
The upsert resets status and cursor but leaves lease_until untouched. A new-generation token can be dropped while the old worker still holds the lease; that worker then exits when its generation-guarded checkpoint fails, leaving no usable token after the lease expires.
Set lease_until = NULL on generation bumps and keep lease release generation-guarded.
Proposed fix
status = 'pending',
+ lease_until = NULL,
scope = $2,📝 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.
| let row = sqlx::query( | |
| "INSERT INTO verify_jobs (guild_id, generation, status, scope, total) | |
| VALUES ($1, 1, 'pending', $2, | |
| (SELECT count(*) FROM guild_user_links WHERE guild_id = $1)) | |
| ON CONFLICT (guild_id) DO UPDATE SET | |
| generation = verify_jobs.generation + 1, | |
| status = 'pending', | |
| scope = $2, | |
| cursor = NULL, | |
| processed = 0, | |
| errors = 0, | |
| counts = '{}', | |
| total = EXCLUDED.total, | |
| updated_at = CURRENT_TIMESTAMP | |
| let row = sqlx::query( | |
| "INSERT INTO verify_jobs (guild_id, generation, status, scope, total) | |
| VALUES ($1, 1, 'pending', $2, | |
| (SELECT count(*) FROM guild_user_links WHERE guild_id = $1)) | |
| ON CONFLICT (guild_id) DO UPDATE SET | |
| generation = verify_jobs.generation + 1, | |
| status = 'pending', | |
| lease_until = NULL, | |
| scope = $2, | |
| cursor = NULL, | |
| processed = 0, | |
| errors = 0, | |
| counts = '{}', | |
| total = EXCLUDED.total, | |
| updated_at = CURRENT_TIMESTAMP |
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 207 -
220, Update the verify_jobs upsert in the generation-bump conflict handler to
set lease_until = NULL alongside the existing status and cursor resets, while
preserving generation-guarded lease release behavior elsewhere.
| let mut guild = Guild::from_db(guild_id, &app_state.pg_pool) | ||
| .await? | ||
| .unwrap_or_else(|| Guild { guild_id, ..Default::default() }); | ||
|
|
||
| // Replacing an existing role's pattern must strip users who matched the | ||
| // OLD pattern but not the new one ⇒ a removal op for the old pattern is | ||
| // folded into the same job as the add. | ||
| let mut scope = ReconScope::role_add(role_id); | ||
| if let Some(old) = guild.verify.roles.iter().find(|r| r.role_id == role_id) { | ||
| scope.merge(ReconScope::role_remove(role_id, old.pattern.clone())); | ||
| } | ||
|
|
||
| guild.verify.roles.retain(|r| r.role_id != role_id); | ||
| let new_role = VerifyRole { role_id, pattern: put_role_request.pattern, members: 0 }; | ||
| guild.verify.roles.push(new_role.clone()); | ||
| guild.save(&app_state.pg_pool).await?; // 1. desired state | ||
|
|
||
| let generation = VerifyJob::supersede(guild_id, scope, &app_state.pg_pool).await?; // 2. job | ||
| enqueue_recon(guild_id, generation, 0, &app_state.sqs).await?; // 3. wake worker |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not save the whole Guild blob from concurrent paths.
Role updates, per-user link handlers, and job completion all perform read-modify-write saves of the Guild blob. Normalizing user_links does not prevent a stale link/count update from overwriting a concurrent role configuration change.
Use optimistic versioning/transactions, or store mutable role-member counts separately and avoid rewriting unrelated Guild fields.
Also applies to: 461-482, 634-642
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 395 -
413, The Guild read-modify-write flow around role updates, per-user link
handlers, and job completion must not save a stale whole-Guild blob. Replace
these full-blob saves with optimistic versioned/transactional updates, or move
mutable role-member counts to separate storage so each path updates only its own
fields; preserve concurrent role configuration and link/count changes across the
affected flows.
| Ordering is deliberate: desired state commits **before** the job bump, and the job bump | ||
| **before** the message, so a crash between any two steps leaves a state that the next | ||
| retry or the next admin action repairs (see §8). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make desired-state persistence and job creation crash-safe.
These are separate writes. A crash after guild.save() but before supersede() leaves the new desired state permanently without a job unless an administrator happens to retry or run recon.
Use a database transaction for desired state plus job-row creation, or add a durable outbox/recovery sweep.
Also applies to: 760-761
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 425 -
427, Update the reconciliation flow described in the “Ordering is deliberate”
section so desired-state persistence and job-row creation occur atomically,
using a database transaction or an equivalent durable outbox/recovery sweep.
Ensure a crash after guild.save() cannot leave the new desired state without a
corresponding job, and update the duplicated guidance at the referenced later
section consistently.
| let (mut processed, mut errors) = (0, 0); | ||
| for row in &batch { | ||
| match reconcile_user(&guild, &job.scope, row, state, &mut job.counts).await { | ||
| Ok(()) => processed += 1, | ||
| Err(UserError::RateLimited { retry_after }) if retry_after <= SHORT_WAIT | ||
| && inline_wait_spent + retry_after <= MAX_INLINE_WAIT => | ||
| { | ||
| inline_wait_spent += retry_after; | ||
| sleep(retry_after).await; // cheap sub-second absorb | ||
| // re-run this user next invocation: don't advance past them | ||
| break; | ||
| } | ||
| Err(UserError::RateLimited { retry_after }) => { | ||
| backoff_secs = retry_after.as_secs() as i32; // long 429 ⇒ free wait via SQS | ||
| break; | ||
| } | ||
| Err(UserError::Skip) => { errors += 1; processed += 1; } // 403/404: count & move on | ||
| Err(UserError::Fatal(e)) => return Err(e), // DB/5xx: redeliver message | ||
| } | ||
| } | ||
|
|
||
| // 4. Checkpoint up to the last fully-processed user. | ||
| let cursor = batch[processed.saturating_sub(1).min(batch.len() - 1)].user_id; | ||
| let releasing = backoff_secs > 0; // ending the invocation early | ||
| if !job.checkpoint(cursor, processed as i32, errors as i32, releasing, &state.pg_pool).await? { | ||
| return Ok(()); // generation bumped mid-flight: the new chain owns the work |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Checkpoint only fully processed users and committed counts.
When the first user is rate-limited, processed == 0 but line 577 still advances the cursor to batch[0], permanently skipping that user. Additionally, reconcile_user mutates job.counts before completion; a partial rate-limit retry can persist those counts and increment them again.
Track the last fully completed user separately, and stage per-user counts until reconcile_user succeeds.
🤖 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 `@docs/low-level-architecture/verify-role-reconciliation.md` around lines 555 -
580, Update the reconciliation loop around reconcile_user to track the last
fully completed user separately from processed, and checkpoint only through that
user; when processed is zero, leave the cursor unchanged rather than advancing
to batch[0]. Stage each user’s count changes until reconcile_user completes
successfully, then commit them, so rate-limited retries neither skip users nor
persist duplicate counts.
… job progress)
Implements docs/low-level-architecture/verify-role-reconciliation.md:
- migrations: guild_user_links (keyset-paginated per-guild link rows) and
verify_jobs (per-guild lease/cursor/progress row)
- common::verify: shared Link/Verify types, ReconScope merge algebra
(add/sync/removal op kinds; removal survives merge with all; add after
removal escalates to sync), VerifyJob supersede/acquire_lease/checkpoint/
complete SQL, GuildUserLink SQL, enqueue_recon publisher
- api: put/delete verify role and recon persist desired state, bump the job
generation and enqueue a wake-up token, returning 202 + job snapshot; new
GET /guilds/{id}/verify/job; per-user link handlers write single rows and
maintain member counts by saturating deltas; Guild::save seeds legacy blob
user_links into the table before the blob drops them
- consumer: verify_recon worker — bounded slices behind a conditional-UPDATE
lease, generation-guarded checkpoints, 429s classified (short waits
absorbed inline under budget, long waits become SQS DelaySeconds), 403/404
skipped and counted, one reconcile_user path replacing the three legacy
synchronous fan-out loops, DiscordRoles trait for testability
- infra: consumer gains sqs:SendMessage, SQS_URL, DISCORD_BOT_TOKEN,
timeout 120s; queue visibility timeout 720s
- ui: 202 handling, VerifyJob store model, job polling with backed-off
cadence and a Syncing roles progress bar
- openapi: verify role endpoints, job endpoint, VerifyJob schema
Unit tests: scope algebra, job status, blob shape, count deltas (common);
scope computation + pattern validation (api); reconcile_user scope x match
matrix, failure classification (consumer); job progress polling (ui).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDUUtVMvN2EJzPDHX1Qst8
- consumer becomes lib+bin so tests/ can drive the worker directly - consumer/tests/harness: disposable-Postgres pool (production migrations, advisory-locked DDL), guild/link seeding, per-test guild reset for re-runnability, and a scripted StubGateway (latency + one-shot 429) - consumer/tests/integration.rs (9 tests, real Postgres): full chain completes and folds member counts; scoped add is O(matchers); stale tokens/live leases rejected; supersede mid-chain restarts the cursor with the merged scope; crashed invocation recovers after lease expiry; long 429 defers with its retry_after and the limited user retries exactly once; short 429 absorbed inline; legacy blob seeded on first job; empty guild completes in one slice - consumer/tests/perf.rs (50k members, release): scan floor ~9.5-9.8k members/s; full sync through the real TwilightRoles client against an in-process axum stub ~3.4k role calls/s end-to-end (~450x Discord's per-guild bucket, worker overhead ~0.2% of the rate-limit floor), with a >=10x-headroom regression guard; 10% partial-match counts exact - scripts/verify-recon-tests.sh provisions local Postgres 16 (initdb or Docker fallback) and runs either suite - docs: low-level testing section updated with measured results; both architecture docs flipped to Implemented Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDUUtVMvN2EJzPDHX1Qst8
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
common/src/verify.rs (1)
34-54: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPrecompile verify regexes before the hot loop
link_arr_matchandlink_matchcompile the pattern on every call.consumer/src/verify/consumer.rscallslink_arr_matchfor every user against each role/removal pattern, so this repeats the same regex compilation many times on large guilds. Compile each pattern once per reconciliation run and pass the compiledRegexthrough the inner loop.🤖 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 `@common/src/verify.rs` around lines 34 - 54, Update link_arr_match and link_match to accept a precompiled regex::Regex instead of compiling the pattern internally, while preserving active-link matching behavior. In consumer/src/verify/consumer.rs, compile each role/removal pattern once per reconciliation run, handle invalid patterns at that boundary with the existing error behavior, and pass the compiled regex through the per-user inner loop.
🤖 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 `@api/openapi/openapi.yaml`:
- Around line 332-340: Update the requestBody schema in the OpenAPI operation to
mark the body as required and add pattern to the schema’s required list.
Preserve the existing object shape and pattern property definition while
ensuring generated clients must provide a body containing pattern.
In `@api/src/guilds/models.rs`:
- Around line 109-117: The save path’s unconditional
GuildUserLink::seed_from_blob can resurrect links deleted by
delete_link_guilds_id when verify.user_links still contains the stale entry.
Update the legacy unlink flow to remove the user from guild.verify.user_links
before Guild::save, or otherwise ensure blob backfill runs only once; add a
regression test for unlinking a blob-only guild and verify the link remains
deleted.
In `@api/src/guilds/verify/controllers.rs`:
- Around line 57-73: Update start_recon_job’s SQS_URL retrieval to handle a
missing environment variable as a normal internal-server error instead of
panicking via expect. Convert the std::env::var failure into the existing
StatusCode/ise error flow, while preserving the current enqueue_recon retry and
error handling behavior.
In `@api/src/users/link_guilds.rs`:
- Around line 63-88: The link snapshot is persisted before Discord role
assignment, preventing retries after a failed add and leaving counter updates
unpersisted. Update the flow around effective_links, add_guild_member_role, and
GuildUserLink::upsert so the new links are stored only after every newly
qualifying role assignment and corresponding bump_members update succeeds, while
preserving the existing retry behavior on failure.
- Around line 72-85: Update the link-driven mutation flow around role additions
and removals, including the corresponding logic near guild unlink handling, so
Discord changes participate in the guild reconciliation job’s ordering and
lease/version validation. Ensure stale worker batches cannot re-add roles after
DELETE or undo PUT changes; use the existing reconciliation mechanism or
validate the user/link version immediately before each worker role mutation.
In `@consumer/src/main.rs`:
- Around line 32-36: Update the twilight_http client construction in the
discord_bot initialization to disable Twilight’s built-in ratelimiter by
configuring the builder with ratelimiter(None). Preserve the existing token
loading and client build flow so rate-limit responses propagate for SQS
handling.
In `@scripts/verify-recon-tests.sh`:
- Line 70: Remove the database URL output from the verification script by
deleting the echo statement that references KB2_TEST_DATABASE_URL. Continue
using the variable for test configuration without printing its value.
In `@ui/src/components/dashboard/VerifyComponent.vue`:
- Around line 47-71: Update trackJob to invalidate stale polling callbacks and
in-flight getVerifyJob responses whenever a newer job is tracked or the
component unmounts. Use a generation/token guard captured by each scheduled
callback and verify it before applying success or failure results, while
preserving polling for the current job. Add a regression test using a deferred
getVerifyJob promise that resolves after unmount and after tracking a newer job,
confirming the stale response is ignored.
---
Nitpick comments:
In `@common/src/verify.rs`:
- Around line 34-54: Update link_arr_match and link_match to accept a
precompiled regex::Regex instead of compiling the pattern internally, while
preserving active-link matching behavior. In consumer/src/verify/consumer.rs,
compile each role/removal pattern once per reconciliation run, handle invalid
patterns at that boundary with the existing error behavior, and pass the
compiled regex through the per-user inner loop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cd058c3-2ee9-460f-b9e9-0fee98717e1c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
api/migrations/20260712120000_guild_user_links.down.sqlapi/migrations/20260712120000_guild_user_links.up.sqlapi/migrations/20260712120001_verify_jobs.down.sqlapi/migrations/20260712120001_verify_jobs.up.sqlapi/openapi/openapi.yamlapi/src/guilds/models.rsapi/src/guilds/verify/controllers.rsapi/src/guilds/verify/models.rsapi/src/users/link_guilds.rsapi/src/users/links.rsapi/src/users/models.rsapi/src/users/utils.rscommon/Cargo.tomlcommon/src/lib.rscommon/src/verify.rsconsumer/Cargo.tomlconsumer/src/lib.rsconsumer/src/main.rsconsumer/src/verify/consumer.rsconsumer/src/verify/discord.rsconsumer/src/verify/mod.rsconsumer/tests/harness/mod.rsconsumer/tests/integration.rsconsumer/tests/perf.rsdocs/high-level-architecture/verify-role-reconciliation.mddocs/low-level-architecture/verify-role-reconciliation.mdinfra/modules/compute/lambda/main.tfinfra/modules/data/sqs/main.tfscripts/verify-recon-tests.shui/src/components/dashboard/VerifyComponent.job.test.jsui/src/components/dashboard/VerifyComponent.vueui/src/helpers/verify.jsui/src/stores/guild.js
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/high-level-architecture/verify-role-reconciliation.md
- docs/low-level-architecture/verify-role-reconciliation.md
| requestBody: | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| pattern: | ||
| type: string | ||
| example: '@example.com$' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Require the request body and pattern.
The handler deserializes a mandatory String, but this contract permits an omitted body or {}. Generated clients can therefore send a spec-valid request that the API rejects.
Proposed contract fix
requestBody:
+ required: true
content:
application/json:
schema:
type: object
properties:
pattern:
type: string
example: '`@example.com`$'
+ required:
+ - pattern📝 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.
| requestBody: | |
| content: | |
| application/json: | |
| schema: | |
| type: object | |
| properties: | |
| pattern: | |
| type: string | |
| example: '@example.com$' | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| type: object | |
| properties: | |
| pattern: | |
| type: string | |
| example: '`@example.com`$' | |
| required: | |
| - pattern |
🤖 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 `@api/openapi/openapi.yaml` around lines 332 - 340, Update the requestBody
schema in the OpenAPI operation to mark the body as required and add pattern to
the schema’s required list. Preserve the existing object shape and pattern
property definition while ensuring generated clients must provide a body
containing pattern.
| if !self.verify.user_links.is_empty() { | ||
| common::verify::GuildUserLink::seed_from_blob( | ||
| self.guild_id, | ||
| &self.verify.user_links, | ||
| pg_pool, | ||
| ) | ||
| .await | ||
| .map_err(ise)?; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid resurrecting intentionally deleted legacy links.
For a legacy guild, delete_link_guilds_id removes guild_user_links(guild_id, user_id) and then calls Guild::save. This unconditional seed sees the stale blob entry and reinserts it with ON CONFLICT DO NOTHING, so the unlink returns 204 but persists the old link again.
Remove the user from guild.verify.user_links before that delete path saves the guild, or make legacy backfill explicitly one-time. Add a regression test covering unlink of a blob-only guild.
🤖 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 `@api/src/guilds/models.rs` around lines 109 - 117, The save path’s
unconditional GuildUserLink::seed_from_blob can resurrect links deleted by
delete_link_guilds_id when verify.user_links still contains the stale entry.
Update the legacy unlink flow to remove the user from guild.verify.user_links
before Guild::save, or otherwise ensure blob backfill runs only once; add a
regression test for unlinking a blob-only guild and verify the link remains
deleted.
| let generation = VerifyJob::supersede(guild_id, scope, &app_state.pg_pool) | ||
| .await | ||
| .map_err(ise)?; | ||
| let queue_url = std::env::var("SQS_URL").expect("SQS_URL must be set"); | ||
| enqueue_recon( | ||
| guild_id, | ||
| generation, | ||
| Duration::ZERO, | ||
| &app_state.sqs, | ||
| &queue_url, | ||
| ) | ||
| .await | ||
| .map_err(ise)?; | ||
| VerifyJob::from_db(guild_id, &app_state.pg_pool) | ||
| .await | ||
| .map_err(ise)? | ||
| .ok_or(StatusCode::INTERNAL_SERVER_ERROR) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the controller and related symbols/files.
printf '\n== controllers.rs outline ==\n'
ast-grep outline api/src/guilds/verify/controllers.rs --view expanded || true
printf '\n== search for enqueue_recon, VerifyJob, SQS_URL, verify_jobs ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' \
-e 'enqueue_recon' \
-e 'VerifyJob' \
-e 'SQS_URL' \
-e 'verify_jobs' \
api/src || trueRepository: KoalaBotUK/KB2
Length of output: 2052
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller file and related job implementation with line numbers.
for f in \
api/src/guilds/verify/controllers.rs \
api/src/guilds/verify/jobs.rs \
api/src/guilds/verify/mod.rs \
api/src/guilds/verify/*.rs
do
if [ -f "$f" ]; then
echo "===== $f ($(wc -l < "$f") lines) ====="
sed -n '1,220p' "$f" | cat -n
fi
doneRepository: KoalaBotUK/KB2
Length of output: 19202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files mentioning verify_jobs or sweeper-like behavior ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' \
-e 'verify_jobs' \
-e 'sweeper' \
-e 'pending.*verify' \
-e 'enqueue_recon' \
-e 'Duration::ZERO' \
api || true
printf '\n== full path list under api/src/guilds/verify ==\n'
find api/src/guilds/verify -maxdepth 2 -type f | sortRepository: KoalaBotUK/KB2
Length of output: 877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== define VerifyJob / enqueue_recon / recon worker locations ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' \
-e '^pub (struct|enum) VerifyJob\b' \
-e '^pub async fn enqueue_recon\b' \
-e 'struct VerifyJob\b' \
-e 'enqueue_recon\b' \
-e 'supersede\b' \
-e 'from_db\b' \
.
printf '\n== top-level file list around common/consumer if present ==\n'
find . -maxdepth 2 -type d | sort | sed -n '1,120p'Repository: KoalaBotUK/KB2
Length of output: 12048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the verify_jobs schema for status/timeout fields that might imply a sweeper.
for f in api/migrations/20260712120001_verify_jobs.up.sql api/migrations/20260712120001_verify_jobs.down.sql; do
if [ -f "$f" ]; then
echo "===== $f ====="
cat -n "$f"
fi
doneRepository: KoalaBotUK/KB2
Length of output: 1276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
common/src/verify.rs \
consumer/src/verify/consumer.rs \
docs/low-level-architecture/verify-role-reconciliation.md
do
if [ -f "$f" ]; then
echo "===== $f ($(wc -l < "$f") lines) ====="
sed -n '260,520p' "$f" | cat -n
echo
fi
doneRepository: KoalaBotUK/KB2
Length of output: 32570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== common/src/verify.rs: enqueue_recon / VerifyJob ====="
sed -n '285,540p' common/src/verify.rs | cat -n
echo "===== consumer/src/verify/consumer.rs: polling loop / send path ====="
sed -n '1,180p' consumer/src/verify/consumer.rs | cat -n
echo "===== docs/low-level-architecture/verify-role-reconciliation.md: crash scenarios ====="
sed -n '740,770p' docs/low-level-architecture/verify-role-reconciliation.md | cat -nRepository: KoalaBotUK/KB2
Length of output: 22844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact lines around the API comment and related documentation mentions.
echo "===== docs mentions of crash recovery / retries ====="
rg -n -C 2 --hidden --glob '!target' --glob '!node_modules' \
-e 'crash recovery' \
-e 'admin retries' \
-e 'visible gap' \
-e 'no job' \
-e 'sweeper' \
-e 'supersede.*enqueue' \
docs common consumer api | sed -n '1,220p'Repository: KoalaBotUK/KB2
Length of output: 4794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact implementation and crash-handling language.
printf '\n== common/src/verify.rs (enqueue + job methods) ==\n'
sed -n '280,540p' common/src/verify.rs | cat -n
printf '\n== consumer/src/verify/consumer.rs (receive/send/retry path) ==\n'
sed -n '1,180p' consumer/src/verify/consumer.rs | cat -n
printf '\n== docs crash / retry statements ==\n'
rg -n -C 2 --hidden --glob '!target' --glob '!node_modules' \
-e 'API crashes after supersede' \
-e 'admin retries' \
-e 'visible gap' \
-e 'no job' \
-e 'sweeper' \
docs/low-level-architecture/verify-role-reconciliation.md api/openapi/openapi.yaml consumer/tests/integration.rs common/src/verify.rs | sed -n '1,240p'Repository: KoalaBotUK/KB2
Length of output: 22929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 --hidden --glob '!target' --glob '!node_modules' \
-e 'SQS_URL' \
-e 'missing queue configuration' \
-e 'queue configuration' \
-e 'expect\("SQS_URL must be set"\)' \
api common consumer docs | sed -n '1,220p'Repository: KoalaBotUK/KB2
Length of output: 3179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== all SQS_URL references ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' 'SQS_URL' .
printf '\n== all expect("...must be set") patterns ==\n'
rg -n --hidden --glob '!target' --glob '!node_modules' 'expect\(".*must be set"\)' .Repository: KoalaBotUK/KB2
Length of output: 2953
Return a normal error for missing SQS_URL
start_recon_job already relies on retry semantics for enqueue failures, so expect("SQS_URL must be set") should not panic the handler. Surface this as a 5xx instead.
🤖 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 `@api/src/guilds/verify/controllers.rs` around lines 57 - 73, Update
start_recon_job’s SQS_URL retrieval to handle a missing environment variable as
a normal internal-server error instead of panicking via expect. Convert the
std::env::var failure into the existing StatusCode/ise error flow, while
preserving the current enqueue_recon retry and error handling behavior.
| let previous_links = | ||
| effective_links(&guild.verify, guild_id, user_id, &app_state.pg_pool) | ||
| .await | ||
| .map_err(ise)?; | ||
| // Single-row write, replacing the old whole-blob read-modify-write. | ||
| GuildUserLink::upsert(guild_id, user_id, &user.links, &app_state.pg_pool) | ||
| .await | ||
| .map_err(ise)?; | ||
|
|
||
| let roles = guild.verify.roles.clone(); | ||
| for verify_role in &roles { | ||
| if role_newly_qualifies(&user.links, previous_links.as_deref(), &verify_role.pattern) { | ||
| add_guild_member_role( | ||
| guild_id, | ||
| user_id, | ||
| verify_role.role_id, | ||
| &app_state.discord_bot, | ||
| ) | ||
| .await?; | ||
| // This path knows exactly when a user newly qualifies, so the | ||
| // count is a ±1 delta — no full recompute, no O(guild) memory. | ||
| guild.verify.bump_members(verify_role.role_id, 1); | ||
| } | ||
| } | ||
|
|
||
| guild.save(&app_state.pg_pool).await?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not mark links reconciled before Discord assignment succeeds.
GuildUserLink::upsert completes before add_guild_member_role. If an add fails, ? returns with the new row already stored; on retry, previous_links equals user.links, role_newly_qualifies is false, and the missing role is never retried. The member counter can also remain unpersisted.
Persist the new snapshot only after all required role mutations and counter updates succeed, or represent this as durable pending reconciliation work.
🤖 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 `@api/src/users/link_guilds.rs` around lines 63 - 88, The link snapshot is
persisted before Discord role assignment, preventing retries after a failed add
and leaving counter updates unpersisted. Update the flow around effective_links,
add_guild_member_role, and GuildUserLink::upsert so the new links are stored
only after every newly qualifying role assignment and corresponding bump_members
update succeeds, while preserving the existing retry behavior on failure.
| let roles = guild.verify.roles.clone(); | ||
| for verify_role in &roles { | ||
| if role_newly_qualifies(&user.links, previous_links.as_deref(), &verify_role.pattern) { | ||
| add_guild_member_role( | ||
| guild_id, | ||
| user_id, | ||
| verify_role.role_id, | ||
| &app_state.discord_bot, | ||
| ) | ||
| .await?; | ||
| // This path knows exactly when a user newly qualifies, so the | ||
| // count is a ±1 delta — no full recompute, no O(guild) memory. | ||
| guild.verify.bump_members(verify_role.role_id, 1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize link-driven Discord mutations with reconciliation jobs.
These handlers mutate Discord directly without participating in the guild job lease. A worker can read a matching row, this DELETE can remove the role and row, then the worker's stale batch can add the role back. The inverse is possible for PUT during a full-sync worker slice.
Route link changes through the same ordered reconciliation mechanism, or add per-user version/lease validation immediately before worker role calls.
Also applies to: 158-171
🤖 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 `@api/src/users/link_guilds.rs` around lines 72 - 85, Update the link-driven
mutation flow around role additions and removals, including the corresponding
logic near guild unlink handling, so Discord changes participate in the guild
reconciliation job’s ordering and lease/version validation. Ensure stale worker
batches cannot re-add roles after DELETE or undo PUT changes; use the existing
reconciliation mechanism or validate the user/link version immediately before
each worker role mutation.
| let discord_bot = Arc::new( | ||
| twilight_http::Client::builder() | ||
| .token(std::env::var("DISCORD_BOT_TOKEN").expect("DISCORD_BOT_TOKEN must be set")) | ||
| .build(), | ||
| ); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
twilight-http Client builder default ratelimiter behavior disable ratelimiter
💡 Result:
In the twilight-http crate, the ClientBuilder automatically creates and attaches a default RateLimiter when the Client is built [1][2][3]. To disable the ratelimiter, you must explicitly call the ratelimiter method on the ClientBuilder with None as the argument [1][4]. Example usage: use twilight_http::client::ClientBuilder; let client = ClientBuilder::new.token("my_token".to_string).ratelimiter(None) // Disables ratelimit handling.build; When you pass None to the ratelimiter method, the client's internal ratelimiter will be skipped entirely before making HTTP requests [1][3]. Subsequent checks on the client, such as calling the ratelimiter method on the built Client, will return None to indicate that ratelimit handling has been disabled [5][6].
Citations:
- 1: https://docs.rs/twilight-http/latest/twilight_http/client/struct.ClientBuilder.html
- 2: https://api.twilight.rs/src/twilight_http/client/builder.rs.html
- 3: https://docs.rs/crate/twilight-http/0.15.0-rc.1/source/src/client/builder.rs
- 4: https://api.twilight.rs/twilight_http/client/struct.ClientBuilder.html?search=
- 5: https://docs.rs/twilight-http/latest/twilight_http/client/struct.Client.html
- 6: https://api.twilight.rs/twilight_http/client/struct.Client.html
🏁 Script executed:
sed -n '1,220p' consumer/src/main.rsRepository: KoalaBotUK/KB2
Length of output: 1814
🏁 Script executed:
sed -n '1,220p' consumer/src/discord.rsRepository: KoalaBotUK/KB2
Length of output: 219
🏁 Script executed:
git ls-files consumer/srcRepository: KoalaBotUK/KB2
Length of output: 369
🏁 Script executed:
rg -n "ApiError::Ratelimited|ratelimit|twilight_http::Client::builder|classify|Discord" consumer/srcRepository: KoalaBotUK/KB2
Length of output: 1794
🏁 Script executed:
sed -n '1,180p' consumer/src/verify/discord.rsRepository: KoalaBotUK/KB2
Length of output: 3492
🏁 Script executed:
sed -n '1,140p' consumer/src/verify/consumer.rsRepository: KoalaBotUK/KB2
Length of output: 5790
Disable Twilight’s built-in ratelimiter here
twilight_http::Client::builder() attaches an in-process ratelimiter by default, so this worker can wait inside Lambda instead of surfacing 429s for SQS to handle. That violates the No long sleeps in compute path in consumer/src/verify/consumer.rs; if SQS should own all waiting, build the client with .ratelimiter(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 `@consumer/src/main.rs` around lines 32 - 36, Update the twilight_http client
construction in the discord_bot initialization to disable Twilight’s built-in
ratelimiter by configuring the builder with ratelimiter(None). Preserve the
existing token loading and client build flow so rate-limit responses propagate
for SQS handling.
| if [ -x "$PG_BIN/initdb" ]; then start_local; else start_docker; fi | ||
| export KB2_TEST_DATABASE_URL="postgres://$DB_USER@127.0.0.1:$PG_PORT/$DB_NAME" | ||
| fi | ||
| echo "KB2_TEST_DATABASE_URL=$KB2_TEST_DATABASE_URL" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not print the database URL.
Line 70 can leak credentials embedded in an externally supplied KB2_TEST_DATABASE_URL to CI logs or terminal captures.
Proposed fix
-echo "KB2_TEST_DATABASE_URL=$KB2_TEST_DATABASE_URL"
+echo "Using KB2_TEST_DATABASE_URL (value redacted)"📝 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.
| echo "KB2_TEST_DATABASE_URL=$KB2_TEST_DATABASE_URL" | |
| echo "Using KB2_TEST_DATABASE_URL (value redacted)" |
🤖 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 `@scripts/verify-recon-tests.sh` at line 70, Remove the database URL output
from the verification script by deleting the echo statement that references
KB2_TEST_DATABASE_URL. Continue using the variable for test configuration
without printing its value.
| // Tracks a reconciliation job: schedules the next status poll while the job | ||
| // is active, refreshes the guild (member counts) once it succeeds. Poll | ||
| // cadence backs off for big-guild jobs that run for minutes/hours. | ||
| function trackJob(job) { | ||
| jobRef.value = job; | ||
| clearTimeout(pollTimer); | ||
| if (!job || !job.isActive()) { | ||
| if (job && job.status === 'succeeded') { | ||
| emits('update'); | ||
| } | ||
| return; | ||
| } | ||
| const delay = job.total > 2000 ? 15000 : 2000; | ||
| pollTimer = setTimeout(async () => { | ||
| try { | ||
| const resp = await getVerifyJob(props.guild.guildId, userRef.value.token.accessToken); | ||
| trackJob(VerifyJob.fromJson(resp.data)); | ||
| } catch (e) { | ||
| // Transient poll failure: keep the bar, try again on the same cadence. | ||
| trackJob(jobRef.value); | ||
| } | ||
| }, delay); | ||
| } | ||
|
|
||
| onUnmounted(() => clearTimeout(pollTimer)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate stale and in-flight poll callbacks.
Line 60 applies every poll response unconditionally. A response for an older job can arrive after a newer role change starts a replacement job; if the stale job is terminal, it hides progress and stops polling while the newer job still runs. clearTimeout also cannot stop a request already in flight after unmount.
Proposed fix
let jobRef = ref(null);
let pollTimer = null;
+let pollEpoch = 0;
function trackJob(job) {
+ const epoch = ++pollEpoch;
jobRef.value = job;
clearTimeout(pollTimer);
if (!job || !job.isActive()) {
if (job && job.status === 'succeeded') {
emits('update');
@@
pollTimer = setTimeout(async () => {
try {
const resp = await getVerifyJob(props.guild.guildId, userRef.value.token.accessToken);
+ if (epoch !== pollEpoch) return;
trackJob(VerifyJob.fromJson(resp.data));
} catch (e) {
+ if (epoch !== pollEpoch) return;
// Transient poll failure: keep the bar, try again on the same cadence.
trackJob(jobRef.value);
}
}, delay);
}
-onUnmounted(() => clearTimeout(pollTimer));
+onUnmounted(() => {
+ pollEpoch += 1;
+ clearTimeout(pollTimer);
+});Add a regression test with a deferred getVerifyJob promise that resolves after unmount and after a newer job is tracked.
📝 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.
| // Tracks a reconciliation job: schedules the next status poll while the job | |
| // is active, refreshes the guild (member counts) once it succeeds. Poll | |
| // cadence backs off for big-guild jobs that run for minutes/hours. | |
| function trackJob(job) { | |
| jobRef.value = job; | |
| clearTimeout(pollTimer); | |
| if (!job || !job.isActive()) { | |
| if (job && job.status === 'succeeded') { | |
| emits('update'); | |
| } | |
| return; | |
| } | |
| const delay = job.total > 2000 ? 15000 : 2000; | |
| pollTimer = setTimeout(async () => { | |
| try { | |
| const resp = await getVerifyJob(props.guild.guildId, userRef.value.token.accessToken); | |
| trackJob(VerifyJob.fromJson(resp.data)); | |
| } catch (e) { | |
| // Transient poll failure: keep the bar, try again on the same cadence. | |
| trackJob(jobRef.value); | |
| } | |
| }, delay); | |
| } | |
| onUnmounted(() => clearTimeout(pollTimer)); | |
| let pollEpoch = 0; | |
| // Tracks a reconciliation job: schedules the next status poll while the job | |
| // is active, refreshes the guild (member counts) once it succeeds. Poll | |
| // cadence backs off for big-guild jobs that run for minutes/hours. | |
| function trackJob(job) { | |
| const epoch = ++pollEpoch; | |
| jobRef.value = job; | |
| clearTimeout(pollTimer); | |
| if (!job || !job.isActive()) { | |
| if (job && job.status === 'succeeded') { | |
| emits('update'); | |
| } | |
| return; | |
| } | |
| const delay = job.total > 2000 ? 15000 : 2000; | |
| pollTimer = setTimeout(async () => { | |
| try { | |
| const resp = await getVerifyJob(props.guild.guildId, userRef.value.token.accessToken); | |
| if (epoch !== pollEpoch) return; | |
| trackJob(VerifyJob.fromJson(resp.data)); | |
| } catch (e) { | |
| if (epoch !== pollEpoch) return; | |
| // Transient poll failure: keep the bar, try again on the same cadence. | |
| trackJob(jobRef.value); | |
| } | |
| }, delay); | |
| } | |
| onUnmounted(() => { | |
| pollEpoch += 1; | |
| clearTimeout(pollTimer); | |
| }); |
🤖 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 `@ui/src/components/dashboard/VerifyComponent.vue` around lines 47 - 71, Update
trackJob to invalidate stale polling callbacks and in-flight getVerifyJob
responses whenever a newer job is tracked or the component unmounts. Use a
generation/token guard captured by each scheduled callback and verify it before
applying success or failure results, while preserving polling for the current
job. Add a regression test using a deferred getVerifyJob promise that resolves
after unmount and after tracking a newer job, confirming the stale response is
ignored.
2 participants
Add comprehensive architecture documentation for the verify role reconciliation feature, split into high-level and low-level design documents.
Summary
This PR introduces architecture documentation for an async job-based system to handle large-scale Discord role reconciliation. The feature addresses the current limitation where role changes are processed synchronously in a single API request, which times out on guilds with more than ~250 linked members.
Changes
High-level architecture (
docs/high-level-architecture/verify-role-reconciliation.md):Low-level architecture (
docs/low-level-architecture/verify-role-reconciliation.md):api,consumer,common,infra,ui)guild_user_linksandverify_jobstables) with migration strategyReconScope,ReconMessage,VerifyJob,GuildUserLink) with SQL operationsDocumentation guide (
docs/CLAUDE.md):Implementation approach
The design uses:
DelaySecondsfor longer backoffThe documents are implementation-ready with concrete code snippets, SQL, and a detailed failure matrix demonstrating safety under all interleaving scenarios.
https://claude.ai/code/session_01PDUUtVMvN2EJzPDHX1Qst8
Summary by CodeRabbit
202 Acceptedwith job information.