Fix resumable-transport DDoS: terminate degenerate pullBundleChunk requests - #27
Open
hahn-kev-bot wants to merge 1 commit into
Open
hahn-kev-bot wants to merge 1 commit into
hahn-kev-bot wants to merge 1 commit into
Conversation
…quests A single resumable client looped pullBundleChunk against an empty repo with stale cached base hashes, spinning isValidBase and OOM-killing the container. Make every degenerate request terminate with a correct, non-retryable response: A. baseHashes is now optional in RestDispatcher; MakeBundle treats an empty list as --all. A cache-cleared client (no baseHashes) gets NOCHANGE on an empty repo / a full clone on a non-empty one instead of a retried FAIL(400). B. isValidBase does a direct per-hash `hg log -r <hash>` existence check (O(k), hex-validated) instead of paging the whole history. C. ProcessRunner surfaces exit code + stderr; getRevisions/getBranchTips/ isValidBase fail fast on a real hg error instead of inferring from empty stdout. D. getRevisions bounds each `hg log` to O(offset+quantity) via -l, preserving newest-first ordering and offset semantics. E. Concurrent-generation guard (in-process registry), stale-lock crash recovery, and bounded respawn -> FAIL before MakeBundle. F. CacheGarbageCollector BackgroundService reaps abandoned .bundle/.metadata/ .async_run/.incoming artifacts (~24h TTL); prod leaked 1,258 .async_run locks. G. Clamp client-supplied chunkSize to a config cap. Tests: new PullFacts regressions (empty-repo first-sync with no baseHashes -> NOCHANGE then push; non-empty repo with no baseHashes -> servable full clone) and CacheGarbageCollector unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
better failure state handling, based on a recent incident which took down the server
🤖 AI summary
Incident: A single "Low Bandwidth" (HgResume v03 / resumable) client DDoS'd prod by looping
pullBundleChunkagainst an empty repo (sena-3) with stale cached base hashes. The server'sisValidBasespun on the empty repo and OOM-killed the container; the client retried everyFAIL(400)with no backoff. This closes the server side of that incident so every degenerate or malicious request terminates with a correct, non-retryable-forever response.All changes are in
csharp/src/HgResume.Api(the C# rewrite).A — empty/missing
baseHashes(the live bug).baseHashesis now optional inRestDispatcher(a cache-cleared client re-sendspullBundleChunkwith none; requiring it produced a retriedFAIL(400)), andHgRunner.MakeBundletreats an empty list as--all. Result: empty repo →NOCHANGE, non-empty repo → full clone. Both terminate.B —
isValidBase→ direct per-hash lookup. Replaced the whole-history paging scan with a per-hashhg log -r <hash>existence check (O(k)), hex-validated (^[0-9a-fA-F]{1,40}$) and passed positionally (no revset/shell injection). Independently fixes the empty-repo spin.C — hg reliability.
ProcessRunnernow returns exit code and stderr;getRevisions/getBranchTips/isValidBasekey on the exit code and fail fast with the stderr on a real hg error, instead of inferring failure from empty stdout (the old uninformativecommand 'hg log' failed!). The empty-repo-1→0sentinel is preserved.D — bounded
getRevisions. Eachhg logis bounded to O(offset+quantity) via-l, preserving newest-first ordering and offset semantics.E — concurrent-generation guard + crash recovery. Before
MakeBundle, an in-flight generation is detected via the in-processRunningregistry (and lock-file age) and polled rather than duplicated; a stale.async_runleft by a crashed/restarted process is reaped and respawned; respawns are bounded by agenAttemptscounter →FAIL.F — cache GC. New
CacheGarbageCollectorBackgroundServicereaps.bundle/.metadata/.async_run/.incomingolder than ~24h (prod leaked 1,258.async_runlocks).G —
chunkSizeclamp to a config cap (HGRESUME_CHUNK_SIZE_MAX, default 20 MB).All new behaviour is env-overridable via
ApiConfigwith safe defaults.Test plan
HgResume.Api.Tests(unit): 17/17 — includes newCacheGarbageCollectorTests(reaps stale artifacts; leaves fresh/unrelated files).HgResume.IntegrationTests(Testcontainers, real image):PullFacts19/19 — includes new regressions: empty-repo first-sync with nobaseHashes→NOCHANGEthen push succeeds; non-empty repo with nobaseHashes→ servable full-clone bundle; plus the existing empty-repo-loop and missing-base-across-pages regressions.PushFacts+MiscFacts+ContractFacts31/31.SendReceiveTests+VerifyHgWorking(end-to-end Chorus client) 4/4.This change is