refactor(llm-access-store): domain-split postgres.rs stateful impl (PR-B)#15
Conversation
Second of two PRs splitting the ~9.4k-line postgres.rs. PR-A moved the pure free-fn leaf layer; this PR-B domain-splits the stateful PostgresControlRepository God-object: 94 of its 103 inherent methods plus 14 of the 16 trait impls move into 12 domain submodules. - cache: request-path caching layer (loads/builds/invalidation) - config/keys/groups/proxy/codex_account/kiro_account/routes/usage/public/ review/status: one per domain, each carrying its `*Store` trait impl plus the inherent methods that back it Infra hubs (connect*, ensure_connection_alive, generation counters, count_rows), the ControlStore impl, all types, the sqlx wrapper, and the usage-rollup struct/fn stay in mod.rs (9361 -> 1648 lines). Behavior-preserving structural move: identical inventory (94+9 methods, 15 trait impls, all types), same 10 tests. Moved private methods become `pub(super)` only where a foreign region calls them (49 of 94) — the minimal non-leaking visibility that preserves the exact "private within the postgres module" boundary; `pub` would expand the public API and `pub(crate)` would over-widen. Submodules read the infra hubs and all types as ancestor-privates; cross-domain trait-method calls import the owning trait. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/gemini review |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive PostgreSQL-backed control repository layer for the LLM access store, covering caching, account management, proxy configurations, routing, and usage tracking. The review feedback focuses on optimizing database and cache interactions by recommending early returns for empty collections and replacing preliminary existence checks with direct updates to reduce redundant queries and roundtrips.
| pub(super) async fn invalidate_authenticated_key_cache_by_ids(&self, key_ids: &[String]) { | ||
| let Some(cache) = self.request_cache.as_ref() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
To avoid redundant database queries and unnecessary cache operations, we should return early if key_ids is empty.
pub(super) async fn invalidate_authenticated_key_cache_by_ids(&self, key_ids: &[String]) {
if key_ids.is_empty() {
return;
}
let Some(cache) = self.request_cache.as_ref() else {
return;
};There was a problem hiding this comment.
The empty-list early return is a fair micro-optimization (skips an empty key-build loop / cache call), and likely a no-op functionally. But it's still a logic edit on pre-existing code that this PR moves verbatim — this is a behavior-preserving structural split whose correctness claim rests on the method bodies being byte-identical to the pre-split source (verified by the item-inventory diff). Folding even a "probably harmless" tweak in here erodes that guarantee. Deferring to the follow-up perf PR.
There was a problem hiding this comment.
Applied in a3b207c (empty-input early return).
| }; | ||
| let scope = self.proxy_scope.cache_key_segment(); | ||
| let view_keys = account_names |
There was a problem hiding this comment.
To avoid performing cache operations with an empty list of keys, we should return early if account_names is empty.
| }; | |
| let scope = self.proxy_scope.cache_key_segment(); | |
| let view_keys = account_names | |
| _ => return, | |
| }; | |
| if account_names.is_empty() { | |
| return; | |
| } | |
| let scope = self.proxy_scope.cache_key_segment(); |
There was a problem hiding this comment.
The empty-list early return is a fair micro-optimization (skips an empty key-build loop / cache call), and likely a no-op functionally. But it's still a logic edit on pre-existing code that this PR moves verbatim — this is a behavior-preserving structural split whose correctness claim rests on the method bodies being byte-identical to the pre-split source (verified by the item-inventory diff). Folding even a "probably harmless" tweak in here erodes that guarantee. Deferring to the follow-up perf PR.
There was a problem hiding this comment.
Applied in a3b207c (empty-input early return).
| pub(super) async fn disable_admin_key_if_present( | ||
| &self, | ||
| key_id: &str, | ||
| updated_at_ms: i64, | ||
| ) -> anyhow::Result<()> { | ||
| if self.load_key_bundle_by_id(key_id).await?.is_some() { | ||
| self.patch_admin_key(key_id, AdminKeyPatch { | ||
| status: Some(core_store::KEY_STATUS_DISABLED.to_string()), | ||
| updated_at_ms, | ||
| ..AdminKeyPatch::default() | ||
| }) | ||
| .await?; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Instead of loading the key bundle first to check if it exists, we can call patch_admin_key directly. Since patch_admin_key already performs this check internally, this avoids a redundant database query.
pub(super) async fn disable_admin_key_if_present(
&self,
key_id: &str,
updated_at_ms: i64,
) -> anyhow::Result<()> {
self.patch_admin_key(key_id, AdminKeyPatch {
status: Some(core_store::KEY_STATUS_DISABLED.to_string()),
updated_at_ms,
..AdminKeyPatch::default()
})
.await?;
Ok(())
}There was a problem hiding this comment.
Routing disable_admin_key_if_present through patch_admin_key does avoid the pre-load, but it's behavior-adjacent, not a no-op: the two paths differ in the SQL executed, the error/return semantics when the key is absent (silent no-op via the current presence check vs. whatever patch_admin_key does on a missing row), and which cache invalidations fire. Whether they're equivalent for every caller needs verification.
This is pre-existing code moved verbatim in a behavior-preserving structural-split PR. Deferring the dedup to the follow-up perf PR where it can be checked in isolation.
There was a problem hiding this comment.
Applied in a3b207c. Verified patch_admin_key already performs the load_key_bundle_by_id presence check and is side-effect-free (Ok(None), no cache/dispatch mutations) on a missing key, so the outer if is_some was a pure redundant round-trip.
| if self | ||
| .get_admin_account_contribution_request_row(request_id) | ||
| .await? | ||
| .is_none() | ||
| { | ||
| return Ok(None); | ||
| } | ||
| self.ensure_connection_alive()?; | ||
| self.client | ||
| .execute( | ||
| "UPDATE llm_account_contribution_requests | ||
| SET status = $2, | ||
| account_id = $3, | ||
| id_token = $4, | ||
| access_token = $5, | ||
| refresh_token = $6, | ||
| admin_note = $7, | ||
| failure_reason = NULL, | ||
| updated_at_ms = $8, | ||
| processed_at_ms = NULL | ||
| WHERE request_id = $1", | ||
| &[ | ||
| &request_id, | ||
| &PUBLIC_ACCOUNT_CONTRIBUTION_STATUS_VALIDATED, | ||
| &account_id, | ||
| &id_token, | ||
| &access_token, | ||
| &refresh_token, | ||
| &action.admin_note, | ||
| &action.updated_at_ms, | ||
| ], | ||
| ) | ||
| .await | ||
| .context("validate postgres admin account contribution request")?; | ||
| self.get_admin_account_contribution_request_row(request_id) | ||
| .await |
There was a problem hiding this comment.
Instead of performing a redundant get_admin_account_contribution_request_row query before the update, we can execute the UPDATE directly and check if any rows were affected. This reduces the number of database roundtrips from three to two.
self.ensure_connection_alive()?;
let rows_affected = self.client
.execute(
"UPDATE llm_account_contribution_requests
SET status = $2,
account_id = $3,
id_token = $4,
access_token = $5,
refresh_token = $6,
admin_note = $7,
failure_reason = NULL,
updated_at_ms = $8,
processed_at_ms = NULL
WHERE request_id = $1",
&[
&request_id,
&PUBLIC_ACCOUNT_CONTRIBUTION_STATUS_VALIDATED,
&account_id,
&id_token,
&access_token,
&refresh_token,
&action.admin_note,
&action.updated_at_ms,
],
)
.await
.context("validate postgres admin account contribution request")?;
if rows_affected == 0 {
return Ok(None);
}
self.get_admin_account_contribution_request_row(request_id)
.awaitThere was a problem hiding this comment.
The 3→2 roundtrip reduction is a real optimization, but it's behavior-adjacent, not a no-op: the current get-then-UPDATE path and the proposed UPDATE-then-get-if-affected path aren't trivially equivalent. They differ in the existence-check semantics (what's returned when the row is absent vs. present-but-unchanged), the concurrency window between the two statements, and whether rows_affected == 0 is provably identical to "row not found" for this schema/trigger setup. Establishing that equivalence requires per-method analysis.
This is pre-existing code moved verbatim in a behavior-preserving structural-split PR. Verifying and applying the optimization belongs in a dedicated follow-up perf PR, not mixed into a move whose correctness claim rests on the bodies being byte-identical.
There was a problem hiding this comment.
Applied in a3b207c. Confirmed equivalence before applying: the pre-SELECT was a bare is_none() with no state guard, and the UPDATE keys only on request_id, so execute + rows_affected == 0 returns the same Ok(None)/row as before — now 2 round-trips instead of 3. (reject_* deliberately keeps its SELECT; it needs issued_key_id/imported_account_name for the cascade.)
| if self | ||
| .get_admin_account_contribution_request_row(request_id) | ||
| .await? | ||
| .is_none() | ||
| { | ||
| return Ok(None); | ||
| } | ||
| self.ensure_connection_alive()?; | ||
| self.client | ||
| .execute( | ||
| "UPDATE llm_account_contribution_requests | ||
| SET status = 'failed', | ||
| admin_note = $2, | ||
| failure_reason = $3, | ||
| updated_at_ms = $4, | ||
| processed_at_ms = NULL | ||
| WHERE request_id = $1", | ||
| &[&request_id, &action.admin_note, &failure_reason, &action.updated_at_ms], | ||
| ) | ||
| .await | ||
| .context("fail postgres admin account contribution request")?; | ||
| self.get_admin_account_contribution_request_row(request_id) | ||
| .await |
There was a problem hiding this comment.
Instead of performing a redundant get_admin_account_contribution_request_row query before the update, we can execute the UPDATE directly and check if any rows were affected. This reduces the number of database roundtrips from three to two.
| if self | |
| .get_admin_account_contribution_request_row(request_id) | |
| .await? | |
| .is_none() | |
| { | |
| return Ok(None); | |
| } | |
| self.ensure_connection_alive()?; | |
| self.client | |
| .execute( | |
| "UPDATE llm_account_contribution_requests | |
| SET status = 'failed', | |
| admin_note = $2, | |
| failure_reason = $3, | |
| updated_at_ms = $4, | |
| processed_at_ms = NULL | |
| WHERE request_id = $1", | |
| &[&request_id, &action.admin_note, &failure_reason, &action.updated_at_ms], | |
| ) | |
| .await | |
| .context("fail postgres admin account contribution request")?; | |
| self.get_admin_account_contribution_request_row(request_id) | |
| .await | |
| self.ensure_connection_alive()?; | |
| let rows_affected = self.client | |
| .execute( | |
| "UPDATE llm_account_contribution_requests | |
| SET status = 'failed', | |
| admin_note = $2, | |
| failure_reason = $3, | |
| updated_at_ms = $4, | |
| processed_at_ms = NULL | |
| WHERE request_id = $1", | |
| &[&request_id, &action.admin_note, &failure_reason, &action.updated_at_ms], | |
| ) | |
| .await | |
| .context("fail postgres admin account contribution request")?; | |
| if rows_affected == 0 { | |
| return Ok(None); | |
| } | |
| self.get_admin_account_contribution_request_row(request_id) | |
| .await |
There was a problem hiding this comment.
The 3→2 roundtrip reduction is a real optimization, but it's behavior-adjacent, not a no-op: the current get-then-UPDATE path and the proposed UPDATE-then-get-if-affected path aren't trivially equivalent. They differ in the existence-check semantics (what's returned when the row is absent vs. present-but-unchanged), the concurrency window between the two statements, and whether rows_affected == 0 is provably identical to "row not found" for this schema/trigger setup. Establishing that equivalence requires per-method analysis.
This is pre-existing code moved verbatim in a behavior-preserving structural-split PR. Verifying and applying the optimization belongs in a dedicated follow-up perf PR, not mixed into a move whose correctness claim rests on the bodies being byte-identical.
There was a problem hiding this comment.
Applied in a3b207c. Confirmed equivalence before applying: the pre-SELECT was a bare is_none() with no state guard, and the UPDATE keys only on request_id, so execute + rows_affected == 0 returns the same Ok(None)/row as before — now 2 round-trips instead of 3. (reject_* deliberately keeps its SELECT; it needs issued_key_id/imported_account_name for the cascade.)
| if self | ||
| .get_admin_sponsor_request_row(request_id) | ||
| .await? | ||
| .is_none() | ||
| { | ||
| return Ok(None); | ||
| } | ||
| self.ensure_connection_alive()?; | ||
| self.client | ||
| .execute( | ||
| "UPDATE llm_sponsor_requests | ||
| SET status = 'approved', | ||
| admin_note = $2, | ||
| failure_reason = NULL, | ||
| updated_at_ms = $3, | ||
| processed_at_ms = $3 | ||
| WHERE request_id = $1", | ||
| &[&request_id, &action.admin_note, &action.updated_at_ms], | ||
| ) | ||
| .await | ||
| .context("approve postgres sponsor request")?; | ||
| self.get_admin_sponsor_request_row(request_id).await |
There was a problem hiding this comment.
Instead of performing a redundant get_admin_sponsor_request_row query before the update, we can execute the UPDATE directly and check if any rows were affected. This reduces the number of database roundtrips from three to two.
self.ensure_connection_alive()?;
let rows_affected = self.client
.execute(
"UPDATE llm_sponsor_requests
SET status = 'approved',
admin_note = $2,
failure_reason = NULL,
updated_at_ms = $3,
processed_at_ms = $3
WHERE request_id = $1",
&[&request_id, &action.admin_note, &action.updated_at_ms],
)
.await
.context("approve postgres sponsor request")?;
if rows_affected == 0 {
return Ok(None);
}
self.get_admin_sponsor_request_row(request_id).awaitThere was a problem hiding this comment.
The 3→2 roundtrip reduction is a real optimization, but it's behavior-adjacent, not a no-op: the current get-then-UPDATE path and the proposed UPDATE-then-get-if-affected path aren't trivially equivalent. They differ in the existence-check semantics (what's returned when the row is absent vs. present-but-unchanged), the concurrency window between the two statements, and whether rows_affected == 0 is provably identical to "row not found" for this schema/trigger setup. Establishing that equivalence requires per-method analysis.
This is pre-existing code moved verbatim in a behavior-preserving structural-split PR. Verifying and applying the optimization belongs in a dedicated follow-up perf PR, not mixed into a move whose correctness claim rests on the bodies being byte-identical.
There was a problem hiding this comment.
Applied in a3b207c. Confirmed equivalence before applying: the pre-SELECT was a bare is_none() with no state guard, and the UPDATE keys only on request_id, so execute + rows_affected == 0 returns the same Ok(None)/row as before — now 2 round-trips instead of 3. (reject_* deliberately keeps its SELECT; it needs issued_key_id/imported_account_name for the cascade.)
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request implements the Postgres-backed control repository (PostgresControlRepository) for the LLM access store, adding comprehensive support for caching, account management (Codex and Kiro), configuration, routing, API keys, proxy configurations, and usage tracking. The review feedback suggests minor code improvements, specifically avoiding unnecessary .to_vec() allocations when passing slices to SQL queries in keys.rs and routes.rs, and removing a leading underscore from the heavily used _update parameter in routes.rs to follow standard Rust naming conventions.
| "SELECT key_id, key_hash | ||
| FROM llm_keys | ||
| WHERE key_id = ANY($1)", | ||
| &[&key_ids.to_vec()], |
There was a problem hiding this comment.
Avoid calling .to_vec() on key_ids when passing it to the SQL query. Since tokio-postgres supports slices directly (i.e., &[T] where T: ToSql), you can pass &key_ids directly. This avoids allocating a temporary Vec<String> and cloning all of its string elements.
| &[&key_ids.to_vec()], | |
| &[&key_ids], |
There was a problem hiding this comment.
This crate binds parameters through sqlx (sqlx_core / sqlx_postgres), not tokio-postgres, so the "tokio-postgres supports slices directly" rationale doesn't apply here. Parameters are bound via the SqlxBindParam blanket impl, which requires T: Clone + Send + Sync + for<'q> Encode<'q, Postgres> + Type<Postgres>; whether the borrowed slice (&[String]) satisfies that in array position — versus the owned Vec<String> that .to_vec() produces — needs a compile check, not an assumption.
More to the point: this is pre-existing code moved verbatim in a behavior-preserving structural-split PR (the .to_vec() predates this PR). Deferring the allocation tweak to a follow-up perf PR where it can be compile-verified in isolation.
There was a problem hiding this comment.
Applied in a3b207c. &[String] compiles and binds identically under sqlx (the tokio-postgres framing didn't apply, but the allocation removal is valid). Verified: clippy -D warnings clean + 67 tests green.
| FROM llm_codex_accounts | ||
| WHERE account_name = ANY($1) | ||
| ORDER BY account_name", | ||
| &[&account_names.to_vec()], |
There was a problem hiding this comment.
Avoid calling .to_vec() on account_names when passing it to the SQL query. Since tokio-postgres supports slices directly, you can pass &account_names directly. This avoids allocating a temporary Vec<String> and cloning all of its string elements.
| &[&account_names.to_vec()], | |
| &[&account_names], |
There was a problem hiding this comment.
This crate binds parameters through sqlx (sqlx_core / sqlx_postgres), not tokio-postgres, so the "tokio-postgres supports slices directly" rationale doesn't apply here. Parameters are bound via the SqlxBindParam blanket impl, which requires T: Clone + Send + Sync + for<'q> Encode<'q, Postgres> + Type<Postgres>; whether the borrowed slice (&[String]) satisfies that in array position — versus the owned Vec<String> that .to_vec() produces — needs a compile check, not an assumption.
More to the point: this is pre-existing code moved verbatim in a behavior-preserving structural-split PR (the .to_vec() predates this PR). Deferring the allocation tweak to a follow-up perf PR where it can be compile-verified in isolation.
There was a problem hiding this comment.
Applied in a3b207c. &[String] compiles and binds identically under sqlx (the tokio-postgres framing didn't apply, but the allocation removal is valid). Verified: clippy -D warnings clean + 67 tests green.
| FROM llm_kiro_accounts | ||
| WHERE account_name = ANY($1) | ||
| ORDER BY account_name", | ||
| &[&account_names.to_vec()], |
There was a problem hiding this comment.
Avoid calling .to_vec() on account_names when passing it to the SQL query. Since tokio-postgres supports slices directly, you can pass &account_names directly. This avoids allocating a temporary Vec<String> and cloning all of its string elements.
| &[&account_names.to_vec()], | |
| &[&account_names], |
There was a problem hiding this comment.
This crate binds parameters through sqlx (sqlx_core / sqlx_postgres), not tokio-postgres, so the "tokio-postgres supports slices directly" rationale doesn't apply here. Parameters are bound via the SqlxBindParam blanket impl, which requires T: Clone + Send + Sync + for<'q> Encode<'q, Postgres> + Type<Postgres>; whether the borrowed slice (&[String]) satisfies that in array position — versus the owned Vec<String> that .to_vec() produces — needs a compile check, not an assumption.
More to the point: this is pre-existing code moved verbatim in a behavior-preserving structural-split PR (the .to_vec() predates this PR). Deferring the allocation tweak to a follow-up perf PR where it can be compile-verified in isolation.
There was a problem hiding this comment.
Applied in a3b207c. &[String] compiles and binds identically under sqlx (the tokio-postgres framing didn't apply, but the allocation removal is valid). Verified: clippy -D warnings clean + 67 tests green.
| "SELECT account_name, balance_json::text, cache_json::text | ||
| FROM llm_kiro_status_cache | ||
| WHERE account_name = ANY($1)", | ||
| &[&account_names.to_vec()], |
There was a problem hiding this comment.
Avoid calling .to_vec() on account_names when passing it to the SQL query. Since tokio-postgres supports slices directly, you can pass &account_names directly. This avoids allocating a temporary Vec<String> and cloning all of its string elements.
| &[&account_names.to_vec()], | |
| &[&account_names], |
There was a problem hiding this comment.
This crate binds parameters through sqlx (sqlx_core / sqlx_postgres), not tokio-postgres, so the "tokio-postgres supports slices directly" rationale doesn't apply here. Parameters are bound via the SqlxBindParam blanket impl, which requires T: Clone + Send + Sync + for<'q> Encode<'q, Postgres> + Type<Postgres>; whether the borrowed slice (&[String]) satisfies that in array position — versus the owned Vec<String> that .to_vec() produces — needs a compile check, not an assumption.
More to the point: this is pre-existing code moved verbatim in a behavior-preserving structural-split PR (the .to_vec() predates this PR). Deferring the allocation tweak to a follow-up perf PR where it can be compile-verified in isolation.
There was a problem hiding this comment.
Applied in a3b207c. &[String] compiles and binds identically under sqlx (the tokio-postgres framing didn't apply, but the allocation removal is valid). Verified: clippy -D warnings clean + 67 tests green.
| async fn save_kiro_auth_update(&self, _update: ProviderKiroAuthUpdate) -> anyhow::Result<()> { | ||
| let Some(mut record) = self.get_kiro_account_row(&_update.account_name).await? else { | ||
| anyhow::bail!("kiro account `{}` is not configured", _update.account_name); | ||
| }; | ||
| record.auth_json = _update.auth_json.clone(); | ||
| record.auth_method = _update.auth_method.clone(); | ||
| record.account_id = _update.account_id.clone(); | ||
| record.profile_arn = _update.profile_arn.clone(); | ||
| record.user_id = _update.user_id.clone(); | ||
| record.status = _update.status.clone(); | ||
| record.last_refresh_at_ms = Some(_update.refreshed_at_ms); | ||
| record.last_error = _update.last_error.clone(); | ||
| record.updated_at_ms = _update.refreshed_at_ms; |
There was a problem hiding this comment.
The parameter _update is named with a leading underscore, which typically indicates an unused variable in Rust. Since this variable is heavily used throughout the function, the leading underscore should be removed to adhere to standard Rust naming conventions and avoid bypassing compiler warnings for unused variables. This also aligns with the naming of update in save_codex_auth_update.
| async fn save_kiro_auth_update(&self, _update: ProviderKiroAuthUpdate) -> anyhow::Result<()> { | |
| let Some(mut record) = self.get_kiro_account_row(&_update.account_name).await? else { | |
| anyhow::bail!("kiro account `{}` is not configured", _update.account_name); | |
| }; | |
| record.auth_json = _update.auth_json.clone(); | |
| record.auth_method = _update.auth_method.clone(); | |
| record.account_id = _update.account_id.clone(); | |
| record.profile_arn = _update.profile_arn.clone(); | |
| record.user_id = _update.user_id.clone(); | |
| record.status = _update.status.clone(); | |
| record.last_refresh_at_ms = Some(_update.refreshed_at_ms); | |
| record.last_error = _update.last_error.clone(); | |
| record.updated_at_ms = _update.refreshed_at_ms; | |
| async fn save_kiro_auth_update(&self, update: ProviderKiroAuthUpdate) -> anyhow::Result<()> { | |
| let Some(mut record) = self.get_kiro_account_row(&update.account_name).await? else { | |
| anyhow::bail!("kiro account '{}' is not configured", update.account_name); | |
| }; | |
| record.auth_json = update.auth_json.clone(); | |
| record.auth_method = update.auth_method.clone(); | |
| record.account_id = update.account_id.clone(); | |
| record.profile_arn = update.profile_arn.clone(); | |
| record.user_id = update.user_id.clone(); | |
| record.status = update.status.clone(); | |
| record.last_refresh_at_ms = Some(update.refreshed_at_ms); | |
| record.last_error = update.last_error.clone(); | |
| record.updated_at_ms = update.refreshed_at_ms; |
There was a problem hiding this comment.
Valid observation — the _update prefix conventionally signals an unused binding, yet the body uses it throughout, and the sibling save_codex_auth_update names it update. But this is pre-existing code moved verbatim in this PR (it reads _update in the pre-split source too). Renaming it here would break the byte-identical guarantee that makes this structural-move PR auditable via the item-inventory diff. Deferring the rename to the follow-up cleanup PR.
There was a problem hiding this comment.
Applied in a3b207c (renamed _update -> update).
Apply the PR review findings on the freshly split postgres submodules. All behavior-preserving (verified: clippy -D warnings + 67 tests green): - review: validate/fail/approve_sponsor drop the pre-SELECT and use a single UPDATE + `rows_affected == 0` existence check (3->2 round-trips). The pre-check was a bare is_none() with no state guard and the UPDATE keys only on request_id, so the result is identical. reject_* keeps its SELECT (it needs issued_key_id / imported_account_name for cascades). - keys: disable_admin_key_if_present calls patch_admin_key directly; the latter already performs the presence check and is a no-op (Ok(None), no cache/dispatch side effects) on a missing key. - keys/routes: pass &[String] to ANY($1) instead of cloning via .to_vec() (sqlx encodes the borrowed slice identically). - cache: early-return invalidate_*_by_ids / _for_provider on empty input. - routes: rename misleading `_update` param to `update`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What
Second and final PR splitting the ~9.4k-line
llm-access-store/src/postgres.rs(PR-A #14 extracted the pure free-fn leaf layer). This PR-B domain-splits the statefulPostgresControlRepositoryGod-object: 94 of its 103 inherent methods + 14 of the 16 trait impls move into 12 domain submodules.mod.rs: 9361 → 1648 lines.New submodules (
postgres/)cacheconfigAdminConfigStorekeysAdminKeyStoregroupsAdminAccountGroupStoreproxyAdminProxyStorecodex_accountAdminCodexAccountStorekiro_accountAdminKiroAccountStoreroutesProviderRouteStoreusageUsageEventSinkpublicreviewAdminReviewQueueStorestatusPublicStatusStoreWhat stays in mod.rs
Infra hubs called by every domain —
connect/connect_with_proxy_scope/connect_fresh_client,ensure_connection_alive, the proxy-metadata/dispatch generation counters,count_rows— plus theControlStoreimpl, all types, the sqlx wrapper (PgRow/SqlxClient/SqlxTransaction), and the usage-rollup struct+fn. Rust descendants read ancestor-privates for free, so keeping the hubs here means submodules call them with zero visibility bumps.Visibility
A moved private method becomes
pub(super)only where a foreign region (another submodule / mod-remaining / tests) calls it — 49 of 94; the other 44 stay private (only intra-submodule callers).pub(super)is the minimal non-leaking visibility: it preserves the exact "private within thepostgresmodule" boundary.pubwould expand the crate's public API (the type is public), andpub(crate)would over-widen — both rejected. The 2 already-pubmoved methods keeppub. Cross-domain trait-method calls (incl. provided/default methods defined inllm-access-core) import the owning trait.Behavior-preserving
Structural move only — method/fn bodies byte-identical.
#[test]/#[tokio::test].Verification
cargo clippy -p llm-access-store --all-targets -- -D warnings→ cleancargo test -p llm-access-store→ 67 passedcargo build -p llm-access(reverse dep) → okrustfmton the 13 changed files only;deps/lance/deps/lancedbuntouched🤖 Generated with Claude Code