Skip to content

Commit 8f51640

Browse files
committed
fix(db): resolve sealing env on the migrator like the app and defer only on batch timeouts
1 parent 9703f13 commit 8f51640

7 files changed

Lines changed: 194 additions & 59 deletions

File tree

‎helm/sim/templates/_helpers.tpl‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,3 +683,50 @@ Validate Copilot configuration
683683
{{- end -}}
684684
{{- end -}}
685685
{{- end }}
686+
687+
{{/*
688+
Inline env entries the app container applies over its Secret, in precedence order (a later entry
689+
for the same name wins): envDefaults the operator did not override, existingSecret-mode app.env
690+
values, then extraEnvVars. Shared with the migrations init container so both resolve a key alike.
691+
*/}}
692+
{{- define "sim.appEnvOverrides" -}}
693+
{{- /*
694+
Skip envDefaults keys that the user has explicitly overridden in app.env
695+
with a non-empty value. K8s `env` takes precedence over `envFrom`, so an
696+
inline default would otherwise mask the user's Secret-bound value.
697+
Also skip envDefaults entirely in existingSecret mode: the pre-existing
698+
Secret is the source of truth, and inlining localhost URL defaults would
699+
silently shadow keys (NEXT_PUBLIC_APP_URL, BETTER_AUTH_URL, etc.) the
700+
user stored in their Secret but left empty in app.env.
701+
*/}}
702+
{{- $appEnv := .Values.app.env | default dict }}
703+
{{- $useExistingSecret := and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }}
704+
{{- if not $useExistingSecret }}
705+
{{- range $key, $value := .Values.app.envDefaults | default dict }}
706+
{{- $override := index $appEnv $key }}
707+
{{- if and (ne (toString $value) "") (ne (toString $value) "<nil>") (or (not $override) (eq (toString $override) "") (eq (toString $override) "<nil>")) }}
708+
- name: {{ $key }}
709+
value: {{ $value | quote }}
710+
{{- end }}
711+
{{- end }}
712+
{{- end }}
713+
{{- /*
714+
In existingSecret mode the chart-managed Secret is not rendered, so
715+
non-empty values in app.env would otherwise go nowhere — inline them
716+
here so the user's overrides actually reach the pod. Skipped in ESO
717+
mode (ESO is the source of truth; inlining would mask the synced value)
718+
and in inline mode (values flow through the chart-managed Secret).
719+
*/}}
720+
{{- if and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }}
721+
{{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" }}
722+
{{- range $key, $value := $appEnv }}
723+
{{- if and (ne (toString $value) "") (ne (toString $value) "<nil>") (not (has $key $chartComputed)) }}
724+
- name: {{ $key }}
725+
value: {{ $value | quote }}
726+
{{- end }}
727+
{{- end }}
728+
{{- end }}
729+
{{- with .Values.extraEnvVars }}
730+
{{ toYaml . }}
731+
{{- end }}
732+
{{- end }}

‎helm/sim/templates/deployment-app.yaml‎

Lines changed: 15 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -59,17 +59,23 @@ spec:
5959
bun run db:migrate
6060
{{- /*
6161
The account token backfill script migration seals with the app's key and honors its
62-
ENCRYPT_ACCOUNT_TOKENS opt-out, read the way the app container reads them; without a
63-
usable key it does nothing and stays pending.
62+
ENCRYPT_ACCOUNT_TOKENS opt-out. Each resolves as it does on the app container: the last
63+
matching entry of the app's inline overrides (envDefaults, existingSecret inlining,
64+
extraEnvVars), else the app Secret. Without a usable key the step stays pending.
6465
*/}}
6566
env:
66-
{{- $useExistingSecret := and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }}
67+
{{- $appOverrides := include "sim.appEnvOverrides" . | fromYamlArray }}
6768
{{- range $key := list "ENCRYPTION_KEY" "ENCRYPT_ACCOUNT_TOKENS" }}
68-
{{- $inline := index ($.Values.app.env | default dict) $key }}
69-
- name: {{ $key }}
70-
{{- if and $useExistingSecret (ne (toString $inline) "") (ne (toString $inline) "<nil>") }}
71-
value: {{ $inline | quote }}
69+
{{- $override := dict }}
70+
{{- range $appOverrides }}
71+
{{- if eq (toString .name) $key }}
72+
{{- $override = . }}
73+
{{- end }}
74+
{{- end }}
75+
{{- if $override }}
76+
- {{ toYaml $override | nindent 14 | trim }}
7277
{{- else }}
78+
- name: {{ $key }}
7379
valueFrom:
7480
secretKeyRef:
7581
name: {{ include "sim.appSecretName" $ }}
@@ -111,42 +117,6 @@ spec:
111117
value: {{ include "sim.ollamaUrl" . | quote }}
112118
- name: PII_URL
113119
value: {{ include "sim.piiUrl" . | quote }}
114-
{{- /*
115-
Skip envDefaults keys that the user has explicitly overridden in app.env
116-
with a non-empty value. K8s `env` takes precedence over `envFrom`, so an
117-
inline default would otherwise mask the user's Secret-bound value.
118-
Also skip envDefaults entirely in existingSecret mode: the pre-existing
119-
Secret is the source of truth, and inlining localhost URL defaults would
120-
silently shadow keys (NEXT_PUBLIC_APP_URL, BETTER_AUTH_URL, etc.) the
121-
user stored in their Secret but left empty in app.env.
122-
*/}}
123-
{{- $appEnv := .Values.app.env | default dict }}
124-
{{- $useExistingSecret := and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }}
125-
{{- if not $useExistingSecret }}
126-
{{- range $key, $value := .Values.app.envDefaults | default dict }}
127-
{{- $override := index $appEnv $key }}
128-
{{- if and (ne (toString $value) "") (ne (toString $value) "<nil>") (or (not $override) (eq (toString $override) "") (eq (toString $override) "<nil>")) }}
129-
- name: {{ $key }}
130-
value: {{ $value | quote }}
131-
{{- end }}
132-
{{- end }}
133-
{{- end }}
134-
{{- /*
135-
In existingSecret mode the chart-managed Secret is not rendered, so
136-
non-empty values in app.env would otherwise go nowhere — inline them
137-
here so the user's overrides actually reach the pod. Skipped in ESO
138-
mode (ESO is the source of truth; inlining would mask the synced value)
139-
and in inline mode (values flow through the chart-managed Secret).
140-
*/}}
141-
{{- if and .Values.app.secrets.existingSecret.enabled (not .Values.externalSecrets.enabled) }}
142-
{{- $chartComputed := list "DATABASE_URL" "SOCKET_SERVER_URL" "OLLAMA_URL" "PII_URL" }}
143-
{{- range $key, $value := $appEnv }}
144-
{{- if and (ne (toString $value) "") (ne (toString $value) "<nil>") (not (has $key $chartComputed)) }}
145-
- name: {{ $key }}
146-
value: {{ $value | quote }}
147-
{{- end }}
148-
{{- end }}
149-
{{- end }}
150120
{{- if .Values.telemetry.enabled }}
151121
{{- $nodeEnv := default (default "production" (index (.Values.app.envDefaults | default dict) "NODE_ENV")) (index (.Values.app.env | default dict) "NODE_ENV") }}
152122
# OpenTelemetry configuration
@@ -159,9 +129,8 @@ spec:
159129
- name: OTEL_RESOURCE_ATTRIBUTES
160130
value: "service.name=sim-app,service.version={{ .Chart.AppVersion }},deployment.environment={{ $nodeEnv }}"
161131
{{- end }}
162-
{{- with .Values.extraEnvVars }}
163-
{{- toYaml . | nindent 12 }}
164-
{{- end }}
132+
{{- /* Operator overrides last, so they win over the defaults above and over envFrom. */}}
133+
{{- include "sim.appEnvOverrides" . | trim | nindent 12 }}
165134
envFrom:
166135
# Bundled Redis URL. Listed FIRST so any operator-supplied REDIS_URL
167136
# in a later source overrides it — see templates/configmap-redis.yaml.

‎helm/sim/tests/secret-modes_test.yaml‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,3 +179,65 @@ tests:
179179
name: ENCRYPTION_KEY
180180
valueFrom:
181181
secretKeyRef: { name: my-existing-secret, key: ENCRYPTION_KEY, optional: true }
182+
183+
- it: an extraEnvVars sealing opt-out reaches the migrations init container
184+
template: deployment-app.yaml
185+
set:
186+
app.env.BETTER_AUTH_SECRET: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
187+
app.env.ENCRYPTION_KEY: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
188+
app.env.INTERNAL_API_SECRET: x
189+
app.env.CRON_SECRET: x
190+
postgresql.auth.password: xxxxxxxx
191+
extraEnvVars:
192+
- name: ENCRYPT_ACCOUNT_TOKENS
193+
value: "false"
194+
asserts:
195+
- contains:
196+
path: spec.template.spec.initContainers[0].env
197+
content: { name: ENCRYPT_ACCOUNT_TOKENS, value: "false" }
198+
- notContains:
199+
path: spec.template.spec.initContainers[0].env
200+
content:
201+
name: ENCRYPT_ACCOUNT_TOKENS
202+
valueFrom:
203+
secretKeyRef: { name: t-sim-app-secrets, key: ENCRYPT_ACCOUNT_TOKENS, optional: true }
204+
205+
- it: an envDefaults sealing opt-out reaches the migrations init container
206+
template: deployment-app.yaml
207+
set:
208+
app.env.BETTER_AUTH_SECRET: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
209+
app.env.ENCRYPTION_KEY: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
210+
app.env.INTERNAL_API_SECRET: x
211+
app.env.CRON_SECRET: x
212+
app.envDefaults.ENCRYPT_ACCOUNT_TOKENS: "false"
213+
postgresql.auth.password: xxxxxxxx
214+
asserts:
215+
- contains:
216+
path: spec.template.spec.initContainers[0].env
217+
content: { name: ENCRYPT_ACCOUNT_TOKENS, value: "false" }
218+
219+
- it: an extraEnvVars ENCRYPTION_KEY override reaches the migrations init container
220+
template: deployment-app.yaml
221+
set:
222+
app.env.BETTER_AUTH_SECRET: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
223+
app.env.ENCRYPTION_KEY: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
224+
app.env.INTERNAL_API_SECRET: x
225+
app.env.CRON_SECRET: x
226+
postgresql.auth.password: xxxxxxxx
227+
extraEnvVars:
228+
- name: ENCRYPTION_KEY
229+
valueFrom:
230+
secretKeyRef: { name: vault-synced, key: encryption-key }
231+
asserts:
232+
- contains:
233+
path: spec.template.spec.initContainers[0].env
234+
content:
235+
name: ENCRYPTION_KEY
236+
valueFrom:
237+
secretKeyRef: { name: vault-synced, key: encryption-key }
238+
- contains:
239+
path: spec.template.spec.containers[0].env
240+
content:
241+
name: ENCRYPTION_KEY
242+
valueFrom:
243+
secretKeyRef: { name: vault-synced, key: encryption-key }

‎helm/sim/values.yaml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,10 @@ app:
109109
BETTER_AUTH_SECRET: "" # REQUIRED - set via --set flag or external secret manager
110110
ENCRYPTION_KEY: "" # REQUIRED - set via --set flag or external secret manager
111111
# A 64-hex ENCRYPTION_KEY also seals stored OAuth account tokens, and the migrations init
112-
# container seals the ones stored before. Set ENCRYPT_ACCOUNT_TOKENS: "false" to opt out.
112+
# container seals the ones stored before. To opt out, set ENCRYPT_ACCOUNT_TOKENS to "false"
113+
# or "0" wherever the app reads its env: here (inline or existingSecret mode), as a key in the
114+
# pre-created Secret, mapped in externalSecrets.remoteRefs.app (External Secrets mode), or in
115+
# app.envDefaults / extraEnvVars. The init container resolves it the same way as the app.
113116
INTERNAL_API_SECRET: "" # REQUIRED - set via --set flag or external secret manager, used for internal service-to-service authentication
114117

115118
# Optional: Scheduled Jobs Authentication
@@ -2006,6 +2009,9 @@ externalSecrets:
20062009
CRON_SECRET: ""
20072010
# Path to API_ENCRYPTION_KEY in external store (optional)
20082011
API_ENCRYPTION_KEY: ""
2012+
# Path to ENCRYPT_ACCOUNT_TOKENS in external store (optional; "false" or "0" opts out of
2013+
# account token sealing)
2014+
ENCRYPT_ACCOUNT_TOKENS: ""
20092015
# Path to MSHIP_SYSPROMPT_OVERRIDE in external store (optional)
20102016
MSHIP_SYSPROMPT_OVERRIDE: ""
20112017
# Path to REDIS_URL in external store (optional)

‎packages/db/script-migrations/0037_seal_account_tokens.integration.ts‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { openText } from '@sim/db/sealed-text'
77
import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
88
import { seal, sealingKeyId } from '@sim/security/encryption'
99
import { createDeferred } from '@sim/testing/helpers/deferred'
10+
import { sleep } from '@sim/utils/helpers'
1011
import { generateId } from '@sim/utils/id'
1112
import postgres, { type Sql } from 'postgres'
1213
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
@@ -212,8 +213,9 @@ describe('sealing stored account tokens in PostgreSQL', () => {
212213
await markSealedReaderRelease('2 days')
213214
})
214215

215-
afterEach(() => {
216+
afterEach(async () => {
216217
vi.unstubAllEnvs()
218+
await admin.unsafe(`DROP FUNCTION IF EXISTS "${schemaName}".on_account_update() CASCADE`)
217219
})
218220

219221
/** When the database first ran the migrate step of a release that reads sealed tokens. */
@@ -331,5 +333,44 @@ describe('sealing stored account tokens in PostgreSQL', () => {
331333
}
332334
expect((await storedRows())[0].access_token).toBe('locked-access')
333335
}, 30_000)
336+
337+
/** Makes every account UPDATE in this schema run `body` first, as the batch's own statement. */
338+
async function onAccountUpdate(body: string) {
339+
await admin.unsafe(`CREATE FUNCTION "${schemaName}".on_account_update() RETURNS trigger
340+
LANGUAGE plpgsql AS $$ BEGIN ${body}; RETURN NEW; END $$`)
341+
await admin.unsafe(`CREATE TRIGGER on_account_update BEFORE UPDATE ON "${schemaName}".account
342+
FOR EACH ROW EXECUTE FUNCTION "${schemaName}".on_account_update()`)
343+
}
344+
345+
it('defers when a batch hits its statement timeout', async () => {
346+
await insertAccount('slow-access', 'slow-refresh')
347+
await onAccountUpdate(
348+
`RAISE EXCEPTION 'canceling statement due to statement timeout' USING ERRCODE = '57014'`
349+
)
350+
351+
await migrate()
352+
353+
expect(await isRecorded()).toBe(false)
354+
expect((await storedRows())[0].access_token).toBe('slow-access')
355+
})
356+
357+
it('fails the run when an operator cancels a batch', async () => {
358+
await insertAccount('cancelled-access', 'cancelled-refresh')
359+
await onAccountUpdate('PERFORM pg_sleep(20)')
360+
361+
const run = migrate()
362+
for (let attempt = 0; attempt < 100; attempt++) {
363+
const [cancelled] = await admin<{ cancelled: boolean }[]>`
364+
SELECT pg_cancel_backend(pid) AS cancelled FROM pg_stat_activity
365+
WHERE datname = current_database() AND wait_event = 'PgSleep'
366+
AND query LIKE '%UPDATE account%'`
367+
if (cancelled?.cancelled) break
368+
await sleep(50)
369+
}
370+
371+
await expect(run).rejects.toThrow('canceling statement due to user request')
372+
expect(await isRecorded()).toBe(false)
373+
expect((await storedRows())[0].access_token).toBe('cancelled-access')
374+
}, 30_000)
334375
})
335376
})

