fix(runner): let an environment own the configuration it installed - #5746
fix(runner): let an environment own the configuration it installed#5746mmabrouk wants to merge 1 commit 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 runner now tracks the configuration applied to each environment. Session reuse and reparking use that state. Lifecycle mismatches map to park or delete actions. Destroyed sandbox IDs block stale reconnects. ChangesSession lifecycle migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RunnerServer
participant SessionPool
participant SessionEnvironment
participant Teardown
RunnerServer->>SessionPool: evaluate reusable session
SessionPool->>SessionEnvironment: read applied configuration
SessionEnvironment-->>SessionPool: return applied fingerprint
SessionPool-->>RunnerServer: reuse or report mismatch
RunnerServer->>Teardown: classify mismatch
Teardown-->>RunnerServer: park or delete environment
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
| * That path was always correct and stays correct. This is a latency fix with a correctness-shaped | ||
| * name, and treating it as a durable guarantee would be wrong. | ||
| */ | ||
| const destroyedSandboxIds = new Set<string>(); |
There was a problem hiding this comment.
This set is a latency fix with a correctness shaped name, and the distinction matters.
The stored sandbox pointer is the latest turn's sandbox_id, and the turns table is append only. There is no pointer row to clear when a destroy deletes the sandbox, so the dead id stays readable until the next turn appends its own row. Reconnecting to it fails and falls through to a fresh create.
That fallback was always correct and stays correct. This set only skips the round trip, inside one runner process.
It is in memory and per process. Another replica, or this replica after a restart, still reads the stale id and still falls through. Treating this as a durable guarantee would be wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
fc8740d to
dabe419
Compare
dabe419 to
8886dd9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
dosubot
Bot
added
size:L
Railway Preview Environment
|
8886dd9 to
df3dc1e
Compare
df3dc1e to
1a5e248
Compare
1a5e248 to
8fe8b6c
Compare
8fe8b6c to
98b9826
Compare
98b9826 to
9abf2a0
Compare
9abf2a0 to
2c90d37
Compare
…s 1-2) The environment now owns its applied state; park and repark no longer accept request-derived fingerprints, so the approval-stale-config bug is unrepresentable. Four named teardown reasons with a parkable allowlist: a config change stops the sandbox instead of deleting it, and true incompatibility still destroys with the reconnect pointer cleared. Revision id, version, and draft flag leave the fingerprint, so a commit with identical content keeps the warm session.
2c90d37 to
4a24d43
Compare
1 participant
Context
Part of the agent-config-editing stack. Targets
agent-config-editing-plan. Read the stack bottom up.A warm session could serve a turn while running a different configuration than the one the request asked for. The session pool stored a
configFingerprintthat its caller passed in, and the caller passed the incoming request's fingerprint. On the ordinary path that is harmless, because the dispatch has already proved the incoming and parked configurations equal. On the approval-resume path it is not. That branch never compares configurations, so re-parking stamped the environment with a configuration it had never applied. The next turn read that stamp, found a match, and continued warm on an environment running something else.Two smaller costs sat next to it. Committing a revision mid-conversation threw away a warm sandbox, because the workflow revision id was part of environment identity. And every configuration mismatch deleted the sandbox, even when the sandbox itself was fine.
Changes
The environment now owns its applied state and the pool reads it.
ParkInputhas noconfigFingerprintfield for a caller to fill in, andLiveSession.configFingerprintis a read-only view ofenvironment.appliedState. The bug is not patched, it is unrepresentable: there is no longer a place to write a configuration the environment never installed.commitAppliedis the only writer, and it runs after a successful acquire.Before:
pool.park({ key, environment, configFingerprint: configFingerprint(request), ... })After:
pool.park({ key, environment, ... }), and the pool readsenvironment.appliedState.configFingerprint.configFingerprintno longer hashes the workflow revision id, the revision version, or the draft flag. Those are turn metadata, not environment identity. Nothing in the sandbox, the daemon, the workspace, or the harness session changes when a revision id changes, so committing a revision now keeps the warm session instead of rebuilding it. They stay inrunContextfor tool binding and observability.Teardown reasons now name the layer that failed instead of collapsing into one
compatibility-mismatch. A wrong harness session or an invalid conversation parks the sandbox, because the daemon and its installed credentials are sound. Stale daemon material or a wrong sandbox still deletes. The parkable set is an allowlist, so a new reason deletes until somebody proves its sandbox is safe to reuse.compatibility-mismatchstays as a deprecated alias that deletes.The runner also remembers the sandbox ids it deleted, so a stale pointer to one of them skips the failed reconnect and goes straight to a fresh create. This is per-process and in-memory. Another replica still reads the stale id and still falls through correctly. It saves a round trip; it is not a durable guarantee.
Tests / notes
What to QA