ingest: закрыты мелочи приёма — вырожденное имя, контракт Result, корреляция add
- имя раздачи нормализуется на границе разбора: вырожденное `-` (metainfo.NoName) даёт пустое имя, пробельное схлопывается — сентинел больше не доходит ни до контекста распознавания, ни до source_ref, ни до подсказки вывода имени - контракт «на любом пути ошибки приёма результат нулевой» объявлен в ingest и удерживается структурно; три транспорта перестали обещать идентификатор, которого нет, и коррелируют отказ по request_id - scoped-логгер загрузки ставится до вызова внешнего сервиса в семи командах воркера — записи об отказе qBittorrent и метабаз получили download_id и infohash; граница разбора bencode записана в docs/research
This commit is contained in:
@@ -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` появится первый внешний вызов;
|
||||
раньше этого момента правка не окупается.
|
||||
Reference in New Issue
Block a user