MS-1135 Combine per-modality duplication code

[luhmirin-s]

JIRA ticket
Will be released in: 2026.1.0

Notable changes

• Combine most of the instances of parallel "face/fingerprint" parameters.
• Implement a way to use the SDK configuration in a more generic way.
• Simplify the local record repository interface to have a single method.

Testing guidance

• Run all kinds of regular flows. Everything should works exactly as before.

Additional work checklist

• Effect on other features and security has been considered
• Design document marked as "In development" (if applicable)
• External (Gitbook) and internal (Confluence) Documentation is up to date (or ticket created)
• Test cases in Testiny are up to date (or ticket created)
• Other teams notified about the changes (if applicable)

[BurningAXE]

Nice! Much simpler this way!

[BurningAXE]

Would it be better if we had a dedicated model ModalitySamples with explicit properties instead of a Map?

[luhmirin-s]

We would lose the ability to look up by modality key, which is used in several places.
I cannot think of a benefit other than a shorter declaration.

[BurningAXE]

Hm, changes in this file seem suspicious - were the added ones missed previously? And the deleted ones unneeded?

[luhmirin-s]

This part was done in parallel with similar fixes in the release branch and later main, so it is messy. IIRC, all remaining records are required to pass the cache integration tests.

[BurningAXE]

of = above?

[luhmirin-s]

AppMatchConfidence is the enum of confidence bands, there is no band above HIGH.

[BurningAXE]

Here but also in lots of other places - it was more work to maintain hardcoded modalities than to have it abstracted!

[BurningAXE]

Why move the filter last? Not that it really matters in practice but it's less efficient this way.

[luhmirin-s]

Tbh, don't remember. Most likely just to do it once on the combined domain samples list.

[alexandr-simprints]

In this implementation, the semaphore is acquired first, and then then potentially blocks on channel.send().

If we have more ranges than available processor slots (i.e., 10 ranges and 4 permits), then all 10 async tasks are launched immediately. The first 4 acquire semaphore permits, and if the channel buffer fills, they block on send() while holding the permission. It seems to me that this defeats the purpose of the semaphore for concurrency control

Maybe we should change it to this:

async {
    val result = semaphore.withPermit { 
        // Only hold semaphore during actual work
        load(range)
    }
    // Release semaphore
    channel.send(result) 
}

[BurningAXE]

I don't really see the issue - you'd have 10 coroutines launched "simultaneously". 4 of them would acquire semaphores, load and then send (the Channel also has capacity of 4), then those 4 semaphores would be released and taken by next 4 coroutines. Am I missing anything?

[luhmirin-s]

Please see the comment above.

[alexandr-simprints]

From memory, channel.send() can suspend when the buffer is full. If it suspends, it is still holding the semaphore permit (since it's inside withPermit).

Looking at the 10 ranges and 4 semaphore permits scenario, with channel capacity of 4:

1. First 4 coroutines acquire permits, load, then hit channel.send() - buffer fills
2. Those 4 are now blocked on send() while holding their permits
3. Next 4 coroutines get permits, load, and also blocks on send()
4. Now all permits are held by coroutines waiting on channel I/O, not doing actual work

That's why it seems that it's better for semaphore to surround only the expensive load() operation, and not the channel 'communication':

async {
    val result = semaphore.withPermit { load(range) }
    channel.send(result)  // Can block, but permit already released
}

With this modification, the permits are released immediately after loading, and it allows the next batch to start work (rather than being stuck behind channel operations).

HOWEVER, according to @luhmirin-s:

While this shows up as "added" code, it is exactly the same as the pre-existing loadIdentities methods. The only change from my side is the renaming and duplication removal.

So it's up to you to change the code or keep it this way.

[sonarqubecloud]

Quality Gate passed

Issues
8 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud