diff --git a/docs/conventions/errors.md b/docs/conventions/errors.md index 4a4cb84..f6e6687 100644 --- a/docs/conventions/errors.md +++ b/docs/conventions/errors.md @@ -66,7 +66,15 @@ jellybit — **приложение, а не библиотека**: внешн - **+ корреляционный ключ** для владельца — `download_id` (если операция к загрузке) либо `request_id`, чтобы по нему найти полную ошибку в логах. Пример: «При обработке загрузки произошла ошибка, download_id=12345», а - не «произошла ошибка» и не сырой текст; + не «произошла ошибка» и не сырой текст. + **Ключ есть не у всякого транспорта, и это называется вслух.** `request_id` + — понятие HTTP-границы (chi `RequestID`); у Telegram и CLI его нет. Если + операция ещё не завела загрузку (отказ приёма), у такого транспорта ключа + нет вовсе — тогда сообщение остаётся без якоря, а диагностика ищется по + записи доменной границы (`capability`, `infohash`). Заводить транспорту + собственный идентификатор запроса ради ключа — решение уровня спеки, а не + умолчание: второй канал корреляции рядом с существующим дороже, чем + отсутствие ключа; - **маппинг доменной ошибки → статус/сообщение** (в jellybit — `httpapi.classifyErr`, единая точка для REST и веб-UI): diff --git a/docs/conventions/logging.md b/docs/conventions/logging.md index f147413..fef7d7c 100644 --- a/docs/conventions/logging.md +++ b/docs/conventions/logging.md @@ -204,6 +204,11 @@ Go-ошибки логируем как атрибут, не как текст - Входящие HTTP-запросы логируем с полями `http.method`, `http.route`, `http.status_code`, `duration_ms`, `transport` (`http`/`web`/`telegram`). +- **Поле, которое уже даёт scoped-логгер, руками не доклеиваем.** Команда, + положившая scoped-логгер в `ctx`, не передаёт `download_id` ещё и аргументом + записи: в JSON получается дублирующийся ключ, и строгий потребитель молча + оставит одно из значений. Правило следует из «логгер несёт ключи сам» и + проверяется чтением — линтером не выражается. - Для корреляции HTTP-запроса допустим `request_id` (напр. chi `RequestID`) — это отдельный слой от корреляции загрузки по `download_id` и не противоречит отказу от `trace_id`. Если запрос порождает загрузку — связь даёт diff --git a/docs/research/README.md b/docs/research/README.md index fe1e7a2..66906d5 100644 --- a/docs/research/README.md +++ b/docs/research/README.md @@ -1,7 +1,13 @@ # Разведка Наблюдения за внешним миром: что реально шлёт источник, чем документация формата -расходится с практикой. Источник истины — этот каталог, а не чужая документация. +расходится с практикой и как ведут себя наши разборщики и зависимости на границе +формата. Источник истины — этот каталог, а не чужая документация. + +Наблюдение о чужом **коде** живёт здесь наравне с наблюдением о чужих +**данных**, но у него есть срок годности: такая записка обязана называть версию +зависимости и условие пересмотра, потому что протухает от обновления `go.mod`, а +не от смены формата. **Каждый вывод — с числами и командой или условиями, которыми получен**, чтобы его можно было перепроверить. Число без провенанса проход обязан читать как @@ -26,3 +32,6 @@ LLM-эндпоинта на живых раздачах. Автоматичес - [torrent-bot-message.md](torrent-bot-message.md) — формат сообщения торрент-бота, из которого приходит magnet и контекст. +- [torrent-bencode-limits.md](torrent-bencode-limits.md) — границы разбора + `.torrent` в `anacrolix/torrent`: аллокация по объявленной длине строки, + паники разбора, отсутствие «имени-заглушки». Проверено на `v1.61.0`. diff --git a/docs/research/torrent-bencode-limits.md b/docs/research/torrent-bencode-limits.md new file mode 100644 index 0000000..fe5fb5a --- /dev/null +++ b/docs/research/torrent-bencode-limits.md @@ -0,0 +1,156 @@ +# Границы разбора `.torrent` в `anacrolix/torrent` + +Наблюдения о том, как ведёт себя библиотека разбора на **недоверенных** байтах +`.torrent`. В отличие от соседней записки про формат сообщения торрент-бота, это +наблюдение о **чужом коде**, а не о чужих данных, и потому у него есть срок +годности. + +**Условие устаревания:** перепроверить при обновлении `anacrolix/torrent`. +Числа и ссылки ниже сняты на `v1.61.0` (версия зафиксирована в `go.mod`); ссылки +вида `файл:строка` относятся к ней и после бампа могут указывать не туда. + +## Аллокация объявленной длины строки + +`bencode` аллоцирует строку по длине, **объявленной во входе**, до того как эти +байты прочитаны: `parseString` делает `make([]byte, length)` и только потом +`io.ReadFull` (`bencode/decode.go:250` и `:258`). Ограничитель один — потолок +`DefaultDecodeMaxStrLen = 1<<27 - 1` ≈ 128 MiB (`decode.go:17`), проверяемый в +`parseStringLength` (`decode.go:223`) **до** аллокации. `metainfo.Load` создаёт +декодер, не переопределяя `MaxStrLen` (`metainfo/metainfo.go:35-37`), то есть +работает с потолком по умолчанию. + +**Замер.** Вход — верхнеуровневый словарь `d7:comment:xxxx`, где `` +объявляет длину, а байтов за ней нет. Мерилось дельтой +`runtime.MemStats.TotalAlloc` вокруг вызова, Go 1.26.5. Программа целиком +(положить в `tmp/bencodealloc/main.go`, запустить `go run ./tmp/bencodealloc`, +каталог после замера удалить — `tmp/` в `.gitignore`): + +```go +package main + +import ( + "bytes" + "fmt" + "runtime" + + "github.com/anacrolix/torrent/metainfo" +) + +func craft(declared int64, tail int) []byte { + var b bytes.Buffer + b.WriteString("d7:comment") + fmt.Fprintf(&b, "%d:", declared) + b.Write(bytes.Repeat([]byte("x"), tail)) + return b.Bytes() +} + +func measure(name string, data []byte) { + runtime.GC() + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + _, err := metainfo.Load(bytes.NewReader(data)) + runtime.ReadMemStats(&after) + fmt.Printf("%-34s вход=%-4d байт аллоцировано=%8.2f MiB err=%v\n", + name, len(data), float64(after.TotalAlloc-before.TotalAlloc)/(1<<20), err) +} + +func main() { + measure("объявлено 1 MiB", craft(1<<20, 16)) + measure("объявлено 64 MiB", craft(64<<20, 16)) + measure("объявлено 128 MiB - 1 (потолок)", craft(1<<27-1, 16)) + measure("объявлено 128 MiB (выше потолка)", craft(1<<27, 16)) + measure("объявлено 1 GiB (выше потолка)", craft(1<<30, 16)) +} +``` + +| Объявленная длина | Размер входа | Аллоцировано | Исход | +| --- | --- | --- | --- | +| 1 MiB | 34 байта | 1.00 MiB | ошибка `unexpected EOF` | +| 64 MiB | 35 байт | 64.01 MiB | ошибка `unexpected EOF` | +| 128 MiB − 1 (потолок) | 36 байт | 128.00 MiB | ошибка `unexpected EOF` | +| 128 MiB (выше потолка) | 36 байт | 0.00 MiB | ошибка `exceeds limit` | +| 1 GiB (выше потолка) | 37 байт | 0.00 MiB | ошибка `exceeds limit` | + +Что из этого следует: + +- **Усиление огромное, и наш лимит размера от него не защищает.** 36 байт входа + дают 128 MiB транзиентной аллокации — это ×3.7 млн, а не «крафт-8 MiB даёт + 128 MiB», как предполагала исходная нить ревью. Предел приёма + `ingest.MaxTorrentSize` (8 MiB) стоит **до** разбора и на эту величину не + влияет вовсе: атакующему хватает трёх десятков байт. +- **Но аллокация ограничена сверху и одна на попытку разбора.** Выше потолка + библиотека отказывает, не аллоцировав ничего; ниже — аллоцирует ровно + объявленное и падает на чтении, обрывая разбор целиком. Дочитать несколько + таких строк в одном входе нельзя: первая же необеспеченная строка роняет + `Load`. Верхняя граница на один принятый `.torrent` — примерно 128 MiB, после + чего память возвращается. +- **Числа выше — это `TotalAlloc`, а не физическая память.** Разграничение + существенное, и без него вывод читается страшнее, чем есть. Деградационный + путь — `make([]byte, N)`, затем немедленно проваленный `io.ReadFull`, — до + страниц буфера **не дотрагивается**, а Linux отдаёт анонимную память + zero-fill-on-demand. Замер RSS (`/proc/self/status`, `VmRSS`) на том же + входе: + + | Что мерили | VmRSS | + | --- | --- | + | 1000 одновременных разборов вырожденного входа, пик | 7.4 MiB | + | `make([]byte, 128 MiB)` без касания страниц | +0.8 MiB | + | то же с касанием одного байта | +0.02 MiB | + | то же с проходом по каждой странице | +128 MiB | + + То есть N одновременных приёмов **не** дают N × 128 MiB физической памяти: до + RAM это не доходит вовсе. Формулировка «N × 128 MiB» верна только про + логический счётчик запрошенных байт кучи. +- **Отсюда вывод сильнее, а не слабее.** Пункт принят не потому, что «профиля + нагрузки нет и авось обойдётся», а потому, что **деградационный путь + структурно не расходует физическую память**. Возвращаться к нему с лимитом + параллелизма повода нет; повод появится, только если найдётся путь, на котором + объявленная строка действительно дочитывается. +- **Это вопрос устойчивости, а не безопасности.** Отказ в обслуживании изнутри + контура явно вынесен за модель угроз (`../security.md` → «Что вне модели»). + +## Паники разбора не выходят наружу — но гард у нас уже́е, чем кажется + +`bencode.Decoder.Decode` ловит паники разбора и возвращает их ошибкой, **кроме** +`runtime.Error` — такую он пере-паникует (`bencode/decode.go:38-46`). То есть +арифметическая ошибка или выход за границы внутри библиотеки поднялись бы +паникой через `metainfo.Load`. + +Наш `recover`-гард в `internal/torrent` стоит только на `files()` и покрывает +`UpvertedFiles`/`FileTree`. Он **не** покрывает `metainfo.Load`, +`UnmarshalInfo` и `HashBytes` — они вызываются вне гарда. + +Достижимого panic-пути через них найти не удалось. Проверялась гипотеза про +отрицательную объявленную длину: `parseStringLength` её пропускает +(`checkBufferedInt`, `decode.go:196-208`, принимает `-5`), и `make([]byte, -5)` +дал бы `runtime.Error`. Путь **недостижим** — диспетчер значений входит в разбор +строки только по ведущей цифре, а `-` отсекается раньше: + +``` +"d7:comment-5:xxxxx" → bencode: syntax error (offset: 10): unknown value type '-' +"d4:infod4:name-5:xxxxxee" → bencode: syntax error (offset: 14): unknown value type '-' +``` + +Это отрицательный результат, а не гарантия: он говорит, что **этот** путь +закрыт, и ничего не говорит об остальных. Расширять гард на `Load` при +обновлении библиотеки — дешёвая страховка, если появится повод. + +## Библиотека не подставляет «имя-заглушку» + +Смежное наблюдение о той же библиотеке, записанное потому, что его отсутствие +стоило ложной нити в ревью приёма 2026-07-08. + +`metainfo.Info.BestName()` (`metainfo/info.go:200-205`) возвращает `NameUtf8`, +иначе `Name`, иначе **пустую строку**. Константа `NoName = "-"` +(`metainfo/info.go:44`) присваивается только в `BuildFromFilePath` — то есть при +**авторинге** раздачи из вырожденного пути (`.`, `..`, `/`), и в разборе не +участвует. + +Значит `-` доходит до нас исключительно тогда, когда раздача **сама объявила** +его полем `name`. Это конвенция «имени нет», принятая ради совместимости с +Transmission (комментарий у константы ссылается на transmission#1775), и +библиотека экспортирует константу именно затем, чтобы на неё ссылались. + +Практический вывод: «раздача без имени» и «раздача с именем `-`» — **разные** +входы, и путать их нельзя. Первый даёт пустую строку сам, второй нормализуется +нами на границе разбора (`internal/torrent`, `displayName`). diff --git a/docs/review.md b/docs/review.md index 6ae9741..7d382dc 100644 --- a/docs/review.md +++ b/docs/review.md @@ -276,3 +276,32 @@ merge-раскладка при повторном добавлении разд асимметрию признака владения. Сам дефект — задачей в беклоге, кандидат `critical`; спека `state-reconciliation` в том же изменении перестала утверждать, что уборка «данных пользователя не касается». + +## 2026-08-06 — нормализация имени раздачи сама производила сентинел, который отбрасывала [пойман] + +- **Где:** `internal/torrent/torrent.go`, функция `displayName` (введена + изменением `ingest-nits`). Норма — `openspec/specs/ingest/spec.md`, требование + «Вырожденное имя раздачи не считается именем». +- **Симптом:** найден на ревью изменения, до мерджа. В эксплуатации не был. +- **Причина:** сравнение с `metainfo.NoName` стояло **до** схлопывания + пробельного. Имя `" - "` сравнение не проходило, а `oneLine` превращал его + ровно в `-`, и вырожденное значение уезжало вниз по потоку всеми тремя + путями: строкой названия в контексте распознавания, в `source_ref` (фолбек на + имя присланного файла не срабатывал — строка непуста) и подсказкой вывода + отображаемого имени. Против `master` это **регрессия**: там стояло + `strings.TrimSpace(...)` перед сравнением с `-`, и фолбек работал. +- **Чем воспроизведён:** двумя независимыми падающими тестами — `adversary` + прогнал приём на входах `" - "`, `"-\n"`, `"\t-"`, `" -"` и получил + `source_ref = "-"` вместо `Dune.torrent`; `reimpl` принёс свой тест на разбор. + В дереве остался табличный `TestParseNoNameSentinelDropped`. +- **Почему не поймали раньше:** ловить было нечему — дефект внесён этим же + изменением и пойман тем же прогоном. Отмечено потому, что это **эвал-сет + наоборот**: случай, где верхняя ступень окупилась. Три прохода из семи + (`specs`, `adversary`, `reimpl`) нашли его независимо, и двое принесли оракул; + проход `code` (конвенции) и гейт его не видели — порядок двух операций внутри + функции не выражается ни правилом линтера, ни конвенцией. +- **Что меняем:** ничего в конвейере. Класс «нормализация и сравнение с + константой идут в неверном порядке» дешевле ловить тестом на границе разбора, + чем правилом; такой тест заведён. Наблюдение о самой библиотеке (что + `BestName()` сентинел **не** синтезирует — исходное основание нити было + неверным) записано в `research/torrent-bencode-limits.md`. diff --git a/internal/httpapi/httpapi.go b/internal/httpapi/httpapi.go index 078603b..fa5d779 100644 --- a/internal/httpapi/httpapi.go +++ b/internal/httpapi/httpapi.go @@ -432,7 +432,9 @@ func (s *server) handleUIAdd(w http.ResponseWriter, r *http.Request) { res, err := s.deps.Ingestor.Ingest(r.Context(), req) if err != nil { - redirectErr(w, r, userErr(r, err, res.DownloadID)) + // Нулевой Result на любом пути ошибки — контракт ingest.Ingest; + // корреляционный ключ веб-формы, как и REST, — request_id. + redirectErr(w, r, userErr(r, err, "")) return } if res.Deduplicated { @@ -585,10 +587,10 @@ func (s *server) handleAPIAdd(w http.ResponseWriter, r *http.Request) { } res, err := s.deps.Ingestor.Ingest(r.Context(), ingest.Request{Source: req.Source, Context: req.Context}) if err != nil { - // res.DownloadID непуст, если сбой после создания задачи (напр. qbit) — - // тогда коррелируем по download_id, иначе (ранний разбор источника) по - // request_id. - s.apiErr(w, r, err, res.DownloadID) + // Приём на любом пути ошибки возвращает нулевой Result (контракт + // ingest.Ingest) — идентификатора загрузки тут нет и быть не может, + // коррелируем по request_id. + s.apiErr(w, r, err, "") return } status := http.StatusCreated diff --git a/internal/httpapi/ui_add_torrent_test.go b/internal/httpapi/ui_add_torrent_test.go index b56d892..ad5fd54 100644 --- a/internal/httpapi/ui_add_torrent_test.go +++ b/internal/httpapi/ui_add_torrent_test.go @@ -2,6 +2,9 @@ package httpapi_test import ( "bytes" + "encoding/json" + "errors" + "fmt" "mime/multipart" "net/http" "net/url" @@ -85,3 +88,50 @@ func TestUIAddUrlencoded(t *testing.T) { t.Errorf("urlencoded source не проброшен: %q", ing.lastReq.Source) } } + +// Отказ приёма на HTTP-границе: идентификатора загрузки нет (контракт +// ingest.Ingest — нулевой Result на любом пути ошибки), поэтому корреляционным +// ключом остаётся request_id запроса. Проверяются оба HTTP-транспорта: REST +// отдаёт ключ полем тела, веб-форма — текстом флеш-сообщения в редиректе. +func TestIngestErrorCorrelatesByRequestID(t *testing.T) { + t.Run("REST", func(t *testing.T) { + ing := &fakeIngestor{err: fmt.Errorf("ingest: create download: %w", errors.New("boom"))} + srv := newServer(t, httpapi.Deps{Ingestor: ing, Commander: &fakeCommander{}, Reader: &fakeReader{}}) + + resp, err := http.Post(srv.URL+"/api/downloads", "application/json", + strings.NewReader(`{"source":"magnet:?xt=urn:btih:abc"}`)) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + var body map[string]any + if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { + t.Fatalf("decode: %v", err) + } + if s, _ := body["request_id"].(string); s == "" { + t.Errorf("в теле отказа нет request_id: %v", body) + } + if _, ok := body["download_id"]; ok { + t.Errorf("в теле отказа обещан download_id: %v", body) + } + }) + + t.Run("веб-форма", func(t *testing.T) { + ing := &fakeIngestor{err: fmt.Errorf("ingest: create download: %w", errors.New("boom"))} + srv := newServer(t, httpapi.Deps{Ingestor: ing, Commander: &fakeCommander{}, Reader: &fakeReader{}}) + + resp, err := noRedirectClient().PostForm(srv.URL+"/ui/downloads", + url.Values{"source": {"magnet:?xt=urn:btih:abc"}}) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + loc := resp.Header.Get("Location") + if !strings.Contains(loc, "request_id%3D") && !strings.Contains(loc, "request_id=") { + t.Errorf("в сообщении отказа нет request_id: %q", loc) + } + if strings.Contains(loc, "download_id") { + t.Errorf("в сообщении отказа обещан download_id: %q", loc) + } + }) +} diff --git a/internal/ingest/ingest.go b/internal/ingest/ingest.go index f7bee5c..2657eb1 100644 --- a/internal/ingest/ingest.go +++ b/internal/ingest/ingest.go @@ -66,7 +66,9 @@ type Request struct { Context string // подсказка для распознавания (опц.) } -// Result — итог приёма. +// Result — итог приёма. При ненулевой ошибке Ingest возвращает НУЛЕВОЙ Result: +// идентификатор загрузки, хеши, состояние и признак дедупликации не +// публикуются (см. Ingest). type Result struct { DownloadID string Infohashes []string // все хеши источника (гибридный magnet: v1 и v2, v1 первым) @@ -78,7 +80,24 @@ type Result struct { // полей ссылки, дедуплицирует по активной задаче, иначе сохраняет загрузку в // `catched` и сразу возвращает результат. Добавление в qBittorrent и вывод // имени выполняет worker (см. download-tracking). -func (s *Service) Ingest(ctx context.Context, req Request) (Result, error) { +// +// Контракт: на ЛЮБОМ пути ошибки возвращается нулевой Result. Приём не создаёт +// наблюдаемых последствий раньше, чем способен вернуть успех, а всё, что может +// отказать после заведения загрузки, делает worker. Транспорты на это +// опираются и не обещают идентификатора, которого нет: HTTP коррелирует отказ +// по request_id, Telegram — ключа не даёт (см. ingest-спеку, требование +// «Результат приёма при ошибке пуст»). +// +// Гарантия структурная — обнуление в одном defer, а не аккуратность каждой +// ветки возврата: перечень веток растёт, и именно расхождение перечня с +// комментариями транспортов породило исходный дефект. +func (s *Service) Ingest(ctx context.Context, req Request) (res Result, err error) { + defer func() { + if err != nil { + res = Result{} + } + }() + src, err := s.parse(req) if err != nil { // Невалидный источник — норма (адресат не команда, а пользователь, и он @@ -179,10 +198,10 @@ func (s *Service) parse(req Request) (parsedSource, error) { } // SourceRef — человекочитаемый референс (имя раздачи), НЕ адрес // добавления: torrent добавляется байтами (см. worker), не по SourceRef. - // Фолбек на имя файла, если у раздачи нет содержательного имени - // (пустое или NoName-сентинел "-"). - ref := strings.TrimSpace(info.DisplayName) - if ref == "" || ref == "-" { + // Фолбек на имя файла, если у раздачи нет содержательного имени: + // вырожденное значение отбросил разборщик, здесь остаётся пустота. + ref := info.DisplayName + if ref == "" { ref = strings.TrimSpace(req.TorrentName) } return parsedSource{ diff --git a/internal/ingest/ingest_test.go b/internal/ingest/ingest_test.go index f8b561e..18f4c58 100644 --- a/internal/ingest/ingest_test.go +++ b/internal/ingest/ingest_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "log/slog" + "reflect" "strings" "testing" @@ -24,13 +25,22 @@ type fakeStore struct { upgradeID string // downloadID последнего вызова UpgradeCatchedMagnetToTorrent upgradeBlob []byte // байты, переданные в апгрейд upgradeUp bool // что вернуть из UpgradeCatchedMagnetToTorrent + + lookupErr error // отказ хранилища на дедуп-чеке + createErr error // отказ хранилища на заведении загрузки } func (f *fakeStore) FindReingestBlockingByInfohash(_ context.Context, _ ...string) (*store.Download, error) { + if f.lookupErr != nil { + return nil, f.lookupErr + } return f.active, nil } func (f *fakeStore) CreateDownloadIfNoActive(_ context.Context, d *store.Download, hashes []string, torrentBlob []byte) (*store.Download, error) { + if f.createErr != nil { + return nil, f.createErr + } if f.active != nil { return f.active, nil } @@ -300,3 +310,31 @@ func TestIngestRejectsNonMagnet(t *testing.T) { t.Error("не должно быть записи задачи") } } + +// Контракт приёма: на ЛЮБОМ пути ошибки транспорту возвращается НУЛЕВОЙ Result. +// Транспорты на это опираются и не обещают идентификатора, которого нет +// (см. ingest-спеку, «Результат приёма при ошибке пуст»). Сравниваем результат +// с нулевым значением ЦЕЛИКОМ, а не по полю DownloadID: следующая ветвь отказа +// может заполнить другое поле. +func TestIngestReturnsZeroResultOnEveryErrorPath(t *testing.T) { + boom := errors.New("boom") + for _, tc := range []struct { + name string + fs *fakeStore + req Request + }{ + {"невалидный источник", &fakeStore{}, Request{Source: "не magnet и не torrent"}}, + {"сбой хранилища на дедуп-чеке", &fakeStore{lookupErr: boom}, Request{Source: sampleMagnet}}, + {"сбой хранилища на заведении", &fakeStore{createErr: boom}, Request{Source: sampleMagnet}}, + } { + t.Run(tc.name, func(t *testing.T) { + res, err := newService(tc.fs).Ingest(context.Background(), tc.req) + if err == nil { + t.Fatal("ожидалась ошибка") + } + if !reflect.DeepEqual(res, Result{}) { + t.Errorf("Result = %+v, want нулевой", res) + } + }) + } +} diff --git a/internal/ingest/torrent_ingest_test.go b/internal/ingest/torrent_ingest_test.go index 895bbd4..7d8e94f 100644 --- a/internal/ingest/torrent_ingest_test.go +++ b/internal/ingest/torrent_ingest_test.go @@ -134,28 +134,40 @@ func TestIngestTorrentTooLarge(t *testing.T) { } } -// У раздачи без имени source_ref берётся из имени файла (фолбек). +// У раздачи без содержательного имени source_ref берётся из имени файла. +// Случая два, и они разные: раздача БЕЗ поля name (BestName() == "") и +// раздача, объявившая вырожденное `-` (metainfo.NoName) — второй нормализует +// разборщик, приём про него уже не знает. func TestIngestTorrentNameFallback(t *testing.T) { - // Info без name → BestName() == "" → фолбек на TorrentName. - info := metainfo.Info{Name: "", Length: 1024, PieceLength: 512, Pieces: make([]byte, 40)} - infoBytes, err := bencode.Marshal(info) - if err != nil { - t.Fatalf("marshal: %v", err) - } - mi := metainfo.MetaInfo{InfoBytes: infoBytes, Announce: "http://t/ann"} - var buf bytes.Buffer - if err := mi.Write(&buf); err != nil { - t.Fatalf("write: %v", err) - } + for _, tc := range []struct { + name string + infoName string + }{ + {"без поля name", ""}, + {"вырожденное имя", metainfo.NoName}, + } { + t.Run(tc.name, func(t *testing.T) { + info := metainfo.Info{Name: tc.infoName, Length: 1024, PieceLength: 512, Pieces: make([]byte, 40)} + infoBytes, err := bencode.Marshal(info) + if err != nil { + t.Fatalf("marshal: %v", err) + } + mi := metainfo.MetaInfo{InfoBytes: infoBytes, Announce: "http://t/ann"} + var buf bytes.Buffer + if err := mi.Write(&buf); err != nil { + t.Fatalf("write: %v", err) + } - fs := &fakeStore{} - _, err = newService(fs).Ingest(context.Background(), - Request{TorrentData: buf.Bytes(), TorrentName: "Fallback.Name.torrent"}) - if err != nil { - t.Fatalf("Ingest: %v", err) - } - if len(fs.created) != 1 || fs.created[0].SourceRef != "Fallback.Name.torrent" { - t.Errorf("source_ref = %q, want фолбек на имя файла", fs.created[0].SourceRef) + fs := &fakeStore{} + _, err = newService(fs).Ingest(context.Background(), + Request{TorrentData: buf.Bytes(), TorrentName: "Fallback.Name.torrent"}) + if err != nil { + t.Fatalf("Ingest: %v", err) + } + if len(fs.created) != 1 || fs.created[0].SourceRef != "Fallback.Name.torrent" { + t.Errorf("source_ref = %q, want фолбек на имя файла", fs.created[0].SourceRef) + } + }) } } diff --git a/internal/qbt/qbt_test.go b/internal/qbt/qbt_test.go index aaa684e..f80218e 100644 --- a/internal/qbt/qbt_test.go +++ b/internal/qbt/qbt_test.go @@ -2,10 +2,13 @@ package qbt import ( "context" + "log/slog" "net/http" "net/http/httptest" "strings" "testing" + + "git.vakhrushev.me/av/jellybit/internal/logctx" ) // fakeQBittorrent — минимальный стенд WebUI API: требует cookie SID, выдаёт @@ -238,3 +241,102 @@ func TestLoginFailure(t *testing.T) { t.Error("ожидалась ошибка логина") } } + +// captureHandler собирает записи журнала для проверки их полей. Свой WithAttrs +// нужен обязательно: делегированный встроенному хендлеру вернул бы ЕГО, и +// логгер, собранный через With (а scoped-логгер загрузки собирается именно +// так), перестал бы захватываться. +type captureHandler struct { + attrs []slog.Attr + records *[]slog.Record +} + +func (h *captureHandler) Enabled(context.Context, slog.Level) bool { return true } + +func (h *captureHandler) Handle(_ context.Context, r slog.Record) error { + rec := r.Clone() + rec.AddAttrs(h.attrs...) + *h.records = append(*h.records, rec) + return nil +} + +func (h *captureHandler) WithAttrs(as []slog.Attr) slog.Handler { + return &captureHandler{attrs: append(append([]slog.Attr{}, h.attrs...), as...), records: h.records} +} + +func (h *captureHandler) WithGroup(string) slog.Handler { return h } + +// recordFields сплющивает запись в map «ключ → значение как текст». +func recordFields(r slog.Record) map[string]string { + out := map[string]string{} + r.Attrs(func(a slog.Attr) bool { + out[a.Key] = a.Value.String() + return true + }) + return out +} + +// Отказ qBittorrent на добавление («Fails.») причины не несёт — единственное, +// что делает такую запись пригодной для разбора, это корреляция с загрузкой. +// Инфохэша клиент не знает и знать не должен (разбор источника — единая точка +// проекта): он берёт scoped-логгер из ctx. Негативная половина: magnet с +// passkey приватного трекера уходит через это же поле формы, и в журнал он +// попасть не должен (инвариант «секреты не попадают в логи»). +func TestAddFailureRecordCarriesScopeAndNoSecret(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v2/auth/login" { + http.SetCookie(w, &http.Cookie{Name: "SID", Value: "token", Path: "/"}) + _, _ = w.Write([]byte("Ok.")) + return + } + _, _ = w.Write([]byte("Fails.")) + })) + defer srv.Close() + + // Значение низкоэнтропийное намеренно: высокоэнтропийная фикстура «похожего + // на секрет» вида краснит gitleaks в pre-commit. Различимости хватает — + // тест ищет вхождение этой строки в полях записи. + const passkey = "passkey-must-not-reach-the-journal" + const magnetURL = "magnet:?xt=urn:btih:541adcff3b6dd5dba7088ea83317d9d6fac331d6" + + "&tr=http%3A%2F%2Ftracker.example%2Fann%3Fpasskey%3D" + passkey + + var records []slog.Record + log := slog.New(&captureHandler{records: &records}). + With("capability", "review", "download_id", "01JB0000000000000000000000", "infohash", + "541adcff3b6dd5dba7088ea83317d9d6fac331d6") + ctx := logctx.With(context.Background(), log) + + err := newClient(t, srv.URL).Add(ctx, AddRequest{URLs: []string{magnetURL}, Category: "jellybit"}) + if err == nil { + t.Fatal("ожидался отказ добавления") + } + + var failure *slog.Record + for i := range records { + if records[i].Message == "external call failed" { + failure = &records[i] + } + } + if failure == nil { + t.Fatalf("записи об отказе внешнего вызова нет, записей: %d", len(records)) + } + f := recordFields(*failure) + if f["download_id"] == "" { + t.Errorf("в записи нет download_id из scoped-логгера: %v", f) + } + if f["infohash"] != "541adcff3b6dd5dba7088ea83317d9d6fac331d6" { + t.Errorf("infohash = %q, want хеш из scoped-логгера", f["infohash"]) + } + if f["ext.operation"] != "torrents/add" { + t.Errorf("ext.operation = %q, want torrents/add", f["ext.operation"]) + } + // Негативная половина: ни одно поле записи не несёт passkey. + for k, v := range f { + if strings.Contains(v, passkey) { + t.Errorf("passkey утёк в поле %q: %q", k, v) + } + } + if strings.Contains(failure.Message, passkey) { + t.Errorf("passkey утёк в сообщение записи") + } +} diff --git a/internal/tgbot/bot.go b/internal/tgbot/bot.go index 8088a54..0e06aba 100644 --- a/internal/tgbot/bot.go +++ b/internal/tgbot/bot.go @@ -262,10 +262,12 @@ func (b *Bot) downloadFile(ctx context.Context, fileID string) ([]byte, error) { func (b *Bot) ingestAndReply(ctx context.Context, chatID int64, req ingest.Request) { res, err := b.ingestor.Ingest(ctx, req) if err != nil { - // res.DownloadID непуст, если сбой после создания задачи (напр. qbit); - // при раннем разборе источника id ещё нет — даём дружелюбный текст без - // него (детали всё равно в логах на доменной границе). - b.send(chatID, opErr("Не удалось принять загрузку", res.DownloadID), nil) + // Приём на любом пути ошибки возвращает нулевой Result (контракт + // ingest.Ingest) — идентификатора загрузки нет. Корреляционного ключа + // у отказа приёма в Telegram нет вовсе (request_id — понятие + // HTTP-границы): текст остаётся дружелюбным без ключа, детали — в + // записи приёма на доменной границе (capability=ingest, infohash). + b.send(chatID, opErr("Не удалось принять загрузку", ""), nil) return } if res.Deduplicated { diff --git a/internal/tgbot/bot_test.go b/internal/tgbot/bot_test.go index b884560..18fc852 100644 --- a/internal/tgbot/bot_test.go +++ b/internal/tgbot/bot_test.go @@ -3,6 +3,7 @@ package tgbot import ( "context" "database/sql" + "errors" "log/slog" "strings" "testing" @@ -55,10 +56,15 @@ func (f *fakeAPI) GetFileDirectURL(string) (string, error) type fakeIngestor struct { lastReq ingest.Request res ingest.Result + err error } func (f *fakeIngestor) Ingest(_ context.Context, req ingest.Request) (ingest.Result, error) { f.lastReq = req + if f.err != nil { + // Контракт приёма: на ошибке результат нулевой (ingest.Ingest). + return ingest.Result{}, f.err + } return f.res, nil } @@ -585,3 +591,26 @@ func TestBot_CallbackStaleButton(t *testing.T) { t.Errorf("answers = %v, want понятный ответ", api.answers) } } + +// Отказ приёма в Telegram: идентификатора загрузки нет (контракт ingest.Ingest — +// нулевой результат на любом пути ошибки), и обещать его пользователю нельзя. +// Корреляционного ключа у этого транспорта тоже нет — записанный вопрос +// изменения `ingest-nits`; тест фиксирует сегодняшнее состояние, чтобы +// «download_id=» не вернулся в текст молча. +func TestBot_IngestErrorPromisesNoDownloadID(t *testing.T) { + b, api, ing, _ := newTestBot(t, []int64{7}) + ing.err = errors.New("boom") + + b.handleMessage(context.Background(), msgFrom(7, "magnet:?xt=urn:btih:ABC")) + + if len(api.sent) != 1 { + t.Fatalf("отправлено сообщений = %d, want 1", len(api.sent)) + } + got := api.sent[0].text + if !strings.Contains(got, "Не удалось принять загрузку") { + t.Errorf("текст отказа = %q", got) + } + if strings.Contains(got, "download_id") || strings.Contains(got, idCode(tid)) { + t.Errorf("в отказе обещан идентификатор загрузки: %q", got) + } +} diff --git a/internal/torrent/torrent.go b/internal/torrent/torrent.go index 510cf8a..4a27712 100644 --- a/internal/torrent/torrent.go +++ b/internal/torrent/torrent.go @@ -33,7 +33,7 @@ type File struct { type Info struct { Infohash string // первичный хеш: v1 приоритетно (нижний hex, 40 для v1, 64 для v2) Infohashes []string // все хеши файла (гибрид несёт v1 и v2); v1 раньше v2 - DisplayName string // имя раздачи (info.name) + DisplayName string // имя раздачи (info.name), нормализованное displayName; пусто, если имени нет Files []File // файлы раздачи (для одиночного — один элемент) TotalLength int64 // суммарный размер Trackers []string // announce + announce-list (плоско, без дублей) @@ -69,7 +69,7 @@ func Parse(data []byte) (Info, error) { return Info{ Infohash: hashes[0], Infohashes: hashes, - DisplayName: meta.BestName(), + DisplayName: displayName(meta.BestName()), Files: fs, TotalLength: total, Trackers: mi.UpvertedAnnounceList().DistinctValues(), @@ -77,6 +77,33 @@ func Parse(data []byte) (Info, error) { }, nil } +// displayName нормализует имя раздачи на границе разбора, чтобы ниже по потоку +// (строка названия контекста, source_ref приёма, подсказка вывода отображаемого +// имени) вырожденных значений не встречалось. +// +// Пробельное схлопываем тем же oneLine, что и комментарий: имя — недоверенный +// вход, а контекст распознавания читается построчно, и имя с переводом строки +// добавило бы в него строку, выглядящую как синтезированный нами факт. Защитой +// от инъекции в промпт это НЕ является — пользовательский текст контекста +// многострочен по замыслу; правило лишь снимает несогласованность между именем +// и комментарием. +// +// Затем отбрасываем `metainfo.NoName` — раздача объявляет это значение сама, +// полем name, по конвенции «имени нет» (библиотека экспортирует константу +// именно затем, чтобы на неё ссылались). Раздача без поля name даёт пустое имя +// и так. +// +// Порядок здесь существенный, и обратный — дефект: имя `" - "` сравнение с +// сентинелом не проходит, а схлопывание превращает его ровно в сентинел, и тот +// уезжает вниз по потоку всеми тремя путями. +func displayName(name string) string { + name = oneLine(name) + if name == metainfo.NoName { + return "" + } + return name +} + // files собирает файлы раздачи и суммарный размер из info (одиночный файл или // дерево). Байты .torrent — недоверенный вход: на кривых/вырожденных раздачах // библиотека (UpvertedFiles/FileTree) способна паниковать (напр. делением на @@ -111,8 +138,11 @@ var announceLabelRe = regexp.MustCompile(`^(?:bt\d*|www|announce|tracker|open)\. func (i Info) Context() string { var lines []string - if name := strings.TrimSpace(i.DisplayName); name != "" { - lines = append(lines, name) + // Имя уже нормализовано разборщиком (displayName): вырожденное значение и + // разделители строк сюда не доходят. Проверка на пустоту остаётся — имени + // у раздачи может не быть вовсе, и тогда строки названия просто нет. + if i.DisplayName != "" { + lines = append(lines, i.DisplayName) } if i.TotalLength > 0 { lines = append(lines, "Размер: "+humanSize(i.TotalLength)) diff --git a/internal/torrent/torrent_test.go b/internal/torrent/torrent_test.go index 1bb7b64..da304d5 100644 --- a/internal/torrent/torrent_test.go +++ b/internal/torrent/torrent_test.go @@ -5,6 +5,7 @@ import ( "crypto/sha1" "crypto/sha256" "encoding/hex" + "fmt" "strings" "testing" @@ -217,12 +218,48 @@ func TestParseInvalidBytes(t *testing.T) { } } -func TestContextEmptyWhenNoFields(t *testing.T) { - // info без имени (NoName-сентинел), без трекера/комментария: строк-фактов - // минимум. Проверяем, что Context не паникует и не тянет сеть (чистая - // функция — сетевых вызовов в коде нет по построению). +// Раздача объявила вырожденное имя `-` (metainfo.NoName — конвенция «имени +// нет», которую раздача выставляет САМА; библиотека его не синтезирует). +// Разборщик нормализует его к пустой строке, поэтому строки названия в +// контексте нет. Трекера и комментария тоже нет — строк-фактов минимум. +func TestParseNoNameSentinelDropped(t *testing.T) { + // Сентинел вокруг пробельного — тот же вырожденный вход: схлопывание + // обязано идти ДО сравнения, иначе " - " превращается в "-" уже после + // проверки и уезжает вниз по потоку (регрессия против master, где фолбек + // source_ref на имя файла срабатывал). + for _, name := range []string{ + metainfo.NoName, + " - ", + "-\n", + "\t-", + "\u00a0-", + } { + t.Run(fmt.Sprintf("%q", name), func(t *testing.T) { + data, _ := build(t, metainfo.Info{ + Name: name, + Length: 0, + PieceLength: 1024, + Pieces: pieces(1), + }, "", "") + info, err := Parse(data) + if err != nil { + t.Fatalf("parse: %v", err) + } + if info.DisplayName != "" { + t.Errorf("DisplayName = %q, want пусто (вырожденное имя)", info.DisplayName) + } + if got := info.Context(); got != "" { + t.Errorf("Context() = %q, want пусто (строки названия быть не должно)", got) + } + }) + } +} + +// Раздача без поля name даёт пустое имя и без нормализации — случай отдельный +// от вырожденного `-` и путать их нельзя (именно эта путаница в комментарии +// теста и породила исходную нить ревью 2026-07-08). +func TestParseMissingNameIsEmpty(t *testing.T) { data, _ := build(t, metainfo.Info{ - Name: "-", // metainfo.NoName Length: 0, PieceLength: 1024, Pieces: pieces(1), @@ -231,5 +268,50 @@ func TestContextEmptyWhenNoFields(t *testing.T) { if err != nil { t.Fatalf("parse: %v", err) } - _ = info.Context() // не паникует; состав строк — по полям + if info.DisplayName != "" { + t.Errorf("DisplayName = %q, want пусто (поля name нет)", info.DisplayName) + } + if got := info.Context(); got != "" { + t.Errorf("Context() = %q, want пусто", got) + } +} + +// Имя — недоверенный вход, а контекст читается построчно: имя с разделителем +// строк не должно добавлять в контекст строку, выглядящую как синтезированный +// нами факт. Число строк такое же, как у раздачи с обычным именем. +func TestParseNameCollapsedToOneLine(t *testing.T) { + // \n, U+2028 (LINE SEPARATOR) и U+2029 (PARAGRAPH SEPARATOR) — всё, чем можно + // разорвать строку; краевые пробелы туда же. + const dirty = " Dune.2024\nТрекер: evil.example\u2028Комментарий: подделка\u2029хвост " + const want = "Dune.2024 Трекер: evil.example Комментарий: подделка хвост" + data, _ := build(t, metainfo.Info{ + Name: dirty, + Length: 2100, + PieceLength: 1024, + Pieces: pieces(3), + }, "http://bt.rutracker.org/announce", "") + info, err := Parse(data) + if err != nil { + t.Fatalf("parse: %v", err) + } + if info.DisplayName != want { + t.Errorf("DisplayName = %q, want %q", info.DisplayName, want) + } + + clean, _ := build(t, metainfo.Info{ + Name: "Dune.2024", + Length: 2100, + PieceLength: 1024, + Pieces: pieces(3), + }, "http://bt.rutracker.org/announce", "") + ref, err := Parse(clean) + if err != nil { + t.Fatalf("parse clean: %v", err) + } + gotLines := len(strings.Split(info.Context(), "\n")) + wantLines := len(strings.Split(ref.Context(), "\n")) + if gotLines != wantLines { + t.Errorf("строк в контексте = %d, want %d (грязное имя добавило строк):\n%s", + gotLines, wantLines, info.Context()) + } } diff --git a/internal/worker/catched_test.go b/internal/worker/catched_test.go index ad177cc..e2fce3c 100644 --- a/internal/worker/catched_test.go +++ b/internal/worker/catched_test.go @@ -18,13 +18,15 @@ type fakeNamer struct { name string fields naming.Fields // извлечённая структура (ok=true, если Title непуст) gotContext string + gotHint string // подсказка имени — третий потребитель torrent.Info.DisplayName calls int onCall func() } -func (f *fakeNamer) Derive(_ context.Context, contextText, _ string) (string, naming.Fields, bool) { +func (f *fakeNamer) Derive(_ context.Context, contextText, hint string) (string, naming.Fields, bool) { f.calls++ f.gotContext = contextText + f.gotHint = hint if f.onCall != nil { f.onCall() } diff --git a/internal/worker/review.go b/internal/worker/review.go index 939a9c5..1d8c6c0 100644 --- a/internal/worker/review.go +++ b/internal/worker/review.go @@ -404,6 +404,10 @@ func (w *Worker) Relink(ctx context.Context, id string) (err error) { if d.State != store.StateReverted && d.State != store.StateCancelled && d.State != store.StateTargetMissing { return fmt.Errorf("relink: download %s is in state %s (expected reverted/cancelled/target_missing): %w", id, d.State, ErrConflict) } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity): иначе запись клиента об отказе уходит + // без download_id/infohash. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) // Источник нужен для распознавания и должен быть докачан — проверяем // синхронно (без дебаунса); отсутствие приводит состояние к реальности // (orphaned/deleted), недокачанный — ErrNotReady. @@ -414,7 +418,6 @@ func (w *Worker) Relink(ctx context.Context, id string) (err error) { if err := w.store.SetOverride(ctx, id, ovrForceReview, "1"); err != nil { return fmt.Errorf("relink: %w", err) } - ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) // Возврат в активную обработку — только через атомарный гард инварианта // «не более одной активной задачи на infohash» (см. design ulid-identity, D4). if err := w.store.ActivateIfNoOtherActive(ctx, id, store.StateRecognizing, "", ""); err != nil { @@ -439,10 +442,13 @@ func (w *Worker) Rerecognize(ctx context.Context, id string) (err error) { if err != nil { return err } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity): иначе запись клиента об отказе уходит + // без download_id/infohash. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) if err := w.ensureSourceReady(ctx, d, "rerecognize"); err != nil { return err } - ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) logctx.From(ctx).Info("review re-recognizing without hint") w.transition(ctx, *d, store.StateRecognizing, "", "") return nil @@ -462,10 +468,13 @@ func (w *Worker) Refine(ctx context.Context, id string, hint string) (err error) if err != nil { return err } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity): иначе запись клиента об отказе уходит + // без download_id/infohash. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) if err := w.ensureSourceReady(ctx, d, "refine"); err != nil { return err } - ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) if err := w.store.AddHint(ctx, id, hint); err != nil { return fmt.Errorf("refine: %w", err) } @@ -688,6 +697,10 @@ func (w *Worker) ChooseCandidate(ctx context.Context, id, candidateID string) (e if err != nil { return err } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity): иначе запись клиента об отказе уходит + // без download_id/infohash. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) rec, err := w.store.GetCurrentRecognition(ctx, id) if err != nil { return fmt.Errorf("choose candidate: %w", err) @@ -724,6 +737,10 @@ func (w *Worker) AddManualSource(ctx context.Context, id, provider, providerID s if err != nil { return err } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity): иначе запись клиента об отказе уходит + // без download_id/infohash. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) rec, err := w.store.GetCurrentRecognition(ctx, id) if err != nil { return fmt.Errorf("add source: %w", err) @@ -809,7 +826,7 @@ func (w *Worker) chooseCandidateLocked(ctx context.Context, id string, d *store. // раздачи (best-effort, косметика). Сбой обновления имени не должен ронять // выбор кандидата: логируем и продолжаем. if err := w.refreshDisplayNameLocked(ctx, id); err != nil { - logctx.From(w.scoped(ctx, capReview, id, d.PrimaryInfohash())). + logctx.From(ctx). Warn("display name refresh after candidate choice failed", "error", err) } return nil @@ -835,6 +852,10 @@ func (w *Worker) SetProviderID(ctx context.Context, id string, provider, provide if err != nil { return err } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity): иначе запись клиента об отказе уходит + // без download_id/infohash. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) // Режиссёр вручную заданного источника из метабазы (credits) — best-effort // косметика: сбой чтения рекогниции не валит смену источника (тип по умолчанию // movie, как трактует candMediaType(nil)). @@ -842,7 +863,7 @@ func (w *Worker) SetProviderID(ctx context.Context, id string, provider, provide if w.recognizer != nil { rec, rerr := w.store.GetCurrentRecognition(ctx, id) if rerr != nil { - logctx.From(w.scoped(ctx, capReview, id, d.PrimaryInfohash())). + logctx.From(ctx). Warn("set provider: recognition lookup for director failed", "error", rerr) rec = nil } diff --git a/internal/worker/torrent_add_test.go b/internal/worker/torrent_add_test.go index 9b8b0a9..06c96bf 100644 --- a/internal/worker/torrent_add_test.go +++ b/internal/worker/torrent_add_test.go @@ -5,11 +5,14 @@ import ( "context" "crypto/sha1" "encoding/hex" + "errors" + "log/slog" "testing" "github.com/anacrolix/torrent/bencode" "github.com/anacrolix/torrent/metainfo" + "git.vakhrushev.me/av/jellybit/internal/logctx" "git.vakhrushev.me/av/jellybit/internal/store" ) @@ -118,3 +121,162 @@ func TestRetryTorrentMissingBytesRollsBack(t *testing.T) { t.Errorf("state = %q, want failed (откат активации)", st.downloads["1"].State) } } + +// Третий потребитель torrent.Info.DisplayName — подсказка вывода отображаемого +// имени, то есть вход LLM. Раздача, объявившая вырожденное имя `-`, не должна +// отдавать его подсказкой: нормализация стоит на границе разбора, и здесь +// проверяется, что она туда доехала (собственный оракул, а не транзитивный). +func TestProcessCatchedNoNameGivesEmptyHint(t *testing.T) { + data, ih := buildTorrent(t, metainfo.NoName) + st := catchedTorrentStore("1", data, ih) + qb := &fakeQbt{} + w := newTestWorker(st, qb) + namer := &fakeNamer{name: "Дюна (2024)"} + w.SetNamer(namer) + + w.processCatched(context.Background()) + + if namer.calls == 0 { + t.Fatal("namer не вызывался — тест ничего не проверил") + } + if namer.gotHint != "" { + t.Errorf("подсказка имени = %q, want пусто (вырожденное имя раздачи)", namer.gotHint) + } +} + +// probeHandler захватывает атрибуты, доклеенные логгером. Свой WithAttrs +// обязателен: делегированный вернул бы чужой хендлер, и scoped-логгер (он +// собирается через With) перестал бы захватываться. +type probeHandler struct { + attrs *[]slog.Attr +} + +func (h probeHandler) Enabled(context.Context, slog.Level) bool { return true } + +func (h probeHandler) Handle(context.Context, slog.Record) error { return nil } + +func (h probeHandler) WithAttrs(as []slog.Attr) slog.Handler { + *h.attrs = append(*h.attrs, as...) + return h +} + +func (h probeHandler) WithGroup(string) slog.Handler { return h } + +// Обязанность вызывающего (capability identity): команда, делающая вызов +// внешнего сервиса в контексте загрузки, кладёт scoped-логгер в ctx ДО этого +// вызова. Без этого запись клиента qBittorrent об отказе добавления уходит без +// download_id/infohash, а причины отказа qBittorrent не сообщает («Fails.») — +// корреляция там единственное, что делает запись пригодной для разбора. +func TestRetryPassesScopedContextToAdd(t *testing.T) { + data, ih := buildTorrent(t, "Fargo.mkv") + st := catchedTorrentStore("1", data, ih) + st.downloads["1"].State = store.StateFailed + qb := &fakeQbt{} + w := newTestWorker(st, qb) + + var attrs []slog.Attr + w.log = slog.New(probeHandler{attrs: &attrs}) + + if err := w.Retry(context.Background(), "1"); err != nil { + t.Fatalf("Retry: %v", err) + } + if len(qb.addCtx) != 1 { + t.Fatalf("вызовов Add = %d, want 1", len(qb.addCtx)) + } + if logctx.FromOr(qb.addCtx[0], nil) == nil { + t.Fatal("в ctx вызова Add нет scoped-логгера загрузки") + } + + got := map[string]string{} + for _, a := range attrs { + got[a.Key] = a.Value.String() + } + if got["download_id"] != "1" { + t.Errorf("download_id = %q, want 1", got["download_id"]) + } + if got["infohash"] != ih { + t.Errorf("infohash = %q, want %q", got["infohash"], ih) + } + if got["capability"] == "" { + t.Errorf("нет capability в scoped-логгере: %v", got) + } +} + +// Best-effort ветки Retry: откат активации при провале Add и сбросы базиса +// таймаутов/счётчика пропусков. Ветки переписаны этим изменением (ручные +// w.log.… схлопнуты в logctx.From(ctx)), поэтому проверяем не только исход, но +// и что аварийная запись несёт корреляцию из scoped-контекста — ради неё +// scoped-логгер сюда и заводился. +func TestRetryBestEffortBranchesCarryScope(t *testing.T) { + t.Run("откат активации при провале Add", func(t *testing.T) { + data, ih := buildTorrent(t, "Fargo.mkv") + st := catchedTorrentStore("1", data, ih) + st.downloads["1"].State = store.StateFailed + qb := &fakeQbt{addErr: errors.New("qbit down")} + w := newTestWorker(st, qb) + st.setStateErr = errors.New("db down") // и откат тоже не проходит + + var attrs []slog.Attr + w.log = slog.New(probeHandler{attrs: &attrs}) + + if err := w.Retry(context.Background(), "1"); err == nil { + t.Fatal("ожидалась ошибка добавления") + } + assertScopeAttrs(t, attrs, ih) + }) + + t.Run("сброс базиса таймаутов не прошёл", func(t *testing.T) { + data, ih := buildTorrent(t, "Fargo.mkv") + st := catchedTorrentStore("1", data, ih) + st.downloads["1"].State = store.StateFailed + st.retriedErr = errors.New("db down") + qb := &fakeQbt{} + w := newTestWorker(st, qb) + + var attrs []slog.Attr + w.log = slog.New(probeHandler{attrs: &attrs}) + + // Best-effort: сам retry состоялся, несмотря на отказ сброса. + if err := w.Retry(context.Background(), "1"); err != nil { + t.Fatalf("Retry: %v", err) + } + if st.downloads["1"].State != store.StateDownloading { + t.Errorf("state = %q, want downloading", st.downloads["1"].State) + } + assertScopeAttrs(t, attrs, ih) + }) + + t.Run("сброс счётчика пропусков не прошёл", func(t *testing.T) { + data, ih := buildTorrent(t, "Fargo.mkv") + st := catchedTorrentStore("1", data, ih) + st.downloads["1"].State = store.StateFailed + st.downloads["1"].SourceMissCount = 3 + st.missCntErr = errors.New("db down") + qb := &fakeQbt{} + w := newTestWorker(st, qb) + + var attrs []slog.Attr + w.log = slog.New(probeHandler{attrs: &attrs}) + + if err := w.Retry(context.Background(), "1"); err != nil { + t.Fatalf("Retry: %v", err) + } + assertScopeAttrs(t, attrs, ih) + }) +} + +// assertScopeAttrs проверяет, что scoped-логгер загрузки собран и несёт +// корреляционные поля. +func assertScopeAttrs(t *testing.T, attrs []slog.Attr, infohash string) { + t.Helper() + got := map[string]string{} + for _, a := range attrs { + got[a.Key] = a.Value.String() + } + if got["download_id"] != "1" { + t.Errorf("download_id = %q, want 1", got["download_id"]) + } + if got["infohash"] != infohash { + t.Errorf("infohash = %q, want %q", got["infohash"], infohash) + } +} diff --git a/internal/worker/worker.go b/internal/worker/worker.go index 759318a..896e60f 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -1035,6 +1035,12 @@ func (w *Worker) Retry(ctx context.Context, id string) (err error) { if d.State != store.StateFailed && d.State != store.StateStuck { return fmt.Errorf("retry: download %s is %s, only failed/stuck are retriable: %w", id, d.State, ErrConflict) } + // Scoped-логгер загрузки — ДО первого вызова внешнего сервиса (обязанность + // вызывающего, capability identity). Иначе запись клиента qBittorrent об + // отказе добавления уходит без download_id/infohash, а причины отказа + // qBittorrent не сообщает («Fails.») — корреляция там единственное, что + // делает запись пригодной для разбора. Форма та же, что у Delete. + ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash()) // Если раздача уже жива и ЗДОРОВА в qBittorrent — перецепляемся к ней, // повторный Add не нужен (и вреден: вслепую дублировал бы торрент). Add — когда // источника в qBittorrent нет. Базис таймаута сбрасывается ниже через @@ -1076,8 +1082,7 @@ func (w *Worker) Retry(ctx context.Context, id string) (err error) { // Байты torrent недоступны — откатываем активацию, задача не должна // «качаться» без раздачи в qBittorrent. if rbErr := w.store.SetDownloadState(ctx, id, d.State, d.ErrorCode.String, d.ErrorMsg.String); rbErr != nil { - w.log.Error("retry rollback failed", - "capability", capReview, "download_id", id, "error", rbErr) + logctx.From(ctx).Error("retry rollback failed", "error", rbErr) } return fmt.Errorf("retry: prepare add: %w", prepErr) } @@ -1085,8 +1090,7 @@ func (w *Worker) Retry(ctx context.Context, id string) (err error) { // Активация уже прошла — откатываем задачу в прежнее состояние, // чтобы не оставить «качающуюся» задачу без раздачи в qBittorrent. if rbErr := w.store.SetDownloadState(ctx, id, d.State, d.ErrorCode.String, d.ErrorMsg.String); rbErr != nil { - w.log.Error("retry rollback failed", - "capability", capReview, "download_id", id, "error", rbErr) + logctx.From(ctx).Error("retry rollback failed", "error", rbErr) } return fmt.Errorf("retry: add to qbittorrent: %w", err) } @@ -1096,8 +1100,7 @@ func (w *Worker) Retry(ctx context.Context, id string) (err error) { // stuck_after на ближайшем тике. Best-effort: сбой лишь лишает свежего окна // (WARN), сам retry уже состоялся. if err := w.store.SetRetriedAt(ctx, id, w.now()); err != nil { - w.log.Warn("retry basis reset failed", - "capability", capReview, "download_id", id, "error", err) + logctx.From(ctx).Warn("retry basis reset failed", "error", err) } // Сброс счётчика пропусков источника: retried source_gone-задача иначе вошла бы // в downloading с source_miss_count == threshold и упала бы снова на ближайшем @@ -1105,11 +1108,10 @@ func (w *Worker) Retry(ctx context.Context, id string) (err error) { // обещанного грейс-окна (MAJOR-3). Best-effort: сбой лишь лишает свежего окна. if d.SourceMissCount != 0 { if err := w.store.SetSourceMissCount(ctx, id, 0); err != nil { - w.log.Warn("retry miss count reset failed", - "capability", capReview, "download_id", id, "error", err) + logctx.From(ctx).Warn("retry miss count reset failed", "error", err) } } - logctx.From(w.scoped(ctx, capReview, id, d.PrimaryInfohash())).Info("state transition", + logctx.From(ctx).Info("state transition", "from", d.State, "to", store.StateDownloading) return nil } diff --git a/internal/worker/worker_test.go b/internal/worker/worker_test.go index 6984412..56772a0 100644 --- a/internal/worker/worker_test.go +++ b/internal/worker/worker_test.go @@ -24,6 +24,9 @@ type fakeStore struct { transitions []transition torrents map[string][]byte // download_id → байты .torrent promoteErr error // если задан — PromoteCatched возвращает его, НЕ меняя state (симуляция транзиентного сбоя БД) + setStateErr error // если задан — SetDownloadState отказывает (ветка отката Retry) + retriedErr error // если задан — SetRetriedAt отказывает (best-effort сброс базиса) + missCntErr error // если задан — SetSourceMissCount отказывает (best-effort сброс счётчика) } type transition struct { @@ -166,6 +169,9 @@ func (f *fakeStore) AddInfohashes(_ context.Context, id string, hashes []string) } func (f *fakeStore) SetDownloadState(_ context.Context, id string, st store.State, code, msg string) error { + if f.setStateErr != nil { + return f.setStateErr + } d, ok := f.downloads[id] if !ok { return fmt.Errorf("download %s not found", id) @@ -213,6 +219,9 @@ func (f *fakeStore) SetParsedContext(_ context.Context, id, jsonStr string) erro } func (f *fakeStore) SetSourceMissCount(_ context.Context, id string, n int) error { + if f.missCntErr != nil { + return f.missCntErr + } d, ok := f.downloads[id] if !ok { return fmt.Errorf("download %s not found", id) @@ -233,6 +242,9 @@ func (f *fakeStore) SetSourceAddedAt(_ context.Context, id string, t time.Time) } func (f *fakeStore) SetRetriedAt(_ context.Context, id string, t time.Time) error { + if f.retriedErr != nil { + return f.retriedErr + } d, ok := f.downloads[id] if !ok { return fmt.Errorf("download %s not found", id) @@ -283,6 +295,7 @@ type fakeQbt struct { torrentsErr error onTorrents func() // вклинивается в момент листинга (симуляция гонки между снимком и re-read) added []qbt.AddRequest + addCtx []context.Context // ctx каждого вызова Add (проверка scoped-логгера) addErr error onAdd func() // вклинивается в момент Add (симуляция отмены в окне после add) files []qbt.File @@ -321,7 +334,8 @@ func (f *fakeQbt) Torrents(_ context.Context, category string) ([]qbt.Torrent, e return out, nil } -func (f *fakeQbt) Add(_ context.Context, ar qbt.AddRequest) error { +func (f *fakeQbt) Add(ctx context.Context, ar qbt.AddRequest) error { + f.addCtx = append(f.addCtx, ctx) if f.onAdd != nil { f.onAdd() } diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/.openspec.yaml b/openspec/changes/archive/2026-08-06-ingest-nits/.openspec.yaml new file mode 100644 index 0000000..84cfc12 --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-06 diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/design.md b/openspec/changes/archive/2026-08-06-ingest-nits/design.md new file mode 100644 index 0000000..b556ee0 --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/design.md @@ -0,0 +1,315 @@ +## Context + +Ревью приёма 2026-07-08 (Fable) оставило четыре нити — N1, N3, N4, N5. Они не +связаны причиной, но связаны местом: все четыре живут на пути «источник → +загрузка → добавление в qBittorrent» и все четыре читаются одним заходом. +Изменение косметическое по последствиям и трогает пять пакетов, поэтому решения +о **месте** правки важнее самих правок. + +Текущее состояние, проверенное по коду: + +- `internal/torrent/torrent.go:72` — `DisplayName: meta.BestName()`. + **Постановка задачи и первая редакция этого дизайна исходили из неверного + факта** — будто библиотека сама подставляет `-` безымянной раздаче. Проверено + по исходникам `anacrolix/torrent@v1.61.0`: `BestName()` + (`metainfo/info.go:200-205`) возвращает `NameUtf8`, иначе `Name`, иначе пустую + строку; `NoName = "-"` (`info.go:44`) присваивается только в + `BuildFromFilePath` при **авторинге** раздачи из вырожденного пути. Значит + `-` доходит до нас лишь тогда, когда раздача объявила его полем `name` сама + (та самая конвенция «имени нет», ради совместимости с которой библиотека + константу и экспортирует). Раздача **без** поля `name` даёт пустое имя, и этот + случай код уже отрабатывает верно. Потребителей `DisplayName` три: + `Info.Context()` (строка названия), `ingest.parse` (`source_ref`, + единственный, кто про `-` знает) и `worker.sourceAddParts` (подсказка имени + для `namer`, то есть вход LLM). +- `internal/ingest/ingest.go` — все выходы с ошибкой возвращают `Result{}` + (строки 74–75, 86, 106–107 на момент ревью). Комментарии + `internal/httpapi/httpapi.go:574` и `internal/tgbot/bot.go:256` описывают + прежний контракт «сбой после создания задачи → непустой `DownloadID`». +- `internal/qbt/qbt.go:246` — ветка `Fails.` пишет `call.Failure` со счётчиками + `urls`/`torrents`. Логгер клиент берёт из `ctx` (`logctx.FromOr`). Из двух + вызовов `qbt.Add` фоновый (`worker.go:511`) идёт со scoped-логгером + (`cctx`, `worker.go:415`), а `Retry` (`worker.go:1084`) — с сырым `ctx` + транспорта: `Retry` не присваивает `ctx = w.scoped(…)`, в отличие от `Delete` + (`review.go:619`), и трижды строит scoped-логгер одноразово прямо в аргументе + записи. **Первая редакция этого дизайна считала `Retry` единственной такой + командой — ревью изменения показало, что их семь** (см. «Что изменилось после + ревью изменения»). +- `anacrolix/torrent v1.61.0`: `bencode/decode.go:17` задаёт + `DefaultDecodeMaxStrLen = 1<<27-1`; `parseStringLength` (там же, `:211`) + сверяет объявленную длину с этим потолком, а `parseString` (`:250`, `:258`) + делает `make([]byte, length)` **до** чтения байтов. `metainfo.Load` + (`metainfo/metainfo.go:35-37`) создаёт декодер без переопределения + `MaxStrLen`, то есть с потолком по умолчанию. + +## Goals / Non-Goals + +**Goals:** + +- Сентинел «без имени» не покидает пакет разбора — ни одному потребителю ниже + по потоку не нужно про него знать. +- Комментарии транспортов описывают контракт `Ingest`, который есть, и контракт + подтверждён тестом, а не только чтением. +- Запись о неуспешном добавлении в qBittorrent коррелируется с загрузкой на + **обоих** путях добавления. +- Знание о пределе аллокации bencode записано с провенансом и замером. + +**Non-Goals:** + +- Не чиним поведение `anacrolix/torrent` — форк, `replace` и собственный + разборщик bencode вне объёма и вне границы домена. +- Не добавляем клиенту `qbt` знание о доменных сущностях (инфохэш, id загрузки) + — см. решение 3. +- Не трогаем `files()` и его фолбек на `meta.BestName()` для пути файла: это + вырожденный случай (файл без пути **и** раздача без имени), к контексту + распознавания отношения не имеющий. +- Не переписываем `Cancel`/`Dismiss` под ту же форму, что `Retry`: внешних + вызовов у них нет, требование `identity` их не касается, правка была бы + churn'ом (записано в «Открытые вопросы»). + +## Decisions + +### 1. N1 — нормализация имени стоит в `torrent.Parse`, а не в `Context()` + +Задача формулировала фикс как «фильтровать `-` и в `Context()`». Это лечит +симптом: `DisplayName` остаётся заражённым, а знание о вырожденном имени +копируется в третье место. Потребителей у поля три, и третий +(`worker.sourceAddParts` → подсказка имени для LLM) в постановке не назван, хотя +страдает так же: `-` уезжает во вход вывода отображаемого имени. + +Решение: нормализовать в `Parse` — `DisplayName` пуст, если разобранное имя +равно `metainfo.NoName`. Тогда `Context()` уже имеет проверку `name != ""` и не +меняется вовсе, `ingest.parse` теряет ветку `|| ref == "-"`, а подсказка имени +чинится без единой строки в `worker`. + +Альтернатива «фильтровать в трёх местах» отвергнута: три копии знания об одном +вырожденном значении — ровно то, что архитектурный проход называет вторым +способом делать одно и то же. Альтернатива «оставить `-` и научить каждого +потребителя» хуже: потребители появляются, а значение одно. + +Сравнение делаем с `metainfo.NoName`, а не со строковым литералом, по одной +причине — библиотека экспортирует константу **именно** затем, чтобы на неё +ссылались («By exposing it in the API we can check for references to this +behaviour», `metainfo/info.go:41-44`). Аргумент «сломается компиляция, а не +поведение» здесь **неверен** и снят: константа строковая, смена её значения +компиляцию не ломает. + +Тем же местом закрывается и вторая грязь в строке названия: имя — недоверенный +вход, а контекст читается построчно, поэтому имя со встроенным переводом строки +добавляет в контекст строку, выглядящую как синтезированный нами факт. +Схлопывание разделителей строк уже применяется к комментарию торрента +(`oneLine`), и распространить его на имя — одна строка. **Инъекцией в промпт это +не управляет и управлять не может:** пользовательский текст контекста +многострочен по замыслу и идёт в промпт как есть; правило устраняет +несогласованность (комментарий схлопываем, имя нет), а не заводит защиту. + +### 1а. Что изменилось после ревью предложения + +Проход `specs` опроверг фактическое основание N1 оракулом — прогоном +крафт-входов через `metainfo.Load`. Из этого следует три правки, уже внесённые +выше и в дельта-спеку: + +- формулировка требования говорит про **объявленное раздачей** вырожденное имя, + а не про сентинел, подставляемый библиотекой; +- случай «раздача без поля `name`» назван отдельно и признан уже работающим — + под него заводится сценарий и тест, потому что раньше он путался с первым + (комментарий `internal/torrent/torrent_test.go:224` «info без имени + (NoName-сентинел)» и есть источник исходной ошибки ревью 2026-07-08); +- критерий приёмки постановки «для безымянного торрента `Context()` не отдаёт + `-`» выполняется **до** изменения. Это дефект критерия, а не задачи: + проверяемое им поведение уже верно, а чинится соседнее. В докладе исход + критерия назван честно, и заодно проверено то, что критерий имел в виду. + +### 2. N3 — контракт объявляется в `ingest`, транспорты его не пересказывают + +Комментарии в `httpapi` и `tgbot` разошлись с кодом потому, что каждый держал +**свою** копию контракта. Правка «переписать оба комментария» повторяет ту же +конструкцию и разойдётся снова. + +Решение: контракт («на любом пути ошибки возвращается нулевой результат») +объявляется один раз — в доке `ingest.Ingest` и `ingest.Result` — и +подтверждается тестом в пакете `ingest`. Транспорты получают короткий +комментарий-ссылку и передают в диагностику **пустой** идентификатор, а не +заведомо пустой `res.DownloadID`: значение, которое доказуемо всегда пусто, +прочитанное как «а вдруг непусто», и породило исходную нить. + +Альтернатива «оставить `res.DownloadID`, поправить только текст» отвергнута: +код продолжит утверждать обратное тому, что говорит комментарий. + +**Контракт держится структурой, а не перечнем веток** (найдено проходом +`rubric`): `Ingest` получает именованный возврат и один `defer`, обнуляющий +результат при ненулевой ошибке. Иначе лекарство повторяет болезнь — сегодня три +ветки возврата аккуратны, а завтра четвёртая вернёт полузаполненный результат, +и тест, перечисляющий три известных класса отказа, этого не заметит. Тест +сравнивает результат с нулевым значением **целиком**, а не по полю +`DownloadID`. + +Вызовов приёма на транспортах **три**, а не два (найдено проходом `specs`): +REST `handleAPIAdd`, веб-форма `handleUIAdd` и `tgbot.ingestAndReply`. Третий в +первой редакции задач отсутствовал — добавлен, иначе требование «транспорты не +обещают идентификатора, которого нет» оказалось бы выполненным на две трети. + +Корреляционный ключ у отказа приёма **разный по транспортам**, и требование +называет это прямо: HTTP и веб-UI дают `request_id`, Telegram не даёт ничего +(`opErr` при пустом идентификаторе возвращает текст без ключа). Это не +регрессия — сегодня `res.DownloadID` уже всегда пуст, и наблюдаемое поведение +Telegram не меняется. Но пробел реален, и заводить ли Telegram-транспорту +собственный ключ — записанный вопрос, а не молчаливое «потом». + +### 3. N4 — корреляция приходит из `ctx`, полей клиенту не добавляем + +Задача предлагала «логировать хеши/первые байты». Клиент `qbt` инфохэша не +знает: на magnet-ветке у него ссылка, на torrent-ветке — байты, и вычислить хеш +он мог бы только собственным разбором источника, а разбор источника — единая +точка проекта (`internal/magnet`, `internal/torrent`). Поле `Infohash` в +`AddRequest` завело бы второй канал корреляции рядом со scoped-логгером и жило +бы только ради лога. + +Настоящая причина того, что оператор не находит запись: `Retry` не кладёт +scoped-логгер загрузки в `ctx`. Фоновый путь добавления это делает и запись +`Fails.` там уже несёт `download_id` и `infohash`; путь retry — нет. То есть N4 +— не «клиент мало пишет», а «один вызывающий не выполнил обязанность, которую +выполняют остальные». + +Решение: `Retry` присваивает `ctx = w.scoped(ctx, capReview, id, +d.PrimaryInfohash())` сразу после чтения загрузки, а одноразовые `w.scoped(…)` +и ручные `w.log.…` внутри него схлопываются в `logctx.From(ctx)`. Обязанность +вызывающего закрепляется требованием в `identity` — **ограниченным вызовом +внешнего сервиса**, а не «первым вызовом, способным что-либо записать» (найдено +проходами `architecture` и `specs`): в широкой формулировке требование объявляло +бы нарушителями `Cancel` и `Dismiss`, которые этот же change сознательно не +трогает, и нормативная спека стала бы ложной в момент архивации. + +Побочный эффект присвоения назван заранее, чтобы на чекпоинте 2 он не читался +регрессией: поля `capability`/`download_id`/`infohash` появятся также у +`qbt.Torrents` из `torrentByInfohash`, у `logCmd` и у предупреждений внутри +метода. Это и есть цель. Двойной `download_id` в `logCmd` (из логгера и явным +аргументом) — существующее поведение пути `Delete`, прецедент есть, чистить его +здесь не будем. + +Тест записи о `Fails.` проверяет не только наличие полей, но и **отсутствие +лишнего**: значение отправленного magnet с узнаваемым `passkey` в записи не +встречается ни в одном поле (найдено проходом `rubric`). Инвариант «секреты не +попадают в логи» — `major`, а `add` — единственный вызов `qbt`, через который +magnet-URI приватного трекера физически проходит. + +«Первые байты источника» в лог не идут: для magnet это часть URI (шум, а на +приватном трекере — потенциально чувствительный passkey), для torrent — начало +bencode, диагностической ценности не несущее. + +Причину отказа qBittorrent мы при этом не получаем и не выдумываем: `Fails.` +причины не несёт, и никакого «это был дубль» из него не выводится. Запись честно +остаётся «отказ без причины», но становится привязанной к загрузке. + +### 4. N5 — исход пункта запись, а не код + +Пункт про аллокации bencode правке не подлежит: это чужая библиотека, поведение +ограничено (потолок ~128 MiB) и завершается ошибкой, а не порчей данных. +Записываем наблюдение в `docs/research/` с провенансом (версия, файл:строка) и +**замером**, снятым воспроизводимой командой во временном каталоге, а не +оценкой из головы. Требование каталога разведки — «каждый вывод с числами и +командой, которой получен» — выполняется буквально. + +Порог, ниже которого пункт стал бы дефектом: если бы аллокация не была +ограничена или не завершалась ошибкой. Это проверяется замером, а не +рассуждением, — потому замер и делается. + +Записка получает **условие устаревания** («перепроверить при обновлении +`anacrolix/torrent`»): наблюдение о коде зависимости протухает от бампа +`go.mod`, в отличие от наблюдения о формате провода, которое живёт своей жизнью. +Вводная `docs/research/README.md` расширяется на полстроки, чтобы каталог честно +включал и такие наблюдения (найдено проходом `architecture`). Туда же уезжает +второе наблюдение о той же библиотеке — про `BestName`/`NoName`: цена нити N1 +ровно в том, что этого наблюдения не было записано нигде. + +## Risks / Trade-offs + +- **Нормализация `DisplayName` меняет вход `namer` для безымянных раздач** → + раньше в подсказку уходил `-`, теперь пусто. Это улучшение (вход LLM чище), но + формально смена поведения. Митигация: путь покрыт тестом приёма на безымянной + раздаче; `namer` пустую подсказку и так обрабатывает (magnet без `dn` даёт то + же самое). +- **Scoped-логгер в `Retry` добавляет поля к записям, которых там раньше не + было** → тесты, сверяющие записи журнала на пути retry, могут разойтись. + Митигация: гейт (`test`, `flaky`) ловит это детерминированно. +- **Замер аллокаций bencode делается на синтетическом входе** → он показывает + поведение декодера, а не профиль реального `.torrent`. Митигация: в записке + прямо сказано, что число получено на крафт-входе, и приведена команда. +- **Пустой идентификатор в диагностике транспорта** → если контракт `Ingest` + когда-нибудь начнёт возвращать id на ошибке, транспорты его не покажут. + Митигация: контракт закреплён спекой и тестом, менять его придётся осознанно + и вместе с транспортами. + +## Migration Plan + +Миграции нет: схема БД, конфигурация, HTTP-контракт и тексты Telegram не +меняются. Нумерованных артефактов изменение не добавляет. Откат — обычный +revert коммита. + +## Что изменилось после ревью изменения (чекпоинт 2) + +Профиль `deep`, семь проходов. Отработано инлайн: + +- **Порядок в `displayName` был неверен** — сравнение с сентинелом стояло до + схлопывания, и имя `" - "` превращалось в `-` уже после проверки. Три прохода + (`specs`, `adversary`, `reimpl`) нашли это независимо, два принесли падающий + тест; `adversary` показал, что это **регрессия против `master`**, где фолбек + `source_ref` на имя файла срабатывал. Порядок исправлен и закреплён в спеке. +- **Команд без scoped-логгера оказалось семь, а не одна.** Кроме `Retry`, + внешний сервис до постановки логгера зовут `Relink`, `Rerecognize`, `Refine` + (qBittorrent через `ensureSourceReady`) и `ChooseCandidate`, + `AddManualSource`, `SetProviderID` (метабаза через `recognizer.Director`). + Развилка разрешена в пользу починки всех семи: узкая альтернатива требовала + сузить требование `identity` до одного пути, и тогда нормативная спека + фиксировала бы слепоту как норму. Конструкция, снимающая класс целиком + (обёртка входа команд), — записанный вопрос ниже. +- Добавлены недостающие оракулы: отказ приёма на обоих HTTP-транспортах + коррелируется по `request_id`; best-effort ветки `Retry` исполняются тестами. +- Записка разведки получила текст программы замера и **разграничение + `TotalAlloc` против RSS**: замер `ops` показал, что физическая память при + деградационном пути не расходуется вовсе, и вывод записки от этого становится + сильнее, а не слабее. + +## Open Questions + +- **Заводить ли обёртку входа команд воркера, делающую пропуск scoped-логгера + невозможным?** Пролог всех ~18 публичных команд однороден: `defer logCmd` → + `w.mu.Lock` → чтение загрузки → проверка состояния → `ctx = w.scoped(…)`. + Последний шаг держится дисциплиной, и дисциплина уже дала семь промахов из + ~дюжины — то есть это не гипотеза, а измеренная частота. Варианты: + (а) обёртка `w.withDownload(ctx, op, capability, id, allowedStates, fn)` — + пропуск невозможен по построению, цена: правка всех команд разом и потеря + гибкости там, где capability меняется по ходу метода (`review.go` дважды + пере-скопливает под `capFileLayout`); (б) тест-перебор публичных команд, + краснеющий на команде без scope, — дешевле и не трогает продовый код, но + ловит только известную форму нарушения; (в) оставить дисциплину. Пока + решения нет, стоит (в) — все известные нарушения починены. Рекомендация — + (б) отдельной задачей: механизация без переписывания, ровно по процедуре + промоута «находка → конвенция → правило». +- **Нужен ли Telegram-транспорту собственный корреляционный ключ у отказа + приёма?** Сегодня `opErr` при пустом идентификаторе возвращает текст без + ключа, а конвенция (`docs/conventions/errors.md`, «Граница и трансляция») + требует к сообщению **корреляционный ключ** — `download_id` либо + `request_id`. На пути приёма из Telegram нет ни того, ни другого, и это + главный сценарий паспорта. Варианты: (а) завести идентификатор запроса на + входе обработчика апдейта (или взять `update_id`) и класть его в scoped-логгер + и в текст отказа — цена в том, что рядом с существующей корреляцией по + `download_id`/`infohash` появляется второй канал, ровно тот класс, который + решение 3 отвергло для `qbt`; (б) не заводить ключ, а поднять запись отказа + разбора с `DEBUG` до `INFO` с `infohash`, и тогда искать по инфохэшу — дешевле, + но у пользователя в руках всё равно ничего нет; (в) оставить как есть. + Пока решения нет, стоит (в): изменение поведения Telegram не меняет и + регрессии не вносит. Рекомендация — (б) отдельной задачей: она закрывает + наблюдаемость, не заводя второго канала корреляции. Пробел существует до этого + изменения и им не создан. +- **Приводить ли `Cancel` и `Dismiss` к той же форме, что `Retry` и `Delete` + (присвоение `ctx = w.scoped(…)` в начале команды вместо одноразового + построения логгера в аргументе записи)?** Варианты: (а) привести — правило + «команда воркера скопливает `ctx` один раз» становится однородным, цена — + правка двух методов без наблюдаемого эффекта, риск разойтись с тестами + журнала; (б) не трогать — у этих команд нет внешних вызовов, поэтому + наблюдаемой разницы нет, цена — форма остаётся неоднородной, и следующий + вызывающий скопирует ту из двух, что попалась. Пока решения нет, стоит (б): + изменение закрывает N4 и не расширяется. Рекомендация — (а) отдельной + задачей-гигиеной, когда в `Cancel`/`Dismiss` появится первый внешний вызов; + раньше этого момента правка не окупается. diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/proposal.md b/openspec/changes/archive/2026-08-06-ingest-nits/proposal.md new file mode 100644 index 0000000..dec4197 --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/proposal.md @@ -0,0 +1,78 @@ +## Why + +Ревью приёма 2026-07-08 оставило четыре нити, каждая из которых по отдельности +дёшева, а вместе они портят три разных наблюдаемых поверхности: контекст +распознавания (грязная строка), комментарии в коде (описывают контракт, которого +уже нет), журнал (запись о неуспешном добавлении без корреляции) и знание о +границе чужой библиотеки (не записано нигде). Пока нити открыты, каждый +следующий проход ревью тратит внимание на то, что уже разобрано. + +## What Changes + +- **N1 — вырожденное имя не покидает разборщик.** Раздача может объявить полем + `name` значение `-` — конвенция «имени нет» (в библиотеке разбора это + константа `metainfo.NoName`). Сейчас `.torrent`-приём отбрасывает `-` только + для `source_ref`, а `torrent.Info.Context()` и подсказка имени для `namer` + берут его как содержательное название. Нормализация переезжает на границу + разбора: `DisplayName` пустеет прямо в `torrent.Parse`, и три места ниже по + потоку перестают знать про вырожденное значение. Тем же местом схлопываются + разделители строк в имени — иначе имя добавляет в построчный контекст строку, + выглядящую как синтезированный нами факт. + **Основание нити уточнено ревью предложения:** библиотека `-` не синтезирует + (`BestName()` на раздаче без имени возвращает пустую строку), поэтому речь о + разборе объявленного значения, а не о фильтре чужого сентинела; случай + «раздача без поля `name`» уже отрабатывается верно и получает свой сценарий, + чтобы впредь не путаться с первым. +- **N3 — комментарии описывают фактический контракт `Ingest`.** После + fast-catch-рефактора `Ingest` возвращает нулевой `Result` на **каждом** пути + ошибки, а комментарии в `httpapi` и `tgbot` до сих пор обещают непустой + `DownloadID` «при сбое после создания задачи». Контракт закрепляется в + доке `Ingest`, удерживается структурно (обнуление результата одним `defer`, а + не аккуратностью каждой ветки), подтверждается тестом и перестаёт + пересказываться неверно на транспортах. Вызовов приёма три, а не два: + REST, веб-форма и Telegram. +- **N4 — запись о неуспешном добавлении в qBittorrent коррелируется.** Клиент + `qbt` берёт логгер из `ctx` и своего инфохэша не знает (и знать не должен — + разбор источника живёт в `magnet`/`torrent`). Путь `worker.Retry` не кладёт в + `ctx` scoped-логгер загрузки, поэтому запись `Fails.` с этого пути уходит без + `download_id`/`infohash`. Retry приводится к тому же виду, что и `Delete`. + **Ревью изменения показало, что таких команд не одна, а семь** — те же + `Relink`, `Rerecognize`, `Refine`, `ChooseCandidate`, `AddManualSource`, + `SetProviderID` зовут qBittorrent или метабазу до постановки логгера. + Починены все семь: иначе требование `identity` архивировалось бы ложным. +- **N5 — граница чужой библиотеки записывается наблюдением.** `anacrolix/torrent` + аллоцирует объявленную bencode-строку до её чтения, с потолком ~128 MiB. + Правке не подлежит (чужой код), поэтому исход — запись в `docs/research/` с + провенансом и замером, а не изменение кода. + +## Capabilities + +### New Capabilities + +Нет — изменение не вводит нового поведения и новых понятий. + +### Modified Capabilities + +- `ingest`: приём `.torrent` перестаёт считать вырожденное имя `-` + содержательным — ни в синтезированном контексте, ни в подсказке имени, ни в + `source_ref`; плюс фиксируется контракт «на ошибке приёма результат пуст» и + поимённо называется корреляционный ключ отказа на каждом транспорте. +- `identity`: корреляция в журнале распространяется на записи о вызовах внешних + сервисов, сделанных в контексте загрузки, — включая путь retry. + +## Impact + +- `internal/torrent` — нормализация `DisplayName` на границе разбора; тест на + безымянной раздаче. +- `internal/ingest` — упрощение фильтра `source_ref`, дока и структурная + гарантия контракта `Result`, тест «на ошибке результат нулевой». +- `internal/httpapi` (REST `handleAPIAdd` **и** веб-форма `handleUIAdd`), + `internal/tgbot` — комментарии и передача пустого id в диагностику вместо + заведомо пустого `res.DownloadID`. +- `internal/worker` — `Retry` кладёт scoped-логгер загрузки в `ctx`. +- `internal/qbt` — тест с подставным сервером на поля записи о неуспешном `add`. +- `docs/research/` — новая записка про границы разбора `.torrent` (предел + `MaxStrLen` bencode + наблюдение о `BestName`/`NoName`); строка в индексе + `docs/research/README.md` и полстроки в его вводной. +- Схема БД, конфиг, API и тексты Telegram не затрагиваются; нумерованных + артефактов (миграций, ADR) изменение не добавляет. diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/review/checkpoint-1-design.md b/openspec/changes/archive/2026-08-06-ingest-nits/review/checkpoint-1-design.md new file mode 100644 index 0000000..59a9f4f --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/review/checkpoint-1-design.md @@ -0,0 +1,137 @@ +# Чекпоинт 1 — ревью предложения, профиль `design` + +- **Change:** `ingest-nits` +- **База диффа:** `master` (`5c79fdfffe9e2ddc78b7ebf9f4cdc3aa83560b2b`); кода на + ветке на момент прогона не было. +- **Профиль:** `design`. **Режим прогона:** `по графу` (все три прохода одной + волной — граф профиля плоский, машину не держит никто). +- **Дата:** 2026-08-06. + +## Запущенные проходы и исход + +| Проход | Исход | +| --- | --- | +| `review-specs` | 6 находок (2 `major`, 4 `minor`) + 5 границ спеки | +| `review-rubric` (фаза 1) | рубрика из 12 свойств, 8 находок (2 `major`, 6 `minor`), 5 promote-кандидатов | +| `review-architecture` | 2 находки (обе `major`) + 3 пункта «дешевле переделать до мерджа» | + +Триаж на этом чекпоинте не запускается — так устроен профиль: находок единицы, +каждая либо правит спеку, либо становится записанным вопросом. Сведение сделал +оркестратор пайплайна. + +## Что отработано инлайн + +Дедуплицировано по причине; в скобках — кто нашёл. + +1. **Фактическое основание N1 было неверным** (`specs`, `major`, оракул — + прогон крафт-входов через `metainfo.Load`). Библиотека сентинел `-` не + синтезирует: `BestName()` на раздаче без имени возвращает пустую строку, а + `NoName` присваивается только при авторинге. Значит `-` доходит до нас лишь + от раздачи, объявившей его полем `name`. Следствия: критерий приёмки + постановки выполнялся **до** изменения; случай «раздача без `name`» не был + покрыт вовсе; неверный комментарий в `internal/torrent/torrent_test.go:224` и + есть источник исходной ошибки ревью 2026-07-08. → требование, дизайн и + предложение переписаны по наблюдению; два входа разведены сценариями и + тестами; комментарий теста правится. +2. **Требование `identity` заявляло больше, чем change делает** (`architecture` + и `specs`, оба `major`): обязанность формулировалась как «до первого вызова, + способного что-либо записать», и поимённо называла `cancel`/`dismiss`, + которые change сознательно не трогает. Нормативная спека стала бы ложной в + момент архивации. → обязанность сужена до **вызова внешнего сервиса**. +3. **`request_id` вменялся всем транспортам, а существует только на HTTP** + (`architecture` `major`, `specs` `minor`, `rubric` `major`). → требование + называет ключ поимённо по транспортам; для Telegram зафиксировано отсутствие + ключа как сегодняшнее состояние, вопрос записан в `design.md` → Open + Questions. Регрессии нет: `res.DownloadID` уже сегодня всегда пуст. +4. **Telegram числился среди `ext.*`-клиентов, которых у него нет** (`specs`, + `minor`). → убран; перечень клиентов не дублируется, а отдан конвенции. +5. **Третий вызов приёма (`handleUIAdd`, веб-форма) отсутствовал в задачах** + (`specs`, `minor`). → добавлен пункт 2.4; иначе требование было бы выполнено + на две трети. +6. **Заявленная однородность с фильтром magnet-заглушек ложна** (`specs`, + `minor`): у magnet заглушка `dn` фильтруется только в контексте, а в + подсказку имени уходит. → различие названо явно, распространение на magnet + объявлено отдельным изменением. +7. **Спека утверждала абсолют «ниже по потоку сентинел не встречается», а путь + файла его выпускает** (`rubric`, `minor`). → нормализуемые поля перечислены + поимённо, про пути файлов сказано, почему они не нормализуются. +8. **Имя с переводом строки подменяет строки построчного контекста** (`rubric`, + `major`). → нормализация расширена на разделители строк и краевые пробелы + тем же `oneLine`, что уже применён к комментарию. В требовании прямо сказано, + что защитой от инъекции в промпт это **не** является: пользовательский текст + многострочен по замыслу. +9. **Контракт «результат пуст» держался перечнем веток** (`rubric`, `minor`). → + удерживается структурно: именованный возврат + один `defer`; тест сравнивает + результат с нулевым значением целиком. +10. **У третьего потребителя `DisplayName` (подсказка namer) не было оракула** + (`rubric`, `minor`). → добавлен пункт 1.5, тест в `internal/worker`. +11. **Тест записи о `Fails.` проверял только наличие полей** (`rubric`, + `minor`). → добавлена негативная половина: `passkey` отправленного magnet в + записи не встречается. Инвариант «секреты не в логи» — `major`. +12. **Сценарий требовал `infohash` там, где тело требования смягчало до «когда + известен»** (`specs`, граница спеки). → в GIVEN добавлено «с известным + инфохэшем». +13. **Каталог `docs/research/` тихо расширялся с чужих данных на чужой код** + (`architecture`). → вводная README расширяется, записка получает условие + устаревания. + +## Записанные вопросы + +Оба — в `design.md` → Open Questions, оба с вариантами, ценой и рекомендацией. + +1. Нужен ли Telegram-транспорту собственный корреляционный ключ у отказа приёма. + Рекомендация — поднять запись отказа разбора до `INFO` с `infohash` отдельной + задачей, а не заводить второй канал корреляции. +2. Приводить ли `Cancel`/`Dismiss` к форме `ctx = w.scoped(…)`. Рекомендация — + отдельной задачей-гигиеной, когда в них появится первый внешний вызов. + +## Урожай (заведение задач — не работа пайплайна) + +- **Заглушка `dn` вида `*-topic-` уходит в подсказку вывода имени**, хотя из + контекста отфильтрована (`worker.sourceAddParts` → `magnet.Parse` → + `DisplayName`, `internal/worker/worker.go:626`). Симметрично N1, но на + magnet-ветке. Правка требует изменения действующего требования «Синтез + контекста распознавания из полей magnet» (сценарий «Строки-факты не становятся + отображаемым именем»), поэтому в границы этой задачи не входит. Оракул: тест + на подсказку для голого magnet с заглушкой. Провенанс: `specs`, change + `ingest-nits`. +- **`infohash` в записи о внешнем вызове снимается со снимка загрузки, а не с + того, с чем вызов ушёл** (`rubric`, `low`). Сегодня значения совпадают; + расхождение возможно у загрузки с несколькими хешами (v1+v2). Класс совпадает + с журналом 2026-08-06 (признак снят с одной сущности, действие применено к + другой), но стоит здесь только разбора, не данных. +- **Пустой `source_ref` у torrent-загрузки** возможен: `.torrent` с именем `-` и + без имени присланного файла. Действующее требование велит `source_ref` быть + человекочитаемым референсом, но случая «нет ни того, ни другого» не описывает. + Дыра не новая, но изменение проходит ровно по ней. Провенанс: `specs`, + границы спеки. +- **Отказ разбора источника пишется на `DEBUG`** (`internal/ingest/ingest.go`), + то есть на боевом `INFO` не пишется вовсе — для Telegram-пути это означает, что + у отказа приёма нет ни ключа у пользователя, ни записи в журнале. Связано с + записанным вопросом 1. +- **Promote candidates** (`rubric`): (а) «scoped-логгер загрузки строится один + раз, на входе публичной команды воркера» → `docs/conventions/logging.md` плюс + механизация тестом-перебором; (б) «нормализация недоверенного имени на границе + — это вырожденные значения, краевые пробелы и разделители строк» → + `docs/review.md`, «Парсер недоверенного входа»; (в) «у каждого транспорта + назван свой корреляционный ключ публичного канала» → + `docs/conventions/errors.md`; (г) «тест клиента внешнего сервиса проверяет и + отсутствие секретосодержащих значений» → `docs/conventions/logging.md`. + +## Границы покрытия + +- **Гейт на этом чекпоинте не запускался** — кода на ветке не было; это + устройство профиля `design`, а не пропуск. +- **Триаж не запускался** — в профиле `design` его нет по построению; сведение + и дедупликацию находок сделал оркестратор пайплайна, то есть **тот же, кто + писал предложение**. Разведённости приёмщика и исполнителя на этом чекпоинте + нет. +- **Не запускались** проходы `code`, `adversary`, `ops`, `reimpl` — они не + входят в профиль `design` и работают после apply (чекпоинт 2). +- **Замер аллокаций bencode** на этом чекпоинте не воспроизводился ни одним + проходом: `specs` проверил только провенанс ссылок по исходникам библиотеки. + Сам замер — задача 4.1, его исход проверяет чекпоинт 2. +- **Ни один проход не открывал живой qBittorrent, LLM и метабазы** — запрещено + `CLAUDE.md` → «Запреты». +- **Ценность самой постановки** («нужно ли закрывать эти четыре нити») ревью не + оценивает ни в одном профиле. diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/review/checkpoint-2-deep.md b/openspec/changes/archive/2026-08-06-ingest-nits/review/checkpoint-2-deep.md new file mode 100644 index 0000000..de4152c --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/review/checkpoint-2-deep.md @@ -0,0 +1,350 @@ +# Чекпоинт 2 — ревью изменения после apply, профиль `deep` + +- **Change:** `ingest-nits` +- **База диффа:** `master` (`5c79fdfffe9e2ddc78b7ebf9f4cdc3aa83560b2b`); + изменения на ветке `task/ingest-nits` не закоммичены. +- **Профиль:** `deep`. **Режим прогона:** `по графу`. +- **Гейт:** ЗЕЛЁНЫЙ — 12/12 шагов `OK`, ни одного `SKIP`/`WARN`; `-race` + реально прогнан; diff-coverage 81 % (7 изменённых строк не покрыты — они и + стали находками F1/F2). +- **Дата:** 2026-08-06. + +## Запущенные проходы и исход + +Состав сверен с профилем `deep`: все семь стадий запускались, расхождений с +профилем нет. + +| Проход | Исход | +| --- | --- | +| `review-gate` | отработал: 2 находки (обе `major`, пробелы покрытия изменённых строк) | +| `review-specs` | отработал: 5 находок + 3 границы спеки | +| `review-code` | отработал: 0 находок (нарушений записанных конвенций нет), 1 promote-кандидат | +| `review-adversary` | отработал: 4 находки + 1 гипотеза; фаззинг `torrent.Parse`+`Context()` 6.1 млн исполнений — паник нет; утечек экранирования нет | +| `review-ops` | отработал: 5 находок (3 с замерами); гипотезы «ERROR-шторм», «сирота при отмене ctx», «kill посреди раскладки» сняты проверкой | +| `review-reimpl` | отработал: 3 находки + перечень мест, где код лучше независимой реализации | +| `review-architecture` | отработал: 2 находки + 3 пункта «дешевле переделать до мерджа»; новых понятий change не вводит | + +На входе — 21 находка + 1 гипотеза + 3 «дешевле до мерджа» + 1 +promote-кандидат. После дедупликации по причине — 16 причин. В основном списке — +6 пунктов (потолок 7 соблюдён); остальное — в гипотезах, promote и урожае, +ничего не выброшено молча. + +Дедупликация: S2 = A1 = R1 (один дефект порядка нормализации, найден тремя +проходами независимо — оракул есть, это повысило приоритет, не confidence); +S1 = R2 = AR1 (ложность требования `identity`; перечни команд расходились — +6 против 3, точный перечень установлен триажем по коду: 6); G1 + S5 (один +пробел — отказ приёма на HTTP-границе без оракула). + +Отчёт чекпоинта 1 (`checkpoint-1-design.md`) учтён: находки, закрытые там +инлайн, повторно не поднимались; урожай и записанные вопросы не дублируются. + +--- + +## Блокирует мердж + +### B1. Имя, схлопывающееся в `-`, проходит нормализацию: `source_ref` и контекст получают сентинел, а фолбек на имя файла регрессировал против master + +- Файл: `internal/torrent/torrent.go:94-99` +- Severity: major (сломано требование дельта-спеки этого же change — + «Вырожденное имя раздачи не считается именем», сценарий «Вырожденное имя даёт + source_ref из имени файла») +- Confidence: high +- Оракул: падающий тест — прогнан триажем: + `tmp/_reimpl/probe/probe_test.go` даёт + `name " - " → DisplayName "-"`, `"-\n" → "-"`, `"\t-" → "-"` + (ожидалось `""`); `Context()` первой строкой несёт `-`. Второй независимый + падающий тест — у прохода `adversary`. Регрессия: на `master` + `internal/ingest` делал `TrimSpace` + сравнение с `-` (`git diff` по + `ingest.go:198-205`), и фолбек на имя присланного файла срабатывал; теперь + `source_ref` становится `-`. +- Причина: в `displayName` сравнение с `metainfo.NoName` стоит **до** + `oneLine`, а `oneLine` (`strings.Fields`) схлопывает `" - "`, `"-\n"`, + `"\t-"` ровно в `-`. +- Последствие: молчаливое — ошибки нет, просто карточка и `source_ref` несут + `-`, контекст распознавания получает строку-мусор вместо отсутствия строки + названия. Ровно тот класс входа, который change обещал закрыть. +- Предложение: в `displayName` сначала `oneLine`, потом сравнение с + `metainfo.NoName` (порядок независимой реализации `reimpl`); тест-входы + `" - "`, `"-\n"`, `"\t-"` в `internal/torrent/torrent_test.go`; в + дельта-спеке зафиксировать порядок (нормализовать → сравнить). +- Найдено проходами: `specs` (S2), `adversary` (A1), `reimpl` (R1) +- Действие: **инлайн** + +### B2. При архивации нормативная спека `identity` станет ложной: шесть команд воркера зовут внешний сервис до scoped-логгера + +- Файл: `internal/worker/review.go:410/417, 442/445, 465/468, 796, 849; + internal/worker/review.go:734→796`; `openspec/changes/ingest-nits/specs/identity/spec.md:12-17`; + `openspec/changes/ingest-nits/proposal.md:36-38` +- Severity: major +- Confidence: high +- Оракул: поимённое построение путей вызовов, проверено триажем по коду: + 1. `Relink` — `ensureSourceReady` (`review.go:410`) → `torrentByInfohash` → + `qbt.Torrents` **до** `ctx = w.scoped(...)` (`:417`); + 2. `Rerecognize` — `:442` до `:445`; + 3. `Refine` — `:465` до `:468`; + 4. `ChooseCandidate` → `chooseCandidateLocked` (`:796`) — + `recognizer.Director` → `metadata` (HTTP) с необогащённым `ctx` + (команда вообще не делает `ctx = w.scoped`, только точечные + `logctx.From(w.scoped(...))` на своих записях); + 5. `AddManualSource` — тот же путь через `:738`; + 6. `SetProviderID` — `Director` на `:849` с необогащённым `ctx`. + Клиенты metadata логируют через `logctx.FromOr(ctx, ...)` + (`internal/metadata/http.go:78`, `tvdb.go:101,128`) — записи уходят без + `download_id`/`infohash`. Требование дельты (`identity/spec.md:13-15`): + «операция … делающая вызов внешнего сервиса, SHALL положить scoped-логгер … + до такого вызова — включая команды, пришедшие с транспорта». `proposal.md:36-38` + утверждает, что `Retry` — «единственная команда воркера», не кладущая + scoped-логгер: ложь, уедет в архив. Остальные команды (`Apply`, `Undo`, + `Delete`, `Defer`, `RefreshDisplayName`) проверены — они требование соблюдают; + `Cancel`/`Dismiss` внешних вызовов не делают. +- Последствие: класс «молчание» — сбой внешнего сервиса с этих шести путей даёт + запись, которую фильтр по id загрузки не находит; спека, ставшая нормативной, + утверждает обратное, и следующая задача будет строиться на ложном тексте. +- Найдено проходами: `specs` (S1), `reimpl` (R2), `architecture` (AR1) +- Действие: **развилка**. Вопрос: как разрешить расхождение требования + `identity` с кодом? + - (а) сузить требование до фактически закрытого пути (`Retry` + фоновый + цикл), убрав «включая команды, пришедшие с транспорта» из нормы в цель. + Цена: спека честна, но шесть команд остаются без корреляции внешних + вызовов, вопрос вернётся на следующем ревью identity. + - (б) починить шесть команд в этом change: `Relink`/`Rerecognize`/`Refine` — + перенести `ctx = w.scoped(...)` выше `ensureSourceReady`; + `ChooseCandidate`/`AddManualSource`/`SetProviderID` — обогатить `ctx` на + входе (форма `Retry`/`Delete`). Цена: ~6 однотипных мелких правок + тесты + по образцу нового probe-хендлера `torrent_add_test.go`; scope растёт + умеренно, инвариантов не трогает. + - (в) = (б) сейчас + отдельная задача в беклог на обёртку + `w.withDownload(...)`, схлопывающую однородный пролог ~18 команд и делающую + пропуск невозможным по построению (рекомендация `architecture`). + Рекомендация проходов — (в). В любом варианте `proposal.md` («единственная + команда») требует правки — при (б)/(в) формулировкой «был не единственным», + при (а) — снятием претензии. + +## Стоит исправить сейчас + +### F1. Сценарий «Отказ приёма на HTTP-границе» остался без оракула: ветка ошибки веб-формы не исполняется ни одним тестом + +- Файл: `internal/httpapi/httpapi.go:437` (изменённая строка); дельта-спека + `ingest`, сценарий «Отказ приёма на HTTP-границе» +- Severity: major +- Confidence: high +- Оракул: `grep request_id internal/httpapi/*_test.go` — пусто; ни один тест не + делает `POST /ui/downloads` (форма добавления): проверено триажем — тесты + ходят только в `/ui/downloads/{id}/...`. REST-ошибки приёма покрыты + (`httpapi_test.go:138,154`), но `request_id` в ответе не проверяет и REST. + Асимметрия: Telegram-путь получил новый тест, веб-форма — нет. +- Последствие: THEN-обещание дельта-спеки (ответ несёт `request_id`, а не + идентификатор загрузки) не проверяет никто; регрессия на этой строке будет + молчаливой. +- Предложение: тест на `handleUIAdd` с `fakeIngestor.err` (редирект с ошибкой, + без download_id) + проверка `request_id` в теле/заголовке отказа на обоих + HTTP-транспортах. +- Найдено проходами: `gate` (G1), `specs` (S5) +- Действие: **инлайн** + +### F2. Best-effort-ветки `Retry` (откат активации, сбросы базиса и счётчика) переписаны диффом и не исполняются тестами + +- Файл: `internal/worker/worker.go:1085, 1093, 1103, 1111` +- Severity: major +- Confidence: high +- Оракул: diff-coverage гейта — 4 из 7 непокрытых изменённых строк; `fakeStore` + не умеет ронять `SetDownloadState`/`SetRetriedAt`/`SetSourceMissCount`. + Строки в диффе: `w.log.Error(...)` заменён на `logctx.From(ctx)...` (проверено + `git diff`). +- Последствие: пути отката «активация прошла, add не удался» держат инвариант + «задача не качается без раздачи в qBittorrent» — и не проверяются; молчаливая + регрессия отката оставила бы задачу в `downloading` без источника. +- Предложение: научить `fakeStore` инъекции ошибок этих трёх сеттеров; тесты: + откат возвращает прежнее состояние/код/сообщение, сбой best-effort-сеттеров + не проваливает состоявшийся retry (только WARN). +- Найдено проходом: `gate` (G2) +- Действие: **инлайн** + +### F3. Записка research обещает несуществующий текст замера и переоценивает риск: мерился `TotalAlloc`, а физический RSS почти не растёт + +- Файл: `docs/research/torrent-bencode-limits.md:22-26, 49-53` +- Severity: minor +- Confidence: high +- Оракул: (1) записка обещает «текст — в этом change, + `openspec/changes/archive/ingest-nits/`» — в каталоге change программы нет + (проверено листингом; в `tmp/` лежит только `bencode-alloc.out`), нарушено + правило воспроизводимости `docs/research/README.md`; (2) замер `ops`: + `make([]byte, 128MiB)` без касания страниц → +800 КБ RSS; деградационный путь + (`make` → проваленный `io.ReadFull` на 36-байтовом входе) страниц не касается; + concurrency=1000 → пик RSS 7 352 КБ. Вывод «N одновременных дают до + N × 128 MiB» относится к виртуальным аллокациям, не к памяти процесса. +- Последствие: будущий читатель примет решение (лимиты, семафор приёма) по + завышенной оценке риска; невоспроизводимость обесценивает записку как + наблюдение. +- Предложение: вложить текст программы в записку (или в change) и дописать + разграничение `TotalAlloc` vs RSS с числами замера `ops` — вывод «принято как + есть» усиливается, а не смягчается. +- Найдено проходами: `adversary` (A4), `ops` (O4) +- Действие: **инлайн** + +### F4. Дельта-спека `ingest` в двух местах описывает не то поведение, которое реализовано и принято + +- Файл: `openspec/changes/ingest-nits/specs/ingest/spec.md:28-30, 32-35` +- Severity: minor +- Confidence: high +- Оракул: (1) спека: нормализация схлопывает «любые разделители строк и краевые + пробелы»; код: `oneLine` = `strings.Join(strings.Fields(s), " ")` + (`torrent.go:234-236`) — любые пробельные последовательности, включая + внутренние табы/пробелы. Поведение кода корректно — неверен текст спеки. + (2) спека: пути файлов «не выводятся из имени»; код: `files()` при пустом + пути делает `p = meta.BestName()` (`torrent.go:114-116`) — сырое имя. + Решение не нормализовать пути остаётся верным (они только сигнал, целевые + пути строятся из ответа qBittorrent) — ложно только основание. +- Последствие: нормативный текст разойдётся с кодом в момент архивации; при B1 + спека всё равно правится — дешевле сделать одним заходом. +- Предложение: (1) сформулировать правило как «любые пробельные + последовательности схлопываются в один пробел»; (2) заменить основание + исключения путей на честное (путь может совпасть с именем при пустом + `fi.Path`; не нормализуются, потому что служат только сигналом). +- Найдено проходом: `specs` (S3, S4) +- Действие: **инлайн** + +## Гипотезы без доказательства + +- **`naming.sanitize` не режет разделители пути, а выведенное имя уходит в + qBittorrent параметром `rename`** (`adversary`; заявлен `major`, + confidence medium — остаётся гипотезой). Неизвестно, применяет ли qBittorrent + `rename` к папке на диске — если да, крафт-имя могло бы дать запись по + произвольному относительному пути под `paths.downloads` (тень инварианта + «источник неприкосновенен»). Оракула нет и не будет в конвейере: ходить в + живой qBittorrent запрещено (`CLAUDE.md` → «Запреты»). Диффом не внесено + (`Retry` namer не зовёт — имя выводится при первом добавлении). Предложение: + задача в беклог — интеграционный тест за env-гейтом + (`*_integration_test.go`) на поведение `rename` c `/` и `..` в имени; до + ответа — дешёвый пояс: резать разделители пути в подсказке имени. + +## Promote candidates + +- **`docs/conventions/errors.md`: корреляционный ключ для транспорта без + понятия запроса.** Конвенция описывает трансляцию ошибок через `request_id`, + но Telegram запроса не имеет — правило «у каждого транспорта назван свой + корреляционный ключ публичного канала» просится в конвенцию (найдено + `code`; пересекается с Open Question 1 в `design.md` и promote-кандидатом + (в) чекпоинта 1 — при заведении объединить). +- **`docs/conventions/logging.md`: команда воркера не дублирует поля + scoped-логгера.** После `ctx = w.scoped(...)` отложенный + `logCmd(ctx, cmd, id, err)` пишет `download_id` и из логгера, и явным + аргументом — дублирующийся ключ в JSON-записи `command failed` (найдено + `adversary`, A3; путь `retry` — в диффе, та же пара давно у `Delete`). + Как находка это nit без записанной конвенции — потому promote: правило + + возможная механизация тестом-перебором команд (родственно promote (а) + чекпоинта 1). + +## Урожай + +Отложено: вне диффа, ниже порога либо требует отдельной задачи. Формат: +формулировка — оракул — провенанс. + +1. **`recognizePending` при недоступном qBittorrent не обрывается по первому + сбою**: каждая `completed`-задача уезжает в `review` с диагностикой про + qBittorrent — тест с фейковым недоступным qbt и 2+ задачами — `ops` (O1), + вне диффа. +2. **`SupersedeForeignLinks` — `SCAN file_link` без индекса под `w.mu`**: + `EXPLAIN QUERY PLAN` + замер ~24 мс на 200k строк, растёт линейно, ретеншена + нет — `ops` (O2), вне диффа; задача: индекс или ретеншен. +3. **`Retry` держит глобальный `w.mu` через оба внешних вызова; таймаут + qbt-клиента (30 с дефолт) не настраивается конфигом**: замер — параллельный + `Cancel` другой загрузки ждал 282 мс при qbt 300 мс — `ops` (O3), диффом не + внесено (присвоение передвинуто внутри той же секции); связать с задачей на + `w.withDownload` из B2-(в) — per-download блокировка там же. +4. **`source_ref` не ограничен по длине**: `.torrent` 7 МиБ с именем 7 МиБ даёт + `source_ref` 7 340 032 байта при соседе `capContext` 16 КиБ; читается + `SELECT *` на каждом тике — замер `adversary` (A2); строка переписана диффом, + но поведение не новое; задача: кап по образцу `capContext`. +5. **Пустые `source_ref`+`display_name`: веб-UI рисует пустой заголовок + карточки, Telegram — `#id`**: чтение кода (`httpapi/download.go:71-81` vs + `tgbot/render.go:187-205`) — `ops` (O5), вне диффа; связан с записью урожая + чекпоинта 1 «пустой source_ref возможен» — закрывать одной задачей. +6. **Два рукодельных slog-хендлера родились в одном change** + (`captureHandler` в `internal/qbt/qbt_test.go`, `probeHandler` в + `internal/worker/torrent_add_test.go`), уже разошлись возможностями — + `architecture` (AR2); порог превращения в находку — третий потребитель. +7. **Нормализация комментария осталась в `Context()`, имени — в `Parse`**, + хотя спека подаёт правило как общее; сегодня `Info.Comment` вне пакета никто + не читает — `reimpl` (R3, confidence low); при следующей правке пакета — + `Comment: oneLine(mi.Comment)` в `Parse` либо оговорка в спеке. +8. **Доккомментарии** (по одной строке, «дешевле до мерджа» от `architecture`; + оркестратор может сделать заодно с B2/F4): `ingest.Ingest` цитирует + требование по русскому имени — при переименовании требования ссылка + оборвётся молча; `magnet.Info.DisplayName` не оговаривает, что оно НЕ + нормализуется, в отличие от `torrent.Info.DisplayName`. +9. **Приоритет `NameUtf8` над `Name` не описан спекой** — граница спеки — + `specs`; при случае — строкой в требование нормализации. +10. **Запись отказа приёма — `DEBUG`, на боевом `INFO` не пишется вовсе**, а + требование отправляет искать диагностику Telegram-отказа по этой записи — + `specs`; уже записано вопросом 1 в `design.md` → Open Questions с + рекомендацией поднять до `INFO` отдельной задачей — не дублировать. + +Отсев вкусовщины: типовых generative-находок (переименования, перестановки, +«вынести в файл», обобщение частного случая) в выводах не оказалось; единственный +кандидат — A3 (дублирующийся ключ) — переведён в promote, т.к. записанной +конвенции под nit нет. Ни одна находка не попала под «Типовые ложноположительные» +`docs/review.md` — раздел просмотрен поимённо (никто не предлагал авторизацию, +интерфейсы под моки, ретраи в тике и т.п.). + +## Границы покрытия + +**Прогон.** Профиль `deep`, режим «по графу», чекпоинт 2. Запускались все семь +проходов профиля (перечень с исходами — в сводке); не запускался никакой — состав +полный. На чекпоинте 1 (профиль `design`) шли `review-specs`, `review-rubric`, +`review-architecture`; `rubric` на чекпоинте 2 не запускается по построению +профиля. + +**Что каждый запущенный проход не мог проверить в принципе (из charter'ов):** + +- `gate` — только механизируемое с объективным оракулом; осмысленность тестов + и спек не видит. +- `specs` — сверяет спеку с кодом; поведение, отсутствующее в обоих, не найдёт. +- `code` — только записанные конвенции; «хорошо ли это» вне них не судит. +- `adversary` — не ходил в живой qBittorrent (запрет `CLAUDE.md`) — гипотеза + про `rename` осталась гипотезой; фаззинг ограничен бюджетом 45 с; целевой + путь из имени раздачи не строится, поэтому главный вопрос раздела «Вопросы к + проходам» (выход за библиотеку через имена) на этом диффе не применим. +- `ops` — реального профиля нагрузки umbar нет ни у кого; замеры синтетические + (200k строк, concurrency=1000), боевые числа могут отличаться. +- `reimpl` — переписывал только затронутые нити; расхождение вне них не + обнаруживает. +- `architecture` — судит по карте и диффу, код не исполняет. +- Триаж — ничего нового не ищет по определению; пропуск любого прохода — его + пропуск; здесь пропусков состава нет. + +**Осталось целиком на человеке.** Два списка из `docs/review.md` → +«Недоступно проверке», раздельно. + +*Не проверит ни один проход (принципиальная граница):* + +- история инцидентов на umbar и что уже ломалось в проде; +- поведение SQLite под реальным объёмом и профилем нагрузки; +- завязка внешних потребителей (Jellyfin, закладки, чужие ссылки) на текущее + поведение; +- качество распознавания как таковое (корпус решено не собирать, + `tasks/REJECTED.md`, 2026-08-06); +- суждение «этой функциональности не должно существовать». + +*Перестали проверять сознательно (пересматривается первым при промахе):* + +- идиоматичность Go — с 2026-08-04, проход `idiom` упразднён при переезде на + плагин `av-dev-pipeline`; различение «идиоматично против распространено» не + спрашивает никто; пересмотр — задача `quality-review-agents`. + +Плюс общее для любого прогона: поведение под реальным потоком; поведение +внешних систем в их боевых версиях (в этом чекпоинте конкретно — реакция +qBittorrent на `rename` с разделителями пути и физическое поведение +`deleteFiles=true`); история инцидентов. + +**Документы проекта.** Всех нужных хватило: `CLAUDE.md` с разделом инвариантов +(severity брались оттуда, а не выводились), `docs/review.md` с «Типовыми +ложноположительными» (отсев шёл по ним) и обоими подразделами «Недоступно +проверке», конвенции, `docs/research/`, дельта-спеки change. Одна оговорка: +журнал дефектов содержит единственную запись (2026-08-06, про уборку торрента) — +оракулов-прецедентов для классов находок этого чекпоинта в нём нет, подтверждение +«такое здесь уже воспроизводилось» было недоступно; все оракулы добывались +тестами и замерами прогона. + +**Потолок.** В основной список не влезли и уехали в урожай десять пунктов — +все перечислены поимённо выше, молча не выброшено ничего. diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/specs/identity/spec.md b/openspec/changes/archive/2026-08-06-ingest-nits/specs/identity/spec.md new file mode 100644 index 0000000..11da37c --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/specs/identity/spec.md @@ -0,0 +1,48 @@ +## MODIFIED Requirements + +### Requirement: Корреляция сущностей в логах + +Записи журнала, относящиеся к сущности, SHALL содержать её id в атрибуте +`_id` (`download_id`, `recognition_id`, `batch_id`, …); работа в +контексте загрузки ведётся через scoped-логгер с `download_id`. Благодаря +глобальной уникальности ULID поиск по значению id (grep/jq) SHALL находить +все записи журнала, относящиеся к сущности, независимо от имени поля. + +Scoped-логгер SHALL передаваться через `context`, а не доклеиваться к каждой +записи руками. Отсюда обязанность вызывающего, и она ограничена наблюдаемым +исходом: **операция, работающая в контексте загрузки и делающая вызов внешнего +сервиса, SHALL положить scoped-логгер этой загрузки в `context` до такого +вызова** — включая команды, пришедшие с транспорта, а не только фоновый цикл +воркера. Однородность формы у команд, внешних вызовов не делающих, это +требование не нормирует: она принадлежит конвенциям кода. + +Причина в том, что клиент внешнего сервиса своей доменной сущности не знает и +знать SHALL NOT — он берёт логгер из `context`. Поэтому вызов внешнего сервиса в +контексте загрузки SHALL давать запись с `download_id` и, когда он известен, +`infohash`; добавлять клиенту поля-дубликаты доменных идентификаторов ради этого +SHALL NOT — источник корреляции один. + +Перечень клиентов, ведущих записи о внешних вызовах, живёт в +`docs/conventions/logging.md` и здесь не дублируется. Telegram-клиент таких +записей не ведёт, и уведомление отправляется вне контекста загрузки намеренно +(иначе оно умирало бы вместе с тиком) — это требование его не касается. + +Отдельно это важно там, где внешний сервис не сообщает причину отказа: ответ +qBittorrent `Fails.` на добавление раздачи причины не несёт, и единственное, что +делает такую запись пригодной для разбора, — корреляция с загрузкой. + +#### Scenario: Путь загрузки по логам + +- **GIVEN** загрузка прошла приём, распознавание и раскладку +- **WHEN** журнал фильтруется по значению её `id` +- **THEN** находятся записи всех этапов (ingest, recognition, file-layout) + +#### Scenario: Неуспешное добавление в qBittorrent с пути retry + +- **GIVEN** загрузка в `failed` с известным инфохэшем, для которой оператор + запросил retry +- **WHEN** qBittorrent отвечает на добавление отказом (`Fails.` либо не-200) +- **THEN** запись о вызове внешнего сервиса содержит `download_id` и `infohash` + загрузки +- **AND** запись находится тем же фильтром по значению id, что и записи + фонового пути добавления diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/specs/ingest/spec.md b/openspec/changes/archive/2026-08-06-ingest-nits/specs/ingest/spec.md new file mode 100644 index 0000000..2abec54 --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/specs/ingest/spec.md @@ -0,0 +1,125 @@ +## ADDED Requirements + +### Requirement: Вырожденное имя раздачи не считается именем + +Система SHALL нормализовать имя раздачи из `.torrent` **на границе разбора +источника** (`internal/torrent`) и не считать содержательным именем вырожденное +значение `-` — тот же литерал, которым экосистема обозначает «имени нет» +(в библиотеке разбора он выставлен константой `metainfo.NoName` и присваивается +при авторинге раздачи из вырожденного пути). Раздача объявляет это значение +**сама**, полем `name`; библиотека его не синтезирует, поэтому речь о разборе +объявленного значения, а не о фильтре чужого сентинела. + +Раздача, у которой поля `name` нет вовсе, уже даёт пустое имя — это отдельный +случай, и он нормализации не требует. Оба случая после разбора SHALL быть +неразличимы: имени нет. + +Нормализация SHALL применяться к **имени раздачи** и охватывать три поля, +которые из него выводятся: + +- строка названия в синтезированном контексте распознавания (см. «Приём + источника из .torrent-файла») — за неимением имени строка названия + отсутствует; +- `source_ref` загрузки — работает прежний фолбек на имя присланного файла; +- подсказка вывода отображаемого имени, которую воркер берёт из метаданных + торрента (см. `download-tracking` «Добавление пойманной загрузки в + qBittorrent») — подсказка остаётся пустой. + +Пути файлов раздачи нормализации **не** подлежат, и это осознанно. Вырожденное +имя попасть в путь файла может — при пустом наборе сегментов путь берётся +фолбеком из имени раздачи, ненормализованного. Допустимо потому, что путь файла +читается только как число файлов и набор расширений (сигнал по дереву), а +целевые пути раскладки строятся не из него, а из ответа qBittorrent +(см. `file-layout`). Появится потребитель, читающий путь как путь, — +нормализовать его надо там же, на границе разбора. + +Имя раздачи — недоверенный вход, а контекст распознавания читается +**построчно**. Поэтому нормализация SHALL схлопывать в один пробел любые +последовательности пробельных символов — включая разделители строк — и убирать +краевые, чтобы имя не могло добавить в контекст строку, выглядящую как +синтезированный нами факт. Схлопывание SHALL выполняться **до** сравнения с +вырожденным значением, иначе имя вида `" - "` сравнение не пройдёт, а после +схлопывания станет ровно вырожденным и уедет вниз по потоку. Тем же правилом +уже обрабатывается комментарий торрента. Защитой от инъекции в промпт оно +**не** является и таковой объявляться SHALL NOT: пользовательский текст +контекста многострочен по замыслу и идёт в промпт как есть (см. «Синтез +контекста распознавания из полей magnet»). + +Правило **не** однородно с фильтром заглушек-идентификаторов `dn` у magnet: там +заглушка вида `*-topic-` отбрасывается только в контексте, а в подсказку +вывода имени по-прежнему уходит (действующее требование «Синтез контекста +распознавания из полей magnet»). Здесь правило строже, и распространение его на +magnet — отдельное изменение, этим требованием оно не заказано. + +#### Scenario: Раздача объявила имя `-` + +- **GIVEN** `.torrent`, у которого поле `name` равно `-` либо становится равным + `-` после схлопывания пробельного (например `" - "`) +- **WHEN** система разбирает источник и синтезирует контекст распознавания +- **THEN** имя раздачи после разбора пусто, строки названия в контексте нет +- **AND** остальные строки-факты (размер, файлы, трекер, комментарий) + синтезируются как обычно +- **AND** подсказка вывода отображаемого имени пуста + +#### Scenario: Раздача без поля `name` + +- **GIVEN** `.torrent` без поля `name` +- **WHEN** система разбирает источник +- **THEN** имя раздачи пусто — так же, как у раздачи, объявившей `-` + +#### Scenario: Вырожденное имя даёт source_ref из имени файла + +- **GIVEN** `.torrent` с именем `-`, присланный файлом `Dune.torrent` +- **WHEN** вызывается приём +- **THEN** `source_ref` загрузки — `Dune.torrent`, а не `-` + +#### Scenario: Имя с переводом строки не добавляет строк в контекст + +- **GIVEN** `.torrent`, имя которого содержит перевод строки и текст, похожий на + синтезированный факт +- **WHEN** система синтезирует контекст распознавания +- **THEN** имя занимает ровно одну строку названия +- **AND** число строк-фактов в контексте такое же, как у раздачи с обычным именем + +### Requirement: Результат приёма при ошибке пуст + +Приём SHALL возвращать транспорту **нулевой результат** на любом пути ошибки: +идентификатор загрузки, инфохэши, состояние и признак дедупликации в этом случае +не публикуются. Причина в том, что быстрый приём не создаёт наблюдаемых +последствий раньше, чем становится способен вернуть успех: разбор источника, +дедуп-чек и заведение загрузки идут до ответа, а всё, что может отказать после +заведения (добавление в qBittorrent, вывод имени), выполняет воркер и на исход +приёма не влияет. + +Контракт SHALL удерживаться **структурно** — одним местом обнуления результата +при ненулевой ошибке, а не аккуратностью каждой ветки возврата: перечень веток +растёт, и именно расхождение перечня с текстом породило исходный дефект. + +Транспорты SHALL опираться на этот контракт и SHALL NOT обещать пользователю или +журналу идентификатор загрузки, которого нет. Корреляционный ключ публичного +отказа приёма зависит от транспорта, и требование называет его поимённо: + +- **HTTP API и веб-UI** — `request_id` запроса (см. `docs/conventions/errors.md`, + «Граница и трансляция»); +- **Telegram** — корреляционного ключа у отказа приёма нет, и требование это + фиксирует как сегодняшнее состояние, а не как цель: диагностика ищется по + записи приёма (`capability=ingest`, `infohash`). Заводить Telegram-транспорту + собственный идентификатор запроса это требование SHALL NOT. + +#### Scenario: Невалидный источник + +- **WHEN** приём получает источник, который не разбирается ни как magnet, ни + как `.torrent` +- **THEN** возвращается ошибка и нулевой результат (идентификатор загрузки пуст) + +#### Scenario: Сбой хранилища на заведении загрузки + +- **WHEN** приём получает валидный источник, но хранилище отказывает при + дедуп-чеке или заведении загрузки +- **THEN** возвращается ошибка и нулевой результат + +#### Scenario: Отказ приёма на HTTP-границе + +- **GIVEN** приём отказал по любой причине +- **WHEN** HTTP-транспорт (REST или веб-форма) отвечает пользователю +- **THEN** ответ несёт `request_id` запроса, а не идентификатор загрузки diff --git a/openspec/changes/archive/2026-08-06-ingest-nits/tasks.md b/openspec/changes/archive/2026-08-06-ingest-nits/tasks.md new file mode 100644 index 0000000..276cf1c --- /dev/null +++ b/openspec/changes/archive/2026-08-06-ingest-nits/tasks.md @@ -0,0 +1,142 @@ +## 1. N1 — вырожденное имя не покидает разборщик + +- [x] 1.1 В `internal/torrent/torrent.go` нормализовать `DisplayName` в `Parse`: + сравнение с `metainfo.NoName` даёт пустую строку; краевые пробелы и + разделители строк схлопываются тем же `oneLine`, что уже применяется к + комментарию. В доке `Info.DisplayName` сказать, что нормализация здесь и + ниже по потоку вырожденное значение не встречается. +- [x] 1.2 В `internal/ingest/ingest.go` убрать ветку `|| ref == "-"` из + фолбека `source_ref` и поправить комментарий (нормализовано разборщиком). +- [x] 1.3 Тесты в `internal/torrent`, три входа врозь: (а) `name` равно `-` → + `DisplayName` пуст и `Context()` без строки названия; (б) `name` + отсутствует → `DisplayName` пуст (случай уже работал, закрепляем, чтобы не + путался с (а)); (в) имя с переводом строки → одна строка названия, число + строк-фактов как у обычного имени. Заодно исправить неверный комментарий + `TestContextEmptyWhenNoFields` («info без имени (NoName-сентинел)») — он и + есть источник исходной ошибки ревью. +- [x] 1.4 Тест в `internal/ingest`: `.torrent` с именем `-`, присланный файлом → + `source_ref` равен имени файла. +- [x] 1.5 Тест в `internal/worker`: для раздачи с именем `-` подсказка, + уходящая в `namer`, пуста (третий потребитель `DisplayName` — вход LLM; + `fakeNamer` дополнить полем `gotHint`). + +## 2. N3 — контракт «на ошибке результат пуст» + +- [x] 2.1 В `internal/ingest/ingest.go` объявить контракт в доке `Ingest` и + `Result` и удержать его **структурно**: именованный возврат + один + `defer`, обнуляющий результат при ненулевой ошибке. +- [x] 2.2 Тест в `internal/ingest`: на каждом классе отказа (невалидный + источник, сбой хранилища на дедуп-чеке, сбой хранилища на заведении) + результат равен нулевому значению **целиком**, а не только по полю + `DownloadID`. +- [x] 2.3 `internal/httpapi/httpapi.go`, REST `handleAPIAdd` — заменить + устаревший комментарий и передавать в `s.apiErr` пустой идентификатор. +- [x] 2.4 `internal/httpapi/httpapi.go`, веб-форма `handleUIAdd` — то же для + `userErr` (третий вызов приёма, в первой редакции задач пропущен). +- [x] 2.5 `internal/tgbot/bot.go` — то же для `opErr`; в комментарии назвать + честно, что корреляционного ключа у отказа приёма в Telegram нет. + +## 3. N4 — корреляция записи о неуспешном добавлении + +- [x] 3.1 В `internal/worker/worker.go` метод `Retry` присваивает + `ctx = w.scoped(ctx, capReview, id, d.PrimaryInfohash())` сразу после + чтения загрузки; одноразовые `w.scoped(…)` и ручные `w.log.…` с + доклеенными `capability`/`download_id` внутри метода схлопнуть в + `logctx.From(ctx)`. +- [x] 3.2 Тест в `internal/qbt` с подставным сервером, отвечающим `Fails.`: + запись о вызове содержит поля из scoped-логгера `ctx` (`download_id`, + `infohash`) и `ext.operation = torrents/add`; **негативная половина** — + значение отправленного magnet с узнаваемым `passkey` в записи не + встречается ни в одном поле, включая `error`. +- [x] 3.3 Тест в `internal/worker`: `Retry`, дошедший до `Add`, вызывает клиента + с `ctx`, несущим scoped-логгер загрузки (`fakeQbt` дополнить записью + переданного `ctx`). + +## 4. N5 — записка о границах разбора `.torrent` + +- [x] 4.1 Снять замер во временном каталоге (`tmp/`, после замера удаляется): + крафт-вход, объявляющий длинную bencode-строку, через `metainfo.Load`; + зафиксировать аллокацию и исход для нескольких объявленных длин, включая + значение выше потолка. +- [x] 4.2 Завести `docs/research/torrent-bencode-limits.md`: версия библиотеки, + файл:строка, команда замера, числа, вывод с условиями; наблюдение о + `BestName`/`NoName` (что библиотека сентинел не синтезирует); отметить, + что panic-гард `files()` не покрывает `Load`/`UnmarshalInfo`/`HashBytes`, + и назвать **условие устаревания** — перепроверить при обновлении + `anacrolix/torrent`. +- [x] 4.3 Добавить строку записки в индекс `docs/research/README.md` и + расширить его вводную: каталог держит и наблюдения о поведении наших + разборщиков и зависимостей на границе формата. + +## 5. Отработка ревью изменения (чекпоинт 2, профиль `deep`) + +- [x] 5.1 B1 — в `torrent.displayName` схлопывание идёт ДО сравнения с + `metainfo.NoName`; тест стал табличным (`-`, `" - "`, `"-\n"`, `"\t-"`, + NBSP+`-`); порядок закреплён в дельта-спеке. +- [x] 5.2 B2 — scoped-логгер до внешнего вызова добавлен ещё в шести командах + (`Relink`, `Rerecognize`, `Refine`, `ChooseCandidate`, `AddManualSource`, + `SetProviderID`); ложное «единственная команда» в `proposal.md` + исправлено. +- [x] 5.3 F1 — тест корреляции по `request_id` на обоих HTTP-транспортах + (REST и веб-форма). +- [x] 5.4 F2 — инъекция ошибок в `fakeStore` (`SetDownloadState`, + `SetRetriedAt`, `SetSourceMissCount`) и тесты best-effort веток `Retry`. +- [x] 5.5 F3 — в записку разведки вложен текст программы замера и добавлено + разграничение `TotalAlloc`/RSS с числами. +- [x] 5.6 F4 — в дельта-спеке исправлены два неточных утверждения (широта + схлопывания и обоснование исключения для путей файлов). + +## 6. Проверка + +- [x] 6.1 `task gate` зелёный (go-шаги отработали, не `SKIP`). +- [x] 6.2 `openspec validate --strict ingest-nits`. + +## Приёмочные критерии + +### Из постановки задачи + +Копия из `docs/tasks/items/ingest-nits.md` — приходят снаружи, пайплайном не +сочиняются и не занижаются. + +- Для безымянного торрента `Context()` не отдаёт «-» как название — поле пустое + (оракул: тест разбора на фикстуре безымянного торрента в `internal/torrent`). + **Замечание к критерию:** ревью предложения показало, что для торрента *без + имени* это верно и до изменения; отдаёт «-» торрент, который сам объявил + `name: "-"`. Проверяются оба входа врозь (задача 1.3). +- Комментарии в `httpapi` и `tgbot` описывают фактическое поведение `Ingest`: + на любом пути ошибки возвращается пустой `Result`, корреляция идёт по + `request_id` (оракул: чтение диффа на ревью — механического оракула нет). +- Лог неудачного добавления в qBittorrent несёт инфохэш для корреляции (оракул: + тест клиента с подставным сервером, проверяющий поля записи). +- Наблюдение про аллокации bencode до `MaxStrLen` записано в `docs/research/` + с провенансом либо явно отклонено строкой в теле задачи (оракул: `task gate`, + шаг канона). + +### Из рубрики прохода `review-rubric` + +Свойства, порождённые до чтения кода; взяты те, что изменение обязано +удовлетворить. Непокрытые названы явно. + +- Нормализация вырожденного имени живёт **ровно в одной точке** — на границе + разбора; ни один потребитель `DisplayName` знания о `-` не содержит (оракул: + 1.1–1.2 плюс `grep -rn 'NoName\|"-"' internal --glob '!internal/torrent/**'` + без попаданий по смыслу «имя раздачи»). +- Нормализация имени охватывает разделители строк и краевые пробелы, а не + только вырожденное значение (оракул: 1.3в). +- У **каждого** из трёх потребителей нормализованного поля свой оракул (оракул: + 1.3, 1.4, 1.5 — три теста в трёх пакетах). +- Контракт «на ошибке результат нулевой» удерживается структурой, а не + перечнем известных путей (оракул: 2.1 — одно место обнуления; 2.2 — сравнение + с нулевым значением целиком). +- Публичная диагностика отказа приёма несёт корреляционный ключ, и ключ назван + поимённо для каждого транспорта (оракул: требование дельта-спеки; для Telegram + зафиксировано отсутствие ключа как сегодняшнее состояние — вопрос записан). +- Запись об отказе внешнего сервиса самодостаточна и **не несёт секретов** + (оракул: 3.2, обе половины). +- **Не покрыто и почему:** свойство «обязанность класть scoped-логгер проверена + механически перебором всех команд воркера» — перебор потребовал бы правки + `Cancel`/`Dismiss`, которую этот change сознательно не делает (открытый вопрос + дизайна); оракул остаётся точечным (3.3). Свойство «поле `infohash` записи + называет тот источник, с которым вызов ушёл» — не покрыто: сегодня значения + совпадают, расхождение возможно лишь у загрузки с несколькими хешами; идёт в + урожай. diff --git a/openspec/specs/identity/spec.md b/openspec/specs/identity/spec.md index 8bab59c..3ba5667 100644 --- a/openspec/specs/identity/spec.md +++ b/openspec/specs/identity/spec.md @@ -56,12 +56,45 @@ URL `/download/{id}`, параметры форм и команд. Синтак глобальной уникальности ULID поиск по значению id (grep/jq) SHALL находить все записи журнала, относящиеся к сущности, независимо от имени поля. +Scoped-логгер SHALL передаваться через `context`, а не доклеиваться к каждой +записи руками. Отсюда обязанность вызывающего, и она ограничена наблюдаемым +исходом: **операция, работающая в контексте загрузки и делающая вызов внешнего +сервиса, SHALL положить scoped-логгер этой загрузки в `context` до такого +вызова** — включая команды, пришедшие с транспорта, а не только фоновый цикл +воркера. Однородность формы у команд, внешних вызовов не делающих, это +требование не нормирует: она принадлежит конвенциям кода. + +Причина в том, что клиент внешнего сервиса своей доменной сущности не знает и +знать SHALL NOT — он берёт логгер из `context`. Поэтому вызов внешнего сервиса в +контексте загрузки SHALL давать запись с `download_id` и, когда он известен, +`infohash`; добавлять клиенту поля-дубликаты доменных идентификаторов ради этого +SHALL NOT — источник корреляции один. + +Перечень клиентов, ведущих записи о внешних вызовах, живёт в +`docs/conventions/logging.md` и здесь не дублируется. Telegram-клиент таких +записей не ведёт, и уведомление отправляется вне контекста загрузки намеренно +(иначе оно умирало бы вместе с тиком) — это требование его не касается. + +Отдельно это важно там, где внешний сервис не сообщает причину отказа: ответ +qBittorrent `Fails.` на добавление раздачи причины не несёт, и единственное, что +делает такую запись пригодной для разбора, — корреляция с загрузкой. + #### Scenario: Путь загрузки по логам - **GIVEN** загрузка прошла приём, распознавание и раскладку - **WHEN** журнал фильтруется по значению её `id` - **THEN** находятся записи всех этапов (ingest, recognition, file-layout) +#### Scenario: Неуспешное добавление в qBittorrent с пути retry + +- **GIVEN** загрузка в `failed` с известным инфохэшем, для которой оператор + запросил retry +- **WHEN** qBittorrent отвечает на добавление отказом (`Fails.` либо не-200) +- **THEN** запись о вызове внешнего сервиса содержит `download_id` и `infohash` + загрузки +- **AND** запись находится тем же фильтром по значению id, что и записи + фонового пути добавления + ### Requirement: Миграция существующих записей Существующие записи SHALL получить ULID-идентификаторы одной миграцией с diff --git a/openspec/specs/ingest/spec.md b/openspec/specs/ingest/spec.md index df69a4d..01e4ce0 100644 --- a/openspec/specs/ingest/spec.md +++ b/openspec/specs/ingest/spec.md @@ -645,3 +645,126 @@ SHALL NOT проваливать добавление загрузки, а са - **WHEN** выполняется шаг добавления - **THEN** структура не выводится, `download.parsed_context` пуст +### Requirement: Вырожденное имя раздачи не считается именем + +Система SHALL нормализовать имя раздачи из `.torrent` **на границе разбора +источника** (`internal/torrent`) и не считать содержательным именем вырожденное +значение `-` — тот же литерал, которым экосистема обозначает «имени нет» +(в библиотеке разбора он выставлен константой `metainfo.NoName` и присваивается +при авторинге раздачи из вырожденного пути). Раздача объявляет это значение +**сама**, полем `name`; библиотека его не синтезирует, поэтому речь о разборе +объявленного значения, а не о фильтре чужого сентинела. + +Раздача, у которой поля `name` нет вовсе, уже даёт пустое имя — это отдельный +случай, и он нормализации не требует. Оба случая после разбора SHALL быть +неразличимы: имени нет. + +Нормализация SHALL применяться к **имени раздачи** и охватывать три поля, +которые из него выводятся: + +- строка названия в синтезированном контексте распознавания (см. «Приём + источника из .torrent-файла») — за неимением имени строка названия + отсутствует; +- `source_ref` загрузки — работает прежний фолбек на имя присланного файла; +- подсказка вывода отображаемого имени, которую воркер берёт из метаданных + торрента (см. `download-tracking` «Добавление пойманной загрузки в + qBittorrent») — подсказка остаётся пустой. + +Пути файлов раздачи нормализации **не** подлежат, и это осознанно. Вырожденное +имя попасть в путь файла может — при пустом наборе сегментов путь берётся +фолбеком из имени раздачи, ненормализованного. Допустимо потому, что путь файла +читается только как число файлов и набор расширений (сигнал по дереву), а +целевые пути раскладки строятся не из него, а из ответа qBittorrent +(см. `file-layout`). Появится потребитель, читающий путь как путь, — +нормализовать его надо там же, на границе разбора. + +Имя раздачи — недоверенный вход, а контекст распознавания читается +**построчно**. Поэтому нормализация SHALL схлопывать в один пробел любые +последовательности пробельных символов — включая разделители строк — и убирать +краевые, чтобы имя не могло добавить в контекст строку, выглядящую как +синтезированный нами факт. Схлопывание SHALL выполняться **до** сравнения с +вырожденным значением, иначе имя вида `" - "` сравнение не пройдёт, а после +схлопывания станет ровно вырожденным и уедет вниз по потоку. Тем же правилом +уже обрабатывается комментарий торрента. Защитой от инъекции в промпт оно +**не** является и таковой объявляться SHALL NOT: пользовательский текст +контекста многострочен по замыслу и идёт в промпт как есть (см. «Синтез +контекста распознавания из полей magnet»). + +Правило **не** однородно с фильтром заглушек-идентификаторов `dn` у magnet: там +заглушка вида `*-topic-` отбрасывается только в контексте, а в подсказку +вывода имени по-прежнему уходит (действующее требование «Синтез контекста +распознавания из полей magnet»). Здесь правило строже, и распространение его на +magnet — отдельное изменение, этим требованием оно не заказано. + +#### Scenario: Раздача объявила имя `-` + +- **GIVEN** `.torrent`, у которого поле `name` равно `-` либо становится равным + `-` после схлопывания пробельного (например `" - "`) +- **WHEN** система разбирает источник и синтезирует контекст распознавания +- **THEN** имя раздачи после разбора пусто, строки названия в контексте нет +- **AND** остальные строки-факты (размер, файлы, трекер, комментарий) + синтезируются как обычно +- **AND** подсказка вывода отображаемого имени пуста + +#### Scenario: Раздача без поля `name` + +- **GIVEN** `.torrent` без поля `name` +- **WHEN** система разбирает источник +- **THEN** имя раздачи пусто — так же, как у раздачи, объявившей `-` + +#### Scenario: Вырожденное имя даёт source_ref из имени файла + +- **GIVEN** `.torrent` с именем `-`, присланный файлом `Dune.torrent` +- **WHEN** вызывается приём +- **THEN** `source_ref` загрузки — `Dune.torrent`, а не `-` + +#### Scenario: Имя с переводом строки не добавляет строк в контекст + +- **GIVEN** `.torrent`, имя которого содержит перевод строки и текст, похожий на + синтезированный факт +- **WHEN** система синтезирует контекст распознавания +- **THEN** имя занимает ровно одну строку названия +- **AND** число строк-фактов в контексте такое же, как у раздачи с обычным именем + +### Requirement: Результат приёма при ошибке пуст + +Приём SHALL возвращать транспорту **нулевой результат** на любом пути ошибки: +идентификатор загрузки, инфохэши, состояние и признак дедупликации в этом случае +не публикуются. Причина в том, что быстрый приём не создаёт наблюдаемых +последствий раньше, чем становится способен вернуть успех: разбор источника, +дедуп-чек и заведение загрузки идут до ответа, а всё, что может отказать после +заведения (добавление в qBittorrent, вывод имени), выполняет воркер и на исход +приёма не влияет. + +Контракт SHALL удерживаться **структурно** — одним местом обнуления результата +при ненулевой ошибке, а не аккуратностью каждой ветки возврата: перечень веток +растёт, и именно расхождение перечня с текстом породило исходный дефект. + +Транспорты SHALL опираться на этот контракт и SHALL NOT обещать пользователю или +журналу идентификатор загрузки, которого нет. Корреляционный ключ публичного +отказа приёма зависит от транспорта, и требование называет его поимённо: + +- **HTTP API и веб-UI** — `request_id` запроса (см. `docs/conventions/errors.md`, + «Граница и трансляция»); +- **Telegram** — корреляционного ключа у отказа приёма нет, и требование это + фиксирует как сегодняшнее состояние, а не как цель: диагностика ищется по + записи приёма (`capability=ingest`, `infohash`). Заводить Telegram-транспорту + собственный идентификатор запроса это требование SHALL NOT. + +#### Scenario: Невалидный источник + +- **WHEN** приём получает источник, который не разбирается ни как magnet, ни + как `.torrent` +- **THEN** возвращается ошибка и нулевой результат (идентификатор загрузки пуст) + +#### Scenario: Сбой хранилища на заведении загрузки + +- **WHEN** приём получает валидный источник, но хранилище отказывает при + дедуп-чеке или заведении загрузки +- **THEN** возвращается ошибка и нулевой результат + +#### Scenario: Отказ приёма на HTTP-границе + +- **GIVEN** приём отказал по любой причине +- **WHEN** HTTP-транспорт (REST или веб-форма) отвечает пользователю +- **THEN** ответ несёт `request_id` запроса, а не идентификатор загрузки