Skip to content

Stop safe mode containers on cancel and cap concurrent runs - #31

Open
Jabe03 wants to merge 10 commits into
kind2-mc:mainfrom
Jabe03:safe-mode-container-cleanup
Open

Jabe03 wants to merge 10 commits into
kind2-mc:mainfrom
Jabe03:safe-mode-container-cleanup

Conversation

@Jabe03

@Jabe03 Jabe03 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Stop safe mode containers on cancel and cap concurrent runs

Follow-up to #30. Needed by kind2-mc/kind2-language-server#22, which cancels out-of-date parses: without this, cancelling a safe mode run stops the docker client but can leave the container running.

Changes

  • Shared docker run command: getDockerRunCommand() builds the resource limits and the Kind 2/solver mounts once, for checks, the interpreter and checkAvailable().
  • --init: the container runs an init process as PID 1, so signals reach Kind 2 and orphaned solver processes are reaped.
  • Killing containers: each safe mode run gets a unique --name (kind2-api-<uuid>). If the run is cancelled or the process is still alive when it ends, the API runs docker kill on that container before destroying the client process. Destroying the client alone doesn't reliably stop the container (for example on Windows).
  • Concurrent run cap: a semaphore shared by every Kind2Api instance limits how many safe mode runs happen at once (default 2, set with the new static setSafeModeMaxRuns(int)). Waiting runs still respond to their monitor's cancellation.
  • checkAvailable() in safe mode now runs Kind 2 inside the container, and waits for the process instead of reading exitValue() before it has exited.
  • --no_include is only passed when safe mode is on (it used to be passed as --no_include false when safe mode was explicitly off).
  • The two interpret overloads now share one runInterpreter method.
  • After review: the docker kill timeout now applies; finished runs get a short grace period instead of being killed; setSafeModeMaxRuns resizes the limit in place, so it also applies to waiting runs; slots are held with try-with-resources; the interpreter stops waiting for a slot after 60 s; one helper builds the Kind 2 command prefix; the interpreter reads stderr and reports it when no trace comes back; programs are sent as UTF-8.

Outside safe mode the command line is unchanged.

Testing

  • ./gradlew clean build passes.
  • With real containers and a statically linked fake kind2 (sleeps, then prints a trace): finished checks and interpreter runs call no docker kill, a cancelled check is killed with no container left, raising and lowering the limit while runs wait takes effect right away, and the interpreter gives up after 60 s when no slot is free.
  • Cancelling a safe mode run through Kind2Api (setSafeMode(true)) left no running containers and no leftover z3 processes in 4 of 4 tries.

🤖 Generated with Claude Code

@Jabe03
Jabe03 marked this pull request as draft October 16, 2026 17:06

@Jabe03 Jabe03 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the safe mode run lifecycle. I'll address each comment with its own commit and reply here.

Not raised here: a cancel that arrives before Docker creates the container (pending issue 3 in the safe mode notes), since how to handle it hasn't been decided yet.

Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java
Comment thread src/main/java/edu/uiowa/cs/clc/kind2/api/Kind2Api.java Outdated
Jabe03 and others added 8 commits October 16, 2026 12:18
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When Kind 2's output ends, the docker client is usually still waiting for
the container's exit status, so checking isAlive() right away made almost
every finished run call docker kill.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replacing the semaphore left callers that were already waiting on the old
one, so a lower limit was not enforced and a higher one did not wake them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
acquireSafeModeRun now returns a closeable slot (a no-op one outside safe
mode) and null only when cancelled, so callers can't forget the release.
It also checks the monitor once more after acquiring, so a run cancelled
just as a slot frees up does not start a container.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The interpreter has no monitor, so a call made while the run limit was
reached blocked the caller's thread until a long check finished. It now
throws a Kind2Exception instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
checkAvailable, checks and the interpreter now share getKind2Command, so
checkAvailable runs the same command that execute does. The generated
command lines are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stderr was never read, so docker errors in safe mode were lost, and enough
stderr output blocked Kind 2 and hung the run. It is now read on its own
thread, and if no trace comes back, interpret throws a Kind2Exception with
it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It was encoded with the platform's default charset while the output is
read as UTF-8, so non-ASCII programs broke on Windows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Jabe03
Jabe03 marked this pull request as ready for review October 23, 2026 17:01

@daniel-larraz daniel-larraz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the safe mode run management and interpreter error reporting. I checked each finding against the code; gson behavior was tested against 2.8.6.

The two worth fixing first are on the new interpreter error-reporting path: harmless stderr now replaces the empty result (L407), and parse failures escape as raw gson exceptions (L398). The rest are lower severity: cleanup/race edge cases, and a few smaller consistency and robustness points.

🤖 Generated with Claude Code

