fix(va-apple-music-url-remediation): make the cooperative live-DJ pause real - #2013
fix(va-apple-music-url-remediation): make the cooperative live-DJ pause real#2013jakebromberg wants to merge 3 commits into
Conversation
…se real, fail-open on probe errors, and per-page not per-row Three defects in the same small piece of plumbing (BS#2009). CheckLiveActivityFn is a detector, not a sleeper, but both phase call sites awaited it and discarded the boolean while passing a pauseMs argument the function doesn't accept — the pause has never actually paused, and the job did not compile (two TS2554s). A probe throw escaped both phases' try/catch, killing the run before it could emit its summary log line and losing both resume cursors. The flowsheet phase also probed once per row instead of once per page, which was invisible only because the discarded result meant nothing happened anyway. Ports waitForQuietPeriod + a fail-open safeProbe from the streaming-url-remediation / flowsheet-ghost-row-sweep donors, built once in runRemediation and shared by both phases so the probe is issued once per page load in each. A throwing probe is now logged, captured, and treated as no-activity rather than propagating. Adds a narrow per-job typecheck (package.json script + a dedicated CI step) since npm run typecheck does not cover jobs/** and this job's tsup build is transpile-only, which is how it shipped non-compiling in the first place. Scoped to this job rather than widening typecheck to all of jobs/**, which would surface an unrelated pile of errors across ~20 other jobs. README records why option (a) — a real pause — was chosen over deleting the plumbing: both sibling one-shot jobs already implement it, the docs already promised it, and the job's next run writes to album_metadata during hours DJs may be live.
…ion, and stop pauseMs<=0 from spinning the probe Review of PR #2013 found two gaps. First, the "sleeps while active, proceeds when quiet" test never actually exercised a sleep: the outer beforeEach pins LIVE_ACTIVITY_PAUSE_MS='0' and the test never overrode it, so stopAwareSleep(0) was a no-op, and its >=2 call-count assertion passed on pure per-page probing alone with no looping at all. Replacing the whole loop with a single discarded probe call left the suite green. Fixed by giving that test a real nonzero pause, asserting on elapsed wall-clock time (the one thing a mutation that deletes the loop cannot fake without also being far slower than a genuine no-op), and pinning the exact call count for the fixture rather than a loose lower bound. Second, LIVE_ACTIVITY_PAUSE_MS=0 is a legal, unremarkable value (requireNonNegativeInt allows it, and nothing before this pause existed made it dangerous), but stopAwareSleep(0) returns without awaiting a timer, so a probe that keeps reporting activity degenerates into an unthrottled hot loop against RDS for the run's entire duration instead of a cooperative pause. Gate on pauseMs<=0 the same way as lookbackSeconds<=0, with a bounded regression test (the mock caps itself after 50 calls so a future regression fails the assertion instead of hanging the suite). Both fixes verified by mutation: reintroduced each defect in isolation, confirmed the relevant test(s) go red, and reverted byte-identical. Also fixed a latent, pre-existing test-isolation leak surfaced by the new precise call-count assertions: one existing test over-queued a db.execute mock value that its own code path never consumes, which was silently bleeding into whatever test ran next.
|
Addressed both review findings in 56e4479. 1. Pause loop is now defended by mutation-verified testsApplied your exact mutation (loop/sleep deleted, single discarded
Verified each of the 4 defects independently by mutation (apply → confirm red → revert → confirm identical + green):
2. pauseMs<=0 no longer spinsOne-line fix as you specified: Bonus: fixed a latent test-isolation leak your review's precision surfaced
Local checks (still no CI —
|
… knob, and bound the lookback=0 regression test Two nits from the second review pass. LIVE_ACTIVITY_PAUSE_MS=0 became a second way to disable this job's pause but nothing said so: the shared docs/env-vars.md entry describes only the sleep duration (correctly — the other jobs reusing that var have no such gate), and this job's README still named only LIVE_ACTIVITY_LOOKBACK_SECONDS=0. Added the job-specific caveat to both docs, and softened the orchestrate.ts comment's inaccurate "is a documented way" to point at where the semantics actually live instead of asserting they already did. The 'skips the probe entirely when the lookback is 0' test used an unconditionally-active probe; mutating out just the lookbackSeconds<=0 clause (leaving pauseMs<=0 intact) hung the suite to its timeout instead of failing, asymmetric with the sibling pauseMs<=0 test that was deliberately capped for exactly this reason. Gave it the same bounded mock. Verified: the isolated mutation now fails in ~1.5s instead of timing out.
|
Both nits folded into 36f2fca. A. Documented the second disable knob
B. Bounded the lookback=0 testSame treatment as the pauseMs<=0 test: capped the probe mock at 50 calls. Verified by isolating the exact mutation you found (drop the Local checks
No other files touched. Still on |
1 participant
Summary
Three defects in
jobs/va-apple-music-url-remediation's cooperative live-DJ pause, all in the same small piece of plumbing:CheckLiveActivityFnis a detector ((lookbackSeconds) => Promise<boolean>), not a sleeper — sleeping is the caller's job. Both phase call sites didif (opts.pauseMs > 0) await opts.checkLive(opts.lookbackSeconds, opts.pauseMs), passing a second argument the function doesn't accept and discarding the boolean it returned. The job ran the probe SELECT, threw away the answer, and proceeded regardless. These two call sites were also the job's twoTS2554compile errors — the job did not compile, and shipped anyway becausenpm run typecheckcoversapps/**/shared/**/@wxyc/databasebut notjobs/**, and the job'stsup --minifybuild is esbuild (transpile-only, no typecheck).checkLivewas awaited outside both phases'try/catch, so a transient RDS blip on the probe SELECT propagated out ofrunRemediationunhandled — the run died mid-page with nosummarylog line, losing bothlast_idcursors.VA_REMEDIATION_BATCH_SIZE = 2000page cost up to 2000 extra round-trips ahead of every LML lookup. Invisible only because defect 1 meant the result was discarded anyway.Fix
Ports
waitForQuietPeriod+ a fail-opensafeProbefrom thestreaming-url-remediation/flowsheet-ghost-row-sweepdonors, verbatim in shape: built once inrunRemediation, shared by both phases, probed once per page before that page loads. A throwing probe is now logged,captureErrord, and treated as no-activity rather than escaping the phase.Decision recorded in the README (option (a), make the pause real, over (b) delete the plumbing): both sibling one-shot jobs already implement a real pause, the README and CLAUDE.md workspace-table row already promised the behavior, and the job's next production run issues ~206 UPDATEs against
album_metadataduring hours DJs may be live.Compile visibility (the third tsc error that turned out not to exist)
Per the issue, a naive
npx tsc --noEmit -p jobs/va-apple-music-url-remediationin a fresh worktree also surfaces aTS2322onlml-fetch.ts'scaller: 'va-apple-music-url-remediation'. This is not a real registration gap.'va-apple-music-url-remediation'is already registered as a class-5 caller inshared/lml-client/src/policy.ts(ALL_LML_CALLERS+CALLER_CLASS), added in the same commit that introduced this job (0789d05b). TheTS2322is a fresh-worktree artifact:@wxyc/lml-client'sdist/index.d.tsdoesn't exist untilshared/lml-clientis built (thelint:prebuildstep CI runs before the realType checkstep), and without it TS falls back to a degraded resolution of the module that manifests as a bogus "not assignable" error on the caller literal rather than the "cannot find module" errors that show up on every other unbuilt import in the same raw run. Afternpm run build --workspace=@wxyc/lml-client(whatlint:prebuilddoes), only the two genuineTS2554s remain. Independently confirmed via the existing CI guard built for exactly this invariant:node scripts/check-lml-caller-classification.mjspasses today, unmodified. No change topolicy.tsin this PR, and no runtime behavior change — this caller has run under its class-5 policy (X-Caller-Budget-Msheader,LML_CLASS5_TIMEOUT_MS-derived budget; the job's own explicittimeoutMsoverride always wins over the class default) since the job first merged.Compile-visibility fix (the real defect)
npm run typecheckintentionally does not coverjobs/**(fleet-wide, ~20 jobs, out of scope to widen here). Added narrow coverage instead:"typecheck": "tsc --noEmit"in this job'spackage.json(mirrors the existing-but-CI-unwired pattern injobs/artist-identity-etl).Type check: va-apple-music-url-remediation (BS#2009)step in.github/workflows/test.yml'slint-and-typecheckjob, right after the rootType checkstep (whoselint:prebuildalready builds this job's@wxyc/database/@wxyc/lml-clientdeps).Verified the new step actually gates: reintroduced the original two-arg
checkLivecall locally and confirmednpm run typecheck --workspace=jobs/va-apple-music-url-remediationfails with the originalTS2554s before reverting.Local checks (GitHub Actions is in
major_outage— no CI ran; these are the actual local results)npx tsc --noEmit -p jobs/va-apple-music-url-remediation— clean, no output.npm run typecheck— clean (all workspaces).npm run lint— 0 errors, 835 pre-existing warnings (unrelated files; none introduced by this diff).npm run format:check— clean.npx jest --config jest.unit.config.ts tests/unit/jobs/va-apple-music-url-remediation/orchestrate.test.ts— 26/26 passed (4 new: sleep-while-active/proceed-when-quiet, once-per-page not once-per-row, throwing-probe doesn't abort + summary carries both cursors, lookback=0 disables the probe entirely).tests/unit/jobs/va-apple-music-url-remediation) — 78/78 passed.npm run test:unit— 410/410 suites, 6501/6501 tests passed (the four suites flagged as known Node-26-timer flake in this environment — album-plays-refresh, cdc-websocket, sse-metrics, lml.client — passed clean this run).lint-and-typecheckguard scripts (lint:migrations, cross-cache-identity flags, precondition guards,legacy_entry_idwrites, LML caller classification, bulk-update+ANALYZE pairing, auth-tables doc) all pass.Rebase hazard
Unmerged PR #2008 (
bugfix/album-metadata-any-array) rewritesinvalidateAlbumBatchand the album-phase page SELECT in this sameorchestrate.ts. This PR does not touchinvalidateAlbumBatch; the album-phase SELECT is touched only to move the (now real) pause probe ahead of it. Expect a rebase against whichever of #2008/this PR merges second.What the issue got wrong
The
TS2322"unregistered caller" claim above — see "Compile visibility" section. The caller was already correctly registered; only the fresh-worktree build artifact made it look otherwise.Closes #2009