Skip to content

test(process): cover cancellation during obstructed root lookup - #3263

Open
xu91102 wants to merge 7 commits into
morluto:mainfrom
xu91102:codex/process-snapshot-absent-descendants
Open

xu91102 wants to merge 7 commits into
morluto:mainfrom
xu91102:codex/process-snapshot-absent-descendants

Conversation

@xu91102

@xu91102 xu91102 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #3238.

Main already adopted the filesystem-obstruction fix and public lifecycle regression in e67c3642c8920a2a9c0e231ee2f16f50b245df31, retaining contributor coauthor credit. This PR keeps the distinct cancellation regression: after observing a selected file, its parent becomes a regular file and cancellation arrives during the awaited final lookup.

The conflict with current main is confined to FilesystemSnapshot.test.ts. The normal merge includes 833136507dd79d959340b09862a53af344e4f1b3, preserves main's typed read/hash cancellation test and resource owner, and adapts this regression to its existing ProcessCaptureSnapshotFailure. It checks the exact explicit caller reason object, cancelled category, partial/truncated snapshot and empty cleanup leases/read failures. Production code, schemas, docs and dependencies match the included main.

Validation

Final head: d1cd67dd128328109df32b933d4d69a17651ba8b; integration merge435620217 has parents previous PR head54b and captured main833; final head retains the complete normal merge/revert/reapply history. No historical CI result is transferred to the new head.

  • Focused selection:11 distinct files,84PASS/4 Windows-only conditional skips. The changed snapshot file was rerun after the explicit-reason adjustment; its final result replaces its earlier overlapping result. Fresh README5 gives12 distinct files89PASS/4skip, across separate invocations.
  • npm run check:6 successful tasks/2 cached,0 errors/1795 warnings; module and architecture guards passed. Docs6 tasks/3 cached plus README5; test discovery1704 exactly once.
  • Actual installed package CLI/MCP verification passed:209 tools/6 prompts, exact-release update, awaited cleanup and verified process lineage. Explicit npm pack --dry-run --json passed. These package/runtime inputs are unchanged by the final test-only reason adjustment.
  • Real macOS owned Node-process CLI/MCP finalization, cancellation and retained-descriptor cleanup/retry are covered. Local Windows/Linux host runtime, hardware and performance were not run. Four Windows-only local tests remain skipped. The separate current-head GitHub Linux/x64 TShark lane passed108 tests/1 skip plus23 synthetic PCAP cases; sigrok passed73 tests plus21 VCD cases. They do not establish hardware acquisition or add to local89PASS.
  • Independent frozen Standards/Spec V2 review: both PASS, final4596 files and fivegitlinks match the reviewed inputs. Current-head CI39358239438 attempt1 completed successfully:34 jobs,17 successful/17 conditional skips. The complete3,017,561-byte log was read. All17 executed jobs first checked out4c168bfd, whose parents are current CI base5fa4defc and this final PR head. The old54b CI38580178010 attempt2 result remains historical.

The normal merge hook auto-fixed two unrelated upstream files. A normal revert restored upstream bytes and a normal single-file commit reapplied the reviewed test; no history rewrite or hook disable was used. The final diff against included main is one test only. A full check on restored inputs succeeded with all six cached tasks and the architecture guard; prior executed V2 checks remain separately recorded.

GitHub Portable20600PASS/170skip aggregates source and five shards; it is not another execution. The actual Windows capability/package lane passed518 tests/31skip, plus47 filesystem and58 native-boundary tests; the macOS Apple artifact lane passed263/4skip. Linux arm64/x64 and macOS arm64 package baselines also succeeded within their logged scope. The Linux build of bundled Windows controls is a cross-build, separate from actual Windows execution. Other17 conditional jobs remain unexecuted; no broader provider, hardware or performance claim.

The initial raw AbortError assertion failed against current main's typed failure and remains recorded. The first independent review also required an explicit caller abort reason; the final regression supplies and checks that same object. Old inaccessible Codex failure causes remain unknown; no unavailable finding is claimed resolved.

Author readiness

Human owner: @xu91102

The declarations remain for the human owner. This update does not attest to human review or change its external nonDraft state.

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@greptile-apps

greptile-apps Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge; the new test covers a distinct cancellation case without changing production behavior.

Summary

Adds one regression test for cancellation during an awaited lookup after a previously observed file’s parent becomes a regular file.

  • Snapshot tests cover cancellation during an obstructed root lookup.

No actionable correctness, security, or repository-rule issues were found. This review checked source code; it did not run tests.

Reviews (3) · Last reviewed commit: "test(process): preserve exact caller can..." · Reviewed by Greptile

@xu91102
xu91102 marked this pull request as draft October 14, 2026 08:12
@morluto morluto added bug Something isn't working area: runtime Process capture and runtime observation labels Oct 14, 2026
morluto added a commit that referenced this pull request Oct 14, 2026
Carry initial root observations into final capture. Treat ENOTDIR as absence only for a previously observed root, preserving initial obstruction and cancellation failures.

Refs: #3238, #3263

Co-authored-by: hhang <169027046+xu91102@users.noreply.github.com>
@xu91102 xu91102 changed the title fix(process): retain absent roots after ancestor replacement test(process): cover cancellation during obstructed root lookup Oct 14, 2026
@kaoru0822-kitauji
kaoru0822-kitauji marked this pull request as ready for review October 15, 2026 06:18
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-15T06:19:00.601927Z 54b47f3 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@morluto

