From 7fa42299c7027f27f2f3299572e314475b94eea1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 11:34:20 -0400 Subject: [PATCH] fix(web): order overlapping settings writes by issue, not by completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.tsx | 18 +++- .../hooks/useLastPersistedSettings.test.tsx | 55 ++++++++--- .../web/src/hooks/useLastPersistedSettings.ts | 98 +++++++++++++------ 3 files changed, 121 insertions(+), 50 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 6855faed..4d481efa 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -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 diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx index 40daa2da..73f11d27 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.test.tsx +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -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); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts index 340a399d..872711d3 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.ts +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -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>(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>(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 }; }