fix(nests-interop): audit-driven cleanup of trace harness + hot-swap kdoc
Self-audit of commits `d7f87971` (trace capture) and `f8dc9c59`
(hot-swap soft-pass revert) caught four issues; this commit
addresses them.
`NativeMoqRelayHarness.kt`:
- `ProcessOutputDrainer.start`: tolerate `bufferedWriter()`
failures so a misconfigured trace-log dir (parent gone, disk
full, etc.) doesn't kill the drain thread and deadlock the
relay subprocess on a full stdout pipe (~64 KB Linux pipe
buffer). Fall back to ring-only capture and System.err-warn,
matching the pattern the relay-startup error handler already
uses.
- Hoist `Regex("[^A-Za-z0-9._-]")` to `tagSanitiser` so we don't
recompile per relay boot.
- Rename `LOG_TIMESTAMP_FMT` → `logTimestampFmt` (it's a runtime
`val`, not a `const val`; existing convention is camelCase for
runtime, SCREAMING_SNAKE for compile-time constants like
`PORT_READY_TIMEOUT_MS`).
`BrowserInteropTest.kt`:
- `chromium_listener_speaker_hot_swap_does_not_crash`: prune the
`pcm.size <= warmupSamples` early-return + the trailing comment
about the skipped FFT. After the soft-pass revert the
post-warmup branch had no assertions, so the early-return was
dead code. Reduce to a single decoderErrors assertion + kdoc
spelling out the soft-pass and pointing at the hang-tier T12
counterpart.
- Update the kdoc to reflect what the test ACTUALLY asserts (it
used to claim FFT-peak coverage that no longer applies).
Other audit findings deferred (not real bugs, low priority):
- `ConcurrentLinkedQueue.size()` O(n) per line in the drainer.
- Per-line `writer.flush()` syscall — kept intentionally for
hung-test post-mortem; documented inline.
- BrowserInteropTest at 1140+ lines could split helpers — pre-
existing situation, not a regression.
This commit is contained in:
Claude
+20
-47
@@ -463,11 +463,26 @@ class BrowserInteropTest {
|
||||
* session stays alive throughout (hot-swap is speaker-side only;
|
||||
* the listener's session is independent), and because the
|
||||
* speaker re-publishes the same broadcast suffix the page sees
|
||||
* `Announce::Ended → Active` and stays subscribed.
|
||||
* `Announce::Ended → Active`.
|
||||
*
|
||||
* Mirror of the hang-tier `speaker_hot_swap_does_not_crash`.
|
||||
* Asserts the FFT peak survives — group-sequence corruption
|
||||
* across the swap (regression on T12) would shift it.
|
||||
* **Soft assertion only.** Empirical post-`:quic`-merge sample
|
||||
* counts vary 0–7.7 k samples (≤ 160 ms TOTAL) — Chromium's
|
||||
* `@moq/lite` 0.2.x client doesn't re-attach across the
|
||||
* `Active::Ended → Active` boundary, so any hard sample-count
|
||||
* floor would fail-flake. T12 (group sequence carry across
|
||||
* hot-swaps) is hard-asserted by the hang-tier counterpart
|
||||
* `speaker_hot_swap_does_not_crash` in [HangInteropTest], which
|
||||
* decodes the full post-swap window with the 440 Hz peak
|
||||
* intact. The browser tier here only verifies:
|
||||
* - the harness boots and the publisher hot-swap completes
|
||||
* without crashing the test process,
|
||||
* - Chromium's WebCodecs `AudioDecoder` doesn't fire a
|
||||
* `decoderErrors > 0` event during the swap window.
|
||||
*
|
||||
* Tracked as the "browser hot-swap re-attach" deferred item in
|
||||
* `nestsClient/plans/2026-05-06-cross-stack-interop-test-results.md`.
|
||||
* Promote to a hard floor once `@moq/lite` / `@moq/hang` gain
|
||||
* a working subscribe-re-attach path.
|
||||
*/
|
||||
@Test
|
||||
fun chromium_listener_speaker_hot_swap_does_not_crash() =
|
||||
@@ -483,49 +498,7 @@ class BrowserInteropTest {
|
||||
"decoderErrors=$errors during hot-swap — expected 0.\n" +
|
||||
"playwright stdout:\n${out.stdout}",
|
||||
)
|
||||
val pcm = readFloat32Pcm(out.pcmFile)
|
||||
val warmupSamples = AudioFormat.SAMPLE_RATE_HZ / 10
|
||||
// SOFT-PASS — kept intentionally on this one scenario.
|
||||
//
|
||||
// Empirical post-`:quic`-merge sample counts vary from
|
||||
// 0 to ~7.7 k samples (≤ 160 ms TOTAL) across sweeps,
|
||||
// straddling the 4.8 k warmupSamples threshold. The
|
||||
// browser path's hot-swap response is fundamentally
|
||||
// unreliable: Chromium's `@moq/lite` 0.2.x client tears
|
||||
// down its catalog/audio subscriptions when it sees
|
||||
// `Announce::Ended → Active` in rapid succession
|
||||
// (T+2.5 s + ~ms swap window) instead of re-attaching
|
||||
// to the new broadcast cycle. ANY hard floor on
|
||||
// `pcm.size` that excludes the regression mode
|
||||
// ("swap crashed the WT session entirely") fail-flakes
|
||||
// because the steady state itself sits at the warmup
|
||||
// window edge.
|
||||
//
|
||||
// The hang-tier counterpart (`speaker_hot_swap_does_not_crash`
|
||||
// in HangInteropTest) hard-asserts the full post-swap
|
||||
// window decodes the 440 Hz peak — that's the place
|
||||
// T12 (group sequence carry across hot-swaps)
|
||||
// regressions are caught. The browser tier here only
|
||||
// verifies the harness boots, the publisher hot-swaps
|
||||
// without crashing the test process, and Chromium
|
||||
// doesn't fire a decoder error.
|
||||
//
|
||||
// Tracked in
|
||||
// `2026-05-06-cross-stack-interop-test-results.md`'s
|
||||
// "browser hot-swap re-attach" deferred item. Resolution
|
||||
// requires a fix in `@moq/lite` / `@moq/hang`'s subscribe
|
||||
// re-attach path — out of scope for this branch.
|
||||
if (pcm.size <= warmupSamples) {
|
||||
System.err.println(
|
||||
"Browser hot-swap: captured ${pcm.size} samples (≤ warmupSamples). " +
|
||||
"Soft-pass per documented browser-side @moq/lite re-attach " +
|
||||
"limitation; T12 is asserted by the hang-tier counterpart.",
|
||||
)
|
||||
return@runBlocking
|
||||
}
|
||||
// If we DID get past warmup, still skip the FFT — the
|
||||
// post-warmup window may be too short (60 ms typical) to
|
||||
// resolve a 440 Hz peak with halfWindowHz=5.
|
||||
// No PCM sample-count or FFT assertion — see kdoc.
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+33
-4
@@ -142,8 +142,9 @@ class NativeMoqRelayHarness private constructor(
|
||||
|
||||
/** Monotonic counter used to disambiguate same-tag boots in one JVM. */
|
||||
private val bootSequence = AtomicInteger(0)
|
||||
private val LOG_TIMESTAMP_FMT =
|
||||
private val logTimestampFmt =
|
||||
DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss-SSS")
|
||||
private val tagSanitiser = Regex("[^A-Za-z0-9._-]")
|
||||
|
||||
fun isEnabled(): Boolean = System.getProperty(ENABLE_PROPERTY) == "true"
|
||||
|
||||
@@ -253,7 +254,7 @@ class NativeMoqRelayHarness private constructor(
|
||||
if (relayLogDir != null) {
|
||||
relayLogDir.mkdirs()
|
||||
val seq = bootSequence.incrementAndGet().toString().padStart(3, '0')
|
||||
val ts = LocalDateTime.now().format(LOG_TIMESTAMP_FMT)
|
||||
val ts = LocalDateTime.now().format(logTimestampFmt)
|
||||
val tag = sanitiseTag(testTag ?: "boot")
|
||||
File(relayLogDir, "$tag-$seq-$ts.log")
|
||||
} else {
|
||||
@@ -337,7 +338,7 @@ class NativeMoqRelayHarness private constructor(
|
||||
*/
|
||||
private fun sanitiseTag(raw: String): String =
|
||||
raw
|
||||
.replace(Regex("[^A-Za-z0-9._-]"), "_")
|
||||
.replace(tagSanitiser, "_")
|
||||
.take(80)
|
||||
.ifBlank { "boot" }
|
||||
|
||||
@@ -457,7 +458,30 @@ private class ProcessOutputDrainer(
|
||||
fun start() {
|
||||
thread =
|
||||
Thread({
|
||||
val writer = sinkFile?.bufferedWriter()
|
||||
// Open the sink file inside the drain thread + tolerant
|
||||
// of failure: if `bufferedWriter()` throws (parent dir
|
||||
// gone, disk full, file-already-a-directory, …) the
|
||||
// drainer must STILL pump the subprocess's
|
||||
// stdout/stderr — otherwise the relay's pipe buffer
|
||||
// fills (~64 KB on Linux) and the subprocess deadlocks
|
||||
// on its next write. Per-test runs are unattended; a
|
||||
// misconfigured log dir shouldn't take the whole
|
||||
// suite down.
|
||||
val writer: java.io.BufferedWriter? =
|
||||
if (sinkFile != null) {
|
||||
try {
|
||||
sinkFile.bufferedWriter()
|
||||
} catch (t: Throwable) {
|
||||
System.err.println(
|
||||
"NativeMoqRelayHarness-$name: failed to open trace log " +
|
||||
"$sinkFile (${t::class.simpleName}: ${t.message}); " +
|
||||
"continuing without per-test capture.",
|
||||
)
|
||||
null
|
||||
}
|
||||
} else {
|
||||
null
|
||||
}
|
||||
try {
|
||||
process.inputStream.bufferedReader().useLines { lines ->
|
||||
for (line in lines) {
|
||||
@@ -467,6 +491,11 @@ private class ProcessOutputDrainer(
|
||||
runCatching {
|
||||
writer.write(line)
|
||||
writer.newLine()
|
||||
// Per-line flush so a hung-test
|
||||
// post-mortem still has the latest
|
||||
// line on disk. Tests don't run
|
||||
// long enough for the syscall cost
|
||||
// to matter (max ~1k lines/test).
|
||||
writer.flush()
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user