(Mirror) cli : Use acquire/release semantics for stopping logic

[ngxson]

@coderabbitai use mirror preset for review

mirror ggml-org#21822

Summary by CodeRabbit

• Bug Fixes

  • Improved Ctrl+C/TTERM interrupt responsiveness and cancellation reliability
  • Double-interrupt now exits immediately with cleaner terminal handling
  • Cancellation/stop state is now correctly reset after generation or interactive cancellation

• Performance

  • Simplified streaming generation loop stopping logic for more stable output

• Other

  • Updated model download and “model required” validation behavior, including server mode when no model path is provided
  • Model download flow can now report via an optional download callback

[coderabbitai]

📝 Walkthrough

Walkthrough

The common_params_handle_models function signature was extended with an optional download callback parameter, which is now wired into download options when provided. Model download skip logic was broadened to include server mode, and the required-model validation no longer exempts server mode.

Changes

Model Parameter Callback & Validation Logic

Layer / File(s)	Summary
Callback parameter and include common/arg.h	Added download.h inclusion and extended common_params_handle_models declaration with an optional common_download_callback * callback parameter (defaulting to nullptr).
Callback assignment in implementation common/arg.cpp	Updated function signature at call site and, when callback is non-null, assigned it into common_download_opts::callback for downstream use.
Download skip and model validation logic common/arg.cpp	Broadened skip_model_download to exclude both LLAMA_EXAMPLE_SERVER and LLAMA_EXAMPLE_EXPORT_GRAPH_OPS modes; removed the server exemption from required-model validation, so server mode now enforces the same model-present check as other modes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A callback hops through parameters wide,
The downloads skip where servers reside,
Now models are checked with equal care,
Validation's fair everywhere! 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete and missing critical information from the template. It lacks an Overview section explaining what the PR does and why, and does not include the required Requirements section with contributing guidelines acknowledgment.	Add an Overview section summarizing the changes and rationale. Include the complete Requirements section with checkboxes confirming agreement with contributing guidelines and AI usage disclosure.
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: updating the CLI to use acquire/release semantics for stopping logic, which aligns with the core modifications in tools/cli/cli.cpp.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tools/cli/cli.cpp`:
- Around line 46-53: The second-interrupt handler uses non-async-signal-safe
functions (fprintf, fflush, std::exit); change it to only call async-signal-safe
primitives: use g_is_interrupted.exchange as-is, then write(2) to STDOUT_FILENO
(or use a literal escape sequence buffer) to clear colors, and call _exit(130)
instead of std::exit; update the block that currently references
g_is_interrupted and the fprintf/fflush/std::exit sequence to use write(...) and
_exit(...) so the signal handler is POSIX async-signal-safe.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b0bbdb66-604d-4985-9d54-54b426872436

📥 Commits

Reviewing files that changed from the base of the PR and between 707c0b7 and 3cdc177.

📒 Files selected for processing (1)

• tools/cli/cli.cpp

[coderabbitai]

🧹 Nitpick comments (1)

common/arg.cpp (1)

601-601: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Stale comment: server mode is already excluded by the enclosing block.

The comment "model is required (except for server)" is misleading because this validation runs inside the if (!skip_model_download) block, which already excludes LLAMA_EXAMPLE_SERVER. The comment suggests there's special server handling in the condition itself, but server mode never reaches this code path.

Consider updating the comment to reflect the actual logic, e.g., removing "(except for server)" since server is handled by the outer block.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@common/arg.cpp` at line 601, The comment at line 601 stating "model is
required (except for server)" is misleading because server mode is already
excluded by the outer if (!skip_model_download) block. Update the comment to
remove the "(except for server)" portion since the server mode never reaches
this code path due to the enclosing block's logic. The comment should reflect
that model validation only applies to the code paths that pass the outer
condition.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@common/arg.cpp`:
- Line 601: The comment at line 601 stating "model is required (except for
server)" is misleading because server mode is already excluded by the outer if
(!skip_model_download) block. Update the comment to remove the "(except for
server)" portion since the server mode never reaches this code path due to the
enclosing block's logic. The comment should reflect that model validation only
applies to the code paths that pass the outer condition.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 55523c74-0d14-4dd8-b1fc-b3bfabc8e8aa

📥 Commits

Reviewing files that changed from the base of the PR and between 07b8b2d and 39af19a.

📒 Files selected for processing (2)

• common/arg.cpp
• common/arg.h