Make sites upgrade and repository cleanup run in parallel#3926
Make sites upgrade and repository cleanup run in parallel#3926jmendeza wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refactors startup cleanup and site upgrade workflows to support concurrent execution. Configuration properties for parallel thread counts and timeouts are added, dependency injection is converted from setter-based to constructor-based, and repository startup cleanup is reimplemented using ExecutorService for parallel site processing with proper lock management. ChangesConcurrent Site Startup Cleanup and Upgrade Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/craftercms/studio/impl/v2/repository/RepositoryStartupCleanup.java (1)
79-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve interrupt status and handle timeout termination explicitly.
InterruptedExceptionis currently swallowed, and timeout completion is not enforced. This can make startup behavior non-deterministic under contention.Proposed fix
public void unlockRepositories() { logger.debug("Clean up git lock for all repositories."); try { unlockSitesRepositories(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.warn("Repository startup cleanup interrupted", e); } catch (Exception e) { logger.error("Error cleaning up git lock", e); } } protected void unlockSitesRepositories() throws InterruptedException { - try (ExecutorService taskExecutor = Executors.newFixedThreadPool(executorThreadCount)) { - for (Site site : siteService.getSitesByState(READY)) { - taskExecutor.execute(() -> unlockSiteRepositories(site)); - } - taskExecutor.shutdown(); - taskExecutor.awaitTermination(executorTimeoutSeconds, SECONDS); - } + ExecutorService taskExecutor = Executors.newFixedThreadPool(executorThreadCount); + try { + for (Site site : siteService.getSitesByState(READY)) { + taskExecutor.execute(() -> unlockSiteRepositories(site)); + } + taskExecutor.shutdown(); + if (!taskExecutor.awaitTermination(executorTimeoutSeconds, SECONDS)) { + logger.warn("Repository startup cleanup timed out after {} seconds", executorTimeoutSeconds); + taskExecutor.shutdownNow(); + } + } finally { + if (!taskExecutor.isShutdown()) { + taskExecutor.shutdownNow(); + } + } }Also applies to: 91-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/studio/impl/v2/repository/RepositoryStartupCleanup.java` around lines 79 - 83, The current catch-all around unlockSitesRepositories() (and the similar block at lines 91-93) swallows InterruptedException and doesn't enforce timeouts; update both call sites to catch InterruptedException separately, call Thread.currentThread().interrupt() to preserve the interrupt status, and log a clear message, and enforce a bounded wait by invoking the unlock logic via a cancellable task (e.g., submit unlockSitesRepositories() to an ExecutorService and use Future.get(timeout, TimeUnit.SECONDS) or CompletableFuture.orTimeout) so timeouts result in task cancellation and an explicit timeout log/cleanup; keep the existing generic Exception catch for other errors but ensure interrupt and timeout branches are handled first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/repository/RepositoryStartupCleanup.java`:
- Around line 79-83: The current catch-all around unlockSitesRepositories() (and
the similar block at lines 91-93) swallows InterruptedException and doesn't
enforce timeouts; update both call sites to catch InterruptedException
separately, call Thread.currentThread().interrupt() to preserve the interrupt
status, and log a clear message, and enforce a bounded wait by invoking the
unlock logic via a cancellable task (e.g., submit unlockSitesRepositories() to
an ExecutorService and use Future.get(timeout, TimeUnit.SECONDS) or
CompletableFuture.orTimeout) so timeouts result in task cancellation and an
explicit timeout log/cleanup; keep the existing generic Exception catch for
other errors but ensure interrupt and timeout branches are handled first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5594f7d-a26d-4e35-97e4-f5b72d874cdd
📒 Files selected for processing (5)
src/main/java/org/craftercms/studio/impl/v2/repository/RepositoryStartupCleanup.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.javasrc/main/resources/crafter/studio/studio-config.yamlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xml
| studio.upgrade.db.population.timeoutSeconds: 7200 | ||
|
|
||
| # Number of threads for site upgrades (meaning the number of sites that will be upgraded in parallel) | ||
| studio.upgrade.sites.threadCount: 10 |
There was a problem hiding this comment.
@russdanner @sumerjabri please chime in on these defaults for the thead count and timeout
| ################################################## | ||
| studio.configuration.site.startup.cleanup.enabled: true | ||
| # Number of threads for site startup cleanup (meaning the number of sites that will be cleaned in parallel on startup) | ||
| studio.configuration.site.startup.cleanup.threadCount: 10 |
There was a problem hiding this comment.
@russdanner @sumerjabri please chime in on these defaults for the thead count and timeout
|
@coderabbitai is this PR ready to move forward? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me review the actual code changes in detail before giving you an assessment. 🏁 Script executed: Length of output: 27027 🏁 Script executed: Length of output: 4466 🏁 Script executed: Length of output: 2948 🏁 Script executed: Length of output: 584
🔴 Critical —
|
|
@coderabbitai removeIndexIfCorrupted runs after unlockRepository. It there is an error getting the repository, it is unlockRepository the one to fail |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7733
The scenario that makes this dangerous is precisely the one this code targets: a corrupted repository. A null guard before Line 142 would be the minimal fix: protected void removeIndexIfCorrupted(String siteId, GitRepositories repository) {
logger.debug("Checking if repository '{}' for site '{}' is corrupted", repository, siteId);
Repository repo = helper.getRepository(siteId, repository);
if (repo == null) {
logger.warn("Could not open repository '{}' for site '{}', skipping corruption check", repository, siteId);
return;
}
File repoDir = repo.getWorkTree();
// ...
} |
|
@coderabbitai please review and let me know if your concerns are satisfied now |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12161
The implementation looks good. The only remaining step is to mark the PR out of draft status when you're ready. |
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/main/resources/crafter/studio/studio-upgrade-context.xml (1)
27-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWire the two newly required constructor dependencies.
The
StudioUpgradeManagerImplconstructor requiresretryingRepositoryOperationFacadeandsiteBootstrapStateProvider, but this bean provides only 11 constructor args. Add the two missing refs or theupgradeManagerbean will fail to instantiate at startup.🔧 Proposed fix
<bean id="upgradeManager" class="org.craftercms.studio.impl.v2.upgrade.StudioUpgradeManagerImpl"> <constructor-arg name="dataSource" ref="dataSource"/> <constructor-arg name="integrityValidator" ref="crafter.databaseValidator"/> <constructor-arg name="contentRepository" ref="contentRepository"/> <constructor-arg name="studioConfiguration" ref="studioConfiguration"/> <constructor-arg name="instanceService" ref="instanceService"/> @@ <constructor-arg name="bpPipelineFactory"> <bean class="org.craftercms.studio.impl.v2.upgrade.pipeline.PrototypeUpgradePipelineFactoryImpl" parent="pipelineFactory"> @@ </bean> </constructor-arg> + <constructor-arg name="retryingRepositoryOperationFacade" ref="studio.retryingRepositoryOperationFacade"/> + <constructor-arg name="siteBootstrapStateProvider" ref="siteBootstrapStateProvider"/> <constructor-arg name="executorThreadCount" value="#{studioConfiguration.getProperty('studio.upgrade.sites.threadCount')}"/> <constructor-arg name="executorTimeoutSeconds" value="#{studioConfiguration.getProperty('studio.upgrade.sites.timeoutSeconds')}"/> </bean>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/crafter/studio/studio-upgrade-context.xml` around lines 27 - 57, The upgradeManager bean configuration is missing two constructor-arg references required by org.craftercms.studio.impl.v2.upgrade.StudioUpgradeManagerImpl: add constructor-arg entries for the retryingRepositoryOperationFacade and siteBootstrapStateProvider dependencies (use ref="retryingRepositoryOperationFacade" and ref="siteBootstrapStateProvider") alongside the existing args so the upgradeManager bean can be instantiated; ensure the names match the constructor parameter names expected by StudioUpgradeManagerImpl.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.java`:
- Around line 243-246: When
taskExecutor.awaitTermination(executorTimeoutSeconds, TimeUnit.SECONDS) returns
false, treat this as a failed upgrade: create a TimeoutException or new
Exception with a descriptive message about the global upgrade timeout, call
upgradeException.addSuppressed(timeoutException) and set
upgradeFailed.set(true), then proceed to call taskExecutor.shutdownNow(); update
the warning log (logger.warn) if desired but ensure the suppressed exception is
recorded and upgradeFailed is set so timeout handling mirrors the
InterruptedException branch (refer to taskExecutor.awaitTermination,
executorTimeoutSeconds, logger.warn, taskExecutor.shutdownNow, upgradeException,
and upgradeFailed).
- Around line 232-239: The taskExecutor.execute lambda only catches
UpgradeException so unchecked exceptions from upgrade(site) can terminate the
thread silently; update the lambda in taskExecutor.execute to also catch broad
throwables (e.g., add a catch (Throwable t) or catch (RuntimeException | Error)
as appropriate) and in that handler call logger.error with context, add the
throwable to upgradeException via addSuppressed, and set upgradeFailed.set(true)
so any non-UpgradeException failures are recorded and cause the overall upgrade
to fail.
In
`@src/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.java`:
- Around line 53-55: The call to siteDAO.getSite(siteId) in
SiteBootstrapStateProviderImpl may return null, so before calling
site.getSiteUuid() add a null guard: check the returned Site from
siteDAO.getSite(siteId) and if null either log a warning via the same logger (or
applicationEventPublisher) and return early, or throw a controlled exception
(e.g., IllegalStateException with a clear message); only call
applicationEventPublisher.publishEvent(new SiteReadyEvent(siteId,
site.getSiteUuid())) when site is non-null. Ensure you reference the existing
variables/methods (siteDAO.getSite, SiteReadyEvent,
applicationEventPublisher.publishEvent) and keep behavior consistent with
surrounding error handling.
- Around line 52-55: The event is published unconditionally causing duplicate
SiteReadyEvent emissions; change markSiteAsReady to only publish when the site
actually transitions to ready by checking the boolean result of
getReadySites().add(siteId) and only call siteDAO.getSite(...) and
applicationEventPublisher.publishEvent(new SiteReadyEvent(...)) when add(...)
returns true (i.e., the site was newly added).
---
Outside diff comments:
In `@src/main/resources/crafter/studio/studio-upgrade-context.xml`:
- Around line 27-57: The upgradeManager bean configuration is missing two
constructor-arg references required by
org.craftercms.studio.impl.v2.upgrade.StudioUpgradeManagerImpl: add
constructor-arg entries for the retryingRepositoryOperationFacade and
siteBootstrapStateProvider dependencies (use
ref="retryingRepositoryOperationFacade" and ref="siteBootstrapStateProvider")
alongside the existing args so the upgradeManager bean can be instantiated;
ensure the names match the constructor parameter names expected by
StudioUpgradeManagerImpl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4194e44-a2a8-424c-84c0-cb5c45dc96a8
📒 Files selected for processing (6)
src/main/java/org/craftercms/studio/impl/v2/repository/RepositoryStartupCleanup.javasrc/main/java/org/craftercms/studio/impl/v2/upgrade/StudioUpgradeManagerImpl.javasrc/main/java/org/craftercms/studio/impl/v2/utils/spring/context/SiteBootstrapStateProviderImpl.javasrc/main/resources/crafter/studio/studio-config.yamlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/crafter/studio/studio-upgrade-context.xml
1 participant
Make sites upgrade and repository cleanup run in parallel
craftercms/craftercms#8681