👔 (dmk) [DSDK-930]: Add IntentQueue to handle intents sequentially #1111
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Intent Queue Service to manage sequential processing of device operations (APDUs, Commands, and Device Actions). The implementation replaces the previous MutexService with a more robust queue-based approach that supports cancellation and timeout handling.
Key changes:
- New
IntentQueueServicewith FIFO queue processing and cancellation support - Refactored
DeviceSessionto use the Intent Queue for all device operations - Updated timeout handling logic for APDUs and Commands using RxJS operators
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| IntentQueueService.ts | New service implementing sequential intent queue with cancellation |
| IntentQueueService.test.ts | Comprehensive test coverage for the new queue service |
| DeviceSession.ts | Refactored to use IntentQueueService, updated timeout handling |
| DeviceSession.test.ts | New comprehensive test file for DeviceSession |
| MutexService.ts | Removed - replaced by IntentQueueService |
| MutexService.test.ts | Removed - replaced by IntentQueueService tests |
| SendApduUseCase.test.ts | Updated to use proper mocking instead of real services |
| ConnectUseCase.ts | Updated to inject and pass IntentQueueService |
| ConnectUseCase.test.ts | Improved test structure with proper mocking |
| DevicePinger.ts | Added debug console.log statement |
| deviceSessionModule.ts | Added IntentQueueService to DI container |
| Various test files | Updated to include IntentQueueService parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/device-management-kit/src/internal/device-session/model/DevicePinger.ts
Outdated
Show resolved
Hide resolved
aussedatlo
force-pushed
the
feat/dsdk-930-intent-queue
branch
2 times, most recently
from
7468665 to
ecce9a6
Compare
aussedatlo
force-pushed
the
feat/dsdk-930-intent-queue
branch
from
ecce9a6 to
d3067ab
Compare
aussedatlo
force-pushed
the
feat/dsdk-930-intent-queue
branch
from
d3067ab to
caf3860
Compare
aussedatlo
force-pushed
the
feat/dsdk-930-intent-queue
branch
from
caf3860 to
0eaaa07
Compare
aussedatlo
force-pushed
the
feat/dsdk-930-intent-queue
branch
from
0eaaa07 to
4c1498b
Compare
jiyuzhuang
added
WIP
| const { observable: o, cancel: c } = this._intentQueueService.enqueue({ | ||
| type: "device-action", | ||
| execute: () => { | ||
| const { observable } = deviceAction._execute({ |
There was a problem hiding this comment.
so the cancel function returned by deviceAction._execute() is no longer used ? it can probably be removed if the new cancellation logic works
There was a problem hiding this comment.
it is, in case we want to bypass the intent queue (t's on the next PR)
| sendApdu: async (apdu: Uint8Array) => this.sendApdu(apdu), | ||
| sendCommand: async <Response, ErrorStatusCodes, Args>( | ||
| command: Command<Response, ErrorStatusCodes, Args>, | ||
| abortTimeout?: number, |
There was a problem hiding this comment.
[SHOULD] Remove the param from InternalApi["sendApdu"] signature if it's not used, or it could lead to mistakes: if in the future a DeviceAction calls sendCommand with an abortTimeout, it will be ignored.
| const result = await this._connectedDevice.sendApdu( | ||
| rawApdu, | ||
| options.triggersDisconnection, | ||
| options.abortTimeout, |
There was a problem hiding this comment.
[SHOULD] This should still be passed, but it's done in the other PR
| try { | ||
| await sendPromise; | ||
| } catch { | ||
| // Expected to timeout |
There was a problem hiding this comment.
[SHOULD] test this: for instance before the try catch block do let caughtError: Error | undefined;, set it in the catch, and assert the value of it below
There was a problem hiding this comment.
because here if it does not throw, your test will still pass
| * Time ──────────────────────────────────────────────────> | ||
| * | ||
| * Intent 1: [████████████] | ||
| * Intent 2: [██████████] | ||
| * Intent 3: [████████] | ||
| * | ||
| * Queue: [1,2,3] → [2,3] → [3] → [] | ||
| * ``` | ||
| * | ||
| * Cancelling Currently Executing Intent: | ||
| * ``` | ||
| * Time ──────────────────────────────────────────────────> | ||
| * | ||
| * Intent 1: [████XX] | ||
| * Intent 2: [██████████] | ||
| * Intent 3: [████████] | ||
| * ↑ Intent 2 starts immediately | ||
| * Queue: [1,2,3] → [2,3] → [3] → [] | ||
| * Cancel 1 ↑ | ||
| * ``` | ||
| * | ||
| * Cancelling Queued Intent: | ||
| * ``` | ||
| * Time ──────────────────────────────────────────────────> | ||
| * | ||
| * Intent 1: [████████████] | ||
| * Intent 2: [CANCELLED] | ||
| * Intent 3: [████████] | ||
| * ↑ Intent 3 starts immediately | ||
| * Queue: [1,2,3] → [1,3] → [3] → [] | ||
| * Cancel 2 ↑ | ||
| * ``` | ||
| * |
There was a problem hiding this comment.
nice looking doc
ofreyssinet-ledger
left a comment
ofreyssinet-ledger
left a comment
There was a problem hiding this comment.
2 things to change:
- should test that the apdu timeout error is caught in DeviceSession test
- should remove abortTimeout from the signature of `InternalApi["sendCommand"]
📝 Description
DeviceSession no longer relies on MutexService to manage who gets to run operations. Instead, it uses a new system: an internal queue that processes tasks one at a time, in the order they arrive. This ensures that only one operation (like sending a command or running a device action) happens at once, without needing complicated locks.
How It Works:
Every operation (whether it’s sending a command, an APDU, or a device action) is wrapped in an “intent” and added to the queue.
The queue processes intents one after another, so there’s no overlap or conflict.
What You Get Back: (Same interface as before)
For commands and APDUs (using sendApdu or sendCommand), you get a Promise that resolves when the operation is done.
For device actions (using executeDeviceAction), you get an Observable that starts sending updates as soon as the action begins.
Cancelling Operations:
For Promises (like sendApdu or sendCommand), you can set an abortTimeout. If the operation takes too long, the Promise will automatically fail. This keeps the queue from getting stuck.
For Observables (device actions), you get a cancel button (a callback) that lets you stop the action at any time, even if it’s still waiting in the queue or already running.
Important Note:
Always set a timeout for sendApdu or sendCommand. Without one, a slow or stuck operation could block the whole queue.
Transports (the part that actually sends and receives data) don’t handle timeouts anymore—DeviceSession takes care of that.
Timeline Examples
gantt title Normal Intents Processing dateFormat HH:mm:ss.SSS axisFormat %L section Intent 1 Executing :active, a1, 00:00:00.000, 100ms section Intent 2 Executing :active, a2b, 00:00:00.120, 30ms section Intent 3 Executing :active, a3b, 00:00:00.160, 20msgantt title Concurrent Intents Processing dateFormat HH:mm:ss.SSS axisFormat %L section Intent 1 Executing :active, a1, 00:00:00.000, 100ms section Intent 2 Queued :a2, 00:00:00.000, 100ms Executing :active, a2b, 00:00:00.100, 50ms section Intent 3 Queued :a3, 00:00:00.000, 150ms Executing :active, a3b, 00:00:00.150, 30msgantt title Cancelling Currently Executing Intent dateFormat HH:mm:ss.SSS axisFormat %L section Intent 1 Executing :active, a1, 00:00:00.000, 50ms Cancelled :crit, c1, 00:00:00.050, 10ms section Intent 2 Queued :a2, 00:00:00.000, 60ms Executing :active, a2b, 00:00:00.060, 60ms section Intent 3 Queued :a3, 00:00:00.000, 120ms Executing :active, a3b, 00:00:00.120, 40msgantt title Cancelling Queued Intent dateFormat HH:mm:ss.SSS axisFormat %L section Intent 1 Executing :active, a1, 00:00:00.000, 100ms section Intent 2 Queued :a2, 00:00:00.000, 50ms Cancelled :crit, c2, 00:00:00.050, 50ms section Intent 3 Queued :a3, 00:00:00.000, 100ms Executing :active, a3b, 00:00:00.100, 50ms❓ Context
✅ Checklist
Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.
🧐 Checklist for the PR Reviewers