Skip to content

Fix keyed add_jobs race and preserve attempts on locked batch jobs - #646

Open
fortizflexidao wants to merge 1 commit into
graphile:mainfrom
fortizflexidao:fix/add-jobs-keyed-race-and-batch-attempts
Open

fortizflexidao wants to merge 1 commit into
graphile:mainfrom
fortizflexidao:fix/add-jobs-keyed-race-and-batch-attempts

Conversation

@fortizflexidao

Copy link
Copy Markdown

Description

Fixes #580.

This PR adds migration 000021.sql to fix two edge cases in graphile_worker.add_jobs when scheduling jobs with a job_key:

  1. Silent drop when a row becomes locked between UPDATE and INSERT (Race in add_job/add_jobs with keyed jobs can drop scheduling requests (and returns no row under contention) #580):
    Under READ COMMITTED, the initial UPDATE ... WHERE key = spec.job_key AND is_available IS NOT TRUE skips unlocked rows without locking them. If a worker claims that row (or another transaction inserts and locks the same key) before INSERT ... ON CONFLICT (key) DO UPDATE WHERE jobs.locked_at IS NULL runs, WHERE jobs.locked_at IS NULL evaluates to false and Postgres returns no row.

    • Lock existing rows matching spec.job_key with FOR UPDATE (ordered by key) before the key-clearing UPDATE, so a concurrent get_job (FOR UPDATE SKIP LOCKED) skips them.
    • Loop over any remaining specs whose key is still held by a locked row after the upsert (covering the case where another transaction inserted and locked the key concurrently).
  2. Lost retries on in-flight batch jobs (json_typeof(payload) = 'array'):
    When a locked job has its key cleared because a new job with the same job_key arrives, add_jobs sets attempts = jobs.max_attempts so the superseded job will not retry if it fails. For batch jobs (JSON array payload), the newly inserted row only contains the new elements, not the elements currently being processed by the locked batch job. Preserving jobs.attempts when json_typeof(jobs.payload) = 'array' and jobs.locked_at is not null allows the in-flight batch to still retry its failed elements if it errors out.

Performance impact

unknown

Security impact

unknown

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race in add_job/add_jobs with keyed jobs can drop scheduling requests (and returns no row under contention)

1 participant