Warn instead of failing when a compat flag is already on by date - #6980
Warn instead of failing when a compat flag is already on by date#6980petebacondarwin wants to merge 1 commit into
Conversation
Naming a compatibility flag that the Worker's compatibility date already enables produced a fatal configuration error, so `workerd serve` exited rather than starting. The compiled flag set is identical either way, so the redundancy is worth pointing out but is not a reason to refuse to run. This became a common papercut once `nodejs_compat` gained a default-on date of 2026-08-04, since configs that had always listed the flag explicitly stopped loading as soon as their compatibility date advanced past it. ValidationErrorReporter gains `addWarning()` for configuration problems that do not prevent the Worker from running. Its default implementation forwards to `addError()`, so a reporter that has not decided how to surface warnings keeps rejecting the configuration rather than silently accepting it; treating a warning as non-fatal is an explicit opt-in. workerd's own reporters opt in. Services from a config file report through a new warning sink on Server, which the CLI prints via the process context so it honours --structured-logging. Dynamically-loaded Workers log to the process log instead, having no config file to point the developer at. The check stays suppressed under FUTURE_FOR_TEST, where it would otherwise fire for nearly every test, because wd_test runs each test at both the oldest and the newest compatibility date.
|
APIError: Invalid Anthropic API Key |
2 similar comments
|
APIError: Invalid Anthropic API Key |
|
APIError: Invalid Anthropic API Key |
|
@petebacondarwin Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
dom96
left a comment
dom96
left a comment
There was a problem hiding this comment.
A few things that need to be changed, but almost ready to go.
I would also suggest creating a validator test in EW for this. Just to make sure validations aren't broken by this (which as things stand would happen I think).
| // how to surface warnings therefore keeps rejecting the configuration rather than silently | ||
| // accepting it; treating a warning as non-fatal is an explicit opt-in. | ||
| virtual void addWarning(kj::String warning) { | ||
| addError(kj::mv(warning)); |
There was a problem hiding this comment.
Any validation report with an error is treated as a failed validation. So this will prevent deployment of workers that have a warning.
We could hook this up through EWC, by adding a new warnings field in the ValidationReport, but that would require changes in EW and EWC. So I think it's fine to simply drop it here: no need to give users this warning when they deploy, just when they run the worker locally.
| // A dynamically-loaded Worker has no configuration file to point the developer at, so the | ||
| // process log is the only place left to say anything. | ||
| for (auto& warning: errorReporter.warnings) { | ||
| KJ_LOG(WARNING, warning); | ||
| } | ||
|
|
There was a problem hiding this comment.
You already log a warning in src/workerd/server/server.c++. This shouldn't be necessary here (and actually may cause problems for the validator).
|
Also we should make sure Bonk runs on this |
2 participants
Problem
Naming a compatibility flag that the Worker's compatibility date already enables was a fatal
configuration error, so
workerd serveexited instead of starting:The compiled flag set is identical whether or not the flag is listed, so the redundancy is worth
pointing out but is not a reason to refuse to run. This became a common papercut once
nodejs_compatgained a default-on date of 2026-08-04: configs that had always listed the flagexplicitly stopped loading as soon as their compatibility date advanced past it.
Change
ValidationErrorReportergainsaddWarning()for configuration problems that don't prevent theWorker from running, and the redundant-flag diagnostic moves onto it.
The part worth reviewing carefully is that
addWarning()'s default implementation forwards toaddError(). A reporter that hasn't decided how to surface warnings therefore keeps rejecting theconfiguration rather than silently accepting it — treating a warning as non-fatal is an explicit
opt-in. That keeps this PR's blast radius inside this repo: I audited every reporter implemented
outside it, including the ones behind Cloudflare's deploy-time validation and Worker startup, and
all of them inherit the default and so behave exactly as they do today. Relaxing those paths is a
separate follow-up.
The
Serverconstructor gains a parameter, but nothing outside this repo constructs it, so thesignature change is contained here too.
workerd's own reporters opt in:
Server::ConfigErrorReporterServer→context.warning()in the CLI, so it honours--structured-loggingServer::DynamicErrorReporter,WorkerLoaderKJ_LOG(WARNING)— a dynamically-loaded Worker has no config file to point atSimpleWorkerErrorReporterwarningsvector alongsideerrorsMockErrorReporter(test fixture)The check stays suppressed under
FUTURE_FOR_TEST, where it would fire for nearly every test:wd_testruns each test at both the oldest and the newest compatibility date.Testing
compatibility-date-test: the existing redundant-flag case moves fromexpectedErrorsto a newexpectedWarnings(which also newly asserts the resulting flag set, confirming the flag is stillapplied). Adds coverage for the
CODE_VERSIONpath, the$compatEnableAllDatesmessage variant,and
FUTURE_FOR_TESTstaying silent.server-test: new end-to-end case asserting the Worker actually starts, serves, and emits thewarning.
inspector-test, is a pre-existing flake — it failsroughly 1-in-8 runs against an identical prebuilt binary, and its config is on compatibility date
2024-01-01 with
nodejs_compat, so no warning fires for it.