Skip to content

Commit 23845a9

Browse files
massimoalbarellomax-agent
andauthored
fix: make OpenClaw memory removal recoverable (#208)
## Why Removal coupled local cleanup to gateway reachability and runtime work held by the plugin’s own background learner. Discovery could also hide broken installations, causing orphan install records to survive. The cancellation flow in #199 addressed stopping learning but left these lifecycle and ownership gaps. ## How Fence capture as soon as removal starts and cancel only the plugin-owned session on local disconnection, even after private files disappear. Restore journal-owned settings, use bounded SDK repair for missing-manifest entries, and delegate package files, install records and policy to OpenClaw. Preserve its [documented disabled marker](https://docs.openclaw.ai/cli/plugins/uninstall-and-update#uninstall) after removal so startup repair cannot reinstall the plugin; explicit setup enables it again. Recognize verified absence for repeated removal and let the native CLI own lifecycle waits. Treat gateway refresh as a separate activation step after local cleanup succeeds. Preserve user edits, remote memories and conversations. --------- Co-authored-by: max-agent <max-agent@Massimos-MacBook-Air-2.local>
1 parent 5e9fb61 commit 23845a9

11 files changed

Lines changed: 413 additions & 140 deletions

File tree

‎packages/openclaw-memory/scripts/e2e.ts‎

Lines changed: 63 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import assert from 'node:assert/strict';
22
import { existsSync } from 'node:fs';
3-
import { chmod, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
3+
import { chmod, mkdir, mkdtemp, realpath, rm, writeFile } from 'node:fs/promises';
44
import { createRequire } from 'node:module';
55
import { tmpdir } from 'node:os';
6-
import { dirname, join, resolve } from 'node:path';
6+
import { dirname, isAbsolute, join, relative, resolve } from 'node:path';
77
import { fileURLToPath } from 'node:url';
88
import { ownerBrowser } from '@repo/browser-testing/owner-browser';
99
import metadata from '../package.json';
@@ -55,11 +55,17 @@ const gatewayPort = gatewayReservation.port!;
5555
// The setup/removal tests below require an offline gateway, not an HTTP 503 listener.
5656
await gatewayReservation.stop(true);
5757

58-
async function command(input: string[] | { args: string[]; stdin: string }): Promise<string> {
59-
const { args, stdin } = Array.isArray(input) ? { args: input, stdin: undefined } : input;
58+
async function command(
59+
input: string[] | { args: string[]; stdin?: string; env?: NodeJS.ProcessEnv },
60+
): Promise<string> {
61+
const {
62+
args,
63+
stdin,
64+
env: overrides,
65+
} = Array.isArray(input) ? { args: input, stdin: undefined, env: undefined } : input;
6066
const process = Bun.spawn(args, {
6167
cwd: directory,
62-
env,
68+
env: { ...env, ...overrides },
6369
stdin: stdin === undefined ? 'ignore' : new Blob([stdin]),
6470
stdout: 'pipe',
6571
stderr: 'pipe',
@@ -189,16 +195,24 @@ config.models={providers:{fixture:{baseUrl:${JSON.stringify(`${model.origin}/v1`
189195
);
190196
// A stale operation record must not mistake this live, unrelated process for its worker.
191197
await writeFile(`${dirname(connectionFile)}.removal.json`, JSON.stringify({ pid: process.pid }));
198+
// npm exec can unpack a local tarball again on every invocation. Isolate its
199+
// caches so this tests competing profile removals rather than npm extraction races.
192200
await Promise.all([
193-
command(['npx', '--yes', packageSpec, 'remove', '--wait']),
194-
command(['npx', '--yes', packageSpec, 'remove', '--wait']),
201+
command({
202+
args: ['npx', '--yes', packageSpec, 'remove', '--wait'],
203+
env: { npm_config_cache: join(directory, 'removal-first-npm-cache') },
204+
}),
205+
command({
206+
args: ['npx', '--yes', packageSpec, 'remove', '--wait'],
207+
env: { npm_config_cache: join(directory, 'removal-second-npm-cache') },
208+
}),
195209
]);
196210
assert(!(await Bun.file(connectionFile).exists()), 'Cancelled setup retained credentials');
197211
assert(!existsSync(dirname(connectionFile)), 'Removal retained its private directory');
198212
await command(['npx', '--yes', packageSpec, 'remove', '--wait']);
199213
assert((await command(['npx', '--yes', packageSpec, 'status'])).includes('"installed": false'));
200214
assert(!existsSync(dirname(connectionFile)), 'Status recreated removed state');
201-
assert(!(await configuration()).plugins.entries?.[PLUGIN_ID]);
215+
assert.deepEqual((await configuration()).plugins.entries?.[PLUGIN_ID], { enabled: false });
202216
await command(['npx', '--yes', packageSpec, 'connect', app.origin]);
203217
const pending = await Bun.file(connectionFile).json();
204218
console.log('Removed an unfinished installation through the npm helper and connected again.');
@@ -427,7 +441,7 @@ config.models={providers:{fixture:{baseUrl:${JSON.stringify(`${model.origin}/v1`
427441
);
428442
await command(['npx', '--yes', packageSpec, 'remove', '--wait']);
429443
const afterRemoval = await configuration();
430-
assert(!afterRemoval.plugins.entries?.[PLUGIN_ID], 'Native uninstall tombstone survived');
444+
assert.deepEqual(afterRemoval.plugins.entries?.[PLUGIN_ID], { enabled: false });
431445
assert.deepEqual(afterRemoval.agents.entries.main.tools.alsoAllow, ['web_search', 'read']);
432446
assert.equal(await Bun.file(join(workspace, 'AGENTS.md')).text(), editedAgents);
433447
assert.equal(
@@ -759,6 +773,46 @@ config.models={providers:{fixture:{baseUrl:${JSON.stringify(`${model.origin}/v1`
759773
'Removal lease survived cleanup',
760774
);
761775
console.log('The npm helper refreshed the gateway after uninstall.');
776+
// Recovery must also work for interrupted installs whose files were removed
777+
// externally, even when this shell cannot authenticate to the running gateway.
778+
await command(['npx', '--yes', packageSpec, 'connect', app.origin]);
779+
const interruptedInventory = JSON.parse(await command(['openclaw', 'plugins', 'list', '--json']));
780+
const interruptedPlugin = interruptedInventory.plugins.find(
781+
(plugin: { id: string }) => plugin.id === PLUGIN_ID,
782+
);
783+
assert(interruptedPlugin, 'Unfinished reinstall did not install the package');
784+
const packageRoot = await realpath(interruptedPlugin.rootDir);
785+
const fixtureRoot = await realpath(directory);
786+
const packagePath = relative(fixtureRoot, packageRoot);
787+
assert(packagePath && !packagePath.startsWith('..') && !isAbsolute(packagePath));
788+
await rm(packageRoot, { recursive: true, force: true });
789+
const invalidGatewayAuth = {
790+
OPENCLAW_GATEWAY_URL: `ws://127.0.0.1:${gatewayPort}`,
791+
OPENCLAW_GATEWAY_TOKEN: 'invalid-removal-test-token',
792+
};
793+
await assert.rejects(
794+
command({
795+
args: ['openclaw', 'gateway', 'call', 'config.get', '--timeout', '3000', '--json'],
796+
env: invalidGatewayAuth,
797+
}),
798+
);
799+
await command({
800+
args: ['npx', '--yes', packageSpec, 'remove', '--wait'],
801+
env: invalidGatewayAuth,
802+
});
803+
const recovered = await configuration();
804+
assert.deepEqual(recovered.plugins.entries?.[PLUGIN_ID], { enabled: false });
805+
assert.notEqual(recovered.plugins.slots?.memory, PLUGIN_ID);
806+
assert(!existsSync(dirname(connectionFile)), 'Orphaned private state survived cleanup');
807+
assert(
808+
!existsSync(`${dirname(connectionFile)}.removal.json`),
809+
'Gateway authorization failure blocked removal',
810+
);
811+
await command(['npx', '--yes', packageSpec, 'remove', '--wait']);
812+
assert((await command(['npx', '--yes', packageSpec, 'status'])).includes('"installed": false'));
813+
console.log(
814+
'Recovered a partially deleted reinstall without gateway authorization, and repeated removal without leftovers.',
815+
);
762816
} finally {
763817
try {
764818
await owner?.close();

‎packages/openclaw-memory/src/connection.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,9 @@ export async function disconnect(directory: string): Promise<{ preserved: string
178178
reason: 'Context Use setup requests gateway refresh after cleanup',
179179
},
180180
// Removing only journal-owned settings can legitimately shrink a fresh config by over half.
181-
writeOptions: { allowConfigSizeDrop: true },
181+
// Restoration must also work after package files and their plugin schema
182+
// have disappeared. The SDK still validates the core configuration.
183+
writeOptions: { allowConfigSizeDrop: true, skipPluginValidation: true },
182184
mutate: (config) => {
183185
if (state) {
184186
preserved = restoreConfiguration({ config, state });

‎packages/openclaw-memory/src/gateway.ts‎

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { resolve } from 'node:path';
2+
import { setTimeout } from 'node:timers/promises';
23
import { z } from 'zod';
34
import { PREPARE_REMOVAL_METHOD } from './contract';
45
import { ConnectionError } from './error';
@@ -13,6 +14,8 @@ const GatewayFailureSchema = z.object({
1314
reason: z.string().optional(),
1415
}),
1516
});
17+
const PROBE_ATTEMPTS = 3;
18+
const PROBE_RETRY_MS = 500;
1619

1720
function gatewayFailure(error: unknown) {
1821
try {
@@ -23,30 +26,46 @@ function gatewayFailure(error: unknown) {
2326
}
2427
}
2528

29+
async function gatewayConfiguration(): Promise<{ path: string } | undefined> {
30+
for (let attempt = 1; ; attempt++) {
31+
try {
32+
return z
33+
.object({ path: z.string() })
34+
.parse(
35+
JSON.parse(
36+
await openclaw(['gateway', 'call', 'config.get', '--timeout', '3000', '--json']),
37+
),
38+
);
39+
} catch (error) {
40+
const failure = gatewayFailure(error);
41+
if (
42+
failure?.type === 'gateway_transport_error' &&
43+
failure.reason?.startsWith('connect ECONNREFUSED ')
44+
) {
45+
return undefined;
46+
}
47+
// A deferred host refresh can briefly close the socket or reject the probe.
48+
// Retry only reads; never replay a cleanup or restart request with an uncertain result.
49+
const transient =
50+
failure?.type === 'gateway_transport_error' ||
51+
(failure?.type === 'gateway_request_error' && failure.code === 'UNAVAILABLE');
52+
if (!transient || attempt >= PROBE_ATTEMPTS) {
53+
throw new ConnectionError(
54+
'Could not verify this profile’s gateway. Resolve its connection or authorization error and retry.',
55+
);
56+
}
57+
await setTimeout(PROBE_RETRY_MS);
58+
}
59+
}
60+
}
61+
2662
async function hasMatchingGateway(): Promise<boolean> {
2763
const local = z
2864
.object({ path: z.string() })
2965
.parse(JSON.parse(await openclaw(['config', 'file', '--json'])));
30-
let remote: { path: string };
31-
try {
32-
remote = z
33-
.object({ path: z.string() })
34-
.parse(
35-
JSON.parse(
36-
await openclaw(['gateway', 'call', 'config.get', '--timeout', '3000', '--json']),
37-
),
38-
);
39-
} catch (error) {
40-
const failure = gatewayFailure(error);
41-
if (
42-
failure?.type === 'gateway_transport_error' &&
43-
failure.reason?.startsWith('connect ECONNREFUSED ')
44-
) {
45-
return false;
46-
}
47-
throw new ConnectionError(
48-
'Could not verify this profile’s gateway. Resolve its connection or authorization error and retry.',
49-
);
66+
const remote = await gatewayConfiguration();
67+
if (!remote) {
68+
return false;
5069
}
5170
// config.get comes from the running gateway itself. Service-status output can
5271
// describe the account's default daemon even when this CLI uses isolated state.
@@ -59,7 +78,16 @@ async function hasMatchingGateway(): Promise<boolean> {
5978
}
6079

6180
export async function prepareGatewayRemoval(): Promise<void> {
62-
if (!(await hasMatchingGateway())) {
81+
let matching: boolean;
82+
try {
83+
matching = await hasMatchingGateway();
84+
} catch {
85+
// Local removal must remain possible when gateway credentials are broken or
86+
// this CLI points at another profile. Never send cleanup to an unverified host.
87+
console.warn('Gateway cleanup could not be reached. Continuing local removal.');
88+
return;
89+
}
90+
if (!matching) {
6391
return;
6492
}
6593
try {
Lines changed: 39 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,61 @@
1-
import { setTimeout } from 'node:timers/promises';
21
import { mutateConfigFile } from 'openclaw/plugin-sdk/config-mutation';
32
import { PLUGIN_ID } from './contract';
43
import { openclaw } from './host-command';
54
import { installedPlugin } from './package-installation';
65

7-
const DRAIN_TIMEOUT_MS = 120_000;
8-
const DRAIN_RETRY_MS = 1_000;
9-
10-
function retainedWorkRejection(error: unknown): boolean {
11-
if (!error || typeof error !== 'object') {
6+
async function alreadyRemoved(error: unknown): Promise<boolean> {
7+
if (!error || typeof error !== 'object' || !('stderr' in error)) {
128
return false;
139
}
14-
const output = ['stdout', 'stderr']
15-
.map((key) => (key in error ? String(error[key as keyof typeof error]) : ''))
16-
.join('\n');
10+
// Host configuration warnings can precede the actual uninstall result.
11+
const messages = String(error.stderr).trim().split('\n');
12+
if (messages.includes(`Plugin not found: ${PLUGIN_ID}`)) {
13+
return true;
14+
}
15+
// Current hosts reject an untracked ID before producing a not-found result.
16+
// An unmanaged package can produce the same rejection, so verify discovery too.
1717
return (
18-
output.includes('still has active retained work') && output.includes('replacement not applied')
18+
messages.some((message) =>
19+
message.startsWith(`Plugin "${PLUGIN_ID}" is not associated with a tracked package install.`),
20+
) && !(await installedPlugin())
1921
);
2022
}
2123

22-
export async function uninstall(): Promise<void> {
23-
if (await installedPlugin()) {
24-
// Older hosts reject retained foreground work before committing. Retry only that
25-
// explicit non-commit result, from the detached remover, until the turn releases it.
26-
const deadline = Date.now() + DRAIN_TIMEOUT_MS;
27-
for (;;) {
28-
try {
29-
console.log(await openclaw(['plugins', 'uninstall', PLUGIN_ID, '--force']));
30-
break;
31-
} catch (error) {
32-
if (Date.now() >= deadline || !retainedWorkRejection(error)) {
33-
throw error;
34-
}
35-
await setTimeout(DRAIN_RETRY_MS);
36-
}
37-
}
38-
}
39-
// OpenClaw retains an enabled:false entry on uninstall. Remove our entry too.
24+
async function writeRemovalEntry(entry: { enabled: false } | undefined): Promise<void> {
4025
await mutateConfigFile({
4126
afterWrite: {
4227
mode: 'none',
4328
reason: 'Context Use setup requests gateway refresh after cleanup',
4429
},
45-
writeOptions: { allowConfigSizeDrop: true },
30+
// Repair only our entry when an orphan install points to a missing manifest.
31+
// The SDK still validates the core configuration.
32+
writeOptions: { allowConfigSizeDrop: true, skipPluginValidation: true },
4633
mutate: (config) => {
47-
if (config.plugins?.entries) {
34+
config.plugins ??= {};
35+
config.plugins.entries ??= {};
36+
if (entry) {
37+
config.plugins.entries[PLUGIN_ID] = entry;
38+
} else {
4839
delete config.plugins.entries[PLUGIN_ID];
4940
}
5041
},
5142
});
5243
}
44+
45+
export async function uninstall(): Promise<void> {
46+
// Even a disabled entry can fail validation while its tracked manifest is missing.
47+
await writeRemovalEntry(undefined);
48+
try {
49+
// OpenClaw owns runtime draining, package files, install records and policy.
50+
console.log(await openclaw(['plugins', 'uninstall', PLUGIN_ID, '--force']));
51+
} catch (error) {
52+
// Discovery excludes missing package files. Still ask the host to remove
53+
// orphaned install records, and accept only its explicit absent result.
54+
if (!(await alreadyRemoved(error))) {
55+
throw error;
56+
}
57+
}
58+
// Keep the host's explicit uninstall choice, including when already absent.
59+
// Startup repair must not automatically reinstall a package the user removed.
60+
await writeRemovalEntry({ enabled: false });
61+
}

0 commit comments

Comments
 (0)