Skip to content

Commit 178fbe9

Browse files
committed
fix(qwp): close SF manifest fd exactly once when quarantine fails in open()
In the no-valid-record branch, SfManifest.open() closed the fd and then called quarantineDebris(), which throws when both rename and remove fail (e.g. a permission-degraded slot dir). The enclosing catch then closed the same fd a second time. With concurrent threads opening files, the OS can reuse the fd number in that window, so the second close could kill an unrelated live descriptor and silently corrupt whatever it backed. Mark ownership released (fd = -1) after the first close and guard the catch, so the fd is closed exactly once on every path. Adds a SegmentRecoveryIntegrityTest case: correctly-sized manifest with both records CRC-broken and a facade failing rename+remove asserts the manifest fd is closed exactly once (fails with 2 closes pre-fix).
1 parent 8d962e0 commit 178fbe9

2 files changed

Lines changed: 102 additions & 1 deletion

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/sf/cursor/SfManifest.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,13 +145,21 @@ static SfManifest open(FilesFacade filesFacade, String dir) {
145145
// bit rot), recovery still fails closed on the
146146
// manifest-required flag check.
147147
filesFacade.close(fd);
148+
// Ownership released: quarantineDebris may throw (when both
149+
// rename and remove fail) and the catch below must not close
150+
// this fd again -- the OS may already have handed the number
151+
// to another thread, and a double-close would silently kill
152+
// an unrelated descriptor.
153+
fd = -1;
148154
quarantineDebris(filesFacade, path, "no valid CRC-protected record");
149155
return null;
150156
}
151157
return new SfManifest(filesFacade, path, fd, selected.generation,
152158
selected.headBase, selected.activeBase);
153159
} catch (Throwable t) {
154-
filesFacade.close(fd);
160+
if (fd != -1) {
161+
filesFacade.close(fd);
162+
}
155163
throw t;
156164
} finally {
157165
Unsafe.free(buffer, RECORD_SIZE, MemoryTag.NATIVE_DEFAULT);

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/sf/cursor/SegmentRecoveryIntegrityTest.java

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -464,6 +464,66 @@ public void testZeroSegmentFilesWithUncollapsedBoundariesFailsClosed() throws Ex
464464
});
465465
}
466466

467+
@Test
468+
public void testUnquarantinableCorruptManifestClosesFdExactlyOnce() throws Exception {
469+
TestUtils.assertMemoryLeak(() -> {
470+
// Correctly-sized manifest with BOTH records CRC-broken: open()
471+
// closes the fd and then tries to quarantine the debris. Make
472+
// rename AND remove fail (permission-degraded slot dir) so
473+
// quarantineDebris throws after the fd is already closed. The
474+
// propagating failure must not close the fd a second time -- the
475+
// OS may already have handed that number to another thread, and
476+
// a double-close would silently kill an unrelated descriptor.
477+
writeManifestBothRecordsCrcBroken();
478+
Map<String, byte[]> before = snapshotDir();
479+
480+
String manifestPath = tmpDir + "/" + MANIFEST_NAME;
481+
int[] manifestFd = {-1};
482+
int[] manifestFdCloses = {0};
483+
FilesFacade facade = new DelegatingFacade() {
484+
@Override
485+
public int close(int fd) {
486+
if (fd >= 0 && fd == manifestFd[0]) {
487+
manifestFdCloses[0]++;
488+
}
489+
return super.close(fd);
490+
}
491+
492+
@Override
493+
public int openRW(String path) {
494+
int fd = super.openRW(path);
495+
if (manifestPath.equals(path)) {
496+
manifestFd[0] = fd;
497+
}
498+
return fd;
499+
}
500+
501+
@Override
502+
public boolean remove(String path) {
503+
return !manifestPath.equals(path) && super.remove(path);
504+
}
505+
506+
@Override
507+
public int rename(String oldPath, String newPath) {
508+
return manifestPath.equals(oldPath) ? -1 : super.rename(oldPath, newPath);
509+
}
510+
};
511+
try {
512+
SegmentRing ring = SegmentRing.openExisting(facade, tmpDir, SEGMENT_SIZE);
513+
if (ring != null) {
514+
ring.close();
515+
}
516+
Assert.fail("recovery must fail when corrupt-manifest quarantine cannot proceed");
517+
} catch (MmapSegmentException expected) {
518+
TestUtils.assertContains(expected.getMessage(), "could not quarantine");
519+
}
520+
Assert.assertTrue("manifest was never opened", manifestFd[0] >= 0);
521+
Assert.assertEquals("manifest fd must be closed exactly once (a double-close can "
522+
+ "kill an unrelated descriptor)", 1, manifestFdCloses[0]);
523+
assertDirUnchanged(before);
524+
});
525+
}
526+
467527
@Test
468528
public void testFreshStartCrashBeforeManifestCreationRecoversViaLegacyPath() throws Exception {
469529
TestUtils.assertMemoryLeak(() -> {
@@ -643,6 +703,39 @@ private void writeManifest(long generation, long headBase, long activeBase) {
643703
}
644704
}
645705

706+
/**
707+
* Writes a correctly-sized (128-byte) manifest whose A and B records are
708+
* BOTH structurally plausible (magic, version, boundaries) but fail their
709+
* CRC check -- the "no valid CRC-protected record" quarantine trigger.
710+
*/
711+
private void writeManifestBothRecordsCrcBroken() {
712+
String path = tmpDir + "/" + MANIFEST_NAME;
713+
int fd = Files.openRW(path);
714+
Assert.assertTrue("could not create manifest", fd >= 0);
715+
try {
716+
Assert.assertTrue(Files.truncate(fd, 128));
717+
long buf = Unsafe.malloc(64, MemoryTag.NATIVE_DEFAULT);
718+
try {
719+
for (long generation = 1; generation <= 2; generation++) {
720+
Unsafe.getUnsafe().setMemory(buf, 64, (byte) 0);
721+
Unsafe.getUnsafe().putInt(buf, 0x314d4653); // SFM1
722+
Unsafe.getUnsafe().putInt(buf + 4, 1); // version
723+
Unsafe.getUnsafe().putLong(buf + 8, generation);
724+
Unsafe.getUnsafe().putLong(buf + 16, 0); // headBase
725+
Unsafe.getUnsafe().putLong(buf + 24, 2); // activeBase
726+
int crc = Crc32c.update(Crc32c.INIT, buf, 60);
727+
Unsafe.getUnsafe().putInt(buf + 60, crc + 1); // broken CRC
728+
long offset = (generation & 1L) * 64;
729+
Assert.assertEquals(64, Files.write(fd, buf, 64, offset));
730+
}
731+
} finally {
732+
Unsafe.free(buf, 64, MemoryTag.NATIVE_DEFAULT);
733+
}
734+
} finally {
735+
Files.close(fd);
736+
}
737+
}
738+
646739
/**
647740
* Snapshot of the durable SF payload files (segments, quarantined
648741
* segments, manifest): name -> content. Lifecycle noise such as the slot

0 commit comments

Comments
 (0)