fix(scan): prevent scan tasks from being lost after transaction commit

[bbdu3]

Summary

This closes #749 .Fix the issue where scan tasks can be lost after the database transaction commits but before they are written to the Redis Stream.

Changes

• Added the scan_task_outbox table, domain model, and repository implementation.
• Persist security audit records and Outbox tasks within the same transaction.
• Added a scheduled Dispatcher supporting:
  • Publishing tasks to the Redis Stream;
  • Retry with exponential backoff;
  • Reclaiming timed-out tasks;
  • Cleaning up completed tasks.
• Added taskId-based consumer idempotency checks to prevent duplicate scans.
• Clean up related Outbox tasks when a SkillVersion is deleted.
• Added tests for Outbox state transitions, transactional persistence, and Redis publish failures.

Verification

• SecurityScanServiceTest: 10 passed
• ScanTaskOutboxTest: 2 passed
• SecurityScanOutboxTest: 1 passed
• skillhub-infra compilation passed
• git diff --check passed

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

bbdu3 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[FenjuFu]

Reviewed against #749. The transactional-outbox approach is the right fix: writing ScanTaskOutbox(PENDING) inside the same transaction as the SkillVersion -> SCANNING / SecurityAudit(taskId) update makes enqueue atomic with the state change, so a crash or Redis outage between commit and publish can no longer strand a version in SCANNING. The dispatcher's claim/lease + expired-lease re-claim, the exponential backoff, the SENT cleanup job, and the consumer-side idempotency guard are all coherent, and the test coverage is good. A few points, most tied back to the issue's own acceptance wording:

1. FAILED is defined but never reached — the issue's "明确进入失败状态" outcome isn't implemented. The issue's Expected Behavior is "最终完成扫描或明确进入失败状态". markRetry always returns the row to PENDING; there is no max-retry cap that transitions to FAILED. A permanently-broken task (bad payload, scanner rejects it) will re-publish every max-backoff (5 min) forever and never surface to an admin. Consider a max-attempts threshold that flips the row to FAILED (and ideally drives the SkillVersion out of SCANNING into a failed state) so the "clearly failed" half of the issue is actually met.

2. Multi-replica dispatch aborts the whole batch on the first claim collision. findPendingDue/findExpiredLeases are plain selects (no FOR UPDATE SKIP LOCKED), and cross-instance safety rests on the @Version optimistic lock at saveAndFlush(outbox). That saveAndFlush is outside the per-task try, so if two dispatchers claim the same row, the OptimisticLockException propagates out of the @Transactional dispatch() and rolls back the entire batch (every already-claimed row in that run reverts). It self-heals next tick, but under contention it can thrash. Either catch the optimistic failure per row and continue, or switch the two finders to SKIP LOCKED pessimistic locking.

3. Idempotency guard only covers completed scans, not in-flight ones. isTaskAlreadyProcessed = existsByTaskIdAndScannedAtIsNotNull. At-least-once redelivery (publish succeeds, then the app dies before markSent, lease expires, re-published) is possible by design — but if the redelivery lands before the first scan finishes, both pass the guard and the scanner runs twice concurrently for one taskId. If markProcessing doesn't already dedup in-flight tasks, consider guarding on "audit exists for taskId" rather than "audit scanned".

4. softDeleteByVersionId doesn't purge the outbox (only hardDelete does). A version soft-deleted while a PENDING outbox row is still queued will still get published and scanned by the dispatcher. Mirror the scanTaskOutboxRepository.deleteByVersionId(...) call (or mark the row terminal) in the soft-delete path too.

5. Minor: ScanTaskOutbox.toScanTask() reconstructs the payload from discrete columns and hardcodes scannerType = SKILL_SCANNER, dropping the original ScanTask attributes map — fine today since triggerScan only ever emits SKILL_SCANNER, but it silently won't round-trip any future scanner type/attribute. Also the entity's constructor/@PrePersist use Instant.now(Clock.systemUTC()) directly while the dispatcher uses the injected Clock — worth unifying on the injected clock for testability.

None of these block the core fix for the strand-in-SCANNING bug; (1) is the one I'd most want addressed since it's part of the issue's stated expected behavior.

[bbdu3]

Thanks for the detailed review. I’ve addressed all five points:

1. Added a configurable maximum attempt limit. Exhausted tasks now move to FAILED, and versions still in SCANNING move to SCAN_FAILED.
2. Changed dispatcher selection to PostgreSQL FOR UPDATE SKIP LOCKED to prevent multi-replica claim collisions from rolling back an entire batch.
3. Added a Redisson watchdog lock keyed by taskId to prevent concurrent duplicate scans. Also fixed edge case where a skipped duplicate could delete the active scan’s temporary directory.
4. Soft deletion now removes pending Outbox tasks for the corresponding version.
5. Added a V45 migration to preserve the original task metadata and creation time instead of reconstructing a partial payload.
   Additional tests cover retry exhaustion, terminal-state protection, expired lease reclaiming, concurrent delivery, lock release on failure, soft deletion, and payload round-tripping.

Verification:

• Domain tests: 15 passed
• Consumer/Dispatcher/Logging tests: 18 passed
• Full backend suite: 1,392 tests, 0 failures, 0 errors, 1 skipped
• Clean Java 21 build: all 8 Maven modules passed
• git diff --check passed

[FenjuFu]

The five requested changes are present, but the new taskId lock introduces a message-loss race with the existing pending-message reclaimer.\n\nIn ScanTaskConsumer.processBusiness, a failed processingLock.tryLock() calls skipCleanup() and returns normally. AbstractStreamConsumer.handleMessageInScope treats every normal return as success: it calls markCompleted(payload) and then �cknowledge(messageId).\n\nThat is unsafe for the exact duplicate-delivery scenario this lock targets:\n\n1. Consumer A is still scanning after the default 2-minute reclaim idle threshold and holds the taskId lock.\n2. Redis auto-claims the same pending entry to consumer B.\n3. B cannot acquire the lock, returns normally, and XACKs the shared stream entry.\n4. If A crashes or is killed after B's acknowledgement, there is no pending entry left to reclaim and no retry message. The version can remain in SCANNING, recreating the strand this PR is meant to eliminate.\n\nskipCleanup protects the temp path but does not protect delivery state. Please make lock contention preserve eventual processing—for example, wait for the watchdog lock and re-check isTaskAlreadyProcessed after acquisition, or add an explicit consumer outcome that leaves a contended message pending without acknowledging it. Add a test that drives the auto-claim/lock-contention path and proves the entry is not acknowledged before the active scan reaches a terminal result.\n\nI independently inspected the max-attempt terminal transition, SKIP LOCKED query, soft-delete cleanup, and metadata migration; those address the earlier points. Remote CI/DCO/CLA are green. My local targeted Maven run was blocked only because the available JDK does not support the project's Java 21 --release flag.

[FenjuFu]

Thanks for addressing all five review points. I rechecked the latest head (1556665) against #749:

• exhausted retries now reach FAILED and move versions still in SCANNING to SCAN_FAILED;
• FOR UPDATE SKIP LOCKED avoids cross-replica claim collisions;
• the taskId-scoped Redisson lock prevents concurrent duplicate scans without deleting the active scan's temporary files;
• soft deletion removes queued outbox tasks;
• the JSONB metadata migration preserves the original task payload.

DCO, CLA, unit tests, RISC-V builds, and the real-services E2E check are all green. The implementation now satisfies the issue's expected recovery and explicit-failure behavior. I am leaving this as a comment only, not an approval.