Skip to content

Fix macOS shared-source publication and preserve cleanup failures - #450

Open
Brian Krabach (bkrabach) wants to merge 6 commits into
mainfrom
fix/macos-shared-source-publication
Open

Brian Krabach (bkrabach) wants to merge 6 commits into
mainfrom
fix/macos-shared-source-publication

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Problem

Addresses microsoft/amplifier-unified#1194 in the canonical Foundation implementation. SharedSourceStore seals the stage root before renaming it. macOS rejects renaming a write-disabled directory; cleanup of its frozen child directories can then hide the original publication error.

Source inspected: Foundation main 81b9bb0. Its shared.py is identical to Unified 0.31.3's lock-selected Foundation 8d26ece. This is not proof of the reporter's installed dependency identity.

Correction

  • Keep provenance validation, object paths, binding format and per-object lock. On POSIX freeze descendants, rename the writable stage root, then seal the destination. verify/cached reject an unsealed root; concurrent ensure waits for the lock. Windows keeps its existing seal-before-rename order.
  • Scope failed cleanup to the stage inode, including its incomplete renamed destination. Restore directory permissions without following symlinks. Preserve the primary publication exception and attach separate cleanup evidence. Never adopt or delete a pre-existing incomplete object.
  • Drain threaded I/O through repeated cancellation before releasing the lock. Successfully sealed objects survive cancellation.

Focused evidence

Prepared regressions cover write-enabled rename ordering, exact immutable bytes and reuse, read-only source adoption, paused publication readers, concurrency, rename/descendant/final-seal faults, cleanup diagnostics, replaced-inode retention, symlinks, repeated cancellation, and real strict module activation/import through a local Git transport.

A narrow macos-latest/Python 3.13 CI job adds actual native rename failure + writable success controls, source tests, and exact installed-wheel tests outside the source checkout. Existing Linux/Windows CI jobs are unchanged. No paid provider or desktop access is needed.

At PR creation: syntax parsing and git diff --check passed; independent read-only review found no source blocker. Runtime results are pending and will be reported separately. No local host package import/install/test, live update, or shared-service change was performed.

Compatibility and remaining acceptance

Completed legacy objects keep their format. Older running Foundation readers do not check the POSIX root seal; do not expose an existing binding to an object being newly published by the corrected writer until those readers use the completion check. Normal binding creation still follows completed publication. This is a cooperative API guarantee, not protection against hostile filesystem writers.

Normal upstream review/approval is required; this PR is not authority to merge or update Unified. Releases owns downstream adoption and exact Unified installed-worker qualification after upstream merge. The local module activation smoke is not an end-to-end Unified conversation start/resume test, and macos-latest is not the reporter's macOS 13 machine.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

