diff --git a/pkg/api/middleware/ratelimit.go b/pkg/api/middleware/ratelimit.go index e8c1415b2..d282f207c 100644 --- a/pkg/api/middleware/ratelimit.go +++ b/pkg/api/middleware/ratelimit.go @@ -26,18 +26,17 @@ import ( "time" "github.com/ZaparooProject/zaparoo-core/v2/pkg/helpers/syncutil" - "github.com/olahol/melody" "github.com/rs/zerolog/log" "golang.org/x/time/rate" ) const ( - RequestsPerMinute = 100 // Simple limit - 100 requests per minute per IP - BurstSize = 20 // Allow burst of 20 requests - WebSocketRateLimitWait = 2 * time.Second + RequestsPerMinute = 100 // Simple limit - 100 requests per minute per IP + BurstSize = 20 // Allow burst of 20 requests ) -// IPRateLimiter manages rate limiters per IP address for both HTTP and WebSocket +// IPRateLimiter manages HTTP admission rate limiters per IP address. +// WebSocket upgrades consume one token; established frames use session queues. type IPRateLimiter struct { limiters map[string]*rateLimiterEntry mu syncutil.RWMutex @@ -174,51 +173,3 @@ func HTTPRateLimitMiddleware(limiter *IPRateLimiter) func(http.Handler) http.Han }) } } - -// WebSocketRateLimitHandler wraps a WebSocket message handler with rate -// limiting. When the per-IP rate limit is exceeded the connection is closed -// rather than receiving a structured JSON-RPC error: this avoids leaking -// plaintext frames onto encrypted sessions (which would not match the -// {"e":...} envelope and could not be decrypted by the client) and gives -// well-behaved clients an unambiguous "back off and reconnect" signal. -func WebSocketRateLimitHandler( - limiter *IPRateLimiter, - handler func(*melody.Session, []byte), -) func(*melody.Session, []byte) { - return WebSocketRateLimitHandlerWithWait(limiter, WebSocketRateLimitWait, handler) -} - -func WebSocketRateLimitHandlerWithWait( - limiter *IPRateLimiter, - waitTimeout time.Duration, - handler func(*melody.Session, []byte), -) func(*melody.Session, []byte) { - return func(session *melody.Session, msg []byte) { - host, exempt := remoteRateLimitHost(session.Request.RemoteAddr) - if exempt { - handler(session, msg) - return - } - - rl := limiter.GetLimiter(host) - - ctx, cancel := context.WithTimeout(context.Background(), waitTimeout) - defer cancel() - waitTimeoutValue := waitTimeout - if err := rl.Wait(ctx); err != nil { - log.Warn(). - Err(err). - Str("ip", host). - Int("msg_size", len(msg)). - Str("wait_timeout", waitTimeoutValue.String()). - Msg("WebSocket rate limit wait failed, closing connection") - - if err := session.Close(); err != nil { - log.Debug().Err(err).Msg("failed to close rate-limited session") - } - return - } - - handler(session, msg) - } -} diff --git a/pkg/api/middleware/ratelimit_test.go b/pkg/api/middleware/ratelimit_test.go index dc5cf44b2..db0751f0f 100644 --- a/pkg/api/middleware/ratelimit_test.go +++ b/pkg/api/middleware/ratelimit_test.go @@ -21,19 +21,12 @@ package middleware import ( "context" - "errors" - "net" "net/http" "net/http/httptest" - "strings" - "sync/atomic" "testing" "time" - "github.com/gorilla/websocket" - "github.com/olahol/melody" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "golang.org/x/time/rate" ) @@ -245,137 +238,3 @@ func TestHTTPRateLimitMiddleware_DoesNotExemptNonLoopbackHostnames(t *testing.T) } assert.Equal(t, 1, callCount) } - -func TestWebSocketRateLimitHandler_WaitsForToken(t *testing.T) { - t.Parallel() - - // burst=1 allows the first message immediately; the second waits for - // the next token instead of closing the session. - rl := NewIPRateLimiterWithLimits(rate.Every(50*time.Millisecond), 1) - var handlerCalls atomic.Int32 - inner := func(_ *melody.Session, _ []byte) { - handlerCalls.Add(1) - } - wrapped := WebSocketRateLimitHandlerWithWait(rl, 250*time.Millisecond, inner) - - m := melody.New() - m.HandleMessage(wrapped) - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - r.RemoteAddr = "192.168.1.1:12345" - _ = m.HandleRequest(w, r) - })) - defer srv.Close() - - wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") - //nolint:bodyclose // websocket conn manages the body - conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) - require.NoError(t, err) - defer func() { _ = conn.Close() }() - - // First message should go through. - require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("hello"))) - require.Eventually(t, func() bool { - return handlerCalls.Load() == 1 - }, 500*time.Millisecond, 10*time.Millisecond, "first message should be handled") - - // Second message should wait for a token and then reach the handler. - require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("again"))) - require.Eventually(t, func() bool { - return handlerCalls.Load() == 2 - }, 500*time.Millisecond, 10*time.Millisecond, "second message should be handled after backpressure wait") - - // The server should keep the connection open. - require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("third"))) - require.Eventually(t, func() bool { - return handlerCalls.Load() == 3 - }, 500*time.Millisecond, 10*time.Millisecond, "connection should remain usable after waiting") -} - -func TestWebSocketRateLimitHandler_ClosesAfterWaitTimeout(t *testing.T) { - t.Parallel() - - // burst=1 and no refill forces the second message to exceed the bounded wait. - rl := NewIPRateLimiterWithLimits(0, 1) - var handlerCalls atomic.Int32 - inner := func(_ *melody.Session, _ []byte) { - handlerCalls.Add(1) - } - wrapped := WebSocketRateLimitHandlerWithWait(rl, 20*time.Millisecond, inner) - - m := melody.New() - m.HandleMessage(wrapped) - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - r.RemoteAddr = "192.168.1.1:12345" - _ = m.HandleRequest(w, r) - })) - defer srv.Close() - - wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") - //nolint:bodyclose // websocket conn manages the body - conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) - require.NoError(t, err) - defer func() { _ = conn.Close() }() - - require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("hello"))) - require.Eventually(t, func() bool { - return handlerCalls.Load() == 1 - }, 500*time.Millisecond, 10*time.Millisecond, "first message should be handled") - - require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("again"))) - assert.Never(t, func() bool { - return handlerCalls.Load() != 1 - }, 150*time.Millisecond, 10*time.Millisecond, "second message should not reach handler") - - // The server should have closed the connection. - _ = conn.SetReadDeadline(time.Now().Add(100 * time.Millisecond)) - _, _, err = conn.ReadMessage() - require.Error(t, err, "connection should be closed after rate limit exceeded") - var netErr net.Error - if errors.As(err, &netErr) { - assert.False(t, netErr.Timeout(), "connection read should fail from server close, not read timeout") - } - assert.True(t, - websocket.IsCloseError(err, - websocket.CloseNormalClosure, - websocket.CloseGoingAway, - websocket.CloseAbnormalClosure, - websocket.CloseNoStatusReceived, - ) || websocket.IsUnexpectedCloseError(err), - "connection read should return a websocket close error, got %v", err, - ) -} - -func TestWebSocketRateLimitHandler_ExemptsLoopback(t *testing.T) { - t.Parallel() - - rl := NewIPRateLimiterWithLimits(0, 1) - var handlerCalls atomic.Int32 - inner := func(_ *melody.Session, _ []byte) { - handlerCalls.Add(1) - } - wrapped := WebSocketRateLimitHandlerWithWait(rl, 20*time.Millisecond, inner) - - m := melody.New() - m.HandleMessage(wrapped) - - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - _ = m.HandleRequest(w, r) - })) - defer srv.Close() - - wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") - //nolint:bodyclose // websocket conn manages the body - conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) - require.NoError(t, err) - defer func() { _ = conn.Close() }() - - for range 3 { - require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("hello"))) - } - - require.Eventually(t, func() bool { - return handlerCalls.Load() == 3 - }, 500*time.Millisecond, 10*time.Millisecond, "loopback websocket messages should bypass rate limiting") -} diff --git a/pkg/api/request_priority.go b/pkg/api/request_priority.go index fd6653017..4dedbc4de 100644 --- a/pkg/api/request_priority.go +++ b/pkg/api/request_priority.go @@ -38,6 +38,17 @@ const ( apiPriorityLow ) +func (p apiRequestPriority) String() string { + switch p { + case apiPriorityHigh: + return "high" + case apiPriorityLow: + return "low" + default: + return "normal" + } +} + func requestTimeoutForAPIMethod(method string) time.Duration { if models.MethodHasUnboundedRuntime(method) { return 0 diff --git a/pkg/api/server.go b/pkg/api/server.go index 73a1afc53..acf21685c 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -103,6 +103,11 @@ var JSONRPCErrorInternalError = models.ErrorObject{ Message: "Internal error", } +var JSONRPCErrorServerBusy = models.ErrorObject{ + Code: -32000, + Message: "Server busy", +} + func makeJSONRPCError(code int, message string) models.ErrorObject { return models.ErrorObject{ Code: code, @@ -1194,6 +1199,33 @@ func handleWSMessage( } if err := enqueueWSRequest(dispatcher, methodMap, &env, plaintext, cs, tracker); err != nil { + var queueFullErr *wsRequestQueueFullError + if errors.As(err, &queueFullErr) { + log.Warn(). + Str("method", queueFullErr.method). + Str("requestId", requestIDForLog(queueFullErr.requestID)). + Str("priority", queueFullErr.priority.String()). + Int("queueDepth", queueFullErr.depth). + Int("queueCapacity", queueFullErr.capacity). + Msg("websocket request rejected because queue is full") + if queueFullErr.requestID.IsAbsent() { + endTrackedRequest() + return + } + dispatcher.enqueueResponse(&wsResponseJob{ + result: requestResult{ + ID: queueFullErr.requestID, + Error: &JSONRPCErrorServerBusy, + ShouldReply: true, + }, + cs: cs, + tracker: tracker, + method: queueFullErr.method, + }) + handoffTrackedRequest() + return + } + log.Warn().Err(err).Msg("failed to queue websocket request") endTrackedRequest() if sendErr := sendWSEncryptedError( @@ -1883,13 +1915,10 @@ func StartWithReady( r.Get("/api/v0.1/events", sseHandler) }) - session.HandleMessage(apimiddleware.WebSocketRateLimitHandler( - rateLimiter, - handleWSMessage( - methodMap, platform, cfg, st, inTokenQueue, confirmQueue, - db, limitsManager, profilesSvc, player, playbackManager, indexPauser, scrapePauser, backupPauser, - encGateway, lastSeenTracker, tracker, - ), + session.HandleMessage(handleWSMessage( + methodMap, platform, cfg, st, inTokenQueue, confirmQueue, + db, limitsManager, profilesSvc, player, playbackManager, indexPauser, scrapePauser, backupPauser, + encGateway, lastSeenTracker, tracker, )) // Static app assets diff --git a/pkg/api/ws_dispatcher.go b/pkg/api/ws_dispatcher.go index cf32ec6fd..c27c002ed 100644 --- a/pkg/api/ws_dispatcher.go +++ b/pkg/api/ws_dispatcher.go @@ -40,6 +40,7 @@ const ( wsNormalConcurrency = 4 wsLowConcurrency = 2 wsQueueSize = 256 + wsLowQueueSize = 16 wsResponseQueueSize = 256 wsGlobalImageConcurrent = 2 ) @@ -51,12 +52,29 @@ const ( ) var ( - wsGlobalImageSlots = make(chan struct{}, wsGlobalImageConcurrent) - wsMediaDBMu syncutil.RWMutex + errWSRequestQueueFull = errors.New("websocket request queue is full") + wsGlobalImageSlots = make(chan struct{}, wsGlobalImageConcurrent) + wsMediaDBMu syncutil.RWMutex ) const wsDispatcherSessionKey = "api.ws.dispatcher" +type wsRequestQueueFullError struct { + method string + requestID models.RPCID + priority apiRequestPriority + depth int + capacity int +} + +func (e *wsRequestQueueFullError) Error() string { + return fmt.Sprintf("%s: method %s", errWSRequestQueueFull, e.method) +} + +func (*wsRequestQueueFullError) Unwrap() error { + return errWSRequestQueueFull +} + func queueDuration(enqueuedAt time.Time) time.Duration { if enqueuedAt.IsZero() { return 0 @@ -111,7 +129,7 @@ func getOrCreateWSDispatcher(parent context.Context, session *melody.Session) *w session: session, high: make(chan *wsRequestJob, wsQueueSize), normal: make(chan *wsRequestJob, wsQueueSize), - low: make(chan *wsRequestJob, wsQueueSize), + low: make(chan *wsRequestJob, wsLowQueueSize), responses: make(chan *wsResponseJob, wsResponseQueueSize), } session.Set(wsDispatcherSessionKey, d) @@ -184,24 +202,26 @@ func (d *wsSessionDispatcher) start() { go d.writer() } -func (d *wsSessionDispatcher) enqueue(job *wsRequestJob, priority apiRequestPriority) error { - var q chan *wsRequestJob +func (d *wsSessionDispatcher) queue(priority apiRequestPriority) chan *wsRequestJob { switch priority { case apiPriorityHigh: - q = d.high + return d.high case apiPriorityLow: - q = d.low + return d.low default: - q = d.normal + return d.normal } +} +func (d *wsSessionDispatcher) enqueue(job *wsRequestJob, priority apiRequestPriority) error { + q := d.queue(priority) select { case <-d.ctx.Done(): return d.ctx.Err() case q <- job: return nil default: - return errors.New("websocket request queue is full") + return errWSRequestQueueFull } } @@ -410,6 +430,16 @@ func enqueueWSRequest( image: isImageAPIMethod(method), } if err := d.enqueue(job, priority); err != nil { + if errors.Is(err, errWSRequestQueueFull) { + q := d.queue(priority) + return &wsRequestQueueFullError{ + requestID: requestID, + method: method, + priority: priority, + depth: len(q), + capacity: cap(q), + } + } return fmt.Errorf("enqueue websocket request: %w", err) } return nil diff --git a/pkg/api/ws_dispatcher_test.go b/pkg/api/ws_dispatcher_test.go index aab11d3b9..27afa3958 100644 --- a/pkg/api/ws_dispatcher_test.go +++ b/pkg/api/ws_dispatcher_test.go @@ -21,14 +21,17 @@ package api import ( "context" + "encoding/base64" "encoding/json" "fmt" + "net" "net/http" "net/http/httptest" "net/url" "testing" "time" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/api/crypto" "github.com/ZaparooProject/zaparoo-core/v2/pkg/api/models" "github.com/ZaparooProject/zaparoo-core/v2/pkg/api/models/requests" "github.com/ZaparooProject/zaparoo-core/v2/pkg/config" @@ -158,6 +161,161 @@ func TestWebSocketPriorityDispatcherHighPriorityBypassesSlowImage(t *testing.T) assert.Less(t, favoriteIndex, queuedImageIndex, "mutation should bypass queued image work") } +func TestWebSocketLowPriorityQueueRejectsWithoutClosingSession(t *testing.T) { + imageStarted := make(chan struct{}, wsLowConcurrency) + releaseImages := make(chan struct{}) + defer close(releaseImages) + + var methodMap MethodMap + require.NoError(t, methodMap.AddMethod(models.MethodMediaImage, func(env requests.RequestEnv) (any, error) { + select { + case imageStarted <- struct{}{}: + default: + } + select { + case <-releaseImages: + return map[string]string{"kind": "image"}, nil + case <-env.Context.Done(): + return nil, env.Context.Err() + } + })) + require.NoError(t, methodMap.AddMethod(models.MethodRun, func(requests.RequestEnv) (any, error) { + return map[string]string{"kind": "run"}, nil + })) + + wsURL, cleanup := startPriorityWSServer(t, &methodMap) + defer cleanup() + + conn := dialWS(t, wsURL) + defer func() { _ = conn.Close() }() + + for id := 1; id <= wsLowConcurrency; id++ { + require.NoError(t, conn.WriteMessage(websocket.TextMessage, + []byte(fmt.Sprintf(`{"jsonrpc":"2.0","method":"media.image","id":%d}`, id)))) + } + for range wsLowConcurrency { + select { + case <-imageStarted: + case <-time.After(2 * time.Second): + t.Fatal("media.image did not start") + } + } + + for id := wsLowConcurrency + 1; id <= wsLowConcurrency+wsLowQueueSize; id++ { + require.NoError(t, conn.WriteMessage(websocket.TextMessage, + []byte(fmt.Sprintf(`{"jsonrpc":"2.0","method":"media.image","id":%d}`, id)))) + } + busyID := wsLowConcurrency + wsLowQueueSize + 1 + require.NoError(t, conn.WriteMessage(websocket.TextMessage, + []byte(fmt.Sprintf(`{"jsonrpc":"2.0","method":"media.image","id":%d}`, busyID)))) + // Saturated JSON-RPC notifications are dropped without an error response. + require.NoError(t, conn.WriteMessage(websocket.TextMessage, + []byte(`{"jsonrpc":"2.0","method":"media.image"}`))) + require.NoError(t, conn.WriteMessage(websocket.TextMessage, + []byte(`{"jsonrpc":"2.0","method":"run","id":1000}`))) + require.NoError(t, conn.WriteMessage(websocket.TextMessage, []byte("ping"))) + + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + var gotBusy, gotRun, gotPong bool + for !gotBusy || !gotRun || !gotPong { + _, msg, err := conn.ReadMessage() + require.NoError(t, err) + if string(msg) == "pong" { + gotPong = true + continue + } + + var resp models.ResponseObject + require.NoError(t, json.Unmarshal(msg, &resp)) + switch { + case resp.ID.Equal(models.NewNumberID(int64(busyID))): + require.NotNil(t, resp.Error) + assert.Equal(t, JSONRPCErrorServerBusy.Code, resp.Error.Code) + assert.Equal(t, JSONRPCErrorServerBusy.Message, resp.Error.Message) + gotBusy = true + case resp.ID.Equal(models.NewNumberID(1000)): + require.Nil(t, resp.Error) + gotRun = true + default: + t.Fatalf("unexpected response while image workers blocked: %s", msg) + } + } + + require.NoError(t, conn.SetReadDeadline(time.Now().Add(100*time.Millisecond))) + _, msg, err := conn.ReadMessage() + if err == nil { + t.Fatalf("unexpected response for rejected notification: %s", msg) + } + var netErr net.Error + require.ErrorAs(t, err, &netErr, "expected read timeout, got %v", err) + assert.True(t, netErr.Timeout(), "expected read timeout, got %v", err) + + select { + case <-imageStarted: + t.Fatal("rejected media.image reached a worker") + default: + } +} + +func TestWebSocketBusyResponseUsesEncryptedSession(t *testing.T) { + cs, clientSecrets := establishTestEncryptionSession(t) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + m := newWebSocketSession() + m.HandleConnect(func(session *melody.Session) { + dispatcher := getOrCreateWSDispatcher(ctx, session) + dispatcher.enqueueResponse(&wsResponseJob{ + result: requestResult{ + ID: models.NewStringID("busy-request"), + Error: &JSONRPCErrorServerBusy, + ShouldReply: true, + }, + cs: cs, + method: models.MethodMediaImage, + }) + }) + m.HandleDisconnect(func(session *melody.Session) { + closeWSDispatcher(session) + }) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = m.HandleRequest(w, r) + })) + defer func() { + _ = m.Close() + srv.Close() + }() + + conn := dialWS(t, "ws"+srv.URL[len("http"):]) + defer func() { _ = conn.Close() }() + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + _, msg, err := conn.ReadMessage() + require.NoError(t, err) + + var frame struct { + Ciphertext string `json:"e"` + } + require.NoError(t, json.Unmarshal(msg, &frame)) + ciphertext, err := base64.StdEncoding.DecodeString(frame.Ciphertext) + require.NoError(t, err) + plaintext, err := crypto.Decrypt( + clientSecrets.s2cGCM, + clientSecrets.s2cNonce, + 0, + ciphertext, + clientSecrets.aad, + ) + require.NoError(t, err) + + var resp models.ResponseObject + require.NoError(t, json.Unmarshal(plaintext, &resp)) + assert.Equal(t, models.NewStringID("busy-request"), resp.ID) + require.NotNil(t, resp.Error) + assert.Equal(t, JSONRPCErrorServerBusy.Code, resp.Error.Code) + assert.Equal(t, JSONRPCErrorServerBusy.Message, resp.Error.Message) +} + func TestWebSocketPriorityDispatcherPreservesHighPriorityOrder(t *testing.T) { t.Parallel() @@ -457,7 +615,7 @@ func TestCloseWSDispatcherCancelsQueuedRequests(t *testing.T) { cancel: cancel, high: make(chan *wsRequestJob, wsQueueSize), normal: make(chan *wsRequestJob, wsQueueSize), - low: make(chan *wsRequestJob, wsQueueSize), + low: make(chan *wsRequestJob, wsLowQueueSize), responses: make(chan *wsResponseJob, wsResponseQueueSize), } diff --git a/pkg/database/mediadb/media_search_path_test.go b/pkg/database/mediadb/media_search_path_test.go index 440a5205f..061e297fe 100644 --- a/pkg/database/mediadb/media_search_path_test.go +++ b/pkg/database/mediadb/media_search_path_test.go @@ -22,6 +22,7 @@ package mediadb import ( "context" "path/filepath" + "strings" "testing" "github.com/ZaparooProject/go-zapscript" @@ -31,6 +32,14 @@ import ( "github.com/stretchr/testify/require" ) +func TestMediaRecursivePathPrefixNormalizesNativeSeparators(t *testing.T) { + t.Parallel() + + path := filepath.Join("roms", "SNES") + backslashPath := strings.ReplaceAll(path, string(filepath.Separator), `\`) + assert.Equal(t, filepath.ToSlash(path)+"/", mediaRecursivePathPrefix(backslashPath)) +} + func TestSearchMediaWithFilters_PathPrefixIsRecursiveAndBoundarySafe(t *testing.T) { t.Parallel() @@ -38,13 +47,13 @@ func TestSearchMediaWithFilters_PathPrefixIsRecursiveAndBoundarySafe(t *testing. defer cleanup() ctx := context.Background() - root := filepath.Join("roms", "SNES") - insidePath := filepath.Join(root, "inside.sfc") - nestedPath := filepath.Join(root, "nested", "inside-too.sfc") - siblingPath := filepath.Join("roms", "SNES2", "outside.sfc") - literalRoot := filepath.Join("roms", "100%_Games") - literalPath := filepath.Join(literalRoot, "literal.sfc") - wildcardLookalikePath := filepath.Join("roms", "100XXGames", "outside.sfc") + root := filepath.ToSlash(filepath.Join("roms", "SNES")) + insidePath := filepath.ToSlash(filepath.Join(root, "inside.sfc")) + nestedPath := filepath.ToSlash(filepath.Join(root, "nested", "inside-too.sfc")) + siblingPath := filepath.ToSlash(filepath.Join("roms", "SNES2", "outside.sfc")) + literalRoot := filepath.ToSlash(filepath.Join("roms", "100%_Games")) + literalPath := filepath.ToSlash(filepath.Join(literalRoot, "literal.sfc")) + wildcardLookalikePath := filepath.ToSlash(filepath.Join("roms", "100XXGames", "outside.sfc")) system := insertSystemWithMedia(t, mediaDB, "SNES", "Inside", insidePath) insertSystemMedia(t, mediaDB, system, "Nested", nestedPath) @@ -71,6 +80,10 @@ func TestSearchMediaWithFilters_PathPrefixIsRecursiveAndBoundarySafe(t *testing. require.Len(t, results, 2) assert.Equal(t, []string{insidePath, nestedPath}, []string{results[0].Path, results[1].Path}) + results = search(strings.ReplaceAll(root, "/", `\`)) + require.Len(t, results, 2) + assert.Equal(t, []string{insidePath, nestedPath}, []string{results[0].Path, results[1].Path}) + results = search(literalRoot) require.Len(t, results, 1) assert.Equal(t, literalPath, results[0].Path) @@ -159,11 +172,11 @@ func TestSearchMediaWithFilters_PathPrefixComposesAcrossCacheAndSQL(t *testing.T defer cleanup() ctx := context.Background() - root := filepath.Join("roms", "SNES", "favorites") - alphaPath := filepath.Join(root, "target-alpha.sfc") - betaPath := filepath.Join(root, "nested", "target-beta.sfc") - notFavoritePath := filepath.Join(root, "target-other.sfc") - outsidePath := filepath.Join("roms", "SNES", "outside", "target-outside.sfc") + root := filepath.ToSlash(filepath.Join("roms", "SNES", "favorites")) + alphaPath := filepath.ToSlash(filepath.Join(root, "target-alpha.sfc")) + betaPath := filepath.ToSlash(filepath.Join(root, "nested", "target-beta.sfc")) + notFavoritePath := filepath.ToSlash(filepath.Join(root, "target-other.sfc")) + outsidePath := filepath.ToSlash(filepath.Join("roms", "SNES", "outside", "target-outside.sfc")) system := insertSystemWithMedia(t, mediaDB, "SNES", "Target Alpha", alphaPath) insertSystemMedia(t, mediaDB, system, "Target Beta", betaPath) diff --git a/pkg/database/mediadb/mediadb_integration_test.go b/pkg/database/mediadb/mediadb_integration_test.go index fdf28e375..80cc9ab5e 100644 --- a/pkg/database/mediadb/mediadb_integration_test.go +++ b/pkg/database/mediadb/mediadb_integration_test.go @@ -718,11 +718,11 @@ func TestMediaDB_RandomGameWithQuery_PathPrefixIntegration(t *testing.T) { insertedSystem, err := mediaDB.InsertSystem(database.System{SystemID: nesSystem.ID, Name: "NES"}) require.NoError(t, err) - matchingDir := filepath.Join("roms", "nes", "favorites") - matchingPath := filepath.Join(matchingDir, "target.nes") - nestedPath := filepath.Join(matchingDir, "nested", "nested.nes") - siblingPrefixPath := filepath.Join("roms", "nes", "favorites-old", "sibling.nes") - outsidePath := filepath.Join("roms", "nes", "other", "outside.nes") + matchingDir := filepath.ToSlash(filepath.Join("roms", "nes", "favorites")) + matchingPath := filepath.ToSlash(filepath.Join(matchingDir, "target.nes")) + nestedPath := filepath.ToSlash(filepath.Join(matchingDir, "nested", "nested.nes")) + siblingPrefixPath := filepath.ToSlash(filepath.Join("roms", "nes", "favorites-old", "sibling.nes")) + outsidePath := filepath.ToSlash(filepath.Join("roms", "nes", "other", "outside.nes")) paths := []string{matchingPath, nestedPath, siblingPrefixPath, outsidePath} for i, path := range paths { name := fmt.Sprintf("Game %d", i+1) @@ -744,7 +744,7 @@ func TestMediaDB_RandomGameWithQuery_PathPrefixIntegration(t *testing.T) { require.NoError(t, mediaDB.CommitTransaction()) result, err := mediaDB.RandomGameWithQuery(context.Background(), &database.MediaQuery{ - PathPrefix: filepath.ToSlash(matchingDir), + PathPrefix: matchingDir, }) require.NoError(t, err) diff --git a/pkg/database/mediadb/sql_helpers.go b/pkg/database/mediadb/sql_helpers.go index f95452a3b..1e7ee6245 100644 --- a/pkg/database/mediadb/sql_helpers.go +++ b/pkg/database/mediadb/sql_helpers.go @@ -26,6 +26,7 @@ import ( "github.com/ZaparooProject/zaparoo-core/v2/pkg/database" "github.com/ZaparooProject/zaparoo-core/v2/pkg/database/slugs" "github.com/ZaparooProject/zaparoo-core/v2/pkg/database/systemdefs" + "github.com/ZaparooProject/zaparoo-core/v2/pkg/helpers/pathutil" ) // return ?, ?,... based on count @@ -41,6 +42,7 @@ func prepareVariadic(p, s string, c int) string { } func mediaRecursivePathPrefix(path string) string { + path = pathutil.CanonicalMediaPath(path) if path == "" || strings.HasSuffix(path, "/") { return path } diff --git a/pkg/database/userdb/backup.go b/pkg/database/userdb/backup.go index 7761595cd..5334d661f 100644 --- a/pkg/database/userdb/backup.go +++ b/pkg/database/userdb/backup.go @@ -605,11 +605,11 @@ func (db *UserDB) RestoreBackup(name string) (database.RestoreInfo, error) { if err = replaceDatabaseFromBackup(afero.NewOsFs(), backupPath, db.GetDBPath()); err != nil { return db.restoreFailed(fmt.Errorf("failed to restore user database backup: %w", err)) } - if err = db.Open(); err != nil { - return db.restoreFailed(fmt.Errorf("failed to reopen restored user database: %w", err)) - } - if err = db.MigrateUp(); err != nil { - return db.restoreFailed(fmt.Errorf("failed to migrate restored user database: %w", err)) + // Keep the replacement private until migrations finish. Publishing the + // handle earlier lets concurrent API work lock the database between Open + // and MigrateUp, causing an otherwise valid live restore to fail. + if err = db.openMigratedDatabase(); err != nil { + return db.restoreFailed(fmt.Errorf("failed to open restored user database: %w", err)) } result := database.RestoreInfo{RestoredFrom: backup, PreRestoreBackup: preRestore} if err = db.ClearCorruptMarker(); err != nil { @@ -669,11 +669,8 @@ func (db *UserDB) RecoverFromCorruption() (database.RestoreInfo, error) { log.Warn().Err(err).Str("path", backup.Path).Msg("failed to restore user database backup") continue } - if err = db.Open(); err != nil { - return database.RestoreInfo{}, fmt.Errorf("failed to reopen restored user database: %w", err) - } - if err = db.MigrateUp(); err != nil { - return database.RestoreInfo{}, fmt.Errorf("failed to migrate restored user database: %w", err) + if err = db.openMigratedDatabase(); err != nil { + return database.RestoreInfo{}, fmt.Errorf("failed to open restored user database: %w", err) } if err = db.ClearCorruptMarker(); err != nil { return database.RestoreInfo{}, fmt.Errorf( @@ -690,11 +687,6 @@ func (db *UserDB) RecoverFromCorruption() (database.RestoreInfo, error) { "failed to create fresh user database after corruption: %w", err, ) } - if err = db.MigrateUp(); err != nil { - return database.RestoreInfo{}, fmt.Errorf( - "failed to migrate fresh user database after corruption: %w", err, - ) - } if err = db.ClearCorruptMarker(); err != nil { return database.RestoreInfo{}, fmt.Errorf( "failed to clear user database corrupt marker after fresh recovery: %w", err, diff --git a/pkg/database/userdb/userdb.go b/pkg/database/userdb/userdb.go index a35c04271..0c0453785 100644 --- a/pkg/database/userdb/userdb.go +++ b/pkg/database/userdb/userdb.go @@ -103,33 +103,56 @@ func (db *UserDB) Open() error { } } + sqlInstance, err := db.openSQLConnection(dbPath) + if err != nil { + return err + } + if !exists { + log.Debug().Msg("user database is new, allocating schema") + if err = sqlAllocate(sqlInstance, dbPath); err != nil { + _ = sqlInstance.Close() + return err + } + } + db.sql.Store(sqlInstance) + return nil +} + +func (db *UserDB) openSQLConnection(dbPath string) (*sql.DB, error) { log.Debug().Msg("opening user database connection") sqlInstance, err := sql.Open("sqlite3", dbPath+sqliteConnParams) if err != nil { - return fmt.Errorf("failed to open user database: %w", err) + return nil, fmt.Errorf("failed to open user database: %w", err) + } + if err = sqlInstance.PingContext(db.ctx); err != nil { + _ = sqlInstance.Close() + return nil, fmt.Errorf("failed to connect to user database: %w", err) } - db.sql.Store(sqlInstance) if _, err = sqlInstance.ExecContext(db.ctx, "PRAGMA cell_size_check=ON"); err != nil { if database.IsCorruptionError(err) { db.MarkCorrupt(fmt.Sprintf("cell_size_check failed during open: %v", err)) log.Warn().Err(err).Msg("user database cell size check failed during open") } else { // cell_size_check is a best-effort safety pragma; a non-corruption failure - // (e.g. a transient "database is locked" while another connection is active - // during a restore) must not disconnect an otherwise-usable database. Keep the - // connection and re-attempt the pragma on the next open. + // must not disconnect an otherwise-usable database. log.Warn().Err(err).Msg("failed to enable user database cell size checks; continuing without") } } + return sqlInstance, nil +} - if !exists { - log.Debug().Msg("user database is new, allocating schema") - err := db.Allocate() - if err != nil { - return err - } +func (db *UserDB) openMigratedDatabase() error { + dbPath := db.GetDBPath() + db.dbPath = dbPath + sqlInstance, err := db.openSQLConnection(dbPath) + if err != nil { + return err } - + if err = sqlMigrateUp(sqlInstance, dbPath); err != nil { + _ = sqlInstance.Close() + return err + } + db.sql.Store(sqlInstance) return nil } diff --git a/pkg/database/userdb/userdb_open_test.go b/pkg/database/userdb/userdb_open_test.go new file mode 100644 index 000000000..b2db4caf1 --- /dev/null +++ b/pkg/database/userdb/userdb_open_test.go @@ -0,0 +1,88 @@ +// Zaparoo Core +// Copyright (c) 2026 The Zaparoo Project Contributors. +// SPDX-License-Identifier: GPL-3.0-or-later +// +// This file is part of Zaparoo Core. +// +// Zaparoo Core is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Zaparoo Core is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with Zaparoo Core. If not, see . + +package userdb + +import ( + "context" + "database/sql" + "os" + "path/filepath" + "testing" + + "github.com/ZaparooProject/zaparoo-core/v2/pkg/database" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOpenSQLConnectionDoesNotPublish(t *testing.T) { + t.Parallel() + + db := &UserDB{ctx: t.Context()} + conn, err := db.openSQLConnection(filepath.Join(t.TempDir(), "user.db")) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, conn.Close()) }) + require.NoError(t, conn.PingContext(t.Context())) + assert.Nil(t, db.sql.Load()) +} + +func TestOpenSQLConnectionClosesConnectionFailure(t *testing.T) { + t.Parallel() + + db := &UserDB{ctx: t.Context()} + conn, err := db.openSQLConnection(filepath.Join(t.TempDir(), "missing", "user.db")) + require.Error(t, err) + assert.Nil(t, conn) + assert.Nil(t, db.sql.Load()) +} + +func TestOpenMigratedDatabasePublishesAfterMigration(t *testing.T) { + db, cleanup := setupTempUserDB(t) + defer cleanup() + + previous := db.sql.Load() + require.NoError(t, db.closeAndDrain()) + require.NoError(t, db.openMigratedDatabase()) + + current := db.sql.Load() + assert.NotSame(t, previous, current) + require.NoError(t, current.PingContext(t.Context())) +} + +func TestOpenMigratedDatabaseClosesMigrationFailureWithoutPublishing(t *testing.T) { + db, cleanup := setupTempUserDB(t) + defer cleanup() + + previous := db.sql.Load() + require.NoError(t, db.closeAndDrain()) + dbPath := db.GetDBPath() + database.RemoveSidecars(dbPath) + require.NoError(t, os.Remove(dbPath)) + + broken, err := sql.Open("sqlite3", dbPath) + require.NoError(t, err) + _, err = broken.ExecContext(context.Background(), `CREATE TABLE goose_db_version (bad TEXT)`) + require.NoError(t, err) + require.NoError(t, broken.Close()) + + err = db.openMigratedDatabase() + require.Error(t, err) + assert.Same(t, previous, db.sql.Load()) + require.NoError(t, os.Remove(dbPath), "failed migration connection must release the database file") +}