chore: Added hooks to remove dangling locks (#41207)
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/17473362736> > Commit: 9bbf40be38011df0829473545833739e11d7b743 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=17473362736&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Thu, 04 Sep 2025 19:26:09 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Improved reliability of Git-connected workflows by automatically cleaning up dangling Git lock/index files before key operations, reducing intermittent errors and stuck states across checkouts, branch create/delete, commits, status, discard, and branch listing. - Chores - Made Git-in-memory detection more robust to avoid false positives when the Git root path is missing or contains whitespace. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
parent
096fc8aa69
commit
d1401ea603
|
|
@ -3,6 +3,7 @@ package com.appsmith.git.configurations;
|
||||||
import lombok.Data;
|
import lombok.Data;
|
||||||
import org.springframework.beans.factory.annotation.Value;
|
import org.springframework.beans.factory.annotation.Value;
|
||||||
import org.springframework.context.annotation.Configuration;
|
import org.springframework.context.annotation.Configuration;
|
||||||
|
import org.springframework.util.StringUtils;
|
||||||
|
|
||||||
@Data
|
@Data
|
||||||
@Configuration
|
@Configuration
|
||||||
|
|
@ -15,6 +16,13 @@ public class GitServiceConfig {
|
||||||
private String readmeTemplatePath;
|
private String readmeTemplatePath;
|
||||||
|
|
||||||
public Boolean isGitInMemory() {
|
public Boolean isGitInMemory() {
|
||||||
return gitRootPath.startsWith("/dev/shm/");
|
if (!StringUtils.hasText(gitRootPath)) {
|
||||||
|
return Boolean.FALSE;
|
||||||
|
}
|
||||||
|
|
||||||
|
final String trimmedRootPath = gitRootPath.strip();
|
||||||
|
return "/dev/shm".equals(trimmedRootPath)
|
||||||
|
|| trimmedRootPath.startsWith("/dev/shm/")
|
||||||
|
|| trimmedRootPath.startsWith("/tmp/shm/");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -486,10 +486,12 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
jsonTransformationDTO.setArtifactType(artifactType);
|
jsonTransformationDTO.setArtifactType(artifactType);
|
||||||
jsonTransformationDTO.setRepoName(baseGitMetadata.getRepoName());
|
jsonTransformationDTO.setRepoName(baseGitMetadata.getRepoName());
|
||||||
|
|
||||||
|
Mono<Boolean> removeDanglingLocksMono = gitHandlingService.removeDanglingLocks(jsonTransformationDTO);
|
||||||
|
|
||||||
if (gitRefDTO.getRefName().startsWith(ORIGIN)) {
|
if (gitRefDTO.getRefName().startsWith(ORIGIN)) {
|
||||||
// checking for local present references first
|
// checking for local present references first
|
||||||
checkedOutArtifactMono = gitHandlingService
|
checkedOutArtifactMono = removeDanglingLocksMono
|
||||||
.listReferences(jsonTransformationDTO, FALSE)
|
.then(gitHandlingService.listReferences(jsonTransformationDTO, FALSE))
|
||||||
.flatMap(gitRefs -> {
|
.flatMap(gitRefs -> {
|
||||||
long branchMatchCount = gitRefs.stream()
|
long branchMatchCount = gitRefs.stream()
|
||||||
.filter(gitRef -> gitRef.equals(finalRefName))
|
.filter(gitRef -> gitRef.equals(finalRefName))
|
||||||
|
|
@ -506,8 +508,8 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
});
|
});
|
||||||
} else {
|
} else {
|
||||||
// TODO refactor method to account for RefName as well
|
// TODO refactor method to account for RefName as well
|
||||||
checkedOutArtifactMono = gitHandlingService
|
checkedOutArtifactMono = removeDanglingLocksMono
|
||||||
.checkoutArtifact(jsonTransformationDTO)
|
.then(gitHandlingService.checkoutArtifact(jsonTransformationDTO))
|
||||||
.flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName(
|
.flatMap(isCheckedOut -> gitArtifactHelper.getArtifactByBaseIdAndBranchName(
|
||||||
baseArtifactId, finalRefName, gitArtifactHelper.getArtifactReadPermission()))
|
baseArtifactId, finalRefName, gitArtifactHelper.getArtifactReadPermission()))
|
||||||
.flatMap(artifact -> gitAnalyticsUtils.addAnalyticsForGitOperation(
|
.flatMap(artifact -> gitAnalyticsUtils.addAnalyticsForGitOperation(
|
||||||
|
|
@ -697,12 +699,14 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
fetchRemoteDTO.setRefType(refType);
|
fetchRemoteDTO.setRefType(refType);
|
||||||
fetchRemoteDTO.setIsFetchAll(TRUE);
|
fetchRemoteDTO.setIsFetchAll(TRUE);
|
||||||
|
|
||||||
|
Mono<Boolean> removeDanglingLock = gitHandlingService.removeDanglingLocks(baseRefTransformationDTO);
|
||||||
Mono<String> fetchRemoteMono =
|
Mono<String> fetchRemoteMono =
|
||||||
gitHandlingService.fetchRemoteReferences(baseRefTransformationDTO, fetchRemoteDTO, baseGitAuth);
|
gitHandlingService.fetchRemoteReferences(baseRefTransformationDTO, fetchRemoteDTO, baseGitAuth);
|
||||||
|
|
||||||
Mono<? extends Artifact> createBranchMono = acquireGitLockMono
|
Mono<? extends Artifact> createBranchMono = acquireGitLockMono
|
||||||
.flatMap(ignoreLockAcquisition ->
|
.flatMap(ignoreLockAcquisition -> removeDanglingLock
|
||||||
fetchRemoteMono.onErrorResume(error -> Mono.error(new AppsmithException(
|
.then(fetchRemoteMono)
|
||||||
|
.onErrorResume(error -> Mono.error(new AppsmithException(
|
||||||
AppsmithError.GIT_ACTION_FAILED, GitCommandConstants.FETCH_REMOTE, error))))
|
AppsmithError.GIT_ACTION_FAILED, GitCommandConstants.FETCH_REMOTE, error))))
|
||||||
.flatMap(ignoreFetchString -> gitHandlingService
|
.flatMap(ignoreFetchString -> gitHandlingService
|
||||||
.listReferences(createRefTransformationDTO, TRUE)
|
.listReferences(createRefTransformationDTO, TRUE)
|
||||||
|
|
@ -906,8 +910,10 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
jsonTransformationDTO.setBaseArtifactId(baseArtifactId);
|
jsonTransformationDTO.setBaseArtifactId(baseArtifactId);
|
||||||
jsonTransformationDTO.setRepoName(referenceArtifactMetadata.getRepoName());
|
jsonTransformationDTO.setRepoName(referenceArtifactMetadata.getRepoName());
|
||||||
|
|
||||||
return gitHandlingService
|
Mono<Boolean> removeDanglingLock = gitHandlingService.removeDanglingLocks(jsonTransformationDTO);
|
||||||
.deleteGitReference(jsonTransformationDTO)
|
|
||||||
|
return removeDanglingLock
|
||||||
|
.then(gitHandlingService.deleteGitReference(jsonTransformationDTO))
|
||||||
.flatMap(isReferenceDeleted -> gitRedisUtils
|
.flatMap(isReferenceDeleted -> gitRedisUtils
|
||||||
.releaseFileLock(artifactType, baseArtifactId, TRUE)
|
.releaseFileLock(artifactType, baseArtifactId, TRUE)
|
||||||
.thenReturn(isReferenceDeleted))
|
.thenReturn(isReferenceDeleted))
|
||||||
|
|
@ -1419,7 +1425,9 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
branchedArtifact.getGitArtifactMetadata().getRefName());
|
branchedArtifact.getGitArtifactMetadata().getRefName());
|
||||||
|
|
||||||
return gitHandlingService
|
return gitHandlingService
|
||||||
.prepareChangesToBeCommitted(jsonTransformationDTO, artifactExchangeJson)
|
.removeDanglingLocks(jsonTransformationDTO)
|
||||||
|
.then(gitHandlingService.prepareChangesToBeCommitted(
|
||||||
|
jsonTransformationDTO, artifactExchangeJson))
|
||||||
.then(updateArtifactWithGitMetadataGivenPermission(branchedArtifact, branchedGitMetadata));
|
.then(updateArtifactWithGitMetadataGivenPermission(branchedArtifact, branchedGitMetadata));
|
||||||
})
|
})
|
||||||
.flatMap(updatedBranchedArtifact -> {
|
.flatMap(updatedBranchedArtifact -> {
|
||||||
|
|
@ -1676,6 +1684,7 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
Mono<GitStatusDTO> lockHandledStatusMono = Mono.usingWhen(
|
Mono<GitStatusDTO> lockHandledStatusMono = Mono.usingWhen(
|
||||||
exportedArtifactJsonMono,
|
exportedArtifactJsonMono,
|
||||||
artifactExchangeJson -> {
|
artifactExchangeJson -> {
|
||||||
|
Mono<Boolean> removeDanglingChanges = gitHandlingService.removeDanglingLocks(jsonTransformationDTO);
|
||||||
Mono<Boolean> prepareForStatus =
|
Mono<Boolean> prepareForStatus =
|
||||||
gitHandlingService.prepareChangesToBeCommitted(jsonTransformationDTO, artifactExchangeJson);
|
gitHandlingService.prepareChangesToBeCommitted(jsonTransformationDTO, artifactExchangeJson);
|
||||||
|
|
||||||
|
|
@ -1695,7 +1704,8 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
error.getMessage()))));
|
error.getMessage()))));
|
||||||
}
|
}
|
||||||
|
|
||||||
return Mono.zip(prepareForStatus, fetchRemoteMono)
|
return removeDanglingChanges
|
||||||
|
.then(Mono.zip(prepareForStatus, fetchRemoteMono))
|
||||||
.then(Mono.defer(() -> gitHandlingService.getStatus(jsonTransformationDTO)))
|
.then(Mono.defer(() -> gitHandlingService.getStatus(jsonTransformationDTO)))
|
||||||
.onErrorResume(throwable -> {
|
.onErrorResume(throwable -> {
|
||||||
/*
|
/*
|
||||||
|
|
@ -1782,7 +1792,6 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
Mono<GitPullDTO> lockHandledpullDTOMono = Mono.usingWhen(
|
Mono<GitPullDTO> lockHandledpullDTOMono = Mono.usingWhen(
|
||||||
gitRedisUtils.acquireGitLock(artifactType, baseArtifactId, GitCommandConstants.PULL, TRUE),
|
gitRedisUtils.acquireGitLock(artifactType, baseArtifactId, GitCommandConstants.PULL, TRUE),
|
||||||
ignoreLock -> {
|
ignoreLock -> {
|
||||||
// TODO: verifying why remote needs to be fetched for status, when only modified is checked
|
|
||||||
Mono<GitStatusDTO> statusMono =
|
Mono<GitStatusDTO> statusMono =
|
||||||
getStatus(baseArtifact, branchedArtifact, false, false, gitType);
|
getStatus(baseArtifact, branchedArtifact, false, false, gitType);
|
||||||
|
|
||||||
|
|
@ -2156,8 +2165,10 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
Mono<? extends Artifact> artifactFromLastCommitMono = Mono.usingWhen(
|
Mono<? extends Artifact> artifactFromLastCommitMono = Mono.usingWhen(
|
||||||
gitRedisUtils.acquireGitLock(artifactType, baseArtifactId, GitCommandConstants.DISCARD, TRUE),
|
gitRedisUtils.acquireGitLock(artifactType, baseArtifactId, GitCommandConstants.DISCARD, TRUE),
|
||||||
ignoreLockAcquisition -> {
|
ignoreLockAcquisition -> {
|
||||||
Mono<? extends ArtifactExchangeJson> artifactJsonFromLastCommitMono = gitHandlingService
|
Mono<Boolean> removeDanglingLock =
|
||||||
.recreateArtifactJsonFromLastCommit(jsonTransformationDTO)
|
gitHandlingService.removeDanglingLocks(jsonTransformationDTO);
|
||||||
|
Mono<? extends ArtifactExchangeJson> artifactJsonFromLastCommitMono = removeDanglingLock
|
||||||
|
.then(gitHandlingService.recreateArtifactJsonFromLastCommit(jsonTransformationDTO))
|
||||||
.onErrorResume(exception -> {
|
.onErrorResume(exception -> {
|
||||||
log.error(
|
log.error(
|
||||||
"Git recreate Artifact Json Failed : {}",
|
"Git recreate Artifact Json Failed : {}",
|
||||||
|
|
@ -2269,6 +2280,9 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
jsonTransformationDTO.setRefName(currentBranch);
|
jsonTransformationDTO.setRefName(currentBranch);
|
||||||
jsonTransformationDTO.setRefType(branchedGitData.getRefType());
|
jsonTransformationDTO.setRefType(branchedGitData.getRefType());
|
||||||
|
|
||||||
|
GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);
|
||||||
|
Mono<Boolean> removeDanglingLocks = gitHandlingService.removeDanglingLocks(jsonTransformationDTO);
|
||||||
|
|
||||||
Mono<String> baseBranchMono;
|
Mono<String> baseBranchMono;
|
||||||
if (TRUE.equals(pruneBranches) && syncDefaultBranchWithRemote) {
|
if (TRUE.equals(pruneBranches) && syncDefaultBranchWithRemote) {
|
||||||
baseBranchMono = syncDefaultBranchNameFromRemote(baseGitData, jsonTransformationDTO, gitType);
|
baseBranchMono = syncDefaultBranchNameFromRemote(baseGitData, jsonTransformationDTO, gitType);
|
||||||
|
|
@ -2276,7 +2290,8 @@ public class CentralGitServiceCEImpl implements CentralGitServiceCE {
|
||||||
baseBranchMono = Mono.just(GitUtils.getDefaultBranchName(baseGitData));
|
baseBranchMono = Mono.just(GitUtils.getDefaultBranchName(baseGitData));
|
||||||
}
|
}
|
||||||
|
|
||||||
Mono<List<GitRefDTO>> branchMono = baseBranchMono
|
Mono<List<GitRefDTO>> branchMono = removeDanglingLocks
|
||||||
|
.then(baseBranchMono)
|
||||||
.flatMap(baseBranchName -> {
|
.flatMap(baseBranchName -> {
|
||||||
return getBranchListWithDefaultBranchName(
|
return getBranchListWithDefaultBranchName(
|
||||||
baseArtifact, baseBranchName, currentBranch, pruneBranches, gitType);
|
baseArtifact, baseBranchName, currentBranch, pruneBranches, gitType);
|
||||||
|
|
|
||||||
|
|
@ -120,4 +120,16 @@ public interface GitHandlingServiceCE {
|
||||||
|
|
||||||
Mono<MergeStatusDTO> pullArtifact(
|
Mono<MergeStatusDTO> pullArtifact(
|
||||||
ArtifactJsonTransformationDTO jsonTransformationDTO, GitArtifactMetadata baseMetadata);
|
ArtifactJsonTransformationDTO jsonTransformationDTO, GitArtifactMetadata baseMetadata);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Removes leftover Git lock and index files for the repository referenced by the provided DTO
|
||||||
|
* to unblock subsequent Git operations.
|
||||||
|
*
|
||||||
|
* <p>This is a best-effort cleanup that deletes ".git/index.lock" and ".git/index" if present.
|
||||||
|
* For in-memory Git repositories, this is a no-op.</p>
|
||||||
|
*
|
||||||
|
* @param jsonTransformationDTO DTO carrying repository identification and context used to resolve the path
|
||||||
|
* @return a Mono that emits TRUE after the cleanup attempt has been scheduled
|
||||||
|
*/
|
||||||
|
Mono<Boolean> removeDanglingLocks(ArtifactJsonTransformationDTO jsonTransformationDTO);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -926,4 +926,27 @@ public class GitFSServiceCEImpl implements GitHandlingServiceCE {
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Removes leftover Git lock and index files for the repository referenced by the provided DTO
|
||||||
|
* to unblock subsequent Git operations.
|
||||||
|
*
|
||||||
|
* <p>This is a best-effort cleanup that deletes ".git/index.lock" and ".git/index" if present.
|
||||||
|
* For in-memory Git repositories, this is a no-op.</p>
|
||||||
|
*
|
||||||
|
* @param jsonTransformationDTO DTO carrying repository identification and context used to resolve the path
|
||||||
|
* @return a Mono that emits TRUE after the cleanup attempt has been scheduled
|
||||||
|
*/
|
||||||
|
@Override
|
||||||
|
public Mono<Boolean> removeDanglingLocks(ArtifactJsonTransformationDTO jsonTransformationDTO) {
|
||||||
|
ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
|
||||||
|
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
|
||||||
|
|
||||||
|
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(
|
||||||
|
jsonTransformationDTO.getWorkspaceId(),
|
||||||
|
jsonTransformationDTO.getBaseArtifactId(),
|
||||||
|
jsonTransformationDTO.getRepoName());
|
||||||
|
|
||||||
|
return commonGitFileUtils.removeDanglingLocks(repoSuffix);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -86,6 +86,7 @@ import static com.appsmith.git.constants.ce.GitDirectoriesCE.DATASOURCE_DIRECTOR
|
||||||
import static com.appsmith.git.constants.ce.GitDirectoriesCE.JS_LIB_DIRECTORY;
|
import static com.appsmith.git.constants.ce.GitDirectoriesCE.JS_LIB_DIRECTORY;
|
||||||
import static com.appsmith.git.constants.ce.GitDirectoriesCE.PAGE_DIRECTORY;
|
import static com.appsmith.git.constants.ce.GitDirectoriesCE.PAGE_DIRECTORY;
|
||||||
import static com.appsmith.git.files.FileUtilsCEImpl.getJsLibFileName;
|
import static com.appsmith.git.files.FileUtilsCEImpl.getJsLibFileName;
|
||||||
|
import static java.lang.Boolean.TRUE;
|
||||||
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
|
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
|
||||||
import static org.springframework.util.StringUtils.hasText;
|
import static org.springframework.util.StringUtils.hasText;
|
||||||
|
|
||||||
|
|
@ -1019,4 +1020,49 @@ public class CommonGitFileUtilsCE {
|
||||||
JsonElement fileFormatVersion = metadataJsonObject.get(CommonConstants.FILE_FORMAT_VERSION);
|
JsonElement fileFormatVersion = metadataJsonObject.get(CommonConstants.FILE_FORMAT_VERSION);
|
||||||
return fileFormatVersion.getAsInt();
|
return fileFormatVersion.getAsInt();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Removes leftover Git lock and index files in the repository to unblock subsequent Git operations.
|
||||||
|
*
|
||||||
|
* <p>Specifically, deletes the files ".git/index.lock" and ".git/index" if they exist. This is a
|
||||||
|
* best-effort cleanup used when a previous Git operation was interrupted and left locks or a stale
|
||||||
|
* index behind. For in-memory Git repositories, this method is a no-op.</p>
|
||||||
|
*
|
||||||
|
* @param repositorySuffix Path of the repository relative to the configured Git root path.
|
||||||
|
* @return A Mono that emits TRUE after the cleanup attempt has been scheduled.
|
||||||
|
*/
|
||||||
|
public Mono<Boolean> removeDanglingLocks(Path repositorySuffix) {
|
||||||
|
return Mono.just(gitServiceConfig.isGitInMemory())
|
||||||
|
.map(inMemoryGit -> {
|
||||||
|
if (Boolean.TRUE.equals(inMemoryGit)) {
|
||||||
|
return TRUE;
|
||||||
|
}
|
||||||
|
|
||||||
|
final String GIT_FOLDER = ".git";
|
||||||
|
final String INDEX_LOCK = "index.lock";
|
||||||
|
final String INDEX = "index";
|
||||||
|
|
||||||
|
Path repositoryPath =
|
||||||
|
Path.of(gitServiceConfig.getGitRootPath()).resolve(repositorySuffix);
|
||||||
|
Path gitDir = repositoryPath.resolve(GIT_FOLDER);
|
||||||
|
|
||||||
|
Path lockFile = gitDir.resolve(INDEX_LOCK);
|
||||||
|
Path indexFile = gitDir.resolve(INDEX);
|
||||||
|
|
||||||
|
try {
|
||||||
|
Files.deleteIfExists(lockFile);
|
||||||
|
} catch (IOException ioException) {
|
||||||
|
log.warn("Error deleting git lock file {}: {}", lockFile, ioException.getMessage());
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
Files.deleteIfExists(indexFile);
|
||||||
|
} catch (IOException ioException) {
|
||||||
|
log.warn("Error deleting git index file {}: {}", indexFile, ioException.getMessage());
|
||||||
|
}
|
||||||
|
|
||||||
|
return TRUE;
|
||||||
|
})
|
||||||
|
.subscribeOn(Schedulers.boundedElastic());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user