fix(#5050): gRPC lifecycle & cleanup

[robfrank]

Closes #5050

Summary

Resolves the lower-severity gRPC lifecycle/housekeeping findings from the 2026-07 audit:

• CON-2 (Medium): in grpc.mode=both, GrpcServerPlugin.configureServer was invoked once per server builder, each constructing a fresh ArcadeDbGrpcService and overwriting this.grpcService/healthManager; stopService closed only the last instance, leaking the first service's idle-reaper daemon thread and its transaction registry. The service and health manager are now built once and reused across both builders.
• CON-6 (Low): stopService is now idempotent/concurrent-safe via an AtomicBoolean stopped guard (the JVM shutdown hook and the plugin-lifecycle stop can both call it); the lifecycle fields read by getStatus() are now volatile.
• H1 (Low): removed a leftover System.out.print(...) in ProtoUtils.toProtoRecord that fired for every property of every record on the client write path (stdout flood + property-value leak).
• DOC-1 (Low): corrected the inverted InsertOptions.TransactionMode proto comments (PER_STREAM commits once at stream end; PER_ROW commits per row; PER_BATCH commits every server_batch_size rows), verified against the commit sites in ArcadeDbGrpcService.
• CODE-1 (Low): acquireLock() resolved RemoteGrpcTransactionExplicitLock to a private nested stub (no bucket/type/lock overrides), shadowing the fully-implemented top-level class. The issue proposed deleting the top-level file, but it carries the real LOCK-command logic and is exercised by the existing RemoteGrpcTransactionExplicitLockIT; the correct resolution is "make the code use it", so the redundant nested stub was removed and acquireLock() now resolves to the real implementation.

Test plan

• Issue5050GrpcPluginLifecycleTest (grpcw): "both"-mode configuration reuses a single ArcadeDbGrpcService (no reaper-thread leak); stopService is idempotent under repeated invocation.
• Issue5050ProtoUtilsNoStdoutTest (grpc-client): toProtoRecord writes nothing to stdout.
• Existing GrpcServerPluginTest, ArcadeDbGrpcServiceReaperTest, and RemoteGrpcTransactionExplicitLockIT still compile/pass (the latter validates the restored top-level explicit-lock class).
• grpc, grpcw, grpc-client modules compile.

[mergify]

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

• Queue this pull request

[gemini-code-assist]

Code Review

This pull request addresses issue #5050 by resolving resource leaks and ensuring idempotency in the gRPC plugin lifecycle. It removes a leftover System.out.print statement in ProtoUtils.toProtoRecord and adds a corresponding regression test. In GrpcServerPlugin, it ensures that only a single instance of ArcadeDbGrpcService and HealthStatusManager are created and reused when configuring multiple server builders, preventing duplicate reaper threads and registry leaks. It also introduces an AtomicBoolean guard to make stopService() idempotent, along with regression tests for these lifecycle improvements. The review feedback suggests making the package-private configureServer method synchronized to prevent potential race conditions during the check-then-act initialization of shared volatile fields.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Since configureServer is now package-private and can be invoked from external contexts (such as tests), and it performs check-then-act initialization on the shared volatile fields grpcService and healthManager, there is a potential race condition if it is called concurrently.

Making the configureServer method synchronized is a simple and robust way to guarantee thread-safe initialization of these shared resources with negligible overhead.

Suggested change

	void configureServer(ServerBuilder<?> serverBuilder, ContextConfiguration config) {
	synchronized void configureServer(ServerBuilder<?> serverBuilder, ContextConfiguration config) {

[codacy-production]

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 · -6.75% coverage variation

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

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (6fac1d8)	137532	102631	74.62%
Head commit (bdf7d80)	169379 (+31847)	114955 (+12324)	67.87% (-6.75%)

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 (#5197)	15	15	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.}

[claude]

PR Review: fix(#5050) gRPC lifecycle & cleanup

Reviewed the full diff plus surrounding context in GrpcServerPlugin, ArcadeDbGrpcService, RemoteGrpcDatabase, and the two new tests. Overall this is a clean, well-scoped, well-documented set of fixes. Each finding maps to a concrete root cause, the accompanying docs/5050-*.md matches the repo convention, and both regression tests are targeted and deterministic. Nice work.

Strengths

• CON-2 fix is correct and the real bug. In both mode configureServer was previously overwriting this.grpcService/healthManager on the second call, and stopService() only closed the last instance, orphaning the first txReaper ScheduledExecutorService (a single-thread pool) plus its transaction registry. The null-guarded single-construction fix is the right approach, and reusing one service/registry across both builders is the intended semantics.
• CON-6: the AtomicBoolean stopped CAS guard plus volatile on grpcServer/xdsServer/healthManager/grpcService correctly closes the shutdown-hook-vs-lifecycle race that getStatus() also reads. Good call making exactly the fields read across threads volatile.
• CODE-1: verified the resolution. The nested private stub lacked the bucket/type/lock overrides, so acquireLock() was silently returning a no-op lock. Removing it makes new RemoteGrpcTransactionExplicitLock(this) resolve to the fully-implemented top-level class in the same package, which RemoteGrpcTransactionExplicitLockIT already covers. This was a latent correctness bug, not just cleanup - worth calling out.
• H1/DOC-1: removing the per-property System.out.print on the client hot path (stdout flood + value leak) and correcting the inverted proto comments are both clearly right.
• The configureServer null-checks are safe from a data-race standpoint because startService invokes it sequentially (standard then xDS) on the same thread - no concurrency on construction.

Minor suggestions (non-blocking)

1. stopped is never reset, so the plugin is single-use. If the server lifecycle ever does stopService() then startService() on the same plugin instance, the second stopService() becomes a permanent no-op, and since grpcService is not nulled on stop, configureServer would also re-add the already-closed service (dead reaper). This is fine for the normal create-once/destroy-once plugin lifecycle, but if restart-in-place is ever possible it would silently leak. Consider resetting stopped.set(false) (and clearing grpcService/healthManager) in startService, or add a one-line comment stating the instance is intentionally single-use.

2. ProtoUtils test does assertions inside db.transaction(...). An AssertionError thrown inside the lambda will propagate through the transaction wrapper (rollback then rethrow), so the test still fails correctly - but pulling the capture/assert out of the transaction (the record just needs to be built, not committed) would make the intent clearer and avoid any dependency on how the tx wrapper handles Error. Optional.

3. Robustness of stopService: the guard is set before cleanup runs, and only InterruptedException is caught. If any other unchecked exception were thrown mid-cleanup, the remaining steps (server shutdown, hook removal) would be skipped with no retry possible. Not a regression versus the prior code, just noting the idempotency guarantee is "runs at most once," not "completes fully."

Test coverage

Good. bothModeReusesSingleServiceInstance asserts isSameAs on the reused service and that the reaper is active; stopServiceIsIdempotent exercises the triple-stop path. Both avoid binding ports so they stay fast. The stdout regression test correctly captures/restores System.out. One small gap: no test asserts the healthManager is also reused across builders (only the service is), though the code path is symmetric.

Nothing here is blocking - the core fixes are correct and safe. LGTM. 👍

[claude]

Review: PR #5197 - gRPC lifecycle & cleanup

Reviewed against CLAUDE.md. Overall this is a clean, well-targeted fix that resolves the deferred lower-severity findings from #5050. Each change is scoped, documented, and backed by a regression test. I verified the correctness of the subtler pieces below.

Verified correct

• CON-2 (single service in both mode): Confirmed startService calls configureServer twice (GrpcServerPlugin.java:163 and :204). The if (this.grpcService == null) guard now builds one ArcadeDbGrpcService (one reaper thread, one tx registry) and reuses it across both builders. Sharing a single BindableService/HealthService instance across two ServerBuilders is safe in gRPC. Fixes the daemon-thread + registry leak.
• CON-6 (idempotent stopService): The AtomicBoolean CAS guard correctly de-duplicates the shutdown-hook vs. lifecycle-stop race. Making grpcServer/xdsServer/healthManager volatile is right since getStatus() reads them from other threads.
• DOC-1 (proto comments): Cross-checked all four modes against the commit sites in ArcadeDbGrpcService (flushCommit at :3244-3249, the switch at :3855-3880). PER_ROW commits per row, PER_BATCH commits every serverBatchSize(ctx) rows (reads getServerBatchSize(), matching the server_batch_size wording), PER_REQUEST/PER_STREAM commit once at stream end. The corrected comments are accurate.
• CODE-1 (explicit-lock stub): Removing the private nested RemoteGrpcTransactionExplicitLock correctly un-shadows the top-level class (a same-named nested type takes precedence over the top-level one in the same package), so acquireLock() now resolves to the real implementation with the bucket/type/lock overrides. Good call keeping the top-level file rather than following the issue's delete-it suggestion, since RemoteGrpcTransactionExplicitLockIT exercises it.
• H1: The stray System.out.print removal is a clear win (stdout flood + property-value leak on the client write path).

Suggestions

1. docs/review-deferred-472d49a0.md looks like an accidental commit. Unlike docs/5050-grpc-lifecycle-cleanup.md (which fits the established NNNN-*.md convention in docs/), this file is internal review-cycle scratch notes referencing "gemini (medium)", "claude build support #1", "claude Bump gremlin.version from 3.4.10 to 3.5.1 #2", "Skipped (with rationale)". It shouldn't ship in the repo history. Recommend deleting it from the PR.

2. Commit trailer Co-Authored-By: Claude Opus 4.8. CLAUDE.md explicitly states "don't add Claude as author of any source code." Worth amending before merge.

3. Issue5050ProtoUtilsNoStdoutTest swaps the global System.out. Standard pattern and correctly restored in finally, but a JVM-shared parallel test run could observe the temporary redirect. Since ArcadeDB typically forks per test class this is low-risk; flagging for awareness only. No change required.

4. Minor (non-blocking): healthManager reuse isn't asserted. Already documented as deferred (no getter, structurally identical null-guard to the tested grpcService path). Reasonable as-is.

Nits

• configureServer becoming synchronized purely as a test seam is a reasonable defensive choice, and the comment explains it well. No production contention since startService drives it sequentially.
• The stopped guard is terminal (restart-in-place unsupported). This matches prior behavior (fields were never reset before either), so no regression, and it is now explicitly documented.

Nice work - the fixes are correct and the tests target the right seams (isIdleReaperActive(), getService()) without binding ports. Addressing points 1 and 2 before merge would be my only asks.

[claude]

PR Review: fix(#5050) gRPC lifecycle & cleanup

Reviewed against the changed files and the surrounding gRPC code. This is a clean, well-scoped set of fixes and every claim in the description was verifiable against the source. Nice work, especially the regression tests and the design doc.

Correctness - verified

• CON-2 (leak): Confirmed. In both mode configureServer runs twice; previously each call built a new ArcadeDbGrpcService, overwriting this.grpcService, and stopService only closed the last one. ArcadeDbGrpcService.close() calls txReaper.shutdownNow(), so the first service single-threaded scheduled reaper plus its tx registry leaked. The if (this.grpcService == null) guard + reuse across both builders resolves it correctly.
• CON-6: The compareAndSet(false, true) guard makes stopService genuinely idempotent, and the removeShutdownHook call is already wrapped for IllegalStateException. Good.
• DOC-1: Spot-checked the corrected proto comments against flushCommit and the streaming loop in ArcadeDbGrpcService: PER_ROW commits per row, PER_BATCH commits every serverBatchSize rows, PER_STREAM/PER_REQUEST commit only at stream end. The new comments now match the code - the old ones were indeed inverted.
• CODE-1: Removing the private nested RemoteGrpcTransactionExplicitLock stub lets acquireLock() resolve to the fully-implemented top-level class (with the real bucket/type/lock overrides). Keeping the top-level class rather than deleting it (as the issue suggested) is the right call since it is covered by RemoteGrpcTransactionExplicitLockIT.
• H1: Straightforward removal of a hot-path System.out.print that leaked property values. Good catch.

Minor nits (non-blocking)

1. Style - fully qualified names in test: Issue5050ProtoUtilsNoStdoutTest uses com.arcadedb.schema.Type.STRING / Type.INTEGER inline. CLAUDE.md asks to import the class and use the simple name (import com.arcadedb.schema.Type;).
2. shutdownHook left non-volatile while grpcServer/xdsServer/healthManager/grpcService were made volatile. It is only read inside stopService (not getStatus), and in practice startService/stopService run on the same lifecycle thread, so this is fine - just flagging the intentional asymmetry in case it was an oversight. Worst case is a missed hook removal during JVM shutdown, which is harmless.

Reusing one service across two ServerBuilders

Worth a sentence in a comment: the same ArcadeDbGrpcService BindableService and the same healthManager.getHealthService() are now added to both the standard and xDS builders in both mode. This is safe because gRPC calls bindService() at build time per server, and sharing the tx registry/reaper across both transports is exactly the intent - but it is a subtle invariant worth noting for future maintainers.

Test coverage - good

• Issue5050GrpcPluginLifecycleTest exercises the builder path without binding ports (fast/deterministic) and asserts both single-instance reuse and idempotent stop.
• Issue5050ProtoUtilsNoStdoutTest captures System.out and asserts emptiness.
• Both are fast enough not to need the slow tag.

Overall: LGTM. Only the fully-qualified-name nit is a real (trivial) style deviation; everything else is optional polish.

[claude]

PR Review: gRPC lifecycle & cleanup (#5197)

Reviewed the full diff against the source. This is a clean, well-targeted set of fixes and I verified each one against the code paths it claims to touch. Nice work - the fixes are correct and each is backed by a regression test.

Verified correct

• CON-2 (service reuse in both mode): Confirmed configureServer is invoked twice from startService (startStandardServer + startXdsServer), and the previous unconditional new ArcadeDbGrpcService(...) did leak the first instance's reaper thread + tx registry since stopService only closed this.grpcService (the last one). The null-guarded single-construction fix is right, and sharing one BindableService across both builders is safe (gRPC calls bindService() per server at build time).
• CON-6 (idempotent stop): AtomicBoolean stopped + compareAndSet guard is correct for the shutdown-hook-vs-lifecycle race; making the lifecycle fields volatile for getStatus() reads is consistent.
• H1: the System.out.print in ProtoUtils.toProtoRecord was on the per-property client write path - good catch, good removal.
• DOC-1: I cross-checked the corrected proto comments against the flushCommit/commit switch in ArcadeDbGrpcService (PER_ROW commits per row, PER_BATCH every serverBatchSize, PER_STREAM/PER_REQUEST only when end). The new comments match actual behavior.
• CODE-1: Confirmed the private nested RemoteGrpcTransactionExplicitLock stub shadowed the fully-implemented top-level class of the same name (same package). Removing the nested stub makes acquireLock() (line ~1217) resolve to the real implementation. Agree this is a latent correctness fix, not just cleanup - worth calling out that it changes runtime behavior (no-op lock to real LOCK command), so the existing RemoteGrpcTransactionExplicitLockIT coverage is important here.

Minor / non-blocking

1. stopped is set before cleanup runs. compareAndSet(false, true) happens first, so if the first stopService() invocation died with an unchecked exception mid-cleanup, later calls short-circuit and skip the remaining teardown. Risk is very low in practice (the risky calls are either benign or already try/catch-wrapped, and awaitTermination's InterruptedException is caught), so this is fine as-is - just noting the trade-off.

2. Test gap (already acknowledged): Issue5050GrpcPluginLifecycleTest asserts single-service reuse but not single-healthManager reuse, even though the health-manager reuse is part of the CON-2 fix. A configureServer-twice + assert-same-health-service check would close that. Optional.

3. docs/5050-grpc-lifecycle-cleanup.md reads as internal process/scratch content - "Review cycles", "max-cycles-reached", and a note about the Co-Authored-By: Claude trailers vs. CLAUDE.md. Given CLAUDE.md's guidance against Claude authorship attribution and that this is process metadata rather than product docs, consider dropping this file (or trimming it to the technical root-cause/fix summary) before merge. The commit Co-Authored-By: Claude trailers themselves would ideally be stripped at squash-merge per CLAUDE.md.

Style/conventions

Consistent with the codebase - final on locals/params, AssertJ assertThat(...).isX() style, proper Apache license headers, tests tagged appropriately (fast, no port binding). Good.

Overall: LGTM. The only actionable item I'd flag for merge is the tracking/scratch doc (point 3).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.06%. Comparing base (6fac1d8) to head (bdf7d80).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5197      +/-   ##
============================================
+ Coverage     65.64%   66.06%   +0.42%     
+ Complexity      918      914       -4     
============================================
  Files          1696     1696              
  Lines        137532   137534       +2     
  Branches      29434    29437       +3     
============================================
+ Hits          90287    90867     +580     
+ Misses        34941    34350     -591     
- Partials      12304    12317      +13     

☔ 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.