CleanupOrphanData loads allUploadIds but never uses it — possible missing orphan-scan case (S1481) #678

New Issue

2026-07-17T19:29:41Z

spikerj commented

2026-07-17 19:29:41 +00:00

Found via S1481 (unused local) triage — flagging intent, not proposing a blind change.

CleanupOrphanDataCommandHandler.cs:36 loads every UploadWorkflow id:

var allUploadIds = await context.UploadWorkflows.Select(w => w.UploadId).ToListAsync(...);

…but allUploadIds is never used. The handler's orphaned-security-scan pass (lines 46-48) only catches scans belonging to workflows orphaned BY BookId (orphanedWorkflowUploadIds). It does NOT catch scans whose workflow row no longer exists at all (scan.UploadId not present in any workflow) — which is exactly what allUploadIds would be needed for.

So either:

There is a missing orphan case — security scans referencing deleted workflows are never cleaned and accumulate — and allUploadIds is the leftover of that intended-but-unwired logic; or
That case is deliberately out of scope, and allUploadIds is just dead code to remove.

Low severity (maintenance cleanup handler; worst case is some orphaned BookSecurityScans rows accumulate). Needs a product/intent decision: complete the scan-orphaned-by-deletion cleanup, or remove the dead query. SonarQube rule csharpsquid:S1481.

Found via S1481 (unused local) triage — flagging intent, not proposing a blind change. CleanupOrphanDataCommandHandler.cs:36 loads every UploadWorkflow id: ``` var allUploadIds = await context.UploadWorkflows.Select(w => w.UploadId).ToListAsync(...); ``` …but `allUploadIds` is never used. The handler's orphaned-security-scan pass (lines 46-48) only catches scans belonging to workflows orphaned BY BookId (orphanedWorkflowUploadIds). It does NOT catch scans whose workflow row no longer exists at all (scan.UploadId not present in any workflow) — which is exactly what `allUploadIds` would be needed for. So either: 1. There is a missing orphan case — security scans referencing deleted workflows are never cleaned and accumulate — and `allUploadIds` is the leftover of that intended-but-unwired logic; or 2. That case is deliberately out of scope, and `allUploadIds` is just dead code to remove. Low severity (maintenance cleanup handler; worst case is some orphaned BookSecurityScans rows accumulate). Needs a product/intent decision: complete the scan-orphaned-by-deletion cleanup, or remove the dead query. SonarQube rule csharpsquid:S1481.

spikerj commented

2026-07-18 17:56:36 +00:00

Triage verdict: option 1 — it was a real missing orphan case. Fix up as backend PR #412.

Traced the data flow skeptically before deciding:

The only production deleter of UploadWorkflows is the cleanup handler itself, and it deletes a workflow's scans in the same SaveChanges — so at first glance the workflow-less-scan case looks unreachable and allUploadIds looks like plain dead code.
But there's a real leak path: a security.scan.requested message in flight while cleanup deletes its (book-orphaned) workflow produces a scan row after the delete. The old predicate (scans of currently-orphaned workflows only) can never reclaim it → permanent residue, one row per race occurrence. Any future/manual workflow deletion has the same effect.
Deleting such scans is safe: UploadOrchestrator saves the workflow row before publishing security.scan.requested, so a scan whose workflow is missing is never a pre-creation transient — only post-deletion residue.

PR #412 completes the original allUploadIds intent (predicate now also matches scans with no remaining workflow row) and adds the handler's first unit tests (dry-run count, execute delete, no-orphan no-op). Business.Tests 7591 green. Will close this ticket when the PR merges.

**Triage verdict: option 1 — it was a real missing orphan case.** Fix up as backend PR [#412](https://git.spikersoft.com/spikerj/spikersoft-backend/pulls/412). Traced the data flow skeptically before deciding: - The **only** production deleter of `UploadWorkflows` is the cleanup handler itself, and it deletes a workflow's scans in the same `SaveChanges` — so at first glance the workflow-less-scan case looks unreachable and `allUploadIds` looks like plain dead code. - But there's a real leak path: a `security.scan.requested` message **in flight** while cleanup deletes its (book-orphaned) workflow produces a scan row *after* the delete. The old predicate (scans of currently-orphaned workflows only) can never reclaim it → permanent residue, one row per race occurrence. Any future/manual workflow deletion has the same effect. - Deleting such scans is safe: `UploadOrchestrator` saves the workflow row *before* publishing `security.scan.requested`, so a scan whose workflow is missing is never a pre-creation transient — only post-deletion residue. PR #412 completes the original `allUploadIds` intent (predicate now also matches scans with no remaining workflow row) and adds the handler's first unit tests (dry-run count, execute delete, no-orphan no-op). Business.Tests 7591 green. Will close this ticket when the PR merges.

spikerj commented

2026-07-18 18:02:08 +00:00

Resolved in backend PR #412 (merged to master 2026-07-18). The orphaned-scan predicate now also reclaims BookSecurityScans rows whose workflow row no longer exists (in-flight-scan-vs-cleanup race / manual deletions), completing the original allUploadIds intent — plus first unit-test coverage for the handler (dry-run count, execute delete, no-orphan no-op). Closing.

Resolved in backend PR [#412](https://git.spikersoft.com/spikerj/spikersoft-backend/pulls/412) (merged to `master` 2026-07-18). The orphaned-scan predicate now also reclaims `BookSecurityScans` rows whose workflow row no longer exists (in-flight-scan-vs-cleanup race / manual deletions), completing the original `allUploadIds` intent — plus first unit-test coverage for the handler (dry-run count, execute delete, no-orphan no-op). Closing.

spikerj closed this issue

2026-07-18 18:02:08 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#678