fix(server): branching from a ledger with no commits is a 400, not a 500 by bplatz · Pull Request #1945 · fluree/db

bplatz · 2026-09-25T16:35:47Z

What this does

POST /branch from a ledger that has no commits returned 500 Internal Server Error ("Source branch … has no commit head"). It now returns 400 Bad Request:

Invalid branch operation: Cannot branch from 'mydb:main': it has no commits yet. Transact to it first.

Two things produced the 500, and both are changed:

Fluree::create_branch raised ApiError::Internal for a source with no commit head. It now raises ApiError::InvalidBranch.
The server's ServerError status mapping had no arm for InvalidBranch, so it fell to the 500 catch-all even though ApiError::status_code() already says 400. It now maps to 400 with @type err:api/BadRequest.

Branching from an empty ledger stays an error rather than being supported. The raft state machine and the DynamoDB nameservice both reject an unborn source, and merge/rebase assume the two branches share a commit.

Docs

docs/api/endpoints.md: POST /branch lists the new 400; GET /merge-preview lists its 409.
docs/cli/server-integration.md: the branch-create and merge-preview error tables match.
docs/api/errors.md: 400 and 409 name the branch errors and their @type.

Testing

API (it_branch.rs): branching from an empty source returns a 400 and creates no branch record.
HTTP (integration.rs): POST /branch from an empty ledger returns 400 err:api/BadRequest. Merging main (which has no source branch) returns 400 err:api/BadRequest with the "no source branch" message from both GET /merge-preview and POST /merge.
Unit (error.rs): InvalidBranch and BranchConflict get the same status and @type whether typed or flattened to a bare status.
Each mapping arm was removed in turn, and at least one test failed each time.

aaj3f

@bplatz 👍 this is good, and it's a small PR so I'll be brief here. I found just a few items you may want to address (details below) but they're just in the categories of test coverage; a few places that deserve same class of fixes in the same files; some stale docs.

Swapping ApiError::Internal for InvalidBranch is the fix anyone would write; noticing that ServerError's status table had drifted from ApiError::status_code() — which has said InvalidBranch => 400 at fluree-db-api/src/error.rs:618 all along — is the one that actually mattered. Two tables describing the same thing, disagreeing, with the wrong one winning at the boundary. That also reframes the merge-preview "side effect" nicely: those 400s were already documented and already intended, they were just being mistranslated on the way out, which is why the docs there needed no change.

I also think you were right to not replace the per-variant table with a blanket delegation to ApiError::status_code(). It's the tempting refactor, and it would have silently reclassified the variants that differ on purpose — ApiError::LedgerConfig is pinned to 500 at error.rs:341 with a comment saying it's operator data, not caller input.

Verified rather than read: mutation-checked both halves independently — reverting loading.rs to ApiError::internal turns create_branch_from_empty_source_fails red (got: Internal error: Source branch mydb:main has no commit head / left: 500 right: 400), and dropping the error.rs:343 arm turns both server tests red on left: 500. Confirmed err:api/BadRequest is a real pre-existing term (fluree-vocab/src/errors.rs:110) and not coined here, and confirmed it_branch.rs is genuinely wired (grp_ledger.rs:4-5) — worth checking explicitly in this crate, since autotests = false means an unwired it_*.rs compiles nowhere and runs never. I also enumerated every producer of ApiError::InvalidBranch in the workspace (24 sites across merge_preview, merge, rebase, revert_preview, revert, loading) to make sure widening 400 doesn't swallow a genuine server fault — every one is caller input: bad option combinations, invalid topology for the request, unreachable/genesis/merge commits, empty selections. No monitoring or retry classifier in the repo keys on 5xx for these, and nothing matches on the old "has no commit head" string.

The one thing I'd like before merge is small: error.rs:120 — the error_type() arm is the only part of the fix with nothing holding it down. With that line deleted, all three new tests stay green while the body reverts to "@type":"err:system/InternalError" on a 400. One assert_eq!(json["@type"], "err:api/BadRequest") in create_branch_from_empty_ledger_is_bad_request closes it, and it's the pattern you used yourself in ff4142f26.

Then a judgment call I'd rather you make than assume: BranchConflict has the identical gap one arm away, and it's live on GET /merge-preview, GET /revert-preview, POST /sweep and POST /sweep-plan. The vocab really is missing a term — but 409 => errors::COMMIT_CONFLICT at :88 is in-file precedent for a generic one, and the sweep case is the sore spot: a message that says "retry when it completes" arrives as a 500 internal error, so a client can't distinguish a transient hold from a crash. Either fold it in with the generic code, or say in the body that the @type naming is a deliberate deferral — I'd just rather it not sit as an unattributed leftover.

