Feature im sm by mlohvynenko · Pull Request #680 · aosedge/aos_core_lib_cpp

mlohvynenko · 2026-11-03T14:38:08Z

No description provided.

Copilot

🟡 Changes recommended

Cancellation races can start uncanceled downloads during shutdown, missing integrity validation leaves damaged items marked installed, and symlink-following size accounting can incorrectly reject valid installations.

4 open issues

What changed in this PR

This PR consolidates Communication Manager (CM) and Service Manager (SM) image handling around the common image manager, updating launchers, storage contracts, and status reporting to use shared interfaces.

Changes:

Reworks image management around shared storage, cancellable downloads, and SM-specific unpacked-layer handling.
Adds parallel CM item installation and carries image index digests through launch requests and stored instances.
Updates tests, mocks, build dependencies, and documentation; adds filesystem modification-time helpers.

File	Description
src/core/sm/tests/mocks/tests/mocks.cpp	Removes obsolete mock include.
src/core/sm/tests/mocks/iteminfoprovidermock.hpp	Deletes SM-specific provider mock.
src/core/sm/tests/mocks/CMakeLists.txt	Removes deleted mock header.
src/core/sm/smclient/smclient.md	Documents shared blob provider.
src/core/sm/smclient/itf/smclient.hpp	Adopts shared blob provider interface.
src/core/sm/launcher/tests/mocks/imagemanagermock.hpp	Adds cancellation mock.
src/core/sm/launcher/tests/launcher.cpp	Updates providers and tests installation selection and cancellation.
src/core/sm/launcher/launcher.md	Documents image-management dependencies.
src/core/sm/launcher/launcher.hpp	Injects shared item providers.
src/core/sm/launcher/launcher.cpp	Uses shared statuses and requests shutdown cancellation.
src/core/sm/imagemanager/tests/stubs/storagestub.hpp	Delegates shared storage and checks layer references.
src/core/sm/imagemanager/tests/mocks/iteminfoprovidermock.hpp	Deletes redundant provider mock.
src/core/sm/imagemanager/tests/mocks/blobinfoprovidermock.hpp	Deletes redundant blob mock.
src/core/sm/imagemanager/itf/storage.hpp	Extends shared storage with layer-reference checks.
src/core/sm/imagemanager/itf/iteminfoprovider.hpp	Deletes superseded provider interface.
src/core/sm/imagemanager/itf/imagemanager.hpp	Replaces status retrieval with cancellation contract.
src/core/sm/imagemanager/itf/blobinfoprovider.hpp	Deletes superseded blob interface.
src/core/sm/imagemanager/imagemanager.md	Documents shared architecture and layer lifecycle.
src/core/sm/imagemanager/imagemanager.hpp	Derives SM manager from common implementation.
src/core/sm/imagemanager/imagemanager.cpp	Implements downloads, layer validation, unpacking, and cleanup.
src/core/sm/imagemanager/config.hpp	Deletes superseded SM configuration.
src/core/sm/imagemanager/CMakeLists.txt	Links common image manager and updates headers.
src/core/common/tools/tests/fs.cpp	Tests modification-time helpers.
src/core/common/tools/fs.hpp	Declares modification-time helpers.
src/core/common/tools/fs.cpp	Implements modification-time access.
src/core/common/tests/mocks/itemstatusprovidermock.hpp	Adds shared status-provider mock.
src/core/common/tests/mocks/downloadermock.hpp	Updates cancellable, multi-URL download signature.
src/core/common/tests/mocks/CMakeLists.txt	Registers status-provider mock.
src/core/common/imagemanager/imagemanager.hpp	Exposes blob validation to derived managers.
src/core/common/downloader/itf/downloader.hpp	Defines cancellable, multi-URL downloads.
src/core/cm/updatemanager/updatemanager.hpp	Adds status-provider dependency.
src/core/cm/updatemanager/updatemanager.cpp	Wires shared status provider into handlers.
src/core/cm/updatemanager/unitstatushandler.hpp	Adopts shared status-listener contract.
src/core/cm/updatemanager/unitstatushandler.cpp	Handles individual item-status notifications.
src/core/cm/updatemanager/tests/updatemanager.cpp	Tests parallel installation and finalization failures.
src/core/cm/updatemanager/tests/mocks/imagemanagermock.hpp	Mocks per-item image-management operations.
src/core/cm/updatemanager/desiredstatushandler.hpp	Adds installation pool and shared status provider.
src/core/cm/updatemanager/desiredstatushandler.cpp	Installs items concurrently and removes stale versions.
src/core/cm/smcontroller/smcontroller.md	Documents shared item provider.
src/core/cm/launcher/tests/stubs/imagestorestub.hpp	Uses shared provider and explicit index digests.
src/core/cm/launcher/tests/launcher.cpp	Tests missing, unknown, and updated index digests.
src/core/cm/launcher/runrequestsloader.cpp	Resolves indexes by request digest.
src/core/cm/launcher/nodemanager.hpp	Changes descriptor lookup to accept index digest.
src/core/cm/launcher/nodemanager.cpp	Uses stored index digests for descriptors.
src/core/cm/launcher/launcher.md	Documents digest propagation and persistence.
src/core/cm/launcher/launcher.hpp	Adopts shared item-provider dependency.
src/core/cm/launcher/launcher.cpp	Updates provider type and logs index digests.
src/core/cm/launcher/itf/types.hpp	Adds index digests to requests and instance records.
src/core/cm/launcher/instancemanager.hpp	Declares reused-instance digest update.
src/core/cm/launcher/instancemanager.cpp	Stores and refreshes instance index digests.
src/core/cm/launcher/instance.hpp	Adds index-digest setter.
src/core/cm/launcher/instance.cpp	Validates images using stored index digest.
src/core/cm/launcher/imageinfoprovider.hpp	Defines digest-based index lookup.
src/core/cm/launcher/imageinfoprovider.cpp	Loads indexes through shared blob paths.
src/core/cm/launcher/balancer.cpp	Uses index digests during balancing.
src/core/cm/imagemanager/tests/mocks/storagemock.hpp	Deletes legacy storage mock.
src/core/cm/imagemanager/tests/mocks/itemstatuslistenermock.hpp	Deletes legacy status-listener mock.
src/core/cm/imagemanager/tests/mocks/blobinfoprovidermock.hpp	Deletes legacy blob-provider mock.
src/core/cm/imagemanager/tests/CMakeLists.txt	Adds shared test dependencies.
src/core/cm/imagemanager/itf/storage.hpp	Deletes superseded CM storage interface.
src/core/cm/imagemanager/itf/itemstatusprovider.hpp	Deletes superseded status interface.
src/core/cm/imagemanager/itf/iteminfoprovider.hpp	Deletes superseded item-provider interface.
src/core/cm/imagemanager/itf/imagemanager.hpp	Defines certificate setup and per-item operations.
src/core/cm/imagemanager/itf/blobinfoprovider.hpp	Deletes superseded blob interface.
src/core/cm/imagemanager/imagemanager.md	Documents shared storage and temporary-download lifecycle.
src/core/cm/imagemanager/imagemanager.hpp	Defines common-derived CM manager and download helpers.
src/core/cm/imagemanager/config.hpp	Extends shared configuration with download path.
src/core/cm/imagemanager/CMakeLists.txt	Adds common image-manager dependency.
src/core/cm/database/itf/database.hpp	Adopts shared image-storage contract.
src/core/cm/database/database.md	Documents shared storage interface.
src/core/cm/communication/itf/communication.hpp	Adopts shared blob-provider contract.
src/core/cm/communication/communication.md	Documents shared blob-provider interface.

