[Arch] Duplicate scrape-result workflow (_apply_result vs scrape_single) with a live timestamp bug #54

Closed
opened 2026-07-01 07:26:39 +02:00 by bullitt · 1 comment
Owner

Severity: High | Category: Domain

scheduler.py:240-331 (_apply_result) and scheduler.py:448-622 (scrape_single) duplicate ~80 lines of the same workflow. Per docs/tech-debt.md, only scrape_single was fixed to use carrier event timestamps; _apply_result (the scheduled-cycle path, used for the majority of updates) still uses wall-clock time.

Recommendation: Extract one internal function _apply_scrape_result(shipment, result, *, trigger_source) called by both paths.

Refactoring safety: Add characterization tests for both current behaviors first (per docs/architecture.md golden principle #4 — every bug fix gets a regression test).

Full detail: docs/architecture-review.md § C + § H.6

**Severity:** High | **Category:** Domain `scheduler.py:240-331` (`_apply_result`) and `scheduler.py:448-622` (`scrape_single`) duplicate ~80 lines of the same workflow. Per `docs/tech-debt.md`, only `scrape_single` was fixed to use carrier event timestamps; `_apply_result` (the scheduled-cycle path, used for the majority of updates) still uses wall-clock time. **Recommendation:** Extract one internal function `_apply_scrape_result(shipment, result, *, trigger_source)` called by both paths. **Refactoring safety:** Add characterization tests for both current behaviors first (per docs/architecture.md golden principle #4 — every bug fix gets a regression test). Full detail: `docs/architecture-review.md` § C + § H.6
Author
Owner

Fixed on branch architecture-rework, commit afcbbb9: _apply_result and scrape_single now share one scheduler._apply_scrape_result().

Correction while implementing: the specific wall-clock-timestamp bug this issue and docs/tech-debt.md described turned out to already be fixed independently in both copies (both already computed event_occurred_at from carrier event dates) — the doc was just never updated. docs/tech-debt.md has been corrected to reflect this and to describe what was actually fixed.

While merging the two copies, found a second, real bug from the same duplication root cause: main.py constructed ScraperScheduler(notifier=mqtt_notifier) before the full _CompositeNotifier (mqtt+webhook+ntfy) existed, so scheduled-cycle state-change notifications only ever reached MQTT, never webhook/ntfy. Fixed by reordering main.py so the composite notifier is built first and passed directly. Verified via direct import: main.scheduler._notifier is main._notifier is now True. Full suite: 252 passed at the time of this commit.

Fixed on branch `architecture-rework`, commit afcbbb9: `_apply_result` and `scrape_single` now share one `scheduler._apply_scrape_result()`. Correction while implementing: the specific wall-clock-timestamp bug this issue and `docs/tech-debt.md` described turned out to already be fixed independently in both copies (both already computed `event_occurred_at` from carrier event dates) — the doc was just never updated. `docs/tech-debt.md` has been corrected to reflect this and to describe what was actually fixed. While merging the two copies, found a second, real bug from the same duplication root cause: `main.py` constructed `ScraperScheduler(notifier=mqtt_notifier)` before the full `_CompositeNotifier` (mqtt+webhook+ntfy) existed, so scheduled-cycle state-change notifications only ever reached MQTT, never webhook/ntfy. Fixed by reordering `main.py` so the composite notifier is built first and passed directly. Verified via direct import: `main.scheduler._notifier is main._notifier` is now `True`. Full suite: 252 passed at the time of this commit.
Sign in to join this conversation.
No labels
arch
harness
security
ux
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
bullitt/trackbox#54
No description provided.