From 106c1d27cd4f734f8a843b6c69b4c8e391b5fdc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mikl=C3=B3s=20Fazekas?= Date: Thu, 9 Jul 2026 14:56:27 +0200 Subject: [PATCH] fix(command_queue): deleteFile now cascade-deletes live artboards and their state machines CommandServer::m_fileDependencies[file] was created empty at loadFile and walked by deleteFile, but instantiateArtboard never appended to it - so the cascade was always a no-op. Deleting a file while artboards instantiated from it were still alive orphaned them (and their state machines) server-side, unreachable through any cleanup path. - instantiateArtboard now registers the artboard in m_fileDependencies, mirroring how instantiateStateMachine registers in m_artboardDependencies. - cleanupArtboard un-registers the artboard from m_fileDependencies so explicit create/delete cycles on a long-lived file don't grow the dependency vector unboundedly. - deleteFile detaches the dependency list before cascading (cleanupArtboard now mutates it) and erases the file only after the cascade, since dependent artboards and state machines may reference file-owned data. --- include/rive/command_server.hpp | 9 ++ src/command_server.cpp | 20 +++- .../unit_tests/runtime/command_queue_test.cpp | 109 ++++++++++++++++++ 3 files changed, 132 insertions(+), 6 deletions(-) diff --git a/include/rive/command_server.hpp b/include/rive/command_server.hpp index 3451b2d81..d448f15f0 100644 --- a/include/rive/command_server.hpp +++ b/include/rive/command_server.hpp @@ -6,6 +6,7 @@ #include "rive/command_queue.hpp" #include "rive/hit_result.hpp" +#include #include #include #include @@ -173,6 +174,14 @@ class CommandServer m_artboardDependencies.erase(dependencyItr); } + for (auto& fileDependency : m_fileDependencies) + { + auto& artboardVector = fileDependency.second; + artboardVector.erase(std::remove(artboardVector.begin(), + artboardVector.end(), + handle), + artboardVector.end()); + } m_artboards.erase(itr); std::unique_lock lock(m_commandQueue->m_messageMutex); m_commandQueue->m_messageStream diff --git a/src/command_server.cpp b/src/command_server.cpp index 3903520ea..c99a8e9b3 100644 --- a/src/command_server.cpp +++ b/src/command_server.cpp @@ -739,18 +739,23 @@ bool CommandServer::processCommands() commandStream >> handle; commandStream >> requestId; lock.unlock(); - m_files.erase(handle); auto itr = m_fileDependencies.find(handle); if (itr != m_fileDependencies.end()) { - auto& artboardVector = itr->second; + // Detach the dependency list before cascading: + // cleanupArtboard un-registers artboards from + // m_fileDependencies, which would otherwise mutate the + // vector while we iterate it. + auto artboardVector = std::move(itr->second); + m_fileDependencies.erase(itr); for (auto artboardHandle : artboardVector) { cleanupArtboard(artboardHandle, requestId); } - - m_fileDependencies.erase(itr); } + // Erase the file after the cascade; dependent artboards and + // state machines may reference file-owned data. + m_files.erase(handle); std::unique_lock messageLock( m_commandQueue->m_messageMutex); messageStream << CommandQueue::Message::fileDeleted; @@ -1022,6 +1027,9 @@ bool CommandServer::processCommands() { m_artboardDependencies[handle] = {}; m_artboards[handle] = std::move(artboard); + assert(m_fileDependencies.find(fileHandle) != + m_fileDependencies.end()); + m_fileDependencies[fileHandle].push_back(handle); std::unique_lock messageLock( m_commandQueue->m_messageMutex); @@ -1172,9 +1180,9 @@ bool CommandServer::processCommands() commandStream >> handle; commandStream >> requestId; lock.unlock(); + // cleanupArtboard also un-registers the artboard from + // m_fileDependencies. cleanupArtboard(handle, requestId); - // We don't remove from the file dependencies here because - // calling erase on a non existent key is fine. break; } diff --git a/tests/unit_tests/runtime/command_queue_test.cpp b/tests/unit_tests/runtime/command_queue_test.cpp index 9c250630e..9f0d458d9 100644 --- a/tests/unit_tests/runtime/command_queue_test.cpp +++ b/tests/unit_tests/runtime/command_queue_test.cpp @@ -264,6 +264,115 @@ TEST_CASE("state machine management", "[CommandQueue]") serverThread.join(); } +TEST_CASE("deleteFile cascade-deletes live artboards and their state machines", + "[CommandQueue]") +{ + auto commandQueue = make_rcp(); + std::thread serverThread(server_thread, commandQueue); + + std::ifstream stream("assets/multiple_state_machines.riv", + std::ios::binary); + FileHandle fileHandle = commandQueue->loadFile( + std::vector(std::istreambuf_iterator(stream), {})); + ArtboardHandle artboardHandle1 = + commandQueue->instantiateDefaultArtboard(fileHandle); + ArtboardHandle artboardHandle2 = + commandQueue->instantiateDefaultArtboard(fileHandle); + StateMachineHandle sm1 = + commandQueue->instantiateStateMachineNamed(artboardHandle1, "one"); + StateMachineHandle sm2 = + commandQueue->instantiateStateMachineNamed(artboardHandle2, "two"); + commandQueue->runOnce([fileHandle, + artboardHandle1, + artboardHandle2, + sm1, + sm2](CommandServer* server) { + REQUIRE(server->getFile(fileHandle) != nullptr); + REQUIRE(server->getArtboardInstance(artboardHandle1) != nullptr); + REQUIRE(server->getArtboardInstance(artboardHandle2) != nullptr); + REQUIRE(server->getStateMachineInstance(sm1) != nullptr); + REQUIRE(server->getStateMachineInstance(sm2) != nullptr); + }); + + // Delete the file WITHOUT deleting its artboards or state machines first. + // The file's dependency cascade must clean all of them up. + commandQueue->deleteFile(fileHandle); + commandQueue->runOnce([fileHandle, + artboardHandle1, + artboardHandle2, + sm1, + sm2](CommandServer* server) { + CHECK(server->getFile(fileHandle) == nullptr); + CHECK(server->getArtboardInstance(artboardHandle1) == nullptr); + CHECK(server->getArtboardInstance(artboardHandle2) == nullptr); + CHECK(server->getStateMachineInstance(sm1) == nullptr); + CHECK(server->getStateMachineInstance(sm2) == nullptr); + }); + + commandQueue->disconnect(); + serverThread.join(); +} + +TEST_CASE("explicit deletes before deleteFile do not double-clean", + "[CommandQueue]") +{ + auto commandQueue = make_rcp(); + std::thread serverThread(server_thread, commandQueue); + + std::ifstream stream("assets/multiple_state_machines.riv", + std::ios::binary); + FileHandle fileHandle = commandQueue->loadFile( + std::vector(std::istreambuf_iterator(stream), {})); + ArtboardHandle artboardHandle = + commandQueue->instantiateDefaultArtboard(fileHandle); + StateMachineHandle sm = + commandQueue->instantiateStateMachineNamed(artboardHandle, "one"); + + commandQueue->deleteStateMachine(sm); + commandQueue->deleteArtboard(artboardHandle); + commandQueue->deleteFile(fileHandle); + commandQueue->runOnce( + [fileHandle, artboardHandle, sm](CommandServer* server) { + CHECK(server->getFile(fileHandle) == nullptr); + CHECK(server->getArtboardInstance(artboardHandle) == nullptr); + CHECK(server->getStateMachineInstance(sm) == nullptr); + }); + + commandQueue->disconnect(); + serverThread.join(); +} + +TEST_CASE("commands on cascade-deleted handles are safely rejected", + "[CommandQueue]") +{ + auto commandQueue = make_rcp(); + std::thread serverThread(server_thread, commandQueue); + + std::ifstream stream("assets/multiple_state_machines.riv", + std::ios::binary); + FileHandle fileHandle = commandQueue->loadFile( + std::vector(std::istreambuf_iterator(stream), {})); + ArtboardHandle artboardHandle = + commandQueue->instantiateDefaultArtboard(fileHandle); + StateMachineHandle sm = + commandQueue->instantiateStateMachineNamed(artboardHandle, "one"); + + commandQueue->deleteFile(fileHandle); + + // The client still holds handles to the cascade-deleted objects; using + // them must route through the normal unknown-handle error path. + commandQueue->advanceStateMachine(sm, 0.016f); + commandQueue->deleteStateMachine(sm); + commandQueue->deleteArtboard(artboardHandle); + commandQueue->runOnce([artboardHandle, sm](CommandServer* server) { + CHECK(server->getArtboardInstance(artboardHandle) == nullptr); + CHECK(server->getStateMachineInstance(sm) == nullptr); + }); + + commandQueue->disconnect(); + serverThread.join(); +} + TEST_CASE("default artboard & state machine", "[CommandQueue]") { auto commandQueue = make_rcp();