Skip to content

Commit bd51e38

Browse files
subrata71subrata71
authored
fix(git): surface the real cause of git failures instead of a static message (#42053)
Fixes APP-15731 Slack thread that prompted this: https://theappsmith.slack.com/archives/C09GSB3APNU/p1784892702257989 ## Why A customer's push to a self-hosted GitLab was rejected. The ticket ran for three days with support checking branch protection, push rules and deploy-key permissions in turn, because Appsmith never showed the reason. It could not: the reason was thrown away before it was ever logged. JGit reports a rejected push in two separate places. `RemoteRefUpdate.getMessage()` carries the terse report-status reason, which for every server-side hook is the same literal `pre-receive hook declined`. The explanation an operator actually needs — "GitLab: You are not allowed to push code to protected branches on this project", a push-rule violation, a secret-scanning block — travels on the sideband channel and is reachable only through `PushResult.getMessages()`. We read the first and dropped the second, then replaced the failure with a hardcoded guess: *"make sure you don't have any rules enabled on the branch X"*. No log level recovered it. That same shape — swallow the real error, emit a static string — recurs across the git flows. This PR fixes the class, not just the instance. ## What changed **The remote's own words now reach both the logs and the user.** `FSGitHandlerCEImpl.summarisePushResult` captures the sideband response, logs it once with repo, ref and remote, and carries it to `GitFSServiceCEImpl`, which puts it in the error the user sees instead of the guess. When the remote sends nothing back, the message names what to check rather than asserting a cause. **Stack traces survive.** Around 20 sites used `log.error("...", e.getMessage())`. SLF4J only captures a stack trace when the `Throwable` is the final argument, so these produced one line and nothing else. **Silent recovery is now visible.** `resetHard` logged a message and returned `false`, and the caller ignored it. After a rejected push nobody could tell whether the local rollback worked, so a commit could exist locally and nowhere else with no trace. **Lock lifecycle is diagnosable.** Contention, retry exhaustion and release logged nothing. "Another git operation is in progress" had no diagnostics at all. Per-attempt contention is at `debug` so a blocked operation does not emit 20 warnings; a single warning is raised when the retries run out. **Auto-commit stops failing invisibly.** It runs in the background, so eligibility errors logged at `debug` and rejected pushes logged not at all were effectively undetectable. `GitAutoCommitHelperImpl` also logged "is not allowed" unconditionally *before* the eligibility check, so every successful auto-commit logged the opposite of what happened. **Correlation ids** (artifact, ref, repo, workspace) added to existing git error logs. ## Behaviour changes Three genuine bugs surfaced during the audit and are fixed here: - The rejection check tested for `REJECTED_OTHERREASON`; JGit's enum is `REJECTED_OTHER_REASON`. The branch was dead, so any rejection whose message was not literally "pre-receive hook declined" fell through and **was reported to the user as a successful push** while the commit never left the server. - Two `Mono.error(error)` results in the merge flows were constructed but never returned, so an `AppsmithException` was replaced by a generic one. - The checkout guard compared a `GitRefDTO` to a `String` and so never matched, leaking JGit's raw `Ref <name> already exists` to the user instead of Appsmith's message. The user-facing text for a rejected push changes deliberately, from the hardcoded branch-rules guess to the remote's actual response. ## Not in this PR Deliberately kept out to stay reviewable: `ObservabilityLogger` emits the stack twice (SLF4J plus `printStackTrace`), and `GlobalExceptionHandler.getResponseDTOMono` releases the lock using a bare application id while `GitRedisUtils` stores it under `application-<id>`, so that defensive release never matches. Both are filed separately. ## Relationship to the EE PR EE counterpart: appsmithorg/appsmith-ee#9373 This PR carries the 15 files shared between CE and EE. The EE PR additionally covers EE-only surfaces with no CE equivalent: package git, the SSH key service, the continuous-delivery publish path, and EE's `FileUtilsImpl`. ## Test plan - [ ] `mvn -pl appsmith-server -am compile` passes (verified locally, BUILD SUCCESS) - [ ] Spotless clean (verified locally) - [ ] Reject a push on a deploy preview with a protected branch on the remote, and confirm the remote's message appears in the server log and in the UI error - [ ] Confirm a successful commit and push emits no new INFO lines - [ ] Hold a git lock and confirm one warning on retry exhaustion, not twenty - [ ] Confirm auto-commit no longer logs "is not allowed" for an eligible run ## Automation /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/30426486864> > Commit: 67dc29f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=30426486864&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 29 Jul 2026 14:28:10 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Git push status reporting with centralized accepted/rejected handling and clearer remote rejection diagnostics. * Fixed remote reference matching during checkout to avoid incorrect “already exists” results. * Enhanced push rejection recovery messaging for non-fast-forward and other remote rejection cases. * **Reliability** * Strengthened contextual error logging across git file/repo operations, including richer path and exception details. * Improved Redis lock acquisition/release and auto-commit eligibility/cleanup diagnostics, including outcomes when lock cleanup is skipped or fails. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 8ac6dc9 commit bd51e38

15 files changed

Lines changed: 454 additions & 133 deletions

File tree

‎app/server/appsmith-git/src/main/java/com/appsmith/git/constants/ce/CommonConstantsCE.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,13 @@ public class CommonConstantsCE {
3535

3636
public static final String WIDGET_ID = "widgetId";
3737
public static final String PARENT_ID = "parentId";
38+
39+
public static final String PUSH_STATUS_PREFIX = "Pushed successfully with status : ";
40+
41+
/**
42+
* Separates the per-ref push status from the remote's own sideband response within the push status string.
43+
* JGit exposes the two through different APIs, and only the sideband response explains why a remote rejected
44+
* a push, so both are carried back to the service layer.
45+
*/
46+
public static final String REMOTE_RESPONSE_DELIMITER = " | remote response: ";
3847
}

‎app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,11 @@ public Mono<Path> saveArtifactToGitRepo(
139139
return Mono.just(baseRepo);
140140
})
141141
.onErrorResume(error -> {
142+
log.warn(
143+
"Failed to update entities in the git repo, falling back to serialization driven by modified resources. repo={}, branch={}",
144+
baseRepo,
145+
branchName,
146+
error);
142147
return Mono.defer(() -> {
143148
return Mono.just(baseRepo).flatMap(baseRepo1 -> {
144149
try {
@@ -197,8 +202,11 @@ && isWhiteListedPath(
197202
whiteListedPaths,
198203
baseRepo.relativize(path).toString());
199204
} catch (IOException e) {
200-
log.error("Unable to find file details. Please check the file at file path: {}", path);
201-
log.error("Assuming that it does not exist for now ...");
205+
log.error(
206+
"Unable to read file details, assuming it does not exist. path={}, repo={}",
207+
path,
208+
baseRepo,
209+
e);
202210
return false;
203211
}
204212
})
@@ -235,8 +243,7 @@ protected Set<String> updateEntitiesInRepo(
235243
Files.deleteIfExists(baseRepo.resolve(filePath));
236244
} catch (IOException e) {
237245
// We ignore files that could not be deleted and expect to come back to this at a later point
238-
// Just log the path for now
239-
log.error("Unable to delete file at path: {}", filePath);
246+
log.error("Unable to delete file. path={}, repo={}", filePath, baseRepo, e);
240247
}
241248
});
242249

@@ -250,11 +257,15 @@ protected Set<String> updateEntitiesInRepo(
250257
resourceUpdated = fileOperations.hasFileChanged(
251258
entry.getValue(), filePathToObjectsFromFS.get(key.getFilePath()));
252259
} catch (IOException e) {
253-
log.error("Error while checking if file has changed", e);
260+
log.error(
261+
"Error while checking if file has changed, treating it as updated. path={}, repo={}",
262+
key.getFilePath(),
263+
baseRepo,
264+
e);
254265
}
255266

256267
if (resourceUpdated) {
257-
log.info("Resource updated: {}", key.getFilePath());
268+
log.debug("Resource updated: {}", key.getFilePath());
258269
String filePath = key.getFilePath();
259270
saveResourceCommon(entry.getValue(), baseRepo.resolve(filePath));
260271

@@ -296,8 +307,7 @@ protected Set<String> updateEntitiesInRepoFallback(GitResourceMap gitResourceMap
296307
Files.deleteIfExists(baseRepo.resolve(filePath));
297308
} catch (IOException e) {
298309
// We ignore files that could not be deleted and expect to come back to this at a later point
299-
// Just log the path for now
300-
log.error("Unable to delete file at path: {}", filePath);
310+
log.error("Unable to delete file. path={}, repo={}", filePath, baseRepo, e);
301311
}
302312
});
303313

@@ -435,8 +445,7 @@ protected boolean saveResource(Object sourceEntity, Path path) {
435445
Files.createDirectories(path.getParent());
436446
return fileOperations.writeToFile(sourceEntity, path);
437447
} catch (IOException e) {
438-
log.error("Error while writing resource to file {} with {}", path, e.getMessage());
439-
log.debug(e.getMessage());
448+
log.error("Error while writing resource to file. path={}", path, e);
440449
}
441450
return false;
442451
}
@@ -454,8 +463,7 @@ protected void saveResourceCommon(Object sourceEntity, Path path) {
454463
}
455464
fileOperations.writeToFile(sourceEntity, path);
456465
} catch (IOException e) {
457-
log.error("Error while writing resource to file {} with {}", path, e.getMessage());
458-
log.debug(e.getMessage());
466+
log.error("Error while writing resource to file. path={}", path, e);
459467
}
460468
}
461469

@@ -490,7 +498,7 @@ private boolean saveActionCollection(Object sourceEntity, String body, String re
490498
validatePathIsWithinGitRoot(metadataPath);
491499
return fileOperations.writeToFile(sourceEntity, metadataPath);
492500
} catch (IOException e) {
493-
log.debug(e.getMessage());
501+
log.error("Error while writing action collection to file. path={}, resource={}", path, resourceName, e);
494502
} finally {
495503
observationHelper.endSpan(span);
496504
}
@@ -529,7 +537,7 @@ private boolean saveActions(Object sourceEntity, String body, String resourceNam
529537
validatePathIsWithinGitRoot(metadataPath);
530538
return fileOperations.writeToFile(sourceEntity, metadataPath);
531539
} catch (IOException e) {
532-
log.error("Error while reading file {} with message {} with cause", path, e.getMessage(), e.getCause());
540+
log.error("Error while writing action to file. path={}, resource={}", path, resourceName, e);
533541
} finally {
534542
observationHelper.endSpan(span);
535543
}

‎app/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.java‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public void saveWidgets(JSONObject sourceEntity, String resourceName, Path path)
8080
objectReader.readTree(sourceEntity.toString()),
8181
path.resolve(resourceName + CommonConstants.JSON_EXTENSION));
8282
} catch (IOException e) {
83-
log.debug("Error while writings widgets data to file, {}", e.getMessage());
83+
log.error("Error while writing widgets data to file. path={}, resource={}", path, resourceName, e);
8484
} finally {
8585
observationHelper.endSpan(span);
8686
}
@@ -141,7 +141,7 @@ public Object readFile(Path filePath) {
141141
try (FileReader reader = new FileReader(filePath.toFile())) {
142142
file = objectReader.readValue(reader, Object.class);
143143
} catch (Exception e) {
144-
log.error("Error while reading file {} with message {} with cause", filePath, e.getMessage(), e.getCause());
144+
log.error("Error while reading file. path={}", filePath, e);
145145
return null;
146146
} finally {
147147
observationHelper.endSpan(span);
@@ -164,11 +164,7 @@ public Map<String, Object> readFiles(Path directoryPath, String keySuffix) {
164164
try (FileReader reader = new FileReader(file)) {
165165
resource.put(file.getName() + keySuffix, objectReader.readValue(reader, Object.class));
166166
} catch (Exception e) {
167-
log.error(
168-
"Error while reading file {} with message {} with cause",
169-
file.toPath(),
170-
e.getMessage(),
171-
e.getCause());
167+
log.error("Error while reading file. path={}", file.toPath(), e);
172168
}
173169
});
174170
}
@@ -192,6 +188,10 @@ public JSONObject getMainContainer(Object pageJson) {
192188
return new JSONObject(objectMapper.writeValueAsString(
193189
pageJSON.get("unpublishedPage").get("layouts").get(0).get("dsl")));
194190
} catch (JsonProcessingException e) {
191+
log.error(
192+
"Error while extracting the main container from the page DSL. page={}",
193+
pageJSON.path("unpublishedPage").path("name").asText(),
194+
e);
195195
throw new RuntimeException(e);
196196
}
197197
}
@@ -209,8 +209,7 @@ public boolean saveResource(Object sourceEntity, Path path) {
209209
Files.createDirectories(path.getParent());
210210
return writeToFile(sourceEntity, path);
211211
} catch (IOException e) {
212-
log.error("Error while writing resource to file {} with {}", path, e.getMessage());
213-
log.debug(e.getMessage());
212+
log.error("Error while writing resource to file. path={}", path, e);
214213
}
215214
return false;
216215
}
@@ -233,7 +232,10 @@ public void scanAndDeleteFileForDeletedResources(Set<String> validResources, Pat
233232
pathLocal.getFileName().toString()))
234233
.forEach(this::deleteFile);
235234
} catch (IOException e) {
236-
log.error("Error while scanning directory: {}, with error {}", resourceDirectory, e.getMessage());
235+
log.error(
236+
"Error while scanning directory for deleted resource files. directory={}",
237+
resourceDirectory,
238+
e);
237239
}
238240
}
239241
}
@@ -256,7 +258,10 @@ public void scanAndDeleteDirectoryForDeletedResources(Set<String> validResources
256258
&& !validResources.contains(path.getFileName().toString()))
257259
.forEach(this::deleteDirectory);
258260
} catch (IOException e) {
259-
log.error("Error while scanning directory {} with error {}", resourceDirectory, e.getMessage());
261+
log.error(
262+
"Error while scanning directory for deleted resource directories. directory={}",
263+
resourceDirectory,
264+
e);
260265
}
261266
}
262267
}
@@ -272,7 +277,7 @@ public void deleteDirectory(Path directory) {
272277
try {
273278
FileUtils.deleteDirectory(directory.toFile());
274279
} catch (IOException e) {
275-
log.error("Unable to delete directory for path {} with message {}", directory, e.getMessage());
280+
log.error("Unable to delete directory. path={}", directory, e);
276281
}
277282
}
278283
}
@@ -287,9 +292,9 @@ public void deleteFile(Path filePath) {
287292
try {
288293
Files.deleteIfExists(filePath);
289294
} catch (DirectoryNotEmptyException e) {
290-
log.error("Unable to delete non-empty directory at {} with cause", filePath, e.getMessage());
295+
log.error("Unable to delete non-empty directory. path={}", filePath, e);
291296
} catch (IOException e) {
292-
log.error("Unable to delete file {} with {}", filePath, e.getMessage());
297+
log.error("Unable to delete file. path={}", filePath, e);
293298
}
294299
}
295300

@@ -307,7 +312,7 @@ public String readFileAsString(Path filePath) {
307312
try {
308313
data = FileUtils.readFileToString(filePath.toFile(), "UTF-8");
309314
} catch (IOException e) {
310-
log.error("Error while reading the file from git repo {} ", e.getMessage());
315+
log.error("Error while reading the file from the git repo. path={}", filePath, e);
311316
} finally {
312317
observationHelper.endSpan(span);
313318
}
@@ -332,7 +337,8 @@ public Mono<Long> deleteIndexLockFile(Path path, int validTimeInSeconds) {
332337
return Mono.just(0L);
333338
}
334339
} catch (IOException ex) {
335-
log.error("Error reading index.lock file: {}", ex.getMessage());
340+
// A lock file left behind here keeps blocking every subsequent git operation on this repo
341+
log.warn("Unable to read the git lock file, it could not be cleaned up. path={}", path, ex);
336342
return Mono.just(0L);
337343
}
338344
}

0 commit comments

Comments
 (0)