‎packages/db/script-migrations/0037_seal_account_tokens.ts‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
} from '@sim/db/sealed-text'
1010
import { createLogger } from '@sim/logger'
1111
import { sealedKeyId, sealingKeyId } from '@sim/security/encryption'
12-
import { getPostgresErrorCode } from '@sim/utils/errors'
12+
import { getPostgresCancellationReason } from '@sim/utils/errors'
1313
import { sleep } from '@sim/utils/helpers'
1414
import type { Sql } from 'postgres'
1515

@@ -152,8 +152,11 @@ const SEALED_READER_RELEASE_MIGRATION = '0036_archive_legacy_scheduled_jobs'
152152
*/
153153
const SEALED_READER_ROLLOUT_GRACE = '1 hour'
154154

155-
/** Lock and statement timeouts a batch sets on itself; either leaves the rest for the next run. */
156-
const BATCH_TIMEOUT_CODES = new Set(['55P03', '57014'])
155+
/**
156+
* The lock and statement timeouts a batch sets on itself; either leaves the rest for the next run.
157+
* An operator's cancel (`user_cancel`) is not one of them, so it fails the run visibly.
158+
*/
159+
const BATCH_TIMEOUTS = new Set(['lock_timeout', 'statement_timeout'])
157160

158161
/**
159162
* Seals the account tokens stored before this deployment sealed its writes, during `migrate`.
@@ -189,9 +192,9 @@ export const sealAccountTokensMigration: ScriptMigration = {
189192
}
190193
logger.info('Account token backfill finished', result)
191194
} catch (error) {
192-
const code = getPostgresErrorCode(error)
193-
if (!code || !BATCH_TIMEOUT_CODES.has(code)) throw error
194-
throw new ScriptMigrationDeferred(`an account token batch timed out (${code})`)
195+
const reason = getPostgresCancellationReason(error)
196+
if (!reason || !BATCH_TIMEOUTS.has(reason)) throw error
197+
throw new ScriptMigrationDeferred(`an account token batch hit its ${reason}`)
195198
}
196199
},
197200
}

‎packages/db/script-migrations/index.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { backfillPublicShareEntitiesMigration } from '@sim/db/script-migrations/
1616
import { validateFileWorkspaceBindingMigration } from '@sim/db/script-migrations/0035_validate_file_workspace_binding'
1717
import { archiveLegacyScheduledJobsMigration } from '@sim/db/script-migrations/0036_archive_legacy_scheduled_jobs'
1818
import { sealAccountTokensMigration } from '@sim/db/script-migrations/0037_seal_account_tokens'
19+
import { createLogger } from '@sim/logger'
1920
import type { Sql } from 'postgres'
2021
import { backfillTableOrderKeys } from './0001_backfill_table_order_keys'
2122
import { backfillPausedBillingAttribution } from './0002_backfill_paused_billing_attribution'
@@ -30,6 +31,9 @@ import { type ScriptMigration, ScriptMigrationDeferred } from './types'
3031

3132
export type { ScriptMigration } from './types'
3233

34+
/** The migrations container's progress log, which operators read on every upgrade. */
35+
const logger = createLogger('ScriptMigrations', { enabled: true, logLevel: 'INFO' })
36+
3337
/**
3438
* Ordered, append-only registry of script migrations. An entry may be deleted
3539
* once a later migration supersedes it (accepting that deployments which
@@ -126,7 +130,7 @@ export async function runScriptMigrations(
126130

127131
const pending = migrations.filter((migration) => !applied.has(migration.name))
128132
if (pending.length === 0) {
129-
console.log('No pending script migrations.')
133+
logger.info('No pending script migrations.')
130134
return
131135
}
132136

@@ -138,14 +142,17 @@ export async function runScriptMigrations(
138142
)
139143
}
140144
}
141-
console.log(`Applying script migration ${migration.name}...`)
145+
logger.info(`Applying script migration ${migration.name}...`)
142146
const startedAt = Date.now()
143147
try {
144148
await migration.up(sql)
145149
} catch (error) {
146150
if (!(error instanceof ScriptMigrationDeferred)) throw error
147-
console.log(
148-
`Script migration ${migration.name} deferred (${error.message}); it runs again on the next migrate.`
151+
logger.info(
152+
`Script migration ${migration.name} deferred; it runs again on the next migrate.`,
153+
{
154+
reason: error.message,
155+
}
149156
)
150157
continue
151158
}
@@ -154,6 +161,6 @@ export async function runScriptMigrations(
154161
await tx`INSERT INTO script_migrations (name) VALUES (${name}) ON CONFLICT (name) DO NOTHING`
155162
}
156163
})
157-
console.log(`Script migration ${migration.name} applied in ${Date.now() - startedAt}ms.`)
164+
logger.info(`Script migration ${migration.name} applied in ${Date.now() - startedAt}ms.`)
158165
}
159166
}

0 commit comments

Comments
 (0)