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
- severity label and bug subtype labels (if applicable)
- If this MR fixes a bug that affects customers, the customer label has been applied.
- This MR has been approved by a maintainer (only one approval is required).
- Ensure the
E2E Omnibus GitLab EEpipeline (triggered by thee2e:test-on-omnibus-eejob) 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:
- Refer to the patch release runbook for engineers and maintainers for guidance.
- Ask questions on the
#releasesSlack channel (internal only). - Once the backport has been merged, the commit changes will be automatically deployed to a release environment that can be used for manual validation. See after merging runbook for details.