Skip to content

fix: create the sync event before queueing its job - #1114

Open
rohan-chaturvedi wants to merge 1 commit into
mainfrom
ccr-3223d18b-5uspv9
Open

rohan-chaturvedi wants to merge 1 commit into
mainfrom
ccr-3223d18b-5uspv9

Conversation

@rohan-chaturvedi

Copy link
Copy Markdown
Member

🔍 Overview

Sync jobs could start before their sync event existed. trigger_sync_tasks queued the job with sync_func.delay(), and only then created the EnvironmentSyncEvent with the job's ID. When a worker picked the job up first, handle_sync_event looked up the sync's latest event and:

  • found none (a new sync): it failed at the end with AttributeError: 'NoneType' object has no attribute 'completed_at', and the sync stayed "in progress";
  • or found an older event of the sync: it overwrote that event with this run's result, and the new event stayed "queued" for good.

This affects every provider. It was seen while testing #1044, with a GitLab sync created through the UI.

💡 Proposed Changes

  • trigger_sync_tasks creates the event first, then queues the job with the event's ID (delay(env_sync, job_id=...), which RQ's @job decorator supports). If queueing fails, the event fails too ("Failed to queue the sync job"), as does the sync.
  • handle_sync_event updates the event of the running job (rq.get_current_job()), so it never updates another run's event. Outside a worker, e.g. when called directly, it uses the sync's latest event, as before.
  • A job without an event still records the sync's status, and looks for its event again at the end, e.g. a job queued by older code during a deploy. It no longer fails with AttributeError.
  • handle_sync_event works out the status and result once, and applies them to the sync and its event. What is recorded doesn't change: the same statuses and messages for success, failure, exceptions, timeouts and missing credentials.

🖼️ Screenshots or Demo

N/A

📝 Release Notes

  • Fixed: a sync could stay "in progress", or its latest run stay "queued", when a worker started the sync job before Phase had recorded the run. This affected all sync providers.

❓ Open Questions

N/A

🧪 Testing

  • New backend/tests/tasks/test_sync_events.py (11 tests, with an in-memory event store):
    • a job that starts before delay() returns updates its own new event;
    • a job updates its own event, not a newer one;
    • an event created while the job runs is still updated;
    • a job without an event still records the sync's status;
    • failures, timeouts and missing credentials are recorded on the event, as before;
    • the event fails when the job can't be queued.
    • 7 of the 11 fail on main. The other 4 cover existing behaviour.
  • Against a real worker, Redis and Postgres (self-hosted stack with GitLab CE 19.4), with the event insert slowed down to 3 s so the worker always starts first:
    • main: for a sync with earlier runs, the job overwrote the previous event and the new one stayed "queued". For a new sync, the job failed with AttributeError and the sync stayed "in progress".
    • This branch: in both cases, the new event ends "completed" with the sync's message, older events are untouched, and there are no errors.
    • New web code with an old worker, e.g. during a rolling deploy: also fixed, since the event exists before the job starts.
    • Old web code with a new worker: no errors, and the sync's status is right. The event is updated if it exists by the time the job ends (with the artificial 3 s delay, it didn't).
  • The backend suite passes, also merged with feat: sync secrets to GitLab CI environment scopes #1044, except test_file_read_permission_error, which fails whenever tests run as root.

🎯 Reviewer Focus

backend/api/tasks/syncing.py: trigger_sync_tasks, _get_sync_event and handle_sync_event.

➕ Additional Context

  • Found while testing feat: sync secrets to GitLab CI environment scopes #1044, where it's listed under pre-existing issues. Both PRs change backend/api/tasks/syncing.py, and they merge cleanly.
  • Not changed here: cancel_sync_tasks doesn't stop a job that is already running, so overlapping runs of a sync are still possible.

✨ How to Test the Changes Locally

  1. cd backend && pytest tests/tasks/test_sync_events.py
  2. To see the race: run Phase with a worker. In python manage.py shell, patch EnvironmentSyncEvent.objects.create to sleep 3 s before creating the event, then call trigger_sync_tasks for a sync. On main, the new event stays "queued", or for a new sync the job fails. On this branch, it completes.

💚 Did You...

  • Ensure linting passes (code style checks)?
  • Update dependencies and lockfiles (if required) N/A
  • Update migrations (if required) N/A
  • Regenerate graphql schema and types (if required) N/A
  • Verify the app builds locally? (backend image)
  • Manually test the changes on different browsers/devices? N/A, backend only

Generated by Claude Code

trigger_sync_tasks queued the job, then created its event. A worker could
start the job first. It then found no event and failed at the end, leaving
the sync "in progress", or updated an older event of the sync and left the
new one "queued".

Create the event first, and queue the job with the event's ID. The job
updates the event with its own ID, and still records the sync's result if
the event is missing.
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.

1 participant