Skip to content

fix(windows): make recovery evidence authoritative - #3789

Open
liugddx wants to merge 3 commits into
apache:mainfrom
liugddx:fix/windows-baseline-authority
Open

fix(windows): make recovery evidence authoritative#3789
liugddx wants to merge 3 commits into
apache:mainfrom
liugddx:fix/windows-baseline-authority

Conversation

@liugddx

@liugddx liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member

Summary

This PR advances #2624 and the Windows support plan in #2142 by making the native recovery check eligible for branch protection and removing two deterministic failure groups from the Windows baseline.

  • restores windows_recovery on every pull request and main push;
  • declares windows_recovery beside test in the ASF-managed required status checks and locks the workflow/config relationship with a CI contract;
  • makes marker publication use the live process fs.promises.open, so the Node 24 Windows initialization-race fixture reliably reaches marker_open_pending without relying on builtin ESM export synchronization;
  • replaces the unsupported fsutil file queryStreams call with one bounded Windows stream query fed by Node's no-reparse tree walk;
  • checks the dependency root, nested directories, and regular files through FindFirstStreamW / FindNextStreamW, including Win32 namespaced paths for long-path compatibility;
  • keeps every query/start/timeout/output/parse/API failure fail-closed.

Runtime PTY cleanup is intentionally excluded because another contributor has already claimed that slice on #2624. The remaining Git fixture cleanup and symlink-permission inventory are also not folded into this PR.

Why the ADS change is needed

fsutil file queryStreams is not a supported subcommand on the current developer Windows build or the GitHub hosted runner. Normal dependency trees therefore failed before their receipt could be verified. The previous ADS test matched the words "alternate data stream" in the query-failure diagnostic, so it passed without proving a named stream had been observed.

The replacement keeps traversal in Node, rejects reparse points before the external query, and sends a JSON array over stdin to one bounded Windows PowerShell 5.1 process. The process hosts the documented Win32 stream enumeration APIs because the PowerShell 5.1 provider does not enumerate directory streams. Only ERROR_HANDLE_EOF is accepted as no streams; other API and process failures reject the environment. Regressions now prove exact named-stream rejection for a regular file, the published dependency root, and a nested directory.

Verification

Local Windows, Node 24.19 for platform-sensitive tests:

  • managed dependency environment: 17 passed, 0 failed, 2 POSIX-only skips;
  • focused file/root/nested-directory ADS cases: 3 passed, 0 failed;
  • managed dependency crash recovery: 5 passed, 0 failed;
  • marker file: 7 passed, 0 failed;
  • focused root-authority concurrency selection: 4 passed, 0 failed;
  • CI planner and Windows harness: 76 passed, 0 failed, 1 privilege skip;
  • repository control-plane YAML contract: 1 passed, 0 failed;
  • npm run windows:inventory: current, 64 declarations;
  • lint, format, Core/Storage builds, and git diff --check: passed.

Hosted exact-head evidence:

  • windows_recovery: passed in 3m17s, including Local IPC, SQLite crash, Runtime continuation, Runtime Host owner-death, and managed workspace crash recovery;
  • test: passed in 17m23s, including all 67 Desktop e2e tests, Storybook smoke, and installed CLI validation.

The post-merge #3788/#3790 blockers were repaired by merged #3796. The overlapping schema-constructor handle fix is inherited from main and no longer appears in this PR diff.

Administration boundary

.asf.yaml is the repository's declarative branch-protection authority. After merge, ASF infrastructure must apply the updated main protection so windows_recovery is actually required. The workflow now reports that stable context on every PR and main push, so enabling it cannot leave unrelated PRs permanently pending.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool and scope: Codex traced recent Windows baseline artifacts, reproduced the system API and handle failures on Windows/Node 24, implemented the storage and workflow changes, and ran the listed local gates.

Refs #2624. Refs #2142.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, affected builds, Windows inventory, and focused Windows suites pass locally
  • Exact-head hosted windows_recovery passes on rebased head da660e47d
  • Exact-head hosted test passes on rebased head da660e47d

Does this PR entail a change in behavior?

  • Yes - windows_recovery returns to PR/main and becomes a required context; Windows storage validation now uses supported Win32 ADS enumeration for files and directories plus a portable marker-race interception boundary.
  • No

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on b1f348d66d:

