fix(docker): make the Server startup timeout configurable - #3187
fix(docker): make the Server startup timeout configurable#3187bitflicker64 wants to merge 2 commits into
Conversation
The entrypoint ran start-hugegraph.sh with a literal -t 120, so a Server that needed longer than 120 seconds to answer on its REST port was terminated by its own container, no matter how much startup budget the orchestrator's probe allowed. Read the timeout from HG_SERVER_STARTUP_TIMEOUT_S instead, keep 120 as the default, and reject values that are not positive whole numbers before init-store runs. Covered by docker-entrypoint-test.sh: default passthrough, an explicit override, and rejection of an invalid value. Documented in the Server docker README. Closes apache#3186
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3187 +/- ##
============================================
- Coverage 37.78% 37.77% -0.02%
+ Complexity 6556 6550 -6
============================================
Files 800 800
Lines 68929 68929
Branches 9157 9157
============================================
- Hits 26046 26038 -8
- Misses 39824 39833 +9
+ Partials 3059 3058 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The configurable timeout is wired correctly for normal values, but its accepted input range can overflow the startup deadline calculation. Evidence: Bash arithmetic reproduces a negative deadline for 9223372036854775807; all reported GitHub checks are successful.
| # to decide. Validated here so a typo fails before init-store runs, rather | ||
| # than reaching the arithmetic in wait_for_startup. | ||
| SERVER_STARTUP_TIMEOUT_S="${HG_SERVER_STARTUP_TIMEOUT_S:-120}" | ||
| if [[ ! "${SERVER_STARTUP_TIMEOUT_S}" =~ ^[1-9][0-9]*$ ]]; then |
There was a problem hiding this comment.
HG_SERVER_STARTUP_TIMEOUT_S accepts any positive integer, but wait_for_startup computes stop_s=$((now_s + timeout_s)) (the shared util.sh implementation). With the documented input 9223372036854775807, Bash overflows that sum to a negative value, so the loop is skipped and the container reports a startup timeout immediately instead of honoring the configured budget. Please reject values that cannot be safely added to the current epoch or use overflow-safe/saturating deadline arithmetic, and add a boundary test.
There was a problem hiding this comment.
Fixed in 6ca3df3. Reproduced first: with now_s=1788438716, $((now_s + 9223372036854775807)) gives -9223372035066337093, so [ "$now_s" -lt "$stop_s" ] is false and the loop never probes.
The accepted range is now 1 to 86400 seconds. The pattern is bounded to five digits (^[1-9][0-9]{0,4}$) before the upper-bound comparison runs, so the comparison itself cannot overflow either; bash wraps a 20-digit literal silently rather than erroring, so the regex has to do that part. Boundary tests added for 86400 accepted, 86401 and 9223372036854775807 rejected.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The 120s default is preserved when the variable is unset and the guard runs ahead of every config write and init-store call, but :- also lets an explicitly empty value fall back to 120 silently, which the README section added here states does not happen. The new test asserts that a bad value does not start the server without pinning the ordering guarantee the code comment claims. Evidence: ran bash hugegraph-server/hugegraph-dist/docker/docker-entrypoint-test.sh at 5cad9dc under bash 5.3 (exit 0); drove the entrypoint with a stubbed bin/ for empty, whitespace, 0, +5 and 450, where empty alone exits 0 with -t 120; reran the suite against an entrypoint with the validation block relocated below init-store.sh, which also exits 0; healthcheck values read from hugegraph-server/Dockerfile:74, hugegraph-server/Dockerfile-hstore:76 and docker/docker-compose.yml:41-46.
| # container terminates a JVM that is still starting and the probe never gets | ||
| # to decide. Validated here so a typo fails before init-store runs, rather | ||
| # than reaching the arithmetic in wait_for_startup. | ||
| SERVER_STARTUP_TIMEOUT_S="${HG_SERVER_STARTUP_TIMEOUT_S:-120}" |
There was a problem hiding this comment.
:- treats an explicitly empty value as unset, so HG_SERVER_STARTUP_TIMEOUT_S= never reaches the regex below and silently becomes 120. Driving this entrypoint with a stubbed bin/: empty exits 0 with -t 120, while 0, +5 and a whitespace-only value are all rejected. That contradicts the README text added in this PR (lines 161-163: a value that is not a positive whole number "stops the container at startup instead of silently falling back to the default"), and it reinstates the failure this PR exists to fix, since a deployment that believes it set 450 still dies at 120. The repo's own compose files use that interpolation style (docker/docker-compose.yml:34), so HG_SERVER_STARTUP_TIMEOUT_S: ${SOME_VAR:-} yields an empty value whenever the host variable is unset.
Dropping the colon makes an empty value fail the guard while an unset one still defaults. Verified: unset gives -t 120, empty exits 1, 450 gives -t 450, and docker-entrypoint-test.sh still exits 0.
| SERVER_STARTUP_TIMEOUT_S="${HG_SERVER_STARTUP_TIMEOUT_S:-120}" | |
| SERVER_STARTUP_TIMEOUT_S="${HG_SERVER_STARTUP_TIMEOUT_S-120}" |
If you would rather keep empty meaning "unset", the README sentence needs to say so instead.
There was a problem hiding this comment.
Fixed in 6ca3df3, taking the suggestion. Confirmed the behaviour: ${v:-120} on an empty value yields 120, ${v-120} yields empty and so fails the guard, and an unset variable still yields 120 either way. docker/docker-compose.yml:34-35 uses the ${VAR:-} style, so this was reachable.
The README sentence now says an empty value is rejected too, rather than the reverse. Empty and whitespace-only are both in the rejected set in the test.
| start_calls_before_invalid_timeout=$(wc -l < "${TEST_HOME}/docker/start-hugegraph-args") | ||
| if ( | ||
| cd "${TEST_HOME}" | ||
| HG_SERVER_STARTUP_TIMEOUT_S=2m bash ./docker-entrypoint.sh | ||
| ); then | ||
| echo "invalid startup timeout unexpectedly succeeded" >&2 | ||
| exit 1 | ||
| fi | ||
| [[ "$(wc -l < "${TEST_HOME}/docker/start-hugegraph-args")" -eq \ | ||
| "${start_calls_before_invalid_timeout}" ]] |
There was a problem hiding this comment.
🧹 This block pins down that the server was not started, but not the ordering stated in the comment above the guard (Validated here so a typo fails before init-store runs). I copied the entrypoint, moved the whole validation block from line 101 down to just above the ./bin/start-hugegraph.sh call so it runs after all three init-store.sh call sites, and docker-entrypoint-test.sh still exited 0. The invalid run executes init-store.sh in that variant, and every init-store-calls assertion sits earlier in the file, so nothing observes it.
Snapshotting the init-store count alongside the start count closes the gap. Verified under bash 5.3: exit 0 against this PR, exit 1 against the relocated guard.
| start_calls_before_invalid_timeout=$(wc -l < "${TEST_HOME}/docker/start-hugegraph-args") | |
| if ( | |
| cd "${TEST_HOME}" | |
| HG_SERVER_STARTUP_TIMEOUT_S=2m bash ./docker-entrypoint.sh | |
| ); then | |
| echo "invalid startup timeout unexpectedly succeeded" >&2 | |
| exit 1 | |
| fi | |
| [[ "$(wc -l < "${TEST_HOME}/docker/start-hugegraph-args")" -eq \ | |
| "${start_calls_before_invalid_timeout}" ]] | |
| start_calls_before_invalid_timeout=$(wc -l < "${TEST_HOME}/docker/start-hugegraph-args") | |
| init_calls_before_invalid_timeout=$(wc -l < "${TEST_HOME}/docker/init-store-calls") | |
| if ( | |
| cd "${TEST_HOME}" | |
| HG_SERVER_STARTUP_TIMEOUT_S=2m bash ./docker-entrypoint.sh | |
| ); then | |
| echo "invalid startup timeout unexpectedly succeeded" >&2 | |
| exit 1 | |
| fi | |
| [[ "$(wc -l < "${TEST_HOME}/docker/start-hugegraph-args")" -eq \ | |
| "${start_calls_before_invalid_timeout}" ]] | |
| [[ "$(wc -l < "${TEST_HOME}/docker/init-store-calls")" -eq \ | |
| "${init_calls_before_invalid_timeout}" ]] |
There was a problem hiding this comment.
Fixed in 6ca3df3. Your relocated-guard variant is now caught: I reproduced it by moving the block below all three init-store.sh call sites, and the suite fails with startup timeout '' was rejected only after init-store ran.
One thing worth flagging from checking it. The bare [[ ]] assertion style does not fail on bash 3.2, still the /bin/bash of macOS: set -e there ignores a failing [[ ]], so my first run of your variant passed locally and only failed under bash 5.3. The new assertions use an explicit || { echo ...; exit 1; } so they fail on both. Verified under 3.2.57 and 5.3.15.
| docker run -itd --name=graph -p 8080:8080 -e HG_SERVER_STARTUP_TIMEOUT_S=450 hugegraph/hugegraph:1.7.0 | ||
| ``` | ||
|
|
||
| Raise it on slow or contended hosts, and wherever an orchestrator already owns |
There was a problem hiding this comment.
🧹 Section 6 directly above says docker ps shows real health status, but the health budget does not move with this variable. For the docker run example above, hugegraph-server/Dockerfile:74 and hugegraph-server/Dockerfile-hstore:76 set --start-period=90s --interval=15s --retries=3, so a container given 450s is marked unhealthy about two minutes in while the entrypoint is still legitimately waiting. The compose files replace that with their own (docker/docker-compose.yml:41-46: start_period: 60s, interval: 10s, retries: 30, so roughly 360s) and hubble gates on it through depends_on: condition: service_healthy, so a timeout raised past that budget blocks dependents rather than only mislabeling the container. Please add a sentence pointing at --health-start-period and the compose start_period.
There was a problem hiding this comment.
Fixed in 6ca3df3. Section 7 now says the health check keeps a budget of its own that this variable does not move: the images' --interval=15s --start-period=90s --retries=3 marks a container unhealthy around 135 seconds, raised with --health-start-period, while the Compose files substitute start_period: 60s, interval: 10s, retries: 30, roughly 360 seconds, which is what depends_on: condition: service_healthy gates Hubble on.
Review follow-up on the configurable Server startup timeout. The guard accepted any positive integer, but wait_for_startup computes its deadline as $((now_s + timeout_s)). Near the 64-bit ceiling that sum wraps negative, the wait loop exits before its first probe, and the container reports a startup timeout immediately: the failure the variable exists to prevent. The accepted range is now 1 to 86400 seconds, bounded first by a five-digit pattern so the comparison itself cannot overflow. The default also used ':-', which treats an explicitly empty value as unset and silently restores 120. Compose writes exactly that whenever an interpolated host variable is missing, so a deployment that believed it had set 450 still died at 120, and the README said such a value would stop the container. Plain '-' keeps the default for an unset variable and rejects an empty one. Tests now cover the accepted upper bound, an unset variable, and seven rejected values including empty, whitespace, 86401 and INT64_MAX, and assert that a rejected value runs neither start-hugegraph nor init-store, which pins the ordering the guard's comment claims. The new assertions exit explicitly rather than relying on set -e with [[ ]], which bash 3.2 ignores. The README documents the range and that the container health check keeps its own budget, which does not move with this variable.
2 participants
Purpose of the PR
docker-entrypoint.shrunsstart-hugegraph.shwith a literal-t 120, so a Server that needs longer than 120 seconds to answer on its REST port is ended by its own container. Under an orchestrator the startup budget belongs to the startup probe, and a probe cannot extend a process that terminates itself first: with the 450-second probe budget the Helm chart in #3132 configures, the process is gone at 120 seconds. Measured on 2026-09-01, 2 of 6 Server starts across two independent installs died this way and recovered only on container restart (log excerpts and the measurement setup are in the issue).Main Changes
docker-entrypoint.shreads the timeout fromHG_SERVER_STARTUP_TIMEOUT_S, defaulting to the current 120, so nothing changes for deployments that do not set it. Both Server images share this entrypoint, so one change covers the RocksDB and HStore images.wait_for_startup.docker-entrypoint-test.shnow records the arguments thestart-hugegraph.shstub receives and asserts the default-t 120, an explicit-t 450override, and rejection of2mbefore the server would have been started.hugegraph-server/hugegraph-dist/docker/README.md. The issue proposed the environment table indocker/README.md, but chore(docker): refactor docker-compose topologies with Hubble #3149 removed that table, so the Server docker README is the current home for entrypoint variables.Verifying these changes
bash hugegraph-server/hugegraph-dist/docker/docker-entrypoint-test.sh(already wired intoserver-ci.yml) covers the default, an override, and an invalid value.Does this PR potentially affect the following parts?
Documentation Status
Doc - Done