Fix stop-edge.sh not stopping its own process when IOTDB_HOME is set - #18546
Fix stop-edge.sh not stopping its own process when IOTDB_HOME is set#18546PDGGK wants to merge 4 commits into
Conversation
start-edge.sh honours an IOTDB_HOME from the environment and passes that value to the JVM as -DIOTDB_HOME=..., while stop-edge.sh recomputed IOTDB_HOME from its own location and matched the command line against that string. When the two differ -- for example a versioned install reached through a symlink -- the stop declined to act on its own process, exited 0 and removed the PID file, so a service manager saw a successful stop while the process kept running. stop-edge.sh now honours IOTDB_HOME the same way start-edge.sh does, and falls back to comparing resolved paths when the literal match does not hit. The original literal comparison is tried first, so any command line that matched before still matches.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
The reported symlink/real-path mismatch is fixed, but I reproduced two cases where the new fallback stops a process from a different installation. The details are in the inline comments.
I suggest retaining Edge's PID-file and installation-identity checks rather than copying the port-based lookup from stop-datanode.sh / stop-confignode.sh: changing the lookup port before stopping can make those scripts miss the old process. Start and stop should use consistent rules for IOTDB_HOME and configuration directories.
There is also a pre-existing lifecycle concern: the PID file is removed before stop_edge_process succeeds. Retaining it on shutdown failure and returning a nonzero status for genuine stop failures would avoid losing track of a running process. This can be handled separately if it is outside this fix.
Verification: I ran the unchanged base (1106d85) and PR (73a96dc) stop scripts on macOS against isolated minimal JVM processes named org.apache.iotdb.edge.EdgeNode, with process inspection and signals restricted to the test-owned processes. In both cross-installation cases, the base script leaves the unrelated process alive and the PR script terminates it. The ordinary symlink-to-real-path case behaves as intended after this change. These were script-level checks, not full IoTDB server tests.
| local home="${command_line#*-DIOTDB_HOME=}" | ||
| home="${home%% -D*}" | ||
| [ -n "$home" ] && [ "$home" != "$command_line" ] || return 1 | ||
| [ "$(resolve_home "$home")" = "${IOTDB_HOME_RESOLVED}" ] |
There was a problem hiding this comment.
[P1] Resolve relative process homes using the process's working directory
start-edge.sh accepts IOTDB_HOME=. and passes it unchanged to the JVM. If an instance was started this way in /srv/edge-a, running /srv/edge-b/sbin/stop-edge.sh from /srv/edge-b with IOTDB_HOME unset resolves the process's . to B. Consequently, find_edge_processes considers A a match and terminates it, even if B has no PID file or running instance. The base script rejects this process; the new fallback accepts it.
Please normalize the home to a physical absolute path at launch, or resolve relative process values against that process's working directory; do not resolve them against the stopping shell's working directory.
There was a problem hiding this comment.
You are right, and thank you for running it. I reproduced it before changing
anything: a synthetic process with cwd in edge-a and -DIOTDB_HOME=. on its
command line, stopped from edge-b -- the base script correctly declines it, my
fallback resolved the . against the stopping shell and terminated it.
I have taken the first of your two suggestions. start-edge.sh now normalises
IOTDB_HOME to a physical absolute path before exporting it, so the value handed
to the JVM identifies the installation on its own:
started through the symlink, before -DIOTDB_HOME=$ROOT/iotdb
started through the symlink, now -DIOTDB_HOME=$ROOT/apache-iotdb-2.0.11-SNAPSHOT-edge-bin
I kept an absolute-path guard in the stop-edge.sh fallback as well, rather than
relying only on the launcher. A process started by an earlier start-edge.sh
during an upgrade can still carry a relative value, and for those the fallback now
declines instead of resolving against our own directory -- the same outcome as the
base script. That leaves resolving against the process's working directory
unnecessary, which I was reluctant to do since it needs lsof or /proc and a
wrong answer there puts us back to stopping the wrong process.
Two things about the normalisation that I would rather state than leave to be
found. It changes the path string that appears in ps, in the console message and
in the logs -- on this machine /tmp becomes /private/tmp -- which is the point
of the change, but anything grepping for the old string is affected. And if the
cd -P cannot run, IOTDB_HOME keeps the value it had, which for the unset case
is still relative; that instance then falls to the literal comparison in
stop-edge.sh, which is today's behaviour. A fallback to a logical cd does not
help here: both still have to reach the directory, so it fails on the same inputs.
Both of your cases are in the regression set now, with the version you reviewed as
a control so they are shown to fail without the change.
| # Resolve symlinks and relative segments so that the same installation reached | ||
| # through a different path still compares equal. | ||
| resolve_home() { | ||
| (cd "$1" 2>/dev/null && pwd -P) || printf '%s' "$1" |
There was a problem hiding this comment.
[P2] Use physical cd semantics before resolving ..
pwd -P only resolves the directory reached by cd; plain cd has already collapsed .. logically. With /x/link pointing to /y/sub, a process home /x/link/../edge accesses /y/edge, but this helper returns /x/edge when that directory exists. Consequently, stopping /x/edge can terminate the /y/edge instance, while stopping /y/edge can miss it. This also reproduces with an absolute IOTDB_HOME, independently of the relative-path issue.
Use cd -P -- "$1" before pwd -P and cover a symlink followed by .. in the regression cases.
There was a problem hiding this comment.
Confirmed, and thank you -- this one reproduces as pure path arithmetic, without
needing a process. With x/link pointing at y/sub and a process home of
x/link/../edge, the physical target is y/edge, and the old helper returned
x/edge:
process home $ROOT/x/link/../edge
physically $ROOT/y/edge
old resolve_home $ROOT/x/edge <- wrong installation
cd -P version $ROOT/y/edge
Fixed in 739dae2 with cd -P -- "$1", and both of your cases are in the
regression set now:
scenario base previous now
-------------------------------------------------- ------ -------- ------
[P1] -DIOTDB_HOME=. in A, stopped from B keep stopped keep
[P2] home x/link/../edge, stopped from x/edge keep stopped keep
[P2] home x/link/../edge, stopped from y/edge - - stopped
symlink home, stopped from the real path keep stopped stopped
The "previous" column is the version you reviewed, so the two cases do fail
without the change rather than passing for an unrelated reason. The four
start/stop environment combinations from the PR description still behave the
same, checked against real Edge processes with 10710 confirmed listening before
each stop.
One limit of the fallback that I should state rather than leave for you to find:
it locates the end of the value by the next -D, which start-edge.sh always
emits after -DIOTDB_HOME. A hand-built command line that ends with
-DIOTDB_HOME=<symlink> is not matched by the fallback. The failure direction is
the safe one -- the extraction keeps the trailing arguments, the resolution fails,
and the process is simply not matched -- so that case behaves as it does today. I have noted it in a comment above the
fallback.
On the PID file being removed before stop_edge_process succeeds: I agree, and I
will open that separately as you suggested, since it changes the exit-code
contract and is worth reviewing on its own.
Two problems in the previous version, both of which could stop a process belonging to a different installation. The helper used a plain cd before pwd -P. Because cd collapses ".." logically, a process home such as <symlink>/../edge resolved against the symlink's parent rather than its target, so stopping one installation could terminate another. It now uses cd -P. The fallback also resolved relative process homes, such as the "." that start-edge.sh accepts and passes through unchanged, against the working directory of the stopping shell rather than that of the started process. A process started with IOTDB_HOME=. in one directory could therefore be matched and stopped from an unrelated installation. The fallback now only considers absolute values and leaves relative ones to the literal comparison, which matches the previous behaviour for them.
Taking the first of the two options suggested in review. The value start-edge.sh hands to the JVM as -DIOTDB_HOME is what stop-edge.sh matches on, so it should identify the installation on its own rather than depend on the path used to launch. Normalising it at that point removes the relative and symlinked forms at the source instead of resolving them later. The absolute-path guard in stop-edge.sh is kept as well, so a process started by an earlier start-edge.sh, whose command line may still carry a relative value, is left alone rather than matched against the stopping shell's directory.
cd succeeds on an empty argument and returns the caller's working directory, so dropping the emptiness check would reopen the same class of mismatch as a relative value. Comment only.
|
One more data point on the convention, from the Edge tools added in #18552 on the
IOTDB_HOME="${IOTDB_HOME:-$(cd "$(dirname "$0")"/../.. && pwd)}"
IOTDB_HOME="$(cd "$IOTDB_HOME" && pwd -P)"That is: honour an So of the four Edge shell entry points, three take the value they are given and |
2 participants
Description
Fixes #18545.
start-edge.shhonours anIOTDB_HOMEtaken from the environment and passes thatvalue to the JVM as
-DIOTDB_HOME=....stop-edge.shrecomputedIOTDB_HOMEfrom its own location and matched the process command line against that string,
so when the two differed it declined to act on its own process, exited 0, and
removed the PID file. A service manager saw a successful stop while the process
kept running; the following
ExecStartthen found the ports occupied and alsoexited 0, so a configuration change silently never took effect.
The change
stop-edge.shnow honoursIOTDB_HOMEthe same waystart-edge.shdoes, andadds a resolved-path comparison as a fallback:
line that matched before still matches;
-DIOTDB_HOME=and compare it with
IOTDB_HOMEafter resolving both withcd ... && pwd -P.The change is additive: it can recognise more processes than before, never fewer.
Behavioural note
This widens the identity test from "the same path string" to "the same resolved
directory", so two different symlinks pointing at one installation now count as
the same installation. That seems to be the intent of the check, but it is a
semantic change and worth a second opinion.
Verification
Four start/stop environment combinations, each from a fresh extraction of the
packaged Edge archive, with
$ROOT/iotdbsymlinked to the install directory.Every row was confirmed to have
10710actually listening before the stop wasissued, so a row cannot pass by the process having died on its own:
The two rows that fail before the change are exactly the ones where
start-edge.shwas given anIOTDB_HOMEthat differs from the path the scriptderives.
Also re-ran the restart sequence from the issue after the change: the stop now
terminates the process, the following start comes up on the edited
dn_rpc_port,and the old port is released.
Built and run on macOS with JDK 22, from the packaged
apache-iotdb-2.0.11-SNAPSHOT-edge-bin.zip.Not changed
scripts/sbin/windows/stop-edge.bat:23setsIOTDB_HOMEunconditionally whilestart-edge.bat:65honours the environment, which is the same shape. I have noWindows machine and did not run it, so I have left it alone rather than propose an
untested change.
Update after review
start-edge.shnow normalisesIOTDB_HOMEto a physical absolute path beforeexporting it, taking the first of the two options suggested in review. The
normalisation happens before every derived variable (
IOTDB_CONF,IOTDB_DATA_HOME,IOTDB_LOG_DIR,CONFIGNODE_*), before-DIOTDB_HOMEisbuilt, and before the PID file path.
stop-edge.shkeeps the absolute-path guard in its fallback. That is not aduplicate of the launcher change: a process started by an earlier
start-edge.shduring an upgrade can still carry a relative value on its command line, and those
should be left to the literal comparison rather than resolved against the stopping
shell's directory.
Observable change
Normalising the path alters the string that appears in
psoutput, in thestart-up message and in the logs, so a monitor matching on the previous string
will need updating. The file locations themselves are unchanged, since the
symlink and the physical path resolve to the same directory.
The two problems raised in review, both of which could stop a process from a
different installation, are covered as regression cases: