[fix] Retry registration on transient controller 5xx errors

[MichaelUray]

Problem

During device registration the agent calls check_header() before checking
the HTTP status code. A transient server-side error (HTTP 5xx — e.g. 502/503/
504 returned by a reverse proxy while the backend is restarting) carries no
X-Openwisp-Controller header, so check_header treats the response as coming
from the wrong server and calls exit 4.

Combined with procd's respawn limit (respawn ... 5), a short controller outage
that happens to overlap a (re)registration attempt makes procd give up
permanently after a few exits — leaving the agent dead until a manual
restart, even though the controller recovered seconds later.

The other three check_header call sites (get_checksum, update_info, status
report) already guard with an is_http_status ... 200 check before calling
check_header, so a 5xx there returns a retryable code. Only the registration
path was missing this guard.

Fix

Treat a 5xx registration response as a temporary failure (return 2, retryable)
before check_header runs. Non-5xx responses still go through check_header,
preserving the existing "wrong server" safety (exit 4) for a 2xx response that
lacks the controller header.

if head -n 1 "$REGISTRATION_PARAMETERS" | grep -qE " 5[0-9][0-9] "; then
    logger -s "Registration temporarily failed (controller returned 5xx), will retry: ..." \
        -t openwisp -p daemon.warning
    return 2
fi

The curl -i output that populates $REGISTRATION_PARAMETERS starts with the
status line (e.g. HTTP/1.1 502 Bad Gateway), so the 5[0-9][0-9] match is
consistent with how is_http_status reads the status elsewhere.

Testing

• shellcheck -s sh on openwisp.agent: clean.
• shfmt format check: clean.
• Focused behavioural check of the new branch against mocked curl -i status
  lines: 502/503/504 → retry (return 2); 200/201/403/404 → fall
  through to check_header unchanged.

This is a small, self-contained change to the registration retry logic; the
existing non-5xx behaviour is preserved.

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fe54d009-7310-4b39-b1d6-1de006326fd8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The registration flow now inspects the controller response status line before validating controller headers. HTTP 5xx responses are logged as warnings and return status 2, allowing a temporary retry. Non-5xx responses continue through the existing header validation and HTTP status handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (2 errors, 1 warning)

Check name	Status	Explanation	Resolution
Bug Fixes	❌ Error	The root cause is fixed in register(), but no regression test was added; the repo test suite has no registration coverage, so the bug repro/fail-when-patch-removed requirement isn’t met.	Add a deterministic registration regression test with mocked curl output that reproduces 5xx registration and fails if the new guard is removed; no Selenium test is needed for this non-UI bug.
General Rules	❌ Error	The new 5xx registration branch has no automated test coverage; the repo’s runtests only execute Lua tests and don’t exercise openwisp.agent.	Add a targeted shell/integration test that mocks curl -i and asserts 5xx returns 2 while 2xx/4xx still reach check_header.
Changes	⚠️ Warning	Non-trivial registration behavior changed, but the PR adds no tests for the new 5xx retry path.	Add focused tests for register(): 5xx should return 2 and non-5xx should keep existing check_header flow. Update docs/changelog only if this behavior is documented.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is descriptive, uses the required [fix] prefix, and matches the registration retry change.
Description check	✅ Passed	The description clearly explains the problem, fix, and testing, but it omits the checklist, issue reference, and screenshot sections from the template.
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.
Features	✅ Passed	PASS: This is a bugfix in registration retry logic, not a feature request, so the feature-specific issue/docs/UI-test checklist is not applicable.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@openwisp-config/files/openwisp.agent`:
- Around line 356-361: Update the registration failure logging in the 5xx branch
to strip the trailing carriage return from the first line before passing it to
logger, and quote REGISTRATION_PARAMETERS in the command substitution to
preserve the intended file path.

🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 19a6cba1-d5a9-4a17-b4c1-d10ad52283f7

📥 Commits

Reviewing files that changed from the base of the PR and between d28df21 and f3e54b3.

📒 Files selected for processing (1)

• openwisp-config/files/openwisp.agent

📜 Review details

⏰ Context from checks skipped due to timeout. (2)

• GitHub Check: QA-Checks and Tests
• GitHub Check: Kilo Code Review

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@openwisp-config/files/openwisp.agent`:
- Line 356: Update the status check in the REGISTRATION_PARAMETERS handling to
match 5xx codes followed by whitespace or end-of-line, rather than requiring a
trailing reason phrase. Preserve the existing transient-error path so HTTP/2
responses such as “503” are handled before check_header().

🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e80caed4-cbd7-47df-ac36-3945e26a70b0

📥 Commits

Reviewing files that changed from the base of the PR and between f3e54b3 and 87264ba.

📒 Files selected for processing (1)

• openwisp-config/files/openwisp.agent

📜 Review details

⏰ Context from checks skipped due to timeout. (2)

• GitHub Check: QA-Checks and Tests
• GitHub Check: Kilo Code Review

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 files)

• openwisp-config/files/openwisp.agent

---

_{Reviewed by hy3:free · Input: 38.4K · Output: 1.7K · Cached: 142.6K}

[MichaelUray]

@coderabbitai review

Both earlier review comments are addressed in the latest commit (28b3e21): the HTTP status line now has its trailing carriage return stripped and the variable is quoted, and the 5xx match is anchored to the status line (^HTTP/[0-9.]+ 5[0-9][0-9]( |$)) so it also matches HTTP/2 responses that omit the reason phrase (e.g. HTTP/2 503).

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- Changed files and latest PR diff ---'
git diff --stat HEAD^ HEAD
git diff -- openwisp-config/files/openwisp.agent

printf '%s\n' '--- Named commit, if present locally ---'
git show --format=fuller --no-ext-diff --unified=30 28b3e21 -- openwisp-config/files/openwisp.agent 2>/dev/null || true

printf '%s\n' '--- register() context ---'
rg -n -A120 -B15 '^[[:space:]]*register\s*\(\)' openwisp-config/files/openwisp.agent

Length of output: 11510

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- FETCH_COMMAND and proxy-related configuration/usages ---'
rg -n -i -C3 \
  '(FETCH_COMMAND|https?_proxy|proxy[[:space:]_=-]|--proxy|CURLOPT_PROXY)' \
  .

printf '%s\n' '--- Existing HTTP-status and response-header parsing ---'
rg -n -C3 \
  '(is_http_status[[:space:]]|head[[:space:]]+-n[[:space:]]+1.*REGISTRATION_PARAMETERS|check_header[[:space:]])' \
  openwisp-config/files/openwisp.agent

Length of output: 9605

---

@MichaelUray Re-reviewed the latest commit: the CRLF normalization, quoted file handling, and anchored HTTP/1.x/HTTP/2 5xx status-line match are correctly in place before check_header(). Non-5xx behavior remains unchanged. No further findings.

🐇 ✅

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.