SEP-2091: Guard the piped substitutions that abort MongoDB detection when mongod is down - #1552
marcuscruz-percona wants to merge 5 commits into
Conversation
…when mongod is down Under set -euo pipefail, an assignment from a pipeline whose first command legitimately exits non-zero kills the script at that line. pgrep does exactly that when no mongod runs, so the three MongoDB detection scripts died before their config-file and well-known-path fallbacks and before the not-found message. find and du behave the same on an unreadable directory, which also reached four multi-line assignments in two backup check scripts. Every such site now ends its pipeline with `|| true` inside the substitution, so the consumer still tests the empty value while the status can no longer reach set -e. The log extractor's not-found message names --log-file, matching the sibling scripts. A corpus-wide pytest check flags any assignment that reads from a pipeline in a script enabling both options. It classifies no commands: a genuinely safe site is silenced by a `# pipefail-safe: <reason>` marker on the line above, and a marker without a reason or without a site to justify is itself flagged. Behaviour tests run the three scripts under bash with pgrep and ps stubbed, for both the absent and the running process; they skip on a BSD userland.
There was a problem hiding this comment.
🟡 Changes recommended
The checker misclassifies declaration assignments, and two behavior tests remain dependent on host MongoDB configuration.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents diagnostic snippets from aborting when expected discovery pipelines return no results under pipefail.
Changes:
- Guards MongoDB and backup-script pipelines.
- Adds behavioral and corpus-wide regression tests.
- Updates messages, checksums, shared test utilities, and changelog.
File summaries
| File | Description |
|---|---|
snippets/mongodb_log_extractor.sh |
Guards process detection and improves failure output. |
snippets/mongodb_ftdc_collect.sh |
Guards process and FTDC discovery pipelines. |
snippets/mongodb_config_files.sh |
Guards process discovery. |
snippets/backup_failed_stale_check.sh |
Guards unreadable log-directory searches. |
snippets/backup_stale_upload_check.sh |
Guards unreadable upload-log searches. |
snippets/postgresql_query_tuning.sh |
Documents a safe pipeline exemption. |
snippets/postgresql_log_extractor.sh |
Documents a safe pipeline exemption. |
snippets/builtin-snippets.sha256 |
Refreshes modified snippet checksums. |
tests/app/sep/snippets/test_pipefail_guards.py |
Adds corpus-wide pipefail validation. |
tests/app/sep/snippets/test_mongodb_detection_scripts.py |
Adds executable MongoDB detection tests. |
tests/app/sep/snippets/test_frontmatter_authoring.py |
Reuses shared snippet enumeration. |
tests/app/sep/snippets/snippet_kit.py |
Centralizes snippet enumeration. |
changelog.d/SEP-2091.fixed.md |
Documents user-visible fixes. |
Review details
Suppressed comments (10)
tests/app/sep/snippets/test_mongodb_detection_scripts.py:197
- Pin UTF-8 for this text fixture so its encoding does not depend on the host locale.
conf.write_text("storage:\n dbPath: /tmp/data\n")
tests/app/sep/snippets/test_mongodb_detection_scripts.py:228
- Pin UTF-8 for this text fixture so its encoding does not depend on the host locale.
path.write_text(
"2023-10-27T15:29:00.000+00:00 I CONTROL inside the window\n"
"2023-10-27T15:40:00.000+00:00 I CONTROL outside the window\n"
)
tests/app/sep/snippets/test_mongodb_detection_scripts.py:254
- Pin UTF-8 for this text fixture so its encoding does not depend on the host locale.
conf.write_text(f"systemLog:\n destination: file\n path: {log}\n")
tests/app/sep/snippets/test_pipefail_guards.py:380
- This class docstring opens with a noun phrase rather than an imperative verb. Rewrite it to state what the class verifies.
"""Every assignment shape the corpus writes is found, and only those."""
tests/app/sep/snippets/test_pipefail_guards.py:465
- This class docstring does not begin in imperative mood. Rewrite it to state the verification action.
"""Whatever bash treats as guarded, the check treats as guarded."""
tests/app/sep/snippets/test_pipefail_guards.py:529
- This class docstring opens with a noun phrase rather than an imperative verb. Rewrite it to state the verification action.
"""A safe site is silenced by an explicit, reasoned marker, and only that."""
tests/app/sep/snippets/test_pipefail_guards.py:597
- This class docstring opens with a noun phrase rather than an imperative verb. Rewrite it to state the verification action.
"""The exact lines that motivated this check classify the way the check promises."""
tests/app/sep/snippets/test_pipefail_guards.py:628
- This class docstring opens with a noun phrase rather than an imperative verb. Rewrite it to state the verification action.
"""Every builtin script honours the guard contract."""
tests/app/sep/snippets/test_mongodb_detection_scripts.py:212
- This class docstring starts with a script name rather than an imperative verb. Rewrite it to describe what the class exercises.
"""``mongodb_log_extractor.sh`` reads the log path from the process, then the config."""
tests/app/sep/snippets/test_mongodb_detection_scripts.py:288
- This class docstring starts with a script name rather than an imperative verb. Rewrite it to describe what the class exercises.
"""``mongodb_ftdc_collect.sh`` reads the data directory from the process, then the config."""
- Files reviewed: 13/13 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…r tests Stop flagging declaration assignments. `local x=$(a | b)`, and the same with `declare`, `readonly`, `export` or `typeset`, exits with the builtin's status rather than the substitution's, so `set -e` never sees the pipeline fail; the shape hides an error instead of propagating one, which is shellcheck's SC2155 and not this check's contract. No builtin script writes one today, so the corpus verdict is unchanged: the same twelve sites are still found before the fix. Widen the host preconditions on the nothing-found behaviour tests. The log extractor and the FTDC collector both read a well-known config file before falling back to well-known paths, so a runner carrying /etc/mongod.conf could resolve a log or data directory the test never staged and fail on that host. Scope the changelog's option claim to the two scripts that take options, and pin UTF-8 on the text reads and writes so a runner's locale cannot change what the fixtures contain.
marcuscruz-percona
added
the
qa passed
Acceptance criteria 5 and 6 conflict — reviewer noteRaising this so the checker's behaviour on the pre-fix corpus isn't read as a miss. Also filed on SEP-2091. Criterion 5 pins the classification contract: an unrecognised command is flagged rather than exempted (fail closed), and a genuinely safe site is silenced by an explicit in-file exemption marker carrying a reason — not by a command allowlist baked into the checker. Criterion 6 asks that the same check, run against the corpus before the fix, flags all six abort-capable sites and does not flag either structurally-safe site ( Before the fix neither safe site carries a marker — the markers are part of the fix. A checker obeying criterion 5 therefore has to flag them. The only way not to would be to recognise This PR follows criterion 5. Against the pre-fix corpus the check reports 12 sites:
The four After the fix the corpus is clean: six sites guarded, two safe sites carrying Suggested resolution: read criterion 6 as calibrating the positive half only — the check must flag every abort-capable site the ticket names — and treat its negative half as met by the marker mechanism rather than by pre-fix silence. Taking the negative half literally means criterion 5 gives up failing closed, and the class of miss that hid the |
Coverage report
This PR does not seem to contain any modification to coverable code. |
…corpus walk The nothing-found and running-process branches were covered for all three scripts, but the config-file step only through a running mongod. That left the fallback the original defect skipped asserted in one script out of three. The well-known locations are absolute paths under /etc a test cannot create, so the step is now driven through a copy of the script whose first candidate names a staged file. Only that one string changes and the substitution is asserted, so a script that stops consulting the well-known location fails the test rather than silently covering nothing. Move the snippets-directory walk out of snippet_kit into snippet_corpus. The kit holds the database seed helpers; enumerating the corpus is an unrelated reason for that module to change. The new module also exports the shell-only subset, so the guard check parametrises over the files it actually reads instead of skipping the rest inside the test body, and a non-empty assertion keeps the gate from passing by covering nothing. Collapse the two walkers in the guard check onto one _skip_expansion helper. Both re-implemented the same three branches on the same characters; inside double quotes a $( still opens a substitution, so the shape belongs in one place. Add the missing param and return descriptions on the process-stub helpers.
Summary
set -euo pipefailan assignment likeMONGOD_PID=$(pgrep -x mongod 2> /dev/null | head -1)inheritspgrep's exit 1 and kills the script on the spot — before the config-file fallback, the well-known-path fallback, and the not-found message it was meant to lead into. Six sites acrossmongodb_log_extractor.sh,mongodb_config_files.shandmongodb_ftdc_collect.shnow end the pipeline with|| trueinside the substitution, so only the substitution's status is swallowed and every consumer still tests the value it received. Four multi-linefind | sort | tail | cutblocks inbackup_failed_stale_check.shandbackup_stale_upload_check.share the same class with a narrower trigger (an unreadable log directory) and are guarded the same way.Error: MongoDB log file not found at '<path>'. Pass --log-file., matching the shapepostgresql_config_files.shalready uses.mongodb_ftdc_collect.shalready named--data-dir;mongodb_config_files.shtakes no arguments, so its message names only what was not found.tests/app/sep/snippets/test_pipefail_guards.pyflags any assignment reading from a command substitution that contains a pipeline, in any script declaring bothset -eandpipefail, unless bash itself treats the site as guarded. It classifies no commands: a list of "commands known to fail" is exactly what missed thedu -shsite this check was written for, so an unrecognised command is flagged, not exempted. A genuinely safe site is silenced only by a# pipefail-safe: <reason>marker on the line above, and a marker with no reason — or one that no longer precedes a site the check would flag — is itself an offence, so a marker cannot outlive the line it was written for. Two such markers are added, on theprintf | sedandecho | trsites in the PostgreSQL scripts.snippets/**is already in thepythonpath filter, so the check runs in CI on any snippet change.Behaviour coverage lives in
tests/app/sep/snippets/test_mongodb_detection_scripts.py: the real scripts run underbashwithpgrepandpsstubbed onPATH, staging both the absent and the running process. It needs GNUgetoptand GNUdate, so it skips on a BSD userland and runs in the Linux matrix.Two known limits, both pre-existing shapes the guards preserve rather than change:
findfails on an unreadablediagnostic.data,FTDC_FILESis empty for the same reason an empty directory leaves it empty, so the script printsNo FTDC files foundand exits 0. Distinguishing the two would mean splitting the assignment from the pipeline; the ticket accepts the simpler guard here.TOTAL_SIZEandFILE_COUNTinterpolate empty into oneechoifdu/findfail. Neither value is used in arithmetic.The checker recognises assignments at the start of a line whose value begins with
$(. Backticks, a substitution nested inside a parameter expansion, and a mid-line assignment after a;are outside its syntactic scope — none occurs in the corpus today, and a site written that way would go unflagged rather than misreported.Jira: SEP-2091
Tested
Automated:
pytest tests/app/sep/snippets/— 1638 passed, 7 skipped. Before the script fixes the corpus check failed on 7 files / 12 sites and 4 of the behaviour tests failed; both go green with the fixes in place.Manual smoke tests on a Linux host, still to confirm on review:
bash snippets/mongodb_config_files.shreaches=== Done ===and exits 0bash snippets/mongodb_log_extractor.sh --time 2023-10-27T15:30:00 --minutes 3 --output stdoutprints the not-found line naming--log-filebash snippets/mongodb_ftdc_collect.sh --dest /tmp/ftdc-probenames--data-dir/etc/mongod.confpresent, the log extractor reportsDetected log file from config— the fallback the original defect skippedChecklist
make test) — ran thetests/app/sep/snippets/suite, not the full matrixmake run-pre-commit)make makemigrations) — no model changeschangelog.d/if the change is user-facing (make changelog-add), or confirmed N/A