Implemented Resources Monitor - #3407
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the “Used Resources” / “Resources Monitor” feature from #3405 by introducing a per-host registry of exclusive resources occupied (or reserved) by adapter instances. The controller keeps an in-memory registry mirrored into system.host.<hostname>.usedResources.<type>, adapters can register/free resources via new API methods, and the CLI performs cleanup when instances are removed while the controller is offline.
Changes:
- Added typed “used resources” model (
UsedResourceType,UsedResourceData,RegisteredResource) and a new supported feature flag (CONTROLLER_USED_RESOURCES). - Implemented controller-side registry bookkeeping + persistence/load/assessment, plus host message handling for register/free and auto-registration of
native.port. - Added adapter-side API surface (AdapterClass + AsyncAdapter + ResourceManager), unit tests, CLI cleanup, and documentation.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/types-dev/objects.d.ts | Adds common.usedResources flag to indicate adapter-managed registry. |
| packages/types-dev/index.d.ts | Introduces typed used-resources structures and CONTROLLER_USED_RESOURCES supported feature. |
| packages/common/src/lib/common/constants.ts | Adds CONTROLLER_USED_RESOURCES to supported features list. |
| packages/controller/src/lib/usedResources.ts | New in-memory registry with register/free/blocking/assessment logic. |
| packages/controller/src/main.ts | Integrates registry into controller lifecycle, persistence, host message handling, and auto-registration. |
| packages/controller/test/testUsedResources.ts | Unit tests for registry keying and mutation behavior. |
| packages/adapter/src/lib/adapter/managers/ResourceManager.ts | New adapter manager forwarding register/free to host and reading registry states. |
| packages/adapter/src/lib/adapter/managers/ResourceManager.test.ts | Tests for forwarding and parsing logic. |
| packages/adapter/src/lib/adapter/asyncAdapter.ts | Exposes used-resources API on AsyncAdapter. |
| packages/adapter/src/lib/adapter/adapter.ts | Exposes used-resources API on AdapterClass. |
| packages/cli/src/lib/setup/setupInstall.ts | Frees used-resources entries when instances are deleted via CLI while controller is down. |
| docs/used-resources.md | Documents the registry, lifecycle, storage layout, and adapter API. |
| getUsedResources<T extends ioBroker.UsedResourceType>(type: T): Promise<ioBroker.RegisteredResource<T>[]> { | ||
| if (type !== undefined) { | ||
| Validator.assertString(type, 'type'); | ||
| } | ||
| return this.#resources.getUsedResources(type); | ||
| } |
| async getUsedResources<T extends ioBroker.UsedResourceType>(type: T): Promise<ioBroker.RegisteredResource<T>[]> { | ||
| return this.#async.getUsedResources(type); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| `registerUsedResource(...)` / `freeUsedResource(...)` itself. Use this when the occupied resources are not | ||
| simply the configured `native.port` (serial ports, multiple ports, USB devices, …). | ||
| - **Controller-managed (automatic)** — if `common.usedResources` is _not_ set, js-controller tracks the | ||
| instance for the adapter: when the instance starts, its configured `native.port` (if any) is auto-registered |
There was a problem hiding this comment.
This sentence and the one before are not matching ... before it is stated that "native.port" can be multiple types of "port" but there it states that we assume it to be "tcpPort" ... or is this only happening when it is a number?
The base assumption that "port" is the only i do not really like.
|
|
||
| There are two ways an instance's resources end up in the registry: | ||
|
|
||
| - **Adapter-managed** — the adapter sets `common.usedResources: true` in its `io-package.json` and calls |
There was a problem hiding this comment.
cam we rename this to "declareUsedResources" or such ... "used resources" is more a controller feature ... but io-package wise we should define what that means for the adapter and the developer.
| @@ -0,0 +1,214 @@ | |||
| # Used Resources Registry | |||
|
|
|||
| A central registry of **exclusive resources** occupied by adapter instances — resources that can only be | |||
There was a problem hiding this comment.
I would include the word "static" here. because e.g. dynamic listening ports should not be marked because they are dynamic anyway
| You normally do not need to call this on shutdown — the host handles stop/crash automatically (see | ||
| [Lifecycle](#lifecycle)). Use it when an instance releases a resource while it keeps running. | ||
|
|
||
| ### `getUsedResources(type?)` |
There was a problem hiding this comment.
the name should reflect that it is "Host" resources here while the other two methods are "instance "only ... so either add Instance to the above methods or add "Host" or "All" here.
| | `type` | string | The resource type, e.g. `"serialPort"`. | | ||
| | `instance` | string | The instance that occupies the resource, e.g. `"mqtt.0"`. | | ||
| | `ts` | number | Timestamp (ms) when the resource was registered. | | ||
| | `isBlocked` | boolean | `true`: the instance is running and actively holding the resource. `false`: the instance is not running but would occupy it when started. | |
There was a problem hiding this comment.
... would maybe occupy it when started" (see your info on config changes)
Apollon77
left a comment
There was a problem hiding this comment.
Reviewed the code side (the docs already have your comments). Ran the branch locally: test-types-check is clean, testUsedResources.ts passes 19/19 and ResourceManager.test.ts passes 12/12 via npm run test-adapter.
The layering is good — a pure registry with an injectable clock, persistence and messaging kept out of it. Two problems are structural rather than local, so I would fix them before the rest:
1. The payload is spread into the resource object after the bookkeeping fields, so it can overwrite them. Reproduced against this branch:
reg.register('tcpPort', { port: 80, instance: 'evil.0', isBlocked: false, type: 'gpio' }, 'mqtt.0');
// stored in the tcpPort bucket as:
// [{ "type": "gpio", "instance": "evil.0", "ts": 1, "isBlocked": false, "port": 80 }]
reg.setInstanceBlocked('mqtt.0', false); // [] -> no match
reg.removeInstance('mqtt.0'); // [] -> entry survives foreverThe entry is attributed to the wrong instance, reports the wrong type, and is unreachable by every by-instance operation, so it never gets cleaned up. free() builds its key the same way, so it cannot remove it either. assess() on the next controller start is the only thing that drops it, and only if the fake instance does not exist.
The root cause is the flat shape of RegisteredResource — the issue proposed a nested data: ResourceData, which removes the whole collision class. If the flat shape has to stay, spreading ...data first and the bookkeeping fields after it at least makes them authoritative.
2. The registry is only filled when an instance process starts, which does not match what isBlocked: false is documented to mean ("the instance is not running, but could use this resource, when started"). A configured instance that has never been started, or is currently stopped after a controller restart, contributes nothing — so the "pick a free port" use case from #3405 still misses exactly the instances the user is most likely to collide with. A native.port change while the instance runs is also not picked up until the next restart. Seeding from the instance objects (in loadUsedResources, and on objectChange) would cover this.
The rest is in the inline comments. Every reproduction below was run against this branch.
| instance, | ||
| ts: this.now(), | ||
| isBlocked: true, | ||
| ...data, |
There was a problem hiding this comment.
...data comes last, so the payload wins over type, instance, ts and isBlocked. Reproduced:
reg.register('tcpPort', { port: 80, instance: 'evil.0', isBlocked: false, type: 'gpio' }, 'mqtt.0');
// [{ "type": "gpio", "instance": "evil.0", "ts": 1, "isBlocked": false, "port": 80 }]
reg.setInstanceBlocked('mqtt.0', false); // [] - no match
reg.removeInstance('mqtt.0'); // [] - entry survivesThe entry lands in the tcpPort bucket while claiming type: 'gpio', is attributed to an instance that never asked for it, and is invisible to every by-instance operation, so it leaks permanently. type is not hypothetical either — it is a natural payload key for a future resource kind.
The clean fix is nesting the payload (data: ioBroker.UsedResourceData) as #3405 proposed; see the comment on RegisteredResource. Keeping the flat shape, spreading ...data first and the four bookkeeping fields after it makes them authoritative and removes the as ioBroker.RegisteredResource cast at the same time.
| * A registered resource as stored on the host: the typed payload enriched with the discriminating | ||
| * `type` and the ownership/bookkeeping fields (`instance`, `ts`). | ||
| */ | ||
| type RegisteredResource<T extends UsedResourceType = UsedResourceType> = { |
There was a problem hiding this comment.
This flat merge of payload and bookkeeping fields is the type-level cause of the overwrite in register(). #3405 proposed keeping them separate:
interface UsedResource {
type: ResourceType;
instance: string;
data: ResourceData;
ts: number;
}With the payload nested, type/instance/ts/isBlocked can never be shadowed, getUsedResourceKey gets an unambiguous thing to hash, and the as ioBroker.RegisteredResource casts in register() and free() disappear. Flattening also means any future payload key colliding with a bookkeeping name is a silent behaviour change rather than a type error — TcpPortResourceData gaining a ts field would be enough.
Separately: UsedResourceType = keyof UsedResourceDataMap is a closed union, while #3405 had | string for adapter-specific kinds. The JSDoc says the map is "intentionally open for module augmentation", which works but requires an adapter to declare global. Is that the intended path for custom types?
| const { type, instance, ts: _ts, isBlocked: _isBlocked, ...data } = resource; | ||
| const sorted = Object.keys(data) | ||
| .sort() | ||
| .map(key => `${key}=${String((data as Record<string, unknown>)[key])}`) |
There was a problem hiding this comment.
String(value) makes the key lossy in three ways I could reproduce:
port: 80 -> a.0|tcpPort|port=80
port: '80' -> a.0|tcpPort|port=80 // collide
path: { a: 1 } -> a.0|usb|path=[object Object]
path: { b: 2 } -> a.0|usb|path=[object Object] // collide
{ port: 80, bind: undefined } -> a.0|tcpPort|bind=undefined,port=80
{ port: 80 } -> a.0|tcpPort|port=80 // differ
The first two make distinct resources indistinguishable. The third is the more likely one in practice: an explicitly-passed bind: undefined and an omitted bind produce different keys, so the same resource registers twice and shows up twice in the UI.
Dropping undefined values and serialising with JSON.stringify over sorted keys handles all three:
const sorted = Object.keys(data)
.filter(key => data[key] !== undefined)
.sort()
.map(key => `${key}=${JSON.stringify(data[key])}`)
.join(',');| isBlocked: false, | ||
| ...data, | ||
| } as ioBroker.RegisteredResource); | ||
| filtered = list.filter(entry => getUsedResourceKey(entry) !== key); |
There was a problem hiding this comment.
Because the key covers every payload key, freeing requires repeating the payload byte for byte, including optional fields. Reproduced:
reg.register('tcpPort', { port: 8080, bind: '0.0.0.0' }, 'web.0');
reg.free('tcpPort', { port: 8080 }, 'web.0'); // [] - nothing freed
reg.free('tcpPort', { port: 8080, bind: '0.0.0.0' }, 'web.0'); // ['tcpPort']This bites the auto-registration path in particular: autoRegisterUsedResources adds bind whenever native.bind is set, so an adapter that later calls freeUsedResource('tcpPort', { port }) silently frees nothing and the port stays listed as occupied. The failure is invisible — free() returns [], nothing is persisted, and the adapter's Promise<void> resolves as if it worked.
Either match on an identifying subset per resource type, or make a non-matching free observable (log it, or return the count so the message handler can report it back).
| const changed = new Set<ioBroker.UsedResourceType>(); | ||
|
|
||
| if (!doNotDeleteAlreadyUsed) { | ||
| for (const t of this.removeInstance(instance)) { |
There was a problem hiding this comment.
The default drop is removeInstance(instance), which spans all types, not just type. So registering two kinds of resource loses the first one unless the caller remembers the flag:
reg.register('serialPort', { port: '/dev/ttyUSB0' }, 'zwave.0');
reg.register('tcpPort', { port: 8080 }, 'zwave.0');
// [{ type: 'tcpPort', instance: 'zwave.0', ... }] - the serial port is goneThe JSDoc documents it, but the correctness of the result now depends on call order across an adapter's whole startup: the first registration must omit the flag and every later one must set it. Adapters that register from more than one async init path (a serial port when the device opens, a TCP port when the server listens) cannot reliably know which call is first, and getting it wrong fails silently.
A separate explicit operation would remove the ordering requirement entirely — clearInstanceResources(instance) called once at adapter start, with register() always additive. That also makes the flag unnecessary in the host message and in the three adapter-facing signatures.
| } | ||
| break; | ||
|
|
||
| case 'registerUsedResource': |
There was a problem hiding this comment.
msg.message.type, .data and .instance are passed through unvalidated.
instance is the one that matters: ResourceManager fills it from this.namespace, but the host message is reachable by anything that can sendToHost, so an instance can register resources in another instance's name, or — via freeUsedResource with only type and someone else's namespace — wipe another instance's entire registration for that type. msg.from is system.adapter.<namespace> and is already checked for the callback, so deriving the instance from it (and rejecting a mismatching msg.message.instance) costs nothing.
type is used unchecked as the last segment of system.host.<name>.usedResources.<type> and as the map key. A missing or misspelled type creates a …usedResources.undefined state; loadUsedResources then reads it back as a real resource type via keys[i].split('.').pop(). Validating against the known UsedResourceDataMap keys, and reporting { error } for anything else, keeps the state tree and the reloaded registry clean.
Smaller point: the { error: e.message } reply is currently unreachable from the adapter API, because ResourceManager uses pushMessage without a callback. See the note there.
| } | ||
|
|
||
| // let the controller track the instance's port if the adapter does not do it itself | ||
| autoRegisterUsedResources(id, instance); |
There was a problem hiding this comment.
This is inside the !wakeUp && proc.process && enabled && mode !== 'once' branch, so auto-registration only happens for daemon starts. mode: 'schedule' and mode: 'once' instances, and extensions with native.webInstance, never get their native.port registered.
The asymmetry is the part that will confuse: startScheduledInstance's exit handler does call setInstanceBlocked(instanceNamespace, false) for scheduled instances, so the unblock path covers a case the register path does not. Today that is a no-op, but it will silently start clearing flags on entries a future seeding step creates.
| } | ||
|
|
||
| const namespace = id.startsWith(SYSTEM_ADAPTER_PREFIX) ? id.substring(SYSTEM_ADAPTER_PREFIX.length) : id; | ||
| persistUsedResourceTypes(usedResources.register('tcpPort', data, namespace)).catch(e => |
There was a problem hiding this comment.
Registration happens only when the instance process starts, which does not deliver the semantics isBlocked: false documents ("the instance is not running, but could use this resource, when started"):
- an instance that exists but has never been started contributes nothing;
- after a controller restart,
assess()unblocks the surviving entries, but instances that stay stopped are only present if they had been started at some point and the persisted state survived; - changing
native.porton a running instance is not picked up until it restarts, so the registry advertises the old port.
Since #3405's motivation is letting the user pick a free port while configuring a new instance, the stopped and freshly-reconfigured instances are exactly the ones that need to be listed. Seeding from the instance objects — in loadUsedResources where the instance view is already being read, and on the objectChange path where native changes arrive — would cover all three, and would make this call site redundant.
Also note this call passes no doNotDeleteAlreadyUsed, so it inherits the cross-type wipe described on UsedResourcesRegistry.register.
| const original = parsed as { instance: string }[]; | ||
| const filtered = original.filter(entry => !matches(entry.instance)); | ||
| if (filtered.length !== original.length) { | ||
| await this.states.setStateAsync(id, { val: JSON.stringify(filtered), ack: true }); |
There was a problem hiding this comment.
This writes the registry states directly, which is correct for the case the JSDoc describes (controller down), but it is not guarded to that case. If a controller is running — in particular one on another host, which the local CLI cannot notice — that host still holds the entries in memory, and its next persistUsedResources for the same type rewrites the state from the in-memory copy and resurrects everything this loop just removed.
Sending freeUsedResource to the owning host when it is alive and falling back to the direct write otherwise would make it deterministic. The host is derivable from the state id (system.host.<host>.usedResources.<type>).
Separately, both catch blocks are silent: the outer one returns on any getKeys failure and the inner one swallows per-key errors including the setState write. Deleting an instance would then report success while leaving its ports listed as occupied. A debug/warn line on each would make that diagnosable.
| reg.register('tcpPort', { port: 1883 }, 'mqtt.0', true); | ||
| assert.deepStrictEqual(reg.getTypes().sort(), ['serialPort', 'tcpPort']); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
19 focused tests on the pure registry, which is the right place to start — but everything the tests cover is the part that is easy to get right, and nothing covers the integration layer where the issues above live: no round-trip through persistUsedResources / loadUsedResources / assess, nothing for autoRegisterUsedResources, nothing for Install._freeUsedResources.
A few cases that would have caught defects reported here:
register()with a payload carrying aninstanceortypekey, thenremoveInstance()— asserts the entry is actually removable.register('tcpPort', { port: 8080, bind: '0.0.0.0' })thenfree('tcpPort', { port: 8080 })— pins whether a partial payload is meant to match.free()the last entry of a type, thengetTypes()— the existinggetTypes lists the types that hold entriesnever exercises the empty-bucket case its name implies (getTypes()currently returns['gpio']for an emptygpiobucket).- persist → load →
assess()with one instance missing, asserting the resulting states.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
packages/controller/src/lib/usedResources.ts:175
- UsedResourcesRegistry.get() only does a shallow clone ({ ...r }), so callers can mutate the returned entry's nested data object and thereby mutate the registry’s internal state (contradicting the intent of returning copies). Return a deep copy of each entry (including data) to prevent accidental external mutation.
get(type?: ioBroker.UsedResourceType): ioBroker.RegisteredResource[] {
const clone = (r: ioBroker.RegisteredResource): ioBroker.RegisteredResource => ({ ...r });
if (type) {
return (this.resources.get(type) || []).map(clone);
}
packages/cli/src/lib/setup/setupInstall.ts:1731
- _freeUsedResources() assumes every parsed entry has a string "instance". If the registry state contains a malformed entry (e.g., instance missing/non-string), matches(entry.instance) can throw (startsWith on non-string) and the whole state cleanup for that id is skipped.
const original = parsed as { instance: string }[];
const filtered = original.filter(entry => !matches(entry.instance));
if (filtered.length !== original.length) {
await this.states.setStateAsync(id, { val: JSON.stringify(filtered), ack: true });
}
| // the instance is no longer running: keep its resource registrations but mark them as not actively blocked | ||
| await persistUsedResourceTypes(usedResources.setInstanceBlocked(adapterInstance, false)); | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/controller/src/main.ts:2220
- When loading persisted
system.host.<name>.usedResources.<type>states, entries are only shape-validated but not checked thatentry.typematches the buckettypederived from the state id. A corrupted/misplaced entry (e.g. aserialPortentry inside thetcpPortstate) would be loaded into the wrong registry bucket and then become unreachable for type-based operations/persistence.
// drop entries that do not have the expected shape, so nothing malformed enters the registry
const valid = parsed.filter(entry => isRegisteredResource(entry));
if (valid.length !== parsed.length) {
packages/adapter/src/lib/adapter/adapter.ts:9157
- Issue #3405 (linked in the PR description) specifies an adapter API method
getUsedResources(type?), but this PR introducesgetHostUsedResources(type?)instead (and nogetUsedResourcesalias). If external consumers/adapters are expected to follow the issue spec, consider addinggetUsedResourcesas a backward-compatible alias (or update the issue/PR description to reflect the renamed API).
/**
* Query the exclusive resources currently registered as used on the **host** this instance runs on.
*
* Unlike `registerUsedResource`/`freeUsedResource`/`clearUsedResources`, which only ever touch the
* resources of this instance, this returns the resources of **all** instances of this host, so the user
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
packages/controller/src/main.ts:6466
- For adapters running in compact groups above 0, this executes in the compact-group controller and mutates its separate empty registry;
persistUsedResourcesthen returns immediately forcompactGroupController. Registrations were stored in the main controller's registry, so an individual compact adapter exit/crash leaves them marked active indefinitely. Forward this lifecycle update to the main controller or have the main controller process the compact adapter's stop event.
await persistUsedResourceTypes(usedResources.setInstanceBlocked(adapterInstance, false));
packages/controller/src/lib/usedResources.ts:104
JSON.stringifyis property-order-sensitive for nested objects, so a filter containing the same nested value in a different insertion order fails to free the registration. Compare structured values with canonical deep equality rather than raw serialization.
if (JSON.stringify(entries[key]) !== JSON.stringify(value)) {
packages/controller/src/main.ts:2197
- The startup assessment treats instances assigned to other hosts as valid owners of this host's persisted entries. If an instance is moved while this controller is down, the old host reloads and retains its registrations because the namespace still exists globally and no object-change event will remove it. Restrict
existingInstancestocommon.host === hostname, just like the registry itself.
existingInstances.add(instance._id.substring(SYSTEM_ADAPTER_PREFIX.length));
if (instance.common?.host === hostname && !instance.common.declareUsedResources) {
controllerManaged.push(instance);
packages/controller/src/main.ts:2219
- Validation checks that each entry has some valid resource type, but not that it matches the state bucket being loaded. A
udpPortentry in thetcpPortstate therefore passes and is returned by the typedgetHostUsedResources('tcpPort')API. Treat a type/bucket mismatch as malformed.
const valid = parsed.filter(entry => isRegisteredResource(entry));
packages/controller/src/main.ts:4679
- Excluding every scheduled instance misses the
common.allowInitpath below, which immediately forks an adapter. Controller-derived resources are never marked active during that process, while adapter-declared registrations made during init remainisBlocked: truebecause the allow-init exit handler never unblocks them. Apply the same start/exit resource lifecycle to the allow-init process.
} else if (mode !== 'schedule') {
// the resources derived from the configuration are held again as soon as the instance runs
// (for "schedule" this happens per run in startScheduledInstance)
await persistUsedResourceTypes(usedResources.setInstanceBlocked(namespace, true));
packages/controller/src/lib/usedResources.ts:75
- Only top-level payload keys are sorted. Two custom resources with semantically identical nested objects but different nested property insertion order produce different keys, so re-registration creates duplicates despite the documented content-based comparison. Use a recursive canonical serializer for payload values.
This issue also appears on line 104 of the same file.
.map(key => `${key}=${JSON.stringify(data[key])}`)
packages/cli/src/lib/setup/setupInstall.ts:1672
- This cleanup helper is invoked by
deleteInstance, but not bydeleteAdapter's direct all-instances deletion branch (which deletes objects/states at lines 1603-1604 without callingdeleteInstance). Deleting a complete adapter while its host is down therefore leaves all of its registry entries behind. Invoke_freeUsedResources(adapter)in that branch as well.
private async _freeUsedResources(adapter: string, instance?: number): Promise<void> {
| if (instance.common.declareUsedResources) { | ||
| // the adapter declares its resources itself: drop what it declared before this (re)start, because | ||
| // the settings may have changed in between. Everything it registers from now on is additive. | ||
| await persistUsedResourceTypes(usedResources.removeInstance(namespace)); |
Apollon77
left a comment
There was a problem hiding this comment.
Re-reviewed after 03c053e27 / 544d03f01. Every point from the previous round is addressed, and in most cases addressed at the root rather than at the symptom:
| Previous finding | Now |
|---|---|
payload overwrote type/instance/isBlocked |
payload nested under data; satisfies replaces the cast; the type carries a JSDoc explaining why |
String() key collisions, undefined duplicates |
JSON.stringify over sorted keys, undefined dropped |
free needed the payload byte for byte |
matchesUsedResourceData — partial filter, free('tcpPort', { port }) matches an entry registered with bind |
register wiped all types by default |
register is additive; dropping is the explicit removeInstance, and the flag is gone from the whole API |
| empty buckets lingered | setEntries deletes the type when it empties |
| persist lost-update race | per-type promise chain, registry read moved after the awaits |
extendObject per write |
created once per type per run |
msg.message unvalidated |
instance derived from msg.from with a mismatch rejected, type checked against a pattern, payload shape checked |
| auto-register only for daemon starts | handled for every mode, plus per-run in startScheduledInstance |
| registry only seeded on process start | derived from the instance object in loadUsedResources and on every objectChange |
| CLI raced a live controller | skips hosts whose alive is true, fails safe, logs instead of swallowing |
The hardening that was not asked for is also worth noting: isRegisteredResource filtering malformed entries on load, isValidUsedResourceType on the state key, and the freeUsedResource no-match warning, which turns the one silent failure I complained about into something diagnosable.
The test file went from 19 to 45 tests with a named regression case for each defect above — does not let the payload overwrite the bookkeeping fields, is additive and independent of the call order, frees by a partial payload without repeating the optional fields, rejects an entry in the old flat format, returns deep copies, drops a type when its last entry is freed. That is the right shape.
Verified locally: testUsedResources.ts 45/45, ResourceManager.test.ts 13/13. I checked types in a scratch worktree rather than switching my working copy, so the workspace packages were unbuilt there and tsc could not resolve @iobroker/* — the files this PR touches produce no errors of their own, but that is not a full gate, so CI is the word on it.
Four things left, one of which I think matters before merge — details inline. Nothing on the registry itself; all four sit in the layer around it.
| * @param dataRequired whether the payload is mandatory - it is for `registerUsedResource` | ||
| * @returns the validated instance, resource type and payload | ||
| */ | ||
| function parseUsedResourceMessage( |
There was a problem hiding this comment.
Nothing here checks that the sending instance actually has common.declareUsedResources set, and that combines badly with the new derivation path.
The chain:
- an adapter calls
registerUsedResource('serialPort', …)but itsio-package.jsonnever setcommon.declareUsedResources— an easy omission, since the API works without it; - the entry is accepted and persisted, so it looks like it worked;
- any later change to that instance object reaches
syncUsedResourcesOfInstance, which takes the!obj.common.declareUsedResourcesbranch and callsseedUsedResourcesOfInstance; - that starts with
usedResources.removeInstance(namespace)(line 2290), so the serial port is gone — replaced by whatevernative.portsays, which for a serial adapter is usually nothing at all.
The adapter is never told, and the trigger is an unrelated object write, so it will look like the registry randomly forgets entries. It is the same failure the old doNotDeleteAlreadyUsed default produced, arriving through the new door.
Rejecting the message when the flag is not set is the tighter fix — the adapter developer then gets a warning naming the missing io-package.json entry instead of a silent disappearance. The instance object is reachable here via procs[SYSTEM_ADAPTER_PREFIX + instance]?.config, so it does not cost a database read on the common path.
The looser alternative is to have seedUsedResourcesOfInstance remove only what it derived itself rather than everything for that instance, but then the two sources can drift and I would not prefer it.
| if (!instance?._id) { | ||
| continue; | ||
| } | ||
| existingInstances.add(instance._id.substring(SYSTEM_ADAPTER_PREFIX.length)); |
There was a problem hiding this comment.
existingInstances is filled from every row of the instance view, without the common.host === hostname filter that controllerManaged two lines below does apply. So assess() keeps an entry as long as the instance exists anywhere in the system.
That leaves a hole for a moved instance. At runtime syncUsedResourcesOfInstance handles it correctly — obj.common.host !== hostname removes the entries. But if the move happens while this controller is down, the instance still exists on restart, assess() keeps its entries, controllerManaged does not re-derive them because it is host-filtered, and no objectChange ever fires for an object that did not change afterwards. The entries then sit in this host's registry indefinitely, showing a port as occupied on a host where nothing occupies it.
Filtering the same way as controllerManaged fixes it:
if (instance.common?.host === hostname) {
existingInstances.add(instance._id.substring(SYSTEM_ADAPTER_PREFIX.length));
}That also makes assess's contract match what the registry is — a per-host registry — rather than a system-wide existence check.
| pendingUsedResourceWrites.set(type, next); | ||
|
|
||
| // forget the chain again once nothing else is queued behind it | ||
| void next.then(() => { |
There was a problem hiding this comment.
The chain has no failure path, and its failure mode is total and silent.
If a queued next ever rejects, this .then never runs, so the entry is never cleared from pendingUsedResourceWrites. Every later write of that type chains onto a rejected promise, whose .then(() => writeUsedResources(type)) callback is never invoked — so that resource type stops being persisted for the rest of the controller's life, with no log line, while the in-memory registry keeps accepting changes. The next restart then reads back stale content.
Today it cannot happen: writeUsedResources wraps everything after const id = … in try/catch and returns normally. But that is an invariant living in a different function, and it is one stray throw — or a logger.warn that fails during shutdown — away from breaking.
Making it structural is a character:
const next = pending.catch(() => {}).then(() => writeUsedResources(type));and a .catch on the cleanup, so the chain always recovers regardless of what a link does.
| from: `system.adapter.${this.namespace}`, | ||
| }; | ||
|
|
||
| await this.states.pushMessage(`system.host.${this.host}`, obj); |
There was a problem hiding this comment.
pushMessage without a callback means the adapter never sees the host's answer, and that matters more now than it did in the previous round, because the host has real rejection paths since this update: an invalid type, a missing payload, a payload that is not an object, an instance mismatch. Each produces sendTo(msg.from, msg.command, { error: … }), which nothing is listening for.
So an adapter developer who passes a misspelled type gets a resolved Promise<void> and no indication of failure — the only trace is a warn in the host log, which is not where somebody debugging their adapter looks first. registerUsedResource returning Promise<void> reads like a completion signal, and here it only signals that the message was queued.
Since the host already implements the reply for all three commands, awaiting it would make the API honest and cost one round trip on a call that happens a handful of times per adapter start. If you would rather keep it fire-and-forget for latency reasons, then the JSDoc should say the promise resolves on hand-off and not on completion — right now nothing warns the reader.
|
|
||
| assert.deepStrictEqual(reg.getTypes(), []); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Narrowing my earlier comment rather than repeating it: 45 tests with a named regression case per fixed defect is exactly what I was hoping for, and the registry is now the best-covered part of this PR.
What is still untested is the layer the remaining three comments are about, and none of it needs a running controller — they are ordinary functions over an instance object and a registry:
seedUsedResourcesOfInstance/getConfiguredTcpPort:native.portas a string,0, out of range, absent, with and withoutnative.bind, and that re-seeding an instance replaces rather than accumulates.syncUsedResourcesOfInstance: deleted object,common.hostpointing elsewhere, and thedeclareUsedResourcesbranch leaving the registry untouched.parseUsedResourceMessage/getUsedResourceMessageInstance: afromthat is not an instance, a body claiming a foreign instance, an invalid type, a non-object payload. These are the validation rules the previous round asked for, and a test is what keeps them from being relaxed later by someone who does not know why they exist.loadUsedResources: persist → load →assesswith one instance missing and one moved, which is where theexistingInstancescomment above would have been caught.
The first three are pure and need nothing but a fixture object. The last one needs stubs for objects/states, which is more work — if only one of the four lands, I would pick parseUsedResourceMessage.
This is the implementation of feature #3405
When the AsyncAdapter will appear, I will modify it to be there.