Skip to content

Commit de617db

Browse files
X-GuardianSimon Heather
andauthored
fix(cli): release terraform state lock when interrupting diff/deploy (#284)
### Related issue Fixes #283 ### Description Interrupting `cdktn diff` or `cdktn deploy` (Ctrl-C) left terraform's state lock held, so the next run failed with `Error acquiring the state lock`. The cause was that the CLI's abort handler tore terraform down instead of letting it exit on its own. Terraform shares the CLI's process group, so Ctrl-C already reaches it directly and starts its graceful shutdown (which releases the lock) — but the CLI killed it before that shutdown could finish. The fix is to stop tearing terraform down on abort and instead wait for it to exit: - **diff/plan** ([`@cdktn/commons/src/util.ts`](packages/@cdktn/commons/src/util.ts)): `exec()` no longer forwards the abort to `child_process.spawn`'s `signal` option, which killed the child with SIGTERM and rejected the promise immediately (synthetic `AbortError`) before terraform had exited — so terraform never ran its graceful shutdown and the lock leaked. It now settles on the child's own `close`, so the run resolves only once terraform has exited and released the lock. - **deploy/destroy** ([`@cdktn/cli-core/src/lib/models/deploy-machine.ts`](packages/@cdktn/cli-core/src/lib/models/deploy-machine.ts)): the xstate machine previously went straight to the final `stopped` state on `STOP`, killing the pty and resolving the run before terraform had finished shutting down. It now passes through an intermediate `stopping` state that marks the run cancelled and waits for terraform's natural `EXITED` before reaching `stopped`. The pty stays alive throughout, so terraform exits on its own and the run only resolves afterwards. **Why no extra signal is needed:** every abort trigger in the codebase (`runCdktfProject`, `watch`) originates from `process.on(SIGINT/SIGTERM/SIGQUIT)`, so terraform — sharing the process group — always receives the signal directly. There is no abort path that requires the CLI to deliver the signal itself. **Verification:** confirmed after the fix that a single Ctrl-C produces only `Interrupt received... Gracefully shutting down...` and terraform exits cleanly on its own. The full lock-release path (interrupt a locking operation, then re-run) still needs a final check against a backend that locks state. ### Checklist - [x] I have updated the PR title to match [CDKTN's style guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1) - [x] I have run the linter on my code locally - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the [documentation](https://github.com/open-constructs/cdk-terrain-docs/tree/main/content) if applicable - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works if applicable - [x] New and existing unit tests pass locally with my changes Co-authored-by: Simon Heather <simon.heather@yulife.com>
1 parent 6d3657a commit de617db

3 files changed

Lines changed: 65 additions & 2 deletions

File tree

packages/@cdktn/cli-core/src/lib/models/deploy-machine.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,10 @@ export type DeployState =
7575
value: { running: "awaiting_sentinel_override" };
7676
context: DeployContext;
7777
}
78+
| {
79+
value: { running: "stopping" };
80+
context: DeployContext;
81+
}
7882
| {
7983
value: "exited";
8084
context: DeployContext & { exitCode: number };
@@ -230,7 +234,7 @@ export const deployMachine = createMachine<
230234
},
231235
on: {
232236
EXITED: "exited",
233-
STOP: "stopped",
237+
STOP: ".stopping", // wait for terraform to exit, don't stop immediately (see the "stopping" state)
234238
},
235239
initial: "processing",
236240
states: {
@@ -309,6 +313,15 @@ export const deployMachine = createMachine<
309313
},
310314
},
311315
},
316+
// On STOP, wait for terraform's own EXITED before reaching the final "stopped" state, so the run only
317+
// resolves once it has exited and released its lock. Don't re-signal it — it already got the interrupt via
318+
// the process group, and a second signal aborts its graceful shutdown.
319+
stopping: {
320+
entry: assign<DeployContext, DeployEvent>({ cancelled: true }),
321+
on: {
322+
EXITED: "#root.stopped",
323+
},
324+
},
312325
},
313326
},
314327
exited: { type: "final" },

packages/@cdktn/cli-core/src/test/models/deploy-machine.test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,52 @@ describe("pty events", () => {
227227
});
228228
});
229229

230+
it("waits for terraform to exit on STOP rather than stopping immediately", (done) => {
231+
// The pty stays alive until we resolve its exit, letting us assert the machine waits for terraform's own exit
232+
// before reaching "stopped".
233+
const stop = jest.fn();
234+
let resolveExit: (code: number) => void = () => {};
235+
const controllablePty: typeof spawnInteractive = () => ({
236+
actions: { write: jest.fn(), writeLine: jest.fn(), stop },
237+
exitCode: new Promise<number>((resolve) => {
238+
resolveExit = resolve;
239+
}),
240+
});
241+
242+
const mockDeployMachine = deployMachine.withConfig({
243+
services: {
244+
runTerraformInPty: (context, event) =>
245+
terraformPtyService(context, event, controllablePty),
246+
},
247+
});
248+
249+
let interrupted = false;
250+
const ptyService = interpret(mockDeployMachine).onTransition((state) => {
251+
if (state.matches({ running: "stopping" }) && !interrupted) {
252+
interrupted = true;
253+
// We are waiting for terraform to exit, not yet at the final "stopped" state, and we have not re-signalled it.
254+
expect(state.matches("stopped")).toBe(false);
255+
expect(stop).not.toHaveBeenCalled();
256+
setTimeout(() => resolveExit(0), 50); // terraform finishes exiting, releasing its lock
257+
}
258+
259+
if (state.matches("stopped")) {
260+
expect(state.context.cancelled).toBe(true);
261+
done();
262+
}
263+
});
264+
265+
ptyService.start();
266+
267+
ptyService.send({
268+
type: "START",
269+
pty: { file: "", args: [], options: { cwd: "" } },
270+
});
271+
272+
// Give the pty service a tick to start before requesting a stop.
273+
setTimeout(() => ptyService.send({ type: "STOP" }), 50);
274+
});
275+
230276
it("transitions to rejected state when done externally", (done) => {
231277
const mockDeployMachine = deployMachine.withConfig({
232278
services: {

packages/@cdktn/commons/src/util.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,12 @@ export const exec = async (
136136
options.noColor = true;
137137
}
138138

139+
// Drop spawn's `signal` option: the child already gets the interrupt via the process group, and a second signal
140+
// aborts terraform's graceful shutdown. Just wait for its own "close".
141+
const { signal: _signal, ...spawnOptions } = options;
142+
139143
return new Promise((ok, ko) => {
140-
const child = spawn(command, args, options);
144+
const child = spawn(command, args, spawnOptions);
141145
const out = new Array<string>();
142146
const err = new Array<string>();
143147
if (stdout !== undefined) {

0 commit comments

Comments
 (0)