fix: harden matrix startup errors + add regressions (#31023) (thanks @efe-arv)

steipete · steipete · commit 84d0a794ec9b · 2026-03-02T00:15:10.000Z
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -94,6 +94,7 @@ Docs: https://docs.openclaw.ai
 
 - Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin.
 - Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek.
+- Matrix/Conduit compatibility: avoid blocking startup on non-resolving Matrix sync start, preserve startup error propagation, prevent duplicate monitor listener registration, remove unreliable 2-member DM heuristics, accept `!room` IDs without alias resolution, and add matrix monitor/client regression coverage. Landed from contributor PR #31023 by @efe-arv. Thanks @efe-arv.
 - Security/Skills: harden skill installer metadata parsing by rejecting unsafe installer specs (brew/node/go/uv/download) and constrain plugin-declared skill directories to the plugin root (including symlink-escape checks), with regression coverage.
 - Discord/DM command auth: unify DM allowlist + pairing-store authorization across message preflight and native command interactions so DM command gating is consistent for `open`/`pairing`/`allowlist` policies.
 - Sessions/Usage accounting: persist `cacheRead`/`cacheWrite` from the latest call snapshot (`lastCallUsage`) instead of accumulated multi-call totals, preventing inflated token/cost reporting in long tool/compaction runs. (#31005)
diff --git a/extensions/matrix/src/matrix/client/shared.test.ts b/extensions/matrix/src/matrix/client/shared.test.ts
@@ -0,0 +1,85 @@
+import type { MatrixClient } from "@vector-im/matrix-bot-sdk";
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { resolveSharedMatrixClient, stopSharedClient } from "./shared.js";
+import type { MatrixAuth } from "./types.js";
+
+const createMatrixClientMock = vi.hoisted(() => vi.fn());
+
+vi.mock("./create-client.js", () => ({
+  createMatrixClient: (...args: unknown[]) => createMatrixClientMock(...args),
+}));
+
+function makeAuth(suffix: string): MatrixAuth {
+  return {
+    homeserver: "https://matrix.example.org",
+    userId: `@bot-${suffix}:example.org`,
+    accessToken: `token-${suffix}`,
+    encryption: false,
+  };
+}
+
+function createMockClient(startImpl: () => Promise<void>): MatrixClient {
+  return {
+    start: vi.fn(startImpl),
+    stop: vi.fn(),
+    getJoinedRooms: vi.fn().mockResolvedValue([]),
+    crypto: undefined,
+  } as unknown as MatrixClient;
+}
+
+describe("resolveSharedMatrixClient startup behavior", () => {
+  afterEach(() => {
+    stopSharedClient();
+    createMatrixClientMock.mockReset();
+    vi.useRealTimers();
+  });
+
+  it("propagates the original start error during initialization", async () => {
+    vi.useFakeTimers();
+    const startError = new Error("bad token");
+    const client = createMockClient(
+      () =>
+        new Promise<void>((_resolve, reject) => {
+          setTimeout(() => reject(startError), 1);
+        }),
+    );
+    createMatrixClientMock.mockResolvedValue(client);
+
+    const startPromise = resolveSharedMatrixClient({
+      auth: makeAuth("start-error"),
+    });
+    const startExpectation = expect(startPromise).rejects.toBe(startError);
+
+    await vi.advanceTimersByTimeAsync(2001);
+    await startExpectation;
+  });
+
+  it("retries start after a late start-loop failure", async () => {
+    vi.useFakeTimers();
+    let rejectFirstStart: ((err: unknown) => void) | undefined;
+    const firstStart = new Promise<void>((_resolve, reject) => {
+      rejectFirstStart = reject;
+    });
+    const secondStart = new Promise<void>(() => {});
+    const startMock = vi.fn().mockReturnValueOnce(firstStart).mockReturnValueOnce(secondStart);
+    const client = createMockClient(startMock);
+    createMatrixClientMock.mockResolvedValue(client);
+
+    const firstResolve = resolveSharedMatrixClient({
+      auth: makeAuth("late-failure"),
+    });
+    await vi.advanceTimersByTimeAsync(2000);
+    await expect(firstResolve).resolves.toBe(client);
+    expect(startMock).toHaveBeenCalledTimes(1);
+
+    rejectFirstStart?.(new Error("late failure"));
+    await Promise.resolve();
+
+    const secondResolve = resolveSharedMatrixClient({
+      auth: makeAuth("late-failure"),
+    });
+    await vi.advanceTimersByTimeAsync(2000);
+    await expect(secondResolve).resolves.toBe(client);
+    expect(startMock).toHaveBeenCalledTimes(2);
+  });
+});
diff --git a/extensions/matrix/src/matrix/client/shared.ts b/extensions/matrix/src/matrix/client/shared.ts
@@ -92,18 +92,19 @@ async function ensureSharedClientStarted(params: {
     // resolveSharedMatrixClient() calls know to retry.
     const startPromiseInner = client.start();
     let settled = false;
+    let startError: unknown = undefined;
     startPromiseInner.catch((err: unknown) => {
       settled = true;
+      startError = err;
       params.state.started = false;
       LogService.error("MatrixClientLite", "client.start() error:", err);
     });
     // Give the sync loop a moment to initialize before marking ready
-    await new Promise(resolve => setTimeout(resolve, 2000));
+    await new Promise((resolve) => setTimeout(resolve, 2000));
     if (settled) {
-      throw new Error("Matrix client.start() failed during initialization");
+      throw startError;
     }
     params.state.started = true;
-
   })();
   sharedClientStartPromises.set(key, startPromise);
   try {
diff --git a/extensions/matrix/src/matrix/monitor/direct.test.ts b/extensions/matrix/src/matrix/monitor/direct.test.ts
@@ -0,0 +1,63 @@
+import type { MatrixClient } from "@vector-im/matrix-bot-sdk";
+import { describe, expect, it, vi } from "vitest";
+import { createDirectRoomTracker } from "./direct.js";
+
+function createMockClient(params: {
+  isDm?: boolean;
+  senderDirect?: boolean;
+  selfDirect?: boolean;
+  members?: string[];
+}) {
+  const members = params.members ?? ["@alice:example.org", "@bot:example.org"];
+  return {
+    dms: {
+      update: vi.fn().mockResolvedValue(undefined),
+      isDm: vi.fn().mockReturnValue(params.isDm === true),
+    },
+    getUserId: vi.fn().mockResolvedValue("@bot:example.org"),
+    getJoinedRoomMembers: vi.fn().mockResolvedValue(members),
+    getRoomStateEvent: vi
+      .fn()
+      .mockImplementation(async (_roomId: string, _event: string, stateKey: string) => {
+        if (stateKey === "@alice:example.org") {
+          return { is_direct: params.senderDirect === true };
+        }
+        if (stateKey === "@bot:example.org") {
+          return { is_direct: params.selfDirect === true };
+        }
+        return {};
+      }),
+  } as unknown as MatrixClient;
+}
+
+describe("createDirectRoomTracker", () => {
+  it("treats m.direct rooms as DMs", async () => {
+    const tracker = createDirectRoomTracker(createMockClient({ isDm: true }));
+    await expect(
+      tracker.isDirectMessage({
+        roomId: "!room:example.org",
+        senderId: "@alice:example.org",
+      }),
+    ).resolves.toBe(true);
+  });
+
+  it("does not classify 2-member rooms as DMs without direct flags", async () => {
+    const tracker = createDirectRoomTracker(createMockClient({ isDm: false }));
+    await expect(
+      tracker.isDirectMessage({
+        roomId: "!room:example.org",
+        senderId: "@alice:example.org",
+      }),
+    ).resolves.toBe(false);
+  });
+
+  it("uses is_direct member flags when present", async () => {
+    const tracker = createDirectRoomTracker(createMockClient({ senderDirect: true }));
+    await expect(
+      tracker.isDirectMessage({
+        roomId: "!room:example.org",
+        senderId: "@alice:example.org",
+      }),
+    ).resolves.toBe(true);
+  });
+});
diff --git a/extensions/matrix/src/matrix/monitor/events.test.ts b/extensions/matrix/src/matrix/monitor/events.test.ts
@@ -138,4 +138,38 @@ describe("registerMatrixMonitorEvents", () => {
     expect(getUserId).toHaveBeenCalledTimes(1);
     expect(sendReadReceiptMatrixMock).not.toHaveBeenCalled();
   });
+
+  it("skips duplicate listener registration for the same client", () => {
+    const handlers = new Map<string, (...args: unknown[]) => void>();
+    const onMock = vi.fn((event: string, handler: (...args: unknown[]) => void) => {
+      handlers.set(event, handler);
+    });
+    const client = {
+      on: onMock,
+      getUserId: vi.fn().mockResolvedValue("@bot:example.org"),
+      crypto: undefined,
+    } as unknown as MatrixClient;
+    const params = {
+      client,
+      auth: { encryption: false } as MatrixAuth,
+      logVerboseMessage: vi.fn(),
+      warnedEncryptedRooms: new Set<string>(),
+      warnedCryptoMissingRooms: new Set<string>(),
+      logger: { warn: vi.fn() } as unknown as RuntimeLogger,
+      formatNativeDependencyHint: (() =>
+        "") as PluginRuntime["system"]["formatNativeDependencyHint"],
+      onRoomMessage: vi.fn(),
+    };
+    const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+
+    registerMatrixMonitorEvents(params);
+    const initialCallCount = onMock.mock.calls.length;
+    registerMatrixMonitorEvents(params);
+
+    expect(onMock).toHaveBeenCalledTimes(initialCallCount);
+    expect(errorSpy).toHaveBeenCalledWith(
+      "[matrix] skipping duplicate listener registration for client",
+    );
+    errorSpy.mockRestore();
+  });
 });
diff --git a/extensions/matrix/src/matrix/monitor/index.test.ts b/extensions/matrix/src/matrix/monitor/index.test.ts
@@ -0,0 +1,18 @@
+import { describe, expect, it } from "vitest";
+import { DEFAULT_STARTUP_GRACE_MS, isConfiguredMatrixRoomEntry } from "./index.js";
+
+describe("monitorMatrixProvider helpers", () => {
+  it("treats !-prefixed room IDs as configured room entries", () => {
+    expect(isConfiguredMatrixRoomEntry("!abc123")).toBe(true);
+    expect(isConfiguredMatrixRoomEntry("!RoomMixedCase")).toBe(true);
+  });
+
+  it("requires a homeserver suffix for # aliases", () => {
+    expect(isConfiguredMatrixRoomEntry("#alias:example.org")).toBe(true);
+    expect(isConfiguredMatrixRoomEntry("#alias")).toBe(false);
+  });
+
+  it("uses a non-zero startup grace window", () => {
+    expect(DEFAULT_STARTUP_GRACE_MS).toBe(5000);
+  });
+});
diff --git a/extensions/matrix/src/matrix/monitor/index.ts b/extensions/matrix/src/matrix/monitor/index.ts
@@ -36,6 +36,11 @@ export type MonitorMatrixOpts = {
 };
 
 const DEFAULT_MEDIA_MAX_MB = 20;
+export const DEFAULT_STARTUP_GRACE_MS = 5000;
+
+export function isConfiguredMatrixRoomEntry(entry: string): boolean {
+  return entry.startsWith("!") || (entry.startsWith("#") && entry.includes(":"));
+}
 
 export async function monitorMatrixProvider(opts: MonitorMatrixOpts = {}): Promise<void> {
   if (isBunRuntime()) {
@@ -153,7 +158,7 @@ export async function monitorMatrixProvider(opts: MonitorMatrixOpts = {}): Promi
         continue;
       }
       const cleaned = normalizeRoomEntry(trimmed);
-      if (cleaned.startsWith("!") || (cleaned.startsWith("#") && cleaned.includes(":"))) {
+      if (isConfiguredMatrixRoomEntry(cleaned)) {
         if (!nextRooms[cleaned]) {
           nextRooms[cleaned] = roomConfig;
         }
@@ -268,7 +273,7 @@ export async function monitorMatrixProvider(opts: MonitorMatrixOpts = {}): Promi
   const mediaMaxMb = opts.mediaMaxMb ?? accountConfig.mediaMaxMb ?? DEFAULT_MEDIA_MAX_MB;
   const mediaMaxBytes = Math.max(1, mediaMaxMb) * 1024 * 1024;
   const startupMs = Date.now();
-  const startupGraceMs = 5000; // 5s grace for slow homeservers (e.g. Conduit filter M_NOT_FOUND retry)
+  const startupGraceMs = DEFAULT_STARTUP_GRACE_MS;
   const directTracker = createDirectRoomTracker(client, { log: logVerboseMessage });
   registerMatrixAutoJoin({ client, cfg, runtime });
   const warnedEncryptedRooms = new Set<string>();
