Performance optimizations: Docker, CI, and runtime#101
Merged
nitrobass24 merged 2 commits intodevelopfrom Mar 1, 2026
Merged
Conversation
…caching, poll intervals - Add .dockerignore to exclude ~50-100 MB of unnecessary build context - Share Angular build artifact from unit-test job to build-arm64 in CI, eliminating a redundant npm ci + ng build (~3-5 min savings per run) - Add cached test image (seedsync-test) so `make test` skips dependency install on subsequent runs (~25s savings per run) - Increase SSE stream poll interval from 100ms to 250ms (imperceptible for dashboard) - Increase multiprocessing logger sleep from 0.1s to 0.5s (5x fewer wake-ups) - Update logger test sleep to accommodate new poll interval Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a .dockerignore, a containerized Python test image and Makefile target, changes CI to build Angular once in unit-test and reuse the artifact in build-arm64, and increases several internal timing constants used by logging and streaming with matching test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / Push
participant Actions as GitHub Actions Runner
participant Unit as unit-test Job
participant Artifact as GitHub Artifacts Storage
participant Arm64 as build-arm64 Job
participant Registry as Container Registry
Dev->>Actions: push/tag on branch
Actions->>Unit: run unit-test job
Unit->>Unit: build Angular frontend
Unit->>Artifact: upload `angular-build` artifact
Actions->>Arm64: trigger build-arm64 (depends on unit-test)
Arm64->>Artifact: download `angular-build` artifact
Arm64->>Arm64: use downloaded frontend in image build
Arm64->>Registry: build/push arm64 images
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/python/tests/unittests/test_common/test_multiprocessing_logger.py (1)
96-150:⚠️ Potential issue | 🟠 MajorIncrease timeout budget for the expanded waits in
test_logger_levels.With four
time.sleep(1)calls, the test now spends ~4s sleeping, leaving too little headroom under the 5s cap (Line 79) for process startup/join and log capture. This can intermittently timeout in CI.✅ Proposed fix
- `@timeout_decorator.timeout`(5) + `@timeout_decorator.timeout`(10) def test_logger_levels(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/tests/unittests/test_common/test_multiprocessing_logger.py` around lines 96 - 150, The test test_logger_levels in TestMultiprocessingLogger is risking CI timeouts because it performs four time.sleep(1) waits; update the waits used around MultiprocessingLogger start/join (the time.sleep calls in the blocks that create mp_logger / p_1 / mp_logger.start / p_1.join / mp_logger.stop) to increase the timeout budget (e.g., change each time.sleep(1) to time.sleep(1.25) or 1.5) so process startup/join and LogCapture have sufficient headroom when using MultiprocessingLogger and process_1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 39-50: Update the GitHub Actions step gating by removing the
'develop' branch condition from the if expressions used by the "Build Angular
frontend" and "Upload Angular build artifact" steps (current condition:
startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/develop');
change them so they only match tag refs (startsWith(github.ref, 'refs/tags/v'))
and apply the same edit to the duplicate block referenced around lines 123-133
so the artifact producer/consumer are gated only for version tag pushes.
In `@Makefile`:
- Around line 40-42: The Makefile currently conditionally skips rebuilding the
Docker image by checking for seedsync-test; remove that image existence check
and always invoke the test-image target before running the container so tests
never run against a stale image. Update the test recipe to call `$(MAKE)
test-image` unconditionally (instead of `@docker image inspect seedsync-test
>/dev/null 2>&1 || $(MAKE) test-image`) and then run `docker run --rm -v
$(PWD)/src/python:/app/python seedsync-test pytest tests/unittests -v
--tb=short`; keep the `test-image` target name and the `seedsync-test` image
name so you only change the invocation logic.
- Line 4: The .PHONY line is missing non-file targets (e.g., all) which can
cause make conflicts; update the .PHONY declaration to include every non-file
target defined in the Makefile (at minimum add "all")—for example ensure targets
referenced by name such as all, build, run, stop, logs, clean, test, test-image
and any other custom targets like fmt, lint, up, down, shell, help, docker-image
are listed so none are treated as file targets.
In `@src/docker/build/test-image/Dockerfile`:
- Around line 1-8: The Dockerfile runs as root; change it to create and switch
to a non-root user: add steps to create a user/group (e.g., "app" or
"pytestuser") with no-login, ensure /app/python exists and is chowned to that
user, and then add a USER instruction so subsequent container runs use that
non-root account; reference the Dockerfile instructions like WORKDIR /app/python
and ENV PYTHONPATH=/app/python to locate where to set ownership and switch user.
Ensure all install steps that require root remain before the USER switch and
only the runtime/test execution runs as the non-root user.
---
Outside diff comments:
In `@src/python/tests/unittests/test_common/test_multiprocessing_logger.py`:
- Around line 96-150: The test test_logger_levels in TestMultiprocessingLogger
is risking CI timeouts because it performs four time.sleep(1) waits; update the
waits used around MultiprocessingLogger start/join (the time.sleep calls in the
blocks that create mp_logger / p_1 / mp_logger.start / p_1.join /
mp_logger.stop) to increase the timeout budget (e.g., change each time.sleep(1)
to time.sleep(1.25) or 1.5) so process startup/join and LogCapture have
sufficient headroom when using MultiprocessingLogger and process_1.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.dockerignore.github/workflows/ci.ymlMakefilesrc/docker/build/test-image/Dockerfilesrc/python/common/multiprocessing_logger.pysrc/python/tests/unittests/test_common/test_multiprocessing_logger.pysrc/python/web/web_app.py
Comment on lines
+39
to
+50
| - name: Build Angular frontend | ||
| if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/develop' | ||
| working-directory: src/angular | ||
| run: npx ng build --configuration=production --output-path /tmp/angular-output/build | ||
|
|
||
| - name: Upload Angular build artifact | ||
| if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/develop' | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: angular-build | ||
| path: /tmp/angular-output | ||
| retention-days: 1 |
There was a problem hiding this comment.
Align artifact producer/consumer gating with the CI branch policy.
These new steps are wired for develop + tag refs, which conflicts with the workflow policy (master for Build & Test, tag pushes for publish/release paths). Please remove develop from this artifact path gating.
Proposed patch
@@
- - name: Build Angular frontend
- if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/develop'
+ - name: Build Angular frontend
+ if: startsWith(github.ref, 'refs/tags/v')
@@
- - name: Upload Angular build artifact
- if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/develop'
+ - name: Upload Angular build artifact
+ if: startsWith(github.ref, 'refs/tags/v')
@@
build-arm64:
name: Build (arm64)
- if: startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/develop'
+ if: startsWith(github.ref, 'refs/tags/v')As per coding guidelines, GitHub Actions CI workflow should trigger: Build & Test on PR to master and push to master; also Publish Image and Create Release only on push of version tags (v*..).
Also applies to: 123-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 39 - 50, Update the GitHub Actions
step gating by removing the 'develop' branch condition from the if expressions
used by the "Build Angular frontend" and "Upload Angular build artifact" steps
(current condition: startsWith(github.ref, 'refs/tags/v') || github.ref ==
'refs/heads/develop'); change them so they only match tag refs
(startsWith(github.ref, 'refs/tags/v')) and apply the same edit to the duplicate
block referenced around lines 123-133 so the artifact producer/consumer are
gated only for version tag pushes.
…st user - List all non-file targets in .PHONY to prevent make conflicts - Always invoke test-image target before running tests to avoid stale images - Add non-root testuser to test Dockerfile for container security Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 6, 2026
This was referenced Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.dockerignore— excludes.git,.github,website/,node_modules,__pycache__, etc., trimming ~50-100 MB from the Docker build contextunit-testjob now builds Angular and uploads the artifact;build-arm64downloads it instead of running a redundantnpm ci+ng build(~3-5 min savings per CI run)seedsync-testDocker image pre-bakes all test dependencies;make testauto-builds it on first run, then reuses it (~25s savings per subsequent run)Test plan
make build— Docker image builds successfully (.dockerignoredoesn't exclude needed files)make test— 456 passed, 33 failed (identical to baseline; all failures are pre-existing lftp/ssh integration tests)make testrun reuses cached test image (skips rebuild)🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests