fix: avoid redundant locale session writes #30

graham · 2026-09-14T23:46:27-05:00

graham commented

2026-09-14 23:46:27 -05:00

Fixes W14 from bugs.md.

SetLocale now compares the session's stored locale with the freshly resolved one and only calls put_session/3 when they differ. Previously every anonymous browser request rewrote the session, forcing a Set-Cookie header onto responses that would otherwise be cacheable. When the write is skipped the response instead declares Cache-Control: private and Vary: Accept-Language, so a shared cache cannot replay a visitor-specific body.

The regression tests seed a real signed session cookie via round-trip so the incoming session is clean (only SetLocale itself could dirty it) and assert after send_resp: an unchanged locale emits no Set-Cookie and the cache headers above, a changed one still emits Set-Cookie.

Verification: focused tests passed (9), full suite passed (2238; one earlier run hit the known pre-existing PacketConsumerTest flake and passed on re-run), mix credo --strict, and commit hooks including Dialyzer passed.

Fixes W14 from bugs.md. `SetLocale` now compares the session's stored locale with the freshly resolved one and only calls `put_session/3` when they differ. Previously every anonymous browser request rewrote the session, forcing a `Set-Cookie` header onto responses that would otherwise be cacheable. When the write is skipped the response instead declares `Cache-Control: private` and `Vary: Accept-Language`, so a shared cache cannot replay a visitor-specific body. The regression tests seed a real signed session cookie via round-trip so the incoming session is clean (only `SetLocale` itself could dirty it) and assert after `send_resp`: an unchanged locale emits no `Set-Cookie` and the cache headers above, a changed one still emits `Set-Cookie`. Verification: focused tests passed (9), full suite passed (2238; one earlier run hit the known pre-existing PacketConsumerTest flake and passed on re-run), `mix credo --strict`, and commit hooks including Dialyzer passed.

graham added 1 commit

2026-09-14 23:46:27 -05:00

fix: avoid redundant locale session writes

Elixir CI / Dialyzer (pull_request) Successful in 40s

Details

Elixir CI / Build and test (pull_request) Successful in 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

31259dbc80

skippy-bot reviewed

2026-09-14 23:55:14 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/aprsme_web/plugs/set_locale.ex:17`	Cookie-free responses are now cache-eligible, but nothing declares their cache key

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

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme_web/plugs/set_locale.ex:17` | Cookie-free responses are now cache-eligible, but nothing declares their cache key | <sub>Reviewed `31259dbc8012`. Comment `skippy review` to re-run.</sub>

lib/aprsme_web/plugs/set_locale.ex

					
				@ -16,0 +14,4 @@

				    # Only rewrite the session when the value actually changes: an

				    # unconditional put_session forces a Set-Cookie onto every otherwise

				    # cacheable anonymous response.

				    if get_session(conn, :locale) == locale do

🟡 Warning — Cookie-free responses are now cache-eligible, but nothing declares their cache key

Suppressing this write is what makes the response storable by a shared cache (aprs.me is proxied by Cloudflare) — but the body is not visitor-independent. It is Gettext output, so it varies by Accept-Language, and the LiveView shell embeds a signed per-visitor session: data-phx-session carries the session map, which live_session :regular_pages fills via regular_pages_session/1 (ip_geolocation, read by MapLive.Navigation.determine_map_location/2).

Nothing in the app sends Vary or Cache-Control for these routes — the only cache header in the repo is NoStore, on the auth pages — so the first response any cache stores is replayed to every visitor: wrong language, and another visitor's IP-derived map centre.

If the cacheable-anonymous-response goal is going ahead, declare the key in the same change: Vary: Accept-Language plus keeping per-visitor state out of the markup, or Cache-Control: private until then. For Cloudflare specifically, Vary other than Accept-Encoding is ignored, so a cache rule keyed on Accept-Language (or leaving these pages uncached) is the version that actually works.

**🟡 Warning — Cookie-free responses are now cache-eligible, but nothing declares their cache key** Suppressing this write is what makes the response storable by a shared cache (aprs.me is proxied by Cloudflare) — but the body is not visitor-independent. It is Gettext output, so it varies by `Accept-Language`, and the LiveView shell embeds a signed per-visitor session: `data-phx-session` carries the session map, which `live_session :regular_pages` fills via `regular_pages_session/1` (`ip_geolocation`, read by `MapLive.Navigation.determine_map_location/2`). Nothing in the app sends `Vary` or `Cache-Control` for these routes — the only cache header in the repo is `NoStore`, on the auth pages — so the first response any cache stores is replayed to every visitor: wrong language, and another visitor's IP-derived map centre. If the cacheable-anonymous-response goal is going ahead, declare the key in the same change: `Vary: Accept-Language` plus keeping per-visitor state out of the markup, or `Cache-Control: private` until then. For Cloudflare specifically, `Vary` other than `Accept-Encoding` is ignored, so a cache rule keyed on Accept-Language (or leaving these pages uncached) is the version that actually works.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 12:01:37 -05:00

fix: mark cookie-free responses private and language-keyed

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

Details

skippy-bot/review Skippy review: clean — 1 non-blocking note open

Details

Elixir CI / Dialyzer (pull_request) Successful in 2m42s

Details

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

Details

fa1b2f010a

skippy-bot reviewed

2026-09-15 12:03:06 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity	Location	Issue
🔵 Suggestion	`test/aprsme_web/plugs/set_locale_test.exs:128`	The no-Set-Cookie assertion here can never fail

Resolved 1 earlier finding: the cookie-free branch now declares its cache key (Cache-Control: private + Vary: Accept-Language), so a shared cache can no longer replay a visitor-specific body. 1 new suggestion, non-blocking.

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

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `test/aprsme_web/plugs/set_locale_test.exs:128` | The no-Set-Cookie assertion here can never fail | Resolved 1 earlier finding: the cookie-free branch now declares its cache key (Cache-Control: private + Vary: Accept-Language), so a shared cache can no longer replay a visitor-specific body. 1 new suggestion, non-blocking. <sub>Reviewed `fa1b2f010a58`. Comment `skippy review` to re-run.</sub>

test/aprsme_web/plugs/set_locale_test.exs Outdated

					
				@ -64,0 +125,4 @@

				    # Set-Cookie — which makes it storable by a shared cache. It must then

				    # declare itself visitor-private: the body varies by Accept-Language and

				    # embeds per-visitor LiveView session data.

				    assert get_resp_header(conn, "set-cookie") == []

🔵 Suggestion — The no-Set-Cookie assertion here can never fail

get_resp_header(conn, "set-cookie") is [] for any conn this test could build: the cookie store writes through Plug.Conn.put_resp_cookie/4, which puts the value in conn.resp_cookies, and Plug only folds cookies into resp_headers inside run_before_send (Plug.Conn, at send time). Nothing here calls send_resp, so the line passes even if SetLocale still rewrote the session on every request - that is exactly the regressed case this test is named after, and it guards nothing.

The meaningful version already exists in "does not rewrite the session when the locale is unchanged", which send_resps before asserting. Either pipe |> send_resp(200, "") in before these three assertions, or keep the pre-send shape and assert conn.resp_cookies == %{} (that one does flip when the session is dirtied). Minor, same paragraph: only the older test seeds a signed cookie, so the PR description's "two regression tests seed a real signed session cookie" holds for just one of them.

**🔵 Suggestion — The no-Set-Cookie assertion here can never fail** `get_resp_header(conn, "set-cookie")` is `[]` for any conn this test could build: the cookie store writes through `Plug.Conn.put_resp_cookie/4`, which puts the value in `conn.resp_cookies`, and Plug only folds cookies into `resp_headers` inside `run_before_send` (Plug.Conn, at send time). Nothing here calls `send_resp`, so the line passes even if `SetLocale` still rewrote the session on every request - that is exactly the regressed case this test is named after, and it guards nothing. The meaningful version already exists in "does not rewrite the session when the locale is unchanged", which `send_resp`s before asserting. Either pipe `|> send_resp(200, "")` in before these three assertions, or keep the pre-send shape and assert `conn.resp_cookies == %{}` (that one does flip when the session is dirtied). Minor, same paragraph: only the older test seeds a signed cookie, so the PR description's "two regression tests seed a real signed session cookie" holds for just one of them.

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-09-15 16:44:14 -05:00

test: assert no-Set-Cookie after send so the check can fail

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

Details

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

Details

Elixir CI / Dialyzer (pull_request) Successful in 41s

Details

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

Details

9381369593

The cookie-free test asserted on resp_headers before send, where the
session cookie never lands (Plug folds resp_cookies into resp_headers
at send time). Seed a real signed session cookie via round-trip — now
shared with the unchanged-locale test — and send_resp before asserting,
so a regressed unconditional put_session actually flips it.

skippy-bot commented

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

First-time contributor

Resolved 1 earlier finding: both no-Set-Cookie tests now send_resp before asserting and both seed a real signed session cookie via request_with_seeded_locale/4, so that assertion can actually fail now.

Nothing new to flag in fa1b2f01..93813695 (test-only commit).

Resolved 1 earlier finding: both no-`Set-Cookie` tests now `send_resp` before asserting and both seed a real signed session cookie via `request_with_seeded_locale/4`, so that assertion can actually fail now. Nothing new to flag in `fa1b2f01..93813695` (test-only commit).

graham merged commit 7bdabdf371 into main

2026-09-15 16:48:39 -05:00

graham deleted branch fix/w14-locale-session-writes

2026-09-15 16:48:39 -05:00

graham referenced this pull request from a commit

2026-09-15 16:48:41 -05:00

fix: avoid redundant locale session writes (#30)

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project