From def8c57aaa8275d9000c09c04775e7ddc91145b8 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 2 Oct 2026 13:44:01 +0700 Subject: [PATCH] Fix resumable-transport DDoS: terminate degenerate pullBundleChunk requests 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 ` 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 --- csharp/src/HgResume.Api/ApiConfig.cs | 37 ++++++ csharp/src/HgResume.Api/AsyncRunner.cs | 29 +++++ .../src/HgResume.Api/CacheGarbageCollector.cs | 91 ++++++++++++++ csharp/src/HgResume.Api/HgResumeApi.cs | 57 +++++++-- csharp/src/HgResume.Api/HgRunner.cs | 114 ++++++++++++------ .../HgResume.Api/Manage/RepoManageService.cs | 2 +- csharp/src/HgResume.Api/ProcessRunner.cs | 14 ++- csharp/src/HgResume.Api/Program.cs | 3 + csharp/src/HgResume.Api/RestDispatcher.cs | 6 +- .../CacheGarbageCollectorTests.cs | 72 +++++++++++ .../HgResume.IntegrationTests/PullFacts.cs | 40 ++++++ 11 files changed, 417 insertions(+), 48 deletions(-) create mode 100644 csharp/src/HgResume.Api/CacheGarbageCollector.cs create mode 100644 csharp/test/HgResume.Api.Tests/CacheGarbageCollectorTests.cs diff --git a/csharp/src/HgResume.Api/ApiConfig.cs b/csharp/src/HgResume.Api/ApiConfig.cs index d024097..4899063 100644 --- a/csharp/src/HgResume.Api/ApiConfig.cs +++ b/csharp/src/HgResume.Api/ApiConfig.cs @@ -43,6 +43,37 @@ public sealed class ApiConfig /// public required bool RequireManageSecret { get; init; } + /// Default upper bound on a client-supplied chunkSize (20 MB, matching the Chorus client). + public const int DefaultChunkSizeMax = 20 * 1024 * 1024; + + /// + /// Upper bound the server clamps a client-supplied chunkSize to. + /// already caps each read at the remaining bundle length, so this is cheap defense against a client + /// asking for an absurd allocation. + /// + public int ChunkSizeMax { get; init; } = DefaultChunkSizeMax; + + /// + /// How old a .async_run lock that is NOT tracked by this process's in-memory registry must be + /// before it is treated as a dead generation (API restarted mid-bundle, lock file left behind) and + /// cleaned up so a new generation can be spawned. A live generation in this process is recognised via + /// the registry regardless of age, so this only governs cross-restart recovery. Default 2 minutes. + /// + public TimeSpan StaleBundleLockThreshold { get; init; } = TimeSpan.FromSeconds(120); + + /// + /// Maximum number of times the server will (re)spawn bundle generation for a single pull transaction + /// before giving up with FAIL, so a bundle that genuinely cannot be produced fails fast instead of + /// respawning hg forever. Default 3. + /// + public int MaxBundleGenAttempts { get; init; } = 3; + + /// Age after which the cache GC reaps a .bundle/.metadata/.async_run/.incoming file. Default 24h. + public TimeSpan CacheTtl { get; init; } = TimeSpan.FromHours(24); + + /// How often the cache GC background service runs. Default 1h. + public TimeSpan CacheGcInterval { get; init; } = TimeSpan.FromMinutes(60); + public static ApiConfig FromEnvironment(bool isDevelopment) { string cache = Env("HGRESUME_CACHE_PATH", "/var/cache/hgresume"); @@ -59,6 +90,12 @@ public static ApiConfig FromEnvironment(bool isDevelopment) ResetCleanupAgeDays = EnvInt("HGRESUME_RESET_CLEANUP_AGE_DAYS", 31), ManageSecret = Environment.GetEnvironmentVariable("HGRESUME_MANAGE_SECRET") is { Length: > 0 } s ? s : null, RequireManageSecret = EnvBool("HGRESUME_REQUIRE_MANAGE_SECRET", !isDevelopment), + ChunkSizeMax = EnvInt("HGRESUME_CHUNK_SIZE_MAX", DefaultChunkSizeMax), + StaleBundleLockThreshold = + TimeSpan.FromSeconds(EnvInt("HGRESUME_STALE_BUNDLE_LOCK_SECONDS", 120)), + MaxBundleGenAttempts = EnvInt("HGRESUME_MAX_BUNDLE_GEN_ATTEMPTS", 3), + CacheTtl = TimeSpan.FromHours(EnvInt("HGRESUME_CACHE_TTL_HOURS", 24)), + CacheGcInterval = TimeSpan.FromMinutes(EnvInt("HGRESUME_CACHE_GC_INTERVAL_MINUTES", 60)), }; } diff --git a/csharp/src/HgResume.Api/AsyncRunner.cs b/csharp/src/HgResume.Api/AsyncRunner.cs index c45d905..2a5b8db 100644 --- a/csharp/src/HgResume.Api/AsyncRunner.cs +++ b/csharp/src/HgResume.Api/AsyncRunner.cs @@ -98,6 +98,35 @@ await File.WriteAllTextAsync(lockFile, public bool IsRunning() => File.Exists(_lockFile); + /// + /// True when this lock is backed by a background task still running in THIS process. Survives as a + /// real liveness signal (unlike a bare lock file, which also persists across a process restart that + /// killed the generation). Used to tell an in-flight bundle from a dead lock left by a crash. + /// + public bool IsTrackedRunning() => Running.ContainsKey(_lockFile); + + /// + /// True when a lock file exists for a generation this process is NOT running (not in ) and it was last written longer than ago — i.e. a + /// generation abandoned by a crash/restart. Such a lock must be cleaned up and respawned rather than + /// polled forever. A lock this process IS running is never stale regardless of age. + /// + public bool IsStaleLock(TimeSpan threshold) + { + if (!File.Exists(_lockFile) || Running.ContainsKey(_lockFile)) + { + return false; + } + try + { + return DateTime.UtcNow - File.GetLastWriteTimeUtc(_lockFile) > threshold; + } + catch + { + return false; // if we can't stat it, don't treat it as reapable + } + } + public async Task IsCompleteAsync(CancellationToken ct = default) { if (!File.Exists(_lockFile)) diff --git a/csharp/src/HgResume.Api/CacheGarbageCollector.cs b/csharp/src/HgResume.Api/CacheGarbageCollector.cs new file mode 100644 index 0000000..2336645 --- /dev/null +++ b/csharp/src/HgResume.Api/CacheGarbageCollector.cs @@ -0,0 +1,91 @@ +namespace HgResume.Api; + +/// +/// Periodically reaps abandoned transaction artifacts from . +/// +/// Only finishPull/PushBundle cleaned these up, so a client that looped (or simply went away) +/// left its .bundle/.metadata/.async_run/.incoming files behind forever. The July 2026 incident +/// left 1,258 leaked .async_run locks. The PHP deployment would have needed a cron job; +/// the .NET host is long-running, so a fits without any image +/// process-model change. +/// +public sealed class CacheGarbageCollector : BackgroundService +{ + // Transaction artifacts BundleHelper/AsyncRunner write into the cache dir. Repo-reset backups live + // under their own TempRepoFolder and are reaped by RepoManageService, so we don't touch those. + private static readonly string[] Extensions = { ".bundle", ".metadata", ".async_run", ".incoming" }; + + private readonly ApiConfig _config; + private readonly ILogger _logger; + + public CacheGarbageCollector(ApiConfig config, ILogger logger) + { + _config = config; + _logger = logger; + } + + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + // Run once at startup (reap anything a prior process left behind), then on the configured cadence. + while (!stoppingToken.IsCancellationRequested) + { + try + { + Collect(); + } + catch (Exception e) + { + _logger.LogWarning(e, "Cache GC pass failed"); + } + + try + { + await Task.Delay(_config.CacheGcInterval, stoppingToken); + } + catch (OperationCanceledException) + { + break; + } + } + } + + /// Deletes cache artifacts whose last-write time is older than . + public int Collect() + { + if (!Directory.Exists(_config.CachePath)) + { + return 0; + } + + DateTime cutoff = DateTime.UtcNow - _config.CacheTtl; + int removed = 0; + foreach (string path in Directory.EnumerateFiles(_config.CachePath)) + { + if (!Extensions.Contains(Path.GetExtension(path))) + { + continue; + } + try + { + if (File.GetLastWriteTimeUtc(path) < cutoff) + { + File.Delete(path); + removed++; + } + } + catch (Exception e) + { + // A file deleted or rewritten by a concurrent request between stat and delete is fine; + // just skip it this pass. + _logger.LogDebug(e, "Cache GC could not reap {Path}", path); + } + } + + if (removed > 0) + { + _logger.LogInformation("Cache GC reaped {Count} stale cache file(s) from {Path}", + removed, _config.CachePath); + } + return removed; + } +} diff --git a/csharp/src/HgResume.Api/HgResumeApi.cs b/csharp/src/HgResume.Api/HgResumeApi.cs index fc214d0..b6d2237 100644 --- a/csharp/src/HgResume.Api/HgResumeApi.cs +++ b/csharp/src/HgResume.Api/HgResumeApi.cs @@ -207,6 +207,13 @@ public async Task PullBundleChunkInternalAsync(string repoId, { return Fail("invalid offset"); } + // Clamp a client-supplied chunkSize to a configured cap. GetChunkAsync already bounds each + // read to the remaining bundle length, so this is cheap defense against an absurd value (and + // keeps offset + chunkSize from overflowing in the size checks below). + if (chunkSize > _config.ChunkSizeMax) + { + chunkSize = _config.ChunkSizeMax; + } var hg = new HgRunner(repoPath); if (!await hg.IsValidBaseAsync(baseHashes, ct)) @@ -248,13 +255,49 @@ public async Task PullBundleChunkInternalAsync(string repoId, ["Error"] = "Cannot request data for bundle that doesnt exist yet", }); } - // first pull request (offset == 0): make a new bundle - asyncRunner = waitForBundleToFinish - ? await hg.MakeBundleAndWaitUntilFinishedAsync(sortedBase, bundleFilename, ct) - : hg.MakeBundle(sortedBase, bundleFilename); - bundle.SetProp("tip", await hg.GetTipAsync(ct)); - bundle.SetProp("repoId", repoId); - bundle.State = BundleHelper.State_Bundle; + + // Concurrent-generation guard + crash recovery. A duplicate first-chunk retry can arrive + // before the .bundle file appears; without this it spawns a second `hg bundle` for the same + // transId. A generation is "in flight" if a background task in this process is still running + // it, or a lock file exists that is not yet old enough to be considered abandoned. + bool generationInFlight = asyncRunner.IsTrackedRunning() || + (asyncRunner.IsRunning() && !asyncRunner.IsStaleLock(_config.StaleBundleLockThreshold)); + + if (generationInFlight) + { + // Someone else is already building this bundle. Don't respawn; poll it via State_Bundle. + if (bundle.State != BundleHelper.State_Bundle && + bundle.State != BundleHelper.State_Downloading) + { + bundle.State = BundleHelper.State_Bundle; + } + } + else + { + // Reap a stale lock left by a generation this process no longer runs (API restarted + // mid-bundle) so it can't deadlock the transaction, then respawn. + if (asyncRunner.IsRunning()) + { + asyncRunner.CleanUp(); + } + + // Bound respawns so a bundle that genuinely can't be produced fails fast with FAIL + // instead of respawning hg forever (the OOM loop). + int attempts = (int.TryParse(bundle.GetProp("genAttempts"), out var prev) ? prev : 0) + 1; + if (attempts > _config.MaxBundleGenAttempts) + { + return Fail($"bundle generation failed after {attempts - 1} attempts"); + } + bundle.SetProp("genAttempts", attempts.ToString()); + + // first pull request (offset == 0): make a new bundle + asyncRunner = waitForBundleToFinish + ? await hg.MakeBundleAndWaitUntilFinishedAsync(sortedBase, bundleFilename, ct) + : hg.MakeBundle(sortedBase, bundleFilename); + bundle.SetProp("tip", await hg.GetTipAsync(ct)); + bundle.SetProp("repoId", repoId); + bundle.State = BundleHelper.State_Bundle; + } } var response = new HgResumeResponse(HgResumeResponse.SUCCESS); diff --git a/csharp/src/HgResume.Api/HgRunner.cs b/csharp/src/HgResume.Api/HgRunner.cs index 33c7c5d..dfda120 100644 --- a/csharp/src/HgResume.Api/HgRunner.cs +++ b/csharp/src/HgResume.Api/HgRunner.cs @@ -13,6 +13,13 @@ public sealed class HgRunner private static readonly Regex ParentMinusOne = new("parent:\\s*-1:", RegexOptions.Compiled); private static readonly Regex NotAMercurialBundle = new("abort:.*not a Mercurial bundle", RegexOptions.Compiled); + // A hg short (12-char) or full (40-char) node id. Anything else cannot be a base revision. + private static readonly Regex HexHash = new("^[0-9a-fA-F]{1,40}$", RegexOptions.Compiled); + // hg's "unknown revision" / "unknown revision or ambiguous" abort, meaning the hash simply isn't + // in this repo (an absent base) — as opposed to a corruption/lock error we must surface. + private static readonly Regex UnknownRevision = + new("abort:.*(unknown revision|ambiguous identifier|filtered revision)", RegexOptions.Compiled); + public string RepoPath { get; } public HgRunner(string repoPath) @@ -92,7 +99,12 @@ public AsyncRunner Update(string revision = "") public AsyncRunner MakeBundle(IReadOnlyList baseHashes, string bundleFilePath) { var args = new List { "bundle" }; - if (baseHashes.Count == 1 && baseHashes[0] == "0") + // An empty baseHashes list means the same thing as ["0"]: the client has no common base (it + // cleared its cache, or this is a first sync), so send everything via --all. Without this an + // empty list produced `hg bundle -t v1` (no --base/--all) → hg aborts → FAIL → the client + // retries forever. For an empty repo --all yields NOCHANGE upstream; for a non-empty repo it + // yields a full clone. Both terminate. + if (baseHashes.Count == 0 || (baseHashes.Count == 1 && baseHashes[0] == "0")) { args.Add("--all"); args.Add(bundleFilePath); @@ -138,7 +150,11 @@ public async Task GetTipAsync(CancellationToken ct = default) public async Task> GetBranchTipsAsync(CancellationToken ct = default) { - var (branches, _) = await ProcessRunner.RunAsync(RepoPath, "hg", ["branches"], ct); + var (branches, branchesExit, branchesErr) = await ProcessRunner.RunAsync(RepoPath, "hg", ["branches"], ct); + if (branchesExit != 0) + { + throw new HgException($"command 'hg branches' failed (exit {branchesExit}): {Truncate(branchesErr)}"); + } var revisionArray = new List(); foreach (var branch in branches) { @@ -174,22 +190,35 @@ private async Task> GetRevisionsInternalAsync(int offset, int quant { throw new ValidationException("quantity parameter much be larger than 0"); } + // Bound each query to the requested window instead of listing the whole history and paging in + // memory. `hg log -l N` returns the newest N changesets in reverse-revision order — the same + // prefix the unbounded log produced — so Skip(offset).Take(quantity) yields an identical result + // while keeping the cost O(offset + quantity) per call rather than O(history). offset is + // non-negative for every real caller; clamp defensively so a bogus negative offset can't ask hg + // for a negative limit. + int window = (offset > 0 ? offset : 0) + quantity; // ':' is illegal in branch names (it is used in tags) so we use it to split hash and branch string[] args = branch is null - ? new[] { "log", "--template", "{node|short}:{branches}\n" } - : new[] { "log", "-b", branch, "--template", "{node|short}:{branches}\n" }; + ? new[] { "log", "-l", window.ToString(), "--template", "{node|short}:{branches}\n" } + : new[] { "log", "-b", branch, "-l", window.ToString(), "--template", "{node|short}:{branches}\n" }; - var (output, _) = await ProcessRunner.RunAsync(RepoPath, "hg", args, ct); + var (output, logExit, logErr) = await ProcessRunner.RunAsync(RepoPath, "hg", args, ct); + if (logExit != 0) + { + // A real hg failure (corruption, stale lock, bad branch, fork-failure-under-load). Fail fast + // with the exit code and stderr rather than the old uninformative "command 'hg log' failed!". + throw new HgException($"command 'hg log' failed (exit {logExit}): {Truncate(logErr)}"); + } if (output.Count == 0) { - var (tip, _) = await ProcessRunner.RunAsync(RepoPath, "hg", + var (tip, tipExit, tipErr) = await ProcessRunner.RunAsync(RepoPath, "hg", ["tip", "--template", "{rev}:{branches}\n"], ct); - if (tip.Count == 1 && tip[0].StartsWith("-1")) + if (tipExit == 0 && tip.Count == 1 && tip[0].StartsWith("-1")) { // Empty repo (hg init, zero changesets). At offset 0 we emit '0:' (from // '-1:') as the sentinel callers expect; past offset 0 there is nothing more, // so return empty. Returning the sentinel for every offset would make paginating - // callers (e.g. IsValidBase) loop forever, since they never see an empty page. + // callers loop forever, since they never see an empty page. if (offset > 0) { return new List(); @@ -197,47 +226,60 @@ private async Task> GetRevisionsInternalAsync(int offset, int quant tip[0] = Regex.Replace(tip[0], "^-1", "0"); return tip; } - throw new HgException($"command 'hg log' failed!\n"); + // hg log exited 0 with no output but this is not the empty-repo sentinel — surface it rather + // than silently returning empty. + throw new HgException( + $"command 'hg log' returned no revisions (hg tip exit {tipExit}): {Truncate(tipErr)}"); } return output.Skip(offset).Take(quantity).ToList(); } + /// + /// True if every requested hash is a real revision in this repo (the "0" sentinel is always valid). + /// Each hash is checked directly with `hg log -r ` — O(k) in the number of hashes — rather + /// than paging the whole history looking for them (which was O(N) per page, O(N·k) overall, and + /// spun forever on an empty repo). Hashes are hex-validated and passed as a positional argument, so + /// there is no revset/shell injection. + /// public async Task IsValidBaseAsync(IReadOnlyList hashes, CancellationToken ct = default) { if (hashes.Count == 1 && hashes[0] == "0") { return true; // special case indicating revision 0 } - int foundHash = 0; - const int q = 200; - int i = 0; - while (foundHash < hashes.Count) + foreach (var hash in hashes) { - var revisions = await GetRevisionsAsync(i, q, ct); - if (revisions.Count == 0) - { - return false; // paged past the last revision without matching every hash - } - foreach (var hashAndBranch in revisions) - { - int colon = hashAndBranch.IndexOf(':'); - string rev = colon >= 0 ? hashAndBranch.Substring(0, colon) : hashAndBranch; - if (hashes.Contains(rev)) - { - foundHash++; - if (foundHash >= hashes.Count) break; - } - } - // A page shorter than the requested quantity means hg returned everything it had, so this - // was the last page. Stop rather than advancing the offset again: this guarantees the loop - // terminates even if GetRevisions ever returns a fixed non-empty page regardless of offset - // (the empty-repo '0:' sentinel bug, or any similar future quirk). - if (revisions.Count < q) + if (!await RevisionExistsAsync(hash, ct)) { - break; + return false; } - i += q; } - return foundHash >= hashes.Count; + return true; } + + private async Task RevisionExistsAsync(string hash, CancellationToken ct) + { + // Anything that is not a hg short/long node id can't be a base. Reject it here so it never + // reaches `hg log -r` as a revset expression. + if (!HexHash.IsMatch(hash)) + { + return false; + } + var (output, exitCode, stderr) = await ProcessRunner.RunAsync(RepoPath, "hg", + ["log", "-r", hash, "--template", "{node|short}\n"], ct); + if (exitCode == 0) + { + return output.Count > 0; + } + // hg exits non-zero for an unknown/ambiguous revision (a legitimately absent base). Distinguish + // that from a genuine hg error (corruption, stale lock), which must surface rather than be + // reported as a merely-invalid base. + if (UnknownRevision.IsMatch(stderr)) + { + return false; + } + throw new HgException($"command 'hg log -r' failed (exit {exitCode}): {Truncate(stderr)}"); + } + + private static string Truncate(string s) => s.Length > 500 ? s.Substring(0, 500) : s; } diff --git a/csharp/src/HgResume.Api/Manage/RepoManageService.cs b/csharp/src/HgResume.Api/Manage/RepoManageService.cs index 36a48d5..59e1f41 100644 --- a/csharp/src/HgResume.Api/Manage/RepoManageService.cs +++ b/csharp/src/HgResume.Api/Manage/RepoManageService.cs @@ -50,7 +50,7 @@ private async Task InitRepoAt(DirectoryInfo repoDirectory, CancellationToken can { repoDirectory.Parent?.Create(); var workingDir = repoDirectory.Parent?.FullName ?? RepoRoot; - var (_, exitCode) = await ProcessRunner.RunAsync( + var (_, exitCode, _) = await ProcessRunner.RunAsync( workingDir, "hg", ["init", repoDirectory.FullName], diff --git a/csharp/src/HgResume.Api/ProcessRunner.cs b/csharp/src/HgResume.Api/ProcessRunner.cs index d1fadbc..0656e91 100644 --- a/csharp/src/HgResume.Api/ProcessRunner.cs +++ b/csharp/src/HgResume.Api/ProcessRunner.cs @@ -3,6 +3,13 @@ namespace HgResume.Api; +/// +/// Result of running an external command: stdout split into lines (PHP exec() style, trailing empty +/// line removed), the process exit code, and the captured stderr (raw). Callers that previously only +/// cared about stdout can still deconstruct the first one or two members. +/// +public readonly record struct ProcessResult(List Lines, int ExitCode, string StdErr); + /// /// Helpers for launching the external `hg` binary. The PHP original shelled out via exec()/`&` /// and GNU /usr/bin/time; here we use System.Diagnostics.Process directly. @@ -11,9 +18,10 @@ public static class ProcessRunner { /// /// Runs a command (mirrors PHP exec()): returns stdout split into lines with the trailing empty - /// line removed, plus the exit code. stderr is discarded (PHP exec captured stdout). + /// line removed, the exit code, and stderr. The PHP original discarded stderr and inferred failure + /// from empty stdout; callers now key on the exit code and can surface stderr on a real failure. /// - public static async Task<(List Lines, int ExitCode)> RunAsync(string workingDir, + public static async Task RunAsync(string workingDir, string program, string[] args, CancellationToken ct = default) { var psi = new ProcessStartInfo @@ -41,6 +49,6 @@ public static class ProcessRunner { lines.RemoveAt(lines.Count - 1); } - return (lines, proc.ExitCode); + return new ProcessResult(lines, proc.ExitCode, stderrTask.Result.Trim()); } } diff --git a/csharp/src/HgResume.Api/Program.cs b/csharp/src/HgResume.Api/Program.cs index 1875d8e..cd2b346 100644 --- a/csharp/src/HgResume.Api/Program.cs +++ b/csharp/src/HgResume.Api/Program.cs @@ -25,6 +25,9 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(sp => sp.GetRequiredService()); builder.Services.AddHostedService(sp => sp.GetRequiredService()); +// Reaps abandoned .bundle/.metadata/.async_run/.incoming cache artifacts a looping or vanished client +// would otherwise leak forever (prod left 1,258 .async_run locks). +builder.Services.AddHostedService(); builder.Services.AddExceptionHandler(); builder.Services.AddProblemDetails(); builder.Services.AddValidation(); diff --git a/csharp/src/HgResume.Api/RestDispatcher.cs b/csharp/src/HgResume.Api/RestDispatcher.cs index 79042c1..5c19b37 100644 --- a/csharp/src/HgResume.Api/RestDispatcher.cs +++ b/csharp/src/HgResume.Api/RestDispatcher.cs @@ -64,7 +64,11 @@ private Task DispatchAsync(string methodName, IQueryCollection ct); case "pullBundleChunk": - RequireParams(methodName, query, "repoId", "baseHashes", "offset", "chunkSize", "transId"); + // baseHashes is intentionally NOT required: a client that clears its revisioncache re-sends + // pullBundleChunk with no baseHashes. Rejecting that with FAIL (400) made the client — which + // retries all 400s — loop forever (the prod DDoS). BaseHashes(query) already yields [] when + // absent, which MakeBundle/IsValidBase treat as a full clone ("--all") that terminates. + RequireParams(methodName, query, "repoId", "offset", "chunkSize", "transId"); return _api.PullBundleChunkAsync( Str(query, "repoId"), BaseHashes(query), diff --git a/csharp/test/HgResume.Api.Tests/CacheGarbageCollectorTests.cs b/csharp/test/HgResume.Api.Tests/CacheGarbageCollectorTests.cs new file mode 100644 index 0000000..d2ebf38 --- /dev/null +++ b/csharp/test/HgResume.Api.Tests/CacheGarbageCollectorTests.cs @@ -0,0 +1,72 @@ +using HgResume.Api; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace HgResume.Api.Tests; + +public sealed class CacheGarbageCollectorTests : IDisposable +{ + private readonly string _cache = Path.Join(Path.GetTempPath(), "CacheGcTests-" + Guid.NewGuid().ToString("N")); + + public CacheGarbageCollectorTests() => Directory.CreateDirectory(_cache); + + public void Dispose() + { + if (Directory.Exists(_cache)) Directory.Delete(_cache, true); + } + + private CacheGarbageCollector MakeGc(TimeSpan ttl) + { + var config = new ApiConfig + { + CachePath = _cache, + RepoSearchPaths = [_cache], + MaintenanceFilePath = Path.Join(_cache, "maintenance_message.txt"), + MaxRequestBodySize = ApiConfig.DefaultMaxRequestBodySize, + ResetCleanupAgeDays = 31, + RequireManageSecret = false, + CacheTtl = ttl, + }; + return new CacheGarbageCollector(config, NullLogger.Instance); + } + + private string Touch(string name, TimeSpan age) + { + string path = Path.Join(_cache, name); + File.WriteAllText(path, "x"); + File.SetLastWriteTimeUtc(path, DateTime.UtcNow - age); + return path; + } + + [Fact] + public void Collect_ReapsStaleTransactionArtifacts() + { + var stale = new[] + { + Touch("abc.bundle", TimeSpan.FromHours(48)), + Touch("abc.metadata", TimeSpan.FromHours(48)), + Touch("abc.async_run", TimeSpan.FromHours(48)), + Touch("abc.bundle.incoming", TimeSpan.FromHours(48)), + }; + + int removed = MakeGc(TimeSpan.FromHours(24)).Collect(); + + Assert.Equal(stale.Length, removed); + Assert.All(stale, p => Assert.False(File.Exists(p))); + } + + [Fact] + public void Collect_LeavesFreshArtifactsAndUnrelatedFiles() + { + string fresh = Touch("fresh.async_run", TimeSpan.FromHours(1)); + string unrelated = Touch("maintenance_message.txt", TimeSpan.FromHours(48)); + string staleBundle = Touch("old.bundle", TimeSpan.FromHours(48)); + + int removed = MakeGc(TimeSpan.FromHours(24)).Collect(); + + Assert.Equal(1, removed); + Assert.True(File.Exists(fresh), "a lock within the TTL must be kept"); + Assert.True(File.Exists(unrelated), "non-transaction files must be left alone"); + Assert.False(File.Exists(staleBundle)); + } +} diff --git a/csharp/test/HgResume.IntegrationTests/PullFacts.cs b/csharp/test/HgResume.IntegrationTests/PullFacts.cs index a0cbd91..11716ab 100644 --- a/csharp/test/HgResume.IntegrationTests/PullFacts.cs +++ b/csharp/test/HgResume.IntegrationTests/PullFacts.cs @@ -207,6 +207,46 @@ public async Task PullBundleChunk_NonEmptyRepoMissingHashAcrossPages_FailsWithou Assert.Equal("FAIL", r.Status); } + [Fact] + public async Task PullBundleChunk_EmptyRepoFirstSyncNoBaseHashes_NoChangeThenPushSucceeds() + { + // The live prod bug: a first sync — or a client that cleared its revisioncache — sends + // pullBundleChunk with NO baseHashes at all. The dispatcher used to reject the absent param with + // FAIL (400), which the Chorus client retries forever, and MakeBundle([]) emitted an invalid hg + // command. The pull must now settle on NOCHANGE against the empty repo (so the client proceeds to + // push) without looping, and the subsequent push must apply cleanly — the whole first sync. + _fx.SeedRepo("empty-hg-repo.zip"); + string tx = nameof(PullBundleChunk_EmptyRepoFirstSyncNoBaseHashes_NoChangeThenPushSucceeds); + Api.FinishPullBundle(tx); + Api.FinishPushBundle(tx); + + // Guard the no-baseHashes pull with a timeout so a regression surfaces as a fast failure rather + // than hanging on the loop. + var call = Task.Run(() => Api.PullBundleChunk("empty-hg-repo", Array.Empty(), 0, 50, tx)); + var finished = await Task.WhenAny(call, Task.Delay(TimeSpan.FromSeconds(30))); + Assert.True(finished == call, + "pullBundleChunk with no baseHashes against an empty repo did not return within 30s — it is looping."); + Assert.Equal("NOCHANGE", (await call).Status); + + // The client now proceeds to push its whole history into the empty repo. + var push = Protocol.PushEntireBundle(Api, "empty-hg-repo", tx, _fx.Fixture("sample_entire.bundle")); + Assert.Equal("SUCCESS", push.Status); + } + + [Fact] + public void PullBundleChunk_NonEmptyRepoNoBaseHashes_ReturnsFullCloneBundle() + { + // Empty baseHashes against a NON-empty repo means "I have no common base" — the server must send + // a full clone (hg bundle --all), identical to passing baseHash "0". Verify the assembled bundle + // matches the full-repo fixture rather than FAILing or looping. + _fx.SeedRepo("sample-hg-repo2.zip"); + string tx = nameof(PullBundleChunk_NonEmptyRepoNoBaseHashes_ReturnsFullCloneBundle); + Api.FinishPullBundle(tx); + var (assembled, last) = Protocol.PullEntireBundle(Api, "sample-hg-repo2", Array.Empty(), tx); + Assert.Equal("SUCCESS", last.Status); + Assert.Equal(_fx.Fixture("sample_entire.bundle"), assembled); + } + [Fact] public void PullBundleChunk_LongMakeBundle_InProgressCode() {