feat: bound artifact version history and audit log growth - #289
Conversation
…ish loops Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: feat/storage-guards
Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: feat/storage-guards
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds gzip compression and retention controls for artifact snapshots and disk blobs, trims oversized audit logs, and introduces consent-gated installation of generic and Git PATH shims during initialization. ChangesStorage retention and compression
Audit log retention
PATH shim installation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InitComponent
participant ShimInstaller
participant GitShimInstaller
participant ShimDirectory
InitComponent->>ShimInstaller: check and install shims
ShimInstaller->>ShimDirectory: hardlink or copy generic shims
ShimInstaller->>GitShimInstaller: install bundled Git shim
GitShimInstaller->>ShimDirectory: copy binary and set permissions
ShimInstaller->>ShimDirectory: ensure directory is on PATH
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…ency Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: feat/storage-guards
Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: feat/storage-guards
Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: feat/storage-guards
|
Added two more pieces to this PR: Gzip compression (artifact version snapshots + blob storage): both stores wrote raw bytes to disk with no compression. Compressed with flate2, using gzip's self-describing magic header ( PATH shim install wired into Note: this only takes effect end-to-end for release/curl-installed builds once two infra pieces are also updated — release.yml's packaging step (only bundles the |
Agentflare-Agent: claude-code_2-1-216_harness Agentflare-Branch: feat/storage-guards
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/agentflare-artifacts/src/store.rs (1)
313-322: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPruning is O(n) per publish → O(n²) over a runaway loop.
prune_old_versionsruns on every publish and re-issuesremove_filefor the whole2..=cutoffrange each time, so the syscall count grows with the version number. A stuck/looprepublishing to version N (the very scenarioMAX_KEPT_VERSIONSis meant to bound) triggers ~O(N²) filesystem operations overall.Since the window slides by exactly one per changed publish, only the single version that just fell out of the window needs removing:
♻️ Prune only the newly-evicted version
fn prune_old_versions(dir: &Path, latest_version: u32, keep: u32) { if latest_version <= keep { return; } - let cutoff = latest_version - keep; - let versions_dir = dir.join(VERSIONS_DIR); - for v in 2..=cutoff { - let _ = fs::remove_file(versions_dir.join(v.to_string())); - } + // The window slides by one per changed publish, so only the version that + // just fell out of it needs deleting; earlier ones were pruned before. + // v1 is the origin anchor and is never in range (cutoff >= 2 guards it). + let cutoff = latest_version - keep; + if cutoff >= 2 { + let _ = fs::remove_file(dir.join(VERSIONS_DIR).join(cutoff.to_string())); + } }Note this trades away catch-up pruning of directories that were already over the cap before this code shipped; if that matters, keep the loop but bound its lower end to a stored high-water mark rather than always restarting at 2.
🤖 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 `@crates/agentflare-artifacts/src/store.rs` around lines 313 - 322, Update prune_old_versions to remove only the single version newly evicted when latest_version exceeds keep, deriving that version from latest_version and keep instead of iterating from 2 through the cutoff. Preserve the existing early return and ignored remove_file errors; do not retain the full-range loop unless it is bounded by a persisted high-water mark.
🤖 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 `@crates/flare-git-core/src/audit.rs`:
- Around line 77-84: Update the audit rotation logic after the meta.len() check
so rotation guarantees the resulting active file is below max_bytes. Prefer
rename-based rotation that replaces the oversized file with a fresh empty file;
if retaining in-place trimming, trim content by byte size rather than only
keep_lines. Ensure subsequent appends can use the size fast path without
repeatedly rereading and rewriting an oversized file.
- Around line 70-86: Update maybe_rotate to perform rotation atomically and
safely under concurrent processes: acquire an exclusive lock covering the
metadata check and entire read-modify-write cycle, write trimmed content to a
temporary file, then rename it over path instead of using fs::write. Preserve
the existing no-op behavior when the file is missing, within budget, or has no
more than keep_lines lines.
---
Nitpick comments:
In `@crates/agentflare-artifacts/src/store.rs`:
- Around line 313-322: Update prune_old_versions to remove only the single
version newly evicted when latest_version exceeds keep, deriving that version
from latest_version and keep instead of iterating from 2 through the cutoff.
Preserve the existing early return and ignored remove_file errors; do not retain
the full-range loop unless it is bounded by a persisted high-water mark.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d14c9f9-dd5b-435d-83c6-cfefb881130d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlcrates/agentflare-artifacts/Cargo.tomlcrates/agentflare-artifacts/src/store.rscrates/agentflare-store/Cargo.tomlcrates/agentflare-store/src/blobs.rscrates/flare-git-core/src/audit.rssrc/cli/git.rssrc/cli/mod.rssrc/components.rssrc/main.rssrc/shim_install.rs
maybe_rotate did an unsynchronized read-modify-write (metadata check, read, truncating write) on every append once over budget. A concurrent git process appending between another's read and write would have its event silently dropped, and a rewrite interrupted mid-write (crash, disk full) could corrupt the log. Fixed by holding an exclusive lock (fs2, already a workspace dependency, mirroring the pattern in src/daemon.rs) across the whole log_event call, and writing rotated content to a temp file then renaming it over the original so a crash can't leave a torn file. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: feat/storage-guards
1 participant
Adds two storage guards, modeled directly on lean-ctx's own
core/archive.rs::cleanup_with— whose comment cites its own past incident: "without an enforcer the archive grew unbounded on disk and starved the host of RAM via the page cache (#417)". agentflare had the same two open-ended growth paths with nothing bounding them.1. Artifact version-snapshot history (agentflare-artifacts)
ArtifactStore::publishwrote a full, uncompressed content snapshot per version intoversions/forever — no cap.prune_old_versionsnow keeps the last 50 version bodies plus v1 (the origin anchor), called on every publish. Deliberately conservative vs. lean-ctx's cache-eviction model: this is user-published data, not disposable cache, so:/loopsession republishing one artifact hundreds of times), never normal editing.versions()(the history/version list) is untouched, so what happened is still visible.diff/get_versionon a pruned version returns a plain NotFound rather than silently returning wrong data.2. Audit log rotation (flare-git-core)
audit::log_eventappended one JSONL line per git-shim invocation with zero rotation — confirmed live on this machine:git.jsonlis already at 7,538 lines.maybe_rotatenow trims a log back to its last 5,000 lines once it exceeds 5MB. Checked via a singlemetadata()stat per append, so the common under-budget case costs one cheap syscall, not a read of the whole file — no hot-path regression for the git shim.Verification
cargo test -p agentflare-artifacts -p flare-git-core: 24 + 71 passed, 0 failed (both crates previously had 0/71 respectively for this new coverage — agentflare-artifacts had no test module at all before this PR)cargo fmt --checkcleancargo clippy --workspace --all-features -- -D warnings: only the two pre-existing Windows-local dead-code errors indaemon_autostart.rs(unix-only helpers, present on master, untouched here)cargo test --workspace: 692 passed / 0 failed, all suites greenSummary by CodeRabbit
agentflare initnow installs command shims and helps add them to yourPATH.