fix(etl): прекъснат derive да не се бърка с първо пускане - #337
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.
Много добре обмислена промяна. Тя решава реален проблем с интегритета на данните: прекъснат derive (изтрито raw_contracts + NULL-нат data_freshness.as_of) вече не се бърка с първо пускане и не води до пълно предеривиране върху жива, попълнена повърхност. Резолюцията на плана е преместена ПРЕДИ каквото и да е докосване на work DB, така че отказът вече не унищожава съществуваща база — това е коректно и добре покрито с тестове. servedCorpusRows() умишлено е COUNT, а не дата, и скоупнат по namespace-ите на id — правилно решение, обяснено ясно. Тестовете са изчерпателни и добре мотивирани.
Нямам блокиращи забележки. Оставям една неблокираща препоръка за втвърдяване на проверката в --work-db пътя (allowlist вместо denylist), защото там евентуална неразпозната стойност на --derive е единственият остатъчен път към тиха загуба на данни.
Не открих враждебен код, инжекционни вектори или изтичане на тайни. SQL-ите ползват литерални LIKE шаблони без интерполация на вход.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах #337 при HEAD bfcf8531 — трасирано срещу actual code.
Коректно (потвърдено): Разграничението interrupted-derive vs first-run е издържано. resolveCatchupPlan ползва servedCorpusRows() само като yes/no, не като watermark (raw_contracts е torn-down след load, data_freshness.as_of е NULL по време на refresh-slice — двата witness-а са transient), затова refuse-ва вместо да гадае. Веригата fail-closed държи: refuse → drop transient staging; assertDeriveWindowSafe (line 527, преди всеки destructive derive) probe-ва EXISTS per full-clear таблица от @full-clear в normalize-raw.sql и fail-closed при непрочетен corpus.
Minor — bare --from без стойност заобикаля собствения refuse: arg('from') за --from без = връща true (helper, line 57–60), затова !arg('from') е false и refuse-ът на line 248 се прескача, независимо от servedCorpusRows(). Надолу String(arg('from') || DEFAULT_FROM) дава from="true". Това не е загуба на данни — validateDay вътре в assertDeriveWindowSafe гърми с windowFrom must be YYYY-MM-DD преди derive-а → hard fail-closed. Но операторът получава cryptic грешка вместо насочващото съобщение на самия guard. Fix: третирай arg('from') === true като липсващ (!arg('from') || arg('from') === true), за да стигне до refuse-а с правилното съобщение.
Иначе — clean. Triage роля → COMMENT (не approve); вердикт: коректно + fail-safe.
bfcf853 to
7609636
Compare
|
Благодаря на двамата — и двете препоръки са приложени в @ydimitrof — allowlist вместо denylist. Проверих и опасението се потвърждава: @lyubomir-bozhinov — гол Два нови теста, по един на всяка находка; и двата падат при връщане на старото поведение (проверено с мутация). Целият пакет |
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Fix-ът е коректен — потвърдено при HEAD 76096362. Bare --from без стойност вече не заобикаля refuse-а: rawFrom = arg('from') → explicitFrom = typeof rawFrom === 'string' ? rawFrom : null (import.mjs:253), тъй че boolean-ът от bare --from става null; !explicitFrom && servedCorpusRows() > 0 (:254) гърми правилно, а String(explicitFrom || DEFAULT_FROM) (:269) пада на DEFAULT_FROM вместо на „true". Точно поправката от бележката, при това по-стегнато (всяко не-string → null). Нямам блокери.
7609636 to
2deac18
Compare
latestLoadedDate се допитва до два свидетеля и двата са преходни по устройство. raw_contracts е временният staging, който се събаря след всяко зареждане. А refresh-slice.sql зачерква data_freshness.as_of в ПЪРВИЯ си батч (setup - за да не твърди полуопреснена повърхност свежест) и го възстановява чак в globals, осемнайсет батча по-късно; всеки батч е отделна атомарна група, тъй че всеки провал между тях оставя as_of празен. Тогава и двата мълчат едновременно - и това е неразличимо от студена база, макар последствията да са различни. Студено пускане наистина иска целия поток. Прекъснат derive върху пълна повърхност обаче преизгражда от DEFAULT_FROM: 2020 - днес, започвайки с DELETE FROM contracts. Не това е поискал прекъснатият бяг. Сервираният корпус различава двата случая - използван САМО като да/не, никога като дата. Нарочно не става трети воден знак: contracts носи дати на публикуване, не дните на кофите, от които се смята прозорецът (сервираната схема изобщо няма колона source), а дата на публикуване, изпреварваща своята кофа, би преместила прозореца ОТВЪД кофи, които никога не са зареждани - тихо прескачане, което е по-лошо от всяко преизграждане. Затова: има редове, няма воден знак - отказваме и казваме защо. Отказът сочи възстановяване, което наистина работи. Клонът без воден знак заковаваше derive: 'full', тъй че препоръчаното --from печаташе редовен план, а живият бяг после се отказваше от него: тесен прозорец с пълен derive е точно комбинацията, която assertDeriveWindowSafe брани. Сега клонът уважава изричен --derive (по подразбиране пак full, защото неговият подразбиращ прозорец тръгва от началото на потока). А пътят с --work-db печата plan.derive, без да му се подчинява: строи нова work база от прозореца и я изпраща НАЕДРО, тоест се държи като пълен derive каквото и да казва планът. За прозорец от началото на потока това е безобидно; за опашката, която отказът препоръчва, би заменило сервирания корпус с тази опашка. Там slice вече се отказва - и планът се смята ПРЕДИ пътят да изтрие заварената work база и да приложи миграциите, защото "няма да продължа" след като щетата е нанесена не е отказ. 13 теста: отказ върху пълна повърхност със съобщение, което насочва; сондата е COUNT, не MAX, и само като последна инстанция; студена база пак планира пълен backfill; --from е аварийният изход; препоръчаното възстановяване е изпълнимо; съобщението назовава точно уважаваните флагове; --work-db отказва slice преди зареждане и без да трие заварената база, а пълен derive минава. Фалшивият node вече логва argv, за да е проверката "преди зареждането" истинска, а не куха. По ревютата: Пазачът на --work-db става allowlist (!== 'full'), не denylist (ydimitrof). validateDeriveMode се вика само по живия път - runWorkBackfill никога не стига до него - тъй че неразпозната стойност на --derive подминаваше проверка само за 'slice' и влизаше право в изпращането наедро. Когато провалът е тиха замяна на сервирания корпус, единственото безопасно подразбиране е "всичко, което не разпознавам положително". Гол --from (без стойност) се брои за липсващ (lyubomir-bozhinov). arg() връща true за флаг без стойност, тъй че отказът се прескачаше и операторът получаваше по-късното "windowFrom must be YYYY-MM-DD" вместо съобщението, което обяснява прекъснатия derive. Затворен провал и в двата случая, но само единият казва какво да направиш. 15 теста (2 нови). Целият пакет scripts/: 138 зелени.
2deac18 to
80dd367
Compare
Планировчикът на
--catchupсе допитва до два свидетеля за докъде е стигнало зареждането, и двата са преходни по устройство:raw_contractsе временният staging, който се събаря след всяко зареждане;refresh-slice.sqlзачеркваdata_freshness.as_ofв първия си батч (setup, за да не твърди полуопреснена повърхност свежест) и го възстановява чак вglobals- осемнайсет батча по-късно. Всеки батч е отделна атомарна група, тъй че всеки провал между тях оставяas_ofпразен.Тогава и двата мълчат едновременно, и това е неразличимо от студена база - макар последствията да са различни. Студено пускане наистина иска целия поток. Прекъснат derive върху пълна повърхност обаче преизгражда от
DEFAULT_FROM: 2020 - днес, започвайки сDELETE FROM contracts. Не това е поискал прекъснатият бяг. Наблюдавано локално: прекъснатamendmentsбатч направи следващия catchup пълно преизграждане, което после гръмна на FOREIGN KEY.Какво прави
contractsноси дати на публикуване, не дните на кофите, от които се смята прозорецът (сервираната схема изобщо няма колонаsource), а дата на публикуване, изпреварваща своята кофа, би преместила прозореца отвъд кофи, които никога не са зареждани - тихо прескачане, което е по-лошо от всяко преизграждане. Затова: има редове, няма воден знак → отказваме и казваме защо.derive: full, тъй че препоръчаното--fromпечаташе редовен план, а живият бяг после се отказваше от него (тесен прозорец + пълен derive е точно комбинацията, коятоassertDeriveWindowSafeбрани). Сега клонът уважава изричен--derive; по подразбиране пакfull, защото неговият подразбиращ прозорец тръгва от началото на потока.--work-dbпечатаplan.derive, без да му се подчинява: строи нова work база от прозореца и я изпраща наедро. За опашката, която отказът препоръчва, това би заменило сервирания корпус с тази опашка. Тамsliceвече се отказва - и планът се смята преди пътят да изтрие заварената work база и да приложи миграциите, защото „няма да продължа" след нанесена щета не е отказ.Студената база пак планира пълен backfill - това е заковано с тест.
Тестове
13 зелени (10 нови): отказ върху пълна повърхност със съобщение, което насочва; сондата е
COUNT, неMAX, и само като последна инстанция; студена база пак планира пълен backfill;--fromе аварийният изход; препоръчаното възстановяване е изпълнимо; съобщението назовава точно уважаваните флагове;--work-dbотказва slice преди зареждане и без да трие заварената база, а пълен derive минава.Фалшивият
nodeв харнеса вече логва argv - иначе проверката „отказва преди зареждането" беше куха и не можеше да се провали.Само за свързаните лица и общи подобрения - без нови функционалности.