Skip to content

Commit c0407d9

Browse files
chrisbbreuerchrisbbreuer
committed
fix(bench): keep the application environment out of benchmark servers
`bench/routing/bunfig.toml` says benchmark servers must not inherit the application's environment. They did. The runner starts from the repository root, so the root bunfig preloads `.env` into its own `process.env`, and `serverEnvironment()` spread that whole environment into every target it spawned - `MAIL_HOST`, `APP_KEY` and the rest, verified on this checkout. The peers ignore all of it and the Stacks targets do not, so the asymmetry landed on exactly one set of rows. A `STACKS_CSP` in someone's `.env` adds a response header to every Stacks response and to no other framework's, changing both the work and the bytes on the wire. The query-logging knobs beyond the documented `DB_QUERY_LOGGING_ENABLED` change the database scenario for one target. `NODE_OPTIONS` can inject a loader. Two people on the same commit would measure different things, with nothing in the report saying so. The three profile selectors the runner already resets were added for this hazard, but three names out of an open set is not isolation. A server now inherits only what a process needs to start - PATH, HOME, locale, temp directories, the Windows equivalents - and the benchmark states everything else explicitly, as it already did for those selectors. `DB_QUERY_LOGGING_ENABLED` is forwarded because the report describes it. Verified across the default matrix: every target boots, the SQLite scenario resolves its fixture, and the Stacks fixture serves correctly with no APP_KEY and no `.env` at all. closes #2589
1 parent 3600b95 commit c0407d9

3 files changed

Lines changed: 104 additions & 2 deletions

File tree

‎bench/routing/README.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,14 @@ accident. The safeguards here exist only to stop that:
5353
`bun:sqlite`. Tests also reject benchmark selectors in framework source and
5454
prevent the fixture from bypassing the router with direct Bun serving,
5555
prebuilt responses, or manual JSON serialization.
56+
- **No application configuration reaches a server.** The runner boots through
57+
this repository's bunfig, which preloads `.env`, and only the Stacks targets
58+
read any of it: a stray `STACKS_CSP` would add a response header to one
59+
framework and to no other, and two people on the same commit would measure
60+
different things. A server inherits only what a process needs to start -
61+
`PATH`, `HOME`, locale, temp directories - and the benchmark states
62+
everything else explicitly. `DB_QUERY_LOGGING_ENABLED` is forwarded because
63+
the report describes it.
5664
- **Every process exposes only benchmark routes.** The Stacks fixture uses the
5765
framework's public programmatic-router configuration to disable application
5866
route discovery, and its public API-server configuration to disable view

‎bench/routing/runtime.test.ts‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, it } from 'bun:test'
22
import { join } from 'node:path'
3-
import { assertProbeResponse, assertResponseParity, assertStableParity, BENCH_ROOT, benchmarkQueryLoggingEnabled, headersFor, probeHeadersFor, serverCommand, serverEnvironment } from './runtime'
3+
import { assertProbeResponse, assertResponseParity, assertStableParity, BENCH_ROOT, benchmarkQueryLoggingEnabled, headersFor, hostEnvironment, probeHeadersFor, serverCommand, serverEnvironment } from './runtime'
44
import { SCENARIOS } from './scenarios'
55
import { DEFAULT_TARGETS, targetById } from './targets'
66

@@ -27,6 +27,30 @@ describe('benchmark server isolation', () => {
2727
expect(env.BENCH_SCENARIO).toBe('static-json')
2828
})
2929

