chore(iii-database): update version to 0.0.4 and enhance README with transaction details#165
Merged
Merged
Conversation
…transaction details - Bump version of iii-database in Cargo.lock to 0.0.4. - Revise README.md to clarify transaction handling, including new transaction functions and their usage. - Remove outdated trigger documentation related to `iii-database::query-poll`. - Add new sections for interactive transactions and prepared statements, detailing their inputs and outputs. - Introduce new skills documentation for various database operations, enhancing user guidance.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces interactive (multi-RPC) transaction support to the ChangesInteractive Transaction Support
TLS Configuration and Error Classification
Cleanup: Cursor and Query-Poll Trigger Removal
Documentation: Skills Index and API Guides
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
skill-check — worker0 verified, 10 skipped (no docs/).
Three for three. Nicely done. |
…ctions - Updated README.md to clarify the behavior of `ca_cert` and introduced `trust_native` option for TLS configurations, allowing for more flexible certificate management. - Modified TlsConfig struct to include `trust_native` with a default value of true, ensuring the system trust store is extended by default. - Improved error classification in MySQL and Postgres connection pools to provide clearer feedback on connection failures, including specific hints for TLS, authentication, and network issues. - Adjusted tests to utilize the new default TlsConfig settings, ensuring consistency across database connection tests.
5 tasks
…ation bypasses (#168) * fix(iii-database): close interactive-transaction side-channel finalization bypasses The `transactionExecute` / `transactionQuery` handlers held a server-side `BEGIN ... COMMIT` open under a UUID and routed caller-supplied SQL onto that pinned connection. Three ways the caller could finalize the txn out-of-band, desynchronizing `TxRegistry` from the conn's real txn state: 1. `is_transaction_control_sql` only inspected the first whitespace- delimited token. `/* */COMMIT` and `--\nCOMMIT` both reached the driver because PG/MySQL strip comments server-side before parsing. 2. `transactionQuery` had no `is_transaction_control_sql` guard at all — PG/MySQL/SQLite all execute `COMMIT`/`ROLLBACK`/`BEGIN` through the prepared-statement path that `run_prepared` uses. 3. The keyword set missed `START TRANSACTION` (ANSI / MySQL spelling of `BEGIN`). On MySQL, `START TRANSACTION` inside an existing txn implicitly commits the outer one and opens a new (untracked) one. Fix: - Move `is_transaction_control_sql` into a dedicated `handlers::tx_sql_guard` module so both handlers share one defense. - Strip leading whitespace, `;`, line comments (`-- ...\n`), and block comments (`/* ... */`, postgres-style nested) *before* tokenizing. The strip is iterative — `/* a */ /* b */COMMIT` is caught too. Byte scanning is sound because the comment markers are pure ASCII. - Add the two-token `START TRANSACTION` form (alongside `SET TRANSACTION`). `START SLAVE` / `START REPLICA` correctly pass through. - Wire the guard into `transactionQuery::handle`, mirroring the existing log event and `INVALID_PARAM` error shape used by `transactionExecute`. Tests: - 11 unit tests in `tx_sql_guard.rs` covering plain keywords, comment prefixes (block / line / chained / nested), START TRANSACTION variants, comment-only / unterminated inputs, comment-prefixed DML pass-through, and UTF-8 safety. - 6 new handler-level rejection tests (3 in each handler) asserting the rejection fires before the registry is touched and the error names the right finalization handler. - 179 total iii-database lib tests pass. * test(iii-database): add bypass repros + HARNESS_MODE dispatch to e2e harness New file `cases-tx-control-bypass.ts` (320 lines) — 4 cases that each attempt one of the side-channel finalization shapes the previous commit closes, then *prove* the desync if accepted by counting outside-tx rows that shouldn't be visible. Post-fix all 4 PASS; pre-fix at least one FAILs with a clear `BYPASS CONFIRMED [#N]` message naming the leaked row count. | # | Bypass | Drivers | |---|---|---| | 1 | `/* */COMMIT` via `transactionExecute` | all 3 | | 1b | `--\nCOMMIT` via `transactionExecute` | pg + mysql | | 2 | `COMMIT` via `transactionQuery` | all 3 | | 3 | `START TRANSACTION` via `transactionExecute` | mysql only | Harness dispatch: - `RunnerMode = 'full' | 'no-bypass' | 'bypass-only'` (exported), passed via `HARNESS_MODE` env. `bypass-only` skips SCHEMA_RESET since each bypass case owns its scratch table. - `run-tests.sh` adds `--bypass-only` / `--no-bypass` flags and a `COMPOSE` env override defaulting to `docker compose` (preserves CI behavior). For `podman-compose`, the script auto-switches to a `podman inspect .State.Health.Status` poll loop because podman-compose 1.x doesn't implement compose v2's `--wait`. - `SCHEMA_RESET` now defensively drops `outbox` + `__iii_cursors` (vestiges of the removed query-poll trigger that survive `--keep` data volumes). - `cases-interactive-tx.ts` gets two additional regression cases asserting comment-prefixed and START-form rejection on both `transactionExecute` and `transactionQuery` paths. README documents the new flags, the bypass-repro table mapping each repro to its `/review` finding, and the `COMPOSE` override for rootless podman.
andersonleal
approved these changes
May 20, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
iii-database/tests/e2e/workers/harness/src/worker.ts (1)
10-15: 💤 Low valueConsider logging a warning when
HARNESS_MODEhas an unrecognized value.The fallback to
'full'is a safe default, but silently accepting typos like'bypass-onyl'could mask configuration errors. A warning log would help catch these without breaking the run.const MODE_ENV = (process.env.HARNESS_MODE ?? 'full') as string; const MODE: RunnerMode = MODE_ENV === 'bypass-only' || MODE_ENV === 'no-bypass' ? MODE_ENV : 'full'; +if (MODE_ENV !== 'full' && MODE_ENV !== 'bypass-only' && MODE_ENV !== 'no-bypass') { + console.warn(`[harness] unrecognized HARNESS_MODE="${MODE_ENV}", defaulting to "full"`); +}🤖 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 `@iii-database/tests/e2e/workers/harness/src/worker.ts` around lines 10 - 15, The code silently defaults MODE to 'full' when HARNESS_MODE is unrecognized; add a warning log to surface typos by checking MODE_ENV vs allowed values before assigning MODE. Inside the module where MODE_ENV and MODE are defined, compute the allowed set (the RunnerMode variants), and if MODE_ENV is not one of them and not undefined, call the existing logger (or console.warn if none) to emit a clear warning mentioning HARNESS_MODE and the supplied MODE_ENV, then proceed to set MODE as before; reference the symbols MODE_ENV, MODE and RunnerMode to locate the change.iii-database/README.md (2)
62-62: 💤 Low valueConsider breaking the dense TLS explanation into shorter paragraphs or bullets.
This 300+ word paragraph covers several distinct concepts (additive trust,
trust_nativeoverride, mysql limitation). Breaking it into bullets or shorter paragraphs would improve scannability for operators looking for specific TLS configuration details.♻️ Suggested restructure
-`ca_cert` lets you point at a CA bundle for self-hosted databases or managed providers whose root isn't in the OS trust store. **Additive by default**: the supplied certs extend the system trust store rather than replacing it, so the same `TlsConfig` surface works for one database that needs a private CA and another that doesn't. Set `tls.trust_native: false` to switch to the strict-isolation posture (only the `ca_cert` certs trusted; the public web PKI is rejected). Postgres only — `mysql_async`'s rustls path always bundles `webpki_roots` and offers no upstream knob to suppress it. +`ca_cert` lets you point at a CA bundle for self-hosted databases or managed providers whose root isn't in the OS trust store. + +**Additive by default**: the supplied certs extend the system trust store rather than replacing it, so the same `TlsConfig` surface works for one database that needs a private CA and another that doesn't. + +**Strict isolation**: Set `tls.trust_native: false` to trust only the `ca_cert` certs and reject the public web PKI. + +**Driver limitation**: This is Postgres only — `mysql_async`'s rustls path always bundles `webpki_roots` and offers no upstream knob to suppress it.🤖 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 `@iii-database/README.md` at line 62, The README paragraph about TLS is too dense and should be split into multiple short paragraphs or bullets to improve readability; break the long paragraph into distinct points covering (1) what `ca_cert` does and that it is additive by default (mention `TlsConfig` as the surface), (2) how to switch to strict isolation using `tls.trust_native: false`, and (3) the `mysql_async`/rustls limitation that always bundles `webpki_roots` (Postgres is the only one that supports the override), keeping the same terminology (`ca_cert`, `TlsConfig`, `tls.trust_native`, `mysql_async`, `rustls`, `webpki_roots`) so operators can quickly find each fact.
160-160: 💤 Low valueConsider sub-bullets for the DRIVER_ERROR description.
The
DRIVER_ERRORcell covers multiple distinct concepts (inner_code format per driver, pool-acquire classification, redaction behavior, logging location) in one paragraph. Sub-bullets would improve quick reference lookup.🤖 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 `@iii-database/README.md` at line 160, The DRIVER_ERROR table cell bundles several distinct details; split it into sub-bullets under the `DRIVER_ERROR` row so each concept is explicit: one bullet for `inner_code` per-driver formats (Postgres = SQLSTATE 5-char, MySQL = server error number as string, SQLite = rusqlite::ErrorCode debug name), one bullet describing pool-acquire failures and the exact message form `pool connection failed (<class>)` with allowed `<class>` values (`tls`, `auth`, `network`, `server-policy`, `unknown`), one bullet stating that this redaction is deliberate to avoid leaking host/user/db fragments, and one bullet indicating that the full driver error is emitted to the worker stderr via `tracing::warn!`; update the `DRIVER_ERROR` cell text to use these sub-bullets for clarity.
🤖 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 `@iii-database/skills/iii-database/interactive-transaction.md`:
- Around line 35-42: The fenced lifecycle diagram code block lacks a language
identifier; update the triple-backtick fence that surrounds the
beginTransaction/transactionQuery/transactionExecute/commitTransaction/rollbackTransaction
block to include the language tag `text` (i.e., change ``` to ```text) so
markdownlint stops warning while preserving the diagram content and formatting.
In `@iii-database/src/driver/postgres.rs`:
- Around line 459-467: The current RETURNING detection only checks for "
RETURNING " in sql and ignores the _returning parameter, causing statements like
"INSERT ...\nRETURNING ..." to miss returned rows; update the decision logic in
the function containing sql, _returning, bind_params, bound_refs and client so
that it treats a statement as returning if either _returning is non-empty or the
SQL contains the token RETURNING in a case-insensitive, word-boundary aware way
(e.g. use a regex like (?i)\bRETURNING\b or equivalent), and route such
statements to client.query so returned_rows/returned_columns are populated
accordingly. Ensure you still use bound_refs for parameter binding and preserve
existing error paths.
In `@iii-database/src/pool/mysql.rs`:
- Line 110: The match arm that classifies server error codes as "auth"
incorrectly includes 1156 in Error::Server(s) if matches!(s.code, 1045 | 1129 |
1130 | 1156 | 1251 | 1698) => "auth"; remove 1156 (ER_NET_PACKETS_OUT_OF_ORDER)
from that matches! list so it is not treated as an authentication error—either
delete 1156 from the tuple or move it into the appropriate network/protocol
branch so that the server-policy fallback handles it.
In `@iii-database/src/pool/tls.rs`:
- Around line 107-108: The code in build_root_store (invoked from
build_client_config) incorrectly treats rustls-native-certs 0.8.x as returning a
struct with .certs and .errors; instead handle the Result<Vec<...>, Error>
return: call rustls_native_certs::load_native_certs() (or equivalent) and match
on the Result, on Ok iterate the Vec of certificates and add them to your
RootCertStore, and on Err convert or map the io::Error into your DbError (or
return Err) rather than accessing .errors; remove any .certs/.errors field
access and update error handling to propagate or log the load error inside
build_root_store so build_client_config compiles.
In `@iii-database/src/transaction.rs`:
- Around line 116-131: The lock() method currently clones entry.conn and drops
the registry read-lock before awaiting arc.lock_owned(), allowing a TOCTOU where
take() can finalize the transaction; fix by re-checking/locking the registry
entry atomically: either mark the entry as "locked" or check a finalized flag
while still holding the registry lock, then clone the Arc and release the
registry lock only after that marker is set (or alternatively after acquiring
the per-conn mutex validate that the entry still exists and is not finalized),
and ensure TxLock construction only proceeds if the entry hasn't been
taken/finalized; update lock(), take(), and the entry struct (e.g., add a
finalized/locked flag) so the sequence prevents acquiring the per-conn mutex for
already-finalized transactions.
---
Nitpick comments:
In `@iii-database/README.md`:
- Line 62: The README paragraph about TLS is too dense and should be split into
multiple short paragraphs or bullets to improve readability; break the long
paragraph into distinct points covering (1) what `ca_cert` does and that it is
additive by default (mention `TlsConfig` as the surface), (2) how to switch to
strict isolation using `tls.trust_native: false`, and (3) the
`mysql_async`/rustls limitation that always bundles `webpki_roots` (Postgres is
the only one that supports the override), keeping the same terminology
(`ca_cert`, `TlsConfig`, `tls.trust_native`, `mysql_async`, `rustls`,
`webpki_roots`) so operators can quickly find each fact.
- Line 160: The DRIVER_ERROR table cell bundles several distinct details; split
it into sub-bullets under the `DRIVER_ERROR` row so each concept is explicit:
one bullet for `inner_code` per-driver formats (Postgres = SQLSTATE 5-char,
MySQL = server error number as string, SQLite = rusqlite::ErrorCode debug name),
one bullet describing pool-acquire failures and the exact message form `pool
connection failed (<class>)` with allowed `<class>` values (`tls`, `auth`,
`network`, `server-policy`, `unknown`), one bullet stating that this redaction
is deliberate to avoid leaking host/user/db fragments, and one bullet indicating
that the full driver error is emitted to the worker stderr via `tracing::warn!`;
update the `DRIVER_ERROR` cell text to use these sub-bullets for clarity.
In `@iii-database/tests/e2e/workers/harness/src/worker.ts`:
- Around line 10-15: The code silently defaults MODE to 'full' when HARNESS_MODE
is unrecognized; add a warning log to surface typos by checking MODE_ENV vs
allowed values before assigning MODE. Inside the module where MODE_ENV and MODE
are defined, compute the allowed set (the RunnerMode variants), and if MODE_ENV
is not one of them and not undefined, call the existing logger (or console.warn
if none) to emit a clear warning mentioning HARNESS_MODE and the supplied
MODE_ENV, then proceed to set MODE as before; reference the symbols MODE_ENV,
MODE and RunnerMode to locate the change.
🪄 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
Run ID: d4fd448f-a3d6-411f-a5ad-910445cba080
⛔ Files ignored due to path filters (1)
iii-database/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
iii-database/README.mdiii-database/skills/iii-database/execute.mdiii-database/skills/iii-database/interactive-transaction.mdiii-database/skills/iii-database/prepared-statements.mdiii-database/skills/iii-database/query.mdiii-database/skills/iii-database/transaction.mdiii-database/skills/index.mdiii-database/src/config.rsiii-database/src/cursor.rsiii-database/src/driver/mysql.rsiii-database/src/driver/postgres.rsiii-database/src/driver/sqlite.rsiii-database/src/error.rsiii-database/src/handlers/begin_transaction.rsiii-database/src/handlers/commit_transaction.rsiii-database/src/handlers/execute.rsiii-database/src/handlers/mod.rsiii-database/src/handlers/prepare.rsiii-database/src/handlers/query.rsiii-database/src/handlers/rollback_transaction.rsiii-database/src/handlers/run_statement.rsiii-database/src/handlers/transaction.rsiii-database/src/handlers/transaction_execute.rsiii-database/src/handlers/transaction_query.rsiii-database/src/handlers/tx_sql_guard.rsiii-database/src/lib.rsiii-database/src/main.rsiii-database/src/pool/mysql.rsiii-database/src/pool/postgres.rsiii-database/src/pool/tls.rsiii-database/src/transaction.rsiii-database/src/triggers/handler.rsiii-database/src/triggers/mod.rsiii-database/src/triggers/query_poll.rsiii-database/src/triggers/row_change.rsiii-database/tests/e2e/README.mdiii-database/tests/e2e/run-tests.shiii-database/tests/e2e/workers/harness/src/cases-boundary.tsiii-database/tests/e2e/workers/harness/src/cases-interactive-tx.tsiii-database/tests/e2e/workers/harness/src/cases-trigger.tsiii-database/tests/e2e/workers/harness/src/cases-tx-control-bypass.tsiii-database/tests/e2e/workers/harness/src/cases.tsiii-database/tests/e2e/workers/harness/src/runner.tsiii-database/tests/e2e/workers/harness/src/worker.tsiii-database/tests/integration.rs
💤 Files with no reviewable changes (3)
- iii-database/tests/e2e/workers/harness/src/cases-trigger.ts
- iii-database/src/triggers/query_poll.rs
- iii-database/src/cursor.rs
Comment on lines
+35
to
+42
| ``` | ||
| beginTransaction(db, isolation?, timeout_ms?) → { transaction: { id, expires_at } } | ||
| ├─ transactionQuery ( transaction_id, sql, params ) → { rows, row_count, columns } | ||
| ├─ transactionExecute ( transaction_id, sql, params ) → { affected_rows, last_insert_id, returned_rows } | ||
| ├─ ... (repeat) ... | ||
| ├─ commitTransaction ( transaction_id ) → { committed: true } | ||
| └─ rollbackTransaction ( transaction_id ) → { rolled_back: true } | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to the lifecycle code block.
The lifecycle diagram uses fenced code blocks without a language specifier, which triggers a markdownlint warning. Add text as the language identifier to satisfy the linter.
🔧 Proposed fix
-```
+```text
beginTransaction(db, isolation?, timeout_ms?) → { transaction: { id, expires_at } }
├─ transactionQuery ( transaction_id, sql, params ) → { rows, row_count, columns }
├─ transactionExecute ( transaction_id, sql, params ) → { affected_rows, last_insert_id, returned_rows }🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 35-35: 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 `@iii-database/skills/iii-database/interactive-transaction.md` around lines 35
- 42, The fenced lifecycle diagram code block lacks a language identifier;
update the triple-backtick fence that surrounds the
beginTransaction/transactionQuery/transactionExecute/commitTransaction/rollbackTransaction
block to include the language tag `text` (i.e., change ``` to ```text) so
markdownlint stops warning while preserving the diagram content and formatting.
Comment on lines
+459
to
+467
| _returning: &[String], | ||
| ) -> Result<ExecuteResult, DbError> { | ||
| let bound = bind_params(params); | ||
| let bound_refs: Vec<&(dyn ToSql + Sync)> = | ||
| bound.iter().map(|p| p as &(dyn ToSql + Sync)).collect(); | ||
|
|
||
| let upper = sql.to_ascii_uppercase(); | ||
| if upper.contains(" RETURNING ") { | ||
| let rows = client |
There was a problem hiding this comment.
RETURNING detection is fragile and can silently drop returned rows.
At Line 465–467, detection only matches " RETURNING " and _returning is ignored (Line 459). Statements like INSERT ...\nRETURNING ... won’t be routed to query(), so returned_rows/returned_columns come back empty.
Proposed fix
-pub async fn tx_execute(
+pub async fn tx_execute(
client: &mut crate::pool::postgres::PgClient,
sql: &str,
params: &[JsonParam],
- _returning: &[String],
+ returning: &[String],
) -> Result<ExecuteResult, DbError> {
@@
- let upper = sql.to_ascii_uppercase();
- if upper.contains(" RETURNING ") {
+ let upper = sql.to_ascii_uppercase();
+ let returns_rows = !returning.is_empty() || upper.contains("RETURNING");
+ if returns_rows {📝 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.
Suggested change
| _returning: &[String], | |
| ) -> Result<ExecuteResult, DbError> { | |
| let bound = bind_params(params); | |
| let bound_refs: Vec<&(dyn ToSql + Sync)> = | |
| bound.iter().map(|p| p as &(dyn ToSql + Sync)).collect(); | |
| let upper = sql.to_ascii_uppercase(); | |
| if upper.contains(" RETURNING ") { | |
| let rows = client | |
| returning: &[String], | |
| ) -> Result<ExecuteResult, DbError> { | |
| let bound = bind_params(params); | |
| let bound_refs: Vec<&(dyn ToSql + Sync)> = | |
| bound.iter().map(|p| p as &(dyn ToSql + Sync)).collect(); | |
| let upper = sql.to_ascii_uppercase(); | |
| let returns_rows = !returning.is_empty() || upper.contains("RETURNING"); | |
| if returns_rows { | |
| let rows = client |
🤖 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 `@iii-database/src/driver/postgres.rs` around lines 459 - 467, The current
RETURNING detection only checks for " RETURNING " in sql and ignores the
_returning parameter, causing statements like "INSERT ...\nRETURNING ..." to
miss returned rows; update the decision logic in the function containing sql,
_returning, bind_params, bound_refs and client so that it treats a statement as
returning if either _returning is non-empty or the SQL contains the token
RETURNING in a case-insensitive, word-boundary aware way (e.g. use a regex like
(?i)\bRETURNING\b or equivalent), and route such statements to client.query so
returned_rows/returned_columns are populated accordingly. Ensure you still use
bound_refs for parameter binding and preserve existing error paths.
| // standard auth surface (access denied, host blocked, bad SSL). | ||
| // Everything else from the server (max_connections, replication | ||
| // lag, etc.) lands under server-policy. | ||
| Error::Server(s) if matches!(s.code, 1045 | 1129 | 1130 | 1156 | 1251 | 1698) => "auth", |
There was a problem hiding this comment.
Error code 1156 appears misclassified as auth.
MySQL error 1156 (ER_NET_PACKETS_OUT_OF_ORDER) indicates "Got packets out of order" — a network/protocol-level failure, not an authentication error. Consider moving it out of the auth set or removing it entirely (letting it fall to server-policy).
Suggested fix
- Error::Server(s) if matches!(s.code, 1045 | 1129 | 1130 | 1156 | 1251 | 1698) => "auth",
+ Error::Server(s) if matches!(s.code, 1045 | 1129 | 1130 | 1251 | 1698) => "auth",📝 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.
Suggested change
| Error::Server(s) if matches!(s.code, 1045 | 1129 | 1130 | 1156 | 1251 | 1698) => "auth", | |
| Error::Server(s) if matches!(s.code, 1045 | 1129 | 1130 | 1251 | 1698) => "auth", |
🤖 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 `@iii-database/src/pool/mysql.rs` at line 110, The match arm that classifies
server error codes as "auth" incorrectly includes 1156 in Error::Server(s) if
matches!(s.code, 1045 | 1129 | 1130 | 1156 | 1251 | 1698) => "auth"; remove 1156
(ER_NET_PACKETS_OUT_OF_ORDER) from that matches! list so it is not treated as an
authentication error—either delete 1156 from the tuple or move it into the
appropriate network/protocol branch so that the server-policy fallback handles
it.
Comment on lines
107
to
+108
| fn build_client_config(tls: &TlsConfig) -> Result<ClientConfig, DbError> { | ||
| let roots = build_root_store(tls.ca_cert.as_deref())?; | ||
| let roots = build_root_store(tls.ca_cert.as_deref(), tls.trust_native)?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
rustls-native-certs 0.8 load_native_certs return type
💡 Result:
In rustls-native-certs 0.8.x, load_native_certs returns: Result<Vec<pki_types::CertificateDer<'static>>, std::io::Error> [1][2] On success, it yields a Vec of parsed root certificates (pki_types::CertificateDer<'static>) loaded from the platform’s native certificate store. [1][2]
Citations:
- 1: https://docs.rs/rustls-native-certs/%5E0.8/
- 2: https://crates.io/crates/rustls-native-certs/0.8.2
🏁 Script executed:
cat -n iii-database/src/pool/tls.rs | sed -n '100,115p'Repository: iii-hq/workers
Length of output: 833
🏁 Script executed:
cat -n iii-database/src/pool/tls.rs | sed -n '170,235p'Repository: iii-hq/workers
Length of output: 2828
🏁 Script executed:
find . -name "Cargo.toml" -type f | head -5Repository: iii-hq/workers
Length of output: 175
🏁 Script executed:
grep -A 5 -B 5 "rustls-native-certs" Cargo.toml 2>/dev/null || grep -r "rustls-native-certs" . --include="Cargo.toml" 2>/dev/nullRepository: iii-hq/workers
Length of output: 112
🌐 Web query:
rustls-native-certs 0.8 load_native_certs struct certs errors fields
💡 Result:
In rustls-native-certs version 0.8, the load_native_certs function does not return a struct with an "errors" field. Instead, it returns a Result type [1][2]. The function signature is: pub fn load_native_certs -> Result<Vec<pki_types::CertificateDer<'static>>, std::io::Error> [1][3] When successful, it returns a Ok(Vec<...>) containing the loaded certificates [1][4]. If it fails, it returns an Err(std::io::Error), which contains the error information [1][4]. This aligns with standard Rust error handling patterns rather than returning a struct that explicitly encapsulates both certificates and errors as fields [1][3]. Note that the crate authors now recommend using rustls-platform-verifier instead of rustls-native-certs for more robust certificate verification and a simpler API [1][2][3].
Citations:
- 1: https://docs.rs/rustls-native-certs/%5E0.8/
- 2: https://crates.io/crates/rustls-native-certs/0.8.2
- 3: https://docs.rs/crate/rustls-native-certs/latest
- 4: https://crates.io/crates/rustls-native-certs/0.6.3
Fix API usage: rustls-native-certs 0.8.x returns Result<Vec<...>, Error>, not a struct.
The code at lines 182–194 accesses result.certs and result.errors as struct fields, but rustls-native-certs 0.8.x (specified in Cargo.toml) returns Result<Vec<pki_types::CertificateDer<'static>>, std::io::Error>. The .certs and .errors field accesses will not compile. Update the error handling to match the actual API: unwrap the Result and handle the error variant directly rather than accessing .errors on a struct field.
🤖 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 `@iii-database/src/pool/tls.rs` around lines 107 - 108, The code in
build_root_store (invoked from build_client_config) incorrectly treats
rustls-native-certs 0.8.x as returning a struct with .certs and .errors; instead
handle the Result<Vec<...>, Error> return: call
rustls_native_certs::load_native_certs() (or equivalent) and match on the
Result, on Ok iterate the Vec of certificates and add them to your
RootCertStore, and on Err convert or map the io::Error into your DbError (or
return Err) rather than accessing .errors; remove any .certs/.errors field
access and update error handling to propagate or log the load error inside
build_root_store so build_client_config compiles.
Comment on lines
+116
to
+131
| pub async fn lock(&self, id: &str) -> Result<TxLock, DbError> { | ||
| // Extract the Arc under a brief read lock, then await `lock_owned` | ||
| // outside the map's read lock. Holding the map read-lock across an | ||
| // `.await` would block all other handler lookups (begin/lock/take) | ||
| // for the duration of any in-flight statement on this id. | ||
| let g = self.inner.read().await; | ||
| let entry = g.get(id).ok_or_else(|| DbError::TransactionNotFound { | ||
| transaction_id: id.to_string(), | ||
| })?; | ||
| let db_name = entry.db_name.clone(); | ||
| let driver = entry.driver; | ||
| let started_at = entry.started_at; | ||
| let arc = Arc::clone(&entry.conn); | ||
| drop(g); | ||
| let guard = arc.lock_owned().await; | ||
| Ok(TxLock { |
There was a problem hiding this comment.
TOCTOU race allows statement execution after finalization.
At Line 121–130, lock() grabs the connection Arc then releases the registry lock before awaiting the per-conn mutex. If take() finalizes in between, this in-flight lock() can still acquire the mutex later and run SQL after commit/rollback. This breaks one-shot finalization semantics.
Suggested direction
- // Current shape: map lookup -> clone Arc -> drop map lock -> await mutex
+ // Use a finalized flag/state tied to the same entry object:
+ // 1) lock(): acquire per-conn mutex, then verify entry is not finalized.
+ // 2) take(): atomically mark finalized before waiting for per-conn mutex.
+ // This removes the lookup/use race without global map-locking around statement execution.🤖 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 `@iii-database/src/transaction.rs` around lines 116 - 131, The lock() method
currently clones entry.conn and drops the registry read-lock before awaiting
arc.lock_owned(), allowing a TOCTOU where take() can finalize the transaction;
fix by re-checking/locking the registry entry atomically: either mark the entry
as "locked" or check a finalized flag while still holding the registry lock,
then clone the Arc and release the registry lock only after that marker is set
(or alternatively after acquiring the per-conn mutex validate that the entry
still exists and is not finalized), and ensure TxLock construction only proceeds
if the entry hasn't been taken/finalized; update lock(), take(), and the entry
struct (e.g., add a finalized/locked flag) so the sequence prevents acquiring
the per-conn mutex for already-finalized transactions.
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.
iii-database::query-poll.ca_certand introducedtrust_nativeoption for TLS configurations, allowing for more flexible certificate management.trust_nativewith a default value of true, ensuring the system trust store is extended by default.Summary by CodeRabbit
Release Notes
New Features
beginTransaction,transactionQuery,transactionExecute,commitTransaction,rollbackTransaction) for multi-step stateful database operations.trust_nativeTLS configuration option for flexible system trust store management.Documentation
Bug Fixes
TRANSACTION_NOT_FOUNDcode and improved driver error classification.Removed Features