misuse and lifecycle suites, and the deadlock writing them found - #9
Merged
Conversation
DX3 asks every client for a suite of deliberately wrong programs, no crash, no hang, no leak, and a clear error for each. Writing the table found three ways this client failed that on its own terms, so the fixes are here with it. A statement or a path that is not a string threw a napi TypeError synchronously out of the call, which broke the promise the README makes that every failure is a rejection, and the message it threw was about converting a JavaScript value into a rust type, to somebody who mistyped a variable. Both are read on this side now and refused inside the task as a ZuUsageError. Parameters passed as an array or a string were bound as 0, 1, 2 and the statement ran with none of the caller's values and said nothing about it. Refused now, with a message that says parameters are named. A parameter that contains itself walked until the stack ran out. Refused at depth 64, which no value anybody writes reaches. The fourth was the one worth the day. A query on a connection whose stream was half-read waited forever, and eight streams on one connection hung the process past what process.exit could escape, because a stream held the connection's mutex for the whole life of the statement while the producer waited on a reader that was itself waiting for that mutex. A stream takes the connection out of the slot now instead of locking it, so the next statement finds the slot empty and is told at once to read the stream out, cancel it, or open a second connection. The take and the two readings of an empty slot are decided under one lock, so a stream and a close that arrive together cannot both find the connection theirs. The sanitizer and leak jobs are the other half of the line. Both were built and run on Linux before being written down: the suite over an ASan-instrumented addon, and the suite under Valgrind with definite leaks fatal. Both are clean, and Valgrind needs no suppression file, which is worth saying out loud because the day it does is the day somebody should read what it wants suppressed.
Both failures were real and neither was in the addon. The leak job wanted a suppression file after all. Which node this runs on decides what leaks: the one in my container links the system OpenSSL and reports nothing, and the one the hosted images install has OpenSSL statically inside it and holds twenty four bytes of built-in compression table from the first use of the default library context to exit. There is no version of this addon that frees that. So tools/valgrind.supp names the binary that allocated rather than the leak that was reported, which is the difference between a rule and a list that grows a line every bad week, and it says in the file what it leaves out. The C allocators were not enough on their own, because node is C++ and holds most of what it holds through operator new, and the names have to be the mangled ones because that is what valgrind matches whatever it prints. The gate is validated the way a gate has to be: a deliberate leak out of a preloaded shared object, which is the shape of the addon itself, has to fail it. The bun job has been red on main since before this branch, on the two stream tests that build sixty thousand people. Bun's node:test shim brings Bun's own five second per-test timeout, which node does not have and a fixture that size does not fit inside on a shared runner. The limit is now sixty seconds, where it catches a hang rather than a fixture.
On x86 the suite passes and the job fails, on a conditional jump inside Maglev while it compiles on a thread of its own. That is V8 reading its own uninitialised memory a dozen frames from anything this package wrote, and the only suppression that would cover it is one for uninitialised values inside node, which is the class of error worth keeping: an addon that hands back a half-filled buffer is what this tool catches and the sanitizer does not. --no-opt turns the tier off instead, which removes the noise where it comes from and costs a job already fifty times slower than the real thing nothing it was measuring.
--no-opt left Maglev running and the leaks job stayed red on the same uninitialised-value report inside MaglevGraphBuilder. The tiers each have their own switch now, and a list of them goes stale the next time V8 grows one, so use the flag that cannot be partially true. The suite runs jitless in ten seconds.
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
1 participant
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.
DX3 asks every client for a suite of deliberately wrong programs: no crash, no hang, no leak, and a clear error for each.
test/misuse.test.mjsis that suite, 29 tests over a frozen table of 22 wrong programs plus seven lifecycle tests, and writing it found four ways this client failed the line on its own terms. The fixes are in this branch with it, because a suite that documents defects rather than catching them is a suite nobody trusts.Clear is the hard word, so the header spells it out as four things a message has to do. It names the thing the caller named. It says what was expected. It is the engine's sentence rather than a syscall's, since "failed to fill whole buffer" is a true statement about a read that tells nobody which file was not a database. And it never describes this crate's insides, because "Failed to convert JavaScript value
Number 42into rust typeString" is a message about napi to somebody who mistyped a variable.That last one was literally what happened. A statement or a path that was not a string threw a raw napi
TypeErrorsynchronously out ofquery,exec,cursor,streamandconnect, which broke the error model and broke the README's own promise that every failure is a rejection, so a caller who wroteawaithad nowhere to catch it. Both are read on this side now and refused inside the task as aZuUsageErrorwith a plain sentence.Parameters passed as an array or as a string were silently accepted and bound as
0,1,2, so the statement ran with none of the caller's values and said nothing about it. They are refused now, with a message that says zu names its parameters rather than numbering them.A parameter that contains itself walked until the stack ran out and took the process with it. It is refused at depth 64, which is set where a real value never reaches and a cycle always does.
The fourth was the one worth the day. A
queryon a connection whose stream was half-read waited forever, and eight concurrent streams on one connection hung the process so hard thatprocess.exit(9)could not escape it.Started::streamheld the connection's mutex for the whole life of the statement while the producer waited on the reader, and the reader was awaiting a statement that was waiting for that mutex. A stream takes the connection out of the slot now rather than locking it, so the next statement finds the slot empty and is told at once to read the stream to the end, cancel it, or open a second connection. The take and the two readings of an empty slot happen under the one lock that decides them, so a stream and a close that arrive together cannot both find the connection theirs, and a close that lands while a statement is running drops the connection rather than putting back what the caller closed.The sanitizer and leak jobs are the other half of the checklist line. Both were built and run on Linux before being written down rather than after. The suite over an ASan-instrumented addon: 110 tests, clean, with
-Zexternal-clangrtand the runtime preloaded because the addon is opened byrequirelong after node has started. The suite under Valgrind with--leak-check=full --errors-for-leak-kinds=definite --error-exitcode=1: 110 tests, clean, no suppression file, which is worth saying out loud because the day it needs one is the day somebody should read what it wants suppressed before writing it down. There is nounsafein this crate, which is the reason to run these rather than the reason not to: what a binding gets wrong is a handle used after its scope closed, and that is a use-after-free even where no block says so.The published surface is unchanged.
binding.d.ctsis byte-identical,check:typesandcheck:packagepass, and the full suite is 119 tests, 110 passing, 9 skipped for the runtime without Temporal.