A push cut off by a deploy is retried instead of failing with a 500 (#177)
A deploy resets the repository Durable Objects, and git never retries a push. The Worker now hands such a push to the new code once more, through a new stub, and deletes the R2-staged body if that fails too. Any other error is not retried. The import campaign's push test prints server errors and recent deploys when a push doesn't land. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AizKXSsengXZRegJxRATkL
Canberk Demirci authored · GitHub committed signed1 parent f24bc68commit e9ed630eed9fa9b548263bb77128b4b170798a7eBrowse files
3 files changed+93−3
modified.github/workflows/import-campaign.yml+9−1View file
| @@ -110,7 +110,15 @@ | ||
| 110 | 110 | want=$(git rev-parse refs/heads/one-push) |
| 111 | 111 | got=$(git -c "http.extraHeader=$auth" ls-remote "$DIFFINERY/$owner/$name.git" refs/heads/main | cut -f1) |
| 112 | 112 | echo "Diffinery has main at ${got:-nothing}; pushed $want" | tee -a "$GITHUB_STEP_SUMMARY" |
| 113 | - [ "$got" = "$want" ] || { echo "::error::the push did not land"; exit 1; } | |
| 113 | + if [ "$got" != "$want" ]; then | |
| 114 | + # What the server recorded for a 5xx (redacted; needs a site admin's token). | |
| 115 | + api GET "/diffinery/admin/errors?per_page=5" | jq -r --arg since "$(date -u -d "@$started" +%FT%TZ)" '.[] | select(.at >= $since) | "\(.at) \(.status) \(.method) \(.route): \(.message)\n\((.stack // "") | split("\n")[0:12] | join("\n"))"' || echo "(no server errors readable)" | |
| 116 | + # A deploy mid-push resets the repository's Durable Object; show whether one ran. | |
| 117 | + echo "Recent deploys on Diffinery:" | |
| 118 | + api GET "/repos/$R/actions/workflows/deploy.yml/runs?per_page=5" | jq -r '.workflow_runs[] | "\(.id) \(.head_branch) \(.head_sha[0:7]) \(.status) \(.conclusion) \(.created_at) \(.updated_at)"' || true | |
| 119 | + echo "::error::the push did not land" | |
| 120 | + exit 1 | |
| 121 | + fi | |
| 114 | 122 | cd .. && git -c "http.extraHeader=$auth" clone -q --bare "$DIFFINERY/$owner/$name.git" back |
| 115 | 123 | git -C back fsck --full --strict && echo "Cloned back and checked: $DIFFINERY/$owner/$name" | tee -a "$GITHUB_STEP_SUMMARY" |
| 116 | 124 | exit 0 |
modifiedapps/diffinery/src/http/git.ts+30−2View file
| @@ -5,7 +5,8 @@ | ||
| 5 | 5 | import { BodyTooLarge, putStream, type BlobBucket } from "../lib/blob-store.js"; |
| 6 | 6 | import { pushBodyKey, type SpilledBody } from "../lib/storage-keys.js"; |
| 7 | 7 | import { importPushAllowed } from "../imports/service.js"; |
| 8 | -import { loadRepo } from "./common.js"; | |
| 8 | +import { loadRepo, repoStub } from "./common.js"; | |
| 9 | +import type { Caller } from "../repo/types.js"; | |
| 9 | 10 | |
| 10 | 11 | /** |
| 11 | 12 | * Git smart HTTP (protocol v2 for fetch/clone, v0 for push) and the Git LFS |
| @@ -48,11 +49,38 @@ | ||
| 48 | 49 | const p = c.get("principal"); |
| 49 | 50 | if (p.type === "account" && p.importId && p.tokenId && (await importPushAllowed(c.env, p.importId, repo.id, p.tokenId))) caller.importId = p.importId; |
| 50 | 51 | const body = await readPushBody(c.env, repo.id, c.req.raw); |
| 51 | - const out = await stub.gitReceivePack(body, caller, { moved }); | |
| 52 | + const out = await receiveWithRetry(c.env, repo.id, stub, body, caller, moved); | |
| 52 | 53 | c.executionCtx.waitUntil(c.get("dir").touchPushed(repo.id)); |
| 53 | 54 | return new Response(out, { headers: { "content-type": "application/x-git-receive-pack-result", ...noCache } }); |
| 54 | 55 | }); |
| 55 | 56 | |
| 57 | +/** | |
| 58 | + * A deploy resets every Durable Object, and one doing a push then fails it mid-way: git doesn't | |
| 59 | + * retry a push, so it would print a 500. The push is handed over once more (to the new code; a | |
| 60 | + * large one is still staged in R2, as the reset repository never got to delete it). Nothing was | |
| 61 | + * lost or done twice: objects are stored by their ids, and refs move only at the end, each by | |
| 62 | + * compare-and-swap (one the reset push already moved is reported as stale, not moved again). | |
| 63 | + */ | |
| 64 | +export async function receiveWithRetry(env: Env, repoId: string, stub: ReturnType<typeof repoStub>, body: Uint8Array | SpilledBody, caller: Caller, moved: boolean): Promise<Uint8Array> { | |
| 65 | + for (let attempt = 1; ; attempt++) { | |
| 66 | + try { | |
| 67 | + return await stub.gitReceivePack(body, caller, { moved }); | |
| 68 | + } catch (e) { | |
| 69 | + if (attempt >= PUSH_TRIES || !resetByDeploy(e)) { | |
| 70 | + if (!(body instanceof Uint8Array)) await env.BUCKET.delete(body.spilled).catch(() => {}); | |
| 71 | + throw e; | |
| 72 | + } | |
| 73 | + // A stub that saw its object reset is broken; a new one reaches the new code. | |
| 74 | + stub = repoStub(env, repoId); | |
| 75 | + } | |
| 76 | + } | |
| 77 | +} | |
| 78 | + | |
| 79 | +const PUSH_TRIES = 2; | |
| 80 | + | |
| 81 | +/** The platform's error for a Durable Object stopped by a new version of its code. */ | |
| 82 | +export const resetByDeploy = (e: unknown) => e instanceof Error && /durable object reset because its code was updated/i.test(e.message); | |
| 83 | + | |
| 56 | 84 | /** A fetch's wants and haves; git's own negotiation stays far below this. */ |
| 57 | 85 | const MAX_UPLOAD_PACK_REQUEST = 16 * 1024 * 1024; |
| 58 | 86 | /** The repository refuses pushes over 100 MB with a clear message; this stops anything far beyond. */ |
addedapps/diffinery/test/unit/push-retry.test.ts+54−0View file
| @@ -1,0 +1,54 @@ | ||
| 1 | +import { describe, expect, it } from "vitest"; | |
| 2 | +import { receiveWithRetry, resetByDeploy } from "../../src/http/git.js"; | |
| 3 | +import type { Env } from "../../src/env.js"; | |
| 4 | +import type { Caller } from "../../src/repo/types.js"; | |
| 5 | + | |
| 6 | +const RESET = "Durable Object reset because its code was updated."; | |
| 7 | + | |
| 8 | +/** A repository namespace whose objects fail the first `failures` pushes with `message`. */ | |
| 9 | +function fakes(failures: number, message = RESET) { | |
| 10 | + let calls = 0; | |
| 11 | + const stubs: number[] = []; | |
| 12 | + const deleted: string[] = []; | |
| 13 | + const stub = (n: number) => ({ | |
| 14 | + gitReceivePack: async () => { | |
| 15 | + stubs.push(n); | |
| 16 | + if (calls++ < failures) throw new Error(message); | |
| 17 | + return new Uint8Array([1]); | |
| 18 | + }, | |
| 19 | + }); | |
| 20 | + let made = 0; | |
| 21 | + const env = { | |
| 22 | + REPO: { idFromName: (name: string) => name, get: () => stub(++made) }, | |
| 23 | + BUCKET: { delete: async (key: string) => void deleted.push(key) }, | |
| 24 | + } as unknown as Env; | |
| 25 | + return { env, first: stub(0) as never, stubs, deleted }; | |
| 26 | +} | |
| 27 | + | |
| 28 | +const caller = { role: "write" } as unknown as Caller; | |
| 29 | +const spilled = { spilled: "push/abc", size: 70 << 20 }; | |
| 30 | + | |
| 31 | +describe("a push cut off by a deploy", () => { | |
| 32 | + it("is handed to the new code once more, through a new stub", async () => { | |
| 33 | + const f = fakes(1); | |
| 34 | + expect(await receiveWithRetry(f.env, "r1", f.first, spilled, caller, false)).toEqual(new Uint8Array([1])); | |
| 35 | + expect(f.stubs).toEqual([0, 1]); | |
| 36 | + expect(f.deleted).toEqual([]); | |
| 37 | + }); | |
| 38 | + | |
| 39 | + it("fails after a second reset, and the staged body is deleted", async () => { | |
| 40 | + const f = fakes(2); | |
| 41 | + await expect(receiveWithRetry(f.env, "r1", f.first, spilled, caller, false)).rejects.toThrow(RESET); | |
| 42 | + expect(f.stubs).toEqual([0, 1]); | |
| 43 | + expect(f.deleted).toEqual(["push/abc"]); | |
| 44 | + }); | |
| 45 | + | |
| 46 | + it("is not retried for any other error", async () => { | |
| 47 | + const f = fakes(1, "Durable Object's isolate exceeded its memory limit and was reset."); | |
| 48 | + await expect(receiveWithRetry(f.env, "r1", f.first, new Uint8Array(4), caller, false)).rejects.toThrow(/memory limit/); | |
| 49 | + expect(f.stubs).toEqual([0]); | |
| 50 | + expect(f.deleted).toEqual([]); | |
| 51 | + expect(resetByDeploy(new Error(RESET))).toBe(true); | |
| 52 | + expect(resetByDeploy("Durable Object reset because its code was updated.")).toBe(false); | |
| 53 | + }); | |
| 54 | +}); |