Fix remaining audit issues: async credential issuance, complete Javadoc, 80% coverage gate#114
Merged
Merged
Conversation
…hread (#109) acceptJob ran the provisioner's CompletableFuture to completion with join() inside onNext, so a slow upstream key-minting call starved every inbound message on the session: pings went unanswered, heartbeat.onInbound never advanced, and a stall beyond 2x heartbeat_interval_sec had the session reaped as HEARTBEAT_LOST mid-submit. One slow job.submit also head-of-line-blocked cancels and acks for every other job. Acceptance is now split: the prologue (resolve, register) stays on the dispatch thread; the epilogue (attach credentials, job.accepted, worker start, watchdogs) runs when issuance completes, chained through a per-session acceptSequence so job.accepted preserves submit order — the FIFO correlation clients rely on, which is why this was deferred in #112. Idempotent replays join the same chain so a replayed acceptance cannot overtake the original and always observes the captured budget (#79). If the session dies or the job is cancelled while issuance is in flight, freshly minted credentials are revoked and nothing is announced; the lease-expiry watchdog measures its delay from the current clock so issuance latency cannot postpone an absolute expires_at. AsyncAcceptanceTest covers both acceptance criteria: pongs continue through a >2x-interval provisioner stall, and a slow first issuance does not let a later acceptance overtake it. Closes #109 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Closes #81 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Document every public/protected type, constructor, method, record
component, and enum constant across the ten published modules
(arcp-core, arcp-client, arcp-runtime, arcp, arcp-otel,
arcp-runtime-jetty, arcp-middleware-jakarta,
arcp-middleware-spring-boot, arcp-middleware-vertx, arcp-tck):
~575 doclint findings fixed, including two broken {@link} references.
Enforce it in the root pom: maven-javadoc-plugin now runs with
doclint `all` (including `missing`) and failOnWarnings=true, so the
CI javadoc job fails on any undocumented exported API. The gate is
overridable for local triage via -Darcp.javadoc.failOnWarnings=false.
A few doclint "use of default constructor" warnings required adding
explicit, documented no-arg constructors with semantics identical to
the implicit ones; no other code changed.
Closes #34
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ure branches (#33) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…#33) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sport, and core wire paths (#33) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… coverage (#33) arcp-otel was the last module under the bar (66.7% branch): the new ArcpOtelBranchTest covers send-failure span recording, optional envelope attributes, trace-context inject (no-op propagator, merge into existing extensions, non-object extensions left untouched) and extract (malformed shapes fall back to Context.current, valid traceparent becomes the receive span's parent), publisher caching, close ordering, and upstream completion propagation. With every library module now >=80% on both counters (line 97.4%, branch 94.7% overall), jacoco:check enforces a 0.80 line+branch BUNDLE minimum at verify time. Library modules opt in via arcp.skip.coverage.check=false; examples/recipes and the sourceless arcp umbrella stay exempt, and check self-skips when tests were skipped (no execution data), so -DskipTests CI jobs are unaffected. Closes #33 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 178 files, which is 28 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (178)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
Author
|
Re the code-quality findings: 9 of 10 are in test-support fakes (JDK dynamic-proxy |
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.
Summary
Closes the four remaining open issues.
#109 — credential issuance blocked the session dispatch thread (fix)
acceptJobran the provisioner future to completion withjoin()insideonNext, so a slow upstream key-minting call starved every inbound message on the session — pings went unanswered and a stall > 2×heartbeat_interval_sechad the session reaped asHEARTBEAT_LOSTmid-submit. Acceptance is now split: the prologue (resolve, register) stays on the dispatch thread; the epilogue (attach credentials,job.accepted, worker start, watchdogs) runs when issuance completes, chained through a per-sessionacceptSequencesojob.acceptedpreserves submit order — the FIFO correlation clients rely on, which is why this was deferred in #112. Idempotent replays join the same chain (and so always observe the #79 captured budget); a session torn down mid-issuance revokes freshly minted credentials; the lease-expiry watchdog measures from the current clock so issuance latency cannot postpone an absoluteexpires_at.AsyncAcceptanceTestcovers both acceptance criteria.#81 — spec citations (docs)
§5.1→§5,§4.1/§4.2→§4in ArcpMapper, Envelope, StdioTransport, WebSocketJsonTransport, and the TCK label; every other§N.Ncitation in src/main audited againstdraft-arcp-1.1.md(all exist; the§1.9in ReplayingPublisher cites Reactive Streams, left as-is).#34 — complete Javadoc + doclint enforcement (docs)
123 src/main files across all ten published modules documented (~575 doclint findings). maven-javadoc-plugin now runs doclint
all(includingmissing) withfailOnWarnings(overridable via-Darcp.javadoc.failOnWarnings=false), so the CI javadoc job fails on undocumented exported API.#33 — coverage ≥80% line+branch, enforced (testing)
The issue's numbers were stale (Gradle-era). Fresh baseline on main was 80.8% line / 62.0% branch. Added ~290 deterministic tests (no sleeps; latches, manual scheduler, mutable clock, hand-rolled fakes — Mockito can't instrument on JDK 25):
jacoco:checknow enforces a 0.80 line+branch BUNDLE minimum at verify time in every library module (examples/recipes and the sourcelessarcpumbrella exempt viaarcp.skip.coverage.check; check self-skips for-DskipTestsjobs). Verified: fullmvn verifygreen, and a 0.99 negative test fails the build as expected.Drive-by: #113 filed for a latent
WebSocketTransport.connecttimeout hang found while writing coverage tests (not fixed here to keep this PR's runtime changes scoped to #109).Closes #109
Closes #81
Closes #34
Closes #33
Test plan
mvn verify(JDK 25, spotless skipped) — all modules, coverage gate activemvn spotless:check(JDK 21)🤖 Generated with Claude Code