Repository navigation
fix: race in recent_visited_task audit field writes - #9943
pablohashescobar wants to merge 1 commit into
Conversation
- Previously the task created the row, then called save(update_fields=...) to set created_by/updated_by. If a concurrent task evicted the row (the cap-at-20 delete) in between, the second save raised "Save with update_fields did not affect any rows". - Set created_by_id/updated_by_id on the initial INSERT and pass disable_auto_set_user=True so BaseModel.save doesn't null them (there is no request user in a background task). - Add a unit test asserting audit fields are set with a single save.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe new recent-visit path sets ChangesRecent visit creation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge risk remains: the recent-visit creation path avoids the second write, and the regression test verifies the persisted audit fields. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change preserves existing user and workspace ownership while saving audit fields together with the new visit. It removes a failure window without adding an entrypoint, increasing privileges, or weakening the reviewed access controls. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused fix addresses the reported race and includes regression coverage, with no unresolved findings.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes a race in recent-visit tracking by setting audit fields during the initial insert.
Changes:
- Removes the race-prone follow-up save while preserving audit fields.
- Adds regression coverage for audit values and a single save call.
| File | Description |
|---|---|
| apps/api/plane/tests/unit/bg_tasks/test_recent_visited_task.py | Verifies audit fields and single-save behavior. |
| apps/api/plane/bgtasks/recent_visited_task.py | Sets audit fields during creation without automatic user overrides. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Description
recent_visited_taskcreated aUserRecentVisitrow and then ran a secondsave(update_fields=["created_by_id", "updated_by_id"])to set the audit fields. If a concurrent task evicted that row in between (the cap-at-20 delete earlier in the task), the second save raised "Save with update_fields did not affect any rows".This sets
created_by_id/updated_by_idin the initial INSERT and saves once withdisable_auto_set_user=True, soBaseModel.savedoesn't null them (there is no request user in a background task). A unit test covers it.Type of Change
Screenshots and Media (if applicable)
Test Scenarios
docker compose -f docker-compose-test.yml run --rm api-tests pytest plane/tests/unit/bg_tasks/test_recent_visited_task.py: a new visit hascreated_by_idandupdated_by_idset to the user, andsaveis called exactly once.recent_visited_task.References
No linked work item.
🤖 Generated with Claude Code
Summary by CodeRabbit