Everything else is genuinely minor: the Http { status: 400 } → errors::INTERNAL fallthrough at :97 (pre-existing, one line, and it hits validate_branch_name on this very endpoint), the stale POST /branch row in docs/cli/server-integration.md:1223, and a message assertion on merge_preview_of_root_branch_is_bad_request.

One process note that isn't about the code at all. This PR's base is fix/server-config-docs, not main — so merging it today puts this commit on #1944's branch, and since the two have diverged (compare(1944head...1945head) = diverged ahead=1 behind=4) that's a merge commit onto that branch rather than a fast-forward. GitHub says MERGEABLE / CLEAN, which is true and is also the trap: the green is about the wrong target. The good news is the dependency isn't real — git merge-base puts #1944, #1945 and #1948 all at ffeeec554, they're siblings off one main commit, and this PR shares no code files with either of them. So I'd just re-target to main. Relatedly, "Stacked on #1944" in the body isn't true of the tree (behind=4 — this branch doesn't contain those four commits), and a reviewer who believes it will assume the config work is present in what they're reading. Your own note on #1944 about #1940 is the right phrasing for this.

Adherence to repo commitments

Patterns / abstractions — ✔ extends the two existing ServerError matches rather than adding a parallel mapping, and correctly declines the blanket-delegation refactor that would have reclassified the deliberately-500 variants.
Performance (speed first, memory second) — ✔ No performance-degradation risk. Two arms in two cold-path matches, on error construction only; the message format! lives inside ok_or_else, so the success path is untouched. No allocation, lock, or I/O added anywhere that runs on a successful request.
Testing — ✔ with the ⚠️ above: three new tests, all wired into real targets (grp_ledger, grp_http) and observed running, and both fix hunks mutation-checked red. The @type half of the fix is the uncovered part.
Conventions — ✔ self-describing subject, thorough body that is accurate everywhere I checked it (including the honest "Not in this PR" note); cargo fmt --all --check and clippy -p fluree-db-api -p fluree-db-server --all-targets clean; CI green on 119068e1f (16 checks, all non-skipped success); docs updated where behavior changed and correctly left alone where it merely caught up to them.

Approving so you can merge when ready — the @type assertion is the only thing I'd genuinely like in first, and the BranchConflict call is yours. Happy to talk through any of it.

aaj3f · 2026-09-28T15:37:14Z

                }
            }
            ServerError::Api(ApiError::LedgerExists(_)) => errors::LEDGER_EXISTS,
+            ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,


🟡 MEDIUM-1 — this arm is the only part of the fix with no test behind it

Pasting this claude-code-observed note as-is, since the deletion probe is the whole argument — with the line removed, all three new tests stay green, while the response body silently reverts to

{"error":"Invalid branch operation: Cannot branch from 'empty:main': it has no commits yet. Transact to it first.","status":400,"@type":"err:system/InternalError"}

…because ServerError::Api(_) => errors::INTERNAL at :242 catches it. So the status half of the fix is regression-proofed and the @type half isn't, and a later refactor of this match would drop it without CI noticing — leaving a 400 that tells every client it's an internal error.

You set the better pattern yourself in ff4142f26 eight days ago (the @type arm landed with assert_eq!(body["@type"], "err:db/LedgerNotFound") next to it), and error.rs's own unit tests assert both halves — r2rml_unsupported_pattern_is_400_with_distinct_type, novelty_backpressure_is_503_with_novelty_code_in_every_shape. One line in create_branch_from_empty_ledger_is_bad_request closes it:

Suggested change

ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,

assert_eq!(json["@type"], "err:api/BadRequest", "{json}");

That addition was checked in both directions — red with the arm removed, green with it.

Added in 68e4ce9. create_branch_from_empty_ledger_is_bad_request now asserts @type is err:api/BadRequest. Checked both ways: it fails with the InvalidBranch arm removed and passes with it restored.

aaj3f · 2026-09-28T15:37:14Z

            // Operator data, not caller input. See `ApiError::LedgerConfig`.
            ServerError::Api(ApiError::LedgerConfig(_)) => StatusCode::INTERNAL_SERVER_ERROR,
            ServerError::Api(ApiError::Format(_)) => StatusCode::BAD_REQUEST,
