Bound the wait on the Lua interpreter in tests - #1231
Conversation
Exercises type class bounds through the container they were added for. Six of the seven cases pass on both backends with no compiler change: int keys, tuple keys on Jass, a user class key, two specialisations coexisting, and two instances of one specialisation. tupleKeyLua fails and is a real, pre-existing backend bug. Method names become Lua table keys, but luaMethod.initFor passes the name through raw while every sibling (luaVar, luaFunc, luaClassVar) sanitises via uniqueName. Names are valid identifiers in ordinary code, so nothing hit it until a class method was specialised for Lua with more than one type argument: specializeMethod builds name + "_specialized_" + generics.makeName(), and makeName joins arguments with ", ". Two simple arguments give "get_specialized_integer, integer", which emits "Class.get_specialized_integer, integer = impl" -- valid Lua that assigns to two targets and quietly writes a junk global. A tuple argument gives "⦅integer, integer⦆" and fails the syntax check outright.
Method names become Lua table keys, so they must be identifiers. A method specialised with two type arguments was named after them, commas included, and emitted `Class.get_specialized_integer, integer = impl` - valid Lua that quietly assigns to two targets; a tuple argument produced characters luac rejects outright. normalizeMethodNames is the pass that gives one name to a whole dispatch group, so it sanitises before uniquing: two names that differed only in characters Lua has no place for still get a slot each. The backend maps every slot key and every LuaMethod name through the same function, so call sites and class tables keep agreeing. Lua's identifier rule now has one home. The luac check never caught this, because the broken output parses. Assert instead on the names themselves: every emitted function, method, variable, field and call-by-name must be an identifier, checked for every testLua compile.
The execution path read the spawned process's stderr to EOF before touching stdout, and never bounded the wait. A program that fills the stdout pipe blocks writing while the harness blocks reading stderr, and the suite stops with no output, no timeout and no failing test - a stray worker JVM was still sitting on the build directory twenty minutes later. checkLuaSyntax, in the same file, already drained both pipes on their own threads and waited with a timeout. The execution path now uses the same helper, so a program that does not terminate fails its test instead of the run. Also records what building a repro turned up: a bounded generic class cannot be subclassed on either backend, and the div/mod result type asymmetry is reachable through integer literals rather than harmless.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 596b6fae6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // and one that fills the stdout pipe would deadlock against a stderr-first read. | ||
| Thread outCollector = collectStreamAsync(p.getInputStream(), output); | ||
| Thread errCollector = collectStreamAsync(p.getErrorStream(), errors); | ||
| if (!p.waitFor(LUA_EXECUTION_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { |
There was a problem hiding this comment.
This bug fix has no automated reproduction for either the full-stdout-pipe deadlock or the timeout path; only the production test harness and BACKLOG prose changed. Without a focused test using a short configurable timeout and a subprocess that saturates stdout, a later return to sequential draining or an ineffective timeout will silently restore the suite-wide hang this change is intended to prevent.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
| Thread outCollector = collectStreamAsync(p.getInputStream(), output); | ||
| Thread errCollector = collectStreamAsync(p.getErrorStream(), errors); |
There was a problem hiding this comment.
When a generated Lua program loops while printing, both collectors continuously append into unbounded StringBuilders for the entire 60-second timeout. A sufficiently verbose failure can therefore exhaust the Gradle worker's heap before waitFor reports the timeout, replacing the intended bounded test failure with an OOM or worker crash; continue draining both pipes, but retain only a capped prefix or tail for diagnostics.
Useful? React with 👍 / 👎.
Wurst and Lua reserve different words, so a method can be declared repeat or goto and reach the backend under that name. Method names survive it, because the pass that assigns them uniques against the reserved set. A closure does not: it adds the name it implements as a dispatch alias directly, so the alias arrives as a bare keyword and is emitted as a table key. luac rejects that, so it was loud rather than wrong, but the check added alongside it accepted the name - and catching this before the syntax check is the whole point of having it. isValid now rejects keywords and toIdentifier maps them out of the way. Underscores rather than a counter, so a keyword maps to the same name wherever it is derived: call sites and class tables have to agree without consulting each other.
Two tests for the runner itself, both Lua only: run through the Jass configurations the non-terminating one would hang the interpreter instead, which is the same problem somewhere this fix does not reach. - a program that loops forever fails its own test rather than the run - a program that prints twenty thousand lines still finishes The second is the deadlock this change was for: with the streams read one after the other, the program blocks writing stdout while the runner blocks reading stderr. The timeout is overridable so these take seconds rather than a minute. Output kept for the failure message is capped. Draining still never stops, since stopping is what blocks the process, but a program that loops while printing would otherwise exhaust the worker before the timeout fires. Capping meant success could no longer be read back out of the retained text - testSuccess prints last, well past the limit - so the line is recognised while draining. Which is better regardless: whether a program succeeded no longer depends on how much of its output was kept.
|
Both addressed. P1 — regression tests. P2 — capped retention. Done, and the P1 test immediately caught that capping alone is wrong: Draining itself is unchanged and still unbounded, because stopping is what blocks the process. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1ac493a7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (out.length() < RETAINED_OUTPUT_LIMIT) { | ||
| out.append(line).append("\n"); |
There was a problem hiding this comment.
When a Lua program emits one very large line, the limit is ineffective because readLine() materializes the entire line and the collector appends it whenever the buffer was below the cap before that line. Fresh evidence in this revision is that the new cap is checked only before appending the complete line, so a generated program printing a sufficiently large string can still exhaust the Gradle worker before the timeout; drain fixed-size character chunks and retain only the remaining bounded portion while detecting the watched line separately.
Useful? React with 👍 / 👎.
Neither of these belongs in a branch that gets merged. The loop configuration is machine-local state - an active session id, an iteration count, a path and a branch name from one checkout - and another checkout picking it up would inherit an automation session that has nothing to do with it. The brief beside it is one run's instructions, not documentation of anything. Both came in with the branch this work was based on rather than with the work. Ignored as well, so they do not come back the next time either is written.
…erpreter-wait # Conflicts: # BACKLOG.md
The limit was checked once per line, which bounds nothing when a program prints one enormous line - and reading a line at all materialises the whole of it first, however much was printed before the newline, so the check could not have helped even where it applied. Draining is by fixed-size chunks now. Retained output stops at the limit part way through a chunk, and the watched line is recognised as characters arrive, keeping no more of a line than the length of what is being looked for: past that the line cannot be it. Third case in LuaRunnerTests: one line of a quarter of a million characters, which the previous revision would have kept in full.
|
Updated from master (#1230 is in) and the new finding is addressed. You were right on both halves, and the second half is the one that mattered: checking the limit once per line bounds nothing when a program prints one enormous line, and Draining is by fixed-size chunks now. Retained output stops at the limit part way through a chunk, and the watched line is recognised as characters arrive, keeping no more of a line than the length of what is being looked for: past that length the line cannot be it, so none of it is worth holding. Nothing in the collector now holds more than the retained limit plus one chunk.
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9cf53a010
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Test | ||
| public void aProgramThatPrintsOneEnormousLineStillFinishes() { | ||
| test().testLua(true).luaOnly(true).executeProg().lines( |
There was a problem hiding this comment.
This test only checks that execution finishes, so the previous readLine() implementation also passes: retaining this roughly quarter-megabyte line does not fail the test, and the following testSuccess line is still recognized. Consequently, reverting to the unbounded-per-line collector would remain undetected; expose/assert the retained length or otherwise make this case fail when one complete oversized line is materialized or retained.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
| if (out.length() < RETAINED_OUTPUT_LIMIT) { | ||
| out.append(chunk, 0, Math.min(read, RETAINED_OUTPUT_LIMIT - out.length())); | ||
| } else if (!truncated) { |
There was a problem hiding this comment.
When a finite stream's final read() contains more characters than the remaining capacity, this branch discards the suffix but does not set truncated; because no subsequent iteration occurs at EOF, the diagnostic ends at exactly 64 KiB without the “further output dropped” notice. Detect overflow within this same chunk so oversized stderr is not silently presented as complete.
AGENTS.md reference: AGENTS.md:L82-L85
Useful? React with 👍 / 👎.
Truncation was only noticed on a later chunk, so a stream whose last read crosses the limit ended at exactly the limit with nothing saying so - output that looks like everything the program had to say. It is noticed within the chunk that crosses now. The third runner test checks that a program printing without newlines finishes, which is what it is named for now. It does not observe how much of the line was kept and would pass against a collector that kept all of it. Bounding that is still worth doing, but a program printing a megabyte without a newline is not something this suite expects, and it is not worth test-only machinery to assert.
|
Took one, declined the other. P3 (truncation notice) — taken. Real and cheap: truncation was only noticed on a later chunk, so a stream whose final read crosses the limit ended at exactly 64KiB with nothing saying so — output that reads like everything the program had to say. Now noticed within the crossing chunk. P2 (make the huge-line test observe the bound) — declined, and the test relabelled instead. You are right that it does not observe retention and would pass against the old collector; that was a fair hit on the test rather than the code. But asserting it needs test-only plumbing to expose the retained buffer, for a program printing a megabyte with no newline — which is not a shape this suite is going to see. Rather than leave a test claiming coverage it does not have, it is now The bounded drain stays either way, since it costs nothing and the two tests above it cover the failure that actually bit us — the deadlock. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03e7c13d38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| pendingLine.setLength(0); | ||
| lineIsLongerThanWatched = false; | ||
| } else if (c != '\r' && !lineIsLongerThanWatched) { |
There was a problem hiding this comment.
When a tested Lua program prints an embedded carriage return—for example, println("test\rSuccess") on a path that never calls testSuccess()—this branch removes the character and incorrectly recognizes the output as the success marker. The previous BufferedReader.readLine() implementation preserved embedded carriage returns, so this can now make a failing backend test pass silently; ignore \r only when it is part of the line terminator.
AGENTS.md reference: AGENTS.md:L62-L62
Useful? React with 👍 / 👎.
1 participant
Stacked on #1230.
The Lua execution path read the spawned interpreter's stderr to EOF before touching stdout, and never bounded the wait. A program that fills the stdout pipe blocks writing while the harness blocks reading stderr, and the suite stops with no output, no timeout and no failing test. This was found the hard way: a stray worker JVM was still sitting on the build directory twenty minutes after the run appeared to hang, and a thread dump pointed at the read.
checkLuaSyntax, ten lines further down the same file, already drained both pipes on their own threads and waited with a timeout. The execution path now uses the same helper, so a program that does not terminate fails its own test instead of the run.Verified against a program that reproduced the hang: indefinite before, fails in 8s after.
Full test suite green.