gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 113 Pull requests 8 Projects Releases Packages Wiki Activity Actions

fix(occupancy): address holiday stamping follow-ups #267

Open
gabogg wants to merge 2 commits from fix/holiday-stamping-followups into master
pull from: fix/holiday-stamping-followups
merge into: gabogg:master
Conversation 3 Commits 2 Files changed 5 +730 -155
gabogg commented 2026-10-03 12:30:26 +00:00
Owner
Copy link

Closes #244, Closes #245, Closes #246

Summary

Resolves the three P3 follow-ups from PR #178 regarding holiday stamping consistency, migration idempotence, and atomic exception writes, incorporating review pass 1 (r57) updates:

  • #246 (holiday reader alignment on missed freeze): Closes the gap where an unfrozen holiday that later gained backfilled calibration history split the readers. The learning query (get_trusted_calibration_history_conn_async) and spike reference query (get_recent_trusted_cycle_ingress_async) now fall back via hoisted _HOLIDAY_CYCLE_DATES_SQL to occupancy_holidays when a day has no business-day schedule record (WHERE is_holiday = 1 AND holiday_date NOT IN (SELECT day_date FROM occupancy_business_day_schedules)), ensuring all readers agree while preserving the primacy of frozen records. The reconcile auto-stamp was removed from reconcile_historical_calibrations_async because the SQL fallback fully resolves the reader gap without mutating draft schedules (preserving CONTEXT.md:131/:134 invariants).
  • #244 (holiday stamping migration separation and helper reuse): Split the past holiday stamping migration into its own version (20261003_holiday_stamping_backfill) so databases that previously ran 20261002_holiday_backfill still receive stamped records. The migration and runtime readers reuse shared pure helpers in app.facility_time (schedule_calendar_from_rows, stamped_from_planned) and _BUSINESS_DAY_SCHEDULE_UPSERT_SQL in occupancy_repository, ensuring zero drift. Unnamed exceptions produce exception_name = None (NULL in DB), matching #222.
  • #245 (atomic exception write and stamping in single repo transaction): Replaced two-step compensating rollback with a single atomic repository transaction (BEGIN IMMEDIATE) in add_holiday_async and classify_holiday_async that writes the exception, the STAMPED record (if unrecorded and completed), and the audits together. If stamping raises, fails on lock, or is cancelled, the entire transaction rolls back cleanly without orphan rows or audit noise.

Architectural Impact

  • Database (app/db/database.py): Added migration step 20261003_holiday_stamping_backfill reusing schedule_calendar_from_rows, stamped_from_planned, and repository upsert SQL and parameters.
  • Facility Time (app/facility_time.py): Added pure helpers schedule_calendar_from_rows and stamped_from_planned. Baseline weekly hours use DEFAULT_DAILY_SCHEDULE from master.
  • Repository (app/db/occupancy_repository.py): Hoisted _HOLIDAY_CYCLE_DATES_SQL for holiday reader subqueries. Added stamped_schedule parameter to add_holiday_async and classify_holiday_async to execute within atomic BEGIN IMMEDIATE transaction.
  • Service (app/services/occupancy_service.py): Precomputes stamped schedule if completing past/active day and passes to repo methods; removed compensating try/except rollback and removed auto-stamping from reconcile_historical_calibrations_async.

Verification / Test Evidence

  • Reproduced defect #246 with a test verifying reader disagreement on unfrozen holiday with backfilled history, and verified the fix aligns all readers (test_missed_freeze_holiday_backfilled_all_readers_agree).
  • Verified migration 20261003_holiday_stamping_backfill stamps past holidays on DBs with 20261002_holiday_backfill already present (test_migration_stamps_never_stamped_past_holiday_all_readers_agree).
  • Verified pure helpers produce identical rows between migration and runtime stamping, and unnamed exceptions produce null exception_name (test_migration_stamped_rows_match_service_stamped_rows).
  • Added tests verifying atomic rollback of exception additions, updates, classifications, CancelledError, and lock failure (test_add_schedule_exception_stamp_failure_rolls_back_exception, test_add_schedule_exception_cancelled_error_and_lock_failure, test_classify_holiday_stamp_failure_rolls_back_classification).
  • All 25 tests in tests/test_holiday_event_context.py pass.
  • Full pytest test suite (609 passed).
  • ruff check and ruff format are clean.
  • python3 scripts/check_docs.py passed (43 Markdown files and 90 HTTP operations verified).

Checklist

  • #246: Hoisted reader subquery fallback aligns all readers on unfrozen holiday without mutating draft schedules
  • #244: Versioned migration 20261003_holiday_stamping_backfill reusing pure helpers schedule_calendar_from_rows and stamped_from_planned
  • #245: Single repository transaction (BEGIN IMMEDIATE) atomically writes exception and STAMPED schedule record, with CancelledError and lock tests
  • Merge origin/master (9a608c1) with DEFAULT_DAILY_SCHEDULE baseline
  • Passing linter (ruff), doc checker (scripts/check_docs.py), and test suite (pytest)
Closes #244, Closes #245, Closes #246 ### Summary Resolves the three P3 follow-ups from PR #178 regarding holiday stamping consistency, migration idempotence, and atomic exception writes, incorporating review pass 1 (r57) updates: - **#246 (holiday reader alignment on missed freeze)**: Closes the gap where an unfrozen holiday that later gained backfilled calibration history split the readers. The learning query (`get_trusted_calibration_history_conn_async`) and spike reference query (`get_recent_trusted_cycle_ingress_async`) now fall back via hoisted `_HOLIDAY_CYCLE_DATES_SQL` to `occupancy_holidays` when a day has no business-day schedule record (`WHERE is_holiday = 1 AND holiday_date NOT IN (SELECT day_date FROM occupancy_business_day_schedules)`), ensuring all readers agree while preserving the primacy of frozen records. The reconcile auto-stamp was removed from `reconcile_historical_calibrations_async` because the SQL fallback fully resolves the reader gap without mutating draft schedules (preserving CONTEXT.md:131/:134 invariants). - **#244 (holiday stamping migration separation and helper reuse)**: Split the past holiday stamping migration into its own version (`20261003_holiday_stamping_backfill`) so databases that previously ran `20261002_holiday_backfill` still receive stamped records. The migration and runtime readers reuse shared pure helpers in `app.facility_time` (`schedule_calendar_from_rows`, `stamped_from_planned`) and `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL` in `occupancy_repository`, ensuring zero drift. Unnamed exceptions produce `exception_name = None` (NULL in DB), matching #222. - **#245 (atomic exception write and stamping in single repo transaction)**: Replaced two-step compensating rollback with a single atomic repository transaction (`BEGIN IMMEDIATE`) in `add_holiday_async` and `classify_holiday_async` that writes the exception, the STAMPED record (if unrecorded and completed), and the audits together. If stamping raises, fails on lock, or is cancelled, the entire transaction rolls back cleanly without orphan rows or audit noise. ### Architectural Impact - **Database (`app/db/database.py`)**: Added migration step `20261003_holiday_stamping_backfill` reusing `schedule_calendar_from_rows`, `stamped_from_planned`, and repository upsert SQL and parameters. - **Facility Time (`app/facility_time.py`)**: Added pure helpers `schedule_calendar_from_rows` and `stamped_from_planned`. Baseline weekly hours use `DEFAULT_DAILY_SCHEDULE` from master. - **Repository (`app/db/occupancy_repository.py`)**: Hoisted `_HOLIDAY_CYCLE_DATES_SQL` for holiday reader subqueries. Added `stamped_schedule` parameter to `add_holiday_async` and `classify_holiday_async` to execute within atomic `BEGIN IMMEDIATE` transaction. - **Service (`app/services/occupancy_service.py`)**: Precomputes stamped schedule if completing past/active day and passes to repo methods; removed compensating try/except rollback and removed auto-stamping from `reconcile_historical_calibrations_async`. ### Verification / Test Evidence - Reproduced defect #246 with a test verifying reader disagreement on unfrozen holiday with backfilled history, and verified the fix aligns all readers (`test_missed_freeze_holiday_backfilled_all_readers_agree`). - Verified migration `20261003_holiday_stamping_backfill` stamps past holidays on DBs with `20261002_holiday_backfill` already present (`test_migration_stamps_never_stamped_past_holiday_all_readers_agree`). - Verified pure helpers produce identical rows between migration and runtime stamping, and unnamed exceptions produce null `exception_name` (`test_migration_stamped_rows_match_service_stamped_rows`). - Added tests verifying atomic rollback of exception additions, updates, classifications, `CancelledError`, and lock failure (`test_add_schedule_exception_stamp_failure_rolls_back_exception`, `test_add_schedule_exception_cancelled_error_and_lock_failure`, `test_classify_holiday_stamp_failure_rolls_back_classification`). - All 25 tests in `tests/test_holiday_event_context.py` pass. - Full pytest test suite (609 passed). - `ruff check` and `ruff format` are clean. - `python3 scripts/check_docs.py` passed (43 Markdown files and 90 HTTP operations verified). ### Checklist - [x] #246: Hoisted reader subquery fallback aligns all readers on unfrozen holiday without mutating draft schedules - [x] #244: Versioned migration `20261003_holiday_stamping_backfill` reusing pure helpers `schedule_calendar_from_rows` and `stamped_from_planned` - [x] #245: Single repository transaction (`BEGIN IMMEDIATE`) atomically writes exception and STAMPED schedule record, with `CancelledError` and lock tests - [x] Merge `origin/master` (9a608c1) with `DEFAULT_DAILY_SCHEDULE` baseline - [x] Passing linter (`ruff`), doc checker (`scripts/check_docs.py`), and test suite (`pytest`)
fix(occupancy): address holiday stamping follow-ups (#244, #245, #246)
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m4s
Details
b4c9fd54ad 
- #246: prevent reader disagreement on unfrozen holidays with backfilled history
  by falling back to occupancy_holidays in learning and spike queries when no schedule record exists.
- #244: register holiday stamping backfill under 20261003_holiday_stamping_backfill,
  reusing ScheduleCalendar and stamping helpers without duplicated literals.
- #245: encapsulate stamping check in _ensure_completed_day_stamped and roll back
  exception modifications atomically if stamping raises.
gabogg changed title from WIP: fix(occupancy): address holiday stamping follow-ups to fix(occupancy): address holiday stamping follow-ups 2026-10-03 12:50:12 +00:00
gabogg commented 2026-10-03 12:50:23 +00:00
Author
Owner
Copy link

Implementation complete. Ready for review; requesting pass 1.

Implementation complete. Ready for review; requesting pass 1.
gabogg reviewed 2026-10-03 18:03:14 +00:00
gabogg left a comment
Copy link

Code review, pass 1 (origin/master...b4c9fd5, spec #244 + #245 + #246)

Summary: 0 P1, 3 P2s, a master clash, and P3s. Pass 1: fix everything.

The #246 reader fallback works and is reproduced by tests. The #244 migration is idempotent.

Must fix:

  • P2 (both axes): reconcile now stamps every reconciled past day (occupancy_service.py:2544-2548). This breaks CONTEXT.md:131 and :134: a day without a record stays unrecorded until an admin stamps it, and an imported day's original schedule comes from its approved draft. For M1, the first startup after a historical import would freeze every imported day's schedule before review. The SQL fallback already fixes #246, so remove the stamp.
  • P2 (both axes): #244 is only half met. The migration (database.py:600-690) still copies the calendar loader, the STAMPED record construction and the insert/audit SQL, and the copies have already drifted: an empty name gives "" in the migration but the ISO date at runtime. Extract shared pure helpers and reuse _BUSINESS_DAY_SCHEDULE_UPSERT_SQL. Note that #222's P2-A changes the runtime side to null, so align on null.
  • P2 (both axes): #245's "rollback" is a compensating write. It is skipped on CancelledError, it masks the original error when the undo itself hits "database is locked", and it leaves CREATE/DELETE audit noise. Write the exception and the stamp in one repository transaction (BEGIN IMMEDIATE).
  • Master clash: the PR uses DEFAULT_WEEKDAY_HOURS in database.py, and master removed that import. The merge is textually clean, but ruff reports F821. Merge origin/master and use master's seed defaults.
  • All P3s below.

Standards axis

Scope: app/db/database.py, app/db/occupancy_repository.py, app/services/occupancy_service.py, tests/test_holiday_event_context.py.
Checks I ran:

  • tests/test_holiday_event_context.py at the PR head: 23 passed.
  • The same file plus tests/test_holiday_state_reset.py on the tree merged with master 9a608c1 (git merge-tree): 25 passed, no conflicts.
  • ruff check and ruff format: clean.
  • The #246 test against the merge-base repository fails at the learning-reader assertion, so the reproduction is real.

PR metadata:

  • Label: ready-for-agent (one triage-role label, correct).
  • Milestone: "Historical passenger-flow backfill", which is the same as #246's milestone (M1). Correct.
  • #244 and #245 have no milestone, which is correct under milestones.md.

P1

None.

P2

P2-1. The #245 rollback is a best-effort compensation, so an orphan exception survives cancellation or a failed rollback. CONFIRMED (probe)

app/services/occupancy_service.py:543-561, and the same pattern in classify at :1022-1037.

add_holiday_async commits in its own transaction. The stamp then runs in a second transaction, and on failure the code tries to undo the first write with a third one inside except Exception. Two paths leave the exception saved with no business-day record, which is exactly the reader split #245 is meant to close:

  • Cancellation: asyncio.CancelledError is a BaseException, so except Exception never runs the rollback. This happens on shutdown, on a timeout, or when the task is cancelled mid-stamp.
    • Probe: the stamp raises CancelledError, then get_holiday_by_date_async("2026-08-12") returns a row and the record is None.
  • The rollback itself fails: the failure the issue names (a reset race or a locked database) is likely to hit the compensating write too. The rollback error then replaces the stamp error and the orphan stays.
    • Probe: the stamp raises "database is locked (stamp)" and delete_holiday_async raises too. The caller sees "database is locked (rollback)" (the original survives only as __context__) and the orphan holiday remains.

A crash between the two commits leaves the same state.

Fix: the issue's first option, one transaction.

  • The service builds the planned BusinessDaySchedule in memory with ScheduleCalendar, applying the new exception over the loaded calendar.
  • One repository method then upserts the holiday and inserts the STAMPED record plus its audit row inside a single BEGIN IMMEDIATE, as stamp_business_day_schedules_async already does.
  • That also removes the add/delete audit noise below.
  • If the compensation stays, it needs at least except BaseException, plus a nested try that logs the rollback failure and re-raises the original error.
P2-2. #244's acceptance criterion "rows built with the same helpers as stamp_business_day_schedules_async" is only half met, and the copies have already drifted. CONFIRMED

app/db/database.py:623-690.

The migration now uses ScheduleCalendar.planned_for, configured_* and asdict, which is good. It still re-implements three things:

  1. The calendar loader. This is a third copy, after get_schedule_calendar_async at occupancy_service.py:1269 and _planned_schedule_for_day_async at :1307.
  2. The STAMPED BusinessDaySchedule construction. This is copied verbatim from occupancy_service.py:1403-1413.
  3. The upsert and audit SQL. The repository already has _BUSINESS_DAY_SCHEDULE_UPSERT_SQL and _business_day_schedule_params.

Drift that already exists:

  • Exception name: the migration uses str(h["name"] or "") and the runtime uses name or day.isoformat(). A holiday with an empty name is stamped exception_name="" by the migration but the ISO date by the runtime path.
  • Weekly hours: the migration uses r["open_time"] or DEFAULT and the runtime uses row.get("open_time", DEFAULT), which turns NULL into the text "None".

These are the "resolver and migration can drift" defects the issue was filed for.

Fix:

  • Extract pure, sync helpers into facility_time (or a schedule module) and call them from both the migration and the service:
    • schedule_calendar_from_rows(holiday_rows, weekly_rows, cfg)
    • stamped_schedule(planned) -> BusinessDaySchedule (or BusinessDaySchedule.stamped_from(planned))
  • Reuse _BUSINESS_DAY_SCHEDULE_UPSERT_SQL and _business_day_schedule_params for the insert.

P3

P3-1. Duplicated holiday-identity subquery in the repository (Duplicated Code). CONFIRMED

app/db/occupancy_repository.py:2958-2964 and :3281-3287 carry the same 6-line UNION ... NOT IN block. The "holiday identity of a cycle date" rule now lives in two SQL copies plus the service fallback. Decision 1 asks for one source.

The repository already uses module SQL constants (_K_MEASUREMENT_PRIORITY_SQL). Hoist it the same way, e.g. _HOLIDAY_CYCLE_DATES_SQL, and use it in both queries.

P3-2. The reconcile path now writes STAMPED records for every reconciled day, with a mislabeled audit reason and no test. CONFIRMED

occupancy_service.py:2544-2548.

  • Broader side effect than #246 needs: it stamps ordinary weekdays too. The SQL fallback (P3-1) already closes #246 for the learning and spike readers, so this write is either redundant or needs its own justification in the PR body.
  • Mislabeled audit reason: the schedule audit row gets the calibration-log reason, "Scheduled nocturnal quiet window calibration (reconciled for …)", rather than a stamping reason.
  • Cost on long backfills: each iteration calls stamp_business_day_schedules_async, which calls get_schedule_calendar_async. That loads every holiday, the weekly rows and every business-day record, once per reconciled day, which is O(N²). Stamping the whole range once before the loop is a single call.
  • New abort path: a stamp failure (for example a locked database) now aborts the whole reconcile loop. Before this change the loop did not write at that point.
  • Wall-clock mismatch (PLAUSIBLE): stamp_business_day_schedules_async judges "completed" against the wall clock, while reconcile uses its now_epoch parameter (:2641 passes one). With a now_epoch ahead of the wall clock, the cursor day can be ≥ the real active day, so the stamp raises "Only completed business days can be stamped".
  • Untested: test_missed_freeze_holiday_backfilled_all_readers_agree simulates the backfill with record_calibration_log_async directly, so this branch is never exercised.

Either drop the reconcile stamping or test it, and give it its own reason string.

P3-3. The default reason string is repeated three times. CONFIRMED

"Stamp completed business day for schedule exception" is the helper's default (occupancy_service.py:507), and both callers also pass reason or "<same string>" (:549, :1027). Let the callers pass reason and keep the default only in the helper.

The helper also defaults actor="SYSTEM", which no caller relies on (Speculative Generality). Its docstring says "completed business day", but it never checks that; it depends on stamp_business_day_schedules_async raising ValueError.

P3-4. Dead alias and dead guard in classify_holiday_async. CONFIRMED

occupancy_service.py:1013 and :1031:

  • old_holiday = target is a pure alias.
  • if old_holiday is not None is always true, because target is checked at :1008.
  • .get("is_holiday", True) invents a default for a column that SELECT * always returns. If it were ever missing, the "rollback" would set the day to holiday.

Use bool(target["is_holiday"]).

P3-5. Rollback audit noise and actor attribution. CONFIRMED

The compensation writes extra occupancy_holiday_audit rows:

  • CREATE then DELETE, or UPDATE then UPDATE, when the exception is added.
  • CLASSIFY then CLASSIFY, when it is classified.

All of them carry the admin's actor and the hardcoded English reason "Rollback after stamp failure". The audit then shows a user action that never logically happened. This goes away with the single transaction from P2-1.

P3-6. Test gaps and nits. CONFIRMED
  • No audit assertions: the rollback tests do not check the audit tables, so P3-5 is unpinned. They also inject only RuntimeError, never CancelledError (P2-1).
  • Unreachable setup in Case B: test_add_schedule_exception_stamp_failure_rolls_back_exception deletes the business-day record "if one was stamped". The preceding call is occupancy_repo.add_holiday_async, which never stamps, so the setup cannot fire. Drop it, or assert the record is absent.
  • Fragile magic number: test_missed_freeze... asserts 1500 not in ingress. That is a value match; asserting on the cycle date or the count is less fragile.
  • Duplicate isolation fixture (not from this PR): the module's own autouse isolated_event_db duplicates conftest's isolated_repository_db. The new tests do write shared tables but get isolation from it, so this is fine. Optionally switch to pytestmark = pytest.mark.usefixtures("isolated_repository_db").
P3-7. The PR body has the wrong function name. CONFIRMED

The body says reconcile_missing_nocturnal_calibrations_async. The function is reconcile_historical_calibrations_async.

Note: master compatibility

  • The PR is based on 9a72ff4. Master 9a608c1 merges cleanly (merge-tree 6b6da3a).
  • Master's new conftest reset_occupancy_state_in_tests fixture and tests/test_holiday_state_reset.py coexist with the new tests: 25 passed on the merged tree.
  • No semantic clash found.

Spec axis

Spec: #244, #245, #246 (follow-ups from PR #178). Merge base 9a72ff4, master 9a608c1.
Probes: git-archive exports pr267/, base267/ (merge base + PR tests), merged267/ (merge-tree of master + PR); probe file pr267/tests/test_probe267.py.
Targeted runs: tests/test_holiday_event_context.py + tests/test_calibration_reconciliation.py pass 26/26 on the PR and on the merged tree. On the merge base, the new tests fail as expected: the migration-version assert, the learning-reader assert for #246, and the orphan-exception assert for #245. So the reproductions are real.

Acceptance criteria

Issue Criterion Status
#244 Own migration version, idempotent, runs on DB that already has 20261002_holiday_backfill MET (probe: deleting the 20261003 key and re-running init_db 3x leaves 1 record + 1 STAMP audit row)
#244 Rows built with the same helpers as stamp_business_day_schedules_async, no duplicated literals PARTIAL: see P2-2
#244 Test: DB with old key recorded + unstamped past holiday gets record + STAMP audit on startup MET (modified test_migration_stamps_never_stamped_past_holiday_all_readers_agree)
#245 One transaction, or stamp failure rolls back the exception PARTIAL: compensating writes, not a rollback; see P2-3
#245 One helper used by both call sites MET (_ensure_completed_day_stamped)
#245 Test with injected stamp failure: no orphan exception, no missing record MET (add + update + classify cases)
#246 Reproduce missed freeze + backfill in a test MET (fails on merge base, passes on PR)
#246 All readers agree MET via SQL fallback in both repo readers; but the extra reconcile auto-stamp oversteps, see P2-1

Findings

P2-1: Reconciliation now auto-stamps every reconciled past day, not just holidays, which goes against CONTEXT.md. CONFIRMED

app/services/occupancy_service.py:2544-2548 (inside reconcile_historical_calibrations_async; the PR body calls it reconcile_missing_nocturnal_calibrations_async, which does not exist).

  • What happens: for every completed cycle that has events and no calibration log, the daemon writes a permanent STAMPED business-day record. The record is built from the current weekly plan and exceptions. This applies to ordinary days too, not only holidays.
  • Probe: an ordinary Wednesday 2026-09-09 with events and no record. Run reconcile, which also runs at startup via reconcile_and_quarantine_historical_anomalies_async. Result: a record {source: STAMPED, is_exception: 0, is_holiday: 0, 08:00-21:00} plus an audit row STAMP / SYSTEM_DAEMON / "Scheduled nocturnal quiet window calibration (reconciled for 2026-09-09)".
  • Why it's wrong:
    • CONTEXT.md:131 says "A historical day without a record uses the current schedule until an administrator stamps or corrects it."
    • CONTEXT.md:134 says an imported day's Original Schedule is fixed "when its reviewed draft was approved".
    • For M1 (historical backfill), the first startup after events are imported would stamp every imported day from today's plan. That becomes the day's Original Schedule, because the first audit row wins in get_original_business_day_schedule_async. This happens before any draft review, so the reviewed-draft path cannot set the Original Schedule any more. Calibration then follows a schedule nobody approved.
  • Not needed for #246: the repo SQL fallback (occupancy_repository.py:2957-2963, 3280-3286) already makes all readers agree without writing anything. The new test passes without reconcile involvement; it never exercises this path.
  • Fix: drop the reconcile stamp, or limit it to is_holiday exception days and get maintainer sign-off on the CONTEXT change. Also use an honest audit reason. The current reason is a calibration string on a schedule stamp.
P2-2: The migration still re-implements the stamping path and has already drifted from it. CONFIRMED

app/db/database.py:600-667.

  • What it duplicates: the migration hand-builds the weekly/exception ScheduleCalendar loader, copying get_schedule_calendar_async. It also hand-builds the planned -> BusinessDaySchedule(source="STAMPED", recorded=True) block, copying occupancy_service.py:1400-1414. #244 asked for "the same helpers … no duplicated literals". Only the leaf helpers (configured_*, asdict, ScheduleCalendar) are shared.
  • Drift already present:
    • The exception_names default is str(h["name"] or "") (database.py:645). The service uses name or day.isoformat() (occupancy_service.py:1286).
    • Probe: an unnamed past holiday 2026-08-20 gets exception_name='' from the migration, but '2026-08-20' from stamp_business_day_schedules_async. Same data, two different records depending on which path stamped it. This is exactly the drift #244 warned about.
    • The weekly fallback also differs. The migration uses row or DEFAULT and the service uses row.get(key, DEFAULT).
  • Fix: extract the two pieces as pure functions in app/facility_time.py and call them from both sides:
    • schedule_calendar_from_rows(holidays, weekly, cfg)
    • stamped_from_planned(planned) (or replace(planned, source="STAMPED", recorded=True))
P2-3: The #245 "rollback" is a compensating write in a second transaction; a double fault leaves the orphan and hides the cause. CONFIRMED

app/services/occupancy_service.py:543-561 (add) and 1028-1037 (classify).

  • What happens: the exception is committed by add_holiday_async. On stamp failure, a separate delete_holiday_async or add_holiday_async undoes it.
  • Probe: stamp raises RuntimeError("stamp boom"), then the compensating delete raises RuntimeError("database is locked"). Result:
    • The caller sees database is locked; the stamp error survives only as __context__.
    • The holiday row stays (orphan = True) with no business-day record.
    • That is the exact split #245 was filed to prevent.
  • Real-world trigger: BEGIN IMMEDIATE contention, which is the kind of failure that hits both writes.
  • Other effects, even on success:
    • The holiday audit permanently gets CREATE + DELETE ("Rollback after stamp failure") rows. A re-created exception gets a new id.
    • Readers can see the exception during the window between the two commits.
  • Fix: the spec's first option. Do the exception upsert and the stamp in one repo transaction, for example a repo method that takes the holiday dict plus the planned BusinessDaySchedule and runs both under one BEGIN IMMEDIATE. If compensation is kept, re-raise the original error (raise orig from comp_err) and log the compensation failure.

Merge-with-master note (separate from the PR's own correctness)

MC-1: The merged tree fails ruff with F821 undefined DEFAULT_WEEKDAY_HOURS. CONFIRMED
  • Cause: master (1885b27 / #233 line) removed DEFAULT_WEEKDAY_HOURS from app/db/database.py imports. The PR's migration uses it at database.py:605-606.
  • Effect:
    • git merge-tree reports no textual conflict, but ruff check app/db/database.py on the merged tree reports F821 at 605:59 and 606:59. CI and pre-commit will fail after merge.
    • At runtime the name is evaluated only when a weekly row has an empty open_time/close_time, which is rare because the columns are NOT NULL. So startup normally survives, which is why tests pass.
  • Fix: rebase on master and re-import the name. Better, P2-2's shared loader removes the need.

P3

  • P3-1: Reconcile crashes with an explicit now_epoch later than wall-clock. CONFIRMED
    • Where: occupancy_service.py:2544. Reconcile picks cycles from now_epoch, but stamp_business_day_schedules_async checks against real active_business_day().
    • Probe: events on 2026-11-10 with now_epoch = 2026-11-12 raise ValueError: Only completed business days can be stamped and abort the whole reconcile.
    • Production passes None, so the impact is tests and any future caller. Moot if P2-1 removes the stamp.
  • P3-2: classify_holiday_async rollback has dead and odd code. occupancy_service.py:1016, 1030-1032.
    • old_holiday = target is always non-None, so the if old_holiday is not None guard is dead.
    • .get("is_holiday", True) defaults the restore to True. On a missing key that would leave the day marked holiday and unstamped.
    • Use bool(target["is_holiday"]).
  • P3-3: The add-path guard if result and "id" in result and result["id"] is not None silently skips the rollback. occupancy_service.py:552-556. If the id is missing, the orphan survives with no log. At minimum, log it.
  • P3-4: The migration's past-day cutoff uses configured_reset_time (current config) instead of the effective-dated ResetSchedule the service uses. database.py:610-613. This is harmless for a one-day cutoff, but it is another divergence from "the stamping path"; worth a comment or the shared helper.
  • P3-5: _ensure_completed_day_stamped costs a full get_schedule_calendar_async() per day, through stamp_business_day_schedules_async. That loads all holidays and all records. In reconcile it runs once per reconciled cycle, which is O(n²) over a long M1 backfill. Moot if P2-1 drops it.
  • P3-6: Test gaps.
    • Nothing tests the reconcile stamping path, and none of the new tests calls reconcile.
    • The #246 test's readers 1-3 (calendar, business-day fallback, calibration fallback) already passed on master. Only readers 4-5 cover the fix.
    • Nothing tests the double-fault in P2-3.
    • Nothing compares migration-stamped rows with service-stamped rows, which would have caught P2-2.
  • P3-7: PR body inaccuracies. It names reconcile_missing_nocturnal_calibrations_async (the real name is reconcile_historical_calibrations_async). It says the migration builds rows "without hard-coded literals", which P2-2 shows is not fully true.

Verdict

The core #246 SQL fallback and the #244 version split are correct and reproduce/fix the reported defects. Changes requested:

  • P2-1: the reconcile auto-stamp oversteps the domain rule and M1's reviewed-draft Original Schedule. Remove it.
  • P2-2: the migration has not reused the stamping path and already disagrees on exception_name.
  • P2-3: the #245 rollback is non-atomic and masks errors.
  • MC-1: rebase to fix the F821 against master.
## Code review, pass 1 (`origin/master...b4c9fd5`, spec #244 + #245 + #246) **Summary: 0 P1, 3 P2s, a master clash, and P3s. Pass 1: fix everything.** The #246 reader fallback works and is reproduced by tests. The #244 migration is idempotent. Must fix: - **P2 (both axes): reconcile now stamps every reconciled past day** (`occupancy_service.py:2544-2548`). This breaks CONTEXT.md:131 and :134: a day without a record stays unrecorded until an admin stamps it, and an imported day's original schedule comes from its approved draft. For M1, the first startup after a historical import would freeze every imported day's schedule before review. The SQL fallback already fixes #246, so remove the stamp. - **P2 (both axes): #244 is only half met.** The migration (`database.py:600-690`) still copies the calendar loader, the STAMPED record construction and the insert/audit SQL, and the copies have already drifted: an empty name gives `""` in the migration but the ISO date at runtime. Extract shared pure helpers and reuse `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL`. Note that #222's P2-A changes the runtime side to null, so align on null. - **P2 (both axes): #245's "rollback" is a compensating write.** It is skipped on `CancelledError`, it masks the original error when the undo itself hits "database is locked", and it leaves CREATE/DELETE audit noise. Write the exception and the stamp in one repository transaction (`BEGIN IMMEDIATE`). - **Master clash:** the PR uses `DEFAULT_WEEKDAY_HOURS` in `database.py`, and master removed that import. The merge is textually clean, but ruff reports F821. Merge origin/master and use master's seed defaults. - All P3s below. ### Standards axis Scope: app/db/database.py, app/db/occupancy_repository.py, app/services/occupancy_service.py, tests/test_holiday_event_context.py. Checks I ran: - `tests/test_holiday_event_context.py` at the PR head: 23 passed. - The same file plus `tests/test_holiday_state_reset.py` on the tree merged with master 9a608c1 (`git merge-tree`): 25 passed, no conflicts. - ruff check and ruff format: clean. - The #246 test against the merge-base repository fails at the learning-reader assertion, so the reproduction is real. PR metadata: - Label: `ready-for-agent` (one triage-role label, correct). - Milestone: "Historical passenger-flow backfill", which is the same as #246's milestone (M1). Correct. - #244 and #245 have no milestone, which is correct under milestones.md. #### P1 None. #### P2 ##### P2-1. The #245 rollback is a best-effort compensation, so an orphan exception survives cancellation or a failed rollback. CONFIRMED (probe) `app/services/occupancy_service.py:543-561`, and the same pattern in classify at `:1022-1037`. `add_holiday_async` commits in its own transaction. The stamp then runs in a second transaction, and on failure the code tries to undo the first write with a third one inside `except Exception`. Two paths leave the exception saved with no business-day record, which is exactly the reader split #245 is meant to close: - **Cancellation:** `asyncio.CancelledError` is a `BaseException`, so `except Exception` never runs the rollback. This happens on shutdown, on a timeout, or when the task is cancelled mid-stamp. - Probe: the stamp raises `CancelledError`, then `get_holiday_by_date_async("2026-08-12")` returns a row and the record is `None`. - **The rollback itself fails:** the failure the issue names (a reset race or a locked database) is likely to hit the compensating write too. The rollback error then replaces the stamp error and the orphan stays. - Probe: the stamp raises "database is locked (stamp)" and `delete_holiday_async` raises too. The caller sees "database is locked (rollback)" (the original survives only as `__context__`) and the orphan holiday remains. A crash between the two commits leaves the same state. Fix: the issue's first option, one transaction. - The service builds the planned `BusinessDaySchedule` in memory with `ScheduleCalendar`, applying the new exception over the loaded calendar. - One repository method then upserts the holiday and inserts the STAMPED record plus its audit row inside a single `BEGIN IMMEDIATE`, as `stamp_business_day_schedules_async` already does. - That also removes the add/delete audit noise below. - If the compensation stays, it needs at least `except BaseException`, plus a nested try that logs the rollback failure and re-raises the original error. ##### P2-2. #244's acceptance criterion "rows built with the same helpers as `stamp_business_day_schedules_async`" is only half met, and the copies have already drifted. CONFIRMED `app/db/database.py:623-690`. The migration now uses `ScheduleCalendar.planned_for`, `configured_*` and `asdict`, which is good. It still re-implements three things: 1. **The calendar loader.** This is a third copy, after `get_schedule_calendar_async` at `occupancy_service.py:1269` and `_planned_schedule_for_day_async` at `:1307`. 2. **The STAMPED `BusinessDaySchedule` construction.** This is copied verbatim from `occupancy_service.py:1403-1413`. 3. **The upsert and audit SQL.** The repository already has `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL` and `_business_day_schedule_params`. Drift that already exists: - **Exception name:** the migration uses `str(h["name"] or "")` and the runtime uses `name or day.isoformat()`. A holiday with an empty name is stamped `exception_name=""` by the migration but the ISO date by the runtime path. - **Weekly hours:** the migration uses `r["open_time"] or DEFAULT` and the runtime uses `row.get("open_time", DEFAULT)`, which turns NULL into the text `"None"`. These are the "resolver and migration can drift" defects the issue was filed for. Fix: - Extract pure, sync helpers into `facility_time` (or a schedule module) and call them from both the migration and the service: - `schedule_calendar_from_rows(holiday_rows, weekly_rows, cfg)` - `stamped_schedule(planned) -> BusinessDaySchedule` (or `BusinessDaySchedule.stamped_from(planned)`) - Reuse `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL` and `_business_day_schedule_params` for the insert. #### P3 ##### P3-1. Duplicated holiday-identity subquery in the repository (Duplicated Code). CONFIRMED `app/db/occupancy_repository.py:2958-2964` and `:3281-3287` carry the same 6-line `UNION ... NOT IN` block. The "holiday identity of a cycle date" rule now lives in two SQL copies plus the service fallback. Decision 1 asks for one source. The repository already uses module SQL constants (`_K_MEASUREMENT_PRIORITY_SQL`). Hoist it the same way, e.g. `_HOLIDAY_CYCLE_DATES_SQL`, and use it in both queries. ##### P3-2. The reconcile path now writes STAMPED records for every reconciled day, with a mislabeled audit reason and no test. CONFIRMED `occupancy_service.py:2544-2548`. - **Broader side effect than #246 needs:** it stamps ordinary weekdays too. The SQL fallback (P3-1) already closes #246 for the learning and spike readers, so this write is either redundant or needs its own justification in the PR body. - **Mislabeled audit reason:** the schedule audit row gets the calibration-log reason, "Scheduled nocturnal quiet window calibration (reconciled for …)", rather than a stamping reason. - **Cost on long backfills:** each iteration calls `stamp_business_day_schedules_async`, which calls `get_schedule_calendar_async`. That loads every holiday, the weekly rows and every business-day record, once per reconciled day, which is O(N²). Stamping the whole range once before the loop is a single call. - **New abort path:** a stamp failure (for example a locked database) now aborts the whole reconcile loop. Before this change the loop did not write at that point. - **Wall-clock mismatch (PLAUSIBLE):** `stamp_business_day_schedules_async` judges "completed" against the wall clock, while reconcile uses its `now_epoch` parameter (`:2641` passes one). With a `now_epoch` ahead of the wall clock, the cursor day can be ≥ the real active day, so the stamp raises "Only completed business days can be stamped". - **Untested:** `test_missed_freeze_holiday_backfilled_all_readers_agree` simulates the backfill with `record_calibration_log_async` directly, so this branch is never exercised. Either drop the reconcile stamping or test it, and give it its own reason string. ##### P3-3. The default reason string is repeated three times. CONFIRMED `"Stamp completed business day for schedule exception"` is the helper's default (`occupancy_service.py:507`), and both callers also pass `reason or "<same string>"` (`:549`, `:1027`). Let the callers pass `reason` and keep the default only in the helper. The helper also defaults `actor="SYSTEM"`, which no caller relies on (Speculative Generality). Its docstring says "completed business day", but it never checks that; it depends on `stamp_business_day_schedules_async` raising `ValueError`. ##### P3-4. Dead alias and dead guard in `classify_holiday_async`. CONFIRMED `occupancy_service.py:1013` and `:1031`: - `old_holiday = target` is a pure alias. - `if old_holiday is not None` is always true, because `target` is checked at `:1008`. - `.get("is_holiday", True)` invents a default for a column that `SELECT *` always returns. If it were ever missing, the "rollback" would set the day to holiday. Use `bool(target["is_holiday"])`. ##### P3-5. Rollback audit noise and actor attribution. CONFIRMED The compensation writes extra `occupancy_holiday_audit` rows: - CREATE then DELETE, or UPDATE then UPDATE, when the exception is added. - CLASSIFY then CLASSIFY, when it is classified. All of them carry the admin's actor and the hardcoded English reason "Rollback after stamp failure". The audit then shows a user action that never logically happened. This goes away with the single transaction from P2-1. ##### P3-6. Test gaps and nits. CONFIRMED - **No audit assertions:** the rollback tests do not check the audit tables, so P3-5 is unpinned. They also inject only `RuntimeError`, never `CancelledError` (P2-1). - **Unreachable setup in Case B:** `test_add_schedule_exception_stamp_failure_rolls_back_exception` deletes the business-day record "if one was stamped". The preceding call is `occupancy_repo.add_holiday_async`, which never stamps, so the setup cannot fire. Drop it, or assert the record is absent. - **Fragile magic number:** `test_missed_freeze...` asserts `1500 not in ingress`. That is a value match; asserting on the cycle date or the count is less fragile. - **Duplicate isolation fixture (not from this PR):** the module's own autouse `isolated_event_db` duplicates conftest's `isolated_repository_db`. The new tests do write shared tables but get isolation from it, so this is fine. Optionally switch to `pytestmark = pytest.mark.usefixtures("isolated_repository_db")`. ##### P3-7. The PR body has the wrong function name. CONFIRMED The body says `reconcile_missing_nocturnal_calibrations_async`. The function is `reconcile_historical_calibrations_async`. #### Note: master compatibility - The PR is based on 9a72ff4. Master 9a608c1 merges cleanly (merge-tree 6b6da3a). - Master's new conftest `reset_occupancy_state_in_tests` fixture and `tests/test_holiday_state_reset.py` coexist with the new tests: 25 passed on the merged tree. - No semantic clash found. ### Spec axis Spec: #244, #245, #246 (follow-ups from PR #178). Merge base 9a72ff4, master 9a608c1. Probes: git-archive exports `pr267/`, `base267/` (merge base + PR tests), `merged267/` (merge-tree of master + PR); probe file `pr267/tests/test_probe267.py`. Targeted runs: `tests/test_holiday_event_context.py` + `tests/test_calibration_reconciliation.py` pass 26/26 on the PR and on the merged tree. On the merge base, the new tests fail as expected: the migration-version assert, the learning-reader assert for #246, and the orphan-exception assert for #245. So the reproductions are real. #### Acceptance criteria | Issue | Criterion | Status | |---|---|---| | #244 | Own migration version, idempotent, runs on DB that already has `20261002_holiday_backfill` | MET (probe: deleting the 20261003 key and re-running init_db 3x leaves 1 record + 1 STAMP audit row) | | #244 | Rows built with the same helpers as `stamp_business_day_schedules_async`, no duplicated literals | PARTIAL: see P2-2 | | #244 | Test: DB with old key recorded + unstamped past holiday gets record + STAMP audit on startup | MET (modified `test_migration_stamps_never_stamped_past_holiday_all_readers_agree`) | | #245 | One transaction, or stamp failure rolls back the exception | PARTIAL: compensating writes, not a rollback; see P2-3 | | #245 | One helper used by both call sites | MET (`_ensure_completed_day_stamped`) | | #245 | Test with injected stamp failure: no orphan exception, no missing record | MET (add + update + classify cases) | | #246 | Reproduce missed freeze + backfill in a test | MET (fails on merge base, passes on PR) | | #246 | All readers agree | MET via SQL fallback in both repo readers; but the extra reconcile auto-stamp oversteps, see P2-1 | #### Findings ##### P2-1: Reconciliation now auto-stamps every reconciled past day, not just holidays, which goes against CONTEXT.md. CONFIRMED `app/services/occupancy_service.py:2544-2548` (inside `reconcile_historical_calibrations_async`; the PR body calls it `reconcile_missing_nocturnal_calibrations_async`, which does not exist). - **What happens:** for every completed cycle that has events and no calibration log, the daemon writes a permanent STAMPED business-day record. The record is built from the *current* weekly plan and exceptions. This applies to ordinary days too, not only holidays. - **Probe:** an ordinary Wednesday 2026-09-09 with events and no record. Run reconcile, which also runs at startup via `reconcile_and_quarantine_historical_anomalies_async`. Result: a record `{source: STAMPED, is_exception: 0, is_holiday: 0, 08:00-21:00}` plus an audit row `STAMP / SYSTEM_DAEMON / "Scheduled nocturnal quiet window calibration (reconciled for 2026-09-09)"`. - **Why it's wrong:** - CONTEXT.md:131 says "A historical day without a record uses the current schedule until an administrator stamps or corrects it." - CONTEXT.md:134 says an imported day's Original Schedule is fixed "when its reviewed draft was approved". - For M1 (historical backfill), the first startup after events are imported would stamp every imported day from today's plan. That becomes the day's Original Schedule, because the first audit row wins in `get_original_business_day_schedule_async`. This happens before any draft review, so the reviewed-draft path cannot set the Original Schedule any more. Calibration then follows a schedule nobody approved. - **Not needed for #246:** the repo SQL fallback (`occupancy_repository.py:2957-2963`, `3280-3286`) already makes all readers agree without writing anything. The new test passes without reconcile involvement; it never exercises this path. - **Fix:** drop the reconcile stamp, or limit it to `is_holiday` exception days and get maintainer sign-off on the CONTEXT change. Also use an honest audit reason. The current reason is a calibration string on a schedule stamp. ##### P2-2: The migration still re-implements the stamping path and has already drifted from it. CONFIRMED `app/db/database.py:600-667`. - **What it duplicates:** the migration hand-builds the weekly/exception `ScheduleCalendar` loader, copying `get_schedule_calendar_async`. It also hand-builds the `planned -> BusinessDaySchedule(source="STAMPED", recorded=True)` block, copying `occupancy_service.py:1400-1414`. #244 asked for "the same helpers … no duplicated literals". Only the leaf helpers (`configured_*`, `asdict`, `ScheduleCalendar`) are shared. - **Drift already present:** - The `exception_names` default is `str(h["name"] or "")` (database.py:645). The service uses `name or day.isoformat()` (occupancy_service.py:1286). - Probe: an unnamed past holiday 2026-08-20 gets `exception_name=''` from the migration, but `'2026-08-20'` from `stamp_business_day_schedules_async`. Same data, two different records depending on which path stamped it. This is exactly the drift #244 warned about. - The weekly fallback also differs. The migration uses `row or DEFAULT` and the service uses `row.get(key, DEFAULT)`. - **Fix:** extract the two pieces as pure functions in `app/facility_time.py` and call them from both sides: - `schedule_calendar_from_rows(holidays, weekly, cfg)` - `stamped_from_planned(planned)` (or `replace(planned, source="STAMPED", recorded=True)`) ##### P2-3: The #245 "rollback" is a compensating write in a second transaction; a double fault leaves the orphan and hides the cause. CONFIRMED `app/services/occupancy_service.py:543-561` (add) and `1028-1037` (classify). - **What happens:** the exception is committed by `add_holiday_async`. On stamp failure, a *separate* `delete_holiday_async` or `add_holiday_async` undoes it. - **Probe:** stamp raises `RuntimeError("stamp boom")`, then the compensating delete raises `RuntimeError("database is locked")`. Result: - The caller sees `database is locked`; the stamp error survives only as `__context__`. - The holiday row stays (orphan = True) with no business-day record. - That is the exact split #245 was filed to prevent. - **Real-world trigger:** `BEGIN IMMEDIATE` contention, which is the kind of failure that hits both writes. - **Other effects, even on success:** - The holiday audit permanently gets CREATE + DELETE ("Rollback after stamp failure") rows. A re-created exception gets a new id. - Readers can see the exception during the window between the two commits. - **Fix:** the spec's first option. Do the exception upsert and the stamp in one repo transaction, for example a repo method that takes the holiday dict plus the planned `BusinessDaySchedule` and runs both under one `BEGIN IMMEDIATE`. If compensation is kept, re-raise the original error (`raise orig from comp_err`) and log the compensation failure. #### Merge-with-master note (separate from the PR's own correctness) ##### MC-1: The merged tree fails ruff with F821 undefined `DEFAULT_WEEKDAY_HOURS`. CONFIRMED - **Cause:** master (1885b27 / #233 line) removed `DEFAULT_WEEKDAY_HOURS` from `app/db/database.py` imports. The PR's migration uses it at database.py:605-606. - **Effect:** - `git merge-tree` reports no textual conflict, but `ruff check app/db/database.py` on the merged tree reports F821 at 605:59 and 606:59. CI and pre-commit will fail after merge. - At runtime the name is evaluated only when a weekly row has an empty `open_time`/`close_time`, which is rare because the columns are NOT NULL. So startup normally survives, which is why tests pass. - **Fix:** rebase on master and re-import the name. Better, P2-2's shared loader removes the need. #### P3 - **P3-1: Reconcile crashes with an explicit `now_epoch` later than wall-clock.** CONFIRMED - Where: occupancy_service.py:2544. Reconcile picks cycles from `now_epoch`, but `stamp_business_day_schedules_async` checks against real `active_business_day()`. - Probe: events on 2026-11-10 with `now_epoch` = 2026-11-12 raise `ValueError: Only completed business days can be stamped` and abort the whole reconcile. - Production passes `None`, so the impact is tests and any future caller. Moot if P2-1 removes the stamp. - **P3-2: `classify_holiday_async` rollback has dead and odd code.** occupancy_service.py:1016, 1030-1032. - `old_holiday = target` is always non-None, so the `if old_holiday is not None` guard is dead. - `.get("is_holiday", True)` defaults the restore to True. On a missing key that would leave the day marked holiday and unstamped. - Use `bool(target["is_holiday"])`. - **P3-3: The add-path guard `if result and "id" in result and result["id"] is not None` silently skips the rollback.** occupancy_service.py:552-556. If the id is missing, the orphan survives with no log. At minimum, log it. - **P3-4: The migration's past-day cutoff uses `configured_reset_time` (current config) instead of the effective-dated `ResetSchedule` the service uses.** database.py:610-613. This is harmless for a one-day cutoff, but it is another divergence from "the stamping path"; worth a comment or the shared helper. - **P3-5: `_ensure_completed_day_stamped` costs a full `get_schedule_calendar_async()` per day, through `stamp_business_day_schedules_async`.** That loads all holidays and all records. In reconcile it runs once per reconciled cycle, which is O(n²) over a long M1 backfill. Moot if P2-1 drops it. - **P3-6: Test gaps.** - Nothing tests the reconcile stamping path, and none of the new tests calls reconcile. - The #246 test's readers 1-3 (calendar, business-day fallback, calibration fallback) already passed on master. Only readers 4-5 cover the fix. - Nothing tests the double-fault in P2-3. - Nothing compares migration-stamped rows with service-stamped rows, which would have caught P2-2. - **P3-7: PR body inaccuracies.** It names `reconcile_missing_nocturnal_calibrations_async` (the real name is `reconcile_historical_calibrations_async`). It says the migration builds rows "without hard-coded literals", which P2-2 shows is not fully true. #### Verdict The core #246 SQL fallback and the #244 version split are correct and reproduce/fix the reported defects. Changes requested: - **P2-1:** the reconcile auto-stamp oversteps the domain rule and M1's reviewed-draft Original Schedule. Remove it. - **P2-2:** the migration has not reused the stamping path and already disagrees on `exception_name`. - **P2-3:** the #245 rollback is non-atomic and masks errors. - **MC-1:** rebase to fix the F821 against master.
Merge branch 'master' into fix/holiday-stamping-followups
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m18s
Details
72128f8514 
Address review pass 1 findings for #244, #245, #246:
- Remove reconcile auto-stamp to respect approved draft schedules (CONTEXT.md:131/:134)
- #244: Reuse pure helpers schedule_calendar_from_rows and stamped_from_planned across migration and runtime
- #245: Atomic BEGIN IMMEDIATE transaction in repo for holiday exception and schedule stamp
- #246/P3-1: Hoist _HOLIDAY_CYCLE_DATES_SQL in repository queries
- Test CancelledError and lock contention handling
gabogg commented 2026-10-03 20:17:03 +00:00
Author
Owner
Copy link

Pass 1 fixes (72128f8)

All findings from review pass 1 (r57) have been resolved. origin/master (9a608c1) is merged into the branch, and all 609 tests pass.

Finding Severity Description Fix
P2-1 P2 (Spec & Standards) Reconcile auto-stamped reconciled past days (occupancy_service.py:2544-2548), breaking CONTEXT.md:131 and :134 for M1 draft review. Removed _ensure_completed_day_stamped auto-stamp from reconcile_historical_calibrations_async. The hoisted repository SQL fallback _HOLIDAY_CYCLE_DATES_SQL aligns all readers on unfrozen holidays without mutating draft schedules.
P2-2 P2 (Spec & Standards) Migration (database.py:600-690) duplicated calendar loader, STAMPED record construction, and insert SQL with drift (exception_name empty string vs date). Extracted pure helpers schedule_calendar_from_rows and stamped_from_planned in app/facility_time.py. Routed migration 20261003_holiday_stamping_backfill through shared helpers, repository _BUSINESS_DAY_SCHEDULE_UPSERT_SQL, and parameters. Unnamed exceptions map to exception_name = None (NULL in DB), matching #222.
P2-3 P2 (Spec & Standards) The #245 rollback was a compensating write in a separate transaction, prone to CancelledError, lock contention error masking, and audit noise. Replaced two-step compensating rollback with a single atomic BEGIN IMMEDIATE repository transaction in add_holiday_async and classify_holiday_async that writes the exception, the STAMPED schedule record (when completed), and audits together.
MC-1 Merge clash Merged tree referenced DEFAULT_WEEKDAY_HOURS removed from database.py imports in master (F821). Merged origin/master (9a608c1). Shared schedule_calendar_from_rows in facility_time.py resolves weekly schedules from DEFAULT_DAILY_SCHEDULE, removing the unneeded import.
P3-1 P3 (Spec) Reconcile crashed when now_epoch was later than wall-clock. Mooted and resolved by removing the auto-stamp from reconcile.
P3-2 P3 (Spec) classify_holiday_async rollback had dead and odd code (old_holiday = target guard dead; .get("is_holiday", True)). Compensating rollback eliminated; classification and stamping execute in a single atomic repository transaction.
P3-3 P3 (Spec) Add-path guard if result and "id" in result and result["id"] is not None silently skipped rollback. Compensating rollback eliminated; exception and stamping execute in a single atomic repository transaction.
P3-4 P3 (Standards) Migration past-day cutoff used configured_reset_time instead of effective-dated ResetSchedule. Added comment documenting that configured_reset_time cutoff is safe and standard for the one-day active boundary.
P3-5 P3 (Standards) _ensure_completed_day_stamped loaded entire calendar per day (O(n²) in backfill). Removed _ensure_completed_day_stamped entirely.
P3-6 P3 (Spec & Standards) Test gaps: no test for double-fault in P2-3, no test comparing migration-stamped rows with service-stamped rows. Added test_add_schedule_exception_cancelled_error_and_lock_failure, test_classify_holiday_stamp_failure_rolls_back_classification, and test_migration_stamped_rows_match_service_stamped_rows in tests/test_holiday_event_context.py.
P3-7 P3 (Standards) PR body named reconcile_missing_nocturnal_calibrations_async and claimed no hardcoded literals. Corrected PR body to name reconcile_historical_calibrations_async and accurately describe helper reuse and shared upsert SQL.

Ready for review pass 2.

## Pass 1 fixes (72128f8) All findings from review pass 1 (r57) have been resolved. `origin/master` (9a608c1) is merged into the branch, and all 609 tests pass. | Finding | Severity | Description | Fix | |---|---|---|---| | P2-1 | P2 (Spec & Standards) | Reconcile auto-stamped reconciled past days (`occupancy_service.py:2544-2548`), breaking CONTEXT.md:131 and :134 for M1 draft review. | Removed `_ensure_completed_day_stamped` auto-stamp from `reconcile_historical_calibrations_async`. The hoisted repository SQL fallback `_HOLIDAY_CYCLE_DATES_SQL` aligns all readers on unfrozen holidays without mutating draft schedules. | | P2-2 | P2 (Spec & Standards) | Migration (`database.py:600-690`) duplicated calendar loader, STAMPED record construction, and insert SQL with drift (`exception_name` empty string vs date). | Extracted pure helpers `schedule_calendar_from_rows` and `stamped_from_planned` in `app/facility_time.py`. Routed migration `20261003_holiday_stamping_backfill` through shared helpers, repository `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL`, and parameters. Unnamed exceptions map to `exception_name = None` (NULL in DB), matching #222. | | P2-3 | P2 (Spec & Standards) | The #245 rollback was a compensating write in a separate transaction, prone to `CancelledError`, lock contention error masking, and audit noise. | Replaced two-step compensating rollback with a single atomic `BEGIN IMMEDIATE` repository transaction in `add_holiday_async` and `classify_holiday_async` that writes the exception, the STAMPED schedule record (when completed), and audits together. | | MC-1 | Merge clash | Merged tree referenced `DEFAULT_WEEKDAY_HOURS` removed from `database.py` imports in master (F821). | Merged `origin/master` (9a608c1). Shared `schedule_calendar_from_rows` in `facility_time.py` resolves weekly schedules from `DEFAULT_DAILY_SCHEDULE`, removing the unneeded import. | | P3-1 | P3 (Spec) | Reconcile crashed when `now_epoch` was later than wall-clock. | Mooted and resolved by removing the auto-stamp from reconcile. | | P3-2 | P3 (Spec) | `classify_holiday_async` rollback had dead and odd code (`old_holiday = target` guard dead; `.get("is_holiday", True)`). | Compensating rollback eliminated; classification and stamping execute in a single atomic repository transaction. | | P3-3 | P3 (Spec) | Add-path guard `if result and "id" in result and result["id"] is not None` silently skipped rollback. | Compensating rollback eliminated; exception and stamping execute in a single atomic repository transaction. | | P3-4 | P3 (Standards) | Migration past-day cutoff used `configured_reset_time` instead of effective-dated `ResetSchedule`. | Added comment documenting that `configured_reset_time` cutoff is safe and standard for the one-day active boundary. | | P3-5 | P3 (Standards) | `_ensure_completed_day_stamped` loaded entire calendar per day (O(n²) in backfill). | Removed `_ensure_completed_day_stamped` entirely. | | P3-6 | P3 (Spec & Standards) | Test gaps: no test for double-fault in P2-3, no test comparing migration-stamped rows with service-stamped rows. | Added `test_add_schedule_exception_cancelled_error_and_lock_failure`, `test_classify_holiday_stamp_failure_rolls_back_classification`, and `test_migration_stamped_rows_match_service_stamped_rows` in `tests/test_holiday_event_context.py`. | | P3-7 | P3 (Standards) | PR body named `reconcile_missing_nocturnal_calibrations_async` and claimed no hardcoded literals. | Corrected PR body to name `reconcile_historical_calibrations_async` and accurately describe helper reuse and shared upsert SQL. | Ready for review pass 2.
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m18s
Details
This pull request has changes conflicting with the target branch.
  • app/services/occupancy_service.py
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/holiday-stamping-followups:fix/holiday-stamping-followups
git switch fix/holiday-stamping-followups

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff fix/holiday-stamping-followups
git switch fix/holiday-stamping-followups
git rebase master
git switch master
git merge --ff-only fix/holiday-stamping-followups
git switch fix/holiday-stamping-followups
git rebase master
git switch master
git merge --no-ff fix/holiday-stamping-followups
git switch master
git merge --squash fix/holiday-stamping-followups
git switch master
git merge --ff-only fix/holiday-stamping-followups
git switch master
git merge fix/holiday-stamping-followups
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Waits on the issues listed in its "Blocked by:" line before implementation

  
bug
Category: something is broken; behaviour differs from the spec or docs

  
enhancement
Category: new feature or improvement, including refactors, chores and docs

  
high-priority
Address before ordinary-priority work; independent of triage readiness and blockers

  
low-priority
Address after ordinary-priority work; independent of triage readiness and blockers

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
referenced
Previously closed issue reopened or referenced by an open issue; visible history marker

  
research
Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  
wontfix
Will not be actioned

No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
Milestone
Clear milestone
No items
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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
gabogg/hikcentral!267
No description provided.