feat(runner): rebase JP's session turns/streams runner onto the decomposed runner#5376
feat(runner): rebase JP's session turns/streams runner onto the decomposed runner#5376mmabrouk wants to merge 3 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:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/runner/src/sessions/alive.ts (1)
198-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize heartbeats with
release().The async interval permits overlapping requests, and
release()only clears future ticks. An in-flightis_running: trueheartbeat can therefore complete after the finalis_running: falseheartbeat and leave the stream marked running. Track and await the active beat before sending the release heartbeat, or replace the interval with a serialized loop.services/runner/src/engines/sandbox_agent/run-turn.ts (1)
543-574: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdvance and persist completed turns without an
agentSessionId.The outer guard suppresses both counter advancement and the append when a harness omits its optional native session ID. This can reuse
turn_indexand leave durable sandbox/continuity state stale.Record the harness session conditionally, but always advance and append a successful session-owned turn.
Proposed fix
if ( stopReason !== "paused" && env.continuityTurnIndex !== undefined && - sessionId && - env.session?.agentSessionId + sessionId ) { - (deps.sessionContinuityStore ?? sessionContinuityStore).record( - sessionId, - plan.harness, - env.session.agentSessionId, - env.continuityTurnIndex, - ); + const continuityStore = + deps.sessionContinuityStore ?? sessionContinuityStore; + const agentSessionId = env.session?.agentSessionId; + if (agentSessionId) { + continuityStore.record( + sessionId, + plan.harness, + agentSessionId, + env.continuityTurnIndex, + ); + } else { + continuityStore.restoreLatestTurn( + sessionId, + env.continuityTurnIndex, + ); + } + const syncCred = runCredential(request); if (syncCred && request.streamId) { const workflowRefs = buildWorkflowReferences( request.runContext?.workflow, ); void (deps.appendSessionTurn ?? appendSessionTurn)( sessionId, plan.harness, env.continuityTurnIndex, { streamId: request.streamId, - agentSessionId: env.session.agentSessionId, + agentSessionId, sandboxId: env.sandbox?.sandboxId,
🧹 Nitpick comments (2)
services/runner/tests/unit/server.test.ts (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicated
listenhelper totests/utils/.The
listentest helper is duplicated across these test files. As per coding guidelines, shared test helpers and fixtures should be placed undertests/utils/. Consider extracting this function into a shared utility module to keep the test environment setup DRY.
services/runner/tests/unit/server.test.ts#L46-L52: Remove this function and import it fromtests/utils/.services/runner/tests/acceptance/server-contract.test.ts#L44-L49: Remove this function and use the extracted helper.Source: Coding guidelines
services/runner/tests/unit/session-alive-interrupt.test.ts (1)
55-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise interruption through the interval heartbeat.
This starts a second watchdog with an already-interrupted first response; it does not test the stated “next beat reports cancellation” path. Use fake timers, start with
true, switch tofalse, advance one interval, and assert the callback fires once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0a8f85c-70dc-4169-b2ee-f458e5222027
📒 Files selected for processing (19)
services/runner/src/engines/sandbox_agent/environment.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/sandbox-reconnect.tsservices/runner/src/engines/sandbox_agent/session-continuity-durable.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/src/sessions/alive.tsservices/runner/src/sessions/persist.tsservices/runner/src/version.tsservices/runner/tests/acceptance/server-contract.test.tsservices/runner/tests/integration/server-smoke.test.tsservices/runner/tests/unit/sandbox-lifecycle.test.tsservices/runner/tests/unit/sandbox-reconnect.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-alive-interrupt.test.tsservices/runner/tests/unit/session-alive.test.tsservices/runner/tests/unit/session-continuity-durable.test.tsservices/runner/tests/unit/session-persist.test.ts
| // The heartbeat response already carries the session_streams row id — free, no extra | ||
| // round-trip. Thread it onto the request so the engine's turn-append write has it. | ||
| request.streamId = watchdog.streamId(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate stream IDs discovered after the first heartbeat.
This snapshots the accessor only once. After a fail-open first heartbeat, a later successful beat updates the watchdog but leaves request.streamId undefined, causing run-turn.ts to skip the durable turn append. Update the request through a watchdog callback, or read the latest ID at the append boundary.
| // Await the FIRST beat so streamId is ready before the caller starts the turn. | ||
| const first = await sendHeartbeat(sessionId, turnId, credential); | ||
| handleBeat(first); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the first heartbeat before awaiting it on the run-start path.
sendHeartbeat() has no timeout, so a stalled API response now prevents the agent turn from starting indefinitely. Add a finite request deadline while retaining the existing fail-open result.
28ce10a to
df38c4a
Compare
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Reviewed the full runner delta (19 files) line by line against the decomposition. Verdict: the re-homing is faithful and the design is genuinely better than what it replaces.
What I verified:
- The three monolith hunks landed in the right modules: the
SandboxAgentDepsnarrowing inruntime-contracts.ts(write/clear pointer deps correctly deleted, read side kept for the reconnect ladder), the twoacquireEnvironmentbranches inenvironment.ts, and the turn-completion append inrun-turn.ts. - The pointer-write deletion is sound reasoning, not just a port: with append-only turns, the fresh sandbox's own turn row supersedes the dead pointer by ordering, so the old staleness guard genuinely dissolves. The explanatory comments left in place are exactly right.
server.tskeeps the decomposition's import split while adding the real behavioral win of this PR:startAliveWatchdogis awaited, andonInterruptedwires the control-plane cancel tocontroller.abort(), so a kill/steer against a session finally reaches the in-flight run. The heartbeat'sstream_idrides the existing round-trip, no extra call.- Tests: 1204/1204 including the new persist contract test whose stub enforces the API's 16-hex span-id contract (the exact seam the live QA caught).
One non-blocking note (inline): the fire-and-forget .catch(() => {}) on the turn append is fine only as long as sessions/persist.ts logs terminal failure after its retries; it does today, keep it that way, because a silent drop here is precisely how the 422 stayed invisible.
| }, | ||
| { authorization: syncCred, log: logger }, | ||
| ); | ||
| ).catch(() => {}); |
There was a problem hiding this comment.
The swallow here is acceptable only because sessions/persist.ts logs the terminal failure after its retries. Worth a one-line comment saying so, since this exact silent-drop shape is how the span-id 422 went unnoticed until live QA.
9e2044c to
3f975df
Compare
df38c4a to
3e0edee
Compare
dosubot
Bot
added
the
size:XL
3f975df to
b90e9e8
Compare
3e0edee to
46f5f6e
Compare
Railway Preview Environment
|
46f5f6e to
03db31c
Compare
Rebases the runner third of JP's feat/sessions-extensions (PR #5363) onto current main, ported into the post-decomposition module layout (PR #5369). JP wrote these against the old monolithic sandbox_agent.ts; they are re-homed by region into the decomposed modules: - The SandboxAgentDeps interface change (syncHarnessSessionDurable renamed to appendSessionTurn; the write/clear sandbox-pointer deps dropped) goes into runtime-contracts.ts. - The two acquireEnvironment branches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) go into environment.ts. - The turn-completion sync (syncHarnessSessionDurable replaced by appendSessionTurn with the streamId gate, workflow references, sandbox id, and trace id) goes into run-turn.ts. The sandbox-reconnect.ts and session-continuity-durable.ts rewrites apply as-is. protocol.ts, sessions/alive.ts, sessions/persist.ts, version.ts, and the tests are JP's versions verbatim. Ported from feat/sessions-extensions@1532ec7fe5 (JP). Reconciliations with the release main: - server.ts: kept main's session-identity / session-pool import split from the decomposition and applied JP's watchdog changes (startAliveWatchdog is now awaited and takes an onInterrupted callback that aborts the controller, request.streamId is threaded from the heartbeat, and buildPersistingEmitter gets turnId and span_id). - tracing/otel.ts, package.json, pnpm-lock.yaml: kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the #5364 trace-id-on-done feature that JP's branch predates. Verified: pnpm run typecheck clean, pnpm test 1202/1202 pass, build:extension ok. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
…persist test Add a persist test whose fetch stub enforces the API contract (span_id must be a 16-hex OTel span id, else 422), so a regression to a non-span-shaped span_id fails here instead of silently dropping records. The old stub returned 200 for any body and hid the mismatch. Also switch the turn/span tagging test to a real 16-hex span id. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
573e236 to
494cdd7
Compare
03db31c to
cae71e4
Compare
1 participant
What this is
The runner third of JP's #5363, rebased onto v0.105.5 and ported into the post-decomposition module layout (#5369). JP wrote these edits against the old monolithic
sandbox_agent.ts(2,477 lines); main has since split it intoenvironment.ts/run-turn.ts/runtime-contracts.ts/ … behind a 44-line facade. This PR re-homes JP's hunks into those modules.Stacked on the backend PR (#5375); the base is
sessions-rebase/backend, so this diff shows only the runner delta.How JP's monolith edits were re-homed
SandboxAgentDepsinterface change (renamesyncHarnessSessionDurable→appendSessionTurn; drop the write/clear sandbox-pointer deps) →runtime-contracts.ts.acquireEnvironmentbranches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) →environment.ts.syncHarnessSessionDurablereplaced byappendSessionTurnwith thestreamIdgate, workflow references, sandbox id, and trace id) →run-turn.ts.sandbox-reconnect.tsandsession-continuity-durable.tsrewrites apply as-is.protocol.ts,sessions/alive.ts,sessions/persist.ts,version.ts, and the tests are JP's versions verbatim.Ported from
feat/sessions-extensions@1532ec7fe5(JP); attribution is in the commit.Reconciliations with the release
server.ts— kept main'ssession-identity/session-poolimport split (from the decomposition) and applied JP's watchdog changes:startAliveWatchdogis now awaited and takes anonInterruptedcallback that aborts the controller (W7.4),request.streamIdis threaded from the heartbeat, andbuildPersistingEmittergetsturnId+span_id.tracing/otel.ts,package.json,pnpm-lock.yaml— kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the [5357] fix(runner): recognize client-tool settles as a resume in cold replay #5364 trace-id-on-donefeature that JP's branch predates. Keeping main's preserves both behaviors, so no port was needed.Testing
pnpm run typecheck: clean.pnpm test: 1202/1202 pass (77 files; the +10 over main's 1192 are JP's new session tests).pnpm run build:extension: ok.For JP
Mechanical rebase of your #5363 runner delta onto the decomposed runner, on a branch I own (
sessions-rebase/runner). Please review the re-homing and the two reconciliations — I did not touch your branches.https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
QA finding fixed: record-ingest 422 (span_id typed as UUID)
QA found the runner's record ingest failed 100% with HTTP 422: the API typed
span_idas UUID, while the runner correctly sends the real 16-hex OTel span id (runContext.trace.span_id). Seedebug/qa-sessions-rebase/FINDING-record-ingest-422.md. The API-side fix lands in the backend lane (#5375); there is no runner behavior change — the runner keeps sending the real span id verbatim.This lane adds a persist contract test (commit
df38c4a709) whose fetch stub enforces the API contract —span_idmust be a 16-hex OTel span id or the stub returns 422 — so a regression to a non-span-shaped span_id fails the test instead of silently dropping records. The old stub returned 200 for any body and hid the mismatch; the turn/span tagging test now uses a real 16-hex span id.pnpm testis now 1204/1204 (was 1202).Post-QA amendment (Step 1 of the approvals-incident fixes):