Spec is PASS — recovery probing now uses live fs.promises.open with correct intercept and bounded ADS walk, correctly retrying outside cleanup.

However Standards remains NO-GO:

  • Both commits lack required Generated-by: <tool> trailer despite AI-authored changes (CONTRIBUTING.md:34).
  • PR template checklist/behavior Yes/No sections are removed, not filled.

Checks on b1f348d66 are test: failure / windows_recovery: failure (both due to queryTurn removal in execution-host-queue.test.ts, not this diff, but still BLOCKED) — not green.

简体中文实现通过,标准合规与 CI 门待补。

@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Current exact-head failures are upstream bootstrap blockers, not failures in this PR: #3790 prevents Runtime Host build and #3788 prevents message-admission startup. Both are repaired together in green PR #3796 because neither standalone fix can pass required CI before the other lands. After #3796 merges, this branch will rebase, drop the now-upstream schema-constructor overlap, and rerun the automatic windows_recovery check through its real recovery steps.

@liugddx
liugddx force-pushed the fix/windows-baseline-authority branch from b1f348d to 7957ebe Compare August 25, 2026 12:30
@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Standards follow-up addressed on new exact head \7957ebe6a: both commits now carry the required \Generated-by: Codex\ trailer, and the PR body restores the Checklist plus behavior Yes/No section. I agree the hosted gates remain blocked by current-main #3788/#3790; their combined repair #3796 is exact-head green and will be merged/rebased before this PR can claim hosted success.

@liugddx
liugddx force-pushed the fix/windows-baseline-authority branch from 7957ebe to 9047c2e Compare August 25, 2026 13:01
@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Rebased onto merged #3796 at new exact head \9047c2eb3. The overlapping schema-constructor handle patch is now upstream and absent from this diff. Rebase-local planner/harness (76 pass / 0 fail / 1 privilege skip), repository control-plane YAML, lint, format, inventory (64 declarations), Core/Storage builds, and diff check pass. Fresh hosted \ est\ and \windows_recovery\ are running; this head should now reach the actual native recovery steps.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this head and found no code blocking issues.

Recovery evidence probing now uses live fs.promises.open with intercept and bounded walk, correctly. No new abstraction.

However machine gate is NO-GO due to branch base drift: execution-host-queue.test.ts:248/249/271 still references removed queryTurn/startTurn/stopTurn — fixed in main by #3796 (c4971c9). Rebase to main ≥ c4971c9 and rerun CI.

Checks on 7957ebe04f are test/windows_recovery: failure due to base drift — not PR-introduced.

简体中文代码无阻断,基座过期需 rebase。

@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Base-drift follow-up is closed on exact head \9047c2eb3: the PR base is merged #3796 commit \c4971c938, the stale queue aliases and migration collision are inherited from main, and the overlapping schema-constructor patch is absent from this diff. Fresh hosted test passed in 17m28s and windows_recovery passed in 3m23s through all native recovery steps. Both commits retain \Generated-by: Codex; the checklist and behavior declaration are complete.

@liugddx
liugddx force-pushed the fix/windows-baseline-authority branch from 9047c2e to d69a179 Compare August 25, 2026 14:16
@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@Astro-Han The base-drift blocker you identified is resolved. I rebased onto the current main and resolved the resulting conflict.

The only textual conflict was in the pull-request workflow allowlist in scripts/ci-test-plan.test.mjs: main added runtime-host-owner-platform.yml, while this PR adds windows-recovery.yml. The resolution retains both entries. git range-diff confirms that the storage fix replayed unchanged; the CI commit differs only by preserving the new main workflow entry.

New exact head: d69a179ec.

Verification after the rebase:

  • local lint, format, Windows inventory, Core/Storage builds, CI planner/harness, control-plane contract, and focused Node 24 Windows storage suites passed;
  • hosted test passed in 18m15s;
  • hosted windows_recovery passed in 3m39s, with all five recovery steps green.

GitHub now reports the PR as mergeable. The remaining BLOCKED state is the required non-author review, not a merge conflict or failing check. There are no unresolved inline review threads.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on d69a179ecb:

Freshness from 7957ebe: rebased with bounded PowerShell+Node walk fallback still fail-closed; marker-open intercept retained. No new P0-P3.

Checks on d69a179ecb5a50976a9652a219d3176ac2f816d7 are test: PASS (32858446128) and windows_recovery: PASS (32858445636).

简体中文该头仍无新增阻断。

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review conclusion on exact head d69a179ecb5a50976a9652a219d3176ac2f816d7 (base 421acbeffc65bf47ed05fdc8d2811e15a97246c9): GO with one non-blocking P2; no P0/P1 findings.

The workflow/config relationship is closed: windows_recovery now has stable PR and main triggers, the ASF-managed context list contains the same job name, and the contract tests pin both sides. The marker-race fixture now intercepts the live fs.promises.open implementation, and the PowerShell query fails closed on start, timeout, output-limit, exit, and parse failures. The remaining P2 is an object-coverage gap in that query, noted inline.

Verification: full build:test; focused Storage/marker/crash tests 27 passed with 2 platform skips; CI planner plus release-control tests 62/62; Windows inventory current at 64 declarations; Biome and diff checks clean. Exact-head hosted test and windows_recovery are completed/success, and the hosted merge-result tree equals the reviewed head tree.

简体中文精确 head `d69a179ecb` 结论:GO,只有 1 条不阻塞的 P2,无 P0/P1。工作流与 required context 的自触发关系、marker 竞态切点及 PowerShell 失败关闭路径均成立;剩余问题是 ADS 查询遗漏目录对象,见行内评论。

Comment thread packages/storage/src/managed-dependency-environment.ts
@liugddx
liugddx force-pushed the fix/windows-baseline-authority branch 2 times, most recently from ef61719 to b11da18 Compare August 25, 2026 16:15
@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

P2 follow-up and current gate status on exact head b11da18e7:

  • the directory ADS gap is fixed and the original inline thread has been replied to and resolved;
  • the branch is rebased onto current main; range-diff shows all three PR commits replayed unchanged;
  • local file/root/nested-directory ADS tests are 3/3 and managed dependency crash recovery is 5/5;
  • hosted windows_recovery is green through all five recovery steps.

Hosted test passed every step through Runtime Host, then reproduced an independent Desktop Plan/catalog-refresh e2e race: ef6171984 failed one scenario and the rebased b11da18e7 failed the adjacent scenario with the same transient disabled Plan row. This PR has no Desktop/UI diff against main, so I recorded the baseline issue as #3814 instead of expanding this Windows storage PR.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on b11da18e7c:

GO — no P0-P3 on code. Windows recovery now uses P/Invoke FindFirstStreamW with correct handle/EOF handling and directory scanning; prior base compilation P1 closed via rebase. windows_recovery: PASS on this head. Remaining test: FAILURE is unrelated e2e composer-plus-menu-stability flake tracked in #3814, not attributable.

简体中文该头无阻断,残留失败与本改动无关。

@liugddx
liugddx force-pushed the fix/windows-baseline-authority branch from b11da18 to da660e4 Compare August 25, 2026 23:24

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update on da660e47:

[P2] Marker authority uses mutable fs.promises.open binding

marker-file.ts:38-42 reads fs.promises.open dynamically per call. Later-loaded code can replace it and forge handles whose stat()/readFile() align with real path — bypasses descriptor/path checks for root identity, weakening flags or causing DoS.

Fix: capture intrinsic at module evaluation or inject explicitly; fixture should patch before dynamic import.

Note: windows_recovery: SUCCESS on this head only ran IPC/SQLite paths, not ADS targeted tests — limited evidence.

简体中文marker 权威绑定可被替换。

@liugddx

liugddx commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@jackwener The directory-ADS P2 is closed on exact head da660e47d, now rebased onto current main (c78f850d1). The original inline thread is resolved, and all three PR commits replay unchanged according to git range-diff.

Fresh exact-head gates are green:

  • test: passed in 17m23s, including all 67 Desktop e2e tests, Storybook smoke, and installed CLI validation;
  • windows_recovery: passed in 3m17s through all five recovery steps.

Local file/root/nested-directory ADS cases remain 3/3 and managed dependency crash recovery remains 5/5 after the rebase. Could you please re-review the current head when available?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants