Sitelet https://github.com/tronprotocol/java-tron/pull/6946/files
Skip to content

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

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 5 commits into
base: release_v4.8.3
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a4045d5
fix(toolkit): keep source DB intact when DbMove copy fails
halibobo1205 Jun 11, 2026
9ec2584
fix(toolkit): reject db mv destinations inside a selected source
halibobo1205 Sep 29, 2026
e6daa90
fix(toolkit): roll back db mv copies on runtime failures
halibobo1205 Sep 29, 2026
f3deb19
test(toolkit): cover db mv rollback and finalize failure paths
halibobo1205 Sep 29, 2026
5d35137
fix(toolkit): show the stop-the-node note in db mv help
halibobo1205 Sep 29, 2026
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
4 changes: 4 additions & 0 deletions plugins/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

This package contains a set of tools for TRON, the followings are the documentation for each tool.

NOTE: All `db` tools operate directly on the database files. Before performing a database
operation (archive, convert, copy, lite, mv, root), you must stop the currently running
FullNode service.

## DB Archive(Requires x86 + LevelDB)

DB archive provides the ability to reformat the manifest according to the current `database`, parameters are compatible with the previous `ArchiveManifest`.
Expand Down
5 changes: 5 additions & 0 deletions plugins/src/main/java/common/org/tron/plugins/Db.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
mixinStandardHelpOptions = true,
version = "db command 1.0",
description = "An rich command set that provides high-level operations for dbs.",
header = Db.STOP_NODE_HEADER,
subcommands = {CommandLine.HelpCommand.class,
DbMove.class,
DbArchive.class,
Expand All @@ -17,4 +18,8 @@
commandListHeading = "%nCommands:%n%nThe most commonly used db commands are:%n"
)
public class Db {

static final String STOP_NODE_HEADER = "All db tools operate directly on the database files.\n"
+ "Before performing a database operation,\n"
+ "you must stop the currently running FullNode service.\n";
}
206 changes: 145 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 @@ -5,16 +5,19 @@
import java.io.File;
import java.io.IOException;
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 All @@ -23,6 +26,7 @@

@Slf4j(topic = "move")
@Command(name = "mv", aliases = "move",
header = Db.STOP_NODE_HEADER,
description = "Move db to pre-set new path . For example HDD,reduce storage expenses.")
public class DbMove implements Callable<Integer> {

Expand Down Expand Up @@ -76,77 +80,163 @@ 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());

