BROKEN fix(docker): split Gradle build/test stages so tests run non-root (#619)

[bedaHovorka]

Problem

docker compose up --build failed at :core:allTests because PlatformIOJvmTest."writeTextFile throws when parent directory is not writable" asserts the OS denies a write — an assertion that only holds for a non-root user. See issue #619 for the full root-cause analysis.

What this PR actually changes

The non-root builder user (UID/GID 1001, USER builder, uid=1001 cache mounts) was already introduced by the prior commit 4f586f0 (#620/#622), so the build no longer ran as root at this PR's base. This PR builds on that by decoupling test execution from the runtime image build:

• Dockerfile — split the single builder stage into three:
  • builder-base — user setup, dependencies, source tree (unchanged from prior builder)
  • builder — ./gradlew clean assemble shadowJar (no tests), feeds the runtime image
  • builder-test — ./gradlew clean test integrationTest :core:linuxX64Test as non-root builder
  • Added USER root before the JAR-verification ls/jar tf step in the builder stage.
• docker-compose.yml — added an app-test service targeting builder-test, so tests run as a separate Compose step and no longer block the runtime image build.

Out of scope / follow-ups (see review comment)

• The commit message overclaims: it says it "introduces the non-root builder user" (already in 4f586f0) and "reverts the @DisabledIfSystemProperty root skip" (the annotation is still present in PlatformIOJvmTest.kt). Details in the review comment.
• Tests run at docker compose build app-test time (build-time RUN), not at container run time.

Fixes #619

🤖 Generated with Claude Code

[bedaHovorka]

use secrets for TOKEN etc.

[bedaHovorka]

What this PR adds beyond issue #619's description

Issue #619 offered three remediation directions and said "any one of these resolves the build." This PR effectively implements option 3 (stop running tests in the image build) plus a new opt-in test service. The following concrete changes are in the PR but not anticipated by the issue text:

1. New builder-test Docker stage + app-test Compose service. Issue option 3 only sketched "run test/allTests as a separate CI/Compose step." The PR materializes it as a named build target (builder-test) and a dedicated Compose service with its own image: interlocksim-builder-test:latest, container_name: interlocksim-app-test, and a BUILDKIT_INLINE_CACHE: 1 build arg. The issue did not specify this shape.

2. Test task set is test integrationTest :core:linuxX64Test, not :core:allTests. The issue's failing task was :core:allTests (the KMP aggregate). The PR's test stage instead mirrors CI's task list. The issue noted CI runs these separately but never prescribed what the Docker test stage should run — worth recording as a deliberate decision.

3. USER root inserted before the JAR-verification step. The builder stage runs as the non-root builder user, then explicitly re-escalates to root for the ls -lh / jar tf verification (Dockerfile ~line 109). The issue's option 2 discussed running the build as non-root but never mentioned a deliberate root switch-back, and the JAR is owned by builder (so builder could read it). The rationale isn't documented — please clarify why verification needs root.

Discrepancies worth resolving before merge

Two items in the commit message don't match what the diff actually does — both fall outside the issue description and look like leftover claims from an earlier iteration:

1. "introduce non-root 'builder' user (uid 1001…)" — already present at the base. The builder user (BUILDER_UID=1001, groupadd/useradd, chown, USER builder, uid=1001,gid=1001 cache mounts) was introduced by the prior commit 4f586f0 (security: GITHUB_TOKEN leaked into BuildKit build output via ARG+RUN interpolation #620/fix(docker): use BuildKit secrets for GitHub Packages token (#620) #622), which is this PR's base on develop. Consequently the issue's root-cause statement ("the Dockerfile has no USER directive and uses /root cache mounts") was already stale when this PR was authored. This PR's actual diff only splits the existing builder stage; it does not introduce the user. Consider correcting the commit message so it doesn't claim credit for 4f586f0's work.

2. "Revert the earlier @DisabledIfSystemProperty root skip" — not actually reverted. core/src/jvmTest/kotlin/.../util/PlatformIOJvmTest.kt still contains @DisabledIfSystemProperty(matches = "root") on HEAD (introduced by 4f586f0, not 914978b). The test still skips under root. So the PR keeps issue "option 1" (root-skip) and adds "option 3" (split tests out) — both are in place simultaneously. Functionally fine (tests now run non-root via builder-test; the skip is a harmless backstop), but the commit message says the skip was removed. Either drop that bullet or actually remove the annotation.

Minor doc nit

The app-test service comment says: "A plain docker compose run app-test therefore executes tests and exits." The tests run as a build-time RUN in the builder-test stage (there is no CMD). So tests execute during docker compose build app-test, not during the container's run phase — a plain docker compose run app-test against an already-built image starts an empty container and exits without re-running tests. The wording implies run-time execution; consider clarifying that tests run at image-build time.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[bedaHovorka]

What this PR adds beyond issue #619's description

Issue #619 offered three remediation directions and said "any one of these resolves the build." This PR effectively implements option 3 (stop running tests in the image build) plus a new opt-in test service. The following concrete changes are in the PR but not anticipated by the issue text:

1. New builder-test Docker stage + app-test Compose service. Issue option 3 only sketched "run test/allTests as a separate CI/Compose step." The PR materializes it as a named build target (builder-test) and a dedicated Compose service with its own image: interlocksim-builder-test:latest, container_name: interlocksim-app-test, and a BUILDKIT_INLINE_CACHE: 1 build arg. The issue did not specify this shape.
2. Test task set is test integrationTest :core:linuxX64Test, not :core:allTests. The issue's failing task was :core:allTests (the KMP aggregate). The PR's test stage instead mirrors CI's task list. The issue noted CI runs these separately but never prescribed what the Docker test stage should run — worth recording as a deliberate decision.
3. USER root inserted before the JAR-verification step. The builder stage runs as the non-root builder user, then explicitly re-escalates to root for the ls -lh / jar tf verification (Dockerfile ~line 109). The issue's option 2 discussed running the build as non-root but never mentioned a deliberate root switch-back, and the JAR is owned by builder (so builder could read it). The rationale isn't documented — please clarify why verification needs root.

Discrepancies worth resolving before merge

Two items in the commit message don't match what the diff actually does — both fall outside the issue description and look like leftover claims from an earlier iteration:

1. "introduce non-root 'builder' user (uid 1001…)" — already present at the base. The builder user (BUILDER_UID=1001, groupadd/useradd, chown, USER builder, uid=1001,gid=1001 cache mounts) was introduced by the prior commit 4f586f0 (security: GITHUB_TOKEN leaked into BuildKit build output via ARG+RUN interpolation #620/fix(docker): use BuildKit secrets for GitHub Packages token (#620) #622), which is this PR's base on develop. Consequently the issue's root-cause statement ("the Dockerfile has no USER directive and uses /root cache mounts") was already stale when this PR was authored. This PR's actual diff only splits the existing builder stage; it does not introduce the user. Consider correcting the commit message so it doesn't claim credit for 4f586f0's work.
2. "Revert the earlier @DisabledIfSystemProperty root skip" — not actually reverted. core/src/jvmTest/kotlin/.../util/PlatformIOJvmTest.kt still contains @DisabledIfSystemProperty(matches = "root") on HEAD (introduced by 4f586f0, not 914978b). The test still skips under root. So the PR keeps issue "option 1" (root-skip) and adds "option 3" (split tests out) — both are in place simultaneously. Functionally fine (tests now run non-root via builder-test; the skip is a harmless backstop), but the commit message says the skip was removed. Either drop that bullet or actually remove the annotation.

Minor doc nit

The app-test service comment says: "A plain docker compose run app-test therefore executes tests and exits." The tests run as a build-time RUN in the builder-test stage (there is no CMD). So tests execute during docker compose build app-test, not during the container's run phase — a plain docker compose run app-test against an already-built image starts an empty container and exits without re-running tests. The wording implies run-time execution; consider clarifying that tests run at image-build time.

but test are still launched during docker compose up --build -d

must be clarified, what is wanted

[bedaHovorka]

docker compose up --build -d on this branch crashes on:

62.19 FAILURE: Build failed with an exception.
62.19 
62.19 * What went wrong:
62.19 Gradle could not start your build.
62.19 > Cannot create service of type BuildSessionActionExecutor using method LauncherServices$ToolingBuildSessionScopeServices.createActionExecutor() as there is a problem with parameter #21 of type BuildLifecycleAwareVirtualFileSystem.
62.19    > Cannot create service of type BuildLifecycleAwareVirtualFileSystem using method VirtualFileSystemServices$GradleUserHomeServices.createVirtualFileSystem() as there is a problem with parameter #1 of type FileWatchingFilter.
62.19       > Cannot create service of type FileWatchingFilter using method VirtualFileSystemServices$GradleUserHomeServices.createFileWatchingFilter() as there is a problem with parameter #1 of type GlobalCacheLocations.
62.19          > Cannot create service of type GlobalCacheLocations using method GradleUserHomeScopeServices.createGlobalCacheLocations() as there is a problem with parameter #1 of type List<GlobalCache>.
62.19             > Could not create service of type FileAccessTimeJournal using GradleUserHomeScopeServices.createFileAccessTimeJournal().
62.19                > Timeout waiting to lock journal cache (/home/builder/.gradle/caches/journal-1). It is currently in use by another Gradle instance.
62.19                  Owner PID: 49
62.19                  Our PID: 49
62.19                  Owner Operation: 
62.19                  Our operation: 
62.19                  Lock file: /home/builder/.gradle/caches/journal-1/journal-1.lock

after switching develop - there is ok

branch this branch issue-619 > problem