[fix] nopy: detect a dependency cycle instead of overflowing the stack
Closes DOCS-AUDIT §6.5, and the substantive half of §1.5. `docs/API.md` has documented a circular-dependency error since it was written; nothing raised it. Two mutually dependent cubes recursed until V8 gave up, and a `RangeError` names no cube — it reads as a nopy crash rather than as a manifest that says something impossible. `BuildContext` now carries a resolution stack: `resolveCube` pushes its (cube, host) pair, delegates the body to `visitCube`, and pops in a `finally`. A pair re-entered while it is still on the stack raises a `NopyUsageError` naming the whole path — `Circular dependency on host1: a → b → c → a`. The whole path, not just the repeated cube, because dependencies are declared dynamically and a hook may `exec` anything at all, so the edge that closed the loop is rarely the one you would guess from the two ends. It has to be a structure of its own. `resolvedCubes` is written by `buildDeployCall`, which runs *after* the descent, so a cycle never reaches it; and it cannot be widened into a "seen" set, because re-entering a *finished* cube with different `param` overrides is exactly what a dependency or a hook is for. That distinction is what the diamond test pins: `shared` is entered twice under `left` and `right` and must still resolve, while `a → b → a` must not. There is still no topological sort and there does not need to be — emission is post-order, so the order already is a topological one. Cycle detection was the one thing a sort would have given that the recursion did not. Six tests: self-dependency, a three-cube loop, the loop reported as usage rather than as a stack overflow, a loop closed by a hook's `exec` rather than a `dependencies()` entry, the diamond, and the same cube on two hosts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCzYTAm9QUhvLNr2EpdagJ
This commit is contained in:
co-authored by
Claude Opus 5
parent
4daf27a3cd
commit
bb8b1bfa5c
@@ -23,6 +23,18 @@ export class BuildContext {
|
|||||||
public readonly cubeSessions: CubeSession[] = [];
|
public readonly cubeSessions: CubeSession[] = [];
|
||||||
private readonly resolvedCubes = new Set<string>();
|
private readonly resolvedCubes = new Set<string>();
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The (cube, host) pairs currently being resolved, innermost last.
|
||||||
|
*
|
||||||
|
* `resolvedCubes` cannot serve here: it is written by `buildDeployCall`, which
|
||||||
|
* runs *after* the recursive descent, so a cycle never reaches it — the two
|
||||||
|
* cubes just recurse until the stack overflows. It cannot be widened into a
|
||||||
|
* "seen" set either, because re-entering a cube with different `param`
|
||||||
|
* overrides is a legitimate thing for a dependency or a hook to do. What is
|
||||||
|
* never legitimate is re-entering one that has not finished.
|
||||||
|
*/
|
||||||
|
private readonly resolving: { cubeId: string; host: string }[] = [];
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
public readonly allCubes: Record<string, Cube>,
|
public readonly allCubes: Record<string, Cube>,
|
||||||
public readonly variables: Variables,
|
public readonly variables: Variables,
|
||||||
@@ -130,6 +142,25 @@ export class BuildContext {
|
|||||||
this.assertVariablesComplete(cube);
|
this.assertVariablesComplete(cube);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Fails a run whose dependency graph loops back on itself.
|
||||||
|
*
|
||||||
|
* Names the whole path rather than just the repeated cube: with dependencies
|
||||||
|
* declared dynamically — and hooks free to `exec` anything at all — the edge
|
||||||
|
* that closed the loop is rarely the one you would guess from the two ends.
|
||||||
|
*/
|
||||||
|
private assertNoCycle(cubeId: string, host: string): void {
|
||||||
|
const at = this.resolving.findIndex((f) => f.cubeId === cubeId && f.host === host);
|
||||||
|
if (at === -1) return;
|
||||||
|
|
||||||
|
const path = [...this.resolving.slice(at).map((f) => f.cubeId), cubeId].join(' → ');
|
||||||
|
throw new NopyUsageError(
|
||||||
|
`Circular dependency on ${host}: ${path}. ` +
|
||||||
|
'A cube cannot depend on itself, directly or through a chain — check the ' +
|
||||||
|
'`dependencies()` of each cube named, and any `before`/`after` hook that calls `exec`.'
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Resolves a cube, its dependencies, and hooks recursively
|
* Resolves a cube, its dependencies, and hooks recursively
|
||||||
*/
|
*/
|
||||||
@@ -145,6 +176,22 @@ export class BuildContext {
|
|||||||
|
|
||||||
log.debug('Resolving cube', { cubeId, host });
|
log.debug('Resolving cube', { cubeId, host });
|
||||||
|
|
||||||
|
this.assertNoCycle(cubeId, host);
|
||||||
|
this.resolving.push({ cubeId, host });
|
||||||
|
try {
|
||||||
|
await this.visitCube(cube, host, overrides);
|
||||||
|
} finally {
|
||||||
|
this.resolving.pop();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The body of {@link resolveCube}, once the cube is known and the cycle guard
|
||||||
|
* has admitted it.
|
||||||
|
*/
|
||||||
|
private async visitCube(cube: Cube, host: string, overrides: CubeVariables): Promise<void> {
|
||||||
|
const cubeId = cube.id;
|
||||||
|
|
||||||
// 1. Declare secrets and schema, then assign overrides and defaults. Both
|
// 1. Declare secrets and schema, then assign overrides and defaults. Both
|
||||||
// declarations have to come first: the cube's first assignment is what
|
// declarations have to come first: the cube's first assignment is what
|
||||||
// seeds the config `env` onto it, and by then it must already be known
|
// seeds the config `env` onto it, and by then it must already be known
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import { z } from 'zod';
|
|||||||
import { BuildContext } from '../src/cubes/dependencies.js';
|
import { BuildContext } from '../src/cubes/dependencies.js';
|
||||||
import { Variables } from '../src/nopy.common.js';
|
import { Variables } from '../src/nopy.common.js';
|
||||||
import type { NopyConfig } from '../src/nopy.config.js';
|
import type { NopyConfig } from '../src/nopy.config.js';
|
||||||
|
import { NopyUsageError } from '../src/nopy.errors.js';
|
||||||
import type { NopySession } from '../src/nopy.session.js';
|
import type { NopySession } from '../src/nopy.session.js';
|
||||||
|
|
||||||
vi.mock('../src/nopy.prompts.js', async () => {
|
vi.mock('../src/nopy.prompts.js', async () => {
|
||||||
@@ -61,6 +62,104 @@ describe('BuildContext error handling', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('BuildContext cycle detection', () => {
|
||||||
|
const depCube = (id: string, deps: string[]) =>
|
||||||
|
new Cube(
|
||||||
|
Manifest.create({ id, name: id, schema: z.object({}), dependencies: () => deps }),
|
||||||
|
`/test/${id}`,
|
||||||
|
'deploy.py'
|
||||||
|
);
|
||||||
|
|
||||||
|
const build = (cubes: Cube[]) =>
|
||||||
|
new BuildContext(
|
||||||
|
Object.fromEntries(cubes.map((c) => [c.id, c])),
|
||||||
|
new Variables(),
|
||||||
|
session(),
|
||||||
|
config,
|
||||||
|
{ method: 'ssh' },
|
||||||
|
{ useDefaults: true }
|
||||||
|
);
|
||||||
|
|
||||||
|
it('rejects a cube that depends on itself', async () => {
|
||||||
|
const context = build([depCube('cube-a', ['cube-a'])]);
|
||||||
|
|
||||||
|
await expect(context.resolveCube('cube-a', 'host1')).rejects.toThrow(
|
||||||
|
'Circular dependency on host1: cube-a → cube-a'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('names the whole path of a longer loop', async () => {
|
||||||
|
const context = build([
|
||||||
|
depCube('cube-a', ['cube-b']),
|
||||||
|
depCube('cube-b', ['cube-c']),
|
||||||
|
depCube('cube-c', ['cube-a']),
|
||||||
|
]);
|
||||||
|
|
||||||
|
await expect(context.resolveCube('cube-a', 'host1')).rejects.toThrow(
|
||||||
|
'Circular dependency on host1: cube-a → cube-b → cube-c → cube-a'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('reports the loop as usage rather than overflowing the stack', async () => {
|
||||||
|
const context = build([depCube('cube-a', ['cube-b']), depCube('cube-b', ['cube-a'])]);
|
||||||
|
|
||||||
|
// The point of the finding: before the guard this recursed until V8 gave up,
|
||||||
|
// and a RangeError names no cube and reads as a nopy crash rather than a
|
||||||
|
// manifest that says something impossible.
|
||||||
|
await expect(context.resolveCube('cube-a', 'host1')).rejects.toBeInstanceOf(NopyUsageError);
|
||||||
|
expect(context.deployCalls).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('catches a loop a hook closes rather than a dependency', async () => {
|
||||||
|
const a = new Cube(
|
||||||
|
Manifest.create({
|
||||||
|
id: 'cube-a',
|
||||||
|
name: 'A',
|
||||||
|
schema: z.object({}),
|
||||||
|
dependencies: () => ['cube-b'],
|
||||||
|
}),
|
||||||
|
'/test/cube-a',
|
||||||
|
'deploy.py'
|
||||||
|
);
|
||||||
|
const b = new Cube(
|
||||||
|
Manifest.create({
|
||||||
|
id: 'cube-b',
|
||||||
|
name: 'B',
|
||||||
|
schema: z.object({}),
|
||||||
|
before: [async (ctx) => ctx.exec('cube-a', {})],
|
||||||
|
}),
|
||||||
|
'/test/cube-b',
|
||||||
|
'deploy.py'
|
||||||
|
);
|
||||||
|
|
||||||
|
await expect(build([a, b]).resolveCube('cube-a', 'host1')).rejects.toThrow(
|
||||||
|
'Circular dependency on host1: cube-a → cube-b → cube-a'
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('allows a diamond, where the shared cube is entered twice but never nested', async () => {
|
||||||
|
const context = build([
|
||||||
|
depCube('top', ['left', 'right']),
|
||||||
|
depCube('left', ['shared']),
|
||||||
|
depCube('right', ['shared']),
|
||||||
|
depCube('shared', []),
|
||||||
|
]);
|
||||||
|
|
||||||
|
await context.resolveCube('top', 'host1');
|
||||||
|
|
||||||
|
expect(context.deployCalls.map((c) => c.cube)).toEqual(['shared', 'left', 'right', 'top']);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('allows the same cube on a different host', async () => {
|
||||||
|
const context = build([depCube('cube-a', [])]);
|
||||||
|
|
||||||
|
await context.resolveCube('cube-a', 'host1');
|
||||||
|
await context.resolveCube('cube-a', 'host2');
|
||||||
|
|
||||||
|
expect(context.deployCalls.map((c) => c.host)).toEqual(['host1', 'host2']);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('BuildContext log configuration', () => {
|
describe('BuildContext log configuration', () => {
|
||||||
const build = (log: NopyConfig['log']) =>
|
const build = (log: NopyConfig['log']) =>
|
||||||
new BuildContext(
|
new BuildContext(
|
||||||
|
|||||||
Reference in New Issue
Block a user