Make .unrecoverable a persistent one-way recovery quarantine

cmdline/check.c

@@ -188,16 +188,13 @@
  * such data, but the containing file remains marked damaged and is kept under
  * the .unrecoverable name instead of being reported as successfully recovered.
  *
- * Missing files are created as .unrecoverable from the beginning, and an
- * existing .unrecoverable file is reused by later fix runs. This allows a
- * recovery to proceed over multiple runs without exposing a partially rebuilt
- * file under its final name.
+ * Missing files are created under their normal names. They are quarantined only
+ * if recovery produces data that cannot be certified as CURRENT. An existing
+ * .unrecoverable file is reused by later fix runs and is never promoted back to
+ * the normal name.
  *
- * Promotion to the final file name is a file-level commit operation. It happens
- * only after the complete file has been processed successfully, its metadata
- * has been restored, and no block remains damaged. A partial -S/-B fix operates
- * only at block level and therefore never performs this file-level promotion
- * or other finalization.
+ * A partial -S/-B fix operates only at block level and therefore never performs
+ * file-level finalization.
  */
 
 /****************************************************************************/
@@ -1293,8 +1290,8 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o
 			/*
 			 * A partial fix operates only at block level. Reaching the last block of a
 			 * file doesn't imply that the whole file was processed, so don't perform any
-			 * file-level finalization, including marking it finished, promoting an
-			 * .unrecoverable file, reporting it recovered, or restoring its timestamp.
+			 * file-level finalization, including marking it finished, reporting its final
+			 * status, or restoring its timestamp.
 			 * This is intentional also when an explicit block range happens to cover all
 			 * the parity blocks, to keep -S/-B behavior independent of the parity size.
 			 */
@@ -1309,35 +1306,6 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o
 
 			/* if the file is damaged, meaning that a fix failed */
 			if (file_flag_has(file, FILE_IS_DAMAGED)) {
-				char path[PATH_MAX];
-				char path_to[PATH_MAX];
-
-				pathprint(path, sizeof(path), "%s%s", disk->dir, file->sub);
-				pathprint(path_to, sizeof(path_to), "%s%s.unrecoverable", disk->dir, file->sub);
-
-				/* ensure to close the file before renaming */
-				ret = handle_close(&handle[j]);
-				if (ret != 0) {
-					/* LCOV_EXCL_START */
-					log_tag("%s:%" PRIu64 ":%s:%s: Close error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno));
-					log_fatal_errno(errno, disk->name);
-					return -1;
-					/* LCOV_EXCL_STOP */
-				}
-
-				/* a previous recovery is already quarantined */
-				if (!was_unrecoverable) {
-					ret = rename(path, path_to);
-					if (ret != 0) {
-						/* LCOV_EXCL_START */
-						log_fatal(errno, "Error renaming '%s%s'. %s.\n", disk->dir, file->sub, strerror(errno));
-						log_tag("%s:%" PRIu64 ":%s:%s: Rename error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno));
-						log_fatal_errno(errno, disk->name);
-						return -1;
-						/* LCOV_EXCL_STOP */
-					}
-				}
-
 				log_tag("status:unrecoverable:%s:%s\n", disk->name, esc_tag(file->sub));
 				msg_info("unrecoverable %s\n", fmt_term(disk, file->sub));
 
@@ -1401,7 +1369,7 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o
 				|| collide_file->mtime_sec != file->mtime_sec /* if the mtime is different, the collision is identified */
 				|| collide_file->mtime_nsec != file->mtime_nsec /* same for mtime_nsec */
 			) {
-				/* set the original modification time before promoting/renaming */
+				/* set the original modification time */
 				ret = handle_utime(&handle[j]);
 				if (ret == -1) {
 					/* LCOV_EXCL_START */
@@ -1418,36 +1386,14 @@ static int file_post(struct snapraid_state* state, int fix, int partial, block_o
 			}
 
 			/*
-			 * This rename is the file-level recovery commit point. Keep the file
-			 * quarantined until every block and metadata was processed without error;
-			 * promoting only after setting the timestamp ensures the final file is
-			 * never exposed with incomplete data or provisional timestamp.
+			 * .unrecoverable is a persistent one-way quarantine marker.
+			 * A later fix may improve its contents, but it must never
+			 * automatically promote it back to the normal filename.
 			 */
 			if (was_unrecoverable) {
-				char path[PATH_MAX];
-				char path_from[PATH_MAX];
-
-				pathprint(path, sizeof(path), "%s%s", disk->dir, file->sub);
-				pathprint(path_from, sizeof(path_from), "%s%s.unrecoverable", disk->dir, file->sub);
-
-				ret = handle_close(&handle[j]);
-				if (ret != 0) {
-					/* LCOV_EXCL_START */
-					log_tag("%s:%" PRIu64 ":%s:%s: Close error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno));
-					log_fatal_errno(errno, disk->name);
-					return -1;
-					/* LCOV_EXCL_STOP */
-				}
-
-				ret = rename(path_from, path);
-				if (ret != 0) {
-					/* LCOV_EXCL_START */
-					log_fatal(errno, "Error renaming '%s'. %s.\n", path_from, strerror(errno));
-					log_tag("%s:%" PRIu64 ":%s:%s: Rename error. %s.\n", es(errno), i, disk->name, esc_tag(file->sub), strerror(errno));
-					log_fatal_errno(errno, disk->name);
-					return -1;
-					/* LCOV_EXCL_STOP */
-				}
+				log_tag("status:unrecoverable:%s:%s\n", disk->name, esc_tag(file->sub));
+				msg_info("unrecoverable %s\n", fmt_term(disk, file->sub));
+				goto close_and_continue;
 			}
 
 			log_tag("status:recovered:%s:%s\n", disk->name, esc_tag(file->sub));
@@ -2083,8 +2029,28 @@ static int state_check_process(struct snapraid_state* state, int fix, struct sna
 
 				/* keep track of damaged files */
 				for (j = 0; j < failed_count; ++j) {
-					if (failed[j].is_bad)
-						file_flag_set(failed[j].file, FILE_IS_DAMAGED);
+					if (!failed[j].is_bad)
+						continue;
+
+					file_flag_set(failed[j].file, FILE_IS_DAMAGED);
+
+					/*
+					 * Persist the unrecoverable state immediately.
+					 * Previous blocks of this file may already have been
+					 * successfully recovered.
+					 */
+					if (fix && !file_flag_has(failed[j].file, FILE_IS_EXCLUDED) && !(state->opt.syncedonly && file_flag_has(failed[j].file, FILE_IS_UNSYNCED))) {
+						ret = handle_quarantine(failed[j].handle, failed[j].file, state->file_mode);
+						if (ret == -1) {
+							/* LCOV_EXCL_START */
+							log_tag("%s:%" PRIu64 ":%s:%s: Quarantine error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno));
+							log_fatal_errno(errno, failed[j].disk->name);
+							log_fatal(errno, "Stopping at block %" PRIu64 "\n", i);
+							++unrecoverable_error;
+							goto bail;
+							/* LCOV_EXCL_STOP */
+						}
+					}
 				}
 			} else {
 				/*
@@ -2149,32 +2115,56 @@ static int state_check_process(struct snapraid_state* state, int fix, struct sna
 						/*
 						 * Write the best recovered candidate even when it is out-of-date. Such data
 						 * may still be useful to the user or to a later multistep recovery.
-						 *
-						 * An out-of-date write is not considered a successful fix: the file is marked
-						 * damaged below and file_post() keeps or moves it to .unrecoverable instead
-						 * of promoting it as recovered.
 						 */
+						/*
+						 * An out-of-date candidate is useful best-effort data,
+						 * but it cannot be certified as the required CURRENT
+						 * contents. Persist this fact before modifying the file
+						 * so that a SIGKILL cannot leave unproven recovered data
+						 * under the normal filename.
+						 */
+						if (failed[j].is_outofdate) {
+							file_flag_set(failed[j].file, FILE_IS_DAMAGED);
+
+							ret = handle_quarantine(failed[j].handle, failed[j].file, state->file_mode);
+							if (ret == -1) {
+								/* LCOV_EXCL_START */
+								log_tag("%s:%" PRIu64 ":%s:%s: Quarantine error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno));
+								log_fatal_errno(errno, failed[j].disk->name);
+								log_fatal(errno, "Stopping at block %" PRIu64 "\n", i);
+								++unrecoverable_error;
+								goto bail;
+								/* LCOV_EXCL_STOP */
+							}
+						}
+
 						ret = handle_write(failed[j].handle, failed[j].file_pos, buffer[failed[j].index], state->block_size);
 						if (ret == -1) {
 							/* LCOV_EXCL_START */
-							log_tag("%s:%" PRIu64 ":%s:%s: Write error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno));
-							log_fatal_errno(errno, failed[j].disk->name);
-							log_fatal(errno, "Stopping at block %" PRIu64 "\n", i);
+							int saved_errno = errno;
 
 							/* mark the file as damaged */
 							file_flag_set(failed[j].file, FILE_IS_DAMAGED);
 
+							ret = handle_quarantine(failed[j].handle, failed[j].file, state->file_mode);
+							if (ret == -1) {
+								log_tag("%s:%" PRIu64 ":%s:%s: Quarantine error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno));
+								log_fatal_errno(errno, failed[j].disk->name);
+							}
+
+							errno = saved_errno;
+							log_tag("%s:%" PRIu64 ":%s:%s: Write error. %s.\n", es(errno), i, failed[j].disk->name, esc_tag(failed[j].file->sub), strerror(errno));
+							log_fatal_errno(errno, failed[j].disk->name);
+							log_fatal(errno, "Stopping at block %" PRIu64 "\n", i);
+
 							++unrecoverable_error;
 							goto bail;
 							/* LCOV_EXCL_STOP */
 						}
 
 						/* if we are not sure that the recovered content is uptodate */
-						if (failed[j].is_outofdate) {
-							/* mark the file as damaged */
-							file_flag_set(failed[j].file, FILE_IS_DAMAGED);
+						if (failed[j].is_outofdate)
 							continue;
-						}
 
 						/*
 						 * Mark the file as containing some fixes

cmdline/handle.c

@@ -14,6 +14,8 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in
 {
 	int ret;
 	int flags;
+	char path[PATH_MAX];
+	char path_unrecoverable[PATH_MAX];
 
 	/* if it's the same file, and already opened, nothing to do */
 	if (handle->file == file && handle->f != -1) {
@@ -21,9 +23,11 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in
 	}
 
 	advise_init(&handle->advise, mode);
-	pathprint(handle->path, sizeof(handle->path), "%s%s", handle->disk->dir, file->sub);
 
-	ret = mkancestor(handle->path);
+	pathprint(path, sizeof(path), "%s%s", handle->disk->dir, file->sub);
+	pathprint(path_unrecoverable, sizeof(path_unrecoverable), "%s.unrecoverable", path);
+
+	ret = mkancestor(path);
 	if (ret != 0) {
 		/* LCOV_EXCL_START */
 		return -1;
@@ -42,63 +46,61 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in
 	 */
 	flags = O_BINARY | O_NOFOLLOW | advise_flags(&handle->advise);
 
-	/* open for read write */
-	handle->f = open(handle->path, flags | O_RDWR);
+	/*
+	 * A previous .unrecoverable file is a persistent one-way quarantine.
+	 * Always give it precedence over the normal filename, even if both exist.
+	 */
+	handle->f = open(path_unrecoverable, flags | O_RDWR);
 
 	/* if failed for missing write permission */
 	if (handle->f == -1 && (errno == EACCES || errno == EROFS)) {
 		int saved_errno = errno;
 		/* open for read-only */
-		handle->f = open(handle->path, flags | O_RDONLY);
+		handle->f = open(path_unrecoverable, flags | O_RDONLY);
 		if (handle->f != -1) {
 			handle->readonly_errno = saved_errno;
 		}
 	}
 
-	/* if failed for missing file */
-	if (handle->f == -1 && errno == ENOENT) {
-		char path_unrecoverable[PATH_MAX];
+	if (handle->f != -1) {
+		handle->is_unrecoverable = 1;
+		pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
+	} else if (errno == ENOENT) {
+		/* no previous quarantine exists, try the normal filename */
+		handle->f = open(path, flags | O_RDWR);
 
-		/* check if exists a .unrecoverable copy */
-		pathprint(path_unrecoverable, sizeof(path_unrecoverable), "%s.unrecoverable", handle->path);
-
-		/*
-		 * Open a previous recovery directly as .unrecoverable, keeping it
-		 * quarantined until the whole file is processed successfully. Promoting
-		 * it now could expose an incomplete or unverified file if this fix aborts
-		 * or fails again. Reuse it for a multistep fix, preserving readable CHG
-		 * blocks because their current hash is not available.
-		 */
-		handle->f = open(path_unrecoverable, flags | O_RDWR);
-		if (handle->f != -1) {
-			handle->is_unrecoverable = 1;
-			pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
-		} else if (errno == ENOENT) {
-			/*
-			 * Create missing file as .unrecoverable so that an interruption or
-			 * crash (e.g. SIGKILL) during recovery never leaves a partially recovered
-			 * file visible under its final name.
-			 */
-			handle->f = open(path_unrecoverable, flags | O_RDWR | O_CREAT | O_EXCL, 0600);
+		/* if failed for missing write permission */
+		if (handle->f == -1 && (errno == EACCES || errno == EROFS)) {
+			int saved_errno = errno;
+			/* open for read-only */
+			handle->f = open(path, flags | O_RDONLY);
 			if (handle->f != -1) {
-				handle->is_unrecoverable = 1;
-				pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
+				handle->readonly_errno = saved_errno;
+			}
+		}
+
+		/* if failed for missing file */
+		if (handle->f == -1 && errno == ENOENT) {
+			/*
+			 * The file is genuinely missing and there is no previous
+			 * quarantine. Create it using the normal filename.
+			 *
+			 * It will be moved to .unrecoverable only if recovery later
+			 * encounters data that cannot be certified CURRENT.
+			 */
+			handle->f = open(path, flags | O_RDWR | O_CREAT | O_EXCL, 0600);
+			if (handle->f != -1) {
+				handle->created = 1;
 			} else if (errno == EEXIST) {
 				/* if created concurrently, reopen safely */
-				handle->f = open(path_unrecoverable, flags | O_RDWR);
-				if (handle->f != -1) {
-					handle->is_unrecoverable = 1;
-					pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
-				} else {
-					pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
-				}
-			} else {
-				pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
+				handle->f = open(path, flags | O_RDWR);
 			}
-		} else {
-			/* report the path that failed to open */
-			pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
 		}
+
+		pathcpy(handle->path, sizeof(handle->path), path);
+	} else {
+		/* report the .unrecoverable path that failed to open */
+		pathcpy(handle->path, sizeof(handle->path), path_unrecoverable);
 	}
 
 	if (handle->f == -1) {
@@ -137,6 +139,93 @@ int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, in
 	return 0;
 }
 
+/**
+ * Move an opened file to its persistent .unrecoverable name.
+ */
+int handle_quarantine(struct snapraid_handle* handle, struct snapraid_file* file, int mode)
+{
+	char path[PATH_MAX];
+	char path_to[PATH_MAX];
+	struct stat st;
+	int ret;
+
+	(void)mode;
+
+	/* already persistently quarantined */
+	if (handle->is_unrecoverable)
+		return 0;
+
+	pathprint(path, sizeof(path), "%s%s", handle->disk->dir, file->sub);
+	pathprint(path_to, sizeof(path_to), "%s%s.unrecoverable", handle->disk->dir, file->sub);
+
+	/*
+	 * Never overwrite a previous quarantine.
+	 */
+	ret = lstat(path_to, &st);
+	if (ret == 0) {
+		errno = EEXIST;
+		log_error(errno, "Unrecoverable file '%s' already exists.\n", path_to);
+		return -1;
+	}
+	if (errno != ENOENT) {
+		/* LCOV_EXCL_START */
+		log_error(errno, "Error accessing file '%s'. %s.\n", path_to, strerror(errno));
+		return -1;
+		/* LCOV_EXCL_STOP */
+	}
+
+#ifdef _WIN32
+	/*
+	 * Windows may not allow renaming an opened file. Close it before
+	 * the rename and reopen the quarantined file afterward.
+	 */
+	ret = handle_close(handle);
+	if (ret != 0)
+		return -1;
+
+	ret = rename(path, path_to);
+	if (ret != 0) {
+		/* LCOV_EXCL_START */
+		log_error(errno, "Error renaming file '%s' to '%s'. %s.\n", path, path_to, strerror(errno));
+		return -1;
+		/* LCOV_EXCL_STOP */
+	}
+
+	/*
+	 * handle_create() gives precedence to the existing .unrecoverable
+	 * file, so it reopens path_to and sets is_unrecoverable.
+	 */
+	ret = handle_create(handle, file, mode);
+	if (ret != 0)
+		return -1;
+
+	if (!handle->is_unrecoverable) {
+		/* LCOV_EXCL_START */
+		errno = ENXIO;
+		log_error(errno, "Error renaming file '%s' to '%s'. %s.\n", path, path_to, strerror(errno));
+		return -1;
+		/* LCOV_EXCL_STOP */
+	}
+#else
+	/*
+	 * POSIX allows renaming an opened file. The descriptor continues
+	 * to reference the same inode, so no close/reopen is required.
+	 */
+	ret = rename(path, path_to);
+	if (ret != 0) {
+		/* LCOV_EXCL_START */
+		log_error(errno, "Error renaming file '%s' to '%s'. %s.\n", path, path_to, strerror(errno));
+		return -1;
+		/* LCOV_EXCL_STOP */
+	}
+
+	handle->is_unrecoverable = 1;
+	pathcpy(handle->path, sizeof(handle->path), path_to);
+#endif
+
+	return 0;
+}
+
 int handle_truncate(struct snapraid_handle* handle, struct snapraid_file* file)
 {
 	int ret;

cmdline/handle.h

@@ -37,7 +37,8 @@ struct snapraid_handle {
 
 /**
  * Create a file.
- * The file is created if missing, and opened with write access.
+ * An existing .unrecoverable file takes precedence over the normal filename.
+ * The normal file is created if both names are missing, and opened with write access.
  * If the file is created, the handle->created is set.
  * If a previous .unrecoverable file is opened, the handle->is_unrecoverable is set.
  * The initial size of the file is stored in the file->st struct.
@@ -46,6 +47,13 @@ struct snapraid_handle {
  */
 int handle_create(struct snapraid_handle* handle, struct snapraid_file* file, int mode);
 
+/**
+ * Move an opened file to its persistent .unrecoverable name.
+ * If already quarantined, do nothing.
+ * On error, log the failure and return -1 with errno set.
+ */
+int handle_quarantine(struct snapraid_handle* handle, struct snapraid_file* file, int mode);
+
 /**
  * Truncate a file if required.
  */