Skip to content

[#885] Give a connection of the JDBC pool a read bound of its own, and take it off for a statement that carries none - #934

Merged
vharseko merged 2 commits into
OpenIdentityPlatform:masterfrom
maximthomas:issues/885-jdbc-read-timeout
Sep 9, 2026
Merged

[#885] Give a connection of the JDBC pool a read bound of its own, and take it off for a statement that carries none#934
vharseko merged 2 commits into
OpenIdentityPlatform:masterfrom
maximthomas:issues/885-jdbc-read-timeout

Conversation

@maximthomas

Copy link
Copy Markdown
Contributor

Part of #885 — its section 2, and it closes section 3 with it. The DDL lock bound of section 1 is a PR of its own; nothing here depends on it.

Problem

Two bounds already stand on a connection of this pool, and between them there is a gap.

Outside those two windows a connection whose peer has gone quiet is read with no deadline of any kind: commit(), rollback(), the DatabaseMetaData lookups of isExistsTable() / isExistsIndex(), the rows a cursor drains, and every statement of a class carrying no bound. An operation that meets a database which stops answering after the login stays parked with nothing able to reach it, and there is no setting a deployment can ask for one with. That is section 3 of the issue as well as section 2, which is why section 3 needs no work of its own.

Change

org.openidentityplatform.opendj.jdbc.read.timeout, in seconds, default 0 — the behaviour of every connection this pool established before it existed.

The default is a consequence, not caution. A bound standing on the connection has to exceed the longest silence the database may legitimately produce; the longest statements belong to StatementBound.BULK, which ships unbounded for reasons of its own. There is no non-zero value here that does not contradict bulk.timeout=0. So the deployment that knows how long its database may go without answering is the one that sets this — and the statements of that class run with it taken off for as long as they run.

It is applied at the login, in the same call that takes the login's bound off. relaxReadBound() becomes applyStandingReadBound(), which sets read.timeout in milliseconds where the login's bound used to be replaced with 0. It is called unconditionally, not only where a login bound was set: with connect.timeout=0 the login takes its bound from what is left of the deadline of the borrow instead, and a deployment running that way would otherwise set the property and get nothing for it.

A read bound standing in the connection string is the deployment's own and is touched by no part of this — it is not replaced at the login, and it is not the one taken off for a bulk statement. But a socketTimeout=0 in a URL is not a bound of theirs: it is the default of the driver written out. That distinction is why this asks bounds() rather than the existing declared(), which answers a different question — whether a property of ours is to be supplied to the connect at all, where on PostgreSQL a URL parameter outranks a system property whatever it says. A bound put on an established connection with setNetworkTimeout is outranked by nothing.

A misconfiguration is reported, never read as a zero. A value that is not a number, and a negative one, are said once and ignored in favour of the default: a deployment that asked for a bound and misspelled it getting none quietly is the one thing this property exists to prevent. A value past MAX_BOUND_SECONDS is taken down to it — that ceiling is what a socket read timeout can hold at all, and a value beyond it reaches the driver as a negative timeout, outside the contract of the call.

The value is read once, at class initialization, the way aliveBypassNanos is, and clamped against the inlined MAX_BOUND_SECONDS constant rather than through JDBCStorage.clampSeconds(): this runs in the initializer of CachedConnection, and a package-private call into another class across two loaders is an IllegalAccessError rather than a call — which would leave the class uninitializable.

A bulk statement lifts it, and puts it back. With a standing bound in force a statement of a class that carries none would die at it, which is the opposite of what bulk.timeout=0 promises. applyBackstop() — which since #882 only ever tightens — gains the other direction:

  • while a statement of an unbounded class is running and the connection carries a bound this backend gave it, that bound comes off for as long as the statement runs;
  • only ours is lifted. A read timeout standing in the connection string is the deployment's own, and lifting it would hand the connection back to the pool with the one bound its URL asked for gone. CachedConnection.standingReadBoundMillis(url) answers with the same predicate applyStandingReadBound() used, so the bound taken off is the bound that was set;
  • what the connection carried is read once and remembered across the lift. Read again while the lift holds it would be the 0 of that lift, and the bound would go back to the pool gone for the rest of the connection's life — which is how an unbounded statement outliving a bounded one on the same connection takes a deployment's bound away for good. A new Backstop.lifted flag keeps armed == 0 meaning what it meant: the connection carries its own value.

JDBCStorage reads the standing bound once per configuration rather than per statement — it follows a system property and the URL of the backend, neither of which changes under a running statement — and reads it again after applyConfigurationChange, since the URL may be another one now.

The two bounds set the wrong way round are said once. A standing bound at or under the bound of an ordinary statement (StatementBound.OPERATION, two minutes by default) means such a statement dies on the socket instead of being cancelled at its own bound — which costs the connection the driver then closes, and names neither property, because timedOut() weighs the statement against its own bound, finds it well inside, and passes the failure through as it found it. The socket read timeout stands behind the cancel of a statement, so it has to be the longer of the two.

That silence is also what the value should be sized against rather than the longest statement, because the lift covers statements alone: the commit at the end of an import is a call of its own with no statement in flight behind it — ImporterImpl.close() commits a whole import in one — and so are the rollback of every borrow and the reads of the catalog. Every one of those runs under this bound whatever the class of the statements before it.

Poolability is unchanged in substance. A connection still carrying the bound of its login must not be pooled — it would fail the statements of every borrower after this one — and that is what the return of applyStandingReadBound() still says. A connection that merely never took the standing bound may be pooled: that is exactly the connection this pool handed out before the property existed.

Testing

New cases in CachedConnectionTestCase: testAnEstablishedConnectionCarriesTheReadBoundAskedFor, testTheReadBoundIsSetWhereTheLoginHadNoneToLift, testAReadBoundOfTheUrlIsNotReplacedByTheConfiguredOne, testOnlyTheReadBoundThisClassSetsIsItsOwnToLift, testTheReadBoundIsConfiguredInSeconds, testAConnectionThatWouldNotTakeTheStandingBoundIsStillPooled, testAConnectionThatWouldTakeNeitherTheStandingBoundNorTheLiftIsNotPooled, testAReadParameterOfTheUrlSetToZeroIsNoBoundOfTheDeployments, testTheDefaultTakesTheBoundOfTheLoginOffAndPutsNothingOnTopOfIt, and testAReadBoundWorthWarningAboutStillInitializesTheClass — the last one against the <clinit> trap above, where a warning reached from a class initializer turns a misconfigured property into ExceptionInInitializerError.

New cases in JDBCStatementBoundTestCase: testABulkStatementTakesTheStandingReadBoundOffTheConnection, testTheStandingReadBoundOutlivesTheLiftAndComesBackAfterIt, testAReadBoundOfTheConnectionStringIsNotTakenOff, testAStandingReadBoundUnderTheBoundOfAStatementCutsItShort, testTheStandingReadBoundIsReadAgainAfterAConfigurationChange.

mvn -o -pl opendj-server-legacy -am -Pprecommit \
    -Dit.test='CachedConnectionTestCase,JDBCStatementBoundTestCase,JDBCStorageRetryTest,StampConnectionTestCase' \
    -Dfailsafe.failIfNoSpecifiedTests=false -Dmaven.javadoc.skip=true verify

@maximthomas
maximthomas requested a review from vharseko September 7, 2026 09:15
…bound of its own, and take it off for a statement that carries none

The bound of jdbc.connect.timeout covers the connect and the login and is
taken off as soon as the login is through; the socket read timeout behind a
cancelled statement is armed for the length of that statement alone. Between
the two, a commit, a rollback, a lookup of the catalog and the rows a cursor
drains are read from a socket with no deadline of any kind, so an operation
that meets a database which stops answering after the login stays parked with
nothing able to reach it.

jdbc.read.timeout is what a deployment asks for one with. It is set on the
connection once the login is through, in the same call that takes the read
bound of the login off, and it defaults to 0 - the behaviour of every
connection this pool established before it existed. The default is a
consequence rather than caution: a bound standing on the connection has to
exceed the longest silence the database may legitimately produce, and the
class those statements belong to ships unbounded.

So the statements that legitimately take longer run with it taken off: the
backstop of JDBCStorage lifts it for as long as a statement of an unbounded
class runs, and puts it back when the last of them is through. It lifts only
what this backend set - a read timeout standing in the connection string is
the deployment's own, and it is neither replaced at the login nor lifted here.
What the connection carried is read once and remembered across the lift; read
again while the lift holds, it would be the zero of that lift, and the bound
would go back to the pool gone for the rest of the connection's life.
@maximthomas
maximthomas force-pushed the issues/885-jdbc-read-timeout branch from 5edae4d to 7b26223 Compare September 7, 2026 09:37

@vharseko vharseko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mechanism reads well and the lift is covered by the tests, but a few things below need changing before this goes in. Line numbers are of the head of this branch.

Blocking

1. JDBCStorage.java:136 — the standing bound is resolved against config.getDBDirectory() rather than poolKey()

poolKey() exists precisely because a db-directory changed on a running backend must not send one path to another pool, and its javadoc says so: "Every path that names a pool goes through here." This one does not.

open() registers jdbc:mysql://db1/x (no read parameter), so every pooled connection carries the 90 s standing bound. db-directory is then changed to jdbc:mysql://db2/x?socketTimeout=600; applyConfigurationChange resets the memo, line 136 reads the new url, bounds() is true and the answer is 0. getConnection() still borrows from the db1 pool, whose connections still carry 90 s — but the lift gate at line 627 is now false, so a BULK statement dies on the socket at 90 s, which is what bulk.timeout=0 promises it will not. The reverse pairing (registered url declares socketTimeout=600, the new one does not) makes the gate true and lifts a bound that is the deployment's own.

2. JDBCStorage.java:627 — the "two bounds the wrong way round" warning is never evaluated at open

Line 627 is the only production call site of standingReadBoundMillis(), and it sits behind state.unbounded > 0. A read-only backend — searches and reads, no import, no clearTree, no create index — runs no statement of an unbounded class, so the memo is never resolved and reportABoundNoStatementCanOutlive never runs.

With read.timeout=60 every ordinary statement of that backend is cut by the socket at 60 s instead of being cancelled at OPERATION's 120 s: the driver closes the connection, timedOut() finds the statement well inside its own bound and passes the failure through, and nothing in the log names either property. The javadoc of READ_TIMEOUT_PROPERTY promises the opposite — "A backend opening with the two set that way says so once." Resolving the memo once in open() would make good on it.

3. JDBCStorage.java:161 — the post-import statistics refresh is neither lifted nor weighed

reportABoundNoStatementCanOutlive weighs only StatementBound.OPERATION, so read.timeout=300 passes it in silence. updateTableStatistics then runs analyze table ... through bounded(con, STATISTICS_TIMEOUT_PROPERTY, 600, cancelArmed=true, ...) — a bounded statement, so applyBackstop takes the tighten path, finds previous=300000 <= wanted=backstopMillis(600)=630000 and leaves the connection carrying 300 s. A ten-minute analyze — the case the javadoc calls one that "legitimately takes as long as a scan of the table it describes" — is cut at 300 s, the driver closes the importer's connection, and since armedMillis is 0 and the elapsed time is well under the 600 s property, timedOut() returns a bare class-08 error naming no property. The statistics of #859 are then silently never refreshed. Any statement carrying a bound of its own longer than OPERATION is in this hole, not only this one.

4. JDBCStorage.java:627 — a configuration change re-arms the bound under a running lift

A BULK statement is running with lifted=true and previous=90000, the connection carrying 0. db-directory changes to a url that declares its own socketTimeout, so the memo is reset. A second BULK statement starts on the same connection: standingReadBoundMillis() is now 0, the state.unbounded > 0 && ... > 0 gate is false, and control falls to giveBack(), which writes 90000 back while two unbounded statements are still in flight. Both then die at 90 s — the failure the lift exists to prevent — and neither has a property to name.

Worth fixing

5. CachedConnection.java:198read.timeout is silently ignored outside the four dialects

ConnectDialect.of() answers null for mariadb, h2, db2 and the rest, so standingReadBoundMillis returns 0, applyStandingReadBound takes its millis == 0 && !readBoundSet early return and no setNetworkTimeout is issued: every connection of that pool stays unbounded, which is the state the property exists to fix. Being conservative there is defensible — such a url may bound the read under a name this class does not know, and ours would sit on top of it — but the silence is not, given that the strict parsing of the value exists so that "a deployment that asked for a bound and misspelled it" is not left with none quietly. reportUnknownDialect speaks only of the connect bound and pool.timeout; a word about this property there would close it.

6. JDBCStorage.java:150cutsStatementsShort weighs the wrong value for a statement carrying no cancel

The catalog lookups of openTree() run with cancelArmed=false, so the socket read timeout is their only layer and what the backstop arms for them is backstopMillis(120)=150000, not 120000. With read.timeout=140: cutsStatementsShort(140000, 120) is false, so nothing is reported, and applyBackstop finds previous=140000 <= 150000, takes the tighten-only early return and leaves state.armed at 0. When the catalog blocks behind the same lock as the create table it guards, the lookup dies at 140 s, armedMillis() is 0, endsAfterMillis is 0 and timedOut() returns the driver's raw class-08 exception naming neither property. Before this PR previous was 0, the backstop armed 150000 and the same failure was reported as BACKSTOP_ALONE plus the OPERATION property — so this is a step back in what the operator is told. For statements that carry no cancel the threshold has to be backstopMillis(statementSeconds).

7. JDBCStorage.java:133 — the memo and its reset race

Thread A reads standingReadBound == -1 and computes the answer against the old config. applyConfigurationChange then writes this.config = cfg and standingReadBound = -1. Thread A resumes and publishes the pre-change answer, so the reset is lost and the storage answers for the old url for the rest of its life — the very case the reset was added for. A generation counter compared on publish, or recomputing under the lock, makes it reliable.

Minor

8. CachedConnection.java:1003bounds() and declared() can disagree about one url. valueInUrl() stops at the first name present even when its value is 0 and never falls back to the last-segment alias, while declaredInUrl() tests both names independently. ...?oracle.jdbc.ReadTimeout=0&ReadTimeout=600 is declared() but not bounds(), so the login keeps the administrator's 600 s and then applyStandingReadBound puts ours on top of it with setNetworkTimeout. Contrived, but the two want to look at the same set of names for "the bound taken off is the bound that was set" to hold.

9. JDBCStorage.java:520lifted patches an overload in armed instead of removing it. armed == 0 now means both "nothing of ours is on this connection" and "our lift is on it", told apart by a boolean beside it, so state.armed == 0 && !state.lifted is repeated verbatim in giveBack(), restorePrevious() and both getNetworkTimeout() reads, plus the state.lifted=false that has to be remembered on the arm path. One Integer applied — null for a connection this backstop has not touched, 0 for a lift, the value for an arm — collapses all of them to applied == null and makes the invariant unstateable by mistake.

10. CachedConnection.java:993 — the block comment of declared() now sits above bounds(). The // paragraph ending "...leaving a connection with no read bound where the administrator had set one" was written for declared(); bounds() and its javadoc are now spliced between them. In a file where these comments are the record of why each predicate answers as it does — and where the two differ precisely in how they read a 0 — it is worth moving down to the method it describes.

…d bound

The bound is resolved against poolKey() rather than against the configuration
as it stands. Every path that names a pool goes through poolKey() because
db-directory may be changed under a running backend, and the connections whose
bound this decides are the ones of the pool open() registered with: read off
the url config names now, the lift would be gated by the answer of a pool this
storage never borrows from - leaving the bound of this backend standing on a
bulk statement, which bulk.timeout=0 promises will not happen, or taking a
bound of the deployment's own off a connection that came with one.

It is resolved once, while the backend opens. applyBackstop() is the only
caller in production and it asks only behind a statement of a class carrying
no bound of its own, so a deployment that gives bulk.timeout a value of its
own has no such statement anywhere and would never be told that its two bounds
are set the wrong way round. A configuration change no longer resets the memo -
poolKey() does not move under one, and a reset would resolve it again under a
lift in flight, handing the bound back to the statements it was taken off for.
What does move poolKey() is a close and a re-open, which is where it is reset.

What the bound is weighed against is the loosest bound a statement of this
backend carries, not the bound of an ordinary one: the statistics refresh after
an import has a property of its own, ten minutes by default, and a bulk.timeout
a deployment sets takes that class out of the lift and into the same weighing.
And it is weighed against the socket layer of that bound rather than against
its cancel, because the cancel is not always there to come first: the catalog
lookups of openTree() ask DatabaseMetaData, which takes no query timeout at
all. A standing bound between the two ends such a statement with the backstop
arming nothing on top of it, so the failure names neither property.

An unknown dialect now reports that it takes no standing read bound either,
where one was asked for. bounds() reads the property name and its last-segment
alias independently, the way declared() does, so a url naming the bound under
both - one turned off, the other set - is the deployment's own under both
predicates. Backstop.armed and Backstop.lifted are one Integer applied: null
for a connection this backstop has not touched, 0 for a lift, the value for an
arm.
@maximthomas

Copy link
Copy Markdown
Contributor Author

Addressed in 439089723c. All ten are in code. Three of them are not the fix the review proposes,
and one repro does not hold — those are called out where they come.


1. The standing bound is resolved against config.getDBDirectory() rather than poolKey() — fixed

Confirmed, and both pairings you trace hold. standingReadBoundMillis() now asks poolKey(), so the
answer is the one of the pool open() registered with — the pool getConnection() actually borrows
from — for as long as the storage is open.

The one place poolKey() really moves is a close and a re-open on a changed db-directory, so the memo
is reset in releasePool() and on the failed-open path beside it, where poolConnectionString is nulled
and nothing of this storage is in flight.

2. The "two bounds the wrong way round" warning is never evaluated at open — fixed, but the repro is not the read-only backend

The conclusion and the remedy are yours: the memo is resolved in open(), right after the pool is
registered, so every backend that opens weighs the two.

The repro is not, though. ID2Entry.afterOpen() opens a bulk cursor on every open of every backend —
its comment says so outright, "Make sure the tree is there and readable, even if the storage is
READ_ONLY"
, and the note under it explains why that walk is openBulkCursor. That reaches
holdBackstop(con, 0), state.unbounded goes to 1, and line 627 resolves the memo. A read-only backend
does get the warning today.

What really leaves it unresolved is bulk.timeout set to anything non-zero: StatementBound.BULK.seconds()
is then > 0, so nothing in this backend ever increments state.unbounded, line 627 is unreachable for the
life of the storage, and the memo is never read. That is also the configuration where the warning matters
most — a bounded bulk class gets no lift either, so the standing bound is free to cut a bulk statement
short. testTheStandingReadBoundIsResolvedWhileTheBackendOpens sets bulk.timeout=3600 for exactly that
reason.

3. A statement carrying a bound of its own longer than OPERATION is neither lifted nor weighed — fixed by weighing it, not by lifting it

Confirmed, arithmetic included: previous=300000 <= backstopMillis(600)=630000, the tighten-only early
return, the ten-minute analyze cut at 300 s, armedMillis() 0, a bare class-08 naming nothing, and the
statistics of #859 silently never refreshed.

The fix is loosestStatementBound(): the warning now weighs the standing bound against the loosest bound
any statement of this backend carries — statistics.timeout (600 s by default) and a bulk.timeout a
deployment set, not StatementBound.OPERATION alone — and names that property in the message.

Not by lifting or relaxing it for such a statement, which was the other candidate. The premise of the
property is that it has to exceed the longest silence the database may legitimately produce; a
read.timeout under statistics.timeout is a misconfiguration rather than a case to accommodate, and
having applyBackstop() quietly loosen the connection to fit would make the property mean "the bound
outside a statement" instead of what it says. Weighed at open, the operator raises one of the two and both
layers compose again.

4. A configuration change re-arms the bound under a running lift — fixed

Real as a mechanism, and it goes with the reset: applyConfigurationChange() no longer touches the memo.
With (1) in place there is nothing left for it to re-read — poolKey() does not move under a
db-directory change — and the case that does move it is handled in (1), at a point where no statement of
this storage is running.

5. read.timeout is silently ignored outside the four dialects — fixed

reportUnknownDialect() now says it, and only where a bound was actually asked for
(readTimeoutMillis > 0): a deployment running on the default asked for no standing bound and has nothing
to act on. The conservative behaviour itself stands, for the reason you give — such a url may carry a read
bound under a name this class does not know, and ours would sit on top of it.

6. cutsStatementsShort weighs the cancel, not the layer that ends a statement carrying none — fixed

Confirmed, and the threshold is now backstopMillis(statementSeconds) as you propose, so
cutsStatementsShort(140000, 120) is true.

That also closes the step back in reporting, indirectly rather than by touching armedMillis(). After the
threshold change the two cases are exhaustive:

  • standing > backstopMillis(loosest) — then previous > wanted for every statement class, the backstop
    arms wanted, and timedOut() reports BACKSTOP_ALONE plus the property exactly as before this branch;
  • standing <= backstopMillis(loosest) — the tighten-only return can swallow the report, and that is
    precisely the configuration the open now warns about, by name and by value.

A connection carrying a read bound of its url is still in the hole you describe — armedMillis() is 0
there too — but that one is pre-existing and untouched by this branch: standingReadBoundMillis() answers
0 for such a url, so nothing here put that bound on or takes it off.

7. The memo and its reset race — fixed

With (4) the only write after open() is the reset in releasePool(), and open() resolves the value
once before any statement of the storage can run. There is no reset left for a concurrent resolve to lose.

8. bounds() and declared() can disagree about one url — fixed

valueInUrl() is gone. bounds() asks boundInUrl(), which tests the property name and its last-segment
alias independently, the way declaredInUrl() does. Your url is the test case:
?oracle.jdbc.ReadTimeout=0&ReadTimeout=600 now answers bounds(), so the administrator's 600 s keeps the
connection and nothing of ours goes on top of it.

9. lifted patches an overload in armed instead of removing it — fixed

armed and lifted are one Integer applied: null for a connection this backstop has not touched, 0
for a lift, the value for an arm. The four repetitions of armed == 0 && !lifted are one applied == null,
and the lifted=false that had to be remembered on the arm path is gone with them.

10. The block comment of declared() sits above bounds() — fixed

Moved down to declared(), which is what it was written for. bounds() keeps its own javadoc, with a line
pointing at that comment for how a parameter of each dialect is recognized and which names a driver also
reads out of the system properties.


Testing

JDBCStatementBoundTestCase: testAStandingReadBoundUnderTheBoundOfAStatementCutsItShort extended to the
new threshold, testTheStandingReadBoundIsReadAgainAfterAConfigurationChange replaced by
testTheStandingReadBoundFollowsTheUrlThePoolWasRegisteredWith, plus
testTheStandingReadBoundIsResolvedWhileTheBackendOpens and
testTheLoosestBoundOfAStatementIsWhatAStandingBoundHasToOutlive.

CachedConnectionTestCase: testAReadBoundUnderEitherNameOfTheUrlIsTheDeploymentsOwn.

mvn -o -pl opendj-server-legacy -am -Pprecommit \
    -Dit.test='CachedConnectionTestCase,JDBCStatementBoundTestCase,JDBCStorageRetryTest,StampConnectionTestCase' \
    -Dfailsafe.failIfNoSpecifiedTests=false -Dmaven.javadoc.skip=true verify

Item 5 is a message change only: this suite has no log capture, so its wording is not pinned by a test.

@vharseko vharseko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. 439089723c closes all ten items, and I checked each against the head rather than against the
answer.

Closed

The four blocking ones.

  • 1standingReadBoundMillis() asks poolKey(), so the answer is the one of the pool open() registered
    with for as long as the storage is open, and the memo is reset where that key really moves: releasePool()
    and the failed-claim path beside it.
  • 2 — resolved in open(), right after openPool() returns, so the warning is weighed by every backend
    that opens rather than by the first statement of an unbounded class.
  • 3loosestStatementBound(). I checked the other side of it too: holdBackstop counts a
    seconds <= 0 statement as unbounded, so a statistics.timeout of 0 goes under the lift rather than under
    the weighing, and leaving it out of the loosest bound is right rather than an omission.
  • 4applyConfigurationChange no longer touches the memo, and the comment where the reset used to be
    says what re-reading it under a lift in flight would do.

The six smaller ones. reportUnknownDialect says it, and only where a bound was asked for; the threshold
is backstopMillis(statementSeconds); the race is gone with item 4; boundInUrl() asks both names
independently the way declaredInUrl does; the comment is back above declared().

Item 9 in particular. I read the Integer applied refactor state by state against the pair it replaces —
null for armed == 0 && !lifted, 0 for the lift, the value for an arm — and every site maps exactly,
armedMillis() included. state.applied != wanted compares numerically rather than by reference, since one
operand is a primitive, and state.applied == null || short-circuits in front of the unboxing.

And the step back in reporting that item 6 attached to the threshold does close the way you argue it. With
standing > backstopMillis(loosest), previous > wanted holds for every class, so the backstop arms and
timedOut() names the property as it did before this branch; with standing <= backstopMillis(loosest) the
tighten-only return can swallow it, and that is exactly the configuration the open now warns about, by name and
by value.

Item 2, where the correction is yours

My repro was wrong and yours is right: ID2Entry.afterOpen() opens a bulk cursor on every open of every
backend, with the comment saying "Make sure the tree is there and readable, even if the storage is
READ_ONLY"
, so state.unbounded does reach 1 there and a read-only backend was already being told. What
really left the memo unresolved is a bulk.timeout set to anything non-zero — no statement of an unbounded
class anywhere — which is what testTheStandingReadBoundIsResolvedWhileTheBackendOpens sets.

Two things, neither blocking

1. The javadoc of the property still names the old threshold — CachedConnection.java:138-142

"It has to exceed the bound of an ordinary statement as well (JDBCStorage.StatementBound.OPERATION, two
minutes by default
): under it, such a statement dies on the socket at this value instead of being cancelled
at its own"

The threshold the code applies now is backstopMillis(loosestStatementBound()) — 600 s of
statistics.timeout plus the 30 s margin, so 630 s at the defaults. Items 3 and 6 landed in JDBCStorage, and
this paragraph is where a deployment reads about the property before setting it, five times below the value
that actually warns: someone following it sets read.timeout=180 and is told at the next open that the two
bounds are the wrong way round.

Worth saying in substance as well as in arithmetic: at the defaults the warning fires for every value under
about ten and a half minutes, which is most of the range this property is useful in. The warning itself carries
the other half of the remedy — it names loosest.property and its seconds, so the operator can see that
lowering statistics.timeout is the other way out — but the javadoc, which is read first, does not mention the
interaction at all.

2. The description is a round behind, in the same two places and in the test list

Squash-merge lands it in the history:

  • "A standing bound at or under the bound of an ordinary statement (StatementBound.OPERATION, two minutes by
    default)"
    — the same understated threshold as above.
  • "JDBCStorage reads the standing bound once per configuration [...] and reads it again after
    applyConfigurationChange
    , since the URL may be another one now"
    — now the opposite, and deliberately so:
    it is resolved once in open() against poolKey() and is not re-read under a configuration change. The
    description states the behaviour items 1 and 4 exist to remove.
  • The test list names testTheStandingReadBoundIsReadAgainAfterAConfigurationChange, which is deleted, and
    omits testAReadBoundUnderEitherNameOfTheUrlIsTheDeploymentsOwn,
    testTheLoosestBoundOfAStatementIsWhatAStandingBoundHasToOutlive,
    testTheStandingReadBoundFollowsTheUrlThePoolWasRegisteredWith and
    testTheStandingReadBoundIsResolvedWhileTheBackendOpens.

Three things I checked and am not raising, since they hold: the new cases vary system properties and a static
of the pool, and @AfterMethod puts back every StatementBound property, statistics.timeout,
read.timeout, CachedConnection.readTimeoutMillis and the access mode; poolConnectionString cannot be left
null under !claimedHere, since poolRegistered is this storage's own flag and that branch is a second
open() on a string already set; and openedOn() registers a real pool entry and gives it back through
close().

Reviewed against pull/934/head (439089723c) vs master; line numbers are that head's. CI shows no failure,
with build-maven (ubuntu-latest, 17) still running. No database and no benchmark — the thresholds are read
off BACKSTOP_MARGIN_SECONDS and the property defaults, not measured.

@vharseko vharseko removed the java label Sep 9, 2026
@vharseko
vharseko merged commit 2a7bb9d into OpenIdentityPlatform:master Sep 9, 2026
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug jdbc tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants