refactor(runner): finish the lifecycle split with a typed context - #5754
refactor(runner): finish the lifecycle split with a typed context#5754mmabrouk wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR decomposes sandbox environment acquisition into controlled context, runtime, mount, and harness lifecycle modules. It centralizes state transitions, freezes daemon environment before provider startup, and adds seam tests for ownership and ordering. ChangesEnvironment lifecycle decomposition
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant environment.ts
participant mount-lifecycle
participant runtime-lifecycle
participant sandbox provider
environment.ts->>mount-lifecycle: Mount and remount local resources
environment.ts->>environment.ts: Freeze daemon environment
environment.ts->>sandbox provider: Start with frozen environment
environment.ts->>runtime-lifecycle: Quiesce runtime and remove files
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| * reviewer's summary was exact: "The type compiles, but it does not enforce what its comments | ||
| * promise." So this revision follows one rule: | ||
| * | ||
| * IF A COMMENT SAYS A UNIT MAY NOT DO SOMETHING, THE TYPE MUST MAKE IT IMPOSSIBLE. |
There was a problem hiding this comment.
This rule is the reason the type looks the way it does. Read it before you read the type.
This is revision 2. An external security review rejected revision 1. It stated its invariants in comments and exposed the mutable environment anyway, and the reviewer's summary was exact. The type compiled, and it did not enforce what its comments promised.
So every restriction here is expressed in the type. A unit that may not write a field cannot reach it. The findings from that review are recorded at the bottom of this file, because each one describes a way this type could have looked correct and enforced nothing.
If this type looks heavier than it needs to be, that history is the answer. Simplifying it back to comments recreates the version that was rejected.
| * THE THREE ORDERING INVARIANTS | ||
| * ============================================================================================ | ||
| * | ||
| * ---- INVARIANT 1: LOCAL MOUNTS RUN BEFORE THE PROVIDER FREEZES THE DAEMON ENV ---- |
There was a problem hiding this comment.
This invariant is load bearing and it is easy to break by accident.
The sandbox provider takes the daemon environment maps by reference. After that point the maps are frozen in effect, because nothing reads them again. A local mount must therefore complete before that moment.
The failure is silent. The guidance path writes AGENT_MOUNT_ENV_VAR to tell the model that durable storage exists. A local mount that lands after the freeze writes into a map nobody reads, so the harness never learns about its own durable storage, and nothing throws.
AcquireInvariantError exists for this class. Every broad catch in these units calls rethrowIfInvariant first, so an invariant violation cannot be swallowed by a catch written for provider errors.
| * CONTINUITY IS BEST EFFORT, AND THAT IS DELIBERATE. A resume that throws is caught, logged, and | ||
| * falls through to a cold `createSession`. The worst outcome is a conversation that replays | ||
| * instead of resuming, which costs latency and never correctness. What the caller must NOT do is | ||
| * treat a successful `resumeSession` as proof that history loaded — see `loadedFromContinuity`. |
There was a problem hiding this comment.
This unit takes an explicit input rather than the acquire context, and the difference is deliberate.
The mount unit needs the context because its six helpers share mutable state and call each other. Nothing here does. Probing and opening are straight line, their inputs are read only, and their outputs are returned rather than assigned.
Threading the context through anyway would imply a coupling that does not exist, and the next reader would look for it.
One caution that predates this PR and stays true. A successful resumeSession does not prove that history loaded. loadedFromContinuity carries that distinction, and a caller that treats the two as the same will replay a conversation it believes was resumed.
| * helpers below were mutually recursive closures sharing one scope in `acquireEnvironment`; they | ||
| * are now ordinary functions that take `ctx` and capture nothing. | ||
| * | ||
| * ZERO BEHAVIOR CHANGE. Every guard, every log string, every early return and every ordering is |
There was a problem hiding this comment.
This whole file is a code move. Every guard, every log string, every early return, and every ordering is preserved.
The differences are mechanical and worth knowing while reading. Reads go through ctx.env. Writes go through a named committer. The two broad catches now begin with rethrowIfInvariant.
The two mounts stay asymmetric because they really are. The durable cwd is keyed by session, signed with signMount, and participates in the safe to delete handshake at teardown. The agent mount is keyed by artifact, lives at a sibling path, and carries the guidance. Only one of them runs each path, which is why these are separate functions instead of one function with a mount kind parameter.
d1ed417 to
716576f
Compare
78ef497 to
8b2ba65
Compare
|
@coderabbitai review |
|
716576f to
4e8d5c9
Compare
8b2ba65 to
bff61b2
Compare
4e8d5c9 to
21957e1
Compare
bff61b2 to
9d2ff4c
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
services/runner/src/environment/acquire-context.ts (1)
158-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused AcquireContext writers or give them callers.
environment.tsstill writesenvironment.closeToolMcpdirectly, andsetOtlpAuthFilePath/setCodexSqliteHomehave no TypeScript callers. Eliminate these methods if the runtime interface should only expose the complete writer list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e34935c-e571-4d82-871e-61cec1649c39
📒 Files selected for processing (9)
services/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/environment.tsservices/runner/src/environment/acquire-context-impl.tsservices/runner/src/environment/acquire-context.tsservices/runner/src/environment/harness-session-lifecycle.tsservices/runner/src/environment/mount-lifecycle.tsservices/runner/src/environment/runtime-lifecycle.tsservices/runner/tests/unit/acquire-context.test.tsservices/runner/tests/unit/environment-units.test.ts
| environment.destroy = async (opts?: { reason?: TeardownReason }) => { | ||
| if (environment.destroyed) return; | ||
| environment.destroyed = true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Create ctx before you assign environment.destroy.
environment.destroy is assigned at line 321 and calls ctx.recordCwdUnmountResult at line 356. ctx is a const declared at line 403, so it stays in the temporal dead zone until that line runs. Two consequences follow.
- If
createAcquireContextat line 403 throws, nocatchcovers lines 397-433.acquireEnvironmentthen rejects without callingenvironment.destroy, andplan.workspace.skillsCleanup()never runs. - Any later edit that calls
environment.destroy()before line 403 raisesReferenceError: Cannot access 'ctx' before initializationinstead of tearing down.
Move the createAcquireContext call and mountDeps above the environment.destroy assignment. The order of executed side effects does not change.
Also applies to: 398-403
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still current, through the tip of the stack: environment.destroy is assigned at the top of acquireEnvironment and closes over ctx, but const { context: ctx } = createAcquireContext(...) runs after that assignment and before the surrounding try block starts. If createAcquireContext throws, nothing calls environment.destroy, so plan.workspace.skillsCleanup() is skipped.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 29 minutes and 25 seconds before sending another message.
| export async function teardown(input: { | ||
| sandbox: { destroySession?: (id: string) => Promise<unknown> } | undefined; | ||
| session: { id: string } | undefined; | ||
| /** True when the pause path already sent its own cancel. */ | ||
| alreadyRequested: boolean; | ||
| }): Promise<void> { | ||
| if (!input.session || input.alreadyRequested) return; | ||
| await input.sandbox?.destroySession?.(input.session.id).catch(() => {}); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect implementations and teardown callers without executing repository code.
rg -n -C 6 '\bdestroySession\s*[:(]' services/runner/src
rg -n -C 6 '\bteardown\s*\(' services/runner/srcRepository: Agenta-AI/agenta
Length of output: 12458
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
async function chainedAfterSyncThrow() {
const sandbox = { destroySession: () => { throw new Error('sync destroy'); } };
await sandbox?.destroySession?.('id').catch(() => {});
}
async function caughtSyncThrowViaTry() {
const sandbox = { destroySession: () => { throw new Error('sync destroy'); } };
try {
await sandbox?.destroySession?.('id');
} catch {}
}
Promise.allSettled([chainedAfterSyncThrow(), caughtSyncThrowViaTry()]).then((results) => {
console.log(JSON.stringify(results, null, 2));
}).catch((err) => {
console.log('callback error:', String(err));
});
JS
echo '--- relevant harness teardown ---'
sed -n '145,175p' services/runner/src/environment/harness-session-lifecycle.ts
echo '--- destroySession definitions ---'
rg -n -C 4 'destroySession\s*:|destroySession\s*\(|export interface .*destroy|interface .*destroy' services/runner/srcRepository: Agenta-AI/agenta
Length of output: 2599
Catch synchronous destroySession errors.
A synchronous exception from destroySession happens before the returned promise is available for .catch, so teardown rejects. Use try/catch around the await to keep the “only refuses to throw” contract.
Proposed fix
- await input.sandbox?.destroySession?.(input.session.id).catch(() => {});
+ try {
+ await input.sandbox?.destroySession?.(input.session.id);
+ } catch {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function teardown(input: { | |
| sandbox: { destroySession?: (id: string) => Promise<unknown> } | undefined; | |
| session: { id: string } | undefined; | |
| /** True when the pause path already sent its own cancel. */ | |
| alreadyRequested: boolean; | |
| }): Promise<void> { | |
| if (!input.session || input.alreadyRequested) return; | |
| await input.sandbox?.destroySession?.(input.session.id).catch(() => {}); | |
| export async function teardown(input: { | |
| sandbox: { destroySession?: (id: string) => Promise<unknown> } | undefined; | |
| session: { id: string } | undefined; | |
| /** True when the pause path already sent its own cancel. */ | |
| alreadyRequested: boolean; | |
| }): Promise<void> { | |
| if (!input.session || input.alreadyRequested) return; | |
| try { | |
| await input.sandbox?.destroySession?.(input.session.id); | |
| } catch {} |
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still current, through the tip of the stack: teardown() still uses await input.sandbox?.destroySession?.(input.session.id).catch(() => {}), which only catches a promise rejection, not a synchronous throw from destroySession itself. The function's own docstring says it must never throw.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 30 minutes and 45 seconds before sending another message.
| export function removeRuntimeFiles(input: RuntimeFilesInput): void { | ||
| if (input.runAgentDir) | ||
| rmSync(input.runAgentDir, { recursive: true, force: true }); | ||
| if (input.otlpAuthFilePath) rmSync(input.otlpAuthFilePath, { force: true }); | ||
| if (input.codexSqliteHome) | ||
| rmSync(input.codexSqliteHome, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the rmSync calls so teardown cannot throw.
environment.destroy states it must never throw, and it calls removeRuntimeFiles at environment.ts line 389, before plan.workspace.skillsCleanup() at line 395. force: true suppresses ENOENT only. A rmSync on runAgentDir can still throw EACCES, EBUSY, or ENOTCONN, for example when the per-run dir sits under a stale FUSE node. The rejection then skips skillsCleanup(), leaks the skills temp root, and rejects destroy().
The sibling delete at environment.ts lines 368-377 already wraps rmSync in try/catch, so this unit is the outlier.
🛡️ Proposed fix
-export function removeRuntimeFiles(input: RuntimeFilesInput): void {
- if (input.runAgentDir)
- rmSync(input.runAgentDir, { recursive: true, force: true });
- if (input.otlpAuthFilePath) rmSync(input.otlpAuthFilePath, { force: true });
- if (input.codexSqliteHome)
- rmSync(input.codexSqliteHome, { recursive: true, force: true });
-}
+export function removeRuntimeFiles(
+ input: RuntimeFilesInput,
+ log: Log = () => {},
+): void {
+ const remove = (path: string | undefined, recursive: boolean) => {
+ if (!path) return;
+ try {
+ rmSync(path, { recursive, force: true });
+ } catch (err) {
+ // Teardown must always reach the end; a strand here is preferable to a rejected destroy.
+ log(`runtime file cleanup failed path=${path}: ${(err as Error).message}`);
+ }
+ };
+ remove(input.runAgentDir, true);
+ remove(input.otlpAuthFilePath, false);
+ remove(input.codexSqliteHome, true);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function removeRuntimeFiles(input: RuntimeFilesInput): void { | |
| if (input.runAgentDir) | |
| rmSync(input.runAgentDir, { recursive: true, force: true }); | |
| if (input.otlpAuthFilePath) rmSync(input.otlpAuthFilePath, { force: true }); | |
| if (input.codexSqliteHome) | |
| rmSync(input.codexSqliteHome, { recursive: true, force: true }); | |
| } | |
| export function removeRuntimeFiles( | |
| input: RuntimeFilesInput, | |
| log: Log = () => {}, | |
| ): void { | |
| const remove = (path: string | undefined, recursive: boolean) => { | |
| if (!path) return; | |
| try { | |
| rmSync(path, { recursive, force: true }); | |
| } catch (err) { | |
| // Teardown must always reach the end; a strand here is preferable to a rejected destroy. | |
| log(`runtime file cleanup failed path=${path}: ${(err as Error).message}`); | |
| } | |
| }; | |
| remove(input.runAgentDir, true); | |
| remove(input.otlpAuthFilePath, false); | |
| remove(input.codexSqliteHome, true); | |
| } |
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still current, through the tip of the stack: removeRuntimeFiles runs three bare rmSync calls with no try/catch, while the sibling delete in environment.ts (agent mountpoint cleanup) already wraps its rmSync and logs on failure. force: true only suppresses ENOENT, not EACCES/EBUSY/ENOTCONN.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 29 minutes and 21 seconds before sending another message.
| export interface BuildRuntimeEnvironmentInput { | ||
| plan: never; | ||
| request: never; | ||
| piSkillSnapshot: unknown; | ||
| log: Log; | ||
| deps: { buildDaemonEnv?: typeof buildDaemonEnv }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
BuildRuntimeEnvironmentInput declares plan and request as never, so the compiler checks nothing across this new module boundary. The root cause is the input type; the as never casts at the call site are its consequence. tsc --strict cannot catch a field rename in RunPlan or AgentRunRequest at either site.
services/runner/src/environment/runtime-lifecycle.ts#L128-L134: typeplanasRunPlanandrequestasAgentRunRequest, then delete theas unknown asstructural aliasespandrinbuildRuntimeEnvironment.services/runner/src/engines/sandbox_agent/environment-setup.ts#L180-L186: remove theplan as neverandrequest as nevercasts and passplanandrequestdirectly.
As per coding guidelines: "Maintain strict TypeScript correctness with tsc --strict".
📍 Affects 2 files
services/runner/src/environment/runtime-lifecycle.ts#L128-L134(this comment)services/runner/src/engines/sandbox_agent/environment-setup.ts#L180-L186
Source: Coding guidelines
There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still current, through the tip of the stack: BuildRuntimeEnvironmentInput still declares plan: never and request: never, so the as never / as unknown as casts at the call sites remain the only type check across this boundary.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 29 minutes and 27 seconds before sending another message.
| it("every operational catch rethrows an invariant violation first", () => { | ||
| // Without this the freeze throw dies in mountLocalAgentCwd's catch and the run continues | ||
| // with a harness that cannot see its durable storage. | ||
| const source = SRC("environment/mount-lifecycle.ts"); | ||
| assert.ok(source.includes("catch (err)"), "the unit still has an operational catch"); | ||
| assert.ok( | ||
| source.includes("rethrowIfInvariant(err)"), | ||
| "an operational catch must start with rethrowIfInvariant", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate each operational catch block.
The current assertion only proves that one rethrowIfInvariant(err) exists. A later catch (err) can swallow an AcquireInvariantError and this test still passes. Iterate over every catch block and assert that rethrowIfInvariant(err) is its first statement.
Proposed test change
- const source = SRC("environment/mount-lifecycle.ts");
- assert.ok(source.includes("catch (err)"), "the unit still has an operational catch");
- assert.ok(
- source.includes("rethrowIfInvariant(err)"),
- "an operational catch must start with rethrowIfInvariant",
- );
+ const source = CODE_ONLY(SRC("environment/mount-lifecycle.ts"));
+ const catches = [...source.matchAll(/catch\s*\(\s*err\s*\)\s*\{/g)];
+ assert.ok(catches.length > 0, "the unit still has an operational catch");
+ for (const match of catches) {
+ assert.match(
+ source.slice(match.index),
+ /^catch\s*\(\s*err\s*\)\s*\{\s*rethrowIfInvariant\(err\);/,
+ "an operational catch must start with rethrowIfInvariant",
+ );
+ }There was a problem hiding this comment.
Acknowledged, fix queued. Confirmed still current, through the tip of the stack: the test still asserts only that mount-lifecycle.ts contains the strings catch (err) and rethrowIfInvariant(err) somewhere, not that every catch block starts with it.
There was a problem hiding this comment.
Rate Limit Exceeded
@mmabrouk have exceeded the limit for the number of chat messages per hour. Please wait 29 minutes and 4 seconds before sending another message.
21957e1 to
706b0ad
Compare
6b52aeb to
6e7026c
Compare
706b0ad to
782b50f
Compare
6e7026c to
0e1f7d0
Compare
dosubot
Bot
added
size:XXL
Railway Preview Environment
|
e1ab55e to
3379ea2
Compare
1ffea8c to
7e44489
Compare
3379ea2 to
d650d49
Compare
7e44489 to
d4ea608
Compare
d650d49 to
5b5d744
Compare
d4ea608 to
8647567
Compare
5b5d744 to
0339026
Compare
8647567 to
41ff499
Compare
0339026 to
5a26619
Compare
41ff499 to
c773584
Compare
c773584 to
31fd7c1
Compare
956bf3e to
454342f
Compare
31fd7c1 to
5ef8fae
Compare
454342f to
3e8840b
Compare
5ef8fae to
8f0bc8b
Compare
8f0bc8b to
cabdc99
Compare
3e8840b to
7e460f7
Compare
cabdc99 to
3f4fc7e
Compare
…ss-session units (S7b) The six mutually recursive mount closures become top-level functions on the externally reviewed AcquireContext: read-only environment view, committer-only writes, an invariant error that escapes operational catches, and the daemon-env freeze enforced before the provider by a source-position test. environment-setup returns to a pure planner; daemon-env construction moves to the runtime unit with the assign-order rule documented. Zero behavior change; 52 seam tests; the stage-name guard proves all seven timing stages still fire.
…ordering, sync teardown throw, guarded removals, real boundary types, first-statement invariant assertion)
1 participant
Context
Part of the agent-config-editing stack. Targets
agent-config-editing-s2. Read the stack bottom up.Expect no behavior change. Every guard, every log string, every early return, and every ordering is preserved. The differences are mechanical: reads go through
ctx.env, writes go through a named committer, and the two broad catches now rethrow invariant violations first. The characterization and keep-alive suites pin the behavior across the move.s7a extracted the three separable units. The last three are not separable the same way. Mount, runtime, and harness session shared one closure holding six mutually recursive helpers.
mountLocalAgentCwdcallsactivateAgentMountGuidance, which writes the daemon environment and the plan's system prompt.reSignAndRemountLocalCwdre-signs a credential and then callsmountLocalDurableCwd. Splitting them into modules meant either importing each other or sharing state through something.Changes
AcquireContextis that something. Every helper takesctxand captures nothing, so the six helpers become ordinary functions in three modules with no module importing another.This is revision 2 of the context. An external security review rejected revision 1 because it stated its invariants in comments and exposed the mutable environment anyway. The reviewer's summary was exact: the type compiled but did not enforce what its comments promised. Revision 2 follows one rule, and the review's findings are recorded at the bottom of the file so the next reader sees what was tried and why it failed. If a comment says a unit may not do something, the type makes it impossible.
The three units:
mount-lifecycle.tsowns the durable cwd and the agent mount. They are not symmetric and the module says why: the cwd mount is keyed by session and participates in the safe-to-delete handshake, while the agent mount is keyed by artifact, lives at a sibling path, and carries the guidance that tells the model durable storage exists. One invariant is load-bearing. A local mount must complete before the sandbox provider takes the daemon environment maps by reference, or the guidance writes into maps nobody reads again.runtime-lifecycle.tsowns the four handles nothing else owns: the loopback tool-MCP server, the OTLP bearer file, the Codex SQLite home, and the per-run agent directory. It is small, and the file says plainly why. The daemon environment is built once and is immutable afterwards, so there is no restart and no credential refresh to own yet. Inventing arestart()that throws would suggest a seam that does not exist.harness-session-lifecycle.tsownsprobe_capabilitiesandcreate_sessionplus session teardown. It takes an explicit input rather than the context, because nothing here shares mutable state or calls a sibling. Threading the context through anyway would imply a coupling that is not there.Continuity stays best effort. A resume that throws is caught and falls through to a cold
createSession. The worst outcome is a replay instead of a resume, which costs latency and never correctness. What a caller must not do is read a successfulresumeSessionas proof that history loaded, andloadedFromContinuityis what carries that distinction.Tests / notes
acquire-context.ts. They are the argument for why the type looks the way it does, and a reviewer who skips them will want to simplify it back.