fix(download): own folder per slskd grab, search timeout in seconds

Checked against slskd 0.26.0's source rather than a live daemon, which
#267 never had.

slskd reads a search's searchTimeout in seconds, counted from the last
response. We sent milliseconds, telling it a search may idle for five
hours, so a search never completed on its own. It now sends seconds,
with slskd's floor of 5.

slskd writes a finished file to <downloads>/<remote leaf folder>/, and
when a name is taken it writes name_<ticks>.ext beside it. collect found
files by name there, so a file left by an earlier failed attempt, or by
the user's own download, was collected in place of this grab's. A user
who changed slskd's destination setting got nothing collected at all.

slskd 0.26 takes a batch download with an explicit destination. Each
grab now enqueues batches into yellowjacket/<uuid>/ (one per disc, since
a batch's files land flat), collects from exactly there, and removes the
folder afterwards, including after a failure. An older daemon answers
the batch route with 400, which is remembered, and the per-user enqueue
is used. There, collect skips files that were already present,
unchanged, before the enqueue, and takes the renamed copy slskd wrote
instead. The comparison is against a snapshot, not a clock, because
slskd may run on another machine. The per-folder lock from #272 is kept
only while batches are not known to work.

The test stub now writes files when they are enqueued, as slskd does,
including the rename, so tests no longer stage files before a grab.
An opt-in TestSlskdLive runs against a real daemon when YJ_SLSKD_URL,
YJ_SLSKD_API_KEY and YJ_SLSKD_DOWNLOADS are set.

Closes #274

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT

backend/download/provider_slskd.go

@@ -5,13 +5,16 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"net/http"
 	"net/url"
 	"os"
 	"path"
 	"path/filepath"
 	"regexp"
 	"slices"
+	"strconv"
 	"strings"
+	"sync/atomic"
 	"time"
 
 	"github.com/google/uuid"
@@ -68,6 +71,10 @@ const (
 	// do not get cut off by the context deadline.
 	slskdSearchWait = 20 * time.Second
 
+	// slskdMinSearchTimeout is the smallest searchTimeout slskd accepts,
+	// in seconds.
+	slskdMinSearchTimeout = 5
+
 	// slskdTransferPoll is how often transfer state is polled.
 	slskdTransferPoll = 3 * time.Second
 
@@ -161,6 +168,10 @@ type slskd struct {
 	transferPoll time.Duration
 	stallAfter   time.Duration
 	absentGrace  time.Duration
+
+	// batches is whether the daemon takes batch downloads, which is
+	// how a grab gets a folder of its own (see enqueue).
+	batches atomic.Int32
 }
 
 // newSlskd builds the provider from config.
@@ -431,14 +442,18 @@ func (s *slskd) searchRequest(id, text string, minFiles int) map[string]any {
 		maximumPeerQueueLength = 100
 	)
 
-	// A tenth of the wait is left for the last poll and the responses
-	// fetch.
-	timeout := s.searchWait - s.searchWait/10
+	// slskd reads this in whole seconds, counted from the last response
+	// rather than from the start, with a floor of 5 (#274).  A tenth of
+	// our own wait is left for the last poll and the responses fetch.
+	timeout := max(
+		int((s.searchWait-s.searchWait/10)/time.Second),
+		slskdMinSearchTimeout,
+	)
 
 	return map[string]any{
 		"id":                       id,
 		"searchText":               text,
-		"searchTimeout":            timeout.Milliseconds(),
+		"searchTimeout":            timeout,
 		"responseLimit":            responseLimit,
 		"fileLimit":                fileLimit,
 		"filterResponses":          true,
@@ -736,20 +751,99 @@ func (s *slskd) Grab(
 		)
 	}
 
+	// Only the per-user enqueue writes into folders other grabs share;
+	// a batch has a folder of its own.  Until the daemon has answered a
+	// batch either way, take the locks anyway.
+	if s.batches.Load() != batchesSupported {
+		release, err := lockSlskdFolders(ctx, s.localFolders(c))
+		if err != nil {
+			return Result{}, err
+		}
+		defer release()
+	}
+
 	// slskd keeps finished transfers listed until someone removes them,
 	// and a transfer is matched to the request by filename.  A record
 	// left by an earlier attempt at the same file from the same peer
 	// would otherwise be read as this attempt's answer the moment the
 	// first poll came back — an old failure failing a transfer that has
 	// not started.  So what is already terminal is noted before enqueueing
-	// and ignored after.
-	release, err := lockSlskdFolders(ctx, s.localFolders(c))
+	// and ignored after.  The files already on disk are noted for the
+	// same reason (see arrivedFile).
+	stale := s.terminalTransferIDs(ctx, username)
+	existing := s.snapshotFolders(c)
+
+	dest, err := s.enqueue(ctx, username, c)
 	if err != nil {
 		return Result{}, err
 	}
-	defer release()
 
-	stale := s.terminalTransferIDs(ctx, username)
+	if err := s.awaitTransfers(
+		ctx, username, stale, c, onProgress,
+	); err != nil {
+		s.discardDestination(dest)
+
+		return Result{}, err
+	}
+
+	result, err := s.collect(c, dst, dest, existing)
+
+	s.discardDestination(dest)
+
+	return result, err
+}
+
+// Whether the daemon has the batch endpoint, learned from the first
+// grab that asks.
+const (
+	batchesUnknown int32 = iota
+	batchesSupported
+	batchesUnsupported
+)
+
+// slskdDestRoot is the folder under slskd's downloads directory that
+// batch destinations are made in, so everything this app asked slskd to
+// write is in one place and nothing else is.
+const slskdDestRoot = "yellowjacket"
+
+// enqueue asks slskd for a candidate's files and returns the folder,
+// relative to the downloads directory, they will be written to — or ""
+// when slskd will choose, which is its per-user enqueue.
+//
+// slskd 0.26 takes a batch with an explicit destination, which is the
+// only way to know for certain where a file lands.  Without one it is
+// `<downloads>/<remote leaf folder>/`, shared with every other download
+// of a same-named folder and with the user's own, renamed with a
+// `_<ticks>` suffix when a name is taken, and moved by the user's
+// `Destination.Subdirectory` setting (#274).
+func (s *slskd) enqueue(
+	ctx context.Context,
+	username string,
+	c Candidate,
+) (string, error) {
+	if s.batches.Load() != batchesUnsupported {
+		dest := slskdDestRoot + "/" + uuid.NewString()
+
+		err := s.enqueueBatches(ctx, username, c, dest)
+		if err == nil {
+			s.batches.Store(batchesSupported)
+
+			return dest, nil
+		}
+
+		if !batchEndpointMissing(err) {
+			return "", err
+		}
+
+		// An older daemon routes this path to the per-user enqueue with
+		// "batches" as the username and rejects the body, which is the
+		// 400; 404 and 405 are a daemon that routes it nowhere.
+		s.batches.Store(batchesUnsupported)
+		s.logger.Info(
+			"slskd has no batch downloads; files will be found by name",
+			"error", err,
+		)
+	}
 
 	wanted := make([]map[string]any, 0, len(c.Files))
 	for _, f := range c.Files {
@@ -762,16 +856,88 @@ func (s *slskd) Grab(
 	if err := s.client.post(
 		ctx, slskdDownloadsPath(username), wanted, nil,
 	); err != nil {
-		return Result{}, err
+		return "", err
 	}
 
-	if err := s.awaitTransfers(
-		ctx, username, stale, c, onProgress,
-	); err != nil {
-		return Result{}, err
+	return "", nil
+}
+
+func batchEndpointMissing(err error) bool {
+	switch statusCode(err) {
+	case http.StatusBadRequest, http.StatusNotFound, http.StatusMethodNotAllowed:
+		return true
+	default:
+		return false
+	}
+}
+
+// enqueueBatches enqueues one batch per destination folder.  A batch's
+// files all land directly in its destination, so a multi-disc rip needs
+// one per disc or disc 2's "01" is renamed out of the way of disc 1's.
+func (s *slskd) enqueueBatches(
+	ctx context.Context,
+	username string,
+	c Candidate,
+	dest string,
+) error {
+	groups := map[string][]map[string]any{}
+
+	for _, f := range c.Files {
+		sub := batchSubfolder(f.Path)
+		groups[sub] = append(groups[sub], map[string]any{
+			"filename": f.Path,
+			"size":     f.Size,
+		})
 	}
 
-	return s.collect(c, dst)
+	subs := make([]string, 0, len(groups))
+	for sub := range groups {
+		subs = append(subs, sub)
+	}
+
+	slices.Sort(subs)
+
+	for _, sub := range subs {
+		body := map[string]any{
+			"username": username,
+			"files":    groups[sub],
+			"options":  map[string]any{"destination": path.Join(dest, sub)},
+		}
+
+		if err := s.client.post(
+			ctx, "/api/v0/transfers/downloads/batches", body, nil,
+		); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// batchSubfolder is where under a batch's destination a file goes: its
+// disc folder, renamed to a form slskd's path sanitising leaves alone
+// and ParsePath still reads a disc number from, or nothing.
+func batchSubfolder(remote string) string {
+	norm := strings.ReplaceAll(remote, `\`, "/")
+
+	if n, ok := discFolder(path.Base(path.Dir(norm))); ok {
+		return "Disc " + strconv.Itoa(n)
+	}
+
+	return ""
+}
+
+// discardDestination removes a batch's folder once its files have been
+// collected or the grab abandoned.  It is this grab's own folder under
+// slskdDestRoot, so nothing in it belongs to anyone else.
+func (s *slskd) discardDestination(dest string) {
+	if dest == "" || !strings.HasPrefix(dest, slskdDestRoot+"/") {
+		return
+	}
+
+	if err := os.RemoveAll(filepath.Join(s.downloadsPath, filepath.FromSlash(dest))); err != nil {
+		s.logger.Debug("could not remove slskd batch folder", "dest", dest, "error", err)
+	}
 }
 
 // slskdFolders serialises grabs that land in the same local folder.
@@ -1102,10 +1268,17 @@ func (s *slskd) transfersFor(
 }
 
 // collect moves finished files out of slskd's download directory into
-// staging.  slskd lays them out as <downloads>/<folder>/<file>, so each
-// wanted file is looked up by its base name under the folder slskd
-// derived from the remote path.
-func (s *slskd) collect(c Candidate, dst string) (Result, error) {
+// staging.
+//
+// With a batch destination each file is exactly where it was asked to
+// go.  Without one slskd lays files out as <downloads>/<folder>/<file>,
+// and arrivedFile has to tell this grab's file from whatever else has
+// that name there.
+func (s *slskd) collect(
+	c Candidate,
+	dst, dest string,
+	existing map[string]fileStamp,
+) (Result, error) {
 	result := Result{Dir: dst, Files: make([]string, 0, len(c.Files))}
 
 	for _, f := range c.Files {
@@ -1113,10 +1286,28 @@ func (s *slskd) collect(c Candidate, dst string) (Result, error) {
 		folder := path.Base(path.Dir(norm))
 		base := path.Base(norm)
 
-		src := filepath.Join(s.downloadsPath, folder, base)
+		var (
+			src  string
+			info os.FileInfo
+			ok   bool
+		)
 
-		info, err := os.Stat(src)
-		if err != nil || info.Size() == 0 {
+		if dest != "" {
+			src = filepath.Join(
+				s.downloadsPath, filepath.FromSlash(dest), batchSubfolder(f.Path), base,
+			)
+
+			var err error
+
+			info, err = os.Stat(src)
+			ok = err == nil && info.Size() > 0
+		} else {
+			src, info, ok = arrivedFile(
+				filepath.Join(s.downloadsPath, folder), base, existing,
+			)
+		}
+
+		if !ok {
 			// Not every requested file arrives; that is expected and
 			// handled by completeness scoring downstream.
 			continue
@@ -1126,7 +1317,7 @@ func (s *slskd) collect(c Candidate, dst string) (Result, error) {
 		// Flattened, disc 2's "01 Intro.flac" overwrites disc 1's, and
 		// the importer loses the folder it reads the disc number from.
 		target := filepath.Join(dst, base)
-		if _, ok := discFolder(folder); ok {
+		if _, disc := discFolder(folder); disc {
 			target = filepath.Join(dst, folder, base)
 		}
 
@@ -1147,3 +1338,116 @@ func (s *slskd) collect(c Candidate, dst string) (Result, error) {
 
 	return result, nil
 }
+
+// fileStamp is enough of a file to tell whether it has been replaced.
+type fileStamp struct {
+	size    int64
+	modTime time.Time
+}
+
+// snapshotFolders records the files already in the folders a per-user
+// enqueue will write to, so collect does not take one of them for the
+// file this grab asked for.  A batch writes to a new folder and needs
+// none.
+func (s *slskd) snapshotFolders(c Candidate) map[string]fileStamp {
+	if s.batches.Load() == batchesSupported {
+		return nil
+	}
+
+	out := map[string]fileStamp{}
+
+	for _, dir := range s.localFolders(c) {
+		entries, err := os.ReadDir(dir)
+		if err != nil {
+			continue
+		}
+
+		for _, e := range entries {
+			info, err := e.Info()
+			if err != nil || !info.Mode().IsRegular() {
+				continue
+			}
+
+			out[filepath.Join(dir, e.Name())] = fileStamp{
+				size:    info.Size(),
+				modTime: info.ModTime(),
+			}
+		}
+	}
+
+	return out
+}
+
+// arrivedFile finds the file slskd wrote for base in dir.
+//
+// slskd's default when a name is taken is to write `name_<ticks>.ext`
+// beside it, so a file with that name left by an earlier failed attempt
+// — or by the user's own download of the same folder — would otherwise
+// be collected while this grab's copy sat beside it under another name.
+// A candidate is the name itself or a renamed form of it that was not
+// already there, unchanged, before the grab enqueued; the newest wins.
+// Comparing against the snapshot rather than a clock matters because
+// slskd may run on another machine whose clock is not ours.
+func arrivedFile(
+	dir, base string,
+	existing map[string]fileStamp,
+) (string, os.FileInfo, bool) {
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		return "", nil, false
+	}
+
+	ext := filepath.Ext(base)
+	stem := strings.TrimSuffix(base, ext)
+
+	var (
+		best     string
+		bestInfo os.FileInfo
+	)
+
+	for _, e := range entries {
+		name := e.Name()
+		if name != base && !isRenamedCopy(name, stem, ext) {
+			continue
+		}
+
+		info, err := e.Info()
+		if err != nil || !info.Mode().IsRegular() || info.Size() == 0 {
+			continue
+		}
+
+		full := filepath.Join(dir, name)
+		if was, ok := existing[full]; ok &&
+			was.size == info.Size() && was.modTime.Equal(info.ModTime()) {
+			continue
+		}
+
+		if bestInfo == nil || info.ModTime().After(bestInfo.ModTime()) {
+			best, bestInfo = full, info
+		}
+	}
+
+	return best, bestInfo, bestInfo != nil
+}
+
+// isRenamedCopy reports whether name is stem_<digits>ext, which is how
+// slskd names a download whose name was taken.
+func isRenamedCopy(name, stem, ext string) bool {
+	rest, ok := strings.CutPrefix(name, stem+"_")
+	if !ok {
+		return false
+	}
+
+	digits, ok := strings.CutSuffix(rest, ext)
+	if !ok || digits == "" {
+		return false
+	}
+
+	for _, r := range digits {
+		if r < '0' || r > '9' {
+			return false
+		}
+	}
+
+	return true
+}