if (toBeMove.isEmpty()) {
printNotExist();
return 0;
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;
}
}
if (hasNestedDestination(toBeMove)) {
return 2;
}
boolean allCopied = ProgressBar.wrap(toBeMove.stream(), "copy task")
.allMatch(this::copy);
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) {
AtomicBoolean hasError = new AtomicBoolean(false);
try (Stream<Path> files = Files.walk(p.original)) {
// Collect the tree before copying anything: a traversal failure must
// surface while no copy task is in flight, otherwise a task started
// before the failure could recreate the destination after the rollback
// has already deleted it, and the retry would then be rejected.
List<Path> sources = files.collect(Collectors.toList());
Files.createDirectories(p.destination);
ProgressBar.wrap(sources.parallelStream(), p.name).forEach(source -> {
if (hasError.get()) {
return;
}
try {

@waynercheung waynercheung Sep 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SHOULD] A runtime exception during a destination write, such as SecurityException under a SecurityManager, escapes this handler and bypasses rollback. I reproduced exit 1 with all sources intact but completed and partial destinations left behind; retry after removing the denial returns 2. Please handle expected filesystem runtime failures inside the workers and during destination creation/traversal, then roll back after all workers have finished. An outer finally alone does not ensure outstanding parallel copies have stopped.

@halibobo1205 halibobo1205 Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e6daa90: the copy phase now handles runtime exceptions like I/O errors. Workers catch them, so the parallel forEach only returns after every worker has finished, and destination creation (Files.createDirectories) moved into the same handler; rollback runs afterwards. Same repro now: exit 1, no leftovers, and the unchanged retry returns 0.

I did not add a regression test for this path: raising a RuntimeException from the JDK file APIs here requires a SecurityManager, which is disabled by default since JDK 17 and removed in JDK 24. The shared failure path is covered by the existing I/O-failure tests.

copyEntry(p, source);
} catch (IOException | RuntimeException e) {
hasError.set(true);
spec.commandLine().getErr().println(e);
}
});
} catch (IOException | RuntimeException 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;
}

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.deleteDir(p.original.toFile())) {

@warku123 warku123 Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great fix for the copy-failure case. One adjacent gap worth flagging while you're in here: FileUtils.deleteDir (plugins utils/FileUtils.java:73-84) still recurses with java.io.File, and isDirectory() follows symlinks — so a directory symlink gets traversed and its target's contents are deleted.

This composes with this PR: after db mv, the original path is a symlink to the moved DB (DbMove.java:176). If a later db lite merge deletes the archive dbs under that original path (DbLite.backupArchiveDbs calls FileUtils.deleteDir per db), it wipes the already-migrated DB on the new disk — two legitimate operations, chained data loss, no attacker needed.

Suggested fix: replace the recursion with Files.walkFileTree + NOFOLLOW_LINKS (delete the link itself, never follow). The copy phase already uses NOFOLLOW_LINKS (:154/:198), so this would make the delete side consistent. The same pattern also exists in common/.../FileUtil.java:114-126 and the toolkit copy in tron-docker's tools/toolkit — happy to split that into a separate PR if you prefer.

@halibobo1205 halibobo1205 Sep 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for flagging this. The symlink traversal issue is valid, but it predates this PR: db mv already created symlinks, and this change does not modify FileUtils.deleteDir or the db lite merge workflow.

I’ll keep this PR focused on preventing source deletion after copy failures and reporting migration failures correctly. The deletion behavior and lite merge’s handling of relocated databases should be addressed in a separate PR, with regression coverage for the combined workflow.

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();
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.deleteDir(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);
}

private boolean hasNestedDestination(List<Property> properties) {
for (Property source : properties) {
for (Property target : properties) {
if (target.destination.startsWith(source.original)) {
spec.commandLine().getErr().println(String.format(
"destination [%s] can not be inside original [%s], please check!",
target.destination, source.original));
return true;
}
}
}
return false;
}


static class Property {

Expand All @@ -167,7 +257,7 @@ public Property(String name, Path original, Path destination) throws IOException
throw new IOException(original + " is symbolicLink!");
}
this.destination = destination.toFile().getCanonicalFile().toPath();

@waynercheung waynercheung Sep 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[MUST] Please reject destinations inside any selected source before copying. With configuration order B, A and A's destination under B's source, B is copied before A's destination exists. Finalizing B deletes A's copy; finalizing A then deletes its original, leaving a dangling link while returning 0. I reproduced this regression: the same configuration preserves A's data on the base. Cross-check the canonical paths with destination.startsWith(source) before creating any destination, return 2 for unsupported layouts, and add self-nesting and cross-database regression tests.

@halibobo1205 halibobo1205 Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed, and it is order-dependent on the base as well. Same layout, both configuration orders:

order base before this fix
B, A (A's destination under B's source) A kept A lost, exit 0
A, B A lost, exit 0 A kept

A destination inside any selected source is now rejected up front with exit 2, regardless of order (9ec2584). Added self-nesting and cross-database tests covering both orders.

I'll update the design writeup in #6940 accordingly. It previously listed this layout as a non-goal, leaving the outcome to the operator and outside the guarantees; it is now rejected before anything is written.

if (this.destination.toFile().exists()) {
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,9 +285,6 @@ 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());
Expand All @@ -207,13 +294,10 @@ public Config convert(String value) throws Exception {
}
Set<String> toBeMove = 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 (!toBeMove.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
Loading
Loading