CASSSIDECAR-484: Fix the spurious oldest segment age in CdcRawDirectorySpaceCleaner

[Klose6]

CdcRawDirectorySpaceCleaner re-reads segment metadata via File.lastModified() after the initial listFiles() scan. If Cassandra (or any other actor) reclaims a segment in the interim, the File.lastModified() will return 0, and nowInMillis - 0 collapses to wall-clock milliseconds.

[jyothsnakonisa]

Thanks for the patch @Klose6 left some minor comments

[jyothsnakonisa]

"segment might already be gone" is represented by lastModified == 0. There are two if conditions that you are adding on the lastModified == 0 condition to fix the bug. This works but what do you think about this alternative?

In the listing segment files pipeline, why don't you filter out segment files which are no longer present, so that you don't need to have if conditions later on?

Suggested change

	.map(CdcRawSegmentFile::createSegmentFile)
	.filter(Objects::nonNull)
	.filter(

[jyothsnakonisa]

CdcRawSegmentFile.createSegmentFile could be a static method to create segment files by reading file attributes atomically to avoid race conditions. Could be something like the following.

static CdcRawSegmentFile createSegmentFile(File logFile)
        {
            try
            {
                BasicFileAttributes attrs = Files.readAttributes(logFile.toPath(), BasicFileAttributes.class);
                return new CdcRawSegmentFile(logFile, attrs.size(), attrs.lastModifiedTime().toMillis());
            }
            catch (IOException e)
            {
                // most commonly NoSuchFileException: segment was reclaimed concurrently
                LOGGER.warn("Skipping cdc segment that disappeared before its attributes could be read path={}",
                            logFile, e);
                return null;
            }
        }

[Klose6]

Hi, thanks for the review! Here the issue we found is sometime the oldestSegmentAge metrics emitted is a very large and unrealistic value(like~50 years) which indicates the segmentFiles.get(0).lastModified()) is 0, this is caused by the segment file was deleted between the listing and deletion, and add the filter in the listing should not help here because the segment can still be gone later before this deletion, so we would like to add the check to fix the spurious value in the metrics here.

[jyothsnakonisa]

Thanks for the reply — but I think this is already handled now that CdcRawSegmentFile reads attributes atomically (from my other comment). Nothing reads the file again after that — the age check and delete loop both use the saved value, not a fresh read. So a segment disappearing after listing can't cause lastModified == 0 anymore.

The only way lastModified is still 0 is if reading attributes fails during listing itself — which is exactly what my filter suggestion catches.

And if a segment gets deleted after listing but before we delete it, that's already safe — Files.deleteIfExists() just no-ops.

So instead of the three lastModified > 0 checks, we could filter out unreadable segments at listing time. That also avoids 0 meaning two things — "file is gone" vs. a genuinely real epoch timestamp, which right now would never get cleaned up.

[jyothsnakonisa]

One more thing worth calling out: filtering also removes the need for the directorySizeBytes -= length special-casing in the delete loop. Right now, when lastModified == 0, the loop assumes the file's already gone and discounts its size from the local budget using the stale cached length — but if that ever turns out to be a false positive (a file with a real epoch mtime that's still on disk), the loop would wrongly believe space was freed and could stop cleaning up too early.

With filtering, that branch goes away entirely. A file that fails readAttributes() just isn't in the list, so it's never counted in the first place. And a file with a genuinely real epoch mtime stays in the list and gets treated like any other old segment — normal age/delete logic applies, no special-casing needed.

[jyothsnakonisa]

indexLength() still calls File.length() live, unlike length()/lastModified() which are now frozen at construction. Probably fine since idx files aren't reclaimed by Cassandra the same way — just flagging for consistency.