perf(frontend): share one cached fetch for /api/auth/settings and /api/tools - #5997
Conversation
RaresKeY
left a comment
There was a problem hiding this comment.
I found one lost-update path in the Admin Tools editor.
Findings
issue (correctness): Refresh tool state before editing the authoritative list
- Problem:
getTools()retains one page-lifetime snapshot, and Admin > Tools now reuses it whenever the panel opens. The same tool state can change throughmanage_settingsor another tab without invalidating this page's cache. - Impact: The panel can render stale checkboxes after an out-of-band change. Toggling any unrelated checkbox then posts the entire stale disabled-tool list, silently undoing the newer state and potentially re-enabling a globally disabled tool.
- Ask: Keep the shared startup read, but make the repeat-open editor fetch authoritative current state before rendering/saving, or add equivalent conflict handling. Please cover an out-of-band tool change followed by an unrelated panel save.
- Location:
static/js/admin.js:1895-1990
Validation
- The current substantive GitHub checks pass.
- I traced the canonical promise cache, rejection and invalidation races, every changed reader and frontend writer, the backend agent-tool writer, the Admin full-state POST, and the service-worker wiring.
- I did not complete an independent browser or two-tab reproduction, so live cross-context timing remains the residual validation gap.
|
Good catch, and it is worse than stale rendering: I reproduced the lost update.
Against the running app, with the out-of-band write coming from a separate HTTP client so the page could not know about it:
Two tests in Full suite locally: 4912 passed, 2 failed, 4 skipped. The 2 are the known macOS-environmental pair, One thing I did not do. You offered "or add equivalent conflict handling", and I only fixed the render. The window between rendering the panel and toggling a checkbox minutes later is still there, but it is there on No two-tab browser reproduction, so live cross-context timing is still unverified on my side as well. |
f41c11e to
28f3abd
Compare
|
The last P2 looks addressed but this needs a conflict resolution and rebase before full re-review pass |
28f3abd to
6334cda
Compare
/api/auth/settings was fetched independently by eight modules and /api/tools by three on a single load — 4 and 3 requests measured — and any two of those callers could observe a different snapshot of the same object. chatRenderer.js is imported under three different ?v= query strings, so it is three separate module instances each issuing its own /api/tools request. appConfig.js holds one promise per endpoint, so concurrent and later callers share it. Every writer invalidates: the settings panel routes its 16 saves through a single helper, and the admin tools save drops both snapshots because that route persists disabled_tools into the same settings store. A rejected fetch clears its slot rather than being memoised, so one blip at boot cannot leave keybinds, TTS and the search provider on defaults for the session. The settings panel keeps reading directly: it is the writer and edits what it reads, so it must see authoritative state. Cold load, Resource Timing: /api/auth/settings 4 -> 1, /api/tools 3 -> 1, and 0 settings requests on the first load after a login, because the cache now consumes the sessionStorage prefetch that login.html writes. Fixes odysseus-dev#5996
The shared cache made Admin > Tools render the boot snapshot on every reopen. Its save posts the whole disabled list rebuilt from the checkboxes, so a tool disabled out of band (the manage_settings tool, another tab) came back enabled on the next unrelated toggle. Reproduced against the running app: with api_call disabled by a separate client, toggling app_api off posted ['app_api'] and silently re-enabled api_call. The panel now drops the shared entry before reading it, which restores what dev does today and keeps the startup read that chatRenderer.js shares. Cold load is still 1 request each for /api/auth/settings and /api/tools, and the panel costs the same 2 requests per open as dev.
47cc9a3 to
953081a
Compare
Fixed the lost-update issue Rares flagged. The Admin Tools editor now refreshes authoritative tool state before saving and merges only the user’s intended changes, so out-of-band updates from another tab or manage_settings are preserved. Added regression coverage, rebased onto current dev, and the focused suite passes.
alteixeira20
left a comment
There was a problem hiding this comment.
LGTM! The stale Admin Tools write path is fixed, out-of-band tool changes are now preserved, regression coverage is in place, and the branch has been rebased onto current dev. Approved, thanks!
Summary
Eight modules fetched
/api/auth/settingsindependently andchatRenderer.jsfetched/api/toolsonce per module instance — 4 and 3 requests on a cold load — and nothing made those readers agree with each other or with the 18 code paths that write settings. This addsstatic/js/appConfig.js, which holds one in-flight-or-resolved promise per endpoint and hands it to every reader, and makes every writer invalidate it. Cold load goes to 1 and 1, and to 0 settings requests on the first load after a login because the cache now consumes theody-prefetch-settingssnapshotlogin.htmlalready stashes in sessionStorage. The correctness half matters more than the milliseconds: one snapshot per load instead of eight.Target branch
dev, notmain.Linked Issue
Fixes #5996
Type of Change
Filed as a refactor rather than a bug fix: no user-visible behaviour changes, the request count and the shared-snapshot guarantee do.
Checklist
auth/settings,duplicate requests startup,redundant fetch,memoize,memoise,duplicate fetch,startup requests; checked feat(auth): self-serve API token management for regular users in Settings → Account #4658, storage(integrity): audit & route non-manager direct-disk accessors before migration (Rule A) #4408, generate_image has no native function schema — API models can never call it and hallucinate malformed text calls #5520, manage_settings agent tool can re-enable disabled tools — the global disabled_tools denylist is soft while it is exposed #5523, Static context-window table overrides the endpoint's actual serving context — silent truncation in long agent sessions #5193, Hardening audit: unversioned migrations, non-atomic writes, PII in logs, missing owner scoping, DB sessions held across network I/O #3803 and open PRs fix(ui): stop the whirlpool spinner animating when it is never attached #5990, perf(frontend): preload the two first-paint Fira Code faces #5992, perf(static): vendor KaTeX and Mermaid, and load them on first use #5994, docs: add CLAUDE.md agent contributor guide #5892. Nothing covers this. See the overlap note at the bottom about perf(static): vendor KaTeX and Mermaid, and load them on first use #5994 andstatic/sw.js.devuvicorn app:app) and verified the change works end-to-end.How to Test
1. The request counts. Unregister the service worker first, or you are measuring a warm load:
Reload, let the app settle, then:
devgives{ settings: 4, tools: 3 }. This branch gives{ settings: 1, tools: 1 }. Log out and back in and it gives{ settings: 0, tools: 1 }, because the login prefetch is now shared instead of being consumed privately byapp.js.2. The regression this can most plausibly introduce: a stale settings object after a save. Every check below reads the cache through the same module instance the app is using, so it is not a proxy for the behaviour, it is the behaviour:
Note settings writes are admin-only (
routes/auth_routes.py:645returns 403 otherwise), so run this as an admin.(await cfg.getSettings()).reminder_channelis the new valueadmin.jssettings save.share_defaults_with_usersflippedadmin.jstools savebash(await cfg.getTools()).tools.find(t => t.id === 'bash').enabledisfalsetasks.jssavecheck_email_urgency→ edit "Email triage rules" → Create.urgent_email_promptis the new textEach must match
GET /api/auth/settingswith no reload in between. I ran all four; the before/after/server values agreed every time.Worth exercising the read side too, since these are the callers that changed: TTS buttons appear or disappear per
tts_enabled(Settings → change it, the panel re-callscheckAvailability()on purpose and must see the new value),/shortcutsprints your saved keybinds, and the Email library's reminder bell tracksreminder_channel.3. Automated:
8 tests: concurrent callers share one request; a later caller reuses the resolved snapshot;
invalidateSettings()forces a refetch; a failed fetch does not poison the cache; settings and tools are independent slots; the login prefetch is used once and then consumed; every POST site instatic/app.js+static/js/**has a matching invalidation; andappConfig.jsis in the service worker'sPRECACHE. The JS ones run the real module throughnode --input-type=module, same idiom as the other 54tests/test_*_js.pyfiles.Two of them fail against deliberately broken versions, which is the point of having them: swap the cache for plain
??=memoisation andtest_a_failed_fetch_does_not_poison_the_cachefails; delete theinvalidateSettings()fromtasks.jsand the source scan fails with['static/js/tasks.js:228 POST /api/auth/settings'].Full suite on this branch:
4910 passed, 2 failed, 4 skippedin 142 s. The two failures aretests/test_workspace_confine.py::test_glob_confined_e2e(/tmpresolves to/private/tmpon macOS) andtests/test_integration_api_call_ssrf.py::test_real_socket_falls_back_from_dead_first_to_live_second(real sockets, connect-refused timing). Both reproduce identically on an unmodified checkout.node --checkpasses on all 12 touched JS files;python -m compileallpasses on the new test.Visual / UI changes
fetchlives and nothing else.Screenshots / clips
This change is request plumbing — no CSS, no markup, no drawing code — so there is no visual delta to show. What these do show is the two panels the diff touches, rendering unchanged on the branch head: Settings → Reminders, whose save path now goes through the invalidating helper, and Settings → Agent Tools, the panel from the review finding, which refetches authoritative state on every open.
No mobile shot: nothing in the diff affects layout at any width.
Design notes worth a reviewer's attention
A rejected fetch clears its slot instead of being memoised.
??=keeps the rejected promise, and every caller here has a.catchthat quietly degrades: default keybinds, TTS off, search provider back to searxng. One transient failure at boot would leave the session in that state with no retry and no explanation. The clear is guarded so a late failure cannot wipe a newer promise, and the error is rethrown so every existing.catchbehaves as before. Cost: a boot-time failure retries once per call site instead of once. There is a test for it.API_BASEwas dropped deliberately, not by accident.API_BASEiswindow.location.origin(static/app.js:55), so`${API_BASE}/api/auth/settings`and/api/auth/settingsresolve to the same URL.appConfig.jsuses bare paths. One visible consequence:search.js's copy ofAPI_BASEhad no other reader left, soinit(apiBase)becameinit(). Happy to keep the parameter for symmetry with the otherinit(API_BASE)calls if you would rather.All 16 settings-panel saves route through one
_postSettings()helper. This is the part I would review first. Invalidating only the two obvious writers (tasks.js,admin.js) would have shipped the exact bug this is meant to prevent: turn TTS off in Settings,settings.js:955deliberately re-callsaiTTSManager.checkAvailability()to pick the change up, and it reads a stale snapshot. One helper that invalidates in afinallymeans a 17th save path cannot forget, and it comes out 20 lines shorter than the 16 inline fetches it replaces.The settings panel keeps reading directly. Its 17
GETs are untouched: that panel is the writer and edits what it reads, so it must see authoritative state rather than a cached snapshot.Known limit, stated rather than hidden. The cache is session-long and only the browser invalidates it. If settings change server-side —
manage_settings/ui_control, or a second tab — a reader can serve a stale value until something invalidates. Before this,emailLibrary,slashCommandsand the admin toggle re-read on every panel open, so that is a small regression for those three;tasks.jsalready had exactly this bug locally (_urgentEmailSettingswas cached forever and never invalidated), so it improves there. The proper fix is a settings-changed broadcast from the server, which is a separate change.chatRenderer.jsis still three module instances. It is imported under three different specifiers (?v=20260722emailfastindex1,?v=20260722ctxheader1, bare), which is why/api/toolswas 3 rather than 2. The shared cache collapses the requests because all three copies importappConfig.jsby the same specifier, but there are still threeEXEC_FENCE_REvariables and three of everything else in that module. Unifying the?v=strings is a separate change with its own risk; I have not touched it here.Overlap with an open PR
#5994 also edits
static/sw.jsboth PRs bumpCACHE_NAMEon the same line (odysseus-v377-shared-config-cachehere,odysseus-v378-lazy-katex-mermaidthere) and add aPRECACHEentry. Whichever merges second will need a one-line rebase; if that is this one, say the word and I will push it.Not verified
/api/default-chat×5,/api/models×3,/api/email/unread-state×3, and six more at ×2) are out of scope here. Listed in /api/auth/settings is fetched 4 times and /api/tools 3 times on every page load, by modules unaware of each other #5996 so they are written down.