oscerd commented on code in PR #29098:
URL: https://github.com/apache/camel/pull/29098#discussion_r4466915721
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -1250,6 +1279,65 @@ private void statefulDeleteKeyState(Exchange exchange)
throws Exception {
*
* @throws IllegalStateException if the key has zero remaining signatures
*/
+ /**
+ * Claims a stateful signature key for this process before it is used.
+ * <p/>
+ * XMSS, XMSSMT, LMS and HSS are one-time-signature schemes, and a store
several processes read from - a shared
+ * directory, a vault - hands each of them the same key at the same index.
The in-process ordering of
+ * {@link #recordKeyUsageAfterSign(Exchange)} does nothing about that, so
the key is claimed from the
+ * {@link KeyLifecycleManager} the first time it is signed with, and held
until this producer stops.
+ */
+ private void claimStatefulKeyBeforeSign(Exchange exchange) {
Review Comment:
This was the most important one — thank you. Fixed in 963ccbd, taking the
"adopt it" branch of your suggestion rather than refusing, so a rolling update
completes instead of wedging.
After the claim is granted, `adoptStoredStatefulKeyIfAhead` re-reads the
stored key and replaces `keyPair` when the stored index is ahead. `keyPair` is
now `volatile`, since it is replaced while other threads read it.
The new `PQCStatefulKeyAdoptionTest` models the two instances as two
independently decoded copies of one LMS key — a PKCS#8 round trip gives
genuinely separate state, so signing with one leaves the other where it was, as
across two processes. Against the previous commit it fails:
```
expected: <1021> but was: <1023>
```
that is, the stored key goes *backwards* by two and those two indices get
handed out a second time. Exactly the failover the PR is about.
Worth noting my first attempt at this test passed against the broken code,
because `InMemoryKeyLifecycleManager` hands back the very object the producer
holds, so there was no divergence to detect. The independent-copies version is
what makes it meaningful.
_Claude Code on behalf of @oscerd_
##########
components/camel-pqc/src/main/java/org/apache/camel/component/pqc/PQCProducer.java:
##########
@@ -572,14 +577,37 @@ protected void doStop() throws Exception {
healthCheckRepository.removeHealthCheck(producerHealthCheck);
producerHealthCheck = null;
}
+ releaseClaimedStatefulKeys();
metrics.close();
metrics = PQCMetrics.NOOP;
super.doStop();
}
+ /**
+ * Gives back the stateful keys this producer claimed, so another process
can take them over once this one is
+ * stopped. A claim that cannot be released is logged rather than failing
the stop: the operating system releases it
+ * with the process anyway.
+ */
+ private void releaseClaimedStatefulKeys() {
+ KeyLifecycleManager klm = getConfiguration().getKeyLifecycleManager();
+ if (klm == null) {
+ claimedStatefulKeys.clear();
+ return;
+ }
+ for (String keyId : claimedStatefulKeys) {
Review Comment:
Agreed, and split out rather than fixed here: **CAMEL-26588**.
A per-key reference count is the right shape, but it changes the ownership
model of the claim, and this PR is already carrying the lock-release fix, the
atomicity fix and the stored-index re-read. Keeping it separate leaves this one
reviewable against what you already read.
_Claude Code on behalf of @oscerd_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
