[ZEPPELIN-6551] Fix Job Manager crash on removal broadcast for a note not in the viewer's list#5337
[ZEPPELIN-6551] Fix Job Manager crash on removal broadcast for a note not in the viewer's list#5337kimyenac wants to merge 3 commits into
Conversation
|
@kimyenac Can we add test cases for this issue? :-) |
511489a to
fb091bf
Compare
|
@jongyoul Thanks for the review! Added e2e regression tests in Reverting the fix makes two of them fail with the reported The Job Manager's WebSocket feed is stubbed in the test because |
voidmatcha
left a comment
voidmatcha
left a comment
There was a problem hiding this comment.
Thanks for feedback! One suggestion.
test 1 and test 3 assert toHaveCount(2) right after the removal, but the list already has 2 items, so they pass on a state that is true before the removal is processed. On a regressed build filterJobs throws and the DOM stays frozen at 2, so the assertion stays true for the whole timeout and never catches it. The error check is one-shot too.
Sending a sentinel job after the removal (WebSocket delivery is FIFO) and awaiting its render guarantees the removal ran first, so both tests actually exercise the regression. (test 2 already has a barrier via the search input.)
… not in the viewer's list A permanent note deletion broadcasts a field-less NoteJobInfo stub (noteName=null, isRemoved=true) to every Job Manager subscriber, while each viewer's job list is owner-filtered. For a note the viewer doesn't own, updateJobs pushed the stub (currentJobIndex === -1), then filterJobs threw "Cannot read properties of undefined (reading 'match')" on job.noteName and kept rethrowing, breaking filter/sort until re-navigation. - updateJobs: don't add removal stubs that aren't already in the list. Guard is nested (not merged into the currentJobIndex === -1 condition) so a removal stub can't fall through to the splice(-1) branch. - filterJobs: guard job.noteName with optional chaining as defense in depth.
…broadcast Cover the removal broadcast that `NotebookServer#onNoteRemove` sends to every Job Manager subscriber: a removal stub for a note absent from the viewer's list must be ignored, must not break the note name filter, and must not drop a listed job. The Job Manager's WebSocket traffic is stubbed because `zeppelin.jobmanager.enable` defaults to false and the backend list is owner-filtered, so the cross-user broadcast cannot be produced from a single session. All other WebSocket traffic still goes to the real server.
…egression Ignoring a removal stub leaves the job list untouched, so the assertions that ran right after the broadcast were already satisfied by the pre-broadcast state, and a regressed build freezes the rendered list in that same state, so they could not fail. With the fix reverted the previous spec still passed the "no listed job is dropped" test outright, and the runtime-error test passed or failed depending on when the pageerror happened to land. Broadcast a sentinel job after the removal and wait for it to render instead. Both messages travel the same socket in FIFO order into the same listener, so the sentinel appearing proves the removal was handled first; on a regressed build filterJobs keeps throwing and it never appears. The note name filter test already had that barrier via the search box, so only its redundant count assertion is dropped. Reverting the fix now fails 3 of the 5 tests, and so does folding the removal guard into the currentJobIndex === -1 condition.
|
@voidmatcha Good catch. With the fix reverted, the old spec passed "no listed job is dropped" outright, and the runtime-error test was a coin flip: run alone it passed in 760ms, before the Fixed in 8007d2247. Both tests now broadcast a sentinel job after the removal and wait for it to render. Same socket, FIFO, same listener, so the sentinel appearing proves the removal was handled. On a regressed build Verified on chromium: 5/5 pass with the fix, tests 1-3 fail with it reverted, and the same 3 fail if the guard is folded into |
3 participants
What is this PR for?
When a note is permanently deleted, the backend broadcasts a field-less
NoteJobInfostub (noteName=null,isRemoved=true) to every Job Manager subscriber, while each viewer's initial job list is owner-filtered (JobManagerService#getNoteJobInfoByUnixTime).For a note the viewer does not own,
updateJobsfell into thecurrentJobIndex === -1branch and pushed the stub intothis.jobs.filterJobsthen ranjob.noteName.match(noteNameReg)and threwTypeError: Cannot read properties of undefined (reading 'match'). Because the stub persists inthis.jobs, every subsequentfilterJobs(each filter keystroke / job update) rethrew, so the page's filter/sort stayed broken until re-navigation.This affects any user viewing
/jobmanagerwhen another user permanently deletes a note they don't own.What does this PR do?
updateJobs: do not add removal stubs that aren't already in the list (if (!updateJob.isRemoved)). The guard is kept nested insideif (currentJobIndex === -1)rather than merged into that condition, so a removal stub can never fall through to theelsebranch'ssplice(currentJobIndex, 1)— merging would callsplice(-1, 1)and silently drop the last job.filterJobs: guardjob.noteName?.match(...)with optional chaining as defense in depth.What type of PR is it?
Bug Fix
What is the Jira issue?
How should this be tested?
cd zeppelin-web-angular && npm run lint/jobmanageras user A; as user B permanently delete a note A does not own; confirm A's page keeps filtering without a consoleTypeError.Questions: