Repository navigation
perf(natives): retain exact guards while reducing reaper overhead - #6353
Conversation
Exact deletion statted every ancestor only to reject symlinks before a no-follow directory open. Let the kernel enforce that predicate and keep failure-only diagnostics, every ancestor open, exact identity and digest checks, and quarantine restoration unchanged. Lore-id: 295d6a81 Constraint: each effect retains fresh no-follow traversal and exact parent authority Rejected: cached parent identity as deletion authority | permits replacement effects Confidence: medium Scope-risk: bounded Reversibility: revert-safe Tested: existing genuine addon 32 native guard regressions and native package types; repeated strict fixture failures retained Not-tested: new native runtime and performance before genuine source-bound build
Record the descriptor generated by the genuine source-bound build. Keep all 50,001 real aged files and protected entries under the original deadline, while avoiding an asynchronous timestamp request per fixture file. Lore-id: 743a5ef2 Constraint: no cardinality deadline or skip waiver Confidence: high Scope-risk: bounded Reversibility: revert-safe Tested: genuine optimized native 125 passed 17 existing skips; unchanged strict fixture 17.84s 11 assertions; native and coding-agent package checks Not-tested: all-platform performance and current full-union qualification
probepark
left a comment
There was a problem hiding this comment.
Code review (CI still running on 059c5ac; approval decision follows once CI finishes).
Review (head 059c5ac, gajae-reviewer on behalf of probepark)
CI: all planned checks green except Virtual integration validation (pending). check:@gajae-code/natives, check:@gajae-code/coding-agent, rust-check/rust-test, native-build and test:packages/natives/test/path-identity-posix.test.ts / session-directory.test.ts passed at this head.
Scope: +106 / -31, 7 files: crates/pi-natives (path_identity.rs), packages/coding-agent (managed-session-storage.ts, session-directory test, changelog fragment), packages/natives (diagnostic-artifact.json descriptor, posix test, changelog fragment).
Conventions: changelog fragments present for both packages; no hand-edited generated files (models.json/schemas/docs-index untouched); no labels.
Notable:
crates/pi-natives/src/path_identity.rs:5485-5509: I checked the error-code mapping against the removed pre-fstatatpath. Symlink ancestor: Linuxopenat(O_NOFOLLOW|O_DIRECTORY)gives ELOOP, andsecurity_codemaps that toreparse_point(L2738). Darwin gives ENOTDIR, the newfstatatprobe sees S_IFLNK and returnsreparse_point. Missing ancestor: ENOENT maps tonot_found, as before. Regular file: ENOTDIR, the probe sees no symlink, so it returnsnot_directory, as before. errno is captured before the probe and beforeclose.parent_fdis closed exactly once on both the failure and success branches. The probe never retries the open and never continues the walk. The four new posix tests (immediate/intermediate symlink, missing/regular) pin these codes.packages/coding-agent/src/session/internal/managed-session-storage.ts:954-955,1034-1035: the!terminalRemnant &&short-circuit does not change behaviour. Both candidate flags are read only in theterminalRemnant ? undefined : …alias branch and in thecontinueguard, and that guard is already satisfied byterminalRemnant.session-directory.test.ts:3558:syncFs.utimesSyncinside the async batch blocks the loop per entry. That is acceptable for a fixture, and the assertions, cardinality and deadline are unchanged.
Blocking: none.
Checked clean: native ancestor walk fd ownership and error codes; reaper candidate classification in both the sync and async twins; test coverage of the new native branches. Not checked: Darwin execution locally. The reviewer pod has no macOS; CI darwin-arm64 tab-worker smoke passed.
|
Fresh full cumulative qualification completed on EXACT059c5ac412a4bc4d462d8d058c381f102262ce80/baseinstalledbb00: |
probepark
left a comment
There was a problem hiding this comment.
Review (head 059c5ac, gajae-reviewer on behalf of probepark)
CI finished; no blocking findings. Code review: #6353 (review)
CI: green. All 35 checks at this head passed or were skipped, including Virtual integration validation, check:@gajae-code/natives, check:@gajae-code/coding-agent, rust-check/rust-test and darwin-arm64 tab-worker smoke (run https://github.com/Yeachan-Heo/gajae-code/actions/runs/37269984906). The approve gate returned ALLOW: 0 pending, 0 failed, and the plan covered check:@gajae-code/natives and check:@gajae-code/coding-agent.
Scope: +106 / -31, 7 files: crates/pi-natives, packages/coding-agent, packages/natives.
Conventions: changelog fragments present; no generated files; no labels.
Blocking: none.
Body verdict line count=0; not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:96d8eae054fb697c0b1047ca06da7df821f05a043fa1f82d0f33d6c51c60fcbc reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;native-fd-close-once-checked;error-code-mapping-checked
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:96d8eae054fb697c0b1047ca06da7df821f05a043fa1f82d0f33d6c51c60fcbc reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;native-fd-close-once-checked;error-code-mapping-checked
Scope
Independent current-dev prerequisite repair for the task-after-session-move stack replacing #6240. Base bb00f4c includes externally installed owner producer#6349 and current Codex partial-call/control-event/provider-veto retry fixes. Exact head059c5ac412a4bc4d462d8d058c381f102262ce80.
Actual red/green evidence
Current-base unchanged fullunion failed20,041.38ms then20,124.53ms; both1211pass30existing-skips1fail, packagecheckNOTRUN. Retained both failures, no blind subsequent replay. One focused baseline passed near deadline; instrumentation then failed21,088.25ms and measured setup3816ms/effect17258ms. Root cause of timing variation not proven. Instrumentation fully removed.
Genuine source-bound native build on67c0c8de772fae4b47b4b8402687fb7e679e6d41 exits0;115explicit exports/10constenums; version0.18.7. Descriptor generated by build, NEVER manually stamped. AddonSHA256ed6fa1b9e21d44e395c3e6ee5c4c233f8f95f7cb5f265e8a92b23a26062cea8a; sourceSHA2567307e6644582648c8ea41ec634d30fc6a8fab9b60ae1ada001d5cde214b663fe; crateTree9a5bfa9ef444051b63c70cd502fb721ee0b78938. Rebased exact head has same native source/crate tree and authenticated generated descriptor.
Fresh exact current-owner composed qualification
Clean strict-native independent QA
native-parent-open-first-currentbb00:1248pass32EXISTINGplatform skips0fail7987assertions1280tests34files505.56s; complete coding-agent package/vendor/Biome/types check0; diff0; clean exactHEAD beforeafter. Includes all installed consumer/owner cases, actual new initial-task owner regression, both previously missed lifecycle files, unchanged full strict fixture, current Codex stream/provider tests and native symlink/parent/file guards.New real POSIX immediate/intermediate parent-symlink and missing/non-directory cases refuse without deleting retained/foreign payload. Native guardunion125pass17existing-skips0fail724assertions142tests3files; focused unchanged strictfixture1pass11assertions17.84s. Native and coding-agent package checks0, Rustaggregate0; existing warnings retained. Full cumulative workspace gates are separately recorded when executed, not inferred from these tests.
Budget and boundaries
Formatted production own AND actualdev60: Rust25+25=50, TS4+4=8, genuine descriptor1+1=2. Sevenfiles; tests/changelogs excluded. No native ABI/public schema/worker changes. Darwin evidence is not all-platform execution, filesystem-global writer linearizability or FD revocation. Caller-owned MCP and task admission authority unchanged. Committed-generation admission remains separate pending implementation qualification. No final goal/cohort/critic completion claim. Leader does not merge; fresh code CI, exact posted write-access approval and external installation remain distinct.