Conversation
…events Plumbing only: the owner now reaches NotebookServer but routing is unchanged. Delivery is switched over in the following commit.
tbonelee
left a comment
There was a problem hiding this comment.
A naming suggestion.
user does not say that this value is the owner of one execution. Two kinds of it meet on a single line in onParagraphOutputUpdated:
note.getParagraph(paragraphId).getUserParagraphMap().get(user)The defect this PR fixes also came from yet another user, the shared paragraph's own field.
I pushed a version renamed to executionOwner. The Java surface and the thrift field are separate commits, so you can take just the first one:
- Java surface: tbonelee@21eeb16
- thrift field + regeneration: tbonelee@8943de2
Pre-existing names like multicastToUser and getUserParagraphMap mean a delivery target or a map key, so I left those alone.
| // Execution owner. Null when the interpreter predates this field; see | ||
| // NotebookServer.onOutputAppend for how ownerless output is handled. | ||
| 6: string user |
There was a problem hiding this comment.
NotebookServer.onOutputAppend is the Helium app output overload that this PR does not rename (the one taking appId). Ownerless output is dropped in onParagraphOutputAppend, so this pointer sends the reader to a different method.
The null half is already stated by the log line in that branch.
| // Execution owner. Null when the interpreter predates this field; see | |
| // NotebookServer.onOutputAppend for how ownerless output is handled. | |
| 6: string user | |
| 6: string user |
| // The execution owner is fixed here, before any output is produced, so that every event | ||
| // emitted by this output carries the same owner regardless of what runs later. |
There was a problem hiding this comment.
Two of the three things this comment says are already in the code. The value is an argument to createInterpreterOutput, so it cannot be computed after the output exists, and the listener captures it as a final parameter exactly the way it captures noteId and paragraphId, neither of which carries such a comment.
What is left, that this is the execution owner, fits in a name:
private InterpreterContext convert(RemoteInterpreterContext ric) {
return convert(ric, createInterpreterOutput(
ric.getNoteId(), ric.getParagraphId(), executionOwnerOf(ric)));
}
private static String executionOwnerOf(RemoteInterpreterContext ric) {
AuthenticationInfo authenticationInfo =
AuthenticationInfo.fromJson(ric.getAuthenticationInfo());
return authenticationInfo == null ? null : authenticationInfo.getUser();
}| // An interpreter that predates the owner field leaves personalized output unaddressed, | ||
| // so it is dropped rather than sent to every reader of the note. |
There was a problem hiding this comment.
The log line right below says the same thing.
The comments on the update and clear branches are different: they carry facts from outside this file (what checkpointOutput saves, and where a new user's copy is cloned from), so those are worth keeping.
What is this PR for?
In a personalized note, paragraph results are stored per user, but the incremental output produced while a paragraph runs is not. The interpreter knows who started the execution, yet the output events it sends back carry no owner, so
NotebookServercannot tell who the output belongs to. Personalized incremental output is therefore dropped, and a user sees nothing until the paragraph finishes.The owner is lost in two places.
OutputAppendEventandOutputUpdateEventhave no user field, andcreateInterpreterOutputcaptures only the note and paragraph ids from the interpreter context, even though the authentication info is available there too. Recovering it on the server is not an option either. The shared paragraph'suserfield is updated only when the note owner runs it, so it can hold a stale user from an earlier run rather than the owner of the current output.This PR carries the execution owner end to end, in three commits:
userfield to the interpreter output events and thread it from the interpreter to the server. The interpreter captures the owner when the output is created, before any chunks are produced, so every event from that execution carries the same owner.AppendOutputRunnernow keys its buffer by owner as well, because two users running the same paragraph would otherwise have their chunks merged into the same buffer, leaving one chunk with no single owner. Behaviour is unchanged in this commit.multicastToUser, write updates to that user's own paragraph copy rather than the shared one, and make clear events discard only the owner's output. An interpreter that predates the field reports no owner, and that output is still dropped rather than broadcast.Shared mode is untouched. A shared paragraph has only one execution producing output for a given index at a time, so adding the owner to the buffer key does not change its batching behavior.
AppendOutputRunnerTest.appendsFromOneExecutionAreStillBatchedIntoOneChunkverifies this.Two notes on scope. The paragraph output callbacks on
RemoteInterpreterProcessListenerare renamed toonParagraphOutputAppend,onParagraphOutputUpdatedandonParagraphOutputClear: once the owner is added, they have the same erasure as the app output overloads thatNotebookServeralso implements, so the class would not compile. The Helium app output events carry no owner either, but I could not find any personalized code path that uses them, so they are left out of this PR.What type of PR is it?
Bug Fix
Todos
NotebookServerWhat is the Jira issue?
How should this be tested?
Server side:
27 tests pass.
NotebookServerStreamingScopeTestcovers the routing, including a test that reproduces the original failure mode: the shared paragraph is given a stale owner and a different user runs the paragraph, and the output must still reach the user who started that execution.The e2e test signs in as two shiro accounts in separate browser contexts, opens one personalized note, and records the
PARAGRAPH_APPEND_OUTPUTandPARAGRAPH_UPDATE_OUTPUTframes each socket receives. The test asserts on WebSocket frames rather than rendered UI because the bug is in server-side connection routing. It runs on the authenticated leg:Reverting the routing change makes the new tests fail:
Screenshots (if appropriate)
N/A
Questions: