Skip to content

fix(toolkit): keep source DB intact when DbMove copy fails #140

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
halibobo1205 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix(toolkit): keep source DB intact when DbMove copy fails #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
235 changes: 174 additions & 61 deletions plugins/src/main/java/common/org/tron/plugins/DbMove.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@
import com.typesafe.config.ConfigFactory;
import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.util.Arrays;
import java.nio.file.attribute.BasicFileAttributes;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import lombok.extern.slf4j.Slf4j;
import me.tongfei.progressbar.ProgressBar;
import org.tron.plugins.utils.FileUtils;
Expand Down Expand Up @@ -76,77 +80,187 @@ public Integer call() throws Exception {
printNotExist();
return 0;
}
List<Property> toBeMove = dbs.stream()
.map(c -> {
try {
return new Property(c.getString(NAME_CONFIG_KEY),
Paths.get(database.toString(), dbPath, c.getString(NAME_CONFIG_KEY)),
Paths.get(c.getString(PATH_CONFIG_KEY), dbPath, c.getString(NAME_CONFIG_KEY)));
} catch (IOException e) {
spec.commandLine().getErr().println(e);
}
return null;
}).filter(Objects::nonNull)
.filter(p -> !p.destination.equals(p.original)).collect(Collectors.toList());
// Canonical path validation lives here, once every option has its final
// value; ConfigConverter is limited to database-independent checks, so
// option order ('-c' before or after '-d') must not change the result.
List<Property> toBeMove = new ArrayList<>();
for (Config c : dbs) {
try {
toBeMove.add(new Property(c.getString(NAME_CONFIG_KEY),
Paths.get(database.toString(), dbPath, c.getString(NAME_CONFIG_KEY)),
Paths.get(c.getString(PATH_CONFIG_KEY), dbPath, c.getString(NAME_CONFIG_KEY))));
} catch (IOException e) {
spec.commandLine().getErr().println(e);
return 2;
}
}
try {
checkNoNesting(toBeMove);
} catch (IllegalArgumentException e) {
spec.commandLine().getErr().println(e.getMessage());
return 2;
}

if (toBeMove.isEmpty()) {
printNotExist();
return 0;
boolean allCopied = ProgressBar.wrap(toBeMove.stream(), "copy task")
.map(this::copy).reduce(Boolean.TRUE, Boolean::logicalAnd);
if (!allCopied) {
cleanupDestinations(toBeMove);
return 1;
}
toBeMove = toBeMove.stream()
.filter(property -> {
if (property.destination.toFile().exists()) {
spec.commandLine().getOut().println(String.format("%s already exist,skip.",
property.destination));
return false;
} else {
return true;
}
}).collect(Collectors.toList());

if (toBeMove.isEmpty()) {
printNotExist();
return 0;
boolean allMoved = ProgressBar.wrap(toBeMove.stream(), "link task")
.map(this::replaceSourceWithLink).reduce(Boolean.TRUE, Boolean::logicalAnd);
if (!allMoved) {
return 1;
}
ProgressBar.wrap(toBeMove.stream(), "mv task").forEach(this::run);
spec.commandLine().getOut().println("move db done.");

} else {
printNotExist();
return 0;
}
return 0;
}

private void run(Property p) {
if (p.destination.toFile().mkdirs()) {
ProgressBar.wrap(Arrays.stream(Objects.requireNonNull(p.original.toFile().listFiles()))
.filter(File::isFile).map(File::getName).parallel(), p.name).forEach(file -> {
Path original = Paths.get(p.original.toString(), file);
Path destination = Paths.get(p.destination.toString(), file);
try {
Files.copy(original, destination,
StandardCopyOption.REPLACE_EXISTING);
} catch (IOException e) {
spec.commandLine().getErr().println(e);
}
});
private boolean copy(Property p) {
if (!p.destination.toFile().mkdirs()) {
spec.commandLine().getErr().println(String.format("%s create failed.", p.destination));
return false;
}

AtomicBoolean hasError = new AtomicBoolean(false);
try (Stream<Path> files = Files.walk(p.original)) {
ProgressBar.wrap(files.parallel(), p.name).forEach(source -> {
try {
copyEntry(p, source);
} catch (IOException e) {
hasError.set(true);
Comment on lines +131 to +136

@codeant-ai codeant-ai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The copy phase traverses and copies a live database tree without stopping the database, acquiring a lock, or taking a snapshot. Concurrent database writes can produce a destination containing files from inconsistent points in time, after which the source is deleted and replaced by a link. The move must require an offline database or use a consistent database-level snapshot/lock. [incomplete implementation]

Severity Level: Critical 🚨 
- ❌ Running-node moves can produce inconsistent RocksDB copies.
- ❌ Finalization can replace the source with incomplete state.
- ⚠️ Restarted nodes may require database recovery or rollback.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** plugins/src/main/java/common/org/tron/plugins/DbMove.java
**Line:** 131:136
**Comment:**
	*Incomplete Implementation: The copy phase traverses and copies a live database tree without stopping the database, acquiring a lock, or taking a snapshot. Concurrent database writes can produce a destination containing files from inconsistent points in time, after which the source is deleted and replaced by a link. The move must require an offline database or use a consistent database-level snapshot/lock.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

spec.commandLine().getErr().println(e);
}
});
} catch (IOException | UncheckedIOException e) {
hasError.set(true);
spec.commandLine().getErr().println(e);
}

if (hasError.get()) {
spec.commandLine().getErr().println(String.format(
"%s copy to %s failed, source kept.",
p.original, p.destination));
return false;
}
return true;
}

// Classify every entry WITHOUT following links: a symlink or special file
// inside the db dir has no safe copy semantics and must fail the move before
// the source is deleted. Directories are created explicitly so empty (nested)
// directories survive the move; an attribute read failure counts as an error
// instead of silently skipping the entry.
private void copyEntry(Property p, Path source) throws IOException {
BasicFileAttributes attributes = Files.readAttributes(
source, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
Path destination = p.destination.resolve(p.original.relativize(source));
if (attributes.isDirectory()) {
Files.createDirectories(destination);
} else if (attributes.isRegularFile()) {
Files.createDirectories(destination.getParent());
Files.copy(source, destination, StandardCopyOption.REPLACE_EXISTING);
} else {
throw new IOException(String.format(
"%s is neither a regular file nor a directory, can not be moved.", source));
}
}

private boolean replaceSourceWithLink(Property p) {
try {
if (!FileUtils.deleteDirNoFollowLinks(p.original.toFile())) {
spec.commandLine().getErr().println(String.format(
"%s delete failed and may be incomplete; the only complete copy is at %s, keep it.",
p.original, p.destination));
printRecoveryHint(p);
return false;
}
Files.createSymbolicLink(p.original, p.destination);
return true;
} catch (IOException | RuntimeException x) {
spec.commandLine().getErr().println(x);
spec.commandLine().getErr().println(String.format(
"%s move failed; the complete copy is at %s, keep it.",
p.original, p.destination));
printRecoveryHint(p);
return false;
}
}

private void printRecoveryHint(Property p) {
spec.commandLine().getErr().println(String.format(
"To recover manually: remove %s if present, then create a symbolic link at %s"
+ " pointing to %s.",
p.original, p.original, p.destination));
}

private void cleanupDestinations(List<Property> properties) {
boolean allCleaned = properties.stream().map(property -> {
File destination = property.destination.toFile();
// Fail closed: only a confirmed-absent path (no dangling link) is done.
if (Files.notExists(destination.toPath(), LinkOption.NOFOLLOW_LINKS)) {
return true;
}
try {
if (FileUtils.deleteDir(p.original.toFile())) {
Files.createSymbolicLink(p.original, p.destination);
if (FileUtils.deleteDirNoFollowLinks(destination)) {
return true;
}
} catch (IOException | UnsupportedOperationException x) {
spec.commandLine().getErr().println(x);
} catch (RuntimeException e) {
spec.commandLine().getErr().println(e);
}
spec.commandLine().getErr().println(String.format(
"%s cleanup failed; remove the leftover copy before retrying.",
property.destination));
return false;
}).reduce(Boolean.TRUE, Boolean::logicalAnd);

if (allCleaned) {
spec.commandLine().getErr().println(
"move db failed; all source databases were kept, please retry.");
} else {
spec.commandLine().getErr().println(String.format("%s create failed.", p.destination));
spec.commandLine().getErr().println(
"move db failed; all source databases were kept, but leftover copies remain.");
}
}

private void printNotExist() {
spec.commandLine().getErr().println(NOT_FIND);
}

// Reject any overlap in the canonical path graph before mutating anything:
// a destination inside ANY source would be wiped when that source is deleted
// in the link phase; overlapping sources make the parent's deletion remove
// the child's fresh link; overlapping destinations interleave two copies.
private static void checkNoNesting(List<Property> properties) {
for (Property a : properties) {
for (Property b : properties) {
if (b.destination.startsWith(a.original)) {
throw new IllegalArgumentException(String.format(
"destination [%s] can not be inside original [%s],please check!",
b.destination, a.original));
}
if (a == b) {
continue;
}
if (b.original.startsWith(a.original)) {
throw new IllegalArgumentException(String.format(
"original [%s] can not overlap original [%s],please check!",
b.original, a.original));
}
if (b.destination.startsWith(a.destination)) {
throw new IllegalArgumentException(String.format(
"destination [%s] can not overlap destination [%s],please check!",
b.destination, a.destination));
}
}
}
}


static class Property {

Expand All @@ -167,7 +281,9 @@ public Property(String name, Path original, Path destination) throws IOException
throw new IOException(original + " is symbolicLink!");
}
this.destination = destination.toFile().getCanonicalFile().toPath();
if (this.destination.toFile().exists()) {
// Fail closed: File.exists() follows links, so a dangling symlink at the
// destination would pass as absent only to fail mkdirs on every retry.
if (!Files.notExists(this.destination, LinkOption.NOFOLLOW_LINKS)) {
throw new IOException(this.destination + " already exist!");
}
if (this.destination.equals(this.original)) {
Expand Down Expand Up @@ -195,25 +311,22 @@ public Config convert(String value) throws Exception {
if (dbs.isEmpty()) {
throw notFind;
}
String dbPath = config.hasPath(DB_DIRECTORY_CONFIG_KEY)
? config.getString(DB_DIRECTORY_CONFIG_KEY) : DEFAULT_DB_DIRECTORY;

dbs = dbs.stream()
.filter(c -> c.hasPath(NAME_CONFIG_KEY) && c.hasPath(PATH_CONFIG_KEY))
.collect(Collectors.toList());

if (dbs.isEmpty()) {
throw notFind;
}
Set<String> toBeMove = new HashSet<>();
// Only database-independent checks may run at conversion time: the
// static `database` option may not have its final value yet ('-c'
// can be parsed before '-d'). Path validation happens in call().
Set<String> names = new HashSet<>();
for (Config c : dbs) {
if (!toBeMove.add(new Property(c.getString(NAME_CONFIG_KEY),
Paths.get(database.toString(), dbPath, c.getString(NAME_CONFIG_KEY)),
Paths.get(c.getString(PATH_CONFIG_KEY), dbPath,
c.getString(NAME_CONFIG_KEY))).name)) {
String name = c.getString(NAME_CONFIG_KEY);
if (!names.add(name)) {
throw new IllegalArgumentException(
"DB config has duplicate key:[" + c.getString(NAME_CONFIG_KEY)
+ "],please check! ");
"DB config has duplicate key:[" + name + "],please check! ");
}
}
} else {
Expand Down
25 changes: 25 additions & 0 deletions plugins/src/main/java/common/org/tron/plugins/utils/FileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,31 @@ public static boolean deleteDir(File dir) {
return dir.delete();
}

/**
* Delete {@code dir} recursively WITHOUT following symbolic links: a symlink
* is removed itself and its target is left untouched. Use for trees that may
* contain links to data that must survive, e.g. database dirs already turned
* into links by {@code db mv}. {@link #deleteDir(File)} keeps the legacy
* follow-links semantics that DbLite/DbConvert rely on to free space.
*/
public static boolean deleteDirNoFollowLinks(File dir) {
if (Files.isSymbolicLink(dir.toPath())) {
return dir.delete();
}
if (dir.isDirectory()) {
String[] children = dir.list();
if (children == null) {
return false;
}
for (String child : children) {
if (!deleteDirNoFollowLinks(new File(dir, child))) {
return false;
}
}
}
return dir.delete();
}

public static boolean createFileIfNotExists(String filepath) {
File file = new File(filepath);
if (!file.exists()) {
Expand Down
Loading
Loading