fix(modules): re-clone a work tree git cannot use, instead of wedging on it #40
thisilike
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/issue-28-wedged-clone"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #28.
The wedge
syncRepopicked clone-vs-fetch from the presence of<srcDir>/.git, never from its validity:A SIGKILL mid-clone — an OOM kill,
docker stoppast the grace period, a host reset — leaves a partial clone behind. Every boot after that fails the same non-transient way,isRetryableFailureis false, the retry loop never takes the repo, and the module stays disabled on that boot and every boot after it. Manualrm -rf /data/modules/src/<slug>was the only recovery, and nothing named it.The fix: a fast-path probe, and bounded recovery on failure
Seven reviews of this PR proved the same lesson seven times: a probe is a guess about what git will accept, and only the operation knows — and the corollary, that an operation refusing is not the same as the tree being unusable. The first predicate (
.gitexists) kept fetch-phase residue; the second (rev-parse --verify HEAD) caught that and still missed the checkout-phase kill and its staleindex.lock. The third review then proved the inverse overcorrection — recovery-on-failure with no bound condemns the tree for faults that are not the tree's, and a typo'd pin paid a full clone on every boot — the fourth proved that the bound's identity was wrong in both of its halves, the fifth that the mechanism which fixed the identity had quietly taken the module's name with it, the sixth that the recovery was still willing to destroy a tree git had only declined to check out, and the seventh that three of the mechanisms above were claimed rather than pinned. So:isUsableWorkTree—rev-parse --show-toplevelreal-path-compared againstsrcDir(a repo, and ours), plusrev-parse --verify HEAD(a finished clone) — only decides whether the cheap fetch is worth attempting. It is documented as, and is, the fast path.--force.checkout --detachrefuses to run when a locally-modified tracked file would be overwritten. That refusal is git answering non-transiently, so the tree became the suspect, was removed and re-cloned — and the recovery then succeeded, which is why the bound could not see the class at all:clearFault()ran, no signature was ever written, and the next boot did it again. Four boots with something writing a tracked file intosrcDirbetween them bought four full clones from thirty-three lines beforeDeno.serve, and destroyed every untracked file undersrcDireach time. Git said no about the checkout, not about the repository.--forceis the checkout this file already argues for — the leftover branch states the invariant, that everything undersrcDircame from the clone, so there are no local changes worth keeping. What it leaves alone is an untracked file at a path the target commit does not track. One at a path the commit does track is overwritten — but plaincheckoutrefuses outright there too ("The following untracked working tree files would be overwritten"), so that is one more refusal class--forcecloses rather than a file it costs: the alternative was the re-clone, which removes the whole tree. Onmainthis shape merely disabled the module; the recovery made it destructive, so it was a regression this PR introduced.GitExitError, non-transient), the tree is re-cloned — once per fault, not once per boot. When the recovery clone fails the same way as the fault that triggered it, that is the proof the tree is innocent: the signature is persisted besidesrcDirand the same fault skips the recovery on every later boot. Any successful sync clears it; a fault with a different message is new evidence and is recovered again. Only git answering may persist a signature — a recovery that died on a network blip or the 120 s self-kill proves nothing about futility, and persisting it would wedge a genuinely broken tree behind a recovery that could still succeed. A write that fails — the full disk that produced the fault is exactly where it fails — is now logged rather than swallowed, since the visible symptom of losing it is a clone per boot with nothing saying why.<srcDir>.recovery-fault-<fnv1a(ref)>.…#mainand…#devslugify to one work tree (external-retry.tssays so and tests it) and do not share a fault: keyed on the directory alone, the entry that synced fine deleted the failing entry's evidence before it was ever read, and two failing entries were each other's "new evidence" — either way a full clone per boot, from thirty-three lines beforeDeno.serve, which is the entire reason the bound exists. The digest rather than the ref itself, because a branch name is a path (release/1.2) and this is a filename.ModuleFailurenow carriesconfiguredRef, and the chain looks the config entry up by (url, ref) rather than by url alone. Pre-existing, but the per-ref bound is what made it matter: a#devfailure retried with the#mainentry checks out the wrong ref and writes or clears the wrong ref's signature. One chain per URL is unchanged and deliberate — two chains in one work tree would fight overindex.lock— and the url-only lookup stays as the fallback for a failure raised outsideprepareExternalModule, where there is no ref.git cloneperforms a checkout of its own, so a checkout fault reproduced by the recovery came back wordedgit clone failed: …with clone's advice appended (Clone succeeded, but checkout failed) and never matched thegit checkout failed: …that condemned the tree — leaving every checkout fault except the pin unbounded, each recovery a completed transfer. Both clones now pass--no-checkoutand check out through the samecheckoutConfiguredRefthe fetch path uses, so the wording matches by construction (and the doubled checkout goes away: clone's was of the default branch, which a pinned ref replaced on the next line). On top of that,faultSignature()strips the leadinggit <cmd> failed:before comparing and persisting, which covers the fetch-vs-clone case — a disk that fills mid-transfer saysNo space leftunder both names — while the two texts that genuinely differ (does not appear to be a git repositoryagainstrepository … does not exist) stay different.--no-checkouthas a cost, and it is paid. Clone's checkout of the default branch was also what left a manifest on disk for a repo whose configured checkout then failed — a pin no ref satisfies, a tree git refuses to write — andreadManifestNamerecovered the module's name from it in the failure record. Without it that work tree is empty, on that boot and every boot after, and the record fell back to the slug: the bell readModule "<slug>" is not runningand/systemfiled the repo underunidentified. Same on a tree that ran for months, was wrecked, and was re-cloned under a bad pin. The blob is in the object store regardless — HEAD is the default branch clone pointed it at — so the fallback inprepareExternalModule's catch now readsgit -C <srcDir> show HEAD:opsdeck.module.json(manifestNameOf, same schema, same "readable, not acceptable" bar) when the file is not in the work tree. What leaves the slug standing is nothing on disk at all, or a HEAD that never resolved.LOCAL_GIT_TIMEOUT_MS) rather thanGIT_TIMEOUT_MS: the name lookup above,repoOrigin'sconfig --get, andisUsableWorkTree's tworev-parses. None of them touches a network and all of them run beforeprepareExternalModulereturns, which is the pre-Deno.servepath — asyncRepothat just spent two minutes on a wedged/datamust not be able to spend two more, per probe, for a nicety. What each gives up by stopping early is small and named: the manifest name costs the slug in one failure record, and a probe that cannot answer rethrows as an ordinary clone-stage failure rather than condemning anything. (Only the name lookup had this in the previous round; review pointed out that the other two make the same argument and did not get it.)checkoutConfiguredRefreports the answer that matters. It tries the ref as given, thenorigin/<ref>— and the bare catch reported the second attempt, which for a tag that exists but whose tree git refuses to write is a ref-lookup failure (--detach does not take a path argument 'origin/v2.0.0') standing in for the realerror: invalid path '.git'. That message is the persisted fault identity, the bell body and the/systemtext. The second attempt is now made only whenrev-parse --verify --quiet origin/<ref>^{commit}says there is such a branch; otherwise the first attempt's error stands. Visible consequence: a pin no ref satisfies is now worded by the first attempt too —… 'v9.9.9'rather than… 'origin/v9.9.9'— which is the name the operator typed.sweepFaultSignatures()runs once at the top ofprepareExternalModules, before anything writes a new one, and removes every.recovery-fault-<hex>file no configured ref could still read.isFilefirst:.survivesslugify, so a repo whose URL ends in.recovery-faultowns a work tree of that exact shape, and the sweep must never be what removes a checkout. Scoped honestly: this is the smallest of the leftovers a dropped repo leaves and the only one swept — itssrcDirstays, as it always did onmain, and so do the<srcDir>.recovering/<srcDir>.replaceda kill inside a recovery leaves, which only the next sync of that same repo removes. Sweeping a few hundred bytes is areadDirand a filename pattern; sweeping checkouts is a decision about which directories under/dataare ours, and that is more machinery than the disk it saves.<fault> (re-clone also failed: <e2>), or<fault> (a fresh clone failed the same way)when identical. An earlier revision threw the recovery's error alone, so the bell described a network blip about a tree whose origin was gone — and retryability was decided by the wrong error. It is now decided over both messages: a tree fault whose re-clone died on a transient network error is retryable, a reproduced fault is not.<srcDir>.recovering) and is swapped in only after its checkout finishes — at the cost of briefly doubling the repo's footprint under/dataduring a recovery. A recovery that fails must not consume the old checkout, whose manifest is what names the module's failure (readManifestNamein theprepareExternalModulecatch). The swap itself is two renames rather than remove-then-rename:Deno.remove(dir)followed by a rename that threw — an ordinary EACCES or EBUSY, no kill required — left neither copy standing and the catch removed the fresh one too, so the record fell back to the slug on a boot that had already lost the module. The suspect is renamed aside (<srcDir>.replaced), the fresh clone takes its name, and the suspect is dropped only after that; a rename that fails puts the old tree back. A kill landing inside the window still costs that checkout — the window is two filesystem ops, not the clone — and what it leaves behind is swept by the next sync, as.recoveringalready was. "Briefly" is now a word a test holds: both removals — the suspect after a successful swap, the half-built clone on the failure path — could be deleted with the suite green, and an unnamed second copy of a repo under/datais the smell this PR exists to end./data, a transient network error — rethrows and surfaces as an ordinary clone-stage failure, on the typedGitExitError+gitAnswered, not message sniffing.lstat, so a dangling symlink is seen) and cloned over.remoteDefaultBranch()asks the remote (git remote set-head origin --auto) and re-reads, instead of the old literalorigin/HEADfallback that composedorigin/origin/HEAD.slugifyappends an 8-hex FNV-1a digest of the credential-free URL, so two configured URLs sharing a directory takes a 32-bit collision on a prefix-colliding pair (~1 in 4·10⁹) rather than an everyday fold (the readable part is lossy:/and:fold to the same-, 71 chars truncate, the all-dots rule is many-to-one). Digest over the credential-free form, so rotating a token does not orphan the clone.ref.urlis redacted (redactSecrets) in every log line that carries it, in the error text git echoes it back into, and in the persisted fault signature (compared redacted on both sides — redaction is deterministic, so equality survives it).git()error messages name the subcommand, notargs[0].index.lock, a zero-byte file git itself says to delete, costs a full re-transfer. Consistent with "don't guess"; at this repo size the wasted clone is cheaper than a third predicate.Slug migration, paid for
The digest suffix moves every existing deployment's srcDir, and both costs of that rotation are handled:
/data/modules/src/<legacy>is migrated, not stranded. When it is this url's own clone and the new directory does not exist yet, it is renamed into place — no re-clone, and a first post-upgrade failure can still recover its manifest name from it. The adoption gate (repoOrigin) compares credential-free forms (shared withslugifyviacredentialFreeUrl): the digest is credential-free precisely so rotating a token does not orphan the clone, and a verbatim comparison would have had this migration remove the very checkout it exists to keep from a deployment that rotated its token between the two versions. It answers three ways rather than two, for the rule the rest of the file follows — only git answering may condemn a directory: a leftover whose origin is some other URL is removed, and one git could not answer for at all (no git on PATH, a wedged/data) is left where it is until a boot that can ask. That question is now asked whether or notsrcDiris occupied. Adoption is only possible while it is free, but removal is the samerm -rfeither way, and skipping the question oncesrcDirexists made the steady-state path the one path that removed on no evidence at all. The legacy formula is kept verbatim (legacyModuleSlug), with guards for its old floor bugs (""/all-dots would name srcRoot or its parent).externalFailureKeyis rekeyed by a boot sweep (migrateExternalAlertKeys, called once inmain.tsafterDeno.serve, before anything writes under the new keys) rather than inside the alert calls. The deployment likeliest to hold a legacy row is the one whose module upgrades and then simply works — that boot raises neither a failure nor a recovery, so a migration living in those calls never reached it, and the old row stood in the bell forever. The sweep also takes the keyedUPDATEoff the alert hot path: it is now oneUPDATEper configured repo per boot. Retiring it outright would need a persisted marker — more machinery than theUPDATEit saves, so it deliberately stays.The bell
briefly()cuts out of the middle, and a chained record half by half. The chained clone record runs past 300 characters with a real URL in both halves, so trimming the tail dropped the parenthesised half whole and left the notification inside a clause it never closed; one middle cut over the whole string kept both reasons and ate the(re-clone also failed:between them, so the two errors read as one run-on sentence closing a parenthesis it never opened. The record's delimiter is one exported constant (RECOVERY_CHAIN, written bysyncRepoand read by the bell), and the budget is split on it: each half gets its own middle cut, the delimiter and the closing paren are kept, and a short half hands what it does not need to the other. A git message carries its label at the front and its reason at the end, which is what a middle cut keeps. The trade, named: for afrontend-buildbody — the builder's whole stdout+stderr — the last 150 characters are whatever the bundler printed last rather than more of the first error./systemcarries the full text either way.That mechanism was entirely unexecuted by the suite until the last round: the bell test's chained record is 268 characters after redaction, under the budget, so all three of its chained assertions passed trivially and reverting
brieflytomain's tail cut passed the whole file. It now has a direct unit test — and this round the handoff got the same treatment, for the same reason: the lopsided fixture was 203 characters, sobrieflyreturned at the length guard and the split never ran, leaving both handoff terms deletable with 176/0 green. Two fixtures now exceed the budget with one half short, asserted on length, because the handoff is precisely what keeps the total at the full 300: droppingMath.min(fault.length, …)comes out at 184 and droppingMath.max(…, budget − recovery.length)at 185.middleCutis clamped and indexed absolutely while it is in hand. Formax <= 2the tail arithmetic reaches zero, wheretext.slice(-0)istext.slice(0)— the whole string plus an ellipsis, longer than its own budget. Unreachable frombrieflytoday (when a cut is needed neither share drops below 137), but threeMathcalls were what stood between them.No longer in this PR
The
rateLimit()sweep fix rode along on a stale premise — a Deno 2.5.6 pin that no longer exists. Main's request-path sweep supersedes it and this branch carries main's version untouched.CLAUDE.md's claim that 2.5.6 is the Dockerfile/CI pin is corrected instead: the single pin isdocker/Dockerfile'sFROM denoland/deno:…line (2.9.5 today), and.forgejo/deno.shderives CI's image from that same line.Tests
Twenty-two around this in
packages/server/tests/external_modules_test.ts(12 → 34):git clone --no-localinside its checkout, the appearance of.git/index.lockproves the kill lands in that phase, then SIGKILL. The stale lock is asserted where the kill was final (Deno.build.os !== "windows"), so where the kill misses, the test can no longer silently degrade into a copy of the healthy-tree test — the Windows-wrapper degradation is documented, and CI (linux, the container's git) runs the lock path for real. It also now asserts that the successful swap left nothing besidesrcDir; droppingDeno.remove(aside, …)fails it.--forcefails it.gitAnsweredis the single predicate deciding whether a work tree getsrm -rf'd, and replacing its body withreturn truepassed all thirty of the tests before it. This one points the tree's origin atgit://127.0.0.1:1/nope.gitwhile the config still names the reachable path, so a re-clone would succeed and take the tree with it: the marker survives, the record is the fault alone and transient, and there is no signature and no.recovering.return truefails it.brieflyfor the reason above. An uncredentialed fault + recovery past the budget: the delimiter and the closing paren survive, each half is cut out of its middle (label at the front, reason at the end), and an unchained body is one middle cut.main's tail cut fails it. Two lopsided fixtures over 300 now pin the budget handoff and assertbriefly(record).length === 300; each of the two handoff terms fails one of them (184 and 185).#v9.9.9. Boot 1 recovers and the record says the fresh clone failed the same way; boot 2 skips the recovery and the record is the fault alone; the old checkout's marker file survives both (the tree is never consumed) and keeps naming the module. The pin then becoming satisfiable (the tag is created) fetches into the same tree and retires the signature — the evidence is about a fault, not about a directory. It also asserts that the failed recovery consumed its own half-built clone (droppingDeno.remove(fresh, …)fails it) and that the failure record carriesconfiguredRef(dropping it fails it, which is what silently reverts the (url, ref) fix).#mainfirst, and only#devfailing. The retry chain's work tree must end up ondev's commit; reverting the lookup to url-only fails it.a fresh clone failed the same wayfor each, boots 2–3 record the fault alone however the good entry is interleaved, and the shared work tree is never consumed under it..git, which every git refuses to write (error: invalid path '.git') in a clone as much as in a work tree. It asserts the fault is worded by our checkout, that boot 2's recovery reproduces it and persists, that boot 3 buys no clone, and that what is on disk is the message with its subcommand stripped. No filter binary and no process-wide environment, so it is safe under the parallel runner.#v9.9.9names the module from the manifest with noopsdeck.module.jsonin the work tree (asserted absent, so the name provably came from the object store), holds that name through the recovery boot and the bounded boot after it, and holds it again for a tree that ran once, was wrecked (.gitremoved), and was re-cloned under the bad pin.v2.0.0; the record carriesinvalid path '.git'and notdoes not take a path argument. Then a ref that is a remote branch (#release, the seed commit, no local branch of that name) still gets its second attempt and syncs.fresh clonenorre-clonein the record (assertNoRecovery) — the old!includes("fresh clone")also passed for a recovery that ran and failed differently.#v9.9.9, then a boot configured#v1.0.0, andprepareExternalModulesleaves nothing behind.faultSignature:git fetch failed: …andgit clone failed: …of one text match, the two texts that genuinely differ do not, and credentials are gone from what lands on disk.Gates on this box:
deno task check,deno lint packages/server/,deno fmt --check, and the fullpackages/server/tests/suite (177 passed, 0 failed) are green at the head commit, which is merged onto currentmain. The same suite through./.forgejo/deno.sh— the derived image built from the Dockerfile'sFROM(denoland/deno:2.9.5+ debian git) — is 177 passed, 0 failed as well. Every mutation named above was run, not reasoned about.Not covered
isUsableWorkTreeto the literal #28 predicate (Deno.stat(dir + "/.git")) still passes the file, because recovery-on-failure catches the killed clone anyway. That is the architecture — the probe only decides whether the cheap fetch is worth attempting — but it means a clone the kernel killed no longer proves the probe, and it belongs here rather than in the claims above.remoteDefaultBranchlikewise: reverting it to the old literal still passes a work tree that lost origin/HEAD re-asks the remote, because the recovery clone rescues it./data) andrepoOrigin'sunknownbranch need the same unavailable git — simulating either means mutatingPATHfor the whole test process, which the parallel runner makes unsafe; both aregitAnswered()plus the typedGitExitErrorby construction. The network half ofgitAnswered— the half that decides whether an innocent tree is deleted — is covered by a healthy tree whose origin went unreachable; theGitExitErrorhalf alone (!isTransientGitErrorremoved) still passes.Deno.renamethat throws there — forcing that means mockingDeno.rename. The.recovering/.replacedsweep and the restore-on-failure path are the containment, and the swap comment claims exactly what it guarantees. What is pinned now is only that a recovery which completes — either way — leaves neither directory behind.moduleSlugcredential-free) plus the sharedcredentialFreeUrlbeing the same function on both sides. Thegit show HEAD:…fallback's own failure modes (HEAD unborn, no git) are the slug, which is what they were before.LOCAL_GIT_TIMEOUT_MSis not pinned by anything: it is a budget, and driving it means a git that hangs for ten seconds in three places.U#mainandU#devshare one notification row. Pre-existing; this PR is what made the pair first-class everywhere else, so it is named here rather than fixed.Re-review at
77c963d, checked against a running git rather than against the description.Verified first
deno task check,deno lint packages/server/,deno fmt --checkandpackages/server/tests/(113 passed) are green at the head commit. Merged onto currentmain(cc65b8c, which has since rewrittenseedRepoin this very file) it is clean and 123 passed.external.tsreverted to its parent and the tests kept, a clone the kernel killed fails (git -C failed: fatal: not a git repository) and a slug is a directory name fails (https://.. -> ..), while a healthy work tree is still fetched passes either way. The proof is real.--show-toplevelover--git-diris the right call for the reason stated, and therealPathcomparison on both sides is needed, not defensive.Blocking: #28 is not closed
The wedge is real, but the shape this fixes is not the shape a killed clone leaves.
git clonewrites.gitin stages:initfirst, then[remote "origin"] url, then the fetch — and only after the fetch the fetch refspec,refs/remotes/origin/HEADand the checkout. A SIGKILL during the fetch, which is the long phase and therefore the OOM window, leaves a.gitthat git accepts as a repository:So
syncRepotakes the fetch branch, and from there:End to end against this branch, letting git write the wreckage instead of a test seeding it (
git clonespawned,SIGKILLafter 1.5 s, thenprepareExternalModuletwice):Non-transient,
stage: "clone", soisRetryableFailureis false, the retry loop never takes it, and the module stays disabled on that boot and every boot after — the issue verbatim, withrm -rf /data/modules/src/<slug>still the only recovery and still nothing naming it. Reproduced on git 2.55 and on git 2.39.5, which is whatdebian:bookworm-slim— the runtime layer ofdocker/Dockerfile— ships.The probe asks "does git call this a repository". The question that decides whether a fetch can recover the directory is "is this a finished clone".
git -C <dir> rev-parse --verify HEADseparates them: a completed clone has a commit at HEAD, the killed one has an unbornrefs/heads/master(fatal: Needed a single revision). It also stays right for a kill during the checkout phase — HEAD resolves there, and thecheckout --detachfurther downsyncRepofinishes the work tree, so that case must not be re-cloned. A marker file written after a successfulsyncRepo, with its absence meaning "unusable", would cover every partial shape and would not depend on reading git's mind. Either way theelsebranch you added then does the recovery it was written for.Related, and the reason this is terminal rather than merely noisy: when
symbolic-ref refs/remotes/origin/HEADfails,targetdefaults to the literalorigin/HEAD, so thecatchfallback composesorigin/origin/HEAD— the default-branch path has no working second chance by construction. Worth fixing in the same pass.Blocking: the tests seed wreckage git never writes
Test 1's two shapes —
.gitas an empty directory,.gitas an empty regular file — are hand-built. A killed clone produces neither (see thels .gitabove), and nothing in git writes an empty.gitfile at all; the gitfile form only ever comes fromgit worktree add/submodules, where it is valid. So the test proves the probe rejects garbage, which is worth having, but not the claim in its own name. The repro above is three lines and seeds it for real.Non-blocking
Deno.statforleftoverfollows symlinks, so a dangling symlink atsrcDirreads as absent, nothing is removed, andgit clonethen dies withfatal: could not create work tree dir '…': File exists— non-transient, wedged again.Deno.lstatsees it andDeno.removehandles it; one word.msg.startsWith("git ")pins the guard togit()'s message format, and nothing tests the coupling. If that format ever changes, the fallthrough flips from "rethrow" to "return false", andfalsenow means delete the work tree. A typed error thrown bygit()carries the same information without the string. Related: every call passes-Cfirst, soargs[0]is-Cand operators readgit -C failed: …in/systemand in the bell for clone, fetch and checkout alike — pre-existing, but this PR adds a fourth-Ccaller and quotes those strings in its own body.slugify's fallback is the constant"repo", sohttps://.andhttps://..land on the samesrcDir; the second repo then finds a valid work tree, fetches the first one'sorigin, and reports the first one's manifest name. Exotic, but a short digest suffix retires the class rather than one instance of it.log.info("cloning", { url: ref.url })— a line this diff moves — writes a credentialed URL into the log unredacted, whileredactSecretsexists and every other egress path (/system, the alerts) uses it. Same for theexternal module skippederror log below it.The direction is right and the
slugifyhardening is a good catch on the way past. It is the recovery predicate that needs to change, plus a test that lets git produce the input.@ -195,1 +199,4 @@log.warn("unusable work tree, re-cloning", { dir });await Deno.remove(dir, { recursive: true });}log.info("cloning", { url: ref.url });Pre-existing, but this diff moves the line:
ref.urlmay embed credentials (the interface above says so, andslugifystrips them for exactly this reason), and this writes it to the log unredacted whileredactSecretsexists. Same for theexternal module skippedlog in the caller.@ -192,3 +191,1 @@false);if (!exists) {if (await isOwnWorkTree(dir)) {This asks "does git call
dira repository", but the branch below needs "is this a finished clone". A SIGKILL during the clone's fetch leaves.gitwith HEAD, config (url only, no fetch refspec), objects and refs — git accepts it,--show-toplevelprintsdir, so this returns true andsyncRepofetches into a repo that can never resolve a ref. Verified end to end on this branch:stage=clone git -C failed: fatal: git checkout: --detach does not take a path argument 'origin/origin/HEAD', on every boot.git -C dir rev-parse --verify HEADseparates the two (unborn HEAD in the killed one), and stays correct for a kill during the checkout phase, which must NOT be re-cloned.@ -195,0 +194,4 @@// whatever is at `dir` is not a repo we can fetch into. Removing it is the// only path back to a running module and it costs nothing: everything// under srcDir came from the clone, so the remote still has all of it.const leftover = await Deno.stat(dir).then(() => true).catch(() => false);Deno.statfollows symlinks, so a dangling symlink atdirthrows NotFound here, nothing is removed, and the clone below dies withfatal: could not create work tree dir '…': File exists— non-transient, so wedged again.Deno.lstatsees the link andDeno.removealready handles it.@ -229,0 +256,4 @@// PATH, or the 120 s self-kill on a wedged /data, would otherwise read as// "not a repo" and remove a work tree that was fine. Let those out — the// caller reports them as a clone-stage failure, which is what they are.if (!msg.startsWith("git ") || isTransientGitError(msg)) throw e;The guard's correctness rests on
git()formatting messages asgit <arg0> …, and nothing tests that. If the format changes, this stops rethrowing and starts answering "not a repo" — which now means the caller deletes the work tree. A typed error fromgit()(or a field on it) carries the same distinction without depending on a prefix.@ -304,0 +345,4 @@// still a path and not a directory: "" and "." are srcRoot itself and ".."// is its parent, and syncRepo now REMOVES what it finds at srcDir before// re-cloning. `https://..` is a configured URL away from that..replace(/^\.*$/, "repo");Fixes the traversal, but the fallback is a constant:
https://.,https://..andhttps://all becomerepoand share onesrcDir. The second one then finds a valid work tree, fetches the first one'soriginand reports the first one's manifest name. A short digest suffix would close the class instead of the instance.@ -266,0 +296,4 @@// what SIGKILL mid-clone leaves on disk: `. .. .git`, and nothing inside// .git. SIGTERM — what `git()`'s own AbortController sends — leaves// nothing, so this is the only shape that survives to the next bootawait Deno.mkdir(`${srcDir}/.git`, { recursive: true });This shape is hand-built — a real killed clone leaves
.githolding HEAD, config, objects and refs, not an empty directory, and nothing in git ever writes an empty.gitFILE (the gitfile form only comes fromgit worktree add/submodules, where it is valid). Let git write the input instead: spawngit clone --upload-pack 'sh -c "sleep 60"', SIGKILL it, then callprepareExternalModule— that residue still fails today, twice in a row.Reworked at
7ff2f98. Every item, blocking and not, in review order:The recovery predicate (blocking, comment on
isOwnWorkTree): reproduced your fetch-phase kill on git 2.55/Windows before changing anything — same residue (.gitwith HEAD, config holding the url and no refspec, objects, refs;--show-toplevelanswers yes,rev-parse --verify HEADanswersfatal: Needed a single revision). The probe is nowisUsableWorkTree: toplevel + real-path compare as before, thenrev-parse --verify HEAD. Your checkout-phase caveat is honored — HEAD resolves there, the probe keeps the tree, andsyncRepo's own checkout finishes it. Chose the git-side predicate over the marker file: it reads the same state git will act on, needs no write after every sync, and the marker would itself be one more thing a kill can half-write.origin/origin/HEAD: fixed in the same pass as asked.remoteDefaultBranch()reads the symbolic ref, and on a miss runsgit remote set-head origin --autoand reads again — a real second chance that asks the remote, with a failure after that surfacing as itself instead of as--detach does not take a path argument.Tests seed wreckage git never writes (blocking): the killed-clone test now uses your recipe —
killCloneMidFetchspawnsgit clone --no-local --upload-pack "sh -c 'sleep 60'", waits until config gains[remote "origin"](past that point the clone is inside a fetch that can never complete), SIGKILLs, and asserts recovery on that boot and the next. The hand-built shapes moved to their own honestly-named test — they still prove the probe rejects what git never wrote. One wrinkle you'll care about when re-verifying: on git ≥ 2.47 a plain fetch recreatesrefs/remotes/origin/HEAD, so the lost-origin/HEAD regression evaporates on a modern box. The test pins it withremote.origin.followRemoteHEAD never— ignored by 2.39.5, honored by 2.47+ — andset-head --autowrites the ref on either vintage. Verified all three regression tests fail with the source change stashed (on git 2.55), and the healthy-work-tree test passes either way.Deno.statmisses a dangling symlink:lstat, one word, as said — plus a test. The test self-skips on a Windows box without the symlink privilege; CI runs it for real.msg.startsWith("git "): gone.git()throws a typedGitExitErrorfor a non-zero exit; the self-kill and a spawn failure stay plain, sogitAnswered()distinguishes "git said no" from "the probe failed" without reading a prefix. The-Cgripe too: messages now name the subcommand (git fetch failed:/git checkout failed:), which also fixes what/systemand the bell show for the pre-existing callers.slugify constant fallback: digest suffix, applied always rather than only in the fallback — the readable part is lossy in three ways (
/and:both fold to-, truncation, the dots rule), and any two URLs sharing a directory makes the second repo fetch the first one's origin. Digest is FNV-1a over the credential-free URL, so rotating a token keeps the clone. Cost stated in the description: existing deployments re-clone once into the new name; dist cache unaffected.Credentialed URL in logs: both lines redacted, and the
skippedline's error string too — git echoes the URL back inside its own stderr, so redacting only the field left the same token one line lower.Also:
rateLimit()'s per-limitersetIntervalleaked from every test that builds an app and failed the suite's op sanitizer on pinned Deno 2.5.6, so the gates could not be green around it — the sweep now rides on the request path (separate commit). Only behavior change: an idle limiter's stale buckets linger until the next request instead of being swept on a clock, which bounds the same memory either way.Gates at head:
deno task check,deno lint packages/server/,deno fmt --check, fullpackages/server/tests/— 117 passed. Description updated to match all of the above.Re-review at
7ff2f98. Everything from the last round re-verified against a running git, plus a fresh hunt for what the new predicate still cannot see.Last round's items, checked
git clone --no-local --upload-pack "sh -c 'sleep 60'"SIGKILLed once its config gains[remote "origin"], thenprepareExternalModuletwice:boot 1: OK,boot 2: OK. WasFAILED stage=cloneon both before.origin/origin/HEADgone, andremoteDefaultBranchis a real second chance.lstatdone.GitExitError/gitAnswereddoes replace the prefix sniffing, and the self-kill and spawn failures stay plainError, so they rethrow — the distinction holds. Subcommand naming works for["-C", dir, …]and forclone. Redaction covers both log lines and the error text.77c963d: killed-clone, dangling-symlink, lost-origin/HEAD and two-distinct-URLs all fail, healthy-work-tree and garbage pass. Against the merge base only healthy passes. Your "garbage passes either way" is true of the previous revision, not ofmain— worth saying which, since againstmainit fails too.deno task check,deno lint packages/server/,deno fmt --check,packages/server/tests/117 passed. Ran the new tests on the toolchain CI actually uses (./.forgejo/deno.sh, sodenoland/deno:2.9.5+ debian git 2.39.5): 16 passed. Merged onto currentmainand ran the whole suite in that same image: 492 passed, 0 failed.killCloneMidFetchis stable — 5 consecutive runs of the file, no flake.Blocking: the checkout-phase kill wedges, and the description says it doesn't
It cannot finish it. Clone holds
.git/index.lockacross the whole checkout, so a kill in that phase leaves the lock behind, and every latergit checkoutin that repository refuses to run. Reproduced with a real clone (4000-file repo,--no-local), waiting forindex.lockto appear so the kill is provably inside the checkout, thenprepareExternalModulethree times:isUsableWorkTreesays yes — toplevel matches,rev-parse --verify HEADresolves, because both are reads and neither needs the index — the fetch succeeds, and the checkout fails the same way forever. Non-transient,stage: "clone", not retryable: issue #28 again, in the phase right after the one you just fixed, and with a window as wide (the checkout of a large repo is seconds to minutes of OOM exposure, and the lock is held for all of it).The pattern is worth naming, because this is the second predicate that was almost right: a probe is a guess about what git will accept, and the only thing that knows is the operation.
rev-parse --verify HEADis a better guess thanstat(.git)and it is still a guess — it misses a staleindex.lock, and it would miss a corrupt pack or a ref file truncated mid-write for the same reason. Recover on failure instead, and the guess stops carrying the weight: wrap the fetch+checkout in a try, and on a non-transientGitExitError, removedirand clone fresh once before giving up. That is fewer lines than the probe, it closes every partial shape including the ones neither of us has thought of, and it costs one wasted clone on a genuinely bad pin. Keep the probe as the fast path — it is right about the fetch-phase residue and saves the failed fetch — but it must not be the only way back.Non-blocking
/data/modules/src/<old-slug>forever, and nothing names it (the same "manualrm -rfand nothing said so" that this PR is about, at one remove). AndexternalFailureKeyismoduleSlug, whose own comment says the slug exists so the key "survives the name changing" — every stored alert key rotates, so a deployment upgrading while a module is failing keeps its old bell row standing forever: the next failure andalertModuleRecoveredboth address the new key. Both are one-time and cheap to handle; neither is in the description.killCloneMidFetchorphans a process per run. SIGKILL on the clone leaves itssh -c 'sleep 60'upload-pack behind — verified,sleep 60still running after the test exits.sleep 5hangs the transport just as well across the millisecond between the config check and the kill, without leaving a minute of CI residue.setInterval(docker run denoland/deno:2.5.6 deno teston a two-line file: FAILED) — but nothing pins 2.5.6 any more:docker/Dockerfileis on2.9.5and.forgejo/deno.shderives CI's image from that sameFROMline, so there is one pin and it is 2.9.5, where the same file passes. Withratelimit.tsreverted tomain's version I get 117/117 onpackages/server/tests/and 458 passed on the full suite, no sanitizer complaint. So the gates were never blocked on it — the change is fine on its own merits (and I'd keep it), but it is an unrelated fix riding along, and the belief comes fromCLAUDE.md, which still says 2.5.6 is what the Dockerfile and CI pin. That line is worth correcting wherever you touch it next.The core mechanism is in much better shape than last round, and the tests now use git's own wreckage as input, which is the part that matters. One more phase to cover, and I'd rather it be covered by recovering on failure than by a third predicate.
@ -195,3 +194,1 @@log.info("cloning", { url: ref.url });await git(["clone", "--quiet", ref.url, dir]);} else {if (await isUsableWorkTree(dir)) {The shape worth changing: this makes a probe the only way back, and a probe can only guess what git will accept.
rev-parse --verify HEADis a better guess thanstat(.git)and still misses a staleindex.lock, a corrupt pack, a ref truncated mid-write. Wrap the fetch+checkout below in a try and, on a non-transientGitExitError, removedirand clone fresh once before giving up — fewer lines than the probe, closes every partial shape, and costs one wasted clone on a bad pin. Keep the probe as the fast path; just don't make it load-bearing.@ -229,0 +270,4 @@* `syncRepo` then fails identically on every boot. So "is this a repository"* is the wrong question, and `rev-parse --verify HEAD` asks the right one: a* finished clone has a commit at HEAD, a killed one does not. A kill during* the checkout phase leaves HEAD resolvable, and `syncRepo`'s own checkoutThis claim does not hold, and it is the blocking finding. Clone holds
.git/index.lockacross the entire checkout, so a kill in that phase leaves the lock — andsyncRepo's own checkout then cannot finish anything:fatal: Unable to create '…/.git/index.lock': File exists. Reproduced with a real clone of a 4000-file repo, waiting forindex.lockto appear so the kill is provably inside the checkout: FAILED stage=clone on boots 1, 2 and 3. The probe passes because both of its questions are reads that never touch the index.@ -304,0 +413,4 @@// valid work tree, fetch the FIRST one's origin and report the first one's// manifest. The digest is over the credential-free URL, so rotating a token// does not orphan the clone.return `${name}-${fnv1a(bare)}`;Two costs the description doesn't carry. Nothing removes the old directory, so every external module leaves a full clone at
/data/modules/src/<old-slug>permanently, unnamed — the same shape of silent leftover this PR exists to end. AndexternalFailureKeyis built frommoduleSlug; its comment says the slug is there so the key survives a name change, so rotating it means a deployment upgrading while a module is failing keeps that bell row forever, since the next failure andalertModuleRecoveredboth address the new key.@ -22,0 +16,4 @@// rather than a setInterval: the map only grows when requests arrive, an// idle limiter has nothing to sweep, and an unstoppable timer per limiter// leaks out of every test that builds an applet lastSweepMs = Date.now();The change is good, the premise is stale. Deno 2.5.6 does fail on a leaked
setInterval, but nothing pins 2.5.6:docker/Dockerfileis on 2.9.5 and.forgejo/deno.shreads CI's image from that same FROM line. With this file reverted to main's version I get 117/117 onpackages/server/tests/and 458 passed on the full suite, no sanitizer complaint — so the gates were not blocked on it, and this is an unrelated fix riding along.CLAUDE.mdstill claims 2.5.6 is the pin, which is where the belief comes from.@ -266,0 +308,4 @@dest,],stdout: "null",stderr: "null",SIGKILL on the clone orphans its upload-pack:
sleep 60is still running after the test process exits (verified).sleep 5hangs the transport just as reliably across the millisecond between the config check and the kill, without leaving a minute of residue on every CI run.Rework for the second review is up at
30dae2b(merge of currentmainindd8f5d8, rework on top), description rewritten to match. Point by point:Blocking — the checkout-phase kill. Taken in the shape you suggested: recovery rides on the operation, the probe is only the fast path. When fetch or checkout fails inside a probe-accepted tree with a non-transient
GitExitError,syncRepore-clones once. One deviation from the literal "removedirand clone fresh": the fresh clone lands beside the tree (<srcDir>.recovering) and is swapped in only after its checkout finishes. Removing first turned out to collide withmain'sd1647be(merged here): a recovery that fails — revoked credential, network gone — would have consumed the old checkout, and with it the manifest name thatreadManifestNamerecovers in the failure record. Your repro is now a test, with git writing the residue: a required smudge filter (sh -c 'sleep 5; cat') pins a real--no-localclone inside its checkout, the appearance of.git/index.lockproves the phase, then SIGKILL. Caveat stated in the test and description: on a Windows dev boxgit.exeon PATH is a wrapper, the kill misses the real clone and the test degrades to the healthy path — the lock path runs for real on CI's git.Slug migration costs. Both paid, not documented away. The old directory is adopted (renamed) when it is this url's own clone —
remote.origin.urlmust match exactly and the new directory must not exist — so no re-clone and no stranded checkout; anything else at the legacy name is removed, never adopted (the legacy fold is many-to-one, and adopting a stranger is exactly the collision the digest retires). The stored bell row is rekeyed in place (NotificationStore.rekey, guarded against an active row already under the new key) before any failure/recovery write. Three new tests cover adopt, refuse-and-remove, and the rekeyed bell row.Orphaned upload-pack.
sleep 5now, with a comment saying why 5 is enough.Rate limit / CLAUDE.md. The ratelimit change is dropped — the branch carries
main's request-path sweep untouched (your read was right: the premise was the stale 2.5.6 pin). CLAUDE.md now states the single pin: the DockerfileFROMline (2.9.5 today), with.forgejo/deno.shderiving CI's image from it.Gates at
30dae2b:deno task check,deno lint packages/server/,deno fmt --check,packages/server/tests/— 143 passed, 0 failed. Merge conflicts against currentmain(readManifestNamerecovery, rate-limit sweep,TEST_GIT_SPAWN) are resolved in the merge commit, so the branch is mergeable again.Re-review at
30dae2b, checked against a running git rather than against the description. The branch's merge base is currentmain(9d9c48e), so nothing here is stale.Verified first
deno task check,deno lint packages/server/,deno fmt --checkgreen.packages/server/tests/through the pinned toolchain (./.forgejo/deno.sh test -A packages/server/tests/, sodenoland/deno:2.9.5+ debian git): 143 passed, 0 failed — the claim holds. On my host the whole-suite run dies with exit 139 (SIGSEGV) after the last test; that reproduces identically on the merge base, so it is my box, not this branch.git clone --no-localwith a stallingrequiredsmudge filter, wait for.git/index.lockto appear, SIGKILL, thenprepareExternalModuletwice.index.lock seen: true / survives the kill: true / probe HEAD resolves: true— so the probe rightly says yes — thensync failed in an existing work tree, re-cloningandboot 1: ok=true,boot 2: ok=true. Same on host git 2.55.0 and inside the pinned image. With onlyexternal.tsswapped back to7ff2f98, the same lab givesBOOT 1: ok=false :: git checkout failed: fatal: Unable to create '…/.git/index.lock': File existsand the same on boot 2. Recovery-on-failure was the right answer to the last round.index.lockon an otherwise healthy tree recovers. A seeded<srcDir>.recoveringis swept and the boot succeeds with nothing left behind. The legacy migration adopts this url's own clone (the file not from the remote survives, nothing lingers at the old name) and removes a leftover — including the case no test covers: a legacy directory that is a real clone of a different origin is removed, and the module clones fresh and reportsmine.GitExitError/gitAnswered,remoteDefaultBranch,lstat, subcommand naming, redaction are all as described. The rate-limit change is genuinely gone (5 files), and theCLAUDE.mdcorrection is accurate:docker/Dockerfile:14isFROM denoland/deno:2.9.5@sha256:…and.forgejo/deno.shderives CI's image from that line.Blocking: the recovery has no bound, and an ordinary bad pin pays for it on every boot
It does not.
gitAnswered(e)is true for every non-transient answer, including the ones a fresh clone reproduces exactly — so the tree gets condemned for faults that are not the tree's.Verified with
OPSDECK_EXTERNAL_MODULES=<repo>#v9.9.9against a repo that has no such ref, three consecutiveprepareExternalModulecalls:One full clone per boot, for as long as the pin is wrong — and
prepareExternalModulesismain.ts:155, thirty-three lines beforeDeno.serve. That is the rule this repo already writes down for the retry loop ("nothing is served untilDeno.serve, so a second of backoff is a second of unreachable UI"), spent on work that cannot succeed. A typo'd#tagis an ordinary operator mistake, and before this PR it failed in milliseconds.Same shape and same cost for anything else that lets a clone finish and a checkout not: a
.gitattributesnaming arequiredfilter the container has no binary for, a path the filesystem refuses, or aDeno.remove(dir)at line 328 that keeps failing. And the classification can flip: because the record carries the recovery's error (next item), a permanent tree fault whose re-clone dies on a network blip becomesretryable, and the retry loop then buys another full clone per attempt.This does not need a third predicate — it needs the recovery bounded rather than unconditional. Either:
rev-parse --verify --quiet <ref>^{commit}/origin/<ref>^{commit}. If the configured ref does not resolve, the checkout failure is the pin's and the tree is innocent. Not a guess about what git will accept — the same question the checkout asks.srcDirwhen a recovery clone fails the same way, and skip the recovery while it is unchanged. That also covers the filter / unwritable-path /remove-fails cases a ref check does not.Keep the probe, keep recovery-on-failure. Just stop paying for it again every boot.
Blocking: the failure record names the second attempt, not the fault
throw e2(line 333) discards the error that triggered the recovery, so/systemand the bell describe the re-clone and the thing that actually happened to the deployment survives only as awarnline in container stdout.Your own retargeted test shows it: the old checkout's fetch fails with
'origin' does not appear to be a git repository, and the record carriesgit clone failed: … Could not resolve host: git.example.com. The operator is told the network is down about a work tree that is wedged. It also decides retryability — the original is non-transient, the recorded one is transient — so that failure now schedules a retry chain it previously would not have.Chain them (
throw new Error(\${first} (re-clone also failed: ${e2})`)` or equivalent). The recovery's error is usually the actionable half; it must not be the only half.Non-blocking
originUrlIscomparesremote.origin.urltoref.urlverbatim (line 273), while the digest is taken over the credential-free form precisely "so rotating a token does not orphan the clone". A deployment that rotated its token between the two versions therefore has this migration remove the very clone it exists to keep. Normalize both sides the wayslugifyalready does and the gate matches the identity the slug uses.srcDiris then absent and the next failure is named by the slug, which is exactly what swap-only-on-success was written to prevent. Small window and the cost is one clone, but the comment claims more than the code guarantees.rekeymigration misses the case it is likeliest to meet, and never retires. It runs insidealertModuleFailure/alertModuleRecovered, so a deployment that upgraded while a module was failing and whose module then simply works on the next boot gets neither call — nothing on the startup path raises a recovery — and the legacy row stands forever anyway. (That gap ismain's, not yours; it just means the cost is not fully paid.) Meanwhile theUPDATEruns on every external failure alert forever, long after any deployment could still hold a pre-digest key.external_modules_test.ts:753seeds a plain directory with one file, sooriginUrlIsrefuses it in itscatch(not a repo at all), never by comparing URLs. Clone a second repo there instead — I did, and it is removed correctly, so this is a one-line change to a test that currently proves the weaker thing.index.lock; assert it whereDeno.build.os !== "windows"and the honesty note stops being load-bearing./datafor the module's repo during a recovery. Deliberate and right, but it is the other cost of swap-on-success and the description does not name it.index.lock— a zero-byte file git itself tells you to delete — costs a full re-transfer of the repo. Consistent with "don't guess" and probably the right trade at this size; worth knowing it is a trade.The mechanism is right, and the third round found the right answer where two predicates failed. What is left is bounding it, and not losing the original error on the way out.
@ -206,0 +250,4 @@if (dir === srcDir) return;if (!await Deno.lstat(dir).then(() => true).catch(() => false)) return;const occupied = await Deno.lstat(srcDir).then(() => true).catch(() => false);if (!occupied && await originUrlIs(dir, url)) {This gate is credential-sensitive and the slug deliberately is not:
originUrlIscomparesremote.origin.urltoref.urlverbatim, while the digest is taken over the credential-free URL precisely "so rotating a token does not orphan the clone". A deployment that rotated its token between the two versions therefore falls through to the removal branch and loses the clone this migration exists to keep. Normalize both sides the wayslugifyalready does.@ -219,0 +311,4 @@// else rethrows — the self-kill, a network fault or a missing git// binary says nothing about the tree, and re-cloning into the same wall// would only hide the real failure.if (!gitAnswered(e)) throw e;The bound is missing. Every non-transient
GitExitErrorcondemns the tree, including the ones a fresh clone reproduces exactly — so a pin that does not exist re-clones the whole repo on every boot, forever, thirty-three lines beforeDeno.serve. Verified:<repo>#v9.9.9, three consecutiveprepareExternalModulecalls, three full clones and three identical failures.Two ways to bound it without a third predicate:
rev-parse --verify --quiet <ref>^{commit}/origin/<ref>^{commit}. If the configured ref does not resolve, the checkout failure is the pin's and the tree is innocent. That is the same question the checkout asks, not a guess about what git will accept.srcDirwhen a recovery clone fails the same way, and skip the recovery while it is unchanged. That also covers the cases the ref check misses — arequiredfilter with no binary, a path the filesystem refuses, aremovethat keeps failing.@ -219,0 +325,4 @@try {await git(["clone", "--quiet", ref.url, fresh]);const commit = await checkoutConfiguredRef(ref, fresh);await Deno.remove(dir, { recursive: true });Two operations, so the rule the comment above states holds against a failed clone but not against a kill in this window:
srcDiris then absent, and the next failure is named by the slug — exactly the outcome swap-only-on-success was written to prevent. Small window and the cost is one clone, but the comment currently claims more than the code guarantees.@ -219,0 +330,4 @@return commit;} catch (e2) {await Deno.remove(fresh, { recursive: true }).catch(() => {});throw e2;throw e2drops the fault that triggered the recovery, so/systemand the bell describe the re-clone and the real event survives only as awarnin container stdout. Your own retargeted test demonstrates it: fetch fails'origin' does not appear to be a git repository, the record saysCould not resolve host: git.example.com. It also decides retryability — non-transient original, transient record — so the failure now schedules a retry chain it would not have before. Chain both errors into the thrown message.@ -347,0 +750,4 @@try {const legacyDir = `${dataDir}/modules/src/${legacyModuleSlug(repo)}`;await Deno.mkdir(legacyDir, { recursive: true });await Deno.writeTextFile(`${legacyDir}/stale`, "");This makes the test prove the weaker thing. A plain directory with one file is refused by
originUrlIs'scatch(not a repo at all), so the URL comparison — the gate the test is named after — never runs. Clone a second seeded repo here instead; I checked, and the foreign-origin clone is removed correctly, so it is a one-line change that turns the assertion into the one you meant.The recovery-on-failure the last round added condemned the work tree for every non-transient git answer, including the ones a fresh clone reproduces exactly — so an ordinary typo'd pin paid a full clone on every boot, on the startup path where every second is unreachable UI. The recovery is now once-per-fault: when the recovery clone fails with the same (redacted) message as the fault that triggered it, that signature is persisted beside srcDir and the same fault skips the clone on every later boot. Any successful sync clears it; a fault with a different message is new evidence and is recovered again. Only git answering may persist — a recovery that died on a network blip or the self-kill proves nothing about futility, and persisting it would wedge a genuinely broken tree behind a recovery that could still succeed. The failure record also stops discarding the fault that triggered the recovery: it now carries both halves ("<fault> (re-clone also failed: <e2>)"), so the bell no longer reports a network blip about a wedged tree, and retryability is decided over both messages instead of the second one alone. Also from the review: - originUrlIs compares credential-free forms (shared with slugify via credentialFreeUrl): a deployment that rotated its token between versions keeps its clone through the legacy migration instead of losing it to a verbatim comparison. - The legacy alert-key migration moved out of the alert calls into a boot sweep (migrateExternalAlertKeys, called once in main.ts after serve): the deployment likeliest to hold a legacy row is the one whose module upgrades and then simply works, which raises no alert at all — and the keyed UPDATE stops running on every failure alert forever. - The swap comment states what the code guarantees (remove+rename window) and the clone-beside comment names the doubled peak footprint. - Tests: the unsatisfiable-pin test proves one recovery clone, not one per boot, and that the old checkout and its manifest name survive throughout; the checkout-kill test asserts the stale index.lock where the kill was final; the foreign-origin migration test seeds a real clone of another repo so the URL gate actually runs; the manifest-name test asserts both halves of the chained record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Rework for the third review is up at
140246d; the description is updated to match. All nine items addressed:Blocking — the recovery has no bound. Took the once-per-fault option, since it also covers what a ref check cannot (a
requiredfilter with no binary, and it degrades gracefully around a failingremove). When the recovery clone fails with the same message as the fault that triggered it — compared and persisted redacted, at<srcDir>.recovery-fault— every later boot skips the re-clone for that fault and rethrows. Any successful sync clears the file; a different message is new evidence and recovers again. OnlygitAnswered(e2)with a matching message may persist: a recovery that died on a network blip or the self-kill proves nothing about futility, and persisting it would wedge a genuinely broken tree behind a recovery that could still succeed. Your exact repro is now a test (an unsatisfiable pin costs one recovery clone, not one per boot): boot 1 recovers and says so, boot 2 skips, the old checkout's marker file and manifest name survive both, the corrected pin retires the signature. Honest residue, in the description: a fault that reproduces under a different subcommand name (git fetch failed:vsgit clone failed:— deleted remote, revoked credential) never matches its own signature and re-runs its recovery per boot; those clones die at the remote's answer before any transfer.Blocking — the record names the second attempt. Chained:
<fault> (re-clone also failed: <e2>), or<fault> (a fresh clone failed the same way)when the recovery reproduced it. Retryability is now decided over both halves — your retargeted test asserts the chained record.Non-blocking, all taken:
originUrlIscompares credential-free forms viacredentialFreeUrl, the same normalizationslugifydigests — a rotated token no longer loses the clone to the migration. No end-to-end test (local test repos are paths, they cannot carry credentials); the gate and the slug now share the one function, which is the actual guarantee.migrateExternalAlertKeys,main.ts, once per boot after serve, before any write under the new keys) — the upgrade-then-simply-works deployment is now covered, and theUPDATEis off the alert hot path. Not retired outright: that needs a persisted marker, which is more machinery than one keyedUPDATEper configured repo per boot. Said so in the code.index.lockwhere the kill was final (os !== "windows"); the honesty note stopped being load-bearing.Gates on this box:
deno task check,deno lint packages/server/,deno fmt --check, fullpackages/server/tests/144 passed at140246d.Re-review at
140246d, checked against a running git rather than against the description. The branch's merge base is currentmain(9d9c48e), so nothing here is stale. Clone counts below are counted, not inferred — agitshim onPATHlogs every invocation.Verified first
deno task check,deno lint packages/server/,deno fmt --checkgreen.packages/server/tests/on this box: 144 passed, 0 failed — the claim holds. Through the pinned toolchain (./.forgejo/deno.sh test -A packages/server/tests/external_modules_test.ts, sodenoland/deno:2.9.5+ debian git 2.39.5): 24 passed. The checkout-kill test's newindex.lockassertion runs for real in that image, so it can no longer degrade into a second copy of the healthy-tree test.git clone --no-localwith a stallingrequiredsmudge filter, wait for.git/index.lock, SIGKILL:index.lock seen before kill: true / survives the kill: true, thenBOOT 1: ok=true clones=1,BOOT 2: ok=true clones=0. Recovery-on-failure works and the repaired tree is fetched thereafter.#v9.9.9against a repo with no such ref, after a healthy boot: One clone, then none, and a corrected pin retires the signature file. That was the ask and it is met.git fetch failed: fatal: '…' does not appear to be a git repository … (re-clone also failed: git clone failed: fatal: repository '…' does not exist). Retryability is decided over the combined text (isRetryableFailure→isTransientGitError(failure.error)), as described.redactSecretsis a singlereplaceAllwith a static regex — pure and deterministic, so signature equality across boots is safe.legacyModuleSlugismain'sslugifyverbatim (9d9c48e:external.ts:315-324), with the floor guards added around it rather than inside it.originUrlIscompares credential-free forms through the sharedcredentialFreeUrl. The swap comment now claims exactly what remove-then-rename guarantees. The rate-limit change is genuinely gone (6 files).GitExitError/gitAnswered,remoteDefaultBranch,lstat, subcommand naming are all as described.migrateExternalAlertKeysismain.ts:199— afterDeno.serve, before the first alert write atmain.ts:223and before the retry loop starts. Nothing on the startup path writes a new-key row ahead of it.NotificationStore.rekey's two untested branches are correct. Drove both by hand: with both keys active the legacy row is left alone (no second live row on one key), and a dismissed legacy row is not resurrected.CLAUDE.mdcorrection is accurate.docker/Dockerfile:14isFROM denoland/deno:2.9.5@sha256:…and.forgejo/deno.sh:31seds the image out of that line. There is no second Deno pin anywhere in.forgejo/.Blocking: two configured refs of one repo defeat the bound entirely
…#mainand…#devslugify to one work tree — this repo says so itself atexternal-retry.ts:222-224, and there is a passing test named two config entries for one repo retry it once. One work tree means one${srcDir}.recovery-fault, andclearFault()(external.ts:309) fires on any successful sync. So the healthy entry deletes the failing entry's evidence before the failing entry ever reads it.One good ref plus one bad ref, three boots:
fault=SETthe whole way and a full recovery clone every boot regardless. Order-independent — bad entry first gives the same. Two bad refs is worse, because each one's signature is the other's "new evidence":Two full clones per boot, forever, from
main.ts:158— thirty-three lines beforeDeno.serve, which is the entire reason the bound exists. The fault's identity is (work tree, configured ref); the file's identity is the work tree alone. Keying the filename on the ref too is a one-line change.Blocking: the bound misses every checkout fault a fresh clone reproduces — and the comment names one of them as covered
It is not covered.
git cloneperforms its own checkout, so a checkout fault reproduced by the recovery surfaces from thegit clonecall and never reachescheckoutConfiguredRef:git checkout failed: …on one side,git clone failed: …on the other, plus clone's extrawarning: Clone succeeded, but checkout failed.The signature cannot match, ever.A
.gitattributesnaming arequiredfilter whose binary is absent, three boots:fault=noneevery time — nothing is persisted, so nothing is bounded.This is not the residual the description already owns. That one is scoped to "a fetch-stage fault the recovery reproduces as a clone-stage one … such clones die at the remote's answer, before any transfer", which I reproduced and which is true. But the pin is the only checkout fault whose recovery keeps the
checkoutname, precisely because clone checks out the default branch and the pin is applied separately afterwards. Every other checkout fault is renamed toclone, and its recovery clone is a completed transfer — git's ownClone succeeded, but checkout failedis the proof. A disk filling during the fetch has the same shape (git fetch failed: … No space leftvsgit clone failed: … No space left) and also pays the transfer.Both holes close by comparing the signature with the leading
git <cmd> failed:stripped. Nothing else in those messages is subcommand-specific, and the two texts that genuinely differ between fetch and clone (does not appear to be a git repositoryvsrepository '…' does not exist) stay different, so it does not over-match.Non-blocking
originUrlIscondemns a work tree on a non-answer (external.ts:290). Its catch returns false for "not a repo", "no origin" and "git could not answer" alike — and false meansDeno.remove(dir, { recursive: true }). It is the one place in this diff that does not apply the rule the rest of it establishes: only an answer may condemn a tree. Blast radius is the one-time migration window and the cost is a re-clone plus the manifest name that names the first post-upgrade failure — small, for the same reason the probe's guard is small, and that one got a typed error.briefly()collapses whitespace and cuts at 300 chars; the chained record with a real URL measures 367. Withhttps://git.example.com/opsdeck/mod-teamspeak.gitthe bell ends… (re-clone also failed: git clone failed: fatal: repository …. The label survives, the reason does not./systemcarries the full text so this is a nit — but the fault half is first and the recovery half is what gets eaten.rekey's both-keys-active branch leaves the legacy row orphaned forever and re-attempts it every boot. Correct as written and documented in the comment; reachable only when an earlier boot's own migration failed. Worth knowing it is the one outcome the sweep cannot repair.prepareInto.The mechanism is right and the pin case is genuinely bounded now. What is left is that the fault's identity is not quite the fault: it is shared between refs that do not share a fault, and it carries a subcommand name that changes as soon as the recovery is the thing that reproduces it.
Pushed as
192ed1f, merged onto currentmain(4e8defb). Both blocking items are fixed, and the fault's identity is now the pair you said it was. Numbers below are from your own labs, re-run with a countinggitshim onPATH.The bound is keyed on (work tree, configured ref)
faultSignatureFile()appends the FNV-1a ofref.refto the filename, so…#mainand…#devsharing one srcDir no longer share one file. The digest rather than the ref itself because a branch name is a path (release/1.2) and this is a filename.Good ref plus bad ref, three boots:
Two bad refs, after a healthy seed boot:
Test: refs that share a work tree do not share a bound — good,
#v9.9.9and#v8.8.8over three boots, asserting two signatures stand at once, that boot 1 recordsa fresh clone failed the same wayfor each and boots 2–3 record the fault alone whatever order the good entry runs in, and that the shared work tree is never consumed under it.The recovery no longer renames the fault it reproduces
Two changes, because the name could move in two ways:
--no-checkout, and the checkout ischeckoutConfiguredRefon both sides. You were right that the pin is the only checkout fault clone does not perform itself — so rather than teach the comparison about clone's advice block, the recovery now runs the same operation the fetch path does, and a checkout fault is worded by the samegit checkouteither way. It also retires the doubled checkout: clone's was of the default branch, which a pinned ref replaced on the next line.faultSignature()strips the leadinggit <cmd> failed:before comparing and before persisting, which is the fetch-vs-clone case you named —git fetch failed: … No space leftagainstgit clone failed: … No space left, where the recovery IS a completed transfer. The two texts that genuinely differ stay different; there is a unit test for both directions.Your
.gitattributeslab, three boots:Test: a checkout fault a fresh clone reproduces is bounded too — a commit whose tree holds a blob named
.git, which every git refuses to write (error: invalid path '.git') in a clone as much as in a work tree. No filter binary and no process-wide environment, so it is safe under the parallel runner, and it runs for real in the pinned image on git 2.39.5. It asserts the fault is worded by our checkout, that boot 2 reproduces and persists, that boot 3 buys no clone, and that what lands on disk is the message with its subcommand stripped.The residual paragraph the description used to own is gone with the residual.
Non-blocking, addressed
originUrlIscondemning on a non-answer — nowrepoOrigin()returning"mine" | "other" | "unknown", ongitAnswered.unknownleaves the directory where it is and logs why; the next boot that can ask decides. It is also only asked when adoption is still possible, since once srcDir exists the answer cannot change the outcome.briefly()eating the half the description calls actionable — the cut comes out of the middle now (150 +…+ 149). Asserted on the chained record: bothdoes not appear to be a gitanddoes not exist)survive it, credentials still do not. The trade for afrontend-buildbody is named in the comment.rekey's both-keys-active branch — left as is, for the reason you gave: correct as written, reachable only when an earlier boot's own migration failed, and the comment says so.One thing you did not ask for
Per-ref keying means a signature outlives a pin an operator corrects by writing a different ref, or a repo they drop from the config — a few hundred bytes under
/datathat nothing would name, which is the smell this whole PR is about.sweepFaultSignatures()runs once at the top ofprepareExternalModules, before anything writes a new one, and removes every.recovery-fault-<hex>file no configured ref could still read.isFilefirst:.survivesslugify, so a repo whose URL ends in.recovery-faultowns a work tree of that exact shape, and the sweep must never be what removes a checkout. Test: a signature no configured ref could read is swept at boot.Gates
deno task check,deno lint packages/server/,deno fmt --checkgreen.packages/server/tests/: 171 passed, 0 failed on this box, and 171 passed through the pinned toolchain (./.forgejo/deno.sh, sodenoland/deno:2.9.5+ debian git 2.39.5) at the head commit.Still not covered
The probe-failure guard (no git on PATH, a hung
/data) and a kill inside the recovery clone or the two-op swap window still have no end-to-end test, for the reasons already in the description.repoOrigin'sunknownbranch joins them — it needs the same unavailable git. The disk-full half of the signature stripping is covered by the unit test rather than by a full disk.Re-review at
192ed1f, checked against a running git rather than against the description. Merge base is currentmain, so nothing here is stale. Clone counts below are counted, not inferred — agitshim onPATHlogs every invocation.Verified first
deno task check,deno lint packages/server/,deno fmt --checkgreen. Through the pinned toolchain (./.forgejo/deno.sh test -A packages/server/tests/, sodenoland/deno:2.9.5+ debian git): 171 passed, 0 failed — the claim is exact. On my host the whole-suite run dies withdouble free or corruption (!prev)after the last test; that reproduces identically on the merge base (exit 139), so it is my box, not this branch..git, four boots:--no-checkouton both clones does make the fault come back worded by ourgit checkout, and what lands on disk is the message with the subcommand stripped.faultSignaturedoes not over-match: the two texts that genuinely differ between fetch and clone stay different.migrateExternalAlertKeysismain.ts:199— afterDeno.serve(190), before the retry loop (205) and before the first alert write (~218).sweepFaultSignaturesruns before the loop and before the early return, so a config emptied of external modules still sweeps.rekey's remaining corner is safe. A dismissed row already sitting at the new key does not block the UPDATE and does not need to:createselectsWHERE dismissed_at IS NULL, so the adopted row is still the one every later write finds.CLAUDE.mdcorrection is accurate.docker/Dockerfile:14isFROM denoland/deno:2.9.5@sha256:…,.forgejo/deno.sh:31seds the image out of that line, and there is no second Deno pin under.forgejo/.Blocking:
--no-checkoutcosts the manifest name, and the comment above it now says otherwiseThat is no longer true, and this PR is what made it untrue. Before,
git clonechecked out the default branch, so a repo whose configured checkout then failed still had a manifest on disk andreadManifestNamerecovered the module's name. With--no-checkout, when our checkout is the thing that fails the work tree is empty and there is nothing to read — on that boot and on every boot after it.Same lab, same repo, first-ever clone with
#v9.9.9,external.tsthe only difference:It is not only the first-ever clone. A tree the probe rejects (fetch-phase kill residue) is removed and re-cloned; if the pin is also wrong, that re-clone leaves an empty tree and a module that had been running for months loses its name:
The cost is the one that catch block exists to prevent: the bell reads
Module "git.example.com-opsdeck-mod-x-74016889" is not running, and/systemputs the repo inunidentified, so every row that module stored goes to "not knowable this boot" for as long as the pin is wrong. It does not falsely mark them orphaned —system.ts:144-148already guards that — so this is diagnostics, not data. But it is diagnostics this file spends a paragraph defending, it is a regression againstmain, and the description names dropping clone's checkout as pure win ("the doubled checkout goes away").It is also cheap to keep. The blob is in the object store even with
--no-checkout—git -C <dir> show HEAD:opsdeck.module.jsonreturns the manifest verbatim; I checked on the wrecked tree above. A fallback to that when the file read fails restores the old behaviour without giving back the--no-checkoutwin. Whichever way it goes, that comment has to stop claiming a first-ever failure is the only case.Non-blocking
… Please make sure you have the c…also failed: git clone failed: fatal: unable to access '…': Could not resolve host: git.example.com). Both reasons survive now, which is the half that matters — but(re-clonefalls in the hole, so the two errors read as one run-on sentence that closes a parenthesis it never opened. Budgeting the halves where they are composed, or biasing the cut to a word boundary, finishes it.checkoutConfiguredRef's barecatchdiscards attempt 1 and reports attempt 2. A tag whose tree git refuses to write:git checkout failed: fatal: git checkout: --detach does not take a path argument 'origin/v2.0.0'— the tag exists, and the real answer waserror: invalid path '.git'. The bound is unaffected (deterministic across boots; one clone then none, verified). Pre-existingcatch, but this PR makes that message the persisted fault identity, the bell body and the/systemtext, so keeping attempt 1's error when attempt 2 is a ref-resolution failure is worth the two lines.assertFalse(error.includes("fresh clone"))passes both for "no recovery ran" and for "a recovery ran and failed differently", since the chained form saysre-clone also failed. Asserting onre-clonecovers both. Nothing hides today — the pin case is deterministic.sweepFaultSignaturesremoves a ~200-byte signature for a repo dropped from the config, arguing that "a leftover under/datathat nothing names is the whole subject of this module" — while that repo's entiresrcDir, and any<srcDir>.recoveringa kill left inside a recovery, stay forever. ThesrcDirleak ismain's;.recoveringis new here. Not worth new machinery, but the stated rationale claims more than the sweep does.The bound is right now, per ref and across the subcommand rename, and I could not construct a fault that wedges a tree recovery could still fix. What is left is that the mechanism which made the bound work quietly took the module's name with it.
`--no-checkout` on both clones made a checkout fault reproducible by the recovery, and quietly took the module's name with it: clone's checkout of the default branch was also what left a manifest on disk for a repo whose configured checkout then failed, and `readManifestName` recovered the name from it in the failure record. Without it that work tree is empty, on that boot and every boot after, so the bell named the slug and /system filed the repo under "unidentified" — a regression against main. The blob is in the object store regardless: the failure-record fallback now reads `git show HEAD:opsdeck.module.json` (`manifestNameOf`, same schema) when the file is not in the work tree, and the comment above it no longer claims a first-ever failure is the only case that leaves the slug standing. Also, from the same review: - `checkoutConfiguredRef` makes its second attempt (`origin/<ref>`) only when `rev-parse --verify --quiet origin/<ref>^{commit}` says there is such a branch. Otherwise the ref-lookup failure of attempt 2 replaced the answer that matters — a tag that exists whose tree git refuses to write said "invalid path '.git'" on attempt 1 — and that message is the persisted fault identity, the bell body and the /system text. A pin no ref satisfies is now worded by attempt 1 too ('v9.9.9', not 'origin/v9.9.9'). - `briefly()` budgets a chained record half by half around the exported `RECOVERY_CHAIN` delimiter: one middle cut kept both reasons and ate the "(re-clone also failed:" between them. - The boot assertions that mean "no recovery ran" assert neither "fresh clone" nor "re-clone" (`assertNoRecovery`); the old form also passed for a recovery that ran and failed differently. - `sweepFaultSignatures` says what it sweeps and what it leaves: the dropped repo's srcDir (main's) and a `.recovering` a kill left (ours). Two tests: a pin that cannot be checked out still names the module (first clone, recovery boot, bounded boot, and a wrecked-then-re-cloned tree — with no manifest in the work tree, asserted), and a pinned ref that resolves but cannot be checked out reports the checkout's answer. The bell test asserts the seam survives the budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Rework for the fifth review is up at
69b7621; description updated to match. All five items, in review order.Blocking —
--no-checkoutcosts the manifest nameTaken the way you suggested: the failure-record fallback in
prepareExternalModule's catch now readsgit -C <srcDir> show HEAD:opsdeck.module.jsonwhenreadManifestNamefinds no file in the work tree, parsed by a newmanifestNameOfinmanifest.ts(same schema, same "readable, not acceptable" bar, shared withreadManifestName). HEAD is the default branch clone pointed it at, so it is the same manifest clone's checkout used to land — behaviour matchesmainagain without giving back the--no-checkoutwin. The comment above it no longer claims a first-ever failure is the only case; it now says what leaves the slug standing: nothing on disk at all, or a HEAD that never resolved.Your lab is now a test, both halves — a pin that cannot be checked out still names the module: a first-ever clone under
#v9.9.9reportsname=named nameFromManifest=truewithopsdeck.module.jsonasserted absent from the work tree (so the name provably came from the object store), holds through the recovery boot and the bounded boot after it, and holds again for a tree that ran once, was wrecked (.gitremoved), and was re-cloned under the bad pin.Non-blocking, all taken
briefly()now splits its budget on the chain: the delimiter is one exported constant (RECOVERY_CHAIN, written bysyncRepo, read by the bell), each half gets its own middle cut, the delimiter and the closing paren are kept, and a short half hands its unused share to the other — still 300 characters total. The bell test asserts(re-clone also failed: git clone failed:survives, along with both reasons and no credentials.checkoutConfiguredRefmakes its second attempt only whenrev-parse --verify --quiet origin/<ref>^{commit}says there is such a branch; otherwise the first attempt's error stands. Your tag case is a test — a pinned ref that resolves but cannot be checked out reports the checkout's answer: the uncheckoutable commit taggedv2.0.0recordsinvalid path '.git', notdoes not take a path argument, and a ref that is a remote branch (#release, no local branch of that name) still gets its second attempt and syncs. One visible consequence, in the description: a pin no ref satisfies is now worded by the first attempt —… 'v9.9.9'rather than… 'origin/v9.9.9'— which is the name the operator typed. Deterministic across boots, so the bound is unaffected.assertNoRecovery()asserts neitherfresh clonenorre-clone, at all four sites.srcDirstays (as onmain) and so does a<srcDir>.recoveringa kill left, which only the next sync of that same repo removes. No new machinery, per your read.Verification
deno task check,deno lint packages/server/,deno fmt --checkgreen.packages/server/tests/: 173 passed, 0 failed on this box (git 2.55, Windows). Same suite in the derived image.forgejo/deno.shbuilds (denoland/deno:2.9.5+ debian git): 173 passed, 0 failed — with the honest caveat that the script itself could not complete here (podman cprefuses a Windows junction undernode_modules), so the same image was run with the tree streamed in minusnode_modulesanddeno installinside. CI runs the script proper.Found on the way, not fixed here
While checking the
rev-parseguard against a running git: a pin on the default branch's name (…#mainwhenmainis the remote's default) never advances past the clone-time commit.git clonecreates a localmain,checkout --detach mainresolves to that local branch, andfetch --tags originonly movesorigin/main— so every boot re-checks-out the same commit:Pre-existing on
main(clone created the local branch there too), and--no-checkoutneither causes nor changes it. Not touched in this PR because the fix has a semantics question of its own — preferorigin/<ref>first and a branch shadows a tag of the same name; prefer the tag and it needs an explicitrefs/tags/probe — and this PR is already five rounds deep. Worth its own issue.Re-review at
69b7621, checked against a running git (2.55) and Deno 2.9.6, and against mutations of your own source rather than against the description.Verified first
deno task check,deno lint packages/server/(60 files),deno fmt --check(326 files) green;packages/server/tests/173 passed, 0 failed. Test count 12 → 30, so "eighteen" is exact.git show HEAD:opsdeck.module.jsonrestores the name; dropping it fails a pin that cannot be checked out still names the module.--no-checkout2, clone-beside/swap-on-success 3, theisBranchguard 1, the sweep 1,lstat1. That is the part that matters and it holds.#mainstaleness you were told about is genuinely pre-existing. Same result on4e8defband on this head;--no-checkoutneither causes nor changes it. Worth its own issue, as you said.CLAUDE.mdcorrection is accurate —docker/Dockerfile:14isFROM denoland/deno:2.9.5@sha256:…,.forgejo/deno.sh:31seds the image out of that line, no 2.5.6 anywhere. Redaction covers every log line that carries a URL, and the slug is credential-free.migrateExternalAlertKeysordering is right (158 → 191 → 199 → 208 → 223).Blocking: a merely dirty work tree is condemned, and the bound cannot see it
checkout --detachrefuses to run when a locally-modified tracked file would be overwritten. That is a non-transientGitExitError, sogitAnsweredis true, so the tree is the suspect, so it is removed and re-cloned. But a dirty tree is not an unusable tree — git said no about the checkout, not about the repository.The reason this is blocking rather than cosmetic is that the recovery succeeds, so
clearFault()runs and no signature is ever written. Four boots with something writing a tracked file intosrcDirbetween them:A full clone every boot, from thirty-three lines before
Deno.serve— the exact cost round 3 blocked on — plus every untracked file undersrcDirdestroyed each time, with onewarnline naming it. The bound structurally cannot catch this class, because it only engages when the recovery reproduces the fault.The fix is one word, and this file already argues for it: the comment at the leftover branch says everything under
srcDircame from the clone. If that is the invariant, thencheckout --force --detachis the checkout you wanted — I verified it takes the new ref with no re-clone and leaves untracked files alone. Onmainthis shape merely disabled the module; here it deletes data and pays a clone forever, so it is a regression this PR introduces.Blocking: the two guards that keep
rm -rfoff an innocent tree have no testgitAnsweredis the single predicate deciding whether a work tree gets removed. I replaced its body withreturn true— a network blip, the 120 s self-kill and a missing git all now condemn the tree — and all 30 tests pass. Same for the probe's rethrow guard.The description says simulating this "means mutating
PATHfor the whole test process, which the parallel runner makes unsafe". That is true of the missing-git half and not of the half that matters more: a healthy tree whose origin is unreachable needs noPATHmutation, and this file already points tests atgit://127.0.0.1:1/nope.git. One test — healthy tree, unreachable origin, assert the tree survives and nothing was re-cloned — closes the gap on the most destructive decision in the diff. Without it, the next edit that widensgitAnswereddeletes a good checkout on a DNS hiccup and the suite says nothing.Non-blocking
briefly()rework is entirely uncovered, and the description says otherwise.alerts.ts:145redacts beforebriefly, and the bell test's chained record is 268 characters after redaction — under the 300 budget, so it never enters the cutting code. All three chained assertions pass trivially, and revertingbrieflytomain's tail-cut passes 30/30. The mechanism is correct — I brute-forced every fault/recovery length pair: it never exceeds 300 and the seam always survives for a non-empty fault — but nothing verifies it. The fixture needs an uncredentialed URL (redaction is what shrinks the credentialed one below the threshold), orbrieflyneeds a direct unit test.isUsableWorkTreeto the literal #28 predicate —Deno.stat(dir + "/.git")— passes 30/30, because recovery-on-failure catches the killed clone anyway. That is the right architecture and you say so in the body; the consequence is that a clone the kernel killed is re-cloned no longer proves the probe, and the earlier "verified these fail with the source change stashed" is no longer true of it. Worth moving to "Not covered", which currently lists only the probe-failure guard.remoteDefaultBranchlikewise. Reverting it to the old literal still passes a work tree that lost origin/HEAD re-asks the remote — the recovery clone rescues it.Deno.writeTextFile(sigFile, faultSig).catch(() => {})fails on the very full disk that produced the fault, so the bound never engages for it. Logging the write failure would at least make it visible.Deno.remove(dir)succeeds andDeno.rename(fresh, dir)throws,catch (e2)then removesfresh— the old checkout and the fresh one are both gone and the record falls back to the slug. The comment covers a kill in that window, not a rename failure. Removingfreshonly whendirstill exists is a one-line guard.migrateLegacyCloneremoves the legacy directory with no origin check whensrcDiralready exists. With a legacy fold collision, repo A's boot deletes repo B's adoptable clone before B ever migrates. One wasted clone, and thelog.infonames it — but the "only git answering may condemn a directory" rule the rest of the file follows does not hold on this path.git show HEAD:…runs with the fullGIT_TIMEOUT_MS, aftersyncRepomay already have spent it, beforeprepareExternalModulereturns — on the pre-serve path.find(r => r.url === configuredAs), so a#devfailure is retried with the#mainentry and writes or clears the wrong ref's signature.readManifestNameno longer goes throughparseManifestFile, duplicating its two lines.The bound is right, per ref and across the subcommand rename, and I could not construct a fault that wedges a tree a recovery could still fix. What is left is that the recovery is now willing to destroy a tree git only declined to check out, and that the guards holding it back are the only untested part of the mechanism.
Rework for the sixth review is up at
ff1640f; description updated to match. Both blocking items are fixed, and every non-blocking one is taken except the two I argue below are already where they belong.Blocking — a merely dirty work tree is condemned
You were right about the shape and right that it is a regression this PR introduced: on
maina refused checkout disabled the module, here it deletes data and pays a clone forever. Fix is the one word you named —--forceon all threecheckout --detachsites.I reproduced the refusal before changing anything, because the naive repro does not show it:
So the class needs both halves — a dirty tracked file and a target that moves it — which is what the test now seeds: boot 1, edit
mod.tsinsrcDir, commit a differentmod.tson the remote, boot 2. a work tree git only declined to check out is kept, not condemned asserts the untracked marker survives (a re-clone removes the tree and it with it), that the new commit actually landed over the local edit, and that no signature was written. Reverting--forcefails it.The justification is the invariant the file already states at the leftover branch — everything under
srcDircame from the clone — so there are no local changes worth keeping, and--forceleaves untracked files alone, which is what the marker asserts through.Blocking — the guards that keep
rm -rfoff an innocent tree have no testConfirmed: with
gitAnsweredreplaced byreturn truethe whole file still passed. Took your suggestion exactly, since it needs noPATHmutation — a healthy tree whose origin went unreachable survives the boot: prepare once, drop an untracked marker, then point the tree'sremote.origin.urlatgit://127.0.0.1:1/nope.gitwhile the config still names the reachable path. That asymmetry is the point — a re-clone would succeed and take the tree with it, so the test fails loudly rather than silently.It asserts the outcome is a failure, that the failure is transient, that the record is the fault alone (
assertNoRecovery), that the marker survives, and that neither a signature nor a.recoveringis left. Withreturn true:The probe's rethrow guard is still uncovered and is now named in Not covered — that half genuinely does need a missing git.
Non-blocking, taken
briefly()is uncovered and the description said otherwise. Correct, and worse than uncovered: reverting it tomain's tail cut passed the file. Went with the direct unit test rather than an uncredentialed fixture, so the length pairs are exercised on purpose rather than incidentally. the bell keeps both halves of a chained record, and the seam builds a record past 300 from uncredentialed halves and asserts the delimiter and the closing paren survive, that each half is cut out of its middle (label at the front, reason at the end), that a short half hands its budget to the other, and that an unchained body is one middle cut.main's tail cut fails it. The description no longer claims the bell test covers this.remoteDefaultBranchare optimisations with no coverage. Both moved to Not covered, worded as you put it: recovery-on-failure rescues each, so a clone the kernel killed no longer proves the probe and a work tree that lost origin/HEAD no longer provesremoteDefaultBranch. That is the architecture and the claim was the thing that was wrong, not the code.could not record the fault: it will be recovered again), redacted. Still not driven by a full disk — said so.diralready removed the record still falls back to the slug. The swap is now two renames —dir→<srcDir>.replaced,fresh→dir, then drop the aside — and a rename that throws puts the old tree back..replacedjoins.recoveringin the sweep at the top ofsyncRepo. A kill inside the window still costs the checkout, which is what the comment claims and no more.migrateLegacyCloneremoves with no origin check whensrcDirexists.repoOriginis now asked on both paths. Adoption still needssrcDirfree; removal is the samerm -rfeither way, and skipping the question there made the steady-state path the one path that condemned a directory on no evidence.git()takes a per-call timeout and thegit show HEAD:…lookup gets 10 s. Losing the name costs the slug in one record; losing two more minutes costs the UI.ModuleFailurecarriesconfiguredRefand the chain looks the entry up by (url, ref), falling back to url-only for a failureload()raised (it never sees a ref). One chain per URL is unchanged — two would fight overindex.lockin the shared tree — so what this fixes is which ref that one chain uses, and therefore which signature it writes or clears.readManifestNameduplicatesparseManifestFile. Both go through onereadManifestText/manifestPathnow.Filed rather than fixed
The
#mainstaleness is #60, with the semantics question (branch-first shadows a tag of the same name; tag-first needs an explicitrefs/tags/probe) written down as the reason it is not a one-liner.Verification
deno task check,deno lint packages/server/(60 files),deno fmt --check(326 files) green.packages/server/tests/: 176 passed, 0 failed atff1640f, and 176 passed, 0 failed through./.forgejo/deno.shproper — the derived image from the Dockerfile'sFROM,denoland/deno:2.9.5+ debian git. Three new tests, and each was checked the way you check them:--forcereverted fails the first,gitAnswered→return truefails the second,main'sbrieflyfails the third.Re-review at
ff1640f, checked against a running git (2.55) and Deno 2.9.6, and against mutations of your own source rather than against the description.Verified first
deno task check,deno lint packages/server/(60 files),deno fmt --check(326 files) green;packages/server/tests/176 passed, 0 failed; the external file 30 → 33. "Twenty-one around this" is exact —mainhas 12 in that file.--forcefrom all three sites fails a work tree git only declined to check out is kept, not condemned;gitAnswered→return truefails a healthy tree whose origin went unreachable survives the boot;main's tail cut fails the bell keeps both halves of a chained record, and the seam. Your three claims about your three tests are all true.srcDirand moved on the remote between each:ok=true marker=true sigs=0every boot, against last round'smarker=falseevery boot.--forcetakes the new commit and leaves the untracked marker.gitAnsweredreduced toe instanceof GitExitError, dropping only the transient check — fails it too. The other half (!isTransientGitErroralone) still passes 176/0, which is exactly what you moved to "Not covered".briefly's seam is genuinely covered now. Head-onlymiddleCut, tail-onlymiddleCutand a dropped closing paren each fail the new test. That is three of its four real properties.--no-checkout2, clone-beside/swap 3, theisBranchguard 1, the sweep 1,lstat1.isUsableWorkTreeto the literal#28predicate, andremoteDefaultBranchto the old literal, each still pass 176/0. They belong in "Not covered" and that is where they are.GitExitErrorhalf ofgitAnsweredall pass 176/0 — as the section says.aside/fresharedirplus a literal suffix and every slug ends in-<8 hex>, so neither can ever name another repo'ssrcDir;prepareExternalModulesis sequential, so two refs sharing a work tree cannot race the swap; a throwingrename(dir, aside)reachescatch (e2)with the old tree intact.Non-blocking
brieflyreturns atif (one.length <= MAX_ERROR_CHARS) return oneand the split never runs. Both handoff mutations pass 176/0: droppingMath.min(fault.length, …), and droppingMath.max(…, budget − recovery.length)for a plain even split. The arithmetic is right — I checked it — but nothing holds it. It needs two fixtures that actually exceed 300 with one short half (a recovery over 253 characters against a short fault, and a fault over 245 against a short recovery) andassertEquals(briefly(record).length, 300)on each: the handoff is precisely what keeps the total at the full budget, and those two mutations come out at 184 and 185.Deno.remove(aside, …)after a successful swap passes 176/0 — and that leaves a full duplicate of the repo under/datafor the whole process lifetime, swept only by that repo's next sync. DroppingDeno.remove(fresh, …)on the failure path passes 176/0 too. "Briefly doubling the repo's footprint" is the word nothing checks, and an unnamed second copy under/datais the smell this PR exists to end. OnereadDir(srcRoot)assertion after the recovery boot in an unsatisfiable pin costs one recovery clone pins both.configuredRef: ref.reffrom the failure record silently reverts the whole fix — the pairedfindthen never matches and every failure falls through to the url-only lookup — and that passes 176/0, as does reverting the lookup itself. Worth knowing too: it is a no-op in the common ordering, since with[U#main, U#dev]both failing,host.failedorder equals config order and both lookups pick#main; it bites when the failing entry is not the first for its URL. Either a unit test over the resolution, or name it alongside the others.repoOrigin'sconfig --getandisUsableWorkTree'srev-parseare local-only, on the pre-Deno.servepath, still carrying the full 120 sGIT_TIMEOUT_MS— which is the argumentNAME_LOOKUP_TIMEOUT_MSmakes one function away. The 10 s budget applies to them for the same reason.--forcedoes touch untracked files, in one case. On git 2.55: an untracked file at a path the target commit tracks makes plaincheckout --detachrefuse ("The following untracked working tree files would be overwritten by checkout"), and--forceoverwrites it. So the comment atcheckoutConfiguredRefclaims slightly more than--forcegives — worth rewording, not changing, since it is still strictly better than the re-clone it replaces and it quietly closes that refusal class too.middleCuthas a latent trap now thatbrieflyis exported. Formax <= 2the tail goes to zero or below, andtext.slice(-0)istext.slice(0)— it returns the whole string plus an ellipsis, longer than its own budget. Unreachable frombrieflytoday (when a cut is needed neither share drops below 137, which I checked over the whole space), but threeMathcalls are what stand between them.Math.max(0, max - 1 - head)andtext.slice(text.length - tail)retire it.U#mainandU#devshare one notification row while the fault signature, the failure record and the retry chain are all per-ref now. Pre-existing, but this PR is what made the pair first-class everywhere else.Both blockers are fixed in the source and each is held by a test that fails under exactly the mutation it needed to fail under, every earlier round's fix is still defended with identical counts, and there is no new way for this code to remove a directory. What is left is test coverage and two comments claiming a little more than the code does, none of it on a destructive path. Approving with those.