fix(recordings): Fix active recordings table refresh behavior for replace:"ALWAYS"

[Josh-Matsuoka]

Fixes: #1286

This changes the activeRecordingsTable notification handling logic to call refreshRecordingsList when receiving creation/deletion notifications rather than updating with only the fields in the notification. This ensures that the state of the table remains accurate.

When the query in the linked issue is run a second time it results in the notifications being fired in the following order:

Recording Created
Recording Deleted

Pre-patch the table would see the recording deleted notification last and remove it from the table despite the recording still being present in the target. Calling refreshRecordingsList and getting the state of the target's recordings from the API shows the same.

How to manually test:

1. Run Cryostat via smoketest or ./mvnw quarkus:dev
2. Run the following query twice: (Replace 8080 with 8181 if in quarkus:dev mode)

http --follow -v --auth='user:pass' POST :8080/api/v4/graphql
Content-Type:application/json
query='
query {
targetNodes(filter: { annotations: ["PORT = 9091"] }) {
name
target {
doStartRecording(recording: {
name: "test9",
template: "Profiling",
templateType: "TARGET",
replace: "ALWAYS"
}) {
name
state
}
}
}
}'

[andrewazores]

Is there anything we can do on the server side to ensure that the recording is deleted first, then immediately recreated (replaced)? This PR hacks around the problem by partially ignoring the state update notifications, but the underlying problem is still there and would affect any other hypothetical clients listening to the same notifications.

…

---- On Fri, 17 Jan 2025 16:35:12 -0500 ***@***.*** wrote ---- Fixes: #1286 This changes the activeRecordingsTable notification handling logic to call refreshRecordingsList when receiving creation/deletion notifications rather than updating with only the fields in the notification. This ensures that the state of the table remains accurate. When the query in the linked issue is run a second time it results in the notifications being fired in the following order: Recording Created Recording Deleted Pre-patch the table would see the recording deleted notification last and remove it from the table despite the recording still being present in the target. Calling refreshRecordingsList and getting the state of the target's recordings from the API shows the same. How to manually test: Run Cryostat via smoketest or ./mvnw quarkus:dev Run the following query twice: (Replace 8080 with 8181 if in quarkus:dev mode) http --follow -v --auth='user:pass' POST :8080/api/v4/graphql Content-Type:application/json query=' query { targetNodes(filter: { annotations: ["PORT = 9091"] }) { name target { doStartRecording(recording: { name: "test9", template: "Profiling", templateType: "TARGET", replace: "ALWAYS" }) { name state } } } }' You can view, comment on, or merge this pull request online at:   #1535 Commit Summary d9df210 Fix ActiveRecordingsTable refresh behavior File Changes (1 file) M src/app/Recordings/ActiveRecordingsTable.tsx (9) Patch Links: https://github.com/cryostatio/cryostat-web/pull/1535.patch https://github.com/cryostatio/cryostat-web/pull/1535.diff — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.

[Josh-Matsuoka]

Looking at the server side we wait for deleteRecording to finish already before continuing to recreating the new one:

getActiveRecording(target, r -> r.name.equals(recordingName))
.ifPresent(r -> this.deleteRecording(r).await().atMost(connectionFailedTimeout));

but it looks like this hooks into the existing transaction via QuarkusTransaction.joiningExisting() so the PostRemove hook will be called after PostPersist if I'm understanding correctly? I'm not entirely clear on how Jakarta and quarkus are interacting here.

[andrewazores]

Makes sense. I think the best approach to fix this bug is to ensure that the operations and their hooks occur in the expected order. It might require some restructuring of that implementation. https://quarkus.io/guides/transaction may be useful.

…

---- On Fri, 17 Jan 2025 17:31:57 -0500 ***@***.*** wrote ---- Looking at the server side we wait for deleteRecording to finish already before continuing to recreating the new one: getActiveRecording(target, r -> r.name.equals(recordingName)) .ifPresent(r -> this.deleteRecording(r).await().atMost(connectionFailedTimeout)); but it looks like this hooks into the existing transaction via QuarkusTransaction.joiningExisting() so the PostRemove hook will be called after PostPersist if I'm understanding correctly? I'm not entirely clear on how Jakarta and quarkus are interacting here. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

[andrewazores]

I think there is also another potential avenue for a fix that doesn't involve reordering transactions, so it should be simpler and less invasive to implement. Since 3.0, recordings have not only a name but also a unique database id field, as well as a remoteId field which mirrors the recording ID assigned by the target JVM instance. I think the WebSocket notifications already include the id field, but the web client handling for the notifications was all written long before that when recordings had no IDs and names were used as unique keys instead. I think updating the client's internal modelling of recordings to include the id, and then listening for and processing the notifications by using the ID to perform state updates instead of the name, should also fix the problem.