Repository navigation
Conversation
👁️ Cyclops Security Review✅ Reviewed ecb7364 (latest) · Coverage: partial ·
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let context = &catalog.context; | ||
| eyre::ensure!( | ||
| context.chain_id == self.chain.chain_id() | ||
| && context.genesis == self.chain.genesis_hash().to_string(), | ||
| "corpus chain context mismatch" | ||
| ); | ||
| let published = &catalog.cursor; |
There was a problem hiding this comment.
🔴 A corpus replay that runs during or across a fill can now lose its data partway through and fail. Replay no longer requires complete and reads the checkpoint pinned when it opened. An extending fill then supersedes that checkpoint and retires it with RETIRED_CHECKPOINT_LIFETIME (1h). The writer's GC, or a later corpus gc (which deletes every unpinned checkpoint with min_age: Duration::ZERO), then removes its SSTs. Fix: keep a running replay's checkpoint alive for the whole read, for example by giving each replay its own checkpoint or lease that is refreshed while it reads, and have gc skip checkpoints that still hold an unexpired lease. [also at: bin/tempo/src/corpus.rs:144 - A replay that overlaps an extending fill can now lose its checkpoint and fail partway, which could not happen on the base branch.]
Why this was flagged
Trigger: an operator replays a large corpus while the nightly corpus fill + corpus gc described in the PR runs. Replay opens store.reader(&catalog) on catalog.checkpoint (store.rs:338-352, DbReaderMode::Checkpoint). The catalog.complete guard at corpus.rs:144 is gone, so nothing stops a replay from starting while a fill is running. Each sealed page in fill.rs makes the previous checkpoint superseded and calls store.retire_checkpoint (store.rs:383-388, 1h lifetime). gc (gc.rs) calls delete_checkpoint on every checkpoint except the newly pinned one and runs GC with zero min_age, so the SSTs under a replay that is still reading disappear and its reads fail. On the base branch, replay needed complete and a complete corpus was never written again, so its pinned checkpoint stayed live.
Verification: The diff removes eyre::ensure!(catalog.complete, "corpus is not complete") from bin/tempo/src/corpus.rs:144, so replay can now start while a fill is running. Each sealed page does let superseded = catalog.checkpoint.replace(checkpoint), publishes, then calls store.retire_checkpoint(&superseded) (fill.rs:479-493). On the base, replay required complete and fill never wrote to a complete corpus.
| /// Fails unless the corpus is complete: a fill clears it before opening its writer, so | ||
| /// collection needs no minimum file age as long as no fill starts meanwhile. Run it after a | ||
| /// fill, not alongside one. Fails as well if the catalog changed meanwhile or its checkpoint | ||
| /// no longer opens. |
There was a problem hiding this comment.
🔴 A corpus gc that overlaps a fill can now delete files the fill's writer just wrote, which the base branch made structurally impossible. gc checks catalog.complete only once, at the start. Its GC then runs with min_age: Duration::ZERO on WAL, compacted and manifest files. A fill that starts in the meantime flips complete and writes new SSTs that no published checkpoint references yet. Fix: gc must not reclaim files of a writer that started after its check. For example, use a non-zero min_age for the wal/compacted/manifest directories, or have the fill fence or detect a running gc. The doc comment alone does not stop this.
Why this was flagged
The new doc comment says to run gc after a fill, "not alongside one". That only works if operators keep the two apart. On the base branch, fill returned early whenever catalog.complete was set and never opened a writer, so gc on a complete corpus could never race a writer. Now fill publishes complete = false at fill.rs:182-189 and opens a writer on a complete corpus whenever end > cursor.next. That is every nightly run, since the PR's goal is to track the archive head. Suppose gc (gc.rs:25-66) has passed its catalog.complete check and then this happens. Its run_gc_once, with min_age: Duration::ZERO for wal_options and compacted_options, deletes WAL/SST/manifest objects that the new writer created but no published checkpoint references yet. The fill then fails, or worse, on missing objects. gc's final e_tag check (gc.rs:79-83) only reports the race after the files are already gone.
Verification: gc.rs:21-24 says collection is safe only "as long as no fill starts meanwhile", but nothing in the code enforces it. fill.rs:174 now returns early only when end <= catalog.cursor.next, then opens a writer (fill.rs:190-201). gc checks catalog.complete once (gc.rs:27-30), then runs run_gc_once with min_age: Duration::ZERO (gc.rs:45-57). The base branch had no such window.
| if end <= catalog.cursor.next { | ||
| info!( | ||
| records = catalog.cursor.next - catalog.context.start, | ||
| verified = args.verify_existing, | ||
| "Corpus already complete; no writes" | ||
| "Corpus already reaches the requested end; no writes" | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) An interrupted fill can leave the corpus stuck with complete: false if it is rerun to an end the published cursor already reaches. corpus gc then keeps refusing until some fill extends past the cursor. The early return now triggers on end <= catalog.cursor.next instead of catalog.complete, so complete is never set again. Fix: when the published cursor already reaches the requested end, also republish complete = true (by CAS on prior), or let the writer path run and seal, so a rerun restores gc eligibility.
Why this was flagged
Trigger: a fill to end 200 seals and publishes at cursor 150 with catalog.complete = stop == end = false (fill.rs:480), then gets cancelled. The operator reruns corpus fill --to 150, or with a clamped end at or below 150. The new check at fill.rs:174 returns Ok with "Corpus already reaches the requested end; no writes" and leaves complete false. corpus gc then fails with "corpus is not complete; its writer may still be running" (gc.rs:27-30), even though no writer is running. On the base branch the early return was keyed on catalog.complete, so an incomplete corpus always went on to the writer path to finish. Here, rerunning the same fill succeeds but does not restore gc eligibility.
Verification: Each seal publishes catalog.complete = stop == end (fill.rs:480), and the claim publishes complete = false (fill.rs:182). A rerun with end <= catalog.cursor.next (fill.rs:174) returns Ok(()) without publishing anything. complete stays false. gc then fails with "corpus is not complete; its writer may still be running" (gc.rs:27-30). On the base, the rerun was rejected rather than reporting success.
| #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct Context { | ||
| pub chain_id: u64, | ||
| pub genesis: String, | ||
| pub forks: String, | ||
| pub start: u64, | ||
| pub end: u64, | ||
| pub parent: String, | ||
| pub tip: String, | ||
| pub first_tx: u64, | ||
| pub end_tx: u64, | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Corpora written by this binary can no longer be read by older binaries, including main's replay and fill. Every catalog and stored context is now serialized without end, tip and end_tx. The old Context requires those fields. This applies to any older binary still deployed (CI replay, a rollback, a pinned fleet). Fix: keep forward compatibility while rollout is mixed, for example by still writing the legacy fields (end = cursor.next, tip, end_tx = cursor.tx) as Options with skip_deserializing. Otherwise state that a rollback needs a corpus rewrite.
Why this was flagged
Condition: any consumer still runs a pre-PR binary against a corpus that a post-PR fill touched. That includes a CI job pinned to an older release, a rollback, or a staggered deploy. The new Context at store.rs:62-70 drops end, tip and end_tx. fill.rs:222-225 rewrites CONTEXT_KEY in the DB, and the next publish rewrites the root catalog JSON without those fields. The base Context declares them as required non-Option fields, so serde in the old binary fails with a missing-field error on store.catalog() and in store.reader() (store.rs:355-361). Old corpus replay, fill and gc all stop working on that corpus. The PR tests only the other direction (legacy_context_with_pinned_end_still_reads). Nothing versions the format, so the failure appears only when the old binary runs.
Verification: Trigger: a pre-PR binary reads a corpus that a post-PR fill has claimed or published. In the base, Context declares pub end: u64, pub tip: String and pub end_tx: u64 as required fields. In the new code, store.rs:62-70 drops those three fields. After that, an old binary's catalog() fails with a serde "missing field end" error.
| provider_factory | ||
| .database_provider_ro()? | ||
| .block_hash(end - 1)? | ||
| .is_some_and(|hash| hash.to_string() == catalog.context.tip), | ||
| .is_some_and(|hash| hash == tip), |
There was a problem hiding this comment.
Low (trigger unconfirmed): Corpus fill no longer checks captured tip hash against pinned tip
This PR removed the completion check in Catalog::validate that required the captured cursor's terminal hash to equal the pinned Context.tip. The seal-time check that remains compares only a fresh archive lookup of end-1 with the hash read at fill start. A fill whose source switches to another branch and back can therefore publish an orphaned terminal block.
Valid? React with 👍 / 👎.
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — 2 findings.
- **Informational · unverified:** gc can delete the checkpoint an overlapping extending fill just published, wedging…
- **Informational · unverified:** Extending a complete corpus lets GC invalidate a pending replay snapshot
| if end <= catalog.cursor.next { | ||
| info!( | ||
| records = catalog.cursor.next - catalog.context.start, | ||
| verified = args.verify_existing, | ||
| "Corpus already complete; no writes" | ||
| "Corpus already reaches the requested end; no writes" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| catalog.complete = false; |
There was a problem hiding this comment.
Informational · unverified: gc can delete the checkpoint an overlapping extending fill just published, wedging…
Mechanism: gc (crates/corpus/src/gc.rs:25-44) reads the catalog once, requires complete, and saves pinned = catalog.checkpoint. It then lists every data object (usage), calls Admin::list_checkpoints(), and deletes every checkpoint whose id != pinned. Admin::list_checkpoints returns whatever checkpoints are in the latest manifest at call time, and delete_checkpoint removes any id unconditionally (slatedb admin.rs:259-287, 559-578).
Full finding
gc can delete the checkpoint an overlapping extending fill just published, wedging the corpus
Mechanism: gc (crates/corpus/src/gc.rs:25-44) reads the catalog once, requires complete, and saves pinned = catalog.checkpoint. It then lists every data object (usage), calls Admin::list_checkpoints(), and deletes every checkpoint whose id != pinned. Admin::list_checkpoints returns whatever checkpoints are in the latest manifest at call time, and delete_checkpoint removes any id unconditionally (slatedb admin.rs:259-287, 559-578). The only guard against a concurrent writer is the up-front complete check. Before this PR that was enough, because fill returned early on any complete corpus (base fill.rs:162-169) and gc's contract said 'fill never opens a writer on a complete corpus' (base gc.rs:21-23). This PR makes complete corpora writable: when end > cursor.next, fill.rs:174-201 clears complete, publishes a claim and opens a writer, and each seal (fill.rs:464-494) creates a new checkpoint and publishes the catalog naming it.
Trigger: (1) gc reads the catalog (complete=true, checkpoint C_old). (2) An extending fill (for example the nightly fill the PR describes) reads the same catalog, claims it, captures a page, creates C_new and publishes the catalog naming C_new. On a short extension (one page) this is also the final publication, with complete=true; the fill retires C_old with a 1 h lifetime and exits successfully. (3) gc, still inside usage()/listing, calls list_checkpoints, sees C_new, and deletes it because C_new != the stale pinned C_old. (4) gc's etag check (gc.rs:79-85) then fails with 'catalog changed during gc', but the deletion has already happened.
Impact: the published catalog names a checkpoint that no longer exists. Store::reader (store.rs:338-353) opens DbReader in Checkpoint mode and gets CheckpointMissing (slatedb db_reader.rs:280-293). From then on every corpus replay, every corpus fill (fill.rs:151-152 opens the reader first) and every corpus gc ('catalog checkpoint is missing from the corpus manifest') fails. The corpus stays unusable until the catalog JSON is repaired by hand. C_old expires an hour later, so it is no longer a fallback. If the deleted checkpoint was an intermediate page, the fill instead fails after its next publication, because retire_checkpoint on the deleted id errors. The underlying data SSTs are still referenced by the writer's latest manifest, so records are not lost, but the tooling cannot recover on its own.
What is not proven: this needs gc and an extending fill to overlap. The PR documents 'Run it after a fill, not alongside one', but nothing enforces it, and the documented risk ('no minimum file age') understates the outcome. No scheduler or workflow in the repository shows whether production jobs can overlap. This is offline tooling with no consensus or fund impact.
POC (deterministic interleaving using the existing test helpers in store.rs/gc.rs): publish a complete catalog with checkpoint C_old and save pinned = C_old. Open the db, create C_new via store.checkpoint, and publish the catalog naming C_new. Run gc's deletion loop with the saved pinned (Admin::list_checkpoints, then delete_checkpoint for each id != pinned). store.reader(&catalog_with_C_new) now fails with CheckpointMissing.
Remediation: before deleting, re-read the catalog (or HEAD and compare its etag to version) and abort if it changed. Also, only delete checkpoints created before the pinned one (compare create_time), so a newer, possibly published, checkpoint is never removed. Alternatively, make fill refuse to claim while a gc-in-progress marker or generation bump written by gc is present.
Valid? React with 👍 / 👎.
| if end <= catalog.cursor.next { | ||
| info!( | ||
| records = catalog.cursor.next - catalog.context.start, | ||
| verified = args.verify_existing, | ||
| "Corpus already complete; no writes" | ||
| "Corpus already reaches the requested end; no writes" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| catalog.complete = false; |
There was a problem hiding this comment.
Informational · unverified: Extending a complete corpus lets GC invalidate a pending replay snapshot
A replay's selected published checkpoint must remain usable until the replay acquires its reader. The new append path reopens previously complete corpora and replaces their checkpoints, but ReplayCommand::execute still reads a catalog before scheduling the blocking task that opens its checkpoint. Concrete interleaving: replay reads catalog C1 naming checkpoint P1 and is descheduled; fill extends that corpus, publishes complete catalog…
Full finding
A replay's selected published checkpoint must remain usable until the replay acquires its reader. The new append path reopens previously complete corpora and replaces their checkpoints, but ReplayCommand::execute still reads a catalog before scheduling the blocking task that opens its checkpoint. Concrete interleaving: replay reads catalog C1 naming checkpoint P1 and is descheduled; fill extends that corpus, publishes complete catalog C2 naming P2, and closes; the documented subsequent corpus gc deletes P1 because it retains only P2; replay resumes and Store::reader(C1) fails with SlateDB's CheckpointMissing(P1), with no retry. This requires neither overlapping fill and GC nor malformed corpus data or compaction. It aborts an otherwise valid replay under the newly supported mutable-corpus lifecycle. The required operational precondition is a replay overlapping extension and subsequent GC on the same corpus; deployed scheduling and remote attacker influence were not established. PoC test: an executed isolated Rust source-slice regression initialized checkpoints [P1, P2], a pending replay reference to P1, and current pinned checkpoint P2, then executed the exact selection loop from gc.rs:42. The assertion that P1 remained available failed. This validates the deletion decision, not a complete SlateDB integration run; the pinned dependency trace establishes the resulting missing-checkpoint error. An integration regression can save a catalog after publishing P1, publish an extended complete catalog at P2, close the writer, call gc, and require Store::reader(saved_catalog) to succeed. Fix snapshot acquisition by retrying catalog acquisition when its checkpoint disappears before opening, or protect replay snapshots with retention/coordination that GC respects.
Valid? React with 👍 / 👎.
Stacked on #8856.
tempo corpus fillnow extends an existing corpus: it appends blocks after the published cursor up to the new end, instead of rejecting any range other than the one the corpus was created with. This lets a nightlycorpus fill+corpus gckeep the corpus at the archive head.The context no longer pins
end,tipandend_tx. The published cursor already holds them. An extending fill checks that the published tip is still canonical and that the fork schedule agrees up to it, then stores its own schedule. Existing corpora need no migration: serde ignores the old fields, and the first extending fill rewrites the stored context.corpus replaynow reads the published blocks without requiringcomplete, so it keeps working while a fill runs.gcmust still run after a fill, not alongside one.Tested on a
--devchain. Fill to the head, a no-op refill, more blocks, then an extending fill, replay, gc and replay again all pass. A corpus written bymain's binary extends with--verify-existingand replays.