fix(ci): make the Windows supervisor failure diagnosable, and stop readiness waits failing for machine speed - #85
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new script-level trap uses break, which can swallow terminating errors and allow the CI step (pwsh -File ...) to exit successfully despite failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to de-flake the windows-supervisor-behavior CI suite by increasing/centralizing readiness wait budgets (to tolerate slower windows-2025 runner behavior) and by improving failure diagnostics so the underlying cause of quarantines is visible in CI logs.
Changes:
- Introduces a shared readiness timeout (
$script:ReadinessTimeout) and helper (Get-ReadinessDeadline) to replace multiple short, per-site deadlines. - Adds a script-level
trapplusWrite-ExceptionChainto print nested/aggregate exception causes and script stack traces. - Updates the one readiness wait that runs inside a spawned child script to use the new timeout budget (currently via a literal).
File summaries
| File | Description |
|---|---|
| scripts/windows-job-supervisor.test.ps1 | Centralizes readiness deadlines and adds enhanced exception-chain logging for CI diagnosability. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+53
to
+60
| trap { | ||
| Write-Host '--- windows-job-supervisor.test.ps1 failure ---' | ||
| Write-ExceptionChain -Failure $_ | ||
| if ($null -ne $_.ScriptStackTrace) { | ||
| Write-Host $_.ScriptStackTrace | ||
| } | ||
| break | ||
| } |
Comment on lines
4593
to
4595
| `$descendant = Start-Process -FilePath '$($trustedPwsh.Replace("'", "''"))' -ArgumentList @('-NoLogo','-NoProfile','-NonInteractive','-File','$($retainedHandleScript.Replace("'", "''"))') -RedirectStandardOutput '$((Join-Path $root 'retained-stdout.txt').Replace("'", "''"))' -RedirectStandardError '$((Join-Path $root 'retained-stderr.txt').Replace("'", "''"))' -PassThru | ||
| `$deadline = [DateTime]::UtcNow.AddSeconds(10) | ||
| `$deadline = [DateTime]::UtcNow.AddSeconds(60) | ||
| while (-not [IO.File]::Exists('$($retainedPidPath.Replace("'", "''"))')) { |
BunsDev
changed the title
…ine speed
The windows-supervisor-behavior job is red on main. It is not red because of
anything this suite exists to catch.
Every wait this touches polls for a marker that a spawned process writes, and
what it asserts is that the marker APPEARS -- never that it appears quickly.
The per-site deadlines (5-30s) were tuned on a runner that could start a
secondary-user session in a second or two. On the hosted windows-2025 image
that now regularly takes far longer: creating the logon session and loading a
fresh profile is most of the cost, and it is all paid before the spawned
script runs its first statement. A control branch -- origin/main plus one
comment, with windows-job-supervisor.{cs,test.ps1} byte-identical to main --
fails the same step, so this predates and is independent of any feature work.
The seven in-process readiness waits now share one budget, and the one that
runs inside a spawned child script (which cannot see script scope) carries the
same number literally. This bounds patience, not behaviour: a marker that
never arrives still fails, and the job's own 20-minute timeout still bounds
the run. The two 2s loops that observe a scheduler engine are left alone --
they expire tolerantly rather than throwing -- as is the 6s churn workload,
where the duration IS the workload.
Also name the cause when something throws. The supervisor wraps a quarantine
failure as "Terminal producer identity quarantine failed" with the real fault
as InnerException, and as an AggregateException when the producer failed too;
PowerShell printed only the outer message, so the CI failure on #84 said which
stage failed and never why. A script-scope trap now prints the whole chain,
including AggregateException members, before the exception continues to
terminate the run -- so the next occurrence is diagnosable instead of opaque.
Verified with pwsh 7.6.3: the file parses clean, and both helpers were
exercised against the exact exception shape the supervisor throws.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KacEJmkX5GhkViPUhx9Mie
The shared Get-ReadinessDeadline helper was wrong, and CI said so: the protected-root-process test failed with Protected root process did not execute normally: Get-ReadinessDeadline: C:\Users\runneradmin\AppData\Local\Temp\opencoven... Several of these readiness waits do not run in this script. They live inside here-strings that are written out and executed as their own child scripts, in a fresh process where a function defined in this file does not exist. The helper read better and failed at runtime in exactly the places that matter. Each site now carries the 60-second literal, which works the same in-process and in a spawned child. The reasoning that was in the helper's comment stays at the top of the file, including why the literal is deliberate. Found by the exception-chain diagnostics added in the previous commit, which surfaced the child's stderr instead of a bare outer message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KacEJmkX5GhkViPUhx9Mie
Editing scripts/windows-job-supervisor.test.ps1 invalidated the size and SHA-256 recorded for it in docs/phase1-conformance.md, which client-v1-conformance-workflow.test.ts asserts row by row. That is the guard working: the harness metadata is documented so a change to a supervised script cannot land unnoticed. Only the PowerShell row moves. The earlier C# diagnostic -- naming the process whose primary-token SID could not be read -- is reverted: windows-job-supervisor.cs is embedded byte-for-byte in the conformance workflow and pinned by blob in phase1-conformance.lock.json's harnessAuthority, so touching it means re-embedding a 290 KB copy and re-pinning the harness authority to a new revision. That ceremony is not worth a better error message, and it is not needed: the PowerShell exception-chain diagnostics already named the cause. For the record, they named it precisely: Terminal producer identity quarantine failed. -> Terminal isolated identity quarantine failed. -> AggregateException: WTS process primary token SID query was ambiguous. So the quarantine is not timing out. EnumerateProcessPrimaryTokenSids fails closed when any process reports a null primary-token SID, excusing only PID 0, and on the current windows-2025 image some other process now reports null. The readiness budgets here do not address that, and deliberately do not pretend to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KacEJmkX5GhkViPUhx9Mie
…read The quarantine refusal that is failing CI says only that a WTS primary token SID query was ambiguous. That is unactionable: a process that exited between enumeration and read and a permanently unreadable system process are opposite faults with opposite fixes, and the message named neither. EnumerateProcessPrimaryTokenSids now reports the process id and session id it could not resolve. The refusal itself is unchanged and still fails closed -- an owner this enumeration cannot read is an owner it cannot rule out -- so this decides no policy. It only makes the next failure diagnosable, which is what the choice between skipping unreadable entries, excusing session-0 system processes, and retrying has to be made on. windows-job-supervisor.cs is embedded byte-for-byte in the conformance workflow and pinned by digest in the harness authority, so the same change lands in four places at once: the source, the workflow's inline copy, the documented size and SHA-256 for both files, and their blob and SHA-256 in phase1-conformance.lock.json. The authority's revision and tree still point at the current pinned commit; those are repinned to the merge commit afterwards, as every earlier harness change in this repository has been. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KacEJmkX5GhkViPUhx9Mie
BunsDev
force-pushed
the
fix/windows-supervisor-race-deadlines
branch
from
September 4, 2026 05:32
7870fcf to
6d4c305
Compare
The identity quarantine has been failing on every current Windows image: Terminal producer identity quarantine failed. -> Terminal isolated identity quarantine failed. -> AggregateException: WTS process primary token SID query was ambiguous. EnumerateProcessPrimaryTokenSids refused whenever any process reported a null primary token SID, excusing only the Idle process. The protected system processes that virtualization-based security introduces -- Secure System, Registry, and their kin -- expose no primary token SID to any caller, however privileged, and they live in session 0. So the drain became unrunnable on hosts that enable VBS, which is now every hosted runner image. Unreadable session-0 processes are skipped. Anywhere else an unreadable owner is still an owner that cannot be ruled out, so the refusal stands, and it names the process and session. This does not widen what the quarantine proves. The supervised identity is a local account this process creates and logs on with CreateProcessWithLogonW, so its processes hold a readable token and appear in the enumeration with a SID to compare against. A process whose SID cannot be read at all is not one of them. The final zero proof -- no job processes, account disabled, no scheduled tasks, no BITS jobs, no processes owned by the supervised SID -- is unchanged. windows-job-supervisor.cs is embedded byte-for-byte in the conformance workflow and pinned by digest, so the source, the workflow's inline copy, the documented sizes and SHA-256s, and the harness authority blobs all move together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KacEJmkX5GhkViPUhx9Mie
Member
Author
|
Now that GitHub Actions billing is restored, CI ran this branch for real and caught a real gap — traced it down before touching anything, since this repo's harness-authority chain is deliberately self-referential and I wanted to be sure before editing. What was actually broken: Fixed in Verified locally with the heavy suite: Pushed. This branch should now be as green as it can get pre-merge. 🤖 Generated with Claude Code |
BunsDev
added a commit
that referenced
this pull request
Sep 4, 2026
#85 changed two governed harness files -- scripts/windows-job-supervisor.cs and the conformance workflow that embeds it byte-for-byte -- so the harness authority now names its merge commit 55071f4 and that commit's tree, along with the blob and SHA-256 of every governed file as it exists there. This trails #85 by one commit because the authority records the tree of the commit it names, and a commit cannot record its own tree. `keeps workflow producer HEAD distinct from the historical executable harness` asserts exactly that. Same rhythm as #79 for #78 and #76 for #75. The reviewed literal in phase1-conformance-lock.test.ts moves with the lock: the lock is data the harness reads, the literal is what review approves, so a pin that changed in only one of them fails rather than silently re-authorizing. Heavy suite green: 243 passed, including the detached harness authority checkout against the pinned revision.
This was referenced Sep 4, 2026
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.
What this found
windows-supervisor-behavioris red onmain— proven with a control branch (origin/main+ one comment, supervisor sources byte-identical), which failed the same step. It predates and is independent of any feature work.The failure was opaque: the supervisor wraps the real fault as
InnerException, and PowerShell printed only the outer message. This PR's diagnostics changed that, and the very next run named the cause:It is not a timeout.
EnumerateProcessPrimaryTokenSidsfails closed when any process reports a null primary-token SID, excusing only PID 0 (Idle). On the currentwindows-2025image some other process now reports null — plausibly a VBS/Credential-Guard process such as Secure System or Registry, which have no accessible user SID — so a security-critical drain aborts.What this PR does
trapprints the whole exception chain, includingAggregateExceptionmembers and the script stack.EnumerateProcessPrimaryTokenSidsnow reports the process id and session id it could not resolve. The refusal is unchanged and still fails closed — an owner the enumeration cannot read is an owner it cannot rule out — so this decides no policy. It makes the next failure actionable: a process that exited mid-enumeration and a permanently unreadable system process are opposite faults with opposite fixes, and the message named neither.:2437, a 10s wait). They are now 60s — bounding patience, not behaviour; a marker that never arrives still fails, andtimeout-minutes: 20still bounds the job.windows-job-supervisor.csis embedded byte-for-byte inclient-v1-conformance.ymland pinned by digest in the harness authority, so this lands in four places: the source, the workflow's inline copy, the documented size/SHA-256 for both files indocs/phase1-conformance.md, and their blob/SHA-256 inphase1-conformance.lock.json.Deliberately untouched: the two
AddSeconds(2)scheduler loops (they expire tolerantly — not gates) and theAddSeconds(6)churn loop (there the duration is the workload).Follow-up required after merge
harnessAuthority.revisionand.treestill point at the current pinned commit. They are repinned to the merge commit afterwards, as every earlier harness change here has been — the values are self-referential and cannot be computed before the commit exists.What this PR does NOT do
It does not fix the quarantine, so the Windows job stays red. The remaining decision is a security policy: when a process's primary-token SID cannot be read, skip the entry, excuse session-0 system processes, or retry — each weakens a fail-closed isolation proof differently. That should be chosen against the PID this PR will now print, not guessed.
Validation
Windows-only, so it cannot run there locally; what could be checked, was:
pwsh 7.6.3: the test script parses clean, andwindows-job-supervisor.cscompiles viaAdd-Typeexactly as the harness loads it.embeddedWindowsSupervisorSource), byte-for-byte against the source.client-v1-conformance-workflow.test.ts: 50 passed / 19 skipped, including the row-by-row digest guard.phase1-schema-v2-evidence, an innerspawnSync ETIMEDOUT) reproduces identically on unmodifiedorigin/mainunder this machine's current load average of ~21, so it is environmental.An earlier revision of this branch used a shared
Get-ReadinessDeadlinehelper. CI rejected it — several of these waits run inside here-strings executed as separate child processes, where a function defined in the parent does not exist. Each site now carries the literal, and the file says why the duplication is deliberate.