fix(agents): self-heal duplicate active handles so the new unique index cannot brick boot (#2, #10)#1841
Conversation
…annot brick boot The new ux_agent_active_handle partial unique index (handle uniqueness among active agents is a new invariant added by the consent flow) is created in _post_init, which runs in lifespan. A pre-invariant DB can legally hold two active rows with the same handle; CREATE UNIQUE INDEX then raises IntegrityError and the controller fails to boot on every restart until manual DB surgery — unacceptable for a no-terminal appliance. - Add _migration_v4_dedupe_active_handles: blank the handle on all but the oldest active row per handle (rows stay active; blanked handles are excluded from the index and can be reclaimed), logging each change. - Guard the index creation itself (catch IntegrityError, warn, continue) so an unanticipated duplicate degrades gracefully rather than bricking boot; write-time uniqueness still applies. - register/PATCH now return 409 (not an unhandled 500) when a handle is already owned by another active agent. Regression tests: a DB seeded with two active 'hermes' rows now boots, keeps the oldest, blanks the duplicate, and enforces uniqueness afterward. Audit findings #2 (boot-brick) and #10 (500 -> 409).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| role=body.role, | ||
| capabilities=body.capabilities or [], | ||
| ) | ||
| except aiosqlite.IntegrityError: |
There was a problem hiding this comment.
SUGGESTION: Broad IntegrityError catch assumes the violation is always the active-handle index.
store.register only performs an INSERT, so today the only UNIQUE constraint it can hit at this point is ux_agent_active_handle (canonical_id collisions are pre-empted by the retry loop above). The 409 + "handle is already owned by another active agent" message is therefore accurate now, but this handler will also swallow any future integrity violation (e.g., a new column with a UNIQUE constraint) and still report it as a handle conflict, which could mislead operators debugging a real schema error.
Consider checking e.args / the error string (or catching only the specific index) so the response reflects the actual constraint, or add a comment documenting that handle-collision is the sole expected cause. Not blocking.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge (non-blocking) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
The boot-brick fix (#2) is correct: the dedupe migration runs before the index creation and non-destructively blanks duplicate active handles (keeping the lowest-id owner), and the index creation is itself guarded so an unforeseen duplicate degrades to a warning instead of bricking boot. The concurrent-approve race is already handled in Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 77.2K · Output: 4.2K · Cached: 223.1K |
…MA (boot-brick) (#1852) The canvas and task stores put their element_id index in SCHEMA: CREATE INDEX ... ON project_canvas_elements(project_id, element_id) CREATE INDEX ... ON project_tasks(project_id, element_id) BaseStore runs SCHEMA (executescript) BEFORE _post_init, so on an existing pre-element_id database the index creation raised 'no such column: element_id' before the _post_init ALTER could add the column, crashing controller boot after an upgrade. Reproduced live: the Pi bricked on startup right after pulling this code (fresh installs were fine because SCHEMA creates the table WITH element_id, so CI never exercised the migration path). Move both indexes out of SCHEMA into _post_init, created after the ALTER. Same class of bug and fix as the registry active-handle index (#1841). Regression test seeds a pre-element_id DB and asserts both stores boot, migrate the column, and build the index.
…MA (boot-brick) (#1853) The canvas and task stores put their element_id index in SCHEMA: CREATE INDEX ... ON project_canvas_elements(project_id, element_id) CREATE INDEX ... ON project_tasks(project_id, element_id) BaseStore runs SCHEMA (executescript) BEFORE _post_init, so on an existing pre-element_id database the index creation raised 'no such column: element_id' before the _post_init ALTER could add the column, crashing controller boot after an upgrade. Reproduced live: the Pi bricked on startup right after pulling this code (fresh installs were fine because SCHEMA creates the table WITH element_id, so CI never exercised the migration path). Move both indexes out of SCHEMA into _post_init, created after the ALTER. Same class of bug and fix as the registry active-handle index (#1841). Regression test seeds a pre-element_id DB and asserts both stores boot, migrate the column, and build the index.
1 participant
Folds two findings from the multi-day audit of the identity epic.
#2 (boot-brick, the important one):
ux_agent_active_handleis created in_post_init(runs in lifespan). Handle uniqueness among active agents is a NEW invariant, so a DB created before it can hold two active rows with the same handle;CREATE UNIQUE INDEXthen raisesIntegrityErrorand the controller fails to boot on every restart until manual DB surgery. This directly gates deploying the epic to any existing install.Fix:
_migration_v4_dedupe_active_handlesblanks the handle on all but the oldest active row per handle (rows stay active; blanked handles are excluded from the partial index and reclaimable), logging each change; then the index creation is itself guarded (catchIntegrityError, warn, continue) so nothing unanticipated can brick boot. Write-time uniqueness still applies.#10:
register/PATCHreturned an unhandled 500 on a duplicate handle; now 409.Regression tests added: a DB seeded with two active
hermesrows boots, keeps the oldest, blanks the duplicate, and enforces uniqueness afterward. Full registry + mint + auth-requests suite: 227 passed.