Fix race condition in bot_registry create_bot() and enqueue_update() #17

Merged

Copilot merged 3 commits from copilot/request-code-review into master

2026-03-04 20:38:00 +01:00

Copilot commented

2026-03-04 20:20:36 +01:00

(Migrated from github.com)

create_bot() and enqueue_update() retrieve the new row ID via a separate SELECT after INSERT. Under concurrent requests, a different INSERT can interleave, returning the wrong ID — causing token assignment to the wrong bot.

InsertBuilder.perform() already returns last_insert_rowid() atomically; the return value was simply being discarded.

bot_registry.vala: Use perform() return value directly instead of post-INSERT SELECT query, in both create_bot() and enqueue_update()
BUG_AUDIT.md: Mark BUG-04 as fixed

// Before: racy SELECT after INSERT
bot.insert().value(...).perform();
int result_id = 0;
foreach (Row row in bot.select({bot.id}).order_by(bot.id, "DESC").limit(1)) {
    result_id = bot.id.get(row);
}

// After: atomic last_insert_rowid via perform() return
int64 new_bot_id = bot.insert().value(...).perform();
return (int) new_bot_id;

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

`create_bot()` and `enqueue_update()` retrieve the new row ID via a separate SELECT after INSERT. Under concurrent requests, a different INSERT can interleave, returning the wrong ID — causing token assignment to the wrong bot. `InsertBuilder.perform()` already returns `last_insert_rowid()` atomically; the return value was simply being discarded. - **bot_registry.vala**: Use `perform()` return value directly instead of post-INSERT SELECT query, in both `create_bot()` and `enqueue_update()` - **BUG_AUDIT.md**: Mark BUG-04 as fixed ```vala // Before: racy SELECT after INSERT bot.insert().value(...).perform(); int result_id = 0; foreach (Row row in bot.select({bot.id}).order_by(bot.id, "DESC").limit(1)) { result_id = bot.id.get(row); } // After: atomic last_insert_rowid via perform() return int64 new_bot_id = bot.insert().value(...).perform(); return (int) new_bot_id; ```  --- ✨ Let Copilot coding agent [set things up for you](https://github.com/rallep71/dinox/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-03-04 20:41:22 +01:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

Fixes a concurrency hazard in the bot-features plugin by returning SQLite’s per-connection last inserted row ID directly from InsertBuilder.perform() instead of doing a separate post-INSERT SELECT to discover the new row ID.

Changes:

Update BotRegistry.create_bot() to use the InsertBuilder.perform() return value as the new bot ID.
Update BotRegistry.enqueue_update() to use the InsertBuilder.perform() return value as the new update ID.
Mark BUG-04 as fixed in the bot-features bug audit document.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
plugins/bot-features/src/bot_registry.vala	Removes racy post-`INSERT` lookup patterns; returns the `perform()`-reported row ID for new bots and queued updates.
plugins/bot-features/BUG_AUDIT.md	Updates BUG-04 entry to indicate the race condition is fixed and documents the new approach.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview Fixes a concurrency hazard in the bot-features plugin by returning SQLite’s per-connection last inserted row ID directly from `InsertBuilder.perform()` instead of doing a separate post-`INSERT` `SELECT` to discover the new row ID. **Changes:** - Update `BotRegistry.create_bot()` to use the `InsertBuilder.perform()` return value as the new bot ID. - Update `BotRegistry.enqueue_update()` to use the `InsertBuilder.perform()` return value as the new update ID. - Mark BUG-04 as fixed in the bot-features bug audit document. ### Reviewed changes Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments. | File | Description | | ---- | ----------- | | plugins/bot-features/src/bot_registry.vala | Removes racy post-`INSERT` lookup patterns; returns the `perform()`-reported row ID for new bots and queued updates. | | plugins/bot-features/BUG_AUDIT.md | Updates BUG-04 entry to indicate the race condition is fixed and documents the new approach. | --- 💡 <a href="/rallep71/dinox/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

plugins/bot-features/src/bot_registry.vala

InsertBuilder.perform() logs on SQLite failure but still returns db.last_insert_rowid(). With the new logic, if this INSERT fails (e.g., bot.token_hash is declared UNIQUE and several callers pass an empty string), create_bot() may return a stale ID from a previous successful INSERT, causing follow-up token assignment/logging to target the wrong bot. Consider making insert failure explicit here (e.g., validate the inserted row exists for new_bot_id and/or change the API to throw/return an error when perform().step() is not DONE).

`InsertBuilder.perform()` logs on SQLite failure but still returns `db.last_insert_rowid()`. With the new logic, if this INSERT fails (e.g., `bot.token_hash` is declared `UNIQUE` and several callers pass an empty string), `create_bot()` may return a stale ID from a previous successful INSERT, causing follow-up token assignment/logging to target the wrong bot. Consider making insert failure explicit here (e.g., validate the inserted row exists for `new_bot_id` and/or change the API to throw/return an error when `perform().step()` is not `DONE`).

plugins/bot-features/src/bot_registry.vala

Same issue as create_bot(): perform() returns last_insert_rowid() even if the INSERT fails. If the insert into update_queue errors (DB locked, constraint/migration issue, etc.), this method will return a stale update ID, and callers may treat it as the newly enqueued update. Please add a success check / error signaling path rather than unconditionally casting and returning the ID.

Same issue as `create_bot()`: `perform()` returns `last_insert_rowid()` even if the INSERT fails. If the insert into `update_queue` errors (DB locked, constraint/migration issue, etc.), this method will return a stale update ID, and callers may treat it as the newly enqueued update. Please add a success check / error signaling path rather than unconditionally casting and returning the ID.