Add per-source ETL write-ownership guard (pull vs push)#3565
Open
blarghmatey wants to merge 2 commits into
Open
Add per-source ETL write-ownership guard (pull vs push)#3565blarghmatey wants to merge 2 commits into
blarghmatey wants to merge 2 commits into
Conversation
Pull-ETL batch loaders (load_courses, load_programs, canvas stale sweep) full-sync prune/unpublish anything not present in the current batch. If a push pipeline (webhook or Dagster asset) also writes the same (etl_source, resource_type), each side treats the other's writes as missing, causing unpublish/republish flapping. There was no source-of-truth flag. Add ETLSourceOwnership (etl_source, resource_type, mode: pull|push), defaulting to pull so existing sources are unaffected. Wire pull_write_allowed into load_courses/load_programs (skip write+prune entirely for a push-owned pair) and the canvas stale-course sweep (skip prune only). Add assert_push_allowed for the upcoming generic learning_resources webhook loader (PR #3557) to call before writing. This is the prerequisite for cutting any source (SEE/Sloan first, already dual-modeled as a Dagster asset) from pull to push; no source is cut over by this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a per-(etl_source, resource_type) write-ownership guard so pull-based full-sync ETL loaders don’t prune/unpublish (or hard-delete) resources that are now owned by a push pipeline (e.g., webhook/Dagster), preventing “flapping” when both pipelines touch the same pair.
Changes:
- Introduces
ETLSourceOwnershipmodel + admin registration to configure ownership aspullvspushper(etl_source, resource_type). - Adds
learning_resources/etl/ownership.pyhelper API (pull_write_allowed,is_push_owned, and strictassert_*_allowedguards). - Updates course/program loaders and the Canvas stale-course sweep to skip writes/prunes when the pair is push-owned, with targeted test coverage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| learning_resources/tasks.py | Skips Canvas stale-course prune when canvas/course is push-owned. |
| learning_resources/tasks_test.py | Adds regression test ensuring stale Canvas courses aren’t pruned when push-owned. |
| learning_resources/models.py | Adds ETLSourceOwnership model to declare per-pair ownership mode. |
| learning_resources/migrations/0116_etlsourceownership.py | Creates the ETLSourceOwnership table and uniqueness constraint. |
| learning_resources/factories.py | Adds ETLSourceOwnershipFactory for tests. |
| learning_resources/etl/ownership.py | Implements ownership lookup and enforcement helpers for pull vs push writers. |
| learning_resources/etl/ownership_test.py | Tests default-pull behavior and both assertion guards. |
| learning_resources/etl/loaders.py | Makes load_courses / load_programs no-op (no write/prune) when push-owned. |
| learning_resources/etl/loaders_test.py | Adds tests verifying loaders do not write/prune when push-owned. |
| learning_resources/admin.py | Registers ETLSourceOwnership in Django admin for runtime configuration. |
The ownership check in sync_canvas_courses only guarded the stale- course prune block, not the S3 archive ingestion loop above it, so a push-owned canvas source would still receive pull-side writes - defeating the point of the guard. Move the pull_write_allowed check to the top of the function, matching load_courses/load_programs: skip the entire sync (ingestion + prune) when push-owned. Flagged by Sentry Seer's automated review on PR #3565. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 2, 2026
Closed
Closed
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.
What are the relevant tickets?
N/A (internal SOA remediation backlog item; no GitHub issue)
Description (What does it do?)
Batch pull-ETL loaders implement full-sync unpublish:
load_coursesprunes anything not in the current batch topublished=False(learning_resources/etl/loaders.py), same forload_programs, and the canvas sync task hard-deletes stale courses (learning_resources/tasks.py). If a pull-ETL (Celery) pipeline and a push pipeline (webhook/Dagster asset) both write the same(etl_source, resource_type), each full sync treats the other's writes as missing, causing unpublish/republish flapping. There was no source-of-truth flag to prevent this.This PR adds a per-
(etl_source, resource_type)write-ownership guard:ETLSourceOwnershipmodel (etl_source,resource_type,mode:pull|push). A missing row defaults topull, so no existing source's behavior changes.learning_resources/etl/ownership.pywithpull_write_allowed/is_push_owned/assert_pull_allowed/assert_push_allowedhelpers.load_coursesandload_programsnow short-circuit (no writes, no prune) for a push-owned pair.sync_canvas_coursesskips its prune block for a push-owned pair.assert_push_allowedis added as the guard primitive for the upcoming genericlearning_resourceswebhook loader (Add generic /api/v1/webhooks/learning_resources/ endpoint (Cohort 2 webhook delivery) #3557) to call before writing, so a push pipeline can't write into a pair pull-ETL still owns.ETLSourceOwnershipin Django admin so ownership can be flipped per source without a deploy.This is a prerequisite for migrating any source from pull to push — it does not cut over any source. SEE/Sloan is the first migration candidate (already dual-modeled as a Dagster asset) but no
ETLSourceOwnershiprow is seeded here.How can this be tested?
learning_resources/etl/ownership_test.py(default-pull behavior, push override is scoped to the exact(etl_source, resource_type)pair,assert_pull_allowed/assert_push_allowedraise correctly).test_load_courses_skips_write_when_push_owned/test_load_programs_skips_write_when_push_ownedinlearning_resources/etl/loaders_test.py— assert no DB writes and no prune occur, and existing published resources are untouched, when a pair is push-owned.test_sync_canvas_courses_skips_stale_prune_when_push_ownedinlearning_resources/tasks_test.py.learning_resources/etl/loaders_test.py,learning_resources/tasks_test.py, andlearning_resources/etl/pipelines_test.pysuites locally against Postgres — 406 passed, no regressions.0116_etlsourceownership.py) matches Django's expected model state viamanage.py makemigrations --check --dry-run(no diff detected).ruff check/ruff format --checkclean on all new/changed files (pre-existing missing-docstring findings on unrelated pre-existing code left as-is, matching repo convention).Reviewer validation: create an
ETLSourceOwnership(etl_source="see", resource_type="course", mode="push")row via/admin/, then runpython manage.py backpopulate_sloan_data(or triggerget_sloan_data) — it should log a skip and make no writes/prunes to SEE courses.Additional Context
Internal tracking:
tk-mit-learn-per-source-ownership-guard-before-any--b7efd0in the SOA simplification & boundary-hardening remediation backlog. Evidence audit:pf-soa-mit-learn-etl-data-ownership-audit-2026-07-d-8e9b00.