Skip to content

Commit 53a0f82

Browse files
fix(namespaces): deterministic catalog storageFor test (kill setTimeout flake) (#1564)
The "StorageRouter integration: catalog registers namespace on storageFor" test relied on `await new Promise((r) => setTimeout(r, 10))` to let a fire-and-forget `catalog.registerResolved(...)` — fired from the router's `onResolve` hook inside `storageFor()` — settle before asserting the record exists. Under CI contention 10ms was not always enough, so the assertion intermittently failed (observed on an unrelated PR's quality job, passing on rerun). Add `NamespaceStorageRouter.whenResolveHooksSettled()`: the router now tracks in-flight resolve-hook promises in a `pendingResolveHooks` set (added before awaiting, removed when each settles — same lifecycle as `inFlightResolved`, so it cannot grow unbounded). Callers can await it to observe the fire-and-forget registrations `storageFor()` kicks off, deterministically, with no timer. Replace the sleep in the target test and 4 sibling router-dedup tests with `await router.whenResolveHooksSettled()`. Additive-only; no change to storage resolution behavior. Left unrelated setTimeout uses alone: the lock-release timing test and the bounded `Promise.race` deadlock guards / interleaving seam settles. Verified: 16 clean runs (10 sequential + 6 parallel under 8 saturated CPU cores), 87/87 each; `tsc --noEmit`, preflight:quick, and entity-hardening (245/245) all green. Independent of the #1546 category-dir work (PR #1563).
1 parent 41fff8c commit 53a0f82

2 files changed

Lines changed: 40 additions & 13 deletions

File tree

