Skip to content

Enforce at most one managed source globally#719

Merged
rdimitrov merged 7 commits intomainfrom
rdimitrov/enforce-managed-source-limit
Apr 20, 2026
Merged

Enforce at most one managed source globally#719
rdimitrov merged 7 commits intomainfrom
rdimitrov/enforce-managed-source-limit

Conversation

@rdimitrov
Copy link
Copy Markdown
Member

Summary

  • Enforce the global limit of at most one managed source across the entire deployment, covering both the API and config-load paths
  • Guard CreateSource inside its serializable transaction: query GetManagedSources and return HTTP 409 when one already exists
  • Guard Initialize (config reload): reject startup when config introduces a managed source while an API-created one exists in the DB
  • Handle PostgreSQL serialization failures (40001) from concurrent races as ErrManagedSourceLimitReached instead of a vague 500
  • Strip existing managed source name from the client-facing error to prevent information leakage (logged server-side instead)
  • Add Swagger @Failure 409 annotation to the upsert source handler
  • Update existing tests and smoke-test skill to account for the new limit
  • Fix smoke-test compose healthcheck to hit the internal server port (aligned with Add database password support via config fields and env vars #716)

Test plan

  • task build passes
  • task lint-fix — 0 issues
  • Unit tests: go test ./internal/api/v1/ — all pass
  • Integration tests: go test ./internal/service/db/ — all pass (including new TestCreateSource_ManagedLimitReached, TestCreateSource_NonManagedAllowedWhenManagedExists, TestInitialize_RejectsConfigManagedWhenAPIManagedExists)
  • Smoke tests via docker compose — 23/23 pass, including managed source limit scenario
  • End-to-end config reload test: create API-managed source, restart with config containing managed source → sync coordinator rejects with clear error

Fixes #718

🤖 Generated with Claude Code

The config file validation already limited managed sources to one, but
the API creation path did not enforce this invariant. Add a check inside
the `CreateSource` serializable transaction that queries
`GetManagedSources` and rejects the request with HTTP 409 when a managed
source already exists (regardless of creation type). Handle PostgreSQL
serialization failures (`40001`) from concurrent races as the same error.

Guard the config-load path as well: `Initialize` now calls
`checkManagedSourceLimit` before upserting CONFIG sources, rejecting
startup when the config introduces a managed source while an API-created
one already exists in the database.

Strip the existing managed source name from the client-facing error
message to avoid information leakage; the name is logged server-side
instead.

Update existing tests that created multiple managed sources to use
file-data sources, and add new integration and API-layer tests for the
limit. Update the smoke-test skill to cover the 409 scenario and use
file-data sources for list-completeness checks.

Fixes #718

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rdimitrov
Copy link
Copy Markdown
Member Author

Implementation Plan

Problem Statement

The config file validation enforces that at most one managed source can exist (internal/config/config.go:838), but the API creation path (CreateSource in internal/service/db/impl_source.go) did not enforce this limit. An API caller could create additional managed sources after startup, bypassing the invariant. Additionally, the config-load path did not check for existing API-created managed sources, so a config reload could also produce a second managed source.

Design Decisions

  1. Reuse the existing GetManagedSources SQL query rather than adding a new one. It returns all rows where source_type = 'managed' regardless of creation_type, which is exactly what we need. The table is small (admin metadata), so overhead is negligible.

  2. Place the API check inside the existing serializable transaction in CreateSource. Since the transaction uses pgx.Serializable isolation, this provides correct serialization against concurrent create requests — PostgreSQL will detect the read-write conflict and abort one with a serialization failure.

  3. Guard the config-load path in Initialize (internal/sync/state/db.go). Before BulkUpsertConfigSources runs, check if the config includes a managed source and an API-created managed source already exists in the DB. Reject initialization if both would coexist.

  4. Map the error to HTTP 409 Conflict, consistent with how ErrSourceInUse and ErrSourceAlreadyExists are handled.

  5. Only guard CreateSource, not UpdateSource. The update path already blocks type changes via validateSourceTypeChange, so a non-managed source cannot be changed to managed.

  6. Strip source name from client-facing error to avoid information leakage. Log server-side via slog.WarnContext instead.

  7. Handle PostgreSQL serialization failures (40001) at both insert and commit time, mapping them to ErrManagedSourceLimitReached for managed sources instead of surfacing a vague 500.

Changes by File

internal/service/service.go

  • Added ErrManagedSourceLimitReached sentinel error after ErrNoManagedSource

internal/service/db/impl_source.go

  • After the "source already exists" check and before "Prepare insert parameters", added a managed source limit check: if req.GetSourceType() == config.SourceTypeManaged, query GetManagedSources and reject if any exist. Logs the existing source name server-side, returns only the sentinel error to the client.
  • At insert time: restructured the pgconn.PgError handling into a switch statement to catch both 23505 (unique violation) and 40001 (serialization failure). For 40001 on managed sources, returns ErrManagedSourceLimitReached.
  • At commit time: added 40001 handling for the same race condition scenario.

internal/api/v1/sources.go

  • Added ErrManagedSourceLimitReached → HTTP 409 mapping in writeSourceError
  • Added @Failure 409 Swagger annotation to the upsertSource handler

internal/sync/state/db.go

  • Added checkManagedSourceLimit function: scans config sources for a managed type, queries GetManagedSources, rejects if any API-created managed source exists
  • Called from Initialize after checkForAPIRegistryConflicts and before BulkUpsertConfigSources

internal/service/db/admin_claims_test.go

  • Added fileDataSourceReq helper (file source with inline data + claims) for tests needing multiple sources
  • Changed createSourceForRegistry and multi-source claim-filtering tests (TestListSources_FiltersByClaims, TestListSources_ReturnsAllMatchingWithMixedClaims, TestListSources_AnonymousReturnsAll) from managedSourceReq to fileDataSourceReq
  • Added TestCreateSource_ManagedLimitReached — creates first managed source (201), attempts second (expects ErrManagedSourceLimitReached)
  • Added TestInitialize_RejectsConfigManagedWhenAPIManagedExists — creates API-managed source, then calls Initialize with config containing a managed source (expects error containing "API-created managed source")
  • Added TestCreateSource_NonManagedAllowedWhenManagedExists — creates managed source, then file-data source (expects success)

internal/api/v1/routes_test.go

  • Added TestUpsertSourceManagedLimitReached — mocks CreateSource returning ErrManagedSourceLimitReached, verifies HTTP 409 response

.claude/skills/smoke-test/SKILL.md

  • Added "Managed source limit" Gherkin scenario and corresponding bash test (409 on second managed source)
  • Changed "List completeness" section from managed sources to file-data sources

docker-compose.smoke-test.yaml

examples/config-docker-with-managed.yaml (new)

  • Test config file with a managed source, used for end-to-end config reload testing

Race Condition Safety

The CreateSource method uses pgx.Serializable isolation. Under PostgreSQL SSI, if two concurrent transactions both read zero managed sources and both attempt to insert one, PostgreSQL detects the serialization anomaly and aborts one with error code 40001. The code explicitly catches this at both insert and commit time, mapping it to ErrManagedSourceLimitReached (HTTP 409) instead of a generic 500.

Why No SQL or sqlc Changes Are Needed

The existing GetManagedSources query (SELECT ... FROM source WHERE source_type = 'managed') returns all managed sources regardless of creation_type. It is already available on the Querier interface. No new query or sqlc regeneration is required.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 22.22222% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.66%. Comparing base (faf30bc) to head (a652118).

Files with missing lines Patch % Lines
internal/sync/state/db.go 0.00% 18 Missing ⚠️
internal/service/db/impl_source.go 32.00% 16 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #719      +/-   ##
==========================================
- Coverage   60.73%   60.66%   -0.08%     
==========================================
  Files         107      107              
  Lines       10404    10445      +41     
==========================================
+ Hits         6319     6336      +17     
- Misses       3539     3561      +22     
- Partials      546      548       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rdimitrov and others added 6 commits April 14, 2026 16:36
The `TestAuthzIntegration_SourceCRUD` test creates a managed source via
the API while the base config already provides one ("internal"). Switch
the test to use file-data sources since it tests CRUD authorization, not
managed source behavior.

Fixes CI failure introduced by the managed source limit enforcement.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Run `task docs` to update the generated OpenAPI spec after adding the
`@Failure 409` annotation to the upsert source handler.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `examples/config-docker-with-managed.yaml` example was added with a
`format: upstream` field that is being removed by #724 (drop ToolHive
registry format support). Drop the field here to avoid a merge conflict
with that branch — regardless of merge order.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Grype vulnerability DB added `GHSA-pc3f-x583-g7j2` (High) against
v0.5.0 between this PR's previous CI run and now, which broke the
Security Scan step. v0.5.1 is the fix. Pulled in transitively via
`k8s.io/client-go`; no direct callers in this repo.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@rdimitrov rdimitrov merged commit c3c5e77 into main Apr 20, 2026
15 checks passed
@rdimitrov rdimitrov deleted the rdimitrov/enforce-managed-source-limit branch April 20, 2026 11:59
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.

Enforce at most one managed source globally via API

3 participants