Skip to content

Commit 2d82ddf

Browse files
committed
merge(dev): sync base for isolate hang fixes and Windows runner select
Pick up kill-grace keep-ref and OAuth queue CI fixes from tip; keep this PR's 30m Windows test ceiling and job-scoped timeout asserts with the new select-windows-runner job.
2 parents f27ed58 + 1ed2d70 commit 2d82ddf

8 files changed

Lines changed: 131 additions & 18 deletions

File tree

.github/workflows/ci.yml

Lines changed: 86 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,62 @@ concurrency:
4545
cancel-in-progress: true
4646

4747
jobs:
48+
# Which Windows runner this run is allowed to use.
49+
#
50+
# The self-hosted Windows box is a maintainer's PERSONAL machine on a home
51+
# network, reachable over Tailscale. Anything that runs on it runs as a local
52+
# user with that user's filesystem and LAN in reach, so the only question that
53+
# matters is whether the commit is trusted BEFORE a runner ever sees it.
54+
#
55+
# `push` on dev/main/preview is trusted: reaching those branches requires the
56+
# push permission, which only the maintainers in MAINTAINERS.md hold.
57+
# `pull_request` is NOT trusted, and no author check can make it so — the
58+
# workflow file itself comes from the PR head, so any `if:` guard written here
59+
# is deleted by the same patch it is meant to stop. Fork PRs therefore stay on
60+
# GitHub-hosted runners, unconditionally.
61+
#
62+
# `workflow_dispatch` is the maintainer escape hatch: dispatch requires write
63+
# access, so a maintainer can deliberately aim a branch at the home box.
64+
select-windows-runner:
65+
name: select windows runner
66+
runs-on: ubuntu-latest
67+
timeout-minutes: 2
68+
outputs:
69+
runner: ${{ steps.pick.outputs.runner }}
70+
label: ${{ steps.pick.outputs.label }}
71+
steps:
72+
- name: Pick runner
73+
id: pick
74+
env:
75+
# Read through env rather than interpolating directly into the script:
76+
# `github.event_name` is a fixed vocabulary, but keeping the habit means
77+
# no future edit here can grow a script-injection sink.
78+
EVENT_NAME: ${{ github.event_name }}
79+
USE_SELF_HOSTED: ${{ vars.OCX_SELF_HOSTED_WINDOWS }}
80+
shell: bash
81+
run: |
82+
set -euo pipefail
83+
trusted=no
84+
case "$EVENT_NAME" in
85+
push|workflow_dispatch) trusted=yes ;;
86+
esac
87+
88+
# Repository variable OCX_SELF_HOSTED_WINDOWS acts as a kill switch. When
89+
# it is anything other than `1` — including unset, which is the state
90+
# before the runner exists — every job falls back to windows-latest and
91+
# CI keeps working with the box powered off.
92+
if [ "$trusted" = "yes" ] && [ "${USE_SELF_HOSTED:-}" = "1" ]; then
93+
echo 'runner=["self-hosted","Windows","X64","ocx-home"]' >> "$GITHUB_OUTPUT"
94+
echo 'label=self-hosted (ocx-home)' >> "$GITHUB_OUTPUT"
95+
else
96+
echo 'runner="windows-latest"' >> "$GITHUB_OUTPUT"
97+
echo 'label=windows-latest' >> "$GITHUB_OUTPUT"
98+
fi
99+
48100
test:
49-
name: ${{ matrix.os }}
50-
runs-on: ${{ matrix.os }}
101+
name: ${{ matrix.name }}
102+
needs: select-windows-runner
103+
runs-on: ${{ matrix.runner }}
51104
# Windows dominates this matrix: on run 30459554635 the same suite took
52105
# ubuntu 4.6min / macos 5.6min / windows 11.8min. Against the previous
53106
# 12-minute ceiling that left ~12s of headroom, so runner variance decided
@@ -64,8 +117,34 @@ jobs:
64117
strategy:
65118
fail-fast: false
66119
matrix:
67-
os: [ubuntu-latest, windows-latest, macos-latest]
120+
# `name` is spelled out rather than derived from `runner`: a runner given
121+
# as a label array renders as "self-hosted Windows X64 ocx-home", so the
122+
# check name would change with the routing and break any branch
123+
# protection rule that names it. Keeping `windows` fixed means the
124+
# required check stays the same whichever machine served it.
125+
include:
126+
- name: ubuntu
127+
runner: ubuntu-latest
128+
- name: windows
129+
runner: ${{ fromJSON(needs.select-windows-runner.outputs.runner) }}
130+
- name: macos
131+
runner: macos-latest
68132
steps:
133+
- name: Show selected runner
134+
if: matrix.name == 'windows'
135+
shell: bash
136+
run: echo "windows leg on ${{ needs.select-windows-runner.outputs.label }}"
137+
138+
# A self-hosted runner keeps its working directory between jobs. Without an
139+
# explicit wipe, a file deleted in the commit under test survives on disk
140+
# and the suite passes against a tree that no longer exists in git.
141+
# `--ephemeral` registration de-registers the runner after each job but does
142+
# not clean the workspace, so this step is what makes the checkout honest.
143+
- name: Clean workspace (self-hosted only)
144+
if: runner.environment == 'self-hosted'
145+
shell: bash
146+
run: git clean -xffd . || true
147+
69148
- name: Checkout
70149
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
71150

