cloudfoundry / cloudfoundry/cloud_controller_ng
Build/droplet loses its lifecycle_data when a shared row is deleted
Nobody has claimed this yet.
- Dominant language
- Ruby
- Stars
- 207
- Forks
- 373
- Avg merge
- 2d 12h
- Merged PRs (30d)
- 56
Description
Summary
buildpack_lifecycle_data and cnb_lifecycle_data rows can be shared between a build and a droplet: during staging a single row ends up with both build_guid and droplet_guid set, pointing at two live parents. Both parents declare an app-level :destroy dependency on that row.
This document describes the issue, inventories the current delete associations and DB foreign keys, and lays out two distinct ways forward.
The issue
How a row becomes shared
The buildpack/CNB staging path creates one lifecycle_data row and attaches it to both parents:
- A build is created; its lifecycle_data row is created owned by the build (
build_guidset). DropletCreate#find_or_create_buildpack_dropletcreates the droplet and attaches the build's same row to the droplet by settingdroplet_guidon it.
The result is one row with build_guid and droplet_guid both set, both referencing live parents.
Aside: requested vs. detected buildpacks
The build's lifecycle_data holds the requested buildpacks (what the user asked for at build time; empty = auto-detect). When staging finishes, StagingCompletionHandler#save_staging_result writes the detected buildpacks (what staging actually resolved) via droplet.buildpack_lifecycle_data.buildpacks = …. Because build and droplet currently share one row, that write overwrites the requested value - after staging, the (shared) row holds detected, not requested buildpacks. This is a subtle semantic difference, not treated as a bug today.
Why deletion leaves a dangling reference
Both parents remove the row on their own destruction, unconditionally:
BuildModel:add_association_dependencies buildpack_lifecycle_data: :destroy, cnb_lifecycle_data: :destroyDropletModel:add_association_dependencies buildpack_lifecycle_data: :destroy, cnb_lifecycle_data: :destroy
add_association_dependencies … :destroy is an app-level hook (Sequel), not a DB constraint. When the droplet is destroyed it destroys the shared row - even though the build still references it - so the build is left with a dangling reference (build.buildpack_lifecycle_data → nil). The same happens in reverse: destroying the build destroys the shared row and leaves the still-live droplet with a dangling reference.
All deletion paths for a shared row
A shared row (build_guid and droplet_guid both set, both live) can be removed by any of the following. A live parent is left with a dangling reference whenever the row is destroyed while that parent still exists - the parent survives but its lifecycle_data is gone.
| Trigger | Mechanism | Effect on the shared row | Dangling reference on a live parent? |
|---|---|---|---|
Delete the build (BuildDelete) |
DB ON DELETE CASCADE via build_guid FK and app-level :destroy on BuildModel |
Row destroyed (DB cascade fires regardless of the Ruby hook) | Yes - the still-live droplet dangles (droplet.buildpack_lifecycle_data → nil) |
Delete the droplet (DropletDelete) |
App-level :destroy on DropletModel |
Row destroyed | Yes - the still-live build dangles (build.buildpack_lifecycle_data → nil) |
Delete the app (AppDelete#delete_subresources) |
Deletes builds then droplets in order (BuildDelete, then DropletDelete) |
Row destroyed when the build is deleted (first) | No lasting dangle - the droplet is deleted immediately after; but the build-delete-dangles-droplet mechanism is exercised here mid-transaction |
| Delete the app's own lifecycle_data | App owns a separate app_guid-keyed row, not the shared one |
Shared row untouched | N/A - app-owned rows are single-parent and not involved in sharing |
The critical asymmetry: both the build-delete and the droplet-delete paths leave the other live parent dangling. The droplet side has no DB FK (relies on the Ruby :destroy hook); the build side has an ON DELETE CASCADE FK, so it destroys the row at the database level - which no app-level fix can intercept. Any solution that keeps rows shared must reconcile both directions.
Inventory: delete associations
add_association_dependencies is app-level (fires in Ruby on Model#destroy); it is not enforced by the database and is bypassed by raw dataset deletes (Model.where(...).delete).
| Parent model | Association | Target row | Dependency (app-level) |
|---|---|---|---|
BuildModel |
buildpack_lifecycle_data |
buildpack_lifecycle_data |
:destroy |
BuildModel |
cnb_lifecycle_data |
cnb_lifecycle_data |
:destroy |
DropletModel |
buildpack_lifecycle_data |
buildpack_lifecycle_data |
:destroy |
DropletModel |
cnb_lifecycle_data |
cnb_lifecycle_data |
:destroy |
*LifecycleDataModel |
buildpack_lifecycle_buildpacks |
child buildpack rows | :destroy |
Back-navigation (lifecycle_data.build / .droplet / .app) exists as many_to_one readers but is not used by production code to drive deletes.
The lifecycle_data readers on both parents already tolerate a missing row (buildpack_lifecycle_data || BuildpackLifecycleDataModel.new.freeze), with the comment "The lifecycle_data row can be destroyed independently of this" - i.e. the codebase already assumes the row's lifetime is decoupled from any single parent.
Inventory: DB foreign keys on the lifecycle_data tables
Owner columns are app_guid, build_guid, droplet_guid on both tables.
| Table | Column | SQL foreign key? | on_delete |
|---|---|---|---|
buildpack_lifecycle_data |
build_guid |
Yes - fk_buildpack_lifecycle_build_guid → builds.guid |
cascade |
buildpack_lifecycle_data |
droplet_guid |
No - index only | - |
buildpack_lifecycle_data |
app_guid |
No - index only | - |
cnb_lifecycle_data |
build_guid |
Yes - fk_cnb_lifecycle_build_guid → builds.guid |
cascade |
cnb_lifecycle_data |
droplet_guid |
No - index only | - |
cnb_lifecycle_data |
app_guid |
No - index only | - |
buildpack_lifecycle_buildpacks |
buildpack_lifecycle_data_guid |
Yes - fk_blbuildpack_bldata_guid → buildpack_lifecycle_data.guid |
cascade |
buildpack_lifecycle_buildpacks |
cnb_lifecycle_data_guid |
Yes - fk_blcnb_bldata_guid → cnb_lifecycle_data.guid |
cascade |
Key asymmetry: build_guid is DB-enforced with cascade on both tables, but droplet_guid is not - only an index. So at the DB level, deleting a build cascades to a build-owned row, but deleting a droplet relies entirely on the app-level :destroy hook.
Two ways forward
Both keep both delete orders safe (the hard requirement). They differ in whether they eliminate row-sharing or merely tolerate it, and in how much migration/backfill they require.
Option A - Stop sharing: droplet gets its own copy + cascade FK
Each parent owns its own single-parent row. The staging path copies the build's lifecycle_data onto a new droplet-owned row instead of attaching the build's row. A cascade FK on droplet_guid becomes the DB-level backstop.
Changes
- Forward fix (
app/actions/droplet_create.rb) -find_or_create_buildpack_dropletcopies stack, buildpacks (rebuilding the childbuildpack_lifecycle_buildpacksrows), and - for CNB - the encryptedregistry_credentials_json, onto a new droplet-owned row. Uses thecreate(..., droplet:)association-arg form to avoid a MySQL concurrent-insert deadlock (mirrorsDropletCopy). - Model
validate- reject any row with more than one owner column set ([app_guid, build_guid, droplet_guid].compact.length > 1). Single-parent becomes an invariant the model refuses to violate. - Migration - add
on_delete: :cascadeFK ondroplet_guidfor both tables, with MySQL collation alignment; delete rows whosedroplet_guidpoints at a now-deleted droplet (NULL-guarded so app/build-owned rows survive). - Backfill - for every pre-existing shared row (
build_guidanddroplet_guidboth set on a live pair), split it: create a droplet-owned copy and cleardroplet_guidon the build's row.
Order within the migration: backfill (split shared rows) → delete rows with a dangling droplet_guid → MySQL collation alignment → add the droplet_guid cascade FK. The FK must be added last, once every row is single-owned.
Pros
- Eliminates the shared-row design entirely; every row has exactly one owner.
- DB-enforced backstop: a raw dataset delete or future migration cannot silently reintroduce a dangling reference - the FK guarantees droplet-owned rows die with their droplet and nothing else.
- Side-effect: because the droplet now has its own row, the staging-completion write lands on the droplet's copy - build's row keeps requested buildpacks, droplet's row holds detected. No production reader depends on this either way; it's a semantic tidy-up, not a requirement.
Cons
- Largest change. Copy logic + validate + migration + a data backfill of existing shared rows, in a specific order.
- Backfill risk. Copying inside a migration bypasses the model layer; CNB's encrypted
registry_credentials_jsonmust be copied as raw ciphertext + salt (not re-encrypted). Backfill cost scales with the number of existing shared rows in production CCDBs. - Multi-step, order-dependent migration; must be idempotent.
Option B - Tolerate sharing: set null FKs + destroy only when no parent remains
Keep rows shareable. Instead of unconditionally destroying the row when a parent is deleted, let the database null out the departing parent's guid and only destroy the row once no owner is left. No copy, no backfill.
Changes
- FKs to
set null- change the existingbuild_guidFKs (both tables) fromon_delete: :cascadetoon_delete: :set null, and add adroplet_guidFK also withon_delete: :set null. Deleting either parent then clears only its own guid column at the DB level; the other owner and the row survive. - Remove the unconditional
:destroydependencies onBuildModel/DropletModelforbuildpack_lifecycle_data/cnb_lifecycle_data- theset nullFK now handles detaching this parent. No model hooks are added. - Reap ownerless rows with a background job. With the
:destroyhooks gone, a row whose last parent is deleted is left with all owner columns null and would leak. Add a periodic cleanup job (same pattern aspending_build_cleanup/orphaned_blobs_cleanup, registered in the clock scheduler) that deletes*_lifecycle_datarows whereapp_guid,build_guid, anddroplet_guidare all null. - Migration - flip the two
build_guidFKs toset null, add thedroplet_guidset nullFK (with MySQL collation alignment). No shared-row split needed -set nulltolerates the existing two-owner rows.
Pros
- No data backfill. Existing shared rows keep working;
set nullhandles them on delete. Migration only alters FK definitions. - No model hooks / no delete-path changes. Detaching is DB-enforced; the only new code is a standalone cleanup job, independent of the delete flows.
- Symmetric by construction - build-delete and droplet-delete both just clear their own guid at the DB level.
- Both delete orders safe.
Cons
- Ownerless rows exist transiently. Between a parent's deletion and the next cleanup-job run, a fully-detached row sits in the table. Harmless (no live parent references it) but not immediately reclaimed.
- Still a schema change (FK redefinition on three columns), though no data migration.
Side-effect: shared rows persist, so the staging-completion write still lands on the shared row - build.lifecycle_data.buildpacks continues to reflect detected rather than requested (same as today). Unlike Option A, the requested/detected separation is not gained. No production reader depends on it.
Comparison
| Dimension | Option A (copy + cascade FK) | Option B (set null FKs) |
|---|---|---|
| Eliminates row sharing | Yes | No (tolerates it) |
| Schema change / migration | Yes | Yes (FK redefinition only) |
| Data backfill of existing rows | Required | Not needed |
| Detach-on-parent-delete enforced by | DB cascade FK | DB set null FK |
| Ownerless-row cleanup | N/A (single owner) | Background cleanup job |
| Fixes existing shared rows | Via backfill | Via set null on delete |
| Change size / steps | Large, multi-step | Moderate |
| Requested vs. detected buildpacks separated | Yes | No |
| Model hooks / delete-path changes | Forward-copy in staging | None (job is standalone) |
Recommendation
- If DB-enforced correctness and eliminating the shared-row design are the priority, Option A - accept the migration + backfill cost.
- If avoiding the data backfill is the priority and keeping shared rows is acceptable, Option B - the
set nullFKs make both delete directions safe at the DB level with no delete-path or model-hook changes, at the cost of a periodic background job that reaps ownerless rows.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by comparing the two proposed approaches, then inspect app/actions/droplet_create.rb, BuildModel, DropletModel, and the lifecycle-data foreign-key migrations. If Option B is chosen, also read the pending_build_cleanup or orphaned_blobs_cleanup pattern and the clock scheduler. Done means a decided, implemented approach that keeps both build-delete and droplet-delete paths safe.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- mysql, ruby
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100