fix(mito): honor unknown file lingering time (#8365)

* fix(mito): honor unknown file lingering time Signed-off-by: discord9 <discord9@163.com> * fix(mito): allow gc test helper arguments Signed-off-by: discord9 <discord9@163.com> * test(mito): cover unknown file ttl boundaries Signed-off-by: discord9 <discord9@163.com> * test: update unknown file ttl config snapshot Signed-off-by: discord9 <discord9@163.com> * chore: fix typo in logical table alter Signed-off-by: discord9 <discord9@163.com> * refactor(mito): make gc delete helper sync Signed-off-by: discord9 <discord9@163.com> * refactor(mito): call gc delete helper directly Signed-off-by: discord9 <discord9@163.com> * refactor(mito): minimize unknown file ttl changes Signed-off-by: discord9 <discord9@163.com> --------- Signed-off-by: discord9 <discord9@163.com>
2026-07-06 14:00:40 +00:00 · 2026-07-03 12:31:35 +08:00
parent fc7a9bf56d
commit 33902eef3e
5 changed files with 363 additions and 20 deletions
--- a/config/config.md
+++ b/config/config.md
@@ -601,7 +601,7 @@
 | `region_engine.mito.gc` | -- | -- | -- |
 | `region_engine.mito.gc.enable` | Bool | `false` | Whether GC is enabled. Need to be the same with metasrv's `gc.enable` or unexpected behavior will occur |
 | `region_engine.mito.gc.lingering_time` | String | `1m` | Lingering time before deleting files.<br/>Should be long enough to allow long running queries to finish.<br/>If set to None, then unused files will be deleted immediately. |
-| `region_engine.mito.gc.unknown_file_lingering_time` | String | `1h` | Lingering time before deleting unknown files(files with undetermine expel time).<br/>expel time is the time when the file is considered as removed, as in removed from the manifest.<br/>This should only occur rarely, as manifest keep tracks in `removed_files` field<br/>unless something goes wrong. |
+| `region_engine.mito.gc.unknown_file_lingering_time` | String | `1d` | Lingering time before deleting unknown files (files with undetermined expel time).<br/>Only applies during full file listing GC.<br/>This uses the object's last-modified timestamp as a heuristic (strict less-than comparison);<br/>do not configure this value too small in production to avoid deleting pre-manifest files<br/>from in-progress compaction or flush.<br/>For active/open regions, an unknown file is deleted only if its object last-modified time exceeds this TTL.<br/>If the object store does not provide a last-modified timestamp, the file is conservatively kept.<br/>For dropped regions, unknown files are deleted immediately. |
 | `region_engine.file` | -- | -- | Enable the file engine. |
 | `region_engine.metric` | -- | -- | Metric engine options. |
 | `region_engine.metric.sparse_primary_key_encoding` | Bool | `true` | Whether to use sparse primary key encoding. |
--- a/config/datanode.example.toml
+++ b/config/datanode.example.toml
@@ -680,11 +680,15 @@ enable = false
 ## If set to None, then unused files will be deleted immediately.
 lingering_time = "1m"

-## Lingering time before deleting unknown files(files with undetermine expel time).
-## expel time is the time when the file is considered as removed, as in removed from the manifest.
-## This should only occur rarely, as manifest keep tracks in `removed_files` field
-## unless something goes wrong.
-unknown_file_lingering_time = "1h"
+## Lingering time before deleting unknown files (files with undetermined expel time).
+## Only applies during full file listing GC.
+## This uses the object's last-modified timestamp as a heuristic (strict less-than comparison);
+## do not configure this value too small in production to avoid deleting pre-manifest files
+## from in-progress compaction or flush.
+## For active/open regions, an unknown file is deleted only if its object last-modified time exceeds this TTL.
+## If the object store does not provide a last-modified timestamp, the file is conservatively kept.
+## For dropped regions, unknown files are deleted immediately.
+unknown_file_lingering_time = "1d"

 [[region_engine]]
 ## Enable the file engine.
--- a/src/mito2/src/gc.rs
+++ b/src/mito2/src/gc.rs
@@ -65,18 +65,36 @@ fn should_delete_file(
    is_linger: bool,
    is_eligible_for_delete: bool,
    is_region_dropped: bool,
-    _entry: &Entry,
-    _unknown_file_may_linger_until: chrono::DateTime<chrono::Utc>,
+    entry: &Entry,
+    unknown_file_may_linger_until: chrono::DateTime<chrono::Utc>,
 ) -> bool {
-    let is_known = is_linger || is_eligible_for_delete;
+    if is_in_manifest || is_in_tmp_ref {
+        return false;
+    }

-    !is_in_manifest
-        && !is_in_tmp_ref
-        && if is_known {
-            is_eligible_for_delete
-        } else {
-            !is_in_tmp_ref && is_region_dropped
-        }
+    let is_known = is_linger || is_eligible_for_delete;
+    if is_known {
+        return is_eligible_for_delete;
+    }
+
+    // Unknown file: not in manifest, tmp_ref, or known removed records.
+    // For dropped regions, unknown files not protected by manifest/tmp refs/cross-region refs
+    // are deleted immediately. This relies on meta collecting FileRefsManifest from related
+    // active regions before issuing dropped-region GC; preserving young unknown files would
+    // also require keeping the table_repart tombstone for retry.
+    // For active/open regions, only delete if the object's last-modified time exceeds the
+    // unknown_file_lingering_time TTL.
+    if is_region_dropped {
+        return true;
+    }
+
+    entry
+        .metadata()
+        .last_modified()
+        .map(|ts| {
+            ts.into_inner().as_millisecond() < unknown_file_may_linger_until.timestamp_millis()
+        })
+        .unwrap_or(false)
 }

 /// Limit the amount of concurrent GC jobs on the datanode
@@ -156,8 +174,10 @@ impl Default for GcConfig {
            enable: false,
            // expect long running queries to be finished(or at least be able to notify it's using a deleted file) within a reasonable time
            lingering_time: Some(Duration::from_secs(60)),
-            // 1 hours, for unknown expel time, which is when this file get removed from manifest, it should rarely happen, can keep it longer
-            unknown_file_lingering_time: Duration::from_secs(60 * 60),
+            // 1 day, for unknown expel time, which is when this file get removed from manifest.
+            // Only applies to full-listing GC for active/open regions. A long default avoids
+            // accidentally deleting pre-manifest files (e.g. compaction/flush still in progress).
+            unknown_file_lingering_time: Duration::from_secs(24 * 60 * 60),
            max_concurrent_lister_per_gc_job: 32,
            max_concurrent_gc_job: 4,
        }
--- a/src/mito2/src/gc/worker_test.rs
+++ b/src/mito2/src/gc/worker_test.rs
@@ -17,6 +17,8 @@ use std::sync::Arc;

 use api::v1::Rows;
 use common_telemetry::init_default_ut_logging;
+use futures::TryStreamExt;
+use object_store::{Entry, ObjectStore, services};
 use store_api::region_engine::RegionEngine as _;
 use store_api::region_request::{RegionCompactRequest, RegionRequest};
 use store_api::storage::{FileRef, FileRefsManifest, RegionId};
@@ -24,7 +26,7 @@ use store_api::storage::{FileRef, FileRefsManifest, RegionId};
 use crate::config::MitoConfig;
 use crate::engine::MitoEngine;
 use crate::engine::compaction_test::{delete_and_flush, put_and_flush};
-use crate::gc::{GcConfig, LocalGcWorker};
+use crate::gc::{GcConfig, LocalGcWorker, should_delete_file};
 use crate::manifest::action::RemovedFile;
 use crate::region::MitoRegionRef;
 use crate::test_util::{
@@ -419,3 +421,320 @@ async fn test_gc_worker_compact_with_ref() {
    assert_eq!(report.deleted_files.get(&region_id).unwrap().len(), 0);
    assert!(report.need_retry_regions.is_empty());
 }
+
+// --- Tests for unknown_file_lingering_time TTL logic ---
+
+/// Helper to write a dummy parquet file to an in-memory object store and
+/// retrieve its Entry via listing.
+async fn write_and_list_entry(store: &ObjectStore, path: &str) -> Entry {
+    store
+        .write(path, b"dummy_parquet_content".as_slice())
+        .await
+        .unwrap();
+    // List the parent directory to get the entry.
+    let parent = std::path::Path::new(path).parent().and_then(|p| {
+        if p.as_os_str().is_empty() {
+            None
+        } else {
+            Some(p)
+        }
+    });
+    let prefix = match parent {
+        Some(p) => format!("{}/", p.to_str().unwrap()),
+        None => String::new(), // root
+    };
+    let lister = store.lister_with(&prefix).await.unwrap();
+    let entries: Vec<Entry> = lister.try_collect().await.unwrap();
+    let expected_name = std::path::Path::new(path)
+        .file_name()
+        .unwrap()
+        .to_str()
+        .unwrap();
+    match entries.iter().find(|e| e.name() == expected_name) {
+        Some(e) => e.clone(),
+        None => {
+            panic!(
+                "entry '{}' not found when listing prefix '{}'; entries: {:?}",
+                expected_name,
+                prefix,
+                entries.iter().map(|e| e.name()).collect::<Vec<_>>()
+            )
+        }
+    }
+}
+
+/// Test: active/open region unknown file within TTL (last_modified newer than
+/// threshold) should NOT be deleted.
+/// NOTE: Memory backend leaves `last_modified` as `None`, so this test also
+/// covers the "missing last_modified → keep" conservative behavior.
+#[tokio::test]
+async fn test_unknown_file_within_ttl_not_deleted() {
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/1.parquet").await;
+
+    // unknown_file_may_linger_until set to epoch (very old) → file is "too young"
+    // since last_modified is ~now.
+    let threshold = chrono::DateTime::from_timestamp(0, 0).unwrap();
+
+    let should_delete = should_delete_file(
+        false, // not in manifest
+        false, // not in tmp_ref
+        false, // not in may_linger
+        false, // not eligible for delete
+        false, // active region (not dropped)
+        &entry, threshold,
+    );
+    assert!(
+        !should_delete,
+        "Active-region unknown file within TTL should NOT be deleted"
+    );
+}
+
+/// Test: active/open region unknown file exceeding TTL (last_modified older
+/// than threshold) should be deleted.
+#[tokio::test]
+async fn test_unknown_file_exceeded_ttl_deleted() {
+    // Use Fs backend so that last_modified is properly set on file metadata.
+    let tmp_dir = common_test_util::temp_dir::create_temp_dir("gc_unknown_ttl");
+    let root = tmp_dir.path().to_string_lossy().to_string();
+    let builder = services::Fs::default().root(&root);
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "2.parquet").await;
+
+    // threshold set far into the future → any file with last_modified before now
+    // is guaranteed to be older than the threshold.
+    let threshold = chrono::Utc::now() + chrono::Duration::days(1);
+
+    let should_delete = should_delete_file(
+        false, // not in manifest
+        false, // not in tmp_ref
+        false, // not in may_linger
+        false, // not eligible for delete
+        false, // active region (not dropped)
+        &entry, threshold,
+    );
+    assert!(
+        should_delete,
+        "Active-region unknown file exceeding TTL should be deleted"
+    );
+}
+
+/// Test: dropped region unknown file should always be deleted regardless of TTL.
+#[tokio::test]
+async fn test_unknown_file_dropped_region_deleted() {
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/3.parquet").await;
+
+    // Even with threshold far in the past (epoch), dropped region deletes immediately.
+    let threshold = chrono::DateTime::from_timestamp(0, 0).unwrap();
+
+    let should_delete = should_delete_file(
+        false, // not in manifest
+        false, // not in tmp_ref
+        false, // not in may_linger
+        false, // not eligible for delete
+        true,  // region dropped
+        &entry, threshold,
+    );
+    assert!(
+        should_delete,
+        "Dropped region unknown file should be deleted immediately"
+    );
+}
+
+/// Test: file in manifest should NOT be deleted even if unknown.
+#[tokio::test]
+async fn test_file_in_manifest_not_deleted() {
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/4.parquet").await;
+    let threshold = chrono::Utc::now() + chrono::Duration::days(1);
+
+    let should_delete = should_delete_file(
+        true,  // in manifest
+        false, // not in tmp_ref
+        false, false, false, // active region
+        &entry, threshold,
+    );
+    assert!(!should_delete, "File in manifest should NOT be deleted");
+}
+
+/// Test: file in tmp_ref should NOT be deleted even if unknown.
+#[tokio::test]
+async fn test_file_in_tmp_ref_not_deleted() {
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/5.parquet").await;
+    let threshold = chrono::Utc::now() + chrono::Duration::days(1);
+
+    let should_delete = should_delete_file(
+        false, true, // in tmp_ref
+        false, false, false, // active region
+        &entry, threshold,
+    );
+    assert!(!should_delete, "File in tmp_ref should NOT be deleted");
+}
+
+/// Test: known removed file that is still lingering should NOT be deleted.
+/// (is_linger=true but is_eligible_for_delete=false)
+#[tokio::test]
+async fn test_known_file_still_lingering_not_deleted() {
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/6.parquet").await;
+    let threshold = chrono::Utc::now() + chrono::Duration::days(1);
+
+    let should_delete = should_delete_file(
+        false, false, true,  // is_linger
+        false, // not yet eligible for delete
+        false, &entry, threshold,
+    );
+    assert!(
+        !should_delete,
+        "Known file still in lingering period should NOT be deleted"
+    );
+}
+
+/// Test: known removed file eligible for delete should be deleted.
+/// (is_linger=true and is_eligible_for_delete=true)
+#[tokio::test]
+async fn test_known_file_eligible_for_delete_deleted() {
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/7.parquet").await;
+    let threshold = chrono::DateTime::from_timestamp(0, 0).unwrap();
+
+    let should_delete = should_delete_file(
+        false, false, true, // is_linger
+        true, // eligible for delete
+        false, &entry, threshold,
+    );
+    assert!(
+        should_delete,
+        "Known file eligible for delete should be deleted"
+    );
+}
+
+// --- Tests for boundary conditions and explicit "keep" semantics ---
+
+/// Test: when `last_modified` equals the cutoff exactly, the file should be
+/// kept (strict `<` comparison, not `<=`).
+#[tokio::test]
+async fn test_unknown_file_at_cutoff_not_deleted() {
+    // Use Fs backend for real last_modified
+    let tmp_dir = common_test_util::temp_dir::create_temp_dir("gc_at_cutoff");
+    let root = tmp_dir.path().to_string_lossy().to_string();
+    let builder = services::Fs::default().root(&root);
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "cutoff.parquet").await;
+
+    // Set threshold to the actual last_modified → should NOT be deleted (strict <)
+    let actual_mtime_millis = entry
+        .metadata()
+        .last_modified()
+        .map(|ts| ts.into_inner().as_millisecond())
+        .expect("Fs backend must provide last_modified");
+    // Construct a chrono::DateTime at exactly the same millisecond
+    let threshold = chrono::DateTime::from_timestamp_millis(actual_mtime_millis).unwrap();
+
+    let should_delete = should_delete_file(
+        false, // not in manifest
+        false, // not in tmp_ref
+        false, // not in may_linger
+        false, // not eligible for delete
+        false, // active region
+        &entry, threshold,
+    );
+    assert!(
+        !should_delete,
+        "File at exact cutoff (last_modified == threshold) should NOT be deleted (strict < comparison)"
+    );
+}
+
+/// Test: active unknown file with missing `last_modified` (e.g. object store
+/// that does not provide the timestamp) should be conservatively kept.
+#[tokio::test]
+async fn test_missing_last_modified_unknown_kept() {
+    // Memory backend does NOT set last_modified
+    let builder = services::Memory::default();
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "test/missing_mtime.parquet").await;
+    // Verify that last_modified is indeed None
+    assert!(
+        entry.metadata().last_modified().is_none(),
+        "Memory backend must not provide last_modified for this test"
+    );
+
+    // threshold in the far past → should still NOT delete
+    let threshold = chrono::DateTime::from_timestamp(0, 0).unwrap();
+
+    let should_delete = should_delete_file(
+        false, false, false, false, // active unknown
+        false, // active region
+        &entry, threshold,
+    );
+    assert!(
+        !should_delete,
+        "Missing last_modified should keep the file (conservative behavior)"
+    );
+}
+
+/// Test: file in manifest should NOT be deleted even when its object
+/// `last_modified` is very old (far below the TTL cutoff).
+#[tokio::test]
+async fn test_file_in_manifest_old_mtime_kept() {
+    let tmp_dir = common_test_util::temp_dir::create_temp_dir("gc_manifest_old");
+    let root = tmp_dir.path().to_string_lossy().to_string();
+    let builder = services::Fs::default().root(&root);
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "in_manifest.parquet").await;
+    // threshold far in the future → mtime is definitely old, but manifest protects
+    let threshold = chrono::Utc::now() + chrono::Duration::days(1);
+
+    let should_delete = should_delete_file(
+        true,  // in manifest
+        false, // not in tmp_ref
+        false, false, false, // not linger/eligible, active
+        &entry, threshold,
+    );
+    assert!(
+        !should_delete,
+        "File in manifest should NOT be deleted even with old last-modified time"
+    );
+}
+
+/// Test: file in tmp_ref should NOT be deleted even when its object
+/// `last_modified` is very old (far below the TTL cutoff).
+#[tokio::test]
+async fn test_file_in_tmp_ref_old_mtime_kept() {
+    let tmp_dir = common_test_util::temp_dir::create_temp_dir("gc_tmpref_old");
+    let root = tmp_dir.path().to_string_lossy().to_string();
+    let builder = services::Fs::default().root(&root);
+    let store = ObjectStore::new(builder).unwrap().finish();
+
+    let entry = write_and_list_entry(&store, "in_tmp_ref.parquet").await;
+    // threshold far in the future → mtime is definitely old, but tmp_ref protects
+    let threshold = chrono::Utc::now() + chrono::Duration::days(1);
+
+    let should_delete = should_delete_file(
+        false, true, // in tmp_ref
+        false, false, false, // not linger/eligible, active
+        &entry, threshold,
+    );
+    assert!(
+        !should_delete,
+        "File in tmp_ref should NOT be deleted even with old last-modified time"
+    );
+}
--- a/tests-integration/tests/http.rs
+++ b/tests-integration/tests/http.rs
@@ -1970,7 +1970,7 @@ mem_threshold_on_create = "auto"
 {vector_index_config}[region_engine.mito.gc]
 enable = false
 lingering_time = "1m"
-unknown_file_lingering_time = "1h"
+unknown_file_lingering_time = "1day"
 max_concurrent_lister_per_gc_job = 32
 max_concurrent_gc_job = 4