Repository navigation
Conversation
👁️ Cyclops Security Review✅ Reviewed 6e90fba (latest) · Coverage: partial ·
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
shekhirin
changed the title
| 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: gc can collect files of an extending fill that runs at the…
fill now reopens complete corpora and opens a writer on them, but gc still assumes a complete corpus has no writer. gc checks complete once, takes no claim, and then runs a GC pass with zero minimum file age. A fill that starts during gc can lose objects it has not yet referenced, and gc only notices the catalog change…
Full finding
gc can collect files of an extending fill that runs at the same time
fill now reopens complete corpora and opens a writer on them, but gc still assumes a complete corpus has no writer. gc checks complete once, takes no claim, and then runs a GC pass with zero minimum file age. A fill that starts during gc can lose objects it has not yet referenced, and gc only notices the catalog change after deleting.
Valid? React with 👍 / 👎.
| /// 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.
🔴 Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the corpus. On the base, a complete corpus was never written again. Now fill.rs:182-189 claims a complete corpus and opens a writer. gc only checks catalog.complete once at gc.rs:27, then runs run_gc_once with min_age: Duration::ZERO and deletes every unpinned checkpoint. Fix: fence fills through the catalog CAS before gc deletes anything, e.g. publish a gc claim a fill refuses or whose version makes the fill's store.publish(&catalog, prior) fail. The doc's "not alongside one" leaves this to scheduling.
Why this was flagged
Trigger: gc starts on a corpus with complete = true, and a corpus fill (nightly, --to omitted) starts before gc finishes. gc reads the catalog and passes the catalog.complete check at gc.rs:27. The fill then reaches fill.rs:182-189, clears complete, CAS-publishes its claim and opens a SlateDB writer that writes WAL SSTs, manifests and new checkpoints. gc keeps going. At gc.rs:42-43 it deletes every checkpoint except the one pinned when gc started, which can include the fill's new checkpoint. At gc.rs:52-66 it collects manifests, WAL and compacted files with min_age: Duration::ZERO, so the live writer's files go too. The e_tag check at gc.rs:80-83 only runs after the deletions. It reports an error but cannot undo them. The fill can then publish a catalog whose checkpoint points at deleted files, or fail partway. On the base, fill returned early on a complete corpus and never opened a writer, so gc after completion was always safe. The new doc line "Run it after a fill, not alongside one" is the only safeguard, and nothing in the code enforces it.
Verification: On the base, fill returned early on a complete corpus, so no writer was ever opened on one. At HEAD, fill.rs:182 sets catalog.complete = false and fill.rs:189 CAS-publishes the claim. gc checks complete once (gc.rs:26-30), then runs run_gc_once with min_age: Duration::ZERO (gc.rs:45-66). The e_tag check at gc.rs:80-83 fails gc only after the deletions have happened.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 2 findings from earlier reviews are still open above.
Still open from earlier reviews (2):
- 🔴
bin/tempo/src/corpus.rs:150—Long corpus replay runs can now fail partway when a nightly corpus fill extends the same corpus and its GC deletes file… - 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Still open from earlier reviews (2):
- 🔴
bin/tempo/src/corpus.rs:150—Long corpus replay runs can now fail partway when a nightly corpus fill extends the same corpus and its GC deletes file… - 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Still open from earlier reviews (2):
- 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the… - 🔴
crates/corpus/src/replay.rs:73—Operators who replay with read-only S3 credentials now fail at startup. Replay never wrote to the corpus before this ch…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 3 findings from earlier reviews are still open above.
Still open from earlier reviews (3):
- 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the… - 🔴
crates/corpus/src/replay.rs:73—Operators who replay with read-only S3 credentials now fail at startup. Replay never wrote to the corpus before this ch… - Also unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (2):
- 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the… - Also unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Still open from earlier reviews (2):
- 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the… - Also unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Code review completed
Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.
Still open from earlier reviews (2):
- 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the… - Also unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
3 optional suggestions (nits or notes on pre-existing code) were found and not posted.
Still open from earlier reviews (1):
- 🔴
crates/corpus/src/gc.rs:25—Operators who schedule corpus gc next to the nightly corpus fill can now have live writer files deleted, corrupting the…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
tempo corpus fillcan now run without a range and extend an existing corpus.--tois optional and defaults to the archive head. Each run appends blocks after the published cursor, instead of rejecting any range other than the one the corpus was created with. A nightlycorpus fill+corpus gcthen keeps the corpus at the 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 replayreads 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.