ci: reindent CD workflows + asset upload orphan-blob fixes#702
Merged
Conversation
Block sequences under steps: were at the same column as their parent key. Technically valid YAML, but unconventional and confused review tooling into reporting the workflows as broken. Reindent both CD_staging.yml and CD_production.yml so step list items are nested one level deeper than steps:, matching the rest of the repo's workflow style. Whitespace-only change: `git diff -w` is a no-op on the deploy logic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Standardizes indentation in the CD GitHub Actions workflows to a conventional 2-space YAML style, addressing review/tooling confusion while keeping deploy logic unchanged.
Changes:
- Reindented
CD_staging.ymljob/steps blocks to conventional YAML indentation. - Reindented
CD_production.ymljob/steps blocks to conventional YAML indentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/CD_staging.yml | Whitespace-only reindent of the staging CD workflow for conventional YAML formatting. |
| .github/workflows/CD_production.yml | Whitespace-only reindent of the production CD workflow for conventional YAML formatting. |
Same indentation reflow as CD_staging.yml and CD_production.yml: job at col 2, steps' list items at col 6. Also joins the "Tag commit" step's git tag command, which was split across three lines inside the -m "..." quoted message: -m "testing gcloud deployment: $ (date -u +%Y-%m-%d)T..." Embedded raw newlines would land in the tag message. Joined onto one line to match CD_staging.yml. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
If session.commit() (or .add/.refresh) raises after gcs_upload has already written the file, the bucket would retain an orphaned object with no Asset row pointing at it. Wrap the DB write in try/except: rollback the session and best-effort delete the blob, then re-raise. A failure to delete the blob is logged but does not mask the original exception. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Asset still references it gcs_upload deduplicates by content hash: when an object with the hash-named blob already exists, it skips the upload and returns the existing blob_name. The previous cleanup-on-failure path would delete that shared object, breaking Assets owned by other Things that point at the same hash-named blob. - gcs_upload now also returns a `created` flag indicating whether this call actually wrote the blob. All callers updated to unpack three values. - In upload_and_record_asset, on DB-write failure: only delete the blob when (a) this request created it AND (b) no Asset row references storage_path == blob_name after rollback. If another Asset still references it, log and leave it alone. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
session.refresh(asset) runs AFTER session.commit() has succeeded. Keeping it inside the cleanup except block meant a transient read failure during refresh would trigger blob deletion while the Asset row was already committed, leaving the DB pointing at a missing GCS object. Move refresh outside the try so cleanup is scoped to pre-commit failures only. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Wrap session.rollback() in its own try/except. A rollback failure is logged but no longer masks the original DB exception, which is re-raised at the end of the handler. - Wrap the reference-count query in its own try/except. If the SELECT itself fails after rollback, we cannot confirm the blob is unreferenced, so default to keeping it (sentinel object → not None). Better to leave a possible orphan blob than delete one another Asset still points at. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Wrap the cleanup block in upload_and_record_asset in one outer try/except so any failure inside cleanup (rollback, reference query, bucket.blob(), delete) is logged but cannot mask the original commit exception that the request needs to surface. Add three direct-call tests in tests/test_asset.py that mock the session and bucket: - test_upload_and_record_cleans_blob_when_created_and_db_fails: asserts rollback runs, blob.delete is called, and the original commit exception is re-raised. - test_upload_and_record_skips_blob_delete_when_blob_was_preexisting: with gcs_upload reporting created=False (hash dedup hit), the cleanup must NOT delete the (possibly shared) blob. - test_upload_and_record_reraises_original_exception_when_rollback_fails: rollback raising must not replace the original commit exception. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jirhiker
changed the title
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.
Why
Two related groups of fixes, combined here (asset changes migrated from #701, which is now closed):
CD workflow indentation
steps:blocks inCD_staging.yml,CD_production.yml,CD_testing.ymlhad list items at the same column assteps:. Valid YAML but unconventional; confused review tooling.Asset upload orphan-blob safety (
POST /asset/upload-and-record)thing_idand before the DB write; a failure left an orphaned blob with no Asset row.How
Workflows
git diff -w≈ no-op on deploy logic).git tag -m "..."in CD_testing.yml.Asset upload
gcs_uploadnow returns(uri, blob_name, created);created=Falseon hash-dedup hit. All callers updated.session.refresh()moved outside the cleanup block (post-commit; must not trigger deletion).tests/test_asset.pycover the failure paths.Notes