Improved Gitlab All in case of many repositories #372
Conversation
WalkthroughThe changes introduce batching to various operations that previously processed large arrays all at once. GitLab repository fetching, scheduling of repository indexing, and database updates for repository indexing status are now performed in sequential or chunked batches. These modifications affect internal control flow but do not alter any exported function or class signatures. Additionally, new NixOS deployment configurations, modules, overlays, and tests for SourceBot are added, along with documentation updates and a new JSON file listing package hashes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GitLabManager
participant GitLabAPI
Caller->>GitLabManager: getGitLabReposFromConfig(config)
GitLabManager->>GitLabAPI: fetch all groups (batched, size=10)
GitLabManager->>GitLabAPI: fetch all users (batched, size=10)
loop For each group batch
GitLabManager->>GitLabAPI: fetch group projects (batch)
end
loop For each user batch
GitLabManager->>GitLabAPI: fetch user projects (batch)
end
GitLabManager-->>Caller: return repositories
sequenceDiagram
participant RepoManager
participant Scheduler
RepoManager->>RepoManager: fetchAndScheduleRepoIndexing(repos)
loop For each batch of 100 repos
RepoManager->>Scheduler: scheduleRepoIndexingBulk(batch)
end
sequenceDiagram
participant Action
participant Prisma
Action->>Action: flagReposForIndex(repoIds)
loop For each batch of 1000 IDs
Action->>Prisma: updateMany(batch, status=NEW)
end
sequenceDiagram
participant Initializer
participant Prisma
Initializer->>Initializer: syncConnections()
loop For each batch of 100 failed repo IDs
Initializer->>Prisma: updateMany(batch, status=NEW)
end
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/web/src/actions.ts (1)
923-933: Good implementation of batching to prevent timeouts.The sequential batching approach effectively addresses the API timeout issues mentioned in the PR objectives. The chunk size of 1000 is reasonable for database operations.
Consider adding error handling to ensure partial failures don't leave the system in an inconsistent state:
for (let i = 0; i < repoIds.length; i += 1000) { + try { await prisma.repo.updateMany({ where: { id: { in: repoIds.slice(i, i + 1000) }, orgId: org.id, }, data: { repoIndexingStatus: RepoIndexingStatus.NEW, } }); + } catch (error) { + logger.error(`Failed to update batch ${i / 1000 + 1}: ${error}`); + throw error; // Re-throw to maintain existing error handling behavior + } }packages/web/src/initialize.ts (1)
70-81: Effective batching implementation for retry operations.The batching approach prevents timeouts when retrying failed repositories. The smaller batch size of 100 (compared to 1000 in actions.ts) is appropriate for retry operations which may be more resource-intensive.
Consider adding error handling similar to the suggestion for actions.ts to ensure robustness during batch processing.
packages/backend/src/gitlab.ts (3)
10-10: Remove unused import.The
logimport from "console" is not used anywhere in the code.-import { log } from "console";
81-81: Consider making batch size configurable.The hardcoded batch size of 10 may not be optimal for all GitLab instances. Consider making this configurable or using different batch sizes based on the instance capabilities.
- const batchSize = 10; + const batchSize = config.batchSize || 10;Also applies to: 130-130
85-120: Sequential batching may impact performance.While sequential processing prevents API overloading, it could significantly slow down the operation. Consider processing a few batches concurrently (e.g., 2-3 batches) to balance performance and API limits.
- // Process groups in batches of 10 - for (let i = 0; i < config.groups.length; i += batchSize) { - const batch = config.groups.slice(i, i + batchSize); - logger.debug(`Processing batch ${i/batchSize + 1} of ${Math.ceil(config.groups.length/batchSize)} (${batch.length} groups)`); - - const batchResults = await Promise.allSettled(batch.map(async (group) => { + // Process groups in batches of 10 with limited concurrency + const maxConcurrentBatches = 2; + const batches = []; + for (let i = 0; i < config.groups.length; i += batchSize) { + batches.push(config.groups.slice(i, i + batchSize)); + } + + for (let i = 0; i < batches.length; i += maxConcurrentBatches) { + const concurrentBatches = batches.slice(i, i + maxConcurrentBatches); + const batchPromises = concurrentBatches.map(async (batch, index) => { + logger.debug(`Processing batch ${i + index + 1} of ${batches.length} (${batch.length} groups)`); + return Promise.allSettled(batch.map(async (group) => {Also applies to: 134-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/backend/src/gitlab.ts(3 hunks)packages/backend/src/repoManager.ts(1 hunks)packages/web/src/actions.ts(2 hunks)packages/web/src/initialize.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/backend/src/gitlab.ts (1)
packages/backend/src/utils.ts (2)
measure(9-17)fetchWithRetry(92-119)
🔇 Additional comments (3)
packages/backend/src/repoManager.ts (1)
168-170: Excellent batching strategy for repository scheduling.This implementation effectively addresses the core issue described in the PR objectives by preventing overwhelming the system when scheduling large numbers of repositories for indexing. The batch size of 100 aligns well with the database operation limits and job queue constraints.
The sequential processing approach is particularly important here as it prevents:
- API timeouts from GitLab
- Database connection exhaustion
- Job queue overload
packages/backend/src/gitlab.ts (2)
49-78: Excellent implementation of the all-groups/all-users feature.The logic properly handles the GitLab cloud restriction, uses retry mechanisms, and includes comprehensive error handling and logging. The approach of fetching all groups and users first, then processing them with the existing batched logic is well-designed.
89-118: Good error handling and logging implementation.The error handling properly distinguishes between 404 (not found) and other errors, and the detailed logging will be helpful for debugging issues with large repository sets. The use of
measureutility provides valuable performance insights.Also applies to: 138-166
| }); | ||
| logger.debug(`Found ${_groups.length} groups in ${groupsDuration}ms.`); | ||
|
|
||
| config.groups = _groups.map(g => g.full_path); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating input parameters.
Modifying the config object directly creates unexpected side effects for callers. Consider storing the discovered groups and users in local variables instead.
- config.groups = _groups.map(g => g.full_path);
+ const discoveredGroups = _groups.map(g => g.full_path);
+ config = { ...config, groups: discoveredGroups };- config.users = _users.map(u => u.username);
+ const discoveredUsers = _users.map(u => u.username);
+ config = { ...config, users: discoveredUsers };Also applies to: 69-69
🤖 Prompt for AI Agents
In packages/backend/src/gitlab.ts at lines 57 and 69, avoid directly mutating
the input parameter `config` by assigning to its properties. Instead, create
local variables to hold the groups and users data derived from `_groups` and
other sources, and use these local variables within the function. This prevents
side effects on the caller's `config` object and maintains functional purity.
| allRepos = allRepos.concat(validRepos); | ||
| notFound.orgs = notFoundOrgs; | ||
| logger.debug(`Found ${validRepos.length} valid repositories in groups.`); | ||
| logger.debug(`Not found groups: ${notFoundOrgs.join(', ')}`); | ||
| logger.debug(`These repositories will be downloaded: ${allRepos.map(repo => repo.path_with_namespace).join('\n')}`); |
There was a problem hiding this comment.
nit: synced not downloaded to be consistent with language elsewhere
|
Thanks for the change! I assume you've already tested this with larger configs? Would love to know if you've noticed a difference |
|
The change is necessary for the sync to work at all, otherwise it fails on timeout. I don't think you should merge the code as is or continue with the batch direction. I think this part should be refactored so every batch of found repository would be processed (filterd/checked) and added to the database and to the sync pipeline. Because when you have a lot of repositories, this step takes a very long time (10 minutes?) and if something fails it might need to start again. It would be nice to see a stream of repositories being detected and processed. Also "100" batch is right for me but maybe not for another instance, another configuration like that is possible but magic numbers are annoying. |
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
docs/docs/deployment-guide.mdx(3 hunks)flake.nix(1 hunks)nix/microvm.nix(1 hunks)nix/missing-hashes.json(1 hunks)nix/nixosModule.nix(1 hunks)nix/nixosTest.nix(1 hunks)nix/overlay.nix(1 hunks)nix/sourcebot.nix(1 hunks)packages/backend/src/gitlab.ts(3 hunks)packages/backend/src/repoManager.ts(1 hunks)packages/web/src/actions.ts(2 hunks)packages/web/src/initialize.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- nix/missing-hashes.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/web/src/initialize.ts
- packages/backend/src/repoManager.ts
- packages/web/src/actions.ts
- packages/backend/src/gitlab.ts
🧰 Additional context used
🪛 Gitleaks (8.26.0)
nix/overlay.nix
8-8: Sourcegraph is a code search and navigation engine.
(sourcegraph-access-token)
🔇 Additional comments (7)
docs/docs/deployment-guide.mdx (3)
7-8: Good structural improvement.Adding the "Container deployment" section improves the documentation hierarchy and makes it clearer that the existing content is about container-based deployment.
12-12: Consistent heading structure.The demotion of "Walkthrough video" and "Step-by-step guide" to level 3 headings maintains proper hierarchical structure under the new "Container deployment" section.
Also applies to: 26-26
101-208: Comprehensive NixOS deployment documentation.The new NixOS deployment section provides thorough instructions covering flake setup, secret management with both sops-nix and agenix, and service configuration. The examples are well-structured and include proper configuration options.
nix/overlay.nix (1)
1-12: Well-structured Nix overlay.The overlay correctly adds the sourcebot package and overrides the zoekt package with a specific revision. The structure follows Nix conventions properly.
nix/nixosTest.nix (1)
1-31: Comprehensive NixOS test implementation.The test properly validates the sourcebot module by checking all critical systemd services and HTTP connectivity. The test structure follows NixOS testing conventions correctly.
nix/microvm.nix (1)
45-62: Good microVM service configuration.The sourcebot service configuration is well-structured with appropriate debug logging and a proper JSON configuration for testing.
flake.nix (1)
1-72: Comprehensive and well-structured Nix flake.The flake configuration properly defines all necessary outputs including NixOS modules, system configurations, packages, checks, and development shells. The structure follows Nix flake conventions and uses flake-utils appropriately for system-specific outputs.
| { | ||
| mountPoint = "/var"; | ||
| image = "sourcebot-var.img"; | ||
| size = 1 * 1024; # 10GB |
There was a problem hiding this comment.
Fix inconsistent size calculation and comment.
The comment says "10GB" but the calculation 1 * 1024 results in 1GB, not 10GB.
- size = 1 * 1024; # 10GB
+ size = 10 * 1024; # 10GB📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size = 1 * 1024; # 10GB | |
| size = 10 * 1024; # 10GB |
🤖 Prompt for AI Agents
In nix/microvm.nix at line 33, the size calculation is inconsistent with the
comment; it currently calculates 1GB (1 * 1024) but the comment says 10GB.
Update the calculation to correctly represent 10GB by multiplying 10 * 1024
instead of 1 * 1024, ensuring the comment and value match.
| interfaces = [ | ||
| { | ||
| type = "user"; | ||
| id = "sourecbot"; |
There was a problem hiding this comment.
Fix typo in interface ID.
The interface ID has a typo: "sourecbot" should be "sourcebot".
- id = "sourecbot";
+ id = "sourcebot";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id = "sourecbot"; | |
| id = "sourcebot"; |
🤖 Prompt for AI Agents
In nix/microvm.nix at line 10, correct the typo in the interface ID by changing
"sourecbot" to "sourcebot" to ensure the identifier is accurate.
|
|
||
| mkdir -p $out/bin | ||
|
|
||
| rm -rf $out/pacakages/web/.next/cache |
There was a problem hiding this comment.
Fix typo in directory path
There's a typo in the path: pacakages should be packages.
-rm -rf $out/pacakages/web/.next/cache
+rm -rf $out/packages/web/.next/cache📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf $out/pacakages/web/.next/cache | |
| rm -rf $out/packages/web/.next/cache |
🤖 Prompt for AI Agents
In nix/sourcebot.nix at line 69, fix the typo in the directory path by changing
"pacakages" to "packages" in the rm command to correctly reference the path
$out/packages/web/.next/cache.
| runHook preInstall | ||
|
|
||
| cp -r packages/web/.next/standalone $out | ||
| cp -r node_modules/* $out/node_modules |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify multiple node_modules copies
The node_modules directory is copied multiple times which might cause files to be overwritten. Please verify if this is intentional or if it could be optimized to avoid redundant copies.
Also applies to: 57-57, 65-65
🏁 Script executed:
#!/bin/bash
# Description: Check if there are unique dependencies in each package's node_modules that justify multiple copies
# Look for package.json files to understand the package structure
fd -t f "package.json" packages/
# Check if packages have their own node_modules directories
fd -t d "node_modules" packages/ -d 2Length of output: 322
Review unnecessary node_modules duplication
It looks like there are no nested node_modules directories under packages/, so each of the three
cp -r node_modules/* $out/node_modulessteps (lines 49, 57 and 65 in nix/sourcebot.nix) is copying the exact same set of dependencies into every output. This not only duplicates files but also makes your derivations larger than necessary.
Consider one of the following optimizations:
- Consolidate into a single top-level copy of
node_modules(e.g. run thecponce before iterating over packages). - Split out per-package dependency sets (e.g. prune
node_modulesto only include each package’s deps).
Files to update:
- nix/sourcebot.nix (lines 49, 57, 65)
🤖 Prompt for AI Agents
In nix/sourcebot.nix around lines 49, 57, and 65, the cp commands redundantly
copy the entire node_modules directory into each package output, causing
unnecessary duplication and larger derivations. To fix this, refactor the script
to copy node_modules only once at a higher level before processing individual
packages, or alternatively prune node_modules per package to include only its
specific dependencies, thereby avoiding repeated full copies and reducing output
size.
| ++ optional (cfg.envFile == null) [ | ||
| "AUTH_SECRET=00000000000000000000000000000000000000000000" | ||
| "SOURCEBOT_ENCRYPTION_KEY=00000000000000000000000000000000" | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Hardcoded secrets with all zeros
The default AUTH_SECRET and SOURCEBOT_ENCRYPTION_KEY values are hardcoded to all zeros when no environment file is provided. This is insecure and should never be used in production.
Consider one of these approaches:
- Generate random secrets during service initialization
- Make the environment file mandatory when auth is enabled
- Add assertions to prevent the service from starting with default secrets
Example implementation for option 2:
assertions = [
{
assertion = !cfg.authEnabled || cfg.envFile != null;
message = "services.sourcebot.envFile must be set when authEnabled is true";
}
];Also applies to: 223-226
🤖 Prompt for AI Agents
In nix/nixosModule.nix around lines 190 to 193 and also lines 223 to 226, the
default AUTH_SECRET and SOURCEBOT_ENCRYPTION_KEY are hardcoded to insecure
all-zero values when no environment file is provided. To fix this, either
generate random secrets during service initialization, make the environment file
mandatory when authentication is enabled by adding an assertion that requires
cfg.envFile to be set if cfg.authEnabled is true, or add assertions to prevent
the service from starting with these default secrets. Implement the assertion
approach by adding a check that throws an error if auth is enabled but envFile
is not provided.
| default = "http://${cfg.hostname}:${toString cfg.port}"; | ||
| description = "Hostname or IP address for the SourceBot web server to bind to"; |
There was a problem hiding this comment.
Fix incorrect option description
The description for authUrl is incorrect - it appears to be copy-pasted from the hostname option.
- description = "Hostname or IP address for the SourceBot web server to bind to";
+ description = "Authentication URL for SourceBot";🤖 Prompt for AI Agents
In nix/nixosModule.nix around lines 71 to 72, the description for the `authUrl`
option is incorrect and mistakenly copied from the `hostname` option. Update the
description to accurately reflect the purpose of the `authUrl` option,
describing it as the URL used for authentication or the endpoint related to
auth, rather than the hostname or IP address for the web server.
| openFirewall = mkOption { | ||
| type = types.bool; | ||
| default = true; | ||
| description = "Open Firwall ports for SourceBot"; |
There was a problem hiding this comment.
Fix typo in option description
- description = "Open Firwall ports for SourceBot";
+ description = "Open Firewall ports for SourceBot";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description = "Open Firwall ports for SourceBot"; | |
| description = "Open Firewall ports for SourceBot"; |
🤖 Prompt for AI Agents
In nix/nixosModule.nix at line 52, there is a typo in the option description
"Open Firwall ports for SourceBot". Correct the typo by changing "Firwall" to
"Firewall" to accurately describe the option.
When there are many repositories on Giltab, doing actions in one hit would fail to API timeout.
This is a quick workaround/hack by using batches.
Also when there a lot of repositories, just getting the list of them might be too much.
Ideally, I would like Sourcebot to add a repository as it finds it instead of handling the entire connection sync. Otherwise this process blocks other connections from syncing, and if it fails/breaks, then it has to start over.
Note: prisma also failing when there are too many connections, not sure what is the magical number is (sometimes 1000 resulted with errors too):
Summary by CodeRabbit