60c801c0c2## Summary Fixes race conditions in concurrent server publishing that could result in data integrity violations: - Multiple versions marked as `isLatest=true` for the same server - Duplicate (serverId, version) combinations - Same server name assigned different serverIDs ## Solution Uses two complementary approaches for correctness: ### 1. PostgreSQL Advisory Locks - Serializes concurrent publishes per server name using `pg_advisory_xact_lock` - Uses FNV-1a hash to convert server name to lock ID - Transaction-scoped locks auto-release on commit/rollback - **Required after #465**: The serverId introduction means concurrent publishes must atomically coordinate whether to reuse an existing serverId (for additional versions) or generate a new one (for first version). Without locks, race conditions would assign different serverIDs to versions of the same server. ### 2. Unique Constraints (Migration 007) Two new indexes provide data integrity guarantees: - **idx_unique_server_version**: Ensures (serverId, version) is unique across all versions - **idx_unique_latest_version**: Partial index ensuring only one version per serverId has `isLatest=true` These constraints serve as defense-in-depth safety nets. Even if lock logic had a bug, the database would reject integrity violations. Also drops redundant `idx_servers_version_id` since `version_id` is already the primary key. ## Why WithPublishLock validation is needed after #465 The `ctx.Err()` check at the start of `WithPublishLock` is critical because: 1. After #465 introduced serverId, the publish flow must atomically check existing versions and assign/reuse serverId 2. If the context is cancelled during lock acquisition, we could deadlock 3. Checking ctx validity before acquiring locks prevents these deadlocks ## Testing Added `TestPublishConcurrentVersionsNoRace` with 100 concurrent goroutines publishing different versions of the same server. All succeed with: - Exactly one version marked as latest - All versions sharing the same serverId - No duplicate (serverId, version) combinations NB: this only covers the in memory db, not the postgres db. It'd be nice to write tests for this one as well, but our current setup is a bit annoying for this. Separately looking into ways to easily run all our tests with real postgres and ideally scrap memory.go completely.
430 lines
11 KiB
Go
430 lines
11 KiB
Go
//nolint:testpackage
|
|
package service
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"sync"
|
|
"testing"
|
|
|
|
"github.com/modelcontextprotocol/registry/internal/config"
|
|
"github.com/modelcontextprotocol/registry/internal/database"
|
|
apiv0 "github.com/modelcontextprotocol/registry/pkg/api/v0"
|
|
"github.com/modelcontextprotocol/registry/pkg/model"
|
|
"github.com/stretchr/testify/assert"
|
|
)
|
|
|
|
func TestValidateNoDuplicateRemoteURLs(t *testing.T) {
|
|
// Create test data
|
|
existingServers := map[string]*apiv0.ServerJSON{
|
|
"existing1": {
|
|
Name: "com.example/existing-server",
|
|
Description: "An existing server",
|
|
Version: "1.0.0",
|
|
Remotes: []model.Transport{
|
|
{Type: "streamable-http", URL: "https://api.example.com/mcp"},
|
|
{Type: "sse", URL: "https://webhook.example.com/sse"},
|
|
},
|
|
},
|
|
"existing2": {
|
|
Name: "com.microsoft/another-server",
|
|
Description: "Another existing server",
|
|
Version: "1.0.0",
|
|
Remotes: []model.Transport{
|
|
{Type: "streamable-http", URL: "https://api.microsoft.com/mcp"},
|
|
},
|
|
},
|
|
}
|
|
|
|
memDB := database.NewMemoryDB()
|
|
service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false})
|
|
|
|
for _, server := range existingServers {
|
|
_, err := service.Publish(*server)
|
|
if err != nil {
|
|
t.Fatalf("failed to publish server: %v", err)
|
|
}
|
|
}
|
|
|
|
tests := []struct {
|
|
name string
|
|
serverDetail apiv0.ServerJSON
|
|
expectError bool
|
|
errorMsg string
|
|
}{
|
|
{
|
|
name: "no remote URLs - should pass",
|
|
serverDetail: apiv0.ServerJSON{
|
|
Name: "com.example/new-server",
|
|
Description: "A new server with no remotes",
|
|
Version: "1.0.0",
|
|
Remotes: []model.Transport{},
|
|
},
|
|
expectError: false,
|
|
},
|
|
{
|
|
name: "new unique remote URLs - should pass",
|
|
serverDetail: apiv0.ServerJSON{
|
|
Name: "com.example/new-server",
|
|
Description: "A new server",
|
|
Version: "1.0.0",
|
|
Remotes: []model.Transport{
|
|
{Type: "streamable-http", URL: "https://new.example.com/mcp"},
|
|
{Type: "sse", URL: "https://unique.example.com/sse"},
|
|
},
|
|
},
|
|
expectError: false,
|
|
},
|
|
{
|
|
name: "duplicate remote URL - should fail",
|
|
serverDetail: apiv0.ServerJSON{
|
|
Name: "com.example/new-server",
|
|
Description: "A new server with duplicate URL",
|
|
Version: "1.0.0",
|
|
Remotes: []model.Transport{
|
|
{Type: "streamable-http", URL: "https://api.example.com/mcp"}, // This URL already exists
|
|
},
|
|
},
|
|
expectError: true,
|
|
errorMsg: "remote URL https://api.example.com/mcp is already used by server com.example/existing-server",
|
|
},
|
|
{
|
|
name: "updating same server with same URLs - should pass",
|
|
serverDetail: apiv0.ServerJSON{
|
|
Name: "com.example/existing-server", // Same name as existing
|
|
Description: "Updated existing server",
|
|
Version: "1.1.0",
|
|
Remotes: []model.Transport{
|
|
{Type: "streamable-http", URL: "https://api.example.com/mcp"}, // Same URL as before
|
|
},
|
|
},
|
|
expectError: false,
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
ctx := context.Background()
|
|
impl := service.(*registryServiceImpl)
|
|
|
|
err := impl.validateNoDuplicateRemoteURLs(ctx, tt.serverDetail)
|
|
|
|
if tt.expectError {
|
|
assert.Error(t, err)
|
|
assert.Contains(t, err.Error(), tt.errorMsg)
|
|
} else {
|
|
assert.NoError(t, err)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestGetByServerID(t *testing.T) {
|
|
memDB := database.NewMemoryDB()
|
|
service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false})
|
|
|
|
// Publish multiple versions of the same server
|
|
server1, err := service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/test-server",
|
|
Description: "Test server v1",
|
|
Version: "1.0.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
_, err = service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/test-server",
|
|
Description: "Test server v2",
|
|
Version: "2.0.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
tests := []struct {
|
|
name string
|
|
serverID string
|
|
expectError bool
|
|
errorMsg string
|
|
checkResult func(*testing.T, *apiv0.ServerJSON)
|
|
}{
|
|
{
|
|
name: "get latest version by server ID",
|
|
serverID: server1.Meta.Official.ServerID,
|
|
expectError: false,
|
|
checkResult: func(t *testing.T, result *apiv0.ServerJSON) {
|
|
t.Helper()
|
|
assert.Equal(t, "2.0.0", result.Version) // Should get latest version
|
|
assert.Equal(t, "Test server v2", result.Description)
|
|
assert.True(t, result.Meta.Official.IsLatest)
|
|
},
|
|
},
|
|
{
|
|
name: "server not found",
|
|
serverID: "00000000-0000-0000-0000-000000000000",
|
|
expectError: true,
|
|
errorMsg: "record not found",
|
|
},
|
|
{
|
|
name: "invalid server ID format",
|
|
serverID: "invalid-uuid",
|
|
expectError: true,
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
result, err := service.GetByServerID(tt.serverID)
|
|
|
|
if tt.expectError {
|
|
assert.Error(t, err)
|
|
if tt.errorMsg != "" {
|
|
assert.Contains(t, err.Error(), tt.errorMsg)
|
|
}
|
|
assert.Nil(t, result)
|
|
} else {
|
|
assert.NoError(t, err)
|
|
assert.NotNil(t, result)
|
|
if tt.checkResult != nil {
|
|
tt.checkResult(t, result)
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestGetByServerIDAndVersion(t *testing.T) {
|
|
memDB := database.NewMemoryDB()
|
|
service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false})
|
|
|
|
// Publish multiple versions of the same server
|
|
server1, err := service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/versioned-server",
|
|
Description: "Versioned server v1",
|
|
Version: "1.0.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
_, err = service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/versioned-server",
|
|
Description: "Versioned server v2",
|
|
Version: "2.0.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
tests := []struct {
|
|
name string
|
|
serverID string
|
|
version string
|
|
expectError bool
|
|
errorMsg string
|
|
checkResult func(*testing.T, *apiv0.ServerJSON)
|
|
}{
|
|
{
|
|
name: "get specific version 1.0.0",
|
|
serverID: server1.Meta.Official.ServerID,
|
|
version: "1.0.0",
|
|
expectError: false,
|
|
checkResult: func(t *testing.T, result *apiv0.ServerJSON) {
|
|
t.Helper()
|
|
assert.Equal(t, "1.0.0", result.Version)
|
|
assert.Equal(t, "Versioned server v1", result.Description)
|
|
assert.False(t, result.Meta.Official.IsLatest)
|
|
},
|
|
},
|
|
{
|
|
name: "get specific version 2.0.0",
|
|
serverID: server1.Meta.Official.ServerID,
|
|
version: "2.0.0",
|
|
expectError: false,
|
|
checkResult: func(t *testing.T, result *apiv0.ServerJSON) {
|
|
t.Helper()
|
|
assert.Equal(t, "2.0.0", result.Version)
|
|
assert.Equal(t, "Versioned server v2", result.Description)
|
|
assert.True(t, result.Meta.Official.IsLatest)
|
|
},
|
|
},
|
|
{
|
|
name: "version not found",
|
|
serverID: server1.Meta.Official.ServerID,
|
|
version: "3.0.0",
|
|
expectError: true,
|
|
errorMsg: "record not found",
|
|
},
|
|
{
|
|
name: "server not found",
|
|
serverID: "00000000-0000-0000-0000-000000000000",
|
|
version: "1.0.0",
|
|
expectError: true,
|
|
errorMsg: "record not found",
|
|
},
|
|
{
|
|
name: "invalid server ID format",
|
|
serverID: "invalid-uuid",
|
|
version: "1.0.0",
|
|
expectError: true,
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
result, err := service.GetByServerIDAndVersion(tt.serverID, tt.version)
|
|
|
|
if tt.expectError {
|
|
assert.Error(t, err)
|
|
if tt.errorMsg != "" {
|
|
assert.Contains(t, err.Error(), tt.errorMsg)
|
|
}
|
|
assert.Nil(t, result)
|
|
} else {
|
|
assert.NoError(t, err)
|
|
assert.NotNil(t, result)
|
|
if tt.checkResult != nil {
|
|
tt.checkResult(t, result)
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestGetAllVersionsByServerID(t *testing.T) {
|
|
memDB := database.NewMemoryDB()
|
|
service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false})
|
|
|
|
// Publish multiple versions of the same server
|
|
server1, err := service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/multi-version-server",
|
|
Description: "Multi-version server v1",
|
|
Version: "1.0.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
_, err = service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/multi-version-server",
|
|
Description: "Multi-version server v2",
|
|
Version: "2.0.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
_, err = service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/multi-version-server",
|
|
Description: "Multi-version server v2.1",
|
|
Version: "2.1.0",
|
|
})
|
|
assert.NoError(t, err)
|
|
|
|
tests := []struct {
|
|
name string
|
|
serverID string
|
|
expectError bool
|
|
errorMsg string
|
|
checkResult func(*testing.T, []apiv0.ServerJSON)
|
|
}{
|
|
{
|
|
name: "get all versions of server",
|
|
serverID: server1.Meta.Official.ServerID,
|
|
expectError: false,
|
|
checkResult: func(t *testing.T, result []apiv0.ServerJSON) {
|
|
t.Helper()
|
|
assert.Len(t, result, 3)
|
|
|
|
// Collect versions
|
|
versions := make([]string, 0, len(result))
|
|
latestCount := 0
|
|
for _, server := range result {
|
|
versions = append(versions, server.Version)
|
|
assert.Equal(t, server1.Meta.Official.ServerID, server.Meta.Official.ServerID)
|
|
assert.Equal(t, "com.example/multi-version-server", server.Name)
|
|
if server.Meta.Official.IsLatest {
|
|
latestCount++
|
|
}
|
|
}
|
|
|
|
// Verify all versions are present
|
|
assert.Contains(t, versions, "1.0.0")
|
|
assert.Contains(t, versions, "2.0.0")
|
|
assert.Contains(t, versions, "2.1.0")
|
|
|
|
// Only one should be marked as latest
|
|
assert.Equal(t, 1, latestCount)
|
|
},
|
|
},
|
|
{
|
|
name: "server not found",
|
|
serverID: "00000000-0000-0000-0000-000000000000",
|
|
expectError: true,
|
|
errorMsg: "record not found",
|
|
},
|
|
{
|
|
name: "invalid server ID format",
|
|
serverID: "invalid-uuid",
|
|
expectError: true,
|
|
},
|
|
}
|
|
|
|
for _, tt := range tests {
|
|
t.Run(tt.name, func(t *testing.T) {
|
|
result, err := service.GetAllVersionsByServerID(tt.serverID)
|
|
|
|
if tt.expectError {
|
|
assert.Error(t, err)
|
|
if tt.errorMsg != "" {
|
|
assert.Contains(t, err.Error(), tt.errorMsg)
|
|
}
|
|
assert.Empty(t, result)
|
|
} else {
|
|
assert.NoError(t, err)
|
|
assert.NotEmpty(t, result)
|
|
if tt.checkResult != nil {
|
|
tt.checkResult(t, result)
|
|
}
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestPublishConcurrentVersionsNoRace(t *testing.T) {
|
|
memDB := database.NewMemoryDB()
|
|
service := NewRegistryService(memDB, &config.Config{EnableRegistryValidation: false})
|
|
|
|
const concurrency = 100
|
|
results := make([]*apiv0.ServerJSON, concurrency)
|
|
errors := make([]error, concurrency)
|
|
|
|
var wg sync.WaitGroup
|
|
for i := range concurrency {
|
|
wg.Add(1)
|
|
go func(idx int) {
|
|
defer wg.Done()
|
|
result, err := service.Publish(apiv0.ServerJSON{
|
|
Name: "com.example/test-concurrent",
|
|
Description: fmt.Sprintf("Version %d", idx),
|
|
Version: fmt.Sprintf("1.0.%d", idx),
|
|
})
|
|
results[idx] = result
|
|
errors[idx] = err
|
|
}(i)
|
|
}
|
|
wg.Wait()
|
|
|
|
for i, err := range errors {
|
|
assert.NoError(t, err, "publish %d failed", i)
|
|
}
|
|
|
|
var sharedServerID string
|
|
for i, result := range results {
|
|
if result != nil && result.Meta != nil && result.Meta.Official != nil {
|
|
if sharedServerID == "" {
|
|
sharedServerID = result.Meta.Official.ServerID
|
|
}
|
|
assert.Equal(t, sharedServerID, result.Meta.Official.ServerID,
|
|
"version %d has different serverID", i)
|
|
}
|
|
}
|
|
|
|
latestCount := 0
|
|
for _, result := range results {
|
|
if result != nil && result.Meta != nil && result.Meta.Official != nil &&
|
|
result.Meta.Official.IsLatest {
|
|
latestCount++
|
|
}
|
|
}
|
|
assert.Equal(t, 1, latestCount, "should have exactly one latest version")
|
|
}
|