Reset existing dap_powered reviewer assignment strategy rows

Marc Shawrequested to merge
607678-reset-dap-powered-rows into master

Adds one post-deploy migration that resets project_settings.reviewer_assignment_strategy rows holding 2 (dap_powered) back to 0 (disabled). This merges after Phase D (!253170 (merged), merged 2026-09-07) - the enum-removal trap that used to force the opposite order is gone. The migration is now a single unbatched UPDATE, and the row count has been measured: 6 rows on gitlab.com.

By the time this runs the bespoke path is already deleted and the flag is already off, so this is tidy-up rather than the step that changes behaviour. Nothing blocks it - the affected rows are 6 internal projects on a Beta flag that never reached GA, and they get a heads-up in the internal feedback thread rather than a formal migration.

Detailed context for AI agents

Background

The dap_powered reviewer assignment strategy was a bespoke project setting, now replaced by the standard Duo Agent Platform flow-trigger setup. !248476 (merged) removed both write paths on 2026-08-20, but rows created before that still hold the old value.

Ordering flip

The old version of this description said the dap_powered enum removal "must not ship until this migration has run." That is no longer the case. The only thing that ever forced this migration (Phase C) before the code deletion (Phase D, !253170 (merged)) was a NoMethodError trap: Phase D was going to delete the dap_powered: 2 enum value, which would have deleted the generated reviewer_assignment_dap_powered? predicate out from under its caller in AutoAssignReviewersWorker. Phase D was rescoped on 2026-08-21 - as !248482 (closed), since closed in favour of !253170 (merged) - to keep the enum value, so that dependency is gone.

With it gone, the correct order is the revertible code change first and the irreversible data change last. Doing it the other way round meant taking an unrecoverable data change to fix a symptom that only existed because the code still read the column.

