feat: Add bucket cleanup worker for hard deletion after retention period #1106
Conversation
WalkthroughAdds soft-delete support for ledgers (new deletedAt field), includeDeleted listing, bucket delete/restore HTTP endpoints, store/controller/store migrations and queries, a cron-scheduled BucketCleanupRunner that hard-deletes aged buckets, CLI flags and FX wiring for the runner, and accompanying unit, integration, and e2e tests and client SDK updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as v2 API
participant Ctrl as SystemController
participant Store as System Store
participant DB as Database
rect rgb(220, 230, 255)
Note over Client,API: Soft-delete via API
Client->>API: DELETE /v2/_/buckets/{bucket}
API->>Ctrl: DeleteBucket(ctx, bucket)
Ctrl->>Store: DeleteBucket(ctx, bucket)
Store->>DB: UPDATE _system.ledgers SET deleted_at=now() WHERE bucket=...
DB-->>Store: OK
Store-->>Ctrl: OK
Ctrl-->>API: 204 No Content
end
rect rgb(220, 255, 230)
Note over Client,API: Restore via API
Client->>API: POST /v2/_/buckets/{bucket}/restore
API->>Ctrl: RestoreBucket(ctx, bucket)
Ctrl->>Store: RestoreBucket(ctx, bucket)
Store->>DB: UPDATE _system.ledgers SET deleted_at=NULL WHERE bucket=...
DB-->>Store: OK
Store-->>Ctrl: OK
Ctrl-->>API: 204 No Content
end
rect rgb(255, 245, 220)
Note over Store,DB: Scheduled hard-delete (BucketCleanupRunner)
loop cron schedule
Store->>DB: SELECT DISTINCT bucket WHERE deleted_at < cutoff_time
DB-->>Store: [bucketA, bucketB]
Store->>DB: DROP SCHEMA IF EXISTS bucketA CASCADE
Store->>DB: DELETE FROM _system.ledgers WHERE bucket='bucketA'
DB-->>Store: OK
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1106 +/- ##
==========================================
+ Coverage 81.98% 82.05% +0.06%
==========================================
Files 186 189 +3
Lines 8942 9116 +174
==========================================
+ Hits 7331 7480 +149
- Misses 1187 1204 +17
- Partials 424 432 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gfyrag
force-pushed
the
feat/bucket-deletion
branch
from
345dd7f to
fcd6974
Compare
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @gfyrag. * #1106 (comment) The following files were modified: * `cmd/worker.go` * `internal/api/v2/controllers_buckets_delete.go` * `internal/api/v2/controllers_buckets_restore.go` * `internal/api/v2/controllers_ledgers_list.go` * `internal/api/v2/routes.go` * `internal/controller/system/controller.go` * `internal/storage/system/migrations.go` * `internal/storage/worker_bucket_cleanup.go` * `internal/worker/module.go` * `pkg/testserver/worker.go`
Docstrings generation was requested by @gfyrag. * #1106 (comment) The following files were modified: * `cmd/worker.go` * `internal/api/v2/controllers_buckets_delete.go` * `internal/api/v2/controllers_buckets_restore.go` * `internal/api/v2/controllers_ledgers_list.go` * `internal/api/v2/routes.go` * `internal/controller/system/controller.go` * `internal/storage/system/migrations.go` * `internal/storage/worker_bucket_cleanup.go` * `internal/worker/module.go` * `pkg/testserver/worker.go` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/api/v2/controllers_buckets_restore.go (1)
15-27: Consider input validation and error differentiation.The handler correctly implements the restore operation, but consider these enhancements:
- Input validation: The bucket parameter is not validated. Consider checking for empty strings or invalid characters.
- Error differentiation: All errors return 500 Internal Server Error. Consider returning 404 Not Found for non-existent buckets or 400 Bad Request for validation errors.
Example enhancement:
func restoreBucket(systemController system.Controller) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { bucket := chi.URLParam(r, "bucket") + if bucket == "" { + api.BadRequest(w, common.ErrValidation, errors.New("bucket parameter is required")) + return + } err := systemController.RestoreBucket(r.Context(), bucket) if err != nil { + // Consider checking error type and returning appropriate status common.InternalServerError(w, r, err) return }internal/api/v2/controllers_buckets_delete.go (1)
14-26: Consider input validation and error differentiation.Similar to the restore handler, this implementation could benefit from:
- Input validation: The bucket parameter is not validated for empty strings or invalid characters.
- Error differentiation: All errors return 500 Internal Server Error. Consider returning 404 Not Found for non-existent buckets or 400 Bad Request for validation errors.
Example enhancement:
func deleteBucket(systemController system.Controller) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { bucket := chi.URLParam(r, "bucket") + if bucket == "" { + api.BadRequest(w, common.ErrValidation, errors.New("bucket parameter is required")) + return + } err := systemController.DeleteBucket(r.Context(), bucket) if err != nil { + // Consider checking error type and returning appropriate status common.InternalServerError(w, r, err) return }internal/storage/worker_bucket_cleanup.go (1)
148-168: Verify panic behavior is intentional.The
OnStarthook launches a goroutine that panics ifRunreturns an error (line 158). This will crash the entire application. While the comment documents this, confirm this is the desired behavior—typically background workers log errors and stop gracefully rather than crashing the app.Additionally, the constructor at lines 150-151 returns
errorbut never returns a non-nil error. Consider simplifying the signature:- fx.Provide(func(logger logging.Logger, db *bun.DB) (*BucketCleanupRunner, error) { - return NewBucketCleanupRunner(logger, db, cfg), nil + fx.Provide(func(logger logging.Logger, db *bun.DB) *BucketCleanupRunner { + return NewBucketCleanupRunner(logger, db, cfg)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.log
📒 Files selected for processing (38)
cmd/worker.go(6 hunks)docs/api/README.md(9 hunks)internal/README.md(10 hunks)internal/api/common/mocks_system_controller_test.go(2 hunks)internal/api/v1/mocks_system_controller_test.go(2 hunks)internal/api/v2/controllers_buckets_delete.go(1 hunks)internal/api/v2/controllers_buckets_delete_test.go(1 hunks)internal/api/v2/controllers_buckets_restore.go(1 hunks)internal/api/v2/controllers_buckets_restore_test.go(1 hunks)internal/api/v2/controllers_ledgers_list.go(2 hunks)internal/api/v2/mocks_system_controller_test.go(2 hunks)internal/api/v2/routes.go(4 hunks)internal/controller/system/controller.go(3 hunks)internal/controller/system/store.go(1 hunks)internal/ledger.go(1 hunks)internal/storage/system/migrations.go(3 hunks)internal/storage/system/queries.go(1 hunks)internal/storage/system/resource_ledgers.go(1 hunks)internal/storage/system/store.go(4 hunks)internal/storage/worker_bucket_cleanup.go(1 hunks)internal/storage/worker_bucket_cleanup_test.go(1 hunks)internal/worker/module.go(3 hunks)pkg/client/README.md(1 hunks)pkg/client/docs/models/components/v2ledger.md(1 hunks)pkg/client/docs/models/operations/v2deletebucketrequest.md(1 hunks)pkg/client/docs/models/operations/v2deletebucketresponse.md(1 hunks)pkg/client/docs/models/operations/v2listledgersrequest.md(1 hunks)pkg/client/docs/models/operations/v2restorebucketrequest.md(1 hunks)pkg/client/docs/models/operations/v2restorebucketresponse.md(1 hunks)pkg/client/docs/sdks/v2/README.md(2 hunks)pkg/client/models/components/v2ledger.go(2 hunks)pkg/client/models/operations/v2deletebucket.go(1 hunks)pkg/client/models/operations/v2listledgers.go(3 hunks)pkg/client/models/operations/v2restorebucket.go(1 hunks)pkg/client/v2.go(1 hunks)pkg/testserver/worker.go(1 hunks)test/e2e/api_buckets_delete_test.go(1 hunks)test/e2e/app_bucket_cleanup_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
internal/api/v2/controllers_buckets_restore.go (2)
internal/controller/system/controller.go (1)
Controller(41-55)internal/api/common/errors.go (1)
InternalServerError(68-73)
internal/worker/module.go (1)
internal/storage/worker_bucket_cleanup.go (2)
BucketCleanupRunnerConfig(18-21)NewBucketCleanupRunnerModule(148-168)
internal/api/v2/controllers_buckets_delete.go (2)
internal/controller/system/controller.go (1)
Controller(41-55)internal/api/common/errors.go (1)
InternalServerError(68-73)
internal/storage/system/resource_ledgers.go (3)
internal/storage/common/resource.go (1)
RepositoryHandlerBuildContext(72-75)internal/storage/system/queries.go (1)
ListLedgersQueryPayload(3-7)internal/ledger.go (1)
Ledger(18-27)
internal/api/v2/controllers_ledgers_list.go (4)
internal/controller/system/controller.go (1)
Controller(41-55)internal/api/common/pagination.go (1)
PaginationConfig(3-6)internal/storage/system/queries.go (1)
ListLedgersQueryPayload(3-7)internal/storage/common/resource.go (1)
ResourceQuery(408-414)
internal/api/v1/mocks_system_controller_test.go (2)
internal/api/common/mocks_system_controller_test.go (2)
SystemController(207-211)SystemControllerMockRecorder(214-216)internal/api/v2/mocks_system_controller_test.go (2)
SystemController(207-211)SystemControllerMockRecorder(214-216)
test/e2e/app_bucket_cleanup_test.go (9)
test/e2e/suite_test.go (1)
UseTemplatedDatabase(202-204)pkg/testserver/server.go (1)
ExperimentalFeaturesInstrumentation(30-35)pkg/testserver/worker.go (2)
BucketCleanupCRONSpecInstrumentation(38-43)BucketCleanupRetentionPeriodInstrumentation(30-35)pkg/client/models/operations/v2createledger.go (1)
V2CreateLedgerRequest(9-13)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-25)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-68)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/operations/v2deletebucket.go (1)
V2DeleteBucketRequest(9-12)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(10-28)
internal/api/v2/mocks_system_controller_test.go (2)
internal/api/common/mocks_system_controller_test.go (2)
SystemController(207-211)SystemControllerMockRecorder(214-216)internal/api/v1/mocks_system_controller_test.go (2)
SystemController(207-211)SystemControllerMockRecorder(214-216)
pkg/client/models/operations/v2listledgers.go (1)
pkg/client/internal/utils/json.go (2)
MarshalJSON(21-115)UnmarshalJSON(117-251)
pkg/testserver/worker.go (1)
cmd/worker.go (4)
WorkerAsyncBlockHasherMaxBlockSizeFlag(29-29)WorkerAsyncBlockHasherScheduleFlag(30-30)WorkerBucketCleanupRetentionPeriodFlag(32-32)WorkerBucketCleanupScheduleFlag(33-33)
internal/storage/worker_bucket_cleanup_test.go (3)
internal/storage/ledger/main_test.go (1)
T(76-80)internal/storage/worker_bucket_cleanup.go (2)
NewBucketCleanupRunner(116-129)BucketCleanupRunnerConfig(18-21)internal/storage/system/store.go (2)
New(205-215)DefaultStore(47-50)
internal/api/v2/controllers_buckets_restore_test.go (1)
internal/api/v2/routes.go (1)
NewRouter(29-133)
internal/api/v2/controllers_buckets_delete_test.go (1)
internal/api/v2/routes.go (1)
NewRouter(29-133)
cmd/worker.go (1)
internal/storage/worker_bucket_cleanup.go (1)
BucketCleanupRunnerConfig(18-21)
internal/api/common/mocks_system_controller_test.go (2)
internal/api/v1/mocks_system_controller_test.go (2)
SystemController(207-211)SystemControllerMockRecorder(214-216)internal/api/v2/mocks_system_controller_test.go (2)
SystemController(207-211)SystemControllerMockRecorder(214-216)
test/e2e/api_buckets_delete_test.go (6)
test/e2e/suite_test.go (1)
UseTemplatedDatabase(202-204)internal/ledger.go (1)
Ledger(18-27)pkg/client/models/operations/v2createledger.go (1)
V2CreateLedgerRequest(9-13)pkg/client/models/operations/v2listledgers.go (1)
V2ListLedgersRequest(10-28)pkg/client/models/operations/v2deletebucket.go (1)
V2DeleteBucketRequest(9-12)pkg/client/models/operations/v2restorebucket.go (1)
V2RestoreBucketRequest(9-12)
pkg/client/v2.go (9)
pkg/client/models/operations/v2deletebucket.go (2)
V2DeleteBucketRequest(9-12)V2DeleteBucketResponse(21-25)pkg/client/models/operations/options.go (4)
Option(41-41)Options(32-39)SupportedOptionRetries(15-15)SupportedOptionTimeout(16-16)pkg/client/internal/utils/utils.go (3)
ReplaceParameters(51-60)ConsumeRawBody(350-360)UnmarshalJsonFromResponseBody(39-49)pkg/client/internal/utils/pathparams.go (1)
GenerateURL(19-38)pkg/client/internal/hooks/hooks.go (1)
HookContext(27-35)pkg/client/internal/config/sdkconfiguration.go (1)
SDKConfiguration(16-25)pkg/client/internal/utils/security.go (1)
PopulateSecurity(27-92)pkg/client/internal/utils/retries.go (3)
RetryConfig(23-23)Retries(25-28)Retry(30-104)pkg/client/models/operations/v2restorebucket.go (2)
V2RestoreBucketRequest(9-12)V2RestoreBucketResponse(21-25)
internal/controller/system/controller.go (1)
internal/tracing/tracing.go (3)
SkipResult(155-157)Trace(130-147)NoResult(149-153)
internal/storage/worker_bucket_cleanup.go (1)
internal/storage/system/store.go (2)
New(205-215)Option(217-217)
internal/storage/system/store.go (1)
internal/ledger.go (1)
Ledger(18-27)
🪛 markdownlint-cli2 (0.18.1)
docs/api/README.md
2453-2453: Blank line inside blockquote
(MD028, no-blanks-blockquote)
2501-2501: Blank line inside blockquote
(MD028, no-blanks-blockquote)
pkg/client/docs/sdks/v2/README.md
1895-1895: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1896-1896: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1897-1897: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1898-1898: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1899-1899: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1900-1900: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1954-1954: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1955-1955: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1956-1956: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1957-1957: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1958-1958: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1959-1959: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (57)
internal/README.md (2)
1-1: This is auto-generated documentation; verify regeneration.This file is generated by gomarkdoc and should be automatically updated when source code changes. Ensure the file was regenerated after all code changes in this PR to prevent stale links and inconsistent documentation.
651-651: DeletedAt field is correctly documented.The new
DeletedAt *time.Timefield with tagsjson:"deletedAt,omitempty"andbun:"deleted_at,type:timestamp,nullzero"is properly represented in the documentation. The nullable timestamp and JSON omit-when-empty semantics are correctly captured.Also applies to: 664-664
pkg/client/docs/models/operations/v2deletebucketrequest.md (1)
1-8: LGTM!The documentation clearly describes the V2DeleteBucketRequest model with the required Bucket field. The format is consistent with other operation documentation in the SDK.
pkg/client/README.md (1)
159-160: LGTM!The documentation correctly lists the new DeleteBucket and RestoreBucket operations in the V2 API section. The placement and format are consistent with the existing operations documentation.
pkg/client/models/components/v2ledger.go (2)
14-14: LGTM!The DeletedAt field is correctly added as an optional pointer type with proper JSON tagging. This supports the soft-delete semantics described in the PR objectives.
52-57: LGTM!The GetDeletedAt accessor correctly implements the nil-safe pattern consistent with other optional field getters in this struct.
pkg/client/models/operations/v2deletebucket.go (2)
9-19: LGTM!The V2DeleteBucketRequest correctly defines the bucket path parameter with proper tagging and a nil-safe accessor.
21-39: LGTM!The V2DeleteBucketResponse correctly structures the response with HTTP metadata and optional error response, with nil-safe accessors following SDK conventions.
pkg/testserver/worker.go (2)
29-35: LGTM!The BucketCleanupRetentionPeriodInstrumentation function correctly appends the retention period flag using the time.Duration's String() method. This follows the same pattern as the existing instrumentation functions.
37-43: LGTM!The BucketCleanupCRONSpecInstrumentation function correctly appends the CRON schedule flag for the bucket cleanup worker. The implementation is consistent with the existing instrumentation helpers.
pkg/client/docs/models/operations/v2restorebucketrequest.md (1)
1-8: LGTM!The documentation clearly describes the V2RestoreBucketRequest model with the required Bucket field, following the same format as other operation documentation.
pkg/client/docs/models/operations/v2deletebucketresponse.md (1)
1-9: LGTM!The documentation correctly describes the V2DeleteBucketResponse model, including the required HTTPMeta field and the optional V2ErrorResponse for error cases. The "Bucket not found" description appropriately documents the error scenario.
internal/ledger.go (1)
26-26: LGTM!The DeletedAt field is correctly implemented as a nullable timestamp to support soft-delete semantics. The field is properly tagged:
- JSON tag uses
omitemptyso null values are excluded from responses- Bun tag maps to
deleted_atdatabase column withnullzerohandlingThis aligns with the PR's soft-delete implementation where new ledgers default to
deleted_at = NULL.pkg/client/docs/models/operations/v2listledgersrequest.md (1)
10-10: Doc entry aligns with includeDeleted feature.
Description and default both mirror the API contract, so clients get clear guidance.internal/api/v2/routes.go (2)
21-28: Router comment adds valuable context.
Expanded docstring cleanly outlines the router surface—nice clarity boost.
56-59: Bucket lifecycle endpoints correctly mounted.
Auth-protected/bucketsroutes now delegate to the new handlers, matching the soft-delete flow.internal/storage/system/migrations.go (2)
14-28: Helpful GetMigrator documentation.
The clarifying comment concisely explains the migration wiring—appreciated.
284-294: Migration covers deleted_at schema update.
Idempotent column addition ensures ledger rows gain the timestamp needed for retention checks without disrupting existing data.internal/storage/system/queries.go (1)
4-6: Query payload toggle looks right.
IncludeDeleteddefaults to false, keeping legacy filtering while enabling opt-in access.internal/controller/system/store.go (1)
18-19: Store interface exposes bucket lifecycle hooks.
These additions let consumers trigger soft delete/restore through the controller cleanly.internal/api/v2/controllers_ledgers_list.go (1)
15-46: LGTM! Well-implemented includeDeleted parameter.The callback pattern cleanly extends the pagination query setup to support the new
includeDeletedparameter. The implementation correctly extracts the boolean query parameter and populates the resource query options.pkg/client/docs/models/operations/v2restorebucketresponse.md (1)
1-9: Documentation looks good.The model fields are clearly documented with appropriate types and requirements.
pkg/client/docs/models/components/v2ledger.md (1)
6-14: LGTM! DeletedAt field correctly documented.The new
DeletedAtfield is properly documented as an optional timestamp, which aligns with the soft-delete functionality.pkg/client/docs/sdks/v2/README.md (2)
37-38: LGTM! Operations list updated correctly.The new DeleteBucket and RestoreBucket operations are properly added to the Available Operations section.
1885-2002: Documentation is comprehensive and accurate.The DeleteBucket and RestoreBucket operation documentation is complete with:
- Clear descriptions of soft-delete behavior
- Proper example usage
- Correct parameter and response documentation
Note: Static analysis detected hard tabs in the code examples. While this doesn't affect functionality, consider using spaces for consistency with markdown best practices.
internal/worker/module.go (1)
22-38: LGTM! Bucket cleanup module correctly integrated.The
BucketCleanupRunnerConfigis properly added toModuleConfigand wired into the FX module lifecycle alongside existing workers. The implementation follows the established pattern for module composition.pkg/client/models/operations/v2listledgers.go (1)
20-60: LGTM! Generated code correctly adds includeDeleted parameter.The
IncludeDeletedfield is properly defined with:
- Appropriate default value (false)
- Correct query parameter metadata
- JSON marshaling support
- Standard getter method
Note: This is generated code (Speakeasy) as indicated by the file header.
internal/storage/system/resource_ledgers.go (1)
31-41: LGTM! Clean implementation of soft-delete filtering.The conditional filtering correctly excludes deleted ledgers by default (when
IncludeDeletedis false), while allowing them to be included when explicitly requested. This aligns well with the new bucket lifecycle management feature.internal/controller/system/controller.go (2)
53-54: LGTM! Interface extension follows established patterns.The new bucket management methods are well-integrated into the Controller interface.
227-237: LGTM! Implementations are consistent with existing methods.Both
DeleteBucketandRestoreBucketcorrectly delegate to the system store with proper tracing instrumentation, following the same pattern used throughout the controller.pkg/client/models/operations/v2restorebucket.go (1)
1-39: Generated SDK code looks standard.This is Speakeasy-generated code with proper nil-safe accessors and standard request/response structures.
internal/api/v1/mocks_system_controller_test.go (2)
274-286: Generated mock code is consistent.Standard gomock-generated mock for DeleteBucket.
449-461: Generated mock code is consistent.Standard gomock-generated mock for RestoreBucket.
test/e2e/app_bucket_cleanup_test.go (4)
29-46: Appropriate test configuration for fast feedback.The short retention period (2 seconds) and frequent cron schedule (every second) are well-suited for e2e testing, enabling rapid verification of the cleanup workflow.
87-102: LGTM! Proper verification of soft delete behavior.The test correctly verifies that the ledger still exists with
deletedAtset immediately after soft deletion.
179-233: Comprehensive multi-bucket test coverage.Excellent verification that:
- Multiple deleted buckets are cleaned up independently
- Non-deleted buckets remain untouched
- The cleanup worker handles concurrent bucket deletion correctly
104-122: Good use of Eventually for timing-sensitive verification.The 5-second timeout is appropriate for the test conditions (2-second retention period, 1-second worker execution interval) and is consistent with other timing-sensitive e2e tests in the codebase.
cmd/worker.go (4)
32-33: Clear flag constants for bucket cleanup configuration.
51-52: Configuration fields properly added.
135-138: Configuration correctly wired to worker module.The bucket cleanup configuration is properly mapped to
BucketCleanupRunnerConfig.
70-71: Approved—cron schedule verified for hourly execution.The cron expression "0 0 * * * *" (6-field format) runs at 00 seconds, 00 minutes of every hour, confirming the hourly cleanup schedule mentioned in the review. The 30-day retention window and hourly cadence are both sound production defaults.
internal/api/v2/controllers_buckets_restore_test.go (1)
18-70: LGTM! Test structure is clean and follows established patterns.The test covers the nominal case and error handling appropriately for a controller unit test. Edge case tests for non-deleted, non-existent, or hard-deleted buckets would be better suited for E2E tests (which already have coverage in
test/e2e/api_buckets_delete_test.go), rather than adding them to the unit test layer.internal/api/v2/controllers_buckets_delete_test.go (1)
18-70: LGTM! Test properly validates the delete endpoint behavior.The test structure is solid and appropriately focuses on HTTP controller behavior with both nominal and error path validation. Edge case coverage (non-existent/already-deleted buckets) is properly addressed in the e2e test layer at
test/e2e/api_buckets_delete_test.go, which is the correct scope for those integration scenarios.docs/api/README.md (4)
135-153: LGTM: Clear and complete documentation.The
includeDeletedparameter is well documented with clear semantics.
173-173: LGTM: Consistent schema documentation.The
deletedAtfield is properly documented as nullable date-time across all schemas.Also applies to: 231-231, 5385-5385, 5405-5405
2429-2475: LGTM: Clear soft-delete semantics documented.The DELETE bucket endpoint documentation clearly explains the soft-delete behavior and sets appropriate expectations.
2477-2523: LGTM: Clear restore semantics documented.The restore bucket endpoint documentation clearly explains how soft-deleted buckets are restored.
internal/storage/worker_bucket_cleanup.go (3)
35-68: LGTM: Solid lifecycle implementation.The Run/Stop pattern using
chan chan struct{}for graceful shutdown is well-implemented. The method correctly handles timer recalculation and stop signals.Note: If
r.run(ctx)panics, the runner will terminate without processing the stop signal. Consider whether panic recovery is needed based on your application's requirements.
70-95: LGTM: Robust batch processing.The cleanup logic correctly continues processing remaining buckets even when individual bucket deletion fails. OpenTelemetry instrumentation is appropriate.
97-110: LGTM: Clean implementation.The
processBucketmethod is straightforward with proper error handling and observability.internal/storage/system/store.go (7)
29-32: LGTM: Clean interface additions.The new Store interface methods follow Go conventions with clear semantics for bucket lifecycle management.
56-69: LGTM: Correct filtering of deleted buckets.The addition of
WHERE deleted_at IS NULLproperly excludes soft-deleted buckets from the distinct buckets list.
115-127: LGTM: Correct soft-delete implementation.The
DeleteBucketmethod properly implements soft deletion with appropriate SQL filtering.Note: This method doesn't check whether any rows were affected. If the bucket doesn't exist or is already deleted, it returns success. Verify this is the intended behavior for the delete bucket API endpoint.
129-140: LGTM: Correct restore implementation.The
RestoreBucketmethod properly clears the soft-delete marker.Similar to
DeleteBucket, this doesn't check whether any rows were affected. Confirm this is acceptable—returning success for non-existent buckets may mask errors.
142-155: LGTM: Correct query for finding stale buckets.The method correctly identifies buckets deleted before the specified time using appropriate SQL filtering.
179-191: LGTM: Consistent filtering of deleted ledgers.The addition of
WHERE deleted_at IS NULLensuresGetLedgerrespects soft-delete status.
157-177: ****The code is correct. Bun supports passing
bun.Identas a placeholder argument, and Bun will renderbun.Identas a properly quoted identifier. The implementation at line 161 follows the documented pattern and properly escapes the schema name to prevent SQL injection:_, err := tx.ExecContext(ctx, `DROP SCHEMA IF EXISTS ? CASCADE`, bun.Ident(bucket))This is the safe, recommended approach for handling identifiers in Bun.
Likely an incorrect or invalid review comment.
| type BucketCleanupRunnerConfig struct { | ||
| RetentionPeriod time.Duration | ||
| Schedule cron.Schedule | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify retention period validation.
The RetentionPeriod field has no constraints preventing invalid values (e.g., negative or zero duration). Ensure validation occurs when this config is constructed from CLI flags.
🏁 Script executed:
#!/bin/bash
# Description: Check if retention period is validated during config construction
# Search for where BucketCleanupRunnerConfig is constructed
rg -n "BucketCleanupRunnerConfig\{" --type go
# Search for validation of retention period
rg -n "RetentionPeriod.*[<>]=|if.*RetentionPeriod" --type goLength of output: 403
I need to inspect the construction site more closely to determine if validation occurs upstream.
🏁 Script executed:
#!/bin/bash
# Description: Inspect construction of BucketCleanupRunnerConfig in cmd/worker.go
# View the context around line 135 in cmd/worker.go
sed -n '120,150p' cmd/worker.go
# Search for where the duration value comes from (flag parsing or config loading)
rg -n "retention|Retention" cmd/worker.go -B3 -A3
# Check if there's validation logic on the config or its fields
rg -n "func.*Validate|if.*RetentionPeriod|RetentionPeriod\s*<=" cmd/worker.goLength of output: 3022
🏁 Script executed:
#!/bin/bash
# Description: Inspect how BucketCleanupRunnerConfig is constructed and if RetentionPeriod is validated
# View context around line 135 in cmd/worker.go where BucketCleanupRunnerConfig is constructed
sed -n '110,160p' cmd/worker.go
# Search for where the duration flag/config value comes from
rg -n "retention|Retention" cmd/worker.go -i -B2 -A2
# Check for any validation on the duration value before assignment
rg -n "flag|duration|Duration" cmd/worker.go -B1 -A1 | head -50Length of output: 4775
🏁 Script executed:
#!/bin/bash
# Description: Check if validation occurs in storage package where BucketCleanupRunnerConfig is used
# Search for BucketCleanupRunner or functions that use BucketCleanupRunnerConfig
rg -n "BucketCleanupRunner|func.*Config" internal/storage/ --type go -A5 | grep -A5 "BucketCleanupRunnerConfig"
# Check where RetentionPeriod field is actually used in the runner
rg -n "RetentionPeriod" internal/storage/ --type go -B2 -A2
# Look for validation patterns on duration fields
rg -n "if.*Duration|Duration.*<|Duration.*>" internal/storage/ --type go | head -20Length of output: 5912
Add validation for RetentionPeriod to ensure positive duration.
The field accepts any time.Duration value without constraints. A negative or zero duration would produce incorrect cutoff time calculations in the cleanup logic (line 75). Add validation in NewBucketCleanupRunner (internal/storage/worker_bucket_cleanup.go:116) or during flag parsing in cmd/worker.go to reject non-positive values.
🤖 Prompt for AI Agents
internal/storage/worker_bucket_cleanup.go around lines 18-21 and
NewBucketCleanupRunner at ~116: the BucketCleanupRunnerConfig.RetentionPeriod
currently accepts non-positive durations which will produce incorrect cutoff
calculations; add validation in NewBucketCleanupRunner to return an error if
cfg.RetentionPeriod <= 0 (or alternatively validate during flag parsing),
ensuring callers cannot construct the runner with zero or negative durations;
update the function signature to return (Runner, error) if it doesn't already,
add a clear error message like "RetentionPeriod must be > 0", and add/adjust
unit tests and any call sites (e.g., cmd/worker.go) to handle the new validation
error.
Description
This PR implements a complete bucket lifecycle management system, including soft deletion, restoration, and automatic hard deletion after a retention period. This allows for safe bucket management with the ability to recover accidentally deleted buckets.
Changes
Core Features
1. Soft Deletion of Buckets
DELETE /v2/_/buckets/{bucket}deleted_attimestamp for all ledgers in the bucket2. Bucket Restoration
POST /v2/_/buckets/{bucket}/restoredeleted_attoNULLfor all ledgers in the bucket3. List Deleted Ledgers
GET /v2(List Ledgers)includeDeleted(boolean)true, includes soft-deleted ledgers in the responsefalseor omitted, only returns active ledgers (default behavior)4. Bucket Cleanup Worker
BucketCleanupRunnerthat performs hard deletion after retention period_system.ledgersfor expired buckets--worker-bucket-cleanup-retention-period: Retention period before hard delete (default: 30 days)--worker-bucket-cleanup-schedule: Cron schedule for worker execution (default: hourly)API Enhancements
deletedAtfield to Ledger API responses: TheV2Ledgerschema now includes adeletedAtfield (nullable date-time) to indicate when a ledger was soft-deletedopenapi.yamlandopenapi/v2.yaml)Database Changes
deleted_atcolumn to_system.ledgerstabletimestamp, nullableTesting
Unit Tests
Bucket deletion:
internal/api/v2/controllers_buckets_delete_test.goBucket restoration:
internal/api/v2/controllers_buckets_restore_test.goWorker logic:
internal/storage/worker_bucket_cleanup_test.goIntegration Tests
Bucket lifecycle:
test/e2e/api_buckets_delete_test.goincludeDeletedquery parameter behaviorWorker execution:
test/e2e/app_bucket_cleanup_test.goConfiguration
--worker-bucket-cleanup-retention-period: Retention period before hard delete (default: 30 days)--worker-bucket-cleanup-schedule: Cron schedule for worker execution (default: hourly)Infrastructure
pkg/testserver/worker.gofor test configurationTechnical Details
Soft Delete Flow
DELETE /v2/_/buckets/{bucket}deleted_at = NOW()for all ledgers in the bucketincludeDeleted=true)Restoration Flow
POST /v2/_/buckets/{bucket}/restoredeleted_at = NULLfor all ledgers in the bucketHard Delete Flow (Worker)
deleted_at < (NOW() - retention_period)DROP SCHEMA IF EXISTS {bucket} CASCADE_system.ledgerswherebucket = {bucket}Testing
All tests pass:
Migration Notes
deleted_atcolumn to_system.ledgersdeleted_at = NULL(not deleted)