[codex] Structure managed endpoint allocation failures#3421
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Approved Bug fix that improves error handling by properly surfacing DNS client failures instead of silently swallowing them. Changes include stricter tunnel response validation and mechanical removal of unnecessary wrapper functions, all covered by tests. You can customize Macroscope's approvability policy. Learn more. |
575a4b3 to
300483c
Compare
Dismissing prior approval to re-evaluate 300483c
71c8e09 to
9e1cf09
Compare
9e1cf09 to
b2cef03
Compare
b2cef03 to
2d14ea0
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
2d14ea0 to
10793eb
Compare
Dismissing prior approval to re-evaluate 10793eb
Summary
Errorand valueless persistence-error constructor wrapper with structural errors; invalid tunnel responses are a distinct cause-free error nested under the provisioning failureEffect.catchTagsfor tunnel/DNS recovery and only treat structured nestedNotFoundfailures as idempotent fallbackmakevaluesVerification
vp test infra/relay/src/environments/ManagedEndpointAllocations.test.ts infra/relay/src/environments/ManagedEndpointProvider.test.ts(19 passed)vp check(passes with 20 unrelated existing warnings)vp run typecheckScope
Excludes relay orchestration/projection and files owned by #3331, #3334, #3335, and #3392.
Note
Medium Risk
Changes live in Cloudflare tunnel/DNS provisioning paths; misclassified errors could break retries or hide outages, though behavior is narrowed and tested for non-not-found checkpoint failures.
Overview
Tightens managed endpoint provisioning so transient DNS “not found” cases still fall back, but real failures surface correctly.
Checkpoint DNS updates no longer swallow every
updateRecorderror viaorElseSucceed; only structuredNotFoundcauses are treated as “record gone, reconcile elsewhere.” Other errors (e.g. Cloudflare outage) fail atensure-dns-recordinstead of silently taking another DNS path—covered by a new test.Tunnel validation now requires the created tunnel’s name to match the expected stable name (not merely non-null), with
returnedTunnelNameon the provisioning failure when it mismatches.Exports and layers:
makeis exported on allocations and provider; tunnel/DNSlayer*helpers pass client implementations directly without extraof()wrappers; Cloudflare binding layer is flattened accordingly. Test fakes use taggedNotFoundcauses for missing DNS records.Reviewed by Cursor Bugbot for commit 10793eb. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden managed endpoint allocation failure handling in
ManagedEndpointProviderensureDnsRecordnow rethrows DNSupdateRecorderrors unless the cause isNotFound; previously all errors were swallowed and treated as a miss, masking real failuresvalidate-tunnel-responseand includereturnedTunnelNamein the errormakefactory from bothManagedEndpointProviderandManagedEndpointAllocationsfor external constructionmakeTunnelClient/makeDnsClientwrapper functions; service objects are now passed directly tolayerTunnelClient/layerDnsClientensureDnsRecordchange is a behavioral breaking change — callers that previously succeeded despite DNS update errors will now see failures propagatedMacroscope summarized 10793eb.