You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[controller] Add a lightweight controller health endpoint - #3064
The controller API lacks a dedicated readiness endpoint. Health-check clients need a cheap, parameter-free way to distinguish a controller ready to serve API requests from one still starting or shutting down.
Solution
Add GET /health on the existing controller HTTP/HTTPS API service. It returns plain-text 200 OK when initialization completed and the required local services are running; otherwise it returns 503 NOT_READY. Eligible standbys pass without leadership checks; passive parent regions remain unready.
Use one shared atomic signal: NOT_READY, READY, STOPPING. The existing startup sequence stays unchanged, with a ready transition added at its successful end. Startup exceptions leave the signal unready. Shutdown marks stopping before unregistration, and late startup completion cannot restore readiness.
The request reads local state only, with no cluster scan, dependency call or wait. The route is enabled by default and respects existing disabled routes. Only exact GET /health receives the non-sensitive HTTP health exception; other methods and lookalike paths follow normal SSL enforcement. HTTPS client-certificate policy is unchanged.
AdminSparkServer has one constructor requiring an explicit serializer and live readiness callback. Production listeners use the controller owner's callback. The existing ServiceFactory.getMockAdminSparkServer(...) helper provides default serialization and readiness tied to the mock listener's start/stop lifecycle. Exact GET /health probes retain request metrics and failure classification but skip per-request INFO audit logging to avoid repetitive logs from periodic probes. Other methods and paths retain normal auditing.
This checks local API readiness and responsiveness, not every backend operation. No discovery-policy change or deployment is included.
Code changes
Added new code behind a config. If so list the config names and their default values in the PR description.
Introduced new log lines.
Confirmed if logs need to be rate limited to avoid excessive logging.
No new configuration property or per-probe log statement is added; HEALTH uses the existing disabled-routes setting.
Concurrency-Specific Checks
Both reviewer and PR author to verify
Code has no race conditions or thread safety issues.
Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed.
No blocking calls inside critical sections that could lead to deadlocks or performance degradation.
Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList).
Validated proper exception handling in multi-threaded code to avoid silent thread termination.
The startup transition uses compare-and-set so late startup cannot overwrite terminal shutdown. Each probe evaluates the required local services directly, then reads the lifecycle phase once. It introduces no collection, lifecycle lock or background thread. Restart creates a fresh controller owner.
How was this PR tested?
Testing Done
Six focused test methods cover the readiness signal, handler response, audit/metrics behavior, actual owner wiring and mock-listener lifecycle. The owner test calls real startup/shutdown with controlled services and covers child HTTP/HTTPS configurations, ACTIVE/PASSIVE parents, failed startup, required-service gates, registration/unregistration ordering and shutdown during a service-state check. The mock-listener test checks 200 OK after startup and 503 NOT_READY before listener teardown.
The existing loopback test runs with SSL enforcement enabled and disabled. It checks GET 200/503 responses with no INFO audit logs, retained metrics, disabled routes, and normal auditing and SSL enforcement for POST and slash-alias requests. All 20 selected controller and mock-server cases passed, including the existing four mock-server test methods; scoped Spotless checks passed after correcting one line-wrap violation.
The earlier 22 selected cases, owner-wiring coverage, integration-helper consumer compilation and scoped Spotless checks passed. Real TLS handshakes and deployment/fault exercises were not run.
New unit tests added.
New integration tests added.
Modified or extended existing tests.
Verified backward compatibility (if applicable).
Does this PR introduce any user-facing or breaking changes?
No. You can skip the rest of this section.
Yes. Clearly explain the behavior change and its impact.
The old 13-argument AdminSparkServer convenience constructor and 14-argument full signature are replaced by one 15-argument constructor with explicit serialization and readiness wiring. Direct consumers must update. The reusable mock-server factory keeps its existing signature and now reports ready after successful startup instead of remaining permanently unready. Other business API behavior is unchanged.
Expose a parameter-free GET /health route on the existing API listener.
Return a fixed liveness response without administrative lookups, and
reuse the disabled-routes setting while preserving other route security.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep health unready until controller initialization completes and the
required local API services can serve requests. Invalidate readiness
before shutdown and prevent late startup from restoring it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve upstream push-job serializer injection alongside the API
readiness signal in both controller listeners.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use one shared three-state signal while preserving the original startup
sequence. Keep health unready until successful initialization and prevent
late completion from undoing shutdown.
Replace the broad test fixtures with focused state, response and policy
checks, retaining existing route, job and stats regressions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Registering a readiness probe here also sends every GET /health through the global audit filters, which log both the request and completion at INFO (lines 336 and 378/381). Readiness agents poll continuously, so this creates two log records per probe on each listener and can materially increase controller log volume. Please exempt HEALTH from those audit INFO calls while retaining its metrics/failure marker, or rate-limit the probe logs.
Simplify readiness constructor wiring and suppress health audit INFO
logs while preserving request metrics and other API auditing.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Match the HTTP method and raw path before skipping health audit logs or
SSL enforcement. Preserve normal handling of POST and slash-alias paths.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
The implementation matches the documented readiness contract and includes focused coverage of lifecycle, concurrency, routing, metrics, and auditing behavior.
Restore the existing request and completion audit logging for health.
Keep the exact GET /health check only in the HTTP SSL exception.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Restore audit suppression for exact GET /health and document why periodic
probes should not generate repetitive INFO logs. Preserve all metrics.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
The implementation matches the documented readiness contract and includes focused coverage of lifecycle, concurrency, routing, metrics, and SSL behavior.
Evaluate local service readiness directly, then read the lifecycle phase
once. Rename the terminal shutdown state to STOPPING and retain the
fail-closed startup transition.
Keep one live readiness callback for both API listeners and preserve
parent-region eligibility.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
🟢 Approval recommended
The readiness lifecycle, endpoint behavior, security exceptions, metrics, auditing, and configuration handling are consistently implemented and tested.
Remove the default-false AdminSparkServer convenience constructor.
Keep default serialization and lifecycle-backed mock readiness in the
existing ServiceFactory helper.
Preserve production owner callbacks and validate mock health before
listener teardown.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Problem Statement
The controller API lacks a dedicated readiness endpoint. Health-check clients need a cheap, parameter-free way to distinguish a controller ready to serve API requests from one still starting or shutting down.
Solution
Add
GET /healthon the existing controller HTTP/HTTPS API service. It returns plain-text200 OKwhen initialization completed and the required local services are running; otherwise it returns503 NOT_READY. Eligible standbys pass without leadership checks; passive parent regions remain unready.Use one shared atomic signal:
NOT_READY,READY,STOPPING. The existing startup sequence stays unchanged, with a ready transition added at its successful end. Startup exceptions leave the signal unready. Shutdown marks stopping before unregistration, and late startup completion cannot restore readiness.The request reads local state only, with no cluster scan, dependency call or wait. The route is enabled by default and respects existing disabled routes. Only exact
GET /healthreceives the non-sensitive HTTP health exception; other methods and lookalike paths follow normal SSL enforcement. HTTPS client-certificate policy is unchanged.AdminSparkServerhas one constructor requiring an explicit serializer and live readiness callback. Production listeners use the controller owner's callback. The existingServiceFactory.getMockAdminSparkServer(...)helper provides default serialization and readiness tied to the mock listener's start/stop lifecycle. ExactGET /healthprobes retain request metrics and failure classification but skip per-request INFO audit logging to avoid repetitive logs from periodic probes. Other methods and paths retain normal auditing.This checks local API readiness and responsiveness, not every backend operation. No discovery-policy change or deployment is included.
Code changes
No new configuration property or per-probe log statement is added;
HEALTHuses the existing disabled-routes setting.Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed.ConcurrentHashMap,CopyOnWriteArrayList).The startup transition uses compare-and-set so late startup cannot overwrite terminal shutdown. Each probe evaluates the required local services directly, then reads the lifecycle phase once. It introduces no collection, lifecycle lock or background thread. Restart creates a fresh controller owner.
How was this PR tested?
Testing Done
Six focused test methods cover the readiness signal, handler response, audit/metrics behavior, actual owner wiring and mock-listener lifecycle. The owner test calls real startup/shutdown with controlled services and covers child HTTP/HTTPS configurations, ACTIVE/PASSIVE parents, failed startup, required-service gates, registration/unregistration ordering and shutdown during a service-state check. The mock-listener test checks
200 OKafter startup and503 NOT_READYbefore listener teardown.The existing loopback test runs with SSL enforcement enabled and disabled. It checks GET 200/503 responses with no INFO audit logs, retained metrics, disabled routes, and normal auditing and SSL enforcement for POST and slash-alias requests. All 20 selected controller and mock-server cases passed, including the existing four mock-server test methods; scoped Spotless checks passed after correcting one line-wrap violation.
The earlier 22 selected cases, owner-wiring coverage, integration-helper consumer compilation and scoped Spotless checks passed. Real TLS handshakes and deployment/fault exercises were not run.
Does this PR introduce any user-facing or breaking changes?
The old 13-argument
AdminSparkServerconvenience constructor and 14-argument full signature are replaced by one 15-argument constructor with explicit serialization and readiness wiring. Direct consumers must update. The reusable mock-server factory keeps its existing signature and now reports ready after successful startup instead of remaining permanently unready. Other business API behavior is unchanged.