Log system notes that fail to persist

zlirequested to merge
zl/614048 into master

What does this MR do and why?

SystemNotes::BaseService#create_note calls Note.create and discards the return value, and no caller checks it either. A system note that fails to save therefore disappears with no log line, no exception, and no metric.

That silence is currently blocking the investigation into intermittently missing "added N commits" notes. On a confirmed production case we can show that the job completed successfully, that no exception was raised, and — from the job's db_main_write_count — that no INSERT was issued. Nothing records why the save was abandoned.

This MR logs that failure. It is observability only: the unpersisted note is still returned and callers behave exactly as before.

The payload distinguishes the two ways a save can fail without raising:

note_errors Meaning
populated a validation rejected the record
empty a callback halted the save

It also carries noteable_type, noteable_id, note_action, and note_bytesize, so a failure can be tied back to a specific noteable and to the size of the body being rendered.

Log volume is negligible: the line is only emitted when a note fails to persist.

Why not Note.create!

Raising here would be a much larger change than it looks. create_note is shared by every system-note service, and in MergeRequests::RefreshService an exception from notify_about_push aborts the rest of the loop and skips execute_async_workers — which is what enqueues MergeRequests::Refresh::PipelineWorker, WebHooksWorker, and (EE) ApprovalWorker. A missing system note would become a missing CI pipeline. UpdateMergeRequestsWorker is also explicitly not idempotent, so the retries would re-run diff reloading.

Logging first tells us the failure rate and the cause before we decide what the fix should be.

References

How to set up and validate locally

bundle exec rspec spec/services/system_notes/base_service_spec.rb

To see the log line, stub a failing save in a Rails console:

project  = Project.first
issue    = project.issues.first
author   = project.first_owner
summary  = NoteSummary.new(issue, project, author, 'added 1 commit', action: 'commit')

allow(Note).to receive(:create).and_return(Note.new)   # or make the record invalid
SystemNotes::BaseService.new(noteable: issue, container: project, author: author)
  .send(:create_note, summary)

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.

Merge request reports