fix(node): map ipfs/arweave DB outages to 503 via bare ? (#251)

[Ayush7614]

Summary

• Replace .map_err(AppError::Internal) with bare ? on /api/v1/ipfs/pins, GET /ipfs/{cid}, and /api/v1/arweave/anchors DB calls.
• Connection-class sqlx failures (PoolClosed, etc.) now downcast through From<anyhow::Error> into AppError::Db and return 503 db_unavailable, matching /api/v1/repos and /api/v1/peers.
• Non-sqlx errors still become Internal via that same conversion.

Test plan

• cargo test -p gitlawb-node pool_closed_via_anyhow_from_is_503_db_unavailable — downcast path
• list_pins_closed_pool_returns_503_db_unavailable — closed-pool route (CI / DATABASE_URL)
• list_anchors_closed_pool_returns_503_db_unavailable — closed-pool route (CI / DATABASE_URL)

Fixes #251

Summary by CodeRabbit

• Bug Fixes
  • Database connection failures now return a clear 503 Service Unavailable response instead of an internal server error.
  • Improved error reporting for Arweave anchor and IPFS pin queries when the database is unavailable.
  • Added coverage to verify consistent database-unavailable responses across affected endpoints.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3e121bbf-9aae-4792-80f3-5e35578cdede

📥 Commits

Reviewing files that changed from the base of the PR and between 347e933 and c29a91a.

📒 Files selected for processing (1)

• crates/gitlawb-node/src/error.rs

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

• crates/gitlawb-node/src/error.rs

---

📝 Walkthrough

Walkthrough

The handlers now propagate SQLx database errors through AppError, allowing closed-pool failures to return HTTP 503 with the db_unavailable payload. Regression tests cover the Arweave anchors, IPFS pins, and IPFS content routes.

Changes

Database error propagation

Layer / File(s)	Summary
Handler error propagation crates/gitlawb-node/src/api/arweave.rs, crates/gitlawb-node/src/api/ipfs.rs	Arweave and IPFS database calls use direct error propagation. Connection failures can therefore resolve to 503 db_unavailable responses.
Closed-pool regression coverage crates/gitlawb-node/src/api/arweave.rs, crates/gitlawb-node/src/api/ipfs.rs, crates/gitlawb-node/src/error.rs	Tests close the SQLx pool and verify the HTTP status and database-unavailable error payload for all affected routes. An AppError conversion test covers an anyhow-wrapped closed-pool error.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• Gitlawb/node#247: Related IPFS database error propagation and AppError response handling.
• Gitlawb/node#255: Related SQLx error handling in GraphQL resolver paths.

Suggested reviewers: beardthelion, jatmn, kevincodex1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description explains the fix and tests, but it omits most required template sections and checklist confirmations.	Complete the required sections for change type, concrete changes, verification, review checklist, and protocol impact.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the database outage handling change for the affected IPFS and Arweave routes.
Linked Issues check	✅ Passed	The changes satisfy issue #251 by returning 503 db_unavailable for all affected routes and adding closed-pool regression tests.
Out of Scope Changes check	✅ Passed	The changed handlers, error conversion test, and regression tests are directly related to issue #251.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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]

🧹 Nitpick comments (1)

crates/gitlawb-node/src/api/ipfs.rs (1)

80-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add closed-pool coverage for get_by_cid.

The handler now changes two database calls to rely on the anyhow → AppError conversion, but the new regression test covers only list_pins. Add a valid-CID closed-pool request for /ipfs/{cid} and assert the 503 db_unavailable payload so both propagation points remain protected.

🤖 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 `@crates/gitlawb-node/src/api/ipfs.rs` around lines 80 - 86, Add regression
coverage for the get_by_cid handler using a valid CID and a closed database
pool, issuing a request to /ipfs/{cid}. Assert that it returns HTTP 503 with the
db_unavailable payload, matching the existing list_pins closed-pool test pattern
and protecting both error-propagation paths.

🤖 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 `@crates/gitlawb-node/src/api/ipfs.rs`:
- Around line 80-86: Add regression coverage for the get_by_cid handler using a
valid CID and a closed database pool, issuing a request to /ipfs/{cid}. Assert
that it returns HTTP 503 with the db_unavailable payload, matching the existing
list_pins closed-pool test pattern and protecting both error-propagation paths.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c8723f36-fd67-4348-bb4a-df2beee5bf0b

📥 Commits

Reviewing files that changed from the base of the PR and between 111cff7 and 6cf5378.

📒 Files selected for processing (3)

• crates/gitlawb-node/src/api/arweave.rs
• crates/gitlawb-node/src/api/ipfs.rs
• crates/gitlawb-node/src/error.rs

[Ayush7614]

@beardthelion — this PR addresses #251 that you filed. Happy to rebase/coordinate with #134 if that lands first on the same call sites.

[Ayush7614]

@coderabbitai — addressed your nit:

Added get_by_cid_closed_pool_returns_503_db_unavailable — valid sha2-256 CIDv1 + closed pool → exact 503 / db_unavailable payload, matching the list_pins pattern so both bare-? DB sites stay covered.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@Ayush7614 Thanks — that covers the previously untested get_by_cid database-propagation path with the expected closed-pool behavior.

🐇 ✅

---

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

Thank you for using CodeRabbit!

[beardthelion]

Approving. The mechanism is right and the sweep went further than the issue asked: after this, no live .map_err(AppError::Internal) remains on a db call anywhere in the crate (the two left in register.rs wrap UCAN work, not queries). I checked the downcast actually survives at all four call sites, since that is the part that silently breaks: list_pinned_cids, list_arweave_anchors, list_all_repos and list_visibility_rules_for_repos each end on a bare ? over fetch_all with no .context() layer, so there is nothing for downcast::<sqlx::Error>() to strip.

I verified the tests are load-bearing rather than green by construction. Reverting each production line back to .map_err(AppError::Internal) turns the matching test red with left: 500 / right: 503, and all three pass unmutated on 7cba310. The get_by_cid test you added on the last push covers the list_all_repos conversion specifically: revert ipfs.rs:80 and it fails at ipfs.rs:294.

Two notes, neither an ask, and one small thing worth doing.

Findings

• [P3] Worth adding to the PR body: 500 responses on these routes now carry db_error where they carried internal_error
crates/gitlawb-node/src/error.rs:150
  For a server-reported query error the status stays 500 and the message text is byte-identical, but the code string changes, and the description currently reads as though only the 503 case moves. I drove a dropped-column error through /api/v1/arweave/anchors to confirm: {"error":"db_error","message":"error returned from database: column \"cid\" does not exist"}. Nothing in the repo branches on that string, and the new code is what the sibling routes already emit for the same condition, so this is an alignment rather than a regression. Description fix only, no code change and no need for another push.

• [P3] Coverage boundary on get_by_cid, recorded so it is not mistaken for full coverage later. Not an ask.
  crates/gitlawb-node/src/api/ipfs.rs:86
  The new test pins the first converted call site but not the second. With ipfs.rs:80 restored and only ipfs.rs:86 reverted, the test still passes, because a closed pool fails list_all_repos first and the second call is never reached. No test can isolate it with this injection, and the conversion there is the same one-line pattern, so I would leave it as is.

• [P2] db_unavailable cannot see server-reported unavailability. Separate defect, not an ask on this PR.
  crates/gitlawb-node/src/error.rs:65
  The classifier matches only PoolTimedOut | PoolClosed | Io | Tls, so no SQLSTATE-bearing error can reach the 503 arm at all, which the probe above confirms. Meanwhile main.rs:685 already downcasts sqlx::Error::Database(db) and switches on code prefixes, treating everything outside 28/3D as transient, so the startup path and the request path currently disagree about the same condition. Which variant a real restart or connection-slot exhaustion emits is a direction I have not run, which is the other reason it does not belong in your change.

Merge order

Taking your offer: this goes first and #134 rebases onto it. #134 adds five .map_err(AppError::Internal)? sites in these same two files, which would put the 500 behavior straight back. I will raise those in that PR's round, so nothing there is on you.

Not yours, but relevant to whoever merges: PR Checks had never run on either head (fork approval pending), so the green on this PR was two triage checks rather than the suite. I have approved the run and it is going now.

[jatmn]

LGTM

[beardthelion]

@kevincodex1 this one is ready to merge. Approved by me on 07-27 and by jatmn on 07-29, both against the current head 7cba310, so neither review is stale. CI is green across all 12 checks and it merges clean.

[kevincodex1]

thanks for your contribution @Ayush7614 please rebase to main and kindly fix conflicts

[Ayush7614]

@kevincodex1 rebased onto current main and resolved conflicts on head 3143404.

Main had grown budget-timeout wrappers around get_by_cid's DB calls; kept those and switched the sqlx error arms to Into<AppError> (instead of AppError::Internal) so closed-pool / connection failures still map to 503 db_unavailable. list_pins + /arweave/anchors use bare ? again, with closed-pool regressions restored.

Ready for another look / merge when CI is green.

[Ayush7614]

cc: @kevincodex1

[kevincodex1]

hello @Ayush7614 kindly do rebase again and fix conflicts

[Ayush7614]

@kevincodex1 — rebased again onto current main (241b366, includes merged #247).

Conflict resolution: error.rs kept all #226 opaque-body tests and added the #251 pool_closed_via_anyhow_from_is_503_db_unavailable downcast test. Dropped the separate LEDGER commit — main already carries those #248 peer-readiness fixture writers from #247.

Head: single commit c29a91a on fix/251-db-outage-503-ipfs-arweave. get_by_cid still uses main's budget-timeout wrappers with Into<AppError> on sqlx failures; list_pins and /arweave/anchors use bare ?.

Ready for merge when CI is green.

[beardthelion]

Re-reviewed on c29a91af, since the earlier approvals all predate today's two rebases.

The mechanism is right and the fail-closed ordering on the access-control query is intact: the timeout arm still returns before the loop, so a rule-map failure can never fall through to an unfiltered scan. Checked the conversion end to end, db_unavailable covers PoolTimedOut, PoolClosed, Io and Tls, and query or schema errors still land on the opaque 500 db_error body rather than being reclassified as retryable. Three of the four changed arms are pinned by the new tests, each verified by reverting the production line and watching the test go red.

Findings

• [P2] Pin the visibility-rules error arm
  crates/gitlawb-node/src/api/ipfs.rs:234
  The closed-pool tests cannot reach this arm, since the pool is shut before list_all_repos and the handler returns at the first query. Reverting this line to AppError::Internal(e) leaves all three new tests green, so the one arm on the access-control query is unpinned. A test that seeds a repo (needed, the db function short-circuits on an empty id list), drops visibility_rules, then drives GET /ipfs/{cid} does pin it: assert 500 with body db_error, which goes red with internal_error if the arm regresses. Not a blocker for merge.

• [P3] Update the Internal-arm comment that still names this handler
  crates/gitlawb-node/src/error.rs:190
  The comment gives GET /ipfs/{cid} as the example of a handler reaching the Internal arm via .map_err(AppError::Internal). This PR removes both of those call sites, so the example now points at a handler that no longer does it. error.rs is already in the diff.