Prevent deleted Lambda auto-annotation requests from overwriting a newer same-task request - #11048
Open
ksm463 wants to merge 3 commits into
Open
Prevent deleted Lambda auto-annotation requests from overwriting a newer same-task request#11048ksm463 wants to merge 3 commits into
ksm463 wants to merge 3 commits into
Conversation
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
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.
Motivation and context
Fixes #11016.
DELETE /api/lambda/requests/{id}unconditionally deleted the RQ job hash, includingwhile the job was
STARTED. Because Lambda auto-annotation derives a deterministicRQ id from the task (
create:task{id}style), deleting a running job immediately freesthat id for reuse while the old work-horse is still alive and still writing annotations.
That allows two failure modes for the same task:
overwrites the newer request's progress/result;
surfaces to users as duplicated or mixed annotations.
This PR implements the minimal-conservative policy (option 1) proposed in #11016:
a running request is no longer cancelable, so its id can never be recycled underneath it.
LambdaQueue.delete_job()takes the sameget_rq_lock_for_job(queue, pk)lock thatenqueue()uses, so a DELETE can no longer race a concurrent POST for the same task.STARTED→409 Conflict, and the hash is left untouched.QUEUED/DEFERRED/SCHEDULED→cancel()thendelete(), so a queued requestcan still be dropped and immediately re-submitted for the same task.
FINISHED/FAILED/CANCELED/STOPPED) → deleted as before.LambdaJob.delete()is removed;RequestViewSet.destroy()was its only caller.Permission checks are unchanged:
check_object_permissions()still runs on thefetch_job()result, and the status is then re-checked inside the lock.Behavior change worth flagging.
DELETEon a running request now returns409where it previously returned
204, so the UI's "Cancel" button on an in-progressauto-annotation will surface an error instead of appearing to succeed. That appearance
was not accurate before this change — the DELETE removed the hash but never stopped the
work-horse, which is exactly the bug above. Genuinely interrupting a running job needs
the larger stop-and-ack design (option 2 in #11016: progress/annotation write guards,
a stop command, idempotent handling). I kept this PR to the minimal fix and am happy to
follow up with option 2, or to adjust the UI message here, if you prefer.
How has this been tested?
Server unit tests, run in the
cvat_cicontainer againstcvat.settings.testing:Two tests that were previously
@skip("Fail: add mock")are now implemented and pass:test_api_v2_lambda_requests_create_two_requests— characterizes the existingenqueue()lock: POSTing a second request for the same task while the first isSTARTEDreturns 409.test_api_v2_lambda_requests_delete_not_finished_request— the regression test forthis fix. It asserts that DELETE on a
STARTEDrequest is rejected, that the job hashsurvives, that a second request for the same task stays blocked until the first reaches
a terminal state, and that afterwards the second request runs exactly once and reuses
the same deterministic id.
Because
cvat.settings.testingforces every RQ queue toASYNC=False, a normal requestcompletes inline and can never be observed as
STARTED. The tests therefore monkeypatchthe queue's
enqueue_jobto persist the job inSTARTEDwithout running it, and flipthe status explicitly to reproduce the lifecycle without a live worker or a real Nuclio
function. No Nuclio deployment is required to run them.
Verified against the fix reverted:
test_api_v2_lambda_requests_delete_not_finished_requestfails with
AssertionError: 204 not found in (400, 409), i.e. it reproduces the reportedbug rather than merely passing.
Full
cvat.apps.lambda_manager.tests.test_lambdarun is green.Checklist
developbranchLicense
Feel free to contact the maintainers if that's a concern.