Skip to content

fix(#8077): the embedded HAServerPlugin.connectCluster API reports the cluster security seed - #8845

Open
robfrank wants to merge 4 commits into
mainfrom
fix/8077-ha-connectcluster-reports-seed-failure
Open

robfrank wants to merge 4 commits into
mainfrom
fix/8077-ha-connectcluster-reports-seed-failure

Conversation

@robfrank

@robfrank robfrank commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #8077

The embedded HAServerPlugin.connectCluster(String) API joined a server with no way for the caller to learn that the leader's security seed left a document uncommitted. This adds HAServerPlugin.connectClusterAndReportSeed(String) (the connect cluster counterpart of #7820's addPeerAndReportSeed) and moves the report down into the plugin, as the issue proposed: RaftHAPlugin overrides it to run the join and then ask the leader for the seed outcome (same seedReportForAdmission as addPeerAndReportSeed, so it never throws after the join and reports an unknown outcome as all three documents). ServerControlPlane.connectCluster now calls the new method and uses its report, so both wire transports still make exactly one seed request per connect cluster (#7834).

The interface default joins through connectCluster and returns an empty Optional, which means "this implementation leaves the seed to its caller", the same convention seedSecurityStateForAdmission uses. In that case ServerControlPlane runs the seed it always ran (leader-side via seedSecurityStateForAdmission, or locally), so an HA implementation written before this change behaves exactly as before. The void connectCluster stays the membership change alone.

Completeness

Invariant: every admission entry point that changes membership can hand its caller the residual seed failure, with one seed request per admission.

$ grep -rn "\.connectCluster(\|connectClusterAndReportSeed(" --include='*.java' server/src/main/java ha-raft/src/main/java grpcw/src/main/java
server/.../ServerControlPlane.java:  pluginReport = ha.connectClusterAndReportSeed(serverAddress);
server/.../http/handler/PostServerCommandHandler.java:  controlPlane.connectCluster(serverAddress);
grpcw/.../ArcadeDbGrpcAdminService.java:  controlPlane.connectCluster(req.getServerAddress());
ha-raft/.../RaftHAPlugin.java:  connectCluster(serverAddress);   (inside connectClusterAndReportSeed)
Entry point Seed report to caller Seed requests Status
HTTP connect cluster (PostServerCommandHandler -> ServerControlPlane) yes, 503 + failedSeeds 1 (plugin's) fixed here (now consumes plugin report)
gRPC ConnectCluster (ArcadeDbGrpcAdminService -> ServerControlPlane) yes, UNAVAILABLE 1 (plugin's) fixed here (same path)
embedded HAServerPlugin.connectClusterAndReportSeed (Raft) yes 1 fixed here
embedded HAServerPlugin.connectCluster (void) no, by signature 0 from this node (the leader seeds on its own, #7531) argued: membership change alone, javadoc points to the reporting form
non-Raft plugin keeping the default via ServerControlPlane fallback, as before 1 unchanged, pinned by Issue7532ConnectClusterReportsSeedFailureTest

Coverage

  • RaftHAPlugin.connectClusterAndReportSeed: Issue8077EmbeddedConnectClusterSeedReportTest (report, clean join, ordering + single seed request, failed join not seeded, seed IOException / unchecked failure never fails the join).
  • ServerControlPlane.connectCluster consuming the report: Issue8077ConnectClusterConsumesPluginSeedReportTest (plugin report used, no second seed either leader-side or local; default plugin still gets the leader seed; interface default contract; no-runtime-membership still a precondition refusal).

Known gaps: None.

Test plan

  • mvn test -pl server,ha-raft -Dtest='Issue8077*Test,Issue7532ConnectClusterReportsSeedFailureTest,Issue7401ServerControlPlaneConnectClusterTest,Issue7834SeedStaysUnconditionalTest,Issue7521SecuritySeedRetryTest,Issue7820EmbeddedAddPeerSeedReportTest,Issue7515SelfJoinRefusedTest,Issue4837DoubleLeaveTest,Issue7559SecurityPreconditionOnFollowerTest' - 75 tests green
  • mvn verify -pl ha-raft,server -DskipITs=false -Dit.test='Issue7401ConnectClusterJoinsPeerIT,Issue7532ConnectClusterHttpSeedFailureIT,Issue7400ConnectClusterHttpIT,Issue7514UnreachablePeerFailsFastIT' - 10 ITs green (live Raft join through ServerControlPlane -> connectClusterAndReportSeed)

Adversarial pass

No subagent tool was available in this run, so the pass was done as a self-review against the issue body:

  • "Two seed requests per connect cluster" (the HA: an admission now seeds the security documents twice, from two nodes, under two different monitors #7834 hazard the issue names): not real - ServerControlPlane only falls back to its own seed when the plugin returns an empty Optional, which Raft never does; asserted by aPluginThatReportsItsOwnSeedIsNotSeededAgain.
  • "The UnsupportedOperationException arm now also covers the seed": not real - Raft's seedReportForAdmission catches every RuntimeException, so a UOE can only come from the join; asserted by anUncheckedFailureWhileSeedingStillDoesNotFailTheJoin.

Review cycles

Cycle Head Changes claude review
1 38e94fd initial fix + tests no blockers; 4 suggestions
2 6867a8d javadoc: an override must not throw after the join; test with the real connectCluster failing (no seed); test that a reporting plugin's UOE still maps to OperationNotAvailableException; field alignment no blockers; 4 suggestions
3 1e48e1e control-plane comment points at the plugin report; if/else instead of the ternary; javadoc says the report is returned, not logged LGTM pending a latency note
4 4e00cd7 javadoc documents the seed-report wait no blockers; 5 suggestions (not applied, max cycles reached)

Deferred items

  • Double SEVERE line on the unknown-outcome path (cycles 1 and 2): skipped. When the leader cannot be asked, seedReportForAdmission logs and returns all three documents, then ServerControlPlane logs the non-empty report. The control-plane line is the one an HTTP or gRPC operator relies on for every non-empty report, and the plugin cannot tell its caller which kind of non-empty report it produced. addPeer already has the same double line. Logging in one place only would need a richer return type than this fix is worth.
  • Comment volume (cycles 2-4): skipped. It matches the surrounding file. The developer can trim it on review.
  • Live embedded IT calling server.getHA().connectClusterAndReportSeed(...) (cycle 3): skipped. Issue7401ConnectClusterJoinsPeerIT already drives the Raft override through ServerControlPlane on a real cluster.
  • Duplicate javadoc between connectCluster and connectClusterAndReportSeed on RaftHAPlugin (cycle 3): skipped as a nit.
  • Cycle 4, not applied because the cycle budget ran out: (1) a sentence in the control-plane comment saying the UOE arm relies on the no-throw-after-join contract; (2) optionally a WARNING inside RaftHAPlugin.connectClusterAndReportSeed for a caller that drops the result (this would double the log on the control-plane path); (3) AssertJ contains(List.of()) instead of isPresent()/get() in aCleanJoinReportsAPresentEmptyList; (4) trim the javadoc; (5) the bot said CI checks were still pending when it looked.

Final state: max-cycles-reached (4/4). No blocking findings. Every review said nothing blocks the merge.

🤖 Generated with Claude Code

…e cluster security seed

Adds HAServerPlugin.connectClusterAndReportSeed, overridden by RaftHAPlugin to
join and then ask the leader for the seed outcome. ServerControlPlane.connectCluster
now consumes that report instead of issuing its own seed request, so there is
still exactly one seed request per connect cluster (#7834).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@robfrank robfrank added this to the 26.10.1 milestone Oct 1, 2026
@mergify

mergify Bot commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@codacy-production

codacy-production Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

🟢 Coverage 100.00% diff coverage · -5.91% coverage variation

Metric Results
Coverage variation ✅ -5.91% coverage variation
Diff coverage ✅ 100.00% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (6abf36d) 205851 174151 84.60%
Head commit (4e00cd7) 238451 (+32600) 187643 (+13492) 78.69% (-5.91%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#8845) 10 10 100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 69b4b9f7-9b31-46c1-a2b1-9f24bb7900a5

📥 Commits

Reviewing files that changed from the base of the PR and between 6abf36d and 4e00cd7.

📒 Files selected for processing (5)
  • ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAPlugin.java
  • ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue8077EmbeddedConnectClusterSeedReportTest.java
  • server/src/main/java/com/arcadedb/server/HAServerPlugin.java
  • server/src/main/java/com/arcadedb/server/ServerControlPlane.java
  • server/src/test/java/com/arcadedb/server/Issue8077ConnectClusterConsumesPluginSeedReportTest.java

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The HA plugin API now exposes an optional security-seed report for cluster connections. The Raft implementation joins before reporting seed results. The control plane consumes a plugin report when available and uses its existing seed path when the plugin provides none.

Changes

Cluster seed reporting

Layer / File(s) Summary
Plugin report API and Raft implementation
server/src/main/java/com/arcadedb/server/HAServerPlugin.java, ha-raft/src/main/java/com/arcadedb/server/ha/raft/RaftHAPlugin.java, ha-raft/src/test/java/com/arcadedb/server/ha/raft/Issue8077EmbeddedConnectClusterSeedReportTest.java
The plugin API adds connectClusterAndReportSeed, with a default implementation that returns Optional.empty(). Raft joins first, then returns its admission seed report. Tests cover reported failures, clean reports, join ordering, join failures, and seed exceptions.
Control-plane report handling
server/src/main/java/com/arcadedb/server/ServerControlPlane.java, server/src/test/java/com/arcadedb/server/Issue8077ConnectClusterConsumesPluginSeedReportTest.java
The control plane uses the plugin’s report when present and avoids a second seed request. If no report is provided, it uses the admission seed result and falls back to a cluster-wide seed only when that result is absent. Tests cover both paths and membership failures.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant ServerControlPlane
  participant HAServerPlugin
  participant RaftHAPlugin
  ServerControlPlane->>HAServerPlugin: connectClusterAndReportSeed(serverAddress)
  HAServerPlugin->>RaftHAPlugin: dispatch reporting method
  RaftHAPlugin->>RaftHAPlugin: connectCluster(serverAddress)
  RaftHAPlugin->>RaftHAPlugin: request admission seed report
  RaftHAPlugin-->>ServerControlPlane: return optional seed report
  ServerControlPlane->>ServerControlPlane: use report or run fallback seed path
Loading

Suggested reviewers: lvca

Merge Risk: ⚪ Minimal · up to 4e00c

The reporting API preserves membership failure behavior and avoids duplicate seeding while retaining compatibility fallback. No actionable merge-blocking risk remains on the supplied evidence.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 4e00c

The reviewed change improves visibility into incomplete security synchronization without granting new membership authority. Existing administrative access checks remain in place, legacy implementations retain their fallback behavior, and security-seed failures remain distinguishable from failed joins.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant exposure is cluster security-state convergence: admission affects a peer's membership and synchronization of users, groups, and API tokens. The reporting change returns failed-document names, not security-document contents, and does not expand the membership operation's authority.

Trust Boundaries and Controls

  • observed — Caller-supplied cluster addresses reach the control plane only after the existing HTTP root-user or gRPC authenticated server-admin checks. Remote admission-seed requests retain cluster-token authentication and leader selection. The new plugin method reuses these paths rather than introducing a new transport entry point.

Resilience and Maintainability Implications

  • observed — Leadership changes and transient seed-request failures use the existing retry path. Interrupted or unreadable seed outcomes become an all-three-document failure report after successful admission, preserving uncertainty rather than declaring security convergence. This reporting does not roll back membership or guarantee exactly-once seeding across process crashes.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements the coding requirements in [#8077]. HAServerPlugin.connectClusterAndReportSeed(String) lets embedded callers receive failed security-document names. RaftHAPlugin joins before it …
Out of Scope Changes check ✅ Passed The reported changes stay within [#8077]. They add the reporting API, move report consumption into the control-plane path, preserve membership-only behavior for the void method, and add focused regres…
Title check ✅ Passed The title clearly identifies the main change: the embedded HAServerPlugin.connectCluster API now reports the cluster security seed.
Description check ✅ Passed The description is detailed and covers the change, motivation, related issue, implementation notes, test coverage, and known gaps. It does not use every template heading or explicitly confirm mvn clea…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Oct 1, 2026

Copy link
Copy Markdown

Review of #8845 (connectClusterAndReportSeed)

Overall: a small, well-scoped change that follows the existing addPeerAndReportSeed pattern. I found nothing that blocks the merge. Notes below, roughly by importance.

Correctness (looks good)

  • The single-seed invariant (HA: an admission now seeds the security documents twice, from two nodes, under two different monitors #7834) holds. Raft never returns an empty Optional (RaftHAPlugin.connectClusterAndReportSeed wraps seedReportForAdmission), so ServerControlPlane never falls through to its own seed request for Raft. The default plugin still gets the old fallback path, so pre-existing HA implementations are unchanged.
  • The join runs before the seed, and seedReportForAdmission catches IOException | RuntimeException, so a seed failure cannot turn a committed join into a failed one. Error is intentionally left to propagate, which is reasonable.

Minor issues / suggestions

  1. Duplicate SEVERE logging on the unknown-outcome path. When the leader cannot be asked, RaftHAPlugin.seedReportForAdmission logs "could not be asked to seed" and returns ALL_SEEDED_SECURITY_DOCUMENTS. ServerControlPlane.connectCluster then sees a non-empty list and logs a second SEVERE line ("could not be seeded to it: ..."). Before this PR the control plane logged the exception once, with the stack trace, in its own catch block. Not harmful, but operators will now see two alarming lines per failure. Consider logging only once (for example, only in the plugin when it owns the report), or accept it knowingly.
  2. UnsupportedOperationException arm is now wider by contract. In ServerControlPlane.connectCluster the arm wraps connectClusterAndReportSeed, so for a third-party plugin that overrides the reporting form and does its own seeding, a UnsupportedOperationException thrown from the seed part would be reported as a precondition refusal after the peer had already joined. Raft is safe (the PR description argues this and a test pins it), but the HAServerPlugin javadoc could state that an override must not throw after the membership change, as the addPeerAndReportSeed contract does. A one-line addition would close this.
  3. Test gaps (small):
    • Issue8077EmbeddedConnectClusterSeedReportTest overrides connectCluster in RecordingPlugin, so the unit test never executes the real RaftHAPlugin.connectCluster body. The ITs listed in the test plan cover that, but a unit test where the real connectCluster fails (for example, the self-join refusal from Issue7515SelfJoinRefusedTest) and is shown not to be followed by a seed would pin the "failed join is not seeded" claim against production code rather than the stub.
    • No server-side test shows a plugin that overrides connectClusterAndReportSeed and throws UnsupportedOperationException still maps to OperationNotAvailableException (the existing test only covers the default path with no membership).
  4. Style nits:
    • In Issue8077ConnectClusterConsumesPluginSeedReportTest.RecordingHAPlugin the field alignment is inconsistent (final Optional<List<String>> report; vs Optional<List<String>> leaderSeed). Please tidy it.
    • The new comments in ServerControlPlane and the javadocs are long and heavily reference issue numbers. That matches the surrounding code, so I will not push on it, but CLAUDE.md prefers short comments that explain only the non-obvious why.
    • Minor: the ternary in ServerControlPlane could be a plain if/else, or pluginReport.orElseGet(...) with the leader seed folded in, but the IOException from seedSecurityStateForAdmission makes the current form reasonable.

Performance / security

  • No hot-path impact. connectClusterAndReportSeed is a control-plane operation, and it takes the same bounded wait for the seed that addPeer already has (HA_SECURITY_SEED_RETRY_TIMEOUT plus margin). No new dependencies, no new surface beyond an interface default method, and no secrets are logged (only document names).

Process

  • Tests follow the repo's assertThat(...) style and the regression-test convention (Issue8077*), and no System.out was left behind. I could not run the tests here, so I am relying on the green runs reported in the PR description.

Nice work, and the completeness table in the description makes the review easy.

…oin and UOE-mapping tests

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 1, 2026

Copy link
Copy Markdown

Review of #8845 (connectClusterAndReportSeed)

I found nothing that blocks the merge. The change follows the addPeerAndReportSeed pattern closely, and the interface default keeps pre-existing HA implementations on the old path. I could not run the tests here, so I am relying on the green runs in the description. This pass checked the current head, which already covers the earlier bot's points on the javadoc contract, the real-join test and the UnsupportedOperationException mapping test.

Correctness

  • The HA: an admission now seeds the security documents twice, from two nodes, under two different monitors #7834 single-seed invariant holds. RaftHAPlugin.connectClusterAndReportSeed (RaftHAPlugin.java:1063-1066) never returns an empty Optional, and seedReportForAdmission swallows IOException | RuntimeException, so a committed join is never turned into a failed one.
  • The ServerControlPlane UnsupportedOperationException arm now wraps the plugin's seed as well as the join. Raft is safe, and the new javadoc says an override must not throw after the membership change. That is enough.

Suggestions (non-blocking)

  1. Double SEVERE logging on the unknown-outcome path. When the leader cannot be reached, seedReportForAdmission logs "Peer '%s' was added but the leader could not be asked to seed..." and returns ALL_SEEDED_SECURITY_DOCUMENTS. ServerControlPlane.connectCluster then sees a non-empty list and logs a second SEVERE line ("joined '%s' but these security documents could not be seeded"). Before this PR there was one line, with the stack trace. Operators now get two alarming lines per failure, and the plugin's message says "Peer was added" even on a connect cluster call. Either accept it knowingly or log in only one place.
  2. Stale comment in ServerControlPlane. The long "ASKED OF THE LEADER rather than run here" block above the final try still reads as if this method issues the seed request. For Raft it is now the plugin that does, and connectCluster only consumes the report. A short sentence pointing at RaftHAPlugin.connectClusterAndReportSeed would stop the next reader from looking for the request here. CLAUDE.md also prefers shorter comments, and this block plus the new javadocs are heavy on issue-number references.
  3. Style. The ternary pluginReport.isPresent() ? pluginReport.get() : ... could be an if/else, which reads more simply than a multi-line ternary. In the test RecordingHAPlugin the field alignment is uneven (final Optional<List<String>> report; against Optional<List<String>> leaderSeed).
  4. Embedded path logging. addPeer logs SEVERE when failedSeeds is non-empty, but connectClusterAndReportSeed returns the list silently. That is fine because the caller owns the report, but the javadoc could say so, since the same javadoc notes that an embedder has no other way to see the failure.

Performance / security

There is no hot-path impact. This is a control-plane call with the same bounded seed wait as addPeer. There are no new dependencies, and only document names are logged.

Tests

The coverage is good. It includes the single-seed ordering, a failed join not being seeded (against both the stub and the production connectCluster), the default-plugin fallback and the precondition mapping. The tests use the assertThat(...) style, and no System.out was left behind.

…port, if/else over ternary, javadoc on who logs

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 1, 2026

Copy link
Copy Markdown

Review of #8845

Overall this is a clean, well-scoped change. The new connectClusterAndReportSeed mirrors addPeerAndReportSeed, and it reuses seedReportForAdmission instead of duplicating the seed logic. The Optional.empty() default is consistent with the seedSecurityStateForAdmission convention. ServerControlPlane still makes exactly one seed request per connect cluster, and an HA plugin that predates the method behaves as before. I found no blocking issues.

What I checked

  • RaftHAPlugin.connectClusterAndReportSeed calls connectCluster first, so a failed join throws before any seed request. seedReportForAdmission catches IOException | RuntimeException and returns ALL_SEEDED_SECURITY_DOCUMENTS, so the method cannot throw after the join committed. The new tests cover both cases.
  • ServerControlPlane: the UnsupportedOperationException arm now wraps the reporting form. For Raft the seed cannot raise a UOE because it is caught inside seedReportForAdmission. The new test covers a plugin that overrides the reporting form and throws a UOE.
  • The Raft path no longer reaches the catch in ServerControlPlane that logs the "could not be seeded" failure. seedReportForAdmission logs its own SEVERE message and returns all three documents. The operator still gets a log line, but its text differs from before, so anything matching the old wording would break. The ITs listed in the PR description should catch that.

Suggestions (non-blocking)

  1. Latency note in the javadoc. addPeer documents that it now waits for the leader seed report, which can take seconds. connectClusterAndReportSeed has the same wait but its javadoc does not say so. A one-line note would help embedders, and so would saying that the void connectCluster stays fast.
  2. Comment volume. The new comments in ServerControlPlane and the javadoc on HAServerPlugin.connectClusterAndReportSeed are long. The repo guidance is to comment only the non-obvious WHY and to avoid referencing issue numbers or callers. The Optional.empty() contract and the "never throw after the join" rule are worth keeping. The restatements of what the code does, such as the long comment above the if (pluginReport.isPresent()) block, could go.
  3. Embedded test uses a stub. Issue8077EmbeddedConnectClusterSeedReportTest overrides connectCluster and seedSecurityStateForAdmission, so it checks the sequencing but not a live Raft join. The listed ITs go through ServerControlPlane, which now calls the new method, so the coverage is there indirectly. A small embedded test on a real cluster calling server.getHA().connectClusterAndReportSeed(...) directly would pin the headline use case of the issue. Not required.
  4. Duplicate javadoc. The {@inheritDoc} plus extra paragraph on RaftHAPlugin.connectClusterAndReportSeed and the paragraph added to connectCluster say the same thing twice. One is enough.

The diff follows CLAUDE.md for style: final on locals and parameters, no fully qualified names, AssertJ assertThat syntax, and no new dependencies. The new code adds no obvious allocation or locking cost on a hot path. Nothing here is a security concern, since the change only passes a report through.

LGTM once you have decided on suggestion 1.

…ortSeed

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 1, 2026

Copy link
Copy Markdown

Review of #8845

Overall this is a clean, well-scoped change. It mirrors the existing addPeerAndReportSeed pattern, keeps a single seed request per connect cluster (#7834), and the Optional.empty() default keeps older HAServerPlugin implementations on the old path. I read the diff and the surrounding code in RaftHAPlugin and ServerControlPlane, but I did not run the tests myself.

What looks good

  • RaftHAPlugin.connectClusterAndReportSeed reuses seedReportForAdmission. It cannot throw after the join, and an unknown outcome is reported as all three documents. This is the right failure semantics.
  • ServerControlPlane only falls back to its own seed when the report is empty. Raft never returns empty, so no double seeding.
  • Tests cover ordering, a failed join not being seeded, the unchecked-failure case and the legacy default. Both the stub-based and the real-connectCluster ("not started") variants are included.

Points worth a look

  1. The UOE arm now also wraps the seed for third-party overrides (ServerControlPlane.java, the catch (UnsupportedOperationException) around connectClusterAndReportSeed). For Raft this is fine because seedReportForAdmission catches every RuntimeException. For any other override that seeds inside connectClusterAndReportSeed, a UOE thrown after the membership change would be mapped to OperationNotAvailableException ("precondition") even though the peer has already joined. The Javadoc already says overrides must not throw after the change, so this is a contract risk only. One sentence in the control-plane comment saying the arm assumes that contract would help.

  2. No log of a non-empty report on the embedded path. ServerControlPlane logs failed documents for connect cluster, but a direct embedder of connectClusterAndReportSeed who ignores the return value gets nothing when the leader reports failed documents (only the "could not ask the leader" log in the exception case). The Javadoc says "returned, not logged" deliberately, so this is a judgment call. A WARNING in RaftHAPlugin.connectClusterAndReportSeed would stop the silent-failure case from the issue reappearing for callers who discard the result, at the price of a double log on the control-plane path.

  3. Test style, minor:

    • Issue8077EmbeddedConnectClusterSeedReportTest.aCleanJoinReportsAPresentEmptyList uses isPresent() then .get(). assertThat(report).contains(List.of()) is more idiomatic AssertJ.
    • RecordingPlugin extends the real RaftHAPlugin and relies on its no-arg construction having no side effects. That works today but couples the test to the constructor.
  4. Comment volume. The new comments in ServerControlPlane and the HAServerPlugin Javadoc are very long. CLAUDE.md asks for comments only where the WHY is non-obvious. The files are already similarly verbose, so I would not block on it, but the Javadoc could be roughly halved without losing the contract (member whenever it returns, waits for the seed, empty Optional meaning).

  5. API subtlety. Optional.empty() meaning "caller seeds" is not obvious, but it matches the seedSecurityStateForAdmission convention and is clearly documented.

Security / performance

No new security surface. The only cost is the bounded seed wait for embedders who switch to the new method, and that is documented. No hot-path or allocation concerns.

CI

Several checks (build-and-package, Analyze, lint) were not green when I looked. Please confirm they pass before merging.

Nothing here blocks the merge; points 1 and 2 are the ones I would consider addressing.

@codecov

codecov Bot commented Oct 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.34%. Comparing base (6abf36d) to head (4e00cd7).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8845   +/-   ##
=========================================
  Coverage     76.34%   76.34%           
- Complexity     3779     3780    +1     
=========================================
  Files          2102     2102           
  Lines        205851   205857    +6     
  Branches      43375    43376    +1     
=========================================
+ Hits         157159   157167    +8     
+ Misses        31742    31741    -1     
+ Partials      16950    16949    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HA admission: the embedded HAServerPlugin.connectCluster API reports no residual security-seed failure

1 participant