Coordinate concurrent image builds and session startup - #61
Conversation
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Two host-level file locks to stop concurrent enclave processes from stepping on each other: one keyed by runtime image name around the build, one keyed by tool plus project hash around session startup. util.WithFileLock is refactored into AcquireFileLock, which adds a non-blocking probe so callers learn whether they actually waited and get an idempotent release function.
The approach fits the existing lock conventions (config.HostLockPath plus util.HashString, same naming shape as the hoststore and skills-mount locks), and builds/tests pass locally including -race and a GOOS=windows build.
Critical points for a maintainer:
- The image-build double check is guarded by
waited, which leaves the exact race the PR is closing open when the other process releases the lock just before this one probes it. - The session-start lock release is wired to
AttachIO.OnStarted, and the doc comment now declares that hook mandatory. The docker backend still drops it in three of the fourrunForegroundbranches, so the new contract is only honored by luck of the current call path. - The lock is deliberately released mid-
Runfor foreground sessions. Correctness there rests onRunInteractiveWithStartHookfiring the hook after the container name is registered, and onExecute's deferred release covering the paths where it never fires. Both hold today, but it is the part most worth a second pair of eyes. - Gateway image builds are still unsynchronized and keyed by profile only, so the reported problem is not fully gone across projects.
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up review after c4a8214 ("fix: address concurrent startup review findings").
All five points from my previous review are addressed in that commit:
coordinateRuntimeImageBuildnow always re-resolves the build plan inside the lock, no morewaitedguard.runForegroundforwardsattach.OnStartedin all four branches via the newRunWithStartHook/RunWithIOAndStartHookhelpers, so the session-start lock is released consistently regardless of TTY/IO wiring.ExecuteBackgroundnow releases the start lock right afterbe.Startsucceeds, before port announcement and post-start IDE launch.- Gateway image builds are now serialized with a lock keyed by the image name (mirrors the runtime-image build lock), closing the check-then-build race.
- The
secondResolveStartedtest channel now closes on the second resolve call, matching its name.
Checked build and tests (go build ./..., go test on internal/app, internal/gateway, internal/docker, internal/runtime, internal/backend/...) locally, all green. No new issues found in the changes since my last review.
These previous comments can be resolved as they are now handled:
- re-resolve build plan unconditionally inside the lock, not just when waited
- docker backend forward OnStarted in all runForeground branches
- release session-start lock right after be.Start succeeds in background path
- serialize gateway image builds with a lock keyed by image name
- fix secondResolveStarted to close on the second resolve call
I can't resolve them myself as I would need write permission on this repository.
What it does
--rebuildwas requested.Thanks @kalyi for initially reporting this!
How to test
Run
make build.Then in parallel start >=2 sessions that trigger a build:
bin/enclave.If you already have an image built for this branch, you can delete it using
docker rmi enclave-<tool>:fix-concurrent-image-build-lock-latest.One of the sessions should execute the image build; the others should print something like
info: Waiting for another enclave process to finish building enclave-<tool>:fix-concurrent-image-build-lock-latest.Follow-ups
None.
Breaking changes
Review checklist