Skip to content

fix(admin): stop listing a pooled account twice on the observed surface - #581

Open
r-uben wants to merge 1 commit into
pleaseai:mainfrom
r-uben:fix/observed-dedup-clean
Open

r-uben wants to merge 1 commit into
pleaseai:mainfrom
r-uben:fix/observed-dedup-clean

Conversation

@r-uben

@r-uben r-uben commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

An account that the pool already resolves also shows up as a separate observed
row, so a machine with two ChatGPT accounts renders three ChatGPT entries on the
admin surface. discover() is machine-level credential discovery with no pool
awareness, so it re-reports credentials the pool already owns.

observed_accounts now resolves the pool's stable upstream identities
(shuntAccountUuid for Claude, chatgpt_account_id for ChatGPT/Codex) per store
family and drops the observed copy when it matches. The pool row is the one kept:
it carries live quota, priority and cooldown that the observed row cannot.

Conservative in the safe direction throughout — hiding a row is the destructive
outcome, so a credential is only suppressed on a confident match:

  • Only the three pooled families can match. Grok, Gemini and Cursor have no pool
    representation, so their observed rows are the only view of them and always survive.
  • A credential with no resolvable identity is kept.
  • Identities are keyed by store family, so two families cannot collide on a shared
    id string.
  • A provider whose scope cannot be resolved logs at debug and contributes no
    identities, leaving its credentials visible rather than failing the endpoint.
    This is a display concern.

Test plan

  • New unit test observed_row_is_suppressed_only_for_its_own_pooled_identity
    covers own-family suppression for Codex and Claude, cross-family non-collision,
    an unpooled second account surviving, Grok always surviving, and None/blank ids.
  • cargo fmt --all --check, cargo clippy --all-targets -- -D warnings clean.
  • cargo test --lib admin (96 passed) and cargo test --test admin_surface
    (48 passed) green on top of current main.
  • Verified live against a local gateway with two ChatGPT accounts: three rows
    before, two after.

Summary by cubic

Pooled accounts were appearing twice on the admin observed surface; they now show only as the pool row, which carries live quota, priority, and cooldown.

  • De-duplicates by stable upstream identity per store family, so an id pooled under Claude cannot hide a ChatGPT credential.
  • Only Claude, ChatGPT/Codex, and Kimi can be matched. Grok, Gemini, and Cursor have no pool representation and always remain visible.
  • Credentials with no resolvable identity are kept, and identity resolution failures log at debug instead of failing the endpoint.

Written for commit cfbac99. Summary will update on new commits.

`GET /admin/api/observed` reports every provider credential it finds on the
gateway host, with no knowledge of the account pool. A login that is *also* a
pooled account therefore appeared twice: once as its pool entry, carrying live
quota and cooldown, and again as a bare observation of the same subscription.
With two ChatGPT accounts pooled, the dashboard showed three rows.

The SPA already coalesces this case, but only for `claude_oauth`
(`ui/src/accounts.ts`): it matches on a uuid that the Claude observed row
carries and the ChatGPT one does not, and `/admin/api/pool` exposes no uuids at
all, so no client can do the same for Codex.

Resolve it at the source instead, where both halves are already in hand: drop a
discovered credential whose stable upstream identity is one the pool resolves.
The pool row is strictly the more informative of the two.

Suppression is keyed by store family *and* identity, so an id pooled under
Claude cannot hide a ChatGPT credential that happens to share the string. Only
the three pooled families can match at all; Grok, Gemini and Cursor have no pool
representation, so their observed rows are the only view of those providers and
always survive. A credential whose identity cannot be established is kept too --
an unlabelled row is a smaller problem than a silently hidden account -- and a
provider whose scope fails to resolve contributes no identities, so its
credentials stay visible rather than disappearing behind a read error.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces de-duplication logic to the observed_accounts admin endpoint by filtering out discovered credentials that are already present in the resolved accounts pool. It adds helper functions pooled_identities and is_already_pooled along with corresponding unit tests. Feedback on these changes suggests resolving the pool accounts concurrently using futures_util::future::join_all to avoid sequential backend latency bottlenecks.

