Skip to content

Add soft-delete for service variants - #546

Open
jackfreem wants to merge 6 commits into
cashapp:masterfrom
jackfreem:jackf/delete-service-variant-ui
Open

Add soft-delete for service variants#546
jackfreem wants to merge 6 commits into
cashapp:masterfrom
jackfreem:jackf/delete-service-variant-ui

Conversation

@jackfreem

@jackfreem jackfreem commented Mar 11, 2026

Copy link
Copy Markdown

Problem

Backfila supports service variants (e.g. customermappings/playpen-jackf, customermappings/playpen-<ldap>) but there was no way to delete them once created. Stale playpen variants accumulate over time with no way to clean them up through the UI or API.
Screenshot 2026-03-12 at 10 13 50 AM

Solution

Adds reversible soft-delete for service variants. A deleted variant is hidden from all listings and detail views; if the variant later re-registers via configure_service, the soft delete is reversed in place (same row, history preserved).

Changes

Database

  • Adds deleted_at TIMESTAMP(3) NULL column to the services table (v024__backfila.sql)
  • Adds ServiceQuery.notDeleted() constraint using IS_NULL on deleted_at

Backend

  • New DeleteServiceVariantAction: POST /services/{service}/variants/{variant}/delete
    • Requires users capability
    • Guards against deleting a variant that has running backfills
    • Returns 400 if the variant doesn't exist or is already deleted
    • Emits an AuditClient event attributing the deletion to the caller
    • Returns a 303 redirect back to the services index (it's the target of the UI form POST)
  • Filters deleted variants from GetServicesAction, GetServiceVariantsAction, and GetServiceDetailsAction
  • ConfigureServiceAction revives a soft-deleted variant in place (clears deleted_at on the existing row); deleted variants don't count toward the variant limit

UI

  • Adds a "Danger Zone" section to the service info page (/services/{service}/{variant}/info) with a "Delete Variant" button and a confirmation dialog

Design decisions

  • Soft delete only — preserves historical backfill run data that references the service_id FK, consistent with how backfill run deletion works
  • Re-registration revives in place (changed per review) — the previous fresh-row approach collided with unq_registry_name_variant; reviving the existing row avoids that and keeps the variant's backfill history
  • Block deletion if running backfills exist — prevents orphaning active work
  • Audit via AuditClient rather than DbEventLogevent_logs.backfill_run_id is NOT NULL, so a service-level event doesn't fit that table without a migration

Testing

DeleteServiceVariantActionTest (runs against real MySQL with unq_registry_name_variant enforced) covers:

  • Happy path deletion + audit event attribution
  • Deleted variant hidden from services/variants listings while live variants stay visible
  • 400 on non-existent or already-deleted variant
  • 400 when running backfills exist
  • Re-registration after deletion revives the same row (asserts the row id is unchanged and deleted_at is cleared)

Also verified end-to-end against a locally running BackfilaDevelopmentService:

  • Info page renders the Danger Zone section
  • Form POST soft-deletes the row (deleted_at stamped) and returns 303 to /services/
  • Repeat POST returns 400; deleted variant's detail page shows the not-found error
  • Full :service:test suite passes (113 tests)

mpawliszyn added a commit to mpawliszyn/fowlcon that referenced this pull request Mar 15, 2026
- docs/versions.md: Public version roadmap (V1 through V1.1 + future ideas).
  V1.001 (agent optimization) and V1.002 (non-agent fixes) are parallel
  workstreams after the rough V1 ships.

- docs/v1/2026-03-15-v1-implementation-plan.md: Implementation plan for
  rough V1. Full pipeline (analysis + walkthrough) with complete prompt
  content for all 6 agent/command files. 9 tasks across 4 chunks.
  Test against cashapp/backfila#546.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>
mpawliszyn added a commit to mpawliszyn/fowlcon that referenced this pull request Mar 15, 2026
* docs: add V1 implementation plan and version roadmap

- docs/versions.md: Public version roadmap (V1 through V1.1 + future ideas).
  V1.001 (agent optimization) and V1.002 (non-agent fixes) are parallel
  workstreams after the rough V1 ships.

- docs/v1/2026-03-15-v1-implementation-plan.md: Implementation plan for
  rough V1. Full pipeline (analysis + walkthrough) with complete prompt
  content for all 6 agent/command files. 9 tasks across 4 chunks.
  Test against cashapp/backfila#546.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* docs: use formal name in plugin manifest, add attribution review to V1.002

- plugin.json author: "Michael Pawliszyn" (not "Mike")
- V1.002 task: review whether attribution should be Block Inc.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* docs: address plan review gaps and add V1.002 tasks

Plan fixes:
- Clarify Agent is the correct tool name (not Task from older docs)
- Note commands inherit all tools (no tools: field needed in orchestrator)
- Note analysis.md deferred from rough V1
- Add concrete plugin verification steps referencing superpowers pattern
- Specify README replacement text
- Clarify coverage-checker scope vs check-tree-quality.sh

V1.002 additions:
- Distribution polish and install docs
- README installation instructions
- analysis.md implementation
- Reconcile coverage-checker vs script scope

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

---------

Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
mpawliszyn added a commit to mpawliszyn/fowlcon that referenced this pull request Mar 15, 2026
…#3)

* docs: add V1 implementation plan and version roadmap

- docs/versions.md: Public version roadmap (V1 through V1.1 + future ideas).
  V1.001 (agent optimization) and V1.002 (non-agent fixes) are parallel
  workstreams after the rough V1 ships.

- docs/v1/2026-03-15-v1-implementation-plan.md: Implementation plan for
  rough V1. Full pipeline (analysis + walkthrough) with complete prompt
  content for all 6 agent/command files. 9 tasks across 4 chunks.
  Test against cashapp/backfila#546.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* docs: use formal name in plugin manifest, add attribution review to V1.002

- plugin.json author: "Michael Pawliszyn" (not "Mike")
- V1.002 task: review whether attribution should be Block Inc.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* docs: address plan review gaps and add V1.002 tasks

Plan fixes:
- Clarify Agent is the correct tool name (not Task from older docs)
- Note commands inherit all tools (no tools: field needed in orchestrator)
- Note analysis.md deferred from rough V1
- Add concrete plugin verification steps referencing superpowers pattern
- Specify README replacement text
- Clarify coverage-checker scope vs check-tree-quality.sh

V1.002 additions:
- Distribution polish and install docs
- README installation instructions
- analysis.md implementation
- Reconcile coverage-checker vs script scope

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add codebase-locator agent prompt

Finds WHERE relevant files are. Tools restricted to Grep/Glob/LS —
cannot read file contents, enforcing 'find, don't analyze' role.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add codebase-analyzer agent prompt

Explains HOW code works. Has Read access for content analysis.
Reports connections (imports, callers, interfaces, tests) for
concept-researcher to synthesize into change boundaries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add codebase-pattern-finder agent prompt

Shows EXAMPLES of repeated patterns. Reports 2-3 detailed examples,
total count, all instance locations, and variations. Key agent for
collapsing mechanical changes into variation nodes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add concept-researcher agent prompt

Coordinates worker agents (locator, analyzer, pattern-finder) and
synthesizes findings into structured concept descriptions. Targets
1,000-2,000 token output. Handles worker failures via supervisor mode.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add coverage-checker agent prompt

Verifies every changed file maps to a tree node. Haiku model for
cost efficiency. Reports gaps and structural issues without fixing them.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add orchestrator command prompt

Full review pipeline: analysis (concept decomposition, researcher
dispatch, tree building, coverage check) and interactive walkthrough
(depth-first traversal, reviewer decisions, state management).

Best-effort V1 prompt. Q7 (tone) and Q8 (change boundary) are
starting points — will be revisited in V1.001 with eval data.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* feat: add Claude Code plugin manifest

Packages Fowlcon as a Claude Code plugin. Agents and commands
auto-discovered from existing directory structure.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* docs: update README for V1 release

Status section reflects V1 rough beta availability with full
analysis and walkthrough pipeline.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

* chore: remove .gitkeep files from agents/ and commands/

Directories now contain real agent prompts and the orchestrator
command, so placeholder files are no longer needed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>

---------

Signed-off-by: Michael Pawliszyn <mpawliszyn@gmail.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

@mpawliszyn mpawliszyn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few observations and questions from a close read of the soft-delete change.

Comment on lines 56 to 61
val variantsForService = queryFactory.newQuery<ServiceQuery>()
.registryName(service)
.notDeleted()
.list(session)

var dbService = variantsForService.firstOrNull() { it.variant == variant }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-registration 'fresh row' path vs. the unique key: the services table keeps UNIQUE KEY unq_registry_name_variant (registry_name, variant) from v016, and v024 did not alter it. When this lookup returns null for a soft-deleted variant and the code then session.save()s a new DbService with the same (registry_name, variant), the DB unique key may reject the insert. Only the re-registration test asserts the fresh-row behavior and nothing exercises it over HTTP — worth confirming the test DB actually enforces the unique key. Connects to the PR note: "Once the migration is run, I'll be able to test the remaining parts of this."

Comment on lines 56 to 61
val variantsForService = queryFactory.newQuery<ServiceQuery>()
.registryName(service)
.notDeleted()
.list(session)

var dbService = variantsForService.firstOrNull() { it.variant == variant }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making the soft delete reversible instead of creating a fresh row on re-registration. Reversing the soft delete (clearing deleted_at on the existing row) is likely what we actually want in the re-registration case: it revives the variant in place, preserves the existing service_id and its history, and sidesteps the (registry_name, variant) unique-key collision noted above (which the current "fresh row via session.save()" path risks hitting). This would replace the .notDeleted() exclusion in the upsert lookup with an un-delete on match.

@jackfreem jackfreem Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, re-registration now clears deleted_at on the existing row instead of inserting a new one. This preserves the service_id and its backfill history and sidesteps the unique-key collision. The upsert lookup no longer excludes deleted rows, and deleted variants no longer count toward MAX_VARIANTS. The test now asserts the same row id survives a delete → re-register cycle (passes against real MySQL with the unique key enforced).

Comment on lines +28 to +29
@Constraint("deleted_at", Operator.IS_NOT_NULL)
fun deleted(): ServiceQuery

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ServiceQuery.deleted() (Operator.IS_NOT_NULL) is added but has zero callers anywhere in this PR or the codebase — dead code as of this change. notDeleted() is used at five sites; deleted() at none. Either wire it to a use (e.g. an admin 'show deleted variants' view) or drop it to keep the query surface minimal.

@jackfreem jackfreem Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped deleted(). Could add it back alongside an admin "show deleted" view if that ever materializes.

Comment on lines +47 to +49
fun softDelete(clock: Clock) {
deleted_at = clock.instant()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

softDelete(clock: Clock) takes the Clock as a parameter; it should use an injected clock instead of asking for one to be passed in. The caller (DeleteServiceVariantAction) already has a Clock injected and just forwards it here — pushing that injection down (or setting deleted_at directly at the call site, as SoftDeleteBackfillAction does) removes the passed-in parameter.

@jackfreem jackfreem Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed softDelete(clock) from the entity. DeleteServiceVariantAction now sets deleted_at = clock.instant() directly at the call site. I kept the action's injected Clock (rather than Instant.now() as in SoftDeleteBackfillAction) for testability.

Comment on lines +20 to +24
import misk.web.actions.WebAction
import misk.web.mediatype.MediaTypes

class DeleteServiceVariantAction @Inject constructor(
@BackfilaDb private val transacter: Transacter,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

caller: ActionScoped<MiskCaller?> is injected but never used. It should be used to write a DbEventLog audit entry attributing the deletion to a user, matching the two closest siblings (SoftDeleteBackfillAction, CancelBackfillAction) which both log who performed the mutation. As written, a variant deletion leaves no record of who did it.

@jackfreem jackfreem Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with AuditClient rather than DbEventLog: event_logs.backfill_run_id is NOT NULL, so a service-level event doesn't fit that table without another migration, whereas AuditClient is already the mechanism for run lifecycle audit events (AuditClientListener).

Deletion now emits an audit event with requestorLDAP = caller principal plus an app log line, and the test asserts the attribution via FakeAuditClient.sentEvents. Happy to widen event_logs instead if you'd prefer a durable in-DB record.

Comment on lines +146 to +159

// Danger zone
div("border-t border-red-200 pt-6") {
span("text-sm font-medium text-red-600 block mb-4") { +"Danger Zone" }
form {
action = "/services/$service/variants/$variant/delete"
method = FormMethod.post
button(classes = "inline-flex items-center px-4 py-2 border border-red-300 text-sm font-medium rounded-md text-red-700 bg-white hover:bg-red-50") {
type = ButtonType.submit
attributes["onclick"] = "return confirm('Are you sure you want to delete the $service ($variant) service variant? This cannot be undone.')"
+"Delete Variant"
}
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following on the reversible-soft-delete suggestion in ConfigureServiceAction: this Danger Zone / Delete Variant UI may not be needed if soft delete is made reversible on re-registration. If re-registering a variant revives it in place, then the delete + re-register cycle handles the stale-variant cleanup without a dedicated UI affordance to trigger deletion. Worth deciding the reversible-soft-delete design first, since it determines whether this button is necessary.

@jackfreem jackfreem Aug 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the button: revive-on-re-register handles resurrection, but there's still no other way to trigger a deletion, and cleaning up stale playpen variants is the point of the PR. The two compose nicely now, delete a stale variant, and if it ever re-registers it comes back with its history intact (so the confirm dialog no longer says "cannot be undone"... it can, by re-registering).

Separately, testing this against a locally running instance (I left Block after the layoffs) surfaced two bugs in the endpoint as reviewed: (1) misk's response-body claim check rejected the Unit return with a JSON @ResponseContentType at startup — the service wouldn't boot with this action installed; (2) the JSON @RequestContentType would have rejected the browser form's urlencoded POST. It now returns a 303 redirect back to the services index, matching the BackfillCreateHandlerAction idiom, and I verified the full flow (render → POST → 303 → variant hidden, second POST → 400) against a running dev instance.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we need the initial button but I'm not sure we need the "can't be undone" piece.

- Add DeleteServiceVariantAction: POST /services/{service}/variants/{variant}/delete
  Guards against deleting variants with running backfills
- Register DeleteServiceVariantAction in BackfilaWebActionsModule
- Filter deleted variants from GetServicesAction, GetServiceVariantsAction,
  GetServiceDetailsAction, and ConfigureServiceAction (so re-registration
  of a deleted variant creates a fresh row)
- Add DeleteServiceVariantActionTest covering happy path, error cases,
  running backfill guard, and re-registration behavior
Adds a "Danger Zone" section to the ServiceInfoAction page with a delete
button that POSTs to DeleteServiceVariantAction with a confirmation dialog.
- ConfigureServiceAction now clears deleted_at on the existing row when a
  deleted variant re-registers, instead of inserting a fresh row which
  collided with unq_registry_name_variant (reproduced locally against
  MySQL: 'Duplicate entry deep-fryer-playpen-jackf'). Deleted variants no
  longer count toward the variant limit.
- DeleteServiceVariantAction attributes deletions via AuditClient
  (event_logs.backfill_run_id is NOT NULL, so DbEventLog doesn't fit a
  service-level event) and logs the caller principal.
- Returns a 303 redirect to the services index so the Danger Zone form
  POST works in a browser; the previous Unit return with a JSON response
  content type failed misk's response-body claim check at startup.
- Drop unused ServiceQuery.deleted(); drop DbService.softDelete(clock) in
  favor of setting deleted_at at the call site.
- Test updates: revive-in-place assertions (same row id), audit event
  assertion, and the services-list test now keeps a live variant so the
  service stays listed.
@jackfreem
jackfreem force-pushed the jackf/delete-service-variant-ui branch from 33783a3 to d33278d Compare August 12, 2026 18:07
@jackfreem
jackfreem requested a review from mpawliszyn August 12, 2026 19:45
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.

2 participants