Skip to content

fix(mosaico): fail fast when mktemp cannot create the scratch dir#2557

Merged
ofir-frd merged 1 commit into
mainfrom
fix/mosaico-smoke-mktemp-guard
Jul 26, 2026
Merged

fix(mosaico): fail fast when mktemp cannot create the scratch dir#2557
ofir-frd merged 1 commit into
mainfrom
fix/mosaico-smoke-mktemp-guard

Conversation

@ofir-frd

Copy link
Copy Markdown
Collaborator

Follow-up to #2556, from a Qodo finding that landed after it merged.

docker/mosaico/smoke_test.sh runs under set -uo pipefail without -e, so a failed mktemp -d left TMPDIR_RUN empty and execution continued. Every subsequent write then targeted the filesystem root:

$ TMPDIR_RUN="$(mktemp -d /nonexistent/tmp.XXXX 2>/dev/null)"
$ echo "$TMPDIR_RUN/health.json"
/health.json

Guard the assignment so the script exits instead.

Verified: guard fires and exits 1 when mktemp fails; ./smoke_test.sh against the pinned image still reports FULL ROUND-TRIP PASSED.

The script runs under set -uo pipefail without -e, so a failed mktemp -d
left TMPDIR_RUN empty and execution continued: curl then wrote to
/health.json at the filesystem root. Verified before and after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the bug label Jul 26, 2026
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fail fast in mosaico smoke test when mktemp scratch dir creation fails

🐞 Bug fix 🕐 Less than 10 minutes

Grey Divider

AI Description

• Abort smoke test immediately if mktemp -d fails to create a scratch directory.
• Prevent accidental writes to filesystem root when TMPDIR_RUN is empty.
Diagram

graph TD
A["smoke_test.sh"] --> B{"mktemp -d ok?"} --> C["TMPDIR_RUN set"] --> D["write health.json"] --> E[("filesystem")]
B --> F["exit 1 (stderr)"]
subgraph Legend
direction LR
_script["Script"] ~~~ _dec{"Decision"} ~~~ _fs[("Filesystem")]
end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Enable `set -e` (errexit) for the script
  • ➕ Automatically fails on many command errors without per-command guards
  • ➕ Reduces risk of similar unchecked failures elsewhere
  • ➖ Can be behavior-changing for intentionally handled failures (requires careful audit)
  • ➖ May necessitate refactors (e.g., cmd || true, conditional checks) to preserve current control flow
2. Validate `TMPDIR_RUN` after assignment (non-empty + directory exists)
  • ➕ Directly asserts the invariant the rest of the script depends on
  • ➕ More robust if mktemp returns an unexpected value
  • ➖ Slightly more verbose than guarding mktemp directly
  • ➖ Still requires remembering to add checks for other critical variables

Recommendation: Keep the current approach: guard the mktemp -d assignment and exit with a clear error. It’s minimally invasive, preserves the existing set -uo pipefail behavior, and directly prevents the dangerous empty-variable path that could redirect writes to /.

Files changed (1) +1 / -1

Bug fix (1) +1 / -1
smoke_test.shAdd fail-fast guard for mktemp scratch directory creation +1/-1

Add fail-fast guard for mktemp scratch directory creation

• Wraps 'mktemp -d' with an explicit error path that prints a failure message to stderr and exits non-zero. Prevents subsequent commands from using an empty 'TMPDIR_RUN' and writing artifacts to the filesystem root.

docker/mosaico/smoke_test.sh

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Qodo Logo

@ofir-frd
ofir-frd merged commit 5fd8fa3 into main Jul 26, 2026
6 checks passed
@ofir-frd
ofir-frd deleted the fix/mosaico-smoke-mktemp-guard branch July 26, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant