fix(api): allocate unnamed provider-key display names atomically

[WhoamiI00]

Fixes #6015

Summary

VaultService names an unnamed provider_key connection after its provider — "OpenAI", then "OpenAI 2" — by reading the names already taken in the project. That read and the insert ran in separate DAO sessions, and header.name has no uniqueness constraint, so two concurrent unnamed creates for the same provider family both observed the same taken_names and both persisted "OpenAI".

Approach

Of the two shapes suggested in the issue, this is the advisory lock rather than the partial unique index — it needs no migration, so there is no index to backfill on existing projects and nothing to roll back.

SecretsDAO.create_with_derived_naming takes the lock, lists the scope's secrets, and inserts, all in one transaction:

await session.execute(
    text("SELECT pg_advisory_xact_lock(hashtextextended(:scope, 0))"),
    {"scope": lock_scope},
)

On the seam concern in the issue — the naming policy does not move. VaultService still owns next_provider_key_name and the slug derivation, and hands them to the DAO as a callback; only the transaction boundary moves. That mirrors SessionAttachmentsDAO, where quota enforcement lives in the DAO for the same reason ("Quota enforcement lives here because the session advisory lock lives here") — a transaction-scoped lock can only live where the session does.

_name_and_slug_provider_key is now synchronous and takes the already-read secrets, since it no longer does its own I/O.

Tests

api/oss/tests/pytest/unit/vault/test_provider_key_name_allocation.py, marked integration and skipped when Postgres is unreachable, matching the session/git DAO suites.

• test_concurrent_unnamed_creates_get_distinct_names — the concurrent-create test the issue asks for. Two create_secret calls against one project, bounded by asyncio.wait_for so a regression fails the run instead of hanging it. On main it fails with exactly the reported symptom:

  AssertionError: ['OpenAI', 'OpenAI'] == ['OpenAI', 'OpenAI 2']
  

• test_sequential_unnamed_creates_still_increment — passes with and without the change, so it pins that the lock does not alter ordinary one-at-a-time naming.

Run against a real Postgres: oss/tests/pytest/unit/vault 5 passed. oss/tests/pytest/unit/sessions has 4 pre-existing failures in test_records_turn_span_dao.py that reproduce identically on a clean main checkout, so they are unrelated. ruff format and ruff check clean.

[vercel]

@WhoamiI00 is attempting to deploy a commit to the agenta projects Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 554daf86-e2a2-4733-b045-a3aa6ec186eb

📥 Commits

Reviewing files that changed from the base of the PR and between a09a0f0 and e51af29.

⛔ Files ignored due to path filters (1)

• .github/pr-assets/6015-provider-key-name-race.png is excluded by !**/*.png

📒 Files selected for processing (6)

• api/oss/src/core/secrets/interfaces.py
• api/oss/src/core/secrets/services.py
• api/oss/src/dbs/postgres/secrets/dao.py
• api/oss/tests/pytest/unit/secrets/test_services.py
• api/oss/tests/pytest/unit/vault/conftest.py
• api/oss/tests/pytest/unit/vault/test_provider_key_name_allocation.py

🚧 Files skipped from review as they are similar to previous changes (6)

• api/oss/tests/pytest/unit/secrets/test_services.py
• api/oss/src/dbs/postgres/secrets/dao.py
• api/oss/tests/pytest/unit/vault/test_provider_key_name_allocation.py
• api/oss/src/core/secrets/services.py
• api/oss/src/core/secrets/interfaces.py
• api/oss/tests/pytest/unit/vault/conftest.py

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Unnamed provider keys now receive automatically generated names and unique slugs.
  • Generated names follow a clear sequence, such as “OpenAI” and “OpenAI 2.”
  • Unnamed managed credentials retain their management settings while receiving a generated name.

• Bug Fixes

  • Improved concurrent provider-key creation to prevent duplicate names or slugs.
  • Preserved existing behavior for provider keys with explicitly supplied names.

• Tests

  • Added coverage for sequential and concurrent unnamed provider-key creation scenarios.

Walkthrough

Unnamed provider-key creation now derives names and slugs inside a PostgreSQL advisory-locked transaction. The DAO interface and service use a callback-based naming flow. Tests cover managed-secret metadata and concurrent and sequential allocation.

Changes

Provider-key name allocation

Layer / File(s)	Summary
Naming contract and service integration api/oss/src/core/secrets/interfaces.py, api/oss/src/core/secrets/services.py, api/oss/tests/pytest/unit/secrets/test_services.py	The DAO interface adds callback-based derived naming. Unnamed provider keys use synchronous naming with scoped secrets supplied by the DAO. Managed secrets retain their management metadata.
Locked transactional creation api/oss/src/dbs/postgres/secrets/dao.py	The PostgreSQL DAO validates scope, acquires the provider-scope advisory lock, loads existing secrets, derives the name, persists the secret, and commits in one transaction.
Concurrent and sequential validation api/oss/tests/pytest/unit/vault/*	Integration tests configure database availability, provision required entities, and verify concurrent distinct names and sequential incremental names.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to e51af

The change makes concurrent unnamed provider-key creation allocate distinct display names atomically; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant VaultService
  participant SecretsDAO
  participant PostgreSQL
  VaultService->>SecretsDAO: create_with_derived_naming
  SecretsDAO->>PostgreSQL: acquire provider-scope advisory lock
  SecretsDAO->>PostgreSQL: load scoped secrets
  SecretsDAO->>SecretsDAO: derive name and slug
  SecretsDAO->>PostgreSQL: persist and commit secret
  SecretsDAO-->>VaultService: return SecretResponseDTO

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.82% which is insufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: atomic allocation of unnamed provider-key display names.
Description check	✅ Passed	The description explains the race condition, advisory-lock implementation, naming behavior, and tests. It is directly related to the changeset.
Linked Issues check	✅ Passed	The implementation satisfies issue #6015 by serializing scoped name allocation and creation with a PostgreSQL transaction-scoped advisory lock. The concurrent integration test verifies distinct names,…
Out of Scope Changes check	✅ Passed	The changes remain within scope. The interface, DAO, service, test support, and integration tests directly support atomic provider-key name allocation and its verification.

Full details: Linked Issues check

Explanation

The implementation satisfies issue #6015 by serializing scoped name allocation and creation with a PostgreSQL transaction-scoped advisory lock. The concurrent integration test verifies distinct names, and sequential allocation remains covered.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: a7da49f8-926f-41b2-a31f-3f28ebf603e0

📥 Commits

Reviewing files that changed from the base of the PR and between 753af38 and fec5a92.

⛔ Files ignored due to path filters (1)

• .github/pr-assets/6015-provider-key-name-race.png is excluded by !**/*.png

📒 Files selected for processing (5)

• api/oss/src/core/secrets/interfaces.py
• api/oss/src/core/secrets/services.py
• api/oss/src/dbs/postgres/secrets/dao.py
• api/oss/tests/pytest/unit/vault/conftest.py
• api/oss/tests/pytest/unit/vault/test_provider_key_name_allocation.py

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[WhoamiI00]

Rebased onto main — it had gone conflicting against the managed-secrets and keep-on-omit work that landed in this module since. Two things came out of the merge that are worth calling out, because neither was a mechanical conflict:

1. management now has to travel down the derived-naming path. _create_secret grew a management argument, and my branch returned early from the unnamed-provider-key branch before reaching it. A managed connection created without a name would have been written with management=None — a manager-owned credential silently becoming an ordinary editable one. create_with_derived_naming now takes management and forwards it to map_secrets_dto_to_dbe, and there is a test for exactly that combination (test_managed_create_keeps_its_management_while_deriving_a_name); removing the forwarding fails it.

2. The early return also skipped the new invalidate_cache. Both paths now share the tail of _create_secret, so the project cache is invalidated the same way regardless of who picked the name.

Also worth noting: update independently grew a resolve_update callback so the read and the write happen under one lock — the same shape as derive_naming here. Happy to rename mine to match if you'd prefer the symmetry.

Verified on the rebased branch:

• pytest oss/tests/pytest/unit — 2672 passed, 75 skipped
• the two concurrency tests against a real Postgres (advisory locks are a server feature, so they self-skip without one) — both pass:

test_concurrent_unnamed_creates_get_distinct_names PASSED
test_sequential_unnamed_creates_still_increment PASSED

The earlier run-api-unit-tests failure was mine — the in-memory DAO in unit/secrets/test_services.py had not been taught the new create. Fixed. The remaining red checks are build-image and Vercel, which fail with denied: installation not allowed to Write organization package on fork PRs.