[hahn-kev-bot]

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 pullBundleChunk against an empty repo (sena-3) with stale cached base hashes. The server's isValidBase spun on the empty repo and OOM-killed the container; the client retried every FAIL(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). baseHashes is now optional in RestDispatcher (a cache-cleared client re-sends pullBundleChunk with none; requiring it produced a retried FAIL(400)), and HgRunner.MakeBundle treats 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-hash hg 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. ProcessRunner now returns exit code and stderr; getRevisions/getBranchTips/isValidBase key on the exit code and fail fast with the stderr on a real hg error, instead of inferring failure from empty stdout (the old uninformative command 'hg log' failed!). The empty-repo -1→0 sentinel is preserved.

D — bounded getRevisions. Each hg log is 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-process Running registry (and lock-file age) and polled rather than duplicated; a stale .async_run left by a crashed/restarted process is reaped and respawned; respawns are bounded by a genAttempts counter → FAIL.

F — cache GC. New CacheGarbageCollector BackgroundService reaps .bundle/.metadata/.async_run/.incoming older than ~24h (prod leaked 1,258 .async_run locks).

G — chunkSize clamp to a config cap (HGRESUME_CHUNK_SIZE_MAX, default 20 MB).

All new behaviour is env-overridable via ApiConfig with safe defaults.

Test plan

• HgResume.Api.Tests (unit): 17/17 — includes new CacheGarbageCollectorTests (reaps stale artifacts; leaves fresh/unrelated files).
• HgResume.IntegrationTests (Testcontainers, real image):
  • PullFacts 19/19 — includes new regressions: empty-repo first-sync with no baseHashes → NOCHANGE then push succeeds; non-empty repo with no baseHashes → servable full-clone bundle; plus the existing empty-repo-loop and missing-base-across-pages regressions.
  • PushFacts + MiscFacts + ContractFacts 31/31.
  • SendReceiveTests + VerifyHgWorking (end-to-end Chorus client) 4/4.

---

This change is 

[rmunn]

One style nit that doesn't need to be addressed but I wanted to mention, and a few minor changes that should be addressed, like changing the lower bound on the regex repeat count. Then after addressing those minor changes, no need for a re-review; it LGTM and can be merged once those changes are addressed. (Which is why I've approved it now rather than using "Request changes", in case I'm unavailable after the changes are made).

[rmunn]

Style comment only. This function is an example of why I dislike the "opening and closing braces on their own line" style that seems to have become standard in C#. I would much rather see this code written as:

try { Collect(); }
catch (Exception e) { _logger.LogWarning(e, "Cache GC pass failed"); }

try { await Task.Delay(_config.CacheGcInterval, stoppingToken); }
catch (OperationCanceledException) { break ; }

5 lines instead of 17, and taking up much less vertical space on-screen. With functions taking up less vertical space, it's easier to read them in the context of surrounding code, rather than scrolling up and down to see surrounding context.

Very much a style nitpick. But this is one of the things that bugs me every time I see it, and this while loop is one of the best examples I've ever seen where readability would have been much improved by having fewer lines of code.

[rmunn]

The comment says 12-char or 40-char hashes, but this regex would match numerical revision numbers like 3 (or even 0) as well, because the {1,40} repeat modifier is lower-bounded at 1. One of the other comments mentions trying to reject revision-number values like 0; if that's the case, then the {1,40} here should probably be {12,40}. Or, if it's possible for Mercurial to produce smaller short hex IDs, {8,40} at the minimum.

Or, if revnums like 0 should actually match here, then the comment should be adjusted accordingly.

[rmunn]

Change hg tip exit to hg tip exit code so it's more obvious in the log that it's a numeric exit code.

[rmunn]

If we use {node} here instead of {node|short} then it will pass the very-careful caching check created in sillsdev/chorus#258 and then if the same revnum is ever checked twice, the second check will return a cached value instead of re-running hg log on the same revnum.

... No, wait. This is going through ProcessRunner, not HgRunner. The Chorus caching code never sees this run. Never mind, this isn't going to have any effect on the Chorus caching.

[rmunn]

It's possible we may want to implement something like the CHORUS_HG_EXE environment variable and use chg rather than hg here, paying far less of a process startup penalty for each hg log invocation after the first.

[rmunn]

I suspect 500 bytes might truncate in the middle of a stacktrace. Shall we make this 4096 bytes instead?