cloud: fix purger garbage leak and move file-number-guard writer in-repo - #18
Conversation
Production buckets accumulated ~150k objects against ~2k live files
(purge cycles reporting total_files=152889, live_files=2122,
obsolete_selected=26). Root cause: the purger's SST deletion gate is
keyed by the epoch embedded in each object name, but thresholds are
only loaded for the CURRENT epoch of each loaded CLOUDMANIFEST. Any
non-live SST from a past epoch missed the threshold map and was
skipped forever ("purge is blocked intentionally"). Since every
reopen/failover rolls a new epoch and live files keep their
creation-epoch names, each restart converted the entire surviving
working set into permanently unreclaimable garbage once compaction
rewrote it. Only files created AND obsoleted within the current epoch
were ever collected.
Purger fixes
------------
* Dead-epoch reclamation: a non-live SST whose epoch is no loaded
CLOUDMANIFEST's current epoch has no possible writer; delete it once
its S3 mtime is older than dead_epoch_file_age_ms (default 1h). The
age guard covers a node mid-open whose files were uploaded before
the purger's listing but whose CLOUDMANIFEST landed after it. The
same guard was added to MANIFEST selection, which previously had
none despite MANIFEST-<epoch> being uploaded BEFORE the
CLOUDMANIFEST that makes the epoch current.
* CLOUDMANIFEST retention now measures from supersession (the
successor max-term CLOUDMANIFEST's own mtime, written exactly once
at generation start) instead of the old generation's MANIFEST mtime
(a last-write time). A read-only old primary, write-idle for hours,
previously looked expired the instant it was superseded and lost
live-file/threshold protection with no grace period.
* smallest_new_file_number marker fallback narrowed to IsNotFound
(fresh branch, no writer). Any other read error aborts the cycle:
the MANIFEST-derived max_file_number is a HIGH watermark of
allocated numbers and can exceed a long-running compaction's
uncommitted outputs, so silently substituting it risked deleting an
in-flight upload.
* Dead-epoch smallest_new_file_number-<epoch> markers are now
collected (previously leaked one object per epoch forever).
* Deletion is batched via a new CloudStorageProvider::
DeleteCloudObjects (S3 DeleteObjects, 1000 keys/request; serial
default for other providers) and capped per cycle
(max_deletions_per_cycle, default 10000) so draining the backlog
cannot monopolize the S3 client.
Guard writer moved into rocksdb-cloud
-------------------------------------
The purger's safety against deleting files uploaded by in-flight
flush/compaction jobs depends on the per-epoch S3 object
smallest_new_file_number-<epoch>. Its writer previously lived in the
embedding application (data_substrate purger_sliding_window /
purger_event_listener); reader and writer now share one repo and one
key-builder (SmallestFileNumberObjectKey), with the key format and
ASCII value encoding unchanged for interop with existing writers.
New design (cloud/file_number_guard.{h,cc}):
* FileNumberSlidingWindow tracks in-flight jobs keyed by
(thread_id, job_id); completed entries linger guard_entry_duration
(default 15s) so the job's MANIFEST update reaches the cloud.
* FileNumberGuardPublisher splits the old single mutex into a state
mutex (window + watermark, microsecond critical sections, no I/O)
and a publish mutex serializing every S3 PUT, with a staleness
re-check after acquisition so concurrent PUTs can never land out of
order and reinstate a higher threshold. The old writer held one
mutex across S3 PUTs, stalling flush/compaction callbacks behind S3
latency for seconds.
* Downward publishes (a job starting below the published watermark)
PUT synchronously before the job proceeds, retry with backoff on
failure, and never advance the last-published value past a failed
PUT. The old writer ignored the PUT status; one transient failure
of the single PUT that lowers the value from UINT64_MAX left MAX in
S3 forever while the purger deleted in-flight uploads.
* The epoch is always read from the cloud manifest at publish time.
The old writer depended on an external SetEpoch call and published
to the malformed key "smallest_new_file_number-" (empty epoch) when
recovery flushes fired before the embedder set it. Empty-epoch
publishes are now structurally impossible and still guarded.
* DBCloudImpl::Open wires everything up when the new option
publish_file_number_guard is set (default false): it writes the 0
sentinel ("purging blocked") for the epoch after the CLOUDMANIFEST
is established and before DB::Open, registers the event listener so
recovery flushes are covered, and schedules the periodic (upward)
publish on CloudScheduler (guard_publish_interval, default 30s).
CloudFileSystem::BlockPurger() republishes the sentinel for use
around leader transfer.
Tests (previously zero coverage of purger or guard):
* file_number_guard_test: window min/linger/expiry semantics, key
format stability, downward trigger condition, sync-point-injected
PUT failures (retry without watermark advance; Stop unblocks),
empty-epoch refusal.
* eloq_purger_test: unit tests for all four purger selectors,
including the dead-epoch and supersession-retention regressions.
* eloq_purger_integration_test (env-gated, S3-compatible endpoint):
purge end-to-end across epoch rotations with full key read-back;
guard end-to-end: sentinel at open, downward publish landing before
the flushed SST appears in the bucket, idle decay to UINT64_MAX,
and a real purge cycle honoring a pinned threshold (file_number >=
threshold never deleted) with the purger reading back the marker
this writer published.
Also: fix CMakeLists source entry (cloud/improved_purger.cc ->
cloud/eloq_purger.cc; cmake configure was broken), and initialize
StderrLogger::log_prefix_len in the no-prefix constructor
(uninitialized read caused std::bad_alloc on first log).
Note: `make shared_lib` (release) requires USE_RTTI=1 due to
pre-existing dynamic_casts in cloud/; debug builds are unaffected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a cloud file-number guard, updates cloud upload and deletion paths, extends Eloq purging with age-based rules and batched deletion, adds database lifecycle callbacks, and registers new unit and integration tests. ChangesCloud guard and purger changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cloud/file_number_guard_test.cc (1)
360-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid fatal
ASSERT_*inside the spawned thread.
ASSERT_OKin a non-main thread only returns from the lambda and does not reliably fail the test. TheStopUnblocksFailingDownwardPublishtest already uses the safer capture-then-assert-after-join()pattern; mirror it here for consistency and correct failure reporting.♻️ Suggested change
- std::thread job([&] { - Status s = pub.OnJobBegin(10, 1, 1); - ASSERT_OK(s); - }); + Status job_status; + std::thread job([&] { job_status = pub.OnJobBegin(10, 1, 1); });Then assert after
job.join():job.join(); + ASSERT_OK(job_status);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/file_number_guard_test.cc` around lines 360 - 363, The `ASSERT_OK` inside the spawned `std::thread job` in this test is unsafe because fatal assertions in a non-main thread may not fail the test reliably. Update the `OnJobBegin` call path to capture the returned `Status` inside the lambda, then perform the assertion after `job.join()` in the same pattern used by `StopUnblocksFailingDownwardPublish`, so failure reporting is consistent and correct.cloud/eloq_purger_command.cc (1)
229-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the new flags to the usage help text.
The
--dead_epoch_file_age_msand--max_deletions_per_cycleflags are not listed in the CLI usage output (lines 229-234). Users discovering the tool via--helpor error output won't know these options exist.📝 Suggested addition
std::cerr << " --cloudmanifest_retention_ms=3600000 CLOUDMANIFEST retention time in milliseconds\n"; + std::cerr << " --dead_epoch_file_age_ms=3600000 Minimum age (ms) before deleting dead-epoch files\n"; + std::cerr << " --max_deletions_per_cycle=10000 Max deletions per cycle (0 = unlimited)\n"; return 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/eloq_purger_command.cc` around lines 229 - 234, The usage/help output in the eloq purger command is missing the new CLI flags, so update the help text printed in the command’s usage path to include both --dead_epoch_file_age_ms and --max_deletions_per_cycle alongside the existing option list. Make the change in the help/usage block for the purger command so users invoking the command with --help or invalid args can discover these flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cloud/eloq_purger_command.cc`:
- Around line 229-234: The usage/help output in the eloq purger command is
missing the new CLI flags, so update the help text printed in the command’s
usage path to include both --dead_epoch_file_age_ms and
--max_deletions_per_cycle alongside the existing option list. Make the change in
the help/usage block for the purger command so users invoking the command with
--help or invalid args can discover these flags.
In `@cloud/file_number_guard_test.cc`:
- Around line 360-363: The `ASSERT_OK` inside the spawned `std::thread job` in
this test is unsafe because fatal assertions in a non-main thread may not fail
the test reliably. Update the `OnJobBegin` call path to capture the returned
`Status` inside the lambda, then perform the assertion after `job.join()` in the
same pattern used by `StopUnblocksFailingDownwardPublish`, so failure reporting
is consistent and correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61d62052-0c7b-42c7-85b3-ecd78c1d6ba2
📒 Files selected for processing (20)
CMakeLists.txtMakefilecloud/aws/aws_s3.cccloud/cloud_file_system.cccloud/cloud_file_system_impl.cccloud/cloud_storage_provider.cccloud/db_cloud_impl.cccloud/eloq_purger.cccloud/eloq_purger.hcloud/eloq_purger_command.cccloud/eloq_purger_integration_test.cccloud/eloq_purger_test.cccloud/file_number_guard.cccloud/file_number_guard.hcloud/file_number_guard_test.ccinclude/rocksdb/cloud/cloud_file_system.hinclude/rocksdb/cloud/cloud_file_system_impl.hinclude/rocksdb/cloud/cloud_storage_provider.hsrc.mkutil/stderr_logger.h
liunyl
left a comment
liunyl
left a comment
There was a problem hiding this comment.
Review summary
Traced the full change (clone of the PR head, cross-checked against RocksDB flush/compaction internals) across five aspects: correctness/concurrency, error handling, tests, type design, and comments.
Verdict: request changes. The purge deletion-direction safety holds up — I could not find a path that deletes a live or in-flight file in steady state, and the subtle arguments (supersession-based retention, publish_mutex_ + post-acquire staleness re-check, narrowed read-error fallback) check out. The problems are two Critical issues in the new file_number_guard (one reintroduces the exact leak class this PR fixes; one is a mislabeled invariant that invites a data-loss regression) and one Important shutdown/outage hazard. See the inline comments for C1/C2/I1 and two suggestions.
Additional suggestions (not inlined)
DeleteObsoleteFilesswallows delete failures into a log line, returns success, and logs the summary atDEBUG— a persistently stuck key is retried forever with no distinct signal. Idempotent by design, so this is an observability gap, not a silent failure.cloud/eloq_purger.ccthreshold == 0("purging blocked") sentinel is consumed with no comment and, uniquely, no log atcloud/eloq_purger.cc:474— a maintainer refactoring that branch won't see that0is load-bearing. Add a comment + anelsekeep-log mirroring the sibling branches.- Encapsulation: the four
SelectObsolete*methods were madepublicpurely for tests — adetail-namespace free function or afriendtest keeps the (excellent, exact-oracle) unit tests without growingEloqPurger's API.SmallestFileNumber()is a mutating query with aconst-looking name (it erases expired entries). The publisher's rawCloudFileSystemImpl*back-pointer + dual ownership (atomicshared_ptrmember and the DB listener list) should be a documented must-outlive invariant or aweak_ptr. - "CLOUDMANIFEST written exactly once" is true by convention (roll-only-at-open), not enforcement — worth a note near the epoch-compaction TODO, since a future rewrite would only lengthen retention (safe direction) but breaks the stated premise.
Test gaps that matter (the confirmed bugs live in untested areas)
The selectors and publish state machine have strong, deterministic, exact-oracle unit tests. But: (1) the never-completed window entry (C1) is exactly the state the window test never exercises; (2) publish concurrency (the load-bearing periodic-vs-downward staleness re-check) is never run multi-threaded; (3) ReadSmallestFileNumber failure semantics, the entire deletion mechanism, bulk-delete counting/batching (>1000 keys), the deletion cap, and the mtime > now clock-skew branch in all four selectors are covered only by the env-gated integration test, which ROCKSDB_GTEST_SKIPs without ELOQ_PURGER_TEST_S3_ENDPOINT — i.e. it won't run in normal CI. Those are pure logic and cheap to cover with fakes.
Strengths
publish_mutex_ serialization + recomputing the window min after acquiring it correctly prevents a stale higher value from overwriting a fresh downward one; downward PUT failure never advances last_published_. Supersession-based CLOUDMANIFEST retention gives write-idle generations a real grace period. Narrowing the read-error fallback to IsNotFound closes a genuine delete-in-flight hole. SmallestFileNumberObjectKey as a single source of truth for the wire format, clock injection throughout, type-safe chrono options, and the stderr_logger uninitialized-read fix are all good. Comments are dense and, C1/C2 aside, largely excellent.
liunyl
left a comment
liunyl
left a comment
There was a problem hiding this comment.
Simplification pass (quality only — no correctness bugs)
A second review focused purely on reuse / simplification / efficiency / altitude, separate from the correctness findings above. Four independent passes; the notable convergence is that three of them flagged the same duplicated age predicate. Net new suggestions are inline. Efficiency came back clean — the diff is actually a net win there (S3 clock read once per cycle instead of per-selector; batched DeleteObjects replacing per-file deletes). Confirmed clean and not worth changing: the DeleteCloudObjects base/override split, the two-pass DeleteObsoleteFiles (forced by the batch API), the O(n) in-memory window scan, and the four once-per-cycle passes over all_files; improved_purger.cc is fully removed with no dangling refs.
(One low-value nitpick was left off: PeriodicPublish writes desired == last_published_ && !sentinel_dirty_ twice — a tiny AlreadyPublished() helper would keep the two copies of that concurrency condition from drifting, but it's cosmetic.)
|
Review resolution summary Pushed 13 follow-up commits through c3e6269. What changed:
CodeRabbit body comments:
Additional review notes:
Verification on the final pushed tree:
Environment limitations:
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cloud/file_number_guard_test.cc (1)
860-875: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the scheduler actually publishes.
The synchronous protection PUT means this test passes even if periodic work never runs.
ASSERT_OK(pub->ProtectFileUpload(7)); pub->OnJobEnd(1, 1); + const int puts_before_periodic = provider_->PutCount(); std::this_thread::sleep_for(std::chrono::milliseconds(100)); + ASSERT_GT(provider_->PutCount(), puts_before_periodic); + ASSERT_EQ(provider_->Content(GuardKey()), std::to_string(kMax)); pub->Stop();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/file_number_guard_test.cc` around lines 860 - 875, Update StartStopSchedulerSmoke to verify that the recurring scheduler performs a publish independently of the synchronous ProtectFileUpload call. Capture the provider PUT count after the initial protection, wait for the scheduled interval, and assert the count increases before calling Stop; retain the existing post-Stop assertion that no further publishes occur.cloud/file_number_guard.cc (1)
324-355: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent a periodic tick from immediately undoing
BlockPurger().A tick concurrent with
BlockPurger()can acquirepublish_mutex_immediately afterward and replace sentinel0with the current watermark orUINT64_MAX. The leader-transfer block can therefore last only one PUT. Preserve the sentinel until an explicit unblock or at least a full publish interval.Also applies to: 375-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/file_number_guard.cc` around lines 324 - 355, Update FileNumberGuardPublisher::PeriodicPublish so a tick cannot overwrite the sentinel written by BlockPurger immediately after it succeeds. Synchronize the periodic publish decision with sentinel_active_ under the existing state/publish locking, and skip or defer watermark/UINT64_MAX publication while the sentinel remains active until explicit unblock or a full publish interval has elapsed. Preserve normal publishing once the sentinel is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud/cloud_storage_provider.cc`:
- Around line 179-198: The upload path around GetFileNumberGuardPublisher must
fail closed when file-number guarding is enabled but the publisher is
unavailable. Detect the enabled-guard configuration together with a null
cfs_impl or publisher, return an error, and do not call CopyLocalFileToDest
unguarded; preserve guarded protection for parsed table files and existing
behavior when guarding is disabled.
In `@cloud/eloq_purger_test.cc`:
- Around line 466-477: The test only validates S3FileNumberReader and never
exercises the purger’s deletion path. Update GuardReadIoErrorFailsClosed to add
an eligible SST to the provider, invoke RunSinglePurgeCycle(), assert that the
cycle fails, and verify provider->delete_attempts() remains empty while
preserving the injected guard read error.
In `@cloud/file_number_guard_test.cc`:
- Around line 760-765: Update the assertions in the installation test after
first_install and second_install complete to verify the provider recorded two
writes, using the provider’s existing write-history or operation-count API,
while retaining the sentinel content and guard assertions.
- Around line 466-470: Update the synchronization block around PeriodicPublish
to wait on block_cv with kAsyncWaitTimeout instead of waiting indefinitely;
ensure the callback is released and the periodic thread is joined before
asserting that upward_put_reached occurred, while preserving the existing
success path.
- Around line 1059-1075: Rename MalformedEpochSstUploadFailsClosed to reflect
that the invalid field is the file number, not the valid epoch. Keep the
existing assertions and setup unchanged; add a separate malformed-epoch test
only if epoch parsing coverage is also required.
---
Outside diff comments:
In `@cloud/file_number_guard_test.cc`:
- Around line 860-875: Update StartStopSchedulerSmoke to verify that the
recurring scheduler performs a publish independently of the synchronous
ProtectFileUpload call. Capture the provider PUT count after the initial
protection, wait for the scheduled interval, and assert the count increases
before calling Stop; retain the existing post-Stop assertion that no further
publishes occur.
In `@cloud/file_number_guard.cc`:
- Around line 324-355: Update FileNumberGuardPublisher::PeriodicPublish so a
tick cannot overwrite the sentinel written by BlockPurger immediately after it
succeeds. Synchronize the periodic publish decision with sentinel_active_ under
the existing state/publish locking, and skip or defer watermark/UINT64_MAX
publication while the sentinel remains active until explicit unblock or a full
publish interval has elapsed. Preserve normal publishing once the sentinel is
cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58efe803-bbd3-4ee7-b8c9-a5dced0180b6
📒 Files selected for processing (28)
cloud/cloud_file_system.cccloud/cloud_file_system_impl.cccloud/cloud_storage_provider.cccloud/db_cloud_impl.cccloud/db_cloud_impl.hcloud/eloq_purger.cccloud/eloq_purger.hcloud/eloq_purger_command.cccloud/eloq_purger_integration_test.cccloud/eloq_purger_test.cccloud/file_number_guard.cccloud/file_number_guard.hcloud/file_number_guard_test.cccloud/gcp/gcp_cs.ccdb/builder.ccdb/db_compaction_filter_test.ccdb/db_flush_test.ccdb/db_impl/db_impl.ccdb/db_impl/db_impl.hdb/db_impl/db_impl_compaction_flush.ccdb/external_sst_file_test.ccdb/flush_job.hdb/import_column_family_test.ccfile/file_util.ccinclude/rocksdb/cloud/cloud_file_system.hinclude/rocksdb/cloud/cloud_file_system_impl.hinclude/rocksdb/cloud/cloud_storage_provider.hinclude/rocksdb/listener.h
🚧 Files skipped from review as they are similar to previous changes (8)
- include/rocksdb/cloud/cloud_storage_provider.h
- cloud/cloud_file_system.cc
- cloud/eloq_purger_command.cc
- include/rocksdb/cloud/cloud_file_system.h
- cloud/db_cloud_impl.cc
- cloud/eloq_purger.h
- cloud/eloq_purger_integration_test.cc
- cloud/eloq_purger.cc
|
Follow-up on the two CodeRabbit comments that could not be posted inline:
The final tree was re-reviewed through Claude Gate #3 after 452a1a3: PASS, no blocking findings. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
MinIO/AWS verification completed on commit 838ee65. The AWS CMake build first exposed a pre-existing source-list mismatch: cloud/cloud_file_cache.cc was present in src.mk but missing from CMakeLists.txt, leaving CloudFileSystemImpl::FileCache* undefined at link time. Commit 838ee65 adds that single missing source. Fresh verification with AWS SDK 1.11.446 and local MinIO: file_number_guard_test 31/31, eloq_purger_test 12/12, and eloq_purger_integration_test 2/2. The integration test reclaimed objects 11 -> 4, SSTs 6 -> 1, manifests 3 -> 1, then reopened and verified all data; FileNumberGuardEndToEnd also passed. |
|
Local red/green reproduction against MinIO confirms the original dead-epoch leak is fixed. I built an isolated worktree from current HEAD and changed only SelectObsoleteSSTFilesWithThreshold missing-threshold branch back to the pre-fix behavior. RED: DeadEpochSstReclaimedOnceOldEnough returned an empty obsolete list instead of 000200.sst-epochDead; PurgeEndToEnd failed after two cycles with objects 11 -> 9, SSTs 6 -> 6, manifests 3 -> 1. GREEN on unmodified commit 838ee65: the unit regression passed, and the identical MinIO scenario passed with objects 11 -> 4, SSTs 6 -> 1, manifests 3 -> 1, followed by full data read-back. No additional test was added because these existing unit and integration regressions already fail on the exact legacy branch and pass on the fix. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
A review of the file number guard protocol found four ways the purger could delete live data by trusting input it should have rejected. All four now fail closed, and DBCloud::Open refuses configurations the protocol cannot protect. Strict parsing of numeric control objects ----------------------------------------- std::stoull skips leading whitespace, accepts a sign, and ignores trailing garbage. For the guard marker that is destructive: "-1" parses as UINT64_MAX, a threshold that authorizes deleting every non-live SST of a *live* epoch. For CLOUDMANIFEST names, "9999x" parses as 9999 and could make a bogus object the group's max term, retiring the authoritative CLOUDMANIFEST. Both sites now require a non-empty run of ASCII digits with full consumption and no overflow (one trailing newline tolerated, since writers commonly append one), and the marker read is bounded rather than slurping the object into a string. Any malformed control object aborts the whole cycle. Previously an unparseable CLOUDMANIFEST term was silently skipped, which hid an object whose lineage cannot be reasoned about. Missing marker no longer falls back to an unsafe threshold ---------------------------------------------------------- On NotFound the purger substituted the MANIFEST-derived max_file_number. That is a HIGH watermark of allocated numbers and can exceed a long-running compaction's uncommitted outputs, so an active unguarded writer could have an in-flight upload deleted. Marker absence is now an error and the cycle aborts (--require_guard_marker, default true, restores the old behavior for a staged rollout). The other half of that protocol is enforced at the writer: DBCloud::Open rejects a writable DB when cloud file deletion is delegated to the purger (disable_cloud_file_deletion or run_purger) but publish_file_number_guard is not set. Read-only opens create no SSTs and are exempt. Absolute-consistency MANIFEST scans for the purger only ------------------------------------------------------ The live-file reader used log::Reader's default kTolerateCorruptedTailRecords. That is right for DB open -- a torn tail is skipped so recovery proceeds -- and wrong for a scanner that deletes: a skipped tail containing a file addition makes a live SST look unreferenced. ManifestReader now takes a WALRecoveryMode (defaulting to the tolerant mode) and the purger's two call sites pass kAbsoluteConsistency. The three open-path callers (RollNewEpoch, RollNewBranch, GetMaxFileNumberFromCurrentManifest) are unchanged, so DB open behavior is identical. disable_manifest_sync rejected under the guard ---------------------------------------------- The guard's safety argument is that a completed flush/compaction implies its MANIFEST is durable in the cloud. The MANIFEST only reaches cloud storage from CloudStorageWritableFileImpl::Sync(), which disable_manifest_sync skips -- the guard would then rise to UINT64_MAX while the cloud MANIFEST still omits committed files, and the purger would delete them. DBCloud::Open now rejects the combination. (This also closes the only path that could publish a MANIFEST object mid-batch, so a partial atomic VersionEdit group cannot be observed by the scanner.) Both Open-time checks run before any cloud I/O so misconfiguration fails fast rather than after a sanitize/fetch round trip. Also in this change ------------------- * RepairDB refuses to run against a CloudFileSystem with publish_file_number_guard set. Repair rebuilds SSTs through BuildTable, but the guard listener is registered by DBCloud::Open on its own options copy, so a standalone RepairDB would upload unprotected files. Detection uses Customizable::CheckedCast rather than dynamic_cast so no RTTI dependency is added to a core file; that required kClassName() on CloudFileSystem. * Files written outside the DB's own SST directories (Checkpoint copies, column family exports) are exempt from the upload gate. They are not DB-visible SSTs, no flush/compaction job registers them, and gating them made Checkpoint::CreateCheckpoint fail outright -- LinkFile is NotSupported for bucket-backed file systems, so checkpoint always falls back to copying through the cloud FS. The publisher now carries the DB directory plus any configured db_paths/cf_paths; an empty list gates everything. Tests ----- * Strict parse: rejects -1, +1, leading/trailing space, trailing garbage, hex, empty, overflow and oversized objects; accepts plain digits, trailing newline, and UINT64_MAX. * Missing marker aborts the cycle with no deletion attempts. * Malformed CLOUDMANIFEST term aborts the cycle. * Torn MANIFEST tail: tolerant mode returns the value, strict mode errors. * Open rejects a purged bucket without the guard, and disable_manifest_sync with the guard; neither check fires when it should not (read-only open, guard disabled). * Checkpoint produces identical results with the guard on and off. Note this asserts guard-neutrality only: a cloud checkpoint contains no SST files either way, which is pre-existing and out of scope. * PurgeEndToEnd now enables the guard, since purging a bucket whose writers publish no markers is no longer a supported configuration. Verified: 36 guard unit, 23 purger unit, 17 core repair, 4 Minio integration. Rollout note: enabling --require_guard_marker requires every writable primary to be running with publish_file_number_guard first. A guarded DB writes its epoch's 0 sentinel before DB::Open returns, so any epoch created after the upgrade always has a marker; until then the purger aborts its cycles, which is harmless (it deletes nothing) and self-resolves once primaries restart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cloud/eloq_purger_test.cc (1)
561-562: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNew test code discards
StatusandIOStatusreturn values. The shared root cause is that these calls drop a returned status. Two effects follow: builds that defineROCKSDB_ASSERT_STATUS_CHECKEDabort in theStatusdestructor, and in the selector case the test no longer proves the call succeeded.
cloud/eloq_purger_test.cc#L561-L562: wrap theSelectObsoleteCloudManifestFilescall inASSERT_OK(...). Apply the same change at the two other call sites in this file (lines 573-575 and 627-628).cloud/eloq_purger_integration_test.cc#L806-L810: replaceDeleteDirwith a call that removes a non-empty directory, and mark both cleanup statuses withPermitUncheckedError().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/eloq_purger_test.cc` around lines 561 - 562, Update cloud/eloq_purger_test.cc at lines 561-562, 573-575, and 627-628 to wrap each SelectObsoleteCloudManifestFiles call in ASSERT_OK so the returned Status is checked. In cloud/eloq_purger_integration_test.cc at lines 806-810, replace DeleteDir with the non-empty-directory removal operation and call PermitUncheckedError() on both cleanup statuses.cloud/eloq_purger_command.cc (1)
239-242: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
--require_guard_markerin the usage output.The usage block lists the two other new flags but omits
--require_guard_marker. That flag is the one an operator must find during a staged rollout, because disabling it is what lets a cycle proceed while some writers do not yet publish the marker.📝 Proposed addition
std::cerr << " --max_deletions_per_cycle=10000 Maximum objects " "deleted per cycle; 0 means unlimited\n"; + std::cerr << " --require_guard_marker=true Abort the cycle " + "when a live epoch has no guard marker\n"; return 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/eloq_purger_command.cc` around lines 239 - 242, Update the usage output near the existing dead-epoch and deletion options to document the --require_guard_marker flag, including its purpose and the behavior when disabled during staged rollout. Preserve the formatting and ordering conventions used by the surrounding help entries.
🧹 Nitpick comments (4)
cloud/eloq_purger.cc (1)
593-618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider lowering the per-object "keeping" logs to DEBUG level.
Both selectors emit one INFO line per candidate object, including every object that is kept. On a bucket with a large backlog (the PR description mentions a 150k-object drain), each cycle writes one INFO line per object. The delete decisions are the actionable events; the keep decisions are diagnostic.
Change the keep branches to
InfoLogLevel::DEBUG_LEVELand keep the delete branches at INFO.Also applies to: 641-665
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/eloq_purger.cc` around lines 593 - 618, Lower the per-object keep log statements in both selector branches, including the branch around the shown dead-epoch handling and the corresponding block around lines 641-665, from InfoLogLevel::INFO_LEVEL to InfoLogLevel::DEBUG_LEVEL. Keep the obsolete-file deletion logs at INFO_LEVEL.cloud/eloq_purger_integration_test.cc (1)
806-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHandle the statuses returned by
DeleteDirandEmptyBucket.Both calls discard a returned status.
DeleteDiralso fails whenever the checkpoint directory is not empty, which is the normal case here, so the directory survives the test. UseDestroyDir(or delete the children first) and mark the cleanup statuses as unchecked, matching thePermitUncheckedError()already used onGetChildren.This shares a root cause with the discarded
Statusreturns incloud/eloq_purger_test.cc; see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/eloq_purger_integration_test.cc` around lines 806 - 810, Update the cleanup lambda after deleting db to use DestroyDir for checkpoint_dir instead of DeleteDir, and explicitly mark the returned statuses from DestroyDir and EmptyBucket as unchecked via PermitUncheckedError(), consistent with the existing GetChildren cleanup handling.cloud/eloq_purger_test.cc (1)
73-81: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
DoCloudReadagainst an offset past the end of the contents.
availableis computed ascontents_.size() - offsetwith unsigned arithmetic. Ifoffsetever exceedscontents_.size(), the subtraction wraps andmemcpycopies a very large length. The current callers read sequentially within the declared file size, so this is not reachable today. A one-line clamp keeps a future truncated-size test from crashing instead of failing.🛡️ Proposed guard
IOStatus DoCloudRead(uint64_t offset, size_t n, const IOOptions &, char *scratch, uint64_t *bytes_read, IODebugContext *) const override { - const size_t available = contents_.size() - static_cast<size_t>(offset); + if (offset >= contents_.size()) { + *bytes_read = 0; + return IOStatus::OK(); + } + const size_t available = contents_.size() - static_cast<size_t>(offset); const size_t to_read = std::min(n, available);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/eloq_purger_test.cc` around lines 73 - 81, Update DoCloudRead to clamp the requested offset against contents_.size() before computing available, so offsets past the end produce zero bytes without unsigned underflow or an invalid memcpy. Preserve the existing bounded read behavior for valid offsets.cloud/file_number_guard_test.cc (1)
1196-1203: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDelete the database handle after the trailing opens.
Both tests reuse
dbfor a final open that is expected not to hit the guard check, then assert only on the status message. Neither test deletesdbafterwards. Today theRecordingStorageProviderreturnsNotSupportedfor every read operation, so those opens cannot succeed and no handle is produced. The tests therefore depend on that provider behavior for their cleanup.Add
delete db;after each trailing open so the tests stay correct if the fixture ever gains a provider that can complete an open. A liveDBCloudoutliving the fixture would hold theCloudFileSystemImplthat the fixture destroys.🧹 Proposed cleanup (apply in both tests)
Status read_only = DBCloud::Open(options, tmp_dir_ + "/guard_required", "", 0, &db, true); ASSERT_EQ(read_only.ToString().find("publish_file_number_guard"), std::string::npos) << read_only.ToString(); + delete db; + db = nullptr; }Also applies to: 1227-1235
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud/file_number_guard_test.cc` around lines 1196 - 1203, Add delete db; immediately after each trailing DBCloud::Open assertion in both affected tests, including the read-only open near the end of the test and the corresponding open around the second referenced location. Keep the existing status assertions unchanged, and ensure the handle is cleaned up even if a future provider allows the open to succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud/eloq_purger_integration_test.cc`:
- Around line 718-727: In the read-back loop over key(i), replace the fatal
ASSERT_OK and ASSERT_EQ checks with non-fatal EXPECT checks and break on
failure, matching the branch-SST loop pattern. Ensure execution reaches delete
db, provider->EmptyBucket(bucket_, object_path_), and the final
ASSERT_TRUE(all_backup_ssts_survived) cleanup and validation.
In `@cloud/eloq_purger.cc`:
- Around line 839-852: Require RollNewBranch and RollNewCookie to validate that
cookies follow the documented <prefix>-<term> contract, with a numeric terminal
term while preserving leading zeros. Reject invalid cookies with a clear error
before MakeCloudManifestFile creates a CLOUDMANIFEST, and document this API
requirement; keep the purger’s malformed-term corruption handling as a safety
fallback.
---
Outside diff comments:
In `@cloud/eloq_purger_command.cc`:
- Around line 239-242: Update the usage output near the existing dead-epoch and
deletion options to document the --require_guard_marker flag, including its
purpose and the behavior when disabled during staged rollout. Preserve the
formatting and ordering conventions used by the surrounding help entries.
In `@cloud/eloq_purger_test.cc`:
- Around line 561-562: Update cloud/eloq_purger_test.cc at lines 561-562,
573-575, and 627-628 to wrap each SelectObsoleteCloudManifestFiles call in
ASSERT_OK so the returned Status is checked. In
cloud/eloq_purger_integration_test.cc at lines 806-810, replace DeleteDir with
the non-empty-directory removal operation and call PermitUncheckedError() on
both cleanup statuses.
---
Nitpick comments:
In `@cloud/eloq_purger_integration_test.cc`:
- Around line 806-810: Update the cleanup lambda after deleting db to use
DestroyDir for checkpoint_dir instead of DeleteDir, and explicitly mark the
returned statuses from DestroyDir and EmptyBucket as unchecked via
PermitUncheckedError(), consistent with the existing GetChildren cleanup
handling.
In `@cloud/eloq_purger_test.cc`:
- Around line 73-81: Update DoCloudRead to clamp the requested offset against
contents_.size() before computing available, so offsets past the end produce
zero bytes without unsigned underflow or an invalid memcpy. Preserve the
existing bounded read behavior for valid offsets.
In `@cloud/eloq_purger.cc`:
- Around line 593-618: Lower the per-object keep log statements in both selector
branches, including the branch around the shown dead-epoch handling and the
corresponding block around lines 641-665, from InfoLogLevel::INFO_LEVEL to
InfoLogLevel::DEBUG_LEVEL. Keep the obsolete-file deletion logs at INFO_LEVEL.
In `@cloud/file_number_guard_test.cc`:
- Around line 1196-1203: Add delete db; immediately after each trailing
DBCloud::Open assertion in both affected tests, including the read-only open
near the end of the test and the corresponding open around the second referenced
location. Keep the existing status assertions unchanged, and ensure the handle
is cleaned up even if a future provider allows the open to succeed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8acec4ca-bec5-4a3b-bd93-5a83f282aec2
📒 Files selected for processing (15)
.gitignorecloud/cloud_storage_provider.cccloud/db_cloud_impl.cccloud/eloq_purger.cccloud/eloq_purger.hcloud/eloq_purger_command.cccloud/eloq_purger_integration_test.cccloud/eloq_purger_test.cccloud/file_number_guard.cccloud/file_number_guard.hcloud/file_number_guard_test.cccloud/manifest_reader.cccloud/manifest_reader.hdb/repair.ccinclude/rocksdb/cloud/cloud_file_system.h
🚧 Files skipped from review as they are similar to previous changes (2)
- include/rocksdb/cloud/cloud_file_system.h
- cloud/cloud_storage_provider.cc
| // Verify the active database too, then clean up the unique object path even | ||
| // when an EXPECT above detects a missing branch SST. | ||
| std::string value; | ||
| for (int i = 0; i < 200; ++i) { | ||
| ASSERT_OK(db->Get(ReadOptions(), key(i), &value)); | ||
| ASSERT_EQ(value, "generation-3"); | ||
| } | ||
| delete db; | ||
| provider->EmptyBucket(bucket_, object_path_).PermitUncheckedError(); | ||
| ASSERT_TRUE(all_backup_ssts_survived); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use EXPECT_ in the read-back loop so cleanup still runs on failure.
The branch-SST loop above deliberately uses EXPECT_OK and defers the fatal check to ASSERT_TRUE(all_backup_ssts_survived) at the end, so that delete db and EmptyBucket always run. This read-back loop breaks that pattern: ASSERT_OK and ASSERT_EQ return from the test on the first failure, so delete db and EmptyBucket are skipped. The run's unique object path then stays in the shared test bucket, and the DBCloud handle leaks.
Switch to EXPECT_ with a break, matching the loop above.
🧹 Proposed change
std::string value;
for (int i = 0; i < 200; ++i) {
- ASSERT_OK(db->Get(ReadOptions(), key(i), &value));
- ASSERT_EQ(value, "generation-3");
+ EXPECT_OK(db->Get(ReadOptions(), key(i), &value)) << key(i);
+ EXPECT_EQ(value, "generation-3") << key(i);
+ if (HasFailure()) {
+ break;
+ }
}
delete db;
provider->EmptyBucket(bucket_, object_path_).PermitUncheckedError();
ASSERT_TRUE(all_backup_ssts_survived);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Verify the active database too, then clean up the unique object path even | |
| // when an EXPECT above detects a missing branch SST. | |
| std::string value; | |
| for (int i = 0; i < 200; ++i) { | |
| ASSERT_OK(db->Get(ReadOptions(), key(i), &value)); | |
| ASSERT_EQ(value, "generation-3"); | |
| } | |
| delete db; | |
| provider->EmptyBucket(bucket_, object_path_).PermitUncheckedError(); | |
| ASSERT_TRUE(all_backup_ssts_survived); | |
| std::string value; | |
| for (int i = 0; i < 200; ++i) { | |
| EXPECT_OK(db->Get(ReadOptions(), &value)) << key(i); | |
| EXPECT_EQ(value, "generation-3") << key(i); | |
| if (HasFailure()) { | |
| break; | |
| } | |
| } | |
| delete db; | |
| provider->EmptyBucket(bucket_, object_path_).PermitUncheckedError(); | |
| ASSERT_TRUE(all_backup_ssts_survived); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud/eloq_purger_integration_test.cc` around lines 718 - 727, In the
read-back loop over key(i), replace the fatal ASSERT_OK and ASSERT_EQ checks
with non-fatal EXPECT checks and break on failure, matching the branch-SST loop
pattern. Ensure execution reaches delete db, provider->EmptyBucket(bucket_,
object_path_), and the final ASSERT_TRUE(all_backup_ssts_survived) cleanup and
validation.
| // Validate that term is a number. A permissive parse is dangerous in | ||
| // both directions: "9999x" would read as 9999 and could make a bogus | ||
| // object the group's max term, retiring the authoritative CLOUDMANIFEST; | ||
| // silently skipping an unparseable name hides an object whose lineage we | ||
| // cannot reason about. Abort the cycle instead. | ||
| uint64_t term = 0; | ||
| try { | ||
| term = std::stoull(term_str); | ||
| } catch (const std::exception &e) { | ||
| // Not a valid pattern, skip this file | ||
| Log(InfoLogLevel::INFO_LEVEL, cfs_->info_log_, | ||
| "[pg] Skipping CLOUDMANIFEST file %s (invalid term: %s)", | ||
| if (!ParseStrictUint64(term_str, &term)) { | ||
| Log(InfoLogLevel::ERROR_LEVEL, cfs_->info_log_, | ||
| "[pg] CLOUDMANIFEST file %s has a malformed term '%s'; aborting " | ||
| "purge cycle", | ||
| candidate_file_path.c_str(), term_str.c_str()); | ||
| continue; | ||
| return Status::Corruption("Malformed CLOUDMANIFEST term", | ||
| candidate_file_path); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find cookie values passed to RollNewCookie/RollNewBranch and cookie option defaults.
set -euo pipefail
echo "== RollNewBranch / RollNewCookie call sites =="
rg -nP -C4 '\b(RollNewBranch|RollNewCookie)\s*\(' --type=cc --type=cpp --type=h
echo "== cookie option definitions and defaults =="
rg -nP -C3 '\b(cookie_on_open|new_cookie_on_open|branch_cookie)\b'
echo "== MakeCloudManifestFile usages =="
rg -nP -C3 '\bMakeCloudManifestFile\s*\('Repository: eloqdata/rocksdb-cloud
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== matching files =="
git ls-files | grep -E '\.(cc|cpp|h|proto|md)$' | sed -n '1,120p'
echo
echo "== RollNewBranch / RollNewCookie call sites in C/C++/proto/header files =="
rg -n -P -C4 '\b(RollNewBranch|RollNewCookie)\s*\(' --glob '*.[ch]' --glob '*.cc' --glob '*.cpp' --glob '*.proto' .
echo
echo "== cookie option definitions and defaults =="
rg -n -P -C3 '\b(cookie_on_open|new_cookie_on_open|branch_cookie|cloud_branch_cookie)\b' --glob '*.[ch]' --glob '*.cc' --glob '*.cpp' --glob '*.proto' --glob '*.md' .
echo
echo "== MakeCloudManifestFile usages =="
rg -n -P -C3 '\bMakeCloudManifestFile\s*\(' --glob '*.[ch]' --glob '*.cc' --glob '*.cpp' --glob '*.proto' .
echo
echo "== relevant purger sections =="
rg -n -P -C5 'ParseStrictUint64|CLOUDMANIFEST|RunSinglePurgeCycle|HasReachedAge|term_str|CloudManifestFileInfo' --glob '*.[ch]' --glob '*.cc' --glob '*.cpp' .Repository: eloqdata/rocksdb-cloud
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== eloq_purger relevant file size =="
wc -l cloud/eloq_purger.cc cloud/eloq_purger.h cloud/cloud_manifest.cc cloud/cloud_manifest.h cloud/cloud_file_system_impl.cc include/rocksdb/cloud/cloud_file_system.h
echo
echo "== cloud_file_system_impl MakeCloudManifestFile / ValidateCookie / RollNewBranch implementation =="
rg -n -P -C8 'MakeCloudManifestFile|ValidateCookie|RollNewBranch|RollNewCookie' cloud/cloud_file_system_impl.cc
echo
echo "== cloud_manifest MakeCloudManifestFile / cookie helpers =="
rg -n -P -C8 'MakeCloudManifestFile|cookie|Cookie' cloud/cloud_manifest.cc cloud/cloud_manifest.h
echo
echo "== eloq_purger manifest loop context =="
sed -n '760,880p' cloud/eloq_purger.cc
echo
echo "== parse helper =="
rg -n -P -C5 'ParseStrictUint64|parse.*uint64|strtoull' --glob '*.[ch]' --glob '*.cc' --glob '*.cpp' . | sed -n '1,160p'Repository: eloqdata/rocksdb-cloud
Length of output: 9578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cloud_manifest helper functions =="
sed -n '1,110p' cloud/cloud_manifest.cc
echo
grep -n "MakeCloudManifestFile\\|ValidateCookie\\|Cookie" cloud/cloud_manifest.cc cloud/cloud_manifest.h
echo
echo "== cloud_file_system_impl cookie validation and write sections =="
sed -n '1980,2165p' cloud/cloud_file_system_impl.cc
echo
echo "== eloq_purger manifest loop context =="
sed -n '760,880p' cloud/eloq_purger.ccRepository: eloqdata/rocksdb-cloud
Length of output: 3450
Enforce numeric CLOUDMANIFEST terms before grouping.
MakeCloudManifestFile(local_dbname, cookie) accepts any cookie, and existing tests use cookies like "1"/"2" or "000001" that are numeric terms, but the API does not require <prefix>-<term>. A branch cookie such as "backup-one" creates CLOUDMANIFEST-backup-one; the current path parses "one" and aborts the purge cycle. Document the required -<postfix>-<term> contract and enforce it at RollNewBranch/RollNewCookie, or make the purger return Status::Corruption with a clear operator message instead of aborting every cycle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud/eloq_purger.cc` around lines 839 - 852, Require RollNewBranch and
RollNewCookie to validate that cookies follow the documented <prefix>-<term>
contract, with a numeric terminal term while preserving leading zeros. Reject
invalid cookies with a clear error before MakeCloudManifestFile creates a
CLOUDMANIFEST, and document this API requirement; keep the purger’s
malformed-term corruption handling as a safety fallback.
3 participants
Production buckets accumulated ~150k objects against ~2k live files (purge cycles reporting total_files=152889, live_files=2122, obsolete_selected=26). Root cause: the purger's SST deletion gate is keyed by the epoch embedded in each object name, but thresholds are only loaded for the CURRENT epoch of each loaded CLOUDMANIFEST. Any non-live SST from a past epoch missed the threshold map and was skipped forever ("purge is blocked intentionally"). Since every reopen/failover rolls a new epoch and live files keep their creation-epoch names, each restart converted the entire surviving working set into permanently unreclaimable garbage once compaction rewrote it. Only files created AND obsoleted within the current epoch were ever collected.
Purger fixes
Guard writer moved into rocksdb-cloud
The purger's safety against deleting files uploaded by in-flight flush/compaction jobs depends on the per-epoch S3 object smallest_new_file_number-. Its writer previously lived in the embedding application (data_substrate purger_sliding_window / purger_event_listener); reader and writer now share one repo and one key-builder (SmallestFileNumberObjectKey), with the key format and ASCII value encoding unchanged for interop with existing writers.
New design (cloud/file_number_guard.{h,cc}):
Tests (previously zero coverage of purger or guard):
Also: fix CMakeLists source entry (cloud/improved_purger.cc ->
cloud/eloq_purger.cc; cmake configure was broken), and initialize StderrLogger::log_prefix_len in the no-prefix constructor (uninitialized read caused std::bad_alloc on first log).
Note:
make shared_lib(release) requires USE_RTTI=1 due to pre-existing dynamic_casts in cloud/; debug builds are unaffected.Summary by CodeRabbit