Skip to content

Fix folderManager NPE issue in WALNode when starting iotdb in a disk-full state #16869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
OneSizeFitsQuorum wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix folderManager NPE issue in WALNode when starting iotdb in a disk-full state #16869

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f0ad8c
fix
OneSizeFitsQuorum Dec 4, 2025
cd69d06
finish
OneSizeFitsQuorum Dec 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions ...ache/iotdb/db/storageengine/dataregion/wal/allocation/AbstractNodeAllocationStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,21 @@
import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.concurrent.atomic.AtomicReference;

public abstract class AbstractNodeAllocationStrategy implements NodeAllocationStrategy {
private static final Logger logger =
LoggerFactory.getLogger(AbstractNodeAllocationStrategy.class);
private static final CommonConfig commonConfig = CommonDescriptor.getInstance().getConfig();

// manage wal folders
protected FolderManager folderManager;
protected AtomicReference<FolderManager> folderManager = new AtomicReference<>();

protected AbstractNodeAllocationStrategy() {
try {
folderManager =
folderManager.set(
new FolderManager(
Arrays.asList(commonConfig.getWalDirs()), DirectoryStrategyType.SEQUENCE_STRATEGY);
Arrays.asList(commonConfig.getWalDirs()), DirectoryStrategyType.SEQUENCE_STRATEGY));
} catch (DiskSpaceInsufficientException e) {
logger.error(
"Fail to create wal node allocation strategy because all disks of wal folders are full.",
Expand All @@ -57,8 +58,16 @@ protected AbstractNodeAllocationStrategy() {

protected IWALNode createWALNode(String identifier) {
try {
return folderManager.getNextWithRetry(
folder -> new WALNode(identifier, folder + File.separator + identifier));
// already in lock, so no need to synchronized
if (folderManager.get() == null) {
folderManager.set(
new FolderManager(
Arrays.asList(commonConfig.getWalDirs()), DirectoryStrategyType.SEQUENCE_STRATEGY));
Comment on lines +61 to +65
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: Multiple threads can simultaneously check folderManager.get() == null and proceed to create multiple FolderManager instances. The comment on line 61 states "already in lock, so no need to synchronized", but AtomicReference alone doesn't prevent this race condition. The check-then-set pattern is not atomic. Consider using synchronized double-checked locking:

if (folderManager.get() == null) {
  synchronized (folderManager) {
    if (folderManager.get() == null) {
      folderManager.set(new FolderManager(...));
    }
  }
}

Alternatively, use compareAndSet() for atomic updates.

Suggested change
// already in lock, so no need to synchronized
if (folderManager.get() == null) {
folderManager.set(
new FolderManager(
Arrays.asList(commonConfig.getWalDirs()), DirectoryStrategyType.SEQUENCE_STRATEGY));
// Atomically initialize folderManager if null
while (folderManager.get() == null) {
FolderManager newManager = new FolderManager(
Arrays.asList(commonConfig.getWalDirs()), DirectoryStrategyType.SEQUENCE_STRATEGY);
if (folderManager.compareAndSet(null, newManager)) {
break;
}
// else, another thread set it, retry check

Copilot uses AI. Check for mistakes.
}
Comment on lines +62 to +66
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for the disk recovery scenario: The new lazy initialization logic (lines 62-66) that handles disk space recovery is not covered by tests. The existing tests in FirstCreateStrategyTest don't simulate a disk-full-at-startup followed by disk-space-recovery scenario. Consider adding a test that:

  1. Simulates disk full at construction time (folderManager becomes null)
  2. Verifies WALFakeNode is returned initially
  3. Frees disk space
  4. Verifies successful WAL node creation on retry (folderManager is recreated)

Copilot uses AI. Check for mistakes.
return folderManager
.get()
.getNextWithRetry(
folder -> new WALNode(identifier, folder + File.separator + identifier));
Comment on lines +67 to +70
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential NPE: Even after the null check and re-initialization attempt (lines 62-66), folderManager.get() could still be null if the FolderManager constructor throws DiskSpaceInsufficientException again. In this case, line 67 will throw NPE when calling .get().getNextWithRetry(). The exception from line 63-65 is not caught here, so it would propagate up, but if it were caught elsewhere, this would be an NPE risk. Consider adding a null check after the initialization attempt or handling the case where re-initialization fails.

Copilot uses AI. Check for mistakes.
} catch (DiskSpaceInsufficientException e) {
logger.error("Fail to create wal node because all disks of wal folders are full.", e);
return WALFakeNode.getFailureInstance(e);
Expand Down
Loading