mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(storage): ask the hook whether a storage job is running, not a stale closure
Every push to dev has been red since the 4.0.0 bump, always on the same test: FE-MOB-MSTOR-015, where the second queued migration POSTs while the first is still running. It passes on the PR runner and fails on the slower push one. Instrumenting the queue effect logs the whole story: DEQUEUE files stateMigrations [] APPLY refresh ["running"] START RESOLVED files error null DEQUEUE journey stateMigrations [] The refresh had already landed the running migration before the single-flight lock was released, exactly as intended. What fires afterwards is the effect of a commit scheduled BEFORE the POST, so its captured admin.state is the pre-POST world, and it runs late enough that the lock is gone. The lock is a ref and is read live; the "is one already running" test was read off that dead closure. Under load the two drift apart and a second migration starts on top of a running one. So the hook answers the question instead. It keeps its world in a ref as well, written where the response is applied rather than when the render lands, and storageBusy() reads that. The queue effect still WAKES on admin.state, it just no longer DECIDES on it, and the server's one-job-at-a-time rule now lives with the hook that talks to the server rather than twice in two shells. Same shape as the dirtyRef/savingRef the hook already keeps for the same reason. An earlier attempt made the panels refresh again after startMigration, which was both redundant (startMigration has awaited its own refresh since #1949) and no fix at all: the stale read is on the next effect run, not on that promise.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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> = {}): 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 <div data-testid="ready">{admin.state ? 'loaded' : 'loading'}</div>;
|
||||
}
|
||||
|
||||
describe('useStorageAdmin', () => {
|
||||
async function mount(): Promise<() => StorageAdmin> {
|
||||
let latest: StorageAdmin | null = null;
|
||||
render(<Probe onReady={(a) => { 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);
|
||||
});
|
||||
});
|
||||
@@ -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<StorageAdminState | null>(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<string | null> => {
|
||||
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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user