fix(cubestore): stop reporting remote files nothing references

[waralexrom]

Summary

Two paths log an ERROR for a file that was removed legitimately. The startup warmup takes one metastore snapshot and then walks it one download at a time, so on a node with many partitions the pass runs long enough that whatever compaction replaces meanwhile is already gone from remote storage by the time the pass asks for it. And GCTask::RemoveRemoteFile logs an error whenever the object is already gone, which on GCS is a 404 where S3 answers 204. Neither is a problem, and together they bury the case that is one: a file the metastore still points at.

Changes

• CubeErrorCauseType::FileNotFound for an absent remote object. It still counts as corrupt data for is_corrupt_data(), and it keeps the name and index of CorruptData everywhere it leaves the process: the wire, which a node of any version has to be able to read, and Display, whose output scheduler matches on to deactivate a table whose import job failed. A listing stays the only classifier, since object stores answer 404 both for a missing key and for a missing bucket, and mistaking the second for the first would deactivate every table.
• GCS reports a delete of an already absent object as done, the way S3's 204 does, and still drops the local copy. A missing bucket carries the same reason code and the client crate keeps the detail private, so the two cannot be told apart there; a bucket that is not there fails every upload and download loudly anyway.
• The warmup asks the metastore what the file's row looks like now, in batches of 256 through the out of queue readers. An id they leave out is a deleted row and an inactive row is compaction having moved on — both debug. A row that still names the file is data that is really missing and stays an error, reported per batch so that a table whose objects were really removed does not produce a line per chunk. A read that fails is reported as a check that did not happen, rather than folded into either verdict.
• cs.warmup.missing.stale and cs.warmup.missing.active. The second is the signal worth alerting on.

Testing

• cargo test -p cubestore --lib — 320 pass.
• New tests: three pin the wire and Display compatibility of the new cause; two pin the GCS detection against the payload a bucket really answers with, since the reason mapping lives in the client crate; one drives a table through compaction, so the stale-vs-active classification is checked against what compaction actually does to the rows rather than against an assumption about it.

Heads up for CI: metastore::tests::delete_old_snapshots and sql::tests::decimal_partition_pruning flake on repeated parallel runs without this branch too — same rate with the new test skipped.

[claude]

Claude finished @waralexrom's task in 5m 35s —— View job

---

Review: no blocking issues — 0 high, 1 medium, 3 low (4 inline comments)

The compatibility story checks out: I verified scheduler/mod.rs:903 matches on the "CorruptData" substring of to_string(), which external_repr() preserves, and get_partitions_out_of_queue / get_chunks_out_of_queue (metastore/mod.rs:3915-3943) really do skip ids they no longer hold — so the deleted-row-vs-unreachable-metastore distinction the batching relies on is sound.

Full review — findings, verification notes, and checklist

Checklist

• Gather context (diff, PR body, repo instructions)
• Verify the wire/Display compatibility claim against actual consumers
• Verify the stale-vs-active classification against the metastore row semantics
• Review the GCS delete change
• Review test coverage
• Post inline comments and summary

Verification notes

The stale/active classification is correct, including a subtlety worth recording. Recheck-by-id is only valid because a row's file name is stable: suffix is assigned once in Partition::new/new_child/Chunk::new and never mutated (only active, main_table_row_count, file_size, warmed_up, last_used change). If suffix were mutable, an active row could legitimately name a different file than the one the pass failed to fetch, and every such row would be reported as missing data.

has_main_table_file() is also the right predicate rather than is_active(), and not by coincidence — it's active && main_table_row_count != 0, exactly what get_full_name() gates the download on. So the check asks "does the row still name this file", not "is the row alive", which is what the doc comment claims.

stale = absent.len() - active cannot underflow: active_* are filtered subsets of the same vectors.

GCS. Treating an absent object as a completed delete matches S3's 204 and still drops the local copy. The missing-bucket conflation is real but correctly argued away in the PR body — a wrong bucket fails every upload and download loudly.

Tests. Good coverage of the parts that are easy to get wrong later: byte-equality of the flexbuffer against corrupt_data, the Display rendering the scheduler greps, and a fixture of the payload GCS really returns so a reason-mapping change in the client crate breaks a test instead of the scheduler. Driving the classification through real CompactionService::compact rather than hand-deactivating rows is the right call.

Gaps: report_absent_warmup_files itself is untested (the per-batch logging, the metric split, and the ABSENT_BATCH flush at partition boundaries all go unexercised), as is the queue.rs file_not_found path. Not blocking, but the flush is the one bit of arithmetic in the change. Minor: chunk_id + 1000 as a stand-in for a nonexistent id is fragile — u64::MAX would be unambiguous.

