Fix a real regression plus three hardening gaps found in a follow-up review
A second review pass specifically targeted at the fixes just made (not the original codebase) — caught one genuine regression from this session's own earlier work, plus a couple of gaps the first pass didn't probe deeply enough to find. - REGRESSION: SupplierOffer::insert() throwing on failure (a Medium-severity fix earlier this session, matching Work/Edition/Isbn) was never given a catch anywhere in the offer-generation path. Before that fix, a bad insert silently continued; after it, nothing stopped the exception from aborting the ENTIRE generate-offers run on the first failure. Fixed with the same per-item catch pattern GutenbergImporter already established. Verified by forcing a real DB failure (renamed the table mid-run): before this fix that aborted the batch immediately; after, it correctly processed all 2000 ISBNs, reported each failure individually, and completed with an accurate failed count — table restored after, 2000 offers confirmed intact. - ProductSync::sync_one() never checked WC_Product::save()'s return value. Verified directly (forced via the wp_insert_post_empty_content filter): save() returns 0 rather than throwing on a wp_insert_post()-level rejection, which would have gone through as Work::set_product_id($id, 0) — silently "succeeding" and invisible in $failed[], inconsistent with every other failure mode in this method already routing through the per-product SAVEPOINT + $failed[] reporting added this session. Now throws instead, confirmed it does NOT corrupt the existing pointer (stays at its prior value, self-heals on a future successful run) rather than writing 0. - docker-compose.yml: the wordpress healthcheck's timeout budget (~130s) could plausibly be exceeded by a legitimately slow (not broken) first boot on the 2-core/8GB box this actually runs on — confirmed that a service_healthy dependency timing out makes `docker compose up -d` (and so deploy.sh, under set -euo pipefail) hard-fail rather than just start late, a new deploy failure mode this session's healthcheck introduced. Widened to ~4.5 minutes of headroom (retries 20->40, start_period 30s->60s), well above deploy.sh's own existing 120s precedent for just the core-file-copy portion of the same boot. - Makefile: the per-target `; rc=$?; rm -f ...; exit $rc` cleanup added earlier this session doesn't reliably run under a real Ctrl+C — a foreground SIGINT terminates that shell chain before the `;` continues. Switched to a `trap 'rm -f ...' EXIT` (matching the idiom deploy.sh/ backup.sh already use), which fires on any shell termination and doesn't need to manually thread the exit code through. Verified by actually sending SIGINT to the whole process group (matching real terminal Ctrl+C, not just a backgrounded job) during `make logs` — the compose env file was correctly removed. Also cleaned up one duplicate leftover product (work_id=1 briefly had two products, from repeated manual test scenarios reusing the same work_id across this long session's testing, not from an active bug — confirmed the current code produces 0 new duplicates on repeated sync-products runs) found while verifying the ProductSync fix above.
This commit is contained in:
@@ -7,42 +7,44 @@ export ENVIRONMENT = $(ENV)
|
||||
# chmod 600 (not the 644 elsewhere in deploy/ — those get read by containers
|
||||
# under a different UID; this one is only ever read by the `docker compose`
|
||||
# CLI on the host, as $(ENV) invokes it, so there's no reason it needs to be
|
||||
# world-readable) — and every target below removes it on the way out (see
|
||||
# each recipe's `; rc=$$?; rm -f ...; exit $$rc`) rather than leaving a
|
||||
# plaintext copy of every API key and both DB passwords sitting in the repo
|
||||
# root after every `make` invocation. Make has no built-in "on exit"
|
||||
# hook across arbitrary targets, so this is repeated per-target rather than
|
||||
# centralized; `$$rc`/`exit $$rc` preserves the underlying command's exit
|
||||
# code through the cleanup so a real failure (e.g. deploy.sh erroring) still
|
||||
# fails the `make` invocation.
|
||||
# world-readable) — and every target below removes it on the way out via a
|
||||
# shell EXIT trap (same idiom deploy.sh/backup.sh already use for their own
|
||||
# temp files), not a `; rm -f ...` tacked onto the end of the command. A
|
||||
# real Ctrl+C on a long-running target (`logs -f`, an interactive `shell`)
|
||||
# sends SIGINT to the recipe's whole foreground process group, which
|
||||
# terminates a plain `cmd; rm -f ...` chain before the `;` ever runs — the
|
||||
# EXIT trap fires regardless of *how* the shell exits (falls through, errors,
|
||||
# or is killed by a signal), and doesn't touch $? either, so the underlying
|
||||
# command's real exit code still propagates to `make`.
|
||||
COMPOSE_ENV_FILE := .env.$(ENV).compose
|
||||
$(shell grep -Ev '^(DB_PASSWORD|DB_ROOT_PASSWORD)=' .env.$(ENV) > $(COMPOSE_ENV_FILE) 2>/dev/null; chmod 600 $(COMPOSE_ENV_FILE) 2>/dev/null)
|
||||
COMPOSE = docker compose -p bookstore-$(ENV) -f docker-compose.yml -f docker-compose.$(ENV).yml --env-file $(COMPOSE_ENV_FILE)
|
||||
CLEANUP = trap 'rm -f $(COMPOSE_ENV_FILE)' EXIT;
|
||||
|
||||
.PHONY: up down ps logs shell wp deploy backup
|
||||
|
||||
up:
|
||||
$(COMPOSE) up -d --build; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) $(COMPOSE) up -d --build
|
||||
|
||||
down:
|
||||
$(COMPOSE) down; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) $(COMPOSE) down
|
||||
|
||||
ps:
|
||||
$(COMPOSE) ps; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) $(COMPOSE) ps
|
||||
|
||||
logs:
|
||||
$(COMPOSE) logs -f; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) $(COMPOSE) logs -f
|
||||
|
||||
# add -u root yourself for one-off root debugging (installing a package, etc.)
|
||||
shell:
|
||||
$(COMPOSE) exec -u www-data wordpress bash; rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) $(COMPOSE) exec -u www-data wordpress bash
|
||||
|
||||
# make wp ENV=staging ARGS="plugin list"
|
||||
wp:
|
||||
$(COMPOSE) exec -u www-data wordpress wp $(ARGS); rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) $(COMPOSE) exec -u www-data wordpress wp $(ARGS)
|
||||
|
||||
deploy:
|
||||
./deploy/deploy.sh $(ENV); rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) ./deploy/deploy.sh $(ENV)
|
||||
|
||||
backup:
|
||||
./deploy/backup.sh $(ENV); rc=$$?; rm -f $(COMPOSE_ENV_FILE); exit $$rc
|
||||
$(CLEANUP) ./deploy/backup.sh $(ENV)
|
||||
|
||||
+15
-2
@@ -108,11 +108,24 @@ services:
|
||||
# ready — a slow first boot (official image copying core files in, could
|
||||
# be worse on modest hardware) could let Caddy serve a transient 502
|
||||
# before php-fpm was actually listening.
|
||||
#
|
||||
# cron and caddy now gate on this via depends_on: condition:
|
||||
# service_healthy, which makes `docker compose up -d` (and so
|
||||
# deploy.sh, under set -euo pipefail) hard-fail if wordpress never
|
||||
# reports healthy in time — a new way for a deploy to fail that didn't
|
||||
# exist before this healthcheck did. Budgeted generously (~4.5 min:
|
||||
# start_period + retries*interval) for the 2-core/8GB box this
|
||||
# actually runs on: deploy.sh's own wp-settings.php wait loop already
|
||||
# budgets up to 120s just for the image's core-file copy step (chosen
|
||||
# for this same hardware), and this check additionally waits for
|
||||
# php-fpm itself to start, plus whatever `--build` is competing for on
|
||||
# a 2-core box. A too-tight budget here would turn "slow but fine" into
|
||||
# a spurious deploy failure, not just a slower one.
|
||||
test: ["CMD-SHELL", "bash -c '(exec 3<>/dev/tcp/127.0.0.1/9000)' 2>/dev/null"]
|
||||
interval: 5s
|
||||
timeout: 3s
|
||||
retries: 20
|
||||
start_period: 30s
|
||||
retries: 40
|
||||
start_period: 60s
|
||||
|
||||
cron:
|
||||
build:
|
||||
|
||||
@@ -79,7 +79,16 @@ class Commands
|
||||
$result = SyntheticOfferGenerator::generate_all(500, function (int $processed) {
|
||||
\WP_CLI::log(" generated offers for {$processed} ISBNs...");
|
||||
});
|
||||
\WP_CLI::success("Generated offers for {$result['processed']} ISBNs.");
|
||||
|
||||
foreach ($result['failed'] as $failure) {
|
||||
\WP_CLI::warning($failure);
|
||||
}
|
||||
|
||||
\WP_CLI::success(sprintf(
|
||||
'Generated offers for %d ISBNs (%d failed).',
|
||||
$result['processed'],
|
||||
count($result['failed'])
|
||||
));
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -45,11 +45,12 @@ class SyntheticOfferGenerator
|
||||
|
||||
/**
|
||||
* @param callable|null $on_progress function(int $processed): void
|
||||
* @return array{processed:int}
|
||||
* @return array{processed:int, failed:string[]}
|
||||
*/
|
||||
public static function generate_all(int $batch_size = 500, ?callable $on_progress = null): array
|
||||
{
|
||||
$processed = 0;
|
||||
$failed = [];
|
||||
$offset = 0;
|
||||
while (true) {
|
||||
$isbns = Isbn::all_paginated($batch_size, $offset);
|
||||
@@ -57,7 +58,18 @@ class SyntheticOfferGenerator
|
||||
break;
|
||||
}
|
||||
foreach ($isbns as $isbn13) {
|
||||
self::generate_for_isbn($isbn13);
|
||||
// SupplierOffer::insert() throws on a real DB failure (fixed
|
||||
// elsewhere this session, matching Work/Edition/Isbn) — that
|
||||
// fix's whole point is making a bad insert visible instead of
|
||||
// silent, but visible must not mean "abort the entire batch."
|
||||
// One bad ISBN (a transient deadlock, a connection blip)
|
||||
// shouldn't cost every other ISBN's offer in the run, same
|
||||
// reasoning as GutenbergImporter's per-row catch.
|
||||
try {
|
||||
self::generate_for_isbn($isbn13);
|
||||
} catch (\RuntimeException $e) {
|
||||
$failed[] = "{$isbn13}: " . $e->getMessage();
|
||||
}
|
||||
$processed++;
|
||||
}
|
||||
if ($on_progress) {
|
||||
@@ -65,6 +77,6 @@ class SyntheticOfferGenerator
|
||||
}
|
||||
$offset += $batch_size;
|
||||
}
|
||||
return ['processed' => $processed];
|
||||
return ['processed' => $processed, 'failed' => $failed];
|
||||
}
|
||||
}
|
||||
|
||||
@@ -163,6 +163,17 @@ class ProductSync
|
||||
}
|
||||
|
||||
$product_id = $product->save();
|
||||
if (!$product_id) {
|
||||
// WC_Product::save() returns 0 rather than throwing on a
|
||||
// wp_insert_post()-level rejection (confirmed directly via the
|
||||
// wp_insert_post_empty_content filter). Left unchecked, this
|
||||
// work_id gets Work::set_product_id($work_id, 0) — silently
|
||||
// "succeeds," isn't visible in $failed[], and every other
|
||||
// failure mode in this method already goes through the
|
||||
// per-product SAVEPOINT + $failed[] reporting. Throwing here
|
||||
// routes it through the same path instead of a special case.
|
||||
throw new \RuntimeException("WC_Product::save() returned no product ID (work_id: {$work->work_id})");
|
||||
}
|
||||
update_post_meta($product_id, '_bsc_work_id', $work->work_id);
|
||||
|
||||
// Unconditional, not "if new": $existing_id can be a stale pointer to
|
||||
|
||||
Reference in New Issue
Block a user