+            ServerError::Api(ApiError::InvalidBranch(_)) => StatusCode::BAD_REQUEST,


🔵 LOW-2 — BranchConflict has the same gap one line away, and it's live on three routes

The "Not in this PR" note says the vocab has no fitting @type for BranchConflict, and that's true — I checked fluree-vocab/src/errors.rs and there's no term. But I think the note undersells where the gap bites, so flagging it for a decision rather than assuming:

fluree-db-api/src/error.rs:619 already declares BranchConflict => 409, and the server has no arm, so it lands on ServerError::Api(_) => StatusCode::INTERNAL_SERVER_ERROR (:389) plus errors::INTERNAL (:242) — the exact shape you're fixing for InvalidBranch. The write routes are fine (consensus derives from ApiError::status_code() and yields 409 already), but three direct routes are not:

GET /merge-preview and GET /revert-preview via fluree-db-api/src/branch_validation.rs:247, whose own comment says "surfacing it here, as a conflict rather than an internal error, lets a preview answer instead of failing" — which the HTTP boundary then undoes.

POST /sweep and POST /sweep-plan via admin.rs:2552 → hold_ledger_for_maintenance. A message that literally says "retry when it completes" is delivered as a 500 err:system/InternalError, so a client can't tell a transient hold from a crash.

There's in-file precedent for a generic 409 code — 409 => errors::COMMIT_CONFLICT at :88 — so if a dedicated term feels premature, reusing that is a two-line change alongside the arms you're already adding. This is minor and non-blocking, but it's the same bug in the same match, and if you agree it's right I'd rather see it folded in here than become a backlog item nobody schedules. If you'd rather not (because the right @type is a naming decision and not yours alone to make unilaterally), that's a fair reason to split it — just say so in the body so it's a decision rather than a leftover.

Folded in (68e4ce9). BranchConflict now maps to 409 with err:db/CommitConflict.

What decided the @type: the committer routes already answered it that way. Merge, rebase and revert flatten the error to ApiError::Http { status: 409 }, and the passthrough maps that to COMMIT_CONFLICT. Reusing it makes merge-preview, revert-preview, sweep and sweep/plan match those routes, with no new term to name.

The new unit test branch_errors_answer_the_same_typed_or_flattened pins status and @type for both the typed and the flattened shape. endpoints.md already documented the sweep's 409, so the code now matches the docs.

aaj3f · 2026-09-28T15:37:14Z

                }
            }
            ServerError::Api(ApiError::LedgerExists(_)) => errors::LEDGER_EXISTS,
+            ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,


⚪ NIT-3 — ApiError::Http { status: 400 } still reports an internal @type

— the real line is fluree-db-server/src/error.rs:97, not in this diff; commenting on the nearest changed line in the same match.

Pre-existing, definitely not introduced here, but adjacent enough to be cheap. The inner status match has arms for 401/403/409/413/422 and then _ => errors::INTERNAL, so a 400 taking that path reports err:system/InternalError. create_branch's own validate_branch_name failure (loading.rs:206-209) goes exactly there — which means the very case the line you edited in endpoints.md calls "Invalid request body (including malformed at value)" returns 400 carrying an internal-error type. One line:

Suggested change

ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,

400 => errors::BAD_REQUEST,

Taken (68e4ce9). One note on scope: it reaches further than validate_branch_name.

The committer flattens merge, rebase and revert's InvalidBranch to Http { status: 400 }. Without this arm, POST /merge of a root branch said InternalError while GET /merge-preview said BadRequest. merging_a_root_branch_is_bad_request_on_both_routes now covers both routes and fails without the arm.

It also changes @type on every tracked query/transact 400, from InternalError to BadRequest; the status is unchanged. I found nothing asserting the old type.

aaj3f · 2026-09-28T15:37:14Z

@@ -2212,7 +2212,7 @@ POST /branch



⚪ NIT-4 — the implementer contract table wasn't updated

Commenting here because docs/cli/server-integration.md:1223 isn't in this diff. That file is the contract third-party servers build to, and its POST /branch row still reads "400 — Invalid branch name (per validate_branch_name); malformed JSON body" with nameservice/storage errors under 5xx. Someone implementing to it keeps returning 500 for the empty-source case. Its siblings are already correct (merge-preview at :1086, rebase at :1632), so it's one stale row rather than a sweep.

Fixed (68e4ce9). The POST /branch 400 row now lists the no-commits case. I also added a 409 row to the merge-preview table for the namespace-conflict BranchConflict, since preview now answers it with a 409.

aaj3f · 2026-09-28T15:37:14Z

@@ -482,6 +482,83 @@ async fn create_branch_at_historical_t() {
    assert_eq!(resp.status(), StatusCode::BAD_REQUEST);


⚪ NIT-5 — merge_preview_of_root_branch_is_bad_request doesn't pin which 400

Its sibling asserts on the message; this one only asserts the status, so any of merge-preview's six InvalidBranch conditions satisfies it. A contains("no source branch") would pin it to the one you mean and match the matcher contract at docs/cli/server-integration.md:1086. Small.

Done (68e4ce9), renamed to merging_a_root_branch_is_bad_request_on_both_routes. It asserts status, @type, and "no source branch" in error for both GET /merge-preview and POST /merge.

aaj3f · 2026-09-28T15:37:14Z

            // Operator data, not caller input. See `ApiError::LedgerConfig`.
            ServerError::Api(ApiError::LedgerConfig(_)) => StatusCode::INTERNAL_SERVER_ERROR,
            ServerError::Api(ApiError::Format(_)) => StatusCode::BAD_REQUEST,
+            ServerError::Api(ApiError::InvalidBranch(_)) => StatusCode::BAD_REQUEST,


🟡 MEDIUM-6 — this PR's base branch means "merge" does not put it on main

No honest anchor for this one — it's about the PR's base pointer, not a line of code, so I've parked it on the fix and it belongs in the conversation rather than inline.

#1945's base is fix/server-config-docs (#1944's branch), not main. So pressing Merge today lands this commit on #1944's branch. And the two have diverged — compare(1944head...1945head) is status=diverged ahead=1 behind=4 — so it would be a real merge commit onto that branch, not a fast-forward. GitHub currently reports this PR MERGEABLE / CLEAN, which is true and also the trap: the green signal is about merging into the wrong target.

The thing that makes this easy to fix rather than a scheduling problem is that the dependency isn't real. git merge-base puts #1944, #1945 and #1948 all at ffeeec554 — they're siblings off one main commit, not branches containing one another, and this PR's single commit shares no code files with either of the others. The only overlap in the whole cluster is docs.

So it seems like the cleanest thing is to re-target this to main now. Failing that, landing #1944 first and letting GitHub auto-retarget on branch deletion also works — it just leaves a window where a merge does something surprising.

Resolved by ordering. #1944 merged, its branch was deleted, and this PR retargeted to main. The first commit was rebased onto current main (a1530a8), so the PR is now two commits on top of it.

aaj3f · 2026-09-28T15:37:15Z

                }
            }
            ServerError::Api(ApiError::LedgerExists(_)) => errors::LEDGER_EXISTS,
+            ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,


⚪ NIT-7 — "Stacked on #1944" isn't true of the tree

Again no real anchor — this is the PR body. Minor, but it costs a reviewer real time: behind=4 means this branch does not contain #1944's four commits, so anyone who reads "Stacked on #1944" and assumes the config-precedence work is present in what they're reviewing is reading a tree that doesn't have it. Same shape as #1944's own honest note about #1940 ("the content is independent of it; it's stacked only because the branch started there") — which is exactly the right phrasing, and #1940 has since merged, so #1944's pointer is genuinely correct.

Dropping the line, or rewording it the way #1944 does, would save the next person the merge-base check I ended up running.

Agreed, it wasn't true of the tree: the branch started from the same main commit as #1944 and never contained its commits. Now that #1944 has merged and this targets main, the line comes out with the body update.

aaj3f · 2026-09-28T15:37:15Z

