CHEF-33330: Make knife ec import resilient to recoverable errors and normalize exit status

[sanghinitin]

Description

https://progresssoftware.atlassian.net/browse/CHEF-33330

knife ec import aborted mid-run on transient server/network failures, and the process exit status was inconsistent between verbose and non-verbose runs. This PR makes the import resilient to recoverable failures and normalizes the exit status.

lib/chef/knife/ec_import.rb

• Introduces RECOVERABLE_NETWORK_ERRORS (Net::HTTPClientException, Net::HTTPFatalError, Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::ETIMEDOUT) so 5xx Internal Server Error responses and dropped connections (Connection reset by peer) are logged and skipped instead of aborting the import.
• Applies that rescue list consistently to org verification, chef_fs_copy, cookbook freezing, and ACL updates.
• Narrowly rescues the known Digest::Base cannot be directly inherited RuntimeError raised by some Ruby builds during cookbook checksum computation; every other RuntimeError is re-raised.
• Surfaces a clear ui.error message for each skipped item, in addition to recording it in the error summary file.

lib/chef/knife/ec_error_handler.rb

• Tracks the number of recorded errors (has_errors?).
• Adds consistent_exit_status / override_exit_status so the command exits 1 whenever errors were recorded, and normalizes knife's non-zero exits (e.g. 100) to 1. Previously a run with -VVV and the same run without it reported different statuses.
• Skips the override under RSpec so the test suite's own exit status is untouched.

Motivation

A single transient Internal Server Error or Connection reset by peer forced a full re-run of a long import. Operators also could not rely on the exit code in automation because it varied with the verbosity flag.

Evidence

New unit coverage in spec/chef/knife/ec_error_handler_spec.rb for #has_errors? and #consistent_exit_status (clean exit, exit 100 normalized to 1, clean SystemExit, raw exception under -VVV, and both error/no-error contexts).

$ bundle exec rspec spec/chef/knife/ec_error_handler_spec.rb spec/chef/knife/ec_import_spec.rb
66 examples, 2 failures

The 2 failures are pre-existing on main and unrelated to this change — verified by running the same spec file in a clean main worktree, which reports the identical 4 examples, 2 failures at the same lines. They are caused by the Net::HTTPServerException / Net::HTTPClientException alias on the local Ruby/Chef version, not by this PR.

Behavioral evidence from the command:

• 5xx / connection-reset during an item copy -> <pattern> failed to copy: <message> is printed, the error is appended to the error summary file, and the import continues with the remaining items.
• After such a run the command exits 1 (instead of 0/100), with or without -VVV.

Checklist

• Commit is signed off (DCO)
• Unit tests added for the new behavior
• No change to the public CLI surface or option set
• Backwards compatible: only previously fatal, unrecoverable paths are affected

[Copilot]

🟡 Changes recommended

Moderate issues remain in verification error handling and exit-hook coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates knife ec import to continue through selected recoverable failures and normalize non-zero exit statuses.

Changes:

• Adds recoverable network and digest error handling.
• Tracks errors and standardizes exit status.
• Adds unit coverage for error tracking and exit behavior.

File summaries

File	Description
spec/chef/knife/ec_error_handler_spec.rb	Tests error tracking and exit-status normalization.
lib/chef/knife/ec_import.rb	Adds resilient handling for import failures.
lib/chef/knife/ec_error_handler.rb	Tracks errors and overrides inconsistent exit statuses.

Review details

Suppressed comments (3)

lib/chef/knife/ec_error_handler.rb:47

• The new exit hook is registered only when EcErrorHandler#initialize runs, but EcBase#knife_ec_error_handler is lazy and EcImport#run reaches it only inside recovery/error paths. If the import fails before any recoverable error is recorded—for example, warn_on_incorrect_clients_group raises while parsing a malformed group file—no hook runs and knife's 100/1 verbosity difference is unchanged. Register the handler at command start (or attach the exit normalization independently of error-file creation) if this normalization is meant to cover all command exits.

        at_exit do
          display(@err_file)
          override_exit_status
        end

lib/chef/knife/ec_import.rb:441

• A recoverable failure during the ACL GET/PUT is only written to the summary file and produces no UI message, so an import can silently skip a user's ACL and the operator cannot tell which item failed. Emit a contextual ui.error here before recording the exception, consistent with the copy failure path.

      rescue *RECOVERABLE_NETWORK_ERRORS => ex
        knife_ec_error_handler.add(ex)

lib/chef/knife/ec_import.rb:172

• This recoverable verification path returns false, and run treats false as proof that the organization does not exist (see line 141). A timeout, reset, or server failure can consequently report that a valid organization is missing, masking the actual failure; use a result that distinguishes not-found from verification failure or only print the not-found message for a 404.

      rescue *RECOVERABLE_NETWORK_ERRORS => ex
        ui.error "Failed to verify organization #{orgname}: #{ex.message}"
        knife_ec_error_handler.add(ex)
        false

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jashaik]

LGTM

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved recovery-path, reporting, coverage, and exit-status issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Update freeze failure spec to expect ui.error · New
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

Resolved since last review (2)

• Net::HTTPClientException is already caught by the rescue above, so a non-404 response of that…
• This new recoverable path is reported with ui.warn, unlike the requested error-level notification…

Previously missed (1)

In code that hasn't changed since last review

