[ZEPPELIN-5718] Notebook lost due to non-atomic file operations.

[JangAyeon]

What is this PR for?

A note can disappear after a restart because saving it is not atomic.

FileSystemStorage.writeFile writes note.zpln.tmp, deletes note.zpln, then renames the temp file. If Zeppelin stops between the delete and the rename, only the .tmp file is left. list() picks up .zpln files only, so the note looks lost. rename() also returns false instead of throwing, and the result was ignored. VFSNotebookRepo.save has the same problem, because FileObject.moveTo deletes the destination before renaming.

This PR:

• FileSystemStorage.writeFile: renames the original to .bak instead of deleting it, and deletes the backup only after the temp file is renamed. Each rename() result is checked, and the original is restored from the backup if the final rename fails. FileSystemConfigStorage and FileSystemRecoveryStorage use the same method, so their files get the same protection.
• FileSystemNotebookRepo.init: restores notes left by an interrupted save, before they are listed. A .zpln file is restored only when it is missing and both its .bak and .tmp files exist:

Files left	When	Result
.zpln + .tmp	Stopped while writing .tmp	Nothing changes. .zpln is complete
.bak + complete .tmp	Stopped between the two renames	Restored from .tmp (latest content)
.bak + incomplete .tmp	-	Restored from .bak
.tmp only or .bak only	Leftover of a removed or moved note, or a note lost by earlier versions	Not restored. Logged as a warning

A .tmp file counts as complete when it parses as a note, since it may be cut off if the write itself was interrupted.
A single leftover file is not restored, because it can't be told apart from a leftover of a removed or moved note.

• VFSNotebookRepo.save: for local file:// paths, replaces the note with Files.move(ATOMIC_MOVE, REPLACE_EXISTING), so the note file is never missing. The affected FileObjects are refreshed, because the move bypasses the VFS cache. Other file systems keep using moveTo, and so do local file systems that do not support atomic moves.

On the question in the issue about dropping VFS2 for local files: this PR keeps VFS2 and uses NIO only for this one move, to keep the change small.

What type of PR is it?

Bug Fix

Todos

• Keep the original file until the new one is in place
• Recover notes left by an interrupted save
• Replace local note files atomically in VFSNotebookRepo
• Add tests

What is the Jira issue?

ZEPPELIN-5718

How should this be tested?

• FileSystemNotebookRepoTest: a normal save leaves no .tmp or .bak file. The other tests create the files a crash leaves behind (the rows of the table above), restart the repo, and check the restored content.
• VFSNotebookRepoTest: saving over an existing note replaces its content and leaves only the .zpln file.

./mvnw clean install -DskipTests -pl zeppelin-server -am
./mvnw test -pl zeppelin-server -Dtest=VFSNotebookRepoTest
./mvnw test -pl zeppelin-plugins/notebookrepo/filesystem -Dtest=FileSystemNotebookRepoTest

Screenshots (if appropriate)

N/A

Questions:

• Does the license files need to update? No
• Is there breaking changes for older versions? No. Notes lost by earlier versions (only a .tmp file left) are not restored automatically, but a warning with the file path is logged so they can be recovered manually.
• Does this needs documentation? No

[kimyenac]

Keeping the original until the new file is in place looks like the right direction to me. I ran into one case, though, where this check treats a note that was already deleted or moved as an interrupted save and brings it back.

What happens

recoverInterruptedWrites restores a .zpln whenever it is missing and a complete .zpln.tmp exists. But FileSystemNotebookRepo.remove() and move() only delete/rename the .zpln itself, so a .tmp (or .bak) left next to it stays behind. On the next restart, that leftover is treated as an interrupted save:

• note removed → it comes back after restart
• note moved → a second .zpln with the same noteId appears at the old path; list() keys by noteId, so only one of the two is listed

A complete .tmp can be left next to an existing .zpln in two ways with this PR:

1. Zeppelin stops after the .tmp is fully written but before file is renamed to .bak.
2. rename(tmpFile, file) fails: replaceFile restores the original from .bak and throws, but does not delete .tmp. This one does not need a crash.

I checked (2) end to end by making rename(*.zpln.tmp → *.zpln) fail once: the save throws, the original .zpln and a complete .tmp remain, and after remove() + restart the note comes back, with the content of the save that had failed.

Repro

These two tests fail on the PR branch when added to FileSystemNotebookRepoTest:

@Test
void testRemovedNoteIsNotRestoredFromLeftoverTmp() throws IOException {
  Note note = createNote("/title_1", "value_1");
  hdfsNotebookRepo.save(note, authInfo);
  writeString(noteFile(note, ".tmp"), note.toJson());
  hdfsNotebookRepo.remove(note.getId(), note.getPath(), authInfo);

  restartRepo();

  assertEquals(0, hdfsNotebookRepo.list(authInfo).size());  // actual: 1
}

@Test
void testMovedNoteIsNotDuplicatedFromLeftoverTmp() throws IOException {
  Note note = createNote("/title_1", "value_1");
  hdfsNotebookRepo.save(note, authInfo);
  writeString(noteFile(note, ".tmp"), note.toJson());
  hdfsNotebookRepo.move(note.getId(), "/title_1", "/dir/title_2", authInfo);

  restartRepo();

  long zplnCount;
  try (Stream<java.nio.file.Path> files = Files.walk(Paths.get(notebookDir))) {
    zplnCount = files.filter(f -> f.toString().endsWith(".zpln")).count();
  }
  assertEquals(1, zplnCount);  // actual: 2
}

Possible fixes (just ideas, happy to go with whatever you prefer)

• In remove() / move(), also delete (or move along) the sibling .tmp / .bak.
• In replaceFile, delete tmpFile when the final rename fails and the original has been restored.
• Only auto-recover when both .bak and .tmp exist without the .zpln, which is exactly the state this PR's writeFile leaves when it is interrupted between the two renames. For the .tmp-only case (notes lost by earlier versions), a complete .tmp can't be told apart from a leftover of a deleted note, so logging it or moving it aside for manual recovery might be safer than restoring it automatically.

[JangAyeon]

Thanks for the careful review and the repro tests, that was a real gap.

I went with options 2 and 3 in comment:

• replaceFile now deletes .tmp when the final rename fails and the original has been restored.
• Recovery only runs when both .bak and .tmp exist without the .zpln, which is the state writeFile leaves when it stops between its two renames. A single leftover .tmp or .bak is logged instead of restored.

I also added your two tests, and they pass now. As you pointed out, this means notes lost by earlier versions (only .tmp left) are no longer restored automatically, so I updated the PR description accordingly.

I didn't change remove() / move() for option 1, to keep this PR focused on the save path. Happy to add it here or in a follow-up if you think it's needed.

[voidmatcha]

The correctness issue raised earlier appears to be resolved in the latest commit. The code looks good to me. 👍

One small documentation mismatch remains: a lone .tmp file is no longer recovered automatically, so please update the PR description and the recoverInterruptedSaves() Javadoc to match the current behavior.

[JangAyeon]

Thanks for the review! I updated the recoverInterruptedSaves() Javadoc in the latest commit and the PR description to match the current behavior.