fix(branch): make branch create success mean the branch is usable - #201
Conversation
Three defects that share one theme: the command reports success on signals that do not mean what a caller needs them to mean. Found while automating branch creation against ap-southeast; details and measurements in InsForge/InsForge#1790. 1. `ready` is a control-plane state, not readiness. `pollUntilReady` returns the moment `branch_state === 'ready'` and never contacts the branch's own host. But `branch_state` flips when the provisioning job returns, while the instance is still coming up — every request to https://<appkey>.<region>.insforge.app resets until it does. Measured on ap-southeast: the host started serving at t+2m in one case and t+11.5m in another, `branch_state` reading 'ready' the whole time. Because create then auto-switches the directory onto that host, the failure surfaces on the user's NEXT command rather than here, as {"error":"fetch failed","code":"UNKNOWN_ERROR"} from whatever they ran. Now: after the control plane says ready, poll GET /api/health on the branch itself until it answers, and say so in the spinner. If it never answers within the budget the command reports that honestly instead of claiming success — the branch still exists and its name and id are still printed, because it is real and it is billing. 2. The 5-minute poll ceiling was below the observed provisioning time. POLL_TIMEOUT_MS was 5 minutes, so the 11.5-minute branch was reported as "still in 'creating' state" when it was simply not finished. Raised to 15, with a separate 10-minute budget for the data-plane wait. 3. A failed create can leave a live branch behind. `createBranchApi` carries no idempotency key. A transport failure on the RESPONSE leg — the POST arrived and the branch was created — throws before `created` is bound, so the CLI exits non-zero with no id and no name while a branch exists and bills. We hit exactly this: $ insforge branch create <name> --mode schema-only --no-switch {"error":"Connection to api.insforge.dev was reset. A proxy, VPN, or firewall may be interfering."} # exit 1 $ insforge branch list # the branch is there, state "creating" Now: on a create failure, ask `branch list` — a control-plane call that still works while the branch's own host is unreachable — whether the branch exists under that name, and adopt it if so. The original error is rethrown unchanged when nothing was created. Also: `ossFetch` called `fetch` unguarded, unlike `platformFetch`, so a dead data plane surfaced as the generic UNKNOWN_ERROR instead of naming the host. The new `probeBackendHealth` routes its errors through `formatFetchError`, so "Connection to <host> was reset" is what a caller sees. Tests cover the not-serving path, the adopt path, and that a genuine failure with nothing created still exits non-zero. The existing create tests needed a mock for the new probe — an unmocked one makes a real request to a fake host and then polls, so beforeEach resets it explicitly (clearAllMocks keeps implementations, which would otherwise leak an unreachable branch into every later test). Agent skills: `insforge-cli/references/branch/overview.md` says a branch takes 30-120s and that `ready` means "usable — can be switched, modified, merged, or reset", and its post-create checklist goes straight to `functions deploy`. Both are worth updating in InsForge/agent-skills once the timing here is confirmed; happy to open that PR alongside if you want it.
|
Thanks for the PR, @iPLAYCAFE-dev! A quick note on our workflow: we ask contributors to open an issue first, get it assigned, then submit a PR that links it (e.g. "Closes #123"). This PR isn't linked to any issue. It'll still be reviewed, but please open an issue and claim it (comment that you'd like it assigned to you) so the work is tracked. |
agent-zhang-beihai
Bot
added
the
needs-issue
WalkthroughBranch creation now adopts an existing branch after transport failures and performs a separate OSS health check after control-plane readiness. Success and auto-switching require the branch host to be serving, with tests covering unavailable hosts, provisioning timeouts, and adoption failure paths. ChangesBranch readiness and adoption
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
Greptile SummaryThis PR makes branch creation succeed only after the new branch is usable. The main changes are:
Confidence Score: 5/5The latest fixes look safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(branch): also require a matching mod..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/branch/create.ts (1)
79-129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDownstream success/JSON/exit-code checks still key off
ready.branch_state, notprovisioned— the "not serving" downgrade doesn't actually change command outcome.Line 84's
ready = { ...ready, branch_state: ready.branch_state }is a no-op (there's noBranch.branch_statevalue for "ready but not serving"), soready.branch_statestays'ready'afterprovisionedis downgraded tofalse. The final block (lines 117-129) branches onready.branch_state === 'ready'instead ofprovisioned, which causes two real problems when serving never comes up:
- Non-JSON with
--switch(default): line 120-124 still prints the "Re-source your dev server env" message, even though the switch was skipped at line 88 (provisioned && opts.switch) — misleading the user into thinking they're now on the new branch.- JSON mode:
outputJson({ branch: ready })fires unconditionally onready.branch_state === 'ready', so automation consumers can't tell serving failed from the payload.- More importantly, nothing throws or sets
process.exitCodefor this case — thespinner?.stop(msg, 1)on line 98-101 only affects the displayed icon/symbol in@clack/prompts(confirmed via clack's own issue tracker: thecodeargument is used purely to pick between "Something went wrong" and cancel-style messaging), it does not fail the process. So the CLI likely still exits0even though the branch host never came up — the exact outcome this PR set out to prevent.Use the
provisionedflag (already tracked) consistently in the final block, and throw/return a non-zero signal when serving never comes up.🐛 Suggested fix
if (json) { outputJson({ branch: ready }); - } else if (ready.branch_state === 'ready') { + } else if (provisioned) { if (opts.switch) { outputInfo( '⚠ Re-source your dev server env (.env) to pick up the new INSFORGE_URL / ANON_KEY.', ); } + } else if (ready.branch_state === 'ready') { + throw new CLIError(`Branch '${name}' reports ready but is not serving yet — retry your next command shortly`); } else { outputInfo( `Branch '${name}' is still in '${ready.branch_state}' state. Run \`insforge branch list\` to check.`, ); }Separately, this scenario also isn't covered by the test at lines 296-324 of
src/commands/branch/create.test.ts— that test only checks the spinner call, not the actual exit code or JSON payload, so it wouldn't have caught this gap.🤖 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 `@src/commands/branch/create.ts` around lines 79 - 129, Use the `provisioned` flag as the source of truth in the final output block instead of `ready.branch_state`, so a branch that never starts serving does not print success guidance or appear successful in JSON. Replace the no-op `ready` assignment in the `waitUntilServing` failure path with an explicit non-success outcome, and propagate failure from the command so it exits non-zero while preserving the existing spinner message and successful behavior.
🤖 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.
Outside diff comments:
In `@src/commands/branch/create.ts`:
- Around line 79-129: Use the `provisioned` flag as the source of truth in the
final output block instead of `ready.branch_state`, so a branch that never
starts serving does not print success guidance or appear successful in JSON.
Replace the no-op `ready` assignment in the `waitUntilServing` failure path with
an explicit non-success outcome, and propagate failure from the command so it
exits non-zero while preserving the existing spinner message and successful
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f481b80e-99d8-4e11-8b4d-07dc7cd065c1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/commands/branch/create.test.tssrc/commands/branch/create.tssrc/lib/api/oss.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…host never serves
Both review findings are real; fixing rather than arguing.
1. Adoption was too broad (greptile P1/security, cubic P1).
The catch adopted a same-name branch after ANY create failure, so a
duplicate-name rejection — a refusal, not a lost response — could switch the
caller into a pre-existing branch with a different mode and different data.
Two guards now, either of which is sufficient:
- only a TAGGED transport failure is eligible. platformFetch's fetch-level
catch now throws CLIError with code NETWORK_ERROR, so "the server said no"
and "we never heard back" are finally distinguishable by callers. Every
HTTP/API rejection rethrows untouched, and the test asserts that
listBranchesApi is not even called in that case.
- the candidate must have been created at or after the moment we sent the
request, so a pre-existing same-name branch can never be adopted. A 60s
skew allowance keeps a genuinely-just-created branch eligible when the
local clock differs from the control plane's; being slightly wide risks
adopting a branch someone made seconds ago under the same name, while
being too narrow re-opens the orphaned-billing bug this exists to fix.
2. A branch that never serves exited 0 (greptile P1, cubic P1).
I stopped the spinner with an error frame and left it there: the process
still exited 0, --json still emitted a ready-looking branch with no serving
field, and the non-JSON path still advised re-sourcing the env. That is
exactly the "exit 0 does not mean ready" defect this PR is meant to remove,
reproduced inside the fix for it.
Now: `serving` is tracked separately from `provisioned`, --json emits
{ branch, serving }, the env hint only prints when the host actually answers,
and the command throws afterwards so the exit is non-zero. The identity is
emitted BEFORE the throw on purpose — the branch exists and is billing, so a
caller has to be able to find and delete it even as the command fails.
Three tests added: an API rejection is not adopted, a branch predating the
request is not adopted, and the never-serving path exits 1 while still printing
the branch id and serving:false.
|
Closes #202. @agent-zhang-beihai — apologies, I submitted before reading the contribution workflow. Issue #202 is open now with the measurements and repro, and I've asked for it to be assigned. Happy to close this PR and resubmit after assignment if you'd prefer the order kept strictly. Both review findings were right, and both are fixed in 1. Adoption was too broad — @greptile-apps (P1/security), @cubic-dev-ai (P1)A duplicate-name response is a refusal, not a lost response, and adopting on it could switch the caller into a pre-existing branch with a different mode and different data. Two guards now, either sufficient on its own:
On the skew allowance: I used 60s. Being slightly wide risks adopting a branch someone else created seconds ago under the same name; being too narrow re-opens the orphaned-billing bug. If you'd rather have zero tolerance and accept the clock-skew false negative, say so and I'll drop it — it's a one-line change and your call, since you know how the control plane stamps 2. A branch that never serves exited 0 — @greptile-apps (P1), @cubic-dev-ai (P1)Correct, and it was worse than the comments say: I stopped the spinner with an error frame and left it there, so the process still exited 0, Now The identity is emitted before the throw on purpose: the branch exists and is billing, so a caller must be able to find and delete it even as the command fails. That's also why the not-serving path prints the Verification
Still open, and it's a maintainer question
|
|
Both findings are fully addressed. Here's my assessment after reading 552cf50: Finding 1 (adoption too broad): The two-guard approach is well-implemented. Finding 2 (never-serves exits 0): One minor observation: in On the skew question: 60s is defensible. The one edge case worth knowing about is whether Both threads resolved. LGTM on 552cf50. Tip: You can customize Greptile's behavior for this repo with |
|
Thanks for the re-review. Both threads resolved on cubic as well, and all four checks are green on On the stamping semantics — I have data for this, and it settles the skew question.
So the loaded-control-plane case you were worried about doesn't apply: the stamp can't drift later than the actual creation, and it lands within the same request. That leaves only genuine clock skew between the client and the control plane, which is what the 60s window is for. I'll leave it at 60s unless a maintainer wants it tighter. On the missing final probe after Awaiting human review. Tracking issue is #202 as requested — and to be explicit about what this PR does and doesn't do: it stops the CLI reporting readiness it hasn't verified, but the underlying gap (a branch reporting |
|
Process box ticked: #202 is now assigned to me (thanks @agent-zhang-beihai), and this PR closes it — so the issue-first workflow is satisfied for anyone reviewing later. Current state, so a human reviewer doesn't have to reconstruct it:
Two things still open that only a maintainer can decide, both already stated above and neither blocking:
Nothing further from me unless review turns something up. |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review — fix(branch): make branch create success mean the branch is usable
Summary: A well-scoped, well-tested fix that makes branch create gate success on data-plane readiness, safely adopts a branch orphaned by a transport failure, and raises the poll budget to match observed provisioning times — no blocking issues found.
Requirements context
No matching spec/plan found under docs/specs/ (the only specs there cover diagnose and db-migrations; this repo has no docs/superpowers/). Assessed against the PR description and the linked issue InsForge/InsForge#1790. The three stated problems (ready ≠ serving, sub-provisioning poll ceiling, orphaned branch on response-leg failure) are each addressed, and the implementation choices match the description faithfully.
Findings
Critical
(none)
Suggestion
Functionality — inconsistent exit semantics for the other "not usable" outcome. src/commands/branch/create.ts:158-164 now correctly exits non-zero when a branch is ready but never serves. But the sibling path — pollUntilReady times out with the branch still in a non-terminal creating state (create.ts:149-153) — still emits only an info line and exits 0. Given this PR's explicit goal ("success means usable"), a branch stuck provisioning past the 15-minute budget is equally unusable, yet automation reading the exit code sees success. Consider making that path exit non-zero too (or documenting why the two are treated differently). Low blast radius, hence not blocking.
Functionality — /api/health proves "listening", not "usable". waitUntilServing (create.ts:228-245) treats any res.ok from probeBackendHealth as serving. As you already note in the PR's open question, /api/health is a static handler that never touches Postgres, so a branch can pass this probe before its DB/schema is actually ready — a narrower version of the very gap this PR closes. This is the best client-side signal available today and you've flagged it honestly; noting it so it isn't lost. If waitForCompletion=true (matching projects restart / backups create) or a data-plane-scoped signal lands cloud-side, this poll should key off that instead.
Information
Software engineering — export const interleaved in the import block. src/lib/api/platform.ts:4-6 places export const NETWORK_ERROR_CODE between two import statements. It's legal (imports hoist) and the lint gate passes (no import/first rule in eslint.config.js), but conventionally declarations sit below the import block — worth relocating for readability.
Software engineering — double error surface on the not-serving path (non-JSON). In the ready-but-not-serving case the spinner stops with an error frame (create.ts:114-118) and then the thrown CLIError also prints Error: … via handleError. Two messages for one condition; harmless and arguably informative, just noting.
Notes on the good parts
- Adoption guards are exactly right.
createBranchOrAdopt(create.ts:197-219) adopts only on a taggedNETWORK_ERROR_CODEtransport failure and only for a same-name branch created at/afterrequestedAt(with a deliberate 60s skew), and rethrows the original error whenlistBranchesApifinds nothing or itself fails (.catch(() => undefined)). API rejections (duplicate name/quota/auth) correctly fall through untouched. TheNETWORK_ERROR_CODEtagging inplatform.ts:101-115is the clean way to distinguish transport failure from an HTTP response. - Test coverage matches the behavior changes — not-serving exit, adopt-on-transport-failure, rethrow-when-nothing-created, no-adopt-on-API-rejection, no-adopt-on-predating-branch, and identity-emitted-before-failure are all exercised. The
beforeEachre-stub ofprobeBackendHealth(test file) with the explanatory comment aboutclearAllMockskeeping implementations is a good catch that prevents cross-test leakage. - Security: no new user input reaches SQL/shell;
probeBackendHealthfetches an unauthenticated URL built from server-assignedappkey/regionwith a bounded 10sAbortSignal.timeout; no secrets logged (errors routed throughformatFetchError, which emits host + code only). No auth/authorization changes. - Performance: both poll loops are sequential and bounded (health: 5s × up to 10 min; provisioning: 3s × up to 15 min) — appropriate for a CLI, no hot-path concerns.
package-lock.jsonchange is only the 0.2.0 version sync (matchespackage.json), so nonpm cibreakage.
Verdict
approved (informational — no Critical findings; the two Suggestions and Information notes are non-blocking). Human green-check remains a separate action.
…OR_CODE export Addresses the review on InsForge#201 (jwfing, approved): - Functionality: `branch create` now exits non-zero when the branch never reaches 'ready' within the poll budget, matching the existing ready-but- not-serving exit. Both outcomes are "not usable", and this PR's goal is that success means usable — so automation reading the exit code must not see 0 for a branch stuck provisioning. Test added (getBranchApi stuck in 'creating' → exit 1), with the shared mock impl restored so it can't leak the full poll budget into later tests. - Software engineering: moved `export const NETWORK_ERROR_CODE` below the import block in platform.ts (it was interleaved between two imports — legal via hoisting, but conventionally declarations sit under the imports). The `/api/health` "listening ≠ usable" note is acknowledged as the best client-side signal available today and left as the PR's documented open question, to key off a data-plane-scoped signal if one lands cloud-side. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough review — all four points addressed in 42f9be6. Suggestion — inconsistent exit semantics. Fixed. The stuck-provisioning path ( Suggestion — Information — Information — double error surface on the not-serving path. Left as-is intentionally: the spinner error frame names the branch state and the Build + lint clean; the new test and the existing branch-create suite pass. (The 4 unrelated |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/branch/create.test.ts (1)
354-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
process.exit/process.stderr.writemocking boilerplate.This override/restore pattern (lines 354-359 here, and repeated at 470-475, 519-522, 555-559) is duplicated verbatim across four new tests. Consider extracting a small helper (e.g.
withCapturedExit(fn)) to reduce copy-paste risk and shrink each test body.♻️ Example helper
async function withCapturedExit(fn: () => Promise<void>): Promise<number | undefined> { let exitCode: number | undefined; const origExit = process.exit; const origStderr = process.stderr.write.bind(process.stderr); process.exit = ((code?: number) => { exitCode = code; throw new Error('__exit__'); }) as typeof process.exit; process.stderr.write = (() => true) as typeof process.stderr.write; try { await fn(); } finally { process.exit = origExit; process.stderr.write = origStderr; } return exitCode; }🤖 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 `@src/commands/branch/create.test.ts` around lines 354 - 359, Extract the repeated process.exit and process.stderr.write override/restore logic from the affected tests into a shared helper such as withCapturedExit. Ensure the helper captures the exit code, restores both originals in a finally block, and returns the captured code; update the four tests to execute their assertions through this helper.
🤖 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 `@src/commands/branch/create.test.ts`:
- Around line 354-359: Extract the repeated process.exit and
process.stderr.write override/restore logic from the affected tests into a
shared helper such as withCapturedExit. Ensure the helper captures the exit
code, restores both originals in a finally block, and returns the captured code;
update the four tests to execute their assertions through this helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1901de5a-a469-4e42-9ca4-ca747bafe653
📒 Files selected for processing (3)
src/commands/branch/create.test.tssrc/commands/branch/create.tssrc/lib/api/platform.ts
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…it-capture test helper Addresses the two follow-up reviews on InsForge#201: - cubic (P2): a transport failure whose response leg is lost could, in the 60s skew window, adopt a collaborator's SAME-NAME branch — and a default --switch would then move local context onto it. createBranchOrAdopt now also requires the candidate's mode to match the requested mode, narrowing the collision to an even more specific coincidence (same name AND same mode AND the same ~60s AND our transport failure). The real fix is a server-issued idempotency token (InsForge/InsForge#1790); this is the tightest client-side guard until then. Test added: a same-name, different-mode branch in the window is NOT adopted. - CodeRabbit (nitpick): extracted `withCapturedExit(fn)` for the repeated process.exit/stderr override-restore boilerplate, and applied it to the tests this PR added (incl. the fake-timer provisioning-timeout one). The older pre-existing blocks are left as-is to keep this diff scoped to the PR's surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both follow-up reviews addressed in cubic (P2) — collaborator same-name adoption in the skew window. Valid, fixed. CodeRabbit (nitpick) — repeated Build + lint clean; |
|
@jwfing — re-review request (I don't have permission to re-request formally from a fork, so flagging here). Two commits landed after your review:
Net: the diff since your pass adds the mode guard + the timeout exit + test/lint tidy; nothing else changed. |
2 participants
Follow-up to InsForge/InsForge#1790, fixing the half of it that lives in this repo. The root cause of the readiness gap is cloud-side, but the CLI is where it becomes a broken user experience, and all three of these are fixable here.
Found while automating branch creation on
ap-southeast. Every measurement below is from real branches, all of which were deleted afterwards.1.
readyis a control-plane state, not readinesspollUntilReadyreturns the momentbranch_state === 'ready'and never contacts the branch's own host. Butbranch_stateflips when the provisioning job returns, while the instance is still coming up — every request tohttps://<appkey>.<region>.insforge.appresets until it does.Measured: the host started serving at t+2m in one case and t+11.5m in another, with
branch_statereadingreadythe whole time.Because
createthen auto-switches the directory onto that host, the failure surfaces on the user's next command rather than here:Now: once the control plane says ready, poll
GET /api/healthon the branch itself until it answers, and say so in the spinner. If it never answers within the budget, the command reports that honestly instead of claiming success — the branch's name and id are still printed, because it is real and it is billing.2. The 5-minute poll ceiling was below the observed provisioning time
POLL_TIMEOUT_MSwas 5 minutes, so the 11.5-minute branch was reported as "still in 'creating' state" when it was simply not finished yet. Raised to 15, with a separate 10-minute budget for the data-plane wait.3. A failed
createcan leave a live branch behindcreateBranchApicarries no idempotency key. A transport failure on the response leg — the POST arrived and the branch was created — throws beforecreatedis bound, so the CLI exits non-zero with no id and no name while a branch exists and bills. We hit exactly this:It then refused
deletefor about five minutes with"Branch is currently busy (creating or merging)."before the state flipped and the delete succeeded.Now: on a create failure, ask
branch list— a control-plane call, so it still works while the branch's own host is unreachable — whether a branch exists under that name, and adopt it if so. The original error is rethrown unchanged when nothing was created.Also
ossFetchcalledfetchunguarded, unlikeplatformFetch, so a dead data plane surfaced as the genericUNKNOWN_ERRORinstead of naming the host. The newprobeBackendHealthroutes its errors throughformatFetchError, soConnection to <host> was resetis what a caller sees. That one line cost us a lot of triage time.Notes for review
beforeEachresets it explicitly, becauseclearAllMockskeeps implementations and an unreachable branch would otherwise leak into every later test.npm run buildpasses;vitestpasses for everything I touched. Four unrelated failures (flyctl×3,migrations×1) reproduce on a cleanmainon this machine (Windows) and are untouched by this change.readybut never serves now stops the spinner with an error frame instead of a success line. That seemed right — it is the case where continuing would break the user's next command — but say the word if you would rather it warn and exit 0.Agent skills
insforge-cli/references/branch/overview.mdsays a branch takes 30–120s and thatreadymeans "usable — can be switched, modified, merged, or reset", and its post-create checklist goes straight tofunctions deploy. Both look worth updating inInsForge/agent-skillsonce you have confirmed the timing on your side — happy to open that PR alongside this one.Open question this does not answer
Is there a branch-scoped signal that reflects the branch's own data plane rather than the provisioning job — or would you accept
waitForCompletion=trueonPOST /projects/v1/{id}/branches, matching the convention already used byprojects restartandbackups create?/api/healthis the best thing available to a client today, but it is a static handler that never touches Postgres, so it proves the process is listening rather than that the branch is usable. If a better signal exists or is planned, this polling should key off that instead.Summary by cubic
Make
branch createreport success only when the branch is actually usable. It now waits for the branch host to serve, fails on stuck provisioning, and adopts an existing branch only in safe, verified cases after transport errors.ready, poll the branch host’s/api/health; includeservingin--json, print the branch identity first, and exit non‑zero if not serving within 10 min.readywithin the 15‑minute poll budget.NETWORK_ERROR_CODE), only if a same‑name branch was created at/after the request time (60s skew) and with a matching mode. Never adopt on API rejections (e.g., duplicate name) or pre‑existing branches.probeBackendHealthroutes throughformatFetchError, so messages name the branch host.Written for commit ea18b81. Summary will update on new commits.
Summary by CodeRabbit
branch createby adopting a matching existing branch when failures are due to transient connection issues.Note
Fix
branch createto only report success when the branch is ready and servingwaitUntilServingstep after provisioning completes: polls/api/healthon the branch host every 5 seconds for up to 10 minutes before reporting success.createBranchOrAdopt: on a transport-layer failure during creation, lists branches and adopts a matching one (by name, mode, andcreated_at) instead of failing immediately.NETWORK_ERROR_CODEinplatformFetchso callers can distinguish transport failures from HTTP errors.branch createnow exits non-zero if the branch provisions but never serves within the health timeout, where previously it would exit zero after provisioning.Macroscope summarized ea18b81.