From 600367e66cc85dd99bdda86f1d2e3f94389b95ff Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 9 Aug 2026 17:32:45 -0400 Subject: [PATCH] perf(stacks): remove repeated self-identity discovery from statuses hot path (#1811) * perf(stacks): hoist self-stack identity resolution out of statuses enrichment GET /api/stacks/statuses resolved the self-stack identity once per stack on every request, including cache hits. The fallback path in isSelfStack lists every container on the node, so a cache hit still paid N container-list calls. Measured on a Docker-shaped local container with 6 stacks: cache hits took 161-168ms with dockerMs=null; the enrichment alone was ~160ms. Resolve the identity once per request (boot-cached compose project name plus a single container-labels read) and compare stack names against it in the response loop. isSelf semantics are unchanged: project-name match, then labels project match, then working-dir basename match, with the same fail-safe false on identity failure. Also expose an enrichmentMs subspan in the developer-mode timing line so cache-hit cost stays decomposable. Measured after the change in the same environment: cache hits 20-34ms (enrichment ~20-33ms, a single listContainers call), computed 44-59ms. Cache-hit latency improved roughly 5-8x; per-stack Docker identity discovery is eliminated (N calls down to 1). * test(stacks): cover the empty-stacks statuses path The statuses handler skips self-stack identity resolution entirely when a node has no stacks. Pin that branch: an empty response still returns 200 with {} and never resolves identity, so a future refactor cannot silently reintroduce a node-wide container-list call on empty-node polls. --- backend/src/__tests__/cache-endpoints.test.ts | 58 ++++++++++++ .../__tests__/hydration-timing-routes.test.ts | 4 + .../src/__tests__/self-stack-guard.test.ts | 92 +++++++++++++++++++ backend/src/helpers/selfStackGuard.ts | 39 ++++++++ backend/src/routes/stacks.ts | 18 +++- 5 files changed, 208 insertions(+), 3 deletions(-) diff --git a/backend/src/__tests__/cache-endpoints.test.ts b/backend/src/__tests__/cache-endpoints.test.ts index 4ee67584..121ce4a3 100644 --- a/backend/src/__tests__/cache-endpoints.test.ts +++ b/backend/src/__tests__/cache-endpoints.test.ts @@ -20,6 +20,7 @@ import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_PASSWORD } from './help import { installArcstatsFsMock, arcstatsBody, DEFAULT_ARC_PATH, type ArcstatsFsMock } from './helpers/arcstatsFsMock'; import { GitSourceService } from '../services/GitSourceService'; import type { PublicGitSource } from '../services/GitSourceService'; +import * as selfStackGuard from '../helpers/selfStackGuard'; // ── Hoisted mocks (must come before importing the app) ───────────────── @@ -284,6 +285,63 @@ describe('GET /api/stacks/statuses caching', () => { listSpy.mockRestore(); }); + it('resolves self-stack identity once per request, cache hits included, and labels each stack from it', async () => { + mockGetStacks.mockResolvedValue(['sencho.yml', 'web.yml']); + mockGetBulkStackStatuses.mockResolvedValue({ + sencho: { status: 'running' }, + web: { status: 'running' }, + }); + const identitySpy = vi + .spyOn(selfStackGuard, 'resolveSelfStackIdentity') + .mockResolvedValue({ projectName: 'sencho', labels: null }); + + const first = await request(app).get('/api/stacks/statuses').set('Cookie', authCookie); + const second = await request(app).get('/api/stacks/statuses').set('Cookie', authCookie); + + // Hoisted: one resolution per request (including the cache hit) serves + // every stack in the response. + expect(identitySpy).toHaveBeenCalledTimes(2); + for (const res of [first, second]) { + expect(res.body['sencho.yml'].isSelf).toBe(true); + expect(res.body['web.yml'].isSelf).toBe(false); + } + + identitySpy.mockRestore(); + }); + + it('skips identity resolution entirely when the node has no stacks', async () => { + mockGetStacks.mockResolvedValue([]); + mockGetBulkStackStatuses.mockResolvedValue({}); + const identitySpy = vi.spyOn(selfStackGuard, 'resolveSelfStackIdentity'); + + const res = await request(app).get('/api/stacks/statuses').set('Cookie', authCookie); + + expect(res.status).toBe(200); + expect(res.body).toEqual({}); + expect(identitySpy).not.toHaveBeenCalled(); + + identitySpy.mockRestore(); + }); + + it('serves 200 with isSelf false everywhere when identity resolution degrades', async () => { + mockGetStacks.mockResolvedValue(['web.yml', 'db.yml']); + mockGetBulkStackStatuses.mockResolvedValue({ + web: { status: 'running' }, + db: { status: 'running' }, + }); + const identitySpy = vi + .spyOn(selfStackGuard, 'resolveSelfStackIdentity') + .mockResolvedValue({ projectName: null, labels: null }); + + const res = await request(app).get('/api/stacks/statuses').set('Cookie', authCookie); + + expect(res.status).toBe(200); + expect(res.body['web.yml'].isSelf).toBe(false); + expect(res.body['db.yml'].isSelf).toBe(false); + + identitySpy.mockRestore(); + }); + it('falls back to local labels (200, not 500) when the git-source lookup throws', async () => { mockGetStacks.mockResolvedValue(['web.yml']); mockGetBulkStackStatuses.mockResolvedValue({ web: { status: 'running' } }); diff --git a/backend/src/__tests__/hydration-timing-routes.test.ts b/backend/src/__tests__/hydration-timing-routes.test.ts index c7e56016..b1ff439f 100644 --- a/backend/src/__tests__/hydration-timing-routes.test.ts +++ b/backend/src/__tests__/hydration-timing-routes.test.ts @@ -103,6 +103,10 @@ describe('[Stacks:debug] GET /api/stacks/statuses', () => { expect(second).toMatch(/cacheOutcome=hit/); // No docker call on a cache hit, so the subspan is null rather than 0. expect(second).toMatch(/dockerMs=null/); + // Enrichment runs on every request, cache hits included, so its subspan + // is a number on both legs. + expect(first).toMatch(/enrichmentMs=\d+/); + expect(second).toMatch(/enrichmentMs=\d+/); // The compute ran the fetcher exactly once across both requests. expect(dockerCalls).toBe(1); }); diff --git a/backend/src/__tests__/self-stack-guard.test.ts b/backend/src/__tests__/self-stack-guard.test.ts index 4fbc92f3..e4fb0055 100644 --- a/backend/src/__tests__/self-stack-guard.test.ts +++ b/backend/src/__tests__/self-stack-guard.test.ts @@ -3,7 +3,9 @@ import SelfIdentityService from '../services/SelfIdentityService'; import DockerController from '../services/DockerController'; import { isSelfStack, + isSelfStackByIdentity, getSelfStackProjectName, + resolveSelfStackIdentity, SELF_STACK_PROTECTED_CODE, SELF_STACK_PROTECTED_MESSAGE, selfStackProtectedBulkResult, @@ -93,6 +95,96 @@ describe('getSelfStackProjectName', () => { }); }); +describe('resolveSelfStackIdentity + isSelfStackByIdentity', () => { + it('matches the stack whose name equals the resolved project name', async () => { + stubComposeProject('sencho'); + const identity = await resolveSelfStackIdentity(); + expect(isSelfStackByIdentity(identity, 'sencho')).toBe(true); + expect(isSelfStackByIdentity(identity, 'web')).toBe(false); + }); + + it('matches by the container compose project label when the project name is unknown', async () => { + const runtimeId = 'c'.repeat(64); + process.env.HOSTNAME = runtimeId.slice(0, 12); + stubComposeProject(null); + vi.spyOn(DockerController, 'getInstance').mockReturnValue({ + getDocker: () => ({ + listContainers: vi.fn().mockResolvedValue([ + { + Id: runtimeId, + Labels: { 'com.docker.compose.project': 'renamed-project' }, + }, + ]), + }), + } as unknown as ReturnType); + + const identity = await resolveSelfStackIdentity(); + expect(isSelfStackByIdentity(identity, 'renamed-project')).toBe(true); + expect(isSelfStackByIdentity(identity, 'web')).toBe(false); + }); + + it('matches by the working directory basename inside the compose dir', async () => { + const runtimeId = 'd'.repeat(64); + process.env.HOSTNAME = runtimeId.slice(0, 12); + stubComposeProject(null); + vi.spyOn(DockerController, 'getInstance').mockReturnValue({ + getDocker: () => ({ + listContainers: vi.fn().mockResolvedValue([ + { + Id: runtimeId, + Labels: { 'com.docker.compose.project.working_dir': '/app/compose/sencho' }, + }, + ]), + }), + } as unknown as ReturnType); + + const identity = await resolveSelfStackIdentity(); + expect(isSelfStackByIdentity(identity, 'sencho', '/app/compose')).toBe(true); + }); + + it('does not match a working directory outside the compose dir', async () => { + const runtimeId = 'g'.repeat(64); + process.env.HOSTNAME = runtimeId.slice(0, 12); + stubComposeProject(null); + vi.spyOn(DockerController, 'getInstance').mockReturnValue({ + getDocker: () => ({ + listContainers: vi.fn().mockResolvedValue([ + { + Id: runtimeId, + Labels: { 'com.docker.compose.project.working_dir': '/srv/other/sencho' }, + }, + ]), + }), + } as unknown as ReturnType); + + const identity = await resolveSelfStackIdentity(); + expect(isSelfStackByIdentity(identity, 'sencho', '/app/compose')).toBe(false); + }); + + it('is false when no identity source is available', async () => { + stubComposeProject(null); + const identity = await resolveSelfStackIdentity(); + expect(isSelfStackByIdentity(identity, 'sencho')).toBe(false); + }); + + it('resolves identity with at most one container list read, shared by all stacks', async () => { + const runtimeId = 'e'.repeat(64); + process.env.HOSTNAME = runtimeId.slice(0, 12); + stubComposeProject('sencho'); + const listContainers = vi.fn().mockResolvedValue([]); + vi.spyOn(DockerController, 'getInstance').mockReturnValue({ + getDocker: () => ({ listContainers }), + } as unknown as ReturnType); + + const identity = await resolveSelfStackIdentity(); + // Reuse the resolved identity across every stack in the request. + for (const name of ['sencho', 'web', 'db', 'cache']) { + isSelfStackByIdentity(identity, name, '/app/compose'); + } + expect(listContainers).toHaveBeenCalledTimes(1); + }); +}); + describe('selfStackProtectedBulkResult', () => { it('returns a per-stack bulk failure with the protected code', () => { const result = selfStackProtectedBulkResult('sencho'); diff --git a/backend/src/helpers/selfStackGuard.ts b/backend/src/helpers/selfStackGuard.ts index ecf2c18a..770586d6 100644 --- a/backend/src/helpers/selfStackGuard.ts +++ b/backend/src/helpers/selfStackGuard.ts @@ -106,6 +106,45 @@ export async function isSelfStack(stackName: string, composeDir?: string): Promi } } +/** + * Identity sources for the self-stack check, resolved once and reused + * across every stack in a request (the /statuses handler resolves it once + * per request). The compose project name is a boot-cached property lookup; + * the container labels fallback (which lists every container on the node) + * is a single read shared by all stacks. Either source degrades + * independently to null, so a failure in one never discards the other. + */ +export interface SelfStackIdentity { + projectName: string | null; + labels: Record | null; +} + +/** + * Resolves both identity sources. Each failure degrades only its own + * source to null, matching the old per-stack behavior where a labels + * failure never discarded the resolved project name. + */ +export async function resolveSelfStackIdentity(): Promise { + const projectName = await getSelfStackProjectName().catch((error) => { + console.error('Failed to resolve self-stack project name; self-stack check degraded:', error); + return null; + }); + const labels = await getRunningContainerLabels().catch((error) => { + console.error('Failed to resolve self-stack container labels; self-stack check degraded:', error); + return null; + }); + return { projectName, labels }; +} + +/** isSelf semantics identical to isSelfStack() when the identity resolves successfully. */ +export function isSelfStackByIdentity(identity: SelfStackIdentity, stackName: string, composeDir?: string): boolean { + if (identity.projectName === stackName) return true; + const labels = identity.labels; + if (!labels) return false; + if (labels['com.docker.compose.project'] === stackName) return true; + return workingDirMatchesStack(labels['com.docker.compose.project.working_dir'], stackName, composeDir); +} + export interface SelfStackProtectedResult { stackName: string; ok: false; diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index 3ed76d97..e0e42bd2 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -69,7 +69,13 @@ import { parseComposeSelection, defaultEnvPath } from '../helpers/gitSourceSelec import { resolveStackEnvSources, discoverStackLocalEnvFiles } from '../helpers/envFileResolution'; import { STACK_STATUSES_CACHE_TTL_MS } from '../helpers/constants'; import { getTerminalWs, DEPLOY_SESSION_HEADER } from '../websocket/generic'; -import { isSelfStack, refuseIfSelfStack, selfStackProtectedBulkResult } from '../helpers/selfStackGuard'; +import { + isSelfStack, + isSelfStackByIdentity, + refuseIfSelfStack, + resolveSelfStackIdentity, + selfStackProtectedBulkResult, +} from '../helpers/selfStackGuard'; import { getActiveCapabilities, STACK_DOWN_REMOVE_VOLUMES_CAPABILITY, SERVICE_SCOPED_UPDATE_CAPABILITY } from '../services/CapabilityRegistry'; import { ServiceUpdateRecoveryService } from '../services/ServiceUpdateRecoveryService'; import { classifyStackApiPath } from '../helpers/stackRouteAuth'; @@ -335,6 +341,7 @@ stacksRouter.get('/statuses', async (req: Request, res: Response) => { let outcome: 'ok' | 'error' = 'ok'; let cacheOutcome: CacheFetchOutcome | null = null; let dockerMs: number | null = null; + let enrichmentMs: number | null = null; let count = 0; try { const { value: result, outcome: fetchOutcome } = await CacheService.getInstance().getOrFetchWithMeta( @@ -357,6 +364,7 @@ stacksRouter.get('/statuses', async (req: Request, res: Response) => { ); cacheOutcome = fetchOutcome; count = Object.keys(result).length; + const enrichmentStartedAt = Date.now(); // Git-source labels are computed live, outside the cache, so linking or // unlinking a stack's Git source is reflected immediately. The Docker // status portion keeps its short TTL; only the cheap source label is fresh. @@ -368,17 +376,20 @@ stacksRouter.get('/statuses', async (req: Request, res: Response) => { } catch (sourceError) { console.error('Failed to load git sources for status labels; defaulting to local:', sourceError); } + // Self-stack identity is resolved once per request instead of once per + // stack, so cache hits no longer pay N container-list calls. + const selfIdentity = count > 0 ? await resolveSelfStackIdentity() : { projectName: null, labels: null }; const withSource: Record = {}; const composeDir = FileSystemService.getInstance(req.nodeId).getBaseDir(); for (const [stack, info] of Object.entries(result)) { const name = stack.replace(/\.(yml|yaml)$/, ''); - const isSelf = await isSelfStack(name, composeDir); withSource[stack] = { ...info, source: gitStackNames.has(name) ? 'git' : 'local', - isSelf, + isSelf: isSelfStackByIdentity(selfIdentity, name, composeDir), }; } + enrichmentMs = Date.now() - enrichmentStartedAt; res.json(withSource); } catch (error) { outcome = 'error'; @@ -391,6 +402,7 @@ stacksRouter.get('/statuses', async (req: Request, res: Response) => { cacheOutcome, count, dockerMs, + enrichmentMs, elapsedMs: Date.now() - startedAt, outcome, });