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.
twooey
9253c6638f