perf(test-kit,api): cut integration-suite time - per-test truncation, shutdown sleep, cold jest cache, shared PrestaShop container (#1920) - #1923
Conversation
04ef34c to
e0088f0
Compare
Tech Lead review — cut per-test truncation, blind shutdown sleep and cold jest cacheVerdict: 🔄 Approve with changes (comment — GitHub blocks self-approval). Genuinely good performance work with measurements behind every claim. One behavioural change in IMPORTANT — skipping empty tables also skips their
|
e0088f0 to
ac7da9b
Compare
…e PS specs Four int-specs each booted their own PrestaShop + MySQL trio. A boot is 77.7 s locally - 45.6 s of it PrestaShop installing itself into MySQL, 23.0 s the OL module install plus Symfony cache warmup - and on CI those four specs were 308 s, 60% of the api integration step after #1923 landed the cheaper wins (#1920). Three of them now share one container, booted on first use: - `orders/allegro-prestashop-carrier-mapping` - `prestashop/prestashop-order-fulfillment-update` - `orders/prestashop-harness-smoke` `prestashop/prestashop-webhook-provisioning` deliberately keeps its own fresh container: its subject IS the from-scratch install ("writes the three OPENLINKER_* configurations on FIRST install"), and installing the OL module writes those same rows, so sharing would erase what the spec asserts. No PrestaShop-side cleanup runs between the sharing specs, and none is needed: every assertion is either over an OL result array or filtered by the `psOrderId` the spec just created, so orders and carts left behind by an earlier file are inert. Verified by reading each assertion rather than assuming. The shared container is always module-installed. The module-installed carrier satisfies the no-module spec's assertions (`external_module_name = 'openlinker'`, matching `olDynamicCarrierId`) while the reverse does not hold. The `OL_SKIP_PS_MODULE_INSTALL=true` escape hatch still works - in that mode a spec takes its own module-less container instead of the shared one. The handoff between specs is a FILE, not an env var. Jest hands every test file a fresh `process` object carrying a COPY of `process.env`, so a value written by one int-spec is invisible to the next - measured, not assumed (same pid, second file reports no cached value). That is also why `startContainers`' `CONTAINERS_PRIMED_ENV_VAR` works for Postgres/Redis: `globalSetup` sets it in the parent before the worker receives its copy, a route a lazily-booted container cannot use. The record is liveness-checked with `docker inspect` before reuse, so a record left behind by a crashed run boots a fresh container instead of failing every spec with a connection error. Because `cleanup` is a closure the `globalTeardown` realm cannot see, the worker records the container ids on disk and teardown stops them from there. Leaving them for the CI orphan sweep (#1285) would be a backstop, not a plan. Measured locally: the three sharing specs went ~380 s -> 132.3 s, and the full api integration suite 622.6 s -> 431.2 s (85 suites, 501 tests, zero failures). On CI, where a boot is ~40 s rather than ~90 s, this is worth ~80 s: two boots removed from the step. Refs #1920
norbert-kulus-blockydevs
changed the title
…1923 review) Two review findings from #1923. IMPORTANT - skipping an empty table also skipped its CASCADE side effects. The pre-#1920 reset ran one `TRUNCATE <t> CASCADE` per listed table, which also clears every table holding an FK to `<t>`, including tables the caller never listed. Probing only the literal list dropped that collateral clear whenever the parent was empty while a dependent still held rows (possible with a nullable FK) - the shape that produces order-dependent int-spec flakes. The probe now runs over the list's transitive CASCADE closure, walked from `pg_constraint` and memoised per DataSource (the FK graph is fixed once `synchronize` builds the schema), so the steady-state reset keeps its two round-trips. The caller's own names are always probed even if Postgres reports no such relation, so a typo still fails loudly. The closure is currently vacuous for the apps/api list - verified against the live test schema, not assumed: the resolver reports zero extra tables, because the synchronize-built schema has exactly four FKs (product_variants -> products, inventory_items -> products, inventory_items -> product_variants, attribute_value_mappings -> attribute_mappings) and every dependent of a listed table is itself listed. Resolving it at runtime means the next FK does not have to be noticed by hand. SUGGESTION - `truncateTables` interpolates table names into SQL. Every in-tree caller passes constants, but the helper is a published seam, so names are now asserted against /^[a-z_][a-z0-9_]*$/i with a clear throw before any statement is issued. Verification: test-kit unit specs 10/10 (3 new - closure probing, one walk per DataSource, identifier rejection); lint + type-check clean; `connection-crud` + `inventory-multivariant-cleanup` int-specs green against real Postgres (5.1 s), which is what exercises the new SQL. Refs #1920 Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
Review addressed - both findings, in
|
| FK (from ORM relation decorators - migrations do not run in this path) | Both ends in tablesToTruncate? |
|---|---|
product_variants.productId -> products |
yes / yes |
inventory_items.productId -> products |
yes / yes |
inventory_items.productVariantId -> product_variants (nullable) |
yes / yes |
attribute_value_mappings.attribute_mapping_id -> attribute_mappings |
neither is listed - nothing to cascade from |
The nullable FK you correctly singled out as the divergence case (inventory_items.productVariantId) has both ends listed, so parent-empty-child-dirty cannot strand rows.
Made explicit anyway. A hand-verified property that a future @ManyToOne silently invalidates is exactly the flake class you flagged, so the probe now runs over the transitive CASCADE closure of the caller's list, walked from pg_constraint:
WITH RECURSIVE requested AS (
SELECT c.oid FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE c.relname IN (...) AND n.nspname = ANY (current_schemas(false))
), closure AS (
SELECT oid FROM requested
UNION
SELECT con.conrelid FROM pg_constraint con
JOIN closure ON con.confrelid = closure.oid WHERE con.contype = 'f'
) SELECT c.relname AS table_name FROM closure JOIN pg_class c ON c.oid = closure.oidResulting state is identical to the old per-table TRUNCATE ... CASCADE loop's, by construction: anything the old loop cleared as collateral is now probed, and an empty table needs no clearing.
Two details worth naming:
- No added round-trip in steady state. The FK graph is fixed once
synchronizebuilds the schema, so the walk is memoised per DataSource (WeakMap, keyed by the table list). One extra query per int-spec file, not perafterEach; the reset stays at probe +TRUNCATE. - The caller's own names are always probed, even when Postgres reports no such relation. A typo'd table must keep failing loudly at probe time rather than being silently dropped from the closure.
The truncateTables doc comment now states the cascade reasoning (it previously justified the optimisation on row counts alone, as you noted), and docs/testing-guide.md says the same in the lifecycle section.
SUGGESTION - interpolated table names: taken
truncateTables now asserts every name against /^[a-z_][a-z0-9_]*$/i and throws test-kit: refusing to truncate "<name>" - ... before issuing any statement. Discovered dependents from the closure walk go through the same assert. Not a hot-path cost (one regex per table, once per reset) and it keeps the published seam honest.
Verification
- test-kit unit specs 10/10, 3 new: closure probing keeps a dependent that only
CASCADEwould have cleared; thepg_constraintwalk runs once per DataSource; a non-identifier table name is rejected with zero queries issued eslint+tsc --noEmitfor@openlinker/test-kitcleanconnection-crud+inventory-multivariant-cleanupint-specs green against real Postgres (2 suites / 10 tests / 5.1 s) - that is what exercises the new SQL rather than a fake
Note on the numbers
The measurements in the description say "18 configured tables"; the list is 19 since mcp_tokens arrived with #1486. The cost model (~10 ms per table, linear in count) is unchanged, so the conclusion stands - flagging it so the figure is not read as exact.
… cold jest cache Three measured, independent costs in the integration suite, none of which is test logic. Numbers below are from instrumented probes and A/B runs on one machine with a revert between variants; see #1920 for the full log, including four hypotheses that measurement refuted. 1. Shutdown slept a flat 2 s, every time `WebhookToJobHandler.stopConsumptionLoop` ended with `setTimeout(2000)` "to let in-flight messages complete". ~77 int-specs tear the harness down, so that was ~2 s x 77 per run - and it guaranteed nothing, since a slow message was cut off at 2 s regardless. The handler now tracks the `processMessage` call actually in progress and awaits that, bounded by the same 2 s: with nothing in flight (every int-spec teardown) shutdown returns immediately. It deliberately does not await the consume loop, which parks in `XREADGROUP ... BLOCK 5000`. A failing in-flight message is swallowed so `app.close()` cannot start rejecting where the old sleep never could. `BLOCK_MS` and `quit()` are untouched - swapping `quit()` for `disconnect()` was measured and buys nothing. 2. Every test truncated all 18 tables, empty or not `TRUNCATE` costs ~10 ms per table regardless of contents (1 table ~10 ms, 18 ~207 ms, all 46 ~390 ms - linear in table count, flat in row count) while a typical test dirties two or three. At ~207 ms x 485 api tests that was ~100 s per run spent clearing empty tables. `truncateTables` now asks in one round-trip which tables hold a row and truncates only those: ~207 ms -> ~13 ms, and a test that dirtied nothing issues no TRUNCATE at all. Batching alone is NOT the win (280 ms -> 215 ms), Postgres durability flags gave nothing, and CASCADE was never the mechanism (the schema has 4 FKs). 3. The transform cache was thrown away between runs Neither integration config set `cacheDirectory`, so each run re-transformed the whole AppModule graph into a throwaway /tmp dir. That tax lands on the run's FIRST suite: the same file takes 6.42 s warm and 76.29 s cold locally, which is why `order-reingestion-echo-guard` reads as a 37 s suite on CI while its own `it` takes 149 ms. Both configs now cache into `.jest-cache/` and the job restores it via `actions/cache`. ts-jest keys entries by content, so a restored older cache is safe - changed files miss and re-transform. Measured on a fixed 12-suite sample: 41.5 s baseline -> 19.2 s with (1) -> 10.1-14.6 s with (1)+(2). Full local api integration suite green: 83 suites, 485 tests, 622.6 s. Docs: `docs/testing-guide.md`'s lifecycle section now describes the reset semantics and corrects a standing inaccuracy - it claimed migrations run before the integration suite. They do not; the schema comes from TypeORM `synchronize`, so the suite never proves the migrations reproduce the entity schema. That gap is flagged, not fixed here. `docs/lessons.md` gains the profiling lesson. Refs #1920 Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
…e PS specs Four int-specs each booted their own PrestaShop + MySQL trio. A boot is 77.7 s locally - 45.6 s of it PrestaShop installing itself into MySQL, 23.0 s the OL module install plus Symfony cache warmup - and on CI those four specs were 308 s, 60% of the api integration step after #1923 landed the cheaper wins (#1920). Three of them now share one container, booted on first use: - `orders/allegro-prestashop-carrier-mapping` - `prestashop/prestashop-order-fulfillment-update` - `orders/prestashop-harness-smoke` `prestashop/prestashop-webhook-provisioning` deliberately keeps its own fresh container: its subject IS the from-scratch install ("writes the three OPENLINKER_* configurations on FIRST install"), and installing the OL module writes those same rows, so sharing would erase what the spec asserts. No PrestaShop-side cleanup runs between the sharing specs, and none is needed: every assertion is either over an OL result array or filtered by the `psOrderId` the spec just created, so orders and carts left behind by an earlier file are inert. Verified by reading each assertion rather than assuming. The shared container is always module-installed. The module-installed carrier satisfies the no-module spec's assertions (`external_module_name = 'openlinker'`, matching `olDynamicCarrierId`) while the reverse does not hold. The `OL_SKIP_PS_MODULE_INSTALL=true` escape hatch still works - in that mode a spec takes its own module-less container instead of the shared one. The handoff between specs is a FILE, not an env var. Jest hands every test file a fresh `process` object carrying a COPY of `process.env`, so a value written by one int-spec is invisible to the next - measured, not assumed (same pid, second file reports no cached value). That is also why `startContainers`' `CONTAINERS_PRIMED_ENV_VAR` works for Postgres/Redis: `globalSetup` sets it in the parent before the worker receives its copy, a route a lazily-booted container cannot use. The record is liveness-checked with `docker inspect` before reuse, so a record left behind by a crashed run boots a fresh container instead of failing every spec with a connection error. Because `cleanup` is a closure the `globalTeardown` realm cannot see, the worker records the container ids on disk and teardown stops them from there. Leaving them for the CI orphan sweep (#1285) would be a backstop, not a plan. Measured locally: the three sharing specs went ~380 s -> 132.3 s, and the full api integration suite 622.6 s -> 431.2 s (85 suites, 501 tests, zero failures). On CI, where a boot is ~40 s rather than ~90 s, this is worth ~80 s: two boots removed from the step. Refs #1920
…1923 review) Two review findings from #1923. IMPORTANT - skipping an empty table also skipped its CASCADE side effects. The pre-#1920 reset ran one `TRUNCATE <t> CASCADE` per listed table, which also clears every table holding an FK to `<t>`, including tables the caller never listed. Probing only the literal list dropped that collateral clear whenever the parent was empty while a dependent still held rows (possible with a nullable FK) - the shape that produces order-dependent int-spec flakes. The probe now runs over the list's transitive CASCADE closure, walked from `pg_constraint` and memoised per DataSource (the FK graph is fixed once `synchronize` builds the schema), so the steady-state reset keeps its two round-trips. The caller's own names are always probed even if Postgres reports no such relation, so a typo still fails loudly. The closure is currently vacuous for the apps/api list - verified against the live test schema, not assumed: the resolver reports zero extra tables, because the synchronize-built schema has exactly four FKs (product_variants -> products, inventory_items -> products, inventory_items -> product_variants, attribute_value_mappings -> attribute_mappings) and every dependent of a listed table is itself listed. Resolving it at runtime means the next FK does not have to be noticed by hand. SUGGESTION - `truncateTables` interpolates table names into SQL. Every in-tree caller passes constants, but the helper is a published seam, so names are now asserted against /^[a-z_][a-z0-9_]*$/i with a clear throw before any statement is issued. Verification: test-kit unit specs 10/10 (3 new - closure probing, one walk per DataSource, identifier rejection); lint + type-check clean; `connection-crud` + `inventory-multivariant-cleanup` int-specs green against real Postgres (5.1 s), which is what exercises the new SQL. Refs #1920 Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
ac382e9 to
3fdd787
Compare
Second review pass — independent, on the rebased headA fresh reviewer went over the whole diff ( IMPORTANT 1 — the closure walk's recursive term has no schema guard, and a cross-schema dependent breaks every reset
Reproduced against a scratch Not reachable in-tree (the api schema is all '), closure AS (' +
' SELECT oid FROM requested' +
' UNION' +
' SELECT con.conrelid FROM pg_constraint con' +
' JOIN closure ON con.confrelid = closure.oid' +
' JOIN pg_class dep ON dep.oid = con.conrelid' +
' JOIN pg_namespace depns ON depns.oid = dep.relnamespace' +
" WHERE con.contype = 'f'" +
' AND depns.nspname = ANY (current_schemas(false))' +
') SELECT c.relname AS table_name FROM closure JOIN pg_class c ON c.oid = closure.oid',(If cross-schema dependents should be cleared instead of ignored: return IMPORTANT 2 — the PrestaShop section of
|
…view findings (#1923) IMPORTANT 1 - the closure walk's recursive term had no schema guard. Only the `requested` CTE filtered on `current_schemas(false)`, so a table in another schema holding an FK to a listed table entered the closure and was then probed by its BARE name. Because the probe is one `UNION ALL` statement, the resulting `relation "x" does not exist` failed every `afterEach` in the suite - strictly louder than the pre-#1920 per-table loop, which never touched that table at all. The recursive term now joins pg_class/pg_namespace and applies the same `current_schemas(false)` filter to the DEPENDENT side. Reproduced and fixed against a scratch postgres:16-alpine: with `other.xref -> public.parent`, the old query returns `xref`, the new one does not, while a transitive dependent (`parent -> child -> grandchild`) and a self-referential FK still behave. Cross-schema dependents are ignored rather than schema-qualified: the reset is scoped to the schema under test. IMPORTANT 2 - the PrestaShop section of docs/testing-guide.md still documented the per-spec boot this branch replaced, in four places (the opt-in sample, the "suite-scoped, one boot per int-spec file" claim, the who-opts-in list, and the vertical-slice guidance). A contributor following it would have booted a fourth container and re-added ~78 s. It now describes the run-scoped shared container, the two rules a spec must respect (nothing resets PS between specs; the shared container is always module-installed), and when to take your own container. SUGGESTION 3 - `startSharedPrestashopContainer` hard-coded `installOlModule: true` and never read `OL_SKIP_PS_MODULE_INSTALL`, so with the hatch set the smoke spec booted a shared container that paid the 23 s install it did not need. It now honours the same env var the individual specs read. SUGGESTION 4 - `XREADGROUP ... COUNT 10` can return a whole batch, but the shutdown drain covers only the message already running: messages 2..N could start against a quitting Redis client. The batch loop now breaks once shutdown is signalled. SUGGESTION 5 - `sharedRecordFile()` keyed on `GITHUB_RUN_ID`, which is stable across "Re-run jobs" while the orphan sweep deliberately skips its own run id - so a re-run could adopt a SIGKILLed attempt's half-mutated PrestaShop DB. The key now carries `GITHUB_RUN_ATTEMPT`, and the comment records that the local `ppid` key only lines the two realms up because both integration configs pin `maxWorkers: 1`. SUGGESTION 6 - the liveness `docker inspect` lacked the `stdio`/`timeout` every sibling `execFileSync` in the file passes, so a stale record printed a Docker error into the job log and an unresponsive daemon could hang the first PS spec's `beforeAll` indefinitely. SUGGESTION 7 - the jest-cache key used `github.sha`, minting a fresh multi-hundred-MB entry per commit, which would evict the pnpm store cache the same job restores (10 GB repo budget, LRU). It is now scoped to what invalidates the cache wholesale (lockfile, base tsconfig, both jest configs) plus the run id, so a save still happens while unchanged-deps runs share one entry. Verification: test-kit unit specs 11/11 (1 new - the dependent-side schema guard); webhook handler specs 15/15 (2 new - the loop stops starting messages after shutdown, and still drains a full batch while running); `connection-crud` int-spec 9/9 against real Postgres; eslint clean on the gated paths; type-check clean for @openlinker/test-kit and @openlinker/api; ci.yml parses and the `apps/*/test/jest-integration.cjs` glob matches both configs. Refs #1920 Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
All seven second-pass findings addressed —
|
| # | Finding | What changed |
|---|---|---|
| 1 (IMPORTANT) | closure walk's recursive term had no schema guard | recursive term now joins pg_class/pg_namespace and applies current_schemas(false) to the dependent side |
| 2 (IMPORTANT) | PS section of docs/testing-guide.md still documented the per-spec boot |
opt-in sample, the "suite-scoped" claim, the who-opts-in list and the vertical-slice guidance rewritten for the run-scoped shared container |
| 3 | startSharedPrestashopContainer ignored OL_SKIP_PS_MODULE_INSTALL |
now reads the same env var the individual specs read |
| 4 | batch loop never re-checked shutdown | break once isRunning is false / the abort signal fired |
| 5 | record key omitted GITHUB_RUN_ATTEMPT |
key carries the attempt; comment records the maxWorkers: 1 dependency of the local ppid key |
| 6 | liveness docker inspect lacked stdio/timeout |
stdio: ['ignore','pipe','ignore'] + timeout: 30_000, matching the house style at :355/:1123 |
| 7 | jest cache key used github.sha |
key scoped to lockfile + base tsconfig + both jest configs, plus github.run_id |
Finding 1 — reproduced, then fixed, against a scratch postgres:16-alpine
Schema fixture: public.parent, public.child(p → parent), public.grandchild(c → child), public.selfref(s → selfref), and other.xref(p → public.parent). Requested list: ['parent','selfref'].
=== NEW query (schema-guarded recursive term) ===
child | grandchild | parent | selfref (4 rows)
=== OLD query (no guard) ===
child | grandchild | parent | selfref | xref (5 rows)
xref is the bug: it was pulled into the closure and then probed by its bare name, which is what took the whole afterEach down (relation "xref" does not exist) — one UNION ALL statement, so a single unreachable relation fails the reset for every table. Transitive dependents and the self-referential FK still behave, so the guard did not narrow anything it should keep.
Design note: cross-schema dependents are ignored, not schema-qualified. The reset is scoped to the schema under test, and a dependent outside search_path is not something an int-spec's TRUNCATE should reach into.
Finding 4 — the window is real but was benign
Kept as a SUGGESTION-level fix, not re-litigated: the pre-existing behaviour was safe in both directions that matter (int-spec teardown has nothing in flight; production route is keyed by event.eventId, so a redelivered message is idempotent). The guard removes the case where messages 2..N of a COUNT 10 batch start against a client that quit() has already been called on.
Verification
@openlinker/test-kitunit specs 11/11 — 1 new: the walk constrains the dependent side to the search path- webhook handler specs 15/15 — 2 new: the loop stops starting messages after shutdown is signalled mid-batch, and still drains a full batch while running
connection-crud.int-spec.ts9/9 against real Postgres — the reworked SQL executes through TypeORM, not only against the unit-test fakeeslintclean on the lint-gated paths (libs/test-kit/src,apps/api/src/webhooks);type-checkclean for@openlinker/test-kitand@openlinker/apici.ymlparses, and theapps/*/test/jest-integration.cjsglob was confirmed to match both integration configs (api + worker)
Two things deliberately left alone: the pre-existing consistent-type-imports / unused-destructure lint noise in prestashop-container.helper.ts (that path is outside the src/** lint gate, and touching it is unrelated churn), and the docs/lessons.md entry, which still reads correctly after these changes.
piotrswierzy
left a comment
piotrswierzy
left a comment
There was a problem hiding this comment.
✅ Approve.
Full review in this comment.
My one finding — that skipping empty tables also skips their CASCADE side effects, which could resurrect the order-dependent-flake class this repo has already been bitten by — was addressed properly rather than asserted away. resolveCascadeClosure expands the caller's list to its real FK closure and memoises per DataSource, so the optimisation and the old semantics both survive. assertSafeTableNames picked up the optional suggestion too.
The measurements are what made this reviewable: recording that batching was tried and is not the win (18 statements 280 ms vs one combined 215 ms) stops the next person re-attempting it.
…2164) Review follow-ups on the recovery primitives. Two were real defects. reclaimOrphans discarded the XCLAIM reply and re-read the body with XRANGE. XCLAIM without FORCE cannot resurrect a non-pending id, so there was no PEL leak — but the opposite failure existed: when a claim legitimately does not transfer (the owner ACKed between the XPENDING and the XCLAIM, or touched the entry so it is no longer idle past the threshold), XRANGE still returns the body, because ACK removes an entry from the PEL but not from the stream. The code then processed and ACKed a message belonging to another live consumer — defeating the very re-assertion the comment claimed to make. The claim reply is now the source of truth: node-redis returns one element per requested id and null where the claim did not transfer, so a non-transfer is either a lost race (skip; not ours) or a trimmed entry (report so the caller clears the dangling id), disambiguated by a single XRANGE. The unit spec previously mocked xClaim as resolving undefined while asserting an entry came back — it encoded the bug — and now pins the correct behaviour, with an integration test covering the owner-ACKed-first race against real Redis. maybeReclaimOrphans sat after the loop's empty-batch `continue` in all three consumers, so it only ran when a batch arrived. Orphans are reclaimed precisely when a stream is quiet — events.master.deletion can sit empty for days — so the feature was dead code in the case it exists for. Moved above the check; the existing lastReclaimAt throttle already prevents it firing every block timeout. Also: - The periodic pass now re-drains own pending before the orphan sweep. A handler that throws leaves its entry un-ACKed in this consumer's PEL, and the orphan sweep deliberately skips self-owned rows, so a single transient failure previously stranded a message until the process restarted. - The webhook handler's recovery path now tracks inFlightMessage and honours the abort flag, as the batch path does. Without it a shutdown mid-recovery returned from stopConsumptionLoop immediately and onModuleDestroy quit the client out from under a message still being processed — the regression #1923 fixed, reintroduced on a new path. - The startup drain is bounded by MAX_DRAIN_PAGES and bails on abort. Its termination previously depended on an ACK-or-throw invariant held across three files with no test; a future non-ACKing branch would have hung onModuleInit and with it application boot. - RECLAIM_INTERVAL_MS moved to the shared module rather than being redeclared per consumer. - Corrected docs and comments that described XAUTOCLAIM and an `id: '0'` read. Neither is used, and the module header explains why — an ADR whose purpose is audit-trail accuracy must not misstate its own implementation. - Documented OL_WORKER_ID in both .env.example files: it is a correctness-relevant knob (two replicas sharing a value share one PEL) that existed only in the plan. Rebased onto current main. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>
…2164) Review follow-ups on the recovery primitives. Two were real defects. reclaimOrphans discarded the XCLAIM reply and re-read the body with XRANGE. XCLAIM without FORCE cannot resurrect a non-pending id, so there was no PEL leak — but the opposite failure existed: when a claim legitimately does not transfer (the owner ACKed between the XPENDING and the XCLAIM, or touched the entry so it is no longer idle past the threshold), XRANGE still returns the body, because ACK removes an entry from the PEL but not from the stream. The code then processed and ACKed a message belonging to another live consumer — defeating the very re-assertion the comment claimed to make. The claim reply is now the source of truth: node-redis returns one element per requested id and null where the claim did not transfer, so a non-transfer is either a lost race (skip; not ours) or a trimmed entry (report so the caller clears the dangling id), disambiguated by a single XRANGE. The unit spec previously mocked xClaim as resolving undefined while asserting an entry came back — it encoded the bug — and now pins the correct behaviour, with an integration test covering the owner-ACKed-first race against real Redis. maybeReclaimOrphans sat after the loop's empty-batch `continue` in all three consumers, so it only ran when a batch arrived. Orphans are reclaimed precisely when a stream is quiet — events.master.deletion can sit empty for days — so the feature was dead code in the case it exists for. Moved above the check; the existing lastReclaimAt throttle already prevents it firing every block timeout. Also: - The periodic pass now re-drains own pending before the orphan sweep. A handler that throws leaves its entry un-ACKed in this consumer's PEL, and the orphan sweep deliberately skips self-owned rows, so a single transient failure previously stranded a message until the process restarted. - The webhook handler's recovery path now tracks inFlightMessage and honours the abort flag, as the batch path does. Without it a shutdown mid-recovery returned from stopConsumptionLoop immediately and onModuleDestroy quit the client out from under a message still being processed — the regression #1923 fixed, reintroduced on a new path. - The startup drain is bounded by MAX_DRAIN_PAGES and bails on abort. Its termination previously depended on an ACK-or-throw invariant held across three files with no test; a future non-ACKing branch would have hung onModuleInit and with it application boot. - RECLAIM_INTERVAL_MS moved to the shared module rather than being redeclared per consumer. - Corrected docs and comments that described XAUTOCLAIM and an `id: '0'` read. Neither is used, and the module header explains why — an ADR whose purpose is audit-trail accuracy must not misstate its own implementation. - Documented OL_WORKER_ID in both .env.example files: it is a correctness-relevant knob (two replicas sharing a value share one PEL) that existed only in the plan. Rebased onto current main. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>
…2164) Review follow-ups on the recovery primitives. Two were real defects. reclaimOrphans discarded the XCLAIM reply and re-read the body with XRANGE. XCLAIM without FORCE cannot resurrect a non-pending id, so there was no PEL leak — but the opposite failure existed: when a claim legitimately does not transfer (the owner ACKed between the XPENDING and the XCLAIM, or touched the entry so it is no longer idle past the threshold), XRANGE still returns the body, because ACK removes an entry from the PEL but not from the stream. The code then processed and ACKed a message belonging to another live consumer — defeating the very re-assertion the comment claimed to make. The claim reply is now the source of truth: node-redis returns one element per requested id and null where the claim did not transfer, so a non-transfer is either a lost race (skip; not ours) or a trimmed entry (report so the caller clears the dangling id), disambiguated by a single XRANGE. The unit spec previously mocked xClaim as resolving undefined while asserting an entry came back — it encoded the bug — and now pins the correct behaviour, with an integration test covering the owner-ACKed-first race against real Redis. maybeReclaimOrphans sat after the loop's empty-batch `continue` in all three consumers, so it only ran when a batch arrived. Orphans are reclaimed precisely when a stream is quiet — events.master.deletion can sit empty for days — so the feature was dead code in the case it exists for. Moved above the check; the existing lastReclaimAt throttle already prevents it firing every block timeout. Also: - The periodic pass now re-drains own pending before the orphan sweep. A handler that throws leaves its entry un-ACKed in this consumer's PEL, and the orphan sweep deliberately skips self-owned rows, so a single transient failure previously stranded a message until the process restarted. - The webhook handler's recovery path now tracks inFlightMessage and honours the abort flag, as the batch path does. Without it a shutdown mid-recovery returned from stopConsumptionLoop immediately and onModuleDestroy quit the client out from under a message still being processed — the regression #1923 fixed, reintroduced on a new path. - The startup drain is bounded by MAX_DRAIN_PAGES and bails on abort. Its termination previously depended on an ACK-or-throw invariant held across three files with no test; a future non-ACKing branch would have hung onModuleInit and with it application boot. - RECLAIM_INTERVAL_MS moved to the shared module rather than being redeclared per consumer. - Corrected docs and comments that described XAUTOCLAIM and an `id: '0'` read. Neither is used, and the module header explains why — an ADR whose purpose is audit-trail accuracy must not misstate its own implementation. - Documented OL_WORKER_ID in both .env.example files: it is a correctness-relevant knob (two replicas sharing a value share one PEL) that existed only in the plan. Rebased onto current main. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>
…K + ADR-049 durability spine (#2223) * fix(core): recover Redis stream messages stranded between read and ACK (#2164) Every consumer read with `id: '>'` — never-delivered entries only — and the repo contained zero calls to XPENDING/XCLAIM/XAUTOCLAIM, so no code path ever read a Pending Entries List. A process killed between read and ACK therefore lost its in-flight message permanently, not temporarily. The comment at job-intake.consumer.ts asserted "message will be re-delivered after timeout"; Redis has no such timeout, which is why this went unnoticed. On the webhook path the loss was also invisible: `webhook_deliveries` is stamped `job_enqueued` before the ACK, so a dropped order was indistinguishable from a delivered one. Three primitives in `@openlinker/shared/redis`, wired into all three consumer groups (webhook-handler, master-deletion-offer-pause, job-intake): - Stable consumer identity via `resolveConsumerName` (OL_WORKER_ID, else hostname). `${prefix}-${process.pid}` was wrong in both directions: in a container PID is typically 1, so replicas collided on one PEL; outside one the name changed every restart, so a process could not reach its own history. This is the precondition for everything else. - Startup drain of own pending history before switching to new messages. - Periodic orphan reclaim for work stranded by a replica that never returned, with the idle threshold floored well above p99 handler duration — a reclaim that fires early steals live work and double-runs it. Built on XPENDING + XCLAIM + XRANGE rather than XREADGROUP/XAUTOCLAIM. This is load-bearing, not stylistic: node-redis v1.5.x transforms an XREADGROUP reply through `transformTuplesReply`, which calls `.length` on the field array, so an entry trimmed while its id remained in the PEL makes the client library throw a TypeError before any of our code runs — aborting the drain and leaving the id unackable, permanently blocking that consumer's recovery. XRANGE returns an empty array for a missing id, which is an answer rather than a crash. A trimmed entry is classified as its own outcome and ACKed, never routed into a handler's error path, where it would persist a bogus dead sync_jobs row or a dead-letter entry describing a failure that never happened. Also guards `persistDeadJob`'s unconditional `markDead`: drain and reclaim make redelivery real, so a redelivered message whose job has since run would otherwise flip a live row to 'dead'. Everything holds to the Redis 6.2 command floor, so #1396's Valkey swap stays a retag rather than a redesign. Covered by 20 unit tests and 9 Testcontainers integration tests, including the crash-then-restart case the issue names and the trimmed-PEL path. Closes #2164 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * docs(adr): ADR-049 durability spine and the domain-event contract (#2165) OL has three "event" streams and zero fan-out: each has at most one consumer, each consumer does exactly one thing (turn the event into a job), and events.sync.jobs has no consumer at all. By the standard test — if the producer expects a specific outcome, it is a command wearing event clothing — all three are commands. The streams buy indirection, not decoupling. Meanwhile durability sits after four hops on the webhook path. Nine decisions, most of which are a decision NOT to build something, each with an observable reversal gate rather than an argued one: 1. The spine is the work row, written in the same transaction as the business change — so the outbox and the queue are the same table. 2. Build the contract; keep one transport. Do not build a general bus at zero fan-out. Gate: the first stream to acquire a second independent consumer. 3. If a bus: composite cursor plus a visibility barrier, never a scalar `id > cursor`. 4. eventId is derived from the business fact, never minted at insert. 5. No EntityManager in a core port signature. 6. Payload schemas are structurally incapable of carrying PII — redaction on write fails open, structural exclusion fails closed. 7. Registration-time catalog validation, not a central type union that would invert the infrastructure spine. 8. Nothing depends on a stream primitive above the Redis 6.2 floor, so #1396's Valkey swap stays a retag. 9. Redis is never the sole record of a fact — stated because it is currently false in at least three places. Numbered 049 per the #2162 reallocation: 046 was claimed by #2203 and 047 by reserved-numbers note to the #2166 branch that lands first. Also adds the docs/lessons.md entry for the commit-order gap that decision 3 rests on: an id is assigned before its transaction commits, so id order is not visibility order, and a reader advancing a scalar cursor past a gap never sees the earlier row again — silently and permanently. Closes #2165 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): trust the XCLAIM reply and run reclaim on an idle stream (#2164) Review follow-ups on the recovery primitives. Two were real defects. reclaimOrphans discarded the XCLAIM reply and re-read the body with XRANGE. XCLAIM without FORCE cannot resurrect a non-pending id, so there was no PEL leak — but the opposite failure existed: when a claim legitimately does not transfer (the owner ACKed between the XPENDING and the XCLAIM, or touched the entry so it is no longer idle past the threshold), XRANGE still returns the body, because ACK removes an entry from the PEL but not from the stream. The code then processed and ACKed a message belonging to another live consumer — defeating the very re-assertion the comment claimed to make. The claim reply is now the source of truth: node-redis returns one element per requested id and null where the claim did not transfer, so a non-transfer is either a lost race (skip; not ours) or a trimmed entry (report so the caller clears the dangling id), disambiguated by a single XRANGE. The unit spec previously mocked xClaim as resolving undefined while asserting an entry came back — it encoded the bug — and now pins the correct behaviour, with an integration test covering the owner-ACKed-first race against real Redis. maybeReclaimOrphans sat after the loop's empty-batch `continue` in all three consumers, so it only ran when a batch arrived. Orphans are reclaimed precisely when a stream is quiet — events.master.deletion can sit empty for days — so the feature was dead code in the case it exists for. Moved above the check; the existing lastReclaimAt throttle already prevents it firing every block timeout. Also: - The periodic pass now re-drains own pending before the orphan sweep. A handler that throws leaves its entry un-ACKed in this consumer's PEL, and the orphan sweep deliberately skips self-owned rows, so a single transient failure previously stranded a message until the process restarted. - The webhook handler's recovery path now tracks inFlightMessage and honours the abort flag, as the batch path does. Without it a shutdown mid-recovery returned from stopConsumptionLoop immediately and onModuleDestroy quit the client out from under a message still being processed — the regression #1923 fixed, reintroduced on a new path. - The startup drain is bounded by MAX_DRAIN_PAGES and bails on abort. Its termination previously depended on an ACK-or-throw invariant held across three files with no test; a future non-ACKing branch would have hung onModuleInit and with it application boot. - RECLAIM_INTERVAL_MS moved to the shared module rather than being redeclared per consumer. - Corrected docs and comments that described XAUTOCLAIM and an `id: '0'` read. Neither is used, and the module header explains why — an ADR whose purpose is audit-trail accuracy must not misstate its own implementation. - Documented OL_WORKER_ID in both .env.example files: it is a correctness-relevant knob (two replicas sharing a value share one PEL) that existed only in the plan. Rebased onto current main. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): stop one poison entry blocking recovery of every stranded sibling (#2164) Review findings on the recovery loops. The first is a real defect in this PR. A HANDLER THROW ABORTED THE WHOLE RECOVERY PASS, PERMANENTLY. `handleRecoveredEntry` delegates to `processMessage`, which rethrows on any non-decode error, and the drain's try/catch spanned the entire page loop — so a single entry whose handler threw aborted the pass and returned. Because `readOwnPending` always pages the PEL from the oldest id, that same entry then led every later drain and every reclaim, and it was never ACKed, so it stayed first forever. One poison message permanently starved recovery of every other stranded message: precisely the failure this recovery path exists to prevent. Each entry now runs inside its own try/catch (`recoverEntrySafely`), so a handler failure logs and the pass continues to the next entry. The failing entry stays un-ACKed and is retried on a later pass; what no longer happens is its siblings being blocked behind it. The outer catch is kept for genuine Redis-level failures. Applied to all three consumers and to both the drain and the reclaim path. REPEATED DELIVERY IS NOW REACHABLE, SO IT NEEDS A SIGNAL. Before #2164 a failing entry was simply never redelivered, so no terminal state was needed; recovery is what makes unbounded retry possible. `XPENDING` already reports a delivery counter and `toPendingRows` was discarding it. It is now surfaced as `PendingRow.deliveryCount`, threaded onto `StreamEntry`, and compared against `MAX_DELIVERY_ATTEMPTS` — crossing it logs at `error` naming the entry as needing intervention. The constant is deliberately generous: it is a terminal-state backstop, not a retry budget, so a handler failing on a transient blip is still allowed to succeed later. A missing counter defaults to 1, never 0, so a reply-shape change cannot make an entry read as never delivered. Auto-dead-lettering is deliberately NOT done, and the reason is recorded in ADR-049 with a reversal gate: two of the three consumers cannot construct their dead-letter payload from a raw pending entry (the webhook handler needs a decoded event, job-intake a parsed job request), and discarding the entry instead would be unrecoverable loss. With per-entry isolation in place the absence of a terminal state no longer blocks anything — it is noise plus a missing alarm, and the alarm now exists. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): make recovery terminate, and alarm on a counter that can actually move (#2164) Review findings on the previous commit. Both are defects it introduced. THE DRAIN NO LONGER TERMINATED, AND onModuleInit AWAITS IT. Isolating a handler failure per entry removed the only thing that ended the startup drain: previously the throw escaped to the outer catch and returned. `readOwnPending` always paged the PEL from '-', and a failing entry is never ACKed, so the same page came back every iteration and `entries.length === 0` became unreachable. The abort escape does not help — `abortController` is created by `startConsumptionLoop()`, which runs *after* the awaited drain. So one poison entry meant MAX_DRAIN_PAGES (1000) iterations of up to COUNT (10) handler invocations plus their Redis round-trips, blocking Nest bootstrap, and `drained` reported ~10,000 recovered messages for one stuck entry. The drain now pages forward with an exclusive `(<id>` cursor (Redis 6.2, the same floor XPENDING sits on), so a pass visits each entry once and terminates naturally. A failed entry stays pending for the next recovery pass rather than being re-attempted inside the current one. THE POISON ALARM COULD NEVER FIRE WHERE POISON LIVES. The previous commit keyed the alarm on Redis' `deliveriesCounter`. Redis increments that on delivery — XREADGROUP and XCLAIM — never on XPENDING or XRANGE, which is all the drain path uses. Re-presenting an entry a thousand times therefore left the counter at 1, so the branch was unreachable on exactly the path where a stuck handler accumulates. The reclaim path increments once, then `owner !== consumer` excludes the entry from every later pass, freezing it at 2. Reaching the threshold needed eleven distinct consumer identities each abandoning the entry — not a thing that happens on a stable-hostname deployment, which is the point of `resolveConsumerName`. Worse, the integration test added alongside it asserted the counter rises by calling XCLAIM directly. True, and irrelevant to the drain — it would have gone on passing while production never incremented. Counting is now local: a per-consumer `RecoveryAttemptTracker` keyed by entry id, incremented on the failure path, cleared on success so a transient failure does not leave an entry permanently near the threshold. It fires once, on the crossing, because a poison entry recurs by definition and an unguarded alarm per pass is fatigue on the channel meant to carry real incidents. Redis' `deliveryCount` is kept as diagnostic context — genuinely useful for cross-replica churn — with its limits and its reclaim-path off-by-one documented on the field. Also: - `recoverEntrySafely` rethrows when the abort signal is set, so a shutdown-time failure ends the pass instead of grinding the rest of the page against a quitting client. - The head-of-line fix is now pinned by integration tests, which is what the review said was missing and what would have caught the non-termination above: a page whose first entry is never ACKed still reaches its siblings; the scan ends rather than re-reading; and Redis' own counter is asserted frozen across drain re-reads, which is the evidence for counting locally. - ADR-049's known-gap paragraph corrected — it presented the delivery-count alarm as the compensating control. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): stop the recovery log over-reporting, and correct the reclaim delivery count (#2164) Two remaining review findings. `drained += entries.length` counted entries ATTEMPTED, not entries handled. The exclusive-cursor fix removed the 1000x inflation, but a pass that failed on three of a ten-entry page still logged "Recovered 10 pending message(s)". That line is read by an operator during the incident it is describing, so over-reporting there is the same class of dishonesty this epic exists to remove. `recoverEntrySafely` now reports whether the entry was handled and the drain counts only successes. The reclaim path's `reclaimed` count is deliberately left as-is: it reports what `reclaimOrphans` actually did — take ownership of N entries — which is true independently of whether handling then succeeded. `deliveryCount` on the reclaim path was stale by exactly one: the XPENDING listing is read before that pass's own XCLAIM, and XCLAIM is itself a delivery. Now compensated at the point of construction rather than only described in the field's doc comment. Still open, deliberately, and listed so they are not mistaken for oversights: the `trimmed` check is expressed in two places (`resolvePendingEntry` and the reclaim branch), `recoverEntrySafely` is triplicated across the three consumers (the attempt counter it depends on is already single-source in `RecoveryAttemptTracker`, which was the load-bearing part), and the two shared Redis modules colocate their types rather than using `*.types.ts` — a deviation from the written standard that matches roughly half of `libs/shared` today. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): report recovery honestly, and hold the shutdown guarantee the code claimed (#2164) Six review findings. Two are defects in the immediately preceding commit; one contradicts a claim that commit's own message made. A DISCARDED ENTRY WAS COUNTED AND LOGGED AS RECOVERED. The previous commit made the drain count successes instead of attempts, using a boolean. A `trimmed` entry ACKs successfully — its payload is gone, destroyed by retention — so it returned true and was counted. A boot finding ten stranded webhook events whose payloads had all been trimmed logged "Recovered 10 pending webhook event(s)". That is worse than the over-count it replaced: it reports permanent loss as successful recovery, on the path where lost work is a dropped order whose delivery row still reads `published`. A boolean cannot express three outcomes, which is why it was wrong on first use. `recoverEntrySafely` now returns `RecoveryOutcome` — recovered / discarded / failed — and the drain counts each, reporting discards separately and at warn. `reclaimed` COUNTED ENTRIES THAT WERE NEVER CLAIMED. The previous commit deliberately left this alone and justified it: "it reports what `reclaimOrphans` actually did — take ownership of N entries". That is false against the code. `reclaimOrphans` also returns a `trimmed` entry on the path where XCLAIM did NOT transfer and XRANGE then found the data gone; no ownership was taken there. Now counts only entries whose claim actually transferred. THE SHUTDOWN GUARDS WERE BOTH DEAD ON THE PATH THAT NEEDED THEM. `onModuleInit` awaits the drain and only then calls `startConsumptionLoop`, which is where `abortController` was created. So during the startup drain — the one pass that can run for many pages — both the loop's abort check and `recoverEntrySafely`'s shutdown rethrow were reading an undefined controller and could never fire. A shutdown arriving mid-drain was invisible to it, which is exactly what the rethrow's comment says it prevents. The controller is now created before the drain, and `startConsumptionLoop` replaces it only when a previous run aborted (the restart-after-backoff path). Also: - `pending-retry` still had the head-of-line defect the drain was fixed for: it re-read the oldest COUNT ids from '-' every tick with no cursor, so poison at the head starved every later own-pending entry for the process lifetime. Now paged with the same exclusive cursor, capped per tick so recovery cannot monopolise the consume loop. - The attempt tracker evicted the id that had been stuck LONGEST, because a Map `set` on an existing key does not reorder. Now delete-then-set, making eviction least-recently-failed. - The alarm fired at MAX_RECOVERY_ATTEMPTS + 1 while the constant is documented as the threshold. Now fires at the documented value. - Redis' `deliveryCount` had no production reader; it is now in the failure log line, which is where an operator distinguishing cross-replica churn from a locally-stuck handler would look. - The frozen-counter integration assertion compared the readings to themselves, so a server reporting 0 every time would have passed while breaking the "defaults to 1, never 0" invariant. Now asserts the absolute value. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * test(worker): pin the recovery-outcome contract that reported loss as recovery (#2164) The previous round fixed the defect but not the gap that let it ship. The review's finding was specifically that the drain's counting logic has no test — "shipping it with no test means B1 was undetectable and a future regression back to \`drained += entries.length\` is equally undetectable" — and the tracker unit tests added instead cover a different thing. Four cases on the consumer itself: a processed entry reports 'recovered', a trimmed entry reports 'discarded' (never 'recovered' — its payload is gone), a throwing handler reports 'failed' rather than propagating and aborting the pass for its siblings, and a shutdown-time failure rethrows instead of being swallowed as a handler error. Verified by injection rather than assumed: reverting the trimmed branch to return 'recovered' fails the second case, and only that case. A test that cannot fail is the same mistake as the self-referential assertion this round already corrected. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> --------- Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>
2 participants
Implements the three measured, self-contained items from #1920. Item #2 (pre-baked PrestaShop image) and item #4 (merging suite files) are deliberately not here - see Scope below.
1. Bounded in-flight drain instead of a flat 2 s sleep on shutdown
WebhookToJobHandler.stopConsumptionLoopsleptsetTimeout(2000)on every shutdown, "to let in-flight messages complete". Because ~77 int-specs tear the harness down, that was ~2 s x 77 paid per CI run, and it gave no real guarantee (a slow message was cut off at 2 s anyway).The handler now tracks the
processMessagecall actually in progress and awaits that, bounded by the same 2 s. Nothing in flight (the common case, including every int-spec teardown) means shutdown returns immediately.It deliberately does not await the consume loop itself: the loop parks in
XREADGROUP ... BLOCK 5000, so awaiting it would stall shutdown for up to 5 s.BLOCK_MSis left alone, andquit()is kept - swapping it fordisconnect()was measured on the same sample and buys nothing (see the issue's Verification log; that was a refuted hypothesis, not an oversight).2. Truncate only the tables a test actually dirtied
truncateTablesissued oneTRUNCATE ... CASCADEper configured table on everyafterEach, all 18 of them, whether the test touched them or not. Measured against the live test container:SELECT 1(round-trip baseline)TRUNCATE1 tableTRUNCATEthe 18 configured tablesTRUNCATEall 46 tables in the schemaThe cost is linear in the table count and flat in the row count - ~10 ms per table even when empty - while a typical test dirties two or three. At ~207 ms x 485 api tests that was ~100 s per run spent clearing empty tables.
Now: one probe round-trip (
SELECT 't' WHERE EXISTS (SELECT 1 FROM "t")UNION ALL'd over the list) narrows the list, then a singleTRUNCATEclears just those. A test that dirtied nothing issues noTRUNCATEat all. Measured: ~207 ms -> ~13 ms per reset.Two things that look like the fix and are not (both measured, both rejected - don't re-litigate them without new numbers):
TRUNCATE a, b, c CASCADEfor all 18): 280 ms -> 215 ms only. The cost is per-table, not per-round-trip.fsync=off,synchronous_commit=off,full_page_writes=off,wal_level=minimal): no gain at all, and passing them viawithCommandmade the 12-suite sample worse (30.3 s vs 19.2 s).CASCADEwas never the mechanism either: the schema has 4 foreign keys in total.3. Persist the jest transform cache
Both integration configs now point
cacheDirectoryat.jest-cache/{api,worker}-integration(gitignored), and thetest-integrationjob restores/saves it viaactions/cache.A cold ts-jest cache costs the first suite of each step ~32 s on CI, because every int-spec pulls the whole
AppModulegraph through ts-jest. Measured locally on the same file: 6.42 s warm vs 76.29 s cold. This is also what madeorder-reingestion-echo-guardlook like a 37 s spec in the CI log - it is the run's first file, and its ownittakes 149 ms. The worker step pays the same tax on its first suite (35.8 s against a 7.5 s floor).ts-jest keys entries by file content, so restoring an older cache is safe: changed files miss and re-transform.
Measurements
12-suite sample, same machine, each change applied in isolation and reverted:
4. One shared PrestaShop container instead of four
Three PS specs now share a single container, booted on first use:
orders/allegro-prestashop-carrier-mapping,prestashop/prestashop-order-fulfillment-update,orders/prestashop-harness-smoke.prestashop/prestashop-webhook-provisioningdeliberately keeps its own fresh container: its subject is the from-scratch install ("writes the threeOPENLINKER_*configurations on first install"), and installing the OL module writes those same rows - sharing would erase what the spec asserts.Phase breakdown of the boot this removes (instrumented locally, warm image cache):
startMysqlstartPrestashop(container start incl. PS installing itself)installOpenLinkerModuleIntoContainer(module +cache:clear+cache:warmup)Three design points worth a reviewer's attention:
psOrderIdthe spec just created, so orders and carts left by an earlier file are inert. Verified by reading each assertion, not assumed.processobject carrying a copy ofprocess.env, so a value written by one int-spec is invisible to the next - measured, not assumed (same pid, second file reports no cached value). That is also whystartContainers'CONTAINERS_PRIMED_ENV_VARworks for Postgres/Redis:globalSetupsets it in the parent before the worker gets its copy, a route a lazily-booted container cannot use. The record is liveness-checked withdocker inspectbefore reuse, so a record left by a crashed run boots fresh instead of failing every spec with a connection error.cleanupis a closure theglobalTeardownrealm cannot see, so the worker records the container ids and teardown stops them. Leaving them to the CI orphan sweep ([BUG] Infrastructure - Integration test Testcontainers leak on self-hosted CI runner exhausts docker0 bridge FDB, breaking pipelines #1285) would be a backstop, not a plan.The shared container is always module-installed (the module-installed carrier satisfies the no-module spec's assertions; the reverse does not hold). The
OL_SKIP_PS_MODULE_INSTALL=trueescape hatch still works - in that mode a spec takes its own module-less container.Measured locally: the three sharing specs went ~380 s -> 132.3 s, and the full api integration suite 622.6 s -> 431.2 s (85 suites, 501 tests, zero failures).
On CI the per-suite effect is unambiguous - two boots gone, -139 s across the four:
allegro-prestashop-carrier-mapping(boots the shared one)prestashop-order-fulfillment-updateprestashop-harness-smokeprestashop-webhook-provisioning(own container, unchanged)Confirmed on an idle runner (re-run of the same commit, 0/4 runners busy, empty queue):
Run integration tests(apps/api)Run worker integration testsIntegration TestsjobThe first run of this commit read 578 s for the api step and was not comparable: the runner was saturated (4/4 busy, 5 runs queued). All 17 non-PS suites present in both runs were slower that time, by +2.7 s each on average, including worker specs this PR cannot touch (
job-intake-execution16.7 -> 41.3 s) - not one was faster, and the worker step moved 137 -> 180 s on identical code. Re-measured once the queue drained.Measured on CI
Run 30455348427, same self-hosted runner class, against the baseline of run 30443862548:
Run integration tests(apps/api)Run worker integration testsIntegration TestsjobSame coverage on both sides: 81 suites / 485 passed / 7 skipped.
Supporting detail: in the baseline 81 of 81 suites reported a duration (jest prints one only above ~5 s); with this PR only 21 do. Per-suite deltas on the comparable ones run -1.7 s to -4.4 s, matching the local A/B.
Read the first run of this PR with care. Its api step showed only 797 s, because the
actions/cachestep was necessarily a MISS on the first run (nothing to restore yet) — and the run's first suite happened to beallegro-prestashop-carrier-mapping, the heaviest import graph in the repo, so it absorbed the whole cold-transform tax and read as 224 s instead of ~108 s. On the re-run, with the cache restored (Cache jest transform output= 3 s), that same suite is back to 108.9 s. Nothing regressed; the tax simply moved onto a fatter file before item 3 could do its job.What is left in the 517 s is now dominated by the four PrestaShop suites (108.9 + 99.6 + 51.5 + 47.9 = 308 s, 60% of the step), which is exactly what issue item #2 (the pre-baked image) targets.
Scope: what is NOT in this PR
actions/cachestep here is a cache, not a topology change.Verification
pnpm lint(incl.check:invariants) - cleantype-checkfor@openlinker/test-kit+@openlinker/api- cleanpnpm --filter @openlinker/api test- 78 suites / 1058 tests (5 new: shutdown returns immediately with nothing in flight, waits for an in-flight message, gives up after the bound, survives a failing in-flight message, still quits)pnpm --filter @openlinker/test-kit test- 7 tests (4 new around the probe/skip semantics)webhook-ingestion.int-spec.tswhose [TASK] Testing — webhook integration tests should drain the enqueued job through the worker handler #1511 case requires the running consumer to actually drain a published event and enqueue the job - the test that would catch a broken shutdownDocs
docs/testing-guide.md- the Testcontainers lifecycle section now describes the reset semantics correctly (only non-empty tables truncated, and why) and corrects a standing inaccuracy: it claimed migrations run before the integration suite. They do not - the schema comes from TypeORMsynchronize, so the suite never proves the migrations reproduce the entity schema. That gap is called out, not fixed here.docs/lessons.md- regression-ledger entry: profile harness phases before optimising a "slow suite"; four plausible hypotheses were wrong on this one.Closes #1920