lekss361/gendesign

Fork 0

fix(#234): NSPD harvest ETA badge + SETNX dedupe (Sub-PR C) #250

Merged

lekss361 merged 3 commits from fix/234-nspd-harvest-eta-badge into main

2026-05-17 06:35:41 +00:00

lekss361 commented

2026-05-17 06:13:57 +00:00

Owner

Summary

UX mitigation для issue #234 (бесконечный спиннер «НСПД загрузка» при пустом nspd_quarter_dumps). Foundational fix #126/#231 идёт в Sub-PR B; этот PR — visible UX patch + Redis SETNX дедупликация чтобы burst N concurrent analyze не плодил N одинаковых harvest task'ов.

Backend (`backend/app/services/site_finder/quarter_dump_lookup.py`)

_acquire_harvest_lock(quarter) — Redis SETNX TTL=120s на ключ nspd_harvest_lock:{quarter}. Реализует TODO из original _trigger_harvest docstring (line 1170).
_trigger_harvest — получает lock перед harvest_quarter.apply_async(), иначе возвращает False (другой запрос триггернул раньше → видим existing harvest_triggered=True от первого).
make_empty_result / EMPTY_DUMP_RESULT — новое поле harvest_eta_seconds: int | None в nspd_dump. Const 60s когда harvest_triggered=True, иначе None.
/analyze response — поле пробрасывается автоматически (response_model отсутствует для analyze endpoint, dict уходит как есть). Schema changes не нужны.

Frontend

frontend/src/types/nspd.ts — добавлено harvest_eta_seconds?: number | null в NspdDumpMeta.
NspdFreshnessBadge.tsx — countdown «НСПД: загрузка ~Nс» через setInterval(1000) instead of бесконечный «НСПД: загрузка…».
NspdZoningBlock.tsx — countdown в loading skeleton; после remaining=0 показывает «загрузка дольше обычного, попробуйте обновить через минуту» + ссылку на ПКК (graceful fallthrough вместо infinite spinner).

Tests

5 новых unit-test в tests/services/test_quarter_dump_lookup.py:
- test_empty_dump_result_has_harvest_eta_seconds_key
- test_make_empty_result_passes_harvest_eta_seconds
- test_make_empty_result_default_eta_is_none
- test_acquire_harvest_lock_succeeds_first_call — SETNX dedupe (monkeypatched redis)
- test_acquire_harvest_lock_redis_unavailable_returns_false — graceful degrade
cd backend && uv run pytest tests/services/test_quarter_dump_lookup.py -k "harvest_eta or harvest_lock or empty_result" → 7 passed

Lint

✅ ruff check + ruff format --check clean
✅ TypeScript: npx tsc --noEmit — my files clean (только unrelated @sentry/nextjs env warnings)
✅ pre-commit hooks pass

Plan context

Sub-PR A — #126 grid-walk foundation (PR #247, open)
Sub-PR C (this PR) — #234 UX mitigation, independent of A/B
Sub-PR B — #231 switch search_by_quarter (depends A merge)
Sub-PR D — #231 re-enable beat + back-fill (depends B)

Notes

Backward compat: harvest_eta_seconds optional на FE (?: number | null). Старые backend без поля → FE падает в null ветку (no countdown, как сейчас).
Redis unavailable scenario: _acquire_harvest_lock возвращает False graceful → caller не триггерит harvest (лучше пропустить, чем нагрузить WAF при transient redis-fail).
Tests existing pre-existing 9 failures в backend/tests/test_quarter_dump_lookup.py (root-level, schema drift) — не мои, verified git stash баселайн.

Closes #234

## Summary UX mitigation для issue #234 (бесконечный спиннер «НСПД загрузка» при пустом `nspd_quarter_dumps`). Foundational fix #126/#231 идёт в Sub-PR B; этот PR — visible UX patch + Redis SETNX дедупликация чтобы burst N concurrent analyze не плодил N одинаковых harvest task'ов. ### Backend (`backend/app/services/site_finder/quarter_dump_lookup.py`) - **`_acquire_harvest_lock(quarter)`** — Redis SETNX TTL=120s на ключ `nspd_harvest_lock:{quarter}`. Реализует TODO из original `_trigger_harvest` docstring (line 1170). - **`_trigger_harvest`** — получает lock перед `harvest_quarter.apply_async()`, иначе возвращает `False` (другой запрос триггернул раньше → видим existing `harvest_triggered=True` от первого). - **`make_empty_result` / `EMPTY_DUMP_RESULT`** — новое поле `harvest_eta_seconds: int | None` в `nspd_dump`. Const 60s когда `harvest_triggered=True`, иначе `None`. - **`/analyze` response** — поле пробрасывается автоматически (response_model отсутствует для analyze endpoint, dict уходит как есть). Schema changes не нужны. ### Frontend - **`frontend/src/types/nspd.ts`** — добавлено `harvest_eta_seconds?: number | null` в `NspdDumpMeta`. - **`NspdFreshnessBadge.tsx`** — countdown «НСПД: загрузка ~Nс» через `setInterval(1000)` instead of бесконечный «НСПД: загрузка…». - **`NspdZoningBlock.tsx`** — countdown в loading skeleton; после `remaining=0` показывает «загрузка дольше обычного, попробуйте обновить через минуту» + ссылку на ПКК (graceful fallthrough вместо infinite spinner). ### Tests - 5 новых unit-test в `tests/services/test_quarter_dump_lookup.py`: - `test_empty_dump_result_has_harvest_eta_seconds_key` - `test_make_empty_result_passes_harvest_eta_seconds` - `test_make_empty_result_default_eta_is_none` - `test_acquire_harvest_lock_succeeds_first_call` — SETNX dedupe (monkeypatched redis) - `test_acquire_harvest_lock_redis_unavailable_returns_false` — graceful degrade - `cd backend && uv run pytest tests/services/test_quarter_dump_lookup.py -k "harvest_eta or harvest_lock or empty_result"` → 7 passed ### Lint - ✅ `ruff check` + `ruff format --check` clean - ✅ TypeScript: `npx tsc --noEmit` — my files clean (только unrelated `@sentry/nextjs` env warnings) - ✅ pre-commit hooks pass ### Plan context - Sub-PR A — #126 grid-walk foundation (PR #247, open) - **Sub-PR C (this PR)** — #234 UX mitigation, independent of A/B - Sub-PR B — #231 switch search_by_quarter (depends A merge) - Sub-PR D — #231 re-enable beat + back-fill (depends B) ### Notes - Backward compat: `harvest_eta_seconds` optional на FE (`?: number | null`). Старые backend без поля → FE падает в `null` ветку (no countdown, как сейчас). - Redis unavailable scenario: `_acquire_harvest_lock` возвращает `False` graceful → caller не триггерит harvest (лучше пропустить, чем нагрузить WAF при transient redis-fail). - Tests existing pre-existing 9 failures в `backend/tests/test_quarter_dump_lookup.py` (root-level, schema drift) — **не мои**, verified `git stash` баселайн. Closes #234

lekss361 added 1 commit 2026-05-17 06:13:58 +00:00

fix(#234 ): NSPD harvest ETA badge + SETNX dedupe (Sub-PR C) 990c109756

Backend (quarter_dump_lookup.py):
- _acquire_harvest_lock: Redis SETNX TTL=120s на quarter, защищает от burst
  N concurrent analyze, ставящих N одинаковых harvest task в очередь
- _trigger_harvest: использует lock перед apply_async, возвращает False если
  lock уже взят (другой запрос триггернул раньше)
- make_empty_result/EMPTY_DUMP_RESULT: новое поле harvest_eta_seconds в
  nspd_dump dict, типичный harvest_quarter = 60с
- /analyze: пробрасывает поле через nspd_dump dict (нет typed schema —
  response_model=None для /analyze endpoint, dict уходит как есть)

Frontend (NspdFreshnessBadge, NspdZoningBlock):
- Countdown «НСПД: загрузка ~Nс» вместо бесконечного спиннера
- После остановки countdown (remaining=0) NspdZoningBlock показывает
  «загрузка дольше обычного» + ссылку на ПКК вместо infinite skeleton

Tests: 5 новых unit + 2 для empty_result schema (всего +7, pass)

Closes #234 (UX-side; data-side resolves когда Sub-PR B + D merged).

lekss361 added 1 commit 2026-05-17 06:19:23 +00:00

Merge remote-tracking branch 'forgejo/main' into fix/234-nspd-harvest-eta-badge b99ac76463

# Conflicts:
#	backend/app/services/site_finder/quarter_dump_lookup.py

lekss361 commented

2026-05-17 06:19:51 +00:00

Author

Owner

Deep Code Review — verdict

Status: 🟠 HIGH — НЕ мержить (mergeable=false + semantic bug)
Files: 5 (P1: backend service + tests, P2: 3 FE files)
Lines: +223 / -11

🚫 Merge blocker (infrastructure)

mergeable=false — конфликт с main 10b79da2 (PR #248 уже изменил quarter_dump_lookup.py). Нужен rebase ветки fix/234-nspd-harvest-eta-badge на текущий main, затем force-push. Скорее всего конфликт в области if row is None: (PR #248) vs _trigger_harvest / make_empty_result (этот PR).

🟠 High — семантический баг: Request B теряет countdown

quarter_dump_lookup.py:164-169 + _trigger_harvest:

harvest_triggered = _trigger_harvest(quarter)  # Request B → False (lock уже взят)
return make_empty_result(
    harvest_triggered=harvest_triggered,
    harvest_eta_seconds=_HARVEST_ETA_SECONDS if harvest_triggered else None,
)

Сценарий:

Request A: lock acquired → harvest_triggered=True → ETA=60 → FE countdown ✅
Request B (через 0.5с на тот же quarter): lock NOT acquired → harvest_triggered=False → harvest_eta_seconds=None → FE НЕ показывает countdown → пользователь B видит infinite spinner (тот самый bug #234).

PR description обещает "видят в response harvest_eta_seconds от первого запроса", но код делает обратное. Семантически нужно: lock-failed callers тоже должны получать harvest_triggered=True + ETA (т.к. harvest уже запущен другим запросом).

Предлагаемый фикс (один из):

_trigger_harvest возвращает True если lock fails (потому что кто-то реально триггернул):

if not _acquire_harvest_lock(quarter):
    logger.info("quarter=%s harvest skipped (lock held)", quarter)
    return True  # ← кто-то другой уже запустил, для FE это same UX

Минус: семантика True смазана (этот call не запускал task).

Разделить return на (triggered, eta):

def _trigger_harvest(quarter) -> tuple[bool, int | None]:
    if not _acquire_harvest_lock(quarter):
        return False, _HARVEST_ETA_SECONDS  # not by us, but ETA still valid
    # ... apply_async ...
    return True, _HARVEST_ETA_SECONDS

Чище семантически.

Возвращать redis.ttl(key) как ETA для Request B — honest countdown:
```
ttl = client.ttl(f"nspd_harvest_lock:{quarter}")
return False, max(1, ttl - 60) if ttl > 0 else None
```
Лучше всего UX-wise (countdown реально совпадает с реальностью).

🟡 Medium — convention drift на `Settings()`

quarter_dump_lookup.py:1208-1210:

from app.core.config import Settings
client = redis.Redis.from_url(Settings().redis_url)

Весь codebase использует singleton settings:

scrape_kn.py:15,96: from app.core.config import settings → redis.Redis.from_url(settings.redis_url)
objective.py:35,98 — то же
celery_app.py:17,48 — то же
config.py:134: settings = Settings() (модульный singleton)

Settings() re-evaluates env каждый вызов. Заменить на from app.core.config import settings для консистентности.

🟡 Minor — race test gap

test_acquire_harvest_lock_succeeds_first_call валидирует SETNX механику, но не покрывает Request B UX semantics (что caller получит при _trigger_harvest=False). Это именно то место где сейчас баг (см. 🟠 выше). Добавить test:

def test_get_quarter_dump_data_request_b_sees_eta_when_lock_held(monkeypatch):
    """Request B (lock уже взят) должен видеть harvest_eta_seconds != None."""
    # mock: row=None, lock fails → assert result["nspd_dump"]["harvest_eta_seconds"] is not None

🟢 Low / nits

_HARVEST_LOCK_TTL_SECONDS=120 vs _HARVEST_ETA_SECONDS=60 — TTL=2×ETA как safety buffer, ОК.
Lock не освобождается на success path (висит до TTL). Acceptable для V1; можно harvest_quarter task в on_success сделать redis.delete(lock_key) для faster recovery, но не блокирует.
NspdZoningBlock.tsx:35-36: harvestTimedOut triggers только если eta !== null (т.к. remaining === 0 от decrement). Если backend не пришлёт ETA (Request B сценарий) — fallback к старому spinner forever. Дополнительный довод за фикс 🟠.

✅ Positive

Atomic SET ... NX EX — корректный SETNX без race window между set+expire ✅
redis.Redis.from_url pattern matches codebase ✅
clearInterval cleanup в обоих badge'ах ✅
encodeURIComponent(cadNum) на ПКК link — XSS safe ✅
rel="noopener noreferrer" на target=_blank ✅
harvest_eta_seconds?: number | null — корректный backward-compat shape ✅
Graceful Redis-down degradation return False + log warning ✅

Cross-file impact

NspdDumpMeta consumers: только NspdFreshnessBadge.tsx, NspdZoningBlock.tsx, site-finder.ts (через parcels response) — все backward compat OK (optional field).
/analyze endpoint: response_model=None, dict-spread → новое поле проходит автоматически ✅.
PR #248 (cad_zouit fallback no-dump) — пересекается на том же файле в районе if row is None: ветки. После rebase проверь что harvest_eta_seconds интегрировано с fallback логикой PR #248.

Action items

Rebase ветку на 10b79da2, resolve conflict с #248.
Fix 🟠: Request B должен возвращать harvest_eta_seconds != None. Рекомендую вариант 3 (honest TTL).
Add test на Request B UX (см. 🟡 #2).
Switch Settings() → settings singleton.
После — переоткрыть review.

PR не мерджу (mergeable=false + 🟠 HIGH). После исправлений → re-review.

## Deep Code Review — verdict **Status:** 🟠 HIGH — НЕ мержить (mergeable=false + semantic bug) **Files:** 5 (P1: backend service + tests, P2: 3 FE files) **Lines:** +223 / -11 ### 🚫 Merge blocker (infrastructure) `mergeable=false` — конфликт с main `10b79da2` (PR #248 уже изменил `quarter_dump_lookup.py`). Нужен rebase ветки `fix/234-nspd-harvest-eta-badge` на текущий main, затем force-push. Скорее всего конфликт в области `if row is None:` (PR #248) vs `_trigger_harvest` / `make_empty_result` (этот PR). ### 🟠 High — семантический баг: Request B теряет countdown `quarter_dump_lookup.py:164-169` + `_trigger_harvest`: ```python harvest_triggered = _trigger_harvest(quarter) # Request B → False (lock уже взят) return make_empty_result( harvest_triggered=harvest_triggered, harvest_eta_seconds=_HARVEST_ETA_SECONDS if harvest_triggered else None, ) ``` **Сценарий:** - Request A: lock acquired → `harvest_triggered=True` → ETA=60 → FE countdown ✅ - Request B (через 0.5с на тот же quarter): lock NOT acquired → `harvest_triggered=False` → **`harvest_eta_seconds=None`** → FE НЕ показывает countdown → пользователь B видит infinite spinner (тот самый bug #234). PR description обещает "видят в response harvest_eta_seconds от первого запроса", но код делает обратное. Семантически нужно: lock-failed callers тоже должны получать `harvest_triggered=True` + ETA (т.к. harvest **уже запущен** другим запросом). **Предлагаемый фикс** (один из): 1. `_trigger_harvest` возвращает `True` если lock fails (потому что *кто-то* реально триггернул): ```python if not _acquire_harvest_lock(quarter): logger.info("quarter=%s harvest skipped (lock held)", quarter) return True # ← кто-то другой уже запустил, для FE это same UX ``` Минус: семантика `True` смазана (этот call не запускал task). 2. Разделить return на `(triggered, eta)`: ```python def _trigger_harvest(quarter) -> tuple[bool, int | None]: if not _acquire_harvest_lock(quarter): return False, _HARVEST_ETA_SECONDS # not by us, but ETA still valid # ... apply_async ... return True, _HARVEST_ETA_SECONDS ``` Чище семантически. 3. Возвращать `redis.ttl(key)` как ETA для Request B — honest countdown: ```python ttl = client.ttl(f"nspd_harvest_lock:{quarter}") return False, max(1, ttl - 60) if ttl > 0 else None ``` Лучше всего UX-wise (countdown реально совпадает с реальностью). ### 🟡 Medium — convention drift на `Settings()` `quarter_dump_lookup.py:1208-1210`: ```python from app.core.config import Settings client = redis.Redis.from_url(Settings().redis_url) ``` Весь codebase использует singleton `settings`: - `scrape_kn.py:15,96`: `from app.core.config import settings` → `redis.Redis.from_url(settings.redis_url)` - `objective.py:35,98` — то же - `celery_app.py:17,48` — то же - `config.py:134`: `settings = Settings()` (модульный singleton) `Settings()` re-evaluates env каждый вызов. Заменить на `from app.core.config import settings` для консистентности. ### 🟡 Minor — race test gap `test_acquire_harvest_lock_succeeds_first_call` валидирует SETNX механику, но **не покрывает Request B UX semantics** (что caller получит при `_trigger_harvest=False`). Это именно то место где сейчас баг (см. 🟠 выше). Добавить test: ```python def test_get_quarter_dump_data_request_b_sees_eta_when_lock_held(monkeypatch): """Request B (lock уже взят) должен видеть harvest_eta_seconds != None.""" # mock: row=None, lock fails → assert result["nspd_dump"]["harvest_eta_seconds"] is not None ``` ### 🟢 Low / nits - `_HARVEST_LOCK_TTL_SECONDS=120` vs `_HARVEST_ETA_SECONDS=60` — TTL=2×ETA как safety buffer, ОК. - Lock не освобождается на success path (висит до TTL). Acceptable для V1; можно `harvest_quarter` task в `on_success` сделать `redis.delete(lock_key)` для faster recovery, но не блокирует. - `NspdZoningBlock.tsx:35-36`: `harvestTimedOut` triggers только если `eta !== null` (т.к. `remaining === 0` от decrement). Если backend не пришлёт ETA (Request B сценарий) — fallback к старому spinner forever. Дополнительный довод за фикс 🟠. ### ✅ Positive - Atomic `SET ... NX EX` — корректный SETNX без race window между set+expire ✅ - `redis.Redis.from_url` pattern matches codebase ✅ - `clearInterval` cleanup в обоих badge'ах ✅ - `encodeURIComponent(cadNum)` на ПКК link — XSS safe ✅ - `rel="noopener noreferrer"` на target=_blank ✅ - `harvest_eta_seconds?: number | null` — корректный backward-compat shape ✅ - Graceful Redis-down degradation `return False` + log warning ✅ ### Cross-file impact - `NspdDumpMeta` consumers: только `NspdFreshnessBadge.tsx`, `NspdZoningBlock.tsx`, `site-finder.ts` (через `parcels` response) — все backward compat OK (optional field). - `/analyze` endpoint: `response_model=None`, dict-spread → новое поле проходит автоматически ✅. - PR #248 (cad_zouit fallback no-dump) — пересекается на том же файле в районе `if row is None:` ветки. После rebase проверь что `harvest_eta_seconds` интегрировано с fallback логикой PR #248. ### Action items 1. **Rebase** ветку на `10b79da2`, resolve conflict с #248. 2. **Fix 🟠**: Request B должен возвращать `harvest_eta_seconds != None`. Рекомендую вариант 3 (honest TTL). 3. **Add test** на Request B UX (см. 🟡 #2). 4. **Switch** `Settings()` → `settings` singleton. 5. После — переоткрыть review. PR **не мерджу** (mergeable=false + 🟠 HIGH). После исправлений → re-review.

lekss361 commented

2026-05-17 06:28:46 +00:00

Author

Owner

Re-review verdict: 🟡 MEDIUM (was 🟠 HIGH)

Прогресс есть, но не все issues из первого ревью закрыты. Близко к merge — функциональное поведение Request B приемлемое (graceful FE fallback), остались два non-blocking nits.

Status by issue

#	Issue из первого ревью	Status
1	`mergeable=false` (conflict с PR #248)	✅ FIXED — rebase merge commit (`b99ac76`), `_get_cad_zouit_overlaps` fallback в `if row is None` сохранён
2	Request B semantics	✅ Functionally OK — но docstring расходится с фактическим поведением (см. ниже)
3	`Settings()` convention drift	❌ NOT fixed — `quarter_dump_lookup.py:1204-1206` всё ещё `Settings()` instance вместо singleton
4	Race test gap для Request B	⚠️ Partial — 2 SETNX теста добавлены (`test_acquire_harvest_lock_*`), но E2E integration test для Request B response shape отсутствует

Request B — фактический поток

Lock denied flow (трассировка по коду):

_trigger_harvest → _acquire_harvest_lock → False → return False
harvest_triggered = False → harvest_eta_seconds = None (lines 172, 202)
FE: isHarvesting = harvest_triggered && !available = false → пропускает skeleton spinner
FE: попадает в if (!data) branch → показывает «Данные ПЗЗ из НСПД недоступны» + кнопка ПКК

Request B видит graceful no-data UI с ПКК fallback, не бесконечный спиннер. UX приемлемая — это валидный design choice (одиночный спиннер пользователю с denied lock не критичен, ПКК альтернатива доступна).

Vault entry fixes/Bug_NSPD_WMS_NotBulk_2026_May14.md явно описывает это поведение как намеренное: «harvest_triggered=False (lock уже held) → harvest_eta_seconds=None».

🟡 Medium issue: docstring расходится с кодом

_trigger_harvest docstring (line 1228) утверждает:

«остальные получают False (т.е. harvest_triggered=False) но видят в response harvest_eta_seconds от первого запроса»

Код этого не делает — для Request B eta=None (см. lines 172, 202). Docstring надо синхронизировать с реальным поведением:

# Replace:
получают `False` (т.е. harvest_triggered=False) но видят в response
`harvest_eta_seconds` от первого запроса и не re-poll сразу.

# With:
получают `False` (т.е. harvest_triggered=False, harvest_eta_seconds=None).
FE при таком ответе показывает graceful no-data UI с ПКК fallback link
(не infinite spinner). Trade-off: Request B не видит countdown, но и
не сбивает работу Request A.

🟡 Medium issue: Settings() drift (issue #3 не закрыт)

quarter_dump_lookup.py:1204-1206:

from app.core.config import Settings
client = redis.Redis.from_url(Settings().redis_url)

Должно быть:

from app.core.config import settings
client = redis.Redis.from_url(settings.redis_url)

Singleton settings = Settings() уже создан в app/core/config.py:134 — на каждом hot-path вызове _acquire_harvest_lock мы инстанцируем новый Pydantic BaseSettings объект, который перечитывает env vars. Не критично, но drift от convention остального codebase.

Что сделано хорошо

✅ SETNX nx=True, ex=120 атомарно — корректный pattern
✅ Lock TTL=120 > harvest ETA 60 — lock жив дольше task'а
✅ Graceful redis-fail (except Exception → return False) — не падаем при transient
✅ Countdown реактивный (useEffect deps правильные, clearInterval в cleanup)
✅ harvestTimedOut branch — после remaining<=0 показывает actionable fallback с ПКК
✅ Lazy import redis внутри try — не ломает unit-тесты без redis
✅ Anti-regression: PR #248 fallback _get_cad_zouit_overlaps сохранён после rebase
✅ Type safety: harvest_eta_seconds?: number | null на FE — backward compat ok

Recommended follow-up

Один маленький commit:

Fix docstring _trigger_harvest (1 параграф)
Replace Settings() → singleton settings (3 строки)
Optional: integration test test_request_b_returns_no_data_with_pkk_fallback для FE behavior

После этого — merge через merge strategy. Production monitoring при merge: счётчик harvest lock contention (logger.info на line 1232) → если burst >>1/sec, для Request B стоит вернуть ETA через redis.ttl().

Не мержу, жду follow-up на 2 пункта выше.

## Re-review verdict: 🟡 MEDIUM (was 🟠 HIGH) Прогресс есть, но не все issues из первого ревью закрыты. Близко к merge — функциональное поведение Request B приемлемое (graceful FE fallback), остались два non-blocking nits. ### Status by issue | # | Issue из первого ревью | Status | |---|---|---| | 1 | `mergeable=false` (conflict с PR #248) | ✅ **FIXED** — rebase merge commit (`b99ac76`), `_get_cad_zouit_overlaps` fallback в `if row is None` сохранён | | 2 | Request B semantics | ✅ **Functionally OK** — но docstring расходится с фактическим поведением (см. ниже) | | 3 | `Settings()` convention drift | ❌ **NOT fixed** — `quarter_dump_lookup.py:1204-1206` всё ещё `Settings()` instance вместо singleton | | 4 | Race test gap для Request B | ⚠️ **Partial** — 2 SETNX теста добавлены (`test_acquire_harvest_lock_*`), но E2E integration test для Request B response shape отсутствует | ### Request B — фактический поток **Lock denied flow** (трассировка по коду): 1. `_trigger_harvest` → `_acquire_harvest_lock` → False → return False 2. `harvest_triggered = False` → `harvest_eta_seconds = None` (lines 172, 202) 3. FE: `isHarvesting = harvest_triggered && !available = false` → пропускает skeleton spinner 4. FE: попадает в `if (!data)` branch → показывает **«Данные ПЗЗ из НСПД недоступны»** + кнопка ПКК Request B видит **graceful no-data UI с ПКК fallback**, не бесконечный спиннер. **UX приемлемая** — это валидный design choice (одиночный спиннер пользователю с denied lock не критичен, ПКК альтернатива доступна). Vault entry `fixes/Bug_NSPD_WMS_NotBulk_2026_May14.md` явно описывает это поведение как намеренное: «`harvest_triggered=False` (lock уже held) → `harvest_eta_seconds=None`». ### 🟡 Medium issue: docstring расходится с кодом `_trigger_harvest` docstring (line 1228) утверждает: > «остальные получают `False` (т.е. harvest_triggered=False) но видят в response `harvest_eta_seconds` от первого запроса» Код этого не делает — для Request B `eta=None` (см. lines 172, 202). Docstring надо синхронизировать с реальным поведением: ```python # Replace: получают `False` (т.е. harvest_triggered=False) но видят в response `harvest_eta_seconds` от первого запроса и не re-poll сразу. # With: получают `False` (т.е. harvest_triggered=False, harvest_eta_seconds=None). FE при таком ответе показывает graceful no-data UI с ПКК fallback link (не infinite spinner). Trade-off: Request B не видит countdown, но и не сбивает работу Request A. ``` ### 🟡 Medium issue: Settings() drift (issue #3 не закрыт) `quarter_dump_lookup.py:1204-1206`: ```python from app.core.config import Settings client = redis.Redis.from_url(Settings().redis_url) ``` Должно быть: ```python from app.core.config import settings client = redis.Redis.from_url(settings.redis_url) ``` Singleton `settings = Settings()` уже создан в `app/core/config.py:134` — на каждом hot-path вызове `_acquire_harvest_lock` мы инстанцируем новый Pydantic `BaseSettings` объект, который перечитывает env vars. Не критично, но drift от convention остального codebase. ### Что сделано хорошо - ✅ SETNX `nx=True, ex=120` атомарно — корректный pattern - ✅ Lock TTL=120 > harvest ETA 60 — lock жив дольше task'а - ✅ Graceful redis-fail (`except Exception` → return False) — не падаем при transient - ✅ Countdown реактивный (`useEffect` deps правильные, `clearInterval` в cleanup) - ✅ `harvestTimedOut` branch — после `remaining<=0` показывает actionable fallback с ПКК - ✅ Lazy import `redis` внутри `try` — не ломает unit-тесты без redis - ✅ Anti-regression: PR #248 fallback `_get_cad_zouit_overlaps` сохранён после rebase - ✅ Type safety: `harvest_eta_seconds?: number | null` на FE — backward compat ok ### Recommended follow-up Один маленький commit: 1. Fix docstring `_trigger_harvest` (1 параграф) 2. Replace `Settings()` → singleton `settings` (3 строки) 3. Optional: integration test `test_request_b_returns_no_data_with_pkk_fallback` для FE behavior После этого — merge через `merge` strategy. Production monitoring при merge: счётчик harvest lock contention (logger.info на line 1232) → если burst >>1/sec, для Request B стоит вернуть ETA через `redis.ttl()`. Не мержу, жду follow-up на 2 пункта выше.

lekss361 referenced this pull request from a commit

2026-05-17 06:31:46 +00:00

fix(#234): address re-review nits — Settings singleton + docstring sync

lekss361 added 1 commit 2026-05-17 06:31:46 +00:00

fix(#234 ): address re-review nits — Settings singleton + docstring sync 81cf274e80

1. _acquire_harvest_lock: Settings() instance → singleton 'settings' import
   from app.core.config — соответствует convention (scrape_kn.py, objective.py,
   celery_app.py все используют singleton).
2. _trigger_harvest docstring: убрана incorrect фраза «остальные ... видят
   harvest_eta_seconds от первого запроса» (код возвращает eta=None для
   denied-lock callers). Заменена на actual behavior: graceful no-data UI
   с ПКК fallback на FE.

PR #250 re-review followup (issues #3 + docstring sync).

lekss361 commented

2026-05-17 06:31:58 +00:00

Author

Owner

Followup commit 81cf274 — оба 🟡 nit'а из re-review:

Settings() → settings singleton — _acquire_harvest_lock теперь импортирует settings из app.core.config (соответствует pattern из scrape_kn.py, objective.py, celery_app.py).
Docstring sync — _trigger_harvest docstring заменён: убрана неверная фраза «остальные ... видят harvest_eta_seconds от первого запроса», добавлено actual behavior (Request B → graceful no-data UI с ПКК fallback, не infinite spinner) + ссылка на vault post-mortem.

Tests: pytest -k "harvest_eta or harvest_lock or empty_result" → 7 passed. Ruff clean.

Integration test для Request B FE behavior не добавил (3-й optional пункт) — это E2E playwright территория, в scope другого PR. Re-review please.

Followup commit `81cf274` — оба 🟡 nit'а из re-review: 1. **Settings() → settings singleton** — `_acquire_harvest_lock` теперь импортирует `settings` из `app.core.config` (соответствует pattern из `scrape_kn.py`, `objective.py`, `celery_app.py`). 2. **Docstring sync** — `_trigger_harvest` docstring заменён: убрана неверная фраза «остальные ... видят harvest_eta_seconds от первого запроса», добавлено actual behavior (Request B → graceful no-data UI с ПКК fallback, не infinite spinner) + ссылка на vault post-mortem. Tests: `pytest -k "harvest_eta or harvest_lock or empty_result"` → 7 passed. Ruff clean. Integration test для Request B FE behavior не добавил (3-й optional пункт) — это E2E playwright территория, в scope другого PR. Re-review please.

lekss361 merged commit 9dd72ece0c into main

2026-05-17 06:35:41 +00:00

lekss361 referenced this pull request from a commit

2026-05-17 06:35:41 +00:00

Merge pull request 'fix(#234): NSPD harvest ETA badge + SETNX dedupe (Sub-PR C)' (#250) from fix/234-nspd-harvest-eta-badge into main

lekss361 commented

2026-05-17 06:36:03 +00:00

Author

Owner

Deep re-review #3 — verdict: APPROVE (merged)

Head SHA 81cf274e (followup commit поверх b99ac764, +8/-5).

Что закрыто из Review 2

Issue #3 (Settings drift) — FIXED. _acquire_harvest_lock теперь использует from app.core.config import settings + settings.redis_url (singleton), соответствует convention scrape_kn.py / objective.py / celery_app.py.
Issue #2 (docstring lie в _trigger_harvest) — FIXED. Старая ложь «остальные видят harvest_eta_seconds от первого запроса» убрана, заменена на правду: lock-denied caller получает harvest_triggered=False / eta=None → FE показывает graceful no-data UI с ПКК fallback (не infinite spinner). Trade-off Request A vs Request B документирован, есть ссылка на vault fixes/Bug_NSPD_WMS_NotBulk_2026_May14.md.

Anti-regression

SETNX + countdown + FE fallback не тронуты followup-коммитом — только 2 точечные правки в _acquire_harvest_lock (import) и docstring _trigger_harvest.

Production UX monitoring (post-merge)

После раскатки:

Логи quarter=... harvest skipped (lock held by another request) — должны появляться при burst-нагрузке.
Если видны redis SETNX lock failed for quarter=... ... warnings — это redis transient fail, ничего не триггерится (намеренно, лучше скип чем дубль).
FE: при попадании на свежетриггеренный квартал (harvest_triggered=true, harvest_eta_seconds=60) — countdown badge НСПД: загрузка ~Nс + после ETA*1.5 жёлтая плашка с ПКК-кнопкой вместо infinite spinner.

Merged via merge commit 9dd72ece. Closes #234 (UX-side).

## Deep re-review #3 — verdict: APPROVE (merged) Head SHA `81cf274e` (followup commit поверх `b99ac764`, +8/-5). ### Что закрыто из Review 2 - **Issue #3 (Settings drift)** — FIXED. `_acquire_harvest_lock` теперь использует `from app.core.config import settings` + `settings.redis_url` (singleton), соответствует convention `scrape_kn.py` / `objective.py` / `celery_app.py`. - **Issue #2 (docstring lie в `_trigger_harvest`)** — FIXED. Старая ложь «остальные видят `harvest_eta_seconds` от первого запроса» убрана, заменена на правду: lock-denied caller получает `harvest_triggered=False` / `eta=None` → FE показывает graceful no-data UI с ПКК fallback (не infinite spinner). Trade-off Request A vs Request B документирован, есть ссылка на vault `fixes/Bug_NSPD_WMS_NotBulk_2026_May14.md`. ### Anti-regression SETNX + countdown + FE fallback не тронуты followup-коммитом — только 2 точечные правки в `_acquire_harvest_lock` (import) и docstring `_trigger_harvest`. ### Production UX monitoring (post-merge) После раскатки: - Логи `quarter=... harvest skipped (lock held by another request)` — должны появляться при burst-нагрузке. - Если видны `redis SETNX lock failed for quarter=... ...` warnings — это redis transient fail, ничего не триггерится (намеренно, лучше скип чем дубль). - FE: при попадании на свежетриггеренный квартал (`harvest_triggered=true`, `harvest_eta_seconds=60`) — countdown badge `НСПД: загрузка ~Nс` + после ETA*1.5 жёлтая плашка с ПКК-кнопкой вместо infinite spinner. Merged via merge commit `9dd72ece`. Closes #234 (UX-side).

lekss361 referenced this pull request

2026-05-17 06:47:13 +00:00

fix(#231): switch search_by_quarter area layers to grid-walk (Sub-PR B) #260

lekss361 referenced this pull request

2026-05-17 07:37:42 +00:00

fix(#231): re-enable nspd-harvest-stale-quarters beat schedule (Sub-PR D) #267

lekss361 referenced this pull request

2026-05-17 07:51:51 +00:00

fix(#231): re-enable nspd-harvest-stale-quarters beat schedule (Sub-PR D) #267