Skip to content

Fixed global role and access-control rows not rotating partial cache keys - refs #1400 - #1402

Open
philayres wants to merge 1 commit into
consected:developfrom
hmsrc:phase1-cache-key-correctness-1400
Open

Fixed global role and access-control rows not rotating partial cache keys - refs #1400#1402
philayres wants to merge 1 commit into
consected:developfrom
hmsrc:phase1-cache-key-correctness-1400

Conversation

@philayres

Copy link
Copy Markdown
Contributor

Summary

Phase 1 of #1400 (the final stage of #1362). Cache-key correctness only — deliberately no behaviour change to when the login-scoped key rotates. current_sign_in_at remains in partial_cache_key throughout, so the existing every-login cache bust stays fully in place.

This is intentionally shipped ahead of the rest of #1400. Every change here only increases invalidation sensitivity, so there is no window in which anything can be served staler than it is today. The hazard runs the other way: removing current_sign_in_at (Phase 3) before fixing the global-row gap below would drop the login safety net while the gap still exists.

The bug

ApplicationHelper#latest_updated_at_token scoped its role/UAC lookups with where(app_type_id:), excluding global rows (app_type_id IS NULL) — the shared roles and access controls that apply across all app types via role-name matching. A change to a global role or access control therefore never rotated partial_cache_key, template_version, the pages#template ETag, or any fragment cached under them.

HandlebarsPrecompilerHelper#app_type_access_control_timestamps already did this correctly with [app_type_id, nil] (PR #1283). The two had drifted because the same query existed twice. This is the last row of #1289's issue 4.

Currently masked in production because current_sign_in_at busts the key on every login — which is exactly what the rest of #1400 removes.

Changes

Fixed the global-row scoping. Role/UAC timestamp lookups now include app_type_id: nil rows, matching the scoping pattern used by UserAndRoles#where_user_and_role and PageLayoutsHelper#page_layout_panels.

Consolidated the duplicated query. The role/UAC timestamp lookup now lives once, on HandlebarsPrecompiler, alongside item_update_classes and item_updates_key — which already serve as the shared source of truth for cache-key inputs. ApplicationHelper#latest_updated_at_token is removed (it had no other callers) and both helpers delegate. Defined as a method rather than a constant, matching the existing convention there: referencing autoloaded AR classes from a top-level constant in an initializer breaks Zeitwerk boot ordering.

Since the divergence fixed above was caused by having two copies of this query, leaving a second copy in place would have preserved the exact failure mode.

Delegated the item-updates class list in partial_cache_key to HandlebarsPrecompiler.item_update_classes, replacing an inline duplicate of the same array. Identical content today; done so the two cannot drift.

Report is deliberately not in that list, and a spec now locks that in. The list also feeds HandlebarsPrecompiler.generation_key, so adding Report would rotate every compiled Handlebars generation on every report config edit — forcing full node-CLI recompiles of content that does not depend on report definitions. Verified there is no user-facing report dependency among partial_cache_key's consumers: report_table_header self-scopes by embedding @report.id/@report.updated_at in its partial name, and page_layouts/_show_row's cache_result sits in the elsif resource_def branch that reports never reach.

Collapsed a duplicated query pair. pages#template computes partial_cache_key twice per request (once for the ETag, once via template_version), differing only in the partial prefix. The role/UAC timestamps are now memoized per app_type_id on the view context, so the second call reuses them. Measured: a 304 revalidation drops from 22 to 20 SQL queries.

Removed the @@prev_partial_cache_key change-logging. It warned on every key change and retained a class-variable hash keyed by partial/auth-type/user id that was never bounded or evicted — a slow leak fed by the per-login key churn this stage exists to remove.

Specs

spec/helpers/application_helper_spec.rb gains coverage for the global-row fix using real Admin::UserAccessControl and Admin::UserRole rows rather than stubbed relations, so it exercises the actual query.

Two details are load-bearing, and both are documented inline:

  • Each example stubs clear_rails_cache_on_save? to false. Without it, creating the row clears the whole Rails cache, dropping server_cache_version — which is itself part of the key — so the key rotates for an unrelated reason and the example passes even with the fix reverted. Same masking spec/models/admin/app_type_available_ids_cache_spec.rb guards against.
  • Rows are created with an explicit future updated_at. Cache-key timestamps interpolate at second granularity, so a same-second write does not rotate the key.

Both examples were verified to fail with the fix reverted (before/after keys byte-identical) and pass with it restored.

Also added: current_sign_in_at still rotates the key for both User and Admin (guards Phase 1 against straying into Phase 3's scope), the ETag/template_version timestamp reuse, absence of the removed logging, and Report's absence from the item-update list.

spec/system/user/app_type_switch_master_tabs_spec.rb gains a multi-user login/reload smoke example, with a comment stating plainly that it is smoke coverage — it would pass identically before and after this fix, and is not the regression test for it.

Also included

spec/requests/pages/handlebars_compile_cost_benchmark_spec.rb documented itself as opt-in via RUN_BENCHMARKS, but carried no benchmark: true tag, so filter_run_excluding never excluded it and it ran in every normal suite pass. Tagged correctly. Verified: 0 examples without RUN_BENCHMARKS, 2 with.

spec/requests/pages/template_revalidation_cost_benchmark_spec.rb is new — it measures the cost of revalidating pages#template rather than caching it immutably, which is the evidence base for #1400's Phase 4 decision to drop Cache-Control: immutable:

304 revalidation   queries=20   elapsed=44.9ms    bytes=0
200 warm fragment  queries=48   elapsed=844.2ms   bytes=14876
200 cold fragment  queries=417  elapsed=1067.6ms  bytes=14876

Confirms the ETag needs no ERB render — stale? returns before render — so a revalidation costs a fraction of the cold render that currently happens on every login.

Verification

  • spec/helpers/application_helper_spec.rb, spec/helpers/handlebars_precompiler_helper_spec.rb, spec/controllers/pages_controller_spec.rb, spec/requests/pages/151 examples, 0 failures.
  • The handlebars helper specs matter most here: app_type_access_control_timestamps feeds handlebars_cache_key and access_control_version, which are load-bearing for the Stage 1 content addressing in Hardened and shared the on-disk Handlebars template compile cache across users - refs #1362 #1377.
  • Rubocop: no new offences on the changed files.
  • Revert test on both global-row examples: fails when reverted, passes when restored.

Refs #1400, #1362, #1289, #1283.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant