fix: multi-replica concurrency defects, then make the pieces separately deployable - #101
Open
hongwei1 wants to merge 28 commits into
Open
fix: multi-replica concurrency defects, then make the pieces separately deployable#101hongwei1 wants to merge 28 commits into
hongwei1 wants to merge 28 commits into
Conversation
The jobscheduler table only carried UniqueIndex(JobId), and every acquire generates a fresh JobId UUID, so nothing in the database stopped two rows from existing for the same job. Both MetricsArchiveScheduler and DataBaseCleanerScheduler take the lock by "find no row for this job name, then create one", which without such a constraint is a check-then-act race: two JVMs - or two threads in one JVM, since the metrics archive run is also reachable from POST /management/system/diagnostics/metrics/run - can both see no row, both insert, and both then run the job. A barrier fan-out of 12 acquirers on the old code produced 12 winners. Add UniqueIndex(Name) and route both schedulers through JobScheduler.tryAcquire, which treats the constraint rejection as the verdict "somebody else got there first" rather than letting it escape as a crashed scheduler tick. MetricsArchiveScheduler.runOnce keeps reading the current holder before it attempts the acquire, because a rejected INSERT leaves the surrounding transaction aborted on PostgreSQL when the manual-trigger endpoint is the caller; the post-failure read is therefore best-effort and the skip verdict does not depend on it. DataBaseCleanerScheduler additionally releases its lock in a finally, so a throw inside the cleanup no longer parks the lock until the 5-day sweep. Adding a unique index to an existing database fails if it already holds duplicates, which is exactly what the old race could produce, so jobscheduler joins mapperaccountholder and mappedentitlement in deduplicateBeforeUniqueIndexSchemify. Keeping the lowest id per name is right here too: the extras are the rows that should never have been granted the lock. ConcurrentSchedulerLockRaceTest covers both the deterministic case and a 12-thread barrier fan-out. Against the pre-fix code they report 12 winners of 12 and a second acquire succeeding.
…eduler locks DataBaseCleanerScheduler.start looked up its own leftover lock rows with By(JobScheduler.Name, apiInstanceId). Name holds the job name and ApiInstanceId holds the instance id, so the query matched nothing, ever: the boot-time self-heal was dead code. A lock orphaned by a kill -9, an OOM or a container eviction therefore survived every redeploy and blocked the cleanup for up to five days, until the age sweep removed it. MetricsArchiveScheduler had the identical bug and had it fixed there, in its own copy of the block. Rather than fix the second copy and leave a third waiting to be written, both now call JobScheduler.clearStaleLocksAtStartup, which owns the "drop my own orphans, leave other nodes' live locks alone, sweep anything too old to matter" policy once. The two schedulers cannot drift apart again. Covered by ConcurrentSchedulerLockRaceTest K3, which seeds two locks for this instance and one for another node: the pre-fix query removes zero rows.
MessageOutboxRelay.relayOnePass read the PENDING rows, filtered them by backoff and published them. MessageOutbox.pending() returns the same rows to every caller, so two relays - two replicas, or the incoming pod of a rolling update overlapping the outgoing one - publish the same row. For OPEN_CORRIDOR that is a real payment consequence, not duplicated work: the same credit notification and the same settlement instruction are delivered twice to the bank's own RabbitMQ vhost. A 12-way barrier fan-out over one row on the old code had all 12 relays proceed to publish. Each due row is now claimed before it is published, via a guarded UPDATE that sets attempts = attempts + 1 and updated_at, conditional on the row still being PENDING with the attempts value the caller read. The affected-row count is the verdict, so exactly one relay owns a row per pass. Bumping updated_at also re-arms the backoff, so a row being published right now does not look due to anybody else, and a relay that dies mid-publish simply leaves the row PENDING with one more attempt recorded - the at-least-once redelivery this table exists for. The terminal saves no longer increment attempts, since the claim already did. A guarded UPDATE rather than SELECT ... FOR UPDATE SKIP LOCKED because the relay runs on a scheduler thread with no http4s request scope, where DoobieUtil.runUpdate falls back to a transactor that commits when the statement returns - a row lock taken there is released immediately and excludes nobody (see the note on DoobieUtil.hasRequestScopeConnection). This follows the conditional-update pattern already used for consent state transitions in DoobieConsentSchedulerQueries. ConcurrentMessageOutboxRelayRaceTest covers two relays on one snapshot, a 12-relay barrier fan-out, and that a claim is a per-pass lease rather than a permanent lock so redelivery still works.
…eplicas Constant.ApiInstanceId appends a fresh UUID to the configured value unless it ends with the literal string "final", in which case every replica shares one id. That is not merely cosmetic: on boot each instance deletes every jobscheduler lock row carrying its own id, to clear locks orphaned by a JVM that died mid-run. With a per-JVM id only its own leftovers match; with a shared id a rolling restart deletes the lock another pod is holding at that moment, and the job it was protecting runs in two places at once. The shared id also merges every replica's Redis cache namespace. Document it at the constant and in sample.props.template, where the prop was not described at all. Behaviour is unchanged - existing deployments depend on it.
OBP-API has never been deployed as more than one process, and the reason is
Boot: every JVM unconditionally runs the full-schema DDL, both migration passes,
all the seed data and every background scheduler. A second replica is therefore
a second concurrent DDL run and a second copy of every scheduled job. Nothing
else was in the way — authentication is stateless, rate limiting, idempotency
and caching are already in Redis, and there is no in-process session.
Introduce instance.role, one prop with four values, so the same image and the
same jar can run as three workloads:
migrator schema + migrations + seed data, then exit. A one-shot Job.
scheduler the background schedulers only. One replica, kept out of the Service.
web serves HTTP only. Scale and roll this one.
all everything, i.e. exactly today's behaviour — and the default, so an
existing deployment that sets nothing is unaffected.
An unrecognised value aborts boot: a typo must not read as "quietly do none of
this work".
Two things every role keeps, and both are deliberate:
- The gRPC server and the process's single ordered shutdown hook. These used to
live in ToSchemify's object initialiser, which ran only as a side effect of
schemifyAll() touching ToSchemify.models. Graceful shutdown was therefore tied
to whether this JVM happened to run the DDL — which is exactly wrong once a
role skips it: a web instance would lose its ordered shutdown at the moment
rolling updates make ordered shutdown matter. They are now started explicitly
from boot. The default chat room seed, which hung off schemifyAll() for the
same reason, is split out as seed data rather than schema.
- MetricBatchWriter and ConnectorMetricBatchWriter. They look like schedulers
but are per-JVM buffer flushers: each request enqueues to an in-process queue
that their own daemon thread drains. Moving them to the scheduler role would
leave every web instance filling a queue nobody reads — all metrics lost, heap
growing without bound. So the schedulers to move are ten, not twelve.
Only migrator exits, and it exits before Http4sApp.httpApp is built so a Job
does not pay for a ResourceDoc registry it will never serve; the shutdown hook
still runs, so the pool and Redis close cleanly. A scheduler instance stays up
and still binds a port, giving it an endpoint for probes even though no Service
routes to it.
The role rules are plain functions of a role name, with the vals applying them
to this JVM's configured role, because instance.role is resolved once at
class-initialisation time: a single JVM can only ever observe one role, so rules
baked into the vals alone would be untestable. InstanceRoleTest covers the four
roles, the normalisation, the rejection of an unknown value, and that the three
dedicated roles partition boot with no overlap and no gap.
Also fixes a documentation bug found here: sample.props.template advertised
transaction_status_scheduler_delay, which Boot never reads — the prop it looks
up is transaction_request_status_scheduler_delay. Anyone who copied the
template's spelling had that scheduler silently disabled.
…e disabled Two things wrong with the whole-version gate, both invisible until you try to turn a version off. Berlin Group was never gated at all. Every other standard goes through Http4sApp.gate, which consults api_disabled_versions / api_enabled_versions; the three Berlin Group route trees (v2, v1.3 and the v1.3 alias) were referenced straight from the per-request chain. So an operator could disable any version they liked except Berlin Group, and nothing said otherwise. They are gated now. The alias needs BOTH its own version and the canonical v1.3 to be allowed: it is a second URL prefix in front of the same endpoints, so leaving it serving them after v1.3 was disabled would make the disable meaningless. The gate took its routes BY VALUE. Scala evaluates the argument before gate can decide anything, so a version excluded by api_disabled_versions still had its entire object initialised — every ResourceDoc registered, every route built — and the result then discarded. Disabling a version cost exactly as much boot work as keeping it, which is the opposite of the point. The parameter is by name now, so an excluded version is never touched. The decision moves to its own object, VersionGate, rather than staying a private method on Http4sApp. Touching Http4sApp initialises every version's routes and needs a database, so a rule living there could only be exercised by a full server-backed suite; as a standalone pure function it is covered by a unit test that runs in milliseconds. VersionGateTest pins both halves — empty when excluded, and the excluded routes never forced — plus that an allowed version is built once rather than per request. Switching the parameter back to by value turns the third one red. Not included: the k8s readiness initialDelaySeconds, which the assessment also lists under this stage. Those manifests live in obp-local-k3s / obp-server-k3s, not in this repository.
…he standard
code.api.util is imported by hundreds of files, the API standards among them —
and it imported them back. APIUtil, ResourceDocRegistry and ErrorResponseConverter
each reached into code.api.berlin, so the core and the standard were mutually
dependent: neither could be compiled, reasoned about or eventually extracted
without the other.
The core genuinely needs three things from Berlin Group, and none of them is
Berlin Group logic. It has to recognise a BG URL, because a BG request gets a
BG-shaped error body instead of OBP's; it has to know that shape in order to
build one; and the resource-doc registry has to rank the BG standard against UK
Open Banking when both register the same partialFunctionName. Those are protocol
vocabulary — two prop-derived ScannedApiVersions and two four-field case classes.
They move into code.api.util.BerlinGroupVocabulary, and ConstantsBG plus the
Berlin Group JSON factory alias them, so the hundreds of call sites inside the
standard and the twenty-odd test imports are untouched and the wire format, being
field-named, is unchanged.
Deliberately NOT the registry inversion where the standard registers itself into
a table the core reads. APIUtil is touched extremely early and from everywhere,
long before any Berlin Group object is initialised, so such a registry would be
reliably empty at the moment the core consults it — and the failure would be
silent: a Berlin Group client receiving OBP-shaped errors. Constants with no
behaviour cost nothing to own here and cannot be mistimed. The reasoning is in
the file so the next person does not have to rediscover it.
CoreDependencyDirectionTest is the gatekeeper: one Maven module means nothing
stops the next edit from adding the import straight back. It scans the core
sources and names the offending file and line. Three files are exempt, each with
its reason recorded, and a second test keeps that list honest by failing if an
exemption stops being needed:
Http4sApp the composition root — naming every standard's routes IS its
job. Inverting it would mean a runtime plugin registry, trading
away the compile-time guarantee that each wired standard exists.
ConsentUtil a real violation, not a principled exemption: 2,600+ lines
coupling versions, persistence and schedulers. Its own task.
BerlinGroupCheck Berlin Group logic that happens to sit in this package; the
fix is to move the file, not to invert anything.
Reintroducing any removed import turns the guard red with the exact line.
ConcurrentBackoffCounterSelfHealTest asserted on a counter that is decremented
from a reaper TimerTask and from the wrapped Future's onComplete — both
asynchronous, and neither ordered against Await.result returning. Each of the
three scenarios slept a fixed amount and then asserted, which encodes a guess
about scheduling latency that is wrong exactly when the machine is busy.
It failed that way in a four-shard parallel run ("behave exactly like the
original Future when it completes immediately", count=1 instead of 0) while
passing three times out of three in isolation.
Poll for the expected value with a deadline instead. The tests now wait only as
long as they must and still fail honestly when the value genuinely never
arrives. The one place a fixed settle is still right is the double-decrement
check, where nothing further is expected to happen and the hazard is the counter
moving to -1; that one keeps its sleep, with the reason written down.
Verified 3/3 in isolation and 3/3 again under eight busy cores, which is the
condition the parallel runner creates.
…cabulary Everything in this repository is one obp-api module, so "ErrorMessages must not reach into the connector" is a convention a reviewer has to notice. This adds a module where it is a build failure instead. obp-kernel holds values and pure functions with no dependency on persistence, HTTP, the connector, or any API standard: ErrorMessages, ApiTag, SortFields, plus small holders for the date patterns and reflection predicates that the messages quote. Everything in obp-api can see obp-kernel; nothing in obp-kernel can see obp-api, and the compiler enforces the arrow. Not obp-commons: that module already depends on lift-persistence, so it is not a place a persistence-free kernel can live. The moved classes keep the package names they had (code.api.util). A package spanning two jars on one classpath is fine, and it means none of the 176 files importing ErrorMessages changed. The module boundary is the point; churn across the call sites would have been pure cost. Four dependencies had to be cut before ErrorMessages could move, and finding them took more than removing the wildcard import — qualified APIUtil.x calls survive that and only show up by grep: - apiFailureToString needs APIFailureNewStyle, CallContext and a json4s Formats. It was never an error *message* anyway, it is how a failure is serialised, so it becomes ApiFailureRenderer in obp-api. One call site. - FrequencyPerDayError read a prop while the object initialised. It is now a def taking the limit, which also fixes a latent bug: as a val it captured whatever the prop said at class-init, so changing the prop produced a message that contradicted the limit actually enforced. One call site, which now passes it. Checked first that it appears in no ResourceDoc error list, so dropping it from the allFields reflection does not affect the Swagger error definitions. - DateWithMs and notExstingBaseClass move down; APIUtil reads them from the kernel so there is one definition rather than a copy on each side. - getDuplicatedMessageNumbers parsed ErrorMessages.scala with scalameta and was used only by one test. It moves into that test, where scalameta already is — the kernel is API vocabulary, not a place for a Scala parser. ApiRole stayed behind deliberately: besides the static enumeration it registers roles created at runtime for dynamic entities and endpoints, which pulls in DynamicEntityHelper and DynamicEndpointHelper. The kernel holds only the two role names the messages quote, with a test pinning them to the real values so the copies cannot drift. Adding a module means teaching the build about it in eight places, and the easy one to miss is the CI install-file step: compile and test run on different runners, and the test job resolves com.tesobe:* from artifacts installed there by hand. Without the new jar the shards cannot resolve obp-api's classpath at all — and this passes locally regardless, because the jar is already in ~/.m2. Verified by deleting ~/.m2/repository/com/tesobe/obp-kernel and re-running the full suite from scratch: 3843 tests, 0 failures.
ABAC rules are user-supplied Scala compiled at runtime. That engine is moving to an optional module, so a deployment that wants no Scala compiler in its image can simply not ship one — but the core still has to ask the question on every account access. APIUtil now asks code.api.util.AbacAccountAccess instead of calling AbacRuleEngine directly, and no longer imports the engine at all. The default when nothing is installed is Full(false), and picking it was the whole point of this change rather than an afterthought. hasAccountAccess reaches ABAC only in its last branch, after public-view, firehose and ordinary view checks have each already refused — the comment there reads "Normal checks failed — try ABAC as fallback". ABAC is therefore a pure GRANT: it hands out access the view model denied, and it can never retract access the view model allowed, because that case returned Full(true) before getting here. So "no engine" must mean "grants nothing", which is identical to the shipped allow_abac_account_access =false. Full(true) would instead be a silent authorisation bypass on every account in the instance, caused by nothing more than an absent optional module. The prop and role guards stay in APIUtil rather than moving behind the seam. They are the core's policy — whether this instance lets ABAC widen access at all, and whether this user may have it widened — not the engine's, and keeping them in front means an instance with the feature off never reaches the seam. A mutable registry is acceptable here where stage 3 rejected one for the Berlin Group vocabulary, and the difference is what "not yet installed" means. There it produced a wrong answer silently (a BG client receiving OBP-shaped errors); here it produces the shipped default, and a late registration can only add capability, never retract an access already granted. There is also no window in practice: Http4sServer finishes Boot.boot before binding a port. AbacAccountAccessTest pins the default, the pass-through of an installed engine's verdict (Failure included — in the fallback position a reasoned denial and a plain decline are both refusals), and that removing the engine returns to the default rather than keeping the last answer. Flipping the default to Full(true) turns two of them red with the consequence spelled out. Also drops two dead imports from APIUtil: OBPAPIDynamicEndpoint and OBPAPIDynamicEntity each occurred exactly once in the file, on their own import line.
…do without Dynamic Resource Docs, Dynamic Message Docs and Connector Methods are bodies of Scala, JS or Java that a privileged user POSTs and the server compiles at runtime. On JDK 24+ the SecurityManager sandbox that used to contain them cannot even be installed — DynamicUtil catches the failure and doPrivileged degrades to a pass-through — so holding the relevant entitlement is effectively host RCE. A deployment that does not use the feature should be able to ship without the toolbox at all. This puts the compiler behind code.api.util.DynamicCode so that it can. The seam is narrow because the compiled artefact does not cross it. All 23 call sites had the same shape: compile, take Failure.msg on failure, booleanToFuture, validateDependency — and then discard the compiled function. So the seam carries only "did it compile" plus the diagnostics, and Http4sEndpointIO, DynamicFunction and both toolboxes stay entirely on the engine side, which is what allows them to move to another module. Two behaviours are preserved deliberately rather than tidied: - validateDependency still THROWS. It raises JsonResponseException, which already carries the 400 and the message the client receives; folding it into the Left would change what callers see. The trait documents that compile failure returns Left while dependency rejection propagates. - compileProblems with no compiler installed returns one problem, not an empty list. Empty is the compiler's way of saying "this compiles"; returning it when nothing examined the code would tell the author their body is fine. With no compiler installed, every check answers Left(ErrorMessages.DynamicCodeExecutionDisabled) — byte for byte what these endpoints already answer when allow_user_generated_scala_code is false. An absent module is therefore indistinguishable from the feature being switched off, which is what it is. Http4s400, Http4s600 and Http4s700 now have zero references to CompiledObjects, DynamicUtil, InternalConnector or DynamicConnector. DynamicCodeTest pins that, having to exclude NewStyle.function.invokeDynamicConnector, which merely contains one of the names while being a call into the service layer. It also pins the no-compiler answers, since those files are 13k, 18k and 9k lines and a single re-added import would quietly put the compiler back on the required path.
…iler A Dynamic Resource Doc is a method body a privileged user POSTs; the server compiles it and the result becomes a live endpoint under /obp/dynamic-endpoint. The core needs to know those endpoints exist in two places — the resource-doc aggregation behind /resource-docs, and the router that dispatches a request to one — and neither should need a compiler to do it. Both sides of what crosses here are already core types: ResourceDoc, and the compiled handler it carries in dynamicHttp4sFunction. So the whole compiled artefact passes through CompiledEndpoints as data the core can hold without being able to produce, and DynamicEndpoints stays on the engine side with the toolbox. With nothing installed there are no compiled endpoints: docs is empty and find matches nothing. That is the accurate answer for an instance without a compiler rather than a degraded one — an endpoint that was never compiled does not exist, so /resource-docs should not list it and a request to it should fall through the chain to the 404 it would have got anyway. APIUtil no longer imports DynamicEndpoints at all, and Http4sDynamicEndpoint's runtime-compiled dispatch now asks the seam. Also drops a dead DynamicEndpoints import from OBPAPIDynamicEntity. Still on the engine side and still to do: the ABAC management endpoints (validate / execute / clear-cache in Http4s600 and Http4s700, distinct from the authorisation path already behind a seam) and the "internal" connector registration in Connector.scala.
…compiler Completes what the previous three commits started. The core no longer references DynamicUtil, CompiledObjects, DynamicEndpoints, InternalConnector, DynamicConnector or AbacRuleEngine anywhere — only two documentation strings still mention them by name. Three more seams, and the defaults differ on purpose: - AbacRules, for the operator-facing rule management (validate, execute, clear-cache). Its default is Failure(DynamicCodeExecutionDisabled), the OPPOSITE of AbacAccountAccess's Full(false), because the two are asked in different situations. The authorisation path is on ordinary data requests and must degrade silently; a Failure there would turn a normal account read into an error for a caller who never asked about ABAC. These operations are what an operator explicitly invoked, and answering Full(false) would tell them their rule evaluated to false when in fact nothing evaluated it. One seam with one default could not serve both without lying to one of them. - OptionalConnectors, because "internal" executes Scala the operator uploaded and so belongs with the compiler rather than in Connector's fixed map. Absent, it fails as InvalidConnector, which is accurate: the connector does not exist. - PractiseEndpointJson, because SwaggerDefinitionsJSON used PractiseEndpoint.RequestRootJsonClass as a worked example — and PractiseEndpoint extends DynamicCompileEndpoint, so the published swagger document was pinning the toolbox to the required path of every deployment. The simple names are unchanged deliberately: the swagger definition is named after them. MakerChecker stays in obp-api and goes through the compile seam instead of moving with the engine. Its 521 lines are approval workflow — submit, expire, intercept, mark-approved — used from Boot and three version files at a dozen sites; only four lines of it compile anything. Fixes a regression this introduced: taking "internal" out of Connector.nameToConnector broke method-routing validation, which returned 400 for connector_name=internal. The lesson is that moving an entry out of a fixed registry means finding who ITERATES the registry, not who references the entry — NewStyle.getSupportedConnectorNames / getConnectorByName and the v6 connector listing all read the shipped map. They read availableConnectors now, and nameToConnector carries a comment saying which one callers want. DynamicCodeTest gains a core-wide direction guard. The first version matched substrings and reported grantEntitlementsToUseDynamicEndpointsInSpaces, newInternalConnector, DynamicConnectorMethod.methodBody and this change's own AbacRuleEngineProvider — none of which touch the engine. It matches whole identifiers now: a guard that floods the log with false positives is worse than none, because it is the one that gets deleted. Injecting a real `import code.abacrule.AbacRuleEngine` into NewStyle turns it red with the file, line and the seam to use instead.
The seams added over the last four commits mean nothing in the core names the
runtime compiler any more. This makes that worth something operationally: the
toolbox can be deleted from a deployment and the instance still starts, serves
and reports the feature as disabled.
rm lib/scala-compiler-*.jar
rm lib/polyglot-*.jar lib/js-language-*.jar lib/truffle-*.jar \
lib/regex-*.jar lib/icu4j-*.jar
That is 71 MB of a 294 MB lib/. Nothing else changes: the jars still ship by
default, so an existing deployment is untouched.
Why it is worth doing at all: on JDK 24+ the SecurityManager sandbox DynamicUtil
tries to install cannot be created, and doPrivileged degrades to a pass-through.
There is no enforcement left, so for an instance that does not use dynamic code,
holding the relevant entitlement is effectively host RCE for no benefit.
Both installs now survive the classes being absent. They catch LinkageError
rather than Exception, because a missing class behind a lazily-resolved reference
arrives as NoClassDefFoundError, which is an Error and not covered by NonFatal.
Verified end to end rather than by reasoning, and that mattered. The first probe
booted cleanly but still advertised `internal` in GET /system/connectors on an
instance that could not use it. The argument that "the exception fires while
evaluating InternalConnector.instance, so register never runs" was wrong:
InternalConnector.instance is a CGLib proxy and does not force DynamicUtil — the
throw actually came one line later, from reading isEnabled for the log message,
after the connector had been registered. install() now touches the toolbox first,
before installing anything, and the catch unwinds the registration too.
Re-verified on a rebuilt deployment with the jars removed: boots with no
exception, binds its port, GET /obp/v7.0.0/root returns 200, the dynamic
endpoints return 401 rather than 500, the log states precisely what is
unavailable, and `internal` is absent from the connector list. Full suite on the
complete deployment: 3854 tests, 0 failures.
sample.props.template documents the removal next to instance.role, since an
operator looking for "how do I not ship this" will look in the props file.
hongwei1
changed the title
…pace The prop has two consumers that do not read the same thing. Constant.ApiInstanceId appends a fresh UUID, so the jobscheduler lock rows and metric rows get an id unique to each JVM. Constant.getGlobalCacheNamespacePrefix re-reads the raw prop instead and falls back to the literal string "obp", so an unset value puts every deployment that also leaves it unset in the namespace "obp_<run.mode>_". Two unrelated deployments pointed at different databases but sharing one Redis then share cache keys, and nothing says so: both log "Global cache namespace prefix: 'obp_prod_'". Found by running this branch on a local k3s cluster, where three new pods silently landed in the same namespace as unrelated instances already running there. The test harness already handles this path (ServerSetup gives each parallel shard a distinct OBP_API_INSTANCE_ID so their Redis keys do not collide); only the deployment side was undocumented. Behaviour unchanged.
JSONFactory3.1.0.scala imported code.api.v5_0_0.HelperInfoJson and never used it: the name appears nowhere else in code/api/v3_1_0. It was one of the twelve places where an older version package references a newer one, and the only one that costs nothing to remove. Behaviour unchanged.
…de deployments The role gating and the removable toolbox already exist; what was missing was the statement that they compose, and the list of paths that makes the composition routable. Adds docs/operations/SPLIT_DEPLOYMENT.md: the four deployments, the two-stage Dockerfile that actually produces a smaller image (deleting jars in a RUN layer after COPY saves nothing), the twelve management paths that need a runtime compiler with ingress rules for them, and the constraint that routing cannot fix — checkAbacAccountAccess sits on the account-access authorisation chain rather than on an endpoint, so a deployment that grants access through ABAC rules cannot run web instances without the toolbox. Moving that call across a process boundary is not an option worth taking: it is a synchronous Await with a 10 second budget on the authorisation hot path. Also records a failure mode found while running this: an instance that starts before the migrator has created the schema throws out of main, but the Hikari housekeeper is a non-daemon thread, so the JVM stays up and the pod reports Running indefinitely. Readiness keeps it out of the Service; without a liveness probe it never restarts. Every figure and log line quoted is from a run on a k3s cluster, including the ingress upstream log showing the split and the before/after of scaling the compiler instance to zero. The note on scheduler mutual exclusion says which path that run exercised and which one it did not. No code change.
AccountAttributeResponseJson and TransactionAttributeResponseJson are fields of
ModeratedCoreAccountJsonV300, TransactionJsonV300 and CoreTransactionJsonV300, but were declared
in the v3.1.0 and v4.0.0 factories. That made v3.0.0 — the version that puts these shapes on the
wire — depend on two newer version packages, against the direction of the bridge cascade.
Neither name carries a version, which is the test that separates this from the other backward
references in the same file: v3.0.0's getBanks calls JSONFactory400.createBankJSON400, whose
return type is BankJson400, and moving that would be relabelling rather than fixing. These two
are genuinely misplaced, so they move to v3_0_0 together with their factory functions, and the
later versions reuse them in the cascade direction.
v3.0.0's backward references drop from five to one — the createBankJSON400 call, which is
structural and cannot be fixed by moving a file.
The JSON on the wire is unchanged. Swagger definition names are unaffected too, because
SwaggerJSONFactory derives them from getClass.getSimpleName, which does not include the package.
FrozenClassTest pins the fully qualified type of every field of a frozen API type, so it flags
this move. Each of the ten reported differences was checked field by field first: all are
code.api.v3_1_0.X -> code.api.v3_0_0.X and code.api.v4_0_0.X -> code.api.v3_0_0.X with identical
field names, cardinality and shapes. The snapshot was then regenerated the way that suite
prescribes (FROZEN_REGENERATE=true, which is required precisely so a CI run cannot rewrite it).
The regenerated snapshot also picks up one line unrelated to this change:
+field code.api.v5_1_0.APITags tags List[String]
APITags arrived in 4a2dbe2 and was never added to the snapshot, because the "structure not
modified" scenario only compares types already present in it — a type that is merely new is not
flagged. Recording it here rather than dropping it: the snapshot now covers it.
… stops depending on it JvalueCaseClass is one line — a JValue carried as a case class so it can stand in for a ResourceDoc example body. Nothing about it is Berlin Group specific, but it was declared in JSONFactory_BERLIN_GROUP_1_3.scala, and twenty UK Open Banking v3.1.0 handlers, JSONFactory1_4_0 and the resource-doc aggregation all reached into the Berlin Group package for it. That is what made the two standards inseparable: UK Open Banking could not be extracted without Berlin Group underneath it, for one wrapper neither of them owns. Moving it to code.api.util leaves both standards depending only on the core. Measured: UK Open Banking's references to code.api.berlin drop from twenty files to zero, and the files outside the Berlin Group package that still reach into it drop from 34 to 13. What is left is real coupling, in four groups: ConstantsBG (7 files), the consent JSON shapes the core reads (4), TransactionStatus and BgSpecValidation (3), and the composition root (2). The resource-doc machinery strips this wrapper by matching the field name jvalueToCaseclass (ResourceDocsAPIMethods.scala:1259, Http4sResourceDocs.scala:243), and SwaggerJSONFactory names definitions from getClass.getSimpleName — neither depends on the package, so the move does not change any emitted document. No behaviour change.
Two descriptions carried a bare placeholder in angle brackets: deleteUser's "DELETED-<random-string>" (v4.0.0) and the metrics diagnostic's "ORIGINALLY_NOT_SET-<uuid>" (v7.0.0). Neither is valid XML, and descriptions are parsed as XML: ResourceDocsTest wraps each one in a div and calls scala.xml.XML.loadString, with the comment "API_Explorer side use this method, so it need to be right" — the Explorer renders them the same way, so an unclosed tag breaks the page rather than merely failing a test. Backticks do not help; the parser sees the raw string, which is why the v7.0.0 one was equally broken despite being written as a code span. Both now use brace placeholders, which read the same and are safe in every consumer. The test only parses the first three docs of each version, so whether this fired depended on document ordering: it passed four consecutive full runs and then failed. That is also why it was not caught when the v4.0.0 line was added in e3ecea8. Neither string appears in scripts/resource_doc_baseline, so the parity audit is unaffected.
…nstantsBG ConstantsBG's two version values were already nothing but aliases of code.api.util.BerlinGroupVocabulary, yet fourteen call sites outside the standard still went through the alias to reach them. They now name the definition directly. SigningBasketsStatus is the substantive half. It is an Enumeration of Berlin Group status codes, but the core is what writes them: MappedSigningBasketProvider stamps RCVD on create and CANC on cancel. Having core persistence import the enumeration from code.api.berlin.group is exactly the inversion BerlinGroupVocabulary exists to undo, so the definition moves there and ConstantsBG keeps an alias for the standard's own 58 call sites. Nothing outside code.api.berlin.group names ConstantsBG any more. Files outside the Berlin Group package that still reach into it: 13 -> 10. What remains is either real (TransactionStatus in the scheduler and connector, BgSpecValidation in BerlinGroupCheck, the consent JSON shapes the core reads and documents) or the composition root, which is where wiring belongs. No behaviour change.
…Open Banking SwaggerDefinitionsJSON imported eighteen JSON types from the UK Open Banking package to build example values, and Http4sUKOBv200AIS imported those examples back out of the core. Core and standard each needed the other to compile. The nineteen example values move to code.api.UKOpenBanking.SwaggerDefinitionsUKOB, next to the types they are examples of, with their own allFields — the shape MessageDocsSwaggerDefinitions already uses. Where the core then aggregates them matters. Putting it in Http4sResourceDocs made CoreDependencyDirectionTest fail, correctly: that file is under code.api.util, which must not name a standard, and the guard's own note says the fix is almost never a fourth exemption. The aggregation belongs in code.api.ResourceDocs1_4_0 instead — naming every standard is that package's job, and SwaggerJSONFactory there already names all three UK Open Banking versions. So SwaggerDefinitionsAggregation lives there and code.api.util names only it. The two documents are kept separate rather than merged. They already differed: the connector message-docs swagger also draws on MessageDocsSwaggerDefinitions, the resource-docs swagger does not. A single combined list would have added message-type definitions to a published document that never had them, so there are two values and each keeps exactly the sources it had. allFields is reflective, so a value that failed to move would silently drop a definition. Both aggregation points were updated, and ResourceDocsTest and SwaggerDocsTest serialise the whole API surface, so a miss shows up as a failure rather than a quietly smaller document. SwaggerFactoryUnitTest used the UK AccountInner as its fixture for rendering Option[String]. It now uses the core's own accountAttributeResponseJson, whose product_instance_code is the same shape, which removes a core test's dependency on a standard. No behaviour change: both swagger documents carry the same definitions as before.
Nothing consumes this yet, which is the point of doing it on its own. Every module that could be split out of obp-api sits above it, not below: the runtime compiler because DynamicUtil injects imports of most of the application into the code it compiles, UK Open Banking and Berlin Group because their handlers go through NewStyle. Their tests — 3,333 lines for UK, 4,680 for Berlin Group, 2,569 for the compiler — all extend code.setup.ServerSetup and its fourteen companions, which live in obp-api's test sources. Without a test-jar those tests cannot move with the code they cover, and a module whose tests stayed behind is not extracted. Kept separate from any module move deliberately. CI compiles and tests on different runners, so a build change has failure modes that a green local run cannot show; this one gets its own CI pass rather than being diagnosed at the same time as a package move. The execution overrides the plugin configuration rather than inheriting it: the main jar needs the executable manifest (Main-Class plus the lib/ classpath prefix), and a jar that is consumed as a dependency should not carry one.
Four endpoints called Atms.atmsProvider and Branches.branchesProvider directly instead of going
through Connector: getAtms and getBranches in v1.4.0, getAtm and getBranch in v2.1.0. Every later
version goes through Connector, where MethodRouting decides per method which connector answers,
and three of the connectors are out of process.
A deployment that routes getAtms to a remote connector therefore got remote data from v3.0.0 and
local data from v1.4.0 for the same bank. The four now go through Connector like the rest.
Error semantics are unchanged. The v1.4.0 list endpoints still raise the same exception when the
connector returns nothing. v2.1.0 getAtm uses NewStyle.function.getAtm, which already answers
AtmNotFoundByAtmId with 404. v2.1.0 getBranch does not use NewStyle.function.getBranch, because
that one answers 400 and this endpoint has always answered 404; it calls Connector and maps the
miss itself.
The provider layer added nothing the connector path lacks: BranchesProvider.getBranches carries
a comment about filtering by licence and then returns the provider's result unfiltered, and the
same is true for atms. Both mapped paths read the same table.
ConnectorRoutingBypassTest keeps it that way: no code outside the provider packages and the
connectors may name atmsProvider or branchesProvider. Whole identifiers only. Verified by adding
a bypass and watching the test name its file and line.
The two v1.4.0 suites installed a mock on the provider and asserted on the mock's data. With the
endpoint reading through Connector, that mock was unreachable and the suites failed with
"0 did not equal 3". They now seed the same three fixtures through Connector.createOrUpdateBranch
and createOrUpdateAtm, the way the v3.0.0 suite does. Two details: the branch fixtures are
declared for BankId("uk") and go in under the bank the request names (verifySameData does not
compare bankId), and seeding happens in beforeEach rather than beforeAll, because
ServerSetup.beforeEach wipes every table before each scenario.
This is the first step of splitting reference-data domains into their own processes: MethodRouting
is the mechanism, and it can only route what passes through Connector.
…mote connector Add the eleven atm/branch methods whose DTOs already exist in obp-commons to ConnectorBuilderUtil.commonMethodNames (createOrUpdateAtm, createOrUpdateBranch, deleteAtm, deleteAtmAttribute, deleteAtmAttributesByAtmId, updateAtmAccessibilityFeatures, updateAtmLocationCategories, updateAtmNotes, updateAtmServices, updateAtmSupportedCurrencies, updateAtmSupportedLanguages) and regenerate the five products that consume the list. Until now only getAtm/getAtms/getBranch/getBranches could be routed to a remote connector: a deployment that routes atms elsewhere could read them, but every write still landed in the local mapped store. The generator's signature rendering had drifted with scala-reflect, which now renders a method type as "(params): Result" rather than "(params)Result"; the unconditional colon insertion produced "): :" in every signature. Insert the colon only when it is missing. Regenerating also brings the products back in line with the list as it stands in the repository. None of this changes behaviour: - rabbitmq gains checkExternalUserCredentials, checkExternalUserExists and getCurrentFxRate, reclassified upstream in eab0d99 - rest gains 13 and akka 24 methods that the grpc product already had - updateCustomerGeneralData loses the "= None" defaults that fd6339e added by hand inside the generated region; the overrides inherit them from Connector, so callers are unaffected - example values catch up with on_behalf_of_user_id and originator - akka imports PaymentServiceTypes, SuppliedAnswerType and TransactionRequestTypes, which its newly generated methods reference MSsqlStoredProcedure.sql is regenerated from the same list and keeps every procedure it had. The Rest binary and text snapshots are regenerated: the text diff is 209 added lines and no removals, and frozen_type_meta_data.txt is unchanged because the new methods only use existing types. MockedRabbitMqAdapter is left untouched. AdapterStubBuilder fails on OutBoundOpenCorridorCreditNotification with a ClassCastException that predates this change and is tracked separately.
New Maven module obp-atms, depending on obp-commons only (same constraint as obp-kernel, enforced by the module boundary rather than convention). It owns two Postgres tables (atms, branches — column-for-column the same shape as code.atms.MappedAtm / code.branches.MappedBranches in obp-api) and speaks the RabbitMQConnector_vOct2024 wire protocol from the consumer side: one shared queue, the AMQP messageId carries the process name, the reply goes to the AMQP replyTo queue with the correlationId echoed back. It reuses com.openbankproject.commons.util.JsonSerializers.nullTolerateFormats, the exact json4s Formats the core uses to write outbound and read inbound adapter messages, including AbstractTypeDeserializer — the serializer that lets trait-typed OutBound fields (atm: AtmT, branch: BranchT) decode into their concrete Commons class. There is no independent codec on the adapter side to drift out of sync with the core's. Routes 13 of the 19 atm/branch Connector methods: getAtm, getAtms, getBranch, getBranches, createOrUpdateAtm, createOrUpdateBranch, deleteAtm, and the six updateAtm* attribute-list methods. Four methods have no InBound/OutBound DTOs at all and stay on the mapped connector. Two more (deleteAtmAttribute, deleteAtmAttributesByAtmId) have DTOs and this module implements handlers for them, but they are deliberately left off the recommended routing table: atm attributes live in the core's MappedAtmAttribute table, which nothing here can take over (their create/read counterparts have no DTOs to route), so routing only the delete would split one logical table across two stores. updateAtmServices carries its payload in a wire field literally named supportedCurrencies (obp-commons dto/JsonsTransfer.scala) even though the value is the services list — a pre-existing mismatch that predates this module, present in the Connector trait's own abstract signature. AtmRepo mirrors LocalMappedConnector's actual behaviour (write to `services`) rather than the field's name, with a comment at both the DTO boundary and the repo. 20 tests: AtmRepoSpec and BranchRepoSpec run the real upsert/find/update/ delete SQL against H2 in Postgres-compatibility mode (a from-scratch hand-written 48/53-column INSERT is exactly where a positional-parameter slip compiles fine and corrupts data); MessageHandlerSpec builds real OutBound* DTOs with the core's own Formats, feeds the raw JSON to the handler, and extracts the reply back into InBound* — the wire-fidelity check, not just this module's own repos. Verified against a real k3s cluster: POST .../atms returns 201 and the row lands in obp-atms's own database while the core's mappedatm table stays at zero rows; GET returns the same data; scaling obp-atms to zero makes only the atm/branch endpoints fail (504 after the RabbitMQ response timeout) while /root and /banks stay 200 throughout; scaling back to one recovers immediately. Full account in docs/operations/ATMS_MICROSERVICE.md, including a pre-existing unrelated schema-migration gap (v_account_access_with_views) this run surfaced on a freshly built image against an existing database.
…alysis The PR's SonarCloud quality gate has been failing on new_duplicated_lines_density (26.9% against a 3% threshold) since before this change, dominated by the five generated connector products under code.bankconnectors — 5,375 of the 5,399 total new duplicated lines, 1,429 alone in AkkaConnector_vDec2018.scala (71.56% density). Each of these files is ConnectorBuilderUtil output implementing the same ~150-method Connector interface for a different wire protocol, so the same method bodies are structurally repeated across all five products by design — that is not a code smell to fix, it is what a generator emitting the same shape five times looks like. sonar-project.properties already excludes test sources from the same check for an analogous reason; extend the same sonar.cpd.exclusions list to the five generated connector files.
getProducts read Products.productsProvider directly, so a deployment that routes getProducts to a remote connector still served this version's callers from the local mapped store — v1.4.0 and v4.0.0 would disagree about the same bank's products. Same defect and same fix as getAtms/getBranches in this file. Empty params are what the endpoint always meant: it never filtered. With Nil, LocalMappedConnector.getProducts reduces to the same MappedProduct.findAll(By(mBankId)) the provider call made, and both paths return an empty bank as a present-but-empty list rather than a failure, so the mapped case is unchanged. ProductsTest installed a ProductsProvider mock, which the endpoint can no longer reach. It now seeds real rows through Connector.createOrUpdateProduct in beforeEach — ServerSetup.beforeEach wipes the tables before every scenario, so per-suite seeding would be gone by the first request — and cleans up in afterEach. The list assertion changes from 3 to 2 products because the mock returned the third bank's product for both banks; a real query is scoped by bank, which is what the scenario claims to test. ConnectorRoutingBypassTest covers productsProvider as well now. The sandbox importer is exempt and says why: its provider call is the pre-flight duplicate check of a bulk import whose writes go straight to the mapped store as Saveable[ProductType], so routing only the read would have it ask the remote store a question it then answers locally. A second test keeps that exemption honest — it fails if the exempted file stops calling a provider, so the exemption cannot outlive its reason. Both guards falsified: reintroducing a productsProvider call turns the first test red with the exact file:line, and pointing the exemption at a provider-free file turns the second red.
|
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
1 participant
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.
Three concurrency defects that are dormant on a single replica and guaranteed to fire on a
second one, plus one documented trap. Nothing here changes deployment, and
Boot.scalaisuntouched — this is the safety groundwork that makes running more than one OBP-API process a
deliberate step rather than a gamble.
Base:
0b03e908e0c4b36ef4a91a7cd8e30d9496c9a87d(developat the time of branching;identical to
Hongwei/develop). One commit per defect, so any of them can be reverted alone.1. The
JobSchedulerleader lock did not lock —3e8a02db6jobschedulercarried onlyUniqueIndex(JobId), and every acquire generates a freshJobIdUUID, so no two acquires could ever collide on it. Both
MetricsArchiveSchedulerandDataBaseCleanerSchedulertake the lock by find no row for this job name, then create one,which with no constraint behind it is a plain check-then-act race.
Failure scenario. Two OBP instances tick their metrics-archive schedulers within the same
few milliseconds. Both
findreturnEmpty, both insert, both run. The two runs then walkoverlapping candidate windows of
MappedMetric, each copying rows toMetricArchiveanddeleting the originals — and each deletes the lock row on the way out, so the table ends up
looking idle while two jobs were in flight. This is reachable on one JVM too:
runOnceis alsocalled from
POST /management/system/diagnostics/metrics/run, so a manual trigger landing on ascheduled tick races the same way.
Fix. Add
UniqueIndex(Name)and route both schedulers throughJobScheduler.tryAcquire,which reads the constraint rejection as the verdict somebody else got there first rather than
letting it escape as a crashed scheduler tick.
MetricsArchiveScheduler.runOncestill reads thecurrent holder before attempting the acquire: on PostgreSQL a rejected INSERT leaves the
surrounding transaction aborted, and the manual-trigger endpoint is a POST, so it runs inside the
request transaction. The read after a failed acquire is therefore best-effort and the skip verdict
does not depend on it.
DataBaseCleanerScheduleralso releases its lock in afinally, so athrow inside the cleanup no longer parks the lock until the 5-day sweep.
Adding a unique index to a database that already holds duplicates fails — which is exactly what
the old race could produce — so
jobschedulerjoinsmapperaccountholderandmappedentitlementin
deduplicateBeforeUniqueIndexSchemify. Keeping the lowest id per name is right here too: theextras are the rows that should never have been granted the lock.
2.
DataBaseCleanerScheduler's startup self-heal was dead code —e62e0e07fJobScheduler.findAll(By(JobScheduler.Name, apiInstanceId))—Nameholds the job name andApiInstanceIdholds the instance id, so the query matched nothing, ever.Failure scenario. A pod is OOM-killed mid-cleanup. The
finallynever runs, so its lock rowsurvives. On restart the self-heal is supposed to clear exactly this — its own orphans — but the
query returns nothing, so the row stays. Token/nonce cleanup is then blocked for up to five days,
until the age sweep removes it. Every redeploy in between makes no difference.
MetricsArchiveSchedulerhad the identical bug and had it fixed there, in its own copy of theblock. Rather than fix the second copy and leave a third waiting to be written, both now call
JobScheduler.clearStaleLocksAtStartup, which owns the "drop my own orphans, leave other nodes'live locks alone, sweep anything too old to matter" policy once.
3.
MessageOutboxRelayrepublished settlement instructions —e462a9455The one defect here with a payment consequence rather than a wasted-work consequence.
relayOnePassread the PENDING rows, filtered by backoff, and published them.MessageOutbox.pending()returns the same rows to every caller, and nothing claimed a row before it was published.
Failure scenario. During a rolling update the incoming and outgoing pods overlap for a few
seconds. Both relays tick, both read the same PENDING rows, both publish. For
OPEN_CORRIDORthatmeans the same Interface C
obp_credit_notificationandobp_settlement_instructionare deliveredtwice to the bank's own RabbitMQ vhost — the bank is told twice to credit the same beneficiary.
Two steady-state replicas do this on every pass, not just during a deploy.
Fix. Each due row is claimed before it is published, via a guarded UPDATE conditional on the
row still being PENDING with the
attemptsvalue the caller read; the affected-row count is theverdict, so exactly one relay owns a row per pass.
attemptsdoubles as the claim token, andbumping
updated_atre-arms the backoff so a row being published right now does not look due toanybody else. A relay that dies mid-publish simply leaves the row PENDING with one more attempt
recorded — the at-least-once redelivery this table exists for. The terminal saves no longer
increment
attempts, since the claim already did.Deliberately not
SELECT ... FOR UPDATE SKIP LOCKED: the relay runs on a scheduler thread withno http4s request scope, where
DoobieUtil.runUpdatefalls back to a transactor that commits whenthe statement returns. A row lock taken there is released immediately and excludes nobody — the
same reasoning already spelled out on
DoobieUtil.hasRequestScopeConnection. The guarded UPDATEfollows the pattern already used for consent state transitions in
DoobieConsentSchedulerQueries.4. Documented: an
api_instance_idending infinalis unsafe with replicas —20aec4601Constant.ApiInstanceIdappends a fresh UUID to the configured value unless it ends with theliteral string
final, in which case every replica shares one id. Both schedulers delete everyjobschedulerrow carrying their own id on boot, so with a shared id a rolling restart deletes thelock another pod is holding at that moment. The shared id also merges every replica's Redis cache
namespace.
Documented at the constant and in
sample.props.template, where the prop was not described at all.Behaviour is unchanged — existing deployments depend on it.
Concurrency verification
Compiling is not evidence for this kind of change, so each fix was checked by reverting it to its
pre-fix form, running the new suites against that, and then restoring it. Both defects produce the
same unmistakable number.
CyclicBarrierCyclicBarrierattempts= 12attempts= 1New suites:
code.concurrency.ConcurrentSchedulerLockRaceTestandcode.concurrency.ConcurrentMessageOutboxRelayRaceTest, both built on the existingConcurrentRaceSetup.runConcurrentWithBarrierfan-out helper so contenders actually enter thecritical section together rather than being serialised by scheduling luck. They live in the
code.concurrencypackage, which runs in CI shard 8 and in the local runner's shard-4 catch-all.Test run
mvn -o test-compileclean. Full local./run_tests_parallel.sh(4 shards + the prop-gated JVM):3832 tests, 0 failures, 0 errors, 18 skipped/canceled in 4m 58s. That is the 3826 of the
pre-change baseline plus exactly the 6 new scenarios, all of which ran and passed inside the full
run (shard 4, which owns the
code.concurrencycatch-all).Beyond the three defects: making the pieces separable
The commits after
20aec4601go further than the original card. They came out of a follow-upquestion — could a bank deploy only what it needs? — and the answer turned out to be yes, but
not along the axis that looks obvious.
What shipped
479f54b54instance.role— one image, three workloads:migrator/scheduler/web. Defaultallis exactly today's behaviour.d94921f0bapi_disabled_versionscould not switch off. Also: the gate took its routes by value, so a disabled version still built every ResourceDoc before being discarded — disabling a version saved no boot work at all.dc64e4937004e844a5CoreDependencyDirectionTestkeeps it that way. Plus a flaky test fixed at the root (fixed sleeps → bounded polling).3f1ee5a50obp-kernel: the dependency-free API vocabulary, in a module the compiler can police. Classes keep their package names, so none of the 176 files importingErrorMessageschanged.dd82fa355205da514d731aa96a571636868be192088e5lib/.Splitting by API version: measured, then dropped
obp-apiclasses, totalThe bridge cascade
v7→v6→…→v1.2.1is a compile-time object reference, so a build can only betrimmed from the newest end: wanting v7 means carrying everything down to v1.2.1. The one
available cut — "v1.2.1–v4 only" — saves 14%, and it is the version banks least want to drop.
Near-zero deployment value, so it is not in this PR.
The axis that does pay
Runtime code compilation.
DynamicUtilholds two Scala toolboxes, a GraalVM JS engine and aJSR-223 Java engine, and on JDK 24+ the SecurityManager sandbox it tries to install cannot be
created — the failure is caught and
doPrivilegeddegrades to a pass-through. For an instancethat does not use dynamic code, holding the relevant entitlement is effectively host RCE for no
benefit.
Worth knowing before touching this: "dynamic" is two subsystems and only one compiles. Dynamic
entity CRUD, projections, queries, the dynamic-endpoint Swagger proxy, method routing, endpoint
mappings and both validations — roughly 5,130 lines and the large majority of the 126 management
endpoints — never reach a compiler. Only about 15 endpoints do.
So the compiler now sits behind five seams, each with a default chosen for who is asking:
AbacAccountAccess(authorisation path) →Full(false). ABAC is consulted only after theordinary view checks have already refused, so it is a pure grant and "no engine" must mean
"grants nothing".
Full(true)would be a silent authorisation bypass on every account.AbacRules(operator-invoked management) →Failure(DynamicCodeExecutionDisabled). The oppositedefault on purpose: answering
Full(false)would tell an operator their rule evaluated to falsewhen nothing evaluated it.
DynamicCode→Left(DynamicCodeExecutionDisabled), byte for byte whatallow_user_generated_scala_code=falsealready produces.compileProblemsreturns a problemrather than an empty list, because empty means "it compiles".
CompiledEndpoints→ nothing. An endpoint that was never compiled does not exist.OptionalConnectors→internalis simply absent, failing asInvalidConnector.Verification
Every fix was checked by reverting it and watching the test go red, not by reading the diff:
attempts=12attempts=1Full(true)import code.abacrule.AbacRuleEnginere-added toNewStyleAnd end to end, because reasoning was not enough: a deployment rebuilt with the toolbox jars
deleted boots with no exception, binds its port, serves
GET /obp/v7.0.0/root→ 200, answers thedynamic endpoints 401 rather than 500, and drops
internalfrom/system/connectors.That probe earned its keep. The first run booted cleanly but still advertised
internal. Theargument that "the error fires while evaluating
InternalConnector.instance, soregisterneverruns" was wrong — that value is a CGLib proxy and does not force
DynamicUtil; the throw actuallycame one line later, from reading
isEnabledfor the log message, after the connector had beenregistered.
install()now touches the toolbox before installing anything.Full suite green at every step: 3832 → 3837 → 3841 → 3843 → 3847 → 3853 → 3854, 0 failures
throughout, each increase matching the tests added.