errorReader.join(DOCKER_EXIT_GRACE_MILLIS);
String errorOutput = errors.toString().trim();
if (!errorOutput.isEmpty()) {
throw new Kind2Exception("Error running the Kind 2 interpreter: " + errorOutput);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harmless stderr output now replaces the empty result. On main the interpreter never read stderr, so a run without a trace returned "". Now any stderr text throws when there is no trace. For example, on hosts without swap accounting docker prints WARNING: Your kernel does not support swap limit capabilities ... on every run that sets --memory/--memory-swap. A Kind 2 error reported as JSON on stdout (no trace element) then turns into Error running the Kind 2 interpreter: WARNING: ..., which hides the real error.

Suggestion: report stderr only when stdout produced no output at all, or attach it to the message instead of changing the empty-trace behavior.

trace = jele.getAsJsonObject().get("trace").toString();
}
}
} catch (JsonIOException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only JsonIOException is caught here, and it is rethrown raw. I checked with gson 2.8.6 (JsonStreamParser is lenient):

  • empty stdout → JsonIOException (End of input). When stderr is also empty, throw outputError lets it escape unwrapped, not as a Kind2Exception.
  • non-JSON stdout such as Error: docker failed → JsonSyntaxException. That isn't caught, so it goes past catch (IOException) and the new stderr report is skipped.

Catching JsonParseException (the parent of both) and wrapping it in Kind2Exception would fix both cases.

*/
public String checkAvailable() throws Exception {
ProcessBuilder builder = new ProcessBuilder(KIND2, "--version");
List<String> command = getKind2Command(null, new ArrayList<>());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In safe mode, checkAvailable now starts a full docker run outside the new run management. It uses the per-run CPU/memory limits but takes no run slot, gives the container no --name, and sets no timeout. If docker hangs (image pull, stuck daemon), readAll + waitFor() block forever, and there is no name to docker kill.

if (process != null) {
// Destroying the docker client does not reliably stop the container (for example on
// Windows), so kill the container itself if the run did not finish on its own.
if (monitor.isCanceled() || !exitsOnItsOwn(process, containerName)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The kill can race with container creation. If the run is cancelled while docker run is still pulling or creating, docker kill <name> fails with "No such container" (only logged through debug). Then process.destroy() kills the client.

  • If the client dies before it sends start, the container is left in the Created state. It doesn't run, but --rm never removes it.
  • If the client sends start in the window between the failed kill and destroy(), the container runs with no owner after the slot is released. On Linux the SIGTERM from destroy() is normally forwarded to the container (sig-proxy), so this mostly matters on Windows, where the client is terminated without a signal.

Retrying the kill, or running docker rm -f <name> after the client exits, would close the gap.

exceptionThrown = true;
throw t;
} finally {
RunSlot slot = acquireSafeModeRun(monitor, 0);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute(String) / execute(Program) can wait forever for a slot. Their monitors never cancel, and the timeout here is 0, so once all slots are held (e.g. by hung runs) these calls block in the tryAcquire loop with no error. That matches the Javadoc ("0 to wait as long as it takes"), but it's inconsistent with interpret, which gives up after 60 s.

return process.waitFor(DOCKER_EXIT_GRACE_MILLIS, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
return !process.isAlive();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An interrupted thread skips the kill wait. After an interrupt this re-sets the flag and returns !isAlive() (usually false), so killContainer runs with the flag set and kill.waitFor throws at once. The docker kill child still runs and kills the container, so this is low severity, but:

  • the slot is released before the container is confirmed stopped, so a new run can briefly go over the cap;
  • a docker kill that hangs is never destroyed.

Clearing the interrupt flag around the cleanup and restoring it afterwards would avoid this.

private void callKind2(String program, Result result, IProgressMonitor monitor, ResultListener listener)
throws IOException, InterruptedException {
ProcessBuilder builder = getKind2ProcessBuilder();
String containerName = newContainerName();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safeMode is read several times per run (newContainerName, getKind2Command, acquireSafeModeRun). A concurrent setSafeMode(true) can produce a docker run with no --name (which killContainer/exitsOnItsOwn then treat as native), or a native run that holds a slot. Changing settings during a run isn't really supported, since every setting is a plain field, so this is minor. Reading safeMode once into a local and passing it down is a cheap fix.

monitor.done();
return;
}
try (RunSlot heldSlot = slot) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: two styles for holding a slot. Here the slot is null-checked and then rebound as heldSlot, while runInterpreter acquires it in the try header. The nullable return (null only on cancel) means every caller has to remember the null check before try-with-resources. Throwing on cancel, or a RunSlot with a cancelled flag, would allow a single pattern.

final StringBuffer errors = new StringBuffer();
Thread errorReader = new Thread(() -> {
try {
errors.append(ApiUtil.readAll(errorStream));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: ApiUtil.readAll turns each byte into a char ((char) i), so the stderr text in the new error message is decoded as Latin-1. A docker error that quotes a non-ASCII host path (e.g. /home/josé/...) comes out garbled, unlike the UTF-8 handling this PR adds for stdin and stdout. Reading through an InputStreamReader with UTF-8 would fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants