fix: refuse to start when the guild data cannot be read

build.gradle.kts

@@ -612,6 +612,23 @@ plugwright {
                 file("plugins/Guilds/config.yml", file("src/test/e2e/server/plugins/Guilds/config.yml"))
                 file("plugins/Guilds/tiers.yml", file("src/test/e2e/server/plugins/Guilds/tiers.yml"))
                 file("plugins/Guilds/buffs.yml", file("src/test/e2e/server/plugins/Guilds/buffs.yml"))
+
+                /*
+                 * Two guild files the plugin finds in its data folder when it starts.
+                 *
+                 * One of them is a guild that reads, and one of them is a file that does not parse.
+                 * The second one is the case `startup-loading.spec.ts` is about: a single file the
+                 * plugin cannot read must not stop it, must not cost it the guilds that did read,
+                 * and must be left on disk untouched.
+                 */
+                file(
+                    "plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json",
+                    file("src/test/e2e/server/plugins/Guilds/data/11111111-1111-1111-1111-111111111111.json"),
+                )
+                file(
+                    "plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json",
+                    file("src/test/e2e/server/plugins/Guilds/data/22222222-2222-2222-2222-222222222222.json"),
+                )
             }
 
             plugins {

docs/e2e-coverage.md

@@ -3,7 +3,7 @@
 Which Guilds feature is tested where, and what is deliberately left out. See `e2e-testing.md` for
 how to run any of it.
 
-141 tests across 12 spec files, plus 3 preflight tests that run before every suite and fail the run
+162 tests across 14 spec files, plus 3 preflight tests that run before every suite and fail the run
 if Guilds or any of its hard dependencies did not load.
 
 ## Preflight — `plugins/guilds-preflight.spec.ts`
@@ -17,7 +17,7 @@ meaningful.
 | The economy and permission providers Vault needs are registered | Vault's `Economy` and `Permission` services resolve |
 | Guilds commands are reachable and its language is English | `/guild` answers, and does so in English |
 
-## A. Guild lifecycle — `tests/lifecycle.spec.ts` (19)
+## A. Guild lifecycle — `tests/lifecycle.spec.ts` (21)
 
 | Requirement | Test |
 | --- | --- |
@@ -32,7 +32,7 @@ meaningful.
 | Status and information commands | toggles the guild between public and private / changes the guild prefix / sets, shows and clears the guild MOTD / reports guild membership through the guild list and rejects guild-scoped commands elsewhere |
 | Name blacklist | rejects a guild name that is a substring of a blacklisted word / accepts a guild name that contains a blacklisted word |
 
-## B. Membership management — `tests/membership.spec.ts` (19)
+## B. Membership management — `tests/membership.spec.ts` (21)
 
 | Requirement | Test |
 | --- | --- |
@@ -51,7 +51,7 @@ meaningful.
 | Guild capacity | a full guild refuses another member |
 | Permissions after leaving or being removed | a player who leaves loses the permissions their guild granted them / a player who is kicked loses the permissions their guild granted them |
 
-## C. Roles and permissions — `tests/roles.spec.ts` (8), `tests/admin.spec.ts` (7)
+## C. Roles and permissions — `tests/roles.spec.ts` (8), `tests/admin.spec.ts` (8)
 
 | Requirement | Test |
 | --- | --- |
@@ -128,7 +128,7 @@ meaningful.
 | Alliance chat between guilds | `chat.spec.ts`, ally chat reaches an allied guild and not the third party |
 | Cleanup on deletion | a deleted guild drops out of its allies |
 
-## H. Invite codes — `tests/codes.spec.ts` (12, of which 5 are one `describe.serial` chain)
+## H. Invite codes — `tests/codes.spec.ts` (18, of which 5 are one `describe.serial` chain)
 
 | Requirement | Test |
 | --- | --- |
@@ -188,6 +188,22 @@ the server answering in another language for the rest of the run. ACF's `usePerI
 false)` is what makes that safe, and one player switching language leaves everyone else in English
 asserts it rather than assuming it.
 
+## L. Startup and the guild data — `tests/startup-loading.spec.ts` (4)
+
+The environment stages two guild files into the plugin's data folder: one that reads and one that
+does not parse.
+
+| Requirement | Test |
+| --- | --- |
+| One unreadable file does not stop the plugin | the plugin starts with a guild file it cannot read, and says which file |
+| The guilds that could be read are available | the guilds that could be read are available |
+| The unreadable file is left alone, and copied aside | the guild file that could not be read is left on disk, with a copy in the backup folder |
+| The readable file is untouched | the guild file that could be read is still readable |
+
+## M. Smoke — `tests/smoke.spec.ts` (7)
+
+The rows for this section belong to the branch that adds the smoke suite.
+
 ## Fixture self-check — `tests/bank.spec.ts`
 
 `STARTING_BALANCE is what the fixtures actually fund` is not a Guilds test. It checks that the

src/main/java/me/glaremasters/guilds/Guilds.java

@@ -255,14 +255,32 @@
             // Load the challenge handler
             challengeHandler = new ChallengeHandler(this);
             challengeHandler.loadChallenges();
-            // Load guildhandler with provider
-            guildHandler = new GuildHandler(this, settingsHandler.getMainConf());
         } catch (IOException e) {
             LoggingUtils.severe("An error occurred loading data! Stopping plugin..", e);
             Bukkit.getPluginManager().disablePlugin(this);
             return;
         }
 
+        /*
+         * Load the guild data.
+         *
+         * The guilds are the data the plugin cannot do without, and a load that failed leaves the
+         * plugin without them. This used to happen inside the guild handler, which caught the
+         * failure, logged it and carried on with an empty guild map: the server came up as if
+         * nothing was wrong, and the first save then wrote that empty map back over the guild data
+         * the plugin had failed to read. The failure stops the plugin instead, and the message
+         * names what could not be read.
+         */
+        try {
+            guildHandler = new GuildHandler(this, settingsHandler.getMainConf());
+        } catch (IOException | RuntimeException e) {
+            LoggingUtils.severe("Guilds could not load the guild data, so it will not start. "
+                    + "The guild files in the data folder have not been changed. "
+                    + "Fix the cause reported below and restart the server.", e);
+            Bukkit.getPluginManager().disablePlugin(this);
+            return;
+        }
+
         // If they have placeholderapi, enable it.
         if (Bukkit.getPluginManager().isPluginEnabled("PlaceholderAPI")) {
             new PlaceholderAPI(guildHandler).register();
@@ -303,7 +321,7 @@
         chatListener = new ChatListener(this);
 
         LoggingUtils.info("Ready to go! That only took " + (System.currentTimeMillis() - startingTime) + "ms");
         getServer().getScheduler().scheduleAsyncRepeatingTask(this, () -> {
             try {
                 if (guildHandler.isMigrating()) {
                     return;

src/main/java/me/glaremasters/guilds/database/CorruptDataBackup.java

@@ -0,0 +1,258 @@
+package me.glaremasters.guilds.database;
+
+import me.glaremasters.guilds.utils.LoggingUtils;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.time.Instant;
+import java.time.ZoneId;
+import java.time.format.DateTimeFormatter;
+
+/**
+ * Keeps a copy of data the plugin could not read.
+ *
+ * <p>A file the plugin cannot parse is often the only copy of that record. The plugin has no
+ * in-memory version of it, so anything it wrote in its place would be a guess. Rather than
+ * discarding the file or writing over it, the file is copied to a backup directory and the
+ * original is left exactly where the operator put it.
+ *
+ * <p>The backups live outside every data folder the plugin reads, under
+ * {@code plugins/Guilds/backups/corrupt/<timestamp>/<data-type>/}. The plugin enumerates only its
+ * own data folders, so a backup is never loaded as a record, however it is named.
+ *
+ * <p>The timestamp comes from the file's own last-modified time rather than from the moment the
+ * backup was taken. An unchanged file therefore always lands in the same directory, and a second
+ * read of the same corruption finds the copy that is already there instead of making another one.
+ * The same holds across a restart.
+ */
+public final class CorruptDataBackup {
+
+    /**
+     * The directory under the plugin's data folder that holds the backups.
+     */
+    public static final String BACKUP_DIRECTORY = "backups/corrupt";
+
+    /**
+     * The timestamp format used for a backup directory.
+     */
+    private static final DateTimeFormatter TIMESTAMP =
+            DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss").withZone(ZoneId.systemDefault());
+
+    private CorruptDataBackup() {
+        throw new UnsupportedOperationException("This is a utility class and cannot be instantiated");
+    }
+
+    /**
+     * What happened when a backup was attempted.
+     */
+    public static final class Result {
+
+        private final boolean created;
+        private final boolean skipped;
+        private final boolean failed;
+        private final File backupFile;
+        private final String failure;
+
+        private Result(boolean created, boolean skipped, boolean failed, File backupFile, String failure) {
+            this.created = created;
+            this.skipped = skipped;
+            this.failed = failed;
+            this.backupFile = backupFile;
+            this.failure = failure;
+        }
+
+        /**
+         * Whether a new copy was written.
+         *
+         * @return true when a backup file was created
+         */
+        public boolean isCreated() {
+            return created;
+        }
+
+        /**
+         * Whether an identical backup was already there.
+         *
+         * @return true when the corruption had already been backed up
+         */
+        public boolean isSkipped() {
+            return skipped;
+        }
+
+        /**
+         * Whether the backup could not be written.
+         *
+         * @return true when the backup failed
+         */
+        public boolean isFailed() {
+            return failed;
+        }
+
+        /**
+         * Where the backup is.
+         *
+         * @return the backup file, or null when the backup failed
+         */
+        public File getBackupFile() {
+            return backupFile;
+        }
+
+        /**
+         * Why the backup failed.
+         *
+         * @return the reason, or null when the backup succeeded
+         */
+        public String getFailure() {
+            return failure;
+        }
+    }
+
+    /**
+     * Copies a file the plugin could not read into the backup directory.
+     *
+     * <p>The copy is byte for byte. The original file is not moved, not truncated and not
+     * overwritten: it is left exactly as it was, and a backup is a safeguard rather than
+     * permission to replace it.
+     *
+     * @param pluginDataFolder the plugin's data folder, {@code plugins/Guilds}
+     * @param sourceFile       the file that could not be read
+     * @param dataType         the kind of record the file holds, used as a directory name
+     * @return what happened
+     */
+    public static Result backup(File pluginDataFolder, File sourceFile, String dataType) {
+        if (pluginDataFolder == null || sourceFile == null || !sourceFile.isFile()) {
+            return new Result(false, false, true, null,
+                    "there was no file to copy: " + (sourceFile == null ? "no file was given" : sourceFile.getAbsolutePath()));
+        }
+
+        final File directory = backupDirectory(pluginDataFolder, sourceFile, dataType);
+        final File original = new File(directory, sourceFile.getName());
+
+        // The same corruption, read again, must not produce a second copy.
+        if (original.isFile() && sameContent(original, sourceFile)) {
+            return new Result(false, true, false, original, null);
+        }
+
+        // Something with that name is already there and holds different data, so the copy needs a
+        // name of its own rather than the one that is taken.
+        File target = original;
+        if (target.exists()) {
+            target = freeName(directory, sourceFile.getName());
+            if (target == null) {
+                return new Result(false, false, true, null,
+                        "the backup directory holds a file with that name and it could not be replaced: " + directory.getAbsolutePath());
+            }
+        }
+
+        try {
+            final Path source = sourceFile.toPath();
+            final Path destination = target.toPath();
+            Files.createDirectories(destination.getParent());
+            Files.copy(source, destination, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.COPY_ATTRIBUTES);
+            return new Result(true, false, false, target, null);
+        } catch (IOException | RuntimeException ex) {
+            final String reason = ex.getMessage() == null ? ex.toString() : ex.getMessage();
+            LoggingUtils.severe("The backup of " + sourceFile.getAbsolutePath() + " could not be written to "
+                    + directory.getAbsolutePath() + ". The file has been left untouched: " + reason, ex);
+            return new Result(false, false, true, null, reason);
+        }
+    }
+
+    /**
+     * The directory a backup of a file goes in.
+     *
+     * <p>The timestamp is the file's own last-modified time, so an unchanged file always maps to
+     * the same directory and is only ever backed up once.
+     *
+     * @param pluginDataFolder the plugin's data folder
+     * @param sourceFile       the file being backed up
+     * @param dataType         the kind of record the file holds
+     * @return the directory
+     */
+    private static File backupDirectory(File pluginDataFolder, File sourceFile, String dataType) {
+        final long modified = sourceFile.lastModified();
+        final String timestamp = modified <= 0L
+                ? TIMESTAMP.format(Instant.now())
+                : TIMESTAMP.format(Instant.ofEpochMilli(modified));
+        return new File(new File(new File(pluginDataFolder, BACKUP_DIRECTORY), timestamp), safeName(dataType));
+    }
+
+    /**
+     * A name that can be used as a directory name.
+     *
+     * @param dataType the kind of record
+     * @return the name
+     */
+    private static String safeName(String dataType) {
+        if (dataType == null || dataType.trim().isEmpty()) {
+            return "unknown";
+        }
+        return dataType.replaceAll("[^A-Za-z0-9._-]", "_");
+    }
+
+    /**
+     * A free name for a backup inside its directory.
+     *
+     * <p>The original name is kept, so the operator can tell what the file was. When something with
+     * that name is already there, a number is added until the name is free, so a backup never
+     * overwrites another one.
+     *
+     * @param directory the backup directory
+     * @param name      the original file name
+     * @return the name to write, or null when none could be found
+     */
+    private static File freeName(File directory, String name) {
+        for (int suffix = 1; suffix < 1000; suffix++) {
+            final File candidate = new File(directory, name + "." + suffix);
+            if (!candidate.exists()) {
+                return candidate;
+            }
+        }
+
+        return null;
+    }
+
+    /**
+     * Whether two files hold the same bytes.
+     *
+     * @param first  the first file
+     * @param second the second file
+     * @return true when their contents are identical
+     */
+    private static boolean sameContent(File first, File second) {
+        try {
+            return digest(first).equals(digest(second));
+        } catch (IOException | RuntimeException ex) {
+            return false;
+        }
+    }
+
+    /**
+     * The SHA-256 digest of a file's contents.
+     *
+     * @param file the file
+     * @return the digest, as hex
+     * @throws IOException if the file cannot be read
+     */
+    private static String digest(File file) throws IOException {
+        final MessageDigest digest;
+        try {
+            digest = MessageDigest.getInstance("SHA-256");
+        } catch (NoSuchAlgorithmException ex) {
+            throw new IOException("SHA-256 is not available", ex);
+        }
+
+        final byte[] hash = digest.digest(Files.readAllBytes(file.toPath()));
+        final StringBuilder hex = new StringBuilder(hash.length * 2);
+        for (byte value : hash) {
+            hex.append(Character.forDigit((value >> 4) & 0xF, 16));
+            hex.append(Character.forDigit(value & 0xF, 16));
+        }
+        return hex.toString();
+    }
+}