From 1f951f51709378f30ef25e9ccc7ad5fe46287e64 Mon Sep 17 00:00:00 2001
From: Brandon Corbett <Bccorb@users.noreply.github.com>
Date: Tue, 6 Oct 2026 23:44:59 -0400
Subject: [PATCH] chore: correct the empty MAX_CONCURRENT_SESSIONS comment in
 parseEnvConfigs

The comment on the max_concurrent_sessions case said an empty value means
no limit. bootstrapSystemConfig skips empty env values before parsing, so
MAX_CONCURRENT_SESSIONS='' is treated as unset: an existing cap is kept,
and with no row the default applies. Only a whitespace-only value reaches
the parser's empty check, where it is empty after trimming.

The comment now describes that. Runtime behavior is unchanged, and the
empty check stays because it still serves whitespace-only values.

Add a bootstrap test, using the real parser and defaults, that pins the
empty value as unset (existing cap kept, default used when no row) and
the whitespace-only value as no limit.

Closes #366
---
 src/utils/parseEnvConfigs.ts                  | 12 ++-
 .../bootstrapMaxConcurrentSessions.spec.ts    | 78 +++++++++++++++++++
 2 files changed, 86 insertions(+), 4 deletions(-)
 create mode 100644 tests/unit/config/bootstrapMaxConcurrentSessions.spec.ts

diff --git a/src/utils/parseEnvConfigs.ts b/src/utils/parseEnvConfigs.ts
index e8863f0..e26daf2 100644
--- a/src/utils/parseEnvConfigs.ts
+++ b/src/utils/parseEnvConfigs.ts
@@ -33,10 +33,14 @@ export function parseSystemConfigEnvValue(key: keyof typeof SYSTEM_CONFIG_ENV_MA
     case 'delay_after':
       return Number(raw);
 
-    // An empty value, or one of the words an operator is likely to reach for,
-    // means no limit. Without this the only way to express "uncapped" through the
-    // environment would be to unset the variable, which a deployment template
-    // cannot easily do.
+    // One of the words an operator is likely to reach for means no limit. Without
+    // this the only way to express "uncapped" through the environment would be to
+    // unset the variable, which a deployment template cannot easily do.
+    //
+    // A truly empty value never gets here: bootstrapSystemConfig skips empty env
+    // values, so they count as unset (an existing cap is kept, otherwise the
+    // default applies). The empty check below only catches a whitespace-only
+    // value, which is empty once trimmed.
     case 'max_concurrent_sessions': {
       const value = raw.trim().toLowerCase();
 
diff --git a/tests/unit/config/bootstrapMaxConcurrentSessions.spec.ts b/tests/unit/config/bootstrapMaxConcurrentSessions.spec.ts
new file mode 100644
index 0000000..2b67560
--- /dev/null
+++ b/tests/unit/config/bootstrapMaxConcurrentSessions.spec.ts
@@ -0,0 +1,78 @@
+import { vi } from 'vitest';
+
+vi.mock('../../../src/models/systemConfig', () => ({
+  SystemConfig: {
+    findByPk: vi.fn(),
+    create: vi.fn(),
+  },
+}));
+
+vi.mock('../../../src/config/systemConfig.envMap', () => ({
+  SYSTEM_CONFIG_ENV_MAP: {
+    max_concurrent_sessions: 'MAX_CONCURRENT_SESSIONS',
+  },
+}));
+
+vi.mock('../../../src/schemas/systemConfig.schema', () => ({
+  SystemConfigSchema: {
+    safeParse: vi.fn(),
+  },
+}));
+
+import { beforeEach, describe, expect, it } from 'vitest';
+
+// The real parser and defaults are used here. Bootstrap skips an empty env value
+// before parsing, so MAX_CONCURRENT_SESSIONS='' behaves as unset rather than as
+// "no limit". Only a whitespace-only value reaches the parser's empty check.
+describe('bootstrapSystemConfig with MAX_CONCURRENT_SESSIONS', () => {
+  beforeEach(() => {
+    vi.resetModules();
+    vi.clearAllMocks();
+    delete process.env.MAX_CONCURRENT_SESSIONS;
+  });
+
+  async function load() {
+    const { SystemConfig } = await import('../../../src/models/systemConfig');
+    const { SystemConfigSchema } = await import('../../../src/schemas/systemConfig.schema');
+    (SystemConfigSchema.safeParse as any).mockReturnValue({ success: true, data: {} });
+    const { bootstrapSystemConfig } = await import('../../../src/config/bootstrapSystemConfig');
+    return { SystemConfig, SystemConfigSchema, bootstrapSystemConfig };
+  }
+
+  it('keeps an existing cap when the value is empty', async () => {
+    const { SystemConfig, SystemConfigSchema, bootstrapSystemConfig } = await load();
+    const row = { value: 3, updatedBy: null, update: vi.fn(), destroy: vi.fn() };
+    (SystemConfig.findByPk as any).mockResolvedValue(row);
+    process.env.MAX_CONCURRENT_SESSIONS = '';
+
+    await bootstrapSystemConfig();
+
+    expect(row.update).not.toHaveBeenCalled();
+    expect(row.destroy).not.toHaveBeenCalled();
+    expect(SystemConfigSchema.safeParse).toHaveBeenCalledWith({ max_concurrent_sessions: 3 });
+  });
+
+  it('uses the default when the value is empty and no row exists', async () => {
+    const { SystemConfig, SystemConfigSchema, bootstrapSystemConfig } = await load();
+    (SystemConfig.findByPk as any).mockResolvedValue(null);
+    process.env.MAX_CONCURRENT_SESSIONS = '';
+
+    await bootstrapSystemConfig();
+
+    expect(SystemConfig.create).not.toHaveBeenCalled();
+    expect(SystemConfigSchema.safeParse).toHaveBeenCalledWith({ max_concurrent_sessions: null });
+  });
+
+  it('clears an existing cap when the value is whitespace only', async () => {
+    const { SystemConfig, SystemConfigSchema, bootstrapSystemConfig } = await load();
+    const row = { value: 3, updatedBy: null, update: vi.fn(), destroy: vi.fn() };
+    (SystemConfig.findByPk as any).mockResolvedValue(row);
+    process.env.MAX_CONCURRENT_SESSIONS = '   ';
+
+    await bootstrapSystemConfig();
+
+    expect(row.destroy).toHaveBeenCalled();
+    expect(row.update).not.toHaveBeenCalled();
+    expect(SystemConfigSchema.safeParse).toHaveBeenCalledWith({ max_concurrent_sessions: null });
+  });
+});