perf(credshare): list a team's vaults in one query instead of two per vault - #128
Merged
Conversation
… vault GET /api/credshare/teams/:slug/vaults built its response in a loop, asking the database for a grant row and a secret count once per vault. libSQL is remote, so each of those is a network round trip, and the endpoint cost 2N+1 of them. On a team with 176 vaults that is 353 round trips and ~10.6s of server time. `logicsrc teams pull` resolves the vault id twice -- once planning the sync, once reading values -- so a pull of a single ten-key vault took ~24s, nearly all of it spent listing vaults the command does not want. Replaced with one SELECT carrying two correlated subqueries. Both are covered by existing primary keys (credshare_secrets is keyed (vault_id, name), credshare_vault_grants (vault_id, user_id)), so the per-vault work becomes an index probe inside the database instead of a round trip across the network. No schema or index change. Measured on a local libSQL seeded to match that team -- 176 vaults, 17 secrets each -- the endpoint goes from 355 round trips to 3, and returns identical rows. The response shape is unchanged: hasAccess is still a real boolean rather than the 0/1 SQLite hands back, and secretCount is still a number. Tests pin behaviour and cost separately. The behavioural cases pass against both the old loop and the new query, which is the point -- only the round-trip count changed. The regression guard asserts the query count for 3 vaults EQUALS the count for 30 rather than matching a magic number, so any future rewrite that reintroduces per-vault I/O fails no matter what the constant part costs. Against the old loop it reports 9 vs 63. Two sibling endpoints have the same shape -- /teams/:slug/members and /vaults/:id/grants both call publicKeyFor() per member. Neither is on the pull path and both scale with member count rather than vault count, so they are left alone here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ThreatCrush Security Scan55 finding(s) HIGH/CRITICAL: 30 | MEDIUM: 25
…and 5 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
marked this pull request as ready for review
August 4, 2026 05:20
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
1 participant
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.
logicsrc teams pulltook ~24 seconds to fetch a ten-key vault. This is where it went.Why
GET /api/credshare/teams/:slug/vaultsbuilt its response in a loop:libSQL is remote, so each
get()is a network round trip, not a local read — the endpoint cost2N+1of them.Measured against production on a team with 176 vaults:
/api/credshare/me/api/credshare/teams/api/credshare/teams/:slug/vaultsconnectwas 33ms in every case, so this is entirely server-side. Andteams pullcalls the endpoint twice — once increateCredentialSyncPlan→inspect, once inrunCredentialSync→readValues, each going throughresolveVaultId— which is the ~24s.Nearly all of it is spent listing 175 vaults the command has no interest in, to resolve one name to one id.
What
One
SELECTwith two correlated subqueries. Both are covered by primary keys that already exist —credshare_secretsis keyed(vault_id, name)andcredshare_vault_grants(vault_id, user_id)— so the per-vault work becomes an index probe inside the database rather than a round trip across the network. No schema change, no new index.Seeded a local libSQL to match that team (176 vaults × 17 secrets):
I chose correlated subqueries over
LEFT JOIN … GROUP BYdeliberately: joining both children multiplies rows, andCOUNT(s.id)then silently reports the wrong number if a vault ever matches more than one grant row. The subquery form cannot express that bug. There's a test for it.The response shape is unchanged —
hasAccessis still a real boolean rather than the0/1SQLite returns, andsecretCountstill a number.Tests
Five, splitting behaviour from cost.
The four behavioural cases (per-vault counts, grant/no-grant, non-member 403, unknown-team 404) pass against both the old loop and the new query. That's intentional — it's the evidence that only the round-trip count changed.
The guard asserts the query count for 3 vaults equals the count for 30, rather than matching a magic number. Any future rewrite that reintroduces per-vault I/O fails it regardless of what the constant part costs. Against the old loop it reports
3 vaults took 9 queries, 30 took 63— exactly2N+3.Full
apps/pwasuite: 26 pass, 0 fail.Not fixed here
/teams/:slug/membersand/vaults/:id/grantshave the same shape —publicKeyFor()once per member. Neither is on the pull path, and both scale with member count rather than vault count, so they aren't causing user-visible pain yet. Worth a follow-up.This also doesn't address the client fetching all 176 vaults twice to resolve one name. That's now ~0.1s a call instead of ~10.6s, so it stopped mattering — but a
?name=filter on the endpoint would makeresolveVaultIdO(1) in vault count.🤖 Generated with Claude Code