Add opt-in SQLite price-history layer (historyDb)

[C2Tech-sys]

What

An opt-in price-history sink. Set historyDb: "price_history.db" in config.yaml and every run appends what it saw to a local SQLite file: cabin fare and every add-on / watchlist item, with paid vs. current price, currency, per-night flag + nights, the rebook decision, and whether a notification fired. Runs are tracked too (start/finish/status, including error when a run dies mid-way), so "cheapest this has been" / trend views become possible on top of the existing alerts.

Zero behavior change when unset

historyDb unset → PriceHistory is a no-op object: no file is created, no SQL runs, every method returns immediately. All 156 existing tests pass unmodified; test_alert_matrix.py in particular is untouched, so the alert/notify decision tree is byte-identical.

Design notes

• stdlib sqlite3 only — no new dependency, nothing added to requirements.txt, no Dockerfile change (class is inline in the script so the Docker image picks it up as-is).
• Each observation commits immediately (WAL mode). A run that crashes on account 2 of 3 keeps everything it recorded before the crash; a runs row with finished_at IS NULL means the process exited without finalizing (e.g. sys.exit from a login failure).
• Hooks are one call per exit path of get_cruise_price() / get_new_order_price() plus run start/finish in main(). The existing alert branches only gain a couple of locals (rebook_decision, notified); no restructuring.
• docker-compose.yml gets a ./data:/app/data volume; docs/config.md documents the option and the (tiny) growth rate.

Tests

test_price_history.py — 14 tests: no-op guarantee (unit + through load_config_objects), schema creation, run lifecycle, round-trips, append-only across runs, hook coverage for price-drop / not-for-sale / no-fare-data / no-longer-for-sale, and the error-finalize path. Added to .github/workflows/pytest.yml.

Independent of #102 (docs only).

🤖 Generated with Claude Code

[jdeath]

I'm going to study this for a bit. I gonna enlist @AESternberg and @tecmage for comments.

[C2Tech-sys]

Thanks for taking a look — no rush at all. A couple of notes that may help the review: it's fully opt-in (historyDb unset = no file, no behavior change; all existing tests pass unmodified), stdlib sqlite3 only, and it commits per observation so a run that dies partway keeps what it saw. I'm running it daily on my own household setup. Happy to adjust anything — schema names, where the hooks sit, splitting it smaller — whatever makes it easier to maintain.

[tecmage]

