chore(observability): restore behavior guards and tidy placement

Audit cleanups on the diff itself, no functional change.

The revert took two tests with it that were unrelated to what it reverted:
that describeRedisConnection never throws, since it runs inside catch blocks
where a throw would replace the real failure, and that the wrapper rethrows the
original error object rather than substituting its own. Both are the properties
that make this instrumentation rather than a behavior change, so both are back.

The diagnostics helper had also been inserted between reserveExecutionSlot's
TSDoc and the function, orphaning that doc onto the helper. Moved above it.

In-body block comments converted to line comments, matching how the rest of
this file annotates statements.

Author: waleedlatif1

apps/sim/lib/billing/calculations/usage-reservation.test.ts

@@ -339,6 +339,13 @@ describe('usage-reservation', () => {
   })
 
   describe('refreshExecutionSlotExpiry', () => {
+    it('rethrows the original error object rather than the diagnostic wrapper', async () => {
+      const original = Object.assign(new Error('Command timed out'), { code: 'ETIMEDOUT' })
+      getMock.mockRejectedValueOnce(original)
+
+      await expect(refreshExecutionSlotExpiry('exec-1', Date.now() + 60_000)).rejects.toBe(original)
+    })
+
     it('refreshes only the locally owned slot and matching pointer', async () => {
       evalMock.mockResolvedValueOnce(1).mockResolvedValueOnce(1)
       await reserveExecutionSlot(memberParams)

apps/sim/lib/billing/calculations/usage-reservation.ts

@@ -425,14 +425,6 @@ export type ReserveExecutionSlotResult =
       reason: ReservationDenialReason
     }
 
-/**
- * Atomic admission reservation that closes the usage-cap check-then-use race.
- *
- * Billing-disabled and self-hosted deployments are no-ops. Hosted deployments
- * require Redis and fail closed when reservation or pointer ownership cannot be
- * proven. A newly-created local reservation is rolled back if pointer
- * registration fails; TTL is only the bounded crash fallback.
- */
 /**
  * Records connection state alongside a failed slot operation.
  *
@@ -461,6 +453,14 @@ async function withReservationDiagnostics<T>(
   }
 }
 
+/**
+ * Atomic admission reservation that closes the usage-cap check-then-use race.
+ *
+ * Billing-disabled and self-hosted deployments are no-ops. Hosted deployments
+ * require Redis and fail closed when reservation or pointer ownership cannot be
+ * proven. A newly-created local reservation is rolled back if pointer
+ * registration fails; TTL is only the bounded crash fallback.
+ */
 export async function reserveExecutionSlot(
   params: ReserveExecutionSlotParams
 ): Promise<ReserveExecutionSlotResult> {

apps/sim/lib/core/config/redis.test.ts

@@ -225,6 +225,22 @@ describe('redis config', () => {
       expect(JSON.stringify(d)).not.toContain('10.0.0.5')
     })
 
+    it('never throws, so it cannot mask the error it is describing', () => {
+      // Called from catch blocks: a throw here would replace the real failure.
+      mockEnv.REDIS_URL = undefined
+      expect(() => describeRedisConnection()).not.toThrow()
+
+      mockEnv.REDIS_URL = 'not a url'
+      expect(() => describeRedisConnection()).not.toThrow()
+      expect(describeRedisConnection().hostKind).toBe('unknown')
+
+      // rediss:// to a bare IP with no REDIS_TLS_SERVERNAME makes the URL
+      // resolution throw; the snapshot must still come back.
+      mockEnv.REDIS_URL = 'rediss://10.0.0.5:6379'
+      mockEnv.REDIS_TLS_SERVERNAME = undefined
+      expect(() => describeRedisConnection()).not.toThrow()
+    })
+
     it('does not date a connection that has been discarded', async () => {
       mockRedisInstance.status = 'ready'
       getRedisClient()

apps/sim/lib/core/config/redis.ts

@@ -137,12 +137,10 @@ function describeRedisUrl(
     // WHATWG keeps IPv6 literals bracketed in `hostname`; `isIP` wants them bare.
     const host = parsed.hostname.replace(/^\[|\]$/g, '')
     const tls = parsed.protocol === 'rediss:'
-    /**
-     * `sniOverride` deliberately mirrors `resolveRedisTlsOptions`, which tests
-     * for IPv4 only. So an IPv6 literal over TLS reports `hostKind: 'ip'` with
-     * `sniOverride: false` — not a contradiction but the useful reading, since
-     * that combination is a connection whose certificate cannot verify.
-     */
+    // `sniOverride` deliberately mirrors `resolveRedisTlsOptions`, which tests for
+    // IPv4 only. So an IPv6 literal over TLS reports `hostKind: 'ip'` with
+    // `sniOverride: false` — not a contradiction but the useful reading, since that
+    // combination is a connection whose certificate cannot verify.
     return {
       hostKind: isIP(host) === 0 ? 'dns' : 'ip',
       tls,
@@ -172,13 +170,10 @@ export function describeRedisConnection(): RedisConnectionDiagnostics {
   // double-cast-allowed: ioredis omits offlineQueue from its public type, and its depth is what separates a command waiting on connection setup from one written to a live socket
   const queued = (client as unknown as OfflineQueueView | null)?.offlineQueue?.length
 
-  /**
-   * Ages describe the client that is currently held. A discarded client leaves
-   * its timestamps behind until the next `getRedisClient()` rebuilds them, and
-   * reporting those against `no-client` would date a connection that no longer
-   * exists — precisely the wrong answer for the investigation this exists to
-   * support. The counters below are deliberately cumulative for the process.
-   */
+  // Ages describe the client currently held. A discarded client leaves its
+  // timestamps behind until the next `getRedisClient()` rebuilds them, and
+  // reporting those against `no-client` would date a connection that no longer
+  // exists. The counters below are deliberately cumulative for the process.
   const ageOf = (at: number | null) => (client === null || at === null ? null : now - at)
 
   return {