Add queue timeout for stuck pre-running submissions#5144
Draft
RishabhJain2018 wants to merge 1 commit into
Draft
Add queue timeout for stuck pre-running submissions#5144RishabhJain2018 wants to merge 1 commit into
RishabhJain2018 wants to merge 1 commit into
Conversation
Introduce submission_queue_time_limit on Challenge (default 2 hours) and periodic cleanup via fail_stuck_submissions management command and Celery beat task. Submissions in submitted/submitting/queued/resuming states that exceed the limit are marked failed with a clear stderr message. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
WalkthroughAdds a configurable per-challenge ChangesAuto-fail submissions stuck in queue
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Command as fail_stuck_submissions_task/Command
participant Job as fail_stuck_submissions
participant Query as get_stuck_submission_queryset
participant DB as Submission
participant EKS as trigger_eks_node_autoscale
Command->>Job: fail_stuck_submissions(challenge_pk, dry_run)
Job->>Query: get_stuck_submission_queryset(challenge_pk)
Query->>DB: filter queue-stuck submissions
DB-->>Job: submission list
loop each submission
Job->>Job: is_submission_queue_timed_out(submission)
alt timed out
Job->>DB: fail_submission_due_to_queue_timeout(submission)
DB->>DB: status=FAILED, completed_at, stderr.txt
DB->>EKS: trigger_eks_node_autoscale(current/previous status)
end
end
Job-->>Command: failed count
🚥 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| try: | ||
| os.makedirs("/tmp/evalai") | ||
| except OSError: |
![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 (2)
apps/jobs/queue_timeout.py (1)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
submission.save()afterstderr_file.save().
FileField.save()with the defaultsave=Truealready callsinstance.save()internally, persistingstatus,completed_at, and the newstderr_filepath. The explicitsubmission.save()on line 66 is an unnecessary extra database write.♻️ Proposed refactor
submission.stderr_file.save( "stderr.txt", ContentFile(QUEUE_TIMEOUT_STDERR.encode("utf-8")), ) - submission.save() challenge = submission.challenge_phase.challenge🤖 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 `@apps/jobs/queue_timeout.py` around lines 62 - 66, The queue timeout handling in submission processing has a redundant extra save after writing the stderr file. In the code that uses submission.stderr_file.save, remove the explicit submission.save() because FileField.save() already persists the model by default; keep the stderr_file save as the single persistence step and ensure the submission status/completed_at updates remain in the same flow around that call.tests/unit/jobs/test_queue_timeout.py (1)
214-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
SUBMITTINGandRESUMINGqueue states.The queryset test only validates
SUBMITTED,QUEUED, andRUNNING. Per the PR objectives,STUCK_QUEUE_STATUSESalso includessubmittingandresuming. Adding assertions for these two statuses would verify the full inclusion set and guard against accidental filter regressions.As per path instructions, tests should cover edge cases and not silently pass.
♻️ Proposed addition
def test_get_stuck_submission_queryset_includes_queue_states(self): submitted = self._create_submission(status=Submission.SUBMITTED) queued = self._create_submission(status=Submission.QUEUED) + submitting = self._create_submission(status=Submission.SUBMITTING) + resuming = self._create_submission(status=Submission.RESUMING) running = self._create_submission(status=Submission.RUNNING) stuck_pks = set( get_stuck_submission_queryset().values_list("pk", flat=True) ) self.assertIn(submitted.pk, stuck_pks) self.assertIn(queued.pk, stuck_pks) + self.assertIn(submitting.pk, stuck_pks) + self.assertIn(resuming.pk, stuck_pks) self.assertNotIn(running.pk, stuck_pks)🤖 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/jobs/test_queue_timeout.py` around lines 214 - 225, The queryset test for get_stuck_submission_queryset currently covers only SUBMITTED, QUEUED, and RUNNING, so extend test_get_stuck_submission_queryset_includes_queue_states to also create submissions in SUBMITTING and RESUMING and assert both pks are included in stuck_pks. Use the existing _create_submission helper and Submission status constants to keep the test aligned with STUCK_QUEUE_STATUSES and guard against regressions in the queue-state filter.Source: Path instructions
🤖 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 `@apps/jobs/queue_timeout.py`:
- Around line 88-108: The batch loop in queue_timeout should isolate failures
per submission so one bad item does not stop processing the rest. Wrap the body
of the per-submission logic around get_stuck_submission_queryset() and
fail_submission_due_to_queue_timeout() in a try/except, log the exception with
submission.pk and challenge.pk for context, and then continue to the next
submission. Keep the existing warning path for successful timeouts, and make
sure the error handling preserves the loop behavior in queue_timeout.py.
---
Nitpick comments:
In `@apps/jobs/queue_timeout.py`:
- Around line 62-66: The queue timeout handling in submission processing has a
redundant extra save after writing the stderr file. In the code that uses
submission.stderr_file.save, remove the explicit submission.save() because
FileField.save() already persists the model by default; keep the stderr_file
save as the single persistence step and ensure the submission
status/completed_at updates remain in the same flow around that call.
In `@tests/unit/jobs/test_queue_timeout.py`:
- Around line 214-225: The queryset test for get_stuck_submission_queryset
currently covers only SUBMITTED, QUEUED, and RUNNING, so extend
test_get_stuck_submission_queryset_includes_queue_states to also create
submissions in SUBMITTING and RESUMING and assert both pks are included in
stuck_pks. Use the existing _create_submission helper and Submission status
constants to keep the test aligned with STUCK_QUEUE_STATUSES and guard against
regressions in the queue-state filter.
🪄 Autofix (Beta)
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: 5131df87-f8dc-4415-a28f-48fa04a85ba5
⛔ Files ignored due to path filters (1)
apps/challenges/migrations/0128_challenge_submission_queue_time_limit.pyis excluded by!**/migrations/**
📒 Files selected for processing (8)
apps/challenges/models.pyapps/challenges/serializers.pyapps/challenges/views.pyapps/jobs/management/commands/fail_stuck_submissions.pyapps/jobs/queue_timeout.pyapps/jobs/tasks.pysettings/common.pytests/unit/jobs/test_queue_timeout.py
Comment on lines
+88
to
+108
| for submission in get_stuck_submission_queryset(challenge_pk): | ||
| if not is_submission_queue_timed_out(submission, now=now): | ||
| continue | ||
|
|
||
| previous_status = submission.status | ||
| queue_started_at = submission.rerun_resumed_at or submission.submitted_at | ||
| challenge = submission.challenge_phase.challenge | ||
|
|
||
| if fail_submission_due_to_queue_timeout(submission, dry_run=dry_run): | ||
| failed_count += 1 | ||
| action = "Would fail" if dry_run else "Failed" | ||
| logger.warning( | ||
| "%s queue-stuck submission pk=%s challenge_pk=%s status=%s " | ||
| "queue_started_at=%s queue_time_limit=%ss", | ||
| action, | ||
| submission.pk, | ||
| challenge.pk, | ||
| previous_status, | ||
| queue_started_at, | ||
| challenge.submission_queue_time_limit, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing per-submission error handling in batch loop.
If fail_submission_due_to_queue_timeout raises an exception for any single submission (e.g., storage failure on stderr_file.save(), or trigger_eks_node_autoscale AWS API error), the exception propagates and terminates the loop. All remaining stuck submissions are skipped until the next 30-minute cycle.
The autoscale failure case is particularly concerning: the submission is already persisted as FAILED by stderr_file.save(), so the next run won't pick it up (it's no longer in STUCK_QUEUE_STATUSES), but the autoscale trigger is permanently lost.
Wrap each iteration in a try/except, log the error, and continue processing the rest.
🛡️ Proposed fix
for submission in get_stuck_submission_queryset(challenge_pk):
if not is_submission_queue_timed_out(submission, now=now):
continue
previous_status = submission.status
queue_started_at = submission.rerun_resumed_at or submission.submitted_at
challenge = submission.challenge_phase.challenge
- if fail_submission_due_to_queue_timeout(submission, dry_run=dry_run):
- failed_count += 1
- action = "Would fail" if dry_run else "Failed"
- logger.warning(
- "%s queue-stuck submission pk=%s challenge_pk=%s status=%s "
- "queue_started_at=%s queue_time_limit=%ss",
- action,
- submission.pk,
- challenge.pk,
- previous_status,
- queue_started_at,
- challenge.submission_queue_time_limit,
- )
+ try:
+ if fail_submission_due_to_queue_timeout(
+ submission, dry_run=dry_run
+ ):
+ failed_count += 1
+ action = "Would fail" if dry_run else "Failed"
+ logger.warning(
+ "%s queue-stuck submission pk=%s challenge_pk=%s "
+ "status=%s queue_started_at=%s "
+ "queue_time_limit=%ss",
+ action,
+ submission.pk,
+ challenge.pk,
+ previous_status,
+ queue_started_at,
+ challenge.submission_queue_time_limit,
+ )
+ except Exception:
+ logger.exception(
+ "Failed to process queue-stuck submission pk=%s "
+ "challenge_pk=%s status=%s",
+ submission.pk,
+ challenge.pk,
+ previous_status,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for submission in get_stuck_submission_queryset(challenge_pk): | |
| if not is_submission_queue_timed_out(submission, now=now): | |
| continue | |
| previous_status = submission.status | |
| queue_started_at = submission.rerun_resumed_at or submission.submitted_at | |
| challenge = submission.challenge_phase.challenge | |
| if fail_submission_due_to_queue_timeout(submission, dry_run=dry_run): | |
| failed_count += 1 | |
| action = "Would fail" if dry_run else "Failed" | |
| logger.warning( | |
| "%s queue-stuck submission pk=%s challenge_pk=%s status=%s " | |
| "queue_started_at=%s queue_time_limit=%ss", | |
| action, | |
| submission.pk, | |
| challenge.pk, | |
| previous_status, | |
| queue_started_at, | |
| challenge.submission_queue_time_limit, | |
| ) | |
| for submission in get_stuck_submission_queryset(challenge_pk): | |
| if not is_submission_queue_timed_out(submission, now=now): | |
| continue | |
| previous_status = submission.status | |
| queue_started_at = submission.rerun_resumed_at or submission.submitted_at | |
| challenge = submission.challenge_phase.challenge | |
| try: | |
| if fail_submission_due_to_queue_timeout( | |
| submission, dry_run=dry_run | |
| ): | |
| failed_count += 1 | |
| action = "Would fail" if dry_run else "Failed" | |
| logger.warning( | |
| "%s queue-stuck submission pk=%s challenge_pk=%s " | |
| "status=%s queue_started_at=%s " | |
| "queue_time_limit=%ss", | |
| action, | |
| submission.pk, | |
| challenge.pk, | |
| previous_status, | |
| queue_started_at, | |
| challenge.submission_queue_time_limit, | |
| ) | |
| except Exception: | |
| logger.exception( | |
| "Failed to process queue-stuck submission pk=%s " | |
| "challenge_pk=%s status=%s", | |
| submission.pk, | |
| challenge.pk, | |
| previous_status, | |
| ) |
🤖 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 `@apps/jobs/queue_timeout.py` around lines 88 - 108, The batch loop in
queue_timeout should isolate failures per submission so one bad item does not
stop processing the rest. Wrap the body of the per-submission logic around
get_stuck_submission_queryset() and fail_submission_due_to_queue_timeout() in a
try/except, log the exception with submission.pk and challenge.pk for context,
and then continue to the next submission. Keep the existing warning path for
successful timeouts, and make sure the error handling preserves the loop
behavior in queue_timeout.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds automatic cleanup for submissions stuck in pre-running queue states (
submitted,submitting,queued,resuming) — the class of issues seen with FALCON H1 submissions after the June 30 outage.Changes
submission_queue_time_limit(default 7200s / 2 hours). Set to0to disable per challenge.apps/jobs/queue_timeout.pyfinds timed-out submissions and marks themfailedwith stderr"Submission exceeded the queue time limit and was not picked up for evaluation."python manage.py fail_stuck_submissions [--challenge-pk PK] [--dry-run]jobs.tasks.fail_stuck_submissions_taskruns every 30 minutessubmission_queue_time_limitadded toupdate_challenge_attributesallowed fieldsHow it works
COALESCE(rerun_resumed_at, submitted_at)exceeds the challenge'ssubmission_queue_time_limit.failed,completed_atis set, and stderr is written.trigger_eks_node_autoscaleis called for docker-based challenges.Configuration
submission_queue_time_limiton the ChallengePOST /api/challenges/challenge/update_challenge_attributes/withsubmission_queue_time_limitsubmission_queue_time_limit: 3600submission_queue_time_limit: 0Notes
submission_time_limit(execution timeout once a pod is running).python manage.py fail_stuck_submissions --dry-runthen without--dry-run.Testing
Added
tests/unit/jobs/test_queue_timeout.py. Could not run pytest in this cloud VM (no Docker/Python deps); please verify in CI or local dev environment.Slack Thread
Summary by CodeRabbit
New Features
Bug Fixes