Draft: Render cached markdown before the save transaction opens

What does this MR do and why?

Models that use cache_markdown_field render markdown into _html cache columns from before_create/before_update callbacks. Rails runs all persistence callbacks inside the save's transaction, so this render currently happens with a transaction open.

The render is mostly CPU work (CommonMark parsing, a pipeline of Nokogiri filters) interleaved with reference-filter database queries, and can take up to several seconds for the worst documents. As a result, every markdown save leaves the transaction idle-in-transaction for the render's duration: row locks are held, and with PgBouncer in transaction pooling mode a server connection stays pinned for that whole time. Notes, including machine-generated system notes, hit this path constantly.

This MR overrides save/save! in Gitlab::MarkdownCache::ActiveRecord::Extension to render before calling super, i.e. before the transaction opens. There is no Rails hook that runs outside the save transaction, so overriding save/save! is the only model-level seam available.

Rendered HTML is still written to in-memory attributes and persisted atomically in the same INSERT/UPDATE as before. saved_changes, mention storage (run_store_mentions!), and API responses are unchanged.

The before_create/before_update callbacks stay in place as a fallback: they only re-render, inside the transaction, if a validation hook mutated a markdown source or its render context after the pre-render (detected by comparing a snapshot of the source values and Banzai render contexts taken before super). On the normal path the callbacks no-op, so each save renders exactly once.

Models whose render context depends on attributes normally assigned during validation implement the new prepare_markdown_render_context hook, which the pre-render invokes first. Note folds its existing (idempotent, recomputed-on-every-validation) ensure_namespace_id into this hook, so group-level notes get their group render context before the pre-render instead of falling back to an in-transaction re-render.

The behavior is gated by the feature flag render_markdown_cache_outside_transaction (gitlab_com_derisk, default off), checked against Feature.current_request. The flag check happens in save before the transaction opens, so it adds no cost inside the transaction. Disabled, behavior is exactly what it was before this change.

Trade-off: with the flag enabled, rendering happens before validations run, so a save that ultimately fails validation now pays for a render it previously would have skipped. This is rare relative to the per-save transaction cost being removed.

Known remainder: callers that wrap save in their own explicit transaction still end up pre-rendering inside that outer transaction. This MR shrinks the common case (the save's own implicit transaction); enumerating and addressing explicit-transaction callers is follow-up work.

The behavior contract is specced in spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb under "rendering relative to the save transaction" (render depth with the flag on/off, single render per save, and the validation-hook-mutation fallback). The rest of the suite runs with the flag enabled, since flags default on in tests.

References

  • Work item: #608786 (its "impact 2" is markdown rendering inside the write transaction)
  • Rollout issue: #611770

Screenshots or screen recordings

There is no intended UI change; the rendered output is byte-identical by design, so a Before/After comparison doesn't apply. As evidence, here is a comment posted on a local GDK with the flag enabled, showing bold, inline code, a GFM reference link, and a rendered task list:

mr249468-comment

How to set up and validate locally

  1. Check out the branch.

  2. In a Rails console, install a small probe and create notes with the flag on and off to observe the transaction depth at render time:

    module RenderTransactionProbe
      def cacheless_render_field(object, field, context = {})
        puts "render of #{object.class.name}##{field} at transaction depth: #{ApplicationRecord.connection.open_transactions}"
        super
      end
    end
    Banzai::Renderer.singleton_class.prepend(RenderTransactionProbe)
    
    project = Project.find_by_full_path('flightjs/Flight')
    issue = project.issues.first
    author = project.first_owner
    
    Feature.enable(:render_markdown_cache_outside_transaction)
    Note.create!(noteable: issue, project: project, author: author, note: 'probe `one`')
    # => render of Note#note at transaction depth: 0
    
    Feature.disable(:render_markdown_cache_outside_transaction)
    Note.create!(noteable: issue, project: project, author: author, note: 'probe `two`')
    # => render of Note#note at transaction depth: 1
  3. This was executed against a live GDK; the observed output matched the comments above: transaction depth 0 with the flag enabled, depth 1 with it disabled, with identical HTML output in both cases.

  4. UI check: with the flag enabled, post a comment containing markdown (bold, inline code, an issue reference, and a task list) on any issue and confirm it renders normally. This is what the screenshot above shows.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Hordur Freyr Yngvason

Merge request reports

Loading
Loading