Surface recoverable ACL failures to the operator

lib/​chef/​knife/​ec_import.rb:438

A recoverable ACL failure is recorded here but never surfaced to the operator, unlike the organization, copy, and cookbook paths above. This leaves a skipped user/group ACL update invisible during the run; emit a ui.error message containing url and ex.message before recording the exception.

[Copilot]

Copilot review overview

🟡 Changes recommended

Four moderate review findings remain unresolved in lib/chef/knife/ec_import.rb.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1 · 1

Open (3)

• Update freeze failure spec to expect ui.error
• Surface ACL update failures through the UI · New
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Address the exit-status hook, misleading organization message, and missing targeted exception-handling coverage.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Update freeze failure spec to expect ui.error
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

Resolved since last review (1)

• Surface ACL update failures through the UI

Previously missed (1)

In code that hasn't changed since last review

Add coverage for recoverable network exception paths

lib/​chef/​knife/​ec_import.rb:43

The newly added recoverable exception cases are not exercised by the import specs: import_net_exception constructs Net::HTTPServerException (aliased to Net::HTTPClientException in this suite), so it does not verify Net::HTTPFatalError or the ECONNRESET/ECONNREFUSED/ETIMEDOUT paths. Add focused examples that assert each is logged and the import continues; otherwise a regression in this rescue list can make the original failure fatal again.

[Copilot]

Copilot review overview

🟡 Changes recommended

Exit normalization unintentionally affects backup and restore commands, and key recovery paths lack coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 · 1 · 1

Open (4)

• Limit exit status normalization to EcImport · New
• Update freeze failure spec to expect ui.error
• Test recovery for newly supported socket failures · New
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Lazy handler initialization prevents some non-zero exits from being normalized.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

• Update freeze failure spec to expect ui.error
• Test recovery for newly supported socket failures
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

Resolved since last review (1)

• Limit exit status normalization to EcImport

Previously missed (1)

In code that hasn't changed since last review

Register exit-status hook before lazy error handling

lib/​chef/​knife/​ec_error_handler.rb:48

The exit-status hook is only registered when EcErrorHandler is instantiated, but EcBase#knife_ec_error_handler creates it lazily and EcImport#run only accesses it after a recoverable error is recorded. Therefore an import that reaches Knife's non-zero exit (for example, 100) before any recorded error never installs this hook and remains unnormalized, despite consistent_exit_status handling the no-error case. Initialize/register the import handler at command startup (or install the status hook independently of lazy error logging), and cover the real lifecycle rather than only calling consistent_exit_status on a manually constructed handler.

[marcparadise]

This looks good, though I'd recommend maybe_exit_status as the name instead of consistent_exit_status, it follows a pattern established in chef, knife , and other projects where we use 'maybe' if something can return a value or nil.

[Copilot]

Copilot review overview

🟡 Changes recommended

Exit normalization is not installed for error-free handler paths and may disrupt non-import commands.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1 · 1

Open (3)

• Guard EcImport constant lookup in shared exit handler · New
• Test recovery for newly supported socket failures
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

Resolved since last review (1)

• Update freeze failure spec to expect ui.error

Previously missed (1)

In code that hasn't changed since last review

Initialize exit-status hook before imports can fail

lib/​chef/​knife/​ec_error_handler.rb:48

The exit-status hook is registered only when EcErrorHandler is instantiated, but EcBase#knife_ec_error_handler creates it lazily on the first recorded error. An import that fails before calling add—the exact no-recorded-error SystemExit(100) case covered by the unit helper—never installs this hook, so the real command still exits 100 instead of 1. Initialize the handler at import startup (or register the hook independently of error recording) so normalization applies to every import run.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Exit normalization is not installed before early failures, and some newly introduced recovery paths lack coverage.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Test recovery for newly supported socket failures
• The import specs cover the existing filesystem/JSON rescue cases, but do not exercise either side…

Resolved since last review (1)

• Guard EcImport constant lookup in shared exit handler

Previously missed (3)

In code that hasn't changed since last review

Initialize exit normalization before lazy error handler creation

lib/​chef/​knife/​ec_error_handler.rb:48

This exit hook is only registered after an EcErrorHandler instance is created, but EcBase#knife_ec_error_handler constructs it lazily and EcImport#run does not initialize it. Consequently, a Knife SystemExit(100) raised before any recoverable error is recorded has no hook and still exits 100, even though the new no-error spec expects normalization to 1. Initialize the handler when an import starts, or register the normalization independently of the lazy error-recording path.

Distinguish verification failures from confirmed missing organizations

lib/​chef/​knife/​ec_import.rb:169

For a newly handled 5xx or socket failure, returning false makes run also print Organization <name> does not exist. Skipping. The organization was not found to be absent—the check was inconclusive—so operators receive a contradictory diagnosis. Use a distinct result for verification failures (or handle them separately in the caller) and reserve false for a confirmed 404.

Test Net::HTTPFatalError handling for real 5xx responses

lib/​chef/​knife/​ec_import.rb:40

The specs labeled as 500 responses construct Net::HTTPServerException, which is an alias of Net::HTTPClientException in the environment noted in the PR, so they do not exercise this newly added Net::HTTPFatalError branch. Add a spec that directly raises Net::HTTPFatalError to verify real 5xx responses are recorded and skipped as intended.