fix(connect): remove tunnels after hosts go offline - #8153
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
github-actions
Bot
added
vouch:trusted
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This change adds a new managed-tunnel recovery workflow, persistent recovery state, startup retries, and automated Cloudflare tunnel deletion, with significant lifecycle and authorization-sensitive side effects. Unresolved findings also identify concurrency hazards around tunnel deletion and recovery retries, requiring human assessment. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
Effect service conventions: four findings, all in newly added error-handling code (three Effect.catchTag usages and one startup Effect.catchCause). Details inline.
Posted via Macroscope — Effect Service Conventions
| Effect.catchTag("ManagedEndpointTunnelClientError", (error) => | ||
| ManagedEndpointProvider.isManagedEndpointNotFound(error.cause) | ||
| ? Effect.succeed(true) | ||
| : Effect.fail(error), | ||
| ), |
There was a problem hiding this comment.
Same here: prefer Effect.catchTags({ ... }) over Effect.catchTag.
| Effect.catchTag("ManagedEndpointTunnelClientError", (error) => | |
| ManagedEndpointProvider.isManagedEndpointNotFound(error.cause) | |
| ? Effect.succeed(true) | |
| : Effect.fail(error), | |
| ), | |
| Effect.catchTags({ | |
| ManagedEndpointTunnelClientError: (error) => | |
| ManagedEndpointProvider.isManagedEndpointNotFound(error.cause) | |
| ? Effect.succeed(true) | |
| : Effect.fail(error), | |
| }), |
Posted via Macroscope — Effect Service Conventions
| Effect.catchCause((cause) => | ||
| Effect.logWarning("Failed to recover the T3 Connect managed tunnel", { cause }), | ||
| ), |
There was a problem hiding this comment.
Effect.catchCause also absorbs interruption, so shutdown-time interruption of this startup/recovery effect (it is also driven by the forked recoveryRequests loop) is downgraded to a warning and reported as success. Suggest handling only the typed error channel with Effect.catch, matching the adjacent reconcileDesiredCloudLink block.
| Effect.catchCause((cause) => | |
| Effect.logWarning("Failed to recover the T3 Connect managed tunnel", { cause }), | |
| ), | |
| Effect.catch((cause) => | |
| Effect.logWarning("Failed to recover the T3 Connect managed tunnel", { cause }), | |
| ), |
Posted via Macroscope — Effect Service Conventions
| Effect.catchTag("ManagedEndpointTunnelClientError", (error) => | ||
| ManagedEndpointProvider.isManagedEndpointNotFound(error.cause) | ||
| ? Effect.succeed(Option.none()) | ||
| : Effect.fail(error), | ||
| ), |
There was a problem hiding this comment.
Convention: catch statically known tagged failures with Effect.catchTags({ ... }), even for a single tag — Effect.catchTag should not be used in new code.
| Effect.catchTag("ManagedEndpointTunnelClientError", (error) => | |
| ManagedEndpointProvider.isManagedEndpointNotFound(error.cause) | |
| ? Effect.succeed(Option.none()) | |
| : Effect.fail(error), | |
| ), | |
| Effect.catchTags({ | |
| ManagedEndpointTunnelClientError: (error) => | |
| ManagedEndpointProvider.isManagedEndpointNotFound(error.cause) | |
| ? Effect.succeed(Option.none()) | |
| : Effect.fail(error), | |
| }), |
Posted via Macroscope — Effect Service Conventions
| Effect.catchTag("ManagedEndpointTunnelClientError", (cause) => | ||
| isManagedEndpointNotFound(cause.cause) | ||
| ? Effect.succeed(Option.none()) | ||
| : Effect.fail( | ||
| new ManagedEndpointDeprovisioningFailed({ | ||
| ...input, | ||
| stage: "load-tunnel", | ||
| tunnelId, | ||
| cause, | ||
| }), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
New tagged-failure handling should use Effect.catchTags({ ... }) (this module already uses it elsewhere) rather than Effect.catchTag.
| Effect.catchTag("ManagedEndpointTunnelClientError", (cause) => | |
| isManagedEndpointNotFound(cause.cause) | |
| ? Effect.succeed(Option.none()) | |
| : Effect.fail( | |
| new ManagedEndpointDeprovisioningFailed({ | |
| ...input, | |
| stage: "load-tunnel", | |
| tunnelId, | |
| cause, | |
| }), | |
| ), | |
| ), | |
| Effect.catchTags({ | |
| ManagedEndpointTunnelClientError: (cause) => | |
| isManagedEndpointNotFound(cause.cause) | |
| ? Effect.succeed(Option.none()) | |
| : Effect.fail( | |
| new ManagedEndpointDeprovisioningFailed({ | |
| ...input, | |
| stage: "load-tunnel", | |
| tunnelId, | |
| cause, | |
| }), | |
| ), | |
| }), |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 26c5820. Configure here.
| ) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Release deletes after stale checks
High Severity
release refreshes allocation generation and Cloudflare status after claimRelease, but still deletes without a final generation claim. recordTunnel updates tunnelId/updatedAt with no CAS on the claimed generation, so a concurrent provision can adopt the same tunnel after the refresh and before tunnels.delete, then receive credentials for a tunnel the reaper has just removed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 26c5820. Configure here.
| Effect.retry({ | ||
| while: (error) => | ||
| error._tag !== "EnvironmentHttpBadRequestError" && | ||
| error._tag !== "EnvironmentCloudEndpointUnavailableError", |
There was a problem hiding this comment.
Recovery aborts on start failure
Medium Severity
The new recovery retry while filter stops on EnvironmentCloudEndpointUnavailableError, which recoverManagedCloudTunnel throws when applyConfig returns a non-running status after the relay has already provisioned. Transient connector start failures (missing binary, spawn glitch) therefore end recovery immediately instead of retrying for up to ten minutes, and with no connector running nothing republishes a recovery request.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 26c5820. Configure here.
| break; | ||
| } | ||
| const allocation = recordedByTunnelName.get(tunnel.name); | ||
| if (allocation !== undefined && allocation.tunnelId !== tunnel.id) { |
There was a problem hiding this comment.
🟡 Medium environments/ManagedEndpointReaper.ts:169
The reaper permanently skips Cloudflare tunnels whose same-name allocation has tunnelId === null, leaving tunnels created before a crash or persistence failure undeleted and still billed. The mismatch check should only skip allocations with a recorded, different tunnel ID so this case reaches reclamation.
| if (allocation !== undefined && allocation.tunnelId !== tunnel.id) { | |
| if (allocation !== undefined && allocation.tunnelId !== null && allocation.tunnelId !== tunnel.id) { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @infra/relay/src/environments/ManagedEndpointReaper.ts around line 169:
The reaper permanently skips Cloudflare tunnels whose same-name allocation has `tunnelId === null`, leaving tunnels created before a crash or persistence failure undeleted and still billed. The mismatch check should only skip allocations with a recorded, different tunnel ID so this case reaches reclamation.
Evidence trail:
Commit 26c5820: infra/relay/src/environments/ManagedEndpointReaper.ts:159-175; infra/relay/src/environments/ManagedEndpointProvider.ts:754-814; infra/relay/src/environments/ManagedEndpointAllocations.ts:214-225,269-289. Cloudflare billing context: https://blog.cloudflare.com/tunnel-for-everyone/
| target: deprovisionTarget, | ||
| }); | ||
| if (!deprovisioned && (yield* links.getForUser(input)) === null) { | ||
| yield* managedEndpointProvider.deprovision(input); |
There was a problem hiding this comment.
🟠 High http/Api.ts:465
The fallback deprovision(input) can delete a newly linked environment's tunnel and DNS record. After the first call returns false, a concurrent relink can replace the allocation before links.getForUser(input) and the fallback call, which reloads and deprovisions that newer allocation; pass the original deprovisionTarget to the retry.
| yield* managedEndpointProvider.deprovision(input); | |
| yield* managedEndpointProvider.deprovision({ | |
| ...input, | |
| target: deprovisionTarget, | |
| }); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @infra/relay/src/http/Api.ts around line 465:
The fallback `deprovision(input)` can delete a newly linked environment's tunnel and DNS record. After the first call returns `false`, a concurrent relink can replace the allocation before `links.getForUser(input)` and the fallback call, which reloads and deprovisions that newer allocation; pass the original `deprovisionTarget` to the retry.
Evidence trail:
Commit 26c5820. Files: infra/relay/src/http/Api.ts:438-465; infra/relay/src/environments/EnvironmentLinker.ts:311-332; infra/relay/src/environments/ManagedEndpointProvider.ts:470-520, 532-563; infra/relay/src/environments/ManagedEndpointAllocations.ts:442-469. Git: `git show 26c5820 -- infra/relay/src/http/Api.ts`
| endpointRuntimeStatus, | ||
| }); | ||
| } | ||
| return yield* dependencies.endpointRuntime.withLinkStateLock( |
There was a problem hiding this comment.
🟠 High cloud/http.ts:459
reconcileDesiredCloudLinkWith can restore a link after unlink has successfully removed it, leaving CLOUD_LINKED_USER_ID and the runtime config present and restarting the connector for the revoked link. The reconcile flow performs its remote request and setCliDesiredCloudLink(true, mode) before applyCloudRelayConfig acquires this lock, so validateLinkedCloudUser accepts the now-missing user; synchronize the entire reconcile/unlink sequence, not only the config application.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/cloud/http.ts around line 459:
`reconcileDesiredCloudLinkWith` can restore a link after `unlink` has successfully removed it, leaving `CLOUD_LINKED_USER_ID` and the runtime config present and restarting the connector for the revoked link. The reconcile flow performs its remote request and `setCliDesiredCloudLink(true, mode)` before `applyCloudRelayConfig` acquires this lock, so `validateLinkedCloudUser` accepts the now-missing user; synchronize the entire reconcile/unlink sequence, not only the config application.
Evidence trail:
Reviewed commit 26c5820: apps/server/src/cloud/http.ts:221-245 (`validateLinkedCloudUser` accepts a missing user); 454-505 (`applyCloudRelayConfig` locks only the config application, validates, persists `CLOUD_LINKED_USER_ID` at 484, and applies runtime config at 467); 552-633 (`reconcileDesiredCloudLinkWith` performs relay requests and calls `setCliDesiredCloudLink(true, mode)` at 621 before `applyCloudRelayConfig`); 886-905 (`cloudUnlinkHandler` removes the link and runtime secrets under the same lock). apps/server/src/cloud/ManagedEndpointRuntime.ts:306-317 (runtime application and link-state lock implementation).
| claimRelease: Effect.fn("relay.managed_endpoint_allocations.claim_release")(function* ( | ||
| input: ClaimManagedEndpointReleaseInput, | ||
| ) { | ||
| const claimedAt = DateTime.formatIso(yield* DateTime.now); |
There was a problem hiding this comment.
🟠 High environments/ManagedEndpointAllocations.ts:412
claimRelease can delete a newly recorded, live tunnel: if recordTunnel runs in the same millisecond after the claim, its updatedAt equals claimedAt, so the generation check incorrectly succeeds. Use a genuinely unique generation (or an atomic claim/version) instead of the millisecond-precision wall-clock string.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @infra/relay/src/environments/ManagedEndpointAllocations.ts around line 412:
`claimRelease` can delete a newly recorded, live tunnel: if `recordTunnel` runs in the same millisecond after the claim, its `updatedAt` equals `claimedAt`, so the generation check incorrectly succeeds. Use a genuinely unique generation (or an atomic claim/version) instead of the millisecond-precision wall-clock string.
Evidence trail:
Commit 26c5820: infra/relay/src/environments/ManagedEndpointAllocations.ts:269-289, 409-440; infra/relay/src/environments/ManagedEndpointProvider.ts:587-631, 674-691, 754-814; infra/relay/src/persistence/schema.ts:86-105. Effect DateTime documentation: https://www.effect.website/docs/v4/data-types/datetime. Verification commands: `git show 26c5820 -- infra/relay/src/environments/ManagedEndpointAllocations.ts infra/relay/src/environments/ManagedEndpointProvider.ts`; `git grep -n -C 4 'recordTunnel\|claimRelease\|claimedAt' 26c5820 -- infra/relay/src`.
1 participant
Cloudflare tunnels stayed provisioned after an environment crashed or went offline. Environments linked through web or mobile could not recreate a deleted tunnel.
The relay now removes tunnels that stay disconnected for at least five minutes. Updated hosts recreate tunnels under the same URL, while older hosts remain protected until they register recovery support.
Verified with 146 focused tests, 129 server integration tests, and typechecks for the relay, server, shared client runtime, web app, and mobile app.
Built by GPT-5.6 Sol with the Codex harness.
Note
Medium Risk
Changes managed tunnel provisioning, release races, and credential-backed recovery across server and relay; incorrect locking or stale release claims could delete live tunnels or apply wrong config.
Overview
Linked environments can automatically get a replacement Cloudflare tunnel after a crash or long offline period, without re-pairing, while keeping the same public URL.
On the T3 server, persisted link state and tunnel config updates run under a
withLinkStateLocksemaphore. Saving relay config canrequestRecovery; whencloudflaredexits, the managed runtime publishes a recovery signal. Startup (and that stream) runsrecoverManagedCloudTunnel, which calls the relay with the stored environment credential and applies the returned runtime config only if local persisted config still matches.On the relay,
POST /v1/environments/:environmentId/tunnelprovisions a new tunnel for an authenticated environment, verifies the endpoint URL is unchanged, and setsrecovery_enabled_aton the allocation. AManagedEndpointReapercron (every 5 minutes) deletesdown/inactivestage tunnels past a 5-minute grace period—orphans directly, recoverable allocations viareleasewith expected tunnel id, status, and inactive timestamp checks so reconnects or newer provisions are not torn down. Legacy allocations without recovery support are skipped until they complete a recovery request.Contracts and docs describe the lifecycle;
claimReleasenow returns a claim generation timestamp instead of a bare boolean.Reviewed by Cursor Bugbot for commit 26c5820. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Remove managed tunnels after hosts go offline and add recovery on reconnect
ManagedEndpointReapersweep job that scans Cloudflare tunnels withdown/inactivestatus past a 5-minute grace period and deletes orphans or releases recoverable tunnels viaprovider.releasewith tunnel-id/inactivity guardsPOST /v1/environments/:environmentId/tunnel(Api.ts) that provisions a replacement tunnel for a linked environment, plus server-siderecoverManagedCloudTunnelthat subscribes torecoveryRequestsand retries with exponential backoffrecoveryEnabledAtcolumn torelay_managed_endpoint_allocations(migration.sql) andenableRecovery/listByTunnelNamesmethods onManagedEndpointAllocations\n- Serializes cloud link-state mutations underwithLinkStateLock(a semaphore) inapplyCloudRelayConfig,unlink, and recovery flows to prevent racesManagedEndpointProvider.deprovisionandreleasenow returnbooleaninstead ofvoid;claimReleasereturns a claim timestampstring | nullinstead ofboolean. Callers likeunlinkEnvironmentRecordretry deprovision when the first attempt returnsfalseand the link is already removed. The relay cron job now runs reaper sweep concurrently with existing pruning.Macroscope summarized 26c5820.