packages/remnic-core/src/namespaces/catalog.test.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -487,13 +487,13 @@ test("StorageRouter integration: catalog registers namespace on storageFor", asy
487487
const config = makeConfig(memoryDir);
488488
const catalog = new NamespaceCatalog(config);
489489
const router = new NamespaceStorageRouter(config, {
490-
onResolve: (namespace, storageDir) => {
491-
void catalog.registerResolved(namespace, storageDir);
492-
},
490+
// Return the registration promise so the router tracks it as in-flight and
491+
// `whenResolveHooksSettled()` can await it deterministically (no timer race).
492+
onResolve: (namespace, storageDir) => catalog.registerResolved(namespace, storageDir),
493493
});
494494
await router.storageFor("project-origin-abc123");
495-
// allow the fire-and-forget registration to settle
496-
await new Promise((r) => setTimeout(r, 10));
495+
// Deterministically await the fire-and-forget registration instead of sleeping.
496+
await router.whenResolveHooksSettled();
497497

498498
const record = await catalog.getNamespaceRecord("project-origin-abc123");
499499
assert.ok(record, "storageFor should have registered the namespace");
@@ -2523,8 +2523,8 @@ test("an async onResolve hook rejection does not crash storage resolution", asyn
25232523
// Must not throw or produce an unhandled rejection that fails the test.
25242524
const sm = await router.storageFor("default");
25252525
assert.ok(sm, "storage resolution succeeds despite a rejecting async hook");
2526-
// Give the swallowed rejection a tick to settle.
2527-
await new Promise((r) => setTimeout(r, 10));
2526+
// Deterministically await the swallowed rejection instead of sleeping.
2527+
await router.whenResolveHooksSettled();
25282528
assert.ok(called >= 1, "the async hook was invoked");
25292529
} finally {
25302530
await rm(memoryDir, { recursive: true, force: true });
@@ -2564,7 +2564,7 @@ test("concurrent storageFor() for one namespace fires the resolve hook ONCE whil
25642564
// Let the in-flight registration settle, then a steady-state cache hit must
25652565
// still be a catalog no-op (now deduped via notifiedResolved).
25662566
release();
2567-
await new Promise((r) => setTimeout(r, 10));
2567+
await router.whenResolveHooksSettled();
25682568
await router.storageFor("project-origin-inflight");
25692569
assert.equal(calls, 1, "a steady-state cache hit after settle must not re-fire the hook");
25702570
} finally {
@@ -2589,20 +2589,20 @@ test("a dropped resolve registration (hook returns false) is retried on a later
25892589
});
25902590

25912591
await router.storageFor("project-origin-retry");
2592-
// Give the async hook a tick to settle and clear the in-flight marker.
2593-
await new Promise((r) => setTimeout(r, 10));
2592+
// Deterministically await the async hook so the in-flight marker is cleared.
2593+
await router.whenResolveHooksSettled();
25942594
assert.equal(calls, 1, "the hook fired once for the dropped registration");
25952595

25962596
// Now the registration will succeed; a later resolve must RETRY (not be
25972597
// suppressed by a stale in-flight/notified marker from the dropped attempt).
25982598
result = undefined; // success (legacy void)
25992599
await router.storageFor("project-origin-retry");
2600-
await new Promise((r) => setTimeout(r, 10));
2600+
await router.whenResolveHooksSettled();
26012601
assert.equal(calls, 2, "a dropped registration must be retried on the next storageFor()");
26022602

26032603
// After a SUCCESSFUL registration, further cache hits are deduped (no retry).
26042604
await router.storageFor("project-origin-retry");
2605-
await new Promise((r) => setTimeout(r, 10));
2605+
await router.whenResolveHooksSettled();
26062606
assert.equal(calls, 2, "a successful registration is not re-fired on subsequent cache hits");
26072607
} finally {
26082608
await rm(memoryDir, { recursive: true, force: true });

packages/remnic-core/src/namespaces/storage.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,11 @@ export class NamespaceStorageRouter {
223223
// entry is always removed when the promise settles, so the map cannot grow
224224
// unbounded (one transient entry per concurrently-resolving namespace).
225225
private readonly inFlightResolved = new Map<string, string>();
226+
// Tracks every in-flight resolve-hook promise so callers can deterministically
227+
// await the fire-and-forget registrations that `storageFor()` kicks off (see
228+
// `whenResolveHooksSettled`). Entries are removed as each hook settles, so the
229+
// set holds at most one promise per concurrently-resolving namespace.
230+
private readonly pendingResolveHooks = new Set<Promise<unknown>>();
226231

227232
// Normalized (trimmed) default namespace identity (NH-FH). `storageFor`
228233
// normalizes its input, so default-namespace branches must compare against the
@@ -325,7 +330,10 @@ export class NamespaceStorageRouter {
325330
// the map holds at most one transient entry per concurrently-resolving
326331
// namespace and cannot grow unbounded.
327332
this.inFlightResolved.set(namespace, storageDir);
328-
Promise.resolve(hook(namespace, storageDir)).then(
333+
const hookResult = Promise.resolve(hook(namespace, storageDir));
334+
// Track the in-flight promise so `whenResolveHooksSettled()` can await it.
335+
this.pendingResolveHooks.add(hookResult);
336+
hookResult.then(
329337
(persisted) => {
330338
// Clear the in-flight marker ONLY if it is still ours (a newer resolve
331339
// for a different dir may have replaced it).
@@ -338,6 +346,7 @@ export class NamespaceStorageRouter {
338346
// On `false` (dropped touch) we intentionally do NOT mark notified, so
339347
// a later `storageFor()` retries the registration. Clearing the
340348
// in-flight marker above is what re-enables that retry.
349+
this.pendingResolveHooks.delete(hookResult);
341350
},
342351
() => {
343352
// Registration failed — clear in-flight AND do NOT mark as notified, so
@@ -348,6 +357,7 @@ export class NamespaceStorageRouter {
348357
if (this.notifiedResolved.get(namespace) === storageDir) {
349358
this.notifiedResolved.delete(namespace);
350359
}
360+
this.pendingResolveHooks.delete(hookResult);
351361
},
352362
);
353363
} catch {
@@ -358,4 +368,21 @@ export class NamespaceStorageRouter {
358368
}
359369
}
360370
}
371+
372+
/**
373+
* Resolve once every in-flight `onResolve` registration has settled.
374+
*
375+
* `storageFor()` fires the resolve hook fire-and-forget, so its catalog side
376+
* effect (e.g. `registerResolved(...)`) is not observable the moment
377+
* `storageFor()` returns. Callers that must act on that side effect — notably
378+
* tests asserting the catalog was updated — should await this instead of
379+
* racing a timer. Resolves immediately when no hook is registered or nothing
380+
* is in flight. The loop re-checks because a settling hook could, in
381+
* principle, trigger a follow-on resolution.
382+
*/
383+
async whenResolveHooksSettled(): Promise<void> {
384+
while (this.pendingResolveHooks.size > 0) {
385+
await Promise.allSettled([...this.pendingResolveHooks]);
386+
}
387+
}
361388
}

0 commit comments

Comments
 (0)