Backport of 'Render only first note when replying to a discussion' to 19-3

What does this MR do and why?

Backport of !262926 (merged) to 19-3-stable-ee.

Replying to a discussion via POST .../merge_requests/:iid/discussions/:discussion_id/notes loaded and Banzai-rendered every note in the discussion just to read the first one. The cost grew linearly with the thread size, so a long thread made each reply O(N) and the whole thread O(N^2). This changes the handler to load only the first note and check its readability, rendering nothing.

This was flagged for backport in https://gitlab.com/gitlab-com/gl-infra/production/-/work_items/23286 to reach supported self-managed and Dedicated versions sooner.

Root cause and details

In the POST route (lib/api/discussions.rb), only notes.first and notes.empty? were used, but readable_discussion_notes loads all N notes and prepare_and_filter_notes -> prepare_notes_for_rendering runs four preloaders and full Banzai Markdown rendering of every note, then an O(N) readable_by? filter. All of it was discarded except element [0].

So reply #k rendered the k-1 earlier notes; across a thread that is O(N^2). Diff notes make each render heavier.

It also explains the behavior at the per-noteable limit (Noteable::MAX_NOTES_LIMIT = 5000): once an MR hit the cap, every further POST still ran the full render before Note#does_not_exceed_notes_limit? rejected it, so a bot retrying against a maxed thread paid the full render on each refused request.

Observed on one MR: 5021 notes, one diff discussion holding ~4529 of them, almost all from a single bot.

Fix

The handler only needs the first note (for to_discussion, can_reply_to_system_note?, part_of_discussion?, can_convert_to_discussion?) and whether the user can read it. It now loads just that note and calls readable_by?, matching the work items API (lib/api/work_items/discussion_notes.rb). No notes are rendered on this path.

readable_by? needs no rendering for regular notes; for a system note it uses all_references, which costs about one note and this endpoint rejects system-note replies anyway. Dropping the old render-then-filter cannot change behavior: confidentiality is uniform across a discussion and these noteable types reject replies to system notes, so a thread whose root is unreadable but whose replies are readable is not reachable.

The readable_discussion_notes helper is unchanged and still used by the GET discussion endpoints, which legitimately render all notes for presentation.

Benchmarks

From the reproducer in https://gitlab.com/gitlab-org/gitlab/-/work_items/634294, CPU per reply POST (best of 3), each note referencing 5 issues and mentioning 1 user:

Notes in thread Total CPU before Total CPU after
1 503 ms 478 ms
100 649 ms 478 ms
400 1,404 ms 537 ms
1,000 3,034 ms 632 ms

Backport notes

This backport applies the net change of the original MR as a single commit. The original branch had review-driven spec churn (an earlier approach added a fallback scan and a helper, both reworked after review); the final merged state touches only lib/api/discussions.rb and spec/requests/api/discussions_spec.rb, which is what is reproduced here. The net diff applied cleanly with no conflicts.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

  • This MR is backporting a bug fix, documentation update, or spec fix, previously merged in the default branch.
  • The MR that fixed the bug on the default branch has been deployed to GitLab.com (not applicable for documentation or spec changes).
  • The MR title is descriptive (e.g. "Backport of 'title of default branch MR'"). This is important, since the title will be copied to the patch blog post.
  • Required labels have been applied to this merge request
  • This MR has been approved by a maintainer (only one approval is required).
  • Ensure the E2E Omnibus GitLab EE pipeline (triggered by thee2e:test-on-omnibus-ee job) has succeeded. If it has failed (or shows warnings), investigate the failures. If you determine the failures are unrelated, you may proceed. If you need assistance investigating, request help in the #s_developer_experience Slack channel to confirm the failures are unrelated to the merge request.

Note to the merge request author and maintainer

If you have questions about the patch release process, please:

Edited by Stan Hu

Merge request reports

Loading
Loading