Resolve cross DB issue in CacheMarkdownField (vulnerability_user_mentions)

Summary

When some markdown attributes change, CacheMarkdownField trigger SQL queries that touch the notes and vulnerability_user_mentions table, and this create a cross-database issue: vulnerability_user_mentions belongs to sec but notes doesn't.

Further details

Typical sequence of queries:

INSERT INTO "notes" ("note", "noteable_type", "author_id", "created_at", "updated_at", "project_id", "noteable_id", "system", "discussion_id", "note_html", "cached_markdown_version", "namespace_id") VALUES ...
INSERT INTO "system_note_metadata" ("action", "created_at", "updated_at", "note_id") VALUES ... RETURNING "id" 
DELETE FROM "vulnerability_user_mentions" WHERE ..

Debug messages:

D, [2024-11-21T02:16:27.467407 #493194] DEBUG -- :   Note Create (0.8ms)  INSERT INTO "notes" ("note", "noteable_type", "author_id", "created_at", "updated_at", "project_id", "noteable_id", "system", "discussion_id", "note_html", "cached_markdown_version", "namespace_id") VALUES ('changed vulnerability status to Detected', 'Vulnerability', 1114, '2024-11-21 10:16:27.453456', '2024-11-21 10:16:27.460055', 392, 86, TRUE, '7b1b177059878d990ef921301342b06c18baa11e', '<p data-sourcepos="1:1-1:40" dir="auto">changed vulnerability status to Detected</p>', 2162688, 1738) RETURNING "id" /*application:test,correlation_id:d2702fed478c1d80022a4b0ba0167e65,db_config_database:gitlabhq_test,db_config_name:main,line:/app/services/system_notes/base_service.rb:22:in `create_note'*/
D, [2024-11-21T02:16:27.467852 #493194] DEBUG -- :   ↳ app/services/system_notes/base_service.rb:22:in `create_note'
D, [2024-11-21T02:16:27.469561 #493194] DEBUG -- :   SystemNoteMetadata Create (0.3ms)  INSERT INTO "system_note_metadata" ("action", "created_at", "updated_at", "note_id") VALUES ('vulnerability_detected', '2024-11-21 10:16:27.468693', '2024-11-21 10:16:27.468693', 674) RETURNING "id" /*application:test,correlation_id:d2702fed478c1d80022a4b0ba0167e65,db_config_database:gitlabhq_test,db_config_name:main,line:/app/services/system_notes/base_service.rb:22:in `create_note'*/
D, [2024-11-21T02:16:27.469934 #493194] DEBUG -- :   ↳ app/services/system_notes/base_service.rb:22:in `create_note'
D, [2024-11-21T02:16:27.472329 #493194] DEBUG -- :   VulnerabilityUserMention Delete All (0.2ms)  DELETE FROM "vulnerability_user_mentions" WHERE "vulnerability_user_mentions"."vulnerability_id" = 86 AND "vulnerability_user_mentions"."note_id" = 674 /*application:test,correlation_id:d2702fed478c1d80022a4b0ba0167e65,db_config_database:gitlabhq_test_sec,db_config_name:sec,line:/app/models/concerns/cache_markdown_field.rb:168:in `store_mentions!'*/
D, [2024-11-21T02:16:27.472540 #493194] DEBUG -- :   ↳ app/models/concerns/cache_markdown_field.rb:168:in `store_mentions!'

https://gitlab.com/gitlab-org/gitlab/-/blob/1889ea9efb6b6b00b971fe08c39d9f9d28164c03/app/models/note.rb#L32

class Note < ApplicationRecord

  # ..

  include CacheMarkdownField

  cache_markdown_field :note, pipeline: :note, issuable_reference_expansion_enabled: true

https://gitlab.com/gitlab-org/gitlab/-/blob/b7319b5332c83905dff1eb12caa3203e070708b2/app/services/system_notes/base_service.rb#L22

    def create_note(note_summary)
      note_params = note_summary.note.merge(system: true)
      note_params[:system_note_metadata] = SystemNoteMetadata.new(note_summary.metadata) if note_summary.metadata?

      Note.create(note_params)
    end

https://gitlab.com/gitlab-org/gitlab/-/blob/05e99f9c9ee31b03d7911116743b9d5ee8c942cf/app/models/concerns/cache_markdown_field.rb#L166-167

    if references.compact.any?
      user_mention_class.upsert(references.merge(identifier), unique_by: identifier.compact.keys)
    else
      user_mention_class.delete_by(identifier)
    end

NOTE: It usually attempts to delete vulnerability_user_mentions for a note is just created. There should be nothing to delete, but the delete query can't be easily skipped because the code is generic. See #482743 (comment 2230106912)

There's already a loose foreign key in place for vulnerability_user_mentions.note_id. See https://gitlab.com/gitlab-org/gitlab/-/blob/aa7f452f1b6047bc71a8421dcde7cf78854dfc72/config/gitlab_loose_foreign_keys.yml#L759-765

Proposal

Change Gitlab::MarkdownCache::ActiveRecord::Extension so that store_mentions! is called after_commit when user_mention_class is VulnerabilityUserMention. See #507439 (comment 2240236221)

Other options:

  • Rely on loose foreign keys to delete the vulnerability_user_mentions of a Note out of a transaction. However, that wouldn't work for upserts. 🤔
  • Use Vulnerability::StateTransition instead of the Note system, keeping all that's related to vulnerabilities in sec. #482743 (comment 2231061974)
Edited by Fabien Catteau