First native run retained: https://github.com/microsoft/amplifier-foundation/actions/runs/40255010559/job/121354871996 . macOS 26.6.2 arm64, UID501, Python3.13.16: 29 focused checks passed; one native negative-control assumption failed because the default test volume permitted a write-disabled directory rename. Installed-wheel step was not reached. This is not a reproduced macOS13 failure. The follow-up test uses a temporary native HFS+ disk image (the filesystem named by Apple rename(2)), with explicit detach in teardown, and checks both old syscall ordering and corrected SharedSourceStore on that same filesystem. No production correction changed, no assertion was removed or converted to an expected failure. New CI result pending.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Second native result: run40255214948/job121355487679 again passed29 checks; the original-order rename also succeeded on a mounted HFS+ image under macOS26.6.2. Detach completed (disk7 ejected). Therefore the documented/reported restriction is not reproduced by this available OS, and the HFS+ setup has been removed rather than becoming a permanent test framework. The native observation now explicitly SKIPS its negative-reproduction claim when the OS permits the rename; source and installed-wheel behavioral checks remain required. A separate CI step tests the exact old shared.py from Unified lock-selected8d26 with the precise injected write-enabled-rename contract; it must fail at that assertion, while the candidate must pass. That control is emulation, not nativeMac13 proof. Both failed runs remain retained; production shared.py is unchanged. Mac13 reporter-side reproduction and packaged Unified startup stay open acceptance boundaries.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Manager review found a real missing branch: GitSourceHandler full-clone fallback replaces its destination inode. The initial fix captured that inode too early and could retain an incomplete target after seal failure. Successor2c27d42 clones beneath a stable owned staging parent; cleanup never adopts an inode after arbitrary clone failure, while the validated checkout inode is captured immediately before publication. Four new focused cases force the actual fallback with a different inode and exercise clone failure, repeated cancellation, post-rename seal failure, and success/reuse. Existing alien-target refusal remains. Native observation is already separated from the contract discriminator: the previouse340 run passed all7CIjobs, with Mac source/wheel29pass1native-reproduction-skip. That result does not cover this new fallback correction; successor CI pending.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Final scoped qualification at ded04e2: https://github.com/microsoft/amplifier-foundation/actions/runs/40255995371 completed successfully (all7jobs). Native macOS26.6.2 ARM64/Python3.13.16 source33passed/1native-reproduction-skip; exact installed wheel outside checkout33passed/1same skip. All4 forced full-clone replacement cases now execute and pass in both; preceding2c27 test-wrapper TypeError is retained, corrected by positional forwarding in ded04. Linux3.13:2501passed/5skipped; Windows3.13:2423passed/84skipped (POSIX-only seal cases explicitly excluded). Exact old8d26 source triggers the marked injected-rename assertion, then the old cleanup OSError; this is contract emulation, not nativeMac13 reproduction. Candidate wheel SHA256755fe6225864ac612f31d1a7a7e6665d35eb3a65be6745b7cd7bd18801dff03f. No maintainer approval/merge or Unified adoption is claimed. The remaining acceptance boundary is reporterMac13 plus normal packaged Unified cold-publication/start/resume, owned by downstream qualification after upstream review. No paid provider, live install, desktop or credential changes.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Readiness independent acceptance of the bounded Foundation candidate

Exact ded04e2 / tree165cb456b960a3d116435a7717b7af4e4cc205c5 is source-clear for the two manager findings. Stable staging-parent custody handles the legitimate clone fallback while retaining alien-inode refusal. Native rejection is now an explicitly limited observation; positive native publication runs independently. Final ded04 changes only the test transport wrapper positional-argument forwarding over reviewed2c27, not production or oracles.

Parent read the full handoff (SHA938a774b), verified all767 retained source-archive entries against immutableGit, and independently fetched the public final Mac raw log: exact60034B/SHA d6b0a7fc139d4a58881a919a5ff7ac4a7bb31a8f111976ef588a202cabaeff32 equals retained bytes. CI40255995371 attempt1 is SUCCESS; its syntheticmerge df938b2 has EXACTcandidate tree and parents81b9+ded04. All7jobs succeeded, not all tests unskipped.

Actual Mac26.6.2 ARM64 UID501 Python3.13.16:33sourcePASS +33installed-wheelPASS, each with1explicit native-reproductionSKIP. All four forced different-inode fallback cases executed and passed. Site-packages outside-checkout identity and wheel755fe6225864ac612f31d1a7a7e6665d35eb3a65be6745b7cd7bd18801dff03f recorded. Exact old8d26 shared.py fails deterministic injected rename-contract assertion; original cleanup masking is retained. This is NOT reproduction of the macOS13 reporter restriction. Linux3.13 raw2501PASS5SKIP; Windows3.13 raw2423PASS84SKIP. Earlier APFS/HFS negative-assumption and transport-wrapper failures remain failures.

No new test/build/CI run in this review. No owned DTU guest; earlier diagnostic CI image detach is retained. Source/evidence custody remains authorcc0. This is bounded source/retained-proof acceptance, not formal approving review or permission to merge. Normal Foundation maintainer approval remains required. After upstream merge, Releases owns accepted-source root/worker identity adoption and the affected packaged Unified cold/readiness/resume check; rootuv.lock alone does not change worker runtime. Mixed-version reader caveat stays explicit. No live update/restart.

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.

2 participants