fix hangs and stranded rows in the download import path
uniqueDir treated any os.Stat error as "name taken", so a title the filesystem rejects (ENAMETOOLONG, EACCES on the parent) made it loop forever: the worker spun at 100% CPU, Pool.Stop never returned, and the database was never checkpointed. It now propagates the error, and sanitizeDirName caps names at 240 bytes so an over-long remote title cannot reach that state at all. A failed temp-dir creation returned without finalizing the download, leaving the row at "downloading" until the next restart — and HasActiveForSubscription blocked the subscription for just as long. It now records the error like every other failure path. enumeratePlaylistIDs and probeDuration took a context but not the process-group kill, so cancelling them could not stop a yt-dlp that had spawned a helper holding the output pipe. The three-line incantation now lives in util.KillableCommand and all four call sites use it. Also: the theme form redirected to an unvalidated Referer (open redirect); it now keeps the path only. Drop a duplicate --write-info-json so the recorded flag string matches what ran.
M.gitignore
@@ -28,8 +28,8 @@ go.work.sum
.env
# Editor/IDE
# .idea/
# .vscode/
.idea/
.vscode/
data/
todo.txt
Minternal/handler/handler_test.go
@@ -50,3 +50,29 @@ func TestApplyPresetFormClearsDependentCommentSettings(t *testing.T) {
})
}
}
// The Referer header is attacker-controlled, so the theme form must never
// redirect to the host it names — only back to a path on this site.
func TestLocalRefererKeepsPathOnly(t *testing.T) {
tests := []struct {
referer string
want string
}{
{"", "/"},
{"https://evil.example/phish", "/phish"},
{"//evil.example/phish", "/phish"},
{"https://vidarchive.local/library?path=music", "/library?path=music"},
{"/queue?sort=status", "/queue?sort=status"},
{"not a url", "/"},
}
for _, tc := range tests {
r := httptest.NewRequest("POST", "/theme", nil)
if tc.referer != "" {
r.Header.Set("Referer", tc.referer)
}
if got := localReferer(r); got != tc.want {
t.Errorf("localReferer(%q) = %q, want %q", tc.referer, got, tc.want)
}
}
}
Minternal/handler/health.go
@@ -4,10 +4,10 @@ import (
"context"
"encoding/json"
"net/http"
"os/exec"
"strings"
"syscall"
"time"
"vidarchive/internal/util"
)
// healthProbeTimeout caps the external version lookups so the endpoint always
@@ -57,18 +57,7 @@ func (h *Handler) Health(w http.ResponseWriter, r *http.Request) {
// "not installed" when the binary is missing and "timed out" when it doesn't
// answer within ctx.
func toolVersion(ctx context.Context, path string, versionArg string) string {
cmd := exec.CommandContext(ctx, path, versionArg)
// Kill the whole process group, not just the tool: yt-dlp and ffmpeg spawn
// helpers that inherit the output pipe, and Output() blocks reading it until
// every holder exits — which would defeat the timeout. WaitDelay is the
// backstop for anything that still survives the signal.
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
cmd.Cancel = func() error {
return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
}
cmd.WaitDelay = time.Second
out, err := cmd.Output()
out, err := util.KillableCommand(ctx, path, versionArg).Output()
if err != nil {
if ctx.Err() != nil {
return "timed out"
Minternal/handler/settings.go
@@ -4,6 +4,7 @@ import (
"fmt"
"log"
"net/http"
"net/url"
"strconv"
"strings"
@@ -197,9 +198,19 @@ func (h *Handler) Theme(w http.ResponseWriter, r *http.Request) {
setCookie(w, "theme", theme)
referer := r.Header.Get("Referer")
if referer == "" {
referer = "/"
http.Redirect(w, r, localReferer(r), http.StatusSeeOther)
}
// localReferer returns where to send the user back to. Only the path is kept:
// Referer is attacker-controlled, so honouring its host would make this route
// an open redirect.
func localReferer(r *http.Request) string {
ref, err := url.Parse(r.Header.Get("Referer"))
if err != nil || !strings.HasPrefix(ref.Path, "/") {
return "/"
}
if ref.RawQuery == "" {
return ref.Path
}
http.Redirect(w, r, referer, http.StatusSeeOther)
return ref.Path + "?" + ref.RawQuery
}
Minternal/service/download.go
@@ -8,17 +8,17 @@ import (
"fmt"
"log"
"os"
"os/exec"
"path/filepath"
"slices"
"strconv"
"strings"
"sync"
"syscall"
"time"
"vidarchive/internal/config"
"vidarchive/internal/models"
"vidarchive/internal/repository"
"vidarchive/internal/util"
)
type DownloadService struct {
@@ -282,7 +282,11 @@ func (s *DownloadService) ExecuteDownload(parent context.Context, d *models.Down
tempDownloadDir := s.tempDirFor(d.ID)
if err := os.MkdirAll(tempDownloadDir, 0755); err != nil {
return false, fmt.Errorf("create temp download dir: %w", err)
// MarkStarted already moved the row to "downloading"; returning without
// finalizing would strand it there until the next restart.
err = fmt.Errorf("create temp download dir: %w", err)
s.finalizeError(d, err)
return false, err
}
// Own the temp dir's lifetime here, where it's created, so it's removed on
// every exit path — including a failed yt-dlp run or an early return that
@@ -303,7 +307,9 @@ func (s *DownloadService) ExecuteDownload(parent context.Context, d *models.Down
if sub != nil {
// Always write info.json so the import step can read the stable identity
// (yt-dlp's video id) used to match/replace existing items.
args = append(args, "--write-info-json")
if !slices.Contains(args, "--write-info-json") {
args = append(args, "--write-info-json")
}
switch sub.RefreshMode {
case "skip":
// Let yt-dlp skip entries already recorded — no re-download.
@@ -415,13 +421,7 @@ func (s *DownloadService) runYTDLP(ctx context.Context, d *models.Download, args
// --socket-timeout bounds a stalled connection. Without it a dead socket pins
// a worker forever. There is no inactivity killer beyond this.
fullArgs := append([]string{"--newline", "--no-write-playlist-metafiles", "--socket-timeout", "30"}, args...)
cmd := exec.CommandContext(ctx, s.cfg.YTDLPPath, fullArgs...)
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
// yt-dlp spawns helpers (ffmpeg, external downloaders). Kill the whole group
// rather than just the parent, which would leave those orphaned.
cmd.Cancel = func() error {
return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
}
cmd := util.KillableCommand(ctx, s.cfg.YTDLPPath, fullArgs...)
stdout, err := cmd.StdoutPipe()
if err != nil {
Minternal/service/execute_download_test.go
@@ -669,3 +669,44 @@ func TestExecuteDownloadKeepsUntouchedCookies(t *testing.T) {
t.Errorf("cookies = %q, want them unchanged (%q)", got, stored)
}
}
// A download whose scratch directory can't be created must be finalized as an
// error, not left in "downloading" — which also keeps its subscription busy.
func TestExecuteDownloadTempDirFailureIsError(t *testing.T) {
e := newExecEnv(t)
e.fakeYTDLP(t, `exit 0`)
// A regular file where the temp tree needs a directory makes MkdirAll fail
// with ENOTDIR.
blocker := filepath.Join(e.scratch, "blocker")
if err := os.WriteFile(blocker, nil, 0644); err != nil {
t.Fatal(err)
}
e.cfg.TempDir = filepath.Join(blocker, "temp")
sub := &models.Subscription{Name: "s", URL: "https://example.com", OutputDir: "feed"}
if err := e.subRepo.Create(sub); err != nil {
t.Fatal(err)
}
d := e.queue(t, &models.Download{SubscriptionID: sqlNullInt64(sub.ID)})
if _, err := e.svc.ExecuteDownload(context.Background(), d); err == nil {
t.Fatal("ExecuteDownload succeeded, want an error")
}
got := e.status(t, d.ID)
if got.Status != "error" {
t.Errorf("status = %q, want error", got.Status)
}
if !got.ErrorMessage.Valid || !strings.Contains(got.ErrorMessage.String, "temp download dir") {
t.Errorf("error_message = %q, want it to name the temp dir", got.ErrorMessage.String)
}
active, err := e.svc.HasActiveForSubscription(sub.ID)
if err != nil {
t.Fatal(err)
}
if active {
t.Error("subscription still reported as having an active run")
}
}
Minternal/service/import.go
@@ -8,16 +8,17 @@ import (
"io"
"log"
"os"
"os/exec"
"path/filepath"
"sort"
"strconv"
"strings"
"syscall"
"unicode/utf8"
"github.com/gabriel-vasile/mimetype"
"vidarchive/internal/models"
"vidarchive/internal/util"
)
// resolveBaseLibraryDir returns the absolute library directory a download writes
@@ -153,7 +154,10 @@ func (s *DownloadService) importItemDir(ctx context.Context, url, itemDir, baseL
}
}
targetDir := s.uniqueDir(baseLibraryDir, name)
targetDir, err := s.uniqueDir(baseLibraryDir, name)
if err != nil {
return err
}
if err := os.MkdirAll(targetDir, 0755); err != nil {
return err
}
@@ -282,15 +286,23 @@ func (s *DownloadService) deriveItemName(itemDir string, info infoJSON, mediaFil
return sanitizeDirName(base)
}
func (s *DownloadService) uniqueDir(base, name string) string {
// uniqueDir returns a directory under base that does not exist yet, appending
// "-1", "-2", ... until it finds one. A stat error other than "does not exist"
// is returned rather than treated as "taken": every candidate would fail the
// same way, so the loop would never end.
func (s *DownloadService) uniqueDir(base, name string) (string, error) {
dir := filepath.Join(base, name)
if _, err := os.Stat(dir); os.IsNotExist(err) {
return dir
}
for i := 1; ; i++ {
candidate := fmt.Sprintf("%s-%d", dir, i)
if _, err := os.Stat(candidate); os.IsNotExist(err) {
return candidate
for i := 0; ; i++ {
candidate := dir
if i > 0 {
candidate = fmt.Sprintf("%s-%d", dir, i)
}
_, err := os.Stat(candidate)
if os.IsNotExist(err) {
return candidate, nil
}
if err != nil {
return "", fmt.Errorf("choose item directory: %w", err)
}
}
}
@@ -357,13 +369,36 @@ func sanitizeDirName(name string) string {
if strings.Trim(name, ".") == "" {
name = "untitled"
}
return name
return truncateDirName(name)
}
// maxDirNameBytes leaves room under the 255-byte filesystem component limit for
// uniqueDir's "-N" suffix.
const maxDirNameBytes = 240
// truncateDirName shortens name to maxDirNameBytes without splitting a rune.
// Over the limit, every filesystem call on the name fails with ENAMETOOLONG.
func truncateDirName(name string) string {
if len(name) <= maxDirNameBytes {
return name
}
cut := maxDirNameBytes
for cut > 0 && !utf8.RuneStart(name[cut]) {
cut--
}
truncated := strings.TrimSpace(name[:cut])
// Trimming can reintroduce the empty/all-dots case the caller already ruled
// out.
if strings.Trim(truncated, ".") == "" {
return "untitled"
}
return truncated
}
// probeDuration returns the duration of a media file in whole seconds. The bool
// is false when ffprobe is unavailable or the file has no usable duration.
func probeDuration(ctx context.Context, ffprobePath, path string) (int, bool) {
out, err := exec.CommandContext(ctx, ffprobePath, "-v", "error",
out, err := util.KillableCommand(ctx, ffprobePath, "-v", "error",
"-show_entries", "format=duration",
"-of", "default=nw=1:nk=1", path).Output()
if err != nil {
Minternal/service/import_test.go
@@ -11,6 +11,7 @@ import (
"syscall"
"testing"
"time"
"unicode/utf8"
"vidarchive/internal/config"
"vidarchive/internal/models"
@@ -42,26 +43,69 @@ func TestUniqueDir(t *testing.T) {
base := t.TempDir()
svc := &DownloadService{}
first := svc.uniqueDir(base, "item")
mustUnique := func(name string) string {
t.Helper()
dir, err := svc.uniqueDir(base, name)
if err != nil {
t.Fatalf("uniqueDir(%q): %v", name, err)
}
return dir
}
first := mustUnique("item")
if filepath.Base(first) != "item" {
t.Errorf("first uniqueDir = %q, want .../item", first)
}
if err := os.MkdirAll(first, 0755); err != nil {
t.Fatal(err)
}
second := svc.uniqueDir(base, "item")
second := mustUnique("item")
if filepath.Base(second) != "item-1" {
t.Errorf("second uniqueDir = %q, want .../item-1", second)
}
if err := os.MkdirAll(second, 0755); err != nil {
t.Fatal(err)
}
third := svc.uniqueDir(base, "item")
third := mustUnique("item")
if filepath.Base(third) != "item-2" {
t.Errorf("third uniqueDir = %q, want .../item-2", third)
}
}
// A name the filesystem rejects makes every os.Stat fail with something other
// than ENOENT. uniqueDir must report that instead of looping forever looking
// for a free suffix.
func TestUniqueDirRejectsUnusableName(t *testing.T) {
svc := &DownloadService{}
done := make(chan struct{})
go func() {
defer close(done)
if _, err := svc.uniqueDir(t.TempDir(), strings.Repeat("a", 300)); err == nil {
t.Error("uniqueDir accepted an over-long name, want an error")
}
}()
select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("uniqueDir did not return: it is looping on a non-ENOENT stat error")
}
}
// Titles come from remote metadata, where nothing bounds their length.
func TestSanitizeDirNameBoundsLength(t *testing.T) {
for _, in := range []string{strings.Repeat("a", 500), strings.Repeat("ミク", 300)} {
got := sanitizeDirName(in)
if len(got) > maxDirNameBytes {
t.Errorf("sanitizeDirName(%d bytes) = %d bytes, want <= %d", len(in), len(got), maxDirNameBytes)
}
if !utf8.ValidString(got) {
t.Errorf("sanitizeDirName(%d bytes) split a rune: %q", len(in), got)
}
}
}
func TestDeriveItemName(t *testing.T) {
svc := &DownloadService{}
itemDir := t.TempDir()
Minternal/service/subscription_run.go
@@ -6,11 +6,12 @@ import (
"fmt"
"log"
"os"
"os/exec"
"path/filepath"
"slices"
"strings"
"vidarchive/internal/models"
"vidarchive/internal/util"
)
// refreshAndAddNew handles a metadata-mode run. The main pass used
@@ -85,7 +86,9 @@ func (s *DownloadService) downloadFresh(ctx context.Context, d *models.Download,
args := s.presetSvc.BuildArgs(preset, d.FormatOverride, d.CustomFlags)
args, cleanup := s.appendCookies(args)
defer cleanup()
args = append(args, "--write-info-json")
if !slices.Contains(args, "--write-info-json") {
args = append(args, "--write-info-json")
}
args = append(args, "-P", tempDir)
args = append(args, "-o", "item-%(autonumber)05d/%(title)s.%(ext)s")
args = append(args, urls...)
@@ -301,7 +304,7 @@ func (s *DownloadService) enumeratePlaylistIDs(ctx context.Context, url string)
defer cleanup()
args = append(args, url)
out, err := exec.CommandContext(ctx, s.cfg.YTDLPPath, args...).Output()
out, err := util.KillableCommand(ctx, s.cfg.YTDLPPath, args...).Output()
if err != nil {
return nil, err
}
Ainternal/util/exec.go
@@ -0,0 +1,28 @@
package util
import (
"context"
"os/exec"
"syscall"
"time"
)
const killWaitDelay = time.Second
// KillableCommand builds a command whose cancellation actually stops it, and is
// the single place that plumbing lives — every context-aware external command
// goes through here.
//
// yt-dlp, ffmpeg and ffprobe spawn helpers that inherit the output pipe, and
// both Wait and Output block reading it until every holder exits. So killing
// only the direct child does not end the call; the group signal does, with
// WaitDelay as the backstop for whatever survives it.
func KillableCommand(ctx context.Context, path string, args ...string) *exec.Cmd {
cmd := exec.CommandContext(ctx, path, args...)
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
cmd.Cancel = func() error {
return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
}
cmd.WaitDelay = killWaitDelay
return cmd
}