[] Add artifact cleanup Asynq worker for expired artifact deletion and docker-compose MinIO init bucket job — with unit tests#277
Conversation
…n and docker-compose MinIO init bucket job — with unit tests
![orange-codens[bot]](https://avatars.githubusercontent.com/in/4028936?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍊 Orange Codens レビュー
🔒 セキュリティ: 今回の diff で最も注目すべき点は、docker-compose.yml の createbuckets サービスで MinIO 認証情報がシェルコマンドに直接展開されている点 (CWE-77) と、HandleCleanup のバッチループで SoftDelete 失敗時に無限ループが発生しうる点 (CWE-835) です。また、ListExpired がクロステナントスコープを持つ可能性について、実装側での明示的な検証が推奨されます。Scheduler のグレースフルシャットダウン漏れも運用上のリスクになりえます。
🟡 medium: 1 / 🔵 low: 3
その他の指摘 (4 件)
- 🟡 [medium]
docker-compose.yml:102— MinIO 認証情報が環境変数経由でシェルコマンドに展開され、コマンドインジェクションの素地がある (confidence: 0.65,sec.sast.injection.command) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker.go:33— SoftDelete 失敗時にループが無限に繰り返す可能性がある (confidence: 0.75,sec.sast.availability.infinite_loop) - 🔵 [low]
backend/cmd/server/main.go:541— Asynq Scheduler がグレースフルシャットダウンされず、プロセス終了時にスケジュールが破損する可能性がある (confidence: 0.70,sec.sast.availability.scheduler_no_shutdown) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker.go:33— ListExpired がテナント (OrganizationID) でスコープされているか不明確で、クロステナント削除のリスクがある (confidence: 0.50,sec.sast.authz.missing_tenant_check)
head: 5dafa00 | Orange Codens (P1)
There was a problem hiding this comment.
🍊 Orange Codens レビュー
重大な問題あり: HandleCleanup の外側バッチループで storage.DeleteObject が全件失敗し続けた場合に無限ループに陥るリスクがある（critical）。また SoftDelete 失敗後も次回ジョブで再度 DeleteObject が呼ばれ得る設計上の問題もある。スケジューラのエラーが Info ログのみで握り潰されている点も運用上問題になりうる。テストはハッピーパスのみでエラーパスがカバーされておらず、タスクタイプ文字列のハードコードも保守性を下げている。
🔒 セキュリティ: 今回の差分で最も注意が必要なのは、docker-compose.yml に埋め込まれた MinIO 認証情報のデフォルトフォールバック（:-minioadmin）です。開発環境で環境変数が未設定のまま使用されると公知デフォルト資格情報で MinIO が操作可能になります。artifact_cleanup_worker.go の無限バッチループはタイムアウトや DoS のリスクを持ち、上限設定を推奨します。スケジューラ停止エラーを Info レベルで記録している点はセキュリティ監視の観点から Error レベルへの変更が望ましいです。
🔴 critical: 1 / 🟠 high: 1 / 🟡 medium: 5 / 🔵 low: 5
その他の指摘 (10 件)
- 🟡 [medium]
backend/cmd/server/main.go:546— scheduler.Run() のエラーが Info ログのみで握りつぶされている (confidence: 0.85,code.error_handling.scheduler_goroutine_silent) - 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker_test.go:160— mockArtifactCleanupStorage が bucket 引数を検証していない (confidence: 0.82,maintainability.test.missing_storage_delete_bucket_assertion) - 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker.go:33— 外側ループで context のキャンセルをチェックしていない (confidence: 0.80,code.context.no_ctx_check_in_loop) - 🟡 [medium]
docker-compose.yml:101— docker-compose の MinIO 認証情報がデフォルト値にフォールバックしハードコード相当になる (confidence: 0.75,sec.sast.injection.command) - 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker.go:47— SoftDelete に OrganizationID を渡しているが ListExpired の結果に対するテナント分離が検証されていない (confidence: 0.55,sec.sast.authz.missing_ownership_check) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker_test.go:148— テスト内でタスクタイプ文字列をハードコードしている (confidence: 0.95,maintainability.test.hardcoded_task_type_string) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker_test.go:105— storage.DeleteObject エラー時の継続動作がテストされていない (confidence: 0.88,maintainability.test.missing_error_case) - 🔵 [low]
backend/cmd/server/main.go:544— Asynq スケジューラの停止エラーを Info レベルでログしており、異常終了の検知が困難 (confidence: 0.85,sec.sast.info_disclosure.scheduler_error_log) - 🔵 [low]
docker-compose.yml:93— createbuckets init サービスに restart ポリシーがなく再起動ループのリスク (confidence: 0.75,maintainability.docker.no_restart_on_init_container) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker.go:33— 期限切れアーティファクトのバッチループに上限がなく、DoS / タスクタイムアウトのリスクがある (confidence: 0.70,sec.sast.dos.unbounded_loop)
head: 39d390a | Orange Codens (P1)
| if len(artifacts) < batchSize { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 [critical] storage.DeleteObject 失敗時に artifact が再度 ListExpired に現れ無限ループになる
storage.DeleteObject が失敗した場合は continue で SoftDelete をスキップするため、次のバッチで同じ artifact が再度 ListExpired に返される。batchSize=100 件すべてが削除失敗し続けると、len(artifacts) < batchSize の条件を満たさず外側の for {} が永遠に回り続ける。
該当箇所:
if err := w.storage.DeleteObject(ctx, w.bucket, artifact.StorageKey); err != nil {
slog.Error("delete artifact object", "artifact_id", artifact.ID, "error", err)
continue
}MinIO が一時的に落ちた場合など、全件 DeleteObject 失敗 → len(artifacts) == 100 → ループ継続、という状況が容易に発生する。ワーカーが停止しない限りゴルーチンが詰まり続ける。
修正案: 内側ループでエラーが起きた artifact をスキップカウントし、1 バッチ内でエラーが一定数を超えたら早期 return するか、または ListExpired にオフセット/カーソルを持たせる。あるいは ctx.Done() チェックを先頭に入れて context キャンセルで抜けられるようにする。
@@ -44,6 +44,10 @@
+ successCount := 0
for _, artifact := range artifacts {
if err := w.storage.DeleteObject(ctx, w.bucket, artifact.StorageKey); err != nil {
slog.Error("delete artifact object", "artifact_id", artifact.ID, "error", err)
continue
}
if err := w.artifactRepo.SoftDelete(ctx, artifact.ID, artifact.OrganizationID); err != nil {
slog.Error("soft delete artifact", "artifact_id", artifact.ID, "error", err)
+ } else {
+ successCount++
+ }
}
+ if successCount == 0 {
+ break // 1件も成功しなければループを抜けて無限ループを防ぐ
+ }code.logic.infinite_loop_on_storage_error | confidence: 0.92
| slog.Error("soft delete artifact", "artifact_id", artifact.ID, "error", err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 [high] SoftDelete 失敗時に artifact が次回バッチで再度 DeleteObject される
SoftDelete が失敗してもループを継続し、エラーを記録するだけでその artifact は soft-delete されない。次回のジョブ実行では ListExpired に同じ artifact が返り、storage.DeleteObject が再度呼ばれる。StorageKey が既に削除済みであれば 404 エラーで DeleteObject が失敗し、無限に同じ artifact が処理され続ける可能性がある。
該当箇所:
if err := w.artifactRepo.SoftDelete(ctx, artifact.ID, artifact.OrganizationID); err != nil {
slog.Error("soft delete artifact", "artifact_id", artifact.ID, "error", err)
}MinIO の DeleteObject べき等を前提にする設計であれば、少なくともコメントか設計メモを残すべき。べき等でない場合は二重削除を防ぐ仕組みが必要。
code.logic.softdelete_error_ignored_in_loop | confidence: 0.78
…n and docker-compose MinIO init bucket job — with unit tests
There was a problem hiding this comment.
🍊 Orange Codens レビュー
重大な問題あり: HandleCleanup において SoftDelete 失敗時に deleted_at が更新されないアーティファクトが次回バッチでも取得され、バッチサイズが丁度 100 件の場合に無限ループとなるロジックバグがある（critical）。また scheduler.Run() のエラーが Info ログのみで握り潰されており、スケジューラが無音で停止しても気づけない（high）。テストでは queue.TypeArtifactCleanup 定数を使わずマジック文字列をハードコードしており保守性が低下している（medium）。これらの修正を優先してください。
🔒 セキュリティ: 本 PR はアーティファクトクリーンアップワーカーの追加であり、SQL インジェクションや認証バイパスのような高リスクな脆弱性は確認されませんでした。主な懸念点は docker-compose.yml における MinIO 初期化スクリプトでの変数引用符欠如（CWE-78）とデフォルト弱認証情報（CWE-1391）、およびワーカー実装における SoftDelete 失敗時の冪等性欠如です。マルチテナント環境での ListExpired のテナント境界については実装を別途確認することを推奨します。
🔴 critical: 1 / 🟠 high: 1 / 🟡 medium: 3 / 🔵 low: 4 / ⚪ info: 1
その他の指摘 (8 件)
- 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker_test.go:143— テストでqueue.TypeArtifactCleanup定数を使わずマジック文字列を直接使用している (confidence: 0.95,maintainability.test.magic_string_constant) - 🟡 [medium]
backend/cmd/server/main.go:541— AsynqScheduler がグレースフルシャットダウンに組み込まれていない (confidence: 0.82,code.resource_management.scheduler_no_shutdown) - 🟡 [medium]
docker-compose.yml:103— docker-compose の MinIO 初期化スクリプトで環境変数がシェルに直接展開される (confidence: 0.72,sec.sast.injection.command) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker.go:44— SoftDelete 失敗後も ListExpired が同一アーティファクトを返し続け、無限ループ的に storage 削除が再試行される可能性 (confidence: 0.80,sec.sast.authz.missing_tenant_check) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker_test.go:119— entity.Artifact の RepositoryID フィールドがテストデータで未設定 (confidence: 0.75,maintainability.test.missing_repositoryid_field) - 🔵 [low]
docker-compose.yml:93— createbuckets サービスに restart ポリシーが設定されていない (confidence: 0.72,maintainability.docker.no_restart_policy) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker.go:34— ListExpired がテナント（OrganizationID）境界を跨いで全組織の期限切れアーティファクトを処理する設計になっているか確認が必要 (confidence: 0.45,sec.sast.authz.idor) - ⚪ [info]
docker-compose.yml:103— MinIO の認証情報にデフォルト値minioadminが使用されている (confidence: 0.90,sec.sast.crypto.weak_default)
head: c8818cf | Orange Codens (P1)
| break | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🔴 [critical] SoftDelete失敗時に同一レコードが無限ループで再処理される
該当箇所:
for _, artifact := range artifacts {
if err := w.storage.DeleteObject(ctx, w.bucket, artifact.StorageKey); err != nil {
slog.Error("delete artifact object", "artifact_id", artifact.ID, "error", err)
continue
}
if err := w.artifactRepo.SoftDelete(ctx, artifact.ID, artifact.OrganizationID); err != nil {
slog.Error("soft delete artifact", "artifact_id", artifact.ID, "error", err)
}
}
if len(artifacts) < batchSize {
break
}SoftDelete が失敗した場合、deleted_at が設定されないため当該 artifact は次の ListExpired 呼び出しでも再び取得される。バッチサイズが 100 件ちょうどの場合（len(artifacts) == batchSize）、ループを抜ける条件を満たせず、StorageKey が既に削除済みのオブジェクトに対して DeleteObject を繰り返す無限ループに陥る。最終的には context タイムアウトまでループが継続し、不必要な MinIO API コールが大量発生する。
修正案: SoftDelete 失敗時も continue し、そのアイテムをスキップするか、または処理済み件数ではなく「SoftDelete に成功した件数」でループ継続判定を行う。あるいは SoftDelete 失敗の artifact を除外したカウントをバッチ終了判定に使う。
@@ -47,6 +47,9 @@ func (w *ArtifactCleanupWorker) HandleCleanup(ctx context.Context, task *asynq.Task) error {
if err := w.artifactRepo.SoftDelete(ctx, artifact.ID, artifact.OrganizationID); err != nil {
slog.Error("soft delete artifact", "artifact_id", artifact.ID, "error", err)
+ continue
}
+ successCount++
}code.logic.infinite_loop_on_soft_delete_failure | confidence: 0.88
| } | ||
| go func() { | ||
| if err := scheduler.Run(); err != nil { | ||
| middleware.Log().Info("asynq scheduler stopped", "error", err) |
There was a problem hiding this comment.
🟠 [high] scheduler.Run() 失敗時に Info ログのみでプロセスが継続する
該当箇所:
go func() {
if err := scheduler.Run(); err != nil {
middleware.Log().Info("asynq scheduler stopped", "error", err)
}
}()scheduler.Run() が起動失敗（Redis 接続不可など）した場合、Info レベルのログを出力するだけでサーバープロセスは正常稼働を続ける。その結果、@hourly の artifact cleanup タスクが一切エンキューされないが、オペレーターはエラーに気づきにくい。既存の asynqServer.Run(mux) も同様のパターンであればプロジェクトの慣習に沿っているかもしれないが、少なくとも Error レベルのログを使うべきである。
@@ -543,7 +543,7 @@ go func() {
if err := scheduler.Run(); err != nil {
- middleware.Log().Info("asynq scheduler stopped", "error", err)
+ middleware.Log().Error("asynq scheduler stopped", "error", err)
}
}()code.error_handling.scheduler_goroutine_silent_stop | confidence: 0.92
# Conflicts: # backend/cmd/server/main.go # backend/go.sum # backend/internal/infrastructure/queue/asynq_client.go
There was a problem hiding this comment.
🍊 Orange Codens レビュー
artifact cleanup worker の基本フローとテスト happy path は PRD の FR-3 に沿っていますが、SoftDelete や DeleteObject が全件失敗した場合に外側ループが終了条件を満たせず無限ループする点が critical です。あわせて、1タスクあたりの処理量上限がなく、失敗パスのテストも不足しています。
🔒 セキュリティ: 本 PR は期限切れアーティファクトを削除する内部 Asynq worker と MinIO バケット初期化の追加であり、diff 上はユーザー入力を直接受け取る処理パスがありません。HandleCleanup は DB から取得した StorageKey を repository / storage 抽象経由で削除するだけで、SQL injection・SSRF・暗号誤用・認証 bypass に該当する具体的コードは見当たりません。Asynq タスクは payload なしで既存 worker と同様 Redis を信頼境界とする設計のため、論理的な脆弱性として報告すべき事項はありません。
🔴 critical: 1 / 🟡 medium: 3 / 🔵 low: 1 / ⚪ info: 1
その他の指摘 (5 件)
- 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker_test.go:89— 失敗パス（DeleteObject / SoftDelete エラー、無限ループ防止）のテストがない (confidence: 0.88,maintainability.test.missing_failure_cases) - 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker.go:33— 1回の HandleCleanup 呼び出しで全期限切れ artifact を処理し切るまでループする (confidence: 0.82,performance.unbounded_work) - 🟡 [medium]
backend/internal/worker/artifact_cleanup_worker.go:43— DeleteObject 失敗時に SoftDelete を試行せず、DeleteObject 成功後の SoftDelete 失敗も再試行されない (confidence: 0.78,code.logic.partial_failure) - 🔵 [low]
backend/internal/worker/artifact_cleanup_worker_test.go:138— テストでタスク種別を文字列リテラルで指定している (confidence: 0.90,maintainability.naming.magic_string) - ⚪ [info]
backend/cmd/server/main.go:636— 各 server プロセスが独立して @hourly スケジューラを起動する (confidence: 0.55,maintainability.ops.multi_instance_scheduler)
head: a83cae3 | Orange Codens (P1)
|
|
||
| if len(artifacts) < batchSize { | ||
| break | ||
| } |
There was a problem hiding this comment.
🔴 [critical] SoftDelete が全件失敗すると HandleCleanup が無限ループする
外側の for ループは len(artifacts) == 0 または len(artifacts) < batchSize のときのみ終了します。SoftDelete 失敗時はログのみで処理を継続するため、期限切れレコードは DB 上で削除されず、次の反復でも同じバッチが返り続けます。
該当箇所:
for {
artifacts, err := w.artifactRepo.ListExpired(ctx, batchSize)
...
for _, artifact := range artifacts {
...
if err := w.artifactRepo.SoftDelete(ctx, artifact.ID, artifact.OrganizationID); err != nil {
slog.Error("soft delete artifact", "artifact_id", artifact.ID, "error", err)
}
}
if len(artifacts) < batchSize {
break
}
}batchSize(100) 件ちょうど返り、全件の SoftDelete が失敗する（DB 障害・権限エラー等）ケースでは len(artifacts) < batchSize にならず、Asynq タスクが完了せずワーカーを占有し続けます。DeleteObject が全件 continue されるケースも同様です。
@@ -48,6 +48,15 @@
}
}
+ deletedCount := 0
+ for _, artifact := range artifacts {
+ // track successful SoftDelete in the inner loop
+ }
+ if deletedCount == 0 && len(artifacts) > 0 {
+ return fmt.Errorf("artifact cleanup made no progress on batch of %d", len(artifacts))
+ }
+
if len(artifacts) < batchSize {
break
}code.logic.infinite_loop | confidence: 0.93
1 participant
Summary
Type of Change
Testing
Checklist
Task Goal
Target Repository
Corevice/open-gitAll work for this task belongs in this repository. Do the implementation inside this repository's directory in the workspace; do not search or edit other repositories unless this task explicitly requires a cross-repo change.
Context
Expired artifacts must be cleaned up asynchronously (per PRD FR-3). This sub-task adds an Asynq periodic worker following the pattern of
ci_worker.goandwebhook_worker.go, and adds a MinIO init service todocker-compose.ymlthat creates theartifactsbucket on startup. The worker runs hourly via Asynq cron.Steps
open-git/backend/internal/infrastructure/queue/asynq_client.go— add constantTypeArtifactCleanup = "artifact:cleanup"and helper:open-git/backend/internal/worker/artifact_cleanup_worker.go— structArtifactCleanupWorkerwith fieldsartifactRepo domain.IArtifactRepository,storage domainservice.ArtifactStorage,bucket string; constructorNewArtifactCleanupWorker(repo, storage, bucket); methodHandleCleanup(ctx, *asynq.Task) errorthat callsrepo.ListExpired(ctx, 100)in a loop, for each callsstorage.DeleteObject(ctx, bucket, artifact.StorageKey)thenrepo.SoftDelete(ctx, artifact.ID, artifact.OrganizationID), logs errors per artifact without aborting the batch, returns nil after processing allopen-git/backend/internal/worker/artifact_cleanup_worker_test.go— using in-memory SQLite (same pattern asci_worker_test.go): seed one expired artifact (expires_at in past, status=completed) and one non-expired; runHandleCleanup; assert expired artifact'sdeleted_atis now set; assert non-expired artifact'sdeleted_atis still NULL; use a mockArtifactStoragethat records callsChanges Made
Implement
ing
the
artifact
cleanup
worker
per
the
task
.
Review
ing
existing
patterns
in
the
named
files
.
Checking
artifact
repository
and
storage
interfaces
for
the
worker
implementation
.
Implement
ing
the
changes
across
the
five
named
files
.
Fix
ing
the
test
:
must Execexpects
* sql .DB,but
the
test
passes
* sql x .DB.Implemented
the
artifact
cleanup
async
worker per FR-3. Summary of changes:
Changes
asynq_client.go— AddedTypeArtifactCleanup = "artifact:cleanup"andEnqueueArtifactCleanuphelper.artifact_cleanup_worker.go— NewArtifactCleanupWorkerthat:MinIO objects, then soft-deletes metadata
artifact_cleanup_worker_test.go— In-memorySQLite test with one expired and one non-expired artifact; verifies only the expired one is cleaned up and storage delete is called.
4
.
main.go— RegisteredHandleCleanupon the Asynq mux and added an@hourlyscheduler forthe cleanup task.
docker-compose.yml— Addedcreatebucketsinit service that creates theartifactsbucket after MinIO is healthy.
[PURPLE-MODIFIED-FILES]
backend/internal/infrastructure/queue/asynq_client.go
backend/internal/worker/artifact_cleanup_worker.go
backend/internal/worker/
artifact
_cleanup
_worker
_test
.go
backend
/cmd
/server
/main
.go
docker
-compose
.yml
[/
P
UR
P
LE
-M
OD
IFIED
-F
ILES
]
Task ID:
019f0d21-ff03-7c3e-9f44-e3442538a921Generated by Purple Codens