From 1020a496ab65e695ad4ac15c9afb7a3a31296720 Mon Sep 17 00:00:00 2001 From: Twooey Date: Thu, 27 Aug 2026 15:50:49 -0400 Subject: [PATCH] Fix seven medium-severity bugs from the full-session code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SupplierOffer::insert(): now checks $wpdb->insert()'s return value and throws, matching Work/Edition/Isbn (the one Catalog class that hadn't been hardened this way). Work::set_product_id() gets the same treatment (was flagged low-severity but same fix, bundled here) — a real update failure now counts as a per-work sync failure instead of silently leaving a stale wc_product_id pointer. - SupplierOffer: fetched_at was written via current_time('mysql') (site- local) while expires_at (SyntheticOfferGenerator) is written in UTC, and best_offer_for_isbns() compared against site-local time too — a mismatch masked today only because dev's gmt_offset is 0. Switched both writer and reader to current_time('mysql', true) (UTC). Verified the read path still finds all active offers correctly under a simulated -5 (US Eastern) offset, not just at offset 0. - HardcoverAdapter: rate-limit throttling moved from "once per work" (in Commands.php) to "once per actual HTTP request" (inside query() itself). find_book()'s ISBN-then-title/author fallback can fire two real requests per work — under the old scheme both shared one throttle sleep, roughly doubling the real request rate against a beta API. query() also now retries network errors/5xx/429 up to 3x (mirroring OpenLibraryAdapter::get_with_retry()), while a GraphQL-level `errors` field or other 4xx throws immediately (retrying a rejected query can't fix it). Commands.php adds a 5-consecutive-failure circuit breaker so a bad token or a wrong field in the still-unverified schema can't silently burn through the whole catalog with zero progress. Verified all of this directly against Hardcover's real API with a deliberately invalid token: 5 fast (non-retried) 401s, correct abort message, and confirmed the failed works were NOT marked synced (so a real token can retry them). - restore.sh: now drops and recreates the target database before restoring the dump, and clears the uploads directory before extracting the archive — previously both restored on top of existing state, so a stray table or file NOT in the backup would silently survive a restore drill. Verified end-to-end against a real local staging stack: planted a stray table and a stray upload file after taking a backup, ran restore.sh, and confirmed both were gone afterward while the actual backed-up data (20 works, a known upload file) came back correctly. Also fixed a real permission gap hit during that same test: a fresh volume's uploads dir is root-owned until something chowns it, which broke the new www-data clear step — now clears as root and chowns to www-data afterward, which also means restore self-heals the exact root-owned-uploads class of bug fixed earlier this session for the cron sidecar. - poll-deploy.sh: added a non-blocking flock so a deploy that runs longer than the cron interval can't have a second poll fire mid-deploy and race its git checkout/reset against the same live working tree. Verified: a concurrent run correctly skips instantly while the lock is held, and proceeds normally once it's released. (Full atomicity of the live PHP file swap under real traffic is a bigger architectural question — blue-green or symlinked releases — flagged to the user rather than attempted here.) - backup.sh: now also archives .env. itself (chmod 600) and includes it in the off-host rclone sync alongside the DB dump and uploads archive. Every API key and both DB passwords previously lived only on the host in this one gitignored file — losing the host lost all of it even with DB/uploads backups intact. --- .gitignore | 1 + deploy/backup.sh | 12 +++ deploy/poll-deploy.sh | 14 +++ deploy/restore.sh | 27 ++++++ .../includes/Catalog/SupplierOffer.php | 18 +++- .../bookstore-core/includes/Catalog/Work.php | 5 +- .../bookstore-core/includes/Cli/Commands.php | 32 ++++++- .../includes/Integration/HardcoverAdapter.php | 95 +++++++++++++------ 8 files changed, 167 insertions(+), 37 deletions(-) diff --git a/.gitignore b/.gitignore index 667a606..4704ee0 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ /backups/ /secrets/ +/.poll-deploy-*.lock vendor/ node_modules/ *.log diff --git a/deploy/backup.sh b/deploy/backup.sh index 0c77a51..b2b1a91 100755 --- a/deploy/backup.sh +++ b/deploy/backup.sh @@ -42,10 +42,22 @@ echo "==> archiving uploads" $COMPOSE run --rm -T -u www-data wordpress tar -czf - -C /var/www/html/wp-content uploads \ > "$BACKUP_DIR/uploads-${TIMESTAMP}.tar.gz" +echo "==> archiving environment config" +# Every API key (Booksrun/Ingram/Helcim/MailerLite/Hardcover), the WP admin +# bootstrap credentials, and both DB passwords live ONLY in this one +# gitignored host file — losing the host without this backed up loses all of +# it, even with the DB dump and uploads intact. Same trust model as the DB +# dump above (also plaintext, also only as protected as $BACKUP_DIR/ +# $BACKUP_REMOTE are) — chmod 600 since, unlike the DB/uploads archives, this +# one is directly the credentials themselves, not data that merely contains some. +cp "$ENV_FILE" "$BACKUP_DIR/env-${TIMESTAMP}" +chmod 600 "$BACKUP_DIR/env-${TIMESTAMP}" + if [[ -n "${BACKUP_REMOTE:-}" ]]; then echo "==> syncing to off-host storage ($BACKUP_REMOTE)" rclone copy "$BACKUP_DIR/db-${TIMESTAMP}.sql.gz" "$BACKUP_REMOTE/${ENVIRONMENT}/" rclone copy "$BACKUP_DIR/uploads-${TIMESTAMP}.tar.gz" "$BACKUP_REMOTE/${ENVIRONMENT}/" + rclone copy "$BACKUP_DIR/env-${TIMESTAMP}" "$BACKUP_REMOTE/${ENVIRONMENT}/" else echo "==> BACKUP_REMOTE not set — backup stayed local only; configure rclone before launch" fi diff --git a/deploy/poll-deploy.sh b/deploy/poll-deploy.sh index a3cea91..3fe8b83 100755 --- a/deploy/poll-deploy.sh +++ b/deploy/poll-deploy.sh @@ -13,6 +13,20 @@ ENVIRONMENT="${1:?Usage: poll-deploy.sh }" REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" cd "$REPO_ROOT" +# A deploy (docker compose up --build + full WP/plugin install) can run +# longer than the cron interval on modest hardware, and the git +# checkout/reset below writes directly onto the live working tree that's +# bind-mounted into the running containers — a second poll firing mid-deploy +# would race the first one's file writes. Non-blocking: if one's already +# running, this run just skips (the next poll picks up wherever HEAD ends up), +# rather than queuing up concurrent deploys. +LOCK_FILE="${REPO_ROOT}/.poll-deploy-${ENVIRONMENT}.lock" +exec 200>"$LOCK_FILE" +if ! flock -n 200; then + echo "$(date -Is) deploy already in progress for ${ENVIRONMENT}, skipping this poll" + exit 0 +fi + case "$ENVIRONMENT" in staging) BRANCH=staging ;; production) BRANCH=main ;; diff --git a/deploy/restore.sh b/deploy/restore.sh index 2b50a34..d3e0b18 100755 --- a/deploy/restore.sh +++ b/deploy/restore.sh @@ -11,6 +11,10 @@ cd "$REPO_ROOT" ENV_FILE=".env.staging" export ENVIRONMENT=staging +# shellcheck source=lib/env.sh +source "$REPO_ROOT/deploy/lib/env.sh" +DB_NAME_VALUE="$(env_get "$ENV_FILE" DB_NAME)" +: "${DB_NAME_VALUE:?set DB_NAME in ${ENV_FILE}}" # See deploy.sh for why: Compose warns on any $-shaped value in --env-file # even when unused, so DB_PASSWORD/DB_ROOT_PASSWORD are filtered out of the @@ -25,9 +29,32 @@ echo "!! this OVERWRITES the staging database and uploads !!" read -r -p "type 'restore' to continue: " confirm [[ "$confirm" == "restore" ]] || { echo "aborted"; exit 1; } +echo "==> dropping and recreating the staging database for a clean restore" +# Without this, a table present in staging but absent from the dump (a stray +# leftover from before a schema change, a one-off test table) would silently +# survive "restore" and go undetected — a drill could pass while masking data +# genuinely missing from the backup. Grants tied to a non-root user are keyed +# to the database NAME in MariaDB's privilege tables, not the schema object's +# identity, so MARIADB_USER's access survives a same-name drop+recreate +# (verified directly against a throwaway database before relying on this). +$COMPOSE exec -T db sh -c "exec mariadb -u root -p\"\$(cat /run/secrets/db_root_password)\" -e 'DROP DATABASE IF EXISTS \`${DB_NAME_VALUE}\`; CREATE DATABASE \`${DB_NAME_VALUE}\`;'" + echo "==> restoring database" gunzip -c "$DB_DUMP" | $COMPOSE exec -T db sh -c "exec mariadb -u\"\$MARIADB_USER\" -p\"\$(cat /run/secrets/db_password)\" \"\$MARIADB_DATABASE\"" +echo "==> clearing existing uploads before restoring" +# Same reasoning as the database above: extracting on top of whatever's +# already there would let a file missing from the archive hide behind one +# that happens to still be present from before the drill. Runs as root +# (unlike the tar extraction below), not www-data: the uploads directory +# itself can be root:root on a volume that's never had a www-data-owned +# write land in it yet (the official image's entrypoint creates it on first +# boot, before dropping to www-data) — confirmed directly against a fresh +# staging volume. chown afterward so the subsequent www-data tar extraction +# below can actually write into it. +$COMPOSE run --rm -T wordpress sh -c \ + "find /var/www/html/wp-content/uploads -mindepth 1 -delete && chown www-data:www-data /var/www/html/wp-content/uploads" + echo "==> restoring uploads" ARCHIVE_DIR="$(cd "$(dirname "$UPLOADS_ARCHIVE")" && pwd)" ARCHIVE_NAME="$(basename "$UPLOADS_ARCHIVE")" diff --git a/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php b/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php index 9a43bba..c092b9c 100644 --- a/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php +++ b/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php @@ -23,7 +23,7 @@ class SupplierOffer ?string $expires_at ): void { global $wpdb; - $wpdb->insert(self::table(), [ + $result = $wpdb->insert(self::table(), [ 'isbn13' => $isbn13, 'supplier_code' => $supplier_code, 'condition' => $condition, @@ -31,10 +31,20 @@ class SupplierOffer 'base_price' => $base_price, 'supplier_ship' => $supplier_ship, 'currency' => 'USD', - 'fetched_at' => current_time('mysql'), + // $gmt=true: expires_at (SyntheticOfferGenerator) is written in + // UTC via gmdate(), and best_offer_for_isbns() below compares + // against this same clock — current_time('mysql') without $gmt + // returns site-LOCAL time, which would silently misjudge offer + // expiry by the site's UTC offset the moment one is configured + // (confirmed: dev's offset is currently 0, masking this until + // then). Both reads and writes here must agree on UTC. + 'fetched_at' => current_time('mysql', true), 'expires_at' => $expires_at, 'status' => $status, ]); + if ($result === false) { + throw new \RuntimeException('bsc_supplier_offer insert failed: ' . $wpdb->last_error . ' (isbn13: ' . $isbn13 . ')'); + } } /** @@ -55,7 +65,9 @@ class SupplierOffer AND (expires_at IS NULL OR expires_at > %s) ORDER BY (base_price + supplier_ship) ASC LIMIT 1"; - $row = $wpdb->get_row($wpdb->prepare($sql, [...$isbn13s, current_time('mysql')])); + // $gmt=true — see the comment on insert()'s fetched_at: expires_at is + // stored in UTC, so the comparison clock must be UTC too. + $row = $wpdb->get_row($wpdb->prepare($sql, [...$isbn13s, current_time('mysql', true)])); return $row ?: null; } diff --git a/wp-content/plugins/bookstore-core/includes/Catalog/Work.php b/wp-content/plugins/bookstore-core/includes/Catalog/Work.php index 6f64d78..1568335 100644 --- a/wp-content/plugins/bookstore-core/includes/Catalog/Work.php +++ b/wp-content/plugins/bookstore-core/includes/Catalog/Work.php @@ -42,7 +42,10 @@ class Work public static function set_product_id(int $work_id, int $wc_product_id): void { global $wpdb; - $wpdb->update(self::table(), ['wc_product_id' => $wc_product_id], ['work_id' => $work_id]); + $result = $wpdb->update(self::table(), ['wc_product_id' => $wc_product_id], ['work_id' => $work_id]); + if ($result === false) { + throw new \RuntimeException('bsc_work.wc_product_id update failed: ' . $wpdb->last_error . " (work_id: {$work_id})"); + } } /** @return object[] */ diff --git a/wp-content/plugins/bookstore-core/includes/Cli/Commands.php b/wp-content/plugins/bookstore-core/includes/Cli/Commands.php index b6a34d1..09bf9b6 100644 --- a/wp-content/plugins/bookstore-core/includes/Cli/Commands.php +++ b/wp-content/plugins/bookstore-core/includes/Cli/Commands.php @@ -148,6 +148,17 @@ class Commands $offset = 0; $batch_size = 50; + // HardcoverAdapter::query() already retries transient failures (3x) + // before throwing, so an exception reaching here has already + // survived retries — if it keeps happening back-to-back, that's not + // per-book flakiness, it's systemic (bad token, an unverified-schema + // field name that's actually wrong, the endpoint down). Without this, + // a bad token would silently burn through the entire catalog with + // zero matches and zero progress. + $consecutive_failures = 0; + $max_consecutive_failures = 5; + $circuit_tripped = false; + while ($processed < $limit) { $works = Work::all_paginated($batch_size, $offset); if (empty($works)) { @@ -169,9 +180,10 @@ class Commands $isbns = Isbn::isbns_for_work((int) $work->work_id); $isbn13 = $isbns[0] ?? ''; + $processed++; try { - $book = HardcoverAdapter::find_book($isbn13, $work->title, $work->primary_author); + $book = HardcoverAdapter::find_book($isbn13, $work->title, $work->primary_author, $delay_ms); if ($book) { $tags = HardcoverAdapter::tags_for_book($book); // wp_set_object_terms() with an empty array CLEARS the @@ -196,14 +208,18 @@ class Commands $missed++; } update_post_meta($product_id, '_bsc_hardcover_synced_at', current_time('mysql')); + $consecutive_failures = 0; } catch (\RuntimeException $e) { \WP_CLI::warning("work_id={$work->work_id} ({$work->title}): " . $e->getMessage()); $missed++; + $consecutive_failures++; + if ($consecutive_failures >= $max_consecutive_failures) { + \WP_CLI::warning("Aborting: {$consecutive_failures} consecutive Hardcover API failures — this looks systemic (bad token, an incorrect field in the unverified query schema, the endpoint down), not per-book misses. Fix the underlying issue and re-run; already-synced works are untouched."); + $circuit_tripped = true; + break 2; + } } - $processed++; - HardcoverAdapter::throttle($delay_ms); - if ($processed % 20 === 0) { \WP_CLI::log(" processed {$processed} (matched {$matched}, missed {$missed})..."); } @@ -212,7 +228,13 @@ class Commands $offset += $batch_size; } - \WP_CLI::success("Hardcover sync: {$matched} matched, {$missed} missed, {$processed} processed."); + \WP_CLI::success(sprintf( + 'Hardcover sync: %d matched, %d missed, %d processed%s.', + $matched, + $missed, + $processed, + $circuit_tripped ? ' (aborted early — see warning above)' : '' + )); } /** diff --git a/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php b/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php index e600899..adfbbeb 100644 --- a/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php +++ b/wp-content/plugins/bookstore-core/includes/Integration/HardcoverAdapter.php @@ -47,10 +47,16 @@ class HardcoverAdapter * to title+author search (needed for the current Gutenberg-synthetic * catalog, whose ISBNs are fake). Fuzzy by nature — a miss returns * null rather than guessing wrong. + * + * $delay_ms is threaded down to every actual GraphQL request made + * (query() throttles itself, once per request — see its docblock) — + * NOT applied once per find_book() call. A miss on the ISBN lookup + * falls through to a second real HTTP request, and rate-limit spacing + * needs to track actual requests made, not logical calls. */ - public static function find_book(string $isbn13, string $title, ?string $author): ?array + public static function find_book(string $isbn13, string $title, ?string $author, int $delay_ms = self::DEFAULT_DELAY_MS): ?array { - return self::find_by_isbn($isbn13) ?? self::find_by_title_author($title, $author); + return self::find_by_isbn($isbn13, $delay_ms) ?? self::find_by_title_author($title, $author, $delay_ms); } /** @@ -84,7 +90,7 @@ class HardcoverAdapter usleep($delay_ms * 1000); } - private static function find_by_isbn(string $isbn13): ?array + private static function find_by_isbn(string $isbn13, int $delay_ms): ?array { $query = <<<'GQL' query FindByIsbn($isbn: String!) { @@ -101,11 +107,11 @@ class HardcoverAdapter } GQL; - $data = self::query($query, ['isbn' => $isbn13]); + $data = self::query($query, ['isbn' => $isbn13], $delay_ms); return $data['editions'][0]['book'] ?? null; } - private static function find_by_title_author(string $title, ?string $author): ?array + private static function find_by_title_author(string $title, ?string $author, int $delay_ms): ?array { $query = <<<'GQL' query FindByTitle($q: String!) { @@ -125,7 +131,7 @@ class HardcoverAdapter } GQL; - $candidates = self::query($query, ['q' => '%' . $title . '%'])['books'] ?? []; + $candidates = self::query($query, ['q' => '%' . $title . '%'], $delay_ms)['books'] ?? []; if (empty($candidates)) { return null; } @@ -145,37 +151,70 @@ class HardcoverAdapter return $candidates[0]; // title matched, author unconfirmed — best-effort, not silent } - private static function query(string $query, array $variables = []): array + /** + * One real HTTP request per call (plus retries — see below), so this is + * the single place that both throttles and retries, rather than leaving + * either to callers. Throttles once per call, before the first attempt — + * find_book() can trigger up to two of these (ISBN miss -> title/author + * fallback), and each is its own request against Hardcover's rate limit, + * not a fraction of one. + * + * Retries network errors/5xx/429 up to 3 attempts (mirrors + * OpenLibraryAdapter::get_with_retry()'s reasoning) — NOT a GraphQL-level + * `errors` response on a 200, and not other 4xx: those are the query + * itself being rejected (bad field name, auth failure, etc.), which a + * retry can't fix. Given the schema here is unverified, a systematically + * wrong query would otherwise retry 3x on literally every single work in + * the catalog for no benefit — callers should treat a thrown exception + * here as a signal to stop, not just log and continue (see Commands.php's + * consecutive-failure circuit breaker). + */ + private static function query(string $query, array $variables, int $delay_ms): array { $token = self::token(); if (!$token) { throw new \RuntimeException('HARDCOVER_API_TOKEN_FILE is not set or empty'); } - $response = wp_remote_post(self::ENDPOINT, [ - 'timeout' => 20, - 'headers' => [ - 'Content-Type' => 'application/json', - 'Authorization' => 'Bearer ' . $token, - ], - 'body' => wp_json_encode(['query' => $query, 'variables' => $variables]), - ]); + self::throttle($delay_ms); - if (is_wp_error($response)) { - throw new \RuntimeException('Hardcover API request failed: ' . $response->get_error_message()); + $max_attempts = 3; + $retry_delay_ms = 500; + $last_error = 'Hardcover API request failed after retries'; + + for ($attempt = 1; $attempt <= $max_attempts; $attempt++) { + $response = wp_remote_post(self::ENDPOINT, [ + 'timeout' => 20, + 'headers' => [ + 'Content-Type' => 'application/json', + 'Authorization' => 'Bearer ' . $token, + ], + 'body' => wp_json_encode(['query' => $query, 'variables' => $variables]), + ]); + + if (is_wp_error($response)) { + $last_error = 'Hardcover API request failed: ' . $response->get_error_message(); + } else { + $code = wp_remote_retrieve_response_code($response); + if ($code === 200) { + $body = json_decode(wp_remote_retrieve_body($response), true); + if (isset($body['errors'])) { + throw new \RuntimeException('Hardcover API errors: ' . wp_json_encode($body['errors'])); + } + return $body['data'] ?? []; + } + if ($code < 500 && $code !== 429) { + throw new \RuntimeException("Hardcover API returned HTTP {$code}: " . wp_remote_retrieve_body($response)); + } + $last_error = "Hardcover API returned HTTP {$code}: " . wp_remote_retrieve_body($response); + } + + if ($attempt < $max_attempts) { + usleep($retry_delay_ms * 1000); + } } - $code = wp_remote_retrieve_response_code($response); - $body = json_decode(wp_remote_retrieve_body($response), true); - - if ($code !== 200) { - throw new \RuntimeException("Hardcover API returned HTTP {$code}: " . wp_remote_retrieve_body($response)); - } - if (isset($body['errors'])) { - throw new \RuntimeException('Hardcover API errors: ' . wp_json_encode($body['errors'])); - } - - return $body['data'] ?? []; + throw new \RuntimeException($last_error); } /** Defensive: accepts plain strings or {tag|name: string} objects — exact shape unverified. */