_{Review effort: Balanced}

+    LOG_DBG() << "Install update item" << Log::Field("id", item.mItemID) << Log::Field("type", item.mType)
+              << Log::Field("version", item.mVersion);
+
+    if (auto err = mImageManager->InstallItem(item); err.Is(ErrorEnum::eCanceled)) {


    }

+    // Don't wait for the items being installed by the launch in progress.
+    if (auto err = mImageManager->Cancel(); !err.IsNone() && stopErr.IsNone()) {


    }

-    if (auto err = mImageManager->GetAllInstalledItems(*currentItems); !err.IsNone()) {
+    if (auto err = mItemStatusProvider->GetItemsStatuses(*currentItems); !err.IsNone()) {


-            return AOS_ERROR_WRAP(err);
-        }
+    // The layer removal frees the size of the layer files, so the accepted space has to match it.
+    auto [size, err] = fs::CalculateSize(*mAllocator, layerPath);


al1img · 2026-11-05T16:55:28Z

Add public virtual common::imagemanager::StorageItf to SM database Itf. Similar to #682.

al1img

Reviewed against design/core/items/imagemanager.md and integrity_recovery.md. The SM image manager otherwise matches the design. The inline comments cover three launcher problems in the repair and install-failure paths, and one design decision that needs a code change.

al1img · 2026-11-05T17:38:22Z

+    LOG_WRN() << "Instance configs are broken, repair update item" << Log::Field("instance", instance)
+              << Log::Field(err);
+
+    if (err = InstallUpdateItem({instance.mItemID, instance.mType, instance.mVersion, instance.mManifestDigest});


SM startup can hang when a config blob is broken at boot.

Launcher::Start calls LoadInstancesData, which reaches this repair through LoadInstanceData and then waits for it in mLaunchPool.Shutdown(). The repair needs GetBlobInfo, and the common image manager retries that until it succeeds or is canceled. In aos_core_cpp, AosCore::Start starts the launcher before mSMClient (aoscore.cpp:202 and :251), and smclient returns eFailed while it has no stub. So Launcher::Start never returns and smclient is never started.

I checked this by reading the code, not by running it, and that aos_core_cpp checkout isn't migrated to the new image manager API yet.

Suggested fix: LoadInstanceData calls LoadInstanceConfigs directly, with no repair. The asynchronous first UpdateInstances then handles the broken content.

al1img · 2026-11-05T17:38:22Z

 {
    LOG_DBG() << "Get instance configs" << Log::Field("instance", instance);

+    auto err = LoadInstanceConfigs(instance, itemConfig, imageConfig);


Config repair isn't once per request, and it races with parallel readers.

integrity_recovery.md allows at most one repair per item per start request, but each PrepareInstance task repairs on its own:

With N instances of a broken item, InstallItem can run up to N times, and each run revalidates the whole item. If the CM copy is broken too, each run retries the download.

InstallItem removes the broken blob and downloads it to the same path. Another instance of the item reading during that window gets eNotFound or a partial file, so it fails with a non-integrity error and gets no retry.

RepairsItemWithBrokenConfigs uses one instance, so it doesn't cover this.

Suggested fix: collect the eIntegrity failures from the prepare stage, then repair each item once after the stage ends, as RepairInstances does for start failures. Please add a test with several instances of one item.

al1img · 2026-11-05T17:38:22Z

        if (!err.IsNone()) {
-            LOG_ERR() << "Failed to add instance data" << Log::Field("instance", instance)
-                      << Log::Field(AOS_ERROR_WRAP(err));
+            SetInstanceState(*instanceData, InstanceStateEnum::eFailed, err);


A failed install marks a running instance as failed without stopping it.

Scenario:

Instance A of item X is active, and instance B's repair failed, so X is in eFailed.

The next request lists both A and B, so X is installed again and fails.

A goes to eInactive and then eFailed here, while it is still running in the runtime.

The CM then gets a false status and, with eIntegrity, starts a recovery for A. The next request also prepares A again as if it had failed. An installError from InstallUpdateItems does the same to every listed instance.

Suggested fix: only apply the install error to instances that aren't already running. Repair trigger 2 in integrity_recovery.md also only covers instances already in eFailed.

al1img · 2026-11-05T17:38:22Z

    }

-    return ErrorEnum::eNone;
+    return InstallUnpackedLayer(path, descriptor.mMediaType, layerPath);


The SM should remove the layer blob once the layer is unpacked.

This was a contradiction in the design: the ValidateLayer text says the blob is removed, while UnpackLayer requirement 2 and the SM section say it is kept. The decision is to remove it, and the design update is coming separately.

Changes needed here:

Remove the layer blob file and free its size with FreeSpace after a successful unpack, and also in the branch above where the layer is already valid. The blob reference stays in storage.

If the file can't be removed, log the error and still succeed, because the layer is fine. The blob stays with its space accepted until it is processed again or the item is removed.

On a failed unpack, keep the blob.

sm/imagemanager/imagemanager.md still says the SM "keeps both the packed blobs and the unpacked layers". Please update it.

Add tests for the blob removal, the space accounting, and a repair that downloads the blob again.

Layers without a diff_id stay packed, as they do now. The common RemoveBlob already returns 0 for a missing file, so it needs no change.

The boot and rootfs runtimes read mLayers[0] through GetBlobPath, so their items must keep using an empty or absent image config.

al1img · 2026-11-05T17:47:52Z

+    (void)descriptor;
+
+    // A layer without diff ID has no image config, so it is kept as packed blob only.
+    if (diffID.IsEmpty()) {


I guess we need to check media types here. We support only:

cMediaTypeLayerTar and cMediaTypeLayerTarGZip

al1img · 2026-11-05T18:01:06Z

+    }
+
+    if (size > 0) {
+        GetSpaceAllocator().FreeSpace(size);


Shell we free space even if remove error occurs?

Yes, as ImageManager::RemoveLayerFiles recalculates actually removed size and returns both size - remainingSize and error

al1img · 2026-11-06T14:19:49Z

        struct stat entryStat;

-        if (stat(path.CStr(), &entryStat) == -1) {
+        if (lstat(path.CStr(), &entryStat) == -1) {


Not sure if zephyr has lstat

updated to use defines and a helper function

al1img · 2026-11-06T14:34:36Z

+    }
+
+    for (const auto& instance : instances) {
+        auto instanceData = FindInstanceData(instance);


FindInstanceData is called here without mMutex, while the first loop of RepairInstances calls it under the lock. Probably safe as only one request runs at a time, but it's inconsistent: could the lookup be moved inside the LockGuard block below?

al1img · 2026-11-06T14:34:37Z

+{
+    UniqueLock lock {mLayersMutex};
+
+    if (auto err = mLayersCondVar.Wait(


LockLayer waits on the layers condition variable, not on the cancel context. The design (Cancel, requirement 2) requires InstallItem waits to be interrupted by Cancel. As is, Cancel (and the launcher stop) can be held until another item finishes a long unpack of the same diff_id. Could this wait be interrupted by the cancel context?

al1img · 2026-11-06T14:41:35Z

    mutable Mutex                                                         mMutex;
    Mutex                                                                 mSubscribersMutex;
    mutable ConditionalVariable                                           mCondVar;
+    Mutex                                                                 mInstallMutex;


Why do we need separate mutex and separate cond var for install? I guess existing one can be reused.

to not trigger Launcher::RunRebootThread() wake up
But it`s ok to use 1 mutex and cond var.
Updated

al1img · 2026-11-06T15:28:10Z

-    if (!err.IsNone()) {
-        return AOS_ERROR_WRAP(err);
+    if (mNumCancels > 0) {
+        return AOS_ERROR_WRAP(Error(ErrorEnum::eCanceled, "layer lock canceled"));


When ValidateLayer gets eCanceled here, ProcessLayer of the common image manager (common/imagemanager/imagemanager.cpp:744) treats it as an invalid layer: it logs Layer validation failed as an error, calls RemoveLayer (a no-op, as the layer is in progress) and then fails with eCanceled on the next cancel check. The result is correct, but each canceled waiter logs a misleading error. Could ProcessLayer return eCanceled as is, next to the eNotSupported check?

al1img · 2026-11-06T15:28:10Z

-    }
+    // The items waiting for a layer processed by another item are canceled as well, so Cancel doesn't wait for the
+    // layer unpack.
+    SetLayersCanceled(true);


Stop of the common image manager calls the common Cancel(), not this override, so mNumCancels isn't raised on Stop and an item waiting in LockLayer still waits for the unpack of another item. In practice the launcher is stopped first and cancels through this method, but should the SM image manager override Stop as well, so it doesn't depend on the stop order?

al1img

Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

The SM uses the blob info and item info providers of the common image manager instead. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Implement the SM image manager on top of the common image manager. It downloads blobs from the CM and keeps the unpacked layers. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Use the common item info, item status and blob info providers in the launcher and smclient, and cancel installed items on launcher stop. Make ValidateBlob of the common image manager protected for the SM image manager. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

CalculateSize and DirIterator used stat, so symlinks to directories were traversed, the targets of file symlinks were counted and dangling symlinks stopped the directory walk. Use lstat to count the links themselves and return errno on failure. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

DatabaseItf lists all storage interfaces the SM database implements. common::database::Database of aos_core_cpp implements the common image manager storage, which creates a diamond in sm::Database. Inherit StorageItf virtually to keep a single instance of it: StorageItf is a pure interface without data. This deviates from MISRA C++:2023 advisory rule 13.1.1. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

UpdateInstancesImpl returned on a failed allocation of the remove or install items after the launch thread pool was started, so the pool was left running and the next request started it again. Allocate all buffers before starting the pool. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

CM image manager registers download files as outdated items using the blob digest as ID. A sha256 digest is 71 chars, longer than cIDLen, so assigning it to OutdatedItem::mID triggered the String assignment assert and aborted CM on start when a leftover download file was present. Size the outdated item ID to hold either an item ID or a digest and return an error instead of asserting when the ID or version doesn't fit. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

A component manifest may reference a regular image config without rootfs instead of the empty config. Installing such an item failed with "diff ID's size mismatch" as the check required a diff ID for every layer. Treat a config without diff IDs as the empty config: its layers are kept as packed blobs without diff IDs. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

sonarqubecloud · 2026-11-06T16:55:50Z

Quality Gate passed

Issues
35 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot AI balanced review requested due to automatic review settings November 3, 2026 14:38

Copilot started reviewing on behalf of mlohvynenko November 3, 2026 14:38

View session

mlohvynenko force-pushed the feature_im_sm branch from 91faa0b to 2ce80fc Compare November 3, 2026 14:46

Copilot AI reviewed Nov 3, 2026

View reviewed changes

mlohvynenko force-pushed the feature_im_sm branch 3 times, most recently from 32689ba to 18bf0c2 Compare November 4, 2026 13:18

al1img reviewed Nov 5, 2026

View reviewed changes

mlohvynenko force-pushed the feature_im_sm branch 3 times, most recently from 9158b73 to 12e7f77 Compare November 6, 2026 12:57

al1img force-pushed the feature_imagemanager branch from 41cbed0 to 7a05670 Compare November 6, 2026 13:34

mlohvynenko force-pushed the feature_im_sm branch from 12e7f77 to 4854412 Compare November 6, 2026 14:16

al1img reviewed Nov 6, 2026

View reviewed changes

mlohvynenko force-pushed the feature_im_sm branch from 4854412 to ea6eaf0 Compare November 6, 2026 15:05

al1img reviewed Nov 6, 2026

View reviewed changes

al1img approved these changes Nov 6, 2026

View reviewed changes

mlohvynenko added 9 commits November 6, 2026 18:40

sm: remove obsolete image manager interfaces and mocks

b9ed687

The SM uses the blob info and item info providers of the common image manager instead. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

sm: imagemanager: remove old implementation

895a85b

Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

sm: launcher: restart instance on integrity check failure

a3daa71

Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com> Reviewed-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

mlohvynenko force-pushed the feature_im_sm branch from ea6eaf0 to c760904 Compare November 6, 2026 16:42

mlohvynenko merged commit 921f188 into aosedge:feature_imagemanager Nov 6, 2026
5 checks passed

mlohvynenko deleted the feature_im_sm branch November 6, 2026 17:04

Conversation

mlohvynenko commented Nov 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

al1img commented Nov 5, 2026

Uh oh!

al1img left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlohvynenko Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

al1img left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Nov 6, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mlohvynenko Nov 6, 2026 •

edited

Loading