mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-10-25 13:11:05 +00:00
fix(frontend): stop client-side preference changes refetching all settings (#2008)
* fix(frontend): stop client-side preference changes refetching all settings The Playwright suite was failing intermittently with HTTP 429 from the global API limiter. Instrumenting a real run showed why: 22,177 API requests over 19.6 minutes, 58% of them 304 Not Modified, and the shared admin bucket averaging about 970 requests/min against its 1000 ceiling, so ordinary bursts were rejected. The failures surfaced far from the burst, as the stacks view failing to load and the Create Stack button never appearing, which reads like an unrelated UI fault rather than a rate-limit problem. GET /api/settings was a quarter of all traffic, peaking at 461/min with 3,810 of 4,781 responses being 304s. useDeveloperMode treated a settings-changed event with no changedKeys as "refetch", and fourteen dispatch sites across ten hooks broadcast with no detail at all. Every one of them is a client-side preference backed by localStorage: pane layout (four sites), density, top-nav alignment, mode and labels, log chip colours, deploy feedback, quick links. So dragging a panel divider or nudging the top nav pulled the whole server-side settings document over the network for a change with no server-side effect. useDeveloperMode now refetches only when changedKeys names developer_mode, matching the two listeners that already gate this way. All three server-side write sites dispatch changedKeys, so a real developer_mode toggle still propagates immediately. The non-production ceiling of the global limiter moves to 10000 so the suite, which shares one account across every spec, stops tripping it. Production is untouched at 200, still configurable with API_RATE_LIMIT. The two changes are complements: this removes a real defect, the ceiling gives the suite headroom. * test: rescale global rate-limit assertions to the new non-production ceiling The pilot enrollment suite asserts which limiter applies by reading the ratelimit-limit header, and hardcoded the global tier at 1000. That value moved, so the assertion failed in CI. The enrollment tier's own ceiling of 100 is unchanged, so the test still proves that a proxy-mode node creation falls back to the global tier rather than the enrollment tier. Two floors in the rate-limiting suite were missed in the previous commit. They read at least 990 remaining, which against a 10000 ceiling no longer constrains anything: the assertions passed for a reason unrelated to what they were checking. Rescaled to 9990 so they keep their original meaning, that at most ten requests were spent from the shared budget. Note that these two assertions were already weak before this change: one request per user leaves 9999 remaining when the buckets are independent and 9998 when they are shared, so a bound of 990 admitted both. They still do. Tightening them to 9999 would make them prove the independence their name claims, which is worth doing separately.
This commit is contained in:
@@ -406,7 +406,7 @@ describe('Enrollment rate limiter wiring', () => {
|
||||
.send({ name: 'pilot-headers', type: 'remote', mode: 'pilot_agent' });
|
||||
|
||||
const limit = parseInt(res.headers['ratelimit-limit'], 10);
|
||||
// Dev limit is 100/min for the enrollment limiter; global is 1000/min.
|
||||
// Dev limit is 100/min for the enrollment limiter; global is 10000/min.
|
||||
expect(limit).toBe(100);
|
||||
});
|
||||
|
||||
@@ -422,7 +422,7 @@ describe('Enrollment rate limiter wiring', () => {
|
||||
});
|
||||
|
||||
const limit = parseInt(res.headers['ratelimit-limit'], 10);
|
||||
expect(limit).toBe(1000);
|
||||
expect(limit).toBe(10000);
|
||||
});
|
||||
|
||||
it('POST /api/nodes/:id/pilot/enroll uses the enrollment limiter', async () => {
|
||||
|
||||
@@ -9,7 +9,7 @@
|
||||
* - Authenticated requests are keyed by user session, not IP
|
||||
*
|
||||
* These tests run against the real Express app with in-memory SQLite.
|
||||
* Rate limits in development mode are 10x production values (1000/3000/5000),
|
||||
* Rate limits in development mode are well above production values (10000/3000/5000),
|
||||
* so we test header presence and tier separation rather than hitting ceilings.
|
||||
*/
|
||||
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
|
||||
@@ -106,16 +106,16 @@ describe('Tier 2: Standard endpoints', () => {
|
||||
it('standard endpoints return global-tier rate limit headers', async () => {
|
||||
const res = await request(app).get('/api/stacks');
|
||||
const limit = parseInt(res.headers['ratelimit-limit'], 10);
|
||||
// Dev mode global limit is 1000 (production would be 200)
|
||||
expect(limit).toBe(1000);
|
||||
// Dev mode global limit is 10000 (production would be 200)
|
||||
expect(limit).toBe(10000);
|
||||
});
|
||||
|
||||
it('standard endpoints do NOT get polling-tier headers', async () => {
|
||||
const res = await request(app).get('/api/containers');
|
||||
const limit = parseInt(res.headers['ratelimit-limit'], 10);
|
||||
// Should be 1000 (global), not 3000 (polling)
|
||||
// Should be 10000 (global), not 3000 (polling)
|
||||
expect(limit).not.toBe(3000);
|
||||
expect(limit).toBe(1000);
|
||||
expect(limit).toBe(10000);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -164,7 +164,7 @@ describe('Node proxy bypass', () => {
|
||||
.get('/api/stacks')
|
||||
.set('Authorization', `Bearer ${scopedToken('node_proxy', 'wrong-secret')}`);
|
||||
|
||||
expect(parseInt(res.headers['ratelimit-limit'], 10)).toBe(1000);
|
||||
expect(parseInt(res.headers['ratelimit-limit'], 10)).toBe(10000);
|
||||
expect(res.status).toBe(401);
|
||||
});
|
||||
|
||||
@@ -179,7 +179,7 @@ describe('Node proxy bypass', () => {
|
||||
const standard = await request(app)
|
||||
.get('/api/stacks')
|
||||
.set('Authorization', `Bearer ${expired}`);
|
||||
expect(parseInt(standard.headers['ratelimit-limit'], 10)).toBe(1000);
|
||||
expect(parseInt(standard.headers['ratelimit-limit'], 10)).toBe(10000);
|
||||
});
|
||||
|
||||
it('valid pilot_tunnel token remains rate-limited (not exempt)', async () => {
|
||||
@@ -187,7 +187,7 @@ describe('Node proxy bypass', () => {
|
||||
.get('/api/stacks')
|
||||
.set('Authorization', `Bearer ${scopedToken('pilot_tunnel')}`);
|
||||
|
||||
expect(parseInt(res.headers['ratelimit-limit'], 10)).toBe(1000);
|
||||
expect(parseInt(res.headers['ratelimit-limit'], 10)).toBe(10000);
|
||||
});
|
||||
|
||||
it('requests with invalid Bearer token still get rate limited', async () => {
|
||||
@@ -210,7 +210,7 @@ describe('Node proxy bypass', () => {
|
||||
.get('/api/stacks')
|
||||
.set('Authorization', `Bearer ${userToken}`);
|
||||
|
||||
expect(parseInt(res.headers['ratelimit-limit'], 10)).toBe(1000);
|
||||
expect(parseInt(res.headers['ratelimit-limit'], 10)).toBe(10000);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -234,9 +234,9 @@ describe('Hybrid key generator', () => {
|
||||
// Each should have its own budget. Floor is suite-aware: earlier cases in
|
||||
// this file (including forged/expired node_proxy requests keyed by IP)
|
||||
// already spent some of the anonymous bucket; independence still holds as
|
||||
// long as both remain near the 1000 ceiling.
|
||||
expect(remaining1).toBeGreaterThanOrEqual(980);
|
||||
expect(remaining2).toBeGreaterThanOrEqual(980);
|
||||
// long as both remain near the 10000 ceiling.
|
||||
expect(remaining1).toBeGreaterThanOrEqual(9980);
|
||||
expect(remaining2).toBeGreaterThanOrEqual(9980);
|
||||
});
|
||||
|
||||
it('two different users get independent rate limit budgets', async () => {
|
||||
@@ -260,8 +260,8 @@ describe('Hybrid key generator', () => {
|
||||
const remainB = parseInt(resB.headers['ratelimit-remaining'], 10);
|
||||
|
||||
// Each user's budget should be near-full
|
||||
expect(remainA).toBeGreaterThanOrEqual(990);
|
||||
expect(remainB).toBeGreaterThanOrEqual(990);
|
||||
expect(remainA).toBeGreaterThanOrEqual(9990);
|
||||
expect(remainB).toBeGreaterThanOrEqual(9990);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -305,7 +305,7 @@ describe('Tier separation under load', () => {
|
||||
const remaining = parseInt(stdRes.headers['ratelimit-remaining'], 10);
|
||||
|
||||
// Should have lost very few from the standard budget (only from other tests)
|
||||
expect(remaining).toBeGreaterThanOrEqual(980);
|
||||
expect(remaining).toBeGreaterThanOrEqual(9980);
|
||||
});
|
||||
|
||||
it('mixed polling and standard requests maintain independent counters', async () => {
|
||||
@@ -331,7 +331,7 @@ describe('Tier separation under load', () => {
|
||||
// Check that standard results have global-tier headers
|
||||
for (let i = 1; i < results.length; i += 2) {
|
||||
const limit = parseInt(results[i].headers['ratelimit-limit'], 10);
|
||||
expect(limit).toBe(1000);
|
||||
expect(limit).toBe(10000);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -145,11 +145,18 @@ const rateLimitBase = {
|
||||
|
||||
// Tier 2: Global API rate limiter. Skips polling endpoints (Tier 0/1), webhook
|
||||
// triggers (Tier W), and internal node-to-node traffic (node_proxy).
|
||||
//
|
||||
// The non-production ceiling is high on purpose. The E2E suite drives one admin
|
||||
// account across every spec, so its aggregate request rate is not comparable to
|
||||
// a single operator's: a measured 19.6-minute run averaged roughly 970
|
||||
// requests/min on that shared bucket against the previous 1000 limit, so
|
||||
// ordinary bursts returned 429 and failed tests far from the burst. Production
|
||||
// keeps its own ceiling.
|
||||
export const globalApiLimiter = rateLimit({
|
||||
...rateLimitBase,
|
||||
max: process.env.NODE_ENV === 'production'
|
||||
? parseInt(process.env.API_RATE_LIMIT || '200', 10)
|
||||
: 1000,
|
||||
: 10000,
|
||||
keyGenerator: rateLimitKeyGenerator,
|
||||
message: { error: 'Too many requests. Please try again shortly.' },
|
||||
skip: (req: Request) => {
|
||||
|
||||
@@ -93,4 +93,20 @@ describe('useDeveloperMode', () => {
|
||||
});
|
||||
expect(mockedFetch.mock.calls.length).toBe(callsBefore);
|
||||
});
|
||||
|
||||
// The client-side preference hooks broadcast with no detail at all, and there
|
||||
// are fourteen such sites. Each one used to pull the whole settings document
|
||||
// over the network for a change with no server-side effect, which is what
|
||||
// drove the shared rate-limit budget to its ceiling during the E2E suite.
|
||||
it('ignores a detail-less settings broadcast from a client-side preference', async () => {
|
||||
mockedFetch.mockResolvedValue(settingsResponse('0'));
|
||||
const { result } = renderHook(() => useDeveloperMode(1));
|
||||
await waitFor(() => expect(result.current).toBe(false));
|
||||
|
||||
const callsBefore = mockedFetch.mock.calls.length;
|
||||
act(() => {
|
||||
window.dispatchEvent(new CustomEvent(SENCHO_SETTINGS_CHANGED));
|
||||
});
|
||||
expect(mockedFetch.mock.calls.length).toBe(callsBefore);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -64,12 +64,16 @@ export function useDeveloperMode(activeNodeId: number | undefined): boolean {
|
||||
void refreshRef.current();
|
||||
}, [activeNodeId]);
|
||||
|
||||
// Propagate a developer-mode toggle immediately. Refetch when the change set
|
||||
// names developer_mode, or when the detail is missing (unknown change set).
|
||||
// Propagate a developer-mode toggle immediately. Only a broadcast that names
|
||||
// developer_mode can change this flag: every server-side settings write
|
||||
// dispatches changedKeys, and the broadcasts without it are the client-side
|
||||
// preference hooks (pane layout, density, top nav, quick links) that only
|
||||
// touch localStorage. Refetching on those would pull the whole settings
|
||||
// document over the network for a change with no server-side effect.
|
||||
useEffect(() => {
|
||||
const handler = (e: Event) => {
|
||||
const detail = (e as CustomEvent<Partial<SenchoSettingsChangedDetail>>).detail;
|
||||
if (!detail?.changedKeys || detail.changedKeys.includes('developer_mode')) {
|
||||
if (detail?.changedKeys?.includes('developer_mode')) {
|
||||
void refreshRef.current();
|
||||
}
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user