proxy: TestFeedbackMailCannotInjectHeaders flakes because the mail goroutine reads CG_MAIL_DEV_SINK after its test restored it

[amiddavid]

The flake

proxy.TestFeedbackMailCannotInjectHeaders can fail with two subject lines where it asserts exactly one. Seen once under concurrent load on the eval box; main passed 3/3 full proxy runs in isolation.

It is not a race on shared memory (-race is clean) and it is unrelated to any open PR — it is a lifetime mismatch between a background sender and a per-test environment variable.

Mechanism

• Handler.deliverFeedback (proxy/feedback.go:153) is documented as running in its own goroutine, and nothing joins it — deliberately, since the row is already committed and a failed mail is meant to be logged and dropped.
• It calls sendMail → devSink() (proxy/mail.go:72), which resolves the sink by reading os.Getenv(CG_MAIL_DEV_SINK) at send time.
• Each test sets its own sink with t.Setenv + t.TempDir (e.g. proxy/feedback_test.go:389). t.Setenv restores the previous value when that test returns.

So a send still in flight when its test finishes reads whatever the next test has set, and writes into that test's sink file. The next test then counts its own mail plus the straggler and the "exactly one real subject line" assertion fails with 2.

The header-injection property the test exists for is unaffected — the extra line is a well-formed subject from a different test, not an injected one. So this is a test-isolation defect, not a security finding.

Why it is load-bearing to fix rather than retry

The assertion counts subject lines in a file to prove a hostile Subject: cannot be smuggled in. A flake there trains readers to re-run it, and this is exactly the assertion that must not be re-run until green — the failure mode it guards is one where an extra subject line is the bug.

Options

1. A sink resolved once per handler, captured at construction instead of read per send. Removes the env-at-send-time dependency entirely and is closest to how every other configured value in the proxy is handled. Changes a package-level function into handler state.
2. A test-only join: have deliverFeedback signal completion (a channel or sync.WaitGroup exposed via a …ForTest accessor) so the test waits before asserting. Smallest change, keeps production behaviour identical, and matches the existing …ForTest convention.
3. Run the affected tests without t.Setenv, using an explicit sink passed through the handler. Subsumed by (1).

I would take (2) if the goal is to stop the flake, and (1) if the goal is to remove the class — a background goroutine reading process-global config at use time will bite again the next time someone adds a test around it.

How it was found

Observed during an independent review of #204/#205/#208 on the eval box, under a concurrent full-package -race run. Explicitly identified as pre-existing and unrelated to those PRs; I confirmed the mechanism in the code (the goroutine at feedback.go:153, the read at mail.go:72, the t.Setenv at feedback_test.go:389) rather than only reproducing the symptom.

Credit for spotting it: the reviewer on #204/#205/#208.