From 5df6818bb84176b922ccc1eefeebe52d1a28e283 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Wed, 4 Nov 2026 20:00:36 +0000 Subject: [PATCH] fix(addons): fail closed on the apps list when the owner does not answer A frontend kept the last apps list per person and answered 200 with a stale mark when the owner timed out. Two frontends then disagreed: one with a cached list answered 200, the other a typed 503. The acceptance contract is that every frontend gives the same typed answer when the owner cannot answer, so the server cache is gone. The route now answers owner_busy (timeout) or owner_unavailable with a 503 from every frontend. An owner that is up and busy still answers from its snapshot, with the operation of each add-on in the list. The Add-ons page keeps the last list in the browser and shows it with the busy notice, so it never goes empty. The specs, the docs and the swagger spec follow the new contract. Assisted-by: Claude:claude-sonnet-5-5 --- core/http/endpoints/addons/apps.go | 2 +- core/http/endpoints/addons/apps_test.go | 2 +- .../react-ui/e2e/addons-busy-owner.spec.js | 23 +++-- .../src/components/addons/AppsView.jsx | 22 ++--- .../src/components/addons/addonState.js | 8 +- core/http/react-ui/src/utils/addonsApi.js | 13 +-- .../tests/unit/addon-session.test.mjs | 2 +- .../tests/unit/addonhomecommands.test.mjs | 2 +- core/http/react-ui/tests/unit/addons.test.mjs | 11 +-- core/services/addons/apps/apps.go | 10 -- core/services/addons/gateway/apps.go | 93 ++----------------- .../services/addons/gateway/busyowner_test.go | 34 ++----- core/services/addons/gateway/gateway.go | 2 - docs/content/features/addons.md | 29 +++--- swagger/docs.go | 18 +--- swagger/swagger.json | 18 +--- swagger/swagger.yaml | 21 +---- 17 files changed, 75 insertions(+), 235 deletions(-) diff --git a/core/http/endpoints/addons/apps.go b/core/http/endpoints/addons/apps.go index 26c31f0be3..a22577cac6 100644 --- a/core/http/endpoints/addons/apps.go +++ b/core/http/endpoints/addons/apps.go @@ -28,7 +28,7 @@ var appID = regexp.MustCompile(`^[a-z][a-z0-9-]{0,63}$`) // @Success 200 {object} apps.List "Apps visible to the caller" // @Failure 401 {object} AddonErrorResponse "Named authentication required" // @Failure 403 {object} AddonErrorResponse "Feature or owner policy denied the request" -// @Failure 503 {object} AddonErrorResponse "Addons or the owner are unavailable, or the owner did not answer in time and this frontend has no earlier list to show (type owner_busy or owner_unavailable)" +// @Failure 503 {object} AddonErrorResponse "Addons or the owner are unavailable, or the owner did not answer in time (type owner_busy or owner_unavailable)" // @Router /api/addons/apps [get] func ListApps(g *gateway.Gateway, db *gorm.DB, opts ...Option) echo.HandlerFunc { s := resolve(opts) diff --git a/core/http/endpoints/addons/apps_test.go b/core/http/endpoints/addons/apps_test.go index 9b335c0bc6..cdef6e64ac 100644 --- a/core/http/endpoints/addons/apps_test.go +++ b/core/http/endpoints/addons/apps_test.go @@ -419,7 +419,7 @@ var _ = Describe("apps routes outcomes and documentation", Label("apps"), func() return m } full := apps.App{ID: "a", Name: "a", Credentialed: true, ContextFiles: []contract.ContextFile{{Scope: "user", Path: "Memory/index.md"}}, UserSettings: []contract.UserSetting{{Key: "save-mode", Values: []string{"ask", "never"}, Default: "ask", Role: "memory.save-mode"}}, Network: &apps.NetworkView{Kind: contract.NetworkKindEgressCheck, Title: "N", Presets: []contract.NetworkPreset{{ID: "off", Title: "Closed"}}}, Title: "t", Titles: map[string]string{"en": "t"}, Description: "d", Descriptions: map[string]string{"en": "d"}, Language: "it", LocalizedTitle: "t", LocalizedDescription: "d", Icon: "i", Version: "1", InstalledAt: "2026-10-30T09:15:00Z", Generation: 1, Enabled: true, State: "running", Update: "update-pending", Operation: &apps.Operation{Kind: "upgrade", Phase: "downloading", Since: "2026-11-04T17:18:00Z"}, MainPage: "/m", HasUI: true, LaunchMode: "always", Tools: []string{"x"}, ToolCount: 1, Shadows: []string{"bash"}, ToolDefinitions: []contract.ToolDefinition{{Name: "x", Description: "d", Kind: "read", InputSchema: json.RawMessage(`{"type":"object"}`), UIOnly: true, Approval: contract.ApprovalAlways, Shadows: "bash", Execution: &contract.ExecutionDescriptor{Class: "commands", Title: "T", Note: "n", Skills: &contract.ExecutionSkills{ProvidesRuntimeFor: []string{"documents"}}}}}, Skills: []contract.SkillDefinition{{Name: "xlsx", Shadows: "xlsx", Slot: &contract.SlotDefinition{Kind: contract.SlotKindDocuments}}}, SkillShadows: []string{"xlsx"}, UI: &apps.UIView{Entry: "/e", CSS: []string{"/c"}, SDK: contract.VersionRange{Min: 1, Max: 1}, Actions: []apps.ChatActionView{{ID: "a", Label: "A", Labels: map[string]string{"en": "A"}, Icon: "star", PackagedIcon: true, Placement: "composer", Tool: "t", Component: "C"}}, Slots: []apps.SlotView{{ID: "s", Slot: "files.preview", Component: "C", Extensions: []string{"docx"}}}, Renderers: []contract.Renderer{{Kind: "approval-card", Tool: "t", Component: "C"}}, Pages: []string{"p"}, Navigation: []string{"p"}, HomeCommands: []apps.HomeCommandView{{ID: "h", Page: "p", Label: "H", Icon: "star"}}, ModelActions: []contract.ModelAction{{Capability: "FLAG_IMAGE", Page: "p"}}, ChatActions: []string{"a"}, Controls: []string{"network-policy"}}, Admin: &apps.AdminView{CanStart: true, CanStop: true, ManagementURL: "/x", HostOperations: &hostops.RequestSummary{Required: []string{"r"}}}} - for name, v := range map[string]any{"apps.App": full, "apps.AdminView": *full.Admin, "apps.UIView": *full.UI, "apps.ChatActionView": full.UI.Actions[0], "apps.SlotView": full.UI.Slots[0], "contract.Renderer": full.UI.Renderers[0], "contract.VersionRange": full.UI.SDK, "contract.ToolDefinition": full.ToolDefinitions[0], "contract.ExecutionDescriptor": *full.ToolDefinitions[0].Execution, "contract.ExecutionSkills": *full.ToolDefinitions[0].Execution.Skills, "contract.SkillDefinition": full.Skills[0], "apps.List": apps.List{Apps: []apps.App{full}, Stale: true, StaleReason: "owner_busy", StaleSince: "2026-11-04T17:18:30Z"}, "apps.Operation": *full.Operation} { + for name, v := range map[string]any{"apps.App": full, "apps.AdminView": *full.Admin, "apps.UIView": *full.UI, "apps.ChatActionView": full.UI.Actions[0], "apps.SlotView": full.UI.Slots[0], "contract.Renderer": full.UI.Renderers[0], "contract.VersionRange": full.UI.SDK, "contract.ToolDefinition": full.ToolDefinitions[0], "contract.ExecutionDescriptor": *full.ToolDefinitions[0].Execution, "contract.ExecutionSkills": *full.ToolDefinitions[0].Execution.Skills, "contract.SkillDefinition": full.Skills[0], "apps.List": apps.List{Apps: []apps.App{full}}, "apps.Operation": *full.Operation} { def, ok := doc.Definitions[name] Expect(ok).To(BeTrue(), name) shape := emitted(v) diff --git a/core/http/react-ui/e2e/addons-busy-owner.spec.js b/core/http/react-ui/e2e/addons-busy-owner.spec.js index 344f7acd2c..5acf2d5a60 100644 --- a/core/http/react-ui/e2e/addons-busy-owner.spec.js +++ b/core/http/react-ui/e2e/addons-busy-owner.spec.js @@ -3,7 +3,7 @@ import { ADMIN, app, installedAddon, sessionFor, mockAddons, fail } from './addo // The owner is busy with an upgrade. The routes are stand-ins // (addons-fixtures.js); the owner's own behaviour is covered by the Go specs -// "reads during a long upgrade" and "apps list while the owner is busy". +// "reads during a long upgrade" and "apps list while the owner is busy". The server fails closed (a typed 503 on a timeout, never an older list); the page keeps its own last list.. const shots = process.env.BUSY_SHOTS const theme = process.env.BUSY_THEME @@ -58,17 +58,20 @@ test.describe('Add-ons: the owner is busy', () => { await expect(page.getByText('No add-ons yet')).toHaveCount(0) }) - test('a stale list from the server keeps the rows, names what is upgrading and shows the phase of each row', async ({ page }) => { + test('a timeout after a list with upgrades in flight keeps the rows, names what is upgrading and shows the phase of each row', async ({ page }) => { await signIn(page) + let busy = false await mockAddons(page, { apps: [notes(), sandbox(), tables()], installed: [installedAddon({ id: 'notes' })], - appsList: () => body({ - apps: [notes({ operation: upgrading('downloading') }), sandbox({ operation: upgrading('starting') }), tables()], - stale: true, staleReason: 'owner_busy', staleSince: '2026-11-04T17:18:30Z', - }), + appsList: () => (busy + ? fail(503, 'owner_busy', { message: 'busy', code: 503 }) + : body({ apps: [notes({ operation: upgrading('downloading') }), sandbox({ operation: upgrading('starting') }), tables()] })), }) await page.goto('/app/addons') + await expect(tile(page, 'notes').getByTestId('operation-chip')).toBeVisible() + busy = true + await page.reload() const banner = page.getByTestId('addons-stale') await expect(banner).toBeVisible() await expect(banner).toContainText('The add-on server is busy (upgrading Notes, Sandbox)') @@ -122,10 +125,14 @@ test.describe('Add-ons: the owner is busy', () => { await expect(page.getByTestId('addons-load-error')).toHaveCount(0) }) - test('a stale copy of an empty list is not the empty state either', async ({ page }) => { + test('a remembered empty list shown during a timeout is not the empty state either', async ({ page }) => { await signIn(page) - await mockAddons(page, { apps: [], appsList: () => body({ apps: [], stale: true, staleReason: 'owner_busy' }) }) + let busy = false + await mockAddons(page, { apps: [], appsList: () => (busy ? fail(503, 'owner_busy', { message: 'busy', code: 503 }) : null) }) await page.goto('/app/addons') + await expect(page.getByText('No add-ons yet').first()).toBeVisible() + busy = true + await page.reload() await expect(page.getByTestId('addons-load-error')).toBeVisible() await expect(page.getByText('No add-ons yet')).toHaveCount(0) }) diff --git a/core/http/react-ui/src/components/addons/AppsView.jsx b/core/http/react-ui/src/components/addons/AppsView.jsx index fa04c87fcd..b27b46a82d 100644 --- a/core/http/react-ui/src/components/addons/AppsView.jsx +++ b/core/http/react-ui/src/components/addons/AppsView.jsx @@ -47,7 +47,7 @@ const browserStore = () => { try { return globalThis.sessionStorage || null } ca export default function AppsView({ api, adapter, onSummary, grants }) { const { t, i18n } = useTranslation('addons') - const [storedState, setState] = useState({ status: 'loading', apps: [], error: null, stale: false, staleReason: '' }) + const [storedState, setState] = useState({ status: 'loading', apps: [], error: null, stale: false }) const [reload, setReload] = useState(0) const reloadRef = useRef(() => {}) const { hasFeature, isAdmin, user, sessionEpoch } = useAuth() @@ -59,7 +59,7 @@ export default function AppsView({ api, adapter, onSummary, grants }) { const actor = JSON.stringify([user?.id, sessionEpoch, isAdmin, canList]) const scope = useRef(actor) scope.current = actor - const state = storedState.actor === actor ? storedState : { status: 'loading', apps: [], error: null, stale: false, staleReason: '' } + const state = storedState.actor === actor ? storedState : { status: 'loading', apps: [], error: null, stale: false } const ingest = registry?.ingest // Poll with a timer chain: one request at a time, aborted on unmount. A @@ -80,16 +80,12 @@ export default function AppsView({ api, adapter, onSummary, grants }) { try { const read = await api.apps.read(ctl.signal) const apps = read.apps - // The server answered with its last copy: the owner did not. Ask - // again with the same backoff as for a failed read. - failures = read.stale ? failures + 1 : 0 + failures = 0 if (alive && current()) { - setState({ actor, status: 'ready', apps, error: null, stale: read.stale, staleReason: read.staleReason }) - if (!read.stale) { - ingest?.(apps) - const store = browserStore() - if (store) saveLastKnown(store, actor, apps) - } + setState({ actor, status: 'ready', apps, error: null, stale: false }) + ingest?.(apps) + const store = browserStore() + if (store) saveLastKnown(store, actor, apps) } } catch (error) { if (!alive || !current() || (error && error.name === 'AbortError')) return @@ -101,7 +97,7 @@ export default function AppsView({ api, adapter, onSummary, grants }) { const remembered = store ? loadLastKnown(store, actor) : null setState(s => { const kept = s.actor === actor && s.status === 'ready' ? s.apps : remembered - return { actor, status: kept ? 'ready' : 'error', apps: kept || [], error, stale: true, staleReason: '' } + return { actor, status: kept ? 'ready' : 'error', apps: kept || [], error, stale: true } }) } finally { inflight = false @@ -320,7 +316,7 @@ export default function AppsView({ api, adapter, onSummary, grants }) { title={t('apps.loadError.title')} testId="addons-load-error" nothingChanged={false} - onRetry={() => { setState({ status: 'loading', apps: [], error: null, stale: false, staleReason: '' }); setReload(n => n + 1) }} + onRetry={() => { setState({ status: 'loading', apps: [], error: null, stale: false }); setReload(n => n + 1) }} > {[ describeError(loadError, t), diff --git a/core/http/react-ui/src/components/addons/addonState.js b/core/http/react-ui/src/components/addons/addonState.js index 4eeb4ea3b0..ada5111777 100644 --- a/core/http/react-ui/src/components/addons/addonState.js +++ b/core/http/react-ui/src/components/addons/addonState.js @@ -319,13 +319,11 @@ export function upgradingTitles(apps) { } // Why the list on screen may be out of date: 'busy' (the owner did not answer -// in time or the server sent its last copy), 'unreachable' or '' when it is -// current. -export function staleKind({ error, stale, staleReason }) { +// in time), 'unreachable' or '' when it is current. +export function staleKind({ error, stale }) { if (!stale) return '' if (error && error.kind === 'busy') return 'busy' - if (staleReason === 'owner_busy') return 'busy' - if (error || staleReason === 'owner_unavailable') return 'unreachable' + if (error) return 'unreachable' return 'busy' } diff --git a/core/http/react-ui/src/utils/addonsApi.js b/core/http/react-ui/src/utils/addonsApi.js index ac2dd5a099..274d14d634 100644 --- a/core/http/react-ui/src/utils/addonsApi.js +++ b/core/http/react-ui/src/utils/addonsApi.js @@ -117,17 +117,12 @@ export function createAddonsApi({ fetch: fetchImpl, base = p => p } = {}) { async list(signal) { return (await api.apps.read(signal)).apps }, - // The list with the marks the server puts on it: stale is true when the - // server answers with the last list it got from the owner (the owner is - // busy or not reachable), so a page can say so and keep showing it. + // The server fails closed: when the owner does not answer it sends a + // typed 503, never an older list. A page that wants to keep showing its + // last list keeps it in its own memory. async read(signal) { const out = await call('/api/addons/apps', { signal }) - return { - apps: Array.isArray(out?.apps) ? out.apps : [], - stale: out?.stale === true, - staleReason: typeof out?.staleReason === 'string' ? out.staleReason : '', - staleSince: typeof out?.staleSince === 'string' ? out.staleSince : '', - } + return { apps: Array.isArray(out?.apps) ? out.apps : [] } }, async start(id, { operationId, generation }, signal) { const out = await call(`/api/addons/apps/${encodeURIComponent(id)}/start`, { method: 'POST', body: { operationId, generation }, signal }) diff --git a/core/http/react-ui/tests/unit/addon-session.test.mjs b/core/http/react-ui/tests/unit/addon-session.test.mjs index 44dc854b2c..983eaccd8c 100644 --- a/core/http/react-ui/tests/unit/addon-session.test.mjs +++ b/core/http/react-ui/tests/unit/addon-session.test.mjs @@ -72,7 +72,7 @@ for (const switchUser of [false, true]) test(`installed records reject late admi let auth = { user: { id: 'admin' }, isAdmin: true, sessionEpoch: 1, hasFeature: () => true } const pending = deferred() let reads = 0 - const api = { apps: { read: async () => ({ apps: [{ id: 'notes', generation: 1 }], stale: false, staleReason: '' }) }, installed: () => { reads++; return pending.promise } } + const api = { apps: { read: async () => ({ apps: [{ id: 'notes', generation: 1 }] }) }, installed: () => { reads++; return pending.promise } } const text = readFileSync(new URL('../../src/components/addons/AppsView.jsx', import.meta.url), 'utf8') // Full Apps body/effects; JSX is compiled, leaf hooks are external boundaries. const names = [...text.matchAll(/^import \{ ([^}]+) \} from/gm)].flatMap(m => m[1].split(',').map(n => n.trim())) diff --git a/core/http/react-ui/tests/unit/addonhomecommands.test.mjs b/core/http/react-ui/tests/unit/addonhomecommands.test.mjs index 57234ad86f..87ea4e1196 100644 --- a/core/http/react-ui/tests/unit/addonhomecommands.test.mjs +++ b/core/http/react-ui/tests/unit/addonhomecommands.test.mjs @@ -257,7 +257,7 @@ for (const accountSwitch of [false, true]) test(`Apps registry: late list cannot let registry = r.render() let auth = { user:{id:'alice'}, sessionEpoch:1, isAdmin:true, hasFeature:() => true } const reads = [] - const api = { apps:{ read:() => { const d = deferred(); reads.push(d); return d.promise.then(apps => ({ apps, stale:false, staleReason:'' })) } } } + const api = { apps:{ read:() => { const d = deferred(); reads.push(d); return d.promise.then(apps => ({ apps })) } } } const h = hookHarness('../../src/components/addons/AppsView.jsx', 'AppsView', { useAuth:() => auth, useContext:() => registry, RegistryContext:{}, useTranslation:() => ({t:x=>x,i18n:{}}), FEATURE_ADDONS:'addons', pollDelay:() => 1000, setTimeout:()=>1, clearTimeout:()=>{}, diff --git a/core/http/react-ui/tests/unit/addons.test.mjs b/core/http/react-ui/tests/unit/addons.test.mjs index d52e085449..68fe1828a6 100644 --- a/core/http/react-ui/tests/unit/addons.test.mjs +++ b/core/http/react-ui/tests/unit/addons.test.mjs @@ -618,16 +618,16 @@ test('addons: the details list the skills of an add-on and which replace a built assert.deepEqual(skillsOf({}), []) }) -test('addons: a busy owner is its own kind of answer, and the list read carries the stale mark', async () => { +test('addons: a busy owner is its own kind of answer, and the list read never carries a stale mark', async () => { assert.equal(classifyResponse(503, { error: { type: 'owner_busy', message: 'busy' } }).kind, 'busy') assert.equal(classifyResponse(503, { error: { type: 'owner_unavailable' } }).kind, 'unavailable') const busy = new AddonsError(classifyResponse(503, { error: { type: 'owner_busy' } })) assert.match(describeError(busy, key => key), /errors\.busy/) - const api = createAddonsApi({ fetch: async () => json(200, { apps: [{ id: 'notes' }], stale: true, staleReason: 'owner_busy', staleSince: '2026-11-04T17:18:00Z' }) }) - assert.deepEqual(await api.apps.read(), { apps: [{ id: 'notes' }], stale: true, staleReason: 'owner_busy', staleSince: '2026-11-04T17:18:00Z' }) + const api = createAddonsApi({ fetch: async () => json(200, { apps: [{ id: 'notes' }] }) }) + assert.deepEqual(await api.apps.read(), { apps: [{ id: 'notes' }] }) assert.deepEqual((await api.apps.list()).map(a => a.id), ['notes']) const plain = createAddonsApi({ fetch: async () => json(200, { apps: [] }) }) - assert.equal((await plain.apps.read()).stale, false) + assert.deepEqual(await plain.apps.read(), { apps: [] }) }) test('addons: an error is never an empty list, and the page names what the owner is upgrading', () => { @@ -644,9 +644,8 @@ test('addons: an error is never an empty list, and the page names what the owner assert.deepEqual(operationOf({ operation: { kind: 'defrag', phase: 'later' } }), { kind: 'working', phase: 'working', since: '' }) assert.equal(staleKind({ stale: false, error: new Error('x') }), '') assert.equal(staleKind({ stale: true, error: { kind: 'busy' } }), 'busy') - assert.equal(staleKind({ stale: true, staleReason: 'owner_busy' }), 'busy') + assert.equal(staleKind({ stale: true }), 'busy') assert.equal(staleKind({ stale: true, error: { kind: 'network' } }), 'unreachable') - assert.equal(staleKind({ stale: true, staleReason: 'owner_unavailable' }), 'unreachable') }) test('addons: the last list of a tab stands in for a page loaded while the owner is busy', () => { diff --git a/core/services/addons/apps/apps.go b/core/services/addons/apps/apps.go index 4dde6450ba..bb8c650914 100644 --- a/core/services/addons/apps/apps.go +++ b/core/services/addons/apps/apps.go @@ -61,16 +61,6 @@ type Operation struct { // List is the response of the apps list. type List struct { Apps []App `json:"apps"` - // Stale is true when the receiving frontend could not reach the owner in - // time and answers with the last list it received from it. The list may be - // out of date; Apps still carries the operations the owner reported then. - Stale bool `json:"stale,omitempty"` - // StaleReason is why the list is stale: owner_busy when the owner did not - // answer within the read deadline, owner_unavailable when it could not be - // reached at all. - StaleReason string `json:"staleReason,omitempty" enums:"owner_busy,owner_unavailable"` - // StaleSince is when the owner last answered, RFC 3339 in UTC. - StaleSince string `json:"staleSince,omitempty"` } // StartRequest is the body of the start and stop routes. The generation is diff --git a/core/services/addons/gateway/apps.go b/core/services/addons/gateway/apps.go index 6da1b94be8..6df6350e59 100644 --- a/core/services/addons/gateway/apps.go +++ b/core/services/addons/gateway/apps.go @@ -6,15 +6,11 @@ import ( "crypto/rand" "crypto/sha256" "encoding/hex" - "errors" "fmt" "io" "net/http" "os" - "slices" "strconv" - "sync" - "time" "github.com/mudler/LocalAI/core/services/addons/apps" "github.com/mudler/LocalAI/pkg/addons/contract" @@ -34,92 +30,17 @@ func (g *Gateway) Apps(ctx context.Context, a contract.Actor) (apps.List, error) return apps.List{}, e } if g.Forward != nil { - list, e := g.Forward.apps(ctx, a) - if e == nil { - g.appsCache.put(a, list) - return list, nil - } - // The owner is busy or unreachable: a page is better served with the - // last list this frontend got for the same person, marked stale, than - // with an error it could mistake for "nothing is installed". A denial - // is never covered by the cache. - if StatusFor(e) == http.StatusServiceUnavailable && ctx.Err() == nil { - if old, ok := g.appsCache.get(a); ok { - var timeout *OwnerTimeout - old.list.Stale, old.list.StaleReason = true, "owner_unavailable" - if errors.As(e, &timeout) { - old.list.StaleReason = "owner_busy" - } - old.list.StaleSince = old.at.UTC().Format(time.RFC3339) - return old.list, nil - } - } - return apps.List{}, e + // The owner's answer is the answer. When the owner cannot answer in + // time, every frontend gives the same typed error (OwnerTimeout is + // owner_busy, anything else owner_unavailable) and never an older list + // of its own: a per-frontend copy would make two frontends disagree + // about whether the owner answered. A page keeps the last list in the + // browser. + return g.Forward.apps(ctx, a) } return g.appsLocal(ctx, a) } -// AppsStaleTTL is how long a frontend keeps the last apps list of a person to -// answer with while the owner does not. -const AppsStaleTTL = 10 * time.Minute - -const appsCacheMax = 256 - -type cachedApps struct { - list apps.List - at time.Time -} - -// appsCache holds the last good list per person. It is a per-process -// convenience for the busy owner, never a source of truth: it is only read -// when the owner cannot answer, and it expires. -type appsCache struct { - mu sync.Mutex - entries map[string]cachedApps - now func() time.Time -} - -func cacheKey(a contract.Actor) string { return a.Mode + "\x00" + a.UserID } - -func (c *appsCache) clock() time.Time { - if c.now != nil { - return c.now() - } - return time.Now() -} - -func (c *appsCache) put(a contract.Actor, list apps.List) { - c.mu.Lock() - defer c.mu.Unlock() - if c.entries == nil { - c.entries = map[string]cachedApps{} - } - if _, ok := c.entries[cacheKey(a)]; !ok && len(c.entries) >= appsCacheMax { - oldest, when := "", time.Time{} - for k, v := range c.entries { - if oldest == "" || v.at.Before(when) { - oldest, when = k, v.at - } - } - delete(c.entries, oldest) - } - list.Stale, list.StaleReason, list.StaleSince = false, "", "" - c.entries[cacheKey(a)] = cachedApps{list: list, at: c.clock()} -} - -func (c *appsCache) get(a contract.Actor) (cachedApps, bool) { - c.mu.Lock() - defer c.mu.Unlock() - e, ok := c.entries[cacheKey(a)] - if !ok || c.clock().Sub(e.at) > AppsStaleTTL { - delete(c.entries, cacheKey(a)) - return cachedApps{}, false - } - // The apps slice is shared with later readers: hand out a copy of it. - e.list.Apps = slices.Clone(e.list.Apps) - return e, true -} - func (g *Gateway) appsLocal(ctx context.Context, a contract.Actor) (apps.List, error) { if g.Manager == nil { return apps.List{}, ErrUnavailable diff --git a/core/services/addons/gateway/busyowner_test.go b/core/services/addons/gateway/busyowner_test.go index a69e6e7e6c..150dfc9928 100644 --- a/core/services/addons/gateway/busyowner_test.go +++ b/core/services/addons/gateway/busyowner_test.go @@ -14,7 +14,6 @@ import ( var _ = Describe("apps list while the owner is busy", func() { alice := contract.Actor{UserID: "alice", CredentialID: "key:alice", Mode: "named-user"} - bob := contract.Actor{UserID: "bob", CredentialID: "key:bob", Mode: "named-user"} allow := func(context.Context, contract.Actor) error { return nil } // busyOwner serves the apps list until stall is set, then holds every @@ -53,43 +52,28 @@ var _ = Describe("apps list while the owner is busy", func() { Expect(e).To(MatchError(ErrUnavailable)) }) - It("answers with the last good list marked stale, and only for the same person", func() { + It("never answers with an older list of its own: every frontend gives the same typed error", func() { front, _, stall := busyOwner() fresh, e := front.Apps(context.Background(), alice) Expect(e).NotTo(HaveOccurred()) - Expect(fresh.Stale).To(BeFalse()) Expect(fresh.Apps).NotTo(BeEmpty()) + // This frontend has a good list of alice. A frontend that never read + // one must not differ from it, so the first one gives no list either. stall.Store(true) list, e := front.Apps(context.Background(), alice) - Expect(e).NotTo(HaveOccurred()) - Expect(list.Stale).To(BeTrue()) - Expect(list.StaleReason).To(Equal("owner_busy")) - Expect(list.StaleSince).NotTo(BeEmpty()) - Expect(list.Apps).To(HaveLen(len(fresh.Apps))) - - _, e = front.Apps(context.Background(), bob) - Expect(e).To(HaveOccurred(), "another person never gets alice's list") + Expect(e).To(HaveOccurred()) + Expect(list.Apps).To(BeEmpty()) + var timeout *OwnerTimeout + Expect(e).To(BeAssignableToTypeOf(timeout)) stall.Store(false) again, e := front.Apps(context.Background(), alice) Expect(e).NotTo(HaveOccurred()) - Expect(again.Stale).To(BeFalse()) + Expect(again.Apps).To(HaveLen(len(fresh.Apps))) }) - It("lets a cached list expire", func() { - front, _, stall := busyOwner() - now := time.Now() - front.appsCache.now = func() time.Time { return now } - _, e := front.Apps(context.Background(), alice) - Expect(e).NotTo(HaveOccurred()) - stall.Store(true) - now = now.Add(AppsStaleTTL + time.Second) - _, e = front.Apps(context.Background(), alice) - Expect(e).To(HaveOccurred()) - }) - - It("does not hide a denial behind the cache", func() { + It("does not hide a denial", func() { front, _, _ := busyOwner() _, e := front.Apps(context.Background(), alice) Expect(e).NotTo(HaveOccurred()) diff --git a/core/services/addons/gateway/gateway.go b/core/services/addons/gateway/gateway.go index a2b1570ddc..73bb2e9951 100644 --- a/core/services/addons/gateway/gateway.go +++ b/core/services/addons/gateway/gateway.go @@ -26,8 +26,6 @@ var ErrNotSent = fmt.Errorf("the add-on is not running now and nothing was sent: // Gateway deliberately admits lifecycle commands only until resource admission // and proposal confirmation are installed. Installation admin is not data access. type Gateway struct { - // appsCache backs the stale answer of the apps list; see Apps. - appsCache appsCache // Transfers holds the handles of connector file transfers on the process // that runs the add-ons. Without it a call that carries a handle is // unavailable. diff --git a/docs/content/features/addons.md b/docs/content/features/addons.md index 7f5d1373bd..0638ef0230 100644 --- a/docs/content/features/addons.md +++ b/docs/content/features/addons.md @@ -950,21 +950,24 @@ the apps list: still those of the running add-on until the new generation is published. A frontend that forwards a read to the owner waits at most eight seconds. When -the owner does not answer in that time: +the owner does not answer in that time, the route fails closed with the same +typed `503` from every frontend, and never with an older list of its own: -- The apps list answers `200` with `"stale": true`, `"staleReason"` (`owner_busy` - or `owner_unavailable`) and `"staleSince"`, and carries the last list this - frontend received for the same person. The copy lives for ten minutes and is - never used for a refused request. -- Without such a copy the route answers `503` with the error type `owner_busy` - (the owner did not answer in time) or `owner_unavailable`. `GET /api/addons/pins` - does the same instead of returning pins without the add-ons it could not check. +- `owner_busy`: the owner is up but did not answer in time. +- `owner_unavailable`: the owner could not be reached at all. -The Add-ons page keeps showing the last list, shows a notice such as "The -add-on server is busy (upgrading Sandbox). Showing the last known state.", marks -each changing add-on as upgrading with its phase, and asks again with a growing -delay. It only says that no add-ons are installed when the owner answered with -an empty list. +A frontend keeps no copy of the apps list, because two frontends with different +copies would give different answers for the same request. `GET /api/addons/pins` +answers the same way instead of returning pins without the add-ons it could not +check. When the owner is reachable and busy, it still answers from its snapshot, +so the apps list is a normal `200` with the `operation` fields above. + +The Add-ons page keeps the last list it received in the browser (in memory and, +for a page loaded during an upgrade, in the session storage of the tab). On a +`503` it keeps showing that list, shows a notice such as "The add-on server is +busy. Showing the last known state.", and asks again with a growing delay. It +only says that no add-ons are installed when the owner answered with an empty +list. ## The official library diff --git a/swagger/docs.go b/swagger/docs.go index b540e05bc2..7633b2006f 100644 --- a/swagger/docs.go +++ b/swagger/docs.go @@ -164,7 +164,7 @@ const docTemplate = `{ } }, "503": { - "description": "Addons or the owner are unavailable, or the owner did not answer in time and this frontend has no earlier list to show (type owner_busy or owner_unavailable)", + "description": "Addons or the owner are unavailable, or the owner did not answer in time (type owner_busy or owner_unavailable)", "schema": { "$ref": "#/definitions/addons.AddonErrorResponse" } @@ -16398,22 +16398,6 @@ const docTemplate = `{ "items": { "$ref": "#/definitions/apps.App" } - }, - "stale": { - "description": "Stale is true when the receiving frontend could not reach the owner in\ntime and answers with the last list it received from it. The list may be\nout of date; Apps still carries the operations the owner reported then.", - "type": "boolean" - }, - "staleReason": { - "description": "StaleReason is why the list is stale: owner_busy when the owner did not\nanswer within the read deadline, owner_unavailable when it could not be\nreached at all.", - "type": "string", - "enum": [ - "owner_busy", - "owner_unavailable" - ] - }, - "staleSince": { - "description": "StaleSince is when the owner last answered, RFC 3339 in UTC.", - "type": "string" } } }, diff --git a/swagger/swagger.json b/swagger/swagger.json index d2829fd4fc..eb86cb5073 100644 --- a/swagger/swagger.json +++ b/swagger/swagger.json @@ -161,7 +161,7 @@ } }, "503": { - "description": "Addons or the owner are unavailable, or the owner did not answer in time and this frontend has no earlier list to show (type owner_busy or owner_unavailable)", + "description": "Addons or the owner are unavailable, or the owner did not answer in time (type owner_busy or owner_unavailable)", "schema": { "$ref": "#/definitions/addons.AddonErrorResponse" } @@ -16395,22 +16395,6 @@ "items": { "$ref": "#/definitions/apps.App" } - }, - "stale": { - "description": "Stale is true when the receiving frontend could not reach the owner in\ntime and answers with the last list it received from it. The list may be\nout of date; Apps still carries the operations the owner reported then.", - "type": "boolean" - }, - "staleReason": { - "description": "StaleReason is why the list is stale: owner_busy when the owner did not\nanswer within the read deadline, owner_unavailable when it could not be\nreached at all.", - "type": "string", - "enum": [ - "owner_busy", - "owner_unavailable" - ] - }, - "staleSince": { - "description": "StaleSince is when the owner last answered, RFC 3339 in UTC.", - "type": "string" } } }, diff --git a/swagger/swagger.yaml b/swagger/swagger.yaml index 3876f0b95b..8455a0907b 100644 --- a/swagger/swagger.yaml +++ b/swagger/swagger.yaml @@ -536,24 +536,6 @@ definitions: items: $ref: '#/definitions/apps.App' type: array - stale: - description: |- - Stale is true when the receiving frontend could not reach the owner in - time and answers with the last list it received from it. The list may be - out of date; Apps still carries the operations the owner reported then. - type: boolean - staleReason: - description: |- - StaleReason is why the list is stale: owner_busy when the owner did not - answer within the read deadline, owner_unavailable when it could not be - reached at all. - enum: - - owner_busy - - owner_unavailable - type: string - staleSince: - description: StaleSince is when the owner last answered, RFC 3339 in UTC. - type: string type: object apps.NetworkView: properties: @@ -12091,8 +12073,7 @@ paths: $ref: '#/definitions/addons.AddonErrorResponse' "503": description: Addons or the owner are unavailable, or the owner did not answer - in time and this frontend has no earlier list to show (type owner_busy - or owner_unavailable) + in time (type owner_busy or owner_unavailable) schema: $ref: '#/definitions/addons.AddonErrorResponse' summary: List the addon apps the caller may use