Add REST API to fall back an encrypted repository to a file-based repository#1276
Open
Add REST API to fall back an encrypted repository to a file-based repository#1276
Conversation
…ository
Motivation:
After migrating a repository to an encrypted repository, there was no way to revert it back to the original file-based git repository.
Modifications:
- Add `FallbackToFileRepositoryCommand` and `CommandType.FALLBACK_TO_FILE_REPOSITORY`
- Add `POST /projects/{projectName}/repos/{repoName}/migrate/file` endpoint, which sets fall back an encrypted repository.
Result:
- System administrators can revert an encrypted repository back to the original file-based git state.
📝 WalkthroughWalkthroughAdds a new "fallback to file repository" feature: a Command type and command class, executor handling, API endpoint, RepositoryManager and GitRepositoryManager support, a wrapper method, and comprehensive tests to migrate an encrypted repository back to file-based storage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as RepositoryServiceV1
participant Executor as StandaloneCommandExecutor
participant Manager as RepositoryManager
participant GitMgr as GitRepositoryManager
participant Worker as RepositoryWorker
Client->>API: POST /projects/{p}/repos/{r}/migrate/file
API->>API: set repository status -> READ_ONLY
API->>Executor: submit FallbackToFileRepositoryCommand
Executor->>Manager: get repository & validate encrypted
Executor->>Worker: schedule fallbackToFileRepository(repositoryName)
Worker->>GitMgr: fallbackToFileRepository(repositoryName)
GitMgr->>GitMgr: remove placeholder, reopen file repo, replace child, close encrypted repo, cleanup
GitMgr-->>Worker: completion / error
Worker-->>Executor: completion
Executor-->>API: notify result
API->>API: set repository status -> ACTIVE (or revert on failure)
API-->>Client: return updated RepositoryDto
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java`:
- Around line 372-382: The pre-check Repository.isEncrypted() is done on the
calling thread using a captured Repository, which can become stale; move the
encryption-state check into the repositoryWorker task so it re-fetches the
current Repository instance under the same execution context. In
fallbackToFileRepository(FallbackToFileRepositoryCommand), remove the early
isEncrypted() call and, inside the CompletableFuture.supplyAsync(...) block
(which runs on repositoryWorker), call repositoryManager.get(c.repositoryName())
again, verify isEncrypted() there and throw the IllegalStateException from
inside the async block (or complete exceptionally) before calling
repositoryManager.fallbackToFileRepository(c.repositoryName()).
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java`:
- Around line 326-335: The POST handler
RepositoryServiceV1.fallbackToFileRepository exposes a destructive revert
without explicit user confirmation and without checking for divergence between
the encrypted repo and the preserved Git repo; update the API and implementation
so the operation requires an explicit confirmation flag (e.g., boolean
forceFallback or confirmFallback) or fails if the encrypted Git has diverged.
Concretely: extend the method signature (and incoming request parsing) to accept
a confirmation token/flag, modify validateFallbackPrerequisites to also check
divergence via GitRepositoryManager.fallbackToFileRepository or a new
GitRepositoryManager.hasDiverged(...) call, and enforce that fallback(author,
project, repository) is only invoked when the confirmation flag is present OR
divergence check passes; also apply the same change to the other endpoint(s)
covering lines 337-359 to ensure consistent validation and gating before calling
setRepositoryStatus(..., RepositoryStatus.READ_ONLY) and fallback(...).
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java`:
- Around line 226-232: The current fallback flow logs, closes the encrypted repo
via ((GitRepository) encryptedRepository).close(...), and then calls
encryptionStorageManager().deleteRepositoryData(...) which may throw and cause
the whole fallback to be reported as failed; change deleteRepositoryData to be
best-effort: wrap the call in a try/catch that logs any exception (including
context like projectRepositoryName(repositoryName)) at warn/error and does not
rethrow, and consider delegating retries/asynchronous cleanup to a background
executor if transient errors are expected so the command completes and
RepositoryManagerWrapper can refresh its cache after the file-based repo is
already live.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryManagerWrapper.java`:
- Around line 66-70: The wrapper must perform an atomic handoff so the wrapped
cache is swapped before the old repo is closed: change the workflow so
RepositoryManagerWrapper.fallbackToFileRepository calls a modified
delegate.fallbackToFileRepository that returns the reopened Repository instance
(instead of void), then immediately do repos.replace(repositoryName,
repoWrapper.apply(reopenedRepo)) with that returned instance, and only after
that allow the delegate to finalize/close the old repo; ensure the delegate-side
fallback implementation is updated to return the reopened repo and defer closing
the old repository until after the caller has swapped in the new one.
In
`@server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java`:
- Around line 76-80: The tearDown() method currently only closes
encryptionStorageManager; update tearDown() to also close any open repository
managers (e.g., reloadedManager and any other RepositoryManager-like fields used
in tests) to avoid leaking JGit/RocksDB handles, and modify the test
fallbackRepoReloadsCorrectlyAfterManagerRestart() to ensure reloadedManager is
closed (either in a finally block or by delegating to tearDown cleanup) so all
repository managers are closed after each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66bee47f-29cf-454a-a320-4712ab0649be
📒 Files selected for processing (9)
server/src/main/java/com/linecorp/centraldogma/server/command/Command.javaserver/src/main/java/com/linecorp/centraldogma/server/command/CommandType.javaserver/src/main/java/com/linecorp/centraldogma/server/command/FallbackToFileRepositoryCommand.javaserver/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryManagerWrapper.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryManager.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java
server/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.java
Show resolved
Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java
Show resolved
Hide resolved
...a/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java
Outdated
Show resolved
Hide resolved
...a/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryManagerWrapper.java
Show resolved
Hide resolved
...necorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java
Show resolved
Hide resolved
jrhee17
approved these changes
Mar 16, 2026
| logger.warn("Failed to restore the encrypted repository placeholder file at: {}", | ||
| repoDir, ex); | ||
| } | ||
| throw new StorageException( |
Contributor
There was a problem hiding this comment.
Note) I wasn't able to find where these exceptions would be logged - no problem as long as they are logged somewhere
Contributor
Author
There was a problem hiding this comment.
It's logged here for the instance
https://github.com/line/centraldogma/pull/1276/changes#diff-3372efe1ec419ecaf68d991f7f6b04b77b97515d9265c2d6ad93cc6ed3d4e648R374
And for other replicas:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java (1)
221-247: Add a negative-path test for repos that were never migrated.This block already creates
otherRepoas an encrypted-from-scratch repository. A dedicated assertion thatfallbackToFileRepository(otherRepo)fails and keeps.encryption-repo-placeholderintact would lock down the unsupported case that the new production path currently has to roll back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java` around lines 221 - 247, Add a negative-path assertion for repos that were never migrated: in FallbackToFileRepositoryTest (e.g., in or alongside fallbackPreservesOtherRepoEncryptionData) call gitRepositoryManager.fallbackToFileRepository(otherRepo) wrapped in an assertThrows (expecting the appropriate runtime/illegal state exception) to confirm the operation fails, then assert gitRepositoryManager.get(otherRepo).isEncrypted() remains true and the repository still contains the ".encryption-repo-placeholder" marker (verify the placeholder file exists in the repo storage or via the same storage API used elsewhere) to ensure the placeholder is not removed on a failed fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java`:
- Around line 202-208: The restore of the durable placeholder file in
GitRepositoryManager must not be downgraded to a warn; when
Files.createFile(Paths.get(..., ENCRYPTED_REPO_PLACEHOLDER_FILE)) fails inside
the rollback paths, attach that failure to the original rollback exception and
rethrow so callers of openChild() see the combined failure. Concretely, in the
rollback blocks where you currently catch IOException and logger.warn(...),
instead catch the IOException, call
originalException.addSuppressed(restoreIOException) or wrap both into a new
IOException (or RuntimeException) that includes both messages, and rethrow the
original/combined exception; apply the same change to both restore sites shown
around the placeholder-restore code paths so the restore failure surfaces to
callers.
- Around line 184-195: Before deleting ENCRYPTED_REPO_PLACEHOLDER_FILE, guard
that the repository is actually an encrypted-migrated repo by checking
encryptedRepository.isEncrypted() and verifying the preserved file-based git
data exists in repoDir (e.g. presence of the .git directory or whatever marker
used by migrateToEncryptedRepository()); if either check fails, throw a
StorageException and do not mutate the repoDir so openFileRepository()/rollback
paths won’t run on an invalid state. Update the block around
get(repositoryName)/repoDir() to perform the isEncrypted() and repo-data-exists
checks before calling Files.delete(...) and keep error handling consistent with
existing StorageException usage.
---
Nitpick comments:
In
`@server/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java`:
- Around line 221-247: Add a negative-path assertion for repos that were never
migrated: in FallbackToFileRepositoryTest (e.g., in or alongside
fallbackPreservesOtherRepoEncryptionData) call
gitRepositoryManager.fallbackToFileRepository(otherRepo) wrapped in an
assertThrows (expecting the appropriate runtime/illegal state exception) to
confirm the operation fails, then assert
gitRepositoryManager.get(otherRepo).isEncrypted() remains true and the
repository still contains the ".encryption-repo-placeholder" marker (verify the
placeholder file exists in the repo storage or via the same storage API used
elsewhere) to ensure the placeholder is not removed on a failed fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1446fa5-5faa-4637-bced-7f5d089be8d1
📒 Files selected for processing (2)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/FallbackToFileRepositoryTest.java
...a/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java
Show resolved
Hide resolved
...a/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java (1)
210-215:⚠️ Potential issue | 🟠 MajorDon’t hide placeholder-restore failures in rollback.
After Line 198,
.encryption-repo-placeholderis the durable repo-type switch. If eitherFiles.createFile(...)fails here, restart behavior can diverge from the in-memory child, but callers only see the original reopen/replace failure. Add the restoreIOExceptionas suppressed and rethrow the combined failure from both rollback paths.🩹 Proposed fix
} catch (Throwable t) { // Restore the placeholder file so the manager stays in a consistent state. try { - Files.createFile(Paths.get(repoDir.getPath(), ENCRYPTED_REPO_PLACEHOLDER_FILE)); + Files.createFile(placeholderPath); } catch (IOException ex) { - logger.warn("Failed to restore the encrypted repository placeholder file at: {}", - repoDir, ex); + t.addSuppressed(ex); } throw new StorageException("failed to reopen the file-based repository after fallback. " + "repositoryName: " + projectRepositoryName(repositoryName), t); } @@ if (!replaceChild(repositoryName, encryptedRepository, fileRepository)) { fileRepository.internalClose(); + final StorageException cause = new StorageException( + "failed to replace the encrypted repository with the file-based repository. " + + "repositoryName: " + projectRepositoryName(repositoryName)); try { - Files.createFile(Paths.get(repoDir.getPath(), ENCRYPTED_REPO_PLACEHOLDER_FILE)); + Files.createFile(placeholderPath); } catch (IOException ex) { - logger.warn("Failed to restore the encrypted repository placeholder file at: {}", - repoDir, ex); + cause.addSuppressed(ex); } - throw new StorageException( - "failed to replace the encrypted repository with the file-based repository. " + - "repositoryName: " + projectRepositoryName(repositoryName)); + throw cause; }Also applies to: 223-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java` around lines 210 - 215, The rollback code in GitRepositoryManager that calls Files.createFile(Paths.get(repoDir.getPath(), ENCRYPTED_REPO_PLACEHOLDER_FILE)) currently swallows IOException by logging only; instead, catch that IOException, add it as a suppressed exception to the original reopen/replace failure (the primary exception caught earlier in the rollback path), and then rethrow the original exception so callers see the combined failure; apply the same change to the other rollback location that restores ENCRYPTED_REPO_PLACEHOLDER_FILE so both rollback paths attach the placeholder-restore IOException as suppressed and rethrow the primary exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java`:
- Around line 210-215: The rollback code in GitRepositoryManager that calls
Files.createFile(Paths.get(repoDir.getPath(), ENCRYPTED_REPO_PLACEHOLDER_FILE))
currently swallows IOException by logging only; instead, catch that IOException,
add it as a suppressed exception to the original reopen/replace failure (the
primary exception caught earlier in the rollback path), and then rethrow the
original exception so callers see the combined failure; apply the same change to
the other rollback location that restores ENCRYPTED_REPO_PLACEHOLDER_FILE so
both rollback paths attach the placeholder-restore IOException as suppressed and
rethrow the primary exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c04e7a7d-7a8e-4266-a700-9687a76786c9
📒 Files selected for processing (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java
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
2 participants
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.
Motivation:
After migrating a repository to an encrypted repository, there was no way to revert it back to the original file-based git repository.
Modifications:
FallbackToFileRepositoryCommandandCommandType.FALLBACK_TO_FILE_REPOSITORYPOST /projects/{projectName}/repos/{repoName}/migrate/fileendpoint, which falls back an encrypted repository.Result: