[ZEPPELIN-5858] Fix moveNote/saveNote race that duplicates notes

[HwangRock]

What is this PR for?

moveNote mutates the folder tree, the notesInfo mapping and the notebook repo without holding any lock, while saveNote derives its target file name from the path carried by the Note object at save time. A save racing with a move therefore writes the note back to its pre-move path, and the same noteId ends up on disk twice. ZEPPELIN-5858 describes exactly this and ships a reproduction; this PR ports that reproduction onto the current API and fixes the race.

Reproduced on current master with the ported test (VFSNotebookRepoWithDelay extends VFSNotebookRepo and injects repo latency, e.g. remote storage):

Saving note 2MXKZT37A to folder_1/note_2MXKZT37A.zpln
Move note 2MXKZT37A to /folder_2/note
Move note 2MXKZT37A from /folder_1/note to /folder_2/note
Saving note 2MXKZT37A to folder_1/note_2MXKZT37A.zpln   <- stale path, resurrects the old file

Expected exactly one .zpln file, but found:
  [folder_2/note_2MXKZT37A.zpln, folder_1/note_2MXKZT37A.zpln]

The fix serializes NoteManager's persistent mutations: moveNote, removeNote and moveFolder now run their tree, mapping and repo mutations inside the monitor saveNote already synchronizes on. Save callers and moveNote share the cached Note instance, so a save that loses the monitor to a concurrent move derives its file name from the already-updated path once it enters — no note is written to its pre-move location. saveNote's existing contract (the Note object's path is authoritative; addOrUpdateNoteNode syncs the mapping to it) is deliberately left untouched, since rename-by-save flows like Notebook.updateNote and note import depend on it.

Two deliberate details:

• The fixed lock order in this code base is note readLock -> NoteManager monitor, because every save caller runs inside processNote holding the note's readLock. moveNote therefore updates the cached note path directly via the note cache instead of calling processNote while holding the monitor, and the rename-triggered resave stays outside the monitor and reuses saveNote. This keeps the lock order consistent on every path.
• The rename-triggered resave realigns the reloaded note with the move target before saving: a note evicted from the cache is reloaded from disk at that point, and the on-disk JSON still carries the pre-move name.

Verified the causality both ways: the reproduction test fails with two .zpln files for the same noteId on master, passes with this change, and fails again if only the NoteManager change is reverted.

What type of PR is it?

Bug Fix

Todos

• Port the reproduction attached to ZEPPELIN-5858 onto the current NotebookRepo API
• Serialize NoteManager mutations

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-5858

How should this be tested?

mvn test -pl zeppelin-server -Dtest=NotebookServiceRaceConditionTest — the test runs renameNote and insertParagraph concurrently against a delay-injecting VFSNotebookRepo subclass, then walks the notebook directory and asserts exactly one .zpln file remains. Without the NoteManager change it finds the same noteId at both the old and the new path.

Regression: NoteManagerTest, NotebookServiceTest, NotebookTest, LuceneSearchTest, ZeppelinRestApiTest (86 tests) all pass.

Questions:

• Does the license files need to update? No
• Is there breaking changes for older versions? No
• Does this needs documentation? No

[tbonelee]

Could you please rebase this onto master branch?

[HwangRock]

@tbonelee Rebased onto master. Tests still pass, thanks.

[tbonelee]

Thanks for bringing this one with an executable reproduction rather than just a description. I ran a few things before reviewing:

what I ran	result
PR head b8a4c22 as-is	NotebookServiceRaceConditionTest, NoteManagerTest, NotebookServiceTest: 13/13 pass
revert only NoteManager.java to master	fails with [folder_2/note_2N1CCKM4T.zpln, folder_1/note_2N1CCKM4T.zpln]

The diagnosis and the fix line up. LGTM.

I left one optional note on the resave block. Nothing that should hold this up.

[tbonelee]

I might be reading this wrong, so take it with a grain of salt.

This resave runs outside the monitor, so my reading is that if another move lands in between, newNotePath may no longer be the current one. In a synthetic setup, where I pin the interleaving with latches, that does show up: master passes and this branch ends up with two files.

expected: <[folder_2/renamed_x.zpln]>
but was:  <[folder_1/renamed_x.zpln, folder_2/renamed_x.zpln]>

But I went through the callers afterwards and could not find a path that actually produces this combination, so it may be unreachable in practice. If it is already ruled out by something I did not look at, please just ignore this.

One option, if you think it is worth anything: newNotePath is fixed at call time whereas notesInfo holds the value as of the read, so reading the latter might be slightly more robust.

String currentPath = this.noteTree.notesInfo.get(noteId);
if (currentPath != null) {
  note.setPath(currentPath);
  saveNote(note, subject);
}

One thing I did confirm, in case it saves you time: dropping the line on its own does not work. NoteManagerTest#testConcurrentOperation starts failing (expected: </dev/project_0/my_note0> but was: </dev/project_0/note0>). With the form above, the three classes in the table above still pass.

If you want to look at the interleaving: set zeppelin.note.cache.threshold to 1 and create a filler note so the note under test gets evicted. Thread 1 calls notebook.moveNote(id, "/folder_1/renamed") and parks inside NotebookRepo#get during the resave. Thread 2 then runs notebook.moveNote(id, "/folder_2/renamed") to completion, which has no resave of its own because the name is unchanged. Then let thread 1 continue. Happy to share the test if that is useful.

Genuinely fine to leave as is.

[HwangRock]

@tbonelee Thanks for running it before reviewing — that saved me a lot of guessing here.

You were reading it right. I ported your setup into a test (threshold at 1, a filler note to force the eviction, a latch inside NotebookRepo#get so thread 1 parks there while thread 2 finishes) and it fails the same way on every run:

Expected exactly one .zpln file for note 2MZVA3AWG,
but found: [folder_2/renamed_2MZVA3AWG.zpln, folder_1/renamed_2MZVA3AWG.zpln]

On whether it is reachable — I think it is. You need one move that renames and one that does not, and the second kind exists on the trash paths: restoreNote() in NotebookService.java:697 only strips /~Trash and keeps the leaf name, and restoreFolder goes through moveNote() in Notebook.java:585 per note, same thing. Meanwhile renameNote() in NotebookService.java:302 calls moveNote from inside processNote, which holds the note's read lock, and that one is shared — so two callers can be inside at the same time. A rename racing a trash restore lands on exactly your interleaving.

About the form you suggested — reading notesInfo is the right idea, but I don't think it closes the window on its own. The read and saveNote taking the monitor are still two separate steps, so a move landing in between writes the stale path again. I instrumented that read point and it does reproduce. Your latch sits just before the read, which is why it passes there.

So I'd rather put the read and the save in the same monitor section, along with the null guard processNote wants when the id is already gone:

processNote(noteId, note -> {
  if (note == null) {
    return null;
  }
  synchronized (this) {
    String currentPath = this.noteTree.notesInfo.get(noteId);
    if (currentPath != null) {
      note.setPath(currentPath);
      saveNote(note, subject);
    }
  }
  return null;
});

Lock order stays as it was — processNote takes the read lock first and the monitor second — and the only extra work under the monitor is a map lookup, since saveNote already does its repo write in there.

Two things I found while digging into this that I'd rather keep out of this PR: removeFolder and addNote are still unsynchronized, which is an asymmetry this PR introduces, and the resave only exists because loadAndProcessNote derives the leaf name from the JSON name instead of from the tree. Happy to file both separately.

I have the fix and the reproduction ready — let me know if this direction looks right and I'll push.

[tbonelee]

Direction looks right to me, please push.

You are right that reading notesInfo on its own leaves the read and the save as two steps. I had that in an earlier draft and trimmed it before posting, which I should not have. The note == null guard is a good addition too.

Both follow-ups sound good, please go ahead.

[HwangRock]

@tbonelee Pushed as e3f715b. Filed the follow-ups as ZEPPELIN-6594 and ZEPPELIN-6595.

NoteManagerMoveResaveRaceTest pins the interleaving — fails on the previous commit, passes on this one. All 14 tests across the four classes green.