Repository navigation
Roll the Base create back when the owner network resolve fails - #15358
teamleaderleo merged 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWhen owner-network resolution fails during Base creation, the workflow refunds the reserved credit, marks the Base generation failed with the provider-unavailable code, and records a failure event. A test checks the returned error and cleanup behavior. ChangesBase create failure handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The normal network-failure path now refunds the reserved credit and releases the Base. If the refund fails, however, the Base can still be released for another attempt, leaving a customer exposed to repeated charges during an outage. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
|
All contributors have signed the CLA ✍️ ✅ |
…ails finishBaseCreate reserves the create credit and records vm.create.requested before it resolves the owner network, and the resolve step has no failure handler. The credit stays spent for a machine that was never created, the requested event never gets a terminal event, and the base and its generation are left claimed. Red before the fix. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…fails The resolve_network step in finishBaseCreate had no failure handler, unlike every step around it. A failure there left the create credit spent, left vm.create.requested with no terminal event, and left the base at state "resetting" with its generation at "creating". Reset then tripped the existingOperationInFlight guard in beginBaseReset. Open has no such guard, but could not finish either: finishBaseCreate returns the same 409 when the existing row has no providerVmId. Both cleared only when markCreateAbandoned reclaimed the row, which takes VM_CREATE_ABANDONED_AFTER_MS plus a run of the ten-minutely vm-reconcile cron, so up to about forty minutes. The credit was never refunded, because the sweeper refunds nothing. Give the step the same rollback the model_plane_provision step below it already has: refund, markBaseCreateFailed with the base, generation, vm and user ids, and a vm.base.create.failed event naming the step. The catchAll tail keeps a failing rollback from masking the network error. openBaseVm, resetBaseVm and reopenBaseIfProviderDeleted all funnel through finishBaseCreate, so all three entrypoints are covered. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ebaf259 to
faaf5bb
Compare
markCreateAbandoned and resolveCreateCleanup also call restoreBaseAfterCreateFailure. What is true is that it is the mark on this code path that does, and that neither of the other two can reach a row the ad-hoc mark has already stamped with a failure code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review: a review subagent went over Fixed
Left, disclosed
Verification after the rebase, all from
No fleet dogfood: this is a |
|
Merge receipt for |
1028a08 test: isolate background workspace git probe fixture (manaflow-ai#15388) 41ad40d fix: keep the terminal area when the window is too narrow for the side panels (manaflow-ai#15369) 2890f0b Roll the Base create back when the owner network resolve fails (manaflow-ai#15358) 7b0a15f Keep agent- and script-opened workspaces and panes in the background (manaflow-ai#15281) 4f14fa3 ci: move CLI regressions to CLI product tests and rebalance the seven app-host shards (manaflow-ai#15177) 906926a ci: dogfood builds are opt-in with the dev-build label (manaflow-ai#15380) 2f6716c PR media: classify app changes by CI's build inputs; a reuse error is no refusal (manaflow-ai#15386) bc28bc4 Release the Base generation when a create is refused for credits (manaflow-ai#15343) 6760c93 iOS: Add Computer never disturbs the active Mac (manaflow-ai#15102) 0f2d3d3 Show Claude sessions that stop on an API error instead of leaving them Running (manaflow-ai#15232) 20ef7c9 Keep the main window floor on the animating setFrame path (manaflow-ai#15368) b4f5dc5 ci: move UI runs pinned to Blacksmith macOS 26 onto owned Macs (manaflow-ai#15383) e02c385 PR media: compile once when CI's build cannot load, and say why a tour skipped (manaflow-ai#15378) ebd1f4f fix(iroh-v2): commit delivery accounting only after the frame is sent (manaflow-ai#15344) # Conflicts: # .github/workflows/ci-guards.yml # .github/workflows/ci-macos.yml # .github/workflows/ci.yml # .github/workflows/pr-media.yml # .github/workflows/test-e2e.yml
1 participant
Summary
When a Base create cannot resolve the owner's private network, the customer was charged for a machine that was never created and the Base was left unusable for half an hour. After this change the credit goes back, the Base and its generation are released immediately, and the next open or reset works.
finishBaseCreatereserves the create credit and recordsvm.create.requested, and only then resolves the owner network. That resolve step had no failure handler, unlike every step around it. Three things followed from a failure there:vm.create.requestedhad no terminal event after it.resettingwith its generation stillcreating.markBaseCreateFailedis the only mark that callsrestoreBaseAfterCreateFailure, which fails the generation and promotes the previous retained generation back onto the base. Without it, theexistingOperationInFlightguard inbeginBaseOpenrefused every later open and reset withVmCreateInProgressError.The step now gets the same rollback the
model_plane_provisionstep immediately below it already has: refund the reservation, mark withmarkBaseCreateFailedcarrying the base id, generation, vm id and user id, and record avm.base.create.failedevent whosemetadata.operationnames the step. The failure code isPROVIDER_CREATE_UNAVAILABLE_FAILURE_CODE, matching the network handler increateVm. TheEffect.catchAll(() => Effect.void)tail keeps a failing rollback from masking the network error, so the caller still sees the network failure rather than a mark-path error.openBaseVm,resetBaseVmand the reopen-after-provider-deletion retry all funnel throughfinishBaseCreate, so all three entrypoints are covered by the one change.How long the Base stayed stuck
The wedge was temporary, not permanent.
markCreateAbandonedlooks up thecreatinggeneration by vm id and does callrestoreBaseAfterCreateFailure, so the abandonment sweeper released the base on its own. The stuck row still matched the sweeper's predicate, because nothing had marked it:statusstayedprovisioning,provider_vm_idandfailure_codestayed null. Recovery therefore took up toVM_CREATE_ABANDONED_AFTER_MS, two provider create deadlines, which is 30 minutes today. The credit loss was permanent either way, since the sweeper refunds nothing.Why add the rollback instead of reordering
resolveOwnerNetworktakes only a user id, provider, billing team id and team directory, so it does not depend on the reservation or the requested events and could run before them, the waycreateVmorders it. Reordering alone would not fix this, though:beginBaseOpenhas already created the base and generation rows by the timefinishBaseCreateruns, so a network failure needsmarkBaseCreateFailedwhichever order the two steps take. Reordering would additionally avoid reserving a credit that is about to be released, which is worth doing, but it changes the timing of a hot create path and is better as its own change than folded into a fix. Left as a follow-up.Steps audited
Every step in
finishBaseCreate, and the thirdreserveCreateCreditcaller:begin_base_open(in the callers)reserveCreateCreditmarkCreateFailed, so the base and generation are not released. Same family of defect, already owned by #15343; left alone to avoid a conflicting duplicate fixrecordCreateRequestedEventsresolve_networkmodel_plane_provisionmarkBaseCreateFailed,vm.base.create.failedprovider_createmarkBaseCreateFailedwith cleanup-pending handlingmark_base_runningmarkBaseCreateFailedrecordCreateSuccessEvents,schedulePromptIdentityPush, base usage eventnativeForkOperation, the thirdreserveCreateCreditcallermarkCreateFailedis correct there, and it has no network resolve stepSo
resolve_networkwas the only step in a Base flow with no rollback at all.Expected conflicts
finishBaseCreateat thereserveCreateCreditcall and thereserveCreateCreditdefinition. Its hunk sits a few lines above mine and passes abaseGenerationoption down; the two changes are adjacent but independent and should merge with a small manual resolution.reserveCreateCredit.Testing
bun test tests/vm-workflows.test.tsfromweb/, on the two commits in this PR. The regression is a plaintest, not adbTest, so it runs without a database.Red, on the test-only commit
45c644e57bf:The two assertions before the failure already pass, which is the point: the caller does see the network error, but nothing was refunded.
Green, on the fix commit
ebaf25945ab:Whole file on the fix commit:
68 pass, 75 skip, 0 fail, 252 expect() calls. The 75 skips are thedbTestcases, which needCMUX_DB_TEST=1and a Postgres this run did not have, so this establishes nothing about the database-backed paths.Also on the fix commit:
bun x tsc --noEmitclean,bun run lint:complexityreports42 findings matched the grandfathered baselinewith no baseline edit, andbun x eslint services/vms/workflows.ts tests/vm-workflows.test.tsexits 0 with two pre-existing unused-import warnings in the test file that this change did not introduce.Not verified: no live Cloud run. The behavior is covered at the workflow level with a stub repository, billing gateway and provider gateway.
Changelog
Fixed: A Cloud Base machine whose private network could not be resolved now refunds the create credit and frees the Base right away, instead of charging for the machine and refusing to open or reset it for the next half hour
🤖 Generated with Claude Code
Summary by cubic
Fixes a Base create so a failure to resolve the owner's private network refunds the reserved create credit and releases the Base immediately, instead of charging for a machine that was never created and leaving the Base unable to open or reset for up to about forty minutes.
finishBaseCreatereserves the credit before resolving the owner network, and that step had no failure handler; it now refunds the reservation, marks viamarkBaseCreateFailed, and records avm.base.create.failedevent naming the step.catchAlltail keeps a failing rollback from masking the network error, so the caller still sees that error.openBaseVm,resetBaseVm, and the reopen-after-provider-deletion retry all funnel throughfinishBaseCreate, so all three entrypoints are covered.Written for commit 3ab0574. Summary will update on new commits.
Summary by CodeRabbit