feat(cli): sweep the adapter markers a deleted script leaves behind (#30)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: mschmicking

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.<n>.scriptEnabled.<id>` and
+   `javascript.<n>.scriptProblem.<id>` — 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

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 <snapshot>` 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/<timestamp>/`. 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 <snapshot>` 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/<timestamp>/`. Read-only against the server.                                                                       |
 
 **Lifecycle**
 
-| Command                                  | Description                                                                                                       |
-| ---------------------------------------- | ----------------------------------------------------------------------------------------------------------------- |
-| `list`                                   | All scripts with instance and enabled state.                                                                      |
-| `start` / `stop` / `restart` `<pattern>` | Toggle `common.enabled`.                                                                                          |
-| `new <path>`                             | 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` `<pattern>` | Toggle `common.enabled`.                                                                                                                                                                                                                    |
+| `new <path>`                             | 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 <dir>` are global and work with every command.
 When in doubt, `--dry-run` shows what would happen and changes nothing.

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.<n>.scriptEnabled.<script id>
+javascript.<n>.scriptProblem.<script id>
+```
+
+Both are created by the adapter's `load()`, which calls `createActiveObject` and
+`createProblemObject` _before_ `prepareScript` checks `common.engine` to decide whether
+this instance should actually run the script. Every instance runs `load()` for every
+non-global script at startup, and again on every source change. So on a three-instance
+system, ten scripts mean sixty of these states, and that is normal.
+
+Deletion, however, _is_ gated on the engine: only the instance that owned the script at
+the moment it was deleted removes its own pair. The pairs on the other instances stay for
+the life of the system, js-controller complains about them, and nothing in ioBroker ever
+collects them.
+
+This has nothing to do with who did the deleting — the Admin UI leaves exactly the same
+residue. Verified against ioBroker.javascript v8.9.2.
+
+`iob-sync doctor` lists them by id, both kinds. To clear one script's leftovers:
+
+```bash
+iob-sync remove script.js.diag.retired-check --yes
+```
+
+`remove` sweeps the markers even when the script itself is already gone from the server —
+in that case it deletes nothing else and leaves your local file alone. `rename` and `move`
+sweep the old id's markers as they go, so this does not accumulate from normal use.
+
+One detail worth knowing if you clean these up by hand: delete the **state value first,
+then the object**. The reverse order leaves a value with no object behind it, which is the
+shape js-controller actually warns about — and it is the order the adapter's own cleanup
+uses (`delObject` then `delState`), so that warning may well have come from ioBroker
+itself rather than from anything you did.
+
 ## A push is refused as a conflict
 
 Local and remote both changed since the last sync. Inspect with `iob-sync diff`, then

src/client/objects.ts

@@ -6,9 +6,12 @@
 import {
   FolderObject,
   IoBrokerObject,
+  MARKER_KINDS,
+  MarkerKind,
   ObjectsApi,
   ObjectViewResult,
   ScriptCommon,
+  ScriptMarkerEntry,
   ScriptObject,
   SocketClient,
 } from '../types';
@@ -17,6 +20,43 @@ const SCRIPT_NAMESPACE = 'script.js.';
 // Verified endkey used by the Admin UI itself to bound a getObjectView range scan.
 const VIEW_ENDKEY = `${SCRIPT_NAMESPACE}香`;
 
+const JS_NAMESPACE = 'javascript.';
+
+/** `javascript.*.<kind>.*` — every instance's markers of one kind, for every script. */
+function markerPattern(kind: MarkerKind): string {
+  return `${JS_NAMESPACE}*.${kind}.*`;
+}
+
+/**
+ * Splits a marker id into the script it belongs to and which kind it is:
+ * `javascript.2.scriptEnabled.common.garage` -> `script.js.common.garage`, scriptEnabled.
+ *
+ * Returns null for anything that is not one — including ids with something other than
+ * a bare instance number in front of the kind, so that a state someone else parked
+ * under `javascript.` cannot be mistaken for ours and deleted.
+ */
+export function parseMarkerId(stateId: string): { scriptId: string; kind: MarkerKind } | null {
+  if (!stateId.startsWith(JS_NAMESPACE)) {
+    return null;
+  }
+  for (const kind of MARKER_KINDS) {
+    const infix = `.${kind}.`;
+    const kindAt = stateId.indexOf(infix);
+    if (kindAt === -1) {
+      continue;
+    }
+    const instance = stateId.slice(JS_NAMESPACE.length, kindAt);
+    if (!/^\d+$/.test(instance)) {
+      continue;
+    }
+    const suffix = stateId.slice(kindAt + infix.length);
+    if (suffix) {
+      return { scriptId: `${SCRIPT_NAMESPACE}${suffix}`, kind };
+    }
+  }
+  return null;
+}
+
 export class AdminObjectsApi implements ObjectsApi {
   constructor(private readonly socket: SocketClient) {}
 
@@ -111,4 +151,89 @@ export class AdminObjectsApi implements ObjectsApi {
   async deleteObject(id: string): Promise<void> {
     await this.socket.emit('delObject', [id]);
   }
+
+  /**
+   * Reads the value side and the object side separately and unions them, because a
+   * marker can exist as either half alone and the halves are what we need to tell apart.
+   *
+   * One half failing is tolerated: `getStates` and `getForeignObjects` are separate
+   * commands with separate histories across Admin versions, and a partial answer here
+   * still beats reporting nothing. Only a total failure propagates.
+   */
+  async listScriptMarkers(): Promise<ScriptMarkerEntry[]> {
+    const perKind = await Promise.all(MARKER_KINDS.map((kind) => this.listMarkersOfKind(kind)));
+    return perKind.flat().sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0));
+  }
+
+  private async listMarkersOfKind(kind: MarkerKind): Promise<ScriptMarkerEntry[]> {
+    const pattern = markerPattern(kind);
+    const [values, objects] = await Promise.allSettled([
+      this.readMarkerValues(pattern),
+      this.socket.emit<Record<string, unknown> | null>('getForeignObjects', [pattern, 'state']),
+    ]);
+
+    if (values.status === 'rejected' && objects.status === 'rejected') {
+      throw values.reason;
+    }
+
+    const valueIds = new Set(values.status === 'fulfilled' ? Object.keys(values.value ?? {}) : []);
+    const objectIds = new Set(
+      objects.status === 'fulfilled' ? Object.keys(objects.value ?? {}) : [],
+    );
+
+    const entries: ScriptMarkerEntry[] = [];
+    for (const id of new Set([...valueIds, ...objectIds])) {
+      const parsed = parseMarkerId(id);
+      // The pattern is a wildcard match, so it can catch ids that merely look the part
+      // (`javascript.0.scriptEnabled` with no suffix, or a non-numeric instance).
+      // parseMarkerId is the authority on what is really ours.
+      if (parsed?.kind !== kind) {
+        continue;
+      }
+      entries.push({
+        id,
+        scriptId: parsed.scriptId,
+        kind,
+        hasValue: valueIds.has(id),
+        hasObject: objectIds.has(id),
+      });
+    }
+    return entries;
+  }
+
+  /**
+   * `getStates` is the current command; `getForeignStates` is its deprecated alias and
+   * the only one older Admin builds answer. Trying both costs one extra round trip on
+   * instances that need it and nothing on instances that do not.
+   */
+  private async readMarkerValues(pattern: string): Promise<Record<string, unknown> | null> {
+    try {
+      return await this.socket.emit<Record<string, unknown> | null>('getStates', [pattern]);
+    } catch {
+      return this.socket.emit<Record<string, unknown> | null>('getForeignStates', [pattern]);
+    }
+  }
+
+  async deleteScriptMarker(entry: ScriptMarkerEntry): Promise<void> {
+    if (!parseMarkerId(entry.id)) {
+      // Belt and braces: this API is reachable from command code, and the whole point
+      // of it is deleting things, so it refuses anything outside its own namespace.
+      throw new Error(`Refusing to delete "${entry.id}": not a script marker.`);
+    }
+
+    // Value first. If this throws we stop here on purpose — deleting the object while
+    // the value survives would manufacture the exact orphan this code exists to remove.
+    if (entry.hasValue) {
+      await this.socket.emit('delState', [entry.id]);
+    }
+
+    if (entry.hasObject) {
+      // Admin's delState is documented to take the object with it. Verify rather than
+      // assume: on an instance where it does not, the object has to go separately.
+      const remaining = await this.socket.emit<IoBrokerObject | null>('getObject', [entry.id]);
+      if (remaining) {
+        await this.socket.emit('delObject', [entry.id]);
+      }
+    }
+  }
 }