fix(cacbg): спирай краула по срок, за да оцелее кешът - #313
Conversation
Test coverage
✅ No workspace dropped below its baseline (tolerance 0.5pp). 📈 Coverage rose by more than 1pp — run |
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: „спирай краула по срок, за да оцелее кешът"
Обща оценка: силен PR, готов за мърдж след дребна корекция в документацията.
Фаза 0 — Сигурност (сканиране): ЧИСТО
- Няма твърдо кодирани тайни, пароли или токени.
- Няма нови/променени URL адреси извън вече използвания
register.cacbg.bg; всичкиactions/cache/*са пинати по SHA (55cc8345…). - Няма злонамерени шаблони, backdoor-и или обфускация.
- Няма нови зависимости. Нов таван
MAX_CONCURRENCY = 8реално подобрява сигурността — премахва възможността--concurrency 500да отвори стотици паралелни връзки към държавен регистър.
Логика (fetch.mjs) — коректна
poolвече връща дали е задържана работа (i < items.length), а не заключение от часовника — това правилно разграничава „последната заявка е паднала след срока, но корпусът е пълен" (exit 0) от „реален срочен стоп" (exit 1). Разграничението е покрито от тест.- Проверката за срок е на два места: в началото на цикъла по папки (out-of-budget сет не оставя следа) и след
pool(стоп в средата на сет). И двата пътя са тествани, вкл. при concurrency 8. - Валидацията на опции е стегната: празен флаг (
--deadline-minutesбез стойност или последван от друг флаг) сега хвърля грешка вместо тихо да падне към подразбиране — правилно. --allow-incompleteумишлено НЕ понижава срочния стоп — коректно за платформа за прозрачност.
Тестове — отлични (3.0/3.0)
Тестовете са проектирани да разкриват дефекти, не да минават тривиално: граница на сет, пропуснат list.xml консумиращ бюджета, точно попадане на срока (>=), срок обвързващ ВСЕки работник при concurrency 8, никога недостигнат срок. Инжектираният часовник прави случаите детерминистични. Няма „чийтър" тестове.
Единствена находка (документация, non-blocking)
Абзацът в ADR-0012 за „re-run" противоречи на реалното поведение на новия ключ run_id-run_attempt и на коментара в самия workflow (виж inline коментара). Това е оперативно подвеждащо и трябва да се коригира, но не блокира.
Заключение
Атомарен, добре тестван и добре обоснован PR. Код и тестове: одобрявам. Препоръка: COMMENT — да се изчисти неточността в ADR-0012 преди мърдж, след което PR е за одобрение.
Текстът описваше поведението отпреди поправката: твърдеше, че всяко
продължаване трябва да е нов dispatch и че re-run "поне се проваля
шумно". И двете вече не са верни.
Ключът е cacbg-raw-${run_id}-${run_attempt}. При "Re-run failed jobs"
GitHub увеличава run_attempt, тоест записът отива на ключ, който още не
съществува, и минава; а restore-keys: cacbg-raw- връща снапшота от
предишния опит, така че краулът продължава оттам. Re-run работи - точно
това решава run_attempt.
Само с run_id записът щеше да се откаже върху неизменим ключ, да се
свали до предупреждение, а стъпката за проверка да намери стария запис и
да удостовери загубата. Затова run_attempt е част от ключа, а не
диагностика.
Поправено на двете места: абзацът в ADR-0012 и коментарът над стъпката
за краул в related-persons-data.yml, който повтаряше същото указание.
Посочено от @ydimitrof в ревюто на PR #313.
95f5f29 to
a5c1bb3
Compare
Пълният корпус е 37 комплекта / ~281 000 декларации и не се побира в 300-минутния таван на related-persons-data. Когато таванът падна по средата (пуск 31889519937), машината уби стъпката, но не и процеса: той продължи да пише, докато `always()` стъпката за запис пускаше tar върху същото дърво. tar се отказа с „file changed as we read it", а actions/cache/save понижава провала при запис до предупреждение — стъпката светна зелена, без да е запазила нищо. Пет часа теглене отидоха на вятъра и следващият пуск започна пак от празен кеш. fetch.mjs вече приема --deadline-minutes: изтече ли срокът, спира да раздава работа, изчаква текущите заявки и се връща. Дървото е спокойно, tar успява, кешът пази изтегленото и следващият пуск продължава оттам. Студен корпус става за два пуска вместо за нула. Спирането по срок е самостоятелна присъда за непълнота, а не част от аритметиката на assessCompleteness — спиране точно на граница между комплекти оставя всеки достигнат комплект пълен, тоест „пълен корпус" без цели години. --allow-incomplete нарочно не го омеква: този ключ значи „видях този недостиг и го приемам", а будилник по средата не е видяна преценка. Заданието спира на 240 при таван 300 и вече проверява, че кешът наистина е записан — стъпката за запис по конструкция не може да падне, тъй че зелено до нея не е доказателство.
Независим преглед с Codex извади две неща, които сам не бях видял. Кешът се ключеше само по run_id, а той е СТАБИЛЕН при повторен опит - затова съществува run_attempt - и записът в кеша е неизменим веднъж създаден. Тоест „Re-run failed jobs", което е естественият отговор на спиране по срок и което самият текст на PR-а подканваше, възстановява снимката от първия опит, краули часове наред и после се опитва да запише върху същия ключ: отказ, понижен до предупреждение, стъпката зелена. А новата проверка щеше да намери записа от първия опит и да удостовери загубата като успех. Ключът вече носи и run_attempt. Второ, спирането по срок се извеждаше от часовника, а не от това дали наистина е задържана работа. Комплект, чиято последна заявка се приземява секунда след бюджета, е изтеглен докрай и не е спрян - но се обявяваше за частичен и излизаше 1. В производство това уцелва точно пуска, който довършва корпуса. Възпроизведох го: notAttempted 0, incomplete false, изход 1. Сега pool() връща дали е задържал редове и това решава. Трето, гол флаг без стойност мълчаливо падаше на подразбирането - за срока това значи „без срок", тоест самата защита изключена от печатна грешка. Сега get() гърми. Важи и за --limit, --concurrency, --folders. Тестове: 37 (от 32), включително срок при 8 работника (единичният работник не можеше да види мутант, при който само един спазва спирането), точно попадение в границата, и пълен корпус, засякъл срока. Десет мутации, всичките убити - включително връщането на самия бъг от втората находка. Коментарът за запаса от 60 минути беше по-уверен от кода: часовникът тръгва вътре във fetch.mjs, а не при старта на задачата, и в същия час трябва да се вместят и всички стъпки надолу. Записано както си е.
`--concurrency` имаше долна, но не и горна граница: `--concurrency 500` беше законен начин да се поискат петстотин едновременни връзки към държавен регистър — от скрипта, чиито backoff, circuit breaker и пауза между заявките съществуват точно за да не се случва това. Таванът е 8 — толкова пуска работният поток и при толкова е мерен корпусът. Свалянето му остава флаг; вдигането е съзнателна промяна в кода, което е и смисълът. ADR-0012 се изравнява с практиката: записаното там „≤6" предхожда текущите 8 и не беше налагано никъде.
--concurrency имаше под, но нямаше покрив: `--concurrency 500` беше законен начин да поискаш от държавен регистър петстотин едновременни връзки - и то от скрипт, чиито отстъпление и прекъсвач съществуват точно за да не се случва това. Таванът е 8, изнесен като MAX_CONCURRENCY до BREAKER_TRIP. Свалянето остава свободно; вдигането е промяна в кода, което е и смисълът. Осем, а не шест, защото това е което заданието наистина пуска и при което е мерен корпусът. „≤6" в ADR-0012 описваше нещо, което никога не е било в сила - документът твърдеше ограничение, а нищо не ограничаваше. Записано както си е, вместо да се прави, че връщаме стара стойност. Един тест заковава константата към заданието, тъй че смъкване на тавана пада при тестовете, а не на четвъртия час от следващия краул. Четири мутации, всичките убити: премахнат таван, смъкнат на 7, вдигнат на 64, и `>` станало `>=`, което тихо би изключило самата осмица. Плюс три остарели коментара: употребата на fetch.mjs не споменаваше тавана; заданието казваше „re-crawling from 2017", а указателят обяви 2015 като най-ранния комплект; и заглавието на fetch.test.mjs обещаваше два капана при вече покрити пет.
И двата теста минаваха, но не можеха да се провалят. Първо: случаят с 8 работници започваше с вече изчерпан бюджет - list.xml изяжда 61 s от 60 s, преди pool() изобщо да е влязъл. Затова мутант, който пита shouldStop() веднъж при влизане и после изчерпва цялата папка, оцеляваше и 39-те теста. Новият случай мести часовника на 10 s на заявка, така че срокът пада в средата на набора с 19 от 24 реда още неподадени: 5 изтеглени при правилната реализация срещу 24 при мутанта. Второ: заемането на реда преди проверката на срока прави withheld false, когато остатъкът е точно колкото работниците. Тогава --allow-incomplete сваля спирането по срок до изход 0 - точно това, което този флаг никога не бива да прави. Стесняването на случая до един ред го убива. Реализацията не е пипана - и двете дупки са в тестовете.
Текстът описваше поведението отпреди поправката: твърдеше, че всяко
продължаване трябва да е нов dispatch и че re-run "поне се проваля
шумно". И двете вече не са верни.
Ключът е cacbg-raw-${run_id}-${run_attempt}. При "Re-run failed jobs"
GitHub увеличава run_attempt, тоест записът отива на ключ, който още не
съществува, и минава; а restore-keys: cacbg-raw- връща снапшота от
предишния опит, така че краулът продължава оттам. Re-run работи - точно
това решава run_attempt.
Само с run_id записът щеше да се откаже върху неизменим ключ, да се
свали до предупреждение, а стъпката за проверка да намери стария запис и
да удостовери загубата. Затова run_attempt е част от ключа, а не
диагностика.
Поправено на двете места: абзацът в ADR-0012 и коментарът над стъпката
за краул в related-persons-data.yml, който повтаряше същото указание.
Посочено от @ydimitrof в ревюто на PR #313.
a5c1bb3 to
fb5a535
Compare
…urrent upstream Periodic heartbeat sync — brings in upstream changes since 2026-08-16 (PR midt-bg#177 head ed0eb60 was last rebased then): midt-bg#313, midt-bg#314, midt-bg#323. The integration-test lane and OTEL store polyfills do not overlap with the upstream changes (related-persons, undici bump, cacbg crawl-deadline fix, TR rate-limit remeasurement), so a clean merge is expected. PR midt-bg#177's only / touch was the original inScope commit, which is independent of upstream deps.
Печат за пълнота до суровия корпус, за да не се публикува отрязан корпус като цял. #313 направи частичния корпус ОЧАКВАНО състояние (краулът спира сам на срока и запазва каквото има), а restore-keys: cacbg-raw- връща най-СКОРОШНИЯ запис, не най-пълния. extract.mjs изброяваше файловете, без да ги сверява с описа - тоест отрязан корпус даваше по-малка повърхност, без грешка никъде, а нито гейтът за монотонност (вижда нетен ръст), нито подът --min-links (само брои) го хващат. Как работи: - fetch.mjs пише .corpus-complete.json САМО когато корпусът се сверява с list.xml, и го чисти в началото на всяко обхождане, тоест всеки ненормален изход оставя корпуса неподпечатан. - extract.mjs отказва без печат (--allow-partial-corpus е изричният override). - workflow-ът се лекува сам: непечатан кеш пуска краула, който за ~2 минути го сверява срещу живия регистър и подпечатва - никаква зависимост от поредността на пусканията. Преминало пет кръга независимо adversarial ревю (Codex), които намериха и затвориха ~26 находки: подмножество, което печата цялото дърво; празен индекс; страница за поддръжка; неатомарно разархивиране; осакатен и многокоренов XML; свиване на списъка; и цяла серия пробиви в PII предпазителя на override пътищата (наследен GIT_DIR, вложени хранилища, pathspec магия, symlink пренасочване, локализиран git, подразбиращи се пътища). Тестове: 25 в новия corpus-sentinel.test.mjs, 332 в целия cacbg+tr пакет; 23 мутанта убити. Follow-up (не блокери): пряко броене на top-level XML елементите, структурна валидация на кеширания списък, bind-mount регресия за CI.
Какво се счупи
Пуск 31889519937 - пълен краул срещу stage. Падна на 300-минутния таван и загуби всичко:
В лога на стъпка 10:
Три неща се събраха:
node- машината накрая докладваTerminate orphan process: pid (2351).always()стъпката пускашеtarвърху същата папка.tarсе отказа.actions/cache/saveпонижава провала при запис до предупреждение и излиза 0. Стъпката светна зелена.Пет часа любезно теглене, нула запазени байта, а следващият пуск започва пак от празен кеш.
Колко голям е корпусът всъщност
ADR-0012 предполагаше ~135 000 декларации за 2017-2025. Указателят на регистъра обявява:
Разликата идва от
*yпреизданията в края на годината и от комплектите за съответствие. Тоест студен корпус не се побира в един пуск - това не беше известно, когато таванът беше избран.Поправката
fetch.mjsприема--deadline-minutes. Изтече ли срокът, краулът спира да раздава работа, изчаква текущите заявки да се приберат и се връща нормално. Дървото е спокойно,tarуспява, кешът пази изтегленото, а следващият пуск продължава оттам -fetch.mjsи без това прескача файловете, които са на диска.Студен корпус става за два пуска вместо за нула.
Срокът се проверява на три места, и трите с отделен тест:
mkdirиlist.xml- празна папка с кеширан списък би изглеждала на следващия пуск като посетен комплект;Спирането по срок е самостоятелна присъда
Не минава през
assessCompleteness, и това е нарочно. Спиране точно на граница между комплекти оставя всеки достигнат комплект напълно изтеглен, тоест аритметиката казва „пълен" при липсващи цели години. Гейтът щеше да пусне корпус без 2015-2019 и да излезе 0.--allow-incompleteсъщо нарочно не го омеква. Този ключ значи „видях този недостиг и го приемам"; будилник, звъннал по средата, не е видяна преценка. Който иска част от корпуса, я взима с--foldersи я приема съзнателно.В заданието
tarму остава цял час;lookup-only, нищо не се тегли). Стъпката за запис по конструкция не може да падне, тъй че зелено до нея не е доказателство - точно това ни подведе.Проверих, че
lookup-onlyиfail-on-cache-missнаистина съществуват вactions/cache/restoreна закования SHA, вместо да го предположа.Тестове
11 в
fetch-gate.test.mjs(4 нови), 21 вfetch.test.mjs(4 нови). Часовникът се подава отвън и се движи от подставения четец на заявки - по едно тиктакане на заявка - тъй че „кога" пада срокът се изразява в заявки, а не в реално време.Шест мутации, всичките убити:
deadlineHit--allow-incompleteпри срокshouldStop--allow-incompleteомеква срока--allow-incompleteпри срокmkdirИзвън обхвата
audit.test.mjsиextract-companies.test.mjsпадат и наmain, и без тази промяна -packages/shared/src/company-name-key.tsвнася./formatбез разширение. Не го пипам тук.Втори кръг: поправки по независим преглед
Пуснах промяната на независим преглед. Той намери две неща, които първият вариант не хващаше. И двете са потвърдени с пускане, не по разсъждение.
1. Кешът се самоизяждаше при повторен опит
Ключът беше само
run_id.run_idне се сменя при повторен опит - точно за това съществуваrun_attempt. Проверено емпирично: три пускания в това репо иматattempt 2при същияrun_id.Записите в Actions кеша са неизменни. Оттам:
Поправката работеше при ново пускане и се проваляше мълчаливо при повторен опит - разлика между два почти еднакви бутона, никъде неотбелязана. По-лошото: новата проверка щеше да прикрие точно загубата, заради която я добавих.
Ключът вече носи
run_id-run_attemptи на трите места. При повторен опит точният ключ не улучва, префиксътcacbg-raw-връща снимката от предишния опит, а записът отива на нов ключ.2. Пълен корпус се обявяваше за частичен
Спирането се извеждаше от часовника, а не от това дали наистина е задържана работа. Комплект, чиято последна заявка се приземява секунда след бюджета, е изтеглен докрай - нищо не е задържано. Възпроизведено:
В производство това уцелва точно пуска, който довършва корпуса: изход 1, зареждането прескочено, четири часа червено за успяла работа.
pool()вече връща дали е задържал редове, и това решава. Същият опит сега дава изход 0.3. Гол флаг без стойност
--deadline-minutesбез стойност падаше на подразбирането, тоест „без срок" - защитата изключена от печатна грешка, мълчаливо.get()вече гърми. Важи и за--limit,--concurrency,--folders, където беше същото.Тестовете бяха слаби на две места
Всички тестове за срока вървяха с един работник, а производството - с осем. Мутант, при който само първият работник спазва спирането, оцеляваше: в производство седем от осем щяха да пишат точно в прозореца, запазен за
tar.Тестът за пропуснат комплект минаваше по грешна причина - изход 1 идваше от заварения гейт за пълнота, не от срока. Сега върви с
--allow-incomplete, тъй че само срокът може да даде този изход.37 теста (от 32). Нови: пълен корпус, засякъл срока; срок при 8 работника; точно попадение в границата; голи флагове.
Десет мутации, всичките убити:
deadlineHitwithheldслед басейна--allow-incompleteпри срокshouldStop--allow-incompleteомеква срока>=става>--allow-incompleteпри срокКоментарът за запаса беше по-уверен от кода
Часовникът тръгва вътре във
fetch.mjs, а не при старта на задачата - значи подготовката излиза от 300-те преди минута нула. Спирането може да надскочи с една верига опити. И на пуск, който не удари срока, в същия час трябва да се вместят извличането, износът от D1, двайсетминутният Търговски регистър, разрешаването, одитът, миграциите и изпращането. Записано както си е, с изричното „ако стъпките надолу пораснат, сваляй срока, не вдигай тавана".Какво съзнателно не влезе
PR-ът прави частичните кешове нормално състояние, а нищо надолу по веригата не различава отрязан корпус от пълен. Заварено свойство, което този PR изостря, но не внася - и одитът за необратимост хваща голямото срутване. Отделно issue, не тук.