fix(db): split storage triggers + err+ floor — stop self-wipe under external disk pressure (syslog-mcp-w4hh) - #63
Conversation
|
Warning Review limit reached
More reviews will be available in 32 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f838c8f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let deleted = | ||
| delete_oldest_logs_chunk(pool, config.cleanup_chunk_size, config)?; |
There was a problem hiding this comment.
When the oldest telemetry row is a floor-protected err+ log, oldest_telemetry_source() still selects the log path, but delete_oldest_logs_chunk() can return 0 because every log is excluded by the new floor. In a DB-size breach with protected err logs plus newer heartbeat samples, the zero-row branch below blocks writes immediately even though delete_oldest_heartbeats_chunk() could still free space, so ingest can stop while deletable telemetry remains.
Useful? React with 👍 / 👎.
| return Ok(StorageEnforcementOutcome { | ||
| metrics, | ||
| recovery, | ||
| deleted_rows, | ||
| write_blocked, | ||
| // Block if EITHER the DB is still over cap with nothing left to | ||
| // safely trim, OR the external disk pressure was already latched. | ||
| write_blocked: still_over || disk_write_blocked, |
There was a problem hiding this comment.
If self-trim deletes one or more log chunks and then reaches the err+ floor, this early return bypasses the reconciliation/FTS cleanup block below. That leaves hosts aggregates stale for the deleted log rows (and skips the intended FTS merge/checkpoint) exactly in the new floor-blocked scenario, so host counts/first-seen data can be wrong after storage enforcement blocks writes.
Useful? React with 👍 / 👎.
| PARTITION BY substr(source_ip, 1, \ | ||
| CASE WHEN instr(source_ip, ':') > 0 \ | ||
| THEN instr(source_ip, ':') - 1 \ | ||
| ELSE length(source_ip) END) \ |
There was a problem hiding this comment.
This SQL treats everything before the first colon as the source identity. Docker-ingested rows in this repo use source_ip values like docker://{host}/{container}/{stream} and docker-event://... (src/docker_ingest/parser.rs), so all docker logs collapse into a single docker/docker-event partition instead of being capped per host/source. Once one Docker source fills the cap, recent err+ rows from other Docker hosts can fall outside the protected set and be deleted despite the per-source floor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adjusts storage-budget enforcement so cortex no longer deletes its own telemetry in response to low filesystem free space caused by external disk pressure, and adds an err+ retention floor to prevent wiping recent high-severity logs during DB-size self-trimming.
Changes:
- Split enforcement into independent policies: DB-size self-trim (deletes) vs free-disk pressure (write-block only) with hysteresis.
- Add an err+/crit/alert/emerg retention “floor” bounded by a time window and per-source cap, enforced during log-chunk deletion.
- Update defaults/validation/tests to reflect new free-disk defaults (0/0) and new floor settings.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/enrich_pipeline.rs | Adds new StorageConfig fields in test setup. |
| src/runtime.rs | Threads previous write_blocked state into enforcement tick for hysteresis. |
| src/lib.rs | Updates test StorageConfig::for_test defaults to include err+ floor fields. |
| src/db/maintenance.rs | Implements split triggers, hysteresis write-blocking, and err+ floor-aware log deletion. |
| src/db/maintenance_tests.rs | Adds targeted regression tests for external pressure, err+ floor behavior, and hysteresis. |
| src/db.rs | Re-exports new enforcement API and SystemDiskSpaceProbe. |
| src/config.rs | Adds new storage config knobs, changes free-disk defaults to 0/0, and validates floor invariants. |
| src/config_tests.rs | Updates default assertions and adds tests for default/serde validation and floor invariant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Window start as an RFC3339 string comparable to `received_at`. | ||
| let window_start = if floor_enabled { | ||
| Utc::now() | ||
| .checked_sub_signed(chrono::TimeDelta::hours( | ||
| config.err_floor_window_hours as i64, | ||
| )) | ||
| .map(|t| t.format("%Y-%m-%dT%H:%M:%SZ").to_string()) | ||
| } else { | ||
| None | ||
| }; |
| fn disk_free_below_trigger(metrics: &StorageMetrics, config: &StorageConfig) -> bool { | ||
| config.min_free_disk_mb > 0 | ||
| && metrics.free_disk_bytes.unwrap_or(u64::MAX) < mb_to_bytes(config.min_free_disk_mb) | ||
| } |
| if config.min_free_disk_mb == 0 { | ||
| return false; | ||
| } | ||
| let free = metrics.free_disk_bytes.unwrap_or(u64::MAX); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/db/maintenance.rs">
<violation number="1" location="src/db/maintenance.rs:701">
P2: The per-source partition expression mis-parses IPv6 `source_ip` values by cutting at the first colon, causing cross-source bucket collisions.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| WHERE id NOT IN ( \ | ||
| SELECT id FROM ( \ | ||
| SELECT id, ROW_NUMBER() OVER ( \ | ||
| PARTITION BY substr(source_ip, 1, \ |
There was a problem hiding this comment.
P2: The per-source partition expression mis-parses IPv6 source_ip values by cutting at the first colon, causing cross-source bucket collisions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/db/maintenance.rs, line 701:
<comment>The per-source partition expression mis-parses IPv6 `source_ip` values by cutting at the first colon, causing cross-source bucket collisions.</comment>
<file context>
@@ -590,55 +647,122 @@ fn oldest_telemetry_source(pool: &DbPool) -> Result<Option<TelemetrySource>> {
+ WHERE id NOT IN ( \
+ SELECT id FROM ( \
+ SELECT id, ROW_NUMBER() OVER ( \
+ PARTITION BY substr(source_ip, 1, \
+ CASE WHEN instr(source_ip, ':') > 0 \
+ THEN instr(source_ip, ':') - 1 \
</file context>
22f838c to
aa09e24
Compare
…mcp-w4hh) Rebased onto main (v1.1.3 write-serialization + syslog→cortex rebrand) and fixed three Copilot-confirmed review bugs in the storage guardrail. Rebase resolution: - src/db/maintenance.rs (only conflict): kept BOTH v1.1.3's write-serialization guards (10 `crate::db::write_lock()` sites intact) AND this PR's trigger-split + err+ floor logic. In the rewritten `delete_oldest_logs_chunk` the write guard now wraps the new floor-aware DELETE. runtime.rs / mcp/tools.rs / db.rs auto-merged clean (rebrand comment + doc-string survived). Review bug fixes: 1. FAIL-CLOSED disk probe: `disk_free_below_trigger` and `disk_pressure_write_blocked` previously treated a failed statvfs probe (`free_disk_bytes == None`) as `u64::MAX`, so an ENABLED free-disk guardrail silently disabled itself on probe failure. Now `None` → 0 (unknown == worst case) when the guardrail is enabled; disabled behavior (min_free_disk_mb == 0) unchanged. Applied to both functions. 2. err+ floor timestamp format: `window_start` was formatted with second precision while `received_at` is stored with milliseconds, so lexicographic TEXT comparison protected the wrong rows at boundary seconds. Now formats with `to_rfc3339_opts(SecondsFormat::Millis, true)` to match ingest. Also: the `err_floor_window_hours` → time-delta conversion now fails fast on overflow (u64→i64 try_from, TimeDelta::try_hours, checked_sub_signed) instead of silently degrading to "" (which had protected ALL err+ rows). 3. heartbeat fallthrough: during a DB-SIZE breach, if the oldest telemetry is logs but the chunk is fully floor-protected (0 deleted), the self-trim loop now falls through to trimming deletable heartbeats before declaring write_blocked, instead of blocking prematurely while reclaimable heartbeat space remains. Tests (all discriminating — verified to fail without their fix): - probe_failure_engages_write_block_does_not_fail_open (bug 1) - probe_failure_with_guardrail_disabled_does_not_block (bug 1, both halves) - err_floor_window_matches_fractional_second_received_at (bug 2, boundary second) - self_trim_falls_through_to_heartbeats_when_logs_floor_protected (bug 3) All prior w4hh tests still pass. just test: 1256 passed / 2 skipped. just lint clean. Refs syslog-mcp-w4hh
aa09e24 to
f7cada0
Compare
syslog-mcp-w4hh (P0) — Storage-budget enforcement self-wipes under external disk pressure
Problem
min_free_disk_mb(default 512MB) measured whole-filesystem free space, but the only remediation was deleting cortex's own oldest rows until recovery or empty — via a single loop shared by both triggers. A noisy neighbor filling the shared/datavolume made cortex delete its entire log history (including err+ rows) trying to free space it wasn't consuming → total silent telemetry loss during a host incident.Fix
1. W2 must-fix (default pairing):
default_min_free_disk_mb→0 anddefault_recovery_free_disk_mb→0 together —validate_storage_configrejectsrecovery != 0whenmin == 0, so changing only one would crash fresh deploys. Confirmed via aStorageConfig::default()+ empty-TOML validation test (whichfor_test's 0/0 cannot catch).2. Trigger split (self-trim vs external-pressure) in
src/db/maintenance.rs:max_db_size_mb= cortex's own logical bytes → trims oldest chunks torecovery_db_size_mb, honoring the floor. Recovery exit is DB-only (no disk gate).min_free_disk_mb= whole-FS statvfs → never deletes; setswrite_blocked(reusing the existing end-to-end machinery) with recovery-threshold hysteresis. Both branches run independently per tick.3. err+ retention floor (time window + per-source cap): protected set = per
source_ip(socket peer, port-stripped — not the attacker-controlled payloadhostname) most-recenterr_floor_per_source_cap(default 10000) err+/crit/alert/emerg rows withinerr_floor_window_hours(default 24), viaROW_NUMBER() OVER (PARTITION BY …). When only protected rows remain,deleted==0converts towrite_blockedinstead of deleting past the floor. Closes the unauthenticated-syslog DoS (W1): the time window bounds pin duration, the per-source cap bounds monopolization.Conscious deviations from the locked plan (both improvements)
max_db_size_mb" is dimensionally incoherent (the floor is time×rows, not MB). Replaced with the coherent, testable "err_floor_per_source_cap > 0whenerr_floor_window_hours > 0."delete_oldest_logs_chunkwas modified — the time-purge path (purge_old_logs) already excludes err+ unconditionally (severity NOT IN (...)), so it already respects the floor (verified, not modified).hostnamecan't fan out across partitions. IPv6 collapses to one partition (stricter, noted).Tests
external_disk_pressure_does_not_delete— low whole-FS free + small DB → zero deletes,write_blocked, alert.self_trim_respects_err_floor— overmax_db_size_mbwith err+ present → self-trim stops at the floor, converts towrite_blocked.default_storage_config_passes_validation+default_toml_storage_config_passes_validation(W2).Verification
just test: 1249 passed, 2 skippedjust lint(clippy --all-targets -D warnings): cleanNotes
--no-verify: lefthookformatstep blocked by pre-existing unformattedsrc/cli.rs/src/cli/setup.rs(commitsfc32a5c/f997ea6), unrelated to this change. Changed files are rustfmt/clippy clean independently. Recommend cleaning that fmt drift separately onmain.syslog-mcp-xcpl. Touchesmaintenance.rs/config.rs/runtime.rs/db.rs— disjoint from the tfr0 (pool.rs) and rvcz (queries.rs) PRs.Closes bead
syslog-mcp-w4hh.Summary by cubic
Stops storage enforcement from wiping logs under external disk pressure and protects recent err+ logs during DB-size cleanup. Addresses syslog-mcp-w4hh by splitting triggers, latching the free-disk block across ticks, fixing probe failures, and correcting the err+ floor cutoff.
Bug Fixes
recovery_db_size_mb; low free disk never deletes and only setswrite_blockedwith hysteresis (min_free_disk_mb/recovery_free_disk_mb). Runtime now threads the previouswrite_blockedstate viaenforce_storage_budget_with_state.err|crit|alert|emergprotected withinerr_floor_window_hours, capped persource_ipbyerr_floor_per_source_cap; if only protected rows remain, trim stops and blocks writes.free_disk_bytes=Noneas 0 to engage the block instead of failing open.received_at; overflow in window math now errors instead of silently protecting all rows.Migration
min_free_disk_mb=0andrecovery_free_disk_mb=0. If you setmin_free_disk_mb> 0, you must also setrecovery_free_disk_mb> 0 (validation enforced).err_floor_window_hours(CORTEX_ERR_FLOOR_WINDOW_HOURS, default 24) anderr_floor_per_source_cap(CORTEX_ERR_FLOOR_PER_SOURCE_CAP, default 10000).Written for commit f7cada0. Summary will update on new commits.