testsoon: drain both cross-thread callbacks before the test returns

[coreyleavitt]

The cross-thread callSoon test added in #694 has a race that leaves a callback queued after the test returns, carrying a pointer into the test's dead stack frame. On linux-i386 CI this aborts testall with *** stack smashing detected *** at the start of the timers suite, at a rate of roughly one run in ten. On other targets the same write lands silently.

The test takes addr of a stack-local flag, spawns a thread that posts two cross-thread callbacks with that pointer, and calls poll() once, without joining the thread first:

• The first push sets the dispatcher's waking flag and wakes the loop; the second push sees the flag still set and skips the wake, as designed.
• processThreadCallbacks clears waking and then drains the MPSC queue. If the drain lands between the two pushes, it moves only the first callback; the second push then re-arms the wake and stays queued for the next poll cycle.
• The single poll() runs callback one, the flag reads true, the check passes, and the test returns. The frame that owned the flag is gone.
• The next poll() anywhere in the process fires the leftover callback, which writes true through the dangling pointer.

In testall, the next poll is the timers suite's first waitFor. On linux-i386 the written byte lands on that waitFor frame's stack canary, which is what the CI failures show: stack smashing detected, SIGABRT, traceback ending at testtime.nim testTimer, always at timers-suite entry, on both Nim 1.6 and devel.

Diagnosis is from a core dump of the faithful CI build (nimble test first config plus -g, no other changes, no debugger attached, looped on a CI runner until it crashed): the canary slot held 0x59014c00 against a per-thread canary of 0x59104c00 read intact from neighboring frames, a single byte changed from 0x10 to 0x01 at canary offset 2, with the adjacent frame-trace record untouched. A one-byte 0x01 store through a stale pointer is exactly flag = true.

Whether the stray write crashes depends on where the byte lands, which varies with binary layout, so the crash itself comes and goes across otherwise-unrelated commits. Instrumenting the test settles the question directly: resetting the flag after the existing check and giving the loop one more spin before the frame dies shows the second callback stranded in 19 of 500 runs of the otherwise unmodified test at master (b71392a), on x86_64. The race is in the test as written; linux-i386 is merely where the consequence is visible.

The fix makes the test deterministic: count the callbacks instead of latching a bool, join the thread before polling so both posts are visible to the drain, and poll until both have run. Nothing queued can then outlive the frame it points into, and the test now fails loudly if either callback is dropped, which the old bool could not detect. Verified with 40 consecutive clean runs of the standalone binary and 120 clean iterations of the previously-crashing linux-i386 CI loop on the fixed commit (https://github.com/coreyleavitt/chronos/actions/runs/31351631105); the same loop on unfixed commits crashes within 8 to 55 iterations.

Found while running CI for #702.

[arnetheduck]

Ah, thanks for the patch - looking at the code I think I intended to put only one item on the queue and the second is a fat finger but hey, we can go with 2 as well now that it works ;)

[arnetheduck]

the thread join shouldn't matter really, ie callSoon is executed independently of what's going on on the other thread.. what might happen however is that two callSoons get coalesced into a single poll handling them both - it's undefined how many wake-ups there are - so the while loop is correct but the comment is slightly misleading

[coreyleavitt]

Yeah you're right, and the join placement was leftover caution, I moved it below the check.

That also broadens what the test can exercise: with the join first, both posts were always queued before the first poll, so the drain was always the batched case. With polling overlapping the thread, either interleaving can occur - both drained in one poll, or arriving separately. Comment reworded around the actual invariant: polls and callbacks don't map 1:1, so the loop counts completions, and nothing may stay queued past this frame

[coreyleavitt]

Ah, thanks for the patch - looking at the code I think I intended to put only one item on the queue and the second is a fat finger but hey, we can go with 2 as well now that it works ;)

Happy accident then - the second post is what surfaced the drain race at all, and under the count it doubles as coverage for the coalesced case