Skip to content

Bounty.issue uses onDelete: CASCADE unlike every other financial relation in the schema — deleting the linked Issue hard-deletes an in-flight, possibly-funded Bounty #53

Description

@chonilius

Overview

Every financially-relevant foreign key in this schema was deliberately hardened against cascading deletes — except one. Bounty.issue still uses onDelete: 'CASCADE':

// src/common/entities/bounty.entity.ts:22-27
@OneToOne(() => Issue, (issue) => issue.bounty, { onDelete: 'CASCADE' })
@JoinColumn()
issue: Issue;

@Column()
issueId: string;

Compare this to literally every other relation on Bounty and its neighbors, each carrying an explicit comment about why cascading deletes are dangerous for financial records:

// src/common/entities/bounty.entity.ts:70-77
// cascade excludes 'remove'/'soft-remove': removing a Bounty entity via the
// ORM must not also remove its Escrow — the escrow row (and the funds it
// represents) must outlive the bounty record. See #27.
@OneToOne(() => Escrow, (escrow) => escrow.bounty, { nullable: true, cascade: ['insert', 'update'] })
escrow: Escrow | null;
// src/common/entities/payment.entity.ts:20-27
// RESTRICT, not CASCADE: a Payment is a record of money that actually
// moved. Deleting its parent Escrow must never silently delete that
// payout record too — the database refuses the delete instead. See #27.
@ManyToOne(() => Escrow, (escrow) => escrow.payments, { onDelete: 'RESTRICT' })
// src/common/entities/escrow.entity.ts:25-31
// The parent link (bounty/milestone/maintenancePool) is `onDelete: 'SET
// NULL'`, not CASCADE: deleting a bounty/milestone must never delete the
// escrow row (and, transitively, its payments) out from under real,
// possibly still-LOCKED, funds.

The migration that hardened all of this (1784272650000-EscrowFkIntegrityAndSponsorId.ts, landed to fix the now-closed sponsor-dashboard FK-integrity issue) re-pointed escrows.bountyId/milestoneId/maintenancePoolId from CASCADE to SET NULL, and payments.escrowId from CASCADE to RESTRICT — establishing a clear, documented principle for this codebase: financial-state-bearing rows must survive the deletion of whatever they're attached to. Bounty.issue's CASCADE is the one relation that migration didn't touch, and it's the most consequential one to have missed: Bounty is where status, claimedById, amount, teamId, escrowId, prUrl, claimedAt, mergedAt, and paidAt all live. Deleting the Issue row a Bounty points to — via Issue.repository's own onDelete: 'CASCADE' (issue.entity.ts:22-24, so deleting a Repository cascades to Issue, which cascades to Bounty two levels deep), or a direct delete of the Issue row itself (an admin cleanup script, a future dedup/merge tool, a bug in code nobody's written yet) — doesn't leave an orphaned-but-intact Bounty the way the hardened Escrow/Payment relations do. It deletes the entire Bounty row outright, along with every field on it, for a bounty that could be sitting in FUNDED, CLAIMED, IN_REVIEW, or MERGED with real, actively-tracked state and (per Escrow.bounty's own SET NULL behavior) a LOCKED escrow that survives the cascade with its bountyId nulled out — a locked pile of money with no bounty record left anywhere to explain what it was for, who claimed it, or who it's owed to.

Requirements

  • Change Bounty.issue's relation from onDelete: 'CASCADE' to onDelete: 'RESTRICT' (matching the Payment → Escrow pattern: a Bounty is itself a financial record once funded, and its parent Issue being deleted should refuse, not cascade) — or, if SET NULL is preferred to allow the Issue row to be cleaned up while preserving the Bounty, make Bounty.issueId nullable and update every place that currently assumes it's non-null.
  • Write a migration for this, following the exact pattern established by 1784272650000-EscrowFkIntegrityAndSponsorId.ts (find the existing FK by introspecting pg_constraint rather than hardcoding TypeORM's auto-generated constraint name, since that migration's own replaceForeignKeyOnDelete helper is directly reusable here).
  • Audit Milestone.repository and MaintenancePool.repository (onDelete: 'CASCADE' from Repository, milestone.entity.ts:23-25, maintenance-pool.entity.ts:24) for the same class of risk — deleting a Repository row currently cascades to delete Milestone/MaintenancePool rows that may have FUNDED/ACTIVE escrows attached, for the identical reason Bounty.issue's cascade is dangerous. Decide and apply a consistent policy across all three.
  • Add a test (can be a repository-level integration test, following the pattern in escrow-fk-integrity.integration.spec.ts) that funds a bounty, deletes its underlying Issue row directly, and asserts the Bounty row survives (or, if SET NULL is chosen, that Bounty.issueId becomes null rather than the whole row disappearing).

Acceptance Criteria

  • Deleting the Issue a funded/claimed/merged Bounty points to no longer deletes the Bounty row.
  • A migration implements the FK change, following the existing replaceForeignKeyOnDelete pattern for finding and replacing the live constraint safely.
  • Milestone.repository/MaintenancePool.repository's cascade behavior is explicitly reviewed and either fixed or deliberately justified in writing.
  • An integration test (real Postgres, not mocked repos) proves a Bounty with real state survives its parent Issue's deletion.

Additional Notes

Precise references: src/common/entities/bounty.entity.ts:22-27 (the bug), :70-77 (the Escrow relation on the same entity, showing the correct, already-adopted pattern right next to it), src/common/entities/payment.entity.ts:20-27 and src/common/entities/escrow.entity.ts:25-39 (the documented design principle this relation violates), src/common/entities/issue.entity.ts:22-24 (Issue.repository, CASCADE — the second-order path: deleting a Repository cascades to Issue cascades to Bounty), src/common/entities/milestone.entity.ts:23-25 and src/common/entities/maintenance-pool.entity.ts:24 (the same Repository → X CASCADE pattern on two more financial entities), src/database/migrations/1784272650000-EscrowFkIntegrityAndSponsorId.ts (the precedent migration and its reusable replaceForeignKeyOnDelete helper).

Honesty about current reachability: nothing in the application's own service layer currently issues a DELETE against Issue or Repository rows (confirmed by grepping for .remove(/.delete( across src/ — the only hits are the idempotency-key cleanup job, unrelated). This is the same caveat the original, now-closed FK-integrity issue carried: the risk is latent, reachable via direct DB access, a future admin endpoint, a data-cleanup script, or ORM-level .remove() calls nobody's written yet — not something today's webhook/sync code paths trigger on their own. That doesn't make the FK definition safe; it makes it a landmine for whoever builds the next piece of tooling that touches these tables directly, exactly as the sponsor-dashboard issue was before it was fixed.

Test/reproduction plan:

// integration test against real Postgres, following escrow-fk-integrity.integration.spec.ts's pattern
const repo = await repositoryRepo.save({ owner: 'x', name: 'y', ... });
const issue = await issueRepo.save({ repositoryId: repo.id, ... });
const bounty = await bountyRepo.save({ issueId: issue.id, sponsorId, amount: '100.0000000', status: BountyStatus.FUNDED, ... });
await issueRepo.delete(issue.id);
const survived = await bountyRepo.findOne({ where: { id: bounty.id } });
// pre-fix: survived === null (CASCADE deleted it)
// post-fix: survived !== null (RESTRICT threw, or SET NULL left the row with issueId: null)

Cross-references: directly extends the now-closed "Sponsor dashboard aggregate figures... missing FK integrity constraints" issue's own principle to a relation that migration didn't cover. Also relevant to the companion "TeamMemberSplit.user uses onDelete: CASCADE" issue — same root pattern (a cascade left over from before this codebase's FK-hardening pass caught up to it) on a different table.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26Stellar WaveIssues in the Stellar wave programThird CampaignCampaign: Third CampaignarchitectureArchitecture/design issuebugSomething isn't workingvery hardVery difficult task, expert-level effort required

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions