mirror of
https://github.com/nexu-io/open-design.git
synced 2026-09-28 13:33:03 +08:00
fix(web): discard workspace-scoped reads that land after the identity moved
Review follow-up on #6225 (PerishCode, two threads — both real). Keying a read by identity stops the wrong identity being SERVED a cached answer. It does nothing about the other direction: a read issued for identity A resolves later, and by then the caller may be identity B. Committing that late answer restores A's data under B — the same staleness this PR removes, arriving through the back door. A workspace switch is precisely when one read is in flight and another starts, so reverse-order completion is reachable, not exotic. Both sites now use one mechanism, `beginWorkspaceScopedRead`: capture the identity the read is issued FOR, request with that captured context, and compare against a ref before any commit. Its docblock states the invariant and the two rules that make it hold (request with the captured context so request and guard cannot disagree; compare against a ref, never a closed-over prop, which would always match and guard nothing). It is the cross-component form of the `requestEpochRef` ordering guard `useWorkspaceContext` already applies to its own read. - `refreshSkills` and `handleSkillsChanged` (App.tsx): a slow read for the workspace the user left no longer overwrites the current catalog. The registry-ready gate is intentionally skipped on a discarded response — it is not an answer about the current identity, and the newer read marks it. - `loadWorkspaceDirectory` (EntryNavRail.tsx): an in-flight account-A read that lands after the rail moved to account B no longer repopulates either the module cache or the visible list. This was the sharper of the two, because it wrote into module scope and so persisted past the render that caused it. Request counts are unchanged (startup 4, switch 1): the guard discards responses, it does not issue requests.
This commit is contained in:
+19
-4
@@ -87,7 +87,11 @@ import {
|
||||
} from './providers/daemon';
|
||||
import { AMR_LOGIN_STATUS_EVENT } from './components/amrLoginPolling';
|
||||
import { CollabDemoView } from './collab/CollabDemoView';
|
||||
import { useWorkspaceBilling, useWorkspaceContext } from './collab/useWorkspaceContext';
|
||||
import {
|
||||
beginWorkspaceScopedRead,
|
||||
useWorkspaceBilling,
|
||||
useWorkspaceContext,
|
||||
} from './collab/useWorkspaceContext';
|
||||
import { resolvePlanTier } from './collab/team-plan';
|
||||
import { deriveTabIdentityScope, UNSET_ACCOUNT_BUCKET } from './collab/tab-scope';
|
||||
import { CommunityView } from './components/CommunityView';
|
||||
@@ -1763,7 +1767,13 @@ function AppInner() {
|
||||
// headerless read is not the "unfiltered" list — it is the list with every
|
||||
// workspace-claimed skill removed, including the ones claimed by the
|
||||
// workspace the user is actually in.
|
||||
const list = await fetchSkills(workspaceContextRef.current);
|
||||
const read = beginWorkspaceScopedRead(workspaceContextRef.current);
|
||||
const list = await fetchSkills(read.context);
|
||||
// A read for the workspace the user has since LEFT must not restore that
|
||||
// workspace's catalog over the current one — see `beginWorkspaceScopedRead`.
|
||||
// Skipping the gate too is deliberate: this response is not an answer about
|
||||
// the current identity, and the newer read that replaced it will mark it.
|
||||
if (!read.isStillCurrent(workspaceContextRef.current)) return;
|
||||
setSkills(list);
|
||||
markSkillRegistryReady('functional');
|
||||
}, [markSkillRegistryReady]);
|
||||
@@ -2908,8 +2918,13 @@ function AppInner() {
|
||||
|
||||
const handleSkillsChanged = useCallback(
|
||||
(affectedSkillId?: string) => {
|
||||
// Scoped, like every other app-level skills read — see `refreshSkills`.
|
||||
void fetchSkills(workspaceContextRef.current).then((list) => setSkills(list));
|
||||
// Scoped AND guarded on commit, like every other app-level skills read —
|
||||
// see `refreshSkills` and `beginWorkspaceScopedRead`.
|
||||
const skillsRead = beginWorkspaceScopedRead(workspaceContextRef.current);
|
||||
void fetchSkills(skillsRead.context).then((list) => {
|
||||
if (!skillsRead.isStillCurrent(workspaceContextRef.current)) return;
|
||||
setSkills(list);
|
||||
});
|
||||
void fetchDesignTemplates().then((list) => setDesignTemplates(list));
|
||||
iframeKeepAlivePool.evictMatching(
|
||||
(entry) => {
|
||||
|
||||
@@ -114,6 +114,55 @@ export function workspaceIdentityCacheKey(
|
||||
].join(':');
|
||||
}
|
||||
|
||||
/**
|
||||
* One workspace-scoped read: the identity it was issued for, plus the check that
|
||||
* must pass before its response may be committed.
|
||||
*
|
||||
* Keying a request (or its `coalescedGet` entry) by identity stops the WRONG
|
||||
* IDENTITY BEING SERVED an answer fetched for someone else. It does nothing
|
||||
* about the other direction: a read issued for identity A resolves later, and by
|
||||
* then the caller may be identity B. Committing that late answer restores A's
|
||||
* data under B — the exact staleness the identity keys exist to prevent,
|
||||
* arriving through the back door. Reverse-order completion is not exotic here;
|
||||
* a workspace switch is precisely when one read is in flight and another starts.
|
||||
*
|
||||
* So every workspace-scoped read follows the same four steps:
|
||||
*
|
||||
* const read = beginWorkspaceScopedRead(contextRef.current);
|
||||
* const data = await fetchSomething(read.context);
|
||||
* if (!read.isStillCurrent(contextRef.current)) return; // ← the invariant
|
||||
* commit(data);
|
||||
*
|
||||
* Two rules make it actually hold:
|
||||
*
|
||||
* 1. Request with `read.context`, never with the caller's own variable, so the
|
||||
* request and the guard can never disagree about whose data was asked for.
|
||||
* 2. Compare against a REF, never a closed-over prop or state value. A closure
|
||||
* captures the identity the read was issued for, so comparing against it
|
||||
* always succeeds and guards nothing.
|
||||
*
|
||||
* This is the cross-component form of the `requestEpochRef` ordering guard
|
||||
* `useWorkspaceContext` already applies to its own read; identity is the right
|
||||
* discriminator for reads that are scoped BY identity.
|
||||
*/
|
||||
export interface WorkspaceScopedRead {
|
||||
/** The context to send with the request — see rule 1 above. */
|
||||
readonly context: WorkspaceCollabContext | null;
|
||||
/** Whether `current` is still the identity this read was issued for. */
|
||||
isStillCurrent(current: WorkspaceCollabContext | null | undefined): boolean;
|
||||
}
|
||||
|
||||
export function beginWorkspaceScopedRead(
|
||||
context: WorkspaceCollabContext | null | undefined,
|
||||
): WorkspaceScopedRead {
|
||||
const issuedFor = context ?? null;
|
||||
const identity = workspaceIdentityCacheKey(issuedFor);
|
||||
return {
|
||||
context: issuedFor,
|
||||
isStillCurrent: (current) => workspaceIdentityCacheKey(current ?? null) === identity,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* `GET /api/workspace/context` is the read that ESTABLISHES the caller's
|
||||
* identity, so — unlike every other workspace read — it cannot be keyed on the
|
||||
|
||||
@@ -58,6 +58,7 @@ import type { EntrySettingsSection } from './EntrySettingsMenu';
|
||||
import { useI18n } from '../i18n';
|
||||
import { useDismissOnOutsideInteraction } from '../hooks/useDismissOnOutsideInteraction';
|
||||
import {
|
||||
beginWorkspaceScopedRead,
|
||||
notifyTeamProjectsChanged,
|
||||
notifyWorkspaceBillingRefresh,
|
||||
notifyWorkspaceContextRefresh,
|
||||
@@ -681,6 +682,11 @@ export function EntryNavRail({
|
||||
setAccountOpen(false);
|
||||
});
|
||||
const [teamOpen, setTeamOpen] = useState(false);
|
||||
// The LATEST context, for async work to compare against. `loadWorkspaceDirectory`
|
||||
// closes over the render's `context` prop, which is the identity its read was
|
||||
// issued for — so only a ref can answer "has the identity moved since?".
|
||||
const contextRef = useRef(context);
|
||||
contextRef.current = context;
|
||||
const [workspaceItems, setWorkspaceItems] = useState<WorkspaceDirectoryItem[]>(
|
||||
() => attributableWorkspaceDirectory(context) ?? [],
|
||||
);
|
||||
@@ -748,28 +754,40 @@ export function EntryNavRail({
|
||||
: [];
|
||||
|
||||
async function loadWorkspaceDirectory() {
|
||||
// Capture the identity this read is FOR, and compare against `contextRef`
|
||||
// (not the closed-over `context`, which is by definition the identity we are
|
||||
// reading for) before committing anything — see `beginWorkspaceScopedRead`.
|
||||
const read = beginWorkspaceScopedRead(contextRef.current);
|
||||
// Only show the loading row when there is nothing to show yet. With a warm
|
||||
// cache the list is already on screen and this read just revalidates it —
|
||||
// but a cache belonging to another account counts as nothing to show.
|
||||
if (attributableWorkspaceDirectory(context) === null) setWorkspaceDirectoryLoading(true);
|
||||
if (attributableWorkspaceDirectory(read.context) === null) {
|
||||
setWorkspaceDirectoryLoading(true);
|
||||
}
|
||||
try {
|
||||
// The coalescing key carries the caller's identity for the same reason the
|
||||
// module cache does: `coalescedGet` shares a settled result for a second,
|
||||
// and this read's answer depends on WHO asked.
|
||||
const cacheKey = `workspace-directory:${workspaceIdentityCacheKey(context)}`;
|
||||
const cacheKey = `workspace-directory:${workspaceIdentityCacheKey(read.context)}`;
|
||||
const items = await coalescedGet(cacheKey, async () => {
|
||||
const response = await fetch('/api/workspace/directory', { cache: 'no-store' });
|
||||
if (!response.ok) throw new Error(`directory ${response.status}`);
|
||||
const body = (await response.json()) as WorkspaceDirectoryResponse;
|
||||
return body.items ?? [];
|
||||
});
|
||||
// The account may have changed while this was in flight. Writing here
|
||||
// would repopulate BOTH the module cache and the visible list with the
|
||||
// previous account's names, after the identity-change effect below had
|
||||
// already cleared them — so an abandoned read must leave no trace.
|
||||
if (!read.isStillCurrent(contextRef.current)) return;
|
||||
cachedWorkspaceDirectory = items;
|
||||
setWorkspaceItems(items);
|
||||
} catch {
|
||||
// A failed revalidation must not blank a list the user is looking at —
|
||||
// keep the last known names and let the next open try again. A list this
|
||||
// caller has no claim to is not "a list the user is looking at".
|
||||
if (attributableWorkspaceDirectory(context) === null) setWorkspaceItems([]);
|
||||
if (!read.isStillCurrent(contextRef.current)) return;
|
||||
if (attributableWorkspaceDirectory(read.context) === null) setWorkspaceItems([]);
|
||||
} finally {
|
||||
setWorkspaceDirectoryLoading(false);
|
||||
}
|
||||
|
||||
@@ -17,6 +17,7 @@
|
||||
// so `route.kind` stays 'home' and no route change fires. Skills never did.
|
||||
|
||||
import { act, cleanup, render, screen, waitFor } from '@testing-library/react';
|
||||
import type { SkillSummary } from '@open-design/contracts';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import { App } from '../../src/App';
|
||||
@@ -45,7 +46,14 @@ import {
|
||||
import { resetCoalescedGet } from '../../src/lib/coalesced-get';
|
||||
|
||||
vi.mock('../../src/components/EntryView', () => ({
|
||||
EntryView: () => <main><div data-testid="entry-home-surface" /></main>,
|
||||
EntryView: ({ skills }: { skills: Array<{ id: string }> }) => (
|
||||
<main>
|
||||
<div data-testid="entry-home-surface" />
|
||||
{skills.map((skill) => (
|
||||
<div key={skill.id} data-testid={`entry-skill-${skill.id}`} />
|
||||
))}
|
||||
</main>
|
||||
),
|
||||
}));
|
||||
|
||||
vi.mock('../../src/components/ProjectView', () => ({
|
||||
@@ -172,6 +180,25 @@ function skillsReadScopes(): Array<string | undefined> {
|
||||
|
||||
const projects: Project[] = [];
|
||||
|
||||
function skill(id: string): SkillSummary {
|
||||
return {
|
||||
id,
|
||||
name: id,
|
||||
description: id,
|
||||
triggers: [],
|
||||
mode: 'prototype',
|
||||
source: 'user',
|
||||
} as unknown as SkillSummary;
|
||||
}
|
||||
|
||||
function deferred<T>() {
|
||||
let resolve!: (value: T) => void;
|
||||
const promise = new Promise<T>((res) => {
|
||||
resolve = res;
|
||||
});
|
||||
return { promise, resolve };
|
||||
}
|
||||
|
||||
describe('App skills list — workspace scope', () => {
|
||||
beforeEach(() => {
|
||||
resetWorkspaceContextCache();
|
||||
@@ -241,4 +268,59 @@ describe('App skills list — workspace scope', () => {
|
||||
await waitFor(() => expect(skillsReadScopes()).toContain('ws-b'));
|
||||
expect(skillsReadScopes()).toEqual(['ws-a', 'ws-b']);
|
||||
});
|
||||
|
||||
// Issuing the right request is only half the guarantee. Each read resolves
|
||||
// later, and nothing stopped a read issued FOR the workspace the user has
|
||||
// since left from committing when it finally landed — restoring that
|
||||
// workspace's catalog over the current one, which is the very staleness this
|
||||
// change exists to remove, arriving through the back door.
|
||||
it("discards a slow read belonging to the workspace the user has left", async () => {
|
||||
let activeWorkspaceId = 'ws-a';
|
||||
vi.stubGlobal(
|
||||
'fetch',
|
||||
vi.fn(async (input: RequestInfo | URL) => {
|
||||
const pathname = new URL(String(input), 'http://d.local').pathname;
|
||||
return {
|
||||
ok: true,
|
||||
json: async () =>
|
||||
pathname.endsWith('/workspace/context')
|
||||
? workspaceContextPayload(activeWorkspaceId)
|
||||
: {},
|
||||
} as Response;
|
||||
}),
|
||||
);
|
||||
|
||||
const readA = deferred<SkillSummary[]>();
|
||||
const readB = deferred<SkillSummary[]>();
|
||||
vi.mocked(fetchSkills).mockImplementation((context) =>
|
||||
context?.workspaceId === 'ws-b' ? readB.promise : readA.promise,
|
||||
);
|
||||
|
||||
render(<App />);
|
||||
await waitFor(() => expect(skillsReadScopes()).toContain('ws-a'));
|
||||
|
||||
// Switch while ws-a's read is still in flight.
|
||||
activeWorkspaceId = 'ws-b';
|
||||
await act(async () => {
|
||||
notifyWorkspaceContextRefresh();
|
||||
await Promise.resolve();
|
||||
});
|
||||
await waitFor(() => expect(skillsReadScopes()).toContain('ws-b'));
|
||||
|
||||
// Reverse order: the workspace the user is actually IN answers first…
|
||||
await act(async () => {
|
||||
readB.resolve([skill('skill-from-b')]);
|
||||
await Promise.resolve();
|
||||
});
|
||||
await waitFor(() => expect(screen.getByTestId('entry-skill-skill-from-b')).toBeTruthy());
|
||||
|
||||
// …and the abandoned workspace answers second. It must change nothing.
|
||||
await act(async () => {
|
||||
readA.resolve([skill('skill-from-a')]);
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
expect(screen.getByTestId('entry-skill-skill-from-b')).toBeTruthy();
|
||||
expect(screen.queryByTestId('entry-skill-skill-from-a')).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -16,7 +16,7 @@
|
||||
// ACCOUNT see", so it is exactly the class of read `workspaceIdentityCacheKey`
|
||||
// warns about: a cache coarser than the identity of the request it holds.
|
||||
|
||||
import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react';
|
||||
import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react';
|
||||
import type { WorkspaceCollabContext } from '@open-design/contracts';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
@@ -176,6 +176,51 @@ describe('workspace switcher directory — account scope', () => {
|
||||
expect(menu().queryByText('Ada private workspace')).toBeNull();
|
||||
});
|
||||
|
||||
// The identity-scoped cache key stops a cached answer being SERVED to the
|
||||
// wrong identity. It does nothing about a request already in flight when the
|
||||
// identity moves: that response lands afterwards and used to be written
|
||||
// straight into both the module cache and component state, repopulating
|
||||
// account A's names after the identity-change effect had cleared them.
|
||||
it("discards an in-flight read that lands after the account changed", async () => {
|
||||
const gate = installGatedFetch(() => ACCOUNT_A_DIRECTORY);
|
||||
|
||||
const view = renderRail(contextFor('wm-a-team'));
|
||||
fireEvent.click(screen.getByTestId('workspace-switcher'));
|
||||
|
||||
// A's read is in flight and deliberately NOT released yet. Swap the account
|
||||
// underneath the mounted rail (sign out, sign in as someone else) — no
|
||||
// unmount, which is what leaves the pending request pointing at A.
|
||||
view.rerender(
|
||||
<I18nProvider initial="en">
|
||||
<EntryNavRail
|
||||
view="home"
|
||||
onViewChange={() => {}}
|
||||
onNewProject={() => {}}
|
||||
open
|
||||
context={contextFor('wm-b-team')}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
|
||||
// Now let A's request answer, after the identity has already moved.
|
||||
await act(async () => {
|
||||
gate.releaseAll();
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
// Component state must not have taken A's names.
|
||||
expect(menu().queryByText('Ada private workspace')).toBeNull();
|
||||
|
||||
// …and the module cache must not have taken them either. Observed by
|
||||
// remounting as A with a read that never answers: an abandoned response
|
||||
// leaves no trace, so there is no warm list to serve even to A.
|
||||
view.unmount();
|
||||
installGatedFetch(() => ACCOUNT_A_DIRECTORY);
|
||||
renderRail(contextFor('wm-a-team'));
|
||||
fireEvent.click(screen.getByTestId('workspace-switcher'));
|
||||
expect(menu().queryByText('Ada private workspace')).toBeNull();
|
||||
});
|
||||
|
||||
it('still serves the warm list across a same-account workspace switch', async () => {
|
||||
const gate = installGatedFetch(() => ACCOUNT_A_DIRECTORY);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user