fix: isolate expensive health checks behind limiter #23

graham · 2026-09-14T23:10:06-05:00

graham commented

2026-09-14 23:10:06 -05:00

Fixes W6 from bugs.md.

the endpoint-level HealthCheck plug now answers only /live and /startup with allocation-free liveness — no DB pool checkout, no PubSub broadcast, no GenServer call
/health and /health/db moved behind the router onto PageController.health with a dedicated per-IP limiter (30/min), well above K8s probe cadence
ShutdownHandler.shutting_down?/0 now reads a :persistent_term flag written at boot and at shutdown initiation, removing the 5 s serialising GenServer.call from every probe
rewrote the health tests around observable HTTP behavior (liveness text, JSON readiness, 429 past the limit, 503 when the shutdown flag is cached) and dropped tests that pinned the removed plumbing; deployment docs updated

Note: /health now returns JSON instead of the previous text/plain since it is served by the controller.

Verification: focused tests passed (26), full suite passed (2225), mix credo --strict, and commit hooks including Dialyzer passed.

Fixes W6 from bugs.md. - the endpoint-level `HealthCheck` plug now answers only `/live` and `/startup` with allocation-free liveness — no DB pool checkout, no PubSub broadcast, no GenServer call - `/health` and `/health/db` moved behind the router onto `PageController.health` with a dedicated per-IP limiter (30/min), well above K8s probe cadence - `ShutdownHandler.shutting_down?/0` now reads a `:persistent_term` flag written at boot and at shutdown initiation, removing the 5 s serialising `GenServer.call` from every probe - rewrote the health tests around observable HTTP behavior (liveness text, JSON readiness, 429 past the limit, 503 when the shutdown flag is cached) and dropped tests that pinned the removed plumbing; deployment docs updated Note: `/health` now returns JSON instead of the previous text/plain since it is served by the controller. Verification: focused tests passed (26), full suite passed (2225), `mix credo --strict`, and commit hooks including Dialyzer passed.

graham added 1 commit

2026-09-14 23:10:06 -05:00

fix: isolate expensive health checks behind limiter

Elixir CI / Dialyzer (pull_request) Successful in 40s

Details

Elixir CI / Build and test (pull_request) Failing after 1m41s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

336179cd88

skippy-bot reviewed

2026-09-14 23:13:20 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/aprsme/shutdown_handler.ex:153`	New global flag is never cleared, so it leaks across tests
🔵 Suggestion	`lib/aprsme_web/router.ex:108`	Comment claims a PubSub readiness check that no longer exists

_{Reviewed 336179cd88a0. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme/shutdown_handler.ex:153` | New global flag is never cleared, so it leaks across tests | | 🔵 Suggestion | `lib/aprsme_web/router.ex:108` | Comment claims a PubSub readiness check that no longer exists | Reviewed `336179cd88a0`. Comment `skippy review` to re-run.

lib/aprsme/shutdown_handler.ex Outdated

					
				@ -148,6 +150,7 @@ defmodule Aprsme.ShutdownHandler do

				  defp initiate_shutdown(state) do

				    # Mark as shutting down

				    :persistent_term.put(@shutting_down_key, true)

🟡 Warning — New global flag is never cleared, so it leaks across tests

shutting_down?/0 is now backed by a process-global :persistent_term that only init/1 ever writes back to false. Two cleanups exist in the suite precisely because a test drives a real shutdown — test/aprsme/signal_handler_test.exs:31-34 (SIGTERM -> the real ShutdownHandler.shutdown()) and the setup at test/aprsme_web/controllers/page_controller_test.exs:7-12 — but both only reset state.shutting_down and :health_status, so they no longer clear the state health checks read. Once that module runs, shutting_down?() stays true for the rest of the VM and later modules fail depending on order: GET /ready 200 and PageController.health/2 200 (page_controller_test.exs:22,46), GET /health 200 (health_check_test.exs:55), and shutting_down?() == false (shutdown_handler_test.exs:30,108). Reset the term too — :persistent_term.erase(@shutting_down_key) (or a ShutdownHandler.reset/0) in both cleanups — rather than only the GenServer state.

**🟡 Warning — New global flag is never cleared, so it leaks across tests** `shutting_down?/0` is now backed by a process-global `:persistent_term` that only `init/1` ever writes back to `false`. Two cleanups exist in the suite precisely because a test drives a *real* shutdown — `test/aprsme/signal_handler_test.exs:31-34` (SIGTERM -> the real `ShutdownHandler.shutdown()`) and the setup at `test/aprsme_web/controllers/page_controller_test.exs:7-12` — but both only reset `state.shutting_down` and `:health_status`, so they no longer clear the state health checks read. Once that module runs, `shutting_down?()` stays `true` for the rest of the VM and later modules fail depending on order: `GET /ready` 200 and `PageController.health/2` 200 (`page_controller_test.exs:22,46`), `GET /health` 200 (`health_check_test.exs:55`), and `shutting_down?() == false` (`shutdown_handler_test.exs:30,108`). Reset the term too — `:persistent_term.erase(@shutting_down_key)` (or a `ShutdownHandler.reset/0`) in both cleanups — rather than only the GenServer state.

skippy-bot marked this conversation as resolved

lib/aprsme_web/router.ex Outdated

					
				@ -98,3 +105,4 @@

				    get "/status.json", PageController, :status_json

				  end

				  # Full readiness (database + PubSub). More expensive than /ready, so it

🔵 Suggestion — Comment claims a PubSub readiness check that no longer exists

This says "database + PubSub", and the pipeline comment above (line 59) says "hit the DB pool and PubSub", but the old plug's Phoenix.PubSub.broadcast/3 ping was deleted in this PR and PageController.health/2 only runs Repo.query!("SELECT 1"). Nothing in the app now fails readiness when PubSub is dead. Either restore the probe or drop PubSub from both comments so the next person reading the gate isn't misled.

**🔵 Suggestion — Comment claims a PubSub readiness check that no longer exists** This says "database + PubSub", and the pipeline comment above (line 59) says "hit the DB pool and PubSub", but the old plug's `Phoenix.PubSub.broadcast/3` ping was deleted in this PR and `PageController.health/2` only runs `Repo.query!("SELECT 1")`. Nothing in the app now fails readiness when PubSub is dead. Either restore the probe or drop PubSub from both comments so the next person reading the gate isn't misled.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 11:22:43 -05:00

fix: clear shutdown persistent_term flag in test cleanups, correct health comments

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

Elixir CI / Dialyzer (pull_request) Has been cancelled

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been cancelled

Details

Elixir CI / Build and test (pull_request) Has been cancelled

Details

1ee986179b

skippy-bot reviewed

2026-09-15 11:23:59 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`test/aprsme_web/controllers/page_controller_test.exs:13`	The flag reset landed on a reader; the module that sets it is still unpatched

Resolved 1 earlier finding (the stale PubSub readiness comments in router.ex). 1 still open, plus 1 new: the persistent_term reset went to a module that only reads the flag.

_{Reviewed 1ee986179b92. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `test/aprsme_web/controllers/page_controller_test.exs:13` | The flag reset landed on a reader; the module that sets it is still unpatched | Resolved 1 earlier finding (the stale PubSub readiness comments in router.ex). 1 still open, plus 1 new: the persistent_term reset went to a module that only reads the flag. Reviewed `1ee986179b92`. Comment `skippy review` to re-run.

test/aprsme_web/controllers/page_controller_test.exs Outdated

					
				@ -9,2 +10,4 @@

				    # directly, and it outlives the GenServer state and the Application env.

				    if pid = Process.whereis(Aprsme.ShutdownHandler) do

				      :sys.replace_state(pid, fn state -> %{state | shutting_down: false} end)

				      :persistent_term.put({Aprsme.ShutdownHandler, :shutting_down}, false)

🟡 Warning — The flag reset landed on a reader; the module that sets it is still unpatched

This clears the flag in PageControllerTest's setup, but this module only reads it. The site that sets it is test/aprsme/signal_handler_test.exs:31-34: SignalHandler.handle_info({:signal, :sigterm}, ...) spawn_links the real ShutdownHandler.shutdown() (lib/aprsme/signal_handler.ex:28-29), whose initiate_shutdown/1 puts the term true, and that cleanup only restores state.shutting_down and :health_status. Aprsme.ShutdownHandler is started in every env (lib/aprsme/application.ex:67-70), so once that module runs the VM keeps shutting_down?() == true and the order-dependent failures my earlier comment listed survive in the modules this commit did not touch: test/aprsme/shutdown_handler_test.exs:30 and :108 (shutting_down?() == false) and test/aprsme_web/plugs/health_check_test.exs:54-58 (GET /health expects 200, gets 503 shutting_down).

Fix it where the flag is written (add ShutdownHandler.reset/0 and call it from that test's cleanup, or :persistent_term.erase(@shutting_down_key)), and make the reset here unconditional rather than nested under if Process.whereis(...).

**🟡 Warning — The flag reset landed on a reader; the module that sets it is still unpatched** This clears the flag in `PageControllerTest`'s setup, but this module only *reads* it. The site that sets it is `test/aprsme/signal_handler_test.exs:31-34`: `SignalHandler.handle_info({:signal, :sigterm}, ...)` spawn_links the real `ShutdownHandler.shutdown()` (`lib/aprsme/signal_handler.ex:28-29`), whose `initiate_shutdown/1` puts the term `true`, and that cleanup only restores `state.shutting_down` and `:health_status`. `Aprsme.ShutdownHandler` is started in every env (`lib/aprsme/application.ex:67-70`), so once that module runs the VM keeps `shutting_down?() == true` and the order-dependent failures my earlier comment listed survive in the modules this commit did not touch: `test/aprsme/shutdown_handler_test.exs:30` and `:108` (`shutting_down?() == false`) and `test/aprsme_web/plugs/health_check_test.exs:54-58` (`GET /health` expects 200, gets 503 `shutting_down`). Fix it where the flag is written (add `ShutdownHandler.reset/0` and call it from that test's cleanup, or `:persistent_term.erase(@shutting_down_key)`), and make the reset here unconditional rather than nested under `if Process.whereis(...)`.

skippy-bot marked this conversation as resolved

graham force-pushed fix/w6-health-limiter from 1ee986179b

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

Elixir CI / Dialyzer (pull_request) Has been cancelled

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been cancelled

Details

Elixir CI / Build and test (pull_request) Has been cancelled

Details

to cfad9e201a

Elixir CI / Build and test (pull_request) Successful in 1m0s

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m45s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

2026-09-15 11:24:38 -05:00

Compare

skippy-bot reviewed

2026-09-15 11:29:43 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/aprsme_web/router.ex:113`	Branch is 12 commits behind main; the merge conflicts here and can restore the public /metrics route
🔵 Suggestion	`test/aprsme_web/controllers/page_controller_test.exs:24`	Stale comment: the plug no longer serves /health

Findings outside the commentable diff

🔵 Suggestion — test/aprsme_web/controllers/page_controller_test.exs:24 — Stale comment: the plug no longer serves /health
This says the /health endpoint is otherwise served by the HealthCheck plug as text/plain. That stopped being true in this PR: the plug passes /health through (your own health_check_test.exs asserts exactly that) and PageController.health/2 is what /health reaches. One-line fix so the next reader is not sent looking for liveness logic in the plug.

Resolved 2 earlier findings: the shutdown :persistent_term flag is now cleared in each cleanup that sets it. 0 still open. Bookkeeping: newdiff fell back to the full PR diff (the previous head is no longer in PR history).

_{Reviewed cfad9e201ade. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/aprsme_web/router.ex:113` | Branch is 12 commits behind main; the merge conflicts here and can restore the public /metrics route | | 🔵 Suggestion | `test/aprsme_web/controllers/page_controller_test.exs:24` | Stale comment: the plug no longer serves /health | #### Findings outside the commentable diff - **🔵 Suggestion — `test/aprsme_web/controllers/page_controller_test.exs:24` — Stale comment: the plug no longer serves /health** This says the `/health` endpoint is otherwise served by the `HealthCheck` plug as text/plain. That stopped being true in this PR: the plug passes `/health` through (your own `health_check_test.exs` asserts exactly that) and `PageController.health/2` is what /health reaches. One-line fix so the next reader is not sent looking for liveness logic in the plug. Resolved 2 earlier findings: the shutdown `:persistent_term` flag is now cleared in each cleanup that sets it. 0 still open. Bookkeeping: `newdiff` fell back to the full PR diff (the previous head is no longer in PR history). Reviewed `cfad9e201ade`. Comment `skippy review` to re-run.

lib/aprsme_web/router.ex Outdated

					
				@ -101,0 +110,4 @@

				  # only cheap liveness for /live and /startup.

				  scope "/", AprsmeWeb do

				    pipe_through [:accepts_json, :health_limiter]

				    get "/health", PageController, :health

🟠 High — Branch is 12 commits behind main; the merge conflicts here and can restore the public /metrics route

This branch is based on 0d034ae6, 12 commits behind main: #11 (SIGTERM routed through a :gen_event handler) and #12 (PromEx moved to a pod-only listener on 4001, public /metrics scope deleted) both landed after it. Merging conflicts in this file and in test/aprsme/signal_handler_test.exs (git merge-tree origin/main cfad9e20 reports content conflicts in both; the PR itself reports mergeable: false). Your new health scope lands exactly where #12 deleted the /metrics block, so a resolution that keeps this branch's side of that hunk re-adds forward "/", PromEx.Plug to the public router and undoes #12. Rebase onto main before resolving, and when you re-apply it, put the :persistent_term reset into #11's rewritten signal_handler_test.exs cleanup or the test leak from the earlier thread returns.

**🟠 High — Branch is 12 commits behind main; the merge conflicts here and can restore the public /metrics route** This branch is based on `0d034ae6`, 12 commits behind `main`: `#11` (SIGTERM routed through a `:gen_event` handler) and `#12` (PromEx moved to a pod-only listener on 4001, public `/metrics` scope deleted) both landed after it. Merging conflicts in this file and in `test/aprsme/signal_handler_test.exs` (`git merge-tree origin/main cfad9e20` reports content conflicts in both; the PR itself reports `mergeable: false`). Your new health scope lands exactly where `#12` deleted the `/metrics` block, so a resolution that keeps this branch's side of that hunk re-adds `forward "/", PromEx.Plug` to the public router and undoes `#12`. Rebase onto `main` before resolving, and when you re-apply it, put the `:persistent_term` reset into `#11`'s rewritten `signal_handler_test.exs` cleanup or the test leak from the earlier thread returns.

skippy-bot marked this conversation as resolved

graham force-pushed fix/w6-health-limiter from cfad9e201a

Elixir CI / Build and test (pull_request) Successful in 1m0s

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m45s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

to a974259277

Elixir CI / Build and test (pull_request) Failing after 58s

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m43s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

2026-09-15 17:54:12 -05:00

Compare

skippy-bot reviewed

2026-09-15 17:57:54 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/aprsme_web/router.ex:123`	The new /health route drops the 1 s bound the plug put on the DB probe
🔵 Suggestion	`test/aprsme/shutdown_handler_test.exs:26`	Two comments still explain the rescue clause this PR deleted

Findings outside the commentable diff

🔵 Suggestion — test/aprsme/shutdown_handler_test.exs:26 — Two comments still explain the rescue clause this PR deleted
shutting_down?/0 no longer calls the GenServer or rescues anything — it reads :persistent_term — but this describe block still justifies the assertion with "If the GenServer.call raises, the rescue _ -> false kicks in", and the shutdown/0 as an external API test (~line 120) still says shutting_down?/0 "is safe to call — it rescues when the GenServer isn't running". The "without the GenServer running" framing is moot too: the read now works with no process at all. Two comment edits so the next reader isn't sent looking for a rescue clause that no longer exists.

Follow-up on cfad9e20..a9742592. Resolved 1 earlier finding: the branch is rebased onto main (04c1bc00), so the merge conflict is gone and the public /metrics route stays deleted. 1 earlier suggestion (stale PubSub comments) was resolved in an earlier round. Bookkeeping: newdiff fell back to the full PR diff — the previous head is no longer in history — so this review covers the whole 7-file PR against the current base (75 files/+2100 in the webhook payload is the stale pre-rebase figure; Forgejo now reports 7 files, +106/-374).

_{Reviewed a9742592770d. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme_web/router.ex:123` | The new /health route drops the 1 s bound the plug put on the DB probe | | 🔵 Suggestion | `test/aprsme/shutdown_handler_test.exs:26` | Two comments still explain the rescue clause this PR deleted | #### Findings outside the commentable diff - **🔵 Suggestion — `test/aprsme/shutdown_handler_test.exs:26` — Two comments still explain the rescue clause this PR deleted** `shutting_down?/0` no longer calls the GenServer or rescues anything — it reads `:persistent_term` — but this describe block still justifies the assertion with "If the `GenServer.call` raises, the `rescue _ -> false` kicks in", and the `shutdown/0 as an external API` test (~line 120) still says `shutting_down?/0` "is safe to call — it rescues when the GenServer isn't running". The "without the GenServer running" framing is moot too: the read now works with no process at all. Two comment edits so the next reader isn't sent looking for a rescue clause that no longer exists. Follow-up on `cfad9e20`..`a9742592`. Resolved 1 earlier finding: the branch is rebased onto `main` (`04c1bc00`), so the merge conflict is gone and the public `/metrics` route stays deleted. 1 earlier suggestion (stale PubSub comments) was resolved in an earlier round. Bookkeeping: `newdiff` fell back to the full PR diff — the previous head is no longer in history — so this review covers the whole 7-file PR against the current base (75 files/+2100 in the webhook payload is the stale pre-rebase figure; Forgejo now reports 7 files, +106/-374). Reviewed `a9742592770d`. Comment `skippy review` to re-run.

lib/aprsme_web/router.ex

					
				@ -111,0 +120,4 @@

				  # only cheap liveness for /live and /startup.

				  scope "/", AprsmeWeb do

				    pipe_through [:accepts_json, :health_limiter]

				    get "/health", PageController, :health

🟡 Warning — The new /health route drops the 1 s bound the plug put on the DB probe

This line makes PageController.health/2 the production /health handler, but that action's DB check is Aprsme.Repo.query!("SELECT 1") (lib/aprsme_web/controllers/page_controller.ex:60) with no timeout, so it inherits the repo's :timeout — DB_TIMEOUT, default 30_000 ms in config/runtime.exs. The plug this PR deletes capped the same probe at timeout: 1000.

A slow-but-alive Postgres/PgBouncer is exactly when /health gets polled hardest, and each poll now holds a pool connection for up to 30 s instead of 1 s. POOL_SIZE is 25 in k8s/deployment.yaml, so 25 concurrent /health requests from a single IP — well inside the new 30/min limiter, and the endpoint is public and anonymous — can pin the whole pool and stall packet writes. That is the W6 symptom this PR is fixing.

Keep the bound at the call site: Aprsme.Repo.query!("SELECT 1", [], timeout: 1_000); the existing rescue already turns the checkout timeout into a 503.

**🟡 Warning — The new /health route drops the 1 s bound the plug put on the DB probe** This line makes `PageController.health/2` the production `/health` handler, but that action's DB check is `Aprsme.Repo.query!("SELECT 1")` (`lib/aprsme_web/controllers/page_controller.ex:60`) with no timeout, so it inherits the repo's `:timeout` — `DB_TIMEOUT`, default 30_000 ms in `config/runtime.exs`. The plug this PR deletes capped the same probe at `timeout: 1000`. A slow-but-alive Postgres/PgBouncer is exactly when `/health` gets polled hardest, and each poll now holds a pool connection for up to 30 s instead of 1 s. `POOL_SIZE` is 25 in `k8s/deployment.yaml`, so 25 concurrent `/health` requests from a single IP — well inside the new 30/min limiter, and the endpoint is public and anonymous — can pin the whole pool and stall packet writes. That is the W6 symptom this PR is fixing. Keep the bound at the call site: `Aprsme.Repo.query!("SELECT 1", [], timeout: 1_000)`; the existing `rescue` already turns the checkout timeout into a 503.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 18:35:28 -05:00

test: assert login_id stays off the public status page

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

Elixir CI / Build and test (pull_request) Successful in 59s

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m52s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

921cfa5f3e

#31 removed login_id from the public status response but this
assertion still expected it rendered, leaving main's CI red
(run 271). Refute the render so the test defends the contract.

graham added 1 commit

2026-09-15 18:44:54 -05:00

fix: bound /health DB probe at 1s, drop stale rescue-clause comments

Elixir CI / Build and test (pull_request) Successful in 54s

Details

skippy-bot/review Skippy review: clean — no open findings

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m46s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

d8c7bb0c40

Review feedback on a9742592:

- db_healthy?/0 now passes timeout: 1_000 to Repo.query! — the plug
  this PR replaced capped the probe at 1s; without the bound each
  /health poll could hold a pool connection for the full DB_TIMEOUT
  (30s), letting 25 concurrent polls pin the pool
- shutdown_handler_test comments no longer explain the GenServer
  rescue clause shutting_down?/0 lost when it moved to :persistent_term
- the :persistent_term reset in PageControllerTest is now
  unconditional (the flag needs no pid), and SignalHandlerTest's
  cleanup resets it too, since its on_exit wires the real shutdown MFA

skippy-bot commented

2026-09-15 18:45:55 -05:00

First-time contributor

🤖 Skippy PR review

Clean on d8c7bb0c40bf — nothing new in 921cfa5f..d8c7bb0c.

Resolved 1 of 1 earlier findings: the /health DB probe is now bounded at the call site (Aprsme.Repo.query!("SELECT 1", [], timeout: 1_000) in lib/aprsme_web/controllers/page_controller.ex:63), matching the timeout: 1000 the deleted endpoint-level plug applied, so a slow Postgres/PgBouncer no longer holds a pool connection for the full 30 s DB_TIMEOUT per poll. The rescue still turns that into a 503.

Also addressed, both suggestions from earlier rounds: the stale comments on shutting_down?/0 in test/aprsme/shutdown_handler_test.exs are gone, and the :persistent_term reset in test/aprsme_web/controllers/page_controller_test.exs is now unconditional instead of nested under if Process.whereis(...).

0 findings open. Bookkeeping: newdiff --since 921cfa5f returned the incremental diff (no [FULL-DIFF-FALLBACK]), so this round covers only d8c7bb0c.

_{Reviewed d8c7bb0c40bf. Comment skippy review to re-run.}

### 🤖 Skippy PR review Clean on `d8c7bb0c40bf` — nothing new in `921cfa5f..d8c7bb0c`. Resolved 1 of 1 earlier findings: the `/health` DB probe is now bounded at the call site (`Aprsme.Repo.query!("SELECT 1", [], timeout: 1_000)` in `lib/aprsme_web/controllers/page_controller.ex:63`), matching the `timeout: 1000` the deleted endpoint-level plug applied, so a slow Postgres/PgBouncer no longer holds a pool connection for the full 30 s `DB_TIMEOUT` per poll. The `rescue` still turns that into a 503. Also addressed, both suggestions from earlier rounds: the stale comments on `shutting_down?/0` in `test/aprsme/shutdown_handler_test.exs` are gone, and the `:persistent_term` reset in `test/aprsme_web/controllers/page_controller_test.exs` is now unconditional instead of nested under `if Process.whereis(...)`. 0 findings open. Bookkeeping: `newdiff --since 921cfa5f` returned the incremental diff (no `[FULL-DIFF-FALLBACK]`), so this round covers only `d8c7bb0c`. Reviewed `d8c7bb0c40bf`. Comment `skippy review` to re-run.

graham merged commit 989e9b4be3 into main

2026-09-15 18:52:33 -05:00

graham deleted branch fix/w6-health-limiter

2026-09-15 18:52:33 -05:00

graham referenced this pull request from a commit

2026-09-15 18:52:33 -05:00

fix: isolate expensive health checks behind limiter (#23)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project