fix: don't force-delete workers when end_date moves to past - #5188
fix: don't force-delete workers when end_date moves to past#5188cursor[bot] wants to merge 2 commits into
Conversation
handle_end_date_change_for_challenge called delete_workers whenever end_date was set in the past, killing queued/running submissions. Route through the pending-aware cleanup Lambda instead so the queue can drain first (#5179). Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5188 +/- ##
==========================================
- Coverage 91.30% 91.29% -0.02%
==========================================
Files 115 115
Lines 8996 9019 +23
==========================================
+ Hits 8214 8234 +20
- Misses 782 785 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
@coderabbitai Please review this! |
|
✅ Action performedReview finished.
|
WalkthroughThe change adds near-term EventBridge scheduling for expired challenge cleanup. It updates or creates schedules, skips unsupported environments, and logs AWS failures. Past challenge end dates now use this scheduler instead of immediate worker deletion. ChangesChallenge cleanup scheduling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChallengeModel
participant EventBridge
participant CleanupLambda
ChallengeModel->>EventBridge: schedule cleanup approximately one minute later
EventBridge->>CleanupLambda: invoke cleanup Lambda
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/challenges/test_aws_utils.py (1)
5564-5650: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest non-not-found scheduler failures.
A non-
ResourceNotFoundExceptionfromupdate_scheduleexecutes the re-raise path and the outer error handler, but no new test covers that behavior. Add a test that injects a differentClientError, verifies thatcreate_scheduleis not called, and confirms that the helper returns normally.As per path instructions, test code must cover edge cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/challenges/test_aws_utils.py` around lines 5564 - 5650, Extend TestScheduleChallengeCleanupSoon with a test for schedule_challenge_cleanup_soon where update_schedule raises a ClientError using a non-ResourceNotFoundException code. Assert create_schedule is not called and the helper returns normally, covering the outer error-handling path without changing existing tests.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/challenges/test_aws_utils.py`:
- Around line 5643-5648: Update test_schedule_soon_skipped_in_debug to patch
valid scheduler configuration and get_boto3_client, then assert the client
factory is not called after schedule_challenge_cleanup_soon runs. Preserve the
DEBUG settings patch and ensure the test fails if the debug guard is removed.
---
Nitpick comments:
In `@tests/unit/challenges/test_aws_utils.py`:
- Around line 5564-5650: Extend TestScheduleChallengeCleanupSoon with a test for
schedule_challenge_cleanup_soon where update_schedule raises a ClientError using
a non-ResourceNotFoundException code. Assert create_schedule is not called and
the helper returns normally, covering the outer error-handling path without
changing existing tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 179d099c-cd29-4c35-b44d-afb3ec7015c8
📒 Files selected for processing (3)
apps/challenges/aws_utils.pyapps/challenges/models.pytests/unit/challenges/test_aws_utils.py
| @patch("challenges.aws_utils.settings", DEBUG=True) | ||
| def test_schedule_soon_skipped_in_debug(self, mock_settings): | ||
| challenge = MagicMock() | ||
| challenge.pk = 42 | ||
|
|
||
| schedule_challenge_cleanup_soon(challenge) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the debug skip behavior.
test_schedule_soon_skipped_in_debug has no assertion. Patch valid scheduler configuration and get_boto3_client, then assert that the client factory is not called. The current test can pass if the debug guard is removed.
As per path instructions, test code must assert behavior and must not silently pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/challenges/test_aws_utils.py` around lines 5643 - 5648, Update
test_schedule_soon_skipped_in_debug to patch valid scheduler configuration and
get_boto3_client, then assert the client factory is not called after
schedule_challenge_cleanup_soon runs. Preserve the DEBUG settings patch and
ensure the test fails if the debug guard is removed.
Source: Path instructions
2 participants
Summary
handle_end_date_change_for_challengecalleddelete_workerswhenever a challenge'send_datewas moved into the past. That force-deleted the ECS service even when submissions were stillqueued/running/resuming, undoing the pending-aware cleanup contract from #5179.Trigger scenario
end_dateinto the past (early close or date correction).delete_workers.Changes
end_datechanges throughschedule_challenge_cleanup_soon, which schedules the pending-aware cleanup Lambda shortly after now (EventBridge rejects pastat()times).Test plan
pytest tests/unit/challenges/test_aws_utils.py::TestScheduleChallengeCleanupSoon tests/unit/challenges/test_aws_utils.py::TestHandleEndDateChange— 11 passedblack --check/isort --checkon changed filesSummary by CodeRabbit