fix(script-levels): remove unsafe @@fallback_responses cache#73409
Open
artem-vavilov wants to merge 2 commits into
Open
fix(script-levels): remove unsafe @@fallback_responses cache#73409artem-vavilov wants to merge 2 commits into
artem-vavilov wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the in-process @@fallback_responses cache in ScriptLevelsController#present_level so fallback milestone responses are computed per-request, avoiding reuse across users and request contexts.
Changes:
- Removed the class-variable cache (
@@fallback_responses) keyed by@script_level.id. - Always recompute
@fallback_response(success/failure) for the current request context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the in-process
@@fallback_responsescache fromScriptLevelsController.This cache was keyed only by
@script_level.id, but fallback milestone responses are not a pure function of a script level. They can depend on the selected level variant, current user, user progress, lock/hidden state, section lesson-extras settings, locale, course URL context, hint-view history, and puzzle-rating eligibility.ScriptLeveland its associations only describe curriculum structure. User-specific and request-specific state is looked up separately throughUserLevel, section hidden state, hint requests, puzzle ratings, experiments, and request context. As a result, this cache can return a fallback response built for a different user or request context, causing incorrect fallback redirects, wrong selectedlevel_ids, stale lesson-extras behavior, incorrect puzzle-rating prompts, and possible cross-user leakage of hint-view response data.This removes the cache instead of moving it to
Rails.cachebecause the problem is not where the cache is stored. The cached value itself is context-dependent and has no safe, stable invalidation boundary. Recomputing it per request keeps the response scoped to the actual user, level, locale, and course context.Examples
code-dot-org/dashboard/app/controllers/application_controller.rb
Lines 232 to 234 in baa3fcb
code-dot-org/dashboard/app/controllers/application_controller.rb
Lines 262 to 264 in baa3fcb
code-dot-org/dashboard/app/helpers/application_helper.rb
Lines 219 to 232 in c585546
code-dot-org/dashboard/app/controllers/application_controller.rb
Lines 266 to 269 in baa3fcb
code-dot-org/dashboard/app/controllers/application_controller.rb
Lines 271 to 273 in baa3fcb
Memory leak analysis
@@fallback_responses(unbounded class variable): Grows one entry per curriculum level visited, never evicts. Bounded only by the ~69,571 levels in the database — a data ceiling, not an enforced limit. Measured cost: 364 MB per worker × 32 = 11.4 GB, 5% of host RAM. (Experiment E3, local only — not yet confirmed in production)