Skip to content

Fix #37: release DO firewalls on team destroy + sweep command for historical orphans#38

Merged
obaid merged 1 commit into
mainfrom
fix/issue-37-firewall-leak
May 22, 2026
Merged

Fix #37: release DO firewalls on team destroy + sweep command for historical orphans#38
obaid merged 1 commit into
mainfrom
fix/issue-37-firewall-leak

Conversation

@obaid
Copy link
Copy Markdown
Contributor

@obaid obaid commented May 22, 2026

Summary

ProvisionDigitalOceanServerJob creates a per-droplet firewall (createFirewall(\"provision-{server.id}\", ...)) but none of our destroy paths released it. Every destroyed team left a firewall behind in DO. The dev account had 36 orphans when this was discovered — droplets and volumes were both 0.

Firewalls don't bill, but DO enforces a per-account cap (default 100) and the Networking → Firewalls UI becomes useless past a few dozen entries.

Fixes #37.

Changes

  • Migration 2026_05_22_173614_add_provider_firewall_id_to_servers_table.php: adds nullable servers.provider_firewall_id.
  • DigitalOceanService::deleteFirewall (new): DELETE /v2/firewalls/{id}, treats 404 as already-gone.
  • ProvisionDigitalOceanServerJob.php:66-72: persists the firewall ID returned by createFirewall onto the server row.
  • DestroyTeamJob::destroyDigitalOcean: calls deleteFirewall after the droplet+volume cleanup; failure is logged but non-fatal (the cleanup command can sweep survivors).
  • Server model: provider_firewall_id added to $fillable.
  • cloud:cleanup-orphan-firewalls command (new): one-shot sweep for historical orphans. Filters by --prefix=provision-,warm- so it cannot accidentally delete unrelated firewalls. Supports --dry-run.

Tests

  • ProvisionDigitalOceanServerJobTest: new — persists the firewall ID on the server so destroy can release it. Asserts provider_firewall_id is populated from the mocked createFirewall response.
  • DestroyTeamTest: new — releases the DO firewall when destroying a team that has one. Asserts deleteFirewall('fw-leak-37') is called.
  • Existing destroys cloud server and volume test still passes (server with null firewall ID takes the no-op path; backwards compatible).

Full suite:

php artisan test --compact tests/Feature tests/Unit
Tests:    106 skipped, 681 passed (2219 assertions)
Duration: 22.59s

Pint: clean.

Live verification (already done)

The dev account had 36 orphans (provision-* × 34, warm-* × 2). All were deleted via direct API calls before this PR was opened. DO is now at:

droplets:  0
volumes:   0
firewalls: 0

After this PR ships, no new orphans should accrue from the destroy path. For other accounts (production, other developers) with historical orphans, run:

php artisan cloud:cleanup-orphan-firewalls --dry-run    # review what would be deleted
php artisan cloud:cleanup-orphan-firewalls              # actually delete

Out of scope

ProvisionLinodeServerJob.php:72 has the same bug pattern — createFirewall is called, but destroyLinode only releases the droplet and volume. Worth a follow-up issue if Linode is in active use; tagging here for visibility but not fixing in this PR (different API, different mock structure, would muddy the diff).

Test plan

  • CI green
  • Live: create + destroy a DO team, confirm zero firewalls remain (curl … /v2/firewalls | jq '.firewalls | length' → 0)
  • Live: php artisan cloud:cleanup-orphan-firewalls --dry-run reports 0 in a clean account

ProvisionDigitalOceanServerJob creates a per-droplet firewall but
none of our destroy paths released it, leaking one firewall per
provisioned team. Confirmed 36 orphans accumulated in the dev
DigitalOcean account.

Changes:
- New migration: servers.provider_firewall_id (nullable string).
- DigitalOceanService::deleteFirewall — 404 = already gone.
- ProvisionDigitalOceanServerJob persists firewall ID on the server
  row after createFirewall.
- DestroyTeamJob::destroyDigitalOcean calls deleteFirewall after the
  droplet/volume cleanup; failures are logged but non-fatal.
- New artisan command `cloud:cleanup-orphan-firewalls --dry-run` to
  sweep historical orphans. Filters by name prefix (provision-,
  warm-) so it can't accidentally delete unrelated firewalls.

Tests:
- Provision job test: provider_firewall_id persisted from createFirewall
  response.
- DestroyTeamJob test: deleteFirewall called when server has firewall ID.
- Existing DO destroy test still passes (server with null firewall ID
  takes the no-op path).

Out of scope: Linode has the same bug pattern
(ProvisionLinodeServerJob:72 createFirewall, destroyLinode doesn't
release). File separately if Linode is in active use.

Fixes #37
@obaid obaid merged commit 0836a02 into main May 22, 2026
1 of 2 checks passed
@obaid obaid deleted the fix/issue-37-firewall-leak branch May 22, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DO firewalls are orphaned on team/server deletion (25 leaked in personal account)

1 participant