@@ -1038,7 +1038,8 @@ pub struct CreateBranchResponse {
 /// - `source`: Source branch (optional, defaults to "main")


⚪ INFO-8 — 4xx still logs at ERROR

Not a regression and not yours — tracing::error!(…, "branch creation failed") plus set_span_error_code(&span, "error:BranchCreateFailed") at ledger.rs:1103 is uniform across the ledger routes. Noting it only because this PR has just reclassified the condition as the caller's fault, so anyone alerting on error-level logs will now page on client typos. Worth knowing; not worth fixing here.

Noted, and leaving it here. It's the same across all the ledger routes, so if it changes it should be by status class in one place, not route by route.

create_branch reported an unborn source as an internal error, and the server's status mapping had no arm for InvalidBranch, so every InvalidBranch reaching it through a direct API call was a 500 as well. That also covers merge-preview's documented 400s (previewing a root branch, merging a branch into itself).

…utes InvalidBranch and BranchConflict reached clients differently by route: previews and sweeps returned them typed and hit the 500 catch-all, while merge, rebase and revert flattened them to a bare status whose 400 fell through to err:system/InternalError. Both now answer 400 err:api/BadRequest and 409 err:db/CommitConflict on every route.

bplatz added bug Something isn't working as expected area:server HTTP surface, routes, error mapping, swagger, timeouts/admission, config graph labels Sep 25, 2026

bplatz requested review from aaj3f and zonotope September 25, 2026 16:35

bplatz added this pull request to stack #1922 September 25, 2026 16:37

bplatz removed this pull request from stack #1922 September 25, 2026 18:27

bplatz force-pushed the fix/branch-from-empty-ledger branch from 8186ad0 to f236e04 Compare September 25, 2026 19:15

bplatz added this pull request to stack #1947 September 25, 2026 19:15

bplatz mentioned this pull request Sep 25, 2026

feat: edge annotations and GRAPH blocks in CONSTRUCT templates #1948

Merged

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from 34bae2a to e86661f Compare September 25, 2026 21:07

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from 6a8f480 to 950bc14 Compare September 25, 2026 21:31

bplatz force-pushed the fix/branch-from-empty-ledger branch 3 times, most recently from 7b9fae7 to 157a6f6 Compare September 25, 2026 23:13

bplatz force-pushed the fix/branch-from-empty-ledger branch from 157a6f6 to c276ddc Compare September 25, 2026 23:32

bplatz force-pushed the fix/branch-from-empty-ledger branch from c276ddc to fed770f Compare September 25, 2026 23:37

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from f103067 to 41e3345 Compare September 26, 2026 00:45

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from cc6926b to ec47cbb Compare September 26, 2026 01:06

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from 895221d to 11e833e Compare September 26, 2026 01:51

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from be45c5a to 44c7232 Compare September 26, 2026 03:00

bplatz force-pushed the fix/branch-from-empty-ledger branch from 44c7232 to c10c225 Compare September 26, 2026 03:16

bplatz force-pushed the fix/branch-from-empty-ledger branch 2 times, most recently from ba78d0c to ca903dc Compare September 26, 2026 09:38

bplatz force-pushed the fix/branch-from-empty-ledger branch from ca903dc to 119068e Compare September 26, 2026 11:25

bplatz mentioned this pull request Sep 26, 2026

feat(api): branch from a point in time with at: "time:<ISO>" #1969

Merged

aaj3f approved these changes Sep 28, 2026

View reviewed changes

Base automatically changed from fix/server-config-docs to main September 28, 2026 23:57

bplatz force-pushed the fix/branch-from-empty-ledger branch from 119068e to 27ffcc2 Compare September 28, 2026 23:57

bplatz merged commit ea287de into main Sep 29, 2026
16 checks passed

bplatz deleted the fix/branch-from-empty-ledger branch September 29, 2026 00:30

Error	Direct routes, before	Committer routes, before	Both, now
`InvalidBranch` (root branch as source, merge into itself, …)	500 `err:system/InternalError`	400 `err:system/InternalError`	400 `err:api/BadRequest`
`BranchConflict` (namespace conflict, maintenance hold, abort on conflict)	500 `err:system/InternalError`	409 `err:db/CommitConflict`	409 `err:db/CommitConflict`

	ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,
	assert_eq!(json["@type"], "err:api/BadRequest", "{json}");

	ServerError::Api(ApiError::InvalidBranch(_)) => errors::BAD_REQUEST,
	400 => errors::BAD_REQUEST,

		@@ -482,6 +482,83 @@ async fn create_branch_at_historical_t() {
		assert_eq!(resp.status(), StatusCode::BAD_REQUEST);

		@@ -1038,7 +1038,8 @@ pub struct CreateBranchResponse {
		/// - `source`: Source branch (optional, defaults to "main")

Conversation

bplatz commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Other branch errors now answer the same on every route

Docs

Testing

Uh oh!

aaj3f left a comment

Choose a reason for hiding this comment

Adherence to repo commitments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bplatz commented Sep 25, 2026 •

edited

Loading