Open
Conversation
…rashed" This reverts commit 5b0efd1.
…reconnection timeout is set to 0
📝 WalkthroughWalkthroughThis PR introduces crash dump collection and management capabilities with platform-specific implementations (RVC2/RVC4), relocates the Platform enum to a dedicated header, refactors compression utilities from tar-specific to generic archive APIs, and expands Python bindings to expose crash dump and platform functionality. Changes span core library, device management, bindings, utilities, examples, and tests. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Dev as Device
participant CB as Callback Handler
participant CDM as CrashDumpManager
participant Dump as CrashDump
App->>Dev: registerCrashdumpCallback(callback)
App->>Dev: crashDevice()
Note over Dev: Device crashes and reboots
Dev->>CB: Trigger crash detected
CB->>CDM: collectCrashDump()
CDM->>Dump: Create platform-specific<br/>CrashDumpRVC2/RVC4
CDM->>Dump: Populate metadata<br/>(version, timestamp, deviceId)
Dump->>Dump: Serialize to tar/bytes
CB->>App: callback(std::shared_ptr<CrashDump>)
App->>Dev: hasCrashed()
Dev-->>App: true
App->>Dump: operator[](key) access<br/>extra JSON data
App->>App: Process and log crash info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utility/Compression.cpp (1)
131-134:⚠️ Potential issue | 🟡 MinorPotential issue with large files or invalid entry sizes.
The entire file is read into memory at once. For large files, this could cause memory issues. Additionally,
archive_entry_sizemay return negative values for unknown sizes (e.g., streaming archives).Consider adding validation and potentially chunked reading for large files.
Suggested validation
size_t size = archive_entry_size(entry); + if(size == 0) { + outFileStream.close(); + break; + } std::vector<uint8_t> buff(size); - archive_read_data(a, buff.data(), size); + la_ssize_t bytesRead = archive_read_data(a, buff.data(), size); + if(bytesRead < 0) { + outFileStream.close(); + throw std::runtime_error(fmt::format("Failed to read data from archive: {}", archive_error_string(a))); + } - outFileStream.write(reinterpret_cast<char*>(buff.data()), size); + outFileStream.write(reinterpret_cast<char*>(buff.data()), bytesRead);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utility/Compression.cpp` around lines 131 - 134, archive_entry_size(entry) can be negative/unknown and allocating a vector of that size reads the whole entry into memory, which fails for large or streaming entries; instead, validate the size and perform chunked reads: if archive_entry_size(entry) > 0 you may still prefer to read in fixed-size chunks, and if size is negative/unknown, loop calling archive_read_data(a, buffer, bufferSize) until it returns 0 (EOF) or error, writing each returned byte-count to outFileStream; reference archive_entry_size, archive_read_data, buff (replace with a fixed-size stack/heap buffer), outFileStream, entry and a to implement this safe, incremental read-and-write loop and handle/archive_read_* error returns appropriately.src/device/DeviceBase.cpp (1)
561-568:⚠️ Potential issue | 🔴 CriticalGate crash handling needs an RVC4 + gate guard.
This block now runs for every
!isRvc2device. Butgateis only initialized in theX_LINK_GATE*boot path, andCrashDumpManagerstill throws onPlatform::RVC3. That gives you either a null dereference at Line 565 or an exception from Line 609 duringclose().Proposed fix
bool waitForGate = true; - if(!isRvc2) { + const bool isRvc4 = deviceInfo.platform == X_LINK_RVC4; + if(isRvc4 && gate) { // Check if the device is still alive and well, if yes, don't wait for gate, crash dump not relevant try { auto gateState = gate->getState(); crashed = (gateState == DeviceGate::SessionState::CRASHED || gateState == DeviceGate::SessionState::DESTROYED); waitForGate = !pimpl->rpcCall("isRunning").as<bool>(); pimpl->logger.debug("Will wait for gate: {}", waitForGate); } catch(const std::exception& ex) { pimpl->logger.debug("isRunning call error: {}", ex.what()); } } ... - if(gate && waitForGate) { + if(isRvc4 && gate && waitForGate) { if(crashed && !crashDumpHandled.load()) { collectAndLogCrashDump(); } gate->waitForSessionEnd(); }Also applies to: 607-611
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/DeviceBase.cpp` around lines 561 - 568, The gate/crash handling currently runs for all !isRvc2 devices but gate and CrashDumpManager are only valid for RVC4 paths; fix by adding an explicit RVC4 + gate guard around this whole block (use isRvc4 && gate != nullptr) so you only call gate->getState(), set waitForGate, log via pimpl->logger, and create/use CrashDumpManager when isRvc4 && gate is non-null; also apply the same isRvc4 && gate != nullptr guard to the later close()/cleanup section referenced (the code that may throw around CrashDumpManager and gate), and keep existing checks like DeviceGate::SessionState::CRASHED/DESTROYED and pimpl->rpcCall("isRunning").as<bool>() intact inside the guarded region.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/depthai/device/CrashDump.hpp`:
- Around line 204-206: Extract the repeated literal "1.0.0" into a single
constant on the base class CrashDump (e.g., a protected static constexpr const
char* CRASH_DUMP_FORMAT_VERSION) and then update the derived classes
CrashDumpRVC2 and CrashDumpRVC4 to return that constant from
getCrashDumpVersion() instead of the hard-coded string.
In `@include/depthai/device/DeviceBase.hpp`:
- Around line 970-977: The code uses inconsistent camelCase for crash dump
identifiers; rename crashdumpCallback and crashdumpCallbackMtx to match
crashDumpHandled (e.g., crashDumpCallback and crashDumpCallbackMtx) and update
all references/usages accordingly (including any captures, assignments, mutex
locks, and function parameter references) so the identifier casing is consistent
across DeviceBase.hpp and any compilation units that reference these symbols.
In `@include/depthai/device/DeviceGate.hpp`:
- Around line 45-46: The public API change to waitForSessionEnd() is breaking;
add a deprecated compatibility shim in the DeviceGate class that preserves the
original waitForSessionEnd() behavior and forwards to the new flow (e.g., call
the new wait method(s) and then getCrashDump() as the old sequence did), mark
the shim as [[deprecated]] and document the migration to the new explicit
getCrashDump() call so consumers have time to adopt the new API; reference the
existing symbols waitForSessionEnd() and getCrashDump() when implementing the
wrapper so it simply invokes the new sequence and returns the original expected
result/side-effects.
In `@src/device/CrashDump.cpp`:
- Around line 293-297: The metadata written by CrashDumpRVC4::toTar includes
originalFilename but the deserializers never restore it; update
CrashDumpRVC4::fromTar (and the other deserialization path around the 336-339
area) to read metadata["originalFilename"] (or use
metadata.value("originalFilename", "")) and assign it to the
CrashDumpRVC4::filename member so filename is preserved across archive/bytes
round-trips.
In `@src/device/CrashDumpManager.cpp`:
- Around line 59-67: The CrashDumpCollector is never setting the CrashDump field
depthaiBuildDatetime before serialization; update the same initialization block
that sets dump->depthaiVersionBuildInfo to also assign
dump->depthaiBuildDatetime (e.g., from build::BUILD_DATETIME or the appropriate
build timestamp constant) so that writeMetadata() persists a populated
depthaiBuildDatetime; locate the initialization using the dump pointer and the
depthaiVersion* assignments to add this single assignment.
In `@src/device/CrashDumpManager.hpp`:
- Around line 25-27: CrashDumpManager is declared movable but contains a raw
DeviceBase* (devicePtr), so default moves leave both objects pointing to the
same device; replace the defaulted move constructor and move assignment by
implementing custom move operations that transfer ownership and nullify the
source's devicePtr (update CrashDumpManager(CrashDumpManager&&) to set
this->devicePtr = other.devicePtr and then other.devicePtr = nullptr, and
implement CrashDumpManager& operator=(CrashDumpManager&&) to guard
self-assignment, release or nullify current devicePtr if needed, move the
pointer from other, set other.devicePtr = nullptr, and return *this) so the
moved-from object is left in a safe empty state.
In `@src/device/DeviceBase.cpp`:
- Around line 1157-1164: The monitor currently only treats
DeviceGate::SessionState::CRASHED as a crash and may skip dump collection for
gates that go straight to DeviceGate::SessionState::DESTROYED; update the check
around gate->getState() (and the local crashed flag) to consider either CRASHED
or DESTROYED as a crash condition so that the existing logic that calls
collectAndLogCrashDump() when crashed && !crashDumpHandled.load() will run for
DESTROYED states as well.
- Around line 465-483: crashDumpHandled is being set before running the user
callback and logCollection::logCrashDump, so move the
crashDumpHandled.store(true) to after those operations and only set it when they
complete successfully; specifically, keep the callback copy logic using
crashdumpCallback and crashdumpCallbackMtx, invoke callbackCopy(crashDump) and
then call logCollection::logCrashDump(pipelineSchema, *crashDump, deviceInfo),
and only after both succeed call crashDumpHandled.store(true). Also wrap the
callback and logCrashDump calls in a try/catch so exceptions are caught and
logged via pimpl->logger.warn/error (and crashDumpHandled remains false on
failure) to allow retry paths to still process the dump.
- Around line 492-515: The loop currently exits as soon as getDeviceById()
returns any match because it uses while(!found && ...), causing a timeout if the
device is still in its pre-reboot state; change the loop condition and logic so
polling continues until either the device is observed in a reboot state (check
rebootingDeviceInfo.state == X_LINK_UNBOOTED || X_LINK_BOOTLOADER) or the
timeout elapses. Concretely, keep calling XLinkConnection::getDeviceById(...)
each iteration and only break/handle the crash when found AND the state matches
the reboot states (the same check used to construct DeviceBase rebootingDevice),
otherwise continue polling until timeout; update references to found,
rebootingDeviceInfo, getDeviceById, and the do/while condition accordingly so
you don't stop on the first sighting of an old-state device.
In `@src/utility/Compression.cpp`:
- Line 121: Replace the debug-only assert with a proper runtime check: in the
scope where filesInArchive and filesOnDisk are compared (the code currently
contains assert(filesInArchive.size() == filesOnDisk.size());), validate sizes
at runtime and handle mismatch deterministically (throw a suitable exception or
return an error code/value and log a clear message) instead of using assert;
ensure the check references filesInArchive.size() and filesOnDisk.size() and
that subsequent code path assumes sizes are equal only after the runtime
validation succeeds.
- Line 21: The assert(filesOnDisk.size() == filesInArchive.size()) must be
replaced with a runtime parameter validation that handles mismatches instead of
being removed in release builds; check sizes of filesOnDisk and filesInArchive,
and if they differ, either throw a descriptive exception (e.g.,
std::invalid_argument or std::runtime_error) or return an error status, logging
the sizes and context, so the subsequent loop that iterates both containers
cannot go out-of-bounds; update the surrounding function (the code block using
filesOnDisk/filesInArchive) to propagate the error appropriately.
In `@src/utility/Platform.cpp`:
- Around line 172-177: The current implementation builds basePath/baseStr and
uses _wmktemp_s plus std::filesystem::create_directories, which has a TOCTOU
race; replace this with a retry loop that atomically creates a uniquely-suffixed
directory using the Windows API CreateDirectory (or CreateDirectoryW) rather
than generating a name then creating it, e.g. generate a random/GUID suffix
appended to basePath (where basePath and baseStr are used) and call
CreateDirectoryW in a loop until it succeeds or a retry limit is reached; on
success return the created std::filesystem::path, and on repeated failure throw
the same std::runtime_error with an explanatory message.
In `@tests/src/ondevice_tests/crashdump_test.cpp`:
- Around line 45-58: Replace the fixed 12-second sleep in the TEST_CASE with a
polling loop that repeatedly checks device.hasCrashed() after short sleeps
(e.g., 100–200ms) up to a maximum timeout (e.g., 12s); call device.crashDevice()
as before, then loop: if device.hasCrashed() break and REQUIRE true, otherwise
sleep a short interval and retry until timeout and then
REQUIRE(device.hasCrashed()) fails—update references in the test to use
dai::Device, device.crashDevice(), device.hasCrashed(),
std::this_thread::sleep_for and std::chrono for timing.
In `@tests/src/ondevice_tests/pipeline/node/record_replay_test.cpp`:
- Around line 55-61: filenamesInArchive(RECORDING_PATH) can include directory
entries (paths ending with '/'), and extractFiles(...) will fail if parent
directories don't exist; update the loop that builds recordingExtFiles to skip
entries that are directories (e.g., filename.back() == '/') and for each file
entry call std::filesystem::create_directories(recordingPath.parent_path()) (use
testFolder + "extracted" base) before calling
dai::utility::extractFiles(RECORDING_PATH, recordingFilenames,
recordingExtFiles); ensure you only pass file entries in recordingFilenames and
matching created parent directories for recordingExtFiles when calling
extractFiles (see filenamesInArchive, recordingFilenames, recordingExtFiles,
extractFiles, RECORDING_PATH, testFolder).
In `@tests/src/onhost_tests/replay_test.cpp`:
- Around line 29-35: The code passes raw recordingFilenames from
dai::utility::filenamesInArchive(RECORDING_PATH) into extractFiles which fails
if the archive contains directory entries or nested paths because extractFiles
expects destination parent directories to exist; update the loop that builds
recordingExtFiles to skip directory entries (e.g., filter out entries that are
directory headers or that end with a slash) and ensure you call
std::filesystem::create_directories(...) for each destination's parent_path
before calling dai::utility::extractFiles; reference filenamesInArchive,
recordingFilenames, recordingExtFiles, extractFiles, RECORDING_PATH and
testFolder when making the change.
---
Outside diff comments:
In `@src/device/DeviceBase.cpp`:
- Around line 561-568: The gate/crash handling currently runs for all !isRvc2
devices but gate and CrashDumpManager are only valid for RVC4 paths; fix by
adding an explicit RVC4 + gate guard around this whole block (use isRvc4 && gate
!= nullptr) so you only call gate->getState(), set waitForGate, log via
pimpl->logger, and create/use CrashDumpManager when isRvc4 && gate is non-null;
also apply the same isRvc4 && gate != nullptr guard to the later close()/cleanup
section referenced (the code that may throw around CrashDumpManager and gate),
and keep existing checks like DeviceGate::SessionState::CRASHED/DESTROYED and
pimpl->rpcCall("isRunning").as<bool>() intact inside the guarded region.
In `@src/utility/Compression.cpp`:
- Around line 131-134: archive_entry_size(entry) can be negative/unknown and
allocating a vector of that size reads the whole entry into memory, which fails
for large or streaming entries; instead, validate the size and perform chunked
reads: if archive_entry_size(entry) > 0 you may still prefer to read in
fixed-size chunks, and if size is negative/unknown, loop calling
archive_read_data(a, buffer, bufferSize) until it returns 0 (EOF) or error,
writing each returned byte-count to outFileStream; reference archive_entry_size,
archive_read_data, buff (replace with a fixed-size stack/heap buffer),
outFileStream, entry and a to implement this safe, incremental read-and-write
loop and handle/archive_read_* error returns appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f8121a3-e8a5-4c94-b056-42eda5817494
📒 Files selected for processing (39)
CMakeLists.txtREADME.mdbindings/python/CMakeLists.txtbindings/python/src/CrashDumpBindings.cppbindings/python/src/CrashDumpBindings.hppbindings/python/src/DeviceBindings.cppbindings/python/src/PlatformBindings.cppbindings/python/src/PlatformBindings.hppbindings/python/src/py_bindings.cppcmake/Depthai/DepthaiDeviceSideConfig.cmakeexamples/cpp/Misc/CrashDump/CMakeLists.txtexamples/cpp/Misc/CrashDump/crash_dump.cppexamples/python/Misc/CrashDump/crash_dump.pyinclude/depthai/device/CrashDump.hppinclude/depthai/device/Device.hppinclude/depthai/device/DeviceBase.hppinclude/depthai/device/DeviceGate.hppinclude/depthai/device/Platform.hppinclude/depthai/utility/Compression.hppinclude/depthai/xlink/XLinkConnection.hppsrc/device/CrashDump.cppsrc/device/CrashDumpManager.cppsrc/device/CrashDumpManager.hppsrc/device/Device.cppsrc/device/DeviceBase.cppsrc/device/DeviceGate.cppsrc/device/Platform.cppsrc/opencv/HolisticRecordReplay.cppsrc/utility/Compression.cppsrc/utility/LogCollection.cppsrc/utility/LogCollection.hppsrc/utility/PipelineImplHelper.cppsrc/utility/Platform.cppsrc/utility/Platform.hpptests/CMakeLists.txttests/src/ondevice_tests/crashdump_test.cpptests/src/ondevice_tests/dynamic_calibration_test.cpptests/src/ondevice_tests/pipeline/node/record_replay_test.cpptests/src/onhost_tests/replay_test.cpp
💤 Files with no reviewable changes (2)
- examples/cpp/Misc/CrashDump/CMakeLists.txt
- src/device/Device.cpp
Comment on lines
+204
to
+206
| std::string getCrashDumpVersion() const override { | ||
| return "1.0.0"; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting version string to a constant.
The version string "1.0.0" is duplicated in both CrashDumpRVC2 and CrashDumpRVC4. If both platforms share the same crash dump format version, consider defining a constant in the base class.
Optional refactor
Add to base class:
class CrashDump {
protected:
static constexpr const char* CRASH_DUMP_FORMAT_VERSION = "1.0.0";
// ...
};Then in derived classes:
std::string getCrashDumpVersion() const override {
return CRASH_DUMP_FORMAT_VERSION;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/depthai/device/CrashDump.hpp` around lines 204 - 206, Extract the
repeated literal "1.0.0" into a single constant on the base class CrashDump
(e.g., a protected static constexpr const char* CRASH_DUMP_FORMAT_VERSION) and
then update the derived classes CrashDumpRVC2 and CrashDumpRVC4 to return that
constant from getCrashDumpVersion() instead of the hard-coded string.
Comment on lines
+970
to
+977
| // Crash dump callback | ||
| std::mutex crashdumpCallbackMtx; | ||
| std::function<void(std::shared_ptr<CrashDump>)> crashdumpCallback; | ||
|
|
||
| // Watchdog thread | ||
| std::thread watchdogThread; | ||
| std::atomic<bool> watchdogRunning{true}; | ||
| std::atomic<bool> crashed{false}; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor naming inconsistency.
The naming convention is inconsistent between crashdumpCallback/crashdumpCallbackMtx (lowercase 'd') and crashDumpHandled (uppercase 'D'). Consider unifying the naming for consistency.
Suggested fix for consistency
// Crash dump callback
- std::mutex crashdumpCallbackMtx;
- std::function<void(std::shared_ptr<CrashDump>)> crashdumpCallback;
+ std::mutex crashDumpCallbackMtx;
+ std::function<void(std::shared_ptr<CrashDump>)> crashDumpCallback;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Crash dump callback | |
| std::mutex crashdumpCallbackMtx; | |
| std::function<void(std::shared_ptr<CrashDump>)> crashdumpCallback; | |
| // Watchdog thread | |
| std::thread watchdogThread; | |
| std::atomic<bool> watchdogRunning{true}; | |
| std::atomic<bool> crashed{false}; | |
| // Crash dump callback | |
| std::mutex crashDumpCallbackMtx; | |
| std::function<void(std::shared_ptr<CrashDump>)> crashDumpCallback; | |
| // Watchdog thread | |
| std::thread watchdogThread; | |
| std::atomic<bool> watchdogRunning{true}; | |
| std::atomic<bool> crashed{false}; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/depthai/device/DeviceBase.hpp` around lines 970 - 977, The code uses
inconsistent camelCase for crash dump identifiers; rename crashdumpCallback and
crashdumpCallbackMtx to match crashDumpHandled (e.g., crashDumpCallback and
crashDumpCallbackMtx) and update all references/usages accordingly (including
any captures, assignments, mutex locks, and function parameter references) so
the identifier casing is consistent across DeviceBase.hpp and any compilation
units that reference these symbols.
Comment on lines
+45
to
+46
| // Waits for the gate session to end | ||
| void waitForSessionEnd(); |
There was a problem hiding this comment.
Preserve a compatibility path for waitForSessionEnd().
This public signature change is source-breaking for existing consumers. If the new flow is waitForSessionEnd(); getCrashDump();, please keep the old behavior behind a deprecated shim or document the migration in the public API before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/depthai/device/DeviceGate.hpp` around lines 45 - 46, The public API
change to waitForSessionEnd() is breaking; add a deprecated compatibility shim
in the DeviceGate class that preserves the original waitForSessionEnd() behavior
and forwards to the new flow (e.g., call the new wait method(s) and then
getCrashDump() as the old sequence did), mark the shim as [[deprecated]] and
document the migration to the new explicit getCrashDump() call so consumers have
time to adopt the new API; reference the existing symbols waitForSessionEnd()
and getCrashDump() when implementing the wrapper so it simply invokes the new
sequence and returns the original expected result/side-effects.
Comment on lines
+293
to
+297
| void CrashDumpRVC4::toTar(const fs::path& tarPath) const { | ||
| // Create metadata | ||
| nlohmann::json metadata; | ||
| writeMetadata(metadata); | ||
| metadata["originalFilename"] = filename; |
There was a problem hiding this comment.
Reload originalFilename on RVC4 deserialization.
Line 297 writes the filename into metadata, but fromTar() never restores it. CrashDumpRVC4::filename comes back empty after any archive/bytes round-trip.
Proposed fix
std::string metadataContent;
readFromFile(metadataPath, metadataContent);
nlohmann::json metadata = nlohmann::json::parse(metadataContent);
readMetadata(metadata);
+ filename = fromJson(metadata, "originalFilename", "");Also applies to: 336-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/device/CrashDump.cpp` around lines 293 - 297, The metadata written by
CrashDumpRVC4::toTar includes originalFilename but the deserializers never
restore it; update CrashDumpRVC4::fromTar (and the other deserialization path
around the 336-339 area) to read metadata["originalFilename"] (or use
metadata.value("originalFilename", "")) and assign it to the
CrashDumpRVC4::filename member so filename is preserved across archive/bytes
round-trips.
Comment on lines
+59
to
+67
| dump->depthaiVersion = build::VERSION; | ||
| dump->depthaiVersionMajor = std::to_string(build::VERSION_MAJOR); | ||
| dump->depthaiVersionMinor = std::to_string(build::VERSION_MINOR); | ||
| dump->depthaiVersionPatch = std::to_string(build::VERSION_PATCH); | ||
| dump->depthaiVersionPreReleaseType = build::PRE_RELEASE_TYPE; | ||
| dump->depthaiVersionPreReleaseVersion = std::to_string(build::PRE_RELEASE_VERSION); | ||
| dump->depthaiVersionBuildInfo = build::BUILD_DATETIME; | ||
| dump->depthaiCommitHash = build::COMMIT; | ||
| dump->depthaiCommitDatetime = build::COMMIT_DATETIME; |
There was a problem hiding this comment.
Populate depthaiBuildDatetime before serializing the dump.
CrashDump::writeMetadata() persists a separate depthaiBuildDatetime field, but this collector never assigns it, so dumps produced here always leave that public field empty.
Proposed fix
dump->depthaiVersionBuildInfo = build::BUILD_DATETIME;
+ dump->depthaiBuildDatetime = build::BUILD_DATETIME;
dump->depthaiCommitHash = build::COMMIT;
dump->depthaiCommitDatetime = build::COMMIT_DATETIME;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dump->depthaiVersion = build::VERSION; | |
| dump->depthaiVersionMajor = std::to_string(build::VERSION_MAJOR); | |
| dump->depthaiVersionMinor = std::to_string(build::VERSION_MINOR); | |
| dump->depthaiVersionPatch = std::to_string(build::VERSION_PATCH); | |
| dump->depthaiVersionPreReleaseType = build::PRE_RELEASE_TYPE; | |
| dump->depthaiVersionPreReleaseVersion = std::to_string(build::PRE_RELEASE_VERSION); | |
| dump->depthaiVersionBuildInfo = build::BUILD_DATETIME; | |
| dump->depthaiCommitHash = build::COMMIT; | |
| dump->depthaiCommitDatetime = build::COMMIT_DATETIME; | |
| dump->depthaiVersion = build::VERSION; | |
| dump->depthaiVersionMajor = std::to_string(build::VERSION_MAJOR); | |
| dump->depthaiVersionMinor = std::to_string(build::VERSION_MINOR); | |
| dump->depthaiVersionPatch = std::to_string(build::VERSION_PATCH); | |
| dump->depthaiVersionPreReleaseType = build::PRE_RELEASE_TYPE; | |
| dump->depthaiVersionPreReleaseVersion = std::to_string(build::PRE_RELEASE_VERSION); | |
| dump->depthaiVersionBuildInfo = build::BUILD_DATETIME; | |
| dump->depthaiBuildDatetime = build::BUILD_DATETIME; | |
| dump->depthaiCommitHash = build::COMMIT; | |
| dump->depthaiCommitDatetime = build::COMMIT_DATETIME; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/device/CrashDumpManager.cpp` around lines 59 - 67, The CrashDumpCollector
is never setting the CrashDump field depthaiBuildDatetime before serialization;
update the same initialization block that sets dump->depthaiVersionBuildInfo to
also assign dump->depthaiBuildDatetime (e.g., from build::BUILD_DATETIME or the
appropriate build timestamp constant) so that writeMetadata() persists a
populated depthaiBuildDatetime; locate the initialization using the dump pointer
and the depthaiVersion* assignments to add this single assignment.
| throw std::runtime_error("Could not open archive."); | ||
| } | ||
| assert(filesInTar.size() == filesOnDisk.size()); | ||
| assert(filesInArchive.size() == filesOnDisk.size()); |
There was a problem hiding this comment.
Same assert issue - replace with runtime check.
Suggested fix
- assert(filesInArchive.size() == filesOnDisk.size());
+ if(filesInArchive.size() != filesOnDisk.size()) {
+ throw std::invalid_argument("filesInArchive and filesOnDisk must have the same size");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert(filesInArchive.size() == filesOnDisk.size()); | |
| if(filesInArchive.size() != filesOnDisk.size()) { | |
| throw std::invalid_argument("filesInArchive and filesOnDisk must have the same size"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utility/Compression.cpp` at line 121, Replace the debug-only assert with
a proper runtime check: in the scope where filesInArchive and filesOnDisk are
compared (the code currently contains assert(filesInArchive.size() ==
filesOnDisk.size());), validate sizes at runtime and handle mismatch
deterministically (throw a suitable exception or return an error code/value and
log a clear message) instead of using assert; ensure the check references
filesInArchive.size() and filesOnDisk.size() and that subsequent code path
assumes sizes are equal only after the runtime validation succeeds.
Comment on lines
+172
to
+177
| auto basePath = std::filesystem::temp_directory_path() / L"depthai_XXXXXX"; | ||
| auto baseStr = basePath.wstring(); | ||
| if(_wmktemp_s(baseStr.data(), baseStr.size() + 1) != 0 || !std::filesystem::create_directories(baseStr)) { | ||
| throw std::runtime_error("Failed to create a unique temporary directory"); | ||
| } | ||
| return std::filesystem::path(baseStr); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Microsoft's _wmktemp_s create a file or directory atomically, or does it only generate a unique pathname? What Windows API pattern is recommended for securely creating a unique temporary directory?
💡 Result:
_wmktemp_s (like _mktemp_s) only generates a unique filename string by modifying the template buffer; it does not create a file or directory, so it is not an atomic create and is inherently subject to a TOCTOU race if you later create something at that path. [1]
Recommended secure Windows API pattern for a unique temp directory: get the temp parent directory (for example via GetTempPath2), generate a hard-to-guess name (GUID/random), then attempt to create it with CreateDirectory; if it fails with ERROR_ALREADY_EXISTS, retry with a new name. CreateDirectory itself is the “atomic existence check + create” step (success or ERROR_ALREADY_EXISTS). [2][3]
Sources:
[1] Microsoft Learn: _mktemp_s, _wmktemp_s [1] (learn.microsoft.com)
[2] Microsoft Learn: CreateDirectory (notes ERROR_ALREADY_EXISTS) [2] (learn.microsoft.com)
[3] Microsoft Learn: “Creating and Using a Temporary File” (uses GetTempPath2 as the temp-location API) [3] (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp-s-wmktemp-s?view=msvc-170
- 2: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createdirectory
- 3: https://learn.microsoft.com/bs-latn-ba/windows/win32/fileio/creating-and-using-a-temporary-file
Create the Windows temp directory atomically.
_wmktemp_s() only generates a unique pathname; it does not create a file or directory. The directory is then created separately by create_directories(), leaving a TOCTOU race window where concurrent processes can collide and fail intermittently. The POSIX mkdtemp() path avoids this with an atomic operation. Use a retry loop calling CreateDirectory with a random or GUID-suffixed path instead, which atomically verifies non-existence and creates the directory in one operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utility/Platform.cpp` around lines 172 - 177, The current implementation
builds basePath/baseStr and uses _wmktemp_s plus
std::filesystem::create_directories, which has a TOCTOU race; replace this with
a retry loop that atomically creates a uniquely-suffixed directory using the
Windows API CreateDirectory (or CreateDirectoryW) rather than generating a name
then creating it, e.g. generate a random/GUID suffix appended to basePath (where
basePath and baseStr are used) and call CreateDirectoryW in a loop until it
succeeds or a retry limit is reached; on success return the created
std::filesystem::path, and on repeated failure throw the same std::runtime_error
with an explanatory message.
Comment on lines
+45
to
+58
| TEST_CASE("hasCrashed returns true after device crash") { | ||
| dai::utility::setEnv("DEPTHAI_CRASHDUMP", "0"); // don't save nor upload crash dump | ||
| dai::utility::setEnv("DEPTHAI_CRASH_DEVICE", "1"); | ||
|
|
||
| dai::Device device; | ||
| REQUIRE_FALSE(device.hasCrashed()); | ||
|
|
||
| device.crashDevice(); | ||
|
|
||
| // Give the device time to crash and for the watchdog to detect it | ||
| std::this_thread::sleep_for(std::chrono::seconds(12)); | ||
|
|
||
| REQUIRE(device.hasCrashed()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a polling loop with shorter sleep intervals instead of a fixed 12-second sleep.
The fixed 12-second sleep may cause unnecessarily long test runs when the crash is detected quickly, or flaky failures if the watchdog timing varies. A polling approach (similar to the Python example) would be more robust.
♻️ Proposed refactor to use polling
device.crashDevice();
- // Give the device time to crash and for the watchdog to detect it
- std::this_thread::sleep_for(std::chrono::seconds(12));
-
- REQUIRE(device.hasCrashed());
+ // Poll for crash detection with timeout
+ bool crashDetected = false;
+ auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(20);
+ while(std::chrono::steady_clock::now() < deadline) {
+ if(device.hasCrashed()) {
+ crashDetected = true;
+ break;
+ }
+ std::this_thread::sleep_for(std::chrono::milliseconds(200));
+ }
+ REQUIRE(crashDetected);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/ondevice_tests/crashdump_test.cpp` around lines 45 - 58, Replace
the fixed 12-second sleep in the TEST_CASE with a polling loop that repeatedly
checks device.hasCrashed() after short sleeps (e.g., 100–200ms) up to a maximum
timeout (e.g., 12s); call device.crashDevice() as before, then loop: if
device.hasCrashed() break and REQUIRE true, otherwise sleep a short interval and
retry until timeout and then REQUIRE(device.hasCrashed()) fails—update
references in the test to use dai::Device, device.crashDevice(),
device.hasCrashed(), std::this_thread::sleep_for and std::chrono for timing.
Comment on lines
+55
to
+61
| auto recordingFilenames = dai::utility::filenamesInArchive(RECORDING_PATH); | ||
| std::vector<std::filesystem::path> recordingExtFiles; | ||
| recordingExtFiles.reserve(recordingFilenames.size()); | ||
| for(const auto& filename : recordingFilenames) { | ||
| recordingExtFiles.push_back(std::filesystem::path(testFolder).append("extracted").append(filename)); | ||
| } | ||
| dai::utility::untarFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles); | ||
| dai::utility::extractFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the Compression implementation files
find . -type f \( -name "Compression.hpp" -o -name "Compression.cpp" \) | head -20Repository: luxonis/depthai-core
Length of output: 135
🏁 Script executed:
# Find the test file
find . -type f -name "record_replay_test.cpp"Repository: luxonis/depthai-core
Length of output: 127
🏁 Script executed:
# Search for filenamesInArchive and extractFiles implementations
rg "filenamesInArchive|extractFiles" --type cpp -B 2 -A 10 | head -100Repository: luxonis/depthai-core
Length of output: 7798
🏁 Script executed:
# Let's examine the implementations more fully
cat -n ./src/utility/Compression.cpp | head -150Repository: luxonis/depthai-core
Length of output: 6345
🏁 Script executed:
# Let's see the dynamic_calibration_test filtering logic
rg "if.*empty|Treat entries" tests/src/ondevice_tests/dynamic_calibration_test.cpp -B 5 -A 8Repository: luxonis/depthai-core
Length of output: 1711
🏁 Script executed:
# Let's examine the record_replay_test.cpp file to see the full context
cat -n ./tests/src/ondevice_tests/pipeline/node/record_replay_test.cpp | head -100Repository: luxonis/depthai-core
Length of output: 4030
🏁 Script executed:
# Check how files are accessed later in record_replay_test
rg "setReplay|extracted|recordingExtFiles" tests/src/ondevice_tests/pipeline/node/record_replay_test.cpp -A 2 -B 2Repository: luxonis/depthai-core
Length of output: 1483
🏁 Script executed:
# Check RECORDING_PATH definition to understand what archive is being tested
rg "RECORDING_PATH" tests/src/ondevice_tests/pipeline/node/record_replay_test.cppRepository: luxonis/depthai-core
Length of output: 238
🏁 Script executed:
# Compare with how dynamic_calibration_test.cpp is structured
head -80 tests/src/ondevice_tests/dynamic_calibration_test.cpp | tail -30Repository: luxonis/depthai-core
Length of output: 1301
Filter directory entries and create parent paths before extracting into extracted/.
filenamesInArchive() returns all archive entries including directories (as paths ending with /), and extractFiles() does not create parent directories—it opens the exact destination path for writing. Passing the raw list will fail if RECORDING_PATH contains any directory entries or nested file paths. Filter directory entries and call create_directories() on parent paths for files before extraction, as shown in dynamic_calibration_test.cpp.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/ondevice_tests/pipeline/node/record_replay_test.cpp` around lines
55 - 61, filenamesInArchive(RECORDING_PATH) can include directory entries (paths
ending with '/'), and extractFiles(...) will fail if parent directories don't
exist; update the loop that builds recordingExtFiles to skip entries that are
directories (e.g., filename.back() == '/') and for each file entry call
std::filesystem::create_directories(recordingPath.parent_path()) (use testFolder
+ "extracted" base) before calling dai::utility::extractFiles(RECORDING_PATH,
recordingFilenames, recordingExtFiles); ensure you only pass file entries in
recordingFilenames and matching created parent directories for recordingExtFiles
when calling extractFiles (see filenamesInArchive, recordingFilenames,
recordingExtFiles, extractFiles, RECORDING_PATH, testFolder).
Comment on lines
+29
to
+35
| auto recordingFilenames = dai::utility::filenamesInArchive(RECORDING_PATH); | ||
| std::vector<std::filesystem::path> recordingExtFiles; | ||
| recordingExtFiles.reserve(recordingFilenames.size()); | ||
| for(const auto& filename : recordingFilenames) { | ||
| recordingExtFiles.push_back(std::filesystem::path(testFolder).append("extracted").append(filename)); | ||
| } | ||
| dai::utility::untarFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles); | ||
| dai::utility::extractFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles); |
There was a problem hiding this comment.
Filter directory entries before calling extractFiles().
Line 29-35 now forwards the raw filenamesInArchive() output to extractFiles(), but src/utility/Compression.cpp opens every requested destination as a file and never creates parent directories. Any directory header or nested path in RECORDING_PATH will make this helper fail. tests/src/ondevice_tests/dynamic_calibration_test.cpp lines 58-84 already has the required filtering and create_directories() logic.
💡 Suggested hardening
- auto recordingFilenames = dai::utility::filenamesInArchive(RECORDING_PATH);
- std::vector<std::filesystem::path> recordingExtFiles;
- recordingExtFiles.reserve(recordingFilenames.size());
- for(const auto& filename : recordingFilenames) {
- recordingExtFiles.push_back(std::filesystem::path(testFolder).append("extracted").append(filename));
- }
- dai::utility::extractFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles);
+ auto recordingFilenames = dai::utility::filenamesInArchive(RECORDING_PATH);
+ std::vector<std::string> srcFiles;
+ std::vector<std::filesystem::path> recordingExtFiles;
+ srcFiles.reserve(recordingFilenames.size());
+ recordingExtFiles.reserve(recordingFilenames.size());
+ for(const auto& filename : recordingFilenames) {
+ if(filename.empty()) continue;
+
+ auto outPath = std::filesystem::path(testFolder).append("extracted").append(filename);
+ const bool isDirEntry = filename.back() == '/' || std::filesystem::path{filename}.filename().string().empty();
+ if(isDirEntry) {
+ std::filesystem::create_directories(outPath);
+ continue;
+ }
+
+ std::filesystem::create_directories(outPath.parent_path());
+ srcFiles.push_back(filename);
+ recordingExtFiles.push_back(outPath);
+ }
+ dai::utility::extractFiles(RECORDING_PATH, srcFiles, recordingExtFiles);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto recordingFilenames = dai::utility::filenamesInArchive(RECORDING_PATH); | |
| std::vector<std::filesystem::path> recordingExtFiles; | |
| recordingExtFiles.reserve(recordingFilenames.size()); | |
| for(const auto& filename : recordingFilenames) { | |
| recordingExtFiles.push_back(std::filesystem::path(testFolder).append("extracted").append(filename)); | |
| } | |
| dai::utility::untarFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles); | |
| dai::utility::extractFiles(RECORDING_PATH, recordingFilenames, recordingExtFiles); | |
| auto recordingFilenames = dai::utility::filenamesInArchive(RECORDING_PATH); | |
| std::vector<std::string> srcFiles; | |
| std::vector<std::filesystem::path> recordingExtFiles; | |
| srcFiles.reserve(recordingFilenames.size()); | |
| recordingExtFiles.reserve(recordingFilenames.size()); | |
| for(const auto& filename : recordingFilenames) { | |
| if(filename.empty()) continue; | |
| auto outPath = std::filesystem::path(testFolder).append("extracted").append(filename); | |
| const bool isDirEntry = filename.back() == '/' || std::filesystem::path{filename}.filename().string().empty(); | |
| if(isDirEntry) { | |
| std::filesystem::create_directories(outPath); | |
| continue; | |
| } | |
| std::filesystem::create_directories(outPath.parent_path()); | |
| srcFiles.push_back(filename); | |
| recordingExtFiles.push_back(outPath); | |
| } | |
| dai::utility::extractFiles(RECORDING_PATH, srcFiles, recordingExtFiles); |
🧰 Tools
🪛 Cppcheck (2.20.0)
[information] 31-31: Include file
(missingIncludeSystem)
[information] 32-32: Include file
(missingIncludeSystem)
[information] 33-33: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/onhost_tests/replay_test.cpp` around lines 29 - 35, The code passes
raw recordingFilenames from dai::utility::filenamesInArchive(RECORDING_PATH)
into extractFiles which fails if the archive contains directory entries or
nested paths because extractFiles expects destination parent directories to
exist; update the loop that builds recordingExtFiles to skip directory entries
(e.g., filter out entries that are directory headers or that end with a slash)
and ensure you call std::filesystem::create_directories(...) for each
destination's parent_path before calling dai::utility::extractFiles; reference
filenamesInArchive, recordingFilenames, recordingExtFiles, extractFiles,
RECORDING_PATH and testFolder when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Reopening device has crashed PR after fixing some issues.
Specification
This PR now fixes:
Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
Tested with both the RVC2 and RVC4 devices, with varying settings.
Summary by CodeRabbit
New Features
Documentation
Examples