🧹 fix: Await Job Cleanup and Measure Retained Resources - #249
Conversation
|
Ready for maintainer-triggered review of exact head This head adds post-cleanup resource telemetry, waits for cleanup before Please focus on cleanup/response ordering, concurrent cleanup samples, session preservation, UID retention on failed removal, and cgroup/tmpfs metric scope. Subsystem self-review is complete. Focused Linux tests passed; the API typecheck still has only its eight pre-existing diagnostics. The real-NsJail detached-descendant test and production soak remain unrun. This head does not claim to fix the production retention mechanism or verify every descendant termination path. A maintainer must request the external review; Lia cannot trigger it through the GitHub App. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3bb21fc1a
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type MemoryKind = typeof memoryKinds[number]; | ||
| type WorkspaceKind = typeof workspaceKinds[number]; | ||
|
|
||
| export const postCleanupMemory = new Gauge({ |
There was a problem hiding this comment.
In Helm deployments that rely on the chart-managed metrics.enabled PodMonitor, these new metrics are never collected. The worker-sandbox monitor in helm/codeapi/templates/podmonitor.yaml selects the service-worker pod and its health port, while these collectors are registered in the separate sandbox API process exposed through the sandbox-runner's sandbox port. Add a sandbox-runner scrape endpoint to the PodMonitor; otherwise the retention metrics added here are unavailable outside structured logs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8303f6569b6f89ae59b3f318f4356fc6ebc6e3b2.
Added a separate sandbox-runner PodMonitor selecting the runner pod labels and /metrics on the named sandbox port. It inherits the configured scrape interval/timeout and preserves the existing service-worker health scrape.
Following the scrape path also exposed a NetworkPolicy blocker. Added explicit, opt-in metrics.sandboxRunner.ingressFrom peers and documented matching the Prometheus namespace AND pod labels. The default empty list preserves worker-only ingress, because this same TCP port also exposes /execute. No egress rules or manifest verification were relaxed.
tests/sandbox_runner_metrics.sh reproduces the missing runner endpoint on the reviewed template and passes after the fix. Coverage includes labels/ports, timing overrides, disabled components, old/missing values, unchanged default policy, and scoped scrape ingress with unchanged egress. Wired into Deployment Config Tests. Helm lint and ShellCheck pass; cleanup/route tests also pass. Live Prometheus scraping was not tested.
|
Ready for maintainer-triggered review of exact head This head addresses Codex's P2 scrape-discovery finding with a sandbox-runner PodMonitor, explicit opt-in NetworkPolicy scrape peers, chart documentation/version bump, and a CI Helm-render regression. Default sandbox access, execution-manifest verification, egress, and the previous cleanup implementation are unchanged. Replied to the inline finding with the fixing commit. Please review end-to-end scrape discovery and ingress restrictions, including older-values compatibility and disabled configurations. Subsystem invariant self-review and focused checks passed; API typechecking still has only its eight pre-existing diagnostics. CI for this head has started. Live Prometheus scraping, production soak, and the real-NsJail descendant test remain unrun. The prior Codex review covers only |
|
@codex review the latest head |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Add post-cleanup resource measurements so runner memory growth can be distinguished from active work, intentionally persistent sessions, and failed disposable cleanup. Also fix
/executeacknowledging completion before its asynchronous cleanup has settled.This is an investigation/observability change, not a claim that the reported production retention is reproduced or resolved. No resource requests, HPA settings, mount sizes, namespace permissions, or process-termination behavior change.
Behavior
Previously:
Now:
memory.current,anon,file, andshmemat the visible cgroup mount root, covering the API and sibling jail cgroups where available./tmpallocated bytes/inodes withstatfs, identify whether it is tmpfs, and count disposable/session/other workspace-root entries without traversing untrusted trees.fileincludesshmem. The runner's/tmpceiling does not bound all container memory, including distinct jail tmpfs mounts. Samples are runner-wide last-cleanup observations, not per-job attribution or continuously refreshed gauges. The README documents scope, unavailable sources, concurrency, intentional retention, and test setup.Verification
bun run buildnpx --no-install tsc --noEmitgit diff --checkExisting typecheck diagnostics are in
api/hosted-app.routes.test.ts,api/session-inputs.routes.test.ts,hosted-app.ts,job-cleanup.test.ts(three),tool-call-socket-proxy-runtime.test.ts, andworkspace-isolation.test.ts, all underapi/src. They were measured before editing and left out of scope.The focused set covers cleanup, isolation/reaper, sessions/checkpoints, input priming/downloads, artifact walking, execution capability checks, route validation/binding/timeouts/disconnects, NsJail wrapper/setup-gate fixtures, workspace commands, and lifecycle hooks. The worker's fixed
/tmpis read-only, so Linux subprocess fixtures ran in a disposable container using its own writable fixture directory; no host permissions were changed.Not run: the opt-in real-NsJail descendant test (requires an appropriately configured isolated runner image), production soak/telemetry comparison, full local test suites, and live HPA validation. Passing filesystem cleanup and wrapper fixtures does not prove production namespace teardown.
Subsystem review
Reviewed writers/readers and ordering through prime, execute, upload, cleanup, quarantine, retained-UID retry, session persistence, and client disconnect. Authorization and user-visible response schemas are unchanged. Metrics do not follow workspace symlinks or use user-controlled paths/labels. No persistent-state migration or service-side coordination is required. Reaper-only changes appear on the next job cleanup sample.
Code graph and source investigation used
mainat67d75d859aee891923c40cde1db073489d74a424. SQL import reachability was a floor; full codegraph service closure was not computed.Codex follow-up: Helm scrape discovery
Head
8303f6569b6f89ae59b3f318f4356fc6ebc6e3b2fixes the valid P2 finding that the new API collectors were not discovered by chart-managed PodMonitors./metrics, named portsandbox, using the runner pod selectors. Preserved the worker'shealthscrape and shared timing overrides.metrics.sandboxRunner.ingressFrompeers because default runner NetworkPolicy blocks Prometheus. Empty defaults preserve worker-only ingress; namespace and pod selectors must target trusted scrapers. The runner port also serves execution requests. Sandbox egress and manifest verification are unchanged.tests/sandbox_runner_metrics.shto Deployment Config Tests. Real Helm renders check monitor/port/selector alignment, timing overrides, disabled components, missing older values, unchanged default access, configured peer restrictions, and unchanged sandbox egress. This regression fails on the reviewed template and passes on the fix.Review-round checks: Helm render regression and Helm lint passed;
block_root_package_delivery.sh,sandbox_runner_healthcheck.sh,bridge_pairing_rollout.sh, andrelease-versioning.shpassed; ShellCheck, bash syntax, YAML syntax and diff whitespace checks passed. The cleanup metrics/job/route tests passed (11 tests). APInpx --no-install tsc --noEmitstill reports exactly the eight baseline diagnostics listed above, with no new diagnostics. No TypeScript production code changed in this round.Subsystem review followed metrics registration,
/metricsrouting, deployment pod labels and ports, both monitor selectors, network ingress/egress, default/disabled settings, and older-values compatibility. Live cluster scraping, production soak, real-NsJail descendant verification, and full local suites remain unrun. The old head's CI was green; the new head has its own CI run and needs a fresh external review.