diff --git a/.claude/skills/deploy-massprice/SKILL.md b/.claude/skills/deploy-massprice/SKILL.md index fb407c0..f7c79c5 100644 --- a/.claude/skills/deploy-massprice/SKILL.md +++ b/.claude/skills/deploy-massprice/SKILL.md @@ -69,9 +69,11 @@ cd /mnt/user/appdata/massprice && docker compose restart `auth_state.json` (the saved MassPrice session imported via the Session page — a live bearer credential for the user's MassPrice account while valid, see `CLAUDE.md`; losing this just costs one re-import, not data), -`run_history.json`, `rules_cache.json`, and `notify_state.json` (the -session-expiry alert's send-once dedupe flag). Both are excluded from the -deploy rsync (which runs `--delete`) and from git. +`run_history.json`, `rules_cache.json`, `session_state.json` (the +"session is known-bad" flag behind the dashboard banner), and +`notify_state.json` (the session-expiry alert's send-once dedupe flag). +Both are excluded from the deploy rsync (which runs `--delete`) and from +git. ## If scheduled runs stop working @@ -84,9 +86,15 @@ Check, in order: it recurs (e.g. a MassPrice redesign changes the modal/selectors), this is the first thing to suspect — see `CLAUDE.md`. 1. **Dashboard banner / Activity page** — a "session expired or missing" - message means go log in again via the Session page. "rule id not - found" means a rule was renamed/deleted on MassPrice — click "Refresh - Rules". + message means go log in again via the Session page. "Rule id … not + found on /Rule/List" means that rule was **deleted** on MassPrice + (a rename can't cause it — schedules are keyed by the stable rule + id): click "Refresh Rules", which re-syncs `schedules.json` against + the live list, drops schedules for deleted rules, and updates cached + names for renamed ones. Renames matter because run verification + matches the Activity page by name — a stale one shows every run as + "Not Confirmed" while it's actually succeeding, so refresh after + renaming anything. 2. **Is the container still running** — `docker ps --filter name=massprice` — a Chromium crash inside the container (rare, but possible if `shm_size` ever gets dropped from `docker-compose.yml`) diff --git a/.env.example b/.env.example index c125470..419842d 100644 --- a/.env.example +++ b/.env.example @@ -29,6 +29,14 @@ TZ=America/Phoenix VERIFY_POLL_MINUTES=5 VERIFY_MAX_ATTEMPTS=12 +# ── Request patience ───────────────────────────────────────────────── +# How long "Refresh Rules" / "Import Session" wait for the single +# browser worker before answering "still working, reload in a moment" +# (the work still finishes in the background). Keep it comfortably under +# gunicorn's 120s timeout — a killed worker takes the scheduler and all +# pending verification polls with it. Rarely worth changing. +REQUEST_WAIT_SECONDS=90 + # ── Session-expiry email alert (optional) ─────────────────────────── # Sent once when the saved MassPrice session expires/is missing — not # on every failed run, just the first one after it goes bad. Same diff --git a/CLAUDE.md b/CLAUDE.md index f71f80e..50870dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,11 +30,25 @@ tool is confirmed working, so there's exactly one thing triggering runs. - **`scheduler.py`** — an in-process APScheduler `BackgroundScheduler` holding one `CronTrigger` job per `(rule, schedule entry)` pair, rebuilt from `schedules.json` on startup and whenever the webui saves - a change. Triggered jobs (and manual "Run Now" clicks) are pushed onto - one `queue.Queue`, drained by a single dedicated worker thread — this - guarantees only one Playwright browser session is ever active at a - time, matching how a human would actually operate the site and - avoiding concurrent-session weirdness. + a change. **Every** Playwright session goes through `submit()` onto + one `queue.Queue`, drained by a single dedicated worker thread — + scheduled runs, "Run Now", "Refresh Rules" and cookie imports alike. + That guarantees only one browser is ever active at a time, matching + how a human would actually operate the site and avoiding + concurrent-session weirdness. Nothing may call `automation.py` + directly from a Flask request thread: with `threads = 4` that opens a + second Chromium alongside a scheduled run, and both write + `auth_state.json` — which Playwright writes itself, non-atomically — + so a torn write reads back as "session expired" and costs a pointless + cookie re-import. + - Request handlers that need an answer (refresh, import) use + `run_browser_task()`, which waits `REQUEST_WAIT_SECONDS` (90) and + then raises `BrowserBusy` → HTTP 503, *without* cancelling the work. + The bound is the point: gunicorn kills any worker whose request + exceeds its `timeout` (120s), and since the jobstore and queue live + in that worker's memory, being killed drops every pending + verification poll. Waiting behind a slow rule run must never be able + to take the scheduler down. - **Must run in exactly one process.** `gunicorn.conf.py` pins `workers = 1` for this reason — a second worker would instantiate a second scheduler and duplicate every run. Fine for a low-traffic @@ -58,7 +72,15 @@ tool is confirmed working, so there's exactly one thing triggering runs. - **`automation.py`** — all Playwright logic. `list_rules()` scrapes `https://massprice.tcgplayer.com/Rule/List` for the live rule id/name pairs (cached to `rules_cache.json`, refreshed via the - Dashboard's "Refresh Rules" button or on first boot). `run_rule(id)` + Dashboard's "Refresh Rules" button or on first boot). A refresh also + runs `_reconcile_schedules()` in `app.py`, which re-syncs + `schedules.json` against that live list: rule **ids** are stable but + names aren't, and a stale cached name silently breaks run + verification (it's what the Activity-page match keys on), while a + rule deleted upstream would otherwise keep firing cron jobs with no + card left in the UI to disable them. Deliberately skipped when the + scrape returns an empty list, so a MassPrice redesign that breaks + the scrape can't delete every saved schedule. `run_rule(id)` finds that rule's `li.rule.widget-rule` row, clicks its `.dropdown-ellipsis` three-dot toggle, clicks the row's `a.run-rule[data-rule-id=...]` item — mirroring exactly what a human @@ -118,10 +140,16 @@ tool is confirmed working, so there's exactly one thing triggering runs. against), the run fails with a clear "session expired or missing" error rather than attempting to log in — see below for why. - **`storage.py`** — tiny JSON read/write helpers (temp-file + - `os.replace`, so a crash mid-write can't corrupt state) for - everything under `/data`: `schedules.json`, `auth_state.json`, - `run_history.json` (bounded to the last 200 entries), `rules_cache.json`, + `os.replace`, so a crash mid-write can't corrupt state) for the state + files under `/data`: `schedules.json`, `run_history.json` (bounded to + the last 200 entries), `rules_cache.json`, `session_state.json` (the + "is the saved session known-bad" flag behind the dashboard banner) and `notify_state.json` (the session-expiry alert's send-once dedupe flag). + `auth_state.json` lives in the same directory but is **not** written + through here — Playwright writes it itself via + `context.storage_state(path=...)` in `automation._save_state()`, so it + gets none of that atomic-write protection. Worth knowing: it's also + the one file two concurrent Playwright sessions can both write. - **`templates/` / `static/`** — a small, plain dark dashboard. This is a deliberately different, simpler visual identity than Arr Summary's teletext system — that design language is specific to that app, not a @@ -175,9 +203,16 @@ automated run re-saves it (`_save_state()`), which tends to slide a expire (inactivity, a password change, TCGplayer forcing re-auth) and need re-importing. -The Dashboard shows a red banner (`_session_warning()` in `app.py`) when -the most recent run/refresh failed specifically because of a missing/ -expired session, so this doesn't just look like "no rules found." +The Dashboard shows a red banner (`_session_warning()` in `app.py`) +whenever the saved session is known to be missing/expired, so this +doesn't just look like "no rules found." The flag behind it lives in +`session_state.json` and is set by `automation._ensure_authenticated()` +on *any* failed page load — a scheduled run, a "Refresh Rules", or a +verification poll — and cleared the moment one authenticates again (or a +fresh export is imported). It's deliberately not inferred from +`run_history.json`: `list_rules()` writes no history entry, so a failed +refresh — often the first symptom the user sees — would never raise the +banner. ## Session-expiry email alert @@ -210,9 +245,10 @@ python3 app.py Runs on port 5000 (`PORT` env to change). Without Chromium installed and without a saved `auth_state.json`, the Dashboard still boots — -`_bootstrap()` in `app.py` best-effort-catches the initial rule sync, so -local UI/schedule-editing work doesn't require Docker or a real MassPrice -session at all. `DATA_DIR` env var overrides where JSON state is +`_bootstrap()` in `app.py` queues the initial rule sync instead of +running it inline, so the failure lands in the worker thread's log and +local UI/schedule-editing work doesn't require Docker or a real +MassPrice session at all. `DATA_DIR` env var overrides where JSON state is read/written (defaults to `/data`, which won't exist locally — point it at a scratch directory). diff --git a/app.py b/app.py index 55232b6..be0b91f 100644 --- a/app.py +++ b/app.py @@ -1,5 +1,6 @@ import hmac import os +import secrets from datetime import datetime, timedelta, timezone from functools import wraps @@ -9,13 +10,20 @@ import automation import scheduler import storage -APP_VERSION = "0.8.0" +APP_VERSION = "0.10.0" APP_VERSION_DATE = "2026-09-01" app = Flask(__name__) -app.secret_key = os.environ.get("SECRET_KEY", "dev-secret-change-me") +# `or default`, not `get(name, default)`: docker-compose.yml always *sets* +# these vars, so a missing/blank value in .env arrives as an empty string +# and the two-arg default never applies. An empty secret_key makes Flask +# raise on every session write, and an empty LOGIN_PASSWORD would make +# compare_digest("", "") return True — i.e. no login at all. +app.secret_key = os.environ.get("SECRET_KEY") or "dev-secret-change-me" +app.config["SESSION_COOKIE_HTTPONLY"] = True +app.config["SESSION_COOKIE_SAMESITE"] = "Lax" -LOGIN_PASSWORD = os.environ.get("LOGIN_PASSWORD", "changeme") +LOGIN_PASSWORD = os.environ.get("LOGIN_PASSWORD") or "changeme" DAYS = ["mon", "tue", "wed", "thu", "fri", "sat", "sun"] DAY_LABELS = {"mon": "Mon", "tue": "Tue", "wed": "Wed", "thu": "Thu", "fri": "Fri", "sat": "Sat", "sun": "Sun"} @@ -25,15 +33,65 @@ def login_required(view): @wraps(view) def wrapped(*args, **kwargs): if not session.get("authenticated"): + # The JSON API answers 401 instead of redirecting: fetch() + # follows a 302 to /login, gets 200 HTML back, and the caller + # reads res.ok as success — "Run Now" would report "Queued" + # for a run that was never queued. + if request.path.startswith("/api/"): + return jsonify({"error": "Signed out — reload the page to log in again."}), 401 return redirect(url_for("login", next=request.path)) return view(*args, **kwargs) return wrapped +def _csrf_token(): + token = session.get("csrf_token") + if not token: + token = secrets.token_urlsafe(32) + session["csrf_token"] = token + return token + + +def csrf_protect(view): + """Header-based CSRF check for the JSON API. The token is minted per + browser session, handed to the page in a tag, and echoed back + by app.js in X-CSRF-Token — a cross-origin page can read neither the + meta tag nor the session cookie's contents, so it can't forge the + header, and a plain cross-site
POST can't set one at all.""" + + @wraps(view) + def wrapped(*args, **kwargs): + sent = request.headers.get("X-CSRF-Token", "") + expected = session.get("csrf_token", "") + # Compared as bytes: `sent` is attacker-controlled, and + # compare_digest raises TypeError on non-ASCII str input. + if not expected or not hmac.compare_digest(sent.encode(), expected.encode()): + return jsonify({"error": "Invalid or missing CSRF token — reload the page."}), 403 + return view(*args, **kwargs) + + return wrapped + + @app.context_processor def inject_version(): - return {"app_version": APP_VERSION, "app_version_date": APP_VERSION_DATE} + return { + "app_version": APP_VERSION, + "app_version_date": APP_VERSION_DATE, + "csrf_token": _csrf_token(), + } + + +def _safe_next(target): + """Only ever redirect to a path on this app. Without this, a crafted + /login?next=https://evil.com turns the login form into an open + redirect. `//host` and `/\\host` are protocol-relative to a browser, + so they're rejected too.""" + if not target or not target.startswith("/"): + return None + if target.startswith("//") or target.startswith("/\\"): + return None + return target @app.route("/login", methods=["GET", "POST"]) @@ -41,9 +99,14 @@ def login(): error = None if request.method == "POST": password = request.form.get("password", "") - if hmac.compare_digest(password, LOGIN_PASSWORD): + # Compared as bytes: compare_digest raises TypeError on + # non-ASCII str, which turned any accented character typed into + # the password box into a 500 instead of "Incorrect password". + # An empty password never authenticates, even if LOGIN_PASSWORD + # somehow ends up empty too. + if password and hmac.compare_digest(password.encode(), LOGIN_PASSWORD.encode()): session["authenticated"] = True - return redirect(request.args.get("next") or url_for("dashboard")) + return redirect(_safe_next(request.args.get("next")) or url_for("dashboard")) error = "Incorrect password" return render_template("login.html", error=error) @@ -83,16 +146,17 @@ def _merged_rules(): def _session_warning(): - """True if the most recent run/refresh failed specifically because - the saved MassPrice session is missing/expired — surfaced as a - dashboard banner so a dead session doesn't just look like "no rules".""" - history = storage.load("run_history.json", []) - if not history: - return False - latest = history[0] - return latest.get("status") == "failure" and "session expired or missing" in ( - latest.get("message") or "" - ) + """True while the saved MassPrice session is known to be missing or + expired — surfaced as a dashboard banner so a dead session doesn't + just look like "no rules". + + Reads the flag that automation._ensure_authenticated() sets on *any* + failed page load (scheduled run, "Refresh Rules", or a verification + poll) and clears as soon as one authenticates again. It deliberately + doesn't infer this from run_history.json: list_rules() never writes + a history entry, so a refresh failure — often the first thing the + user notices — would never have raised the banner.""" + return bool(storage.load("session_state.json", {}).get("expired")) @app.route("/") @@ -117,10 +181,13 @@ def session_page(): @app.route("/api/session/import", methods=["POST"]) @login_required +@csrf_protect def api_session_import(): body = request.get_json(force=True, silent=True) or {} try: - automation.import_cookies(body.get("cookies", "")) + scheduler.import_session(body.get("cookies", "")) + except scheduler.BrowserBusy as exc: + return jsonify({"error": str(exc)}), 503 except automation.AutomationError as exc: return jsonify({"error": str(exc)}), 409 except Exception as exc: @@ -129,10 +196,26 @@ def api_session_import(): def _parse_ts(value): + """Parse a stored ISO timestamp into an aware datetime, or None. + Everything written by _log_history() is UTC-aware; a naive value + (hand-edited or from an older build) is assumed to be UTC so it can + still be compared against an aware cutoff without raising.""" try: - return datetime.fromisoformat(value) + dt = datetime.fromisoformat(value) except (TypeError, ValueError): return None + return dt if dt.tzinfo else dt.replace(tzinfo=timezone.utc) + + +@app.template_filter("localtime") +def localtime(value): + """Render a stored UTC timestamp in the configured display timezone. + Schedules are set in TZ, so showing run times in UTC reads as wrong + even when the run fired at exactly the right moment.""" + dt = _parse_ts(value) + if dt is None: + return value or "—" + return dt.astimezone(scheduler.TZ).strftime("%b %-d, %-I:%M %p %Z") @app.route("/activity") @@ -140,24 +223,96 @@ def _parse_ts(value): def activity(): history = storage.load("run_history.json", []) cutoff = datetime.now(timezone.utc) - timedelta(hours=24) - recent = [e for e in history if (_parse_ts(e.get("timestamp")) or cutoff) >= cutoff] + # An entry whose timestamp can't be parsed can't be shown to be + # recent, so it's excluded rather than silently kept. + recent = [ + e for e in history if (ts := _parse_ts(e.get("timestamp"))) is not None and ts >= cutoff + ] return render_template("activity.html", history=recent) +def _reconcile_schedules(rules): + """Bring schedules.json back in line with the live rule list. + + Rule ids are stable; names are not, and schedules.json caches the + name. Two things went wrong without this: + + - **Renamed rule.** The stale name is what run verification matches + against on MassPrice's Activity page, so every run kept working + while reporting "Not Confirmed" forever — training you to ignore + the one signal that catches a silently-failing run. + - **Deleted rule.** Its card vanishes from the dashboard (those come + from rules_cache.json) while its cron jobs keep firing and failing + from schedules.json, with no UI left to switch them off. + + Skipped entirely on an empty rule list: a scrape that breaks after a + MassPrice redesign shouldn't be able to delete every saved schedule. + """ + if not rules: + return + schedules = storage.load("schedules.json", {}) + if not schedules: + return + + live = {r["id"]: r["name"] for r in rules} + kept = {} + changed = False + for rule_id, entry in schedules.items(): + if rule_id not in live: + app.logger.info("Dropping schedule for rule %s — gone from MassPrice", rule_id) + changed = True + continue + if entry.get("name") != live[rule_id]: + app.logger.info( + "Rule %s renamed %r -> %r, updating stored schedule", + rule_id, + entry.get("name"), + live[rule_id], + ) + entry["name"] = live[rule_id] + changed = True + kept[rule_id] = entry + + if changed: + storage.save("schedules.json", kept) + scheduler.rebuild_jobs() + + @app.route("/api/rules/refresh", methods=["POST"]) @login_required +@csrf_protect def api_refresh_rules(): try: - rules = automation.list_rules() + rules = scheduler.refresh_rules() + except scheduler.BrowserBusy as exc: + return jsonify({"error": str(exc)}), 503 except Exception as exc: return jsonify({"error": str(exc)}), 502 + _reconcile_schedules(rules) return jsonify({"rules": rules}) @app.route("/api/rules//schedule", methods=["POST"]) @login_required +@csrf_protect def api_save_schedule(rule_id): body = request.get_json(force=True, silent=True) or {} + + # Validate before persisting: an entry rebuild_jobs() can't parse + # would otherwise be stored, skipped on every rebuild, and quietly + # never run. + raw_schedules = body.get("schedules", []) + if not isinstance(raw_schedules, list): + return jsonify({"error": "schedules must be a list"}), 400 + bad = next((e for e in raw_schedules if not scheduler.is_valid_entry(e)), None) + if bad is not None: + return jsonify( + { + "error": "Unusable schedule entry — each one needs an HH:MM time and " + f"at least one valid day. Got: {bad!r}" + } + ), 400 + schedules = storage.load("schedules.json", {}) cache = storage.load("rules_cache.json", {"rules": []}) name = next((r["name"] for r in cache.get("rules", []) if r["id"] == rule_id), None) @@ -165,7 +320,7 @@ def api_save_schedule(rule_id): schedules[rule_id] = { "name": name, "enabled": bool(body.get("enabled", True)), - "schedules": body.get("schedules", []), + "schedules": raw_schedules, } storage.save("schedules.json", schedules) scheduler.rebuild_jobs() @@ -174,6 +329,7 @@ def api_save_schedule(rule_id): @app.route("/api/rules//run", methods=["POST"]) @login_required +@csrf_protect def api_run_rule(rule_id): cache = storage.load("rules_cache.json", {"rules": []}) name = next((r["name"] for r in cache.get("rules", []) if r["id"] == rule_id), None) @@ -182,15 +338,14 @@ def api_run_rule(rule_id): def _bootstrap(): - scheduler.start(tz=os.environ.get("TZ")) + scheduler.start() # Best-effort initial rule sync so the dashboard isn't blank before - # the user ever clicks "Refresh Rules". Silently skipped if - # Playwright/credentials aren't available yet (e.g. local dev). + # the user ever clicks "Refresh Rules". Queued rather than run inline + # so boot doesn't block on Chromium, and so it can't race a run; a + # failure (no Playwright or no session yet, e.g. local dev) is logged + # by the worker and otherwise ignored. if storage.load("rules_cache.json", None) is None: - try: - automation.list_rules() - except Exception: - pass + scheduler.submit(automation.list_rules) _bootstrap() diff --git a/automation.py b/automation.py index f726a26..19d63ea 100644 --- a/automation.py +++ b/automation.py @@ -81,6 +81,22 @@ def _is_logged_out(page): return "/Rule" not in page.url +def _set_session_expired(expired): + """Track whether the saved session is currently known-bad, so the + dashboard banner (app.py's _session_warning()) can reflect a failed + "Refresh Rules" too — list_rules() writes no run_history entry, so + inferring this from run history alone missed refresh failures + entirely. Written only on a state change, since the success path + runs on every rule run and every verification poll.""" + state = storage.load("session_state.json", {}) + if bool(state.get("expired")) == bool(expired): + return + payload = {"expired": bool(expired)} + if expired: + payload["at"] = datetime.now(timezone.utc).isoformat() + storage.save("session_state.json", payload) + + def _notify_session_expired_once(): """Send the email alert only on the first failure after the session goes bad, not on every scheduled run that fails while it's down — @@ -107,11 +123,13 @@ def _notify_session_expired_once(): def _ensure_authenticated(page): page.goto(f"{BASE_URL}/Rule/List") if _is_logged_out(page): + _set_session_expired(True) _notify_session_expired_once() raise AutomationError( "MassPrice session expired or missing — log in via the " "Session page and paste in a fresh cookie export." ) + _set_session_expired(False) def session_status(): @@ -173,8 +191,11 @@ def _find_latest_activity(page, rule_name): "new completion appeared" check is done reliably.""" page.goto(f"{BASE_URL}/Activity/List") page.wait_for_timeout(1000) + # json.dumps() quotes and escapes the name the same way CSS does for + # `"` and `\`; interpolating it raw meant a rule named e.g. `6" Box` + # produced an unparseable selector and blew up the whole check. rows = page.locator("div.activity[data-activitylogid]").filter( - has=page.locator(f'.activity__name[title="{rule_name}"]') + has=page.locator(f".activity__name[title={json.dumps(rule_name)}]") ) if rows.count() == 0: return None, None @@ -195,6 +216,10 @@ def run_rule(rule_id, rule_name=None): status = "failure" message = "" watermark_id = None + # Distinct from `watermark_id is None`, which is also the legitimate + # "this rule has no prior Activity entries at all" result. Only a + # successful capture makes the later comparison meaningful. + watermark_captured = False with sync_playwright() as p: browser = p.chromium.launch(headless=True, args=LAUNCH_ARGS) try: @@ -208,10 +233,14 @@ def run_rule(rule_id, rule_name=None): # apart from "the same old entry is still sitting there". # Best-effort: if this fails for any reason, proceed with the # actual run anyway — the run itself matters more than the - # optional follow-up verification. + # optional follow-up verification — but skip verification + # entirely rather than run it without a watermark, which + # would match the *previous* run's Complete entry and report + # a false "verified" for a run that may never have landed. if rule_name: try: watermark_id, _ = _find_latest_activity(page, rule_name) + watermark_captured = True except Exception: logger.exception("Failed to capture activity watermark for %s", rule_name) page.goto(f"{BASE_URL}/Rule/List") @@ -256,9 +285,13 @@ def run_rule(rule_id, rule_name=None): browser.close() entry_id = _log_history(rule_id, rule_name, status, message) - will_verify = status == "sent" and bool(rule_name) + will_verify = status == "sent" and bool(rule_name) and watermark_captured if will_verify: set_verification_pending(entry_id) + elif status == "sent": + logger.warning( + "Run for rule %s sent but not verifiable (no activity watermark)", rule_id + ) return { "status": status, @@ -273,7 +306,12 @@ def check_completion(rule_name, watermark_id): """One-shot check: has a new "Complete" entry appeared for rule_name past watermark_id yet? Returns ("complete", "") or ("not_yet", msg). Pure check — no storage side effects; the caller (scheduler.py's - poll loop) decides what that means for the stored history entry.""" + poll loop) decides what that means for the stored history entry. + + A watermark_id of None means the rule had no Activity entries at all + when the run was submitted, so any entry that appears is new. That's + only ever reached via a *successful* watermark capture — run_rule() + refuses to start a poll loop when the capture itself failed.""" with sync_playwright() as p: browser = p.chromium.launch(headless=True, args=LAUNCH_ARGS) try: @@ -371,6 +409,14 @@ def _convert_cookie(raw): Playwright's storage_state cookie shape.""" expires = raw.get("expirationDate") same_site_raw = (raw.get("sameSite") or "unspecified").lower() + same_site = _SAME_SITE_MAP.get(same_site_raw, "Lax") + secure = bool(raw.get("secure", False)) + # Chromium rejects a SameSite=None cookie that isn't also Secure, and + # drops it from the imported session without any error — which can + # quietly cost the one cookie that carried the login. Everything here + # is https-only in practice, so forcing the flag is safe. + if same_site == "None": + secure = True return { "name": raw["name"], "value": raw["value"], @@ -378,8 +424,8 @@ def _convert_cookie(raw): "path": raw.get("path", "/"), "expires": expires if expires is not None else -1, "httpOnly": bool(raw.get("httpOnly", False)), - "secure": bool(raw.get("secure", False)), - "sameSite": _SAME_SITE_MAP.get(same_site_raw, "Lax"), + "secure": secure, + "sameSite": same_site, } @@ -433,6 +479,7 @@ def import_cookies(raw_text): "itself, after finishing login, not from the login page." ) _save_state(context) + _set_session_expired(False) storage.save("notify_state.json", {"session_expired_notified": False}) finally: browser.close() diff --git a/docker-compose.yml b/docker-compose.yml index 9941f64..7b189e5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -18,6 +18,7 @@ services: - TZ=${TZ:-America/Phoenix} - VERIFY_POLL_MINUTES=${VERIFY_POLL_MINUTES:-5} - VERIFY_MAX_ATTEMPTS=${VERIFY_MAX_ATTEMPTS:-12} + - REQUEST_WAIT_SECONDS=${REQUEST_WAIT_SECONDS:-90} - GMAIL_USER=${GMAIL_USER:-} - GMAIL_APP_PASSWORD=${GMAIL_APP_PASSWORD:-} - ALERT_EMAIL=${ALERT_EMAIL:-} diff --git a/gunicorn.conf.py b/gunicorn.conf.py index 26f9812..1bddc18 100644 --- a/gunicorn.conf.py +++ b/gunicorn.conf.py @@ -9,5 +9,9 @@ workers = 1 threads = 4 worker_class = "gthread" bind = "0.0.0.0:" + os.environ.get("PORT", "5000") -timeout = 120 # a rule run launches Chromium, logs in, and clicks through +# Killing a worker here kills the APScheduler jobstore and the run queue +# with it (both live in process memory), dropping every pending +# verification poll — so requests that wait on browser work bound their +# own wait well under this, via scheduler.REQUEST_WAIT_SECONDS (90). +timeout = 120 keepalive = 5 diff --git a/requirements.txt b/requirements.txt index b9546a5..5d656cc 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,3 +2,6 @@ flask==3.1.0 gunicorn==23.0.0 playwright==1.47.0 apscheduler==3.10.4 +# Imported directly by scheduler.py. It only ever resolved as an +# apscheduler 3.x transitive dependency, which 4.x drops. +pytz==2024.2 diff --git a/scheduler.py b/scheduler.py index c702bb0..b366ecd 100644 --- a/scheduler.py +++ b/scheduler.py @@ -1,8 +1,22 @@ """In-process cron scheduling for rule runs. -A single background worker thread owns every Playwright run — both -APScheduler-triggered runs and manual "Run Now" clicks are pushed onto -one queue.Queue so exactly one browser session is ever active at a time. +A single background worker thread owns *every* Playwright session in +this process — scheduled runs, manual "Run Now" clicks, "Refresh Rules" +and cookie imports all go through submit() onto one queue.Queue, so +exactly one browser is ever open at a time. Nothing may call into +automation.py directly from a Flask request thread: with gunicorn's +threads=4 that would open a second Chromium alongside a scheduled run, +and both would write /data/auth_state.json (which Playwright writes +itself, non-atomically) — a torn write there reads back as "session +expired" and costs a pointless cookie re-import. + +Request threads that need an answer use run_browser_task(), which waits +on the queued job for REQUEST_WAIT_SECONDS and then gives up *waiting* +without cancelling the work. That bound matters: gunicorn kills a worker +whose request exceeds its `timeout` (120s), and since the APScheduler +jobstore and this queue live in that worker's memory, being killed drops +every pending verification poll on the floor. + This must run in exactly one process; see gunicorn.conf.py's workers=1. The same worker also drives a poll loop that confirms a run actually @@ -18,6 +32,8 @@ import logging import os import queue import threading +from concurrent.futures import Future +from concurrent.futures import TimeoutError as FutureTimeoutError from datetime import datetime, timedelta import pytz @@ -31,7 +47,8 @@ logger = logging.getLogger("massprice.scheduler") _work_queue: "queue.Queue" = queue.Queue() -# Resolved once and reused everywhere a timezone is needed. Must be +# Resolved once and reused everywhere a timezone is needed (app.py also +# imports this to render stored UTC timestamps for display). Must be # passed explicitly at construction time, never left to be inferred — # both BackgroundScheduler AND CronTrigger independently call tzlocal's # get_localzone() in their own __init__ if not given one, which fails @@ -40,9 +57,17 @@ _work_queue: "queue.Queue" = queue.Queue() # CronTrigger built without its own explicit timezone still resolves # get_localzone() itself, regardless of what the scheduler is configured # with, and crashes the same way. Every trigger built below must pass -# timezone=_TZ explicitly. -_TZ = pytz.timezone(os.environ.get("TZ", "UTC")) -_scheduler = BackgroundScheduler(timezone=_TZ) +# timezone=TZ explicitly. +_TZ_NAME = os.environ.get("TZ") or "UTC" +try: + TZ = pytz.timezone(_TZ_NAME) +except pytz.UnknownTimeZoneError: + # A typo in TZ would otherwise raise at import and crash-loop the + # container with a traceback that doesn't obviously point at .env. + logger.error("Unknown TZ %r — falling back to UTC", _TZ_NAME) + TZ = pytz.utc + +_scheduler = BackgroundScheduler(timezone=TZ) _started = False VALID_DAYS = ["mon", "tue", "wed", "thu", "fri", "sat", "sun"] @@ -56,24 +81,58 @@ VALID_DAYS = ["mon", "tue", "wed", "thu", "fri", "sat", "sun"] VERIFY_POLL_MINUTES = int(os.environ.get("VERIFY_POLL_MINUTES", "5")) VERIFY_MAX_ATTEMPTS = int(os.environ.get("VERIFY_MAX_ATTEMPTS", "12")) +# How long a web request will wait for its queued browser work before +# answering "still working". Must stay comfortably under gunicorn's +# `timeout` (120s in gunicorn.conf.py) — the point is to end the request +# ourselves rather than let gunicorn kill the worker and take the +# scheduler and every pending poll down with it. +REQUEST_WAIT_SECONDS = int(os.environ.get("REQUEST_WAIT_SECONDS", "90")) + + +class BrowserBusy(Exception): + """Queued browser work didn't finish inside REQUEST_WAIT_SECONDS. + The work is still queued and will complete — this only means we + stopped waiting on it inside the HTTP request.""" + + +def submit(fn, *args): + """Hand browser work to the single worker thread; returns a Future. + + Every Playwright session in this process goes through here — see + this module's docstring for why calling automation.py from a request + thread is not allowed. + """ + future = Future() + _work_queue.put((fn, args, future)) + return future + + +def run_browser_task(fn, *args): + """submit() + wait for the result, for request handlers that need + one. Re-raises whatever the work raised; raises BrowserBusy if it's + still going when our patience runs out.""" + try: + return submit(fn, *args).result(timeout=REQUEST_WAIT_SECONDS) + except FutureTimeoutError: + raise BrowserBusy( + "Still working on it — a rule run is probably ahead of this in the " + "queue. It'll finish in the background; reload in a moment." + ) + def _worker_loop(): while True: - item = _work_queue.get() - kind = item[0] + fn, args, future = _work_queue.get() + if not future.set_running_or_notify_cancel(): + _work_queue.task_done() + continue try: - if kind == "run": - _, rule_id, rule_name = item - result = automation.run_rule(rule_id, rule_name) - if result.get("will_verify"): - schedule_verification( - result["entry_id"], rule_name, result["watermark_activity_id"] - ) - elif kind == "verify": - _, entry_id, rule_name, watermark_id, attempt = item - _run_verification_check(entry_id, rule_name, watermark_id, attempt) - except Exception: - logger.exception("Unhandled error processing %r", item) + future.set_result(fn(*args)) + except Exception as exc: + # Recorded on the Future for whoever is waiting, and logged + # here too — nothing waits on a scheduled run's Future. + future.set_exception(exc) + logger.exception("Queued %s failed", getattr(fn, "__name__", fn)) finally: _work_queue.task_done() @@ -97,16 +156,37 @@ def _run_verification_check(entry_id, rule_name, watermark_id, attempt): schedule_verification(entry_id, rule_name, watermark_id, attempt=attempt + 1) +def _run_and_verify(rule_id, rule_name): + result = automation.run_rule(rule_id, rule_name) + if result.get("will_verify"): + schedule_verification(result["entry_id"], rule_name, result["watermark_activity_id"]) + return result + + def enqueue_run(rule_id, rule_name=None): - _work_queue.put(("run", rule_id, rule_name)) + """Fire-and-forget — the cron trigger and the "Run Now" button both + land here, and neither waits for the run to finish.""" + return submit(_run_and_verify, rule_id, rule_name) def enqueue_verify(entry_id, rule_name, watermark_id, attempt): - _work_queue.put(("verify", entry_id, rule_name, watermark_id, attempt)) + return submit(_run_verification_check, entry_id, rule_name, watermark_id, attempt) + + +def refresh_rules(): + """Re-scrape the live rule list on the worker thread. Called from a + request handler, so it goes through run_browser_task() rather than + calling automation.list_rules() inline.""" + return run_browser_task(automation.list_rules) + + +def import_session(raw_text): + """Validate and save a pasted cookie export on the worker thread.""" + return run_browser_task(automation.import_cookies, raw_text) def schedule_verification(entry_id, rule_name, watermark_id, attempt=1): - run_date = datetime.now(_TZ) + timedelta(minutes=VERIFY_POLL_MINUTES) + run_date = datetime.now(TZ) + timedelta(minutes=VERIFY_POLL_MINUTES) _scheduler.add_job( enqueue_verify, trigger="date", @@ -121,6 +201,37 @@ def _job_id(rule_id, index): return f"rule-{rule_id}-{index}" +def _parse_entry(entry): + """Normalize one saved schedule entry into (days, hour, minute), or + None if it isn't usable. Accepts both "HH:MM" and "HH:MM:SS" — an + emits seconds in some browsers, and the old + two-value unpack raised ValueError on those, aborting the whole + rebuild partway through (see rebuild_jobs).""" + if not isinstance(entry, dict): + return None + days = [d for d in entry.get("days") or [] if d in VALID_DAYS] + time_str = entry.get("time") + if not days or not isinstance(time_str, str): + return None + parts = time_str.split(":") + if len(parts) < 2: + return None + try: + hour, minute = int(parts[0]), int(parts[1]) + except ValueError: + return None + if not (0 <= hour <= 23 and 0 <= minute <= 59): + return None + return days, hour, minute + + +def is_valid_entry(entry): + """Public form of _parse_entry() for app.py's request validation, so + the webui rejects an unusable entry at save time instead of storing + one that every rebuild silently skips.""" + return _parse_entry(entry) is not None + + def rebuild_jobs(): """Clear and re-add every recurring cron job from schedules.json. @@ -128,41 +239,51 @@ def rebuild_jobs(): schedule_verification()) must survive a schedule edit elsewhere, since they're tracking an already-in-flight run's poll loop, not a recurring schedule. + + Every entry is parsed and added inside its own guard: this runs + *after* the removal loop below has already cleared the existing + cron jobs, so one malformed entry escaping as an exception would + leave the scheduler holding no jobs at all — every schedule silently + dead until the next restart. """ for job in _scheduler.get_jobs(): if job.id.startswith("rule-"): job.remove() schedules = storage.load("schedules.json", {}) + skipped = 0 for rule_id, rule in schedules.items(): - if not rule.get("enabled", True): + if not isinstance(rule, dict) or not rule.get("enabled", True): continue - for idx, entry in enumerate(rule.get("schedules", [])): - days = [d for d in entry.get("days", []) if d in VALID_DAYS] - time_str = entry.get("time") - if not days or not time_str: + for idx, entry in enumerate(rule.get("schedules") or []): + parsed = _parse_entry(entry) + if parsed is None: + skipped += 1 + logger.warning("Skipping unusable schedule entry %r on rule %s", entry, rule_id) continue - hour, minute = time_str.split(":") - trigger = CronTrigger( - day_of_week=",".join(days), - hour=int(hour), - minute=int(minute), - timezone=_TZ, - ) - _scheduler.add_job( - enqueue_run, - trigger=trigger, - args=[rule_id, rule.get("name")], - id=_job_id(rule_id, idx), - replace_existing=True, - misfire_grace_time=300, - ) - logger.info("Rebuilt %d scheduled job(s)", len(_scheduler.get_jobs())) + days, hour, minute = parsed + try: + trigger = CronTrigger( + day_of_week=",".join(days), + hour=hour, + minute=minute, + timezone=TZ, + ) + _scheduler.add_job( + enqueue_run, + trigger=trigger, + args=[rule_id, rule.get("name")], + id=_job_id(rule_id, idx), + replace_existing=True, + misfire_grace_time=300, + ) + except Exception: + skipped += 1 + logger.exception("Failed to schedule entry %r on rule %s", entry, rule_id) + logger.info("Rebuilt %d scheduled job(s), skipped %d", len(_scheduler.get_jobs()), skipped) -def start(tz=None): - # tz is accepted for backwards-compat call sites but is a no-op now - # that timezone is fixed at construction time above. +def start(): global _started if _started: return diff --git a/static/js/app.js b/static/js/app.js index 1a174e4..8d78d4c 100644 --- a/static/js/app.js +++ b/static/js/app.js @@ -1,3 +1,38 @@ +// Every state-changing call carries the per-session CSRF token from the +// tag in _shell.html; the API rejects a POST without it. +function csrfHeaders(extra) { + const meta = document.querySelector('meta[name="csrf-token"]'); + return Object.assign({}, extra || {}, { "X-CSRF-Token": meta ? meta.content : "" }); +} + +// Single door for the JSON API. Returns {ok} on success, or +// {ok: false, error, handled} — `handled` meaning it already told the +// user and is navigating away, so the caller should stay quiet. +// A dead login session answers 401 rather than redirecting (see +// login_required in app.py), which is what makes it distinguishable +// from a real success here. +async function apiPost(url, body) { + const headers = csrfHeaders(body ? { "Content-Type": "application/json" } : {}); + let res; + try { + res = await fetch(url, { + method: "POST", + headers, + body: body ? JSON.stringify(body) : undefined, + }); + } catch (err) { + return { ok: false, error: "Couldn't reach the server" }; + } + if (res.status === 401) { + alert("Your login session expired — returning to the login page."); + window.location.reload(); + return { ok: false, error: "signed out", handled: true }; + } + if (res.ok) return { ok: true }; + const data = await res.json().catch(() => ({})); + return { ok: false, error: data.error || `Request failed (${res.status})` }; +} + document.addEventListener("DOMContentLoaded", () => { const refreshBtn = document.getElementById("refresh-rules"); if (refreshBtn) { @@ -6,10 +41,9 @@ document.addEventListener("DOMContentLoaded", () => { const original = refreshBtn.textContent; refreshBtn.textContent = "Refreshing…"; try { - const res = await fetch("/api/rules/refresh", { method: "POST" }); - if (!res.ok) { - const data = await res.json().catch(() => ({})); - alert("Refresh failed: " + (data.error || res.status)); + const out = await apiPost("/api/rules/refresh"); + if (!out.ok) { + if (!out.handled) alert("Refresh failed: " + out.error); return; } window.location.reload(); @@ -34,14 +68,9 @@ function initSessionControls() { importBtn.disabled = true; importBtn.textContent = "Verifying…"; try { - const res = await fetch("/api/session/import", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ cookies: textarea.value }), - }); - const data = await res.json().catch(() => ({})); - if (!res.ok) { - alert(data.error || "Import failed"); + const out = await apiPost("/api/session/import", { cookies: textarea.value }); + if (!out.ok) { + if (!out.handled) alert(out.error); return; } window.location.reload(); @@ -90,10 +119,9 @@ function initRuleCard(card) { btn.disabled = true; btn.textContent = "Queuing…"; try { - const res = await fetch(`/api/rules/${ruleId}/run`, { method: "POST" }); - btn.textContent = res.ok ? "Queued" : "Failed"; - } catch (err) { - btn.textContent = "Failed"; + const out = await apiPost(`/api/rules/${ruleId}/run`); + btn.textContent = out.ok ? "Queued" : "Failed"; + if (!out.ok && !out.handled) alert("Could not queue that run: " + out.error); } finally { setTimeout(() => { btn.textContent = original; @@ -135,9 +163,12 @@ async function saveSchedule(card, opts = {}) { .split(/\s+/) .filter(Boolean), })); - await fetch(`/api/rules/${ruleId}/schedule`, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ enabled, schedules }), - }); + const out = await apiPost(`/api/rules/${ruleId}/schedule`, { enabled, schedules }); + if (!out.ok && !out.handled) { + alert("Schedule not saved: " + out.error); + // The card was already updated optimistically, so reload to put + // what's on screen back in sync with what's actually stored — + // otherwise a rejected schedule looks exactly like a saved one. + window.location.reload(); + } } diff --git a/templates/_shell.html b/templates/_shell.html index 1370b76..f4ab281 100644 --- a/templates/_shell.html +++ b/templates/_shell.html @@ -3,6 +3,7 @@ + {% block title %}MassPrice Scheduler{% endblock %} diff --git a/templates/activity.html b/templates/activity.html index db092f4..570db66 100644 --- a/templates/activity.html +++ b/templates/activity.html @@ -13,7 +13,7 @@ {% for entry in history %} - {{ entry.timestamp }} + {{ entry.timestamp | localtime }} {{ entry.rule_name or entry.rule_id }} {{ entry.status | capitalize }} diff --git a/templates/dashboard.html b/templates/dashboard.html index c01fb18..cbcce2a 100644 --- a/templates/dashboard.html +++ b/templates/dashboard.html @@ -4,7 +4,7 @@

Rules

- {% if rules_updated_at %}Rules last synced {{ rules_updated_at }}{% endif %} + {% if rules_updated_at %}Rules last synced {{ rules_updated_at | localtime }}{% endif %}
@@ -34,7 +34,7 @@
{% if rule.last_run %} - Last run {{ rule.last_run.timestamp }} — {{ rule.last_run.status }} + Last run {{ rule.last_run.timestamp | localtime }} — {{ rule.last_run.status }} {% if rule.last_run.status == 'failure' %}
{{ rule.last_run.message }}
{% endif %} {% else %} No runs yet diff --git a/templates/session.html b/templates/session.html index eca14e4..7584ef7 100644 --- a/templates/session.html +++ b/templates/session.html @@ -6,7 +6,7 @@

Status

{% if status.state == 'saved' %} -

Session saved {{ status.saved_at }}

+

Session saved {{ status.saved_at | localtime }}

If scheduled runs start failing with a "session expired" error, come back here and import a fresh export.

{% else %}

No session saved yet — scheduled runs will fail until you import one.