<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Close this issue](https://contributors.gitlab.com/manage-issue?action=close&projectId=278964&issueIid=560134) </details> <!--IssueSummary end--> ## Problem The `UpdateDiffPositionService` currently crashes when the position tracer returns a nil position for outdated diff discussions. While MR !200242 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: ```ruby # 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: ```ruby 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: ```ruby 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 - Temporary fix for the crash - Error logs: https://log.gprd.gitlab.net/app/r/s/bLbsG ## Labels ~"backend" ~"devops::create" ~"group::code review" ~"type::maintenance" ~"maintenance::usability"