Skip to content

Fix personal times table, tweak Rubocop settings - #8560

Merged
adi-herwana-nus merged 2 commits into
masterfrom
adi/personal-times-index
Aug 28, 2026
Merged

Fix personal times table, tweak Rubocop settings#8560
adi-herwana-nus merged 2 commits into
masterfrom
adi/personal-times-index

Conversation

@adi-herwana-nus

@adi-herwana-nus adi-herwana-nus commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Backs the (course_user_id, lesson_plan_item_id) uniqueness of Course::PersonalTime with a database
unique index, so the concurrent-write race behind a production incident fails loud instead of silently
creating the duplicate that poisons every later personalisation run.

This is part of the backend half of the incident the client-side PR (#8559) closes from the front.

Background

course_personal_times is a rich join of course_users × course_lesson_plan_items — at most one row
per pair, carrying that user's personalised start_at/bonus_end_at/end_at and a fixed flag. That
"one per pair" invariant was only ever enforced by a model-level uniqueness validation
(validates :course_user, uniqueness: { scope: :lesson_plan_item }), which is a check-then-insert with
no backing unique index.

In the incident, two overlapping PATCHes from the lesson plan editor enqueued two concurrent
CoursewidePersonalizedTimelineUpdateJob runs. For each user both runs passed the validation (neither
transaction saw the other's uncommitted row) and both inserted a row for the same pair. From then on
every personalisation run touching that pair failed the uniqueness validation — including the timeline
recomputation performed synchronously inside submission finalise, which rolled the finalise back. The
duplicate was a permanent poison: affected students could no longer finalise anything in the course.

Key changes

1. Unique index on (course_user_id, lesson_plan_item_id)

db/migrate/20260828083444_add_unique_index_to_course_personal_times.rb.

  • The invariant is now enforced by the database. A genuine race no longer produces two rows: one
    writer commits, the other's transaction raises ActiveRecord::RecordNotUnique and rolls back.
  • Fail loud, by choice. We deliberately did not pair the index with an idempotent
    create_or_find_by/upsert. The point is that the persisted timeline stays consistent with at least
    one write request, and nothing partial survives. Because no duplicate is ever committed, there is
    nothing left to poison future runs — a race now costs at most a transient failure of the racing
    operation (a background job retries and finds the winner's row; a synchronous finalise returns an
    error the student retries) instead of permanently breaking the pair.
  • The model validation stays. On the ordinary single-threaded path it still fires first and returns
    a friendly RecordInvalid — the index is purely the race-only safety net beneath it.
  • Drops the now-redundant index_course_personal_times_on_course_user_id. The composite index leads
    with course_user_id, so it serves every lookup the single-column index did. The
    lesson_plan_item_id index is kept.

Deploy prerequisite: the index build fails while duplicate rows exist, so the remaining duplicate
Course::PersonalTime rows must be removed first (a manual, one-off cleanup — see the incident
summary). This is not part of the migration.

2. Schema-dump stabilisation (first migration under Rails 8.1)

db/migrate/20260828084949_stabilize_schema_dump_artifacts.rb.

This is the first migration to run since the Rails 8.1 upgrade, and 8.1's schema dumper rewrites
schema.rb: column lists are alphabetised, and a few pre-existing objects render differently. Most of
that is unavoidable, one-time churn. Two of the renderings were unstable — they flip depending on
whether a database was built by running migrations or by db:schema:load — so we pinned them to their
dumper-stable form to stop schema.rb drifting on every future migration. Both are semantically
unchanged; verified by comparing predicate trees on a throwaway index and by a rolled-back live
SchemaDumper run.

  • Renamed the course_assessment_answer_programming_test_results id sequence to the
    <table>_id_seq convention. It had kept a truncated name from an old table rename, which stopped the
    PK dumping as id: :serial. The rename is catalog-only and the column default follows it by OID
    (fast, no table lock); it is guarded so db:schema:load-built databases — which already carry the
    conventional name — are skipped.
  • Recreated the index_course_rubric_playground_evaluation_on_answer_rubric partial unique index
    with the per-element predicate form. Postgres normalises the original IN (...) predicate to a
    whole-array cast that pg_get_expr renders one way from a migration and another way after a
    schema.rb round-trip; the per-element form is the fixed point of that round-trip. Same columns, same
    uniqueness, same rows.

One residual dumper change is left as-is: course_material_text_chunks.weight loses its redundant
null: false (a serial column is already NOT NULL; 8.1 stops emitting the redundant option). The
column is unchanged in the database and there is nothing to migrate without a semantic type change.

Testing

  • No application code changes. The write paths (find_or_create_personal_time_for callers,
    PersonalTimesController#create) already save through the validation, and nothing references the
    renamed index or sequence.
  • 46 examples across personal_time_spec, personal_times_controller_spec,
    personalization_concern_spec and coursewide_personalized_timeline_update_job_spec pass with the
    index in place. Confirmed a single-threaded duplicate is still caught by the validation
    (RecordInvalid); only a true race reaches the index.
  • Isolated the intended schema.rb change from the 8.1 alphabetisation churn with a sorted line-set
    diff against the committed baseline — the only semantic differences are the personal-times index swap,
    the pinned rubric predicate, the version bump, and the one residual serial "weight" render. No
    column, index or foreign key was dropped or altered.
  • Verified the stabilised sequence and index forms are equivalent to the originals and produce a stable
    dump (rolled back, DB left untouched).

Notes / follow-ups

Remaining incident mitigations, tracked separately:

  • Clean up the 4 remaining duplicate rows (prerequisite for the index build in production).
  • Decouple the timeline recomputation from the submission finalise transaction, so a background
    concern can never roll back student work. Lands after this index, since it increases concurrency.
  • Add per-course_user locking across the two personalisation entry points, and handle
    PersonalTimesController#create (the single-cell writer outside that funnel).
  • FixedPersonalizationStrategy#execute ignores items_to_shift and deletes personal times
    wholesale — destructive during a coursewide run.
  • Publishing / toggling has_personal_times brings an item into the personalised set with zero
    personal-time rows (the standing TODO(#3448) asymmetry) — the state that makes duplicate inserts
    possible at all.
  • course_personal_times has no created_at/updated_at, which made the incident hard to date.

reference_times is the same rich-join shape one level up (reference_timeline × lesson_plan_item,
same uniqueness: { scope: :lesson_plan_item } with no backing index); worth giving it the same index
if we want the same guarantee there.

This PR also includes some tweaks to our rubocop configuration, to align it with the actual state of the codebase.

…item_id

- resolve wrongly-named sequence for programming_test_results table
…irectives

- completely disable Style/OneClassPerFile
- disable Lint/EmptyBlock in spec directory

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Course::PersonalTime data model by enforcing the (course_user_id, lesson_plan_item_id) uniqueness invariant at the database level, and reduces future churn by stabilizing Rails 8.1 schema dump artifacts. It also updates RuboCop configuration and removes many inline suppression directives now intended to be unnecessary under the updated settings.

Changes:

  • Add a unique composite index on course_personal_times(course_user_id, lesson_plan_item_id) and drop the redundant single-column index.
  • Add a Rails 8.1 migration to stabilize schema dump output (sequence rename + partial index predicate normalization) and commit the Rails 8.1 schema version bump.
  • Update RuboCop configuration/todo and remove many inline rubocop:disable directives across app/spec/lib code.

Reviewed changes

Copilot reviewed 77 out of 78 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
spec/support/stubs/ssid/api_stubs.rb Remove inline RuboCop disable directive.
spec/support/stubs/langchain/llm_stubs.rb Remove inline RuboCop disable directive.
spec/support/stubs/codaveri/evaluate_api_stubs.rb Remove module-length RuboCop directives.
spec/support/reference_timelines_helper.rb Remove inline RuboCop disable/enable directives.
spec/support/devise.rb Remove inline RuboCop disable directive.
spec/factories/course_assessment_question_voice_responses.rb Remove inline RuboCop disable/enable directives.
spec/controllers/course/forum/forums_controller_spec.rb Remove inline RuboCop disable directive.
spec/controllers/course/assessment_marketplace_component_spec.rb Remove inline RuboCop disable directive.
lib/tasks/db/add_missing_email_settings.rake Remove inline RuboCop disable directive.
lib/extensions/materials/active_record/base.rb Remove inline RuboCop disable directive.
lib/extensions/attachable/active_record/base.rb Remove inline RuboCop disable directives.
lib/autoload/duplicator.rb Remove inline RuboCop disable directive.
lib/autoload/course/assessment/programming_test_case_report_builder.rb Remove inline RuboCop disable/enable directives.
db/schema.rb Rails schema version bump to 8.1 + schema dump normalization + new personal-times index.
db/migrate/20260828084949_stabilize_schema_dump_artifacts.rb New migration to stabilize schema dump artifacts (sequence rename + index predicate recreation).
db/migrate/20260828083444_add_unique_index_to_course_personal_times.rb New migration adding composite unique index and dropping redundant index.
app/services/course/video/reminder_service.rb Remove inline RuboCop disable/enable directives.
app/services/course/survey/reminder_service.rb Remove inline RuboCop disable/enable directives.
app/services/course/assessment/submission/update_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/submission/ssid_plagiarism_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/submission/csv_download_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/question/programming/type_script/type_script_package_service.rb Remove inline RuboCop disable directive on class header formatting.
app/services/course/assessment/question/programming/rust/rust_package_service.rb Remove inline RuboCop disable directive on class header formatting.
app/services/course/assessment/question/programming/r/r_package_service.rb Remove inline RuboCop disable directive on class header formatting.
app/services/course/assessment/question/programming/python/python_package_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/question/programming/programming_package_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/question/programming/language_package_service.rb Remove inline RuboCop disable directives from abstract methods.
app/services/course/assessment/question/programming/java/java_package_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/question/programming/java_script/java_script_package_service.rb Remove inline RuboCop disable directive on class header formatting.
app/services/course/assessment/question/programming/go/go_package_service.rb Remove inline RuboCop disable directive on class header formatting.
app/services/course/assessment/question/programming/cpp/cpp_package_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/question/programming/c_sharp/c_sharp_package_service.rb Remove inline RuboCop disable directive on class header formatting.
app/services/course/assessment/question/programming_codaveri/type_script/type_script_package_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/question/programming_codaveri/rust/rust_package_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/question/programming_codaveri/r/r_package_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/question/programming_codaveri/java_script/java_script_package_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/question/programming_codaveri/go/go_package_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/question/programming_codaveri/c_sharp/c_sharp_package_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/question/codaveri_problem_generation_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/programming_codaveri_evaluation_service.rb Remove inline RuboCop disable directives.
app/services/course/assessment/marketplace/apply_version_service.rb Remove inline RuboCop disable/enable directives.
app/services/course/assessment/answer/programming_codaveri_auto_grading_service.rb Remove inline RuboCop disable directive.
app/services/course/assessment/answer/programming_codaveri_async_feedback_service.rb Remove inline RuboCop disable directives.
app/models/course/story.rb Remove inline RuboCop disable directive.
app/models/course/rubric.rb Remove inline RuboCop disable directive.
app/models/course/learning_rate_record.rb Remove inline RuboCop disable directive.
app/models/course/gradebook/level_config.rb Remove inline RuboCop disable directive.
app/models/course/assessment/question/programming.rb Remove inline RuboCop disable directive.
app/models/course/assessment/question.rb Remove inline RuboCop disable directive.
app/models/course/assessment/answer.rb Remove inline RuboCop disable directive.
app/models/course/assessment.rb Adjust inline RuboCop disable directive scope.
app/models/concerns/course/course_user_type_concern.rb Remove inline RuboCop disable/enable directives.
app/helpers/course/discussion/topics_helper.rb Remove inline RuboCop disable directive.
app/helpers/application_html_formatters_helper.rb Remove inline RuboCop disable/enable directives.
app/controllers/system/admin/instance/courses_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/statistics/assessments_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/statistics/aggregate_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/learning_map_controller.rb Remove inline RuboCop disable directives.
app/controllers/course/gradebook_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/courses_controller.rb Remove inline RuboCop disable directives.
app/controllers/course/assessment/submission/submissions_controller.rb Remove inline RuboCop disable directives.
app/controllers/course/assessment/rubrics_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/assessment/question/scribing_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/assessment/question/rubric_based_responses_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/assessment/question/forum_post_responses_controller.rb Remove inline RuboCop disable directive.
app/controllers/course/admin/component_settings_controller.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/unread_counts_concern.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/lesson_plan/strategies/stragglers_personalization_strategy.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/lesson_plan/strategies/otot_personalization_strategy.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/lesson_plan/strategies/fomo_personalization_strategy.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/lesson_plan/strategies/base_personalization_strategy.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/lesson_plan/learning_rate_concern.rb Remove inline RuboCop disable directives.
app/controllers/concerns/course/assessment/submission/monitoring_concern.rb Remove inline RuboCop disable directive.
app/controllers/concerns/course/assessment/question/koditsu_question_concern.rb Remove inline RuboCop disable/enable directives.
app/controllers/concerns/course/assessment/answer/update_answer_concern.rb Remove inline RuboCop disable directive.
app/controllers/concerns/application_user_time_zone_concern.rb Remove inline RuboCop disable directive.
.rubocop.yml Calibrate metrics thresholds; move some exclusions from todo into main config.
.rubocop_todo.yml Regenerate/trim todo entries to match updated RuboCop config and codebase state.
Suppressed comments (1)

app/services/course/assessment/question/programming/language_package_service.rb:69

  • extract_meta doesn’t use attachment / template_files in this base class implementation (it always raises), and the RuboCop suppression was removed. Prefix these arguments with _ to clearly mark them intentionally unused and avoid Lint/UnusedMethodArgument offenses.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@adi-herwana-nus
adi-herwana-nus merged commit 73566ba into master Aug 28, 2026
15 checks passed
@adi-herwana-nus
adi-herwana-nus deleted the adi/personal-times-index branch August 28, 2026 09:46
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.

2 participants