fix compose credentials, shutdown ordering and cache eviction
Five findings from a review pass. - compose.yml took the bcrypt hash through `environment`, where Compose interpolates `$`. A real hash was corrupted and the quotes became part of the value. Credentials move to `.env` via env_file, which is read literally. Adds .env.example; .env was already ignored. - Scheduler.Stop cancelled the context and returned without waiting. Main closes the database next, so a checkDue still in flight queried a closed handle. Stop now waits on a WaitGroup, the way Pool already did. - Scan cache eviction derived its key from the raw libraryDir, but the cache is keyed off the symlink-resolved root. Under a symlinked library the eviction missed, so a pruned or overwritten item stayed visible for the 10s TTL. Adds evictCachedDir, and resolveBaseLibraryDir now resolves the default case too, which is where the mismatch started. - /healthz refused to cache a "timed out" probe. One wedged tool therefore made every request fork three processes. A timeout is now cached for 10s rather than the full minute. - Nine handlers wrote err.Error() into the response and bypassed serverError. Seven identical ParseForm blocks collapse into a parseForm helper that answers "Invalid form". TestPruneEvictsCacheUnderSymlinkedRoot fails on the old eviction key. TestSchedulerStopWithoutStart catches a Stop that blocks when the loop was never started.
A.env.example
@@ -0,0 +1,8 @@
# Copy to .env, then fill in. compose.yml reads this file with env_file, which
# does no variable interpolation, so a bcrypt hash can be pasted verbatim.
# Quotes are NOT stripped here either, so do not add any.
#
# Generate the hash with:
# htpasswd -bnBC 12 "" 'your-password' | tr -d ':\n'
VIDARCHIVE_USERNAME=admin
VIDARCHIVE_PASSWORD_HASH=
MREADME.md
@@ -24,6 +24,8 @@ Missing tools are reported at startup and on `/healthz`; the app still starts.
With the container image:
```sh
cp .env.example .env
htpasswd -bnBC 12 "" 'your-password' | tr -d ':\n' # paste the output into .env
docker compose up -d # or: podman-compose up -d
```
Mcompose.yml
@@ -10,12 +10,12 @@ services:
- "8080:8080"
volumes:
- ./data:/data
env_file:
- .env
environment:
- VIDARCHIVE_PORT=8080
- VIDARCHIVE_DATA_DIR=/data
- VIDARCHIVE_WORKERS=2
- VIDARCHIVE_USERNAME=admin
- VIDARCHIVE_PASSWORD_HASH="$2y$12$..."
# How often the subscription scheduler looks for due runs, in seconds
- VIDARCHIVE_SCHEDULER_INTERVAL=60
# Uncomment to run behind HTTPS proxy:
Minternal/handler/health.go
@@ -22,11 +22,19 @@ const healthProbeTimeout = 3 * time.Second
// reach the port costs the host three processes per request.
const toolCacheTTL = time.Minute
// toolRetryTTL is the shorter TTL used when a probe timed out. A timeout says
// less about the tool than a real answer, so it is worth re-probing sooner. It
// is still cached, though: not caching it hands an unauthenticated caller three
// forks per request for as long as the tool stays wedged.
const toolRetryTTL = 10 * time.Second
// toolCache holds the last tool version probe. The mutex is held across the
// probe itself, so a burst of requests collapses into one set of forks.
// probe itself, so a burst of requests collapses into one set of forks. ttl
// varies per result; see toolRetryTTL.
type toolCache struct {
mu sync.Mutex
at time.Time
ttl time.Duration
results map[string]string
}
@@ -75,7 +83,7 @@ func (h *Handler) toolVersions(ctx context.Context) map[string]string {
h.tools.mu.Lock()
defer h.tools.mu.Unlock()
if h.tools.results != nil && time.Since(h.tools.at) < toolCacheTTL {
if h.tools.results != nil && time.Since(h.tools.at) < h.tools.ttl {
return h.tools.results
}
@@ -84,10 +92,11 @@ func (h *Handler) toolVersions(ctx context.Context) map[string]string {
"ffmpeg": toolVersion(ctx, h.cfg.FFmpegPath, "-version"),
"ffprobe": toolVersion(ctx, h.cfg.FFprobePath, "-version"),
}
// A timed-out probe says nothing about the tool, so don't cache that verdict.
if !slices.Contains(slices.Collect(maps.Values(results)), "timed out") {
h.tools.results, h.tools.at = results, time.Now()
ttl := toolCacheTTL
if slices.Contains(slices.Collect(maps.Values(results)), "timed out") {
ttl = toolRetryTTL
}
h.tools.results, h.tools.at, h.tools.ttl = results, time.Now(), ttl
return results
}
Minternal/handler/helpers.go
@@ -105,3 +105,14 @@ func redirectWithSuccess(w http.ResponseWriter, r *http.Request, path, message s
flashSuccess(w, message)
http.Redirect(w, r, path, http.StatusSeeOther)
}
// parseForm parses a form body and reports whether it succeeded. On failure it
// writes a 400 with a generic message: ParseForm's own error names the offending
// bytes, which is noise to the user and detail we don't need to hand out.
func parseForm(w http.ResponseWriter, r *http.Request) bool {
if err := r.ParseForm(); err != nil {
http.Error(w, "Invalid form", http.StatusBadRequest)
return false
}
return true
}
Minternal/handler/queue.go
@@ -78,8 +78,7 @@ func (h *Handler) DownloadDetail(w http.ResponseWriter, r *http.Request) {
}
func (h *Handler) CreateDownload(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
Minternal/handler/settings.go
@@ -38,8 +38,7 @@ func (h *Handler) Settings(w http.ResponseWriter, r *http.Request) {
}
func (h *Handler) CreatePreset(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
@@ -118,8 +117,7 @@ func (h *Handler) UpdatePreset(w http.ResponseWriter, r *http.Request) {
return
}
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
@@ -157,8 +155,7 @@ func (h *Handler) DeletePreset(w http.ResponseWriter, r *http.Request) {
}
func (h *Handler) UpdateSettings(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
@@ -186,8 +183,7 @@ func (h *Handler) UpdateSettings(w http.ResponseWriter, r *http.Request) {
}
func (h *Handler) Theme(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
Minternal/handler/subscription.go
@@ -15,12 +15,12 @@ import (
func (h *Handler) Subscriptions(w http.ResponseWriter, r *http.Request) {
subscriptions, err := h.subscriptionSvc.GetAll()
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
h.serverError(w, r, "list subscriptions", err)
return
}
presets, err := h.presetSvc.GetAll()
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
h.serverError(w, r, "list presets", err)
return
}
@@ -89,8 +89,7 @@ func (h *Handler) subscriptionFromForm(r *http.Request) (*models.Subscription, e
}
func (h *Handler) CreateSubscription(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
@@ -116,8 +115,7 @@ func (h *Handler) UpdateSubscription(w http.ResponseWriter, r *http.Request) {
if !ok {
return
}
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
if !parseForm(w, r) {
return
}
Minternal/service/import.go
@@ -25,8 +25,10 @@ import (
// into, applying the optional per-download OutputDir while rejecting any path
// that escapes the library root.
func (s *DownloadService) resolveBaseLibraryDir(d *models.Download) (string, error) {
// Both branches go through the library service's guard, so every caller gets
// a symlink-resolved path. Cache keys are derived from that resolved root.
if !d.OutputDir.Valid || d.OutputDir.String == "" {
return s.cfg.LibraryDir, nil
return s.librarySvc.ResolveWithinLibrary("")
}
// Reuse the library service's guard so both entry points enforce the boundary
// the same way — it resolves symlinks, which a plain prefix check does not.
@@ -147,9 +149,7 @@ func (s *DownloadService) importItemDir(ctx context.Context, url, itemDir, baseL
// name (or land on the new title if it changed upstream).
if mode == "overwrite" && videoID != "" {
if existing, ok := s.librarySvc.FindByVideoID(baseLibraryDir, videoID); ok {
if rel, err := filepath.Rel(s.cfg.LibraryDir, existing); err == nil {
s.librarySvc.evictCachedScan(filepath.ToSlash(rel))
}
s.librarySvc.evictCachedDir(existing)
os.RemoveAll(existing)
}
}
Minternal/service/library.go
@@ -109,6 +109,20 @@ func (s *LibraryService) evictCachedScan(relPath string) {
s.scanCache.Delete(relPath)
}
// evictCachedDir drops the cached scan for an absolute item directory.
//
// The key must be relative to the symlink-resolved root, because that is what
// resolveItemDir stored it under. Computing it against the raw libraryDir misses
// whenever that is a symlink, leaving a deleted or replaced item visible until
// the TTL expires.
func (s *LibraryService) evictCachedDir(itemDir string) {
rel, err := filepath.Rel(s.libraryRoot(), itemDir)
if err != nil {
return
}
s.evictCachedScan(filepath.ToSlash(rel))
}
// refLock is a mutex plus the number of callers holding or waiting for it.
type refLock struct {
mu sync.Mutex
@@ -661,9 +675,7 @@ func (s *LibraryService) PruneToIDSet(baseDir string, keep map[string]bool) (int
if meta.VideoID == "" || keep[meta.VideoID] {
return true
}
if rel, err := filepath.Rel(s.libraryDir, itemDir); err == nil {
s.evictCachedScan(filepath.ToSlash(rel))
}
s.evictCachedDir(itemDir)
if err := os.RemoveAll(itemDir); err != nil {
slog.Warn("prune failed to remove item", "dir", itemDir, "err", err)
return true
Minternal/service/subscription_library_test.go
@@ -52,3 +52,40 @@ func TestPruneToIDSet(t *testing.T) {
t.Error("expected unidentified item to survive")
}
}
// A symlinked library root used to break cache eviction: the cache is keyed off
// the resolved root, so computing the key from the raw configured path missed,
// and a pruned item stayed visible until the scan TTL expired.
func TestPruneEvictsCacheUnderSymlinkedRoot(t *testing.T) {
parent := t.TempDir()
real := filepath.Join(parent, "real")
link := filepath.Join(parent, "link")
if err := os.MkdirAll(real, 0o755); err != nil {
t.Fatalf("mkdir real: %v", err)
}
if err := os.Symlink(real, link); err != nil {
t.Skipf("symlinks unavailable: %v", err)
}
lib := NewLibraryService(link, "ffmpeg", "ffprobe")
writeItem(t, real, "drop", "name = \"Drop\"\nvideo_id = \"d1\"\n", nil)
// Populate the scan cache, the way a listing or detail page would.
if _, err := lib.GetByRelPath("drop"); err != nil {
t.Fatalf("seed cache: %v", err)
}
// Prune against the resolved root, which is what resolveBaseLibraryDir hands
// the production caller.
removed, err := lib.PruneToIDSet(lib.libraryRoot(), map[string]bool{"other": true})
if err != nil {
t.Fatalf("prune: %v", err)
}
if removed != 1 {
t.Fatalf("expected 1 removed, got %d", removed)
}
if _, err := lib.GetByRelPath("drop"); err == nil {
t.Error("pruned item still served from the scan cache")
}
}
Minternal/service/subscription_run.go
@@ -210,9 +210,7 @@ func (s *DownloadService) applyMetadata(existing string, info infoJSON, sourceIn
return fmt.Errorf("install refreshed info JSON: %w", err)
}
}
if rel, err := filepath.Rel(s.cfg.LibraryDir, existing); err == nil {
s.librarySvc.evictCachedScan(filepath.ToSlash(rel))
}
s.librarySvc.evictCachedDir(existing)
return nil
}
Minternal/worker/scheduler.go
@@ -4,6 +4,7 @@ import (
"context"
"database/sql"
"log/slog"
"sync"
"time"
"vidarchive/internal/models"
@@ -20,6 +21,7 @@ type Scheduler struct {
interval time.Duration
ctx context.Context
cancel context.CancelFunc
wg sync.WaitGroup
}
func NewScheduler(subscriptionSvc *service.SubscriptionService, downloadSvc *service.DownloadService, pool *Pool, interval time.Duration) *Scheduler {
@@ -39,14 +41,21 @@ func NewScheduler(subscriptionSvc *service.SubscriptionService, downloadSvc *ser
func (s *Scheduler) Start() {
s.backfillNextRuns()
s.wg.Add(1)
go s.loop()
}
// Stop cancels the loop and waits for it to return. Waiting matters: main
// closes the database next, and a checkDue still in flight would query a closed
// handle or queue a run nobody will service.
func (s *Scheduler) Stop() {
s.cancel()
s.wg.Wait()
}
func (s *Scheduler) loop() {
defer s.wg.Done()
ticker := time.NewTicker(s.interval)
defer ticker.Stop()
Minternal/worker/scheduler_test.go
@@ -240,17 +240,41 @@ func TestRunDefersBrokenSchedule(t *testing.T) {
}
}
// Stop must return promptly and end the loop.
// Stop must return promptly and end the loop. It waits for the loop goroutine,
// because main closes the database right after it returns.
func TestSchedulerStop(t *testing.T) {
e := newTestScheduler(t)
e.sched.Start()
e.sched.Stop()
stopWithin(t, e.sched, 5*time.Second)
if err := e.sched.ctx.Err(); err == nil {
t.Error("scheduler context still live after Stop")
}
}
// Stop on a scheduler that was never started must return rather than block
// forever waiting for a loop goroutine that does not exist.
func TestSchedulerStopWithoutStart(t *testing.T) {
e := newTestScheduler(t)
stopWithin(t, e.sched, 5*time.Second)
}
// stopWithin fails the test if Stop has not returned within d, instead of
// hanging until the whole package times out.
func stopWithin(t *testing.T, s *Scheduler, d time.Duration) {
t.Helper()
done := make(chan struct{})
go func() {
s.Stop()
close(done)
}()
select {
case <-done:
case <-time.After(d):
t.Fatalf("Stop did not return within %v", d)
}
}
// An interval of zero or less would spin the ticker, so the constructor clamps it.
func TestNewSchedulerRejectsNonPositiveInterval(t *testing.T) {
e := newTestScheduler(t)