fix(va-apple-music-url-remediation): bind the album_metadata id list as a PG array literal - #2008
fix(va-apple-music-url-remediation): bind the album_metadata id list as a PG array literal#2008jakebromberg wants to merge 2 commits into
Conversation
…as a PG array literal
`invalidateAlbumBatch` interpolated a bare `number[]` into its `sql` template. Drizzle expands a JS array into a comma-separated parameter list, so Postgres received `ANY(($1, $2, … $202))` — a row constructor, which `ANY` rejects at parse time (42809, "op ANY/ALL (array) requires array on right side"). The failure is dataset-independent, so the album_metadata phase has never written a row: the 2026-08-06 run reported 206 candidates and 0 invalidated.
Bind a `{1,2,3}` array-literal string with an explicit `::int[]` cast instead, matching the inline form in `jobs/album-critic-reviews-etl/antijoin.ts` (the BS#1068/BS#1071 trap) and five other jobs. Single call site, so no new helper.
Regression coverage at two levels. A new integration spec executes both statement shapes against real Postgres — the row-constructor form must be rejected with 42809, the array-literal form must invalidate exactly the targeted rows, honor the IS NOT NULL guard, and carry a full 202-wide page. A unit assertion pins the bound value and the cast, scoped to what the mock can actually observe: `tests/__mocks__/drizzle-orm.ts` stubs the `sql` tag and the suite's `renderSql` helper splices values inline, so no unit-level view can see the wire shape — which is why the existing assertions stayed green through the broken run.
Closes #2007
|
CI has not run yet — blocked by an active GitHub Actions platform incident, not by this branch. Run 31121659641: githubstatus.com reports Actions in Not re-running while the incident is open — each attempt burns billed minutes and cannot succeed. Re-trigger once Actions is green ( Local results are in the PR body: lint, format:check, typecheck and the full unit suite (410 suites / 6498 tests) all pass. The new integration spec has never been executed — no Docker daemon on the authoring machine — so CI remains its first and only run. |
…ta invalidation Review follow-up to the ANY()-binding fix. The regression test did not test the code. The spec's UPDATE was a hand-mirrored string constant that no code path read, so reverting the fix left it green — the only coupling to the shipped statement was a comment. Rebuild it on the `artist-unicode-dedup-merge.spec.js` precedent (itself added for BS#1897 review MED-1): tsup now emits a CommonJS bundle alongside the ESM entrypoint, and the spec `require`s `dist/orchestrate.cjs` and calls the real exported `invalidateAlbumBatch` against Postgres. Verified both directions — with the bare-array bind restored, 5 of the 6 tests fail with the production error `42809 op ANY/ALL (array) requires array on right side`. Add the missing compare-and-set. `applyFlowsheetBatch` guards on `IS NOT DISTINCT FROM v."old_url"` because this job overwrites a non-null value while two other writers touch the column; the album arm had only `apple_music_url IS NOT NULL`, which merely skips already-null rows. Between phase 2's page SELECT and its UPDATE, the enrichment worker can re-verify an album through LML's post-#1139 guarded matcher and write the correct url as 'verified' — the old predicate would null that and reset its re-ask budget, opening a DJ-visible window through flowsheet.service.ts's coalesce until the BS#1915 sweep re-healed it. The page SELECT now carries each row's observed url and the UPDATE is a VALUES join, mirroring the flowsheet arm. That also removes the array bind entirely rather than fixing it: each id and url is its own parameter, so nothing hand-rolls a PG array literal (urls are text and would need real escaping). Pin `updated_at`. Migration 0084's BEFORE UPDATE trigger is flowsheet-only, so nothing stamps album_metadata for us; dropping the SET would silently freeze the freshness signal the BS#1915 sweep and the CDC consumers read, and no test noticed. Asserted in both tiers — the mirror image of the flowsheet arm's `not.toMatch(/updated_at/)`. Document the phase-2-only re-run. `runRemediation` always runs flowsheet first and gates phase 2 on its success, and production already completed the flowsheet arm on 2026-08-06, so the corrective re-run this unblocks would otherwise re-spend LML on 56 surviving triples at up to 3 passes each — and skip the 206 album rows a second time if anything tripped. Parking `VA_REMEDIATION_FLOWSHEET_AFTER_ID` past max(flowsheet.id) is now documented as the supported way to run phase 2 alone, with the 2026-08-06 result recorded. Also drop the spec comment that described `IS NOT NULL` as the guard against a concurrent write, which it never was.
|
Review follow-up pushed (d879a10). All four merge-blocking findings addressed; details in the updated PR body. The headline change is finding 1: the spec now That run used a throwaway local Postgres 18 (migrations applied by hand plus a minimal FK fixture), not the Docker CI stack — there is still no Docker daemon on the authoring machine, so the full integration tier has not run. CI still has not run. Actions remains in |
|
#2011's fixture-marker convention landed in #2012 — a `beforeAll` pre-clean keyed on a stable predicate in the seeded data (e.g. `album_title LIKE 'bs1915-reask-test-%'`), rather than the in-memory id array `afterAll` alone relies on. Not stacked on this branch deliberately, per #2011's sequencing note, since this PR is already blocked on the CI outage. `va-apple-music-url-remediation-invalidate.spec.js` inherits the same `afterAll`-only shape from its donor (`enrichment-worker-streaming-reask.spec.js`), and its fixture rows are the exact ones #2011 is about — library rows with a V/A-shaped `artist_name` joined to `album_metadata` rows with a non-null `apple_music_url`, which is `albumMetadataNet` itself. Worth adding the same `beforeAll` pre-clean (a distinctive marker in `library.album_title`, e.g. `bs2000-invalidate-%`) when this rebases past #2012. |
|
Heads-up on a merge conflict, verified by execution rather than inspection. PR #2014 (the BS#2010 lint rule) wraps this PR's Resolution is mechanical: take this PR's body (the If a resolver keeps the pair by accident it degrades to a warning, not a build break — One coupling worth knowing for the future: linting the tree with the rule removed from the config turns those directive comments into hard errors ( PR #2013 (BS#2009) is also live in this file but does not touch |
1 participant
Closes #2007
What was broken
jobs/va-apple-music-url-remediation/orchestrate.ts'sinvalidateAlbumBatchbound its id list as a bare JS array:Drizzle expands a JS array inside a
sqltemplate into a comma-separated parameter list, so Postgres receivedANY(($1, $2, ... $202))— a row constructor, not an array.ANYrequires an array, and the rejection happens at parse time (SQLSTATE42809,op ANY/ALL (array) requires array on right side), which makes it dataset-independent: the shipped statement could never have invalidated a row, on any page, for any input.The 2026-08-06 09:33 PDT run (
run_id47dfff79-44d7-4768-8a18-3dd49278dc67) reportedalbum_metadata: {"candidates":206,"invalidated":0,"batches":1}. The flowsheet phase of that same run completed correctly (52 nulled, 56 verified and kept, 10 indeterminate) — only thealbum_metadataarm is broken, and it is the DJ-visible one, sinceflowsheet.service.tsservescoalesce(album_metadata.apple_music_url, flowsheet.apple_music_url).The fix
invalidateAlbumBatchnow sends a VALUES-join UPDATE, mirroringapplyFlowsheetBatchtwenty lines above it in the same file. The page SELECT carries each row's observedapple_music_urlalong with its id.This does two things at once. It removes the array bind entirely rather than repairing it — each id and url is its own parameter, so nothing has to hand-roll a PG array literal (urls are text and would need real escaping, which is the same class of hazard the original bug came from). And it carries the compare-and-set the album arm was missing.
Audited the rest of the job:
ANY(appeared exactly once injobs/va-apple-music-url-remediation/, and no longer appears at all. Every other binding inorchestrate.tsis a scalar; no other file in the job contains SQL.Compare-and-set (review finding 2)
The album arm had only
apple_music_url IS NOT NULL, which merely skips already-null rows. The race that guards against nothing: phase 2 SELECTs album 4711 with its polluted url →apps/enrichment-worker/enrich.tsre-verifies it through LML's post-#1139 guarded matcher and writes the correct url withapple_music_status='verified'→ our UPDATE nulls it and resetsstreaming_reask_attemptsto 0. It self-heals through the BS#1915 sweep, but it is an avoidable round-trip and a DJ-visible null window.Now
AND t."apple_music_url" IS NOT DISTINCT FROM v."old_url", the same guard and the same rationale as the flowsheet arm. Everything else is unchanged: stillapple_music_status='unresolved', stillstreaming_reask_attempts=0.updated_at(review finding 3)Migration 0084's BEFORE UPDATE trigger is flowsheet-only, so nothing stamps
album_metadatafor us. Dropping"updated_at" = NOW()would have silently frozen the freshness signal the BS#1915streaming-reask.tssweep and the CDC consumers read, and every test would still have passed. Now asserted in both tiers — the mirror image of the flowsheet arm'sexpect(updateSql).not.toMatch(/updated_at/).README: the phase-2-only re-run (review finding 4)
runRemediationalways runs the flowsheet phase first and gates phase 2 on!flowsheet.failed. Production already completed the flowsheet arm, so a naive corrective re-run would re-spend LML on the ~56 surviving triples at up to 3 passes each with a 15 s inter-pass delay — and if the rescue-rate detector or a write error tripped,album_phase_skippedwould fire and the 206 rows would be skipped a second time.Parking
VA_REMEDIATION_FLOWSHEET_AFTER_IDpastmax(flowsheet.id)is now documented under Running phase 2 alone, with both reasons and the warning to read the ceiling from a livemax(id)rather than the previous run'slast_id(which is only the table max if that run reached the end). The 2026-08-06 result is recorded under Run result.Test coverage
The previous revision's regression test did not test the code. Its
UPDATE_SET/arrayLiteralStatementwere string constants that no code path inorchestrate.tsread: reverting the fix, switching to::bigint[], or swapping in drizzle'sinArray()all left it green. The only coupling to the shipped statement was a comment.Rebuilt on the precedent already in this repo:
tests/integration/artist-unicode-dedup-merge.spec.jsrequires the compileddist/merge.cjsand runs the real exported functions (added for BS#1897 review MED-1 — the same finding, litigated here once before). So:jobs/va-apple-music-url-remediation/tsup.config.tsnow emitsorchestrate.tsin both formats, producingdist/orchestrate.cjsbeside the ESMdist/job.jsentrypoint. Byte-for-byte the arrangementjobs/artist-unicode-dedupuses, including the sharedchunk-*.jsthe ESM entry now imports;Dockerfile.va-apple-music-url-remediationcopies the wholedist/directory, soENTRYPOINT ["node", ".../dist/job.js"]is unaffected.requires that bundle,jest.unmock('drizzle-orm')(the repo-wide manual mock is auto-applied even in the integration tier — same line the dedup spec carries), and calls the realinvalidateAlbumBatch.npm run buildcoversjobs/**, and CI's integration job runs it before the DB init, sodist/orchestrate.cjsexists in CI. Documented in the spec header for local runs.Six tests: the statement executes and invalidates its targets while sparing an untargeted row;
updated_atadvances; a url that changed under us is left alone; a mixed page writes only the still-matching rows; a full 202-wide production page; empty-page no-op.Verified in both directions. With the bare-array bind restored and the bundle rebuilt, 5 of the 6 fail with
PostgresError: op ANY/ALL (array) requires array on right side(the sixth is the empty-page no-op, which short-circuits before the SQL). Restored and rebuilt, all 6 pass. Two unit assertions also flip red on the same revert.Unit tier limits, stated plainly.
tests/__mocks__/drizzle-orm.tsstubs thesqltag as{ sql: strings, values }and@wxyc/databaseresolves totests/mocks/database.mock.ts, so nothing in the unit suite serializes or parses SQL — there is no unit-level view of the wire shape. The suite'srenderSqlhelper additionally splices bound values inline, which is why the pre-existinginvalidateAlbumBatch SQLtest stayed green through the entire broken run: it rendered the defective predicate asANY(), the array collapsing to an empty string.renderSqlnow carries a comment saying so. The unit assertions that remain (compare-and-set,updated_at, no bare-array bind) are real, but the proof that the statement is valid SQL that writes the right rows comes only from the integration spec.Checks run
npm run lintnpm run format:checknpm run typechecknpx tsc --noEmit -p jobs/va-apple-music-url-remediation/tsconfig.jsonnpm run test:unittests/integration/va-apple-music-url-remediation-invalidate.spec.jsnpm run test:integration(full tier)GitHub Actions is still in a
major_outage(critical incident opened 2026-08-06 08:22 PDT), so CI has not run and was not re-triggered — run 31121659641's bootstrap jobs were cancelled with no runner ever allocated. This must go green before merge, in particular the new integration spec under the real CI fixture.Pre-existing job typecheck errors — not noise
npm run typecheckdoes not coverjobs/**, so the job was checked directly. Its twoTS2554errors are the dead cooperative-pause bug, not incidental:CheckLiveActivityFnis(lookbackSeconds: number) => Promise<boolean>— a detector, not a pause — butorchestrate.tscallsawait opts.checkLive(opts.lookbackSeconds, opts.pauseMs)and discards the boolean, so the job never defers to a live DJ. Because the typecheck script skipsjobs/**, CI is green on a job that does not compile. Left alone here at the reviewer's direction; it is going into a separate ticket along with the unguardedcheckLivethrow, the per-row-vs-per-page probe asymmetry, the seventh inlineintArrayLiteralcopy, theas unknown ashardening, and the fixture pre-clean. The rescue-rate detector is #2006's scope and is untouched.