Fix seven medium-severity bugs from the full-session code review
- 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.<environment> 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.
This commit is contained in:
@@ -4,6 +4,7 @@
|
||||
|
||||
/backups/
|
||||
/secrets/
|
||||
/.poll-deploy-*.lock
|
||||
vendor/
|
||||
node_modules/
|
||||
*.log
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -13,6 +13,20 @@ ENVIRONMENT="${1:?Usage: poll-deploy.sh <staging|production>}"
|
||||
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 ;;
|
||||
|
||||
@@ -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")"
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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[] */
|
||||
|
||||
@@ -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,13 +208,17 @@ 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)' : ''
|
||||
));
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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,13 +151,38 @@ 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');
|
||||
}
|
||||
|
||||
self::throttle($delay_ms);
|
||||
|
||||
$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' => [
|
||||
@@ -162,21 +193,29 @@ class HardcoverAdapter
|
||||
]);
|
||||
|
||||
if (is_wp_error($response)) {
|
||||
throw new \RuntimeException('Hardcover API request failed: ' . $response->get_error_message());
|
||||
}
|
||||
|
||||
$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 ($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'] ?? [];
|
||||
}
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
throw new \RuntimeException($last_error);
|
||||
}
|
||||
|
||||
/** Defensive: accepts plain strings or {tag|name: string} objects — exact shape unverified. */
|
||||
private static function as_string_list($value): array
|
||||
|
||||
Reference in New Issue
Block a user