Full agreed sequence

  1. !242698 (merged) (Add merge_request created Duo Agent Platform trigger) merged 2026-08-28, and merge_request_create_flow_trigger was enabled globally on gprd 2026-09-01 (issue #618743 (closed)). It restores coverage for merge requests opened non-draft, which the bespoke path had and the marked-ready trigger does not.
  2. dap_powered_recommend_reviewers was turned off on gprd and gstg 2026-09-02. Rollout issue #602418 (closed). This is the step where the feature actually stops running, and it reverts in seconds.
  3. !253170 (merged) (Phase D) merged 2026-09-07, deleting what was by then already-dead code.
  4. This MR merges and the reset runs - the only remaining step.

What this migration is actually for

Worth being precise, because the original issue framing overstated it. Stale rows never break anything. StrategyFactory::STRATEGIES maps only code_owners, so a dap_powered row misses the lookup, build returns nil, and AssignService returns skipped('No strategy available') as a success. That is true before the enum is dropped and after it, since an unmapped raw value deserializes to nil and misses the same lookup.

And the feature loss does not happen here either - it happens at step 2, the flag flip. By the time this migration runs those projects have already stopped getting DAP assignment.

So what is left is genuine but small:

  1. wasted work - reviewer_auto_assignment_enabled? stays true (!= 'disabled'), so AutoAssignReviewersWorker keeps getting enqueued on every create and ready transition just to no-op
  2. dishonest state - the row holds a live-looking value that nothing acts on, and the settings checkbox shows unchecked

That is the whole case for it. If a reviewer decides that is not worth a production data migration, closing this MR is a defensible outcome - the rows are permanently inert after Phase D either way.

Correction to carry over

An earlier version of this description claimed there is "no longer any way to move a project off the dap_powered value." That is wrong and must not be repeated. The settings checkbox has checked_value: 'code_owners' and unchecked_value: 'disabled', so any save on the project's Merge requests settings page clears a stale row. The rows are self-healing on .com. The migration matters mainly because self-managed instances cannot be fixed by hand.

Why disabled and not code_owners

Resetting to code_owners would silently start assigning every code owner on projects that deliberately chose the narrower DAP strategy. disabled is the honest default; owners who want code owners can opt back in from settings.

Migration shape

One post-deploy migration, 20260825144211. A single unbatched UPDATE - no each_batch, no BATCH_SIZE, no disable_ddl_transaction!:

def up
  execute(<<~SQL)
    UPDATE project_settings
    SET reviewer_assignment_strategy = 0
    WHERE reviewer_assignment_strategy = 2
  SQL
end

There is no index on reviewer_assignment_strategy, so the cost is one sequential scan of project_settings however this is written. each_batch paid for three scans and bought nothing, because batching only helps when the match set is large enough to want the lock broken up, and six rows is not that. Only the six matched rows are locked, for the duration of the scan. Expected runtime is roughly 107 s, well inside the 10 minute post-deploy guideline.

project_settings is table_size: medium and not on the high-traffic list (rubocop/rubocop-migrations.yml). Its primary key is project_id, not id.

Measured on gitlab.com

db:gitlabcom-database-testing ran against the production main clone at commit c7f0967e, on the previous each_batch revision:
matching rows 6
total runtime 309.6 s, 3 timing-guideline warnings
first boundary probe 124.6 s (SELECT project_id ... ORDER BY project_id LIMIT)
the UPDATE 107.3 s, 6 rows
second boundary probe 74.9 s, 0 rows
ci database 3.1 s, skipped (wrong gitlab_schema)

That is what motivated dropping the batching: two of those three scans existed only to discover there was no second batch.

It also settles the temporary partial index question, which an earlier revision had open for the database reviewer. In the same pipeline, AddIndexProjectSettingsOnAiAuditEventsStorage - an index on this same table - measured 77.1 s. That is most of the 107 s an index would save, and it would be dropped again immediately, so the three-migration shape from the service desk settings backfill (db/post_migrate/20260717205311..13) is not worth it here.

There is no schema change at all, so db/structure.sql is untouched and only one db/schema_migrations/ version file is added.

down is a no-op. The column does not retain the previous value, so this is not reversible without a record of the affected projects captured beforehand - which is what the audit is for.

No blockers, and no audit

Earlier revisions of this MR and of #607678 (closed) blocked on auditing which affected rows ever actually functioned, and on notifying or migrating those projects first. Both are dropped. Nothing blocks this MR.

There are 6 affected rows, they are internal projects, and the feature was a Beta capability behind a wip flag with default_enabled: false that never reached GA. The consent argument for notify-before-change was calibrated for customer projects, where creating a flow trigger inside someone's project is an action in their space; it does not apply to internal projects on an opt-in beta. Affected teams get a note in the internal feedback thread (#603172), and anyone who wants the behaviour back creates a Merge request > Marked ready trigger on the Recommend Reviewers flow, per !248483 (merged).

Consequence to be aware of rather than to act on: down stays a no-op with no rollback artifact, so after this runs there is no record of which projects held the value. For reference, the gate for "ever functioned" was Project#dap_powered_recommend_reviewers_available? (feature flag with project actor, plus duo_foundational_flows_enabled, plus recommend_reviewers/v1 in enabled_flow_catalog_item_ids, plus StageCheck) - deleted by !253170 (merged), and reading a flag that is off by step 2, so it is not reconstructable after the fact.

Enum removal is a separate later cleanup

The dap_powered: 2 enum value is NOT removed here, and it is no longer part of Phase D either. When it does go it has to carry three things:

  • a correction to doc/api/projects.md, which !248476 (merged) amended in four places to promise that GET /projects/:id can still return dap_powered for projects configured before 19.4
  • an update to ee/spec/views/projects/settings/merge_requests/_reviewer_auto_assignment_settings.html.haml_spec.rb:62, which calls build(:project_setting, reviewer_assignment_strategy: :dap_powered) and will raise ArgumentError the moment the enum value goes
  • an update to ee/spec/workers/merge_requests/auto_assign_reviewers_worker_spec.rb:94, added by !253170 (merged), which has a context ("when reviewer_assignment_strategy is still dap_powered") that deliberately exercises the stale value and will need updating when the enum value is dropped

Notes for review

  • The migration uses the literal 2 and 0 rather than the enum, so it does not depend on application code.
  • Residual write path worth flagging, deliberately not fixed here: the HTML settings controller permits the attribute with no value allowlist (ee/app/controllers/ee/projects/settings/merge_requests_controller.rb:38) and the enum has no validate: (app/models/project_setting.rb:92), so a hand-crafted form POST could still write dap_powered back after this runs. Judged not worth blocking on: the value is inert, no UI affords it, and it closes when the enum is eventually dropped. A model validation would be worse - it would make existing dap_powered rows fail validation on unrelated project_setting saves.
  • The previous pipeline failed static-analysis 2/2 on gitlab:js:routes:updated_check - path-helper drift from master unrelated to this MR. The branch was rebased onto master 7bd1e20d6730 on 2026-09-07, which resolved it.
  • Milestone 19.4 is correct - master is 19.4.0-pre.

Testing

bundle exec rspec spec/migrations/20260825144211_reset_dap_powered_reviewer_assignment_strategy_spec.rb

4 examples, 0 failures. RuboCop clean. scripts/validate_migration_checksum and scripts/validate_migration_timestamps pass. Migration run up locally against main/ci/sec.

Re-verified after the 2026-09-07 rebase onto master 7bd1e20d6730: same 4 examples, 0 failures, RuboCop clean on both files. The diff is still exactly 3 files (migration, db/schema_migrations/20260825144211, migration spec), no schema change.

Coverage: the dap_powered row resets to disabled; code_owners and disabled rows are untouched; and the old batching example was replaced by one that asserts exactly one UPDATE project_settings statement is issued with four matching rows present, so it fails the moment batching is reintroduced. down is asserted to leave the reset in place.

The db/schema_migrations/ checksum file did not need regenerating - the checksum is a SHA256 of the version string, not of the migration body.

References

MR acceptance checklist

Edited by Marc Shaw

Merge request reports