fix(core): fix spurious table metadata reload failures - #7312
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
kafka1991
added
Bug
kafka1991
changed the title
Code review (level 3 — full pass)Verdict: approve. Correct, minimal, well-targeted fix with a strong deterministic regression test. No Critical or Moderate issues. One Minor portability note. What the change does
Verification performed
Minor
Assert.assertEquals(CairoException.ERRNO_FILE_DOES_NOT_EXIST, e.getErrno()); // == 2
SummaryCorrect fix, complete for the documented bug, with a deterministic platform-independent regression test that fails before and passes after. No regressions or tradeoffs — the change only affects the error path and has no performance impact. The substantive verification was out-of-diff (sweep of sibling 🤖 Generated with Claude Code |
[PR Coverage check]😍 pass : 2 / 2 (100.00%) file detail
|
bluestreak01
left a comment
bluestreak01
left a comment
There was a problem hiding this comment.
Approve.
Correct, minimal, complete fix. MemoryCMRImpl.of() captured errno after the cleanup close() on the length-failure path; since errno is thread-local native state and close() issues a native close(2) that overwrites it, the CairoException carried the cleanup's errno instead of length()'s. handleMetadataLoadException() keys retry-vs-fatal off isFileCannotRead(), so a transient ENOENT got misclassified as fatal (the flaky testMetadataFileDoesNotExist2). Capturing errno into a local before close() is the right fix.
Verified out-of-diff:
- Other throw sites in both
of()variants build their CairoException at the failure site (openRO,mapRO) before any cleanup; the outer catch rethrows an already-built exception. The length-from-fd path was the only affected site. - Swept sibling errno sites:
MemoryCMORImpl.growToFileSize,MemoryPMARImpl.truncate,TableTransactionLog(close() does no native work after a failed mmap; fds are locals closed in finally after the errno read),Mig607,MapWriter,TableWriter.closeRemove,DatabaseCheckpointAgent— all read errno from a fresh failing syscall or do no native cleanup first. This is the only genuine occurrence.
The regression test is deterministic and platform-independent: length() override returns ENOENT(2), close() override clobbers errno to EBADF(9) via closeDetached(-1), matching the reported pre-fix expected:<2> but was:<9>. Correct alphabetical placement, try-with-resources inside assertMemoryLeak, portable isErrnoFileDoesNotExist assertion.
No Critical or Moderate issues. No regressions or performance impact.
bluestreak01
added
READY
3 participants
Problem
MemoryCMRImpl.of()readerrnoafter the cleanupclose()call when a file's length could not be read:errnois thread-local global state. The interveningclose()performs a nativeclose()syscall (whenever the fd-cache reference count for the descriptor drops to zero), which can overwrite theerrnoleft by the failedlength(). The exception then carried the cleanup call'serrnoinstead of the real one.This matters because metadata reload classifies the failure by
errno:TableUtils.handleMetadataLoadException()retries the read (until the spin-lock timeout, then reports a clean "Metadata read timeout") only whileCairoException.isFileCannotRead()is true. With a clobberederrno, a transient "file does not exist" condition — for example a_metafile that is briefly unreadable while a concurrent metadata change swaps it — was misclassified as fatal, and the lower-level "could not get length" error surfaced instead of the read being retried.The behavior was non-deterministic because whether
close()overwriteserrnodepends on transient fd-cache state, which is why it showed up as a flakyTableReaderTest#testMetadataFileDoesNotExist2.Found in
Fix
Capture
errnoimmediately after the failedlength()call, beforeclose(). This mirrors the existing convention in the nativerenamepath infiles.c, which saves and restoreserrnoaroundclose().A wider sweep of
errnoreads that follow a cleanup call turned up no other genuine occurrences: the look-alikes either run a fresh failing syscall between the cleanup and theerrnoread (so the value is already correct), or the interveningclose()does no native work on the failure path (e.g.TableTransactionLogonlymunmaps when a mapping exists, and the mmap had just failed).Tradeoffs
The change is a one-line reordering. It has no effect other than carrying the correct
errno, and no performance impact.Test plan
CairoMemoryTest#testSmallFileReportsLengthErrnoNotCloseErrno. It drivesMemoryCMRImpl.smallFile()through aFilesFacadewhoselength()fails with ENOENT and whoseclose()deterministically clobberserrnowith EBADF, then asserts the thrownCairoExceptionreports thelength()errno (isFileCannotRead()true). The reproduction is deterministic and platform-independent.expected:<2> but was:<9>) and passes after.TableReaderTest#testMetadataFileDoesNotExist2, the originally flaky test, passes.