Skip to content

Write author-owned payload files through a staging file #103

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

Write author-owned payload files through a staging file #103

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
4 changes: 3 additions & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ any missing shipped files under `rtx-remix/` without replacing existing files. T
is author-owned: an existing file is replaced only when its content is a
version of that file that has shipped, so an untouched old default takes the
current one and edited confs are never overwritten, even during full
extraction. `src/main/resources/rtx-remix-shipped-payload.sha256` records
extraction. Each file is written to a staging file beside it and moved into
place, because a partial file matches no shipped version and would be kept as
an edit. `src/main/resources/rtx-remix-shipped-payload.sha256` records
every shipped version; a payload change appends its line and never removes
one, and `ShippedPayloadHistoryTest` fails with the missing line. Deleting a
shipped conf restores its default on the next launch. A present conf that fails to acquire still prevents renderer
Expand Down
62 changes: 56 additions & 6 deletions src/main/java/com/nvidia/rtxremix/minecraft/NativeExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import java.io.IOException;
import java.io.InputStream;
import java.nio.file.AtomicMoveNotSupportedException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.FileVisitResult;
import java.nio.file.Files;
Expand Down Expand Up @@ -123,8 +124,9 @@ static boolean archSubdirPresent(Set<String> jarArchDirs, String detectedDir) {
* <p>The version marker is written only after the copy and both staged
* moves succeed. A failure before that point leaves the marker stale so the
* next launch retries extraction. Auxiliary directories may already be
* partially updated, and a failed SDK swap can leave the live SDK path
* absent; callers must not rely on rollback to the prior payload.
* partially updated, each file in them whole, and a failed SDK swap can
* leave the live SDK path absent; callers must not rely on rollback to the
* prior payload.
*
* @return false when extraction failed with an error this launch, so the
* caller can say why the runtime is missing
Expand Down Expand Up @@ -224,7 +226,8 @@ static void extractIfNeeded(Path nativeRoot, Path gameDir, String modVersion, St
Files.deleteIfExists(rtxConfStaging);

// Mod-owned files stage first and replace their live copies only once
// every copy succeeds. Author-owned directories are repaired in place.
// every copy succeeds. Author-owned directories are repaired in place,
// one whole file at a time.
try (var topStream = Files.list(nativeRoot)) {
for (Path entry : (Iterable<Path>) topStream::iterator) {
String name = entry.getFileName().toString();
Expand Down Expand Up @@ -278,7 +281,8 @@ private static void extractAuthorPayload(Path nativeRoot, Path gameDir, ShippedP
for (Path entry : (Iterable<Path>) topStream::iterator) {
String name = entry.getFileName().toString();
if (Files.isDirectory(entry) && !name.startsWith("remix-sdk-") && !TEXTURES_DIR.equals(name)) {
extractAuthorTree(entry, gameDir.resolve(name), name, history, fileCount, totalBytes, keptCount);
extractAuthorTree(entry, gameDir.resolve(name), name, history, Files::newInputStream,
fileCount, totalBytes, keptCount);
}
}
}
Expand Down Expand Up @@ -376,11 +380,16 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
* {@code history} records as shipped, which proves nobody edited it.
* Anything else is left untouched and counted in {@code keptCount}.
*
* <p>Every write goes through {@link #writeThroughStaging}. A partial file
* on a live path matches no shipped version, so each later launch would
* keep it as an edit and the update could never finish.
*
* @param historyRoot the name {@code src} has under the JAR's {@code native/}
* root, which prefixes the paths {@code history} records
* @param opener opens each bundled file that is written
*/
static void extractAuthorTree(Path src, Path dst, String historyRoot, ShippedPayloadHistory history,
AtomicInteger fileCount, AtomicLong totalBytes,
FileOpener opener, AtomicInteger fileCount, AtomicLong totalBytes,
AtomicInteger keptCount) throws IOException {
Files.createDirectories(dst);
Files.walkFileTree(src, new SimpleFileVisitor<>() {
Expand All @@ -405,19 +414,60 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
}
LOGGER.info("Updating unedited {} to its current shipped default", target);
}
copyFile(file, target, attrs, fileCount, totalBytes);
writeThroughStaging(file, target, opener);
fileCount.incrementAndGet();
totalBytes.addAndGet(attrs.size());
return FileVisitResult.CONTINUE;
}
});
}

/** Opens a bundled file for copying; tests substitute one whose stream fails partway. */
@FunctionalInterface
interface FileOpener {
InputStream open(Path file) throws IOException;
}

/**
* Writes the bundled {@code file} to {@code target} through a staging file
* beside it, so the live path keeps its previous content until the whole
* copy exists. A failed write removes the staging file.
*/
private static void writeThroughStaging(Path file, Path target, FileOpener opener) throws IOException {
Path staging = target.resolveSibling(target.getFileName() + ".staging");
try {
try (InputStream in = opener.open(file)) {
Files.copy(in, staging, StandardCopyOption.REPLACE_EXISTING);
}
retrying("Moving " + staging + " to " + target, () -> moveReplacing(staging, target),
Thread::sleep, RENAME_RETRY_DELAYS_MS);
} catch (IOException e) {
deleteQuietly(staging);
throw e;
}
}

/** Replaces {@code target} with {@code source}, in one step where the file system can. */
private static void moveReplacing(Path source, Path target) throws IOException {
try {
Files.move(source, target, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
} catch (AtomicMoveNotSupportedException e) {
Files.move(source, target, StandardCopyOption.REPLACE_EXISTING);
}
}

private static void createTargetDirectory(Path src, Path dst, Path dir) throws IOException {
Path rel = src.relativize(dir);
if (!rel.toString().isEmpty()) {
Files.createDirectories(dst.resolve(rel.toString()));
}
}

/**
* Streams {@code file} straight onto {@code target}, so a failure leaves a
* partial file there. Only for a target inside a staging directory that is
* discarded unless every copy succeeds.
*/
private static void copyFile(Path file, Path target, BasicFileAttributes attrs,
AtomicInteger fileCount, AtomicLong totalBytes) throws IOException {
Files.createDirectories(target.getParent());
Expand Down
87 changes: 85 additions & 2 deletions src/test/java/com/nvidia/rtxremix/minecraft/NativeExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.SequenceInputStream;
import java.io.UncheckedIOException;
import java.nio.file.AccessDeniedException;
import java.nio.file.FileAlreadyExistsException;
Expand Down Expand Up @@ -40,6 +43,8 @@ class NativeExtractorTest {

private static final String WATER_CONF = "rtx-remix/mods/minecraft-logic/confs/water.conf";

private static final String READ_FAILURE = "bundled file became unreadable";

@ParameterizedTest
@CsvSource({
"aarch64, remix-sdk-arm64",
Expand Down Expand Up @@ -104,7 +109,7 @@ void extractAuthorTreePreservesAuthoredEditsAndAddsNewFiles(@TempDir Path tmp) t
AtomicLong bytes = new AtomicLong();
AtomicInteger kept = new AtomicInteger();
NativeExtractor.extractAuthorTree(tmp.resolve("src"), tmp.resolve("dst"), "rtx-remix", NO_HISTORY,
files, bytes, kept);
Files::newInputStream, files, bytes, kept);

assertEquals("author's hand-tuned values", Files.readString(dst.resolve("cave.conf")));
assertEquals("newly shipped profile", Files.readString(dst.resolve("swamp.conf")));
Expand All @@ -123,12 +128,72 @@ void extractAuthorTreeRestoresADeletedFileToItsShippedDefault(@TempDir Path tmp)
AtomicInteger files = new AtomicInteger();
AtomicLong bytes = new AtomicLong();
AtomicInteger kept = new AtomicInteger();
NativeExtractor.extractAuthorTree(src, dst, "rtx-remix", NO_HISTORY, files, bytes, kept);
NativeExtractor.extractAuthorTree(src, dst, "rtx-remix", NO_HISTORY, Files::newInputStream,
files, bytes, kept);

assertEquals("shipped default", Files.readString(dst.resolve("water.conf")));
assertEquals(0, kept.get());
}

@Test
void anInterruptedUpdateLeavesTheUneditedDefaultForARetryToReplace(@TempDir Path tmp) throws IOException {
// A partial live file would match no shipped version, so every later
// launch would keep it as an author's edit and never finish the update.
Path src = Files.createDirectories(tmp.resolve("src"));
Path dst = Files.createDirectories(tmp.resolve("dst"));
Files.writeString(src.resolve("cave.conf"), "current cave");
Files.writeString(src.resolve("water.conf"), "current water");
Files.writeString(dst.resolve("cave.conf"), "author's cave");
Files.writeString(dst.resolve("water.conf"), "previous water");
ShippedPayloadHistory history = history(line(tmp, "previous water", "rtx-remix/water.conf"));
AtomicInteger files = new AtomicInteger();
AtomicLong bytes = new AtomicLong();
AtomicInteger kept = new AtomicInteger();

IOException failure = assertThrows(IOException.class, () -> NativeExtractor.extractAuthorTree(
src, dst, "rtx-remix", history, NativeExtractorTest::openFailingPartway, files, bytes, kept));

assertEquals(READ_FAILURE, failure.getMessage());
assertEquals("previous water", Files.readString(dst.resolve("water.conf")));
assertEquals("author's cave", Files.readString(dst.resolve("cave.conf")));
assertEquals(List.of("cave.conf", "water.conf"), fileNames(dst),
"the staging file must not outlive the failure");
assertEquals(0, files.get());

NativeExtractor.extractAuthorTree(src, dst, "rtx-remix", history, Files::newInputStream,
files, bytes, kept);

assertEquals("current water", Files.readString(dst.resolve("water.conf")));
assertEquals("author's cave", Files.readString(dst.resolve("cave.conf")));
assertEquals(List.of("cave.conf", "water.conf"), fileNames(dst));
assertEquals(1, files.get());
}

@Test
void anInterruptedFirstWriteLeavesNoFileForLaterLaunchesToKeep(@TempDir Path tmp) throws IOException {
// A file that is missing is written the same way, and its partial copy
// would be kept as an edit just as a partial update would.
Path src = Files.createDirectories(tmp.resolve("src"));
Path dst = Files.createDirectories(tmp.resolve("dst"));
Files.writeString(src.resolve("swamp.conf"), "newly shipped profile");
AtomicInteger files = new AtomicInteger();
AtomicLong bytes = new AtomicLong();
AtomicInteger kept = new AtomicInteger();

IOException failure = assertThrows(IOException.class, () -> NativeExtractor.extractAuthorTree(
src, dst, "rtx-remix", NO_HISTORY, NativeExtractorTest::openFailingPartway, files, bytes, kept));

assertEquals(READ_FAILURE, failure.getMessage());
assertEquals(List.of(), fileNames(dst));

NativeExtractor.extractAuthorTree(src, dst, "rtx-remix", NO_HISTORY, Files::newInputStream,
files, bytes, kept);

assertEquals("newly shipped profile", Files.readString(dst.resolve("swamp.conf")));
assertEquals(List.of("swamp.conf"), fileNames(dst));
assertEquals(0, kept.get());
}

@Test
void aVersionChangeUpdatesAConfStillHoldingAnEarlierShippedDefault(@TempDir Path tmp) throws IOException {
// Issue #20: a release that retunes a shipped profile must reach
Expand Down Expand Up @@ -424,6 +489,24 @@ private static ShippedPayloadHistory history(String... lines) {
}
}

/** Opens {@code file} as a stream that yields the first half of its bytes and then fails. */
private static InputStream openFailingPartway(Path file) throws IOException {
byte[] content = Files.readAllBytes(file);
InputStream failing = new InputStream() {
@Override
public int read() throws IOException {
throw new IOException(READ_FAILURE);
}
};
return new SequenceInputStream(new ByteArrayInputStream(content, 0, content.length / 2), failing);
}

private static List<String> fileNames(Path dir) throws IOException {
try (var listing = Files.list(dir)) {
return listing.map(file -> file.getFileName().toString()).sorted().toList();
}
}

/** A history line recording {@code content} as a shipped version of {@code path}. */
private static String line(Path tmp, String content, String path) throws IOException {
Path sample = Files.createTempFile(tmp, "shipped", ".conf");
Expand Down