morluto commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Compared this PR with main fc712b375 during the fixed/superseded backlog audit.

The obstructed-root lookup defect was fixed in e67c364, but the cancellation-during-lookup regression added here is still distinct from main's existing coverage.

Please retain the cancellation, bounded completion and cleanup/ownership oracle when rebasing. This is useful fault-boundary coverage and should not be discarded simply because #3238 is closed; the PR can be narrowed to the missing regression.

PR head reviewed: 54b47f3e7. This is a source comparison; no new PR-branch tests or provider runs were performed for this comment.

xu91102 commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed against main fc712b3750cbe16755c4f78c1f90fdf09fbaca31: head 54b47f3e778f92e9dff1143e93c7635bbb69f9fe is already narrowed to FilesystemSnapshot.test.ts; the production obstruction fix and public lifecycle coverage remain owned by main.

The retained regression observes the selected file, replaces its parent with a regular file, starts the awaited final lookup, then aborts. It requires AbortError rather than a successful ENOTDIR-as-absence result. Completion remains bounded by the existing Vitest timeout (30s, 60s under coverage); createTestTempDirectory registers awaited onTestFinished cleanup of the exact canonical owned directory, including failed/timed-out cases. I will preserve these cancellation, completion and cleanup invariants in any necessary later integration.

No production rewrite or additional harness was added. Current-head CI 38580178010 remains successful. This reply records a fresh diff/configuration check; it does not claim a new test or provider run.

@CassieFox23

Copy link
Copy Markdown
Contributor

TL;DR: I ran the one test file this PR changes, on Node 24.18.0. The new test passes on current main. It also catches the bug: when I removed one line of main's fix by hand, the new test failed.

Thanks, xu91102, for the work. I tested head 54b47f3 on top of main 89faa86 (#3050).

Results

Check Result
Test file on main plus this PR 7 passed, 4 skipped, 0 failed
Same file after I removed the abort check in main's fix 1 failed, 6 passed, 4 skipped
Source file after I put the line back byte-identical to main's (cmp), not rerun
  • The PR only adds a test. So there is no before and after on main. The production fix is already on main, as commit e67c364 (2026-10-14).
  • The new test makes a root path fail with ENOTDIR, then cancels. It expects an AbortError. It does not accept "the path is absent".
  • The removed line was the abort check in the missing-path branch of the lstat helper. It sits at line 67 of the snapshot source file. Without it, the call returned a result instead of rejecting. The failing test was the new one. Its title says it rejects cancellation during a root lookup that is now blocked.
  • The four skipped tests are Windows-only. They run when the platform is win32.
  • CI: GitHub shows all checks green on this head, including Source tests, five test shards and CI required. One old check run was cancelled. Newer runs passed. I did not run CI and I only read that status.
  • The box was shared with other test agents, so timings are rough.

Not run

  • The four Windows-only tests in that file, and anything on macOS.
  • Node 22 and Node 26.
  • Any other test file, and the full suite.
  • A before run on main. The PR adds no source change, so the mutation above stands in for it.
Commands
git -C ~/code/rea-mirror/rea.git fetch origin
git -C ~/code/rea-mirror/rea.git archive 89faa863ed472d8a13a826014331d3b81cfc5de3 | tar -x -C /tmp/t8-1015/main
npm ci   # Node 24.18.0, exit 0
git -C ~/code/rea-mirror/rea.git diff 89faa863e...54b47f3e7 > 3263.diff
git apply 3263.diff
npm run test:focused -- src/process/capture/FilesystemSnapshot.test.ts   # exit 0, 7 passed 4 skipped
sed -i '67d' src/process/capture/FilesystemSnapshot.ts   # drops signal?.throwIfAborted() in the ENOENT/ENOTDIR branch
npm run test:focused -- src/process/capture/FilesystemSnapshot.test.ts   # exit 1, 1 failed 6 passed 4 skipped
cp FilesystemSnapshot.ts.orig src/process/capture/FilesystemSnapshot.ts   # restored, cmp clean

I ran grep -i doctor on the test file first. It had no hits.

Test setup
  • Box: Nobara 44 (Fedora 44 base), Linux 7.2.9
  • CPU: AMD Ryzen 9 5900X, 12 cores / 24 threads
  • RAM: 32 GB DDR4-3600
  • Node: 24.18.0
  • Head: 54b47f3
  • Base: 89faa86
  • Other: vitest 5.0.3

🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊

Merge current main833 while preserving both the observed-root cancellation regression and main typed read/hash cancellation coverage. Match the shared ProcessCaptureSnapshotFailure owner and retain exact explicit caller reason identity.

Refs morluto#3238
Reverse the unpublished merge relative to its included-main parent to restore exact upstream inputs, then reapply only the reviewed cancellation test in a normal commit. Preserve all history; no hook is disabled or bypass flag used.

Refs morluto#3238
Reapply only the independently reviewed obstructed-root cancellation regression after restoring upstream bytes changed by the normal merge hook. Keep main typed failures, caller reason identity, partial snapshot and cleanup facts.

Refs morluto#3238

This branch has not been deployed

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

Labels

area: runtime Process capture and runtime observation bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Process capture fails when an observed file parent becomes a regular file

3 participants