Repository navigation
fix(producer): render inside an agent's macOS sandbox no longer dies on uptime - #5842
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 168 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Reviewed exact HEAD 6d8f8c65bafb4ef465e8e5d40f1ca746f13c18db against API/effective BASE dc0e80e73af185cc892ea90d5a9f1412a055e9a1.
Wrapping the fallback in attempt correctly prevents the optional uptime read from aborting directory creation (renderDirOwner.ts:38–40). The new test calls the real work-directory creation path with that read throwing. The problem is how the newly missing identity is subsequently interpreted by the destructive sweep.
[P2] Treat an unavailable boot identity as unknown during reclamation
packages/producer/src/services/render/renderDirOwner.ts:40: when both probes fail, the stamp now omits boot. On macOS the PID-namespace probe is also unavailable. inThisPidScope still accepts equality of the optional fields (64–66), so two missing boot/PID-namespace values are treated as evidence that the recorded PID belongs to this machine.
Consider two same-hostname machines sharing an output directory, with those probes unavailable on both. Machine A stamps a matching work-<uuid>-<suffix> directory with {pid: P, host: H} while its render is alive. Machine B starts another render against the shared directory; PID P does not exist on B. The scope comparison passes, the local PID check reports the owner gone (77–81), and createRenderWorkDir recursively removes A's live work directory (135–139). A staging directory without backup is susceptible too. This is not evidence that the remote owner exited.
This shared-directory path is reachable: renderOrchestrator.ts:1066–1074 puts non-Windows work directories beside the requested output, and 2785–2789 sweeps that directory before starting the new render. artifactTransaction.ts:141–149 also stamps staging siblings with this helper. The identity helper itself explicitly calls out same-hostname machines sharing a folder as the reason host identity alone is insufficient (23–25). BASE's failed uptime read instead escapes into isAbandoned's catch, which retains the directory; although BASE also fails the new render's creation, it does not perform this destructive reclamation.
Allow owner stamping without uptime, but fail closed on reclamation when the available fields cannot establish PID locality. Preserve the existing behavior for known matching/mismatching scopes, live PIDs and staging backups. Add a regression with both boot and PID-namespace identity unavailable, an existing matching owner record and an absent local PID: the existing directory must be retained while the new directory can still be created. The new test checks only creation/PID on an empty parent; it does not cover this sweep, and its Linux run retains the real PID-namespace probe.
Evidence and limits: Source-derived BASE-relative transition witness, not an executed multi-machine or macOS reproduction. All existing changed BASE source was read fully before changed HEAD source; both changed HEAD files were read fully. No tests, sandbox probes, process/PID probes, browser/render execution, builds or CI queries were run. Author test/manual-run claims and bot gate status were not independently certified.
Verdict: REQUEST CHANGES
Reasoning: The optional uptime fallback newly allows absent identity fields to authorize deletion of a live render's work or staging directory on a shared filesystem.
— Review by tai (pr-review)
…on uptime The render work dir's owner stamp falls back to os.uptime() for a boot time when /proc is missing. The seatbelt refuses uptime (uv_uptime EPERM), so every render died with that error before Chrome started. The fallback is now optional like the /proc read, and render reaches the existing message that says to rerun outside the sandbox.
Without a boot identity, or on Linux without the pid namespace, two missing values matched, so on a shared output folder a same-hostname machine could delete another machine's live work dir. Such a scope is now unknown, and the dir is left alone; macOS, which has no pid namespace, still reclaims by boot.
6d8f8c6 to
216b022
Compare
|
@terencecho thanks, fixed in 216b022 (rebased on main):
|
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Source re-review at 216b0221030bc7f11588b4818c7d8b5cfde6eef6, against effective/API base e44f456b7cbf8da6389797f4fb4a776d1ea8885c.
- The added guard narrows owner-bearing reclamation eligibility for a fixed probe outcome:
packages/producer/src/services/render/renderDirOwner.ts:62–91requires a known boot identity, plus a known PID namespace on Linux, before checking local PID liveness. Unknown owners do not enter the ownerless-age branch (103–115). - The new regression calls the real sweep with both identity probes unavailable and an existing foreign record; it checks that the old directory survives while the new one is created (
renderDirOwner.sandbox.test.ts:77–94). The Darwin case preserves known-boot reclamation without requiring Linux's namespace (96–104). These are test-source observations, not passing test results.
Prior blocker resolved: My earlier P2 review concerned missing identity fields comparing equal across same-hostname machines sharing output. That witness now reaches unknown, not gone, so the sweep retains the existing directory. For a fixed probe outcome, the added guard only narrows owner-bearing reclamation eligibility relative to BASE; wrapping uptime no longer couples successful creation to the prior missing-identity deletion witness. Existing backup protection and the known-scope comparison remain. This supersedes my old source objection; it is not a claim that all pre-existing process-ownership limitations are solved.
Nonblocking prose correction: The PR body still says “Without a boot time … A dead owner is still reclaimed.” Current code intentionally retains unknown-scope owners, even if the PID is absent locally. Please align that sentence with your follow-up comment.
Evidence and limits: Two independent legacy-first source passes fully read the existing changed BASE file before both changed HEAD files; I independently verified complete tree identities, the exact two-leaf changed union, all three source versions, and read those changed files in full. Supporting audits covered the orchestrator's creation/sweep, transaction staging/rollback, cleanup and CLI clean. Large orchestrator/stage reads and additional reference scans were partial, not whole-file semantic coverage. Indexed discovery plus current-byte render-domain/named-consumer scans are not an exhaustive whole-repository or external-caller census. No macOS/shared-machine reproduction, PID/uptime/sandbox/browser/render/native probes, tests, installs, builds or CI queries were run; author test/manual claims are unverified. Existing coarse boot identity, PID-error handling and abrupt-owner/native-child lifetime limits are not newly certified.
Approval effects: The 16 acquired workflow event headers show no declared review-submission subscriber. Three publication/sync workflows and their explicit publication helpers were read fully; the other workflow bodies were header-only. You state an external loop merges after approval, so this is not an inert action. A subsequent main push can run normal checks/catalog publication, including conditional generated-catalog PR/check dispatch. External loop/protection/environment policy, third-party actions and the complete transitive build graph were not verified. This APPROVE may satisfy the stated external loop's merge condition and thereby lead to normal main-push workflows; I am not directly issuing a merge or dispatch command. That loop and its policy were not independently verified.
Verdict: APPROVE
Reasoning: The prior missing-identity deletion witness is closed and no introduced blocker was verified in the audited delta and supporting paths. Owner-bearing records with unavailable scope identity now remain unreclaimed, while optional uptime failure no longer aborts render-directory creation.
— tai
What
hyperframes renderinside an agent's macOS sandbox (Codexworkspace-write, the seatbelt) now ends with the message that says what to do: "The browser can't start inside this sandbox ... Rerun this command with escalation, outside the sandbox." Before this, every render there died earlier withA system error occurred: uv_uptime returned EPERM (operation not permitted)and a suggestion to try--docker, which does not help.Why
Each render stamps its temp work dir with an owner (
renderDirOwner.ts), so a later render can reclaim the dirs of renders that died. The stamp records a boot identity:/proc/sys/kernel/random/boot_idwhere it exists, otherwise the boot time derived fromos.uptime(). The/procread was wrapped inattempt(), but theuptime()fallback was not. macOS has no/proc, and the seatbelt refusesuptime(uv_uptimeEPERM), so the stamp threw and the render failed before it reached Chrome.Without a boot time, an owner record only loses the reboot check. A dead owner is still reclaimed, and a live process that reused the pid is still left alone.
Repro on 0.8.170, macOS arm64:
codex sandbox -c sandbox_mode='"workspace-write"' -c sandbox_workspace_write.network_access=true -- hyperframes render -o out.mp4Related work
Refs #5734, which added the "rerun outside the sandbox" message this now reaches. Making render itself run inside the seatbelt (one process over a pipe, as check, snapshot and capture now do) is a separate decision and not in this PR.
How
The
uptime()fallback goes through the sameattempt()as the/procread, sobootis left out when neither is available.Test plan
renderDirOwner.sandbox.test.tscreates a render work dir withuptime()throwing EPERM and the/procboot id missing (so it takes the same path on Linux CI). It fails on main with the uptime error and passes 3 runs in a row with the fix; the existingrenderDirOwner.test.tsstill passes.