Skip to content

Commit 1b4d32d

Browse files
sirtimidclaude
andcommitted
fix(ocap-kernel): address PR review — avoid mutation, fix this binding, add tests
- Avoid direct mutation of potentially-hardened entries in initRemoteIdentity bootstrap clearing loop (use spread instead) - Extract getRelayEntries as local function in makeKernelStore to avoid fragile this binding in getKnownRelayAddresses - Add test: init clears bootstrap flag on relays removed from bootstrap set - Add test: init enforces MAX_KNOWN_RELAYS pool cap Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent fe5495a commit 1b4d32d

3 files changed

Lines changed: 89 additions & 43 deletions

File tree

packages/ocap-kernel/src/remotes/kernel/remote-comms.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,49 @@ describe('remote-comms', () => {
591591
expect(secondSeen).toBeGreaterThan(firstSeen);
592592
});
593593

594+
it('init clears bootstrap flag on relays removed from the bootstrap set', async () => {
595+
await initRemoteIdentity(mockKernelStore, {
596+
relays: ['/dns4/relayA.example/tcp/443/wss/p2p-circuit'],
597+
});
598+
599+
// Re-init with a different bootstrap set
600+
await initRemoteIdentity(mockKernelStore, {
601+
relays: ['/dns4/relayB.example/tcp/443/wss/p2p-circuit'],
602+
});
603+
604+
const entries = mockKernelStore.getRelayEntries();
605+
expect(
606+
entries.find((entry) => entry.addr.includes('relayA'))?.isBootstrap,
607+
).toBe(false);
608+
expect(
609+
entries.find((entry) => entry.addr.includes('relayB'))?.isBootstrap,
610+
).toBe(true);
611+
});
612+
613+
it('init enforces MAX_KNOWN_RELAYS pool cap', async () => {
614+
// Pre-seed MAX_KNOWN_RELAYS learned relays
615+
mockKernelStore.setRelayEntries(
616+
Array.from({ length: MAX_KNOWN_RELAYS }, (_, i) => ({
617+
addr: `/dns4/learned${i}.example/tcp/443/wss/p2p-circuit`,
618+
lastSeen: 100,
619+
isBootstrap: false,
620+
})),
621+
);
622+
623+
const bootstrapRelays = Array.from(
624+
{ length: 5 },
625+
(_, i) => `/dns4/bootstrap${i}.example/tcp/443/wss/p2p-circuit`,
626+
);
627+
await initRemoteIdentity(mockKernelStore, { relays: bootstrapRelays });
628+
629+
const entries = mockKernelStore.getRelayEntries();
630+
expect(entries).toHaveLength(MAX_KNOWN_RELAYS);
631+
// All bootstrap relays must survive
632+
for (const addr of bootstrapRelays) {
633+
expect(entries.some((entry) => entry.addr === addr)).toBe(true);
634+
}
635+
});
636+
594637
it('init marks bootstrap relays and preserves learned relays', async () => {
595638
// Pre-seed a learned relay
596639
mockKernelStore.setRelayEntries([

packages/ocap-kernel/src/remotes/kernel/remote-comms.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,10 @@ export async function initRemoteIdentity(
124124
byAddr.set(addr, { addr, lastSeen: now, isBootstrap: true });
125125
}
126126
// Clear bootstrap flag on entries no longer in the current bootstrap set
127-
for (const entry of byAddr.values()) {
128-
if (entry.isBootstrap && !bootstrapSet.has(entry.addr)) {
129-
entry.isBootstrap = false;
127+
// (create new objects to avoid mutating potentially-hardened entries)
128+
for (const [addr, entry] of byAddr.entries()) {
129+
if (entry.isBootstrap && !bootstrapSet.has(addr)) {
130+
byAddr.set(addr, { ...entry, isBootstrap: false });
130131
}
131132
}
132133
let merged = [...byAddr.values()];

packages/ocap-kernel/src/store/index.ts

Lines changed: 42 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,46 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) {
286286
kdb.rollbackSavepoint(name);
287287
}
288288

289+
/**
290+
* Read relay entries from storage, auto-migrating from the legacy
291+
* `string[]` format if necessary.
292+
*
293+
* @returns The relay entries.
294+
*/
295+
function getRelayEntries(): RelayEntry[] {
296+
const raw = kv.get('knownRelays');
297+
if (!raw) {
298+
return [];
299+
}
300+
const parsed: unknown = JSON.parse(raw);
301+
Array.isArray(parsed) || Fail`knownRelays must be an array`;
302+
303+
// Migrate legacy string[] format → RelayEntry[] (persisted back to storage)
304+
if (parsed.length > 0 && typeof parsed[0] === 'string') {
305+
if (!parsed.every((entry: unknown) => typeof entry === 'string')) {
306+
Fail`knownRelays legacy format must be all strings`;
307+
}
308+
const migrated: RelayEntry[] = (parsed as string[]).map((addr) => ({
309+
addr,
310+
lastSeen: 0,
311+
isBootstrap: false,
312+
}));
313+
kv.set('knownRelays', JSON.stringify(migrated));
314+
return migrated;
315+
}
316+
317+
// New RelayEntry[] format
318+
for (const entry of parsed) {
319+
(typeof entry === 'object' &&
320+
entry !== null &&
321+
typeof (entry as RelayEntry).addr === 'string' &&
322+
typeof (entry as RelayEntry).lastSeen === 'number' &&
323+
typeof (entry as RelayEntry).isBootstrap === 'boolean') ||
324+
Fail`knownRelays entries must have addr, lastSeen, isBootstrap`;
325+
}
326+
return parsed as RelayEntry[];
327+
}
328+
289329
return harden({
290330
...id,
291331
...queue,
@@ -350,45 +390,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) {
350390
): void {
351391
kv.set(key, value);
352392
},
353-
/**
354-
* Read relay entries from storage, auto-migrating from the legacy
355-
* `string[]` format if necessary.
356-
*
357-
* @returns The relay entries.
358-
*/
359-
getRelayEntries(): RelayEntry[] {
360-
const raw = kv.get('knownRelays');
361-
if (!raw) {
362-
return [];
363-
}
364-
const parsed: unknown = JSON.parse(raw);
365-
Array.isArray(parsed) || Fail`knownRelays must be an array`;
366-
367-
// Migrate legacy string[] format → RelayEntry[]
368-
if (parsed.length > 0 && typeof parsed[0] === 'string') {
369-
if (!parsed.every((entry: unknown) => typeof entry === 'string')) {
370-
Fail`knownRelays legacy format must be all strings`;
371-
}
372-
const migrated: RelayEntry[] = (parsed as string[]).map((addr) => ({
373-
addr,
374-
lastSeen: 0,
375-
isBootstrap: false,
376-
}));
377-
kv.set('knownRelays', JSON.stringify(migrated));
378-
return migrated;
379-
}
380-
381-
// New RelayEntry[] format
382-
for (const entry of parsed) {
383-
(typeof entry === 'object' &&
384-
entry !== null &&
385-
typeof (entry as RelayEntry).addr === 'string' &&
386-
typeof (entry as RelayEntry).lastSeen === 'number' &&
387-
typeof (entry as RelayEntry).isBootstrap === 'boolean') ||
388-
Fail`knownRelays entries must have addr, lastSeen, isBootstrap`;
389-
}
390-
return parsed as RelayEntry[];
391-
},
393+
getRelayEntries,
392394

393395
/**
394396
* Persist relay entries to storage.
@@ -405,7 +407,7 @@ export function makeKernelStore(kdb: KernelDatabase, logger?: Logger) {
405407
* @returns The relay addresses.
406408
*/
407409
getKnownRelayAddresses(): string[] {
408-
return this.getRelayEntries().map((entry) => entry.addr);
410+
return getRelayEntries().map((entry) => entry.addr);
409411
},
410412
});
411413
}

0 commit comments

Comments
 (0)