Phase 1: Auto-Resolve Todos on MR Close/Merge
## Executive Summary
This proposal outlines Phase 1 implementation for automatically resolving stale todos when merge requests are closed or merged. The current system resolves todos only for the user who performs the action, leaving other assignees/reviewers with accumulating stale todos that clutter their todo dashboard.
**Key Goals:**
1. Auto-resolve todos for ALL affected users (not just the actor) when an MR closes/merges
2. Target specific "stale-prone" action types that become irrelevant after MR completion
3. Implement behind feature flag for safe rollout
4. Maintain performance with async processing
---
## Problem Statement
### Current Behavior
When a merge request is merged or closed, the system calls:
```ruby
# app/services/todo_service.rb:219-225
def resolve_todos_for_target(target, current_user)
attributes = attributes_for_target(target)
resolve_todos(pending_todos([current_user], attributes), current_user)
GraphqlTriggers.issuable_todo_updated(target)
end
```
**Issue:** This only resolves todos for `current_user` (the person who merged/closed the MR), but leaves pending todos for:
- Other assignees
- Reviewers who were requested but didn't complete review
- Approvers whose approval is no longer needed
- Anyone marked as APPROVAL_REQUIRED
### Impact
From issue comments and description:
- Todos accumulate over time, filling dashboards with "outdated litter"
- Users who primarily use GitLab for code review (not task management) find the Todo feature unusable
- Mentioned as important to commercial customers
- Estimated 6.3M stale todos on GitLab.com (from issue description)
---
## Proposed Solution
### Overview
Modify the todo resolution logic to automatically resolve specific action types for ALL affected users when an MR is merged or closed.
### Stale-Prone Action Types
These action types become definitively irrelevant when an MR is closed/merged:
```ruby
STALE_ON_MR_COMPLETION = [
Todo::ASSIGNED, # 1 - Assignment is no longer actionable
Todo::APPROVAL_REQUIRED, # 5 - (EE) Approval no longer needed
Todo::REVIEW_REQUESTED, # 9 - Review no longer needed
Todo::ADDED_APPROVER # 13 - (EE) Approval no longer needed
].freeze
```
**Excluded Action Types** (remain pending after MR close/merge):
- `MENTIONED` (2) - User may still want to see the mention for context
- `DIRECTLY_ADDRESSED` (7) - Similar to mentions, may need follow-up
- `BUILD_FAILED` (3) - Historical information that user may want to review
- `UNMERGEABLE` (6) - Historical information
- `REVIEW_SUBMITTED` (11) - Completed action, user may want to track
### Architecture Changes
#### 1. New Service Method
**File:** `app/services/todo_service.rb`
Add new method to resolve todos for all affected users:
```ruby
# Resolves specific todo action types for all users when MR is closed/merged
#
# @param [MergeRequest] merge_request - The closed/merged MR
# @param [User] current_user - The user who performed the action
# @return [Array<Integer>] IDs of resolved todos
def resolve_stale_todos_for_merge_request(merge_request, current_user)
return [] unless merge_request.closed? || merge_request.merged?
stale_action_types = [
Todo::ASSIGNED,
Todo::APPROVAL_REQUIRED,
Todo::REVIEW_REQUESTED,
Todo::ADDED_APPROVER
]
# Find all pending todos for this MR with stale-prone action types
stale_todos = Todo.pending
.where(
target_type: 'MergeRequest',
target_id: merge_request.id,
action: stale_action_types
)
# Resolve todos and update user caches
todos_ids = stale_todos.batch_update(
state: :done,
resolved_by_action: :system_done,
snoozed_until: nil
)
# Update todo counts for all affected users
affected_user_ids = stale_todos.distinct.pluck(:user_id)
affected_user_ids.each do |user_id|
user = User.find_by_id(user_id)
user&.update_todos_count_cache
end
# Trigger GraphQL subscription updates
GraphqlTriggers.issuable_todo_updated(merge_request)
todos_ids
end
```
#### 2. Async Worker for Performance
**New File:** `app/workers/merge_requests/resolve_stale_todos_worker.rb`
```ruby
# frozen_string_literal: true
module MergeRequests
class ResolveStaleTodosWorker
include ApplicationWorker
data_consistency :always
sidekiq_options retry: 3
feature_category :code_review_workflow
urgency :low
idempotent!
def perform(merge_request_id, current_user_id)
merge_request = MergeRequest.find_by_id(merge_request_id)
current_user = User.find_by_id(current_user_id)
return unless merge_request && current_user
TodoService.new.resolve_stale_todos_for_merge_request(
merge_request,
current_user
)
end
end
end
```
#### 3. Integration Points
**File:** `app/services/merge_requests/post_merge_service.rb`
```ruby
def execute(merge_request, source = nil)
return if merge_request.merged?
merge_request.mark_as_merged
create_event(merge_request)
# Existing: resolve todos for current user only
todo_service.merge_merge_request(merge_request, current_user)
# NEW: resolve stale todos for all affected users
if Feature.enabled?(:auto_resolve_stale_mr_todos, merge_request.project)
MergeRequests::ResolveStaleTodosWorker.perform_async(
merge_request.id,
current_user.id
)
end
merge_request_activity_counter.track_merge_mr_action(user: current_user)
# ... rest of method
end
```
**File:** `app/services/merge_requests/close_service.rb`
```ruby
def execute(merge_request, commit = nil, skip_reset: false)
return error('Merge request is not open') unless merge_request.open?
unless can_close_merge_request?(merge_request)
return error("Insufficient permissions to close the merge request", :forbidden)
end
if merge_request.close
event_service.close_mr(merge_request, current_user)
create_note(merge_request, 'closed')
notification_service.async.close_mr(merge_request, current_user)
# Existing: resolve todos for current user only
todo_service.close_merge_request(merge_request, current_user)
# NEW: resolve stale todos for all affected users
if Feature.enabled?(:auto_resolve_stale_mr_todos, merge_request.project)
MergeRequests::ResolveStaleTodosWorker.perform_async(
merge_request.id,
current_user.id
)
end
execute_hooks(merge_request, 'close')
invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches
# ... rest of method
end
merge_request
end
```
---
## Implementation Details
### Feature Flag
**Name:** `auto_resolve_stale_mr_todos`
**Type:** `development` initially, then `ops` for gradual rollout
**Scope:** Project-level (allows per-project enablement)
**Default:** `false` (disabled by default)
**Registration:**
```ruby
# config/feature_flags/development/auto_resolve_stale_mr_todos.yml
---
name: auto_resolve_stale_mr_todos
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/issues/20637
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/XXXXX
milestone: '17.x'
type: development
group: group::code review
default_enabled: false
```
### Performance Considerations
1. **Async Processing:** Use Sidekiq worker to avoid blocking MR merge/close operations
2. **Batch Updates:** Leverage `Todo.batch_update` which uses single SQL UPDATE
3. **Index Requirements:** Ensure indexes exist on `todos(target_id, target_type, state, action)`
4. **Query Performance:** Expected query pattern:
```sql
UPDATE todos
SET state = 'done',
resolved_by_action = 0,
updated_at = NOW()
WHERE target_type = 'MergeRequest'
AND target_id = ?
AND state = 'pending'
AND action IN (1, 5, 9, 13)
```
### Database Impact
**Estimated Load:**
- GitLab.com merges/closes ~50,000 MRs per day (estimate)
- Average 2-3 pending stale todos per MR
- ~100,000-150,000 todo updates per day
- Spread across 24 hours = ~1.7 updates/second (negligible)
**Existing Indexes (from schema):**
```ruby
# app/models/todo.rb (from schema)
add_index "todos", ["target_type", "target_id"], name: "index_todos_on_target_type_and_target_id"
add_index "todos", ["user_id", "state"], name: "index_todos_on_user_id_and_state"
add_index "todos", ["state"], name: "index_todos_on_state"
```
**Recommended Additional Index:**
```ruby
add_concurrent_index :todos, [:target_type, :target_id, :state, :action],
name: 'index_todos_on_target_and_state_and_action',
where: "state = 'pending' AND target_type = 'MergeRequest'"
```
---
## Testing Strategy
### Unit Tests
**File:** `spec/services/todo_service_spec.rb`
```ruby
describe '#resolve_stale_todos_for_merge_request' do
let(:merge_request) { create(:merge_request) }
let(:user1) { create(:user) }
let(:user2) { create(:user) }
let(:current_user) { create(:user) }
context 'when MR is merged' do
before do
merge_request.mark_as_merged
end
it 'resolves ASSIGNED todos for all users' do
todo1 = create(:todo, :assigned, target: merge_request, user: user1)
todo2 = create(:todo, :assigned, target: merge_request, user: user2)
service.resolve_stale_todos_for_merge_request(merge_request, current_user)
expect(todo1.reload.state).to eq('done')
expect(todo2.reload.state).to eq('done')
expect(todo1.resolved_by_action).to eq('system_done')
end
it 'resolves REVIEW_REQUESTED todos' do
todo = create(:todo, :review_requested, target: merge_request, user: user1)
service.resolve_stale_todos_for_merge_request(merge_request, current_user)
expect(todo.reload.state).to eq('done')
end
it 'does not resolve MENTIONED todos' do
todo = create(:todo, :mentioned, target: merge_request, user: user1)
service.resolve_stale_todos_for_merge_request(merge_request, current_user)
expect(todo.reload.state).to eq('pending')
end
it 'updates todo counts for affected users' do
create(:todo, :assigned, target: merge_request, user: user1)
expect(user1).to receive(:update_todos_count_cache)
service.resolve_stale_todos_for_merge_request(merge_request, current_user)
end
end
context 'when MR is closed' do
before do
merge_request.close!
end
it 'resolves stale todos' do
todo = create(:todo, :review_requested, target: merge_request, user: user1)
service.resolve_stale_todos_for_merge_request(merge_request, current_user)
expect(todo.reload.state).to eq('done')
end
end
context 'when MR is still open' do
it 'does not resolve any todos' do
todo = create(:todo, :assigned, target: merge_request, user: user1)
service.resolve_stale_todos_for_merge_request(merge_request, current_user)
expect(todo.reload.state).to eq('pending')
end
end
end
```
**File:** `spec/workers/merge_requests/resolve_stale_todos_worker_spec.rb`
```ruby
require 'spec_helper'
RSpec.describe MergeRequests::ResolveStaleTodosWorker do
describe '#perform' do
let(:merge_request) { create(:merge_request, :merged) }
let(:user) { create(:user) }
let(:service) { instance_double(TodoService) }
before do
allow(TodoService).to receive(:new).and_return(service)
end
it 'calls TodoService to resolve stale todos' do
expect(service).to receive(:resolve_stale_todos_for_merge_request)
.with(merge_request, user)
described_class.new.perform(merge_request.id, user.id)
end
it 'handles missing merge request gracefully' do
expect { described_class.new.perform(0, user.id) }.not_to raise_error
end
it 'handles missing user gracefully' do
expect { described_class.new.perform(merge_request.id, 0) }.not_to raise_error
end
end
end
```
### Integration Tests
**File:** `spec/services/merge_requests/post_merge_service_spec.rb`
```ruby
context 'with auto_resolve_stale_mr_todos feature flag' do
let(:assignee) { create(:user) }
let(:reviewer) { create(:user) }
before do
merge_request.assignees << assignee
merge_request.reviewers << reviewer
create(:todo, :assigned, target: merge_request, user: assignee)
create(:todo, :review_requested, target: merge_request, user: reviewer)
end
context 'when feature flag is enabled' do
before do
stub_feature_flags(auto_resolve_stale_mr_todos: true)
end
it 'enqueues worker to resolve stale todos' do
expect(MergeRequests::ResolveStaleTodosWorker)
.to receive(:perform_async)
.with(merge_request.id, current_user.id)
service.execute(merge_request)
end
end
context 'when feature flag is disabled' do
before do
stub_feature_flags(auto_resolve_stale_mr_todos: false)
end
it 'does not enqueue worker' do
expect(MergeRequests::ResolveStaleTodosWorker)
.not_to receive(:perform_async)
service.execute(merge_request)
end
end
end
```
### Request Specs
**File:** `spec/requests/api/merge_requests_spec.rb`
Add test case for PUT `/merge_requests/:id/merge`:
```ruby
context 'when auto-resolving stale todos' do
let(:assignee) { create(:user) }
before do
stub_feature_flags(auto_resolve_stale_mr_todos: true)
merge_request.assignees << assignee
create(:todo, :assigned, target: merge_request, user: assignee)
end
it 'resolves stale todos for all affected users', :sidekiq_inline do
api_put merge_request, current_user
expect(assignee.todos.pending.count).to eq(0)
expect(assignee.todos.done.count).to eq(1)
end
end
```
---
## Rollout Plan
### Stage 1: Development & Testing (Week 1-2)
1. Implement service method and worker
2. Add comprehensive test coverage
3. Create feature flag
4. Code review and merge
### Stage 2: Staging Validation (Week 3)
1. Deploy to staging.gitlab.com
2. Enable feature flag for select test projects
3. Validate functionality and performance
4. Monitor Sidekiq queue depth and job duration
### Stage 3: Production Rollout (Week 4-6)
1. **Week 4:** Enable for GitLab internal projects (dogfooding)
- Monitor: todo resolution rates, user feedback
2. **Week 5:** Enable for 10% of GitLab.com projects
- Monitor: database load, Sidekiq performance, error rates
3. **Week 6:** Gradual rollout to 50%, then 100%
- Continue monitoring metrics
4. **Week 7:** Remove feature flag, make default behavior
### Monitoring Metrics
**Application Metrics:**
- `gitlab_todo_service_resolve_stale_duration_seconds` - Time to resolve stale todos
- `gitlab_todo_service_resolve_stale_total` - Counter of resolved todos
- `gitlab_todos_resolved_by_action{action="system_done"}` - Counter by resolution type
**Database Metrics:**
- Query duration for todo update queries
- Todos table size reduction over time
- User todo count cache hit rate
**User Experience Metrics:**
- Todo dashboard visit frequency (should increase)
- Time spent on todo dashboard (should increase)
- Manual todo dismissal rate (should decrease)
---
## Success Criteria
### Quantitative
- [ ] Zero new stale todos created for target action types
- [ ] Worker job completion rate > 99.9%
- [ ] P95 job duration < 500ms per MR
- [ ] Zero production incidents related to feature
- [ ] Database query time < 50ms per update
### Qualitative
- [ ] No user complaints about incorrect todo resolution
- [ ] Positive feedback from dogfooding team
- [ ] Code review approval from Code Review team
- [ ] Security review approval (if required)
---
## Risks & Mitigations
### Risk 1: Users Want to Keep Certain Todos
**Scenario:** A user wants to review an MR even after it's merged
**Mitigation:**
- Phase 1 only targets clearly stale action types (ASSIGNED, REVIEW_REQUESTED, APPROVAL_REQUIRED)
- Does NOT auto-resolve MENTIONED or DIRECTLY_ADDRESSED todos
- Users can still manually mark todos as "pending" if they want to track something
- Future: Add user preference to opt-out of auto-resolution
### Risk 2: Performance Impact on Large MRs
**Scenario:** MR with 50+ pending todos causes slow updates
**Mitigation:**
- Async processing prevents blocking merge operation
- Batch update uses single SQL statement regardless of todo count
- Monitored rollout catches issues early
- Can add rate limiting or batch size limits if needed
### Risk 3: EE Action Types Unavailable in CE
**Scenario:** `APPROVAL_REQUIRED` and `ADDED_APPROVER` don't exist in CE
**Mitigation:**
```ruby
stale_action_types = [
Todo::ASSIGNED,
Todo::REVIEW_REQUESTED
]
# Only include EE action types if they exist
if defined?(Todo::APPROVAL_REQUIRED)
stale_action_types << Todo::APPROVAL_REQUIRED
stale_action_types << Todo::ADDED_APPROVER
end
```
### Risk 4: Race Condition with Reopened MRs
**Scenario:** MR is closed, todos resolved, then MR is reopened
**Mitigation:**
- Todos stay resolved (correct behavior - they were already actioned)
- New assignment/review request creates fresh todos
- Users can manually re-create todos if needed
- Document expected behavior in feature documentation
---
## Future Enhancements (Out of Scope for Phase 1)
### User Preferences
Allow users to customize auto-resolution behavior:
```ruby
# app/models/user_preference.rb
attribute :auto_resolve_mr_todos, :boolean, default: true
attribute :auto_resolve_issue_todos, :boolean, default: false
```
### Phase 2: Historical Data Cleanup
Background migration to clean up existing 6.3M stale todos (separate proposal)
### Extended Action Types
Consider adding:
- `UNMERGEABLE` - when MR becomes mergeable or is closed
- `BUILD_FAILED` - when build succeeds or MR is closed
### Issue Todo Resolution
Apply similar logic when issues are closed
---
## Documentation Updates
### User Documentation
**File:** `doc/user/todos.md`
Add section:
```markdown
### Automatic todo resolution
When a merge request is merged or closed, GitLab automatically marks the following
types of todos as done for all affected users:
- **Assigned** - You were assigned to the merge request
- **Review requested** - You were requested to review the merge request
- **Approval required** - Your approval was required (GitLab Premium and Ultimate)
- **Approver added** - You were added as an approver (GitLab Premium and Ultimate)
Other todo types (like mentions or direct addresses) are not automatically resolved,
as you may still want to review the conversation or code changes after the merge
request is closed.
You can still manually mark any automatically-resolved todo as pending if you want
to track it.
```
### API Documentation
**File:** `doc/api/merge_requests.md`
Update merge and close endpoints to mention automatic todo resolution behavior.
### Changelog Entry
```yaml
---
title: Auto-resolve stale todos when merge requests are closed or merged
merge_request: XXXXX
author:
type: changed
```
---
## Files to Create/Modify
### New Files
- [ ] `app/workers/merge_requests/resolve_stale_todos_worker.rb`
- [ ] `spec/workers/merge_requests/resolve_stale_todos_worker_spec.rb`
- [ ] `config/feature_flags/development/auto_resolve_stale_mr_todos.yml`
### Modified Files
- [ ] `app/services/todo_service.rb` - Add `resolve_stale_todos_for_merge_request` method
- [ ] `app/services/merge_requests/post_merge_service.rb` - Call new worker
- [ ] `app/services/merge_requests/close_service.rb` - Call new worker
- [ ] `spec/services/todo_service_spec.rb` - Add test coverage
- [ ] `spec/services/merge_requests/post_merge_service_spec.rb` - Add test coverage
- [ ] `spec/services/merge_requests/close_service_spec.rb` - Add test coverage
- [ ] `spec/requests/api/merge_requests_spec.rb` - Add request spec
- [ ] `doc/user/todos.md` - Update user documentation
- [ ] `doc/api/merge_requests.md` - Update API documentation
---
## Estimated Effort
- **Implementation:** 3-5 days
- **Testing:** 2-3 days
- **Code Review:** 1-2 days
- **Documentation:** 1 day
- **Rollout & Monitoring:** 1-2 weeks
**Total:** ~3 weeks from start to full rollout
---
## Questions for Discussion
1. **Should we resolve todos for the MR author?**
- Author already gets their todos resolved when they close/merge
- But if another user closes/merges, should author's todos also resolve?
- Recommendation: Yes, for consistency
2. **Should this apply to draft MRs?**
- Draft MRs can still be closed/merged
- Recommendation: Yes, same behavior
3. **Should we add a GraphQL mutation to manually trigger this?**
- Useful for testing and edge cases
- Recommendation: Not for Phase 1, can add later if needed
4. **Should we send notifications about auto-resolved todos?**
- Could be noisy for users with many todos
- Recommendation: No notifications, silent resolution
5. **What about MRs that are closed then reopened?**
- Todos stay resolved (they were addressed)
- New assignment/review creates fresh todos
- Recommendation: Document this behavior, no special handling
---
## References
- **Issue:** https://gitlab.com/gitlab-org/gitlab/-/issues/20637
- **Related Issue:** https://gitlab.com/gitlab-org/gitlab-ce/issues/30521
- **Todo Model:** [app/models/todo.rb](app/models/todo.rb)
- **TodoService:** [app/services/todo_service.rb](app/services/todo_service.rb)
- **MR Close Service:** [app/services/merge_requests/close_service.rb](app/services/merge_requests/close_service.rb)
- **MR Post-Merge Service:** [app/services/merge_requests/post_merge_service.rb](app/services/merge_requests/post_merge_service.rb)
epic