Findings

Sev	Where	Issue
Medium	cluster/mod.rs:2193-2200	A failed recheck discards the batch and warns without naming any ids — and this runs at startup, when the RPC to the router is most likely to blip
Low	remotefs/queue.rs:294	? on the listing probe masks the original download error; now load-bearing for the classification
Low	cluster/mod.rs:2161	The trailing flush is skipped by the two is_cancelled() returns; the comment only reads correctly if you know that
Low	cluster/mod.rs:2173	The surviving error line doesn't name the file; warmup_file_is_absent reads as a pure predicate but logs

Not verified

I did not compile or run the test suite — cubestore has no prebuilt target/ in this checkout and a cold cargo check on this dependency tree exceeds the time available here. The reasoning above is from reading the code and its consumers; treat the author's reported cargo test -p cubestore --lib result as the build signal.

Security: nothing relevant — no new inputs, no auth paths, no user-controlled data. Performance: two extra out-of-queue metastore reads per 256 absent files, against one download each, so negligible; memory is bounded by the batch plus one partition's chunks.

· branch `cubestore-fix-warmup-error-log`

[claude]

A failed recheck discards the whole batch, including the ids that may be the genuinely-missing ones. This runs at startup, i.e. exactly when the RPC to the router is most likely to blip, so it isn't a rare path — and there's no retry and no second pass, so for that run the active signal for those ids is lost with only a warn that names no ids.

Cheap mitigation: log the ids alongside the count (they're bounded by ABSENT_BATCH), so an operator chasing a missing table has something to check by hand:

Suggested change

	Err(e) => {
	// Assuming either verdict would either hide missing data or report a batch of live
	// files as lost, so report that the check itself did not happen.
	log::warn!(
	"Could not check {} absent warmup file(s) against the metastore: {}",
	absent.len(),
	e
	);
	Err(e) => {
	// Assuming either verdict would either hide missing data or report a batch of live
	// files as lost, so report that the check itself did not happen.
	log::warn!(
	"Could not check {} absent warmup file(s) against the metastore: {}. \
	Unchecked partitions: {:?}, chunks: {:?}",
	absent.len(),
	e,
	absent.partition_ids,
	absent.chunk_ids
	);
	return;
	}

Fix this →

[claude]

The ? on the listing means a failed list replaces the original download error entirely, so the caller sees "connection reset while listing" instead of the actual download failure. Pre-existing, but this classification is now load-bearing: with the new warmup path, a flaky list also silently downgrades a real FileNotFound into a generic error (and vice versa hides the true cause). Preserving err when the probe itself fails keeps both signals honest:

Suggested change

	if self.remote_fs.list(file.clone()).await?.is_empty() {
	match self.remote_fs.list(file.clone()).await {
	Ok(listing) if listing.is_empty() => {
	return Err(CubeError::file_not_found(format!(
	"File {} doesn't exist in remote file system",
	file
	)));
	}
	Ok(_) => {}
	Err(list_err) => {
	log::warn!(
	"Could not check whether {} exists in remote fs: {}",
	file,
	list_err
	);
	}
	}

Fix this →

[claude]

Nit, but this is the one line an operator now sees for a real warmup failure. ack_error! logged the same bare "Error: {:?}", so it's parity — yet the surrounding code just gained the file identity, and only some CubeErrors from download_file carry the path in their message (the size-check ones do; an underlying io/transport error may not). Since the caller knows the remote path, passing it in would make the surviving error line self-contained.

Also worth noting: warmup_file_is_absent reads as a pure predicate but logs as a side effect — a name like report_unless_absent (or returning an enum) would say what it does.

[claude]

The comment sits above the flush, but the two is_cancelled() returns at L2124/L2137 mean this line never runs on a cancelled pass — so the intent reads correctly only if you already know those returns exist. Consider stating it at the return sites, or restructuring the loop to break so the single flush covers both exits. Behaviourally fine (shutdown, next start re-walks), just easy to misread.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.28%. Comparing base (528d755) to head (e1d4957).
⚠️ Report is 35 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (528d755) and HEAD (e1d4957). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (528d755)	HEAD (e1d4957)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11601       +/-   ##
===========================================
- Coverage   79.58%   59.28%   -20.31%     
===========================================
  Files         480      228      -252     
  Lines       99544    18200    -81344     
  Branches     3636     3681       +45     
===========================================
- Hits        79224    10790    -68434     
+ Misses      19801     6861    -12940     
- Partials      519      549       +30     

Flag	Coverage Δ
cube-backend	59.28% <ø> (+<0.01%)	⬆️
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.