fix(workspaces): stop a destroy that succeeded from answering 500 - #99
Open
pythonlearner1025 wants to merge 1 commit into
Open
fix(workspaces): stop a destroy that succeeded from answering 500#99pythonlearner1025 wants to merge 1 commit into
pythonlearner1025 wants to merge 1 commit into
Conversation
A destroy returned 500 while the server was deleted, the row reached destroyed, and no error was recorded anywhere. Two contract violations between them account for exactly that. WorkspaceTunnels.cleanup is documented as "callers log the returned errors", and it wraps a client whose own cleanup "never throws". Its D1 write was outside that contract: a transient D1 failure threw out of cleanup. Destroy calls cleanup AFTER vmProvider.destroy has already deleted the server, so that throw reached the router's onError as an opaque 500 for work that had irreversibly half-succeeded. The row stayed in destroying, the orphan sweep found the server already gone, skipped its own destroy, and transitioned the row to destroyed with error = NULL. Server deleted, row destroyed, nothing recorded, 500 to the caller. cleanup now pushes that failure into result.errors. Not clearing the columns is the safe half: both Cloudflare deletes tolerate an already-deleted resource, so the janitor's retry is a no-op and clears them then. Destroy's existing "honest destroy" branch handles it — 200, phase destroying, janitor finishes. The caller also dropped cleanup.errors on the floor, which is the "no error recorded" half. It now reports them, because the janitor's transition sets error back to NULL and these errors are the only account of why a destroy needed two attempts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J6fUBY1B27EzvDwbhfBf52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A destroy returned 500 while the server was deleted, the row reached
destroyed, and no error was recorded. Here is the whole path, and the two contract violations that produce exactly that triple.What happened
DELETE /workspaces/:idreachesawait vmProvider.destroy(row.vm_id)and Hetzner deletes the server. This is the irreversible point.workspaceTunnels.cleanup(runtime.db, row). It callsCloudflareTunnels.cleanup, which is documented "Never throws: the caller decides what a partial failure means" — and it does not. ButWorkspaceTunnels.cleanupthen does its own D1 write to cleartunnel_id/dns_record_id, outside that contract. A transient D1 failure throws out ofcleanup.router.onError, missesHttpErrorandframeworkHttpError, and becomes{"error": "internal server error"}with status 500.destroying, because the final transaction never ran.runOrphanSweeppicks the row up. It callsprovider.inspect(row.vm_id), which returnsnullfor the already-deleted server, so it skips its owndestroy. It then transitionsdestroying→destroyed— settingerror = NULL.Net: server deleted, row destroyed, no error on the row, and a 500 in the caller's hand. Nothing about it is mysterious once the throw site is on the wrong side of the never-throws line.
The second half: why nothing was recorded
cleanupis documented "Callers log the returned errors". The destroy handler does not — it checkscleanup.errors.length > 0, takes the honest-destroy branch, and drops the errors on the floor. So even on the path that already worked correctly, the reason a destroy needed two attempts was never written down. Combined with the janitor clearingerrorto NULL, a destroy that failed once leaves no trace at all.The fix
WorkspaceTunnels.cleanupno longer throws. Its D1 write is wrapped and the failure joinsresult.errors. Not clearing the columns is the safe half of that failure: both Cloudflare deletes passnotFoundIsNull, so the janitor's retry is a no-op against Cloudflare and clears the columns then. The existing honest-destroy branch takes over — 200, phasedestroying, janitor finishes — which is already how this exact outcome is handled when the client reports rather than throws.The caller reports
cleanup.errorsviaruntime.reportError("workspace_destroy_cleanup_incomplete", …).The remaining 500
The final
transaction(...)can still throw, and still yields the same triple. That one is a genuine unexpected DB failure and a 500 is the honest answer, so it is left alone — but it is now the only way to reach that state, rather than one of two.Verification
New test in
test/workspace-tunnels.test.tsdrivescleanupwith aDbwhoserun()throws, and asserts Cloudflare's half still reports deleted while the D1 failure arrives as an entry inerrorsrather than an exception.All three gates pass:
npm run typecheck,npm run lint:gate(102 anti-slop / 0 blitz-house, baseline unchanged), and the control-plane suite at 626 passed / 38 skipped.🤖 Generated with Claude Code
https://claude.ai/code/session_01J6fUBY1B27EzvDwbhfBf52