fix(omo-senpi): prevent orphaned memory test fixtures - #7479
Merged
Conversation
…essions - hold-lock fixture: destroy stdin on every ownership-loss path so a control-channel exit can actually terminate the process - hold-lock lifecycle tests: pin the control-channel exit with the holder's own exit marker (fails against origin/dev's fixture), and observe the ORIGINAL holder spawned by the abruptly killed wrapper through its pid file + exit marker instead of a substitute probe - model-preflight grandchild test: assert the original wrapper is dead, the original grandchild is alive past degradation, and no helper survives control-channel teardown (pidTerminalWithin boolean asserted) - process-liveness: close the /proc watcher TOCTOU window (watch() creation can throw ENOENT after a successful liveness probe), add readPidFileWhenWritten for liveness-independent pid capture, drop the unused waitForFileEvent
- hold-lock abrupt-wrapper test: the exit marker precedes the process's actual exit, so termination is now awaited through captureIdentity + pidTerminalWithin (event-first on linux, bounded probe elsewhere) instead of a point-in-time pidAlive probe that raced on ubuntu CI - model-preflight grandchild test: explicit 30s test budget; the bun default 5s does not cover wrapper+grandchild spawn costs on slower CI runners (the probe/outer bounds stay tight)
fs.watch does not reliably deliver file-creation events on every platform (observed: marker written 5ms after the trigger, zero watch events fired), which made the lifecycle waits consume their full bound and produced the 5s ubuntu CI timeout. All foreign-process/file waits now go through probeUntil (bounded, condition-checked, never an ordering oracle): - readPidFileWhenWritten / readPidWhenWritten rewritten on probeUntil - waitForFileToExist added; the control-channel test now uses the child's own exit event as the terminal oracle with the marker as a point-in-time corroboration - abrupt-wrapper fail-safe kills through the early identity snapshot (killIfAlive) instead of a raw numeric pid - preflight grandchild test shares readPidFileWhenWritten; unused watch/dirname/basename imports dropped
…down The grandchild writes its pid file before connect(), so the teardown snapshot of accepted sockets could miss its connection on slower runners (the ubuntu CI failure at pidTerminalWithin). The grandchild now writes an explicit connected marker once its control socket is established, and the test awaits that marker before closing the channel - the teardown deterministically terminates the original pipe-holding grandchild.
Watching /proc/<pid> cannot detect termination: a zombie keeps its /proc entry present and procfs delivers no reliable fs events, so the watcher slept through the death it waited for (the ubuntu CI failure at the grandchild termination assertion). The non-linux branch was a bare full-duration sleep with a single probe - a fixed-sleep oracle. pidTerminalWithin now rides probeUntil with the zombie-aware pidAlive (zombie detection via /proc stat state, ESRCH via signal probe) and identityMatches on every platform; a reused pid reads as terminal because its command no longer matches.
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
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.
Root cause
Memory lock fixtures and the model-preflight pipe-holder regression harness could outlive an abruptly terminated test parent. The lock fixture's lifetime was not fully event-driven, and the preflight wrapper/grandchild used forever-running intervals. Facts-test teardown also removed temporary roots without awaiting tracked child termination. Numeric PID cleanup could signal a reused, unrelated process.
Changes
hold-lock.tsto the parent-owned stdin channel (end/close/error) with no polling interval.Test evidence
bunx tsgo --noEmit -p packages/omo-senpi/tsconfig.json: exit 0.git diff --check: pass.bun run test:senpiwas started and produced extensive passing output but exceeded the 300s execution cap before completion; not claimed as passing.origin/devconfirmed the old model-preflight interval harness remained present; the borrowed stdin fixture regression is not a valid RED discriminator under Bun/macOS because Bun closes inherited stdin when the wrapper exits.Supersedes #7342.
Summary by cubic
Prevents
omo-senpimemory lock fixtures and preflight pipe-holder helpers from outliving an abruptly terminated test parent, which previously stranded orphaned processes holding lock markers near 100% CPU.hold-lock.tsexits when its parent-owned stdin channel reports end, close, or error, replacing the PPID polling watchdog, and destroys stdin on every ownership-loss path so the process can terminate.pidTerminalWithinrides the same bounded probe on every platform because a zombie keeps its/procentry and can sleep through a watcher.probeUntil) becausefs.watchfile-creation events are unreliable on some platforms and caused CI timeouts.Written for commit a9f210d. Summary will update on new commits.