Repository navigation
Conversation
Two unrelated-looking bugs share the same root defect: code that catches
an exception, then re-logs or re-throws it in a way that discards the
actual error detail.
- Script.executeInternal(): the non-zero-exit failure path opened a new
BufferedReader on a stream a draining Task had already consumed and
closed, so interpreter.processError() always threw "Stream closed"
instead of surfacing the real process output. Fixed by reusing
task.getResult() when interpreter.drain() is true, mirroring the
success path's existing logic. Two occurrences in the same file.
- DefaultModuleDefinitionSet.loadContext(): the caught exception was
passed as an ordinary format-string argument to logger.error() instead
of SLF4J's trailing-throwable argument, so the stack trace was never
logged. The re-thrown RuntimeException also didn't chain the original
cause. Fixed both: pass the exception as the throwable arg, and set it
as the new exception's cause.
Both were found while diagnosing real failures where the underlying
error was invisible in the logs and had to be recovered by manually
patching these exact lines.
Contributor
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14498 +/- ##
=========================================
Coverage 20.63% 20.63%
- Complexity 21695 21697 +2
=========================================
Files 6489 6489
Lines 590136 590136
Branches 72361 72363 +2
=========================================
+ Hits 121788 121802 +14
+ Misses 455100 455081 -19
- Partials 13248 13253 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✔️ debian ✔️ suse15. SL-JID 19722 |
Contributor
|
the el build failed but with errors that I would then expect on other builds as well. I am re-running the subjob to verify if it is a fluke FYI |
This branch has not been deployed
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
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.
Description
Fixes two unrelated-looking bugs that share the same root defect: code that catches an
exception, then re-logs or re-throws it in a way that discards the real error detail, leaving
only a generic/misleading message in the logs.
1.
Script.executeInternal()(two occurrences) — when a drainingOutputInterpreter(e.g.
AllLinesParser) is used and the process exits non-zero, the failure path opened asecond
BufferedReaderon the same process stream a backgroundTaskhad already consumedand closed. Calling
processError()on it always threwIOException: Stream closedinstead ofthe real process output — the actual stdout/stderr content was silently discarded even though
it had already been captured in
task.getResult(). The success path a few lines above alreadydoes this correctly; the failure path never applied the same check.
2.
DefaultModuleDefinitionSet.loadContext()— the caught exception was passed tologger.error()as an ordinary format-string argument instead of SLF4J's trailing-throwableargument, so the stack trace was never actually logged — only a one-line
toString()summary.The re-thrown
RuntimeExceptionalso didn't chain the original exception as its cause, so evena caller further up the stack couldn't recover it via
getCause().Functional behavior change: none for the success/non-error paths. On failure, both now surface
*(not tied to a filed issue — found during manual diagnosis, no tracking ticket exists yet)*the real underlying error instead of a misleading placeholder.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
(Neither bug causes incorrect behavior or data loss by itself — both are observability bugs
that hide the real cause of an unrelated failure. Marked Minor on that basis)
Screenshots (if appropriate):
N/A — logging/exception-handling fix, no UI or visible behavior change.
How Has This Been Tested?
mvn -pl :cloud-utils install -DskipTestsandmvn -pl :cloud-framework-spring-module install -DskipTests, bothBUILD SUCCESS).DefaultModuleDefinitionSetfix verified end-to-end: reproduced a real module-load failure,confirmed the log previously showed only
nested exception is java.lang.ExceptionInInitializerErrorwith nothing further; after the fix, rebuilding andre-running shows the full cause chain down to the actual root exception.
Script.javafix has not been exercised against a live non-zero-exit drained-interpreterfailure in this round of testing — the only real-world repro we had was a one-off incident
(malformed SSH key rejected by OpenSSH) that isn't easily reproduced on demand. The fix is a
direct structural mirror of the already-correct success-path logic a few lines above, in the
same method.
How did you try to break this feature and the system with this change?
Script.java: confirmed the non-draining-interpreter path (interpreter.drain() == false) is completely untouched by the ternary —processError(reader)still runs exactly asbefore for that case.
taskis only read whendrain()is true, which is the sameprecondition the existing success path already relies on, so no new null-pointer path is
introduced.
DefaultModuleDefinitionSet: checked for any other code relying on thisRuntimeExceptionnot having a cause set — found none. Setting the cause is purelyadditive; nothing downstream inspects or depends on
getCause()being null.