From de9d34e7cbf57f9dfaf6442c3fa23fa60758745f Mon Sep 17 00:00:00 2001 From: Maurice Schmicking <17197791+mschmicking@users.noreply.github.com> Date: Mon, 17 Aug 2026 23:14:45 +0200 Subject: [PATCH] feat(cli): sweep the adapter markers a deleted script leaves behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The javascript adapter keeps two bookkeeping states beside every script, on every javascript instance rather than only the one that runs it: javascript..scriptEnabled. javascript..scriptProblem. Both are created by the adapter's load(), which calls createActiveObject and createProblemObject before prepareScript checks common.engine to decide whether this instance should run the script at all. Every instance runs load() for every non-global script at startup and again on every source change, so all of them hold a pair for every script. Deletion, though, is gated on the engine: only the instance that owned the script at the moment it was deleted removes its own pair. Every other instance keeps one for a script that no longer exists, js-controller warns about it for the life of the system, and nothing in ioBroker ever collects it. This is independent of who deleted the script — the Admin UI leaves the identical residue. remove, rename and move now sweep both markers of the id they just deleted. The sweep runs after the object is gone and after the trash backup, and is best-effort: it warns rather than turning a completed delete into a failure. Order is enforced in ObjectsApi.deleteScriptMarker — value first, object second, never the reverse, because an object deleted out from under a surviving value is exactly the orphan being cleaned up. (The adapter's own cleanup gets this backwards, which is a likely source of the "state has no object" warnings.) remove also stops refusing an id whose script is already gone when markers remain, since otherwise pre-existing ones cannot be cleared by anything. It sweeps them, touches nothing else, and keeps the local file. doctor gains a read-only `markers` check that counts both kinds, names the orphans, and warns rather than fails — nothing here is broken. Both kinds are handled through MARKER_KINDS in types.ts rather than a pattern repeated per call site: an earlier draft covered only scriptEnabled and left eight orphaned scriptProblem states on a live instance while doctor reported it clean. Verified against ioBroker.javascript v8.9.2 on a live three-instance system, and covered by tests against the in-process fake server. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 16 +++ README.md | 40 +++--- docs/TROUBLESHOOTING.md | 48 ++++++- src/client/objects.ts | 125 +++++++++++++++++ src/commands/doctor.ts | 85 ++++++++++- src/commands/remove.ts | 105 +++++++++++++- src/commands/rename.ts | 7 +- src/types.ts | 53 +++++++ test/commands-destructive.test.ts | 177 +++++++++++++++++++++++ test/commands-doctor.test.ts | 83 ++++++++++- test/fake-server.ts | 87 ++++++++++++ test/objects.test.ts | 226 ++++++++++++++++++++++++++++++ 12 files changed, 1014 insertions(+), 38 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bbf6ce8..b8c910d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -31,6 +31,22 @@ instruction from the user: 4. **Deletion is explicit.** Only `remove`, `rename` and `move` delete anything, each requires `--yes`, and each writes the full object JSON to `.iobroker-sync/trash/` _before_ deleting. A failed backup aborts the operation. + + The one thing they delete without backing it up is the pair of adapter markers + belonging to the script being deleted — `javascript..scriptEnabled.` and + `javascript..scriptProblem.` — and only after that script is gone. Those are + adapter-generated derived state (`common.enabled` and `common.engine` are already in + the trash copy), so there is nothing in them to lose. See `cleanUpScriptMarkers` in + `commands/remove.ts` for why they have to be swept at all, and + `ObjectsApi.deleteScriptMarker` for the ordering rule (value first, object second, + never the reverse). The sweep is best-effort: it warns, it never fails the command. + + **Both kinds or neither.** The adapter creates and deletes the two together, so code + that handles only `scriptEnabled` cleans up half a mess and reports success. That is + not hypothetical: the first version of this sweep shipped that way and left eight + orphaned `scriptProblem` states on a live instance while `doctor` called it clean. + `MARKER_KINDS` in `types.ts` is the single list; anything iterating markers iterates it. + 5. **Copy-then-delete must verify first.** ioBroker has no native rename/move, so both are implemented as copy-verify-delete. The verification compares the actual source text. Checking only that "something exists at the new id" is not verification — a diff --git a/README.md b/README.md index 80f1cd7..718d2b2 100644 --- a/README.md +++ b/README.md @@ -108,29 +108,29 @@ If `logs` shows nothing, that is usually the adapter's own log level rather than **Sync** -| Command | Description | -| ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `init` | Write `.iobroker-sync.json`, verify the connection, create the script folder. Asks interactively when run without flags. `--types` also sets up TypeScript definitions. | -| `types` | Set up editor intellisense (`log`, `schedule`, ...). `--force`, `--offline`. | -| `login` / `logout` | Save or remove the password for this instance. Never stored in the project. | -| `trust` | Accept the instance's current TLS certificate. Only needed after it changes. `--yes` skips the prompt. | -| `doctor` | Check config, certificate, login, connection and a live round-trip, and say which one is wrong. Read-only, never prompts. Run this first when something looks broken. | -| `pull [pattern]` | Download scripts to disk. Never deletes or overwrites local files. | -| `push [pattern]` | Upload locally modified scripts. Never deletes remote objects. | -| `status` | Show what changed, locally and remotely. | -| `diff [pattern]` | Unified diff of local vs server. `--against ` compares with a backup instead. | -| `watch` | Push on save. `--pull` also applies remote changes. | -| `logs [pattern]` | Stream the server log. `--level`, `--limit`. Read-only. | -| `backup [pattern]` | Snapshot every script — source _and_ full object — to `.iobroker-sync/backup//`. Read-only against the server. | +| Command | Description | +| ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `init` | Write `.iobroker-sync.json`, verify the connection, create the script folder. Asks interactively when run without flags. `--types` also sets up TypeScript definitions. | +| `types` | Set up editor intellisense (`log`, `schedule`, ...). `--force`, `--offline`. | +| `login` / `logout` | Save or remove the password for this instance. Never stored in the project. | +| `trust` | Accept the instance's current TLS certificate. Only needed after it changes. `--yes` skips the prompt. | +| `doctor` | Check config, certificate, login, connection, a live round-trip and leftover adapter markers, and say which one is wrong. Read-only, never prompts. Run this first when something looks broken. | +| `pull [pattern]` | Download scripts to disk. Never deletes or overwrites local files. | +| `push [pattern]` | Upload locally modified scripts. Never deletes remote objects. | +| `status` | Show what changed, locally and remotely. | +| `diff [pattern]` | Unified diff of local vs server. `--against ` compares with a backup instead. | +| `watch` | Push on save. `--pull` also applies remote changes. | +| `logs [pattern]` | Stream the server log. `--level`, `--limit`. Read-only. | +| `backup [pattern]` | Snapshot every script — source _and_ full object — to `.iobroker-sync/backup//`. Read-only against the server. | **Lifecycle** -| Command | Description | -| ---------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | -| `list` | All scripts with instance and enabled state. | -| `start` / `stop` / `restart` `` | Toggle `common.enabled`. | -| `new ` | Create a new script (disabled) plus any missing folders. | -| `rename` / `move` / `remove` | Destructive. Require `--yes` and back up the object first. `remove` keeps the local file unless `--delete-local`. | +| Command | Description | +| ---------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `list` | All scripts with instance and enabled state. | +| `start` / `stop` / `restart` `` | Toggle `common.enabled`. | +| `new ` | Create a new script (disabled) plus any missing folders. | +| `rename` / `move` / `remove` | Destructive. Require `--yes` and back up the object first. `remove` keeps the local file unless `--delete-local`. All three also clean up the `scriptEnabled`/`scriptProblem` states the old id leaves behind on every javascript instance. | `--dry-run`, `--verbose`, `--json` and `-C ` are global and work with every command. When in doubt, `--dry-run` shows what would happen and changes nothing. diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index 818e9f1..4288e85 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -3,9 +3,9 @@ Things that look like bugs and are not. See the [README](../README.md) for the overview. **Start with `iob-sync doctor`.** It checks the config, the certificate, the login, the -connection and a live round-trip, and names the one that is wrong — including the two -cases below, which are the ones that reliably send people down the wrong path. It is -read-only and never prompts, so it is safe to run at any time. +connection, a live round-trip and the adapter's leftover script markers, and names the one +that is wrong — including the cases below, which are the ones that reliably send people +down the wrong path. It is read-only and never prompts, so it is safe to run at any time. ## Commands time out, but the connection "works" @@ -86,6 +86,48 @@ a script or move it to another javascript instance, it cannot — that is delibe sync bug cannot stop a running script. Use `start` / `stop` for `enabled`; instance moves must be done in Admin. +## `doctor` warns about orphaned markers + +Nothing is broken, and no script is affected. + +The javascript adapter keeps two bookkeeping states beside every script, on **every** +javascript instance — not only the one that runs it: + +``` +javascript..scriptEnabled.