Thin-jar packaging, connector stability fixes, and portable/robust CI test tooling#2855
Merged
Merged
Conversation
Drop maven-shade-plugin in favor of maven-jar-plugin + maven-dependency-plugin: obp-api.jar now contains only this module's classes/resources (287MB -> ~31MB), with runtime dependencies copied to a sibling target/lib/ directory. The jar manifest carries Main-Class, a Class-Path pointing at lib/, and the same Add-Opens list the shaded jar used, so `java -jar` keeps working unchanged. This lets Docker layer caching skip re-pulling/pushing the full dependency set on every build when only application code changes. All four Dockerfiles that run `java -jar` (which only honors manifest Class-Path, not -cp) are updated to copy lib/ alongside the jar; Dockerfile_PreBuild's ENTRYPOINT and explicit add-opens flags are untouched. Dockerfile.dev is not touched: it is currently broken independently of this change (COPY build.sbt fails since build.sbt was removed repo-wide), so its `-pl .,obp-commons` reactor-scope question never gets reached.
… lines) The project migrated fully to Maven; build.sbt was already deleted. This removes the last git-tracked sbt remnants so the repo is unambiguously Maven-only: - Delete project/ (build.properties, plugins.sbt). No pom.xml or build script references it; the macro/paradise compiler plugin is already configured via scala-maven-plugin in pom.xml. - Drop the two dead COPY lines in development/docker/Dockerfile.dev: 'COPY build.sbt .' (referenced the already-deleted build.sbt, which broke this Dockerfile) and 'COPY project/ ./project/'. The sbt commands in the introductory system documentation are left as-is: they are the build instructions for the separate OBP-SEPA-Adapter project, which genuinely uses sbt.
chore: remove residual sbt build files (Maven-only cleanup)
…se default DB pool - Http4sBGv13PIS.updatePaymentPsuDataAll: in the SCA-finalised branch the status save (saveTransactionRequestStatusImpl) was nested inside map, so it ran fire-and-forget and the response could be built before COMPLETED was persisted. Switch map to flatMap so the save is awaited, matching the sibling failed/default branches. Mirrors the earlier cancelPayment fix. - JwtUtil.verifyHmacSignedJwt: the entry DEBUG log printed the full consent JWT and its HMAC shared secret, letting anyone with log-read access forge consent JWTs. Drop the token and secret from the message. - CustomDBVendor: raise the hikari.maximumPoolSize default from 10 to 20. Each request holds its transaction connection for its whole lifetime, so a pool of 10 exhausts at ~5 concurrent requests. Still prop-overridable.
incrementFutureCounter runs synchronously but the matching decrement only ran inside the wrapped Future's map/recover. A Future that never completes (hung backend) never decremented, leaving openFuturesCount to grow unbounded, ratcheting getBackOffFactor to its worst tier and causing canOpenFuture to reject nearly all calls with ServiceIsTooBusy until the JVM restarted. Add a self-cancelling reaper TimerTask that decrements the counter on a timeout ceiling if the Future hasn't completed by then, guarded by an AtomicBoolean so the reaper and the Future's own completion can never both decrement. The 2-arg overload is preserved unchanged for existing callers; a new 3-arg overload takes an explicit reaper timeout.
…Q connector Two related bugs in the RabbitMQ RPC path: - ResponseCallback.handle called promise.success with a block that could throw while evaluating the channel.close() cleanup. Since the throw happened while evaluating the argument to success, a channel-close failure meant the promise was never completed at all, and the exception escaped onto the RabbitMQ client dispatch thread. Fix: complete the promise with trySuccess before touching the channel, and only log a close failure instead of letting it prevent completion. - The RPC reply wait had no timeout: if an adapter never replied, the future returned by sendRequestUndGetResponseFromRabbitMQ hung forever, holding open the borrowed connection's per-request channel and the caller's slot in the backoff counter. Wrap the consume/take in FutureUtil.futureWithTimeout, bounded by a new rabbitmq_connector.response_timeout prop (default: the existing long_endpoint_timeout convention), kept under the reply queue's own 60s TTL/x-expires. On timeout the per-request channel is closed to release it back cleanly.
fix: await SCA payment status persistence, redact JWT secret log, raise default DB pool
fix: self-heal backoff counter and RabbitMQ RPC timeout/promise bug
…tes in CI (#50) * fix: make run_tests_parallel.sh portable, detect silently-aborted suites in CI run_tests_parallel.sh no longer depends on one developer's machine: - Discover JDK 25 across macOS/SDKMAN/Linux instead of a hard-coded path; abort with clear guidance if none is found instead of silently falling back to whatever JDK is active. - Replace the python3/xml.etree surefire-report audit with a pure-shell parser, so a broken or missing python3 (e.g. pyexpat/libexpat ABI mismatch) can no longer turn a green run into a false FAIL. Lint and the speed report (both non-authoritative) degrade to a visible skip instead when python3 is unavailable. - Inject harmless placeholder rabbitmq_connector.* props (matching the existing local default.props convention) so RabbitMQUtilsResponseCallbackTest actually executes instead of aborting during RabbitMQUtils' static init. That last point exposed a real CI gap: comparing the local abort against a downloaded CI shard log showed CI hits the exact same ExceptionInInitializerError, but has been reporting green regardless, because the "Report failing tests" step only grepped for "*** FAILED ***" and never checked for "*** RUN ABORTED ***" / "*** SUITE ABORTED ***" — which is what scalatest prints for an aborted suite, and maven.test.failure.ignore=true makes mvn exit 0 either way. Fixed the same grep pattern in both build_pull_request.yml and build_container.yml. Verified locally: full run now reports ALL SHARDS PASSED (2926 tests, 0 failures, 0 errors). * fix: give CI shard 8 the rabbitmq_connector.* props it needs The abort-detection fix in the previous commit now correctly surfaces RabbitMQUtilsResponseCallbackTest's pre-existing *** RUN ABORTED *** (RabbitMQUtils' object initializer throws ExceptionInInitializerError without these 5 mandatory props) instead of silently ignoring it. Add the same inert placeholder values used locally (default.props convention: localhost/5671/obp/obp//) to both workflows' "Setup props" step. The test mocks the channel and connector=star never routes to the real rabbitmq connector, so nothing here opens a real broker connection — this only lets the object initializer complete.
…g its init in tests RabbitMQUtilsResponseCallbackTest never touches RabbitMQUtils' real connector method, but referencing a class nested inside a Scala object forces the whole enclosing object to initialize first — including its 5 mandatory rabbitmq_connector.* props. That's what PR #50 worked around by injecting harmless placeholder values in three places (run_tests_parallel.sh env vars, both CI workflows' Setup props step). Moving ResponseCallback to a top-level class removes the need for that workaround entirely: the test now instantiates it directly without ever touching RabbitMQUtils' companion object, so those props are never read. Removed the now-unnecessary placeholder injections from all three places. Verified: RabbitMQUtilsResponseCallbackTest passes standalone with no rabbitmq_connector.* props or env vars set anywhere, and the full local suite reports ALL SHARDS PASSED (2926 tests, 0 failures, 0 errors).
…192) (#51) The pure-shell surefire-report parser repeated the "[0-9]+" literal 4 times across the tests/failures/errors/skipped extractions. Factored into a small _sf_attr() helper backed by one shared _SF_DIGITS pattern. Verified against the same fabricated pass/fail/truncated report fixtures used to validate the original parser — identical output.
- S7679: assign positional parameters ($1, $2) to local variables instead of using them directly in the function body. - S7682: add an explicit return statement at the end of the function. Verified: full local suite still reports ALL SHARDS PASSED (2926 tests, 0 failures, 0 errors).
hongwei1
changed the title
hongwei1
changed the title
|
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.
Summary
This branch bundles several independent, already-landed pieces of work with a sync to
OBP/develop(SIWE auth support, OAuth2/OIDC consumer resolution).Packaging
lib/directory (287MB → ~31MB), so Docker layer caching skips re-pulling the full dependency set when only application code changes. All four Dockerfiles updated; manifest keeps the sameAdd-Openslist sojava -jarbehaves unchanged.project/, deadDockerfile.devCOPY lines) now that the project is fully Maven-only, plus unusedweb-app_2_3.dtdcopies,Dockerfile.local, and outdatedbranding.md.Connector/stability fixes
Http4sBGv13PIS), instead of fire-and-forget — the response could previously race ahead of the COMPLETED write.JwtUtildebug logging no longer prints the consent JWT + its HMAC secret in the clear.CustomDBVendor) — each request holds its transaction connection for its full lifetime, so 10 exhausted at ~5 concurrent requests.StarConnector's backoff counter now self-heals when a future hangs forever instead of completing (FutureUtil) — previouslyopenFuturesCountonly decremented inside the future'smap/recover, so a hung backend call would ratchet the backoff to its worst tier permanently.ResponseCallback.handlecould throw while evaluatingchannel.close()inside thepromise.successblock — a close failure meant the promise was never completed and the exception leaked onto the RabbitMQ client dispatch thread.Test tooling portability + CI hardening (this session)
run_tests_parallel.shno longer depends on one developer's machine: JDK 25 is discovered portably (macOSjava_home/SDKMAN/Linux, refusing to run rather than silently falling back to the wrong JDK), and the authoritative surefire-report verdict is computed in pure shell instead of viapython3/xml.etree(a broken local Python/libexpat install could previously turn a green run into a false FAIL).build_pull_request.ymlandbuild_container.yml's "Report failing tests" step only grepped for*** FAILED ***, never*** RUN ABORTED ***/*** SUITE ABORTED ***— combined withmaven.test.failure.ignore=true, this letRabbitMQUtilsResponseCallbackTestsilently abort in CI on every run without ever failing the build.ResponseCallbackwas nested insideobject RabbitMQUtils, so referencing it in a test forced Scala to initialize the whole object — including 5 mandatoryrabbitmq_connector.*props the test never needs (it mocks the channel and never opens a real connection). MovedResponseCallbackto a top-level class so the test needs no RabbitMQ configuration at all.Test plan
./run_tests_parallel.sh→ALL SHARDS PASSED(2926 tests, 0 failures, 0 errors), including the previously-aborting RabbitMQ test with no RabbitMQ configuration present.docker/reportjobs also green onbuild_container.yml). One intermittent shard-8 failure was observed and re-confirmed as an unrelated pre-existing concurrency race inConcurrentConsentStatusRaceTest("H2"), not caused by this PR — same commit passed shard 8 cleanly on a subsequent run of the same workflow.