Did an in-depth audit of this one — hands-on, not just a read-through. Overall: high-quality PR, worth merging once one design gap is closed (fixes submitted as C2Tech-sys#1, which flows into this PR when merged there).

What held up under audit:

• Enumerated every exit path of both pricing functions against the hooks: coverage is complete (one exception below), the recorded prices are the exact post-adjustment values the comparisons used, and nothing mutates paid_price after capture.
• Drove all six cabin alert paths through the real code with a live database attached: every row's status/rebook_decision/notified matched the console behavior exactly, including per-account Apprise routing.
• The no-op guarantee is real — disabled means zero filesystem contact, and the alert decision tree only gains locals (alert-matrix contract tests pass untouched).
• Mutation-tested the new test suite: planted bugs (hook removed, notified never set, finish_run neutered) were all caught. Well-aimed tests.
• SQLite WAL verified working on WSL /mnt/c single-process, and 4 concurrent writers on native Linux fs: clean.

Findings (fixed in the linked PR):

1. The design gap: no exception guard around the database — a historyDb pointing at a missing directory or a corrupt file crashes the script at config load, and a mid-run write failure kills the price run (all three reproduced). The sink should degrade to its no-op form with a warning, never take down the run.
2. Concurrent runs (scheduled task overlapping a manual run) die with database is locked on WSL-mounted drives — busy_timeout is armed after the WAL switch, which itself needs a lock.
3. One unhooked exit: the add-on path's "not available for passenger" bail records nothing — yet that's precisely the back-in-stock waiting state (the Suggestion: improve Watchlist text string #70 use case) a history query wants rows for.
4. SAMPLE-config sets historyDb uncommented — everyone copying the sample gets the opt-in feature switched on.

Minor, noted but not all patched: cabin rows stored reservation_id unstringified while addon rows coerce (fixed in the linked PR); addon promo text is only recorded on the drop path, so best-price rows lose promo info; watchlist cabin rows share item_kind="cabin_fare" with booked cabins (distinguishable by NULL reservation_id); the apprise_test run status is undocumented; and linked reservations produce one row per account per run — honest data, but trend queries should account for it. Also worth a docs sentence: the file contains login emails and real reservation ids, so it shouldn't be shared/committed.

Nice work @C2Tech-sys — the immediate-commit/WAL design and the runs table are exactly right for a crash-prone-network cron script.

[tecmage]

I just got off Icon today and had some time to check while waiting for our flight home. It looks good, just needed a few tweaks. I'll do some more in depth testing tomorrow and the rest of the week when I'm back at my normal computer.

[C2Tech-sys]

Merged your fixes into the PR — thank you, that's a better failure model than I had, and the busy_timeout ordering is a catch I'd never have hit on my filesystem. Appreciate the mutation-testing pass; glad the suite earned its keep. It's now running in production on our household instance against real bookings, so I'll have real-world data on it this week. Happy to fold in anything else your testing turns up.

[jdeath]

Thanks. I'll merge this when everyone is happy.

I will do a release when get #105 non-USD cabin checking working again.

[C2Tech-sys]

Real-world update from our production instance: the history layer has been recording twice-daily runs against live bookings all week without a hiccup. Side benefit of that data: it surfaced the TA-booking pricing bug jdeath just merged in #108 — a cabin that "wasn't for sale" for days turned out to be a countryCode="None" rejection, and it prices correctly now. No issues to report against this PR.

[C2Tech-sys]

Heads up that this PR now gates a follow-up, in case that's useful for prioritising it.

Running multi-account in production surfaced a failure mode worth fixing: login() calls sys.exit(1), and main()'s per-account loop doesn't guard it, so ONE account with a stale password kills the entire run. In my case the good account happened to be first, so it got priced before the bad one aborted things — had the order been reversed I'd have lost the whole check, silently, with no prices and no alerts.

I have that fixed and running here: the failing account is skipped (naming whether it was the login or the post-login profile fetch that failed — thanks to the error body now logged by #109), the run continues through the remaining accounts, and it exits with a distinct code 2 meaning "completed, but N accounts were skipped". The distinct code matters: the successful accounts' prices are already committed and their price-drop alerts already sent, so a scheduler that treats it like a fatal 1 and retries would re-alert those users.

The reason it isn't a PR yet is that it records the partial outcome through this PR's history hook (finish_run("partial_failure", ...)), so it only makes sense on top of #103. Happy to send it the moment this lands — and equally happy to rework it to stand alone if you'd rather it not depend on the history layer.

[jdeath]

It probably wait to be together. I really want to get #105 solved first before adding this, just to avoid any conflict.

[C2Tech-sys]

Heads up before you touch the conflict: #105 landing put this PR into CONFLICTING (3 hunks in CheckRoyalCaribbeanPrice.py — the history schema/class vs the new CheckinPaymentTracker, the record_addon hook vs the new return watch_tracker_record, and start_run() vs the per-run tracker setup; the test file merges clean). I'm merging main into this branch now, keeping both sides and re-attaching every history hook to #105's restructured summary code, with the full suite green — should show MERGEABLE again shortly. No need to resolve it on your end.

[C2Tech-sys]

Brought up to date with main (7d2275b, post-#105) as a single merge commit, bd783e8. Resolution: kept both the PriceHistory layer and the new CheckinPaymentTracker; record_addon stays immediately before the new return watch_tracker_record; start_run() sits alongside the per-run tracker setup, and the two .clear() calls on the lists #105 removed are gone. All 14 history hooks preserved (verified against the previous head), the PriceHistory class is byte-identical to what tecmage reviewed, nothing from #105 altered. Suite: 205 passed. Should read MERGEABLE now.

[jdeath]

thanks