fix(node): map ipfs/arweave DB outages to 503 via bare ? (#251) - #254
fix(node): map ipfs/arweave DB outages to 503 via bare ? (#251)#254Ayush7614 wants to merge 2 commits into
Conversation
Replace .map_err(AppError::Internal) with bare ? so connection-class
sqlx failures downcast to AppError::Db and return 503 db_unavailable
on /ipfs/pins, /ipfs/{cid}, and /arweave/anchors (Gitlawb#251).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesDatabase error propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/ipfs.rs (1)
80-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd closed-pool coverage for
get_by_cid.The handler now changes two database calls to rely on the
anyhow→AppErrorconversion, but the new regression test covers onlylist_pins. Add a valid-CID closed-pool request for/ipfs/{cid}and assert the 503db_unavailablepayload 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
📒 Files selected for processing (3)
crates/gitlawb-node/src/api/arweave.rscrates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/error.rs
|
@beardthelion — this PR addresses #251 that you filed. Happy to rebase/coordinate with #134 if that lands first on the same call sites. |
beardthelion
added
crate:node
CodeRabbit nit: exercise the /ipfs/{cid} list_all_repos bare-? path
against a closed pool so both DB propagation sites stay guarded.
|
@coderabbitai — addressed your nit: Added |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
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_errorwhere they carriedinternal_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/anchorsto 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. Withipfs.rs:80restored and onlyipfs.rs:86reverted, the test still passes, because a closed pool failslist_all_reposfirst 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_unavailablecannot see server-reported unavailability. Separate defect, not an ask on this PR.
crates/gitlawb-node/src/error.rs:65
The classifier matches onlyPoolTimedOut | PoolClosed | Io | Tls, so no SQLSTATE-bearing error can reach the 503 arm at all, which the probe above confirms. Meanwhilemain.rs:685already downcastssqlx::Error::Database(db)and switches on code prefixes, treating everything outside28/3Das 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.
2 participants
Summary
.map_err(AppError::Internal)with bare?on/api/v1/ipfs/pins,GET /ipfs/{cid}, and/api/v1/arweave/anchorsDB calls.PoolClosed, etc.) now downcast throughFrom<anyhow::Error>intoAppError::Dband return 503db_unavailable, matching/api/v1/reposand/api/v1/peers.Internalvia that same conversion.Test plan
cargo test -p gitlawb-node pool_closed_via_anyhow_from_is_503_db_unavailable— downcast pathlist_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
503 Service Unavailablewith the expected database-unavailable error payload, rather than an internal server error.503behavior and the correct error code/message when the database connection pool is closed.