test: fabricate HOME in the prelaunch-interrupt lease case - #5
Merged
Conversation
t48 launches the real implementer-loop to prove that an interrupt arriving before any companion job exists releases the lease instead of poisoning it. It did so without fabricating HOME, so what the loop found on disk depended on the host. On a developer machine the Codex companion is installed, workspace-writer enumeration returns an empty list, and the lease releases cleanly. On the CI runner no companion exists, that enumeration reports liveness unknown, and write_lock_release correctly refuses to release a lease it cannot prove quiescent — printing a message containing "lease retained", which the case greps for. The case failed on CI and passed everywhere it was authored. Give it the same fabricated HOME the rest of the suite uses: a fake companion at the canonical plugin path, a node forwarder built from REAL_NODE so the nvm shim is never invoked under the overridden HOME, and an empty status document. The git shim stays first on PATH so the digest window it opens is unaffected. All four assertions are unchanged. The interrupt path and the conservative retention in write_lock_release are both correct and untouched; only the test's environment was uncontrolled. Verified in both environments: 54 passed, 0 failed under an empty HOME (53/1 before this change) and under the developer HOME.
elkaix
added a commit
that referenced
this pull request
Aug 12, 2026
… suite (#6) CI failed on 37b53fd with two suites red, neither a product defect. The same content had passed as PR #5's check minutes earlier. t14_midpoint_warning_continues_same_job asked for a job to complete inside a window of (ceiling/2, ceiling) and then spent seven serialized poll cycles crossing it, each spawning a node process, against an 8-second ceiling. Every unit of runner slowness ate the margin until the loop cancelled at its deadline — behaving exactly as designed — and the case read that as failure. It had already failed this way on c2f2ea9 nine days earlier. Drive completion from the event instead of from the clock. The fixture's MAESTRO_TEST_JOB_TERMINAL_FLAG, which tests/job-lock.sh already uses, reports the job as running until a named file exists. The case now waits for the halfway warning, then creates that file, so completion is causally after the warning and one poll cycle away rather than seven. The ceiling lives in a single variable the assertion pattern derives from, so the two can no longer drift apart. All four assertions are unchanged. tests/shared-git-dir.sh re-ran the entire lease suite that tests/run.sh had already run and passed in the same job, piping it through `tail -1` so a failure inside it could not be diagnosed at all. It added no coverage, cost ~55s, and doubled the chance that any timing-sensitive lease case failed the build. Its other checks stay; the re-run goes. 42.87s to 2.07s. The remaining changes widen nine event waits from 5s to 30s across liveness, lease, and provenance-edge. These are preventive, not fixes for anything observed: each loop breaks as soon as its event arrives, so a larger bound costs nothing when the host is healthy and removes a false negative when it is not. A genuinely absent event still fails, only later. t19 is deliberately unchanged. It fails in the Codex sandbox because ps reports no process start identity there, so the contender fails closed instead of waiting; it passes on the GitHub runner and on developer hosts. Verified with the full suite under a fabricated empty HOME and /bin/bash 3.2, which is what the runner actually provides: 18 passed, 0 failed.
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
1 participant
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.
Fixes the CI failure on
main(run 31548589527):tests/lease.shcaset48_prelaunch_interrupt_releases_without_poison, which also tooktests/shared-git-dir.shdown with it because that suite re-runs the lease suite.One root cause, two red suites.
Root cause
t48launches the realimplementer-loop.shto prove review finding 4's guarantee —an interrupt arriving before any companion job exists releases the lease instead of
poisoning it. It did that without fabricating
HOME, so what the loop found ondisk depended on the host:
write_lock_workspace_writersreturns an empty list,
write_lock_releaseremoves the lock, the case passes.write_lock_releasecorrectly refuses to release a lease whose writer liveness itcannot determine, printing a message containing
lease retained. The case greps thewhole output for that substring and fails.
Reproduced deterministically by running the case under an empty
HOME:The interrupt path was never wrong — the case's
rc=4andMAESTRO_FINAL: LOOP INTERRUPTED rc=4assertions passed even on CI. The conservative retention inwrite_lock_releaseis also correct and deliberate: liveness unknown must neverrelease a lease. No product code is changed by this PR.
Fix
Give the case the fabricated
HOMEthe rest of the suite already uses (therun_clear_leasepattern): a fake companion symlinked at the canonical plugin path, anodeforwarder built fromREAL_NODEso the nvm shim is never invoked under anoverridden
HOME, and an empty status document. The existinggitshim stays first onPATH, so the digest window the case opens is unaffected. All four assertions areunchanged.
Verification
bash tests/lease.shpasses on a developer host with the bug present, so every checkwas run under a fabricated empty
HOMEas well — otherwise the verification cannot fail.HOME(emulates CI)HOMEshared-git-dir.shalso green under emptyHOME. Scope:tests/lease.shonly.Separately worth noting:
liveness.sh, which had keptmainred since c2f2ea9 onAug 2, passes on CI as of b3adebc — the write-path refactor fixed it.