fix(git): surface the real cause of git failures instead of a static message - #42053
Merged
Conversation
…message Git failures were undiagnosable from logs. The flows caught real errors and either logged nothing, logged only e.getMessage() so no stack trace was captured, or replaced the remote's own explanation with a hardcoded string. The clearest case is a rejected push. JGit reports a rejection in two places: RemoteRefUpdate.getMessage() carries the terse report-status reason, which for every server-side hook is the same literal "pre-receive hook declined", while the explanation an operator needs travels on the sideband channel and is only reachable through PushResult.getMessages(). Appsmith read the first and dropped the second, then replaced the failure with "make sure you don't have any rules enabled on the branch X" - a guess. No log level recovered it, because the message was discarded before it was ever logged. The remote's response is now logged and reaches the user-facing error, so a customer can read GitLab's or GitHub's own reason rather than a guess. Also fixed across the git flows: - stack traces preserved by passing the Throwable as SLF4J's final argument - failed rollback after a rejected push is now reported, so a commit that exists locally but not on the remote is visible - lock contention, retry exhaustion and release are logged, making "another git operation is in progress" diagnosable - auto-commit eligibility failures raised from debug to warn, and a rejected auto-commit push is no longer silent - correlation ids (artifact, ref, repo, workspace) added to existing git logs Three behaviour bugs found alongside and fixed: - the rejection check tested for REJECTED_OTHERREASON; JGit's enum is REJECTED_OTHER_REASON, so the branch was dead and any rejection whose message was not literally "pre-receive hook declined" was reported to the user as a successful push - two Mono.error(error) results in the merge flows were never returned - the checkout guard compared a GitRefDTO to a String and so never matched, leaking JGit's raw "Ref <name> already exists" to the user
Contributor
WalkthroughGit push handling now propagates sanitized remote responses and classifies rejection statuses. Filesystem, Redis lock, auto-commit, checkout, and Git utility paths add contextual structured logging while preserving existing error-handling behavior in most flows. ChangesGit diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FSGitHandlerCEImpl
participant JGit
participant RemoteRepository
participant GitFSServiceCEImpl
FSGitHandlerCEImpl->>JGit: Push branch or tag
JGit->>RemoteRepository: Send ref updates
RemoteRepository-->>JGit: Return statuses and sideband response
JGit-->>FSGitHandlerCEImpl: Return PushResult
FSGitHandlerCEImpl->>GitFSServiceCEImpl: Return summarized push status
GitFSServiceCEImpl->>GitFSServiceCEImpl: Reset rejected updates
Possibly related issues
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (1)
1655-1682: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resetHard's rollback-failure path can be bypassed by acheckoutToBranchfailure.
resetHardchainscheckoutToBranch(repoSuffix, branchName).flatMap(...), but theonErrorResumethat returnsfalse(and now gets the improved log message) only wraps the innergit.reset()callable — not the leadingcheckoutToBranchcall. IfcheckoutToBranchitself throws (e.g. lock contention, dirty working tree during a rollback attempt), that error propagates directly out ofresetHardinstead of completing withfalse.Downstream in
GitFSServiceCEImpl.pushArtifactErrorRecovery, the new.doOnNext(isReset -> ...)rollback-failure log and the intendedbuildPushRejectionMessage(...)exception are only reached ifresetHardcompletes with a boolean — if it errors first, the caller instead surfaces whatevercheckoutToBranchthrew, losing the "push was rejected, rollback failed" context this PR is trying to add.🐛 Proposed fix: cover the whole rollback chain
public Mono<Boolean> resetHard(Path repoSuffix, String branchName) { return this.checkoutToBranch(repoSuffix, branchName) .flatMap(aBoolean -> Mono.using( () -> Git.open(createRepoPath(repoSuffix).toFile()), git -> Mono.fromCallable(() -> { Span jgitResetHardSpan = observationHelper.createSpan(GitSpan.JGIT_RESET_HARD); git.reset() .setMode(ResetCommand.ResetType.HARD) .setRef("HEAD~1") .call(); jgitResetHardSpan.end(); return true; }) .onErrorResume(e -> { log.error( "Hard reset to HEAD~1 failed, the local commit could not be rolled back. repo={}, branch={}", repoSuffix, branchName, e); return Mono.just(false); }) .timeout(Duration.ofMillis(Constraint.TIMEOUT_MILLIS)) .tag(HARD_RESET, TRUE.toString()) .name(GitSpan.FS_RESET) .tap(Micrometer.observation(observationRegistry)), Git::close)) + .onErrorResume(e -> { + log.error( + "Checkout before hard reset failed, the local commit could not be rolled back. repo={}, branch={}", + repoSuffix, + branchName, + e); + return Mono.just(false); + }) .subscribeOn(scheduler); }🤖 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 `@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java` around lines 1655 - 1682, Move or add the error-resuming fallback around the entire resetHard chain, including checkoutToBranch and the subsequent Git.open/reset operation, so any rollback failure completes with false rather than propagating an error. Preserve the existing failure logging and ensure the successful path still returns true after the hard reset.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
658-698: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat
NOT_ATTEMPTED/AWAITING_REPORT/NON_EXISTINGas failures too.summarisePushResultalready marks any non-OK/UP_TO_DATEupdate as a rejection, butpushArtifactErrorRecoveryonly handles the fourREJECTED_*statuses. These other rejection states fall through to the success path, so a failed atomic push can skip rollback and be reported as successful.🤖 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 `@app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java` around lines 658 - 698, Update pushArtifactErrorRecovery to handle every non-success remote update status, including NOT_ATTEMPTED, AWAITING_REPORT, and NON_EXISTING, instead of checking only the four REJECTED_* values. Reuse the same rejection/rollback path and buildPushRejectionMessage behavior used for existing failures, while preserving the GIT_UPSTREAM_CHANGES handling for non-fast-forward results.
🤖 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
`@app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.java`:
- Around line 354-360: Update the warning in GitRouteAspectCE’s retry exhaustion
handling to report the actual number of attempts: use the retry count that
includes the initial subscription, or add one to retrySignal.totalRetries()
before passing it to the “after {} attempts” placeholder. Keep the existing
gitCommand, key, and failure logging unchanged.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitSolutionCEImpl.java`:
- Around line 390-394: Update the error logging in the auto-commit rejection
flow around pushResponse to sanitize the remote-controlled text before passing
it to log.error: normalize CR/LF and other control characters, and truncate the
logged copy to a bounded length. Keep the original full pushResponse unchanged
for any user-facing error handling that requires it.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java`:
- Around line 115-121: Update the warning in the Mono.defer/onErrorResume flow
of AutoCommitEligibilityHelperImpl to identify whether the failure came from
fetching the latest DSL version or the filesystem page DSL, rather than always
attributing it to DSL-version fetching. Preserve the existing page, application,
refName, and error details while distinguishing both zipWith failure sources.
- Around line 159-164: Update the auto-commit eligibility flow around the
existing doOnError in AutoCommitEligibilityHelperImpl so any failure releases
the AUTO_COMMIT_ELIGIBILITY lock for defaultApplicationId before propagating the
original error. Make the cleanup error-safe, ensuring a lock-release failure
does not replace or suppress the initial eligibility-check failure, while
preserving the existing success-path behavior.
---
Outside diff comments:
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java`:
- Around line 1655-1682: Move or add the error-resuming fallback around the
entire resetHard chain, including checkoutToBranch and the subsequent
Git.open/reset operation, so any rollback failure completes with false rather
than propagating an error. Preserve the existing failure logging and ensure the
successful path still returns true after the hard reset.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java`:
- Around line 658-698: Update pushArtifactErrorRecovery to handle every
non-success remote update status, including NOT_ATTEMPTED, AWAITING_REPORT, and
NON_EXISTING, instead of checking only the four REJECTED_* values. Reuse the
same rejection/rollback path and buildPushRejectionMessage behavior used for
existing failures, while preserving the GIT_UPSTREAM_CHANGES handling for
non-fast-forward results.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eca0b4f5-ccba-479f-8eed-d7e763a354ed
📒 Files selected for processing (15)
app/server/appsmith-git/src/main/java/com/appsmith/git/constants/ce/CommonConstantsCE.javaapp/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.javaapp/server/appsmith-git/src/main/java/com/appsmith/git/files/operations/FileOperationsCEv2Impl.javaapp/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/aspect/ce/GitRouteAspectCE.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitSolutionCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitAsyncEventManagerImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/GitAutoCommitHelperImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
… lock on failure Addresses CodeRabbit review on appsmith#42053 and appsmith-ee#9373. - Text the remote git server controls (the sideband response and the per-ref rejection reason) is now stripped of control characters and capped before it reaches a log line or the user-facing error. Carriage returns would otherwise let a remote forge log entries and an unbounded response would flood them (CWE-117). Sanitising where the text enters the system also covers the auto-commit push log and the rejection message shown to the user, so no consumer has to remember to do it. - The rejected-push log no longer passes its arguments as an Object[], which PMD's InvalidLogMessageFormat cannot see through. - Lock retry exhaustion reported one attempt short, because RetrySignal.totalRetries() excludes the initial subscription. - The client auto-commit eligibility warning blamed the DSL version fetch for failures that can equally come from reading the page DSL off the file system. - The auto-commit eligibility lock was released only on the success path, so a transient failure left it held until its TTL expired and suppressed every later eligibility check for that artifact. It is now released on the error path too, without masking the original error.
…d it Addresses CodeRabbit review on appsmith#42053 and appsmith-ee#9373. The previous commit attached the lock cleanup to the whole chain, including addFileLock itself. Because addFileLock raises GIT_FILE_IN_USE when it loses on contention, and releaseFileLock deletes the Redis key unconditionally with no notion of ownership, a request that lost the race would delete the lock held by the request that won it - letting a third request in while the winner was still working. That is worse than the leak it was meant to fix. The cleanup is now scoped inside then(), so it only runs for failures that occur after this request successfully acquired the lock. Verified against reactor-core 3.7.18: on contention the release is not invoked at all, and on a post-acquisition failure the lock is released exactly once with the original error preserved.
wyattwalter
approved these changes
Jul 30, 2026
4 tasks
2 tasks
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.
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 literalpre-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 throughPushResult.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.summarisePushResultcaptures the sideband response, logs it once with repo, ref and remote, and carries it toGitFSServiceCEImpl, 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 theThrowableis the final argument, so these produced one line and nothing else.Silent recovery is now visible.
resetHardlogged a message and returnedfalse, 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
debugso 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
debugand rejected pushes logged not at all were effectively undetectable.GitAutoCommitHelperImplalso 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:
REJECTED_OTHERREASON; JGit's enum isREJECTED_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.Mono.error(error)results in the merge flows were constructed but never returned, so anAppsmithExceptionwas replaced by a generic one.GitRefDTOto aStringand so never matched, leaking JGit's rawRef <name> already existsto 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:
ObservabilityLoggeremits the stack twice (SLF4J plusprintStackTrace), andGlobalExceptionHandler.getResponseDTOMonoreleases the lock using a bare application id whileGitRedisUtilsstores it underapplication-<id>, so that defensive release never matches. Both are filed separately.Relationship to the EE PR
EE counterpart: https://github.com/appsmithorg/appsmith-ee/pull/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 compilepasses (verified locally, BUILD SUCCESS)Automation
/ok-to-test tags="@tag.All"
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/30426486864
Commit: 67dc29f
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 29 Jul 2026 14:28:10 UTC
Summary by CodeRabbit
Bug Fixes
Reliability