@@ -115,6 +194,10 @@ jobs:
115194
strategy:
116195
fail-fast: false
117196
matrix:
197+
# Deliberately NOT routed to the self-hosted box. This job runs
198+
# `npm install -g`, which writes into the machine's global prefix and
199+
# would leave an `ocx` on a maintainer's personal PATH. It is an
200+
# 8-minute job, so there is nothing to win by moving it.
118201
os: [ubuntu-latest, windows-latest, macos-latest]
119202
steps:
120203
- name: Checkout

src/adapters/cursor/native-exec-shell.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,12 @@ function waitForBackgroundShellClose(entry: BackgroundShellEntry): Promise<boole
340340
settled = true;
341341
resolveWait(false);
342342
}, CURSOR_BACKGROUND_SHELL_TERM_GRACE_MS);
343-
unrefTimer(timer);
343+
// Deliberately REF'D: this is the bounded kill-grace wait that shutdown
344+
// drain awaits. Bun on Windows can starve unref'd timers when a pending
345+
// promise is the only other work, which would leave drainAndShutdown
346+
// waiting on this resolution forever. The timer self-clears within the
347+
// 2-second grace window (or earlier on close), so a ref cannot keep the
348+
// process alive beyond that bound.
344349
void entry.closePromise.then(() => {
345350
if (settled) return;
346351
settled = true;

src/config.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
hardenSecretDir,
1919
hardenSecretPath,
2020
hardenSecretPathAsync,
21+
windowsSecretAclApplies,
2122
} from "./lib/windows-secret-acl";
2223
import { recordOwnedConfigPath } from "./lib/config-ownership";
2324
import { assertNotRealHomeUnderTest } from "./lib/test-home-guard";
@@ -1506,6 +1507,7 @@ export function readConfigDiagnostics(): ConfigDiagnostics {
15061507

15071508
const CONFIG_MUTATION_DB_FILENAME = "config-mutation.sqlite";
15081509
const CONFIG_MUTATION_DB_SIDECARS = ["-journal", "-wal", "-shm"] as const;
1510+
let warnedConfigMutationDirectoryAcl = false;
15091511

15101512
export class ConfigMutationLockError extends Error {
15111513
readonly code = "CONFIG_MUTATION_LOCK_UNAVAILABLE";
@@ -1526,8 +1528,18 @@ function configMutationDatabasePath(): string {
15261528
} else {
15271529
try { chmodSync(dir, 0o700); } catch { /* best-effort on existing dir */ }
15281530
}
1529-
if (process.platform === "win32") {
1530-
hardenSecretDir(dir, { required: true });
1531+
if (windowsSecretAclApplies()) {
1532+
try {
1533+
hardenSecretDir(dir, { required: true });
1534+
} catch (error) {
1535+
if (!warnedConfigMutationDirectoryAcl) {
1536+
warnedConfigMutationDirectoryAcl = true;
1537+
const diagnostics = error instanceof Error ? error.message : "ACL hardening failed";
1538+
console.warn(
1539+
`[opencodex] Config mutation coordination directory ACL hardening did not complete; continuing without it. ${diagnostics}`,
1540+
);
1541+
}
1542+
}
15311543
}
15321544
const path = join(dir, CONFIG_MUTATION_DB_FILENAME);
15331545
recordOwnedConfigPath(dir, path);

src/lib/windows-secret-acl.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,11 @@ function effectivePlatform(): string {
182182
return platformOverride ?? platform;
183183
}
184184

185+
/** Test-aware platform predicate for callers that must avoid even a no-op ACL call. */
186+
export function windowsSecretAclApplies(): boolean {
187+
return (platformOverride ?? process.platform) === "win32";
188+
}
189+
185190
/** Error carrying an honest code: ETIMEDOUT only for real timeouts, EICACLS otherwise. */
186191
function icaclsError(step: string, result: IcaclsResult): NodeJS.ErrnoException {
187192
const err = new Error(

src/responses/spill-store.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,13 @@ function record(event: "write" | "fsync" | "close" | "harden" | "publish" | "dir
8282

8383
function fsyncDirectoryBestEffort(dir: string): void {
8484
let fd: number | null = null;
85+
// Record the durability attempt before opening the directory: Windows cannot
86+
// open/fsync directory handles this way, but callers still cross this seam.
87+
record("dir-fsync");
8588
try {
8689
fd = openSync(dir, "r");
8790
if (spillIoForTest?.fsync) spillIoForTest.fsync(fd);
8891
else fsyncSync(fd);
89-
record("dir-fsync");
9092
} catch {
9193
// Windows and some filesystems do not support fsync on directory handles.
9294
} finally {

tests/ci-workflows.test.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,15 @@ describe("GitHub Actions hardening", () => {
3434
jobs?: Record<string, { "timeout-minutes"?: number } | undefined>;
3535
};
3636

37-
// Job-scoped: a global count of "30" and "8" still passes if the values are
38-
// swapped between `test` and `npm-global-smoke`. Pin ownership explicitly.
39-
// Do not raise `test` again — hung tests (e.g. unref'd oauth waitMs) must
40-
// be fixed, not absorbed by a larger ceiling.
37+
// Job-scoped: a global count still passes if values are swapped between jobs.
38+
// Pin ownership explicitly. `test` is 30m for Windows isolate margin on #827
39+
// after state-store admission — do not raise again; fix hung tests instead
40+
// (unref'd oauth waitMs / shell kill-grace). Selector stays at 2; smoke at 8.
41+
expect(ci.jobs?.["select-windows-runner"]?.["timeout-minutes"]).toBe(2);
4142
expect(ci.jobs?.test?.["timeout-minutes"]).toBe(30);
4243
expect(ci.jobs?.["npm-global-smoke"]?.["timeout-minutes"]).toBe(8);
43-
// Both jobs must stay bounded — an unbounded job can hang a queue for hours.
44-
expect(count(workflow, "timeout-minutes:")).toBe(2);
44+
// Every job must stay bounded — an unbounded job can hang a queue for hours.
45+
expect(count(workflow, "timeout-minutes:")).toBe(3);
4546
expect(workflow).toContain("actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0");
4647
expect(workflow).toContain("oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6");
4748
expect(workflow).toContain("actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e");

tests/config.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1619,15 +1619,16 @@ describe("config.ts – Windows ACL hardening integration", () => {
16191619
}
16201620
});
16211621

1622-
test("saveConfig throws when hardenSecretDir fails in required mode on win32", () => {
1622+
test("saveConfig degrades when config-mutation directory hardening fails on win32", () => {
16231623
const origPlatform = process.platform;
16241624
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
16251625
try {
16261626
const spy = spyOn(windowsAcl, "hardenSecretDir").mockImplementation((_path, opts) => {
16271627
if (opts?.required) throw new Error("ACL hardening failed: access denied");
16281628
return { ok: true };
16291629
});
1630-
expect(() => saveConfig(getDefaultConfig())).toThrow(/ACL/i);
1630+
expect(() => saveConfig(getDefaultConfig())).not.toThrow();
1631+
expect(existsSync(getConfigPath())).toBe(true);
16311632
spy.mockRestore();
16321633
} finally {
16331634
Object.defineProperty(process, "platform", { value: origPlatform, configurable: true });

tests/shutdown-drain.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,13 @@ describe("background shell shutdown drain", () => {
119119
const unresolvedChild = installShutdownShell();
120120
setBackgroundShellRuntimeForTests({
121121
setTimer(callback) {
122-
const timer = setTimeout(callback, 0);
123-
timer.unref?.();
124-
return timer;
122+
// Keep this fixture timer REF'D: Bun on Windows can stop servicing
123+
// unref'd timers while the test's only pending work is a promise,
124+
// which left drainAndShutdown waiting forever and hung the isolate
125+
// process until the 20-minute CI job timeout (same starvation the
126+
// OAuth queue tests hit). A ref'd 0ms timer fires immediately and
127+
// cannot keep the process alive.
128+
return setTimeout(callback, 0);
125129
},
126130
});
127131
const unresolvedServer = fakeServer();

0 commit comments

Comments
 (0)