Skip to content

fix(workspaces): enforce readiness command deadlines - #357

Open
TheNaubit wants to merge 1 commit into
theam:mainfrom
TheNaubit:fix/workspace-readiness-deadline
Open

TheNaubit wants to merge 1 commit into
theam:mainfrom
TheNaubit:fix/workspace-readiness-deadline

Conversation

@TheNaubit

@TheNaubit TheNaubit commented Sep 9, 2026 •

Copy link
Copy Markdown

What changes

Give each readiness command only the time left before the deadline. This also covers the first check when reopening a prepared workspace, and rejects a successful result if it arrives too late.

A runtime timeout becomes environment_not_ready; other runtime errors keep their existing behavior. Setup and start commands keep their longer timeout.

This is a nonbreaking timeout fix with no migration or permission/budget changes. Timing out a readiness check keeps the prepared files and data and doesn't rerun the seed step.

Why

Closes #353

The two-minute deadline only controlled when the loop started another check. One hanging command could still make the operation wait for up to 30 minutes.

Verification

I ran the checks in a local Docker Sandbox microVM, with no host directories mounted.

  • pnpm --filter @facility/api exec vitest run test/project-readiness.test.ts test/project-environment.integration.test.ts --fileParallelism=false passed all 15 tests.
  • The tests cover retries with less time remaining, late success, timeout errors, healthy workspace reuse and unrelated runtime errors. The integration test stops a real sleeping command and checks that the setup and seed data survive.
  • API typecheck and pnpm verify passed.

I couldn't complete the Docker workspace checks. The published runner is linux/amd64, and the local sandbox is ARM64. It exits with exec /usr/local/bin/docker-entrypoint.sh: exec format error; the test then gets HTTP 409 because the container has stopped. Trying emulation inside the sandbox didn't fix it. These checks still need to pass on a compatible runner before merge.

  • pnpm verify passes locally
  • Behaviour verified beyond the test suite (say how)
  • Documentation updated, or no user-facing change

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d029dba663

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +560 to +565
const result = await this.command(
input,
input.manifest.environment.ready ?? "",
primaryPath(input.credentials),
remaining,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Make Docker readiness timeouts hard deadlines

With the Docker workspace driver, passing remaining here does not establish a hard deadline: DockerWorkspaceRuntime.exec only asynchronously sends SIGTERM when its timer fires, then continues awaiting the exec stream's end. A configured ready command that traps SIGTERM (or leaves a descendant holding stdout/stderr open) will keep that stream open indefinitely, so this await never rejects with workspace_command_timeout and prepare/preview operations still hang past the two-minute budget. Make the runtime timeout settle independently of stream closure and terminate/escalate the command process before relying on it here.

Useful? React with 👍 / 👎.

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing this.

I confirmed the Docker issue noted in the existing review: a 100 ms readiness deadline still waited about 2 seconds for sleep 2 to finish. Please make the timeout settle independently of stream closure, ensure the command is terminated, and add regression coverage.

@Lob26

Lob26 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

On the Docker behaviour adrian flagged, that a 100 ms deadline still waited about 2 s for sleep 2: I think the cause is in the runtime rather than in your deadline arithmetic, and it is one line.

DockerWorkspaceRuntime.exec settles on stream closure, not on the timer:

const timeout = command.timeoutMs
  ? setTimeout(() => { timedOut = true; void this.terminateExecution(container, execution); }, command.timeoutMs)
  : undefined;
try {
  await waitForStream(stream);      // <- the promise waits here
  ...
  if (timedOut) throw new WorkspaceRuntimeError("workspace_command_timeout", ...);

The timer only sets a flag and fires terminateExecution, which resolves the exec's pid and sends kill -TERM through a second exec. So await exec(...) resolves when the attached stream closes, and timedOut is read after that. Two things stretch it: the round trip to inspect and start the killer, and SIGTERM reaching only the pid of the exec's own process, so a shell that spawned a child keeps the stream open until the child exits.

That is why passing a smaller deadline down does not shorten the wall clock. Making the timeout settle independently, the way adrian asked, probably means racing waitForStream against the timer in exec and treating the terminate as best-effort cleanup, rather than changing anything in project-environment.ts.

Worth flagging that this touches the same path as #324, which changes the workspace bootstrap and the readiness probe in docker.ts. Neither of us can see the other side from our own branch, so whichever lands second will want a look at the merge.

I have the Docker side loaded from that work, so if you want a patch for the exec race against your branch, say the word. Equally happy to leave it entirely with you.

This branch has not been deployed

No deployments
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.

Readiness checks can run past the two-minute timeout

3 participants