Bug
demo_build.sh extracts every per-row column via the pattern
grep "$IPADDRESS" "$GITDEMOFARMMAP" | tr -d '\n' | cut -f N
grep substring-matches — so $IPADDRESS=two matches both the two row AND the two_a row. The downstream tr -d '\n' | cut -f N then concatenates both rows into one line and pulls field N from the joined text, which is usually the wrong value.
Surfaced during rabbit review of PR #152, but the issue is long-standing — I noted it during PR #147 (Phase 1) work and the test harness explicitly carries single-row fixtures expressly to work around the ambiguity (see tools/build-tests/test.sh:14-17: "Should contain ONLY the row(s) needed for this scenario to avoid the grep '$IPADDRESS' substring-match ambiguity").
In production, this affects any row whose docker_number is a substring of another row's. Currently visible cases:
two ⊂ two_a
four ⊂ four_a ⊂ four_b
five ⊂ five_a ⊂ five_b
- etc.
Proposed fix
Rabbit's suggestion (from PR #152 review at L426):
map_row="$(awk -F $'\t' -v ip="$IPADDRESS" '$1 == ip { print; exit }' "$GITDEMOFARMMAP")"
if [ -z "$map_row" ]; then
echo "ERROR: no demo-map entry for $IPADDRESS" | tee -a "$LOG" >&2
exit 1
fi
# then each column extraction reads from $map_row instead of re-grepping
OPENEMRREPO="$(printf '%s\n' "$map_row" | cut -f 2)"
Benefits:
- Anchors the match on field 1 (
$1 == ip) — exact match, no substring ambiguity.
exit after first match — even if there are duplicate keys (shouldn't be), only the first is used.
- Adds an explicit error path if the row isn't found (currently the script silently runs with empty column values).
- One pass over the map file per iteration instead of one per column extraction (~10 grep invocations per demo iteration today).
Why this matters
This is the only place demo_build.sh's per-row dispatch can silently pick the wrong values. Today most rows happen to escape it (their docker_number isn't a substring of another active row), but adding a cluster like two_b would silently break the two demo even though two_b is the row you intended to add.
The test harness's "single-row fixtures" workaround means our regression suite can't catch a regression that this fix would close.
Severity
Major. Latent bug, currently doesn't bite because of dataset coincidence, but breaks unexpectedly when new clusters are added.
Source
Rabbit review on PR #152 (L426). Same bug I noted during PR #147 work as "the ip-map ambiguity bug" but explicitly out-of-scope for that PR.
Out of scope for this issue
- Refactoring the loop's other per-iteration state derivation. The fix should be the minimum needed: one map lookup at iteration start, all column extractions read from the captured row.
Bug
demo_build.shextracts every per-row column via the patterngrepsubstring-matches — so$IPADDRESS=twomatches both thetworow AND thetwo_arow. The downstreamtr -d '\n' | cut -f Nthen concatenates both rows into one line and pulls field N from the joined text, which is usually the wrong value.Surfaced during rabbit review of PR #152, but the issue is long-standing — I noted it during PR #147 (Phase 1) work and the test harness explicitly carries single-row fixtures expressly to work around the ambiguity (see
tools/build-tests/test.sh:14-17: "Should contain ONLY the row(s) needed for this scenario to avoid the grep '$IPADDRESS' substring-match ambiguity").In production, this affects any row whose
docker_numberis a substring of another row's. Currently visible cases:two⊂two_afour⊂four_a⊂four_bfive⊂five_a⊂five_bProposed fix
Rabbit's suggestion (from PR #152 review at
L426):Benefits:
$1 == ip) — exact match, no substring ambiguity.exitafter first match — even if there are duplicate keys (shouldn't be), only the first is used.Why this matters
This is the only place demo_build.sh's per-row dispatch can silently pick the wrong values. Today most rows happen to escape it (their docker_number isn't a substring of another active row), but adding a cluster like
two_bwould silently break thetwodemo even thoughtwo_bis the row you intended to add.The test harness's "single-row fixtures" workaround means our regression suite can't catch a regression that this fix would close.
Severity
Major. Latent bug, currently doesn't bite because of dataset coincidence, but breaks unexpectedly when new clusters are added.
Source
Rabbit review on PR #152 (
L426). Same bug I noted during PR #147 work as "the ip-map ambiguity bug" but explicitly out-of-scope for that PR.Out of scope for this issue