Investigate proper handling of nil positions in UpdateDiffPositionService

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

Problem

The UpdateDiffPositionService currently crashes when the position tracer returns a nil position for outdated diff discussions. While MR !200242 (merged) fixes the immediate crash with a defensive nil check, this reveals a deeper architectural issue that needs investigation.

Current Behavior

When Gitlab::Diff::PositionTracer#trace returns { position: nil, outdated: true }, the service attempts to:

1. Update discussion notes with note.change_position = position (nil)
2. Call position.added? on the nil position, causing a crash
3. Create system notes for outdated discussions

Root Cause Analysis

The issue stems from conflicting expectations:

1. Position Tracer legitimately returns nil when lines are no longer in the MR:

   # If the line is no longer in the MR, we unfortunately cannot show
   # the current state on the CD diff or any change on the BD diff,
   # so we treat it as outdated.
   { position: nil, outdated: true }

2. DiffNote Validation requires complete positions when original_position is nil:

   def positions_complete
     return if self.original_position.complete? && self.position.complete?
     errors.add(:position, "is incomplete")
   end

3. Position Processing assumes position is never nil:

   if position.added?
     trace_added_line(position)
   elsif position.removed?
     trace_removed_line(position)
   else # unchanged
     trace_unchanged_line(position)
   end

Investigation Areas

1. Position Tracer Contract: Should the tracer return nil positions, or should it return a special "untraceable" position object?

2. DiffNote Validation: Should validation be updated to handle nil change_position when the discussion is outdated?

3. System Notes: Should SystemNotes::MergeRequestService#diff_discussion_outdated be updated to handle nil positions gracefully?

4. Service Architecture: Should the UpdateDiffPositionService have different code paths for traceable vs untraceable positions?

Proposed Solutions to Evaluate

1. Update Position Tracer: Return a special position object instead of nil
2. Update DiffNote Model: Allow nil change_position for outdated discussions
3. Update System Notes: Handle nil positions in outdated discussion notes
4. Service Refactoring: Separate handling of traceable vs untraceable positions

Acceptance Criteria

• Determine the correct architectural approach for handling untraceable positions
• Implement a solution that doesn't require defensive nil checks
• Ensure consistent behavior across all position-related services
• Add comprehensive tests for edge cases
• Document the expected behavior for future developers

Related

• MR !200242 (merged) - Temporary fix for the crash
• Error logs: https://log.gprd.gitlab.net/app/r/s/bLbsG

Labels

backend devopscreate groupcode review typemaintenance maintenanceusability