modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix(web): order overlapping settings writes by issue, not by completion

Browse source 
Review round 2 on #2091. Recording on completion made the *last to
finish* the winner, and `updateServerSettings` waits for a list read
after its PUT — so an older write with a slow read finishes last and
would overwrite the record with a value a newer write had already
replaced on disk. Both writers can overlap: two quick toggles, or the
settings modal's debounced flush.

A write is now announced with `begin` when it is issued and confirmed
with `landed` when it completes, and a confirmation is dropped if a
later-issued write for that server has already confirmed. The high-water
mark is kept apart from the record so a straggler cannot resurrect a
record a fresh list read dropped.

Issue order is a proxy for disk order rather than a proof of it — HTTP
promises nothing about two in-flight PUTs — but it is the best signal on
this side until the servers route returns a revision to order by. Noted
on the hook alongside the other consequence of that gap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 11:34:20 -04:00
commit 7fa42299c7
3 changed files with 121 additions and 50 deletions
Download patch file Download diff file Expand all files Collapse all files

18
clients/web/src/App.tsx
View file

@ -3769,6 +3769,10 @@ function App() {
// server's rehydrated secrets, so it can trigger the pending
// plaintext-to-encrypted upgrade even though the user only toggled
// pagination (#1950 review r22).
// Announced before the request goes out, so two toggles in flight at once
// are ordered by when they were issued rather than by which one's list
// reload finished first (#2089).
const write = lastPersistedSettings.begin(activeServerId);
void refreshingPersist(updateServerSettings, refreshInitialConfig)(
activeServerId,
next,
@ -3778,7 +3782,7 @@ function App() {
// whatever is written next, since the `servers` entry it was derived
// from will keep describing the old value if the reload behind this
// write — or any later one — fails (#2089).
lastPersistedSettings.record(activeServerId, next);
write.landed(next);
})
.catch((err: unknown) => {
// Persist failed: revert the optimistic override and roll the live
@ -4168,17 +4172,21 @@ function App() {
// outside the coverage gate, so wiring written here is tested by nothing
// (#1950 review r17).
onPersist: async (id: string, value: InspectorServerSettings) => {
// Announced before the request, like the toggle's own write: the debounced
// flush can put two saves for one server in flight, and the later-issued
// one describes disk however the two happen to finish (#2089).
const write = lastPersistedSettings.begin(id);
await refreshingPersist(updateServerSettings, refreshInitialConfig)(
id,
value,
);
// Recorded for the same reason the pagination toggle records its own
// write (#2089): this is what disk holds now, and the `servers` entry it
// was edited from will keep describing the previous value for as long as
// list reads keep failing. Both settings writers feed the same per-server
// write: this is what disk holds now, and the `servers` entry it was
// edited from will keep describing the previous value for as long as list
// reads keep failing. Both settings writers feed the same per-server
// record, so a rollback in either takes the most recent write for that
// server, not just the most recent write *of its own kind*.
lastPersistedSettings.record(id, value);
write.landed(value);
},
// Surface failures via toast — the modal usually closes
// immediately on user dismiss, so a silent fail-on-flush would

55
clients/web/src/hooks/useLastPersistedSettings.test.tsx
View file

@ -76,13 +76,13 @@ describe("useLastPersistedSettings", () => {
it("returns the recorded write while the entry has not been re-read", () => {
const { api } = harness([entry("A")]);
const written = settings({ paginatedLists: true });
api.record("A", written);
api.begin("A").landed(written);
expect(api.resolve("A", settings())).toBe(written);
});
it("ignores a record made for a different server", () => {
const { api } = harness([entry("A"), entry("B")]);
api.record("A", settings({ paginatedLists: true }));
api.begin("A").landed(settings({ paginatedLists: true }));
const fallback = settings();
expect(api.resolve("B", fallback)).toBe(fallback);
});
@ -94,14 +94,14 @@ describe("useLastPersistedSettings", () => {
// slot would have lost A's baseline and reproduced #2089.
const { api } = harness([entry("A"), entry("B")]);
const writtenA = settings({ paginatedLists: true });
api.record("A", writtenA);
api.record("B", settings({ autoRefreshOnListChanged: true }));
api.begin("A").landed(writtenA);
api.begin("B").landed(settings({ autoRefreshOnListChanged: true }));
expect(api.resolve("A", settings())).toBe(writtenA);
});
it("stops trusting the record once a fresh list read replaces the entry", () => {
const { api, setServers } = harness([entry("A")]);
api.record("A", settings({ paginatedLists: true }));
api.begin("A").landed(settings({ paginatedLists: true }));
// A successful `GET /api/servers` rebuilds the list, so the entry is a new
// object even when its values are unchanged. That is the signal the list
// has caught up, and it is authoritative again from here.
@ -110,29 +110,56 @@ describe("useLastPersistedSettings", () => {
expect(api.resolve("A", fresh)).toBe(fresh);
});
it("pairs the record with the entry as of the write, not of the render", () => {
// A list read that lands between a write being issued and completing must
// not be mistaken for one that happened after it. `record` is called on
// completion and pairs with the list as it stands then, so the read below
// — which precedes the write — does not invalidate the record.
it("pairs the record with the entry as of completion, not of issue", () => {
// A list read that lands while the write is in flight must not be mistaken
// for one that happened after it. The pairing is taken in `landed`, so the
// read below — which the write outlives — does not invalidate the record.
const { api, setServers } = harness([entry("A")]);
const write = api.begin("A");
setServers([entry("A")]);
const written = settings({ paginatedLists: true });
api.record("A", written);
write.landed(written);
expect(api.resolve("A", settings())).toBe(written);
});
it("keeps only the most recent write for a server", () => {
const { api } = harness([entry("A")]);
api.record("A", settings({ paginatedLists: true }));
api.begin("A").landed(settings({ paginatedLists: true }));
const later = settings({ autoRefreshOnListChanged: true });
api.record("A", later);
api.begin("A").landed(later);
expect(api.resolve("A", settings())).toBe(later);
});
it("lets the later-issued write win however the two finish", () => {
// `updateServerSettings` waits for a list read after its PUT, so an older
// write with a slow read can complete last. Ordering by completion would
// then record a value that a newer write has already replaced on disk, and
// the next failed write would roll back to it.
const { api } = harness([entry("A")]);
const first = api.begin("A");
const second = api.begin("A");
const newest = settings({ paginatedLists: true });
second.landed(newest);
first.landed(settings({ paginatedLists: false }));
expect(api.resolve("A", settings())).toBe(newest);
});
it("does not let a straggler resurrect a record a fresh read dropped", () => {
// The high-water mark outlives the record, so an earlier write reporting
// after `resolve` invalidated things cannot reinstate a superseded value.
const { api, setServers } = harness([entry("A")]);
const first = api.begin("A");
api.begin("A").landed(settings({ paginatedLists: true }));
const fresh = settings();
setServers([entry("A", fresh)]);
expect(api.resolve("A", fresh)).toBe(fresh);
first.landed(settings({ paginatedLists: false }));
expect(api.resolve("A", fresh)).toBe(fresh);
});
it("falls back when the server has left the list entirely", () => {
const { api, setServers } = harness([entry("A")]);
api.record("A", settings({ paginatedLists: true }));
api.begin("A").landed(settings({ paginatedLists: true }));
setServers([]);
const fallback = settings();
expect(api.resolve("A", fallback)).toBe(fallback);

98
clients/web/src/hooks/useLastPersistedSettings.ts
View file

@ -23,34 +23,50 @@ import type {
* single slot would let a save on B discard the baseline a later failed write
* on A still needs — reproducing the very bug this hook exists to prevent.
*
* A record is not trusted indefinitely. It is paired with the `servers` entry
* for that server **as of when the write completed**, and is dropped once the
* list carries a different entry object: a successful read rebuilds the list,
* so a fresh object is the signal that the entry has caught up with — or been
* overtaken by (an edit made outside the Inspector) — our write, and from there
* the entry is authoritative again. The pairing is taken at completion rather
* than at scheduling because a read that lands *while the write is in flight*
* would otherwise be mistaken for one that happened after it.
* ## Which of two overlapping writes wins
*
* A write is announced with `begin` when it is *issued* and confirmed with
* `landed` when it completes, and a confirmation is ignored if a later-issued
* write for that server has already confirmed. Completion order cannot be used
* for this: `updateServerSettings` waits for a list read after its PUT, so an
* older write with a slow read can finish last and would otherwise overwrite
* the record with a value that is no longer on disk.
*
* Issue order is a proxy for disk order, not a proof of it — HTTP does not
* promise that two in-flight PUTs arrive in the order they were sent. It is the
* best signal available on this side: the backend serializes writes to the
* catalog, but neither the route nor `useServers` returns a revision the client
* could order by. If one is added later, this is where it replaces the counter.
*
* ## When a record stops being believed
*
* A record is paired with the `servers` entry for that server **as of when the
* write completed**, and is dropped once the list carries a different entry
* object: a successful read rebuilds the list, so a fresh object is the signal
* that the entry has caught up with — or been overtaken by (an edit made
* outside the Inspector) — our write, and from there the entry is authoritative
* again. The pairing is taken at completion rather than at issue because a read
* that lands *while the write is in flight* would otherwise be mistaken for one
* that happened after it.
*
* One case the client cannot decide is left: a background read whose response
* was generated before the write landed but which arrives after it still looks
* newer here, and the record is dropped in its favour. Ordering that reliably
* needs a revision or ETag on `GET /api/servers` that `useServers` does not
* expose today. What the fallback yields in that case is the freshest
* successful read — the same value the rest of the UI renders from — rather
* than an arbitrary one.
* newer here, and the record is dropped in its favour. That needs the same
* missing revision as above. What the fallback yields in that case is the
* freshest successful read — the same value the rest of the UI renders from —
* rather than an arbitrary one.
*
* The list and the records are held in refs rather than state deliberately:
* nothing renders from either, they are read only inside callbacks at the
* moment a write completes or fails, and holding them in state would re-render
* the whole composition root on every settings save.
* moment a write is issued, completes, or fails, and holding them in state
* would re-render the whole composition root on every settings save.
*/
export interface LastPersistedSettings {
/**
* Record a settings write that reached disk. Call it when the write
* *completes*, not when it is issued.
* Announce a settings write for `serverId` at the moment it is **issued**.
* Confirm it with the returned handle once it completes.
*/
record: (serverId: string, written: InspectorServerSettings) => void;
begin: (serverId: string) => SettingsWrite;
/**
* The best available answer to "what is on disk for this server". Returns the
* recorded write while the list still carries the entry that write was paired
@ -63,6 +79,14 @@ export interface LastPersistedSettings {
) => InspectorServerSettings;
}
export interface SettingsWrite {
/**
* Record that this write reached disk. A no-op if a write issued after this
* one has already been confirmed for the same server.
*/
landed: (written: InspectorServerSettings) => void;
}
interface WriteRecord {
written: InspectorServerSettings;
entry: ServerEntry | undefined;
@ -75,23 +99,35 @@ export function useLastPersistedSettings(
servers: ServerEntry[],
): LastPersistedSettings {
const recordsRef = useRef<Map<string, WriteRecord>>(new Map());
// Read the list through a ref so `record` pairs with the entry as it stands
// when the write finishes, not the one captured when the write was issued.
// Mirrored in an effect rather than assigned during render: writing a ref
// mid-render is an error under `react-hooks/refs`, and the callbacks that
// read it all run from settled promises, long after the commit.
const nextSequenceRef = useRef(0);
// Highest issue order confirmed per server. Kept apart from the records so a
// record dropped by `resolve` can't be resurrected by a straggler that was
// issued earlier: the high-water mark outlives the record it produced.
const confirmedRef = useRef<Map<string, number>>(new Map());
// Read the list through a ref so a write pairs with the entry as it stands
// when it finishes, not the one captured when it was issued. Mirrored in an
// effect rather than assigned during render: writing a ref mid-render is an
// error under `react-hooks/refs`, and every reader runs from a settled
// promise or an event handler, long after the commit.
const serversRef = useRef(servers);
useEffect(() => {
serversRef.current = servers;
}, [servers]);
const record = useCallback(
(serverId: string, written: InspectorServerSettings) => {
const entry = serversRef.current.find((s) => s.id === serverId);
recordsRef.current.set(serverId, { written, entry });
},
[],
);
const begin = useCallback((serverId: string): SettingsWrite => {
const sequence = ++nextSequenceRef.current;
return {
landed: (written: InspectorServerSettings) => {
const confirmed = confirmedRef.current.get(serverId) ?? 0;
// A write issued after this one already reported; it describes disk
// more recently, whichever of the two happened to finish first.
if (confirmed > sequence) return;
confirmedRef.current.set(serverId, sequence);
const entry = serversRef.current.find((s) => s.id === serverId);
recordsRef.current.set(serverId, { written, entry });
},
};
}, []);
const resolve = useCallback(
(
@ -114,5 +150,5 @@ export function useLastPersistedSettings(
[],
);
return { record, resolve };
return { begin, resolve };
}