From ddd071f7e5fd205fd013d7801c09781737704cdf Mon Sep 17 00:00:00 2001
From: ARUNAVO RAY <developer@arunavoray.dev>
Date: Wed, 18 Mar 2026 15:05:00 +0530
Subject: [PATCH] fix: prevent excessive disk usage from repo backups (#235)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix: prevent excessive disk usage from repo backups (#234)

Legacy configs with backupBeforeSync: true but no explicit backupStrategy
silently resolved to "always", creating full git bundles on every sync
cycle. This caused repo-backups to grow to 17GB+ for users with many
repositories.

Changes:
- Fix resolveBackupStrategy to map backupBeforeSync: true → "on-force-push"
  instead of "always", so legacy configs only backup when force-push is detected
- Fix config mapper to always set backupStrategy explicitly ("on-force-push")
  preventing the backward-compat fallback from triggering
- Lower default backupRetentionCount from 20 to 5 bundles per repo
- Add time-based retention (backupRetentionDays, default 30 days) alongside
  count-based retention, with safety net to always keep at least 1 bundle
- Add "high disk usage" warning on "Always Backup" UI option
- Update docs and tests to reflect new defaults and behavior

* fix: preserve legacy backupBeforeSync:false on UI round-trip and expose retention days

P1: mapDbToUiConfig now checks backupBeforeSync === false before
defaulting backupStrategy, preventing legacy "disabled" configs from
silently becoming "on-force-push" after any auto-save round-trip.

P3: Added "Snapshot retention days" input field to the backup settings
UI, matching the documented setting in FORCE_PUSH_PROTECTION.md.
---
 docs/FORCE_PUSH_PROTECTION.md              |  8 +++-
 src/components/config/ConfigTabs.tsx       |  3 +-
 src/components/config/GitHubConfigForm.tsx | 30 +++++++++++--
 src/lib/db/schema.ts                       |  3 +-
 src/lib/gitea-enhanced.test.ts             |  2 +-
 src/lib/repo-backup.test.ts                |  4 +-
 src/lib/repo-backup.ts                     | 51 +++++++++++++++++++---
 src/lib/utils/config-defaults.ts           |  3 +-
 src/lib/utils/config-mapper.ts             | 12 +++--
 src/types/config.ts                        |  1 +
 10 files changed, 94 insertions(+), 23 deletions(-)

diff --git a/docs/FORCE_PUSH_PROTECTION.md b/docs/FORCE_PUSH_PROTECTION.md
index f15b1d2..b1e75d5 100644
--- a/docs/FORCE_PUSH_PROTECTION.md
+++ b/docs/FORCE_PUSH_PROTECTION.md
@@ -78,7 +78,11 @@ These appear when any non-disabled strategy is selected:
 
 ### Snapshot Retention Count
 
-How many backup snapshots to keep per repository. Oldest snapshots are deleted when this limit is exceeded. Default: **20**.
+How many backup snapshots to keep per repository. Oldest snapshots are deleted when this limit is exceeded. Default: **5**.
+
+### Snapshot Retention Days
+
+Maximum age (in days) for backup snapshots. Bundles older than this are deleted during retention enforcement, though at least one bundle is always kept. Set to `0` to disable time-based retention. Default: **30**.
 
 ### Snapshot Directory
 
@@ -96,7 +100,7 @@ The old `backupBeforeSync` boolean is still recognized:
 
 | Old Setting | New Equivalent |
 |---|---|
-| `backupBeforeSync: true` | `backupStrategy: "always"` |
+| `backupBeforeSync: true` | `backupStrategy: "on-force-push"` |
 | `backupBeforeSync: false` | `backupStrategy: "disabled"` |
 | Neither set | `backupStrategy: "on-force-push"` (new default) |
 
diff --git a/src/components/config/ConfigTabs.tsx b/src/components/config/ConfigTabs.tsx
index 5fb7af9..9eb9877 100644
--- a/src/components/config/ConfigTabs.tsx
+++ b/src/components/config/ConfigTabs.tsx
@@ -51,7 +51,8 @@ export function ConfigTabs() {
       starredReposMode: 'dedicated-org',
       preserveOrgStructure: false,
       backupStrategy: "on-force-push",
-      backupRetentionCount: 20,
+      backupRetentionCount: 5,
+      backupRetentionDays: 30,
       backupDirectory: 'data/repo-backups',
       blockSyncOnBackupFailure: true,
     },
diff --git a/src/components/config/GitHubConfigForm.tsx b/src/components/config/GitHubConfigForm.tsx
index c0cb441..d24bc4f 100644
--- a/src/components/config/GitHubConfigForm.tsx
+++ b/src/components/config/GitHubConfigForm.tsx
@@ -234,7 +234,7 @@ export function GitHubConfigForm({
                   {
                     value: "always",
                     label: "Always Backup",
-                    desc: "Snapshot before every sync",
+                    desc: "Snapshot before every sync (high disk usage)",
                   },
                   {
                     value: "on-force-push",
@@ -272,7 +272,7 @@ export function GitHubConfigForm({
 
               {(giteaConfig.backupStrategy ?? "on-force-push") !== "disabled" && (
                 <>
-                  <div className="grid grid-cols-1 md:grid-cols-2 gap-4">
+                  <div className="grid grid-cols-1 md:grid-cols-3 gap-4">
                     <div>
                       <label htmlFor="backup-retention" className="block text-sm font-medium mb-1.5">
                         Snapshot retention count
@@ -282,11 +282,11 @@ export function GitHubConfigForm({
                         name="backupRetentionCount"
                         type="number"
                         min={1}
-                        value={giteaConfig.backupRetentionCount ?? 20}
+                        value={giteaConfig.backupRetentionCount ?? 5}
                         onChange={(e) => {
                           const newConfig = {
                             ...giteaConfig,
-                            backupRetentionCount: Math.max(1, Number.parseInt(e.target.value, 10) || 20),
+                            backupRetentionCount: Math.max(1, Number.parseInt(e.target.value, 10) || 5),
                           };
                           setGiteaConfig(newConfig);
                           if (onGiteaAutoSave) onGiteaAutoSave(newConfig);
@@ -294,6 +294,28 @@ export function GitHubConfigForm({
                         className="w-full rounded-md border border-input bg-background px-3 py-2 text-sm shadow-sm transition-colors placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring"
                       />
                     </div>
+                    <div>
+                      <label htmlFor="backup-retention-days" className="block text-sm font-medium mb-1.5">
+                        Snapshot retention days
+                      </label>
+                      <input
+                        id="backup-retention-days"
+                        name="backupRetentionDays"
+                        type="number"
+                        min={0}
+                        value={giteaConfig.backupRetentionDays ?? 30}
+                        onChange={(e) => {
+                          const newConfig = {
+                            ...giteaConfig,
+                            backupRetentionDays: Math.max(0, Number.parseInt(e.target.value, 10) || 0),
+                          };
+                          setGiteaConfig(newConfig);
+                          if (onGiteaAutoSave) onGiteaAutoSave(newConfig);
+                        }}
+                        className="w-full rounded-md border border-input bg-background px-3 py-2 text-sm shadow-sm transition-colors placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring"
+                      />
+                      <p className="text-xs text-muted-foreground mt-1">0 = no time-based limit</p>
+                    </div>
                     <div>
                       <label htmlFor="backup-directory" className="block text-sm font-medium mb-1.5">
                         Snapshot directory
diff --git a/src/lib/db/schema.ts b/src/lib/db/schema.ts
index e2721ba..fd64254 100644
--- a/src/lib/db/schema.ts
+++ b/src/lib/db/schema.ts
@@ -75,7 +75,8 @@ export const giteaConfigSchema = z.object({
   mirrorMilestones: z.boolean().default(false),
   backupStrategy: backupStrategyEnum.default("on-force-push"),
   backupBeforeSync: z.boolean().default(true), // Deprecated: kept for backward compat, use backupStrategy
-  backupRetentionCount: z.number().int().min(1).default(20),
+  backupRetentionCount: z.number().int().min(1).default(5),
+  backupRetentionDays: z.number().int().min(0).default(30),
   backupDirectory: z.string().optional(),
   blockSyncOnBackupFailure: z.boolean().default(true),
 });
diff --git a/src/lib/gitea-enhanced.test.ts b/src/lib/gitea-enhanced.test.ts
index 2173778..c837cdd 100644
--- a/src/lib/gitea-enhanced.test.ts
+++ b/src/lib/gitea-enhanced.test.ts
@@ -575,7 +575,7 @@ describe("Enhanced Gitea Operations", () => {
           token: "encrypted-token",
           defaultOwner: "testuser",
           mirrorReleases: false,
-          backupBeforeSync: true,
+          backupStrategy: "always",
           blockSyncOnBackupFailure: true,
         },
       };
diff --git a/src/lib/repo-backup.test.ts b/src/lib/repo-backup.test.ts
index d491bd6..99c19dc 100644
--- a/src/lib/repo-backup.test.ts
+++ b/src/lib/repo-backup.test.ts
@@ -162,8 +162,8 @@ describe("resolveBackupStrategy", () => {
     expect(resolveBackupStrategy(makeConfig({ backupStrategy: "block-on-force-push" }))).toBe("block-on-force-push");
   });
 
-  test("maps backupBeforeSync: true → 'always' (backward compat)", () => {
-    expect(resolveBackupStrategy(makeConfig({ backupBeforeSync: true }))).toBe("always");
+  test("maps backupBeforeSync: true → 'on-force-push' (backward compat, prevents silent always-backup)", () => {
+    expect(resolveBackupStrategy(makeConfig({ backupBeforeSync: true }))).toBe("on-force-push");
   });
 
   test("maps backupBeforeSync: false → 'disabled' (backward compat)", () => {
diff --git a/src/lib/repo-backup.ts b/src/lib/repo-backup.ts
index 0fa4463..36b42ef 100644
--- a/src/lib/repo-backup.ts
+++ b/src/lib/repo-backup.ts
@@ -65,13 +65,17 @@ async function runGit(args: string[], tokenToMask: string): Promise<void> {
   }
 }
 
-async function enforceRetention(repoBackupDir: string, keepCount: number): Promise<void> {
+async function enforceRetention(
+  repoBackupDir: string,
+  keepCount: number,
+  retentionDays: number = 0,
+): Promise<void> {
   const entries = await readdir(repoBackupDir);
   const bundleFiles = entries
     .filter((name) => name.endsWith(".bundle"))
     .map((name) => path.join(repoBackupDir, name));
 
-  if (bundleFiles.length <= keepCount) return;
+  if (bundleFiles.length === 0) return;
 
   const filesWithMtime = await Promise.all(
     bundleFiles.map(async (filePath) => ({
@@ -81,9 +85,33 @@ async function enforceRetention(repoBackupDir: string, keepCount: number): Promi
   );
 
   filesWithMtime.sort((a, b) => b.mtimeMs - a.mtimeMs);
-  const toDelete = filesWithMtime.slice(keepCount);
 
-  await Promise.all(toDelete.map((entry) => rm(entry.filePath, { force: true })));
+  const toDelete = new Set<string>();
+
+  // Count-based retention: keep only the N most recent
+  if (filesWithMtime.length > keepCount) {
+    for (const entry of filesWithMtime.slice(keepCount)) {
+      toDelete.add(entry.filePath);
+    }
+  }
+
+  // Time-based retention: delete bundles older than retentionDays
+  if (retentionDays > 0) {
+    const cutoffMs = Date.now() - retentionDays * 86_400_000;
+    for (const entry of filesWithMtime) {
+      if (entry.mtimeMs < cutoffMs) {
+        toDelete.add(entry.filePath);
+      }
+    }
+    // Always keep at least 1 bundle even if it's old
+    if (toDelete.size === filesWithMtime.length && filesWithMtime.length > 0) {
+      toDelete.delete(filesWithMtime[0].filePath);
+    }
+  }
+
+  if (toDelete.size > 0) {
+    await Promise.all([...toDelete].map((fp) => rm(fp, { force: true })));
+  }
 }
 
 export function isPreSyncBackupEnabled(): boolean {
@@ -126,9 +154,12 @@ export function resolveBackupStrategy(config: Partial<Config>): BackupStrategy {
   }
 
   // 2. Legacy backupBeforeSync boolean → map to strategy
+  //    Note: backupBeforeSync: true now maps to "on-force-push" (not "always")
+  //    because mappers default backupBeforeSync to true, causing every legacy config
+  //    to silently resolve to "always" and create full git bundles on every sync.
   const legacy = config.giteaConfig?.backupBeforeSync;
   if (legacy !== undefined) {
-    return legacy ? "always" : "disabled";
+    return legacy ? "on-force-push" : "disabled";
   }
 
   // 3. Env var (new)
@@ -251,7 +282,13 @@ export async function createPreSyncBundleBackup({
     1,
     Number.isFinite(config.giteaConfig?.backupRetentionCount)
       ? Number(config.giteaConfig?.backupRetentionCount)
-      : parsePositiveInt(process.env.PRE_SYNC_BACKUP_KEEP_COUNT, 20)
+      : parsePositiveInt(process.env.PRE_SYNC_BACKUP_KEEP_COUNT, 5)
+  );
+  const retentionDays = Math.max(
+    0,
+    Number.isFinite(config.giteaConfig?.backupRetentionDays)
+      ? Number(config.giteaConfig?.backupRetentionDays)
+      : parsePositiveInt(process.env.PRE_SYNC_BACKUP_RETENTION_DAYS, 30)
   );
 
   await mkdir(repoBackupDir, { recursive: true });
@@ -268,7 +305,7 @@ export async function createPreSyncBundleBackup({
     await runGit(["clone", "--mirror", authCloneUrl, mirrorClonePath], giteaToken);
     await runGit(["-C", mirrorClonePath, "bundle", "create", bundlePath, "--all"], giteaToken);
 
-    await enforceRetention(repoBackupDir, retention);
+    await enforceRetention(repoBackupDir, retention, retentionDays);
     return { bundlePath };
   } finally {
     await rm(tmpDir, { recursive: true, force: true });
diff --git a/src/lib/utils/config-defaults.ts b/src/lib/utils/config-defaults.ts
index 9c95173..be05c06 100644
--- a/src/lib/utils/config-defaults.ts
+++ b/src/lib/utils/config-defaults.ts
@@ -95,7 +95,8 @@ export async function createDefaultConfig({ userId, envOverrides = {} }: Default
       pullRequestConcurrency: 5,
       backupStrategy: "on-force-push",
       backupBeforeSync: true, // Deprecated: kept for backward compat
-      backupRetentionCount: 20,
+      backupRetentionCount: 5,
+      backupRetentionDays: 30,
       backupDirectory: "data/repo-backups",
       blockSyncOnBackupFailure: true,
     },
diff --git a/src/lib/utils/config-mapper.ts b/src/lib/utils/config-mapper.ts
index 8a91086..812bb1f 100644
--- a/src/lib/utils/config-mapper.ts
+++ b/src/lib/utils/config-mapper.ts
@@ -101,9 +101,10 @@ export function mapUiToDbConfig(
     mirrorPullRequests: mirrorOptions.mirrorMetadata && mirrorOptions.metadataComponents.pullRequests,
     mirrorLabels: mirrorOptions.mirrorMetadata && mirrorOptions.metadataComponents.labels,
     mirrorMilestones: mirrorOptions.mirrorMetadata && mirrorOptions.metadataComponents.milestones,
-    backupStrategy: giteaConfig.backupStrategy,
+    backupStrategy: giteaConfig.backupStrategy || "on-force-push",
     backupBeforeSync: giteaConfig.backupBeforeSync ?? true,
-    backupRetentionCount: giteaConfig.backupRetentionCount ?? 20,
+    backupRetentionCount: giteaConfig.backupRetentionCount ?? 5,
+    backupRetentionDays: giteaConfig.backupRetentionDays ?? 30,
     backupDirectory: giteaConfig.backupDirectory?.trim() || undefined,
     blockSyncOnBackupFailure: giteaConfig.blockSyncOnBackupFailure ?? true,
   };
@@ -146,9 +147,12 @@ export function mapDbToUiConfig(dbConfig: any): {
     personalReposOrg: undefined, // Not stored in current schema
     issueConcurrency: dbConfig.giteaConfig?.issueConcurrency ?? 3,
     pullRequestConcurrency: dbConfig.giteaConfig?.pullRequestConcurrency ?? 5,
-    backupStrategy: dbConfig.giteaConfig?.backupStrategy || undefined,
+    backupStrategy: dbConfig.giteaConfig?.backupStrategy ||
+      // Respect legacy backupBeforeSync: false → "disabled" mapping on round-trip
+      (dbConfig.giteaConfig?.backupBeforeSync === false ? "disabled" : "on-force-push"),
     backupBeforeSync: dbConfig.giteaConfig?.backupBeforeSync ?? true,
-    backupRetentionCount: dbConfig.giteaConfig?.backupRetentionCount ?? 20,
+    backupRetentionCount: dbConfig.giteaConfig?.backupRetentionCount ?? 5,
+    backupRetentionDays: dbConfig.giteaConfig?.backupRetentionDays ?? 30,
     backupDirectory: dbConfig.giteaConfig?.backupDirectory || "data/repo-backups",
     blockSyncOnBackupFailure: dbConfig.giteaConfig?.blockSyncOnBackupFailure ?? true,
   };
diff --git a/src/types/config.ts b/src/types/config.ts
index 534bf60..260b311 100644
--- a/src/types/config.ts
+++ b/src/types/config.ts
@@ -22,6 +22,7 @@ export interface GiteaConfig {
   backupStrategy?: BackupStrategy;
   backupBeforeSync?: boolean; // Deprecated: kept for backward compat, use backupStrategy
   backupRetentionCount?: number;
+  backupRetentionDays?: number;
   backupDirectory?: string;
   blockSyncOnBackupFailure?: boolean;
 }