fix(plugins): bound the setup-field match, not the worker's birth (#607) - #609
Merged
Conversation
The pattern match budget measured a window that included worker startup and module evaluation, so a valid credential could be rejected because the host was busy and a healthy pattern could be permanently reported to the operator as uncheckable. - The worker now handshakes when its message handler is installed, and readiness waits for that instead of the 'online' event. 'online' fires when the thread starts, well before the module has been evaluated; under tsx that gap is the loader re-running (84-212 ms measured), and on a cold start it was ~838 ms. All of it was charged to the budget. - The worker stays ref'd until it handshakes. Unref'ing on 'online' deadlocks: nothing holds the event loop open while awaiting the handshake, which surfaced as "Promise resolution is still pending but the event loop has already resolved" across the suite on node 22. - Budget raised 50 ms -> 250 ms. The worker boundary is what protects the event loop; this number only bounds how long one admin-only write waits. The measured IPC floor alone was 4-36 ms, so 50 ms rejected valid values with no regex work to speak of. - A budget overrun no longer records a pattern problem on its own. The write still fails closed immediately, but the durable "this field is unchecked" verdict now needs three consecutive overruns; any completed match resets the count. - A dead worker is retried once on a fresh thread. A budget expiry is never retried, so a hostile pattern still costs exactly one budget. - warmPatternWorker() is called at startup so the first operator to save a credential does not pay thread creation inside their request. Tests pin the cold-start path, the strike counter and its reset, using a pattern calibrated by measurement (500-char subject ~1.6 s vs 'abcd' ~0 ms on the same source). Verified on node 22.22.3 and 26.3.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #607.
The defect
The setup-field pattern check runs every match in a worker thread under a wall-clock budget, because a regex cannot be interrupted on the thread running it. That mechanism is right and stays. What was wrong is the window being measured: the budget included worker startup and module evaluation, neither of which is regex work.
Two user-visible consequences, both timing-dependent — which is why they read as intermittent "my correct value was rejected" reports:
getPatternProblems()and then reported to the operator as "this field declares a format check that could not be applied" — permanently, for the life of the process.Both are the failure class the customer test report was about: the UI stating something that is not true.
Root cause
ensureWorker()resolved readiness on the worker's'online'event.'online'fires when the thread starts, not when the worker module has been evaluated and its message listener exists. Everything in that gap was charged to the match budget:node --import tsx(how the suite runs)Changes
1. The worker handshakes. It posts
{ ready: true }after installing its message handler, and readiness waits for that. The budget timer now starts after the handshake, so it bounds regex execution plus one IPC round trip.2. The worker stays ref'd until it handshakes. This one bit me and is worth calling out: unref'ing on
'online'(which an intermediate revision of this branch did) is a deadlock — nothing else holds the event loop open while awaiting the handshake, so on an otherwise-idle loop node settles and the promise never resolves. It surfaced asPromise resolution is still pending but the event loop has already resolvedacross the whole file on node 22, while node 26 happened to hide it. Both versions are now verified.3. Budget 50 ms → 250 ms. The budget is not what protects the event loop — the worker boundary is. A runaway regex burns a thread nothing is waiting on, so this number only bounds how long one admin-only setup write waits before giving up. With a measured IPC floor of 4-36 ms, 50 ms was rejecting valid values with no regex work to speak of.
4. An overrun no longer blames the pattern on its own. The write still fails closed on the first overrun — that is unchanged and deliberate. But the durable verdict now needs
PATTERN_OVERRUN_STRIKES(3) consecutive overruns, and any completed match resets the count. A genuinely hostile pattern overruns every time and trips it within three writes; a healthy one recovers.5. A dead worker is retried once, on a fresh thread. A budget expiry is never retried, so a hostile pattern still costs exactly one budget. This only stops one caller's
terminate()landing on another caller's worker from being reported as "your value is invalid".6.
warmPatternWorker()at startup, so the first operator to save a credential does not pay thread creation inside their request. Fire-and-forget; the worker is still created on demand.Tests
New coverage in
middleware/test/setupFieldPatternValidation.test.ts:warmPatternWorkeris idempotent;The slow/fast pair is calibrated by measurement rather than assumed:
^[a-z]+[a-z]+[a-z]+[a-z]+$takes ~1.6 s on a 500-character subject and ~0 ms onabcd, so the overrun is not a race and the reset test exercises the same pattern (strikes are keyed by context and pattern).A measurement note that cost a round, recorded in the test file so the next person does not repeat it: V8 caches compiled regexes by source, so timing a subject after another subject has already run the same source reports the warm number. An earlier revision of these tests picked a subject that measured 0.011 ms that way — and 2152 ms on a first call in a fresh process.
Verification
middlewarefull suite on node 22.22.3: 5502 pass, 0 fail (--test-concurrency=4)tsc --noEmitclean,eslintclean on the changed filesprettier --checkflags both changed files, but it flags them identically onorigin/main— pre-existing, so reformatting here would bury the change in an unrelated diff.Not in scope
middleware/test/is still not typechecked bynpm run typecheck(#573) — the LSP surfaces a pre-existing error in this file at an untouched line.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.