fix(agent): Windows helper lifecycle durability (RDS accumulation)#2520
Merged
ToddHebebrand merged 47 commits intoJul 16, 2026
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
added 4 commits
July 14, 2026 22:16
Deploying breeze with
|
| Latest commit: |
6bcf403
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d4fd00e6.breeze-9te.pages.dev |
| Branch Preview URL: | https://toddhebebrand-helper-session.breeze-9te.pages.dev |
Add an OS-neutral, fakeable Windows recovery state machine behind windowsRecoveryBackend/watchedProcess so transition ordering is testable on any host without importing golang.org/x/sys/windows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the fail-closed forced-recovery stub with the verified state machine: validate -> terminate -> process exited -> (Stopped -> start | SCM already restarting -> observe) -> Running with a new live PID. Forced recovery never uses RecoveryRequest.StateFilePID for a destructive action. It re-queries the SCM PID, proves the image and liveness of the handle, and revalidates the PID immediately before the kill, so a recycled PID cannot get an arbitrary process terminated. Every uncertainty (config/query failure, open failure, image mismatch, PID drift, exit timeout, unsettled SCM, same-or-dead new PID) fails closed to a terminal failover disposition rather than looping on the next attempt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the placeholder Windows osServiceController with concrete adapters that satisfy windowsRecoveryBackend and watchedProcess, and wire the verified recovery state machine into NewRecoveryManager on Windows. Forced recovery is now reachable in production for the first time. The backend reads SCM state/PID via mgr.Service.Query and the configured image via mgr.Service.Config + DecomposeCommandLine/FullPath. Process handles are opened with OpenProcess and verified by QueryFullProcessImageName before TerminateProcess, so the state-file PID is never used for a destructive action. Unrecognized SCM states, out-of-range PIDs and unexpected wait results fail closed rather than being mapped onto a state the machine would act on. Waits poll WaitForSingleObject in 100ms slices so an SCM stop interrupts an in-flight exit wait, and handle operations serialize so Close cannot race an in-flight query or wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the bool-based recovery dispatch in the watchdog main loop with the structured RecoveryRequest/RecoveryResult flow from Tasks 1-4. - recoveryJournalFields() renders intent, action, disposition, phase, initial/final state, elapsed, failure class, and action-taken. State-file PID and old/new SCM PIDs stay three distinct keys: the state-file PID is a stale hint, the SCM PIDs are what the service manager reported. Conflating them would erase the evidence that recovery never acted destructively on the hint. - shouldVerifyRecovery() fails closed — only an error-free VerifyHeartbeat disposition enters heartbeat verification. Zero-value, errored, and unknown dispositions cannot read as "recovered". Terminal dispositions enter failover immediately via the existing EventRecoveryExhausted transition. - A runCtx cancelled from independent forwarder goroutines (signal or SCM stop, cause-tagged) threads through every recovery request, including the failover-dispatch helpers. An SCM stop now aborts in-flight recovery waits instead of holding the service in STOP_PENDING for the recovery deadline. - Failover commands map to explicit intents via failoverRecoveryIntent(), so start_agent can never select the ladder's forced attempt 2. Heartbeat-based post-restart verification remains the application-health success criterion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes slice 7's rollout-notes deliverable from the helper lifecycle durability design. Records one finding that contradicts the spec: the design assumes rollout "beginning with RDS hosts and a Windows workstation cohort", but no cohort or ring control exists. agent_versions.isLatest is a single global boolean per (version, platform, arch, component), so promotion is all-or-nothing. Staging has to be done by hand — install on chosen hosts, leave isLatest on the old version, observe, then promote. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…al check StopAcceptingAndWait closed unpublished connections while their handler goroutines were still running. handleConnection reads the raw pipe handle via ipc.GetPeerCredentials, and go-winio's Fd() reads win32File.handle with no synchronization while Close() writes it. -race on windows-latest caught this in TestNamedPipeListenAndAccept; it is intermittent, so it passed the previous run. The consequence is worse than a torn read. GetPeerCredentials feeds GetNamedPipeClientProcessId -> OpenProcess -> token -> SID, so a handle closed mid-call and reused by the OS for another object could attribute a connection to the wrong process. This is the authentication path. Cancel unpublished connections with a deadline in the past instead. It cancels pending IO immediately, never touches the handle, and leaves the handler as sole owner of closing the connection. A deadline, unlike a closed handle, can be overwritten: the handler armed its own handshake deadline right after the shutdown cancelled it, undoing the cancellation. So arming now happens under acceptMu via armHandshakeDeadline, which fails closed once acceptStopped is set. Without that ordering StopAcceptingAndWait stalls for a full HandshakeTimeout -- TestBrokerStopAcceptingAndWaitUnblocksStalledPreAuthConnection catches it (verified by mutation: dropping the check reproduces the stall). Introduced by d413bde on this branch; not present on main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces 'tracked separately' with the issue that records the root cause of each of the six failures, so the narrowed job list explains itself. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment in TerminateHelperKey referenced 'the follow-up noted in the remediation plan'; that follow-up is now filed as #2530 (bounded ownership retention for scheduled helpers). Comment-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ToddHebebrand
marked this pull request as ready for review
July 15, 2026 17:34
ToddHebebrand
marked this pull request as draft
July 15, 2026 18:29
…s denied Found by running this branch as the real BreezeAgent service on a Server 2022 RD Session Host with 6 concurrent RDP sessions (#2536). On a real terminal server every session's processes live in the session's own job, so the helper cannot join the agent's Job Object and AssignProcessToJobObject returns ERROR_ACCESS_DENIED. The previous code treated assignment as mandatory and fail-closed, so NO helper spawned for any RDS session and reconcile looped forever. This is a regression this branch introduced: main has no Job Object, so on main helpers spawn on RDS (then accumulate via the old SID-cap bug). Fix: - job.Assign is now best-effort. On denial the helper still spawns and is tracked/terminated via its process handle (logoff, shutdown, reconcile all terminate by handle). Only OS-enforced KILL_ON_JOB_CLOSE cleanup after an agent *crash* is lost; the single-instance guard + reconcile reclaim orphans. - helper creation now attempts CREATE_BREAKAWAY_FROM_JOB (with a no-flag fallback) to keep full job ownership where the environment allows it. - the spawn diagnostic records jobOwned so the field can be observed. Validated live on the RDS host: before, 3 helpers (console only), sessions 2-7 looping on job-assign-denied; after, 14 helpers (2 per session x 7 sessions), stable across reconciles, every SYSTEM helper admitted with a distinct session-scoped key windows:S-1-5-18:session:1..7 (12 of 14 via best-effort handle ownership, 2 with full job ownership). No accumulation, no rejections. Test: TestWindowsHelperSpawnerSpawnsWhenJobAssignDenied asserts a denied assign still spawns (resumed, not terminated, jobOwned=false) — the exact gap the old fakeHelperJob (Assign always nil) hid, so 20-session unit tests passed while a real RDS host failed. The former fail-closed-on-assign test is removed as it encoded the reversed contract; resume-failure fail-close is unchanged. Refs #2536 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ToddHebebrand
marked this pull request as ready for review
July 16, 2026 04:08
ToddHebebrand
deleted the
ToddHebebrand/helper-sessions-in-remote-desktop
branch
July 16, 2026 04:26
ToddHebebrand
added a commit
that referenced
this pull request
Jul 16, 2026
Technical-docs sweep for the **v0.96.0** release (range `v0.95.1..main`). Companion to the tagged release; no code changes, docs + docs-review mapping only. ## Pages updated - **TD SYNNEX AI tool** — `features/mcp-server.mdx`, `features/ai.mdx`, `features/distributor-integrations.mdx`: new `lookup_distributor_product` (Tier 2 / `ai:write`; live EC Express price+availability; reseller cost redacted for org-scoped callers, error+no outbound call when no partner in context) (#2559) - **M365 customer Graph-read host config** — `deploy/environment.mdx` new "Customer Microsoft 365 Graph-read consent (optional)" section: 9 env vars, **all required only when `M365_CUSTOMER_GRAPH_READ_ONBOARDING_ENABLED=true`** (dark by default); `deploy/production.mdx` optional `m365-graph-read-executor` sidecar note; `features/identity-integrations.mdx` pointer (#2495, #2511) - **Quotes** — `features/quotes.mdx`: cloning, change-customer-on-draft, per-table subtotals, customer bill-to "Prepared for" block, optional send message, click-to-zoom, Pax8-order-on-acceptance cross-link (#2535, #2584, #2549) - **Backups** — `backup/overview.mdx` (per-device "Run backup now", timezone-correct next-run, `breeze-backup` helper now bundled by shell-script installs), `backup/restoring.mdx` (System image badge, mode+mtime + system-image restore), `backup/verification.mdx` (integrity = size+SHA-256 vs manifest, real corruption detection) (#2558, #2564, #2581) - **Enrollment keys** — `agents/enrollment-keys.mdx`: required Site selector on create form (#2557) ## Mapping / tracker - `scripts/docs-review/mapping.json`: repoint pax8 → `distributor-integrations.mdx`; add `pax8Orders`, `pax8Order*`, `quoteToPax8Order`, `aiToolsCatalog`, `m365ControlPlane/**`, `runtimeConfig.ts`→environment, `m365CustomerGraphRead`/`m365ConsentCallback`, `apps/m365-graph-read-executor/**`, quote pdf/email services, `agent/scripts/install/**` - `scripts/docs-review/last-reviewed.json`: bumped to `20276393e` (2026-07-16) ## Verification notes - The M365 consent **callback URL derives from `PUBLIC_URL`** in the API; the `..._CALLBACK_URL` env var is consumed by the **executor app**, not the API — documented accordingly (not as dead config). - The `m365-graph-read-executor` sidecar is **published (GHCR) but not wired into the tracked compose files** — documented as such, not as a turnkey service. - `apps/docs` `astro build` passes. Intentionally **skipped** (no admin/integrator doc surface): versioned extension contract #2548 (developer-only SDK), Windows helper RDS lifecycle #2520 (internal agent fix), backup execution/result-recording internals #2554/#2556. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Todd Hebebrand <todd@lanternops.io> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
Windows agent + helper lifecycle durability. Implements slices 1–5 of
docs/superpowers/specs/2026-07-14-windows-agent-helper-lifecycle-durability-design.md, plus a remediation pass over that work.The problem (from the spec): Windows terminal servers accumulate far more
breeze-user-helper.exeprocesses than there are eligible sessions — dozens of SYSTEM-owned helpers on a server with ≤20 users. IPC admission was keyed only by SID with a 5-connection cap per identity; every SYSTEM helper sharesS-1-5-18, so a terminal server exhausts that bucket after five. Rejected helpers stay alive in a reconnect loop, are invisible to reconciliation, and get replacements spawned every 30s.The fix: admission is now keyed by Windows session and SID, so each RDS session gets an independent quota bucket; helpers are owned by a kill-on-close Job Object; and a single-instance guard enforces one full agent per machine.
test-agent-windows(required) will likely fail on first run. It is new on this branch and has never executed. 3 tests fail on real Windows for reasons that predate this branch (mainfails identically): two authenticate assystemrole but the test process isn't SYSTEM, andTestNamedPipeSessionDetectorasserts every WTS session has a username when session 0 legitimately has none. Needs a decision — see the plan doc.refactor(watchdog): return structured recovery outcomes). Its plan doc is committed; tasks 2–5 are unstarted.-raceonwindows-latestis unverified — it requires cgo.Remediation included in this branch
A full review of the original slice 1–5 work found 8 defects, each confirmed against source. Four of them independently reproduce the exact symptom the spec set out to fix — helper accumulation:
helperProcess.Alive()→(bool, error)GetExitCodeProcessfailure read as "dead", soreservehanded out a duplicate helper. The siblingownedPeerProcess.Alive()already returned an error — the two now match, and all four call sites fail closed.KillStaleHelperswas deleted with no replacement. It was precisely what killed the pre-auth-rejected helpers the spec describes.trackedHelper.launchedAtwas written twice and never read — the timeout hook existed but was never wired.WarnDebug; the default level isinfo, so a failed kill left no evidence at all while the broker forgot the helper and spawned another.Waitreturns(-1, err)on failure; recording-1as real marked a live helper exited, after which nothing ever terminated it.WTSEnumerateSessionsWfailure at boot left the named pipe never created for the process lifetime — no remote desktop, no PAM, no helper IPC — while the device kept heartbeating healthy. The fail-closed refusal is preserved; only the recovery is added."user"took the SYSTEM-token branch, including a zero-valueHelperKey. Unreachable today only by data-flow accident.mainhad 10 cases inTestRoleIdentityRejection; this branch had 7. Mutation testing confirmed 4 gates shipped green when deleted, including the session-0 sentinel from the #1009 fail-closed fix../internal/eventlog— its tests ran on no platform at all (the Linux job skips them via build tag). Nine other packages were also omitted.Verification
go test -race -count=1 ./...(agent, darwin): greengo build+go vet: clean on darwin andGOOS=windowsmainfails identically.Known gaps
HasHelperKeyOwnerdoesn't filter closed sessions and nothing clears the entry); it needs a bounded-retention design.Resolved since this PR was opened
WTS_REMOTE_CONNECT (0x3)reconnect is now handled (clearFatal+ reconcile on reconnect) — validated live on a real RDS host: disconnect → reconnect respawns the helper pair.docs/runbooks/windows-helper-lifecycle-rollout.md.Full detail and reasoning:
docs/superpowers/plans/2026-07-14-helper-lifecycle-review-remediation.md.Fixes
Closes #2536 — RDS: helpers fail to spawn on an RD Session Host (Job Object breakaway). Fixed and validated live on a 21-session RDS host.
Closes #2537 — Windows: multiple
breeze-agent.exeinstances can run at once. The single-instance guard is new on this branch; before/after reproduced on real Windows (mainruns 2, branch refuses the 2nd).Related follow-ups (not closed here): #2530 (bounded ownership retention), #2523 (Windows CI test debt), #2531, #2532.
🤖 Generated with Claude Code