30+
it('keeps the application environment out of every server', () => {
31+
// The runner boots through the repository bunfig, which preloads `.env`,
32+
// and only the Stacks targets read any of it. A stray `STACKS_CSP` would
33+
// add a response header to one framework and to no other.
34+
const environment = hostEnvironment({
35+
PATH: '/usr/bin',
36+
HOME: '/home/bench',
37+
STACKS_CSP: "default-src 'self'",
38+
APP_KEY: 'secret',
39+
MAIL_HOST: 'smtp.example.com',
40+
DB_PASSWORD: 'secret',
41+
DB_QUERY_LOGGING_SLOW_THRESHOLD: '1',
42+
NODE_OPTIONS: '--require ./loader.js',
43+
})
44+
45+
expect(environment).toEqual({ PATH: '/usr/bin', HOME: '/home/bench' })
46+
})
47+
48+
it('forwards the documented benchmark opt-in so the server matches the report', () => {
49+
expect(hostEnvironment({ DB_QUERY_LOGGING_ENABLED: 'true' }))
50+
.toEqual({ DB_QUERY_LOGGING_ENABLED: 'true' })
51+
expect(hostEnvironment({})).toEqual({})
52+
})
53+
3054
it('treats persistent query logging as an explicit benchmark profile', () => {
3155
const previous = process.env.DB_QUERY_LOGGING_ENABLED
3256
try {

‎bench/routing/runtime.ts‎

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,80 @@ export function serverCommand(server: string): string[] {
6969
]
7070
}
7171

72+
/**
73+
* What a benchmark server inherits from the machine it runs on.
74+
*
75+
* Not the whole parent environment. The runner boots through the repository's
76+
* own bunfig, which preloads `.env`, so spreading `process.env` handed every
77+
* server the developer's application configuration - and only the Stacks
78+
* targets read any of it. A stray `STACKS_CSP` adds a header to every Stacks
79+
* response and to nobody else's; a stray query-logging threshold changes the
80+
* database row for one framework. Two people on the same commit would measure
81+
* different things, and the difference would land entirely on one target.
82+
*
83+
* So the host contributes only what a process needs to run at all, and the
84+
* benchmark states everything else explicitly. `NODE_OPTIONS` is deliberately
85+
* absent: it can inject a loader into one process and not another.
86+
*/
87+
const HOST_ENVIRONMENT = [
88+
'PATH',
89+
'HOME',
90+
'USER',
91+
'LOGNAME',
92+
'SHELL',
93+
'TERM',
94+
'TMPDIR',
95+
'TMP',
96+
'TEMP',
97+
'TZ',
98+
'LANG',
99+
'LC_ALL',
100+
'LC_CTYPE',
101+
'BUN_INSTALL',
102+
'BUN_INSTALL_CACHE_DIR',
103+
'XDG_CACHE_HOME',
104+
'XDG_CONFIG_HOME',
105+
'XDG_DATA_HOME',
106+
// Windows needs these to start a process at all.
107+
'APPDATA',
108+
'COMSPEC',
109+
'LOCALAPPDATA',
110+
'PATHEXT',
111+
'PROGRAMDATA',
112+
'PROGRAMFILES',
113+
'SYSTEMDRIVE',
114+
'SYSTEMROOT',
115+
'USERPROFILE',
116+
'WINDIR',
117+
] as const
118+
119+
/**
120+
* Benchmark switches the README documents as opt-in, passed through when set.
121+
*
122+
* `DB_QUERY_LOGGING_ENABLED` turns the database scenario into an
123+
* observability-cost profile. The runner already reads it to label the report,
124+
* so the server it describes has to see the same value.
125+
*/
126+
const FORWARDED_ENVIRONMENT = ['DB_QUERY_LOGGING_ENABLED'] as const
127+
128+
export function hostEnvironment(source: Record<string, string | undefined> = process.env): Record<string, string> {
129+
const environment: Record<string, string> = {}
130+
for (const name of [...HOST_ENVIRONMENT, ...FORWARDED_ENVIRONMENT]) {
131+
const value = source[name]
132+
if (value != null)
133+
environment[name] = value
134+
// Windows environment names are case-insensitive but arrive capitalized.
135+
const actual = Object.keys(source).find(key => key.toUpperCase() === name && key !== name)
136+
if (actual != null && source[actual] != null)
137+
environment[actual] = source[actual]
138+
}
139+
return environment
140+
}
141+
72142
/** Give every framework the same production environment. */
73143
export function serverEnvironment(target: Target, withDb: boolean, scenarioId?: string): Record<string, string> {
74144
return {
75-
...process.env,
145+
...hostEnvironment(),
76146
APP_ENV: 'production',
77147
NODE_ENV: 'production',
78148
BENCH_PORT: String(PORT),

0 commit comments

Comments
 (0)