Conversation
There was a problem hiding this comment.
💡 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".
| const result = await this.command( | ||
| input, | ||
| input.manifest.environment.ready ?? "", | ||
| primaryPath(input.credentials), | ||
| remaining, | ||
| ); |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
On the Docker behaviour adrian flagged, that a 100 ms deadline still waited about 2 s for
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 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 Worth flagging that this touches the same path as #324, which changes the workspace bootstrap and the readiness probe in I have the Docker side loaded from that work, so if you want a patch for the |
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=falsepassed all 15 tests.pnpm verifypassed.I couldn't complete the Docker workspace checks. The published runner is
linux/amd64, and the local sandbox is ARM64. It exits withexec /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 verifypasses locally