Repository navigation
fix(assignment-delete): atomic GitHub cleanup, FK indexes, lock down RPCs - #1086
Conversation
…RPCs On 2026-10-15 deleting CS 3650 assignment 1259 (238 repos) queued 239 archive_repo_and_lock jobs before calling delete_assignment_with_all_data, which then hit the 8s statement timeout and rolled back. The archive jobs kept running against an assignment that stayed live, and a later "sync to handout" queued 238 syncs against repos being archived. - Index six unindexed FK columns. Each deleted repository seq-scanned workflow_events (1.19M rows) for its FK check: 81s in production. Production-shaped test: 27.8s -> 0.22s. - delete_assignment_with_all_data(..., p_enqueue_github_archives) queues the archive jobs inside its own transaction, from the rows it actually deletes, and cancels queued sync_repo_to_handout jobs for them. It returns the GitHub targets it removed. - assignment-delete deletes the DB first. Synchronous GitHub deletes run after the commit and fall back to an archive job on failure. - github-async-worker drops sync_repo_to_handout when the repository row no longer exists instead of syncing with no baseline. - Revoke EXECUTE from PUBLIC/anon/authenticated on delete_assignment_with_all_data and enqueue_github_archive_repo. Both are SECURITY DEFINER with no caller check and were callable with the anon key (hotfixed in production 2026-10-15). - One-off script used to finish the 1259 cleanup. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review: 1
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 056256c813
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Failure
…t race claimed syncs Review feedback on #1086: - The RPC's target filter rejected names the TypeScript parser accepts ("Org / repo" -> Org/repo), so a template/solution that passed the edge function's validation could be silently left out of cleanup. Parse in SQL exactly like parseGitHubRepoFullName: trim, split on '/', exactly two trimmed non-empty parts. - Deleting the queue row of a sync_repo_to_handout job a worker has already claimed does not stop that worker. Lock the assignment's sync rows FOR UPDATE (pgmq.read claims with SKIP LOCKED, so nothing can be claimed mid-delete) and refuse with 55006 / HTTP 409 if any is already claimed (read_ct > 0, vt in the future). Delayed requeues (read_ct 0) are still cancelled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ea013b6cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ue archives; safe legacy mode Second round of review feedback on #1086: - Sync producers: queue_repository_syncs locks repository rows, not queue rows, so it could enqueue after the delete's queue scan. The RPC now locks the assignment's repository rows (FOR UPDATE, id order, as the producer does) before scanning, so a concurrent sync press waits and then finds the rows gone. Also scans async_calls_low_priority. - Stale strategy: the RPC queues an archive job for EVERY target whenever the caller passes true, and the edge function picks immediate-delete vs archive-only from the targets the RPC actually deleted. Immediate deletes are now only a fast path: a failure, timeout or crash part-way still leaves every repository queued (archiveRepoAndLock treats a 404 as done). The edge function always passes true. - Rollout: a two-argument call is an old assignment-delete that has already committed its own archives. Refusing it would recreate the incident, so false skips the claimed-sync refusal and queues nothing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcbef0d0d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k; strict name parsing Third round of review feedback on #1086: - A repository created after the edge function's only-template-commits check was swept into DELETE ... RETURNING and archived, or deleted outright on the synchronous path, without ever being checked. The edge function now passes the repository ids it validated (p_expected_repository_ids), and the RPC refuses with 40001 (HTTP 409, retry re-checks) if the locked set differs. - The Phase 1 repository read ignored its error and was capped at PostgREST's 1000 rows; it is now paged and a failure stops the delete. - JS trim() and SQL btrim() disagree on tabs and Unicode whitespace. Both sides now trim ASCII spaces only and require GitHub's name characters ([A-Za-z0-9._-]), so unusual values are rejected identically. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0577324f47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… running Fourth round of review feedback on #1086: "claimed" was read_ct > 0 and vt in the future, but a handler can outlive its visibility timeout (a batch of 4 measured ~420s against the 300s chart default), and then its message looks unclaimed while the worker is still mutating GitHub. A queued sync now counts as running if it is claimed OR its repository carries the handler's own in_progress marker naming that msg_id. The marker lasts exactly as long as the handler; markers whose message has left the queue are stale and block nothing, so a crashed worker cannot pin an assignment. The pre-marker window is already closed by the repository row lock (the marker write blocks, then matches no row). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8069d6be01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…r org drain lease Fifth round of review feedback on #1086: the in_progress marker is one value per repository, so with two overlapping handlers (H2/H3) the newer can overwrite and clear it while the older, past its VT, still runs. A sync message that has been read at least once now also counts as running while a live async_worker_slots lease exists for the org the slot allocator charges it to (owner of args.repository_full_name, lowercased, same queue). A leaseholder renews its slot while it works and releases it only after every in-flight message finishes, and the delete holds the message rows locked so no new handler can start on them. With org slots disabled (chart default orgSlotGlobalCap 0) there is no lease to consult; production runs with them enabled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
#1087 also uses version 20261015120000, which migrate.sh rejects as a duplicate. Its migration revokes grants on the (bigint, bigint) signature this one drops, so this one has to sort after it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad4d0a649f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHERE q.message->>'method' = 'sync_repo_to_handout' | ||
| AND q.message->'args'->>'repository_id' = ANY (v_repository_ids::text[]) |
There was a problem hiding this comment.
When an assignment still has a queued or running create_repo message, this scan ignores it because it selects only sync_repo_to_handout. The create_repo handler performs the slow GitHub creation and permission sync before updating the repository row, so the delete can remove that row and enqueue an archive concurrently; the archive may observe a 404 and finish while creation later succeeds, or a running handler may retry after the archive renamed the first repository, leaving a new active repository with student access after the assignment is gone. Lock/refuse running creation jobs and cancel queued ones, or make the worker verify that its repository and assignment still exist before its first GitHub mutation.
Useful? React with 👍 / 👎.
|
🚀 Preview deployed for
Sample login emails
Shared password lives in OpenBao at The preview tears down automatically when this PR closes. |
This branch was successfully deployed
1 participant
Incident (2026-10-15)
Deleting CS 3650 fa26 "Project 1: Shell" (assignment 1259, 238 repos) queued 239
archive_repo_and_lockjobs before callingdelete_assignment_with_all_data. The RPC then hit authenticator's 8sstatement_timeoutand rolled back. The archive jobs need only org/repo, so they kept archiving, renaming and removing collaborators from the repos of an assignment that stayed live. An hour later a "sync to handout" queued 238sync_repo_to_handoutjobs against those same repos. Finishing the delete by hand (scripts/oneoff/…) took 81s for an assignment with 0 submissions.Changes
1. Root cause of the timeout: unindexed FK columns (
20261016120000_assignment_delete_atomic_archive.sql)Every deleted row runs a check for each FK that references it, and without an index that check is a sequential scan. The worst offenders:
workflow_events.repository_idsubmissions.repository_check_run_idrepository_check_runs.target_submission_id(partial)submission_files.assignment_group_id(partial)submission_file_symbol_index.assignment_group_id(partial)grader_result_output.assignment_group_id(partial)At 238 repos × one
workflow_eventsscan each, that adds up to the 81s. Group assignments would be far worse, since every group deleted scanssubmission_filesonce. The partial indexes (WHERE … IS NOT NULL) cover columns that are 73–99% NULL, and an FK check'scol = $1can still use them.2. GitHub cleanup commits with the delete
delete_assignment_with_all_data(p_assignment_id, p_class_id, p_enqueue_github_archives default false, p_expected_repository_ids default null). Withtrue, it queues an archive job for every repo it deletes, inside its own transaction. A failed delete queues nothing.DELETE … RETURNINGonrepositories, plus the template and grader repos, so a repo created between the edge function's safety checks and the RPC still gets archived. But it only cleans up repos that passed those checks. The edge function passes the repo IDs it validated (p_expected_repository_ids), and once the rows are locked the RPC refuses with40001(HTTP 409, and a retry re-runs the checks) if the set differs. So a repo nobody checked for student work is never archived or deleted./, and each part must match^[A-Za-z0-9._-]+$.Org / repobecomesOrg/repo, and a tab, a non-breaking space or an internal space is rejected in both places.FOR UPDATE, inidorder, the same lockqueue_repository_syncstakes). A concurrent sync press waits until the delete commits, then finds the rows gone.sync_repo_to_handoutjobs for those repos onasync_callsandasync_calls_low_priority. If one is already running, it can't be stopped, so the delete refuses with55006. Running means claimed (read_ct > 0, still invisible), or carrying the handler's ownin_progressmarker on the repository row with thatmsg_id. The marker covers a handler that outlives its visibility timeout. It counts only while its message is still queued, so a crashed worker can't pin the assignment. A message counts as running too if it has been read and its org's drain lease (async_worker_slots) is live. That covers two handlers overlapping on the same repo, where the newer one can overwrite the older one's marker. This last check applies only with org slots enabled, which production has; see Follow-ups. The delete returns HTTP 409 ("try again in a few minutes"). Locking the queue rowsFOR UPDATE, withpgmq.readusingSKIP LOCKED, means nothing can be claimed while the delete runs.github_repositories,github_archives_queuedandhandout_syncs_cancelled.(bigint, bigint)signature is dropped. A two-argument call resolves tofalse. That caller is a pre-PR edge function that has already queued its own archives, sofalseskips the claimed-sync refusal and queues nothing. Refusing it would recreate the incident. That is the pre-PR behaviour, minus the timeout.3.
assignment-deleteedge functionp_enqueue_github_archives: truealong with the repo IDs it checked.archiveRepoAndLocktreats a 404 as done).4.
github-async-workersync_repo_to_handoutnow drops a job whose repository row no longer exists. A missing row used to read as "no baseline" and fall through to a full sync.5. Lock down both RPCs
delete_assignment_with_all_dataandenqueue_github_archive_repoareSECURITY DEFINERwith no caller check, and both were executable byanonandauthenticated(PUBLIC default).supabase_admin, since a REVOKE run aspostgreswas a silent no-op.supabase_admin, so the revoke takes effect.6.
scripts/oneoff/2026-10-15-delete-assignment-1259.sh, the script used to finish the incident cleanup, kept for the record.Testing
Migration applied with
psql --single-transactionassupabase_admintosupabase/postgres:17.4.1.075(the prod image), loaded with a schema-only dump of prodpublic. The test assignment had 241 repos, including one malformed name and one case-only duplicate.workflow_eventsheld 1.2M rows, none of them on the test assignment.statement_timeoutleft 0 archive jobs queued and the assignment intact.buildAssignmentDeleteArchiveDebugId, and each job has itsapi_gateway_callsrow.pgmq.readduring an open delete claimed none of the locked syncs." Org / spaced-repo "and" Org /handout-doomed "normalize and get queued. Names containing a tab, a non-breaking space or an internal space are dropped.anonandauthenticatedgetpermission deniedon both functions.assignment-delete-repository-cleanuppasses, 14 tests (5 new).deno checkshows the same 17 errors as main, all ingithub-async-workerand_shared, none inassignment-delete.The worker change has no unit test.
processEnvelopehas no harness intests/unit.Deploy notes
Merge after #1087. This migration is numbered
20261016120000so that it runs after #1087's20261015120000_lock_down_security_definer_functions.sql.migrate.shrejects duplicate versions, and #1087 revokes grants on the(bigint, bigint)signature this migration drops. I applied both, in that order, to the prod schema: both functions end up executable only byservice_roleandpostgres, and #1087'sCheckFunctionGrantsreports 0 errors and 0 warnings on the combined tree.CONCURRENTLYbefore deploying to prod. The migration's plainCREATE INDEXblocks writes for as long as the build runs, which matters most onsubmission_files(submission path, 5.5 GB). TheCONCURRENTLYstatements are in the migration header. With the indexes already built, the migration'sIF NOT EXISTSmakes it a no-op.Follow-ups (not in this PR)
orgSlotGlobalCap: 0, the chart default), an older handler that outlives its VT while a newer one for the same repo has already cleared the marker can't be detected. Closing that needs a per-handler lease in the worker. The repos involved have passed the only-template-commits check, so the worst case is a sync writing into a repo that's being archived.class_metrics_workflow_events_trgis row-level, about 22µs per deletedworkflow_eventsrow. That's roughly 4s for an assignment with 200k events, so it's the next thing that would push a large delete toward the timeout.archive_repo_and_lockremoves collaborators one at a time (~34 per repo at ~3s each), so a 238-repo delete takes about 2.5 hours to drain the queue.🤖 Generated with Claude Code