Infra and test hygiene: CI gates, k8s probes, fixture/env-restore fixes #26
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/infra-test-hygiene"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Audit config/infra/tests group.
CI/infra:
Tests:
Verified: all touched test files pass; full suite green on combined tree.
🤖 Skippy PR review
2 findings — 1 blocking before merge.
priv/repo/migrations/20260920160342_replace_hrrr_profiles_null_surface_refractivity_index.exs:97config/dev.exs:122start_notify_listeneris read by nothing, so it does not enable anything in devReviewed
420bf907d34d. Commentskippy reviewto re-run.@ -120,2 +119,4 @@config :microwaveprop, start_freshness_monitor: true# Rust pipeline NOTIFY consumer (ScoreCache invalidation + /map refresh)config :microwaveprop, start_notify_listener: true🔵 Suggestion —
start_notify_listeneris read by nothing, so it does not enable anything in devThis is the only occurrence of
:start_notify_listenerin the repo: noApplication.get_env/fetch_envcall reads it, andMicrowaveprop.Propagation.NotifyListeneris not in the supervision tree either (its onlylib/mentions are comments, plus a test module). So the line changes nothing in dev, and the NotifyListener does not start. Either land the reader and the child spec together with this flag, or drop the key so the config does not read as if the consumer were off by default.@ -64,0 +94,4 @@enddefp lock! doexecute "SELECT pg_advisory_lock(#{@advisory_lock_key})"🟠 High — Advisory lock is not held on a stable session, so it can leak and block a booting pod
@disable_ddl_transaction truemeans no transaction pins a connection for this migration, and@disable_migration_lock truealso skips thecheckoutwrapper Ecto puts around its own advisory-lock strategy. Eachexecutetherefore runs on whichever pool connection it happens to check out, solock!()takes the lock on one connection, theCONCURRENTLYbuilds below run on others, andpg_advisory_unlockin theafterblock can land on a connection that never held it: it returns false,execute/1discards the result, and the lock stays held by that idle pooled connection for the life of the pod.Concretely: pod A takes the lock on connection C1, unlocks on Cn (no-op), and C1 keeps holding it. Pod B, booting in that window, blocks in its own
pg_advisory_lockforever (the runner executes raw commands withtimeout: :infinity), pinned on a pool slot, and itsall: truerun never reaches later migrations. So the lock provides accidental, leaky mutual exclusion instead of what the comment claims.Ecto has this exact case covered: set
migration_lock: :pg_advisory_lockonMicrowaveprop.Repoand delete both@disable_migration_lock trueand the hand-rolledlock!/unlockhelpers. That strategy checks out one connection for the whole migration (so the lock and theCONCURRENTLYbuilds share a session) and usespg_try_advisory_lockwith retry rather than blocking forever. If you keep the hand-rolled version, pin a session explicitly, e.g. wrapup/0anddown/0bodies inEcto.Adapters.SQL.checkout(Ecto.Adapter.lookup_meta(repo()), [], fn -> ... end).420bf907d333864f4fa1🤖 Skippy PR review
1 finding — 1 blocking before merge.
config/runtime.exs:263:exportsqueue silently kills the CSV/PDF export feature in prodBranch was rebased (
420bf907d34dis no longer in the PR history), so this pass covers the full diff and only adds what was not already posted. Resolved the earlierstart_notify_listenersuggestion:NotifyListeneris now in the supervision tree and reads the flag, so the dev key is no longer dead. 1 earlier finding still open: the migration's hand-rolled advisory lock.Reviewed
33864f4fa15a. Commentskippy reviewto re-run.@ -261,4 +261,3 @@space_weather: 1,mechanism: 4,exports: 1,gefs: 1🟠 High — Dropping the
:exportsqueue silently kills the CSV/PDF export feature in prodThe
:exportsqueue is not unused: it is the queuelive_table's only two workers declare (use Oban.Worker, queue: :exportsinLiveTable.Workers.CsvExportWorker/PdfExportWorker), andLiveTable.ExportHelpers.handle_event("export-csv"/"export-pdf")inserts them on every Export click.config/config.exsnever declaredexports, so with the key removed here (and inconfig/dev.exs) nothing declares the queue anywhere and no pod starts a producer for it, on eitherPROP_ROLE.Oban validates only the queue name's length at insert time (
deps/oban/lib/oban/job.ex:334), not that the queue is configured, so{:ok, _job} = ... |> Oban.insert()still succeeds and the job sits inavailableforever.MicrowavepropWeb.LiveTableResourcesubscribes toexports:<client_id>and only reacts to{:file_ready, path}, which only the (never-running) worker broadcasts, so the button does nothing visible:/contacts(its table setsexports: %{formats: [:csv]}) plus every table taking live_table's defaultexports: %{enabled: true}(/beacons, /users, /admin/contact-edits, the rover tables). Meanwhileavailablerows pile up inoban_jobs.Either restore
exports: 1inshared_queues(and inconfig/dev.exs), or drop the export affordance together with its plumbing:MicrowavepropWeb.ExportStore,ExportControllerand the/exports/:tokenroute exist only to serve these jobs.Addressed in
166787c9:up/0anddown/0now run insiderepo().checkout/1, sopg_advisory_lock, the CONCURRENTLY builds, andpg_advisory_unlockall share one session. (Kept the hand-rolled lock rather than a repo-widemigration_lockchange so the strategy stays scoped to this migration.)start_notify_listenerunread — stale:notify_listener.ex:40reads the key andapplication.exsupervises the listener since #18 merged. The dev flag was still redundant (the default istrue), so it's dropped.33864f4fa1166787c9fc🤖 Skippy PR review
1 finding — 1 blocking before merge.
CHANGELOG.md:34Branch was force-pushed (
33864f4fa1is no longer in the PR history), so this pass covers the full diff and only adds what was not already posted. Resolved the advisory-lock finding: repo().checkout/1 now pins the lock, the CONCURRENTLY builds and the unlock to a single pooled session. 1 earlier finding still open (the dropped :exports queue). The PR was merged into main asbeb52529while this review ran, so both blockers are now live on main.Reviewed
166787c9fca4. Commentskippy reviewto re-run.@ -31,6 +31,7 @@ defaults to the current hour rather than the furthest-future forecast.Path-averaged wind no longer mixes components from different grid cells whenone is missing, and the scoring recalibrator now trains on the samemissing-data rules the live scorer uses.<<<<<<< HEAD🟠 High — Merge conflict markers are committed in CHANGELOG.md
Lines 34, 77 and 83 are raw conflict markers (
<<<<<<< HEAD,=======,>>>>>>> 33864f4f (Infra and test hygiene: ...)), so the rebase conflict was committed unresolved.Microwaveprop.Changelogparses this file at compile time and/changelogrenders it to visitors, so the marker lines show up as body prose of the entries they land in (the<<<<<<<line in "More accurate path wind and recalibration inputs",=======in "Forecast pipeline reliability fixes",>>>>>>>in the new "Infrastructure and reliability cleanup" entry). It does not crash the compile: the parser only splits on^##/^###, and>>>>>>> 33864f4f (...)is not a heading, so this ships silently and is now on main (mergebeb52529). Fix: delete lines 34, 77 and 83, keep the infra entry, recompile. This is the only file in the tree with markers.