dastest: a hung sweep names the program that hung - flushed, timestamped start lines, run_tests targets streamed through Ninja - and the first hang they named, the debug-agent threadlock test, fixed on a new try_invoke_in_context - #3949
Conversation
b2616cf to
d297a12
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, consistent with existing patterns (e.g., prior USES_TERMINAL usage), and directly address the stated CI output-buffering problem without altering test semantics.
Pull request overview
Improves test-sweep debuggability by emitting and flushing per-program (and per-batch) “started” progress lines so a hung/cancelled CI run still identifies the last test file/batch executed, and by making Ninja stream test-target output via USES_TERMINAL.
Changes:
- Add
log::started(...)indastestto always print+flush an immediate progress line (including under--failures-only). - Emit progress lines in both sequential sweeps (
run N/M: ...) and isolated workers (worker K starts: ...). - Mark
run_tests_*CMake custom targets withUSES_TERMINALso Ninja doesn’t buffer output until command completion; document the change inCHANGELIST.md.
File summaries
| File | Description |
|---|---|
| tests/CMakeLists.txt | Adds USES_TERMINAL to test-run custom targets so Ninja streams output during runs. |
| dastest/log.das | Introduces log::started to print+flush progress lines immediately (bypassing --failures-only suppression). |
| dastest/dastest.das | Logs per-file and per-batch start lines via log::started to pinpoint hangs/timeouts. |
| CHANGELIST.md | Records the behavioral change for users/CI maintainers. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new isolated-mode test currently assumes worker 1 runs the batch, which can be scheduler-dependent and make the test flaky.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
d297a12 to
f9e2fa4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new progress output can break existing machine-readable stdout modes (e.g., --bench-format json), and the new test helper currently ignores the spawned process exit code.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
f9e2fa4 to
dea591d
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are targeted, validated by a new regression test, and align with the stated goal of preserving actionable progress output during hangs.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
dea591d to
f5646f3
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, low-risk, and include targeted regression coverage validating the new stderr start-line behavior in both execution modes.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
f5646f3 to
06e0d3a
Compare
06e0d3a to
e04d739
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated deadlock regression test still depends on a fixed sleep window that can allow false-positive passes on slow/loaded runners (see stored review comment).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
e04d739 to
5c8c170
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, test-covered for the new “start line” behavior, and the deadlock fix is localized to test ordering/handshakes with documented rationale.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, include targeted regression coverage for the new logging behavior, and the concurrency fix is guarded by explicit synchronization to prevent the previously observed deadlock.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ped start lines, run_tests targets streamed through Ninja - and the first hang they named, the debug-agent threadlock test, fixed on a new try_invoke_in_context A mac Debug cell hung twice today in its interpreter sweep and the log ended in silence: `--failures-only` prints nothing for a passing file, and the sweep runs through `cmake --build --target run_tests_interpreter`, where Ninja holds a command's output until it finishes - a killed sweep loses everything it printed, including what would have named the hang. - `log::started` prints a timestamped line to stderr and flushes it at once, in every mode; stdout stays clean for `--bench-format json` and the isolated result protocol. The sequential sweep logs `run N/M: <file>` before each program; the isolated dispatcher logs `worker K starts: <files>` before each batch, and the batch's own child prints its `run` lines into the stderr the parent folds in on any abnormal exit. - Every `run_tests_*` custom target carries `USES_TERMINAL`, so Ninja runs it in the console pool and streams its output. - `tests/dastest/test_start_lines.das` runs dastest as a child in both modes and asserts the exit code and the start line. The lines named the hang on this branch's own JIT lane: tests/debug_agent/test_callback_threadlock.das. Its onTick spins until the main thread releases it, and the main thread waits for a second thread started by new_thread. The tick holds the agent registry for the whole callback, and a context clone walks that registry for onCreateContext, so when the tick reaches onTick before the second thread's clone the clone blocks, the release never comes, and the process deadlocks. A 16-core box always wins that race; a 3-core runner does not. The test wanted to assert "a pinvoke is blocked while the callback runs", and nothing in the language could observe that: `invoke_in_context` waits, so every shape of the test leaned on a second thread and a sleep window. New `try_invoke_in_context(ctx, "fn", args...) : bool` is the non-blocking form - `try_lock` on the context mutex, false with nothing run when another thread holds it - and the test proves the serialization from one thread: refused while onTick holds the lock, runs once the tick returned. No second thread, no window. The contract lands on DapiDebugAgent, in the daslib architecture doc, and as the one rule of tests/debug_agent/REVIEW.md: a hook never waits on a thread that has yet to create a context. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
5c8c170 to
c890341
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes concurrency-sensitive debugger cross-context invocation behavior in C++ and CI-critical test-runner output/streaming semantics, which warrants final human validation across platforms.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
Why. The mac Debug cells hung twice today in their interpreter sweep and the log ended in silence:
--failures-onlyprints nothing for a passing file, and the sweep runs throughcmake --build --target run_tests_interpreter, where Ninja holds a command's output until it finishes - a cancelled sweep loses everything it printed, including what would have named the hang. Once the lines existed they named it on this PR's own JIT lane:tests/debug_agent/test_callback_threadlock.das, whose hook waits on a thread that cannot start while the hook runs.What changes.
log::startedprints a timestamped line (iso8601_now()) to stderr and flushes it at once, in every mode; the sequential sweep logsrun N/M: <file>before each program, the isolated dispatcher logsworker K starts: <files>before each batch, and a batch's child prints its ownrunlines into the stderr the parent folds in on any abnormal exit, timeout included. stdout stays clean for--bench-format jsonand the isolated result protocol.run_tests_*custom target carriesUSES_TERMINAL, so Ninja runs it in the console pool and streams its output.g_DebugAgentMutex) for the whole callback, and every context construction walks that registry foronCreateContext, so anew_threadstarted while the hook blocks never begins; the main thread waited on it, the hook waited on the main thread. A 16-core box always won that race, a 3-core runner did not.invoke_in_contextwaits - so every shape of it leaned on a second thread and a sleep window. Newtry_invoke_in_context(ctx, "fn", args...) : bool(debugapi) is the non-blockinginvoke_in_context:try_lockon the context mutex,falsewith nothing run when another thread holds it. The test now proves the serialization from one thread: refused whileonTickholds the lock, runs once the tick returned. No second thread, no timing window.pinvoke_implandtry_pinvoke_implshare one core (pinvoke_named).DapiDebugAgentand indaslib/ARCHITECTURE.mdsection 24.1, andtests/debug_agent/REVIEW.md(new) binds it for the folder where agents with working hooks live.Observable behavior.
COMMENTline -> a log ending at<time> run N/M: <the file that hung>.--failures-onlysweep -> summary only -> one stderr line per program (the sequential mac Debug sweep grows by about 1,600 lines).test_callback_threadlock.dason a small runner -> hangs when the tick reaches the callback before the second thread's context clone -> no second thread exists to race.try_invoke_in_contexton a context another thread holds -> (new) returnsfalseat once; the same call on a free context runs the function and returnstrue.Where to look.
dastest/log.das(started), the two call sites indastest/dastest.das,tests/CMakeLists.txt,tests/dastest/test_start_lines.das,pinvoke_named/try_pinvoke_implinsrc/builtin/module_builtin_debugger.cpp, andtests/debug_agent/test_callback_threadlock.das.Validation, claims, ledger
Validation
tests/dastest/test_start_lines.dasruns dastest as a child in both modes with--failures-only, asserts the child's exit code and the start line naming the fixture; removing the sequential line fails it.dastest/tests(61) passes.enteredhangs on both tiers (alarm-killed at 60 s); the original ordering passes 40 of 40 on the 16-core box, which is why it never showed here.overlap == 0- and passes interpreted and 21 of 21 under JIT;tests/debug_agent(54) passes on the sharedpinvoke_namedcore.Uncategorized), the new handmade entry filled, sphinx-Wgreen.USES_TERMINALwas not reconfigured locally (the build dir belongs to another branch); CI's configure is the check.Not done
onTickafter releasing) would make a blocking hook legal; no agent in the tree needs one, so it stays a proposal.timeout-minuteson the build matrix job, so a hung cell fails inside the budget instead of holding a runner for hours.try_invoke_in_contexttakes a function name only; thefunction-pointer andlambdaforms ofinvoke_in_contexthave notry_twin yet.