Comment thread src/admin/mod.rs
Comment on lines +814 to +871
async fn pooled_identities(state: &AppState) -> HashSet<(crate::accounts::StoreFamily, String)> {
let mut pooled = HashSet::new();
for (name, provider) in &state.config.providers {
let (label, family, dir, scan): (
_,
_,
_,
fn() -> std::io::Result<Vec<crate::config::AccountConfig>>,
) = match provider.auth {
AuthMode::ClaudeOauth => (
"Claude",
crate::accounts::StoreFamily::Claude,
claude_store::default_accounts_dir(),
claude_store::scan_accounts,
),
AuthMode::ChatgptOauth => (
"codex",
crate::accounts::StoreFamily::Chatgpt,
crate::auth::codex::store::default_accounts_dir(),
crate::auth::codex::store::scan_accounts,
),
AuthMode::KimiOauth => (
"Kimi",
crate::accounts::StoreFamily::Kimi,
crate::auth::kimi::store::default_accounts_dir(),
crate::auth::kimi::store::scan_accounts,
),
_ => continue,
};
match crate::auth::shared::resolve_pool_accounts(
label,
&provider.accounts,
&provider.account_scope,
family,
dir,
scan,
)
.await
{
Ok(resolved) => pooled.extend(
resolved
.iter()
.filter_map(|account| account.uuid.as_deref())
.map(str::trim)
.filter(|uuid| !uuid.is_empty())
.map(|uuid| (family, uuid.to_string())),
),
Err(error) => {
tracing::debug!(
provider = %name,
%error,
"admin: could not resolve pool identities for observed-row de-duplication"
);
}
}
}
pooled
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[MEDIUM] Resolve pool accounts concurrently to reduce latency

Problem: The pooled_identities function sequentially awaits resolve_pool_accounts for each configured provider in a loop. If there are multiple providers, this sequential execution can unnecessarily increase the latency of the observed_accounts admin API endpoint.

Rationale: Section 6 (Performance — Backend) of the Org Style Guide emphasizes avoiding backend latency bottlenecks. Sequential asynchronous operations that can be run in parallel should be executed concurrently.

Suggestion: Use futures_util::future::join_all to resolve the pool accounts for all providers concurrently.

async fn pooled_identities(state: &AppState) -> HashSet<(crate::accounts::StoreFamily, String)> {
    let futures = state.config.providers.iter().map(|(name, provider)| async move {
        let (label, family, dir, scan): (
            _,
            _,
            _,
            fn() -> std::io::Result<Vec<crate::config::AccountConfig>>,
        ) = match provider.auth {
            AuthMode::ClaudeOauth => (
                "Claude",
                crate::accounts::StoreFamily::Claude,
                claude_store::default_accounts_dir(),
                claude_store::scan_accounts,
            ),
            AuthMode::ChatgptOauth => (
                "codex",
                crate::accounts::StoreFamily::Chatgpt,
                crate::auth::codex::store::default_accounts_dir(),
                crate::auth::codex::store::scan_accounts,
            ),
            AuthMode::KimiOauth => (
                "Kimi",
                crate::accounts::StoreFamily::Kimi,
                crate::auth::kimi::store::default_accounts_dir(),
                crate::auth::kimi::store::scan_accounts,
            ),
            _ => return None,
        };
        match crate::auth::shared::resolve_pool_accounts(
            label,
            &provider.accounts,
            &provider.account_scope,
            family,
            dir,
            scan,
        )
        .await
        {
            Ok(resolved) => {
                let identities: Vec<_> = resolved
                    .iter()
                    .filter_map(|account| account.uuid.as_deref())
                    .map(str::trim)
                    .filter(|uuid| !uuid.is_empty())
                    .map(|uuid| (family, uuid.to_string()))
                    .collect();
                Some(identities)
            }
            Err(error) => {
                tracing::debug!(
                    provider = %name,
                    %error,
                    "admin: could not resolve pool identities for observed-row de-duplication"
                );
                None
            }
        }
    });

    let results = futures_util::future::join_all(futures).await;
    let mut pooled = HashSet::new();
    for res in results {
        if let Some(identities) = res {
            pooled.extend(identities);
        }
    }
    pooled
}
References
  1. Section 6 (Performance — Backend) of the Org Style Guide emphasizes avoiding backend latency bottlenecks. (link)
  2. Poll independent network requests concurrently (e.g., using join_all) to prevent a slow or hanging provider from delaying others.

@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

This PR is not safe to merge until matched Claude accounts retain their observed quota and status data while being displayed as a single row.

Fix All in Claude CodeFindings

  1. P1 Matched quota data is lost
Fix with agent prompt
### Issue 1
src/admin/mod.rs:798
When a Claude credential matches a pooled account, this filter removes it before `build_claude_observed_row` can fetch its local-client quota. The dashboard intentionally folds those observed quota windows into the managed row because the pool can have missing or stale windows until it receives relevant response headers. As a result, the account is shown only once, but its observed quota and status signal is lost. Preserve the observation data when coalescing the rows.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR resolves stable identities for managed OAuth accounts and filters matching machine-discovered credentials from the observed-account endpoint.

  • Keys identities by store family to prevent cross-provider collisions.
  • Keeps observations whose identity is absent or cannot be resolved.
  • Adds focused matching tests for same-family, cross-family, blank, and unsupported-provider cases.
  • Currently discards Claude observation data that the dashboard needs to enrich its managed row.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  D[Discover local credential] --> M{Identity matches pool?}
  M -->|No| B[Build observed row and fetch quota]
  M -->|Yes| X[Discard observation]
  P[Pool endpoint] --> U[Managed dashboard row]
  B --> F[Fold observed quota into managed row]
  F --> U
  X -. Missing observed quota .-> U
Loading

Reviews (1) · Last reviewed commit: "fix(admin): stop listing a pooled accoun..."

Comment thread src/admin/mod.rs
let rows = futures_util::future::join_all(
discovered
.into_iter()
.filter(|observed| !is_already_pooled(observed, &pooled))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Matched quota data is lost

When a Claude credential matches a pooled account, this filter removes it before build_claude_observed_row can fetch its local-client quota. The dashboard intentionally folds those observed quota windows into the managed row because the pool can have missing or stale windows until it receives relevant response headers. As a result, the account is shown only once, but its observed quota and status signal is lost. Preserve the observation data when coalescing the rows.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/admin/mod.rs
Line: 798

Comment:
**Matched quota data is lost**

When a Claude credential matches a pooled account, this filter removes it before `build_claude_observed_row` can fetch its local-client quota. The dashboard intentionally folds those observed quota windows into the managed row because the pool can have missing or stale windows until it receives relevant response headers. As a result, the account is shown only once, but its observed quota and status signal is lost. Preserve the observation data when coalescing the rows.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.91667% with 50 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/admin/mod.rs 47.91% 50 Missing ⚠️

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 101 untouched benchmarks


Comparing r-uben:fix/observed-dedup-clean (cfbac99) with main (6e7ccb1)

Open in CodSpeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant