From af25488cf9bd464935f3e6a7fdc25cc055ae5a1d Mon Sep 17 00:00:00 2001 From: "David A. Wheeler" Date: Fri, 26 Jun 2026 09:24:25 -0400 Subject: [PATCH 1/2] Doc: propose model-callback CDN purge as future work Section 9.3 honestly noted that purging is wired into the controller update/destroy actions and the bulk recalculation, but not a model callback, so a future write path that changes a displayed project field could leave the cached anonymous show page stale until Surrogate-Control max-age expires. Add Section 11 recording, as a deliberate (not-yet-implemented) proposal, an after_commit model callback that purges on any project change. Includes the exact callback, the implementation details that keep it correct (notably gating on skip_callbacks so the bulk recalc's single purge_all is not turned into thousands of per-project purges, after_commit vs after_save, enqueue vs synchronous purge, scope to update/destroy, removing the now-redundant controller purges, and purge-on-any-update over a fragile column allowlist), the pros and cons, the residual cross-model (user_display_name) limitation, and tests. Doc-only; no behavior change. Co-Authored-By: Claude Opus 4.8 Signed-off-by: David A. Wheeler --- docs/cdn-cache-not-logged-in.md | 152 ++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/docs/cdn-cache-not-logged-in.md b/docs/cdn-cache-not-logged-in.md index d6ac1c4a9..87b326c3f 100644 --- a/docs/cdn-cache-not-logged-in.md +++ b/docs/cdn-cache-not-logged-in.md @@ -993,6 +993,18 @@ that exact key. end ``` +* **Known limitation — non-controller writes.** Purging is wired into the + controller `update`/`destroy` actions + ([`projects_controller.rb:694,765-770,814`](../app/controllers/projects_controller.rb)) + and the bulk recalculation (`Project.update_all_badge_percentages`, which + ends with `FastlyRails.purge_all`) — **not** into a model callback. Any + *other* path that changes a displayed project field via `save` / + `update_column` without purging would leave the cached show page stale until + `Surrogate-Control`'s `max-age` expires (10 days). No such path is known + today, but the safety net is fragile. Moving the purge into an `after_commit` + model callback would make purging robust against *any* write path; the design, + with its pros and cons, is in **Section 11**. + ### 9.4 Page variance between users Two distinct variance axes must both be safe. @@ -1251,3 +1263,143 @@ new paths (e.g. `/en` and `/en/criteria`), confirming first-`MISS`-then-`HIT`, no `Set-Cookie`, and that a session-cookie request returns `private, no-store`. If anything misbehaves, set `BADGEAPP_CACHE_MISC_PAGES=false` to disable instantly. + +--- + +## 11. Potential future work: purge the CDN from a model callback + +> **Status: not implemented; a proposal.** Today CDN purging is triggered +> explicitly from the controller `update`/`destroy` actions and from the bulk +> recalculation (`Project.update_all_badge_percentages`). This section records +> the design for making purging a model-level `after_commit` callback — a more +> robust "single source of truth" — together with its pros, cons, and the +> pitfalls that make a *naive* callback worse than the status quo. It is written +> down so the decision can be made deliberately later; it is **not** required +> for the project-show caching to be correct as shipped. + +### 11.1 Motivation + +Purging is currently wired into specific code paths rather than the data model: + +* Controller `update`/`destroy` call `@project.purge_cdn_project` and schedule + a delayed `PurgeCdnProjectJob` + ([`projects_controller.rb:694,765-770,814`](../app/controllers/projects_controller.rb)). +* The bulk recalculation issues one `FastlyRails.purge_all` + ([`project.rb`](../app/models/project.rb), `update_all_badge_percentages`). + +Both known write paths are covered, so the shipped feature is correct. The +weakness is structural: nothing *guarantees* that a **future** code path which +changes a displayed project field (name, badge percentages, +`achieved_passing_at` / `lost_passing_at`, any criterion answer or +justification) also purges. Such a path would leave the cached anonymous show +page stale until `Surrogate-Control`'s `max-age` expires (10 days). Because the +show page renders almost every project field, the set of "fields that matter" is +effectively "the whole record", so the safe rule is *purge whenever the project +changes*. An `after_commit` callback expresses exactly that, independent of who +performed the save. + +### 11.2 Proposed implementation + +Add a single callback to [`app/models/project.rb`](../app/models/project.rb), +reusing the existing `record_key` and `PurgeCdnProjectJob`: + +```ruby +# Purge this project's cached CDN resources (badge, JSON, and the anonymous +# show HTML -- all tagged with record_key) whenever the project changes by +# ANY path, not just controller edits. Skipped during bulk recalculation, +# which issues a single purge_all itself (update_all_badge_percentages). +after_commit :enqueue_cdn_purge, on: %i[update destroy], unless: :skip_callbacks + +private + +def enqueue_cdn_purge + PurgeCdnProjectJob.perform_later(record_key) + # Delayed re-purge closes the rolling-deploy / read-repopulation race -- + # the same recovery the controller schedules today. + PurgeCdnProjectJob + .set(wait: ApplicationController::BADGE_PURGE_DELAY.seconds) + .perform_later(record_key) +end +``` + +The implementation details that make this correct (a naive callback is *worse* +than today without them): + +1. **Gate on `skip_callbacks` — the critical pitfall.** + `update_all_badge_percentages` sets the `cattr_accessor :skip_callbacks` + true, saves 10,000+ projects, and then issues **one** `FastlyRails.purge_all`. + Without `unless: :skip_callbacks`, the callback would fire on every one of + those commits and enqueue 20,000+ individual purge jobs in place of that + single purge_all — a severe regression. The flag is still true during each + in-loop commit, so the guard suppresses it correctly. +2. **Use `after_commit`, not `after_save`.** Purge only after the data is + durably committed; purging inside the transaction lets a concurrent anonymous + read re-populate the cache with pre-commit (or about-to-roll-back) content. +3. **Enqueue the job; do not purge synchronously in the callback.** A + synchronous `FastlyRails.purge_by_key` adds a Fastly network round-trip (10 s + timeout) to *every* commit on *every* path. `PurgeCdnProjectJob` is async + with retry/backoff and is the better fit now that purging fires everywhere. + (This is a slight behavior change from the controller's current synchronous + pre/post-save purge.) +4. **Scope to `update`/`destroy`.** A newly created project has no cached page + yet, so a create-time purge is wasted work. `record_key` still resolves in + `after_commit` on destroy (the id remains in memory). +5. **Remove the now-redundant controller purges** (the calls at + `projects_controller.rb:694,765-770,814`), or every controller edit purges + twice. Collapsing three call sites into one definition is the main + maintainability payoff. +6. **Purge on *any* update; do not build a "displayed-columns changed" + allowlist.** Because the show page renders nearly every field, an allowlist + is fragile and would risk re-introducing the exact staleness bug this change + exists to prevent. Reads vastly outnumber writes here, so an occasional purge + for a bookkeeping-only update is cheap insurance — correctness over hit rate. + +### 11.3 Pros and cons + +**Pros** + +* **Robust by construction.** Any current or future write path — console + fix-ups, rake tasks, background jobs, new controllers — purges automatically. + Closes the Section 9.3 "non-controller writes" limitation. +* **DRY.** One definition replaces three controller call sites; the purge + policy lives next to the data it protects. +* **Reuses existing machinery** (`record_key`, `PurgeCdnProjectJob`, + `BADGE_PURGE_DELAY`); no new infrastructure. + +**Cons** + +* **A new global invariant with a sharp edge.** The correctness of the + high-volume bulk path now depends on the `skip_callbacks` guard. If a future + refactor introduces another bulk write without that flag, it could trigger a + purge storm. (Mitigated by a test; see Section 11.4.) +* **More purges overall.** Every update — including ones that touch only + non-displayed bookkeeping columns — now enqueues two purge jobs. Negligible + given the read/write ratio, but non-zero load on the job queue and Fastly API. +* **Slight timing change.** Moving from synchronous to job-based purging adds + sub-second job-pickup latency before eviction; the delayed re-purge already + tolerates this. +* **Not a full solution to cross-model staleness** (next subsection), so it can + create a false sense of "all staleness is handled". + +### 11.4 Residual limitation: cross-model changes + +The show page renders `@project.user_display_name`. A model callback on +`Project` does **not** fire when the owning `User` renames themselves, so that +project's cached page would still show the old name until the next project edit +or `max-age` expiry. Fully closing this would need an `after_commit` on `User` +that purges every `record_key` of that user's projects — a larger change with +its own performance considerations (a prolific owner could trigger many +purges). It is almost certainly not worth it for a display-name change, but it +should be acknowledged rather than silently assumed handled. + +### 11.5 Tests + +* Assert a plain change enqueues the purge: + `assert_enqueued_with(job: PurgeCdnProjectJob, args: [project.record_key])` + after `project.update!(name: 'x')`. +* Assert the **bulk path does not** enqueue per-project purges (guarding the + `skip_callbacks` pitfall in 11.2.1): run `update_all_badge_percentages` and + assert zero `PurgeCdnProjectJob` enqueues from the loop (the single + `purge_all` is a separate call). +* Keep the Section 9.3 `Surrogate-Key` test, which ties the cached page to the + key the callback purges. From a0ddac8db0e7a574601d979170b3af9e15561f10 Mon Sep 17 00:00:00 2001 From: "David A. Wheeler" Date: Fri, 26 Jun 2026 09:52:49 -0400 Subject: [PATCH 2/2] Make CDN re-purge unconditional on project update/destroy projects#update previously scheduled the delayed re-purge only inside the "@project.save succeeded" branch. But update_additional_rights writes the AdditionalRight table (rendered on the anonymous /permissions page) in its own transaction, which commits independently of @project.save and is not rolled back if the save later fails. On a save-fails-after-rights-changed path, only the early immediate purge fired -- losing the delayed re-purge that closes the TCP-reorder race, so the CDN could hold obsolete collaborator data until max-age. Schedule the delayed re-purge unconditionally on entry to update (after the request is authorized), alongside the early immediate purge, and drop the now -redundant post-save delayed purge (keeping the post-save immediate purge for freshest-data eviction). Extra purges are harmless and the request is already authorized by that point. Apply the same immediate+delayed pair to destroy, which previously did only an immediate purge: a stale re-cache of a deleted project's page is exactly the case worth the extra re-purge. Also correct an outdated comment: ActiveJob is backed by solid_queue (a database queue) in production, so the delayed purge is durable and survives a restart mid-wait -- it is not "stored in RAM" as the old comment claimed. Add a regression test (CdnCachingTest): a permissions-only update (changing only additional_rights, project row unchanged) must enqueue PurgeCdnProjectJob with the project's record_key. Guards against re-gating the purge on @project.saved_changes?, which would silently serve stale rights. Co-Authored-By: Claude Opus 4.8 Signed-off-by: David A. Wheeler --- app/controllers/projects_controller.rb | 65 ++++++++++++++++---------- test/integration/cdn_caching_test.rb | 23 +++++++++ 2 files changed, 64 insertions(+), 24 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 0a41cd5a6..44675c3dc 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -690,8 +690,34 @@ def create def update # Only accept updates if there's no repo_url change OR if change is ok if repo_url_unchanged_or_change_allowed? - # Send CDN purge early, to give it time to distribute purge request + # Purge this project from the CDN as soon as the request is authorized, + # BEFORE saving, doing BOTH an immediate purge and a delayed re-purge -- + # and do BOTH unconditionally here, not only after a successful save. + # + # Why a delayed re-purge at all: the server always has the newest data + # once committed, but TCP/IP does not guarantee the CDN receives our + # replies in send order. The CDN can receive an *old* in-flight response + # *after* our purge and cache it, holding stale data until max-age. A + # second purge a few seconds later evicts that straggler; the next + # request then repopulates the cache with correct data. + # + # Why unconditionally (rather than only on @project.save success): + # update_additional_rights (below) writes the AdditionalRight table in + # its own transaction -- data the anonymous /permissions page renders -- + # and that write commits independently of @project.save and is NOT + # rolled back if the save later fails. So a save-fails-after-rights- + # changed path still changes what anonymous users see; scheduling the + # delayed re-purge here closes the TCP-reorder race for that path too. + # Extra purges are harmless (no long-term effect), and by this point the + # request is already authorized. + # + # Note: in production ActiveJob is backed by solid_queue (a database + # queue), so the delayed purge is durable -- it survives a restart + # during its wait, and the race-closer is not lost. @project.purge_cdn_project + PurgeCdnProjectJob.set( + wait: BADGE_PURGE_DELAY.seconds + ).perform_later(@project.record_key) # Capture the level being worked on (baseline or traditional badge) old_badge_level = current_working_level(@criteria_level, @project) final_project_params = project_params @@ -744,29 +770,11 @@ def update # after saving. if @project.save successful_update(format, old_badge_level, @criteria_level) - # We must send a purge later, not just now, due to a subtle race - # condition. Here's what is going on. - # The server and the CDN communicate over TCP/IP. This *server* - # will always produce the newest information once it's committed. - # However, TCP/IP does *NOT* guarantee that different replies - # from a server will be received (by the CDN) in the same order that - # they were sent. This means that the CDN can receive *old* data - # after # receiving a purge request and newer data, resulting in - # a CDN caches with obsolete data that will be held for a long time. - # A solution: Wait a short time, then send *another* purge. That way - # even if the CDN receives updates out-of-order, that old data will - # be purged. The next request following this additional purge will - # receive the updated data, and then the CDN will have correct data. - # - # Note: ActiveJob by default stores jobs in RAM. If the system is - # restarted while a job is active, and jobs are stored in RAM, the - # job will be lost and not executed. The long-term solution is to put - # jobs in the database. - PurgeCdnProjectJob.set( - wait: BADGE_PURGE_DELAY.seconds - ).perform_later(@project.record_key) - # Also send CDN purge last, to increase likelihood of being purged - # and replaced with correct data even before the delayed purpose. + # Final immediate purge after the commit, so the freshest data + # evicts any stale copy as soon as possible. The delayed re-purge + # that closes the TCP-reorder race was already scheduled + # unconditionally on entry (see the comment there), so we do not + # schedule another one here. @project.purge_cdn_project else format.html { render :edit, criteria_level: @criteria_level } @@ -811,7 +819,16 @@ def destroy end format.json { head :no_content } end + # Purge the deleted project from the CDN, both immediately and on a delay. + # The delayed re-purge closes the same TCP-reorder race as in update (see + # the long comment there): an old, in-flight show response could reach the + # CDN just after the immediate purge and re-cache a deleted project's page + # for up to max-age. record_key still resolves after destroy! (the id + # remains in memory). solid_queue makes the delayed job durable. @project.purge_cdn_project + PurgeCdnProjectJob.set( + wait: BADGE_PURGE_DELAY.seconds + ).perform_later(@project.record_key) end # rubocop:enable Metrics/MethodLength, Metrics/AbcSize diff --git a/test/integration/cdn_caching_test.rb b/test/integration/cdn_caching_test.rb index 68ec4ed46..d616b966d 100644 --- a/test/integration/cdn_caching_test.rb +++ b/test/integration/cdn_caching_test.rb @@ -7,6 +7,8 @@ # rubocop:disable Metrics/ClassLength class CdnCachingTest < ActionDispatch::IntegrationTest + include ActiveJob::TestHelper + setup do @project = projects(:one) end @@ -158,6 +160,27 @@ class CdnCachingTest < ActionDispatch::IntegrationTest response.headers['Surrogate-Key'] end + # A permissions-only edit changes the AdditionalRight table -- which the + # anonymous /permissions page renders via additional_rights_to_s -- without + # necessarily changing the projects row. The cached page must still be + # purged. projects#update schedules the delayed re-purge UNCONDITIONALLY on + # entry (not gated on @project.save), so even a rights-only change (and the + # save-fails-after-rights-changed path) enqueues a purge of the project's + # surrogate key. This guards against a future refactor that re-gates the + # purge on @project.saved_changes? and would silently serve stale rights. + test 'permissions-only update enqueues a CDN purge of the project key' do + log_in_as(@project.user) + assert_enqueued_with( + job: PurgeCdnProjectJob, args: [@project.record_key] + ) do + patch "/en/projects/#{@project.id}", params: { + # Leave the project row unchanged; exercise the rights-only path. + project: { name: @project.name }, + additional_rights_changes: "+ #{users(:test_user_mark).id}" + } + end + end + def with_forgery_protection original = ActionController::Base.allow_forgery_protection ActionController::Base.allow_forgery_protection = true