From 47e45f4d672aa62748eb02343b1234deb7cebc04 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Wed, 12 Aug 2026 15:14:08 -0400 Subject: [PATCH] feat(downloads): relay node-local artifacts through proxies (#608) * feat(downloads): relay node-local artifacts through proxies * fix(downloads): harden remote artifact relay * fix(downloads): close distributed artifact review gaps * fix(downloads): bound relays and refresh origins * fix(downloads): recover stalled remote artifacts * fix(transcode): report session cleanup failures * fix(downloads): harden artifact cleanup recovery * fix(downloads): close remote artifact lifecycle races * fix(downloads): honor configured artifact storage * fix(downloads): recover proxy-observed artifact misses * test(downloads): isolate remote cleanup assertions * fix(downloads): bound remote artifact recovery * fix(downloads): allow concurrent artifact relays * fix(downloads): harden distributed artifact recovery --- README.md | 16 +- cmd/silo/main.go | 22 +- .../2026-06-18-offline-sync-mobile-design.md | 7 +- internal/api/handlers/downloads.go | 41 +- internal/api/handlers/downloads_test.go | 97 ++++ internal/config/admin_settings.go | 2 +- internal/config/config.go | 3 + internal/config/db_loader.go | 6 +- internal/config/db_loader_test.go | 6 +- internal/config/restart_keys.go | 10 +- internal/config/restart_keys_test.go | 2 + internal/config/yaml_import.go | 2 +- internal/downloadprepare/transport.go | 377 ++++++++++++- internal/downloadprepare/transport_test.go | 196 ++++++- internal/downloads/artifact.go | 4 + internal/downloads/artifact_repo.go | 231 +++++++- internal/downloads/artifact_repo_test.go | 524 +++++++++++++++++- internal/downloads/artifact_test.go | 80 +++ internal/downloads/artifacts.go | 497 +++++++++++++++-- internal/downloads/bandwidth.go | 28 +- internal/downloads/model.go | 7 + internal/downloads/remote_preparer.go | 170 ++++-- internal/downloads/remote_preparer_test.go | 260 ++++++--- internal/downloads/repo_managed_test.go | 2 +- internal/downloads/service.go | 107 +++- internal/downloads/service_quota_test.go | 4 +- internal/downloads/service_remote_test.go | 204 +++++++ internal/nodepool/planner.go | 35 +- internal/nodepool/planner_test.go | 25 + internal/proxy/download_test.go | 163 ++++++ internal/proxy/server.go | 99 +++- internal/streamtoken/token.go | 10 + internal/transcodenode/server.go | 235 ++++++-- internal/transcodenode/server_test.go | 298 +++++++++- ...812033954_add_download_artifact_origin.sql | 20 + ...12045218_add_download_artifact_orphans.sql | 20 + ...4845_validate_download_artifact_origin.sql | 6 + ...ownload_artifact_orphan_fairness_index.sql | 6 + 38 files changed, 3485 insertions(+), 337 deletions(-) create mode 100644 internal/downloads/service_remote_test.go create mode 100644 migrations/sql/20260812033954_add_download_artifact_origin.sql create mode 100644 migrations/sql/20260812045218_add_download_artifact_orphans.sql create mode 100644 migrations/sql/20260812144845_validate_download_artifact_origin.sql create mode 100644 migrations/sql/20260812163547_add_download_artifact_orphan_fairness_index.sql diff --git a/README.md b/README.md index f844fd3a..276189e1 100644 --- a/README.md +++ b/README.md @@ -136,16 +136,20 @@ Multi-host operators can use those examples as a starting point for a dedicated file connected to the deployment's shared PostgreSQL and Redis services. Proxy nodes serve source downloads from the same absolute media paths used by direct playback. -Prepared-download work can also run on transcode nodes and the resulting file can be served by a -proxy node when `download.artifact_dir` is explicitly configured. Mount that directory at the same -absolute path on the API, transcode, and proxy nodes. With the default node-local artifact directory, -Silo keeps preparation and prepared-file delivery on the API node. Downloads with a configured -server-wide or per-user bandwidth limit also remain API-local so those aggregate limits stay exact. +Prepared-download work can also run on transcode nodes. Each selected transcode node retains its +result on node-local storage and exposes it only through Silo's authenticated internal artifact API; +the paired proxy relays those bytes, so no shared artifact mount is required. Dedicated transcode +nodes default to retaining prepared downloads in a protected directory inside the transcode volume +captured at process startup. `download.artifact_dir` overrides that location for both dedicated +transcode nodes and the integrated/API-local fallback, so mount the configured path on every process +that prepares downloads. Changing either artifact-path setting requires a restart. Downloads with a +configured server-wide or per-user bandwidth limit remain API-local so those aggregate limits stay exact. Clients discover distributed delivery through `proxy_delivery` on the download capability response. When it is true, they may opt into `GET` or `HEAD /api/v1/downloads/{id}/file-proxy` and `/api/v1/direct-download-proxy`; those routes may return a temporary redirect to a proxy node. The established `/file` and `/direct-download` routes keep serving bytes directly with their existing -status-code contract. +status-code contract; for a node-local prepared artifact, the API itself performs the authenticated +relay on that fallback route. ### Deployment Notes diff --git a/cmd/silo/main.go b/cmd/silo/main.go index 5d94ba56..015c0c8d 100644 --- a/cmd/silo/main.go +++ b/cmd/silo/main.go @@ -718,6 +718,15 @@ func main() { var handler http.Handler if mode == "proxy" { srv := proxy.NewServer(watcher, tracker) + srv.SetRemoteArtifactMissReporter(downloads.NewArtifactManager( + downloads.NewArtifactRepository(pool), + downloads.NewRepository(pool), + nil, + downloads.NewPlaybackPreparer(), + nodeID, + watcher.Config, + nil, + )) handler = srv.Handler() } else { srv := transcodenode.NewServer(watcher, tracker) @@ -921,8 +930,14 @@ func main() { proxyPool := nodepool.NewProxyPool() transcodePool := nodepool.NewTranscodePool() - proxyNodes, _ := nodeRepo.ListEnabled(context.Background(), nodepool.NodeTypeProxy) - transcodeNodes, _ := nodeRepo.ListEnabled(context.Background(), nodepool.NodeTypeTranscode) + proxyNodes, err := nodeRepo.ListEnabled(appCtx, nodepool.NodeTypeProxy) + if err != nil { + log.Fatalf("load enabled proxy nodes: %v", err) + } + transcodeNodes, err := nodeRepo.ListEnabled(appCtx, nodepool.NodeTypeTranscode) + if err != nil { + log.Fatalf("load enabled transcode nodes: %v", err) + } proxyPool.SetNodes(proxyNodes) transcodePool.SetNodes(transcodeNodes) @@ -2074,6 +2089,9 @@ func main() { downloadWorkPlanner = deps.NodePlanner } preparer := downloads.NewNodeAwarePreparer(downloads.NewPlaybackPreparer(), downloadWorkPlanner, liveDownloadConfig) + if deps.NodeRepo != nil { + preparer.SetOriginLookup(deps.NodeRepo) + } artifactMgr := downloads.NewArtifactManager( downloads.NewArtifactRepository(deps.DB), downloads.NewRepository(deps.DB), diff --git a/docs/superpowers/specs/2026-06-18-offline-sync-mobile-design.md b/docs/superpowers/specs/2026-06-18-offline-sync-mobile-design.md index f32164eb..82a117f1 100644 --- a/docs/superpowers/specs/2026-06-18-offline-sync-mobile-design.md +++ b/docs/superpowers/specs/2026-06-18-offline-sync-mobile-design.md @@ -235,7 +235,9 @@ DROP TABLE IF EXISTS public.download_artifacts; `params_hash = sha256(format | container | codec_video | codec_audio | resolution | audio_track_index | subtitle_burn_in)`. Two devices (or a web user and a phone) requesting the same 1080p H.264/AAC of the same source share one -artifact. `last_used_at` drives LRU cleanup; `output_path` is on a configured artifact volume. The +artifact. `last_used_at` drives LRU cleanup. `output_path` remains the deterministic API-local +fallback target; distributed preparation additionally stores an opaque artifact id plus its +transcode-node origin and group, allowing an authenticated proxy relay without a shared filesystem. The `attempts`/`lease_*`/`next_retry_at` columns make the table a **durable, recoverable job queue** (see the "Durable artifact queue" subsection) so a crash mid-encode cannot strand a download in `preparing`. @@ -244,7 +246,8 @@ artifact. `last_used_at` drives LRU cleanup; `output_path` is on a configured ar `server_settings` is key/value, so new keys need no migration — written when an admin saves them, parsed in `internal/config/db_loader.go`: - `download.transcode_enabled` (bool, default **false**) — server gate for transcode-to-file. -- `download.artifact_dir` (string) — artifact output volume; defaults under the existing transcode/temp root. +- `download.artifact_dir` (string) — local artifact output volume; dedicated transcode nodes default + to a protected directory inside their existing transcode volume. - `download.max_concurrent_prepares` (int, default 2) — encode/remux worker-pool size. - `download.artifact_max_bytes` (int64, default 0 = unlimited) — LRU eviction budget for artifacts. diff --git a/internal/api/handlers/downloads.go b/internal/api/handlers/downloads.go index 2ff3e256..ce7fa530 100644 --- a/internal/api/handlers/downloads.go +++ b/internal/api/handlers/downloads.go @@ -8,6 +8,7 @@ import ( "log/slog" "net/http" "net/url" + "path/filepath" "strconv" "strings" "sync" @@ -471,6 +472,9 @@ func (h *DownloadHandler) handleDownloadFile(w http.ResponseWriter, r *http.Requ // the write deadline with progress instead. sw := httpstream.NewRollingDeadlineWriter(w) if err := h.svc.ServeFile(r.Context(), sw, r, userID, profileID, deviceID, id, filter); err != nil { + if errors.Is(err, downloads.ErrResponseCommitted) { + return + } h.writeDownloadFileError(w, r, id, err) } } @@ -573,9 +577,12 @@ func (h *DownloadHandler) proxyTarget() (downloadFileResolver, string, bool) { } func (h *DownloadHandler) redirectToProxy(w http.ResponseWriter, r *http.Request, secret string, target *downloads.FileTarget, userID int, profileID string) (bool, error) { - if target == nil || strings.TrimSpace(target.Path) == "" { + if target == nil || (strings.TrimSpace(target.Path) == "" && target.OriginArtifactID == "") { return false, fmt.Errorf("download target is empty") } + if target.OriginArtifactID != "" && strings.TrimSpace(target.OriginNodeURL) == "" { + return false, nil + } if !target.ProxyEligible { return false, nil } @@ -584,25 +591,41 @@ func (h *DownloadHandler) redirectToProxy(w http.ResponseWriter, r *http.Request reservationKey = fmt.Sprintf("direct-%d-%d", userID, target.MediaFileID) } sessionID := fmt.Sprintf("download-%s-%d", reservationKey, time.Now().UnixNano()) - plan := h.nodePlanner.PlanDownload(sessionID) + plan := h.nodePlanner.PlanDownload(sessionID, target.OriginNodeGroup) if plan.ProxyNode == nil { return false, nil } releaseReservation := func() { h.nodePlanner.ReleaseSession(sessionID) } + mediaPath := target.Path + downloadFilename := "" + if target.OriginArtifactID != "" { + mediaPath = "" + if strings.TrimSpace(target.Path) != "" { + downloadFilename = filepath.Base(target.Path) + } + } token, err := streamtoken.Sign(streamtoken.Claims{ - SessionID: sessionID, - MediaPath: target.Path, - PlayMethod: streamtoken.PlayMethodDownload, - UserID: userID, - ProfileID: profileID, - MediaFileID: target.MediaFileID, + SessionID: sessionID, + MediaPath: mediaPath, + PlayMethod: streamtoken.PlayMethodDownload, + TranscodeNode: target.OriginNodeURL, + DownloadArtifactID: target.OriginArtifactID, + DownloadArtifactRowID: target.ArtifactID, + DownloadFilename: downloadFilename, + UserID: userID, + ProfileID: profileID, + MediaFileID: target.MediaFileID, }, secret, proxyDownloadTokenTTL) if err != nil { releaseReservation() return false, fmt.Errorf("sign proxy download token: %w", err) } location := strings.TrimRight(plan.ProxyNode.URL, "/") + "/downloads/file/" + url.PathEscape(token) - cacheKey := strings.TrimRight(plan.ProxyNode.URL, "/") + "\x00" + target.Path + targetKey := target.Path + if target.OriginArtifactID != "" { + targetKey = target.OriginNodeURL + "\x00" + target.OriginArtifactID + } + cacheKey := strings.TrimRight(plan.ProxyNode.URL, "/") + "\x00" + targetKey if !h.proxyCanServe(r.Context(), cacheKey, location) { releaseReservation() return false, nil diff --git a/internal/api/handlers/downloads_test.go b/internal/api/handlers/downloads_test.go index c33011df..3582b259 100644 --- a/internal/api/handlers/downloads_test.go +++ b/internal/api/handlers/downloads_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "strings" @@ -34,6 +35,7 @@ type fakeDownloadService struct { deleteErr error patchErr error serveErr error + serveBody string directErr error manifest *downloads.OfflineManifest batchManifests []*downloads.OfflineManifest @@ -184,6 +186,9 @@ func (f *fakeDownloadService) ServeDirect(_ context.Context, w http.ResponseWrit func (f *fakeDownloadService) ServeFile(_ context.Context, w http.ResponseWriter, _ *http.Request, userID int, profileID, deviceID, downloadID string, _ catalog.AccessFilter) error { f.gotServe = identityCall{userID, profileID, deviceID, downloadID} + if f.serveBody != "" { + _, _ = w.Write([]byte(f.serveBody)) + } if f.serveErr != nil { return f.serveErr } @@ -466,6 +471,21 @@ func TestManagedFileThreadsIdentity(t *testing.T) { } } +func TestManagedFileDoesNotAppendJSONAfterRelayResponseCommitted(t *testing.T) { + svc := &fakeDownloadService{ + serveBody: "partial", + serveErr: fmt.Errorf("remote relay failed: %w", downloads.ErrResponseCommitted), + } + h := NewDownloadHandler(svc) + req := withChiID(downloadTestRequest(http.MethodGet, "/downloads/dl1/file", nil, 7, "pA", "devA"), "dl1") + rec := httptest.NewRecorder() + h.HandleDownloadFile(rec, req) + + if rec.Code != http.StatusOK || rec.Body.String() != "partial" { + t.Fatalf("response status=%d body=%q", rec.Code, rec.Body.String()) + } +} + func TestManagedFileRedirectsAuthorizedArtifactToProxy(t *testing.T) { const secret = "download-proxy-secret" proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -507,6 +527,83 @@ func TestManagedFileRedirectsAuthorizedArtifactToProxy(t *testing.T) { } } +func TestManagedFileRedirectsNodeLocalArtifactThroughSameGroupProxy(t *testing.T) { + const secret = "download-proxy-secret" + proxyA := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("wrong-group proxy was selected") + w.WriteHeader(http.StatusOK) + })) + defer proxyA.Close() + proxyB := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodHead { + t.Errorf("preflight method = %s", r.Method) + } + w.WriteHeader(http.StatusOK) + })) + defer proxyB.Close() + svc := &proxyDownloadService{ + fakeDownloadService: &fakeDownloadService{}, + managedTarget: &downloads.FileTarget{ + DownloadID: "dl-remote", + ArtifactID: "artifact-row", + Path: "/prepared/Movie Final.mp4", + MediaFileID: 42, + OriginNodeURL: "http://transcode-b.internal:8096", + OriginNodeGroup: "host-b", + OriginArtifactID: "artifact-remote", + ProxyEligible: true, + }, + } + groupA, groupB := "host-a", "host-b" + proxies := nodepool.NewProxyPool() + proxies.SetNodes([]*nodepool.Node{ + {URL: proxyA.URL, Group: &groupA, Enabled: true, Healthy: true}, + {URL: proxyB.URL, Group: &groupB, Enabled: true, Healthy: true}, + }) + h := NewDownloadHandler(svc) + h.SetProxyDelivery(nodepool.NewPlanner(proxies, nodepool.NewTranscodePool()), func() string { return secret }) + req := withChiID(downloadTestRequest(http.MethodGet, "/downloads/dl-remote/file-proxy", nil, 7, "pA", "devA"), "dl-remote") + rec := httptest.NewRecorder() + h.HandleDownloadFileViaProxy(rec, req) + if rec.Code != http.StatusTemporaryRedirect { + t.Fatalf("status = %d body=%s", rec.Code, rec.Body.String()) + } + location := rec.Header().Get("Location") + prefix := proxyB.URL + "/downloads/file/" + if !strings.HasPrefix(location, prefix) { + t.Fatalf("Location = %q, want proxy-b", location) + } + claims, err := streamtoken.Verify(strings.TrimPrefix(location, prefix), secret) + if err != nil { + t.Fatal(err) + } + if claims.MediaPath != "" || claims.DownloadFilename != "Movie Final.mp4" || claims.TranscodeNode != "http://transcode-b.internal:8096" || + claims.DownloadArtifactID != "artifact-remote" || claims.DownloadArtifactRowID != "artifact-row" { + t.Fatalf("claims = %+v", claims) + } +} + +func TestManagedFileFallsBackWhenRemoteLocatorHasNoOriginURL(t *testing.T) { + svc := &proxyDownloadService{ + fakeDownloadService: &fakeDownloadService{}, + managedTarget: &downloads.FileTarget{ + DownloadID: "dl-incomplete", + OriginArtifactID: "artifact-without-origin", + ProxyEligible: true, + }, + } + proxies := nodepool.NewProxyPool() + proxies.SetNodes([]*nodepool.Node{{URL: "http://proxy.invalid", Enabled: true, Healthy: true}}) + h := NewDownloadHandler(svc) + h.SetProxyDelivery(nodepool.NewPlanner(proxies, nodepool.NewTranscodePool()), func() string { return "secret" }) + req := withChiID(downloadTestRequest(http.MethodGet, "/downloads/dl-incomplete/file-proxy", nil, 7, "pA", "devA"), "dl-incomplete") + rec := httptest.NewRecorder() + h.HandleDownloadFileViaProxy(rec, req) + if rec.Code != http.StatusOK || rec.Body.String() != "served" { + t.Fatalf("status=%d body=%q", rec.Code, rec.Body.String()) + } +} + func TestManagedListThreadsIdentity(t *testing.T) { svc := &fakeDownloadService{} h := NewDownloadHandler(svc) diff --git a/internal/config/admin_settings.go b/internal/config/admin_settings.go index 960b6511..a5efb4cd 100644 --- a/internal/config/admin_settings.go +++ b/internal/config/admin_settings.go @@ -50,7 +50,7 @@ var adminSettingDefaults = map[string]string{ "markers.lazy_playback": "false", "playback.ffmpeg_path": "/usr/lib/jellyfin-ffmpeg/ffmpeg", - "playback.transcode_dir": DefaultTranscodeDir, + playbackTranscodeDirSettingKey: DefaultTranscodeDir, "playback.hw_accel": "auto", "playback.transcode_enabled": "true", "playback.local_transcode_fallback": "true", diff --git a/internal/config/config.go b/internal/config/config.go index 018c1c61..4049dc2c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -408,6 +408,9 @@ var defaultJellyfinCompatServerID = uuid.NewSHA1( []byte("https://silo.local/jellycompat"), ).String() +const playbackTranscodeDirSettingKey = "playback.transcode_dir" +const downloadArtifactDirSettingKey = "download.artifact_dir" + // DefaultTranscodeDir is the fallback playback.transcode_dir; download // artifacts default to a sibling directory (see EffectiveDownloadArtifactDir). const DefaultTranscodeDir = "/tmp/silo-transcode" diff --git a/internal/config/db_loader.go b/internal/config/db_loader.go index 79e46715..a76fc740 100644 --- a/internal/config/db_loader.go +++ b/internal/config/db_loader.go @@ -318,7 +318,7 @@ func LoadFromDB(m map[string]string) (*Config, error) { // Playback cfg.Playback.FFmpegPath = stringOr(m, "playback.ffmpeg_path", "/usr/lib/jellyfin-ffmpeg/ffmpeg") - cfg.Playback.TranscodeDir = stringOr(m, "playback.transcode_dir", DefaultTranscodeDir) + cfg.Playback.TranscodeDir = stringOr(m, playbackTranscodeDirSettingKey, DefaultTranscodeDir) cfg.Playback.HWAccel = stringOr(m, "playback.hw_accel", "auto") cfg.Playback.HWDevice = stringOr(m, "playback.hw_device", "") chapterThumbnailWorkers, err := intOr(m, "playback.chapter_thumbnail_workers", 1) @@ -605,9 +605,9 @@ func LoadFromDB(m map[string]string) (*Config, error) { if artifactMaxBytes < 0 { return nil, fmt.Errorf("invalid value for %q: must be non-negative", "download.artifact_max_bytes") } - artifactDir := strings.TrimSpace(stringOr(m, "download.artifact_dir", "")) + artifactDir := strings.TrimSpace(stringOr(m, downloadArtifactDirSettingKey, "")) if artifactDir != "" && !filepath.IsAbs(artifactDir) { - return nil, fmt.Errorf("invalid value for %q: must be an absolute path", "download.artifact_dir") + return nil, fmt.Errorf("invalid value for %q: must be an absolute path", downloadArtifactDirSettingKey) } cfg.Download.TranscodeEnabled = downloadTranscodeEnabled cfg.Download.ArtifactDir = artifactDir diff --git a/internal/config/db_loader_test.go b/internal/config/db_loader_test.go index f8879d29..7aa06c42 100644 --- a/internal/config/db_loader_test.go +++ b/internal/config/db_loader_test.go @@ -36,7 +36,7 @@ func TestLoadFromDBMetadataPresignExpiryRejectsInvalidDuration(t *testing.T) { } func TestLoadFromDBDownloadArtifactDirRequiresAbsolutePath(t *testing.T) { - cfg, err := LoadFromDB(map[string]string{"download.artifact_dir": "/mnt/silo-downloads"}) + cfg, err := LoadFromDB(map[string]string{downloadArtifactDirSettingKey: "/mnt/silo-downloads"}) if err != nil { t.Fatalf("LoadFromDB() with absolute artifact dir: %v", err) } @@ -44,8 +44,8 @@ func TestLoadFromDBDownloadArtifactDirRequiresAbsolutePath(t *testing.T) { t.Fatalf("artifact dir = %q", cfg.Download.ArtifactDir) } - _, err = LoadFromDB(map[string]string{"download.artifact_dir": "relative/downloads"}) - if err == nil || !strings.Contains(err.Error(), "download.artifact_dir") { + _, err = LoadFromDB(map[string]string{downloadArtifactDirSettingKey: "relative/downloads"}) + if err == nil || !strings.Contains(err.Error(), downloadArtifactDirSettingKey) { t.Fatalf("relative artifact dir error = %v", err) } } diff --git a/internal/config/restart_keys.go b/internal/config/restart_keys.go index 269402e0..27d6267a 100644 --- a/internal/config/restart_keys.go +++ b/internal/config/restart_keys.go @@ -8,7 +8,7 @@ import "strings" // // Keys absent from this map and from restartRequiredPrefixes apply without a // restart: they are either read live from the settings repo at request time -// (overlays, branding, markers, download.*, ...) or hot-reloaded through the +// (overlays, branding, markers, download limits, ...) or hot-reloaded through the // nodeconfig watcher. When converting a frozen consumer to the live config, // remove its key here — the admin UI restart banner is driven by this // registry. @@ -34,11 +34,15 @@ var restartRequiredKeys = map[string]bool{ // ffmpeg path and hwaccel live (new transcode sessions), but several // startup-built consumers still freeze them (scanner ffprobe, chapter // thumbnails, audiobook enricher) — keep restart-required until those - // convert. transcode_dir is fully live (only the playback handler reads - // it). The chapter-thumbnail worker pool is sized at construction. + // convert. transcode_dir is also captured by dedicated transcode nodes for + // session and prepared-download storage. A configured download.artifact_dir is + // likewise captured by both API and transcode-node artifact managers. The + // chapter-thumbnail worker pool is sized at construction. "playback.ffmpeg_path": true, "playback.hw_accel": true, "playback.hw_device": true, + playbackTranscodeDirSettingKey: true, + downloadArtifactDirSettingKey: true, "playback.chapter_thumbnail_workers": true, // Scanner / matcher toggles captured at construction. Worker counts, diff --git a/internal/config/restart_keys_test.go b/internal/config/restart_keys_test.go index c8e67291..a81b773a 100644 --- a/internal/config/restart_keys_test.go +++ b/internal/config/restart_keys_test.go @@ -11,6 +11,8 @@ func TestRestartRequired(t *testing.T) { {"server.listen", true}, {"auth.jwt_secret", true}, {"ratelimit.backend", true}, + {playbackTranscodeDirSettingKey, true}, + {downloadArtifactDirSettingKey, true}, // Prefix-covered namespaces. {"database.max_connections", true}, {"userdb.backend", true}, diff --git a/internal/config/yaml_import.go b/internal/config/yaml_import.go index fbd903da..e8a613af 100644 --- a/internal/config/yaml_import.go +++ b/internal/config/yaml_import.go @@ -208,7 +208,7 @@ func YAMLToSettingsMap(path string) (map[string]string, error) { // Playback setIfNonEmpty(m, "playback.ffmpeg_path", raw.Playback.FFmpegPath) - setIfNonEmpty(m, "playback.transcode_dir", raw.Playback.TranscodeDir) + setIfNonEmpty(m, playbackTranscodeDirSettingKey, raw.Playback.TranscodeDir) setIfNonEmpty(m, "playback.hw_accel", raw.Playback.HWAccel) if raw.Playback.ChapterThumbnailWorkers != 0 { m["playback.chapter_thumbnail_workers"] = strconv.Itoa(raw.Playback.ChapterThumbnailWorkers) diff --git a/internal/downloadprepare/transport.go b/internal/downloadprepare/transport.go index c3a3e465..c4f33b97 100644 --- a/internal/downloadprepare/transport.go +++ b/internal/downloadprepare/transport.go @@ -1,25 +1,66 @@ -// Package downloadprepare defines the internal API used to execute a prepared -// download artifact on a dedicated transcode node. +// Package downloadprepare defines the internal API used to create and relay a +// prepared download artifact on a dedicated transcode node. package downloadprepare import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" + "net/url" + "regexp" + "strconv" "strings" + "sync" + "sync/atomic" + "time" "github.com/Silo-Server/silo-server/internal/playback" ) +const ( + ArtifactDirectoryName = "download-artifacts" + RelayReadIdleTimeout = 2 * time.Minute +) + +var ( + artifactIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,128}$`) + ErrArtifactNotFound = errors.New("remote download artifact not found") + ErrRelayReadIdle = errors.New("remote download artifact read stalled") +) + +func newHTTPClient(responseHeaderTimeout time.Duration) *http.Client { + baseTransport, ok := http.DefaultTransport.(*http.Transport) + if !ok { + baseTransport = &http.Transport{} + } + transport := baseTransport.Clone() + transport.ResponseHeaderTimeout = responseHeaderTimeout + return &http.Client{ + Transport: transport, + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, + } +} + +var ( + // Preparing a full file can legitimately take hours. Its lease-bound request + // context supplies cancellation, so imposing a response-header timeout here + // would abort every encode longer than that timeout before the node replies. + defaultPrepareHTTPClient = newHTTPClient(0) + defaultHTTPClient = newHTTPClient(60 * time.Second) +) + // Request is the environment-neutral portion of a prepared-file recipe. The -// transcode node supplies its own FFmpeg path, hardware mode, and device list. +// transcode node supplies its own FFmpeg path, hardware mode, device list, and +// output path. ArtifactID is an opaque handle, never a caller-selected path. type Request struct { - JobID string `json:"job_id"` + ArtifactID string `json:"artifact_id"` InputPath string `json:"input_path"` - OutputPath string `json:"output_path"` SourceVideoCodec string `json:"source_video_codec,omitempty"` SourceVideoProfile string `json:"source_video_profile,omitempty"` SourceVideoBitDepth int `json:"source_video_bit_depth,omitempty"` @@ -32,13 +73,21 @@ type Request struct { TotalDuration float64 `json:"total_duration,omitempty"` } +// Result identifies a completed artifact without exposing the node's local +// filesystem layout. +type Result struct { + ArtifactID string `json:"artifact_id"` + FileSize int64 `json:"file_size"` +} + +func ValidArtifactID(id string) bool { return artifactIDPattern.MatchString(id) } + // NewRequest freezes the byte-affecting recipe while deliberately omitting // environment-specific execution settings. -func NewRequest(jobID string, opts playback.TranscodeOpts, outputPath string) Request { +func NewRequest(artifactID string, opts playback.TranscodeOpts) Request { return Request{ - JobID: jobID, + ArtifactID: artifactID, InputPath: opts.InputPath, - OutputPath: outputPath, SourceVideoCodec: opts.SourceVideoCodec, SourceVideoProfile: opts.SourceVideoProfile, SourceVideoBitDepth: opts.SourceVideoBitDepth, @@ -77,43 +126,311 @@ func (r Request) TranscodeOpts(ffmpegPath, hwAccel, hwDevice string, sink playba } } -// RemotePreparer executes one request on a selected transcode node. +// RemotePreparer executes and manages artifacts on a selected transcode node. type RemotePreparer interface { - Prepare(ctx context.Context, nodeURL, jwtSecret string, req Request) error + Prepare(ctx context.Context, nodeURL, jwtSecret string, req Request) (Result, error) + Stat(ctx context.Context, nodeURL, jwtSecret, artifactID string) (Result, error) + Delete(ctx context.Context, nodeURL, jwtSecret, artifactID string) error } -// HTTPPreparer implements RemotePreparer over the bearer-protected node API. -// The request context is the only overall timeout because a full-file encode -// may legitimately run for hours; artifact lease loss cancels that context. +// HTTPPreparer implements RemotePreparer over bearer-protected node APIs. A +// full-file prepare has no transport timeout; its caller owns cancellation. +// Metadata and relay operations use a bounded response-header wait by default. type HTTPPreparer struct { - Client *http.Client + Client *http.Client + ReadIdleTimeout time.Duration } -func (p HTTPPreparer) Prepare(ctx context.Context, nodeURL, jwtSecret string, req Request) error { +func (p HTTPPreparer) Prepare(ctx context.Context, nodeURL, jwtSecret string, req Request) (Result, error) { body, err := json.Marshal(req) if err != nil { - return fmt.Errorf("remote download prepare: marshal request: %w", err) + return Result{}, fmt.Errorf("remote download prepare: marshal request: %w", err) } - httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, strings.TrimRight(nodeURL, "/")+"/downloads/prepare", bytes.NewReader(body)) + responseCtx, cancel := context.WithCancel(ctx) + httpReq, err := p.request(responseCtx, http.MethodPost, nodeURL, jwtSecret, "/downloads/prepare", bytes.NewReader(body)) if err != nil { - return fmt.Errorf("remote download prepare: build request: %w", err) + cancel() + return Result{}, fmt.Errorf("remote download prepare: %w", err) } httpReq.Header.Set("Content-Type", "application/json") - httpReq.Header.Set("Authorization", "Bearer "+jwtSecret) - - client := p.Client - if client == nil { - client = http.DefaultClient - } - resp, err := client.Do(httpReq) + resp, err := p.prepareClient().Do(httpReq) if err != nil { - return fmt.Errorf("remote download prepare: request: %w", err) + cancel() + return Result{}, fmt.Errorf("remote download prepare: request: %w", err) + } + // The encode itself remains unbounded while waiting for response headers, but + // once the node starts its small JSON result body, each read must make progress. + resp.Body = newIdleReadCloser(resp.Body, p.readIdleTimeout(), cancel) + defer func() { _ = resp.Body.Close() }() + return decodeResult(resp, "remote download prepare") +} + +func (p HTTPPreparer) Stat(ctx context.Context, nodeURL, jwtSecret, artifactID string) (Result, error) { + if !ValidArtifactID(artifactID) { + return Result{}, fmt.Errorf("remote download artifact stat: invalid artifact id") + } + httpReq, err := p.request(ctx, http.MethodHead, nodeURL, jwtSecret, "/downloads/artifacts/"+url.PathEscape(artifactID), nil) + if err != nil { + return Result{}, fmt.Errorf("remote download artifact stat: %w", err) + } + resp, err := p.client().Do(httpReq) + if err != nil { + return Result{}, fmt.Errorf("remote download artifact stat: request: %w", err) } defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusOK { - message, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) - return fmt.Errorf("remote download prepare: node returned %d: %s", resp.StatusCode, strings.TrimSpace(string(message))) + if resp.StatusCode == http.StatusNotFound { + return Result{}, ErrArtifactNotFound + } + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + return Result{}, responseError(resp, "remote download artifact stat") + } + size, err := strconv.ParseInt(resp.Header.Get("Content-Length"), 10, 64) + if err != nil || size < 0 { + return Result{}, fmt.Errorf("remote download artifact stat: invalid content length") + } + return Result{ArtifactID: artifactID, FileSize: size}, nil +} + +func (p HTTPPreparer) Delete(ctx context.Context, nodeURL, jwtSecret, artifactID string) error { + if !ValidArtifactID(artifactID) { + return fmt.Errorf("remote download artifact delete: invalid artifact id") + } + deleteCtx, cancel := context.WithCancel(ctx) + httpReq, err := p.request(deleteCtx, http.MethodDelete, nodeURL, jwtSecret, "/downloads/artifacts/"+url.PathEscape(artifactID), nil) + if err != nil { + cancel() + return fmt.Errorf("remote download artifact delete: %w", err) + } + resp, err := p.client().Do(httpReq) + if err != nil { + cancel() + return fmt.Errorf("remote download artifact delete: request: %w", err) + } + resp.Body = newIdleReadCloser(resp.Body, p.readIdleTimeout(), cancel) + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNotFound { + return nil + } + if resp.StatusCode != http.StatusNoContent { + return responseError(resp, "remote download artifact delete") } - _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096)) return nil } + +// Open returns an authenticated streaming response for a GET or HEAD relay. +// The caller owns closing the body and copying only safe response headers. +func (p HTTPPreparer) Open(ctx context.Context, nodeURL, jwtSecret, artifactID, method string, sourceHeader http.Header) (*http.Response, error) { + if !ValidArtifactID(artifactID) { + return nil, fmt.Errorf("remote download artifact open: invalid artifact id") + } + if method != http.MethodGet && method != http.MethodHead { + return nil, fmt.Errorf("remote download artifact open: unsupported method %s", method) + } + relayCtx, cancel := context.WithCancel(ctx) + httpReq, err := p.request(relayCtx, method, nodeURL, jwtSecret, "/downloads/artifacts/"+url.PathEscape(artifactID), nil) + if err != nil { + cancel() + return nil, fmt.Errorf("remote download artifact open: %w", err) + } + for _, name := range []string{"Range", "If-Match", "If-Range", "If-None-Match", "If-Modified-Since", "If-Unmodified-Since"} { + if value := sourceHeader.Get(name); value != "" { + httpReq.Header.Set(name, value) + } + } + resp, err := p.client().Do(httpReq) + if err != nil { + cancel() + return nil, fmt.Errorf("remote download artifact open: request: %w", err) + } + resp.Body = newIdleReadCloser(resp.Body, p.readIdleTimeout(), cancel) + return resp, nil +} + +// idleReadCloser bounds only time spent blocked in an upstream Read. Time +// between reads is deliberately ignored so local bandwidth throttling and slow +// clients cannot be mistaken for a stalled origin. +type idleReadCloser struct { + source io.ReadCloser + timeout time.Duration + readStart chan struct{} + readDone chan struct{} + stop chan struct{} + stopped chan struct{} + closeOnce sync.Once + timedOut atomic.Bool + cancel context.CancelFunc +} + +func newIdleReadCloser(source io.ReadCloser, timeout time.Duration, cancel context.CancelFunc) io.ReadCloser { + if source == nil || timeout <= 0 { + return source + } + r := &idleReadCloser{ + source: source, + timeout: timeout, + readStart: make(chan struct{}), + readDone: make(chan struct{}), + stop: make(chan struct{}), + stopped: make(chan struct{}), + cancel: cancel, + } + go r.watch() + return r +} + +func (r *idleReadCloser) Read(p []byte) (int, error) { + if r.timedOut.Load() { + return 0, ErrRelayReadIdle + } + select { + case r.readStart <- struct{}{}: + case <-r.stop: + return 0, io.ErrClosedPipe + } + n, err := r.source.Read(p) + select { + case r.readDone <- struct{}{}: + case <-r.stop: + } + if r.timedOut.Load() { + return n, ErrRelayReadIdle + } + return n, err +} + +func (r *idleReadCloser) Close() error { + r.closeOnce.Do(func() { + close(r.stop) + if r.cancel != nil { + r.cancel() + } + _ = r.source.Close() + <-r.stopped + }) + return nil +} + +func (r *idleReadCloser) watch() { + defer close(r.stopped) + timer := time.NewTimer(time.Hour) + if !timer.Stop() { + <-timer.C + } + reading := false + for { + select { + case <-r.readStart: + timer.Reset(r.timeout) + reading = true + case <-r.readDone: + if reading && !timer.Stop() { + select { + case <-timer.C: + default: + } + } + reading = false + case <-timer.C: + if reading { + r.timedOut.Store(true) + if r.cancel != nil { + r.cancel() + } + _ = r.source.Close() + reading = false + } + case <-r.stop: + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + return + } + } +} + +func (p HTTPPreparer) request(ctx context.Context, method, nodeURL, jwtSecret, path string, body io.Reader) (*http.Request, error) { + base, err := url.Parse(strings.TrimSpace(nodeURL)) + if err != nil || (base.Scheme != "http" && base.Scheme != "https") || base.Host == "" { + return nil, fmt.Errorf("invalid node URL") + } + base.Path = strings.TrimRight(base.Path, "/") + path + base.RawQuery = "" + base.Fragment = "" + req, err := http.NewRequestWithContext(ctx, method, base.String(), body) + if err != nil { + return nil, fmt.Errorf("build request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+jwtSecret) + return req, nil +} + +func (p HTTPPreparer) client() *http.Client { + if p.Client != nil { + return p.Client + } + return defaultHTTPClient +} + +func (p HTTPPreparer) prepareClient() *http.Client { + if p.Client != nil { + return p.Client + } + return defaultPrepareHTTPClient +} + +func (p HTTPPreparer) readIdleTimeout() time.Duration { + if p.ReadIdleTimeout > 0 { + return p.ReadIdleTimeout + } + return RelayReadIdleTimeout +} + +// CopyResponseHeaders forwards only representation/range metadata that is safe +// across the transcode-node relay boundary. +func CopyResponseHeaders(dst, src http.Header) { + for _, name := range []string{ + "Accept-Ranges", "Content-Length", "Content-Range", + "Content-Type", "ETag", "Last-Modified", + } { + if value := src.Get(name); value != "" { + dst.Set(name, value) + } + } +} + +// RelayStatusAllowed identifies the complete set of normal ServeContent +// outcomes that an internal relay must preserve unchanged. +func RelayStatusAllowed(status int) bool { + return status >= http.StatusOK && status < http.StatusMultipleChoices || + status == http.StatusNotModified || + status == http.StatusPreconditionFailed || + status == http.StatusRequestedRangeNotSatisfiable +} + +func decodeResult(resp *http.Response, operation string) (Result, error) { + if resp.StatusCode == http.StatusNotFound { + return Result{}, ErrArtifactNotFound + } + if resp.StatusCode != http.StatusOK { + return Result{}, responseError(resp, operation) + } + var result Result + if err := json.NewDecoder(io.LimitReader(resp.Body, 64<<10)).Decode(&result); err != nil { + return Result{}, fmt.Errorf("%s: decode response: %w", operation, err) + } + if !ValidArtifactID(result.ArtifactID) || result.FileSize < 0 { + return Result{}, fmt.Errorf("%s: invalid response", operation) + } + return result, nil +} + +func responseError(resp *http.Response, operation string) error { + message, err := io.ReadAll(io.LimitReader(resp.Body, 4096)) + if err != nil { + return fmt.Errorf("%s: read node error response: %w", operation, err) + } + return fmt.Errorf("%s: node returned %d: %s", operation, resp.StatusCode, strings.TrimSpace(string(message))) +} diff --git a/internal/downloadprepare/transport_test.go b/internal/downloadprepare/transport_test.go index 041ad86d..52fac25b 100644 --- a/internal/downloadprepare/transport_test.go +++ b/internal/downloadprepare/transport_test.go @@ -3,9 +3,12 @@ package downloadprepare import ( "context" "encoding/json" + "errors" + "io" "net/http" "net/http/httptest" "testing" + "time" ) func TestHTTPPreparerSendsAuthenticatedRecipe(t *testing.T) { @@ -20,14 +23,18 @@ func TestHTTPPreparerSendsAuthenticatedRecipe(t *testing.T) { if err := json.NewDecoder(r.Body).Decode(&got); err != nil { t.Fatal(err) } - w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(Result{ArtifactID: got.ArtifactID, FileSize: 123}) })) defer server.Close() - want := Request{JobID: "job-1", InputPath: "/media/movie.mkv", OutputPath: "/artifacts/movie.mp4", TargetCodecVideo: "h264", TargetCodecAudio: "aac"} - if err := (HTTPPreparer{}).Prepare(context.Background(), server.URL+"/", "secret", want); err != nil { + want := Request{ArtifactID: "job-1", InputPath: "/media/movie.mkv", TargetCodecVideo: "h264", TargetCodecAudio: "aac"} + result, err := (HTTPPreparer{}).Prepare(context.Background(), server.URL+"/", "secret", want) + if err != nil { t.Fatal(err) } + if result != (Result{ArtifactID: "job-1", FileSize: 123}) { + t.Fatalf("result = %+v", result) + } if got != want { t.Fatalf("request = %+v, want %+v", got, want) } @@ -39,8 +46,189 @@ func TestHTTPPreparerReportsNodeFailure(t *testing.T) { })) defer server.Close() - err := (HTTPPreparer{}).Prepare(context.Background(), server.URL, "secret", Request{JobID: "job-2"}) + _, err := (HTTPPreparer{}).Prepare(context.Background(), server.URL, "secret", Request{ArtifactID: "job-2"}) if err == nil { t.Fatal("expected remote failure") } } + +func TestHTTPPreparerManagesOpaqueArtifact(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/downloads/artifacts/artifact-1" { + t.Fatalf("path = %q", r.URL.Path) + } + if r.Header.Get("Authorization") != "Bearer secret" { + t.Fatalf("Authorization = %q", r.Header.Get("Authorization")) + } + switch r.Method { + case http.MethodHead: + w.Header().Set("Content-Length", "42") + case http.MethodDelete: + w.WriteHeader(http.StatusNoContent) + default: + t.Fatalf("method = %s", r.Method) + } + })) + defer server.Close() + client := HTTPPreparer{} + result, err := client.Stat(context.Background(), server.URL, "secret", "artifact-1") + if err != nil || result != (Result{ArtifactID: "artifact-1", FileSize: 42}) { + t.Fatalf("Stat = (%+v, %v)", result, err) + } + if err := client.Delete(context.Background(), server.URL, "secret", "artifact-1"); err != nil { + t.Fatal(err) + } +} + +func TestHTTPPreparerDeleteStopsStalledErrorBodyRead(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + <-r.Context().Done() + })) + defer server.Close() + + started := time.Now() + err := (HTTPPreparer{ReadIdleTimeout: 50 * time.Millisecond}).Delete( + context.Background(), server.URL, "secret", "artifact-stalled-delete", + ) + if !errors.Is(err, ErrRelayReadIdle) { + t.Fatalf("Delete error = %v, want ErrRelayReadIdle", err) + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("stalled delete response took %s, want a bounded failure", elapsed) + } +} + +func TestHTTPPreparerRejectsArtifactPathTraversal(t *testing.T) { + if _, err := (HTTPPreparer{}).Stat(context.Background(), "http://node", "secret", "../escape"); err == nil { + t.Fatal("expected invalid artifact id error") + } +} + +func TestDefaultPrepareClientHasNoResponseHeaderDeadline(t *testing.T) { + transport, ok := defaultPrepareHTTPClient.Transport.(*http.Transport) + if !ok { + t.Fatalf("transport type = %T", defaultPrepareHTTPClient.Transport) + } + if transport.ResponseHeaderTimeout != 0 { + t.Fatalf("prepare response header timeout = %s, want none", transport.ResponseHeaderTimeout) + } +} + +func TestHTTPPreparerPrepareStopsStalledResultBodyRead(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + <-r.Context().Done() + })) + defer server.Close() + + started := time.Now() + _, err := (HTTPPreparer{ReadIdleTimeout: 50 * time.Millisecond}).Prepare( + context.Background(), server.URL, "secret", Request{ArtifactID: "artifact-stalled-result"}, + ) + if !errors.Is(err, ErrRelayReadIdle) { + t.Fatalf("Prepare error = %v, want ErrRelayReadIdle", err) + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("stalled result read took %s, want a bounded failure", elapsed) + } +} + +func TestRelayStatusAllowedPreservesServeContentOutcomes(t *testing.T) { + for _, status := range []int{ + http.StatusOK, + http.StatusPartialContent, + http.StatusNotModified, + http.StatusPreconditionFailed, + http.StatusRequestedRangeNotSatisfiable, + } { + if !RelayStatusAllowed(status) { + t.Errorf("status %d should be relayed", status) + } + } + for _, status := range []int{http.StatusTemporaryRedirect, http.StatusBadRequest, http.StatusInternalServerError} { + if RelayStatusAllowed(status) { + t.Errorf("status %d should be rejected", status) + } + } +} + +func TestCopyResponseHeadersDoesNotExposeOriginArtifactFilename(t *testing.T) { + src := http.Header{ + "Content-Disposition": {`attachment; filename="opaque-artifact.mp4"`}, + "Content-Type": {"video/mp4"}, + "Content-Length": {"42"}, + } + dst := make(http.Header) + CopyResponseHeaders(dst, src) + if got := dst.Get("Content-Disposition"); got != "" { + t.Fatalf("Content-Disposition = %q, want omitted", got) + } + if dst.Get("Content-Type") != "video/mp4" || dst.Get("Content-Length") != "42" { + t.Fatalf("copied headers = %v", dst) + } +} + +func TestHTTPPreparerOpenStopsStalledBodyRead(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "video/mp4") + w.WriteHeader(http.StatusOK) + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() + } + <-r.Context().Done() + })) + defer server.Close() + + resp, err := (HTTPPreparer{ReadIdleTimeout: 50 * time.Millisecond}).Open( + context.Background(), server.URL, "secret", "artifact-stalled", http.MethodGet, nil, + ) + if err != nil { + t.Fatal(err) + } + defer func() { _ = resp.Body.Close() }() + started := time.Now() + _, err = io.ReadAll(resp.Body) + if !errors.Is(err, ErrRelayReadIdle) { + t.Fatalf("ReadAll error = %v, want ErrRelayReadIdle", err) + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("stalled read took %s, want a bounded failure", elapsed) + } +} + +func TestHTTPPreparerOpenAllowsContinuedBodyProgress(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + flusher, _ := w.(http.Flusher) + for _, chunk := range []string{"one", "two", "three"} { + _, _ = io.WriteString(w, chunk) + if flusher != nil { + flusher.Flush() + } + time.Sleep(10 * time.Millisecond) + } + })) + defer server.Close() + + resp, err := (HTTPPreparer{ReadIdleTimeout: 50 * time.Millisecond}).Open( + context.Background(), server.URL, "secret", "artifact-progress", http.MethodGet, nil, + ) + if err != nil { + t.Fatal(err) + } + defer func() { _ = resp.Body.Close() }() + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatal(err) + } + if string(body) != "onetwothree" { + t.Fatalf("body = %q", body) + } +} diff --git a/internal/downloads/artifact.go b/internal/downloads/artifact.go index 87ed179e..d0045eb6 100644 --- a/internal/downloads/artifact.go +++ b/internal/downloads/artifact.go @@ -36,6 +36,10 @@ type Artifact struct { AudioTrackIndex int TargetBitrateKbps int OutputPath string + OriginNodeID int + OriginNodeURL string + OriginNodeGroup string + OriginArtifactID string FileSize int64 Status string ErrorMessage string diff --git a/internal/downloads/artifact_repo.go b/internal/downloads/artifact_repo.go index 2b21c38f..9e91ee26 100644 --- a/internal/downloads/artifact_repo.go +++ b/internal/downloads/artifact_repo.go @@ -11,7 +11,8 @@ import ( ) const artifactColumns = `id, media_file_id, format, params_hash, container, codec_video, codec_audio, - resolution, audio_track_index, target_bitrate_kbps, output_path, file_size, status, error_message, + resolution, audio_track_index, target_bitrate_kbps, output_path, + origin_node_id, origin_node_url, origin_node_group, origin_artifact_id, file_size, status, error_message, attempts, max_attempts, lease_owner, lease_expires_at, next_retry_at, created_at, completed_at, last_used_at` @@ -21,6 +22,18 @@ type ArtifactRepository struct { pool *pgxpool.Pool } +// RemoteArtifactOrphan is a node-local cleanup candidate. The durable queue +// retains abandoned locators and verifies indeterminate readiness writes before +// deleting bytes, so transient failures cannot leak or delete a winning file. +type RemoteArtifactOrphan struct { + ID int64 + DownloadArtifactID string + OriginNodeID int + OriginNodeURL string + OriginArtifactID string + Attempts int +} + // NewArtifactRepository creates an ArtifactRepository. func NewArtifactRepository(pool *pgxpool.Pool) *ArtifactRepository { return &ArtifactRepository{pool: pool} @@ -31,7 +44,8 @@ func scanArtifact(row pgx.Row) (*Artifact, error) { var leaseOwner *string if err := row.Scan( &a.ID, &a.MediaFileID, &a.Format, &a.ParamsHash, &a.Container, &a.CodecVideo, &a.CodecAudio, - &a.Resolution, &a.AudioTrackIndex, &a.TargetBitrateKbps, &a.OutputPath, &a.FileSize, &a.Status, &a.ErrorMessage, + &a.Resolution, &a.AudioTrackIndex, &a.TargetBitrateKbps, &a.OutputPath, + &a.OriginNodeID, &a.OriginNodeURL, &a.OriginNodeGroup, &a.OriginArtifactID, &a.FileSize, &a.Status, &a.ErrorMessage, &a.Attempts, &a.MaxAttempts, &leaseOwner, &a.LeaseExpiresAt, &a.NextRetryAt, &a.CreatedAt, &a.CompletedAt, &a.LastUsedAt, ); err != nil { @@ -149,14 +163,15 @@ func (r *ArtifactRepository) Heartbeat(ctx context.Context, id, owner string, le // another node — cannot flip a row it no longer owns. Returns false when the // fence rejected the write (the lease was lost); the caller must then NOT flip // linked downloads, leaving that to the current owner. -func (r *ArtifactRepository) MarkReady(ctx context.Context, id, owner, outputPath string, fileSize int64) (bool, error) { +func (r *ArtifactRepository) MarkReady(ctx context.Context, id, owner, outputPath string, originNodeID int, originNodeURL, originNodeGroup, originArtifactID string, fileSize int64) (bool, error) { tag, err := r.pool.Exec(ctx, `UPDATE download_artifacts - SET status = 'ready', output_path = $2, file_size = $3, error_message = '', + SET status = 'ready', output_path = $2, origin_node_id = $3, origin_node_url = $4, + origin_node_group = $5, origin_artifact_id = $6, file_size = $7, error_message = '', completed_at = now(), last_used_at = now(), lease_owner = NULL, lease_expires_at = NULL, next_retry_at = NULL - WHERE id = $1 AND lease_owner = $4 AND status = 'running'`, - id, outputPath, fileSize, owner, + WHERE id = $1 AND lease_owner = $8 AND status = 'running'`, + id, outputPath, originNodeID, originNodeURL, originNodeGroup, originArtifactID, fileSize, owner, ) if err != nil { return false, fmt.Errorf("marking artifact ready: %w", err) @@ -237,7 +252,8 @@ func (r *ArtifactRepository) Requeue(ctx context.Context, id string) error { tag, err := r.pool.Exec(ctx, `UPDATE download_artifacts SET status = 'queued', attempts = 0, error_message = '', next_retry_at = NULL, - lease_owner = NULL, lease_expires_at = NULL, completed_at = NULL + lease_owner = NULL, lease_expires_at = NULL, completed_at = NULL, + origin_node_id = 0, origin_node_url = '', origin_node_group = '', origin_artifact_id = '' WHERE id = $1`, id, ) @@ -250,6 +266,79 @@ func (r *ArtifactRepository) Requeue(ctx context.Context, id string) error { return nil } +// RequeueRemote atomically transfers a ready remote locator into the cleanup +// queue and clears it from the artifact row. The locator can therefore never +// be deleted while the row still advertises it if either database write fails. +// applied is false when the row changed concurrently or no longer exists. +func (r *ArtifactRepository) RequeueRemote(ctx context.Context, artifact *Artifact) (linked []*Download, applied bool, err error) { + return r.requeueRemote(ctx, artifact, false) +} + +// RequeueRemoteExactLocator additionally fences on the origin URL. Proxy miss +// reports use it so an older signed URL cannot requeue a row after an +// administrator has moved the same node/artifact locator to a new endpoint. +func (r *ArtifactRepository) RequeueRemoteExactLocator(ctx context.Context, artifact *Artifact) (linked []*Download, applied bool, err error) { + return r.requeueRemote(ctx, artifact, true) +} + +func (r *ArtifactRepository) requeueRemote(ctx context.Context, artifact *Artifact, fenceURL bool) (linked []*Download, applied bool, err error) { + if artifact == nil { + return nil, false, nil + } + tx, err := r.pool.Begin(ctx) + if err != nil { + return nil, false, fmt.Errorf("beginning remote artifact requeue: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + query := `UPDATE download_artifacts + SET status = 'queued', attempts = 0, error_message = '', next_retry_at = NULL, + lease_owner = NULL, lease_expires_at = NULL, completed_at = NULL, + origin_node_id = 0, origin_node_url = '', origin_node_group = '', origin_artifact_id = '' + WHERE id = $1 AND status = 'ready' AND origin_node_id = $2 AND origin_artifact_id = $3` + args := []any{artifact.ID, artifact.OriginNodeID, artifact.OriginArtifactID} + if fenceURL { + query += ` AND origin_node_url = $4` + args = append(args, artifact.OriginNodeURL) + } + tag, err := tx.Exec(ctx, query, args...) + if err != nil { + return nil, false, fmt.Errorf("requeuing remote artifact: %w", err) + } + if tag.RowsAffected() == 0 { + return nil, false, nil + } + rows, err := tx.Query(ctx, + `UPDATE downloads + SET status = 'preparing', bytes_sent = 0, completed_at = NULL, + error_message = '', updated_at = now() + WHERE artifact_id = $1 AND status NOT IN ('cancelled', 'failed', 'revoked') + RETURNING `+downloadColumns, + artifact.ID, + ) + if err != nil { + return nil, false, fmt.Errorf("resetting linked downloads for remote artifact requeue: %w", err) + } + linked, err = scanDownloads(rows) + rows.Close() + if err != nil { + return nil, false, fmt.Errorf("scanning reset downloads for remote artifact requeue: %w", err) + } + if _, err := tx.Exec(ctx, + `INSERT INTO download_artifact_orphans (download_artifact_id, origin_node_id, origin_node_url, origin_artifact_id) + VALUES ($1, $2, $3, $4) + ON CONFLICT (origin_node_id, origin_artifact_id) DO UPDATE + SET download_artifact_id = EXCLUDED.download_artifact_id, + origin_node_url = EXCLUDED.origin_node_url, next_retry_at = NULL`, + artifact.ID, artifact.OriginNodeID, artifact.OriginNodeURL, artifact.OriginArtifactID, + ); err != nil { + return nil, false, fmt.Errorf("enqueueing remote artifact cleanup during requeue: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return nil, false, fmt.Errorf("committing remote artifact requeue: %w", err) + } + return linked, true, nil +} + // TouchLastUsed bumps last_used_at for LRU accounting (called on serve). func (r *ArtifactRepository) TouchLastUsed(ctx context.Context, id string) error { _, err := r.pool.Exec(ctx, `UPDATE download_artifacts SET last_used_at = now() WHERE id = $1`, id) @@ -259,6 +348,26 @@ func (r *ArtifactRepository) TouchLastUsed(ctx context.Context, id string) error return nil } +// RefreshRemoteLocator persists an enabled node's current URL/group while +// fencing against a concurrent requeue or replacement locator. +func (r *ArtifactRepository) RefreshRemoteLocator(ctx context.Context, artifact *Artifact) (bool, error) { + if artifact == nil { + return false, nil + } + tag, err := r.pool.Exec(ctx, + `UPDATE download_artifacts + SET origin_node_url = $2, origin_node_group = $3 + WHERE id = $1 AND status = 'ready' + AND origin_node_id = $4 AND origin_artifact_id = $5`, + artifact.ID, artifact.OriginNodeURL, artifact.OriginNodeGroup, + artifact.OriginNodeID, artifact.OriginArtifactID, + ) + if err != nil { + return false, fmt.Errorf("refreshing remote artifact locator: %w", err) + } + return tag.RowsAffected() == 1, nil +} + // ListReady returns ready artifacts ordered by least-recently-used first. func (r *ArtifactRepository) ListReady(ctx context.Context) ([]*Artifact, error) { rows, err := r.pool.Query(ctx, @@ -346,3 +455,111 @@ func (r *ArtifactRepository) DeleteArtifact(ctx context.Context, id string) erro } return nil } + +// EnqueueRemoteOrphan durably records an abandoned attempt before its owning +// artifact lease or locator is discarded. +func (r *ArtifactRepository) EnqueueRemoteOrphan(ctx context.Context, downloadArtifactID string, nodeID int, nodeURL, artifactID string) error { + _, err := r.pool.Exec(ctx, + `INSERT INTO download_artifact_orphans (download_artifact_id, origin_node_id, origin_node_url, origin_artifact_id, next_retry_at) + VALUES ($1, $2, $3, $4, now() + interval '30 seconds') + ON CONFLICT (origin_node_id, origin_artifact_id) DO UPDATE + SET download_artifact_id = EXCLUDED.download_artifact_id, + origin_node_url = EXCLUDED.origin_node_url, + next_retry_at = EXCLUDED.next_retry_at`, + downloadArtifactID, nodeID, nodeURL, artifactID, + ) + if err != nil { + return fmt.Errorf("enqueueing remote artifact cleanup: %w", err) + } + return nil +} + +// ListRemoteOrphansDue interleaves due candidates across origins so a large +// unreachable-node backlog cannot starve cleanup for healthy origins while a +// single healthy origin can still use the full batch. +func (r *ArtifactRepository) ListRemoteOrphansDue(ctx context.Context, limit int) ([]RemoteArtifactOrphan, error) { + if limit <= 0 { + limit = 100 + } + rows, err := r.pool.Query(ctx, + `WITH due AS ( + SELECT id, download_artifact_id, origin_node_id, origin_node_url, origin_artifact_id, attempts, + row_number() OVER ( + PARTITION BY origin_node_id, origin_node_url + ORDER BY attempts, created_at, id + ) AS origin_rank, + created_at + FROM download_artifact_orphans + WHERE next_retry_at IS NULL OR next_retry_at <= now() + ) + SELECT id, download_artifact_id, origin_node_id, origin_node_url, origin_artifact_id, attempts + FROM due + ORDER BY origin_rank, attempts, created_at, id + LIMIT $1`, limit) + if err != nil { + return nil, fmt.Errorf("listing remote artifact cleanup queue: %w", err) + } + defer rows.Close() + out := make([]RemoteArtifactOrphan, 0, limit) + for rows.Next() { + var orphan RemoteArtifactOrphan + if err := rows.Scan(&orphan.ID, &orphan.DownloadArtifactID, &orphan.OriginNodeID, &orphan.OriginNodeURL, &orphan.OriginArtifactID, &orphan.Attempts); err != nil { + return nil, fmt.Errorf("scanning remote artifact cleanup row: %w", err) + } + out = append(out, orphan) + } + return out, rows.Err() +} + +// PrepareRemoteOrphanCleanup serializes cleanup against requeue and verifies +// that a locator from an indeterminate MarkReady result did not actually win +// the artifact row. owned locators have their stale cleanup row removed; +// abandoned locators are retry-leased before the caller performs remote I/O. +func (r *ArtifactRepository) PrepareRemoteOrphanCleanup(ctx context.Context, orphan RemoteArtifactOrphan) (owned, claimed bool, err error) { + tx, err := r.pool.Begin(ctx) + if err != nil { + return false, false, fmt.Errorf("beginning remote artifact cleanup claim: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + var originNodeID int + var originArtifactID string + err = tx.QueryRow(ctx, + `SELECT origin_node_id, origin_artifact_id + FROM download_artifacts WHERE id = $1 FOR UPDATE`, + orphan.DownloadArtifactID, + ).Scan(&originNodeID, &originArtifactID) + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + return false, false, fmt.Errorf("checking remote artifact ownership: %w", err) + } + var present int64 + if err := tx.QueryRow(ctx, `SELECT id FROM download_artifact_orphans WHERE id = $1 FOR UPDATE`, orphan.ID).Scan(&present); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return false, false, nil + } + return false, false, fmt.Errorf("locking remote artifact cleanup row: %w", err) + } + owned = originNodeID == orphan.OriginNodeID && originArtifactID == orphan.OriginArtifactID + if owned { + if _, err := tx.Exec(ctx, `DELETE FROM download_artifact_orphans WHERE id = $1`, orphan.ID); err != nil { + return false, false, fmt.Errorf("clearing owned remote artifact cleanup row: %w", err) + } + } else { + if _, err := tx.Exec(ctx, + `UPDATE download_artifact_orphans + SET attempts = attempts + 1, next_retry_at = now() + interval '2 minutes' + WHERE id = $1`, orphan.ID); err != nil { + return false, false, fmt.Errorf("claiming remote artifact cleanup row: %w", err) + } + } + if err := tx.Commit(ctx); err != nil { + return false, false, fmt.Errorf("committing remote artifact cleanup claim: %w", err) + } + return owned, true, nil +} + +func (r *ArtifactRepository) DeleteRemoteOrphan(ctx context.Context, id int64) error { + if _, err := r.pool.Exec(ctx, `DELETE FROM download_artifact_orphans WHERE id = $1`, id); err != nil { + return fmt.Errorf("deleting remote artifact cleanup row: %w", err) + } + return nil +} diff --git a/internal/downloads/artifact_repo_test.go b/internal/downloads/artifact_repo_test.go index 3087092b..7bb049a3 100644 --- a/internal/downloads/artifact_repo_test.go +++ b/internal/downloads/artifact_repo_test.go @@ -9,7 +9,9 @@ import ( "github.com/jackc/pgx/v5/pgxpool" + "github.com/Silo-Server/silo-server/internal/downloadprepare" "github.com/Silo-Server/silo-server/internal/idgen" + "github.com/Silo-Server/silo-server/internal/models" ) func newArtifactTestRepo(t *testing.T) (*ArtifactRepository, *pgxpool.Pool, int) { @@ -23,6 +25,12 @@ func newArtifactTestRepo(t *testing.T) (*ArtifactRepository, *pgxpool.Pool, int) if present == nil { t.Skip("download_artifacts migration has not been applied") } + if err := pool.QueryRow(ctx, `SELECT to_regclass('public.download_artifact_orphans')::text`).Scan(&present); err != nil { + t.Fatalf("check download_artifact_orphans: %v", err) + } + if present == nil { + t.Skip("download_artifact_orphans migration has not been applied") + } suffix := time.Now().UnixNano() var folderID int @@ -60,6 +68,16 @@ func newArtifact(t *testing.T, fileID int, hash string) *Artifact { } } +func remoteOrphansForArtifact(orphans []RemoteArtifactOrphan, artifactID string) []RemoteArtifactOrphan { + filtered := make([]RemoteArtifactOrphan, 0, 1) + for _, orphan := range orphans { + if orphan.DownloadArtifactID == artifactID { + filtered = append(filtered, orphan) + } + } + return filtered +} + // TestArtifactQueueClaimAndLeaseRecovery is the Phase 3 / invariant-3 acceptance // test: a crash mid-encode (an expired lease) is recovered on the next sweep so // the job re-enqueues and reaches ready, and concurrent workers never claim the @@ -110,7 +128,7 @@ func TestArtifactQueueClaimAndLeaseRecovery(t *testing.T) { if err != nil || claim2.ID != row.ID || claim2.Attempts != 2 { t.Fatalf("reclaim ClaimNext = (%+v, %v), want attempts=2", claim2, err) } - if applied, err := repo.MarkReady(ctx, row.ID, "worker-2", claim2.OutputPath, 4242); err != nil || !applied { + if applied, err := repo.MarkReady(ctx, row.ID, "worker-2", claim2.OutputPath, 0, "", "", "", 4242); err != nil || !applied { t.Fatalf("MarkReady = (%v, %v), want (true, nil)", applied, err) } done, err := repo.GetByKey(ctx, fileID, "transcode", "hash-recovery") @@ -181,7 +199,7 @@ func TestArtifactMarkFencedByOwner(t *testing.T) { } // A non-owner cannot mark the job ready or failed. - if applied, err := repo.MarkReady(ctx, row.ID, "owner-2", "/tmp/x.mp4", 10); err != nil || applied { + if applied, err := repo.MarkReady(ctx, row.ID, "owner-2", "/tmp/x.mp4", 0, "", "", "", 10); err != nil || applied { t.Fatalf("MarkReady(non-owner) = (%v, %v), want (false, nil)", applied, err) } if _, applied, err := repo.MarkFailedOrRetry(ctx, row.ID, "owner-2", "boom", time.Second); err != nil || applied { @@ -195,11 +213,511 @@ func TestArtifactMarkFencedByOwner(t *testing.T) { } // The real owner succeeds. - if applied, err := repo.MarkReady(ctx, row.ID, "owner-1", "/tmp/x.mp4", 10); err != nil || !applied { + if applied, err := repo.MarkReady(ctx, row.ID, "owner-1", "/tmp/x.mp4", 0, "", "", "", 10); err != nil || !applied { t.Fatalf("MarkReady(owner) = (%v, %v), want (true, nil)", applied, err) } } +func TestArtifactRemoteLocatorRoundTripsAndRequeueClearsIt(t *testing.T) { + repo, _, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, "hash-remote-locator")) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", "", 17, "http://transcode", "host-a", "artifact-opaque", 4242); err != nil || !applied { + t.Fatalf("MarkReady = (%v, %v)", applied, err) + } + ready, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if ready.OutputPath != "" || ready.OriginNodeID != 17 || ready.OriginNodeURL != "http://transcode" || ready.OriginNodeGroup != "host-a" || ready.OriginArtifactID != "artifact-opaque" || ready.FileSize != 4242 { + t.Fatalf("ready artifact = %+v", ready) + } + ready.OriginNodeURL = "http://transcode-new" + ready.OriginNodeGroup = "host-new" + if applied, err := repo.RefreshRemoteLocator(ctx, ready); err != nil || !applied { + t.Fatalf("RefreshRemoteLocator = (%v, %v)", applied, err) + } + refreshed, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if refreshed.OriginNodeURL != "http://transcode-new" || refreshed.OriginNodeGroup != "host-new" { + t.Fatalf("refreshed artifact = %+v", refreshed) + } + if err := repo.Requeue(ctx, row.ID); err != nil { + t.Fatal(err) + } + queued, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if queued.OriginNodeID != 0 || queued.OriginNodeURL != "" || queued.OriginNodeGroup != "" || queued.OriginArtifactID != "" { + t.Fatalf("requeued artifact retained remote locator: %+v", queued) + } +} + +func TestArtifactReadyPersistsRefreshedOriginLocator(t *testing.T) { + repo, _, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, "hash-refresh-origin-locator")) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", "", 17, "http://old-url", "old-group", "artifact-refresh", 4242); err != nil || !applied { + t.Fatalf("MarkReady = (%v, %v)", applied, err) + } + manager := &ArtifactManager{ + repo: repo, + preparer: &lifecycleTestPreparer{ + resolvedURL: "http://new-url", resolvedGroup: "new-group", + }, + } + resolved, err := manager.Ready(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if resolved.OriginNodeURL != "http://new-url" || resolved.OriginNodeGroup != "new-group" { + t.Fatalf("resolved artifact = %+v", resolved) + } + persisted, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if persisted.OriginNodeURL != "http://new-url" || persisted.OriginNodeGroup != "new-group" { + t.Fatalf("persisted artifact = %+v", persisted) + } +} + +func TestArtifactReadyMapsRemovedOriginToInactiveWhenRequeueLosesFence(t *testing.T) { + repo, _, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, "hash-removed-origin-stale-fence")) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", "", 17, "http://removed", "host-a", "artifact-removed", 4242); err != nil || !applied { + t.Fatalf("MarkReady = (%v, %v)", applied, err) + } + manager := &ArtifactManager{ + repo: repo, + preparer: &lifecycleTestPreparer{ + resolvedNode: 18, + resolveErr: ErrArtifactOriginRemoved, + }, + } + _, err = manager.Ready(ctx, row.ID) + if !errors.Is(err, ErrDownloadNotActive) || !errors.Is(err, ErrArtifactOriginRemoved) { + t.Fatalf("Ready error = %v, want inactive removed-origin error", err) + } +} + +func TestArtifactRecoveryContinuesAfterStaleLocatorRefresh(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + ready := make([]*Artifact, 0, 2) + for i := range 2 { + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-stale-locator-batch-%d", i))) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", "", 17, "http://old-url", "host-a", fmt.Sprintf("artifact-%d", i), 4242); err != nil || !applied { + t.Fatalf("MarkReady(%d) = (%v, %v)", i, applied, err) + } + artifact, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + ready = append(ready, artifact) + } + + // Simulate another worker replacing the first locator after this recovery + // pass read it. Its refresh must lose the fence without suppressing checks + // for the remaining artifacts on the still-healthy origin. + if _, err := pool.Exec(ctx, + `UPDATE download_artifacts SET origin_artifact_id = 'artifact-replaced' WHERE id = $1`, + ready[0].ID, + ); err != nil { + t.Fatal(err) + } + preparer := &lifecycleTestPreparer{ + resolvedURL: "http://new-url", resolvedGroup: "host-new", + stat: downloadprepare.Result{FileSize: 4242}, + } + manager := &ArtifactManager{repo: repo, preparer: preparer} + manager.probeRemoteArtifactGroup(ctx, preparer, ready) + + if len(preparer.statIDs) != 1 || preparer.statIDs[0] != ready[1].ID { + t.Fatalf("probed artifacts = %v, want only continuing artifact %s", preparer.statIDs, ready[1].ID) + } + persisted, err := repo.GetByID(ctx, ready[1].ID) + if err != nil { + t.Fatal(err) + } + if persisted.OriginNodeURL != "http://new-url" || persisted.OriginNodeGroup != "host-new" { + t.Fatalf("continued artifact locator = %+v", persisted) + } +} + +func TestArtifactInvalidRemoteResultQueuesCleanup(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, "hash-invalid-remote-result")) + if err != nil { + t.Fatal(err) + } + claimed, err := repo.ClaimNext(ctx, "worker", time.Minute) + if err != nil { + t.Fatal(err) + } + preparer := &lifecycleTestPreparer{prepared: PreparedArtifact{ + OriginNodeID: 17, OriginNodeURL: "http://transcode", OriginNodeGroup: "host-a", + OriginArtifactID: "artifact-zero-byte", FileSize: 0, + }} + manager := &ArtifactManager{ + repo: repo, owner: "worker", fileRepo: fakeFileResolver{file: &models.MediaFile{ID: fileID, FilePath: "/media/movie.mkv"}}, + preparer: preparer, + } + manager.encodeOne(ctx, claimed) + + queued, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if queued.Status != ArtifactQueued { + t.Fatalf("artifact status = %q, want queued retry", queued.Status) + } + var orphanID int64 + if err := pool.QueryRow(ctx, + `SELECT id FROM download_artifact_orphans + WHERE download_artifact_id = $1 AND origin_node_id = 17 AND origin_artifact_id = 'artifact-zero-byte'`, + row.ID, + ).Scan(&orphanID); err != nil { + t.Fatalf("remote cleanup row: %v", err) + } + t.Cleanup(func() { _ = repo.DeleteRemoteOrphan(ctx, orphanID) }) +} + +func TestArtifactRecoveryDeletesWrongSizedRemoteBeforeRequeue(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, "hash-remote-size-mismatch")) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", row.OutputPath, 17, "http://transcode", "host-a", "artifact-truncated", 4242); err != nil || !applied { + t.Fatalf("MarkReady = (%v, %v)", applied, err) + } + + preparer := &lifecycleTestPreparer{stat: downloadprepare.Result{ArtifactID: "artifact-truncated", FileSize: 42}} + manager := &ArtifactManager{ + repo: repo, + downloads: NewRepository(pool), + preparer: preparer, + } + manager.recover(ctx) + + got, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if got.Status != ArtifactQueued || got.OriginArtifactID != "" { + t.Fatalf("recovered artifact = %+v, want queued without remote locator", got) + } + orphans, err := repo.ListRemoteOrphansDue(ctx, 10) + if err != nil { + t.Fatal(err) + } + orphans = remoteOrphansForArtifact(orphans, row.ID) + if len(orphans) != 1 || orphans[0].OriginNodeID != 17 || orphans[0].OriginArtifactID != "artifact-truncated" { + t.Fatalf("remote cleanup queue = %+v", orphans) + } + if preparer.deleted != "artifact-truncated" { + t.Fatalf("deleted remote artifact = %q, want artifact-truncated", preparer.deleted) + } + t.Cleanup(func() { _ = repo.DeleteRemoteOrphan(ctx, orphans[0].ID) }) +} + +func TestRemoteRecoveryBudgetBoundsUnreachableOrigins(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + for i := range 5 { + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-recovery-budget-%d-%d", time.Now().UnixNano(), i))) + if err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, + `UPDATE download_artifacts + SET status = 'ready', file_size = 42, completed_at = now(), + origin_node_id = $2, origin_node_url = $3, origin_artifact_id = $4 + WHERE id = $1`, + row.ID, 32000+i, fmt.Sprintf("http://unreachable-recovery-%d", i), fmt.Sprintf("artifact-blocked-%d", i), + ); err != nil { + t.Fatal(err) + } + } + preparer := &lifecycleTestPreparer{statWait: true} + manager := &ArtifactManager{repo: repo, preparer: preparer, remoteRecoveryBudget: 50 * time.Millisecond} + begin := time.Now() + manager.recoverReadyArtifacts(ctx) + if elapsed := time.Since(begin); elapsed > time.Second { + t.Fatalf("recovery elapsed = %s, want bounded return", elapsed) + } + if started := preparer.statStarted.Load(); started == 0 || started > 4 { + t.Fatalf("started probes = %d, want one bounded wave of at most four", started) + } +} + +func TestArtifactRemoteRequeueAtomicallyQueuesCleanup(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-remote-atomic-requeue-%d", time.Now().UnixNano()))) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", row.OutputPath, 23, "http://transcode-old", "host-a", "artifact-abandoned", 4242); err != nil || !applied { + t.Fatalf("MarkReady = (%v, %v)", applied, err) + } + ready, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + var userID int + if err := pool.QueryRow(ctx, + `INSERT INTO users (username, role, download_allowed) VALUES ($1, 'user', true) RETURNING id`, + fmt.Sprintf("requeue-user-%d", time.Now().UnixNano()), + ).Scan(&userID); err != nil { + t.Fatal(err) + } + downloadID := fmt.Sprintf("requeue-download-%d", time.Now().UnixNano()) + if err := NewRepository(pool).Create(ctx, &Download{ + ID: downloadID, UserID: userID, MediaFileID: fileID, + ContentID: "requeue-content", Kind: KindQueued, Status: StatusCompleted, + Format: FormatTranscode, ArtifactID: row.ID, FileSize: ready.FileSize, + CreatedAt: time.Now(), UpdatedAt: time.Now(), + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _, _ = pool.Exec(ctx, `DELETE FROM downloads WHERE id = $1`, downloadID) + _, _ = pool.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }) + // An indeterminate MarkReady result may enqueue the locator even though its + // database write committed. Cleanup must recognize the winning locator and + // remove only the stale queue row, never the node-local bytes. + if err := repo.EnqueueRemoteOrphan(ctx, row.ID, ready.OriginNodeID, ready.OriginNodeURL, ready.OriginArtifactID); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, `UPDATE download_artifact_orphans SET next_retry_at = NULL WHERE origin_node_id = $1 AND origin_artifact_id = $2`, ready.OriginNodeID, ready.OriginArtifactID); err != nil { + t.Fatal(err) + } + orphans, err := repo.ListRemoteOrphansDue(ctx, 10) + orphans = remoteOrphansForArtifact(orphans, row.ID) + if err != nil || len(orphans) != 1 { + t.Fatalf("uncertain cleanup queue = %+v (%v)", orphans, err) + } + owned, claimed, err := repo.PrepareRemoteOrphanCleanup(ctx, orphans[0]) + if err != nil || !claimed || !owned { + t.Fatalf("PrepareRemoteOrphanCleanup = (owned=%v claimed=%v err=%v)", owned, claimed, err) + } + if left, err := repo.ListRemoteOrphansDue(ctx, 10); err != nil || len(remoteOrphansForArtifact(left, row.ID)) != 0 { + t.Fatalf("owned cleanup rows left = %+v (%v)", left, err) + } + // The in-memory URL may already have followed an administrative edit; the + // transaction deliberately matches stable node/id fields and stores the + // refreshed URL as the cleanup target. + ready.OriginNodeURL = "http://transcode-new" + linked, applied, err := repo.RequeueRemote(ctx, ready) + if err != nil || !applied { + t.Fatalf("RequeueRemote = (%v, %v)", applied, err) + } + if len(linked) != 1 || linked[0].ID != downloadID || linked[0].Status != StatusPreparing { + t.Fatalf("reset linked downloads = %+v", linked) + } + queued, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if queued.Status != ArtifactQueued || queued.OriginArtifactID != "" { + t.Fatalf("queued artifact = %+v", queued) + } + orphans, err = repo.ListRemoteOrphansDue(ctx, 10) + if err != nil { + t.Fatal(err) + } + orphans = remoteOrphansForArtifact(orphans, row.ID) + if len(orphans) != 1 || orphans[0].DownloadArtifactID != row.ID || orphans[0].OriginNodeURL != "http://transcode-new" || orphans[0].OriginArtifactID != "artifact-abandoned" { + t.Fatalf("remote cleanup queue = %+v", orphans) + } + t.Cleanup(func() { _ = repo.DeleteRemoteOrphan(ctx, orphans[0].ID) }) + // A stale caller cannot enqueue a second cleanup after the row changed. + if _, applied, err := repo.RequeueRemote(ctx, ready); err != nil || applied { + t.Fatalf("stale RequeueRemote = (%v, %v), want false, nil", applied, err) + } +} + +func TestProxyMissingReportFencesCompleteRemoteLocator(t *testing.T) { + repo, _, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-proxy-missing-report-%d", time.Now().UnixNano()))) + if err != nil { + t.Fatal(err) + } + if _, err := repo.ClaimNext(ctx, "worker", time.Minute); err != nil { + t.Fatal(err) + } + if applied, err := repo.MarkReady(ctx, row.ID, "worker", "", 23, "http://transcode-current", "host-a", "artifact-missing", 4242); err != nil || !applied { + t.Fatalf("MarkReady = (%v, %v)", applied, err) + } + manager := NewArtifactManager(repo, nil, nil, nil, "proxy-test", nil, nil) + stillReady, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + stale := *stillReady + stale.OriginNodeURL = "http://transcode-stale" + if applied, err := manager.requeueRemoteArtifactExactNow(ctx, &stale, "stale API miss"); err != nil || applied { + t.Fatalf("stale exact requeue = (%v, %v), want false, nil", applied, err) + } + if err := manager.ReportRemoteArtifactMissing(ctx, row.ID, "http://transcode-stale", "artifact-missing"); err != nil { + t.Fatal(err) + } + stillReady, err = repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if stillReady.Status != ArtifactReady { + t.Fatalf("stale report changed artifact = %+v", stillReady) + } + if err := manager.ReportRemoteArtifactMissing(ctx, row.ID, "http://transcode-current", "artifact-missing"); err != nil { + t.Fatal(err) + } + queued, err := repo.GetByID(ctx, row.ID) + if err != nil { + t.Fatal(err) + } + if queued.Status != ArtifactQueued || queued.OriginArtifactID != "" { + t.Fatalf("artifact after authoritative missing report = %+v", queued) + } + orphans, err := repo.ListRemoteOrphansDue(ctx, 10) + if err != nil { + t.Fatal(err) + } + orphans = remoteOrphansForArtifact(orphans, row.ID) + if len(orphans) != 1 || orphans[0].DownloadArtifactID != row.ID || orphans[0].OriginArtifactID != "artifact-missing" { + t.Fatalf("remote cleanup queue = %+v", orphans) + } + t.Cleanup(func() { _ = repo.DeleteRemoteOrphan(ctx, orphans[0].ID) }) +} + +func TestRemoteCleanupBudgetBoundsUnreachableOrigins(t *testing.T) { + repo, _, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-cleanup-budget-%d", time.Now().UnixNano()))) + if err != nil { + t.Fatal(err) + } + suffix := time.Now().UnixNano() + originArtifactID := fmt.Sprintf("artifact-blocked-%d", suffix) + originURL := fmt.Sprintf("http://unreachable-%d", suffix) + if err := repo.EnqueueRemoteOrphan(ctx, row.ID, 31, originURL, originArtifactID); err != nil { + t.Fatal(err) + } + started := make(chan struct{}) + preparer := &lifecycleTestPreparer{deleteStarted: started, deleteWait: true} + manager := &ArtifactManager{repo: repo, preparer: preparer, remoteCleanupBudget: 50 * time.Millisecond} + begin := time.Now() + manager.cleanupRemoteOrphans(ctx) + if elapsed := time.Since(begin); elapsed > time.Second { + t.Fatalf("cleanup elapsed = %s, want bounded return", elapsed) + } + select { + case <-started: + default: + t.Fatal("remote deletion was not attempted") + } + orphans, err := repo.ListRemoteOrphansDue(ctx, 100) + if err != nil { + t.Fatal(err) + } + for _, orphan := range orphans { + if orphan.DownloadArtifactID == row.ID { + t.Fatalf("timed-out orphan remained immediately due: %+v", orphan) + } + } +} + +func TestListRemoteOrphansDueIsFairAcrossOrigins(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + row, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-orphan-fairness-%d", time.Now().UnixNano()))) + if err != nil { + t.Fatal(err) + } + suffix := time.Now().UnixNano() + artifactIDs := []string{ + fmt.Sprintf("fair-origin-a-first-%d", suffix), + fmt.Sprintf("fair-origin-a-second-%d", suffix), + fmt.Sprintf("fair-origin-b-%d", suffix), + } + for i, artifactID := range artifactIDs { + nodeID := 31001 + nodeURL := "http://origin-a" + if i == 2 { + nodeID = 31002 + nodeURL = "http://origin-b" + } + if err := repo.EnqueueRemoteOrphan(ctx, row.ID, nodeID, nodeURL, artifactID); err != nil { + t.Fatal(err) + } + } + t.Cleanup(func() { + _, _ = pool.Exec(ctx, `DELETE FROM download_artifact_orphans WHERE origin_artifact_id = ANY($1)`, artifactIDs) + }) + if _, err := pool.Exec(ctx, + `UPDATE download_artifact_orphans + SET next_retry_at = NULL, created_at = '-infinity' + WHERE origin_artifact_id = ANY($1)`, + artifactIDs, + ); err != nil { + t.Fatal(err) + } + + due, err := repo.ListRemoteOrphansDue(ctx, 2) + if err != nil { + t.Fatal(err) + } + seen := make(map[int]int) + for _, orphan := range due { + if orphan.OriginNodeID == 31001 || orphan.OriginNodeID == 31002 { + seen[orphan.OriginNodeID]++ + } + } + if seen[31001] != 1 || seen[31002] != 1 { + t.Fatalf("due candidates by origin = %+v, want one candidate for each origin", seen) + } +} + // TestHasActiveLinkCoversEphemeralRows pins the eviction guard: an ephemeral // (device-less web) download row must protect its artifact from LRU cleanup // exactly like a managed row does, and terminal rows must not. diff --git a/internal/downloads/artifact_test.go b/internal/downloads/artifact_test.go index b5160dd4..9624a6af 100644 --- a/internal/downloads/artifact_test.go +++ b/internal/downloads/artifact_test.go @@ -4,11 +4,14 @@ import ( "context" "encoding/json" "strings" + "sync/atomic" "testing" "time" "github.com/Silo-Server/silo-server/internal/config" + "github.com/Silo-Server/silo-server/internal/downloadprepare" "github.com/Silo-Server/silo-server/internal/models" + "github.com/Silo-Server/silo-server/internal/playback" ) func TestParamsHashStableAndDistinct(t *testing.T) { @@ -49,6 +52,83 @@ func TestArtifactOutputPathDeterministic(t *testing.T) { } } +type lifecycleTestPreparer struct { + deleted string + prepared PreparedArtifact + resolveErr error + resolvedNode int + resolvedURL string + resolvedGroup string + stat downloadprepare.Result + statError error + statIDs []string + statStarted atomic.Int32 + statWait bool + deleteErr error + deleteStarted chan struct{} + deleteWait bool +} + +func (p *lifecycleTestPreparer) PrepareFile(context.Context, string, playback.TranscodeOpts, string) (PreparedArtifact, error) { + return p.prepared, nil +} +func (p *lifecycleTestPreparer) ResolveArtifact(_ context.Context, artifact *Artifact) error { + if p.resolvedNode != 0 { + artifact.OriginNodeID = p.resolvedNode + } + if p.resolvedURL != "" { + artifact.OriginNodeURL = p.resolvedURL + artifact.OriginNodeGroup = p.resolvedGroup + } + return p.resolveErr +} +func (p *lifecycleTestPreparer) StatArtifact(ctx context.Context, artifact *Artifact) (downloadprepare.Result, error) { + if p.statWait { + p.statStarted.Add(1) + select { + case <-ctx.Done(): + return downloadprepare.Result{}, ctx.Err() + case <-time.After(2 * time.Second): + return downloadprepare.Result{}, context.DeadlineExceeded + } + } + p.statIDs = append(p.statIDs, artifact.ID) + return p.stat, p.statError +} +func (p *lifecycleTestPreparer) DeleteArtifact(ctx context.Context, artifact *Artifact) error { + p.deleted = artifact.OriginArtifactID + if p.deleteStarted != nil { + close(p.deleteStarted) + } + if p.deleteWait { + <-ctx.Done() + return ctx.Err() + } + return p.deleteErr +} + +func TestArtifactManagerDeletesRemoteBytesThroughOwningNode(t *testing.T) { + preparer := &lifecycleTestPreparer{} + m := &ArtifactManager{preparer: preparer} + artifact := &Artifact{ID: "row-1", OriginNodeURL: "http://transcode", OriginArtifactID: "opaque-1"} + if !m.deleteArtifactBytes(context.Background(), artifact) { + t.Fatal("remote cleanup failed") + } + if preparer.deleted != "opaque-1" { + t.Fatalf("deleted = %q", preparer.deleted) + } +} + +func TestRemoteArtifactRequeueRejectsNonPositiveNodeID(t *testing.T) { + manager := &ArtifactManager{repo: &ArtifactRepository{}} + _, err := manager.requeueRemoteArtifactNow(context.Background(), &Artifact{ + ID: "row-1", OriginNodeID: -1, OriginNodeURL: "http://transcode", OriginArtifactID: "opaque-1", + }, "invalid node") + if err == nil { + t.Fatal("expected invalid remote locator error") + } +} + func TestEffectiveArtifactDir(t *testing.T) { // Explicit config wins verbatim. if got := effectiveArtifactDir("/data/artifacts", "/tmp/silo-transcode"); got != "/data/artifacts" { diff --git a/internal/downloads/artifacts.go b/internal/downloads/artifacts.go index 1c259051..19d2d10c 100644 --- a/internal/downloads/artifacts.go +++ b/internal/downloads/artifacts.go @@ -6,10 +6,14 @@ import ( "fmt" "log/slog" "os" + "strings" "sync" "time" + "github.com/google/uuid" + "github.com/Silo-Server/silo-server/internal/config" + "github.com/Silo-Server/silo-server/internal/downloadprepare" "github.com/Silo-Server/silo-server/internal/idgen" "github.com/Silo-Server/silo-server/internal/models" "github.com/Silo-Server/silo-server/internal/playback" @@ -21,21 +25,52 @@ const ( artifactMaxAttempts = 3 ) -// EncodePreparer produces a single finalized file for an artifact. The default -// implementation calls playback.PrepareFile; tests substitute a fake. +// PreparedArtifact describes either a local prepared file or an opaque artifact +// retained by a transcode node. +type PreparedArtifact struct { + OutputPath string + OriginNodeID int + OriginNodeURL string + OriginNodeGroup string + OriginArtifactID string + FileSize int64 +} + +func (a PreparedArtifact) Remote() bool { + return a.OriginNodeID > 0 && a.OriginNodeURL != "" && a.OriginArtifactID != "" +} + +// EncodePreparer produces a single finalized artifact. The default +// implementation calls playback.PrepareFile and returns a local path; pooled +// implementations may return an opaque remote locator instead. type EncodePreparer interface { - PrepareFile(ctx context.Context, opts playback.TranscodeOpts, outputPath string) error + PrepareFile(ctx context.Context, artifactID string, opts playback.TranscodeOpts, outputPath string) (PreparedArtifact, error) } type playbackPreparer struct{} -func (playbackPreparer) PrepareFile(ctx context.Context, opts playback.TranscodeOpts, outputPath string) error { - return playback.PrepareFile(ctx, opts, outputPath) +func (playbackPreparer) PrepareFile(ctx context.Context, _ string, opts playback.TranscodeOpts, outputPath string) (PreparedArtifact, error) { + if err := playback.PrepareFile(ctx, opts, outputPath); err != nil { + return PreparedArtifact{}, err + } + stat, err := os.Stat(outputPath) + if err != nil { + return PreparedArtifact{}, fmt.Errorf("stat prepared artifact: %w", err) + } + return PreparedArtifact{OutputPath: outputPath, FileSize: stat.Size()}, nil } // NewPlaybackPreparer returns the production EncodePreparer (ffmpeg-backed). func NewPlaybackPreparer() EncodePreparer { return playbackPreparer{} } +// remoteArtifactLifecycle is implemented by the pooled preparer so recovery +// and quota cleanup can manage node-local artifacts without filesystem access. +type remoteArtifactLifecycle interface { + ResolveArtifact(ctx context.Context, artifact *Artifact) error + StatArtifact(ctx context.Context, artifact *Artifact) (downloadprepare.Result, error) + DeleteArtifact(ctx context.Context, artifact *Artifact) error +} + // ArtifactNotifier publishes an event when a linked download changes state. type ArtifactNotifier func(ctx context.Context, d *Download) @@ -43,13 +78,15 @@ type ArtifactNotifier func(ctx context.Context, d *Download) // jobs, drains them through a bounded worker pool with leased heartbeats, and // recovers stranded jobs on startup. type ArtifactManager struct { - repo *ArtifactRepository - downloads *Repository - fileRepo FileResolver - preparer EncodePreparer - owner string - liveCfg func() *config.Config - notify ArtifactNotifier + repo *ArtifactRepository + downloads *Repository + fileRepo FileResolver + preparer EncodePreparer + owner string + liveCfg func() *config.Config + notify ArtifactNotifier + remoteRecoveryBudget time.Duration + remoteCleanupBudget time.Duration mu sync.Mutex kick func() @@ -99,6 +136,37 @@ func NewArtifactManager( } } +// ReportRemoteArtifactMissing fences a proxy-observed 404 against the signed +// database-row and node locator, then atomically requeues the artifact and its +// linked downloads. Stale tokens are harmless: the exact-locator transition +// only applies while the complete locator still owns the ready row. +func (m *ArtifactManager) ReportRemoteArtifactMissing(ctx context.Context, artifactID, originNodeURL, originArtifactID string) error { + if m == nil || m.repo == nil || strings.TrimSpace(artifactID) == "" || + strings.TrimSpace(originNodeURL) == "" || !downloadprepare.ValidArtifactID(originArtifactID) { + return errors.New("remote artifact locator unavailable for missing report") + } + artifact, err := m.repo.GetByID(ctx, artifactID) + if err != nil { + if errors.Is(err, ErrNotFound) { + return nil + } + return err + } + if artifact.Status != ArtifactReady || artifact.OriginNodeURL != originNodeURL || artifact.OriginArtifactID != originArtifactID { + return nil + } + linked, applied, err := m.repo.RequeueRemoteExactLocator(ctx, artifact) + if err != nil || !applied { + return err + } + for _, download := range linked { + m.publish(ctx, download) + } + slog.WarnContext(ctx, "remote download artifact re-queued", "component", "downloads", "artifact_id", artifact.ID, "node", artifact.OriginNodeURL, "reason", "proxy observed remote output missing") + m.triggerDrain() + return nil +} + // SetKick wires a low-latency drain trigger (e.g. taskmanager RunTask) invoked // when a new job is enqueued. func (m *ArtifactManager) SetKick(kick func()) { @@ -117,6 +185,17 @@ func (m *ArtifactManager) Ready(ctx context.Context, id string) (*Artifact, erro if a.Status != ArtifactReady { return nil, fmt.Errorf("artifact is %s: %w", a.Status, ErrDownloadNotActive) } + if lifecycle, ok := m.preparer.(remoteArtifactLifecycle); ok && a.OriginArtifactID != "" { + if err := m.resolveRemoteArtifact(ctx, lifecycle, a); err != nil { + if !errors.Is(err, ErrArtifactOriginRemoved) { + return nil, err + } + if m.requeueRemoteArtifact(ctx, a, "origin node removed") { + return nil, fmt.Errorf("artifact origin was removed and preparation was requeued: %w", ErrDownloadNotActive) + } + return nil, fmt.Errorf("artifact origin was removed: %w", errors.Join(ErrDownloadNotActive, err)) + } + } _ = m.repo.TouchLastUsed(ctx, id) return a, nil } @@ -214,20 +293,26 @@ func (m *ArtifactManager) triggerDrain() { } } -// RunOnce performs a startup-safe recovery sweep and then drains the queue -// until empty. It is safe to call concurrently across nodes; FOR UPDATE SKIP -// LOCKED prevents double-encoding. +// RunOnce repairs queue state and drains pending work before probing ready +// remote artifacts. Network health checks therefore never block newly queued +// downloads at startup; any artifacts requeued by the bounded probe pass are +// drained in the second pass. func (m *ArtifactManager) RunOnce(ctx context.Context) error { - m.recover(ctx) + m.recoverQueueState(ctx) + if err := m.drain(ctx); err != nil { + return err + } + m.recoverReadyArtifacts(ctx) return m.drain(ctx) } -// recover repairs state a crash or lost lease can leave behind: it reclaims -// expired-lease running rows (back to queued, or failed when attempts are -// exhausted), reconciles linked downloads against terminal artifact states, and -// re-queues ready artifacts whose output file is missing on disk. Safe to run -// repeatedly (each step is idempotent). +// recover is retained as the focused recovery entrypoint used by tests. func (m *ArtifactManager) recover(ctx context.Context) { + m.recoverQueueState(ctx) + m.recoverReadyArtifacts(ctx) +} + +func (m *ArtifactManager) recoverQueueState(ctx context.Context) { if _, err := m.repo.ReclaimExpiredLeases(ctx); err != nil { slog.WarnContext(ctx, "download artifact lease reclaim failed", "component", "downloads", "error", err) } @@ -248,9 +333,13 @@ func (m *ArtifactManager) recover(ctx context.Context) { m.publish(ctx, d) } } +} - // Disk-presence sweep: stats every ready file, so it runs on the startup - // pass and then hourly rather than on every tick. +// recoverReadyArtifacts checks local paths directly and groups remote probes +// by origin. Origins run concurrently, but each origin is short-circuited after +// its first transient failure so one blackholed node costs one bounded timeout, +// never one timeout per artifact. +func (m *ArtifactManager) recoverReadyArtifacts(ctx context.Context) { if !m.maintenanceDue(&m.lastDiskSweep) { return } @@ -259,19 +348,170 @@ func (m *ArtifactManager) recover(ctx context.Context) { slog.WarnContext(ctx, "download artifact ready scan failed", "component", "downloads", "error", err) return } + remoteGroups := make(map[string][]*Artifact) for _, a := range ready { - if a.OutputPath == "" { + if a.OriginArtifactID != "" { + key := fmt.Sprintf("%d\x00%s", a.OriginNodeID, a.OriginNodeURL) + remoteGroups[key] = append(remoteGroups[key], a) continue } - if _, statErr := os.Stat(a.OutputPath); statErr != nil { - slog.WarnContext(ctx, "download artifact output missing, re-queuing", "component", "downloads", "artifact_id", a.ID, "path", a.OutputPath) - if err := m.repo.Requeue(ctx, a.ID); err != nil { - slog.WarnContext(ctx, "re-queue artifact failed", "component", "downloads", "artifact_id", a.ID, "error", err) - continue + if a.OutputPath != "" { + if _, statErr := os.Stat(a.OutputPath); statErr != nil { + slog.WarnContext(ctx, "download artifact output missing, re-queuing", "component", "downloads", "artifact_id", a.ID, "path", a.OutputPath) + if err := m.repo.Requeue(ctx, a.ID); err != nil { + slog.WarnContext(ctx, "re-queue artifact failed", "component", "downloads", "artifact_id", a.ID, "error", err) + } } - m.triggerDrain() } } + if len(remoteGroups) == 0 { + return + } + lifecycle, ok := m.preparer.(remoteArtifactLifecycle) + if !ok { + slog.WarnContext(ctx, "remote download artifacts cannot be checked", "component", "downloads") + return + } + budget := m.remoteRecoveryBudget + if budget <= 0 { + budget = defaultRemoteRecoveryBudget + } + recoveryCtx, cancel := context.WithTimeout(ctx, budget) + defer cancel() + const maxConcurrentOriginProbes = 4 + sem := make(chan struct{}, maxConcurrentOriginProbes) + var wg sync.WaitGroup + for _, artifacts := range remoteGroups { + artifacts := artifacts + wg.Add(1) + go func() { + defer wg.Done() + select { + case sem <- struct{}{}: + defer func() { <-sem }() + case <-recoveryCtx.Done(): + return + } + m.probeRemoteArtifactGroup(recoveryCtx, lifecycle, artifacts) + }() + } + wg.Wait() +} + +func (m *ArtifactManager) probeRemoteArtifactGroup(ctx context.Context, lifecycle remoteArtifactLifecycle, artifacts []*Artifact) { + for _, a := range artifacts { + if err := m.resolveRemoteArtifact(ctx, lifecycle, a); err != nil { + if errors.Is(err, ErrArtifactOriginRemoved) { + m.requeueRemoteArtifact(ctx, a, "origin node removed") + continue + } + if errors.Is(err, ErrDownloadNotActive) { + // The row changed concurrently; the origin itself is still healthy. + continue + } + slog.WarnContext(ctx, "remote download artifact resolution failed; skipping remaining origin batch", "component", "downloads", "artifact_id", a.ID, "error", err) + return + } + result, err := lifecycle.StatArtifact(ctx, a) + switch { + case errors.Is(err, ErrArtifactOriginRemoved): + m.requeueRemoteArtifact(ctx, a, "origin node removed") + case errors.Is(err, downloadprepare.ErrArtifactNotFound): + m.requeueRemoteArtifact(ctx, a, "remote output missing") + case err != nil: + // One node-level failure suppresses the rest of this origin's batch. + slog.WarnContext(ctx, "remote download artifact check failed; skipping remaining origin batch", "component", "downloads", "artifact_id", a.ID, "node", a.OriginNodeURL, "error", err) + return + case result.FileSize != a.FileSize: + m.requeueWrongSizedRemoteArtifact(ctx, lifecycle, a) + } + } +} + +// requeueWrongSizedRemoteArtifact commits the durable fence before deleting +// bytes, but delays replacement work until the known-invalid locator has been +// quarantined. If deletion fails, the transaction's orphan row retains the +// exact locator for the regular retrying cleanup pass. +func (m *ArtifactManager) requeueWrongSizedRemoteArtifact(ctx context.Context, lifecycle remoteArtifactLifecycle, a *Artifact) { + applied, err := m.requeueRemoteArtifactWithFence(ctx, a, "remote output size mismatch", false, false) + if err != nil { + slog.WarnContext(ctx, "re-queue remote artifact failed", "component", "downloads", "artifact_id", a.ID, "error", err) + return + } + if !applied { + return + } + if err := lifecycle.DeleteArtifact(ctx, a); err != nil { + slog.WarnContext(ctx, "deleting rejected remote artifact failed", "component", "downloads", "artifact_id", a.ID, "node", a.OriginNodeURL, "error", err) + } + m.triggerDrain() +} + +func (m *ArtifactManager) resolveRemoteArtifact(ctx context.Context, lifecycle remoteArtifactLifecycle, artifact *Artifact) error { + oldURL, oldGroup := artifact.OriginNodeURL, artifact.OriginNodeGroup + if err := lifecycle.ResolveArtifact(ctx, artifact); err != nil { + return err + } + if artifact.OriginNodeURL == oldURL && artifact.OriginNodeGroup == oldGroup { + return nil + } + applied, err := m.repo.RefreshRemoteLocator(ctx, artifact) + if err != nil { + return err + } + if !applied { + return fmt.Errorf("remote artifact changed while refreshing its origin: %w", ErrDownloadNotActive) + } + return nil +} + +func (m *ArtifactManager) requeueRemoteArtifact(ctx context.Context, a *Artifact, reason string) bool { + applied, err := m.requeueRemoteArtifactNow(ctx, a, reason) + if err != nil { + slog.WarnContext(ctx, "re-queue remote artifact failed", "component", "downloads", "artifact_id", a.ID, "error", err) + return false + } + return applied +} + +// requeueRemoteArtifactNow synchronously fences a stale remote locator, resets +// every linked download, and schedules the abandoned node-local file for +// cleanup. Request paths use the returned error so a failed state transition is +// never disguised as an ordinary missing catalog item. +func (m *ArtifactManager) requeueRemoteArtifactNow(ctx context.Context, a *Artifact, reason string) (bool, error) { + return m.requeueRemoteArtifactWithFence(ctx, a, reason, false, true) +} + +func (m *ArtifactManager) requeueRemoteArtifactExactNow(ctx context.Context, a *Artifact, reason string) (bool, error) { + return m.requeueRemoteArtifactWithFence(ctx, a, reason, true, true) +} + +func (m *ArtifactManager) requeueRemoteArtifactWithFence(ctx context.Context, a *Artifact, reason string, exactURL, triggerDrain bool) (bool, error) { + if m == nil || m.repo == nil || a == nil || a.ID == "" || a.OriginNodeID <= 0 || a.OriginArtifactID == "" { + return false, errors.New("remote artifact locator unavailable for requeue") + } + var linked []*Download + var applied bool + var err error + if exactURL { + linked, applied, err = m.repo.RequeueRemoteExactLocator(ctx, a) + } else { + linked, applied, err = m.repo.RequeueRemote(ctx, a) + } + if err != nil { + return false, err + } + if !applied { + return false, nil + } + for _, download := range linked { + m.publish(ctx, download) + } + slog.WarnContext(ctx, "remote download artifact re-queued", "component", "downloads", "artifact_id", a.ID, "node", a.OriginNodeURL, "reason", reason) + if triggerDrain { + m.triggerDrain() + } + return true, nil } // drain claims and encodes jobs through a bounded worker pool until the queue is @@ -332,7 +572,15 @@ func (m *ArtifactManager) encodeOne(ctx context.Context, a *Artifact) { } opts := m.buildOpts(file, a) - if err := m.preparer.PrepareFile(hbCtx, opts, a.OutputPath); err != nil { + // Each lease attempt owns a distinct node-local object. A worker that loses + // the readiness fence can therefore queue its object for deletion without + // racing the replacement worker's output on the same node. + remoteAttemptID := a.ID + "-" + uuid.NewString() + prepared, err := m.preparer.PrepareFile(hbCtx, remoteAttemptID, opts, a.OutputPath) + if err != nil { + if prepared.Remote() { + m.enqueueRemoteCleanup(ctx, a.ID, prepared, true) + } switch { case ctx.Err() != nil: // Parent shutting down: leave the job 'running'; its lease expires and @@ -349,27 +597,35 @@ func (m *ArtifactManager) encodeOne(ctx context.Context, a *Artifact) { } } - fi, err := os.Stat(a.OutputPath) - if err != nil { - // A remote node can report success while writing to a path that is not - // actually shared with the API node. Never publish that artifact: retry - // so another node or the local fallback can produce an observable file. - msg := fmt.Sprintf("prepared artifact output unavailable: %v", err) - slog.WarnContext(ctx, "prepared artifact output unavailable", "component", "downloads", "artifact_id", a.ID, "path", a.OutputPath, "error", err) + remoteFieldsPresent := prepared.OriginNodeID != 0 || prepared.OriginNodeURL != "" || prepared.OriginNodeGroup != "" || prepared.OriginArtifactID != "" + if prepared.FileSize <= 0 || (remoteFieldsPresent && !prepared.Remote()) || (!prepared.Remote() && prepared.OutputPath == "") { + msg := "prepared artifact returned an invalid storage locator" + slog.WarnContext(ctx, "prepared artifact returned an invalid storage locator", "component", "downloads", "artifact_id", a.ID) + if prepared.Remote() { + m.enqueueRemoteCleanup(ctx, a.ID, prepared, true) + } m.failJob(ctx, a, msg) return } - size := fi.Size() + size := prepared.FileSize // Fenced on lease ownership: if we lost the lease between encode and commit, // applied is false and the current owner is responsible for flipping links — // do not flip them here or we would race/duplicate that owner's work. - applied, err := m.repo.MarkReady(ctx, a.ID, m.owner, a.OutputPath, size) + outputPath := prepared.OutputPath + if prepared.Remote() { + // Keep the deterministic API-local fallback path in the row so a later + // remote-missing requeue can safely fall back to integrated preparation. + outputPath = a.OutputPath + } + applied, err := m.repo.MarkReady(ctx, a.ID, m.owner, outputPath, prepared.OriginNodeID, prepared.OriginNodeURL, prepared.OriginNodeGroup, prepared.OriginArtifactID, size) if err != nil { slog.ErrorContext(ctx, "marking artifact ready failed", "component", "downloads", "artifact_id", a.ID, "error", err) + m.cleanupRejectedPrepared(ctx, a.ID, prepared) return } if !applied { slog.WarnContext(ctx, "download artifact ready skipped; lease lost", "component", "downloads", "artifact_id", a.ID) + m.enqueueRemoteCleanup(ctx, a.ID, prepared, true) return } flipped, err := m.downloads.MarkLinkedDownloadsReady(ctx, a.ID, size) @@ -382,6 +638,54 @@ func (m *ArtifactManager) encodeOne(ctx context.Context, a *Artifact) { } } +func (m *ArtifactManager) cleanupRejectedPrepared(ctx context.Context, artifactID string, prepared PreparedArtifact) { + if !prepared.Remote() { + return + } + // Exec can report a transport error after PostgreSQL committed. Re-read the + // row before classifying this attempt as abandoned so cleanup never deletes + // the locator that actually won the readiness fence. + current, err := m.repo.GetByID(ctx, artifactID) + if err == nil { + if current.Status == ArtifactReady && + current.OriginNodeID == prepared.OriginNodeID && + current.OriginArtifactID == prepared.OriginArtifactID { + return + } + } + // A successful reread (or a definitively missing row) proves this locator + // lost. Other database errors leave MarkReady's outcome indeterminate, so a + // failed queue write must not fall back to deleting potentially-owned bytes. + safeImmediateDelete := err == nil || errors.Is(err, ErrNotFound) + m.enqueueRemoteCleanup(ctx, artifactID, prepared, safeImmediateDelete) +} + +func (m *ArtifactManager) enqueueRemoteCleanup(ctx context.Context, artifactID string, prepared PreparedArtifact, safeImmediateDelete bool) bool { + if !prepared.Remote() { + return true + } + cleanupCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) + defer cancel() + if err := m.repo.EnqueueRemoteOrphan(cleanupCtx, artifactID, prepared.OriginNodeID, prepared.OriginNodeURL, prepared.OriginArtifactID); err != nil { + slog.WarnContext(ctx, "persisting abandoned remote artifact failed", "component", "downloads", "artifact_id", prepared.OriginArtifactID, "node", prepared.OriginNodeURL, "error", err) + // A unique attempt ID permits best-effort immediate deletion when the + // caller knows the readiness write did not commit. An indeterminate write + // must retain the bytes if the verification queue is unavailable. + if safeImmediateDelete { + lifecycle, ok := m.preparer.(remoteArtifactLifecycle) + if !ok { + return false + } + artifact := &Artifact{OriginNodeID: prepared.OriginNodeID, OriginNodeURL: prepared.OriginNodeURL, OriginNodeGroup: prepared.OriginNodeGroup, OriginArtifactID: prepared.OriginArtifactID} + if deleteErr := lifecycle.DeleteArtifact(cleanupCtx, artifact); deleteErr == nil { + return true + } + } + return false + } + return true +} + func (m *ArtifactManager) failJob(ctx context.Context, a *Artifact, msg string) { terminal, applied, err := m.repo.MarkFailedOrRetry(ctx, a.ID, m.owner, msg, backoffFor(a.Attempts)) if err != nil { @@ -476,9 +780,11 @@ func (m *ArtifactManager) buildOpts(file *models.MediaFile, a *Artifact) playbac // The server-disk *quota* is download.artifact_max_bytes (see the download // limits & restrictions design); this sweep is not a quota. const ( - failedArtifactRetention = 24 * time.Hour - unlinkedArtifactRetention = 30 * 24 * time.Hour - ephemeralDownloadRetention = 7 * 24 * time.Hour + failedArtifactRetention = 24 * time.Hour + unlinkedArtifactRetention = 30 * 24 * time.Hour + ephemeralDownloadRetention = 7 * 24 * time.Hour + defaultRemoteRecoveryBudget = 15 * time.Second + defaultRemoteCleanupBudget = 15 * time.Second ) // Cleanup runs the hygiene sweep, then evicts ready artifacts (LRU first) once @@ -486,6 +792,7 @@ const ( // active download row (managed or ephemeral) — only artifacts whose links are // all terminal are evictable. func (m *ArtifactManager) Cleanup(ctx context.Context) error { + m.cleanupRemoteOrphans(ctx) m.sweepStale(ctx) budget := m.downloadConfig().ArtifactMaxBytes if budget <= 0 { @@ -514,10 +821,8 @@ func (m *ArtifactManager) Cleanup(ctx context.Context) error { if active { continue } - if a.OutputPath != "" { - if err := os.Remove(a.OutputPath); err != nil && !os.IsNotExist(err) { - slog.WarnContext(ctx, "removing evicted artifact file failed", "component", "downloads", "artifact_id", a.ID, "error", err) - } + if !m.deleteArtifactBytes(ctx, a) { + continue } if err := m.repo.DeleteArtifact(ctx, a.ID); err != nil { slog.WarnContext(ctx, "deleting evicted artifact row failed", "component", "downloads", "artifact_id", a.ID, "error", err) @@ -529,6 +834,68 @@ func (m *ArtifactManager) Cleanup(ctx context.Context) error { return nil } +func (m *ArtifactManager) cleanupRemoteOrphans(ctx context.Context) { + lifecycle, ok := m.preparer.(remoteArtifactLifecycle) + if !ok { + return + } + budget := m.remoteCleanupBudget + if budget <= 0 { + budget = defaultRemoteCleanupBudget + } + cleanupCtx, cancel := context.WithTimeout(ctx, budget) + defer cancel() + orphans, err := m.repo.ListRemoteOrphansDue(cleanupCtx, 100) + if err != nil { + slog.WarnContext(ctx, "listing abandoned remote artifacts failed", "component", "downloads", "error", err) + return + } + groups := make(map[string][]RemoteArtifactOrphan) + for _, orphan := range orphans { + key := fmt.Sprintf("%d\x00%s", orphan.OriginNodeID, orphan.OriginNodeURL) + groups[key] = append(groups[key], orphan) + } + const maxConcurrentOriginDeletes = 4 + sem := make(chan struct{}, maxConcurrentOriginDeletes) + var wg sync.WaitGroup + for _, group := range groups { + group := group + wg.Add(1) + go func() { + defer wg.Done() + select { + case sem <- struct{}{}: + defer func() { <-sem }() + case <-cleanupCtx.Done(): + return + } + for _, orphan := range group { + owned, claimed, err := m.repo.PrepareRemoteOrphanCleanup(cleanupCtx, orphan) + if err != nil { + slog.WarnContext(ctx, "claiming abandoned remote artifact cleanup failed; skipping remaining origin batch", "component", "downloads", "artifact_id", orphan.OriginArtifactID, "error", err) + return + } + if !claimed || owned { + continue + } + artifact := &Artifact{ + OriginNodeID: orphan.OriginNodeID, OriginNodeURL: orphan.OriginNodeURL, + OriginArtifactID: orphan.OriginArtifactID, + } + if err := lifecycle.DeleteArtifact(cleanupCtx, artifact); err != nil { + slog.WarnContext(ctx, "abandoned remote artifact cleanup failed; skipping remaining origin batch", "component", "downloads", "artifact_id", orphan.OriginArtifactID, "node", orphan.OriginNodeURL, "error", err) + return + } + if err := m.repo.DeleteRemoteOrphan(cleanupCtx, orphan.ID); err != nil { + slog.WarnContext(ctx, "clearing abandoned remote artifact cleanup row failed", "component", "downloads", "artifact_id", orphan.OriginArtifactID, "error", err) + return + } + } + }() + } + wg.Wait() +} + // sweepStale is the age-based hygiene pass: cold terminally-failed artifacts // (with their leftover .part files), orphaned ready artifacts no download row // references, and expired ephemeral download rows. Best-effort; every step @@ -564,13 +931,8 @@ func (m *ArtifactManager) sweepStale(ctx context.Context) { // removeArtifact deletes an artifact's output file, its .part leftover, and // its row. Used by the hygiene sweep for rows nothing can serve again. func (m *ArtifactManager) removeArtifact(ctx context.Context, a *Artifact, reason string) { - if a.OutputPath != "" { - if err := os.Remove(a.OutputPath); err != nil && !os.IsNotExist(err) { - slog.WarnContext(ctx, "removing swept artifact file failed", "component", "downloads", "artifact_id", a.ID, "error", err) - } - if err := os.Remove(a.OutputPath + ".part"); err != nil && !os.IsNotExist(err) { - slog.WarnContext(ctx, "removing swept artifact partial failed", "component", "downloads", "artifact_id", a.ID, "error", err) - } + if !m.deleteArtifactBytes(ctx, a) { + return } if err := m.repo.DeleteArtifact(ctx, a.ID); err != nil { slog.WarnContext(ctx, "deleting swept artifact row failed", "component", "downloads", "artifact_id", a.ID, "error", err) @@ -579,6 +941,33 @@ func (m *ArtifactManager) removeArtifact(ctx context.Context, a *Artifact, reaso slog.InfoContext(ctx, "swept stale download artifact", "component", "downloads", "artifact_id", a.ID, "reason", reason, "bytes", a.FileSize) } +func (m *ArtifactManager) deleteArtifactBytes(ctx context.Context, a *Artifact) bool { + if a.OriginArtifactID != "" { + lifecycle, ok := m.preparer.(remoteArtifactLifecycle) + if !ok { + slog.WarnContext(ctx, "remote artifact cleanup unavailable", "component", "downloads", "artifact_id", a.ID) + return false + } + if err := lifecycle.DeleteArtifact(ctx, a); err != nil { + slog.WarnContext(ctx, "removing remote artifact failed", "component", "downloads", "artifact_id", a.ID, "node", a.OriginNodeURL, "error", err) + return false + } + return true + } + if a.OutputPath == "" { + return true + } + if err := os.Remove(a.OutputPath); err != nil && !os.IsNotExist(err) { + slog.WarnContext(ctx, "removing artifact file failed", "component", "downloads", "artifact_id", a.ID, "error", err) + return false + } + if err := os.Remove(a.OutputPath + ".part"); err != nil && !os.IsNotExist(err) { + slog.WarnContext(ctx, "removing artifact partial failed", "component", "downloads", "artifact_id", a.ID, "error", err) + return false + } + return true +} + // backoffFor returns the retry delay for the next attempt after a failure. func backoffFor(attempts int) time.Duration { if attempts < 1 { diff --git a/internal/downloads/bandwidth.go b/internal/downloads/bandwidth.go index b31e693d..3373f4af 100644 --- a/internal/downloads/bandwidth.go +++ b/internal/downloads/bandwidth.go @@ -78,6 +78,16 @@ func (bm *BandwidthManager) getUserLimiter(userID int) *rate.Limiter { // ThrottledReader wraps an io.ReadSeeker with bandwidth limiting for the given user. // If no limits are configured, the original reader is returned as-is. func (bm *BandwidthManager) ThrottledReader(ctx context.Context, r io.ReadSeeker, userID int) io.ReadSeeker { + limited := bm.ThrottledStreamReader(ctx, r, userID) + if limited == r { + return r + } + return &throttledReadSeeker{Reader: limited, seeker: r} +} + +// ThrottledStreamReader applies the same aggregate limits to a non-seekable +// stream, such as an authenticated response relayed from a transcode node. +func (bm *BandwidthManager) ThrottledStreamReader(ctx context.Context, r io.Reader, userID int) io.Reader { if bm == nil { return r } @@ -96,10 +106,18 @@ func (bm *BandwidthManager) ThrottledReader(ctx context.Context, r io.ReadSeeker } } -// throttledReader implements io.ReadSeeker with bandwidth limiting. -// Seek is delegated directly (not throttled), only forward reads are limited. +type throttledReadSeeker struct { + io.Reader + seeker io.Seeker +} + +func (r *throttledReadSeeker) Seek(offset int64, whence int) (int64, error) { + return r.seeker.Seek(offset, whence) +} + +// throttledReader applies bandwidth limiting to forward reads. type throttledReader struct { - r io.ReadSeeker + r io.Reader ctx context.Context serverLim *rate.Limiter userLim *rate.Limiter @@ -139,10 +157,6 @@ func (t *throttledReader) Read(p []byte) (int, error) { return n, err } -func (t *throttledReader) Seek(offset int64, whence int) (int64, error) { - return t.r.Seek(offset, whence) -} - // BytesRead returns the total bytes read through this throttled reader. func (t *throttledReader) BytesRead() int64 { return t.bytesRead diff --git a/internal/downloads/model.go b/internal/downloads/model.go index 6b5b63ba..c58d04e8 100644 --- a/internal/downloads/model.go +++ b/internal/downloads/model.go @@ -79,6 +79,13 @@ var ( ErrInvalidSubtitleRef = errors.New("invalid subtitle reference") ErrAssetNotFound = errors.New("download asset not found") ErrFormatUnavailable = errors.New("requested download format is not available") + // ErrResponseCommitted reports a transfer failure after response headers + // were written. Handlers must not append an API error body, while service + // lifecycle code must still treat the transfer as incomplete. + ErrResponseCommitted = errors.New("download response already committed") + // ErrArtifactOriginRemoved means a durable remote locator points at a node + // no longer present in the enabled transcode pool. + ErrArtifactOriginRemoved = errors.New("download artifact origin node was removed") // ErrQualityUnavailable means the requested quality is valid and permitted in // principle but cannot be fulfilled by the current server wiring. ErrQualityUnavailable = errors.New("requested download quality is not available") diff --git a/internal/downloads/remote_preparer.go b/internal/downloads/remote_preparer.go index 6b624ac1..8be7bbfa 100644 --- a/internal/downloads/remote_preparer.go +++ b/internal/downloads/remote_preparer.go @@ -2,13 +2,11 @@ package downloads import ( "context" + "errors" + "fmt" "log/slog" - "os" - "path/filepath" "strings" - "github.com/google/uuid" - "github.com/Silo-Server/silo-server/internal/config" "github.com/Silo-Server/silo-server/internal/downloadprepare" "github.com/Silo-Server/silo-server/internal/nodepool" @@ -17,12 +15,18 @@ import ( // NodeAwarePreparer keeps artifact queue ownership central while executing the // expensive FFmpeg process on a healthy transcode node when capacity permits. -// Integrated installations and saturated node pools fall back to local work. +// The node retains completed bytes behind an authenticated opaque-id endpoint; +// integrated installations and unavailable pools fall back to local work. type NodeAwarePreparer struct { - local EncodePreparer - planner nodepool.TranscodeWorkPlanner - liveCfg func() *config.Config - remote downloadprepare.RemotePreparer + local EncodePreparer + planner nodepool.TranscodeWorkPlanner + liveCfg func() *config.Config + remote downloadprepare.RemotePreparer + originLookup artifactOriginLookup +} + +type artifactOriginLookup interface { + GetByID(ctx context.Context, id int) (*nodepool.Node, error) } func NewNodeAwarePreparer(local EncodePreparer, planner nodepool.TranscodeWorkPlanner, liveCfg func() *config.Config) *NodeAwarePreparer { @@ -37,47 +41,139 @@ func NewNodeAwarePreparer(local EncodePreparer, planner nodepool.TranscodeWorkPl } } -func (p *NodeAwarePreparer) PrepareFile(ctx context.Context, opts playback.TranscodeOpts, outputPath string) error { +// SetOriginLookup supplies the authoritative node record used when the active +// pool temporarily misses an enabled node, and to recover a changed URL after +// a disabled node has left that pool. +func (p *NodeAwarePreparer) SetOriginLookup(lookup artifactOriginLookup) { + p.originLookup = lookup +} + +func (p *NodeAwarePreparer) PrepareFile(ctx context.Context, artifactID string, opts playback.TranscodeOpts, outputPath string) (PreparedArtifact, error) { cfg := p.config() jwtSecret := "" if cfg != nil { jwtSecret = strings.TrimSpace(cfg.Auth.JWTSecret) } - // The default artifact directory is node-local. Remote preparation is safe - // only when the operator has deliberately configured a shared artifact path. - if cfg == nil || jwtSecret == "" || p.remote == nil || p.planner == nil || strings.TrimSpace(cfg.Download.ArtifactDir) == "" { - return p.local.PrepareFile(ctx, opts, outputPath) + if cfg == nil || jwtSecret == "" || p.remote == nil || p.planner == nil || !downloadprepare.ValidArtifactID(artifactID) { + return p.local.PrepareFile(ctx, artifactID, opts, outputPath) } - jobID := strings.TrimSuffix(filepath.Base(outputPath), filepath.Ext(outputPath)) - node, release := p.planner.ReserveTranscodeWork("download-prepare-" + jobID) + node, release := p.planner.ReserveTranscodeWork("download-prepare-" + artifactID) if node == nil { - return p.local.PrepareFile(ctx, opts, outputPath) + return p.local.PrepareFile(ctx, artifactID, opts, outputPath) } - slog.InfoContext(ctx, "dispatching download artifact prepare", "component", "downloads", "job_id", jobID, "node", node.URL) - // A remote attempt gets its own staging output. If the HTTP connection fails - // while the node is still winding down FFmpeg, a local fallback can safely - // use outputPath.part without the two processes writing the same file. - remoteOutputPath := outputPath + ".remote-" + uuid.NewString() + ".mp4" - defer func() { _ = os.Remove(remoteOutputPath) }() - err := p.remote.Prepare(ctx, node.URL, jwtSecret, downloadprepare.NewRequest(jobID, opts, remoteOutputPath)) + slog.InfoContext(ctx, "dispatching download artifact prepare", "component", "downloads", "artifact_id", artifactID, "node", node.URL) + result, err := p.remote.Prepare(ctx, node.URL, jwtSecret, downloadprepare.NewRequest(artifactID, opts)) release() - // PrepareFile publishes its staging output with an atomic rename, so a - // visible file is complete even if the HTTP response was lost afterward. - if _, statErr := os.Stat(remoteOutputPath); statErr == nil { - if renameErr := os.Rename(remoteOutputPath, outputPath); renameErr == nil { - return nil - } else { - err = renameErr - } - } else if err == nil { - err = statErr + if err == nil && result.ArtifactID == artifactID { + return remotePreparedArtifact(node, result), nil + } + if err == nil { + err = fmt.Errorf("remote download prepare returned artifact id %q, want %q", result.ArtifactID, artifactID) } if ctx.Err() != nil { - return ctx.Err() + return remotePreparedArtifact(node, downloadprepare.Result{ArtifactID: artifactID}), ctx.Err() } - slog.WarnContext(ctx, "remote download artifact prepare unavailable; falling back to local", "component", "downloads", "job_id", jobID, "node", node.URL, "error", err) - return p.local.PrepareFile(ctx, opts, outputPath) + // A completed encode can outlive a lost HTTP response. Probe the same opaque + // id before falling back so retry/recovery does not duplicate expensive work. + if recovered, statErr := p.remote.Stat(ctx, node.URL, jwtSecret, artifactID); statErr == nil && recovered.ArtifactID == artifactID { + slog.InfoContext(ctx, "recovered completed download artifact after lost response", "component", "downloads", "artifact_id", artifactID, "node", node.URL) + return remotePreparedArtifact(node, recovered), nil + } else if statErr == nil { + return remotePreparedArtifact(node, downloadprepare.Result{ArtifactID: artifactID}), + fmt.Errorf("remote download artifact recovery returned artifact id %q, want %q", recovered.ArtifactID, artifactID) + } else if !errors.Is(statErr, downloadprepare.ErrArtifactNotFound) { + slog.WarnContext(ctx, "remote download artifact recovery probe failed", "component", "downloads", "artifact_id", artifactID, "node", node.URL, "error", statErr) + // The POST may have completed even though its response was lost. If the + // follow-up probe is also indeterminate, retry the same opaque id later + // instead of falling back locally and orphaning completed remote bytes. + return remotePreparedArtifact(node, downloadprepare.Result{ArtifactID: artifactID}), errors.Join(err, fmt.Errorf("remote download artifact recovery probe: %w", statErr)) + } + if ctx.Err() != nil { + return PreparedArtifact{}, ctx.Err() + } + slog.WarnContext(ctx, "remote download artifact prepare unavailable; falling back to local", "component", "downloads", "artifact_id", artifactID, "node", node.URL, "error", err) + return p.local.PrepareFile(ctx, artifactID, opts, outputPath) +} + +func remotePreparedArtifact(node *nodepool.Node, result downloadprepare.Result) PreparedArtifact { + group := "" + if node.Group != nil { + group = *node.Group + } + return PreparedArtifact{ + OriginNodeID: node.ID, + OriginNodeURL: strings.TrimRight(node.URL, "/"), + OriginNodeGroup: group, + OriginArtifactID: result.ArtifactID, + FileSize: result.FileSize, + } +} + +func (p *NodeAwarePreparer) ResolveArtifact(ctx context.Context, artifact *Artifact) error { + if artifact == nil || artifact.OriginNodeID == 0 || p.planner == nil { + return ErrArtifactOriginRemoved + } + node, ok := p.planner.TranscodeNode(artifact.OriginNodeID) + if !ok || node == nil { + if p.originLookup != nil { + configured, err := p.originLookup.GetByID(ctx, artifact.OriginNodeID) + switch { + case err == nil && configured != nil && configured.Type == nodepool.NodeTypeTranscode: + applyArtifactOrigin(artifact, configured) + if configured.Enabled { + return nil + } + case err != nil && !errors.Is(err, nodepool.ErrNodeNotFound): + return fmt.Errorf("looking up artifact origin node: %w", err) + } + } + return ErrArtifactOriginRemoved + } + applyArtifactOrigin(artifact, node) + return nil +} + +func applyArtifactOrigin(artifact *Artifact, node *nodepool.Node) { + artifact.OriginNodeURL = strings.TrimRight(node.URL, "/") + artifact.OriginNodeGroup = "" + if node.Group != nil { + artifact.OriginNodeGroup = *node.Group + } +} + +func (p *NodeAwarePreparer) StatArtifact(ctx context.Context, artifact *Artifact) (downloadprepare.Result, error) { + if err := p.ResolveArtifact(ctx, artifact); err != nil { + return downloadprepare.Result{}, err + } + secret, err := p.remoteCredentials(artifact) + if err != nil { + return downloadprepare.Result{}, err + } + return p.remote.Stat(ctx, artifact.OriginNodeURL, secret, artifact.OriginArtifactID) +} + +func (p *NodeAwarePreparer) DeleteArtifact(ctx context.Context, artifact *Artifact) error { + // Prefer the authoritative current URL, including for a disabled node. A + // deleted node has no newer record, so retain the last persisted URL as the + // best-effort cleanup target. + _ = p.ResolveArtifact(ctx, artifact) + secret, err := p.remoteCredentials(artifact) + if err != nil { + return err + } + return p.remote.Delete(ctx, artifact.OriginNodeURL, secret, artifact.OriginArtifactID) +} + +func (p *NodeAwarePreparer) remoteCredentials(artifact *Artifact) (string, error) { + if artifact == nil || strings.TrimSpace(artifact.OriginNodeURL) == "" || !downloadprepare.ValidArtifactID(artifact.OriginArtifactID) { + return "", errors.New("remote artifact locator is incomplete") + } + cfg := p.config() + if cfg == nil || strings.TrimSpace(cfg.Auth.JWTSecret) == "" || p.remote == nil { + return "", errors.New("remote artifact credentials are unavailable") + } + return strings.TrimSpace(cfg.Auth.JWTSecret), nil } func (p *NodeAwarePreparer) config() *config.Config { diff --git a/internal/downloads/remote_preparer_test.go b/internal/downloads/remote_preparer_test.go index bdc95b9a..c46aad4a 100644 --- a/internal/downloads/remote_preparer_test.go +++ b/internal/downloads/remote_preparer_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" "os" - "path/filepath" "testing" "github.com/Silo-Server/silo-server/internal/config" @@ -13,74 +12,181 @@ import ( "github.com/Silo-Server/silo-server/internal/playback" ) -type recordingEncodePreparer struct { - calls int -} +type recordingEncodePreparer struct{ calls int } -func (p *recordingEncodePreparer) PrepareFile(context.Context, playback.TranscodeOpts, string) error { +func (p *recordingEncodePreparer) PrepareFile(_ context.Context, _ string, _ playback.TranscodeOpts, outputPath string) (PreparedArtifact, error) { p.calls++ - return nil + return PreparedArtifact{OutputPath: outputPath, FileSize: 8}, nil } type recordingRemotePreparer struct { - nodeURL string - secret string - request downloadprepare.Request + nodeURL string + secret string + request downloadprepare.Request + deleted string + deleteURL string +} + +func (p *recordingRemotePreparer) Prepare(_ context.Context, nodeURL, secret string, req downloadprepare.Request) (downloadprepare.Result, error) { + p.nodeURL, p.secret, p.request = nodeURL, secret, req + return downloadprepare.Result{ArtifactID: req.ArtifactID, FileSize: 1234}, nil +} + +func (p *recordingRemotePreparer) Stat(_ context.Context, _, _ string, artifactID string) (downloadprepare.Result, error) { + return downloadprepare.Result{ArtifactID: artifactID, FileSize: 1234}, nil +} + +func (p *recordingRemotePreparer) Delete(_ context.Context, nodeURL, _ string, artifactID string) error { + p.deleted = artifactID + p.deleteURL = nodeURL + return nil +} + +type staticArtifactOriginLookup struct { + node *nodepool.Node + err error +} + +func (l staticArtifactOriginLookup) GetByID(context.Context, int) (*nodepool.Node, error) { + return l.node, l.err } type unavailableRemotePreparer struct{} -func (unavailableRemotePreparer) Prepare(context.Context, string, string, downloadprepare.Request) error { - return os.ErrNotExist +func (unavailableRemotePreparer) Prepare(context.Context, string, string, downloadprepare.Request) (downloadprepare.Result, error) { + return downloadprepare.Result{}, os.ErrNotExist } +func (unavailableRemotePreparer) Stat(context.Context, string, string, string) (downloadprepare.Result, error) { + return downloadprepare.Result{}, downloadprepare.ErrArtifactNotFound +} +func (unavailableRemotePreparer) Delete(context.Context, string, string, string) error { return nil } type responseLostRemotePreparer struct{} -func (responseLostRemotePreparer) Prepare(_ context.Context, _, _ string, req downloadprepare.Request) error { - if err := os.WriteFile(req.OutputPath, []byte("prepared"), 0o600); err != nil { - return err - } - return context.DeadlineExceeded +func (responseLostRemotePreparer) Prepare(context.Context, string, string, downloadprepare.Request) (downloadprepare.Result, error) { + return downloadprepare.Result{}, context.DeadlineExceeded } +func (responseLostRemotePreparer) Stat(_ context.Context, _, _ string, artifactID string) (downloadprepare.Result, error) { + return downloadprepare.Result{ArtifactID: artifactID, FileSize: 55}, nil +} +func (responseLostRemotePreparer) Delete(context.Context, string, string, string) error { return nil } -func (p *recordingRemotePreparer) Prepare(_ context.Context, nodeURL, secret string, req downloadprepare.Request) error { - p.nodeURL = nodeURL - p.secret = secret - p.request = req - return os.WriteFile(req.OutputPath, []byte("prepared"), 0o600) +type indeterminateRemotePreparer struct{} + +func (indeterminateRemotePreparer) Prepare(context.Context, string, string, downloadprepare.Request) (downloadprepare.Result, error) { + return downloadprepare.Result{}, context.DeadlineExceeded +} +func (indeterminateRemotePreparer) Stat(context.Context, string, string, string) (downloadprepare.Result, error) { + return downloadprepare.Result{}, os.ErrDeadlineExceeded +} +func (indeterminateRemotePreparer) Delete(context.Context, string, string, string) error { return nil } + +type mismatchedRecoveryRemotePreparer struct{} + +func (mismatchedRecoveryRemotePreparer) Prepare(context.Context, string, string, downloadprepare.Request) (downloadprepare.Result, error) { + return downloadprepare.Result{}, context.DeadlineExceeded +} +func (mismatchedRecoveryRemotePreparer) Stat(context.Context, string, string, string) (downloadprepare.Result, error) { + return downloadprepare.Result{ArtifactID: "unexpected-artifact", FileSize: 55}, nil +} +func (mismatchedRecoveryRemotePreparer) Delete(context.Context, string, string, string) error { + return nil } func TestNodeAwarePreparerUsesLeastLoadedHealthyNode(t *testing.T) { + group := "host-a" pool := nodepool.NewTranscodePool() pool.SetNodes([]*nodepool.Node{ {URL: "http://busy", Enabled: true, Healthy: true, ActiveJobs: 3}, - {URL: "http://idle", Enabled: true, Healthy: true, ActiveJobs: 1}, + {ID: 17, URL: "http://idle", Enabled: true, Healthy: true, ActiveJobs: 1, Group: &group}, {URL: "http://unhealthy", Enabled: true, Healthy: false}, }) local := &recordingEncodePreparer{} remote := &recordingRemotePreparer{} cfg := &config.Config{} cfg.Auth.JWTSecret = "secret" - cfg.Download.ArtifactDir = t.TempDir() p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) p.remote = remote opts := playback.TranscodeOpts{InputPath: "/media/movie.mkv", TargetCodecVideo: "h264", TargetCodecAudio: "aac"} - outputPath := filepath.Join(cfg.Download.ArtifactDir, "job-1.mp4") - if err := p.PrepareFile(context.Background(), opts, outputPath); err != nil { + prepared, err := p.PrepareFile(context.Background(), "artifact-1", opts, "/local/artifact-1.mp4") + if err != nil { t.Fatal(err) } if local.calls != 0 { t.Fatalf("local calls = %d, want 0", local.calls) } - if remote.nodeURL != "http://idle" || remote.secret != "secret" { - t.Fatalf("remote call = node %q secret %q", remote.nodeURL, remote.secret) + if remote.nodeURL != "http://idle" || remote.secret != "secret" || remote.request.ArtifactID != "artifact-1" || remote.request.InputPath != opts.InputPath { + t.Fatalf("remote call = node %q secret %q request %+v", remote.nodeURL, remote.secret, remote.request) } - if remote.request.InputPath != opts.InputPath || remote.request.OutputPath == outputPath || filepath.Dir(remote.request.OutputPath) != filepath.Dir(outputPath) || filepath.Ext(remote.request.OutputPath) != ".mp4" { - t.Fatalf("remote request = %+v", remote.request) + if prepared.OutputPath != "" || prepared.OriginNodeID != 17 || prepared.OriginNodeURL != "http://idle" || prepared.OriginNodeGroup != group || prepared.OriginArtifactID != "artifact-1" || prepared.FileSize != 1234 { + t.Fatalf("prepared = %+v", prepared) } - if got, err := os.ReadFile(outputPath); err != nil || string(got) != "prepared" { - t.Fatalf("promoted output = %q, %v", got, err) +} + +func TestNodeAwarePreparerResolvesCurrentArtifactNodeURL(t *testing.T) { + group := "host-new" + pool := nodepool.NewTranscodePool() + pool.SetNodes([]*nodepool.Node{{ID: 17, URL: "http://new-url", Group: &group, Enabled: true, Healthy: true}}) + p := NewNodeAwarePreparer(nil, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), nil) + artifact := &Artifact{OriginNodeID: 17, OriginNodeURL: "http://old-url", OriginNodeGroup: "host-old", OriginArtifactID: "artifact-1"} + if err := p.ResolveArtifact(context.Background(), artifact); err != nil { + t.Fatal(err) + } + if artifact.OriginNodeURL != "http://new-url" || artifact.OriginNodeGroup != group { + t.Fatalf("artifact = %+v", artifact) + } +} + +func TestNodeAwarePreparerUsesCurrentDisabledNodeURLForCleanup(t *testing.T) { + group := "host-new" + pool := nodepool.NewTranscodePool() + remote := &recordingRemotePreparer{} + cfg := &config.Config{} + cfg.Auth.JWTSecret = "secret" + p := NewNodeAwarePreparer(nil, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) + p.remote = remote + p.SetOriginLookup(staticArtifactOriginLookup{node: &nodepool.Node{ + ID: 17, Type: nodepool.NodeTypeTranscode, URL: "http://new-url", Group: &group, Enabled: false, + }}) + artifact := &Artifact{OriginNodeID: 17, OriginNodeURL: "http://old-url", OriginNodeGroup: "host-old", OriginArtifactID: "artifact-1"} + if err := p.ResolveArtifact(context.Background(), artifact); !errors.Is(err, ErrArtifactOriginRemoved) { + t.Fatalf("ResolveArtifact error = %v, want ErrArtifactOriginRemoved", err) + } + if artifact.OriginNodeURL != "http://new-url" || artifact.OriginNodeGroup != group { + t.Fatalf("refreshed artifact = %+v", artifact) + } + if err := p.DeleteArtifact(context.Background(), artifact); err != nil { + t.Fatal(err) + } + if remote.deleteURL != "http://new-url" || remote.deleted != "artifact-1" { + t.Fatalf("cleanup target = %q %q", remote.deleteURL, remote.deleted) + } +} + +func TestNodeAwarePreparerRecoversEnabledOriginMissingFromPool(t *testing.T) { + group := "host-new" + pool := nodepool.NewTranscodePool() + p := NewNodeAwarePreparer(nil, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), nil) + p.SetOriginLookup(staticArtifactOriginLookup{node: &nodepool.Node{ + ID: 17, Type: nodepool.NodeTypeTranscode, URL: "http://new-url", Group: &group, Enabled: true, + }}) + artifact := &Artifact{OriginNodeID: 17, OriginNodeURL: "http://old-url", OriginNodeGroup: "host-old", OriginArtifactID: "artifact-1"} + if err := p.ResolveArtifact(context.Background(), artifact); err != nil { + t.Fatalf("ResolveArtifact error = %v", err) + } + if artifact.OriginNodeURL != "http://new-url" || artifact.OriginNodeGroup != group { + t.Fatalf("refreshed artifact = %+v", artifact) + } +} + +func TestNodeAwarePreparerReportsRemovedArtifactOrigin(t *testing.T) { + pool := nodepool.NewTranscodePool() + pool.SetNodes([]*nodepool.Node{{ID: 17, URL: "http://disabled", Enabled: false, Healthy: true}}) + p := NewNodeAwarePreparer(nil, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), nil) + artifact := &Artifact{OriginNodeID: 17, OriginNodeURL: "http://removed", OriginArtifactID: "artifact-1"} + if err := p.ResolveArtifact(context.Background(), artifact); !errors.Is(err, ErrArtifactOriginRemoved) { + t.Fatalf("ResolveArtifact error = %v, want ErrArtifactOriginRemoved", err) } } @@ -91,26 +197,44 @@ func TestNodeAwarePreparerFallsBackLocallyWithoutEligibleCapacity(t *testing.T) local := &recordingEncodePreparer{} cfg := &config.Config{} cfg.Auth.JWTSecret = "secret" - cfg.Download.ArtifactDir = t.TempDir() p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) p.remote = &recordingRemotePreparer{} - if err := p.PrepareFile(context.Background(), playback.TranscodeOpts{}, "/artifacts/job-2.mp4"); err != nil { - t.Fatal(err) + prepared, err := p.PrepareFile(context.Background(), "artifact-2", playback.TranscodeOpts{}, "/artifacts/job-2.mp4") + if err != nil || prepared.OutputPath == "" || local.calls != 1 { + t.Fatalf("prepared=%+v err=%v local calls=%d", prepared, err, local.calls) } - if local.calls != 1 { - t.Fatalf("local calls = %d, want 1", local.calls) +} + +func TestNodeAwarePreparerRetainsRequestedLocatorAfterMismatchedRecovery(t *testing.T) { + pool := nodepool.NewTranscodePool() + pool.SetNodes([]*nodepool.Node{{ID: 17, URL: "http://idle", Enabled: true, Healthy: true}}) + local := &recordingEncodePreparer{} + cfg := &config.Config{} + cfg.Auth.JWTSecret = "secret" + p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) + p.remote = mismatchedRecoveryRemotePreparer{} + + prepared, err := p.PrepareFile(context.Background(), "artifact-requested", playback.TranscodeOpts{}, "/local/job.mp4") + if err == nil { + t.Fatal("expected mismatched recovery error") + } + if !prepared.Remote() || prepared.OriginArtifactID != "artifact-requested" || prepared.FileSize != 0 { + t.Fatalf("prepared locator = %+v, want requested remote artifact", prepared) + } + if local.calls != 0 { + t.Fatalf("local calls = %d, want no fallback after indeterminate remote result", local.calls) } } func TestNodeAwarePreparerFallsBackLocallyWithoutNodeCredentials(t *testing.T) { pool := nodepool.NewTranscodePool() - pool.SetNodes([]*nodepool.Node{{URL: "http://idle", Enabled: true, Healthy: true}}) + pool.SetNodes([]*nodepool.Node{{ID: 18, URL: "http://idle", Enabled: true, Healthy: true}}) local := &recordingEncodePreparer{} p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return &config.Config{} }) p.remote = &recordingRemotePreparer{} - if err := p.PrepareFile(context.Background(), playback.TranscodeOpts{}, "/artifacts/job-3.mp4"); err != nil { + if _, err := p.PrepareFile(context.Background(), "artifact-3", playback.TranscodeOpts{}, "/artifacts/job-3.mp4"); err != nil { t.Fatal(err) } if local.calls != 1 { @@ -118,60 +242,48 @@ func TestNodeAwarePreparerFallsBackLocallyWithoutNodeCredentials(t *testing.T) { } } -func TestNodeAwarePreparerKeepsDefaultNodeLocalArtifactDirLocal(t *testing.T) { +func TestNodeAwarePreparerUsesRemoteWithDefaultNodeLocalArtifactDir(t *testing.T) { pool := nodepool.NewTranscodePool() - pool.SetNodes([]*nodepool.Node{{URL: "http://idle", Enabled: true, Healthy: true}}) + pool.SetNodes([]*nodepool.Node{{ID: 19, URL: "http://idle", Enabled: true, Healthy: true}}) local := &recordingEncodePreparer{} cfg := &config.Config{} cfg.Auth.JWTSecret = "secret" p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) p.remote = &recordingRemotePreparer{} - if err := p.PrepareFile(context.Background(), playback.TranscodeOpts{}, filepath.Join(t.TempDir(), "job-local.mp4")); err != nil { - t.Fatal(err) - } - if local.calls != 1 { - t.Fatalf("local calls = %d, want 1", local.calls) + prepared, err := p.PrepareFile(context.Background(), "artifact-local", playback.TranscodeOpts{}, "/local/job.mp4") + if err != nil || !prepared.Remote() || local.calls != 0 { + t.Fatalf("prepared=%+v err=%v local calls=%d", prepared, err, local.calls) } } -func TestNodeAwarePreparerFallsBackLocallyWhenRemoteOutputIsUnavailable(t *testing.T) { +func TestNodeAwarePreparerFallsBackLocallyWhenRemoteUnavailable(t *testing.T) { pool := nodepool.NewTranscodePool() pool.SetNodes([]*nodepool.Node{{URL: "http://idle", Enabled: true, Healthy: true}}) local := &recordingEncodePreparer{} cfg := &config.Config{} cfg.Auth.JWTSecret = "secret" - cfg.Download.ArtifactDir = t.TempDir() p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) p.remote = unavailableRemotePreparer{} - if err := p.PrepareFile(context.Background(), playback.TranscodeOpts{}, filepath.Join(cfg.Download.ArtifactDir, "job-4.mp4")); err != nil { - t.Fatal(err) - } - if local.calls != 1 { - t.Fatalf("local calls = %d, want 1", local.calls) + prepared, err := p.PrepareFile(context.Background(), "artifact-4", playback.TranscodeOpts{}, "/local/job-4.mp4") + if err != nil || prepared.OutputPath == "" || local.calls != 1 { + t.Fatalf("prepared=%+v err=%v local calls=%d", prepared, err, local.calls) } } -func TestNodeAwarePreparerPromotesCompletedOutputAfterResponseLoss(t *testing.T) { +func TestNodeAwarePreparerRecoversCompletedArtifactAfterResponseLoss(t *testing.T) { pool := nodepool.NewTranscodePool() - pool.SetNodes([]*nodepool.Node{{URL: "http://idle", Enabled: true, Healthy: true}}) + pool.SetNodes([]*nodepool.Node{{ID: 21, URL: "http://idle", Enabled: true, Healthy: true}}) local := &recordingEncodePreparer{} cfg := &config.Config{} cfg.Auth.JWTSecret = "secret" - cfg.Download.ArtifactDir = t.TempDir() p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) p.remote = responseLostRemotePreparer{} - outputPath := filepath.Join(cfg.Download.ArtifactDir, "job-5.mp4") - if err := p.PrepareFile(context.Background(), playback.TranscodeOpts{}, outputPath); err != nil { - t.Fatal(err) - } - if local.calls != 0 { - t.Fatalf("local calls = %d, want 0", local.calls) - } - if got, err := os.ReadFile(outputPath); err != nil || string(got) != "prepared" { - t.Fatalf("promoted output = %q, %v", got, err) + prepared, err := p.PrepareFile(context.Background(), "artifact-5", playback.TranscodeOpts{}, "/local/job-5.mp4") + if err != nil || !prepared.Remote() || prepared.FileSize != 55 || local.calls != 0 { + t.Fatalf("prepared=%+v err=%v local calls=%d", prepared, err, local.calls) } } @@ -181,13 +293,12 @@ func TestNodeAwarePreparerDoesNotFallBackAfterLeaseCancellation(t *testing.T) { local := &recordingEncodePreparer{} cfg := &config.Config{} cfg.Auth.JWTSecret = "secret" - cfg.Download.ArtifactDir = t.TempDir() p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) p.remote = unavailableRemotePreparer{} ctx, cancel := context.WithCancel(context.Background()) cancel() - err := p.PrepareFile(ctx, playback.TranscodeOpts{}, filepath.Join(cfg.Download.ArtifactDir, "job-6.mp4")) + _, err := p.PrepareFile(ctx, "artifact-6", playback.TranscodeOpts{}, "/local/job-6.mp4") if !errors.Is(err, context.Canceled) { t.Fatalf("error = %v, want context canceled", err) } @@ -195,3 +306,20 @@ func TestNodeAwarePreparerDoesNotFallBackAfterLeaseCancellation(t *testing.T) { t.Fatalf("local calls = %d, want 0", local.calls) } } + +func TestNodeAwarePreparerDoesNotFallBackWhenRecoveryProbeIsIndeterminate(t *testing.T) { + pool := nodepool.NewTranscodePool() + pool.SetNodes([]*nodepool.Node{{ID: 20, URL: "http://idle", Enabled: true, Healthy: true}}) + local := &recordingEncodePreparer{} + cfg := &config.Config{} + cfg.Auth.JWTSecret = "secret" + p := NewNodeAwarePreparer(local, nodepool.NewPlanner(nodepool.NewProxyPool(), pool), func() *config.Config { return cfg }) + p.remote = indeterminateRemotePreparer{} + + if _, err := p.PrepareFile(context.Background(), "artifact-7", playback.TranscodeOpts{}, "/local/job-7.mp4"); err == nil { + t.Fatal("expected indeterminate remote error") + } + if local.calls != 0 { + t.Fatalf("local calls = %d, want 0", local.calls) + } +} diff --git a/internal/downloads/repo_managed_test.go b/internal/downloads/repo_managed_test.go index b8d93506..e17059ed 100644 --- a/internal/downloads/repo_managed_test.go +++ b/internal/downloads/repo_managed_test.go @@ -289,7 +289,7 @@ func TestReconcileLinkedDownloads(t *testing.T) { if _, err := arepo.ClaimNext(ctx, "w", time.Minute); err != nil { t.Fatalf("claim ready artifact: %v", err) } - if ok, err := arepo.MarkReady(ctx, readyArt.ID, "w", "/tmp/ready.mp4", 4242); err != nil || !ok { + if ok, err := arepo.MarkReady(ctx, readyArt.ID, "w", "/tmp/ready.mp4", 0, "", "", "", 4242); err != nil || !ok { t.Fatalf("MarkReady = (%v, %v)", ok, err) } diff --git a/internal/downloads/service.go b/internal/downloads/service.go index 8eb1dae3..61c21c1c 100644 --- a/internal/downloads/service.go +++ b/internal/downloads/service.go @@ -16,6 +16,7 @@ import ( "github.com/Silo-Server/silo-server/internal/access" "github.com/Silo-Server/silo-server/internal/catalog" "github.com/Silo-Server/silo-server/internal/config" + "github.com/Silo-Server/silo-server/internal/downloadprepare" "github.com/Silo-Server/silo-server/internal/idgen" "github.com/Silo-Server/silo-server/internal/models" "github.com/Silo-Server/silo-server/internal/playback" @@ -79,11 +80,16 @@ type Capability struct { // from the request-scoped resolver methods below; a path alone is never an // authorization grant. type FileTarget struct { - Path string - DownloadID string - MediaFileID int - // ProxyEligible is true only when the target lives on storage expected to - // be mounted at the same absolute path on proxy nodes. + Path string + DownloadID string + MediaFileID int + ArtifactID string + OriginNodeID int + OriginNodeURL string + OriginNodeGroup string + OriginArtifactID string + // ProxyEligible is true when a proxy can read the path directly or relay the + // opaque artifact from its owning transcode node. ProxyEligible bool } @@ -903,7 +909,7 @@ func (s *Service) ServeFile(ctx context.Context, w http.ResponseWriter, r *http. if err != nil { return err } - return s.serveLocalFile(ctx, w, r, target.Path, userID) + return s.serveFileTarget(ctx, w, r, target, userID) } // Re-check policy — admin may have disabled downloads or revoked permission. @@ -977,7 +983,7 @@ func (s *Service) ResolveManagedFile(ctx context.Context, userID int, profileID, return nil, err } proxyEligible := proxyDeliveryAllowed(cfg) - return s.resolveDownloadBytesTarget(ctx, dl, filter, proxyEligible, proxyEligible && strings.TrimSpace(cfg.ArtifactDir) != "") + return s.resolveDownloadBytesTarget(ctx, dl, filter, proxyEligible, proxyEligible) } // PatchStatus lets a client confirm a managed entry's local state @@ -1083,7 +1089,7 @@ func (s *Service) serveDownloadBytes(ctx context.Context, w http.ResponseWriter, if err != nil { return err } - return s.serveLocalFile(ctx, w, r, target.Path, userID) + return s.serveFileTarget(ctx, w, r, target, userID) } func (s *Service) resolveDownloadBytesTarget(ctx context.Context, dl *Download, filter catalog.AccessFilter, sourceProxyEligible, preparedProxyEligible bool) (*FileTarget, error) { @@ -1112,7 +1118,17 @@ func (s *Service) resolveDownloadBytesTarget(ctx context.Context, dl *Download, if !catalog.FileAllowedByAccess(&served, filter) { return nil, catalog.ErrItemNotFound } - return &FileTarget{Path: artifact.OutputPath, DownloadID: dl.ID, MediaFileID: file.ID, ProxyEligible: preparedProxyEligible}, nil + return &FileTarget{ + Path: artifact.OutputPath, + DownloadID: dl.ID, + MediaFileID: file.ID, + ArtifactID: artifact.ID, + OriginNodeID: artifact.OriginNodeID, + OriginNodeURL: artifact.OriginNodeURL, + OriginNodeGroup: artifact.OriginNodeGroup, + OriginArtifactID: artifact.OriginArtifactID, + ProxyEligible: preparedProxyEligible, + }, nil } if file.MissingSince != nil { return nil, catalog.ErrItemNotFound @@ -1145,8 +1161,7 @@ func (s *Service) serveLocalFile(ctx context.Context, w http.ResponseWriter, r * return fmt.Errorf("stat file: %w", err) } - filename := sanitizeFilename(filepath.Base(path)) - w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, filename)) + w.Header().Set("Content-Disposition", attachmentDisposition(path)) w.Header().Set("Content-Type", playback.MimeFromExtension(path)) var reader io.ReadSeeker = f @@ -1158,6 +1173,76 @@ func (s *Service) serveLocalFile(ctx context.Context, w http.ResponseWriter, r * return nil } +func attachmentDisposition(path string) string { + filename := sanitizeFilename(filepath.Base(path)) + return fmt.Sprintf(`attachment; filename="%s"`, filename) +} + +func (s *Service) serveFileTarget(ctx context.Context, w http.ResponseWriter, r *http.Request, target *FileTarget, userID int) error { + if target == nil { + return catalog.ErrItemNotFound + } + if target.OriginArtifactID == "" { + return s.serveLocalFile(ctx, w, r, target.Path, userID) + } + secret := "" + if s.artifacts != nil && s.artifacts.liveCfg != nil { + if cfg := s.artifacts.liveCfg(); cfg != nil { + secret = strings.TrimSpace(cfg.Auth.JWTSecret) + } + } + if secret == "" { + return errors.New("remote artifact credentials unavailable") + } + client := downloadprepare.HTTPPreparer{} + resp, err := client.Open(ctx, target.OriginNodeURL, secret, target.OriginArtifactID, r.Method, r.Header) + if err != nil { + return err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNotFound { + artifact := &Artifact{ + ID: target.ArtifactID, + OriginNodeID: target.OriginNodeID, + OriginNodeURL: target.OriginNodeURL, + OriginNodeGroup: target.OriginNodeGroup, + OriginArtifactID: target.OriginArtifactID, + } + if _, err := s.artifacts.requeueRemoteArtifactExactNow(ctx, artifact, "remote output missing"); err != nil { + return err + } + return fmt.Errorf("remote artifact was missing and preparation was requeued: %w", ErrDownloadNotActive) + } + if !downloadprepare.RelayStatusAllowed(resp.StatusCode) { + return fmt.Errorf("remote artifact node returned %d", resp.StatusCode) + } + // Preserve an origin-provided disposition, but supply the same sanitized + // attachment filename as local delivery when the node omits one. + if strings.TrimSpace(target.Path) != "" { + w.Header().Set("Content-Disposition", attachmentDisposition(target.Path)) + } + downloadprepare.CopyResponseHeaders(w.Header(), resp.Header) + w.WriteHeader(resp.StatusCode) + if r.Method == http.MethodHead { + return nil + } + var reader io.Reader = resp.Body + if s.bandwidth != nil { + reader = s.bandwidth.ThrottledStreamReader(ctx, reader, userID) + } + if _, err := io.Copy(w, reader); err != nil { + // Headers and a partial body are already committed, so returning an error + // normally makes an API handler append JSON to the media response. Return a + // committed-response sentinel so lifecycle state records the failed transfer + // while the handler leaves the partial media response untouched. + if ctx.Err() == nil { + slog.WarnContext(ctx, "remote download artifact relay interrupted", "component", "downloads", "artifact_id", target.OriginArtifactID, "error", err) + } + return fmt.Errorf("%w: relaying remote artifact: %w", ErrResponseCommitted, err) + } + return nil +} + // pickBestFile selects the highest-resolution file from a list. func pickBestFile(files []*models.MediaFile) *models.MediaFile { if len(files) == 1 { diff --git a/internal/downloads/service_quota_test.go b/internal/downloads/service_quota_test.go index 47f25197..e0806b3b 100644 --- a/internal/downloads/service_quota_test.go +++ b/internal/downloads/service_quota_test.go @@ -17,8 +17,8 @@ import ( // test never drains the queue, so it is never invoked. type stubQuotaPreparer struct{} -func (stubQuotaPreparer) PrepareFile(_ context.Context, _ playback.TranscodeOpts, _ string) error { - return nil +func (stubQuotaPreparer) PrepareFile(_ context.Context, _ string, _ playback.TranscodeOpts, outputPath string) (PreparedArtifact, error) { + return PreparedArtifact{OutputPath: outputPath}, nil } // TestArtifactQuotaCheckedBeforeEnqueue pins two quota invariants for diff --git a/internal/downloads/service_remote_test.go b/internal/downloads/service_remote_test.go new file mode 100644 index 00000000..0c5d8beb --- /dev/null +++ b/internal/downloads/service_remote_test.go @@ -0,0 +1,204 @@ +package downloads + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/Silo-Server/silo-server/internal/config" +) + +func TestServiceRelaysRemoteArtifactOnEstablishedRoute(t *testing.T) { + const secret = "artifact-secret" + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/downloads/artifacts/artifact-1" || r.Header.Get("Authorization") != "Bearer "+secret { + t.Errorf("origin request = %s %s auth=%q", r.Method, r.URL.Path, r.Header.Get("Authorization")) + http.Error(w, "unexpected origin request", http.StatusBadRequest) + return + } + if r.Header.Get("Range") != "bytes=1-3" { + t.Errorf("Range = %q", r.Header.Get("Range")) + http.Error(w, "unexpected range", http.StatusBadRequest) + return + } + w.Header().Set("Content-Length", "3") + w.Header().Set("Content-Range", "bytes 1-3/5") + w.Header().Set("Content-Type", "video/mp4") + w.Header().Set("Content-Disposition", `attachment; filename="artifact-1.mp4"`) + w.WriteHeader(http.StatusPartialContent) + _, _ = w.Write([]byte("123")) + })) + defer origin.Close() + cfg := &config.Config{} + cfg.Auth.JWTSecret = secret + svc := &Service{artifacts: &ArtifactManager{liveCfg: func() *config.Config { return cfg }}} + req := httptest.NewRequest(http.MethodGet, "/api/v1/downloads/dl/file", nil) + req.Header.Set("Range", "bytes=1-3") + rr := httptest.NewRecorder() + err := svc.serveFileTarget(req.Context(), rr, req, &FileTarget{ + Path: `/artifacts/Movie "Final".mp4`, + OriginNodeURL: origin.URL, + OriginArtifactID: "artifact-1", + }, 7) + if err != nil { + t.Fatal(err) + } + if rr.Code != http.StatusPartialContent || rr.Body.String() != "123" || rr.Header().Get("Content-Range") != "bytes 1-3/5" || + rr.Header().Get("Content-Disposition") != `attachment; filename="Movie _Final_.mp4"` { + t.Fatalf("response status=%d body=%q range=%q disposition=%q", rr.Code, rr.Body.String(), rr.Header().Get("Content-Range"), rr.Header().Get("Content-Disposition")) + } +} + +func TestServiceRequeuesReadyArtifactWhenRemoteFileIsMissing(t *testing.T) { + repo, pool, fileID := newArtifactTestRepo(t) + ctx := context.Background() + artifact, _, err := repo.EnsureQueued(ctx, newArtifact(t, fileID, fmt.Sprintf("hash-missing-remote-%d", time.Now().UnixNano()))) + if err != nil { + t.Fatal(err) + } + origin := httptest.NewServer(http.NotFoundHandler()) + defer origin.Close() + const ( + secret = "artifact-secret" + originNodeID = 424242 + originArtifactID = "artifact-missing" + ) + if _, err := pool.Exec(ctx, + `UPDATE download_artifacts + SET status = 'ready', file_size = 23, completed_at = now(), + origin_node_id = $2, origin_node_url = $3, origin_node_group = 'host-a', origin_artifact_id = $4 + WHERE id = $1`, + artifact.ID, originNodeID, origin.URL, originArtifactID, + ); err != nil { + t.Fatal(err) + } + var userID int + if err := pool.QueryRow(ctx, + `INSERT INTO users (username, role, download_allowed) VALUES ($1, 'user', true) RETURNING id`, + fmt.Sprintf("missing-remote-user-%d", time.Now().UnixNano()), + ).Scan(&userID); err != nil { + t.Fatal(err) + } + downloadID := fmt.Sprintf("missing-remote-download-%d", time.Now().UnixNano()) + downloadRepo := NewRepository(pool) + if err := downloadRepo.Create(ctx, &Download{ + ID: downloadID, UserID: userID, MediaFileID: fileID, + ContentID: "missing-remote-content", Kind: KindQueued, Status: StatusCompleted, + Format: FormatTranscode, ArtifactID: artifact.ID, FileSize: 23, + CreatedAt: time.Now(), UpdatedAt: time.Now(), + }); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _, _ = pool.Exec(ctx, + `DELETE FROM download_artifact_orphans + WHERE download_artifact_id = $1 AND origin_node_id = $2 AND origin_artifact_id = $3`, + artifact.ID, originNodeID, originArtifactID, + ) + _, _ = pool.Exec(ctx, `DELETE FROM downloads WHERE id = $1`, downloadID) + _, _ = pool.Exec(ctx, `DELETE FROM users WHERE id = $1`, userID) + }) + + cfg := &config.Config{} + cfg.Auth.JWTSecret = secret + svc := &Service{artifacts: &ArtifactManager{ + repo: repo, + liveCfg: func() *config.Config { return cfg }, + }} + req := httptest.NewRequest(http.MethodGet, "/api/v1/downloads/dl/file", nil) + err = svc.serveFileTarget(req.Context(), httptest.NewRecorder(), req, &FileTarget{ + ArtifactID: artifact.ID, + OriginNodeID: originNodeID, + OriginNodeURL: origin.URL, + OriginNodeGroup: "host-a", + OriginArtifactID: originArtifactID, + }, 7) + if !errors.Is(err, ErrDownloadNotActive) { + t.Fatalf("serveFileTarget error = %v, want ErrDownloadNotActive", err) + } + + queued, err := repo.GetByID(ctx, artifact.ID) + if err != nil { + t.Fatal(err) + } + if queued.Status != ArtifactQueued || queued.OriginNodeID != 0 || queued.OriginArtifactID != "" { + t.Fatalf("artifact after missing remote response = %+v", queued) + } + linked, err := downloadRepo.GetByID(ctx, downloadID) + if err != nil { + t.Fatal(err) + } + if linked.Status != StatusPreparing || linked.BytesSent != 0 { + t.Fatalf("linked download after missing remote response = %+v", linked) + } + var cleanupURL string + if err := pool.QueryRow(ctx, + `SELECT origin_node_url FROM download_artifact_orphans + WHERE download_artifact_id = $1 AND origin_node_id = $2 AND origin_artifact_id = $3`, + artifact.ID, originNodeID, originArtifactID, + ).Scan(&cleanupURL); err != nil { + t.Fatalf("remote cleanup row: %v", err) + } + if cleanupURL != origin.URL { + t.Fatalf("remote cleanup URL = %q, want %q", cleanupURL, origin.URL) + } +} + +func TestServiceDoesNotAppendAPIErrorAfterRemoteBodyIsCommitted(t *testing.T) { + const secret = "artifact-secret" + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "10") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "123") + })) + defer origin.Close() + cfg := &config.Config{} + cfg.Auth.JWTSecret = secret + svc := &Service{artifacts: &ArtifactManager{liveCfg: func() *config.Config { return cfg }}} + req := httptest.NewRequest(http.MethodGet, "/api/v1/downloads/dl/file", nil) + rr := httptest.NewRecorder() + err := svc.serveFileTarget(req.Context(), rr, req, &FileTarget{ + OriginNodeURL: origin.URL, + OriginArtifactID: "artifact-1", + }, 7) + if !errors.Is(err, ErrResponseCommitted) { + t.Fatalf("serveFileTarget error = %v, want ErrResponseCommitted", err) + } + if rr.Code != http.StatusOK || rr.Body.String() != "123" { + t.Fatalf("response status=%d body=%q", rr.Code, rr.Body.String()) + } +} + +func TestServiceRelaysRemotePreconditionFailure(t *testing.T) { + const secret = "artifact-secret" + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("If-Match"); got != `"old-etag"` { + t.Errorf("origin If-Match = %q", got) + http.Error(w, "unexpected condition", http.StatusBadRequest) + return + } + w.Header().Set("ETag", `"new-etag"`) + w.WriteHeader(http.StatusPreconditionFailed) + })) + defer origin.Close() + cfg := &config.Config{} + cfg.Auth.JWTSecret = secret + svc := &Service{artifacts: &ArtifactManager{liveCfg: func() *config.Config { return cfg }}} + req := httptest.NewRequest(http.MethodGet, "/api/v1/downloads/dl/file", nil) + req.Header.Set("If-Match", `"old-etag"`) + rr := httptest.NewRecorder() + if err := svc.serveFileTarget(req.Context(), rr, req, &FileTarget{ + OriginNodeURL: origin.URL, + OriginArtifactID: "artifact-1", + }, 7); err != nil { + t.Fatal(err) + } + if rr.Code != http.StatusPreconditionFailed || rr.Header().Get("ETag") != `"new-etag"` { + t.Fatalf("response status=%d etag=%q body=%q", rr.Code, rr.Header().Get("ETag"), rr.Body.String()) + } +} diff --git a/internal/nodepool/planner.go b/internal/nodepool/planner.go index 392f60d9..94d42609 100644 --- a/internal/nodepool/planner.go +++ b/internal/nodepool/planner.go @@ -26,7 +26,7 @@ type SessionPlanner interface { // have no predictable bitrate, so implementations must not admit them onto a // proxy with a configured bandwidth cap. type DownloadPlanner interface { - PlanDownload(sessionID string) Plan + PlanDownload(sessionID string, preferredGroup ...string) Plan ReleaseSession(sessionID string) } @@ -35,6 +35,7 @@ type DownloadPlanner interface { // remote operation ends or falls back locally. type TranscodeWorkPlanner interface { ReserveTranscodeWork(workID string) (*Node, func()) + TranscodeNode(nodeID int) (*Node, bool) } // reservation bridges the gap between assigning a session to a node and the @@ -108,11 +109,25 @@ func (p *Planner) PlanSession(sessionID, currentTranscodeURL string, needsTransc return p.PlanSessionWith(sessionID, currentTranscodeURL, needsTranscode, estBitrateKbps, nil) } +// TranscodeNode returns the current pool record for a persistent node id. It +// lets durable artifact locators follow an administrator-edited node URL/group. +func (p *Planner) TranscodeNode(nodeID int) (*Node, bool) { + if p == nil || p.transcodes == nil || nodeID == 0 { + return nil, false + } + for _, node := range p.transcodes.Nodes() { + if node != nil && node.ID == nodeID && node.Enabled { + return node, true + } + } + return nil, false +} + // PlanDownload picks a healthy proxy for an unbounded file transfer. A // configured proxy bandwidth cap cannot be reserved accurately without a known // transfer rate, so capped proxies are excluded instead of being oversubscribed // during the egress meter's convergence window. -func (p *Planner) PlanDownload(sessionID string) Plan { +func (p *Planner) PlanDownload(sessionID string, preferredGroup ...string) Plan { if p == nil || p.proxies == nil || sessionID == "" { return Plan{} } @@ -123,7 +138,11 @@ func (p *Planner) PlanDownload(sessionID string) Plan { p.pruneReservations(now) delete(p.reserved, sessionID) - var candidates []*Node + group := "" + if len(preferredGroup) > 0 { + group = preferredGroup[0] + } + var candidates, fallback []*Node for _, node := range p.proxies.Nodes() { if node == nil || !node.Enabled || !node.Healthy || !p.underCap(node, now) { continue @@ -131,12 +150,18 @@ func (p *Planner) PlanDownload(sessionID string) Plan { if node.MaxBandwidthKbps != nil && *node.MaxBandwidthKbps > 0 { continue } - candidates = append(candidates, node) + fallback = append(fallback, node) + if group != "" && node.Group != nil && *node.Group == group { + candidates = append(candidates, node) + } + } + if group == "" || len(candidates) == 0 { + candidates = fallback } if len(candidates) == 0 { return Plan{} } - const rrKey = "download" + rrKey := "download:" + group proxy := candidates[p.rr[rrKey]%len(candidates)] p.rr[rrKey]++ p.reserved[sessionID] = &reservation{proxyURL: proxy.URL, createdAt: now} diff --git a/internal/nodepool/planner_test.go b/internal/nodepool/planner_test.go index cb9362d8..78749e7c 100644 --- a/internal/nodepool/planner_test.go +++ b/internal/nodepool/planner_test.go @@ -590,3 +590,28 @@ func TestPlanDownloadSkipsBandwidthCappedProxiesAndReservesJobCapacity(t *testin t.Fatal("download was not admitted after reservation release") } } + +func TestPlanDownloadPrefersArtifactOriginGroup(t *testing.T) { + groupA, groupB := "host-a", "host-b" + proxies := NewProxyPool() + proxies.SetNodes([]*Node{ + {URL: "http://proxy-a", Group: &groupA, Enabled: true, Healthy: true}, + {URL: "http://proxy-b", Group: &groupB, Enabled: true, Healthy: true}, + }) + planner := NewPlanner(proxies, NewTranscodePool()) + plan := planner.PlanDownload("download-grouped", groupB) + if plan.ProxyNode == nil || plan.ProxyNode.URL != "http://proxy-b" { + t.Fatalf("plan = %+v, want proxy-b", plan) + } +} + +func TestPlanDownloadFallsBackWhenOriginGroupHasNoProxy(t *testing.T) { + group := "host-a" + proxies := NewProxyPool() + proxies.SetNodes([]*Node{{URL: "http://proxy-a", Group: &group, Enabled: true, Healthy: true}}) + planner := NewPlanner(proxies, NewTranscodePool()) + plan := planner.PlanDownload("download-fallback", "host-missing") + if plan.ProxyNode == nil || plan.ProxyNode.URL != "http://proxy-a" { + t.Fatalf("plan = %+v, want proxy-a fallback", plan) + } +} diff --git a/internal/proxy/download_test.go b/internal/proxy/download_test.go index bd598c7c..bdc970ce 100644 --- a/internal/proxy/download_test.go +++ b/internal/proxy/download_test.go @@ -1,6 +1,7 @@ package proxy import ( + "context" "io" "net/http" "net/http/httptest" @@ -15,6 +16,19 @@ import ( "github.com/Silo-Server/silo-server/internal/streamtoken" ) +type recordingArtifactMissReporter struct { + artifactID string + originNodeURL string + originArtifactID string +} + +func (r *recordingArtifactMissReporter) ReportRemoteArtifactMissing(_ context.Context, artifactID, originNodeURL, originArtifactID string) error { + r.artifactID = artifactID + r.originNodeURL = originNodeURL + r.originArtifactID = originArtifactID + return nil +} + func newDownloadProxyServer(t *testing.T, secret string) *Server { t.Helper() w := nodeconfig.NewWatcher(nil, nil, nil, nodeconfig.BootstrapOverrides{}) @@ -59,6 +73,155 @@ func TestProxyDownloadServesAuthorizedRange(t *testing.T) { } } +func TestProxyDownloadRelaysNodeLocalArtifactRange(t *testing.T) { + const secret = "download-proxy-secret" + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/downloads/artifacts/artifact-1" { + t.Errorf("origin path = %q", r.URL.Path) + http.Error(w, "unexpected path", http.StatusBadRequest) + return + } + if auth := r.Header.Get("Authorization"); auth != "Bearer "+secret { + t.Errorf("origin Authorization = %q", auth) + http.Error(w, "unexpected authorization", http.StatusBadRequest) + return + } + if got := r.Header.Get("Range"); got != "bytes=2-5" { + t.Errorf("origin Range = %q", got) + http.Error(w, "unexpected range", http.StatusBadRequest) + return + } + w.Header().Set("Accept-Ranges", "bytes") + w.Header().Set("Content-Disposition", `attachment; filename="artifact-1.mp4"`) + w.Header().Set("Content-Length", "4") + w.Header().Set("Content-Range", "bytes 2-5/10") + w.Header().Set("Content-Type", "video/mp4") + w.WriteHeader(http.StatusPartialContent) + _, _ = io.WriteString(w, "2345") + })) + defer origin.Close() + token, err := streamtoken.Sign(streamtoken.Claims{ + SessionID: "download-remote-1", + PlayMethod: streamtoken.PlayMethodDownload, + TranscodeNode: origin.URL, + DownloadArtifactID: "artifact-1", + DownloadFilename: "Movie Final.mp4", + UserID: 7, + }, secret, time.Minute) + if err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest(http.MethodGet, "/downloads/file/"+token, nil) + req.Header.Set("Range", "bytes=2-5") + rr := httptest.NewRecorder() + newDownloadProxyServer(t, secret).Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusPartialContent || rr.Body.String() != "2345" { + t.Fatalf("status = %d body = %q", rr.Code, rr.Body.String()) + } + if got := rr.Header().Get("Content-Range"); got != "bytes 2-5/10" { + t.Fatalf("Content-Range = %q", got) + } + if got := rr.Header().Get("Content-Disposition"); !strings.Contains(got, "Movie Final.mp4") || strings.Contains(got, "artifact-1.mp4") { + t.Fatalf("Content-Disposition = %q", got) + } +} + +func TestProxyDownloadCORSAllowsConditionalHeaders(t *testing.T) { + req := httptest.NewRequest(http.MethodOptions, "/downloads/file/token", nil) + req.Header.Set("Origin", "https://web.example") + req.Header.Set("Access-Control-Request-Method", http.MethodGet) + req.Header.Set("Access-Control-Request-Headers", "Range, If-Range, If-None-Match, If-Modified-Since, If-Match, If-Unmodified-Since") + rr := httptest.NewRecorder() + newDownloadProxyServer(t, "download-proxy-secret").Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + allowed := strings.ToLower(rr.Header().Get("Access-Control-Allow-Headers")) + for _, name := range []string{"range", "if-range", "if-none-match", "if-modified-since", "if-match", "if-unmodified-since"} { + if !strings.Contains(allowed, name) { + t.Errorf("Access-Control-Allow-Headers = %q, missing %s", allowed, name) + } + } +} + +func TestProxyDownloadRelaysNodeLocalArtifactPreconditionFailure(t *testing.T) { + const secret = "download-proxy-secret" + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("If-Match"); got != `"old-etag"` { + t.Errorf("origin If-Match = %q", got) + http.Error(w, "unexpected condition", http.StatusBadRequest) + return + } + w.Header().Set("ETag", `"new-etag"`) + w.WriteHeader(http.StatusPreconditionFailed) + })) + defer origin.Close() + token, err := streamtoken.Sign(streamtoken.Claims{ + SessionID: "download-remote-412", + PlayMethod: streamtoken.PlayMethodDownload, + TranscodeNode: origin.URL, + DownloadArtifactID: "artifact-1", + }, secret, time.Minute) + if err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest(http.MethodGet, "/downloads/file/"+token, nil) + req.Header.Set("If-Match", `"old-etag"`) + rr := httptest.NewRecorder() + newDownloadProxyServer(t, secret).Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusPreconditionFailed || rr.Header().Get("ETag") != `"new-etag"` { + t.Fatalf("status=%d etag=%q body=%q", rr.Code, rr.Header().Get("ETag"), rr.Body.String()) + } +} + +func TestProxyDownloadReportsNodeLocalArtifactMissing(t *testing.T) { + const secret = "download-proxy-secret" + origin := httptest.NewServer(http.NotFoundHandler()) + defer origin.Close() + token, err := streamtoken.Sign(streamtoken.Claims{ + SessionID: "download-remote-missing", + PlayMethod: streamtoken.PlayMethodDownload, + TranscodeNode: origin.URL, + DownloadArtifactID: "artifact-opaque", + DownloadArtifactRowID: "artifact-row", + }, secret, time.Minute) + if err != nil { + t.Fatal(err) + } + reporter := &recordingArtifactMissReporter{} + server := newDownloadProxyServer(t, secret) + server.SetRemoteArtifactMissReporter(reporter) + rr := httptest.NewRecorder() + server.Handler().ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/downloads/file/"+token, nil)) + + if rr.Code != http.StatusNotFound { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } + if reporter.artifactID != "artifact-row" || reporter.originNodeURL != origin.URL || reporter.originArtifactID != "artifact-opaque" { + t.Fatalf("missing report = %+v", reporter) + } +} + +func TestProxyDownloadRejectsRemoteArtifactWithInvalidOpaqueID(t *testing.T) { + const secret = "download-proxy-secret" + token, err := streamtoken.Sign(streamtoken.Claims{ + SessionID: "download-remote-invalid", + PlayMethod: streamtoken.PlayMethodDownload, + TranscodeNode: "http://origin.invalid", + DownloadArtifactID: "../escape", + }, secret, time.Minute) + if err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + newDownloadProxyServer(t, secret).Handler().ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/downloads/file/"+token, nil)) + if rr.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } +} + func TestProxyDownloadRejectsPlaybackToken(t *testing.T) { const secret = "download-proxy-secret" token, err := streamtoken.Sign(streamtoken.Claims{ diff --git a/internal/proxy/server.go b/internal/proxy/server.go index cffce927..89c2fc40 100644 --- a/internal/proxy/server.go +++ b/internal/proxy/server.go @@ -18,6 +18,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/go-chi/cors" + "github.com/Silo-Server/silo-server/internal/downloadprepare" "github.com/Silo-Server/silo-server/internal/downloads" "github.com/Silo-Server/silo-server/internal/nodeconfig" "github.com/Silo-Server/silo-server/internal/nodesessions" @@ -27,10 +28,11 @@ import ( // Server is the HTTP handler for proxy mode. type Server struct { - watcher *nodeconfig.Watcher - tracker *nodesessions.Tracker - httpClient *http.Client - egress *egressMeter + watcher *nodeconfig.Watcher + tracker *nodesessions.Tracker + httpClient *http.Client + artifactMissReporter remoteArtifactMissReporter + egress *egressMeter // subCache stores full-track PGS (.sup) extracts under the transcode dir // so repeat selections skip the whole-file ffmpeg demux. subCache *playback.SubtitleCache @@ -43,6 +45,10 @@ type Server struct { downloadUserBPS int64 } +type remoteArtifactMissReporter interface { + ReportRemoteArtifactMissing(ctx context.Context, artifactID, originNodeURL, originArtifactID string) error +} + // NewServer creates a new proxy server backed by a config watcher and session // tracker. func NewServer(watcher *nodeconfig.Watcher, tracker *nodesessions.Tracker) *Server { @@ -51,14 +57,26 @@ func NewServer(watcher *nodeconfig.Watcher, tracker *nodesessions.Tracker) *Serv tracker: tracker, // No overall timeout — stream bodies are long-lived. Hung nodes are // bounded by the transport's response-header timeout instead. - httpClient: &http.Client{Transport: newStreamTransport()}, - egress: newEgressMeter(), + httpClient: &http.Client{ + Transport: newStreamTransport(), + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, + }, + egress: newEgressMeter(), subCache: playback.NewSubtitleCache(func() string { return watcher.Config().Playback.TranscodeDir }), } } +// SetRemoteArtifactMissReporter wires the authoritative database transition +// used when an origin returns 404 after the API's proxy preflight. It must be +// called during construction, before the server begins handling requests. +func (s *Server) SetRemoteArtifactMissReporter(reporter remoteArtifactMissReporter) { + s.artifactMissReporter = reporter +} + // newStreamTransport tunes the proxy→transcode-node connection pool. Many // concurrent viewers fan their segment fetches through one proxy→node pair, // and Go's default of 2 idle connections per host causes constant connection @@ -81,8 +99,11 @@ func (s *Server) Handler() http.Handler { r.Use(cors.Handler(cors.Options{ AllowedOrigins: []string{"*"}, AllowedMethods: []string{"GET", "HEAD", "OPTIONS"}, - AllowedHeaders: []string{"Accept", "Authorization", "Content-Type", "Range"}, - MaxAge: 86400, + AllowedHeaders: []string{ + "Accept", "Authorization", "Content-Type", "Range", + "If-Match", "If-Modified-Since", "If-None-Match", "If-Range", "If-Unmodified-Since", + }, + MaxAge: 86400, })) r.Get("/api/v1/health", s.handleHealth) r.Group(func(r chi.Router) { @@ -172,7 +193,8 @@ func (s *Server) handleDownloadFile(w http.ResponseWriter, r *http.Request) { if claims == nil { return } - if claims.PlayMethod != streamtoken.PlayMethodDownload || strings.TrimSpace(claims.MediaPath) == "" { + remoteArtifact := claims.DownloadArtifactID != "" && strings.TrimSpace(claims.TranscodeNode) != "" + if claims.PlayMethod != streamtoken.PlayMethodDownload || (strings.TrimSpace(claims.MediaPath) == "" && !remoteArtifact) { http.Error(w, "unauthorized", http.StatusUnauthorized) return } @@ -185,6 +207,10 @@ func (s *Server) handleDownloadFile(w http.ResponseWriter, r *http.Request) { s.tracker.Track(r.Context(), info) defer s.tracker.Remove(context.WithoutCancel(r.Context()), claims.SessionID) } + if remoteArtifact { + s.relayDownloadArtifact(w, r, claims) + return + } f, err := os.Open(claims.MediaPath) if err != nil { @@ -210,6 +236,61 @@ func (s *Server) handleDownloadFile(w http.ResponseWriter, r *http.Request) { http.ServeContent(w, r, stat.Name(), stat.ModTime(), reader) } +func (s *Server) relayDownloadArtifact(w http.ResponseWriter, r *http.Request, claims *streamtoken.Claims) { + if !downloadprepare.ValidArtifactID(claims.DownloadArtifactID) { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + cfg := s.watcher.Config() + if cfg == nil || strings.TrimSpace(cfg.Auth.JWTSecret) == "" { + http.Error(w, "download unavailable", http.StatusServiceUnavailable) + return + } + client := downloadprepare.HTTPPreparer{Client: s.httpClient} + resp, err := client.Open(r.Context(), claims.TranscodeNode, cfg.Auth.JWTSecret, claims.DownloadArtifactID, r.Method, r.Header) + if err != nil { + slog.WarnContext(r.Context(), "download artifact relay failed", "component", "proxy", "artifact_id", claims.DownloadArtifactID, "node", claims.TranscodeNode, "error", err) + http.Error(w, "download unavailable", http.StatusBadGateway) + return + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNotFound { + if s.artifactMissReporter != nil && strings.TrimSpace(claims.DownloadArtifactRowID) != "" { + reportCtx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), 5*time.Second) + err := s.artifactMissReporter.ReportRemoteArtifactMissing( + reportCtx, claims.DownloadArtifactRowID, claims.TranscodeNode, claims.DownloadArtifactID, + ) + cancel() + if err != nil { + slog.WarnContext(r.Context(), "report missing remote download artifact", "component", "proxy", "artifact_id", claims.DownloadArtifactRowID, "error", err) + } + } + http.NotFound(w, r) + return + } + if !downloadprepare.RelayStatusAllowed(resp.StatusCode) { + http.Error(w, "download unavailable", http.StatusBadGateway) + return + } + downloadprepare.CopyResponseHeaders(w.Header(), resp.Header) + if filename := filepath.Base(strings.TrimSpace(claims.DownloadFilename)); filename != "" && filename != "." { + if disposition := mime.FormatMediaType("attachment", map[string]string{"filename": filename}); disposition != "" { + w.Header().Set("Content-Disposition", disposition) + } + } + w.WriteHeader(resp.StatusCode) + if r.Method == http.MethodHead { + return + } + var reader io.Reader = resp.Body + if bandwidth := s.downloadBandwidthManager(); bandwidth != nil { + reader = bandwidth.ThrottledStreamReader(r.Context(), reader, claims.UserID) + } + if _, err := io.Copy(w, reader); err != nil && r.Context().Err() == nil { + slog.WarnContext(r.Context(), "download artifact relay interrupted", "component", "proxy", "artifact_id", claims.DownloadArtifactID, "error", err) + } +} + func (s *Server) downloadBandwidthManager() *downloads.BandwidthManager { cfg := s.watcher.Config() if cfg == nil { diff --git a/internal/streamtoken/token.go b/internal/streamtoken/token.go index 34523e83..36be9140 100644 --- a/internal/streamtoken/token.go +++ b/internal/streamtoken/token.go @@ -49,6 +49,16 @@ type Claims struct { UserID int `json:"uid,omitempty"` ProfileID string `json:"pid,omitempty"` MediaFileID int `json:"mfid,omitempty"` + // DownloadArtifactID is an opaque transcode-node artifact handle. For + // download tokens TranscodeNode is its authenticated origin; MediaPath stays + // empty so node-local filesystem paths never leave the owning node. + DownloadArtifactID string `json:"daid,omitempty"` + // DownloadArtifactRowID identifies the authoritative database row so a + // proxy can fence and requeue a signed remote locator that returns 404. + DownloadArtifactRowID string `json:"darid,omitempty"` + // DownloadFilename is the client-facing attachment name. Remote artifact + // ids are internal attempt handles and must never become saved filenames. + DownloadFilename string `json:"dfn,omitempty"` // Reconstruction recipe — the byte-affecting encode parameters, mirroring the // former playback.RecipeCard. Zero for direct/remux tokens, which reconstruct diff --git a/internal/transcodenode/server.go b/internal/transcodenode/server.go index 91636ce9..651fcd53 100644 --- a/internal/transcodenode/server.go +++ b/internal/transcodenode/server.go @@ -95,11 +95,13 @@ type sessionTracker interface { // Server is the HTTP handler for transcode mode. type Server struct { - watcher *nodeconfig.Watcher - tracker sessionTracker - ffmpegSink playback.FFmpegLogSink - inputPaths InputPathAuthorizer - sessions map[string]*playback.TranscodeSession + watcher *nodeconfig.Watcher + tracker sessionTracker + ffmpegSink playback.FFmpegLogSink + inputPaths InputPathAuthorizer + transcodeDir string + artifactRoot string + sessions map[string]*playback.TranscodeSession // lastAccess records, per registered session id, when a manifest or segment // request last touched the job (registration counts as the first access). // Guarded by mu alongside sessions; the idle reaper closes jobs whose entry @@ -107,6 +109,9 @@ type Server struct { lastAccess map[string]time.Time reaperOnce sync.Once mu sync.RWMutex + // reloadMu keeps force-reload teardown atomic with session creation and + // reconstruction. It is always acquired before lifecycleMu or mu. + reloadMu sync.RWMutex activeJobs atomic.Int32 // reconstructGroup single-flights node-side session reconstruction per session @@ -120,11 +125,12 @@ type Server struct { reconstructSemOnce sync.Once reconstructSem chan struct{} - // lifecycleMu guards lifecycleLocks, the per-session mutexes that serialize + // lifecycleMu guards lifecycleLocks, the per-session locks that serialize // every path which spawns ffmpeg into a session's output dir (fresh start and // reconstruct). reconstructGroup only single-flights reconstructs against each // other; without this a reconstruct racing a fresh /transcode/start could run - // two ffmpeg writers against the same dir. + // two ffmpeg writers against the same dir. Artifact readers use the shared side + // so concurrent relays remain independent while prepare/delete stay exclusive. lifecycleMu sync.Mutex lifecycleLocks map[string]*sessionLifecycleLock @@ -133,19 +139,17 @@ type Server struct { recipeStore recipeStore } -// sessionLifecycleLock is a refcounted per-session mutex; the refcount lets the +// sessionLifecycleLock is a refcounted per-session lock; the refcount lets the // node drop the map entry once no path holds or waits on it so the map stays // bounded over the node's lifetime. type sessionLifecycleLock struct { - mu sync.Mutex + mu sync.RWMutex refs int } -// lockSessionLifecycle acquires the per-session lifecycle mutex and returns a -// release func. Held across "check existing → spawn → register" so a fresh start -// and a reconstruct never run concurrent ffmpeg writers for one session's dir. -func (s *Server) lockSessionLifecycle(sessionID string) func() { +func (s *Server) retainSessionLifecycleLock(sessionID string) *sessionLifecycleLock { s.lifecycleMu.Lock() + defer s.lifecycleMu.Unlock() if s.lifecycleLocks == nil { s.lifecycleLocks = make(map[string]*sessionLifecycleLock) } @@ -155,17 +159,39 @@ func (s *Server) lockSessionLifecycle(sessionID string) func() { s.lifecycleLocks[sessionID] = lk } lk.refs++ - s.lifecycleMu.Unlock() + return lk +} +func (s *Server) releaseSessionLifecycleLock(sessionID string, lk *sessionLifecycleLock) { + s.lifecycleMu.Lock() + defer s.lifecycleMu.Unlock() + lk.refs-- + if lk.refs == 0 { + delete(s.lifecycleLocks, sessionID) + } +} + +// lockSessionLifecycle acquires the per-session lifecycle mutex and returns a +// release func. Held across "check existing → spawn → register" so a fresh start +// and a reconstruct never run concurrent ffmpeg writers for one session's dir. +func (s *Server) lockSessionLifecycle(sessionID string) func() { + lk := s.retainSessionLifecycleLock(sessionID) lk.mu.Lock() return func() { lk.mu.Unlock() - s.lifecycleMu.Lock() - lk.refs-- - if lk.refs == 0 { - delete(s.lifecycleLocks, sessionID) - } - s.lifecycleMu.Unlock() + s.releaseSessionLifecycleLock(sessionID, lk) + } +} + +// lockSessionLifecycleRead holds the shared side of a lifecycle lock. Artifact +// relays can therefore proceed concurrently, while preparation and deletion +// remain exclusive for the full transfer. +func (s *Server) lockSessionLifecycleRead(sessionID string) func() { + lk := s.retainSessionLifecycleLock(sessionID) + lk.mu.RLock() + return func() { + lk.mu.RUnlock() + s.releaseSessionLifecycleLock(sessionID, lk) } } @@ -194,11 +220,27 @@ func NewServer(watcher *nodeconfig.Watcher, tracker *nodesessions.Tracker) *Serv if tracker != nil { trackerImpl = tracker } + transcodeDir := config.DefaultTranscodeDir + artifactDir := "" + if watcher != nil { + if cfg := watcher.Config(); cfg != nil { + if strings.TrimSpace(cfg.Playback.TranscodeDir) != "" { + transcodeDir = cfg.Playback.TranscodeDir + } + artifactDir = cfg.Download.ArtifactDir + } + } + artifactRoot := filepath.Join(transcodeDir, downloadprepare.ArtifactDirectoryName) + if strings.TrimSpace(artifactDir) != "" { + artifactRoot = config.EffectiveDownloadArtifactDir(artifactDir, transcodeDir) + } s := &Server{ - watcher: watcher, - tracker: trackerImpl, - sessions: make(map[string]*playback.TranscodeSession), - lastAccess: make(map[string]time.Time), + watcher: watcher, + tracker: trackerImpl, + transcodeDir: transcodeDir, + artifactRoot: artifactRoot, + sessions: make(map[string]*playback.TranscodeSession), + lastAccess: make(map[string]time.Time), } return s } @@ -214,22 +256,14 @@ func NewServer(watcher *nodeconfig.Watcher, tracker *nodesessions.Tracker) *Serv // writing into TranscodeDir/: a dir younger than the max token // lifetime may still be reused, while older dirs are never reconstructable. func (s *Server) StartOrphanSweeper(ctx context.Context) { - dir := "" - if cfg := s.watcher.Config(); cfg != nil { - dir = cfg.Playback.TranscodeDir - } + dir := s.transcodeDir playback.StartPeriodicOrphanCleanup(ctx, "transcodenode", dir, func() (int, error) { - // Re-read config each run so a hot-reloaded TranscodeDir is honored. - cfg := s.watcher.Config() - if cfg == nil { - return 0, nil - } // Spare the live registered jobs by id, not by age alone: now that the // sweep runs during live traffic, a long-lived session that re-serves // already-written segments stops advancing its dir mtime, so the age // guard could misclassify it as orphaned. The live set is authoritative // (in-flight reconstructs are covered by their fresh writes + age guard). - return playback.CleanupOrphanedTranscodeDirs(cfg.Playback.TranscodeDir, s.activeSessionIDs(), playback.MaxTokenTTL) + return playback.CleanupOrphanedTranscodeDirs(dir, s.activeSessionIDs(), playback.MaxTokenTTL) }, playback.OrphanCleanupInterval) } @@ -239,10 +273,23 @@ func (s *Server) StartOrphanSweeper(ctx context.Context) { func (s *Server) activeSessionIDs() map[string]struct{} { s.mu.RLock() defer s.mu.RUnlock() - active := make(map[string]struct{}, len(s.sessions)) + active := make(map[string]struct{}, len(s.sessions)+1) for id := range s.sessions { active[id] = struct{}{} } + // Node-local prepared downloads may live under the existing persistent + // transcode volume. They have their own lifecycle and must never be mistaken + // for an orphaned HLS session directory. Protect the top-level container for + // both the default and an explicitly nested artifact directory. + if s.transcodeDir != "" && s.artifactRoot != "" { + if rel, err := filepath.Rel(s.transcodeDir, s.artifactRoot); err == nil && rel != "." && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + if first, _, ok := strings.Cut(rel, string(filepath.Separator)); ok { + active[first] = struct{}{} + } else { + active[rel] = struct{}{} + } + } + } return active } @@ -409,6 +456,9 @@ func (s *Server) Handler() http.Handler { r.Get("/hw-capabilities", s.handleHWCapabilities) r.Post("/chapter-thumbnails/extract", s.handleChapterThumbnailExtract) r.Post("/downloads/prepare", s.handleDownloadPrepare) + r.Head("/downloads/artifacts/{artifact_id}", s.handleDownloadArtifact) + r.Get("/downloads/artifacts/{artifact_id}", s.handleDownloadArtifact) + r.Delete("/downloads/artifacts/{artifact_id}", s.handleDeleteDownloadArtifact) r.Post("/transcode/start", s.handleStart) r.Delete("/transcode/{session_id}", s.handleStop) r.Get("/transcode/{session_id}/master.m3u8", s.handleManifest) @@ -425,8 +475,8 @@ func (s *Server) handleDownloadPrepare(w http.ResponseWriter, r *http.Request) { http.Error(w, "invalid request body", http.StatusBadRequest) return } - if strings.TrimSpace(req.JobID) == "" || strings.TrimSpace(req.InputPath) == "" || strings.TrimSpace(req.OutputPath) == "" { - http.Error(w, "job_id, input_path, and output_path are required", http.StatusBadRequest) + if !downloadprepare.ValidArtifactID(req.ArtifactID) || strings.TrimSpace(req.InputPath) == "" { + http.Error(w, "a valid artifact_id and input_path are required", http.StatusBadRequest) return } @@ -438,13 +488,16 @@ func (s *Server) handleDownloadPrepare(w http.ResponseWriter, r *http.Request) { if !s.requireApprovedInputPath(w, r, req.InputPath) { return } - artifactRoot := config.EffectiveDownloadArtifactDir(cfg.Download.ArtifactDir, cfg.Playback.TranscodeDir) + artifactRoot := s.artifactRoot if err := os.MkdirAll(artifactRoot, 0o755); err != nil { http.Error(w, "artifact directory unavailable", http.StatusInternalServerError) return } - if !pathWithinRoot(artifactRoot, req.OutputPath) || !strings.EqualFold(filepath.Ext(req.OutputPath), ".mp4") { - http.Error(w, "output_path must be an mp4 under the configured artifact directory", http.StatusBadRequest) + outputPath := filepath.Join(artifactRoot, req.ArtifactID+".mp4") + unlock := s.lockSessionLifecycle("download-artifact-" + req.ArtifactID) + defer unlock() + if stat, err := os.Stat(outputPath); err == nil && stat.Mode().IsRegular() && stat.Size() > 0 { + writeDownloadPrepareResult(w, req.ArtifactID, stat.Size()) return } @@ -453,7 +506,7 @@ func (s *Server) handleDownloadPrepare(w http.ResponseWriter, r *http.Request) { defer s.activeJobs.Add(-1) if s.tracker != nil { finishTracking := s.trackDownloadPrepare(jobCtx, nodesessions.SessionInfo{ - SessionID: "download-" + req.JobID, + SessionID: "download-" + req.ArtifactID, NodeURL: s.tracker.NodeURL(), NodeName: s.tracker.NodeName(), Type: "download_prepare", @@ -466,16 +519,89 @@ func (s *Server) handleDownloadPrepare(w http.ResponseWriter, r *http.Request) { } opts := req.TranscodeOpts(cfg.Playback.FFmpegPath, cfg.Playback.HWAccel, cfg.Playback.HWDevice, s.ffmpegSink) - if err := playback.PrepareFile(jobCtx, opts, req.OutputPath); err != nil { + if err := playback.PrepareFile(jobCtx, opts, outputPath); err != nil { if jobCtx.Err() == nil { - slog.ErrorContext(jobCtx, "prepare download artifact", "component", "transcodenode", "job_id", req.JobID, "error", err) + slog.ErrorContext(jobCtx, "prepare download artifact", "component", "transcodenode", "artifact_id", req.ArtifactID, "error", err) } http.Error(w, "failed to prepare download artifact", http.StatusInternalServerError) return } + stat, err := os.Stat(outputPath) + if err != nil || !stat.Mode().IsRegular() { + http.Error(w, "prepared download artifact unavailable", http.StatusInternalServerError) + return + } + writeDownloadPrepareResult(w, req.ArtifactID, stat.Size()) +} +func writeDownloadPrepareResult(w http.ResponseWriter, artifactID string, fileSize int64) { w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]string{"job_id": req.JobID, "status": "ready"}) + _ = json.NewEncoder(w).Encode(downloadprepare.Result{ArtifactID: artifactID, FileSize: fileSize}) +} + +func (s *Server) sessionOutputDir(sessionID string) string { + return filepath.Join(s.transcodeDir, sessionID) +} + +func (s *Server) handleDownloadArtifact(w http.ResponseWriter, r *http.Request) { + artifactID := chi.URLParam(r, "artifact_id") + if !downloadprepare.ValidArtifactID(artifactID) { + http.NotFound(w, r) + return + } + cfg := s.watcher.Config() + if cfg == nil { + http.Error(w, "node not configured", http.StatusServiceUnavailable) + return + } + // Share the lock with other readers, but serialize with preparation and + // deletion. A recovery HEAD must not report a definitive 404 while + // PrepareFile is still publishing this artifact. + unlock := s.lockSessionLifecycleRead("download-artifact-" + artifactID) + defer unlock() + path := filepath.Join(s.artifactRoot, artifactID+".mp4") + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + http.NotFound(w, r) + return + } + http.Error(w, "artifact unavailable", http.StatusInternalServerError) + return + } + defer func() { _ = f.Close() }() + stat, err := f.Stat() + if err != nil || !stat.Mode().IsRegular() { + http.Error(w, "artifact unavailable", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Disposition", `attachment; filename="`+artifactID+`.mp4"`) + w.Header().Set("Content-Type", playback.MimeFromExtension(path)) + w.Header().Set("ETag", `"`+artifactID+`-`+strconv.FormatInt(stat.Size(), 10)+`"`) + http.ServeContent(w, r, stat.Name(), stat.ModTime(), f) +} + +func (s *Server) handleDeleteDownloadArtifact(w http.ResponseWriter, r *http.Request) { + artifactID := chi.URLParam(r, "artifact_id") + if !downloadprepare.ValidArtifactID(artifactID) { + http.NotFound(w, r) + return + } + cfg := s.watcher.Config() + if cfg == nil { + http.Error(w, "node not configured", http.StatusServiceUnavailable) + return + } + unlock := s.lockSessionLifecycle("download-artifact-" + artifactID) + defer unlock() + path := filepath.Join(s.artifactRoot, artifactID+".mp4") + for _, candidate := range []string{path, path + ".part"} { + if err := os.Remove(candidate); err != nil && !os.IsNotExist(err) { + http.Error(w, "failed to remove artifact", http.StatusInternalServerError) + return + } + } + w.WriteHeader(http.StatusNoContent) } // trackDownloadPrepare runs the monitoring lifecycle off the request path. @@ -598,13 +724,15 @@ func (s *Server) handleStart(w http.ResponseWriter, r *http.Request) { if !s.requireApprovedInputPath(w, r, req.InputPath) { return } + s.reloadMu.RLock() + defer s.reloadMu.RUnlock() cfg := s.watcher.Config() if cfg == nil { http.Error(w, "node not configured", http.StatusServiceUnavailable) return } - outputDir := filepath.Join(cfg.Playback.TranscodeDir, req.SessionID) + outputDir := s.sessionOutputDir(req.SessionID) opts := playback.TranscodeOpts{ InputPath: req.InputPath, @@ -841,6 +969,8 @@ func (s *Server) spawnReconstruct(r *http.Request, sessionID string, requestedSe return nil } defer release() + s.reloadMu.RLock() + defer s.reloadMu.RUnlock() // Serialize against a concurrent fresh /transcode/start for this session so the // two never run ffmpeg writers against the same dir. Re-check under the lock and @@ -858,7 +988,7 @@ func (s *Server) spawnReconstruct(r *http.Request, sessionID string, requestedSe if cfg == nil { return nil } - outputDir := filepath.Join(cfg.Playback.TranscodeDir, sessionID) + outputDir := s.sessionOutputDir(sessionID) opts := card.TranscodeOpts(outputDir, cfg.Playback.FFmpegPath, s.ffmpegSink) opts.SessionID = sessionID // Re-resolve environment-specific encode knobs from this node's live config; the @@ -976,9 +1106,9 @@ func (s *Server) handleStop(w http.ResponseWriter, r *http.Request) { slog.ErrorContext(r.Context(), "close transcode session", "component", "transcodenode", "error", err, "session", sessionID, "playback_session_id", sessionID) } - cfg := s.watcher.Config() - outputDir := filepath.Join(cfg.Playback.TranscodeDir, sessionID) - os.RemoveAll(outputDir) + if err := os.RemoveAll(s.sessionOutputDir(sessionID)); err != nil { + slog.WarnContext(r.Context(), "remove transcode session directory", "component", "transcodenode", "session", sessionID, "error", err) + } // Drop the recipe so a buffered/retrying request after a node restart cannot // reconstruct a new ffmpeg for this now-stopped session. Best-effort: a stop @@ -1164,16 +1294,19 @@ func (s *Server) handleSegment(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleForceReload(w http.ResponseWriter, r *http.Request) { + s.reloadMu.Lock() + defer s.reloadMu.Unlock() if err := s.watcher.ForceReload(r.Context()); err != nil { http.Error(w, "reload failed: "+err.Error(), http.StatusInternalServerError) return } - cfg := s.watcher.Config() s.mu.Lock() stopped := make([]string, 0, len(s.sessions)) for id, session := range s.sessions { session.Close() - os.RemoveAll(filepath.Join(cfg.Playback.TranscodeDir, id)) + if err := os.RemoveAll(s.sessionOutputDir(id)); err != nil { + slog.WarnContext(r.Context(), "remove transcode session directory during reload", "component", "transcodenode", "session", id, "error", err) + } delete(s.sessions, id) delete(s.lastAccess, id) stopped = append(stopped, id) diff --git a/internal/transcodenode/server_test.go b/internal/transcodenode/server_test.go index 98d8a5a0..34efe304 100644 --- a/internal/transcodenode/server_test.go +++ b/internal/transcodenode/server_test.go @@ -42,6 +42,29 @@ type blockingSessionTracker struct { removeHasDeadline bool } +type blockingResponseWriter struct { + header http.Header + writeStarted chan struct{} + releaseWrite chan struct{} + startOnce sync.Once +} + +func newBlockingResponseWriter() *blockingResponseWriter { + return &blockingResponseWriter{ + header: make(http.Header), + writeStarted: make(chan struct{}), + releaseWrite: make(chan struct{}), + } +} + +func (w *blockingResponseWriter) Header() http.Header { return w.header } +func (*blockingResponseWriter) WriteHeader(int) {} +func (w *blockingResponseWriter) Write(p []byte) (int, error) { + w.startOnce.Do(func() { close(w.writeStarted) }) + <-w.releaseWrite + return len(p), nil +} + func newBlockingSessionTracker() *blockingSessionTracker { return &blockingSessionTracker{ trackStarted: make(chan struct{}), @@ -85,11 +108,43 @@ func newTestServer(t *testing.T) *Server { cfg := &config.Config{} cfg.Auth.JWTSecret = testSecret cfg.Playback.TranscodeDir = t.TempDir() + cfg.Download.ArtifactDir = filepath.Join(cfg.Playback.TranscodeDir, downloadprepare.ArtifactDirectoryName) w.SetConfigForTest(cfg) return &Server{ - watcher: w, - inputPaths: allowInputPaths{}, - sessions: make(map[string]*playback.TranscodeSession), + watcher: w, + inputPaths: allowInputPaths{}, + transcodeDir: cfg.Playback.TranscodeDir, + artifactRoot: filepath.Join(cfg.Playback.TranscodeDir, downloadprepare.ArtifactDirectoryName), + sessions: make(map[string]*playback.TranscodeSession), + } +} + +func TestNewServerUsesConfiguredDownloadArtifactDir(t *testing.T) { + w := nodeconfig.NewWatcher(nil, nil, nil, nodeconfig.BootstrapOverrides{}) + cfg := &config.Config{} + cfg.Playback.TranscodeDir = filepath.Join(t.TempDir(), "transcodes") + cfg.Download.ArtifactDir = filepath.Join(t.TempDir(), "prepared-downloads") + w.SetConfigForTest(cfg) + + server := NewServer(w, nil) + if server.artifactRoot != cfg.Download.ArtifactDir { + t.Fatalf("artifact root = %q, want configured %q", server.artifactRoot, cfg.Download.ArtifactDir) + } +} + +func TestNewServerKeepsDefaultDownloadArtifactsInsideTranscodeVolume(t *testing.T) { + w := nodeconfig.NewWatcher(nil, nil, nil, nodeconfig.BootstrapOverrides{}) + cfg := &config.Config{} + cfg.Playback.TranscodeDir = filepath.Join(t.TempDir(), "transcodes") + w.SetConfigForTest(cfg) + + server := NewServer(w, nil) + want := filepath.Join(cfg.Playback.TranscodeDir, downloadprepare.ArtifactDirectoryName) + if server.artifactRoot != want { + t.Fatalf("artifact root = %q, want mounted path %q", server.artifactRoot, want) + } + if _, protected := server.activeSessionIDs()[downloadprepare.ArtifactDirectoryName]; !protected { + t.Fatal("default artifact directory is not protected from the transcode orphan sweep") } } @@ -125,21 +180,21 @@ func TestHandleStartRequireReadyRejectsExitedFFmpeg(t *testing.T) { } } -func TestHandleDownloadPrepareWritesConfiguredSharedArtifact(t *testing.T) { +func TestHandleDownloadPrepareKeepsStartupArtifactRootAcrossReload(t *testing.T) { server := newTestServer(t) - artifactDir := t.TempDir() - server.watcher.Config().Download.ArtifactDir = artifactDir + artifactDir := server.artifactRoot + server.watcher.Config().Download.ArtifactDir = t.TempDir() + server.watcher.Config().Playback.TranscodeDir = t.TempDir() ffmpegPath := filepath.Join(t.TempDir(), "ffmpeg.sh") - if err := os.WriteFile(ffmpegPath, []byte("#!/bin/sh\nfor last; do :; done\ntouch \"$last\"\n"), 0o755); err != nil { + if err := os.WriteFile(ffmpegPath, []byte("#!/bin/sh\nfor last; do :; done\nprintf artifact > \"$last\"\n"), 0o755); err != nil { t.Fatal(err) } server.watcher.Config().Playback.FFmpegPath = ffmpegPath server.watcher.Config().Playback.HWAccel = "none" - outputPath := filepath.Join(artifactDir, "artifact.mp4") + outputPath := filepath.Join(artifactDir, "artifact-1.mp4") body, err := json.Marshal(downloadprepare.Request{ - JobID: "artifact-1", + ArtifactID: "artifact-1", InputPath: "/media/movie.mkv", - OutputPath: outputPath, TargetCodecVideo: "copy", TargetCodecAudio: "copy", AudioTrackIndex: -1, @@ -154,20 +209,79 @@ func TestHandleDownloadPrepareWritesConfiguredSharedArtifact(t *testing.T) { if rr.Code != http.StatusOK { t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) } - if _, err := os.Stat(outputPath); err != nil { + info, err := os.Stat(outputPath) + if err != nil { t.Fatalf("prepared output: %v", err) } + responseBody := rr.Body.String() + var result downloadprepare.Result + if err := json.Unmarshal([]byte(responseBody), &result); err != nil { + t.Fatal(err) + } + if result.ArtifactID != "artifact-1" || result.FileSize != info.Size() || strings.Contains(responseBody, artifactDir) { + t.Fatalf("prepare result = %+v body=%s", result, responseBody) + } if got := server.activeJobs.Load(); got != 0 { t.Fatalf("active jobs after prepare = %d, want 0", got) } } +func TestSessionOutputDirKeepsStartupPathAcrossReload(t *testing.T) { + server := newTestServer(t) + startupDir := server.transcodeDir + server.watcher.Config().Playback.TranscodeDir = t.TempDir() + + if got, want := server.sessionOutputDir("session-1"), filepath.Join(startupDir, "session-1"); got != want { + t.Fatalf("session output dir = %q, want startup path %q", got, want) + } +} + +func TestHandleStartWaitsForForceReloadGate(t *testing.T) { + server := newTestServer(t) + ffmpegPath := filepath.Join(t.TempDir(), "failing-ffmpeg.sh") + if err := os.WriteFile(ffmpegPath, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil { + t.Fatal(err) + } + server.watcher.Config().Playback.FFmpegPath = ffmpegPath + body, err := json.Marshal(TranscodeStartRequest{ + SessionID: "reload-gated-start", InputPath: "/media/movie.mkv", + TargetCodecVideo: "h264", TargetCodecAudio: "aac", SegmentDuration: 2, RequireReady: true, + }) + if err != nil { + t.Fatal(err) + } + req := httptest.NewRequest(http.MethodPost, "/transcode/start", bytes.NewReader(body)) + rr := httptest.NewRecorder() + done := make(chan struct{}) + server.reloadMu.Lock() + locked := true + defer func() { + if locked { + server.reloadMu.Unlock() + } + }() + go func() { + server.handleStart(rr, req) + close(done) + }() + select { + case <-done: + t.Fatal("start completed while force-reload gate was held") + case <-time.After(50 * time.Millisecond): + } + server.reloadMu.Unlock() + locked = false + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("start did not resume after force-reload gate was released") + } +} + func TestHandleDownloadPrepareTrackingDoesNotBlockAndRemovesAfterTrack(t *testing.T) { server := newTestServer(t) tracker := newBlockingSessionTracker() server.tracker = tracker - artifactDir := t.TempDir() - server.watcher.Config().Download.ArtifactDir = artifactDir ffmpegPath := filepath.Join(t.TempDir(), "ffmpeg.sh") if err := os.WriteFile(ffmpegPath, []byte("#!/bin/sh\nfor last; do :; done\ntouch \"$last\"\n"), 0o755); err != nil { t.Fatal(err) @@ -175,9 +289,8 @@ func TestHandleDownloadPrepareTrackingDoesNotBlockAndRemovesAfterTrack(t *testin server.watcher.Config().Playback.FFmpegPath = ffmpegPath server.watcher.Config().Playback.HWAccel = "none" body, err := json.Marshal(downloadprepare.Request{ - JobID: "artifact-tracking", + ArtifactID: "artifact-tracking", InputPath: "/media/movie.mkv", - OutputPath: filepath.Join(artifactDir, "artifact.mp4"), TargetCodecVideo: "copy", TargetCodecAudio: "copy", AudioTrackIndex: -1, @@ -227,13 +340,11 @@ func TestHandleDownloadPrepareTrackingDoesNotBlockAndRemovesAfterTrack(t *testin } } -func TestHandleDownloadPrepareRejectsOutputOutsideArtifactRoot(t *testing.T) { +func TestHandleDownloadPrepareRejectsInvalidArtifactID(t *testing.T) { server := newTestServer(t) - server.watcher.Config().Download.ArtifactDir = t.TempDir() body, err := json.Marshal(downloadprepare.Request{ - JobID: "artifact-2", + ArtifactID: "../artifact-2", InputPath: "/media/movie.mkv", - OutputPath: filepath.Join(t.TempDir(), "escape.mp4"), TargetCodecVideo: "h264", TargetCodecAudio: "aac", }) @@ -252,7 +363,7 @@ func TestHandleDownloadPrepareRejectsOutputOutsideArtifactRoot(t *testing.T) { func TestHandleDownloadPrepareRejectsUnavailableConfig(t *testing.T) { server := newTestServer(t) server.watcher.SetConfigForTest(nil) - body := []byte(`{"job_id":"artifact-3","input_path":"/media/movie.mkv","output_path":"/artifacts/movie.mp4"}`) + body := []byte(`{"artifact_id":"artifact-3","input_path":"/media/movie.mkv"}`) req := httptest.NewRequest(http.MethodPost, "/downloads/prepare", bytes.NewReader(body)) rr := httptest.NewRecorder() @@ -285,6 +396,153 @@ func TestDownloadPrepareRouteRequiresNodeBearer(t *testing.T) { } } +func TestDownloadArtifactRoutesServeRangeAndDeleteNodeLocalFile(t *testing.T) { + server := newTestServer(t) + root := server.artifactRoot + if want := filepath.Join(server.watcher.Config().Playback.TranscodeDir, downloadprepare.ArtifactDirectoryName); root != want { + t.Fatalf("artifact root = %q, want %q", root, want) + } + if _, protected := server.activeSessionIDs()[downloadprepare.ArtifactDirectoryName]; !protected { + t.Fatal("artifact directory is not protected from the orphan transcode sweep") + } + if err := os.MkdirAll(root, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(root, "artifact-range.mp4") + if err := os.WriteFile(path, []byte("0123456789"), 0o600); err != nil { + t.Fatal(err) + } + + req := httptest.NewRequest(http.MethodGet, "/downloads/artifacts/artifact-range", nil) + req.Header.Set("Authorization", "Bearer "+testSecret) + req.Header.Set("Range", "bytes=3-6") + rr := httptest.NewRecorder() + server.Handler().ServeHTTP(rr, req) + if rr.Code != http.StatusPartialContent || rr.Body.String() != "3456" { + t.Fatalf("GET status=%d body=%q", rr.Code, rr.Body.String()) + } + if got := rr.Header().Get("Content-Range"); got != "bytes 3-6/10" { + t.Fatalf("Content-Range = %q", got) + } + + deleteReq := httptest.NewRequest(http.MethodDelete, "/downloads/artifacts/artifact-range", nil) + deleteReq.Header.Set("Authorization", "Bearer "+testSecret) + deleteRR := httptest.NewRecorder() + server.Handler().ServeHTTP(deleteRR, deleteReq) + if deleteRR.Code != http.StatusNoContent { + t.Fatalf("DELETE status=%d body=%s", deleteRR.Code, deleteRR.Body.String()) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("artifact still exists: %v", err) + } +} + +func TestDownloadArtifactHeadWaitsForInFlightPrepare(t *testing.T) { + server := newTestServer(t) + const artifactID = "artifact-in-flight" + unlocks := server.lockSessionLifecycle("download-artifact-" + artifactID) + req := httptest.NewRequest(http.MethodHead, "/downloads/artifacts/"+artifactID, nil) + req.SetPathValue("artifact_id", artifactID) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("artifact_id", artifactID) + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + rr := httptest.NewRecorder() + done := make(chan struct{}) + go func() { + server.handleDownloadArtifact(rr, req) + close(done) + }() + select { + case <-done: + unlocks() + t.Fatal("HEAD reported a result while preparation held the artifact lock") + case <-time.After(50 * time.Millisecond): + } + path := filepath.Join(server.artifactRoot, artifactID+".mp4") + if err := os.MkdirAll(server.artifactRoot, 0o755); err != nil { + unlocks() + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("ready"), 0o600); err != nil { + unlocks() + t.Fatal(err) + } + unlocks() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("HEAD did not resume after preparation published the artifact") + } + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", rr.Code, rr.Body.String()) + } +} + +func TestDownloadArtifactHeadDoesNotWaitForConcurrentRelay(t *testing.T) { + server := newTestServer(t) + const artifactID = "artifact-concurrent-read" + if err := os.MkdirAll(server.artifactRoot, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(server.artifactRoot, artifactID+".mp4") + if err := os.WriteFile(path, []byte("ready"), 0o600); err != nil { + t.Fatal(err) + } + + blocked := newBlockingResponseWriter() + releaseOnce := sync.Once{} + release := func() { releaseOnce.Do(func() { close(blocked.releaseWrite) }) } + defer release() + getDone := make(chan struct{}) + go func() { + req := httptest.NewRequest(http.MethodGet, "/downloads/artifacts/"+artifactID, nil) + req.Header.Set("Authorization", "Bearer "+testSecret) + server.Handler().ServeHTTP(blocked, req) + close(getDone) + }() + + select { + case <-blocked.writeStarted: + case <-time.After(2 * time.Second): + t.Fatal("GET did not begin relaying the artifact") + } + + headDone := make(chan *httptest.ResponseRecorder, 1) + go func() { + req := httptest.NewRequest(http.MethodHead, "/downloads/artifacts/"+artifactID, nil) + req.Header.Set("Authorization", "Bearer "+testSecret) + rr := httptest.NewRecorder() + server.Handler().ServeHTTP(rr, req) + headDone <- rr + }() + select { + case rr := <-headDone: + if rr.Code != http.StatusOK { + t.Fatalf("HEAD status = %d, body = %s", rr.Code, rr.Body.String()) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("HEAD waited for a concurrent artifact relay") + } + + release() + select { + case <-getDone: + case <-time.After(2 * time.Second): + t.Fatal("GET did not finish after the response writer was released") + } +} + +func TestDownloadArtifactRoutesRequireBearer(t *testing.T) { + server := newTestServer(t) + for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodDelete} { + rr := httptest.NewRecorder() + server.Handler().ServeHTTP(rr, httptest.NewRequest(method, "/downloads/artifacts/artifact-1", nil)) + if rr.Code != http.StatusUnauthorized { + t.Fatalf("%s status=%d body=%s", method, rr.Code, rr.Body.String()) + } + } +} + func TestHandleStartDistinctReplacementFailurePreservesPredecessor(t *testing.T) { server := newTestServer(t) server.sessions["public-session"] = &playback.TranscodeSession{} diff --git a/migrations/sql/20260812033954_add_download_artifact_origin.sql b/migrations/sql/20260812033954_add_download_artifact_origin.sql new file mode 100644 index 00000000..d4e8d465 --- /dev/null +++ b/migrations/sql/20260812033954_add_download_artifact_origin.sql @@ -0,0 +1,20 @@ +-- +goose Up +ALTER TABLE public.download_artifacts + ADD COLUMN origin_node_id integer NOT NULL DEFAULT 0, + ADD COLUMN origin_node_url text NOT NULL DEFAULT '', + ADD COLUMN origin_node_group text NOT NULL DEFAULT '', + ADD COLUMN origin_artifact_id text NOT NULL DEFAULT '', + ADD CONSTRAINT download_artifacts_origin_check + CHECK ( + (origin_node_id = 0 AND origin_node_url = '' AND origin_node_group = '' AND origin_artifact_id = '') + OR + (origin_node_id > 0 AND origin_node_url <> '' AND origin_artifact_id <> '') + ) NOT VALID; + +-- +goose Down +ALTER TABLE public.download_artifacts + DROP CONSTRAINT download_artifacts_origin_check, + DROP COLUMN origin_artifact_id, + DROP COLUMN origin_node_group, + DROP COLUMN origin_node_url, + DROP COLUMN origin_node_id; diff --git a/migrations/sql/20260812045218_add_download_artifact_orphans.sql b/migrations/sql/20260812045218_add_download_artifact_orphans.sql new file mode 100644 index 00000000..f6fcf437 --- /dev/null +++ b/migrations/sql/20260812045218_add_download_artifact_orphans.sql @@ -0,0 +1,20 @@ +-- +goose Up +CREATE TABLE public.download_artifact_orphans ( + id bigserial PRIMARY KEY, + download_artifact_id text NOT NULL, + origin_node_id integer NOT NULL, + origin_node_url text NOT NULL, + origin_artifact_id text NOT NULL, + attempts integer NOT NULL DEFAULT 0, + next_retry_at timestamptz, + created_at timestamptz NOT NULL DEFAULT now(), + UNIQUE (origin_node_id, origin_artifact_id), + CONSTRAINT download_artifact_orphans_locator_check + CHECK (download_artifact_id <> '' AND origin_node_id > 0 AND origin_node_url <> '' AND origin_artifact_id <> '') +); + +CREATE INDEX download_artifact_orphans_due_idx + ON public.download_artifact_orphans (next_retry_at, created_at); + +-- +goose Down +DROP TABLE IF EXISTS public.download_artifact_orphans; diff --git a/migrations/sql/20260812144845_validate_download_artifact_origin.sql b/migrations/sql/20260812144845_validate_download_artifact_origin.sql new file mode 100644 index 00000000..881eb62a --- /dev/null +++ b/migrations/sql/20260812144845_validate_download_artifact_origin.sql @@ -0,0 +1,6 @@ +-- +goose Up +ALTER TABLE public.download_artifacts + VALIDATE CONSTRAINT download_artifacts_origin_check; + +-- +goose Down +SELECT 1; diff --git a/migrations/sql/20260812163547_add_download_artifact_orphan_fairness_index.sql b/migrations/sql/20260812163547_add_download_artifact_orphan_fairness_index.sql new file mode 100644 index 00000000..bb7d161a --- /dev/null +++ b/migrations/sql/20260812163547_add_download_artifact_orphan_fairness_index.sql @@ -0,0 +1,6 @@ +-- +goose Up +CREATE INDEX download_artifact_orphans_origin_fairness_idx + ON public.download_artifact_orphans (origin_node_id, origin_node_url, attempts, created_at, id); + +-- +goose Down +DROP INDEX IF EXISTS public.download_artifact_orphans_origin_fairness_idx;