diff --git a/client/src/components/Admin/storage/AdminStoragePanel.tsx b/client/src/components/Admin/storage/AdminStoragePanel.tsx index 954ef6a2c..0feae34f7 100644 --- a/client/src/components/Admin/storage/AdminStoragePanel.tsx +++ b/client/src/components/Admin/storage/AdminStoragePanel.tsx @@ -126,8 +126,7 @@ export default function AdminStoragePanel(): React.ReactElement { useEffect(() => { if (migrationQueue.length === 0 || !admin.state) return if (migrationStartInFlight.current) return - if (admin.state.migrations.some((m) => m.status === 'running')) return - if (admin.state.backfills.some((b) => b.status === 'running')) return + if (admin.storageBusy()) return const [next, ...rest] = migrationQueue migrationStartInFlight.current = true setMigrationQueue(rest) diff --git a/client/src/components/Admin/storage/useStorageAdmin.test.tsx b/client/src/components/Admin/storage/useStorageAdmin.test.tsx new file mode 100644 index 000000000..e58889810 --- /dev/null +++ b/client/src/components/Admin/storage/useStorageAdmin.test.tsx @@ -0,0 +1,96 @@ +import React, { useEffect } from 'react'; +import { http, HttpResponse } from 'msw'; +import { describe, expect, it } from 'vitest'; +import type { StorageAdminState } from '@trek/shared'; +import { server } from '../../../../tests/helpers/msw/server'; +import { render, screen, waitFor } from '../../../../tests/helpers/render'; +import { useStorageAdmin, type StorageAdmin } from './useStorageAdmin'; + +function baseState(overrides: Partial = {}): StorageAdminState { + return { + backends: [ + { name: 'uploads-local', type: 'local', source: 'built-in', options: { root: '/data/uploads' }, categories: ['files'] }, + ], + categories: { files: { backend: 'uploads-local', source: 'default' } } as StorageAdminState['categories'], + health: { replicaFailures: [] }, + seedFilePresent: false, + usage: null, + backfills: [], + migrations: [], + version: 0, + ...overrides, + }; +} + +function runningState(): StorageAdminState { + return baseState({ + migrations: [ + { category: 'files', from: 'uploads-local', to: 'off-box', status: 'running', done: 1, total: 3, copied: 1, skipped: 0, failed: 0, startedAt: 1 }, + ], + }); +} + +/** + * Exposes the hook to the test without a panel in the way — what the panels' + * queue effect depends on is the hook's own timing, not any rendered output. + */ +function Probe({ onReady }: { onReady: (admin: StorageAdmin) => void }): React.ReactElement { + const admin = useStorageAdmin('generic', 'conflict'); + // After every render, not during one: the test wants whatever the latest + // render produced, and a render must stay free of side effects. + useEffect(() => { onReady(admin); }); + return
{admin.state ? 'loaded' : 'loading'}
; +} + +describe('useStorageAdmin', () => { + async function mount(): Promise<() => StorageAdmin> { + let latest: StorageAdmin | null = null; + render( { latest = a; }} />); + await screen.findByText('loaded'); + return () => latest!; + } + + it('FE-ADMIN-STOR-048: startMigration resolves only once storageBusy() already reports the started migration', async () => { + let posted = false; + server.use( + http.get('/api/admin/storage', () => HttpResponse.json(posted ? runningState() : baseState())), + http.post('/api/admin/storage/migrations', () => { + posted = true; + return HttpResponse.json({ started: true }); + }), + ); + const admin = await mount(); + expect(admin().storageBusy()).toBe(false); + + // The queue effect releases its single-flight lock the moment this + // resolves, so the world it then decides on must already be post-POST. + expect(await admin().startMigration('files', 'off-box')).toBeNull(); + expect(admin().storageBusy()).toBe(true); + }); + + it('FE-ADMIN-STOR-049: storageBusy() is current at response time, before the render that shows it', async () => { + server.use(http.get('/api/admin/storage', () => HttpResponse.json(runningState()))); + const admin = await mount(); + const before = admin(); + + // No await on a render: refreshState resolving is the only synchronisation + // point, and the ref must already carry the new world at that instant — + // that is the whole point of it, since a deferred commit can run an effect + // whose captured `state` is several responses behind. + await before.refreshState(); + expect(before.storageBusy()).toBe(true); + await waitFor(() => expect(admin().state!.migrations).toHaveLength(1)); + }); + + it('FE-ADMIN-STOR-050: a failed refresh leaves storageBusy() on the last world that was actually applied', async () => { + server.use(http.get('/api/admin/storage', () => HttpResponse.json(runningState()))); + const admin = await mount(); + expect(admin().storageBusy()).toBe(true); + server.use(http.get('/api/admin/storage', () => HttpResponse.error())); + + // A transient GET failure must not read as "the server went idle" — that + // is the one answer that would let a second migration start. + await expect(admin().refreshState()).rejects.toThrow(); + expect(admin().storageBusy()).toBe(true); + }); +}); diff --git a/client/src/components/Admin/storage/useStorageAdmin.ts b/client/src/components/Admin/storage/useStorageAdmin.ts index 880122493..85a8dd58f 100644 --- a/client/src/components/Admin/storage/useStorageAdmin.ts +++ b/client/src/components/Admin/storage/useStorageAdmin.ts @@ -16,6 +16,20 @@ function isConflictError(err: unknown): boolean { export interface StorageAdmin { state: StorageAdminState | null + /** + * Whether the server is already busy with a storage job: a migration or a + * backfill, since its one-job-at-a-time rule spans both. Answered off the + * last response rather than off `state`, so it is current the instant that + * response lands rather than on the commit that renders it. + * + * That difference is the whole point. An effect re-run by a DEFERRED commit + * still closes over the `state` of the render it belongs to, which under load + * is several responses old by the time it runs, and the queue effect deciding + * on that closure POSTed a second migration on top of a running one. Anything + * deciding whether to fire a request asks this; anything rendering reads + * `state`. Never called during render. + */ + storageBusy: () => boolean /** The settings-owned document (the PUT body), with local edits layered in. Carries the version the draft was built at. */ draft: StorageConfigPut | null dirty: boolean @@ -85,6 +99,15 @@ export function useStorageAdmin(genericError: string, conflictError: string): St savingRef.current = saving }, [saving]) + // Written by every state application, at response time rather than at commit + // time, and read back through storageBusy(). See the interface doc. + const stateRef = useRef(null) + const storageBusy = useCallback((): boolean => { + const live = stateRef.current + if (!live) return false + return live.migrations.some((m) => m.status === 'running') || live.backfills.some((b) => b.status === 'running') + }, []) + // Bumped by every deliberate state application (initial load, save // success). A refreshState() call captures the seq before its GET and // drops the response if the seq moved on — i.e. a save applied its own @@ -93,6 +116,7 @@ export function useStorageAdmin(genericError: string, conflictError: string): St const applyState = useCallback((next: StorageAdminState) => { stateSeq.current += 1 + stateRef.current = next setState(next) setDraftState(settingsDocumentOf(next)) setDirty(false) @@ -145,6 +169,7 @@ export function useStorageAdmin(genericError: string, conflictError: string): St const seq = stateSeq.current const next = await adminApi.getStorage() if (stateSeq.current !== seq || savingRef.current) return + stateRef.current = next setState(next) if (!dirtyRef.current) setDraftState(settingsDocumentOf(next)) }, []) @@ -317,7 +342,14 @@ export function useStorageAdmin(genericError: string, conflictError: string): St const refreshStats = useCallback(async (): Promise => { try { const usage = await adminApi.refreshStorageStats() - setState((prev) => (prev ? { ...prev, usage } : prev)) + // Off stateRef rather than a functional updater, so the ref and the + // rendered state stay one world: every writer here sets both. + const prev = stateRef.current + if (prev) { + const next = { ...prev, usage } + stateRef.current = next + setState(next) + } return null } catch (err: unknown) { return getApiErrorMessage(err, genericError) @@ -326,6 +358,7 @@ export function useStorageAdmin(genericError: string, conflictError: string): St return { state, + storageBusy, draft, dirty, loading, diff --git a/client/src/mobile/screens/admin/MAdminStoragePanel.tsx b/client/src/mobile/screens/admin/MAdminStoragePanel.tsx index 9c90d2a59..36007caa0 100644 --- a/client/src/mobile/screens/admin/MAdminStoragePanel.tsx +++ b/client/src/mobile/screens/admin/MAdminStoragePanel.tsx @@ -291,8 +291,7 @@ export default function MAdminStoragePanel(): React.ReactElement { useEffect(() => { if (migrationQueue.length === 0 || !admin.state) return if (migrationStartInFlight.current) return - if (admin.state.migrations.some((m) => m.status === 'running')) return - if (admin.state.backfills.some((b) => b.status === 'running')) return + if (admin.storageBusy()) return const [next, ...rest] = migrationQueue migrationStartInFlight.current = true setMigrationQueue(rest)