Fail upgrade tool on download and write errors

[mattn]

A write error while extracting the amalgamation was masked by scanner.Err() returning nil, so a truncated sqlite3-binding.c was reported as successfully extracted (and then committed by upgrade.sh). Fail on write and close errors, check the HTTP status of both downloads, and only remove the merge source file after a successful merge.

Summary by CodeRabbit

• Bug Fixes
  • Improved download validation by checking server responses before parsing and reading.
  • Prevented source files from being removed if the merge/write process doesn’t fully succeed.
  • Enhanced error handling during amalgamation extraction, including stricter handling of write and close failures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 42ec74b4-8d37-49b9-b5a2-e3b63b440149

📥 Commits

Reviewing files that changed from the base of the PR and between 322330f and 6ebb665.

📒 Files selected for processing (1)

• upgrade/upgrade.go

🚧 Files skipped from review as they are similar to previous changes (1)

• upgrade/upgrade.go

---

📝 Walkthrough

Walkthrough

The upgrade utility validates HTTP responses, removes merged source files only after successful destination writes and closes, and propagates extraction write and close failures.

Changes

Upgrade error handling

Layer / File(s)	Summary
HTTP download validation upgrade/upgrade.go	Both download requests require http.StatusOK; the second response body is closed before processing completes.
Merged source cleanup upgrade/upgrade.go	The source file is removed only after the destination merge and close succeed, with close and removal errors returned.
Amalgamation extraction errors upgrade/upgrade.go	Extraction now handles fmt.Fprintln failures and reports output-file close errors.

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

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: the upgrade tool now fails on download, write, and close errors.
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.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-upgrade-error-handling

---

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 `@upgrade/upgrade.go`:
- Around line 104-106: Update mergeFile so fdst is explicitly closed before
os.Remove(src), returning any close error and only deleting the source after a
successful close. Replace the deferred-only close handling with logic that also
preserves and propagates fdst.Close errors on all earlier return paths.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c7558d61-b0b4-438a-8644-b76d972b6dfc

📥 Commits

Reviewing files that changed from the base of the PR and between fadf4a7 and 322330f.

📒 Files selected for processing (1)

• upgrade/upgrade.go

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Close the destination before removing the source.

fdst.Close() remains deferred and its error is ignored, but os.Remove(src) runs first. A destination close/flush failure can therefore delete the only source copy while mergeFile still returns success. Explicitly close fdst, propagate that error, and remove src only after the close succeeds; preserve close errors on earlier return paths as well.

🤖 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 `@upgrade/upgrade.go` around lines 104 - 106, Update mergeFile so fdst is
explicitly closed before os.Remove(src), returning any close error and only
deleting the source after a successful close. Replace the deferred-only close
handling with logic that also preserves and propagates fdst.Close errors on all
earlier return paths.