Skip to content

Fix bash job scripts: require_command checks, curl temp file leak, batched MySQL inserts#187

Merged
sfreeman422 merged 2 commits intorefactor/python-to-bashfrom
copilot/sub-pr-186
Mar 22, 2026
Merged

Fix bash job scripts: require_command checks, curl temp file leak, batched MySQL inserts#187
sfreeman422 merged 2 commits intorefactor/python-to-bashfrom
copilot/sub-pr-186

Conversation

Copy link

Copilot AI commented Mar 22, 2026

Three unresolved code review issues in the bash job scripts: missing dependency validation in health-job, a temp file leak on curl failure in fun-fact-job, and per-row MySQL connections in pricing-job.

Changes

  • health-job/script.sh: Added require_command function (matching the pattern in other jobs) with checks for curl, grep, and mktemp in main().

  • fun-fact-job/script.sh: Fixed temp file leak in send_slack_message — curl now uses || true with an empty response_code guard that cleans up the temp file and returns early on failure, consistent with the health-job pattern.

  • pricing-job/script.sh: Replaced O(teams × items) individual mysql_query calls with a single batched invocation wrapped in a transaction:

sql_batch=""
for team_id in "${teams[@]}"; do
    for item_row in "${items[@]}"; do
        sql_batch+="INSERT INTO price(...) VALUES(...);"$'\n'
    done
done

mysql_query "START TRANSACTION;
${sql_batch}COMMIT;" || { echo 'Batch insert failed; transaction has been rolled back' >&2; return 1; }

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

…batch MySQL inserts

Co-authored-by: sfreeman422 <16405652+sfreeman422@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dev-chat/mocker/sessions/1c571688-b133-4945-9edc-a6d11e23df1d
Copilot AI changed the title [WIP] Refactor jobs to use bash instead of python for portability Fix bash job scripts: require_command checks, curl temp file leak, batched MySQL inserts Mar 22, 2026
Copilot AI requested a review from sfreeman422 March 22, 2026 15:02
@sfreeman422 sfreeman422 marked this pull request as ready for review March 22, 2026 15:02
Copilot AI review requested due to automatic review settings March 22, 2026 15:02
@sfreeman422 sfreeman422 merged commit 0ec8128 into refactor/python-to-bash Mar 22, 2026
7 checks passed
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens reliability and performance of the cron-executed bash job scripts under packages/jobs/ by adding dependency validation, preventing temp file leaks on curl failures, and reducing MySQL connection overhead via batched inserts.

Changes:

  • Add require_command checks in health-job for curl, grep, and mktemp.
  • Ensure fun-fact-job cleans up its curl response temp file even when curl fails.
  • Batch pricing-job MySQL inserts into a single transaction instead of per-row mysql_query calls.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/jobs/health-job/script.sh Adds require_command helper and validates required CLI dependencies in main().
packages/jobs/fun-fact-job/script.sh Prevents temp file leakage on curl failure by avoiding set -e exit and cleaning up on error.
packages/jobs/pricing-job/script.sh Switches to transactional, batched inserts to reduce per-row MySQL invocations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +144 to +145
mysql_query "START TRANSACTION;
${sql_batch}COMMIT;" || { echo 'Batch insert failed; transaction has been rolled back' >&2; return 1; }
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Building the entire INSERT workload into sql_batch and passing it as a single mysql -e argument can hit OS ARG_MAX/"Argument list too long" limits and/or MySQL max_allowed_packet as teams/items grow, causing the job to fail before any SQL runs. Consider streaming the SQL to mysql via stdin (or a temp file) and/or chunking into smaller batches (e.g., per team or fixed-size batches) to avoid unbounded query size and quadratic string-concatenation overhead.

Suggested change
mysql_query "START TRANSACTION;
${sql_batch}COMMIT;" || { echo 'Batch insert failed; transaction has been rolled back' >&2; return 1; }
{
echo 'START TRANSACTION;'
printf '%s' "${sql_batch}"
echo 'COMMIT;'
} | mysql --host="${MYSQL_HOST}" --user="${MYSQL_USER}" --password="${MYSQL_PASSWORD}" "${MYSQL_DATABASE}" \
|| { echo 'Batch insert failed; transaction has been rolled back' >&2; return 1; }

Copilot uses AI. Check for mistakes.
sfreeman422 added a commit that referenced this pull request Mar 22, 2026
* Refactored jobs to use bash instead of python for portability

* Update packages/jobs/pricing-job/script.sh

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix bash job scripts: require_command checks, curl temp file leak, batched MySQL inserts (#187)

* Initial plan

* Fix code review feedback: require_command checks, temp file cleanup, batch MySQL inserts

Co-authored-by: sfreeman422 <16405652+sfreeman422@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dev-chat/mocker/sessions/1c571688-b133-4945-9edc-a6d11e23df1d

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sfreeman422 <16405652+sfreeman422@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sfreeman422 <16405652+sfreeman422@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants