feat(app-server): sync managed Codex credentials (Phase 1) - #77
feat(app-server): sync managed Codex credentials (Phase 1)#77simonrosenberg wants to merge 18 commits into
Conversation
The previous guard suppressed onclose for every consumer of useWebSocket whenever the URL changed, which also swallowed setIsConnected and reconnect bookkeeping for sockets unrelated to credential binding. Compare the closing socket's conversation against the selected one at the single call site that acts on 1013 instead, so the shared hook keeps its existing behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…5394) Addresses review findings that the Phase 1 barrier changed behaviour for untagged sandboxes, which #15393 requires to keep their previous pause and delete semantics. - _prepare_for_pause: resolve the agent server URL inside the `required` guard so an unmanaged sandbox with no exposed URL still pauses and deletes. With this, a flag-off deployment does one conversation scan and no HTTP. - delete_sandbox: catch Exception, not just httpx.HTTPError, so a SandboxError from the barrier still commits the session key revocation. The barrier keeps running before the revocation, per merge gate 9. - resume_sandbox: drop the commit on the shared request session. It flushed the caller's staged work, so a failed managed delete durably removed the sub-conversations it had staged. The caller owns the transaction, as before. - Docker get_sandbox: restore APIError handling so a daemon hiccup degrades to missing rather than 500ing every conversation view. - Docker delete_sandbox: skip the barrier for an unresolvable container rather than raising past container.stop()/remove() and leaking it with its volume. - Managed conversation delete: treat MISSING like a missing sandbox, and stop re-raising per sub-conversation so one failure cannot orphan its siblings. - load_credential_binding: only an invalid credential maps to 422. A ValueError from an unreadable store surfaced as "please authenticate again". - SaaS store(): lock the Codex rows in the same order as replace_versioned. Also drops the fixture stub that made the key revocation tests vacuous and covers the raising-predicate and barrier-failure paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dropping the fixture-wide managed-marker stub left these three tests calling the real _has_managed_credential_conversation, which opens an admin injector against a conversation database CI has not initialised. Stub _prepare_for_pause per test instead of restoring the broad fixture stub, which would make the key revocation tests vacuous again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Revert the MISSING shortcut in managed conversation delete. Remote get_sandbox() swallows every runtime-API failure and reports MISSING, so a transient 503 is indistinguishable from a gone runtime and the shortcut could skip the final credential flush before dropping metadata. Absence is not definitive here, so fail closed. A managed conversation whose runtime is genuinely gone still cannot be deleted; distinguishing a definitive 404 from a transient error inside get_sandbox() is left as follow-up. - Docker pause/delete: losing the image tag hides the sandbox, not its binding, so query the marker instead of assuming either way. Managed fails closed; legacy keeps its previous fail-open behaviour. This replaces the unconditional failure on both paths, which pre-dated the barrier on neither. - _delete_sub_conversations: remember a managed child's failure, finish the sibling loop, then re-raise, so siblings are still deleted but the parent is not removed around an unflushed managed child. - Replace the conversation-URL comparison with a socket identity check in useWebSocket: only dispatch onClose when the closing socket is still the current one. This drops the URL parser and its tests and covers every stale socket callback rather than only the 1013 path. The provider regression test is kept and still fails without the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 20s deadline fails deterministically on a 2-CPU runner: four spawn processes each re-import the app tree, then serialise on the cross-process lock, while `-n auto --forked --cov` competes for the same two cores. Observed 6.3s locally when idle, 11-13s under load, and two consecutive CI failures with exitcode None (the process had not exited, so the CAS assertions never ran). Every assertion is unchanged, so the exactly-one-winner guarantee is still proven; the deadline stays bounded so a real lock bug still fails rather than hanging. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both CI failures were `waitFor(() => expect(isConnected).toBe(true))` at 1004ms and 1014ms — the 1000ms asyncUtilTimeout default. The msw WebSocket handshake does not reliably complete inside 1s on a 2-CPU runner, and this file has eight such waits, so any of them can fail. The file is already documented as CI-flaky (#11944) with four tests skipped for the same reason. Raised in beforeAll and restored in afterAll so the longer deadline cannot leak into other files sharing the worker. A longer deadline only delays a failure, so no assertion is weakened. Not reproducible locally: the file passes standalone, and in the full 2496-test suite, even under 10x CPU load. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
Follow-up with a concrete regression test:
I pushed commit 22bc0f1 to this branch. It adds a file-backed SQLite test with two independent SQLAlchemy sessions, following the async SQLite fixture pattern in enterprise/tests/unit/conftest.py and the real RemoteSandboxService pattern in TestDeleteSandboxKeyHandling.
The test fails both locally and in CI. CI ran 2,351 app-server tests: 2,350 passed, and test_resumed_key_is_visible_to_callback_session_before_request_commit was the only failure because get_sandbox_by_session_api_key returned None for the fresh key:
https://github.com/OpenHands/enterprise/actions/runs/30351645934/job/90250267607
This confirms that resume_sandbox only stages the rotated hash in the outer request transaction, while the credential callback authenticates through a separate request/session before that transaction can commit. Please revise the session-key rotation so the fresh hash is durably visible before the Agent Server activation probe, without committing unrelated work staged in the shared request session, and make the regression test pass.
Regression-test commit: 22bc0f1
Superseded by a self-contained PR comment addressed directly to the PR author.
|
@simonrosenberg I found a release-blocking transaction-visibility issue in the managed credential reactivation path. ProblemWhen a SaaS remote sandbox resumes, the runtime API returns a fresh session key. RemoteSandboxService.resume_sandbox updates session_api_key_hash on the SQLAlchemy object, but it does not commit that update. The activation endpoint then waits for the sandbox and calls activate_codex_credential_binding in the same outer request. Agent Server handles that activation by synchronously probing the credential callback with the fresh session key. The callback is a separate HTTP request with an independent database session, and its authentication path calls get_sandbox_by_session_api_key. Since the resume request has not committed yet, the callback still sees the old hash and rejects the fresh key. The outer request cannot commit until activation returns, while activation waits for this callback probe, so managed reactivation after a pause fails deterministically. The paused or error managed-delete path has the same resume-then-activate ordering. ReproductionI pushed this regression-test commit directly to the PR branch: The test follows the file-backed async SQLite pattern in enterprise/tests/unit/conftest.py and the real RemoteSandboxService setup in TestDeleteSandboxKeyHandling. It:
The second session returns None, demonstrating that the callback cannot authenticate the key that activation just supplied. The test fails identically locally and in CI. CI ran 2,351 app-server tests; 2,350 passed, and this regression was the only failure: https://github.com/OpenHands/enterprise/actions/runs/30351645934/job/90250267607 Requested revisionPlease persist the rotated session-key hash in a transaction that is visible to the callback before the Agent Server activation probe begins, and make the regression test pass. Please avoid indiscriminately committing the shared request session, since callers may have unrelated lifecycle mutations staged there. |
Managed reactivation failed deterministically. resume_sandbox staged the rotated session_api_key_hash on the shared request session without committing, then activation had the Agent Server synchronously probe the credential callback with the fresh key. The callback is a separate request with its own session, so it still read the old hash and rejected the key, while the outer request could not commit until activation returned. Persist the hash through a sibling session on the same engine and commit it there, so the callback observes it immediately without committing the caller's unrelated staged work -- which is what previously caused a failed managed delete to durably remove its sub-conversations. The commit runs before the value is staged locally: were it staged first, an autoflush would leave the outer transaction holding a write lock on that row and the two transactions would block each other. A single-writer backend still cannot commit while the request session holds a write, which pause_old_sandboxes creates whenever the user is over the sandbox cap. That now raises a SandboxError instead of escaping as a raw OperationalError, so it is a retryable failure rather than a resume that reports success while the callback keeps rejecting the key. Covers the paused/error managed-delete path too, since it resumes through the same method. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — diagnosis confirmed, and this was my regression. Fixed in CauseAn earlier review pass flagged that Fix
One ordering detail worth naming: the commit runs before the value is staged locally. Staged first, an autoflush would leave the outer transaction holding a write lock on that row — and since the outer request is waiting on activation, which waits on the callback probe, the two transactions would block each other. That inversion is the deadlock your report describes, just relocated. Your second point is covered without a separate change: the paused/error managed-delete path resumes via the same method ( A limitation I found in my own fix — please sanity-check this callI wrote an adversarial test and it failed, so flagging it rather than leaving it latent. A single-writer backend cannot commit while the request session already holds a write, and Rather than paper over it, that path now raises The clean structural fix would be to make Verification
Note the e2e I ran yesterday would not have caught this: local Docker does not rotate session keys on resume, so the callback never saw a stale hash. Only SaaS remote exercises it. |
|
@OpenHands /codereview-roasted read latest comments make sure no bugs were introduced when fixing the bugs pointed out by neubig. |
|
I'm on it! simonrosenberg can track my progress at all-hands.dev |
|
@OpenHands /codereview-roasted read latest comments make sure no bugs were introduced when fixing the bugs pointed out by neubig. |
|
I'm on it! simonrosenberg can track my progress at all-hands.dev |
simonrosenberg
left a comment
simonrosenberg
left a comment
There was a problem hiding this comment.
🔴 Needs improvement
GitHub does not permit the PR author account to submit
REQUEST_CHANGESon its own PR. ThisCOMMENTreview is intended as blocking feedback.
[CRITICAL ISSUES]
- Concurrent remote resumes can restore an invalidated session-key hash after the new isolated commit. See the inline finding.
[TESTING GAPS]
- The new visibility and rollback tests pass, but neither overlaps two request sessions through their final request commits. Add a regression that proves the newest runtime rotation remains canonical after both outer sessions finish.
Focused verification: 4 relevant remote-sandbox persistence tests passed. I also reproduced the race with the real StoredRemoteSandbox, _commit_session_api_key_hash, and two independent AsyncSessions: helper A committed hash-A, helper B committed hash-B, outer B committed, then outer A committed; the final database value was hash-A.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This PR changes authentication, secret synchronization, and sandbox lifecycle ordering. The demonstrated race can make the canonical hash disagree with the only live runtime key, deterministically breaking callback authentication under overlapping resumes. Recommendation: Do not auto-merge. Request a human reviewer to validate the transaction/identity-map fix and its concurrency test.
VERDICT:
❌ Needs rework: The visibility fix solves Neubig’s single-request failure but introduces a stale outer-transaction overwrite under concurrency.
KEY INSIGHT:
An isolated commit is not enough if the original request still marks the same ORM attribute dirty and flushes it later.
This review was created by an AI agent (OpenHands) on behalf of the requester.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs.- Re-request a review; the reviewer reads guidelines from the PR branch.
- When merged, the guideline file goes through normal maintainer review.
Resolve with AI? Install the iterate skill and run
/iterateto drive CI, review, and QA.Was this review helpful? React with 👍 or 👎 to give feedback.
|
Reviewed PR #77 and posted a blocking review: #77 (review) Key finding: the fix commits the rotated hash in a sibling transaction, then marks the same ORM attribute dirty in the outer request. Two overlapping resumes can commit out of order and restore an invalidated older hash. I reproduced this with the real model/helper and two Verification:
The review was submitted as |
…77) Committing the rotated hash in a sibling transaction was not sufficient: the value was then assigned back onto the request-scoped ORM row, leaving the attribute dirty so the outer session re-emitted it at request exit. Two overlapping resumes could therefore commit out of order -- A rotates, B rotates and invalidates A's key, then A's request commits and restores A's hash, so the callback rejects the only live runtime key. Use set_committed_value so the identity map reflects the persisted value without marking it dirty. The sibling transaction stays the only writer of this column. Adds an overlapping-resume regression that drives two request sessions through their final commits in reverse order and asserts the newer hash survives; it fails against the previous assignment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@OpenHands /codereview-roasted |
|
I'm on it! simonrosenberg can track my progress at all-hands.dev |
simonrosenberg
left a comment
simonrosenberg
left a comment
There was a problem hiding this comment.
🟢 Good taste
GitHub does not permit the PR author account to submit APPROVE on its own PR. This COMMENT review records an approval-equivalent, non-blocking verdict.
The latest fix in 184cc3208 correctly closes the prior race: set_committed_value updates the request-scoped identity map without marking session_api_key_hash dirty, so the outer request cannot re-emit an older hash after the sibling transaction commits. The new SQLite-backed overlap regression fails under plain assignment and passes with the committed-value update. I found no bug introduced by this correction.
Focused verification:
- 5 targeted persistence tests passed: callback visibility, rollback-safe invalidation, blocked isolated persist, persistence across outer rollback, and reversed outer-commit overlap.
- Exact-head CI is green for Linux Python, enterprise Python, frontend unit/E2E, Docker, and all lint jobs.
- The upstream runtime API atomically permits only one
PAUSED/ERROR→STARTINGtransition, so normal concurrent resume requests cannot both rotate a key.
VERDICT:
✅ Worth merging: The previously blocking transaction-visibility and stale outer-flush defects are both resolved.
KEY INSIGHT:
The isolated transaction must be the sole writer of the rotated hash; the request session may observe that value but must never flush it.
This review was created by an AI agent (OpenHands) on behalf of the requester.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This remains intrinsically high-risk because it changes authentication, secret synchronization, sandbox lifecycle ordering, and recovery across backend and frontend boundaries. The implementation has focused database-backed regressions, exact-head CI, and documented end-to-end evidence, but it should not be auto-merged. Recommendation: Require human reviewer/architect sign-off on the credential-binding protocol and production SaaS lifecycle behavior.
|
Reviewed PR #77 and posted an approval-equivalent review: #77 (review)
Security note: the repository remote initially contained an embedded GitHub credential, which appeared in command output. I sanitized the remote to |
resume_sandbox calls pause_old_sandboxes to make room, but that pass could select the very sandbox being resumed: the resume endpoint has no status guard, so a client can resume an already-running sandbox, and if the user is at the cap and that sandbox is their oldest it gets picked. pause_sandbox then stages session_api_key_hash = None on the request session. The following _get_stored_sandbox autoflushes it, so the request holds a row lock on that row, and the sibling transaction that commits the rotated hash blocks behind it while the request is awaiting that very commit. PostgreSQL cannot report this as a deadlock -- one side is an application await, not a lock cycle -- so the request hangs on the lock instead of failing. Exclude the target from the pause pass. This also drops a pointless pause-then-resume of the same sandbox, which would have discarded its key. Both pause_old_sandboxes implementations needed it; patching only the base class left RemoteSandboxService's override unchanged, which the new test caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correction: SQLite framing was wrong, and there is a real PostgreSQL hangPushed In my earlier note on The actual bug
PostgreSQL can't report this as a deadlock — one side is an application FixExclude the target from the pause pass. It also removes a pointless pause-then-resume of the same sandbox, which would have thrown away the key we're about to rotate. Worth noting: Verification
The |
Code review (high effort, workflow-backed)10 verified findings, most severe first. Correctness / security1. Session-key hash invalidated after fallible network calls, not before — 2. 3. Partial sub-conversation deletion surfaces as a misleading 404 — 4. New activate endpoint 404s if 5. 6. Router duplicates resume/status logic, diverges from the service on 7. (Plausible, not fully confirmed) Deleted invariant against auto-resume on WS disconnect — Cleanup8. Fail-open/closed branch duplicated 6x in one method — 9. Unconditional full DB scan on every pause/delete for a yes/no check — 10. Multi-paragraph narrative docstrings — One duplicated-CAS-preserve-logic finding across |
Triage of the 10 findings, plus why this PR keeps not convergingI verified all 10 findings from the review above against the code at head ( The 10 findings
Four findings no review pass has reportedThe first two are conversation-bricking, and I only realised how severe after reading the SDK's latching behaviour in 1.37.1:
So the SDK contract is: 403 is clearable by reactivation; 404, 422 and 5xx are permanent. Given that:
Why this keeps not convergingFive mechanisms, in increasing order of importance. "Clean" has never meant what it sounds like. The 07-30 07:41 review says "No regression found in There is no executable definition of correct. #15393 has 12 numbered merge gates. None exists as a named test. So every reviewer re-derives the spec from the code, and the code's local reasoning always admits a different reading than the prose. Findings 1, 8 and 9 are the same reviewer-vs-spec collision three times. The design mandates contradictory invariants and the code sits on the seam. "Revoke the session key up front" (the comment this PR deleted from Every bug that actually shipped lives in the one layer with no real tests. The four regressions found by humans and bots — transaction visibility, identity-map re-flush, The deepest one. PlanThree subtractive design changes. Each deletes code and turns a behaviour into a structural invariant. All three are app-server-side — no SDK change, 1.37.1 stays pinned, and the #15393 cut ("no runtime-api or Agent Server changes") holds. A. Take B. One reactivation call site. Extract C. Make the barrier conditional at the top, not fail-open six times inside. Then decide finding 1 once, explicitly. Gate 9 versus prompt revocation is a security trade-off with a real number attached — barrier timeout plus two round trips. Pick it, bound the timeout, write One rule the design never stated, and should: the callback may only emit a non-403 failure when the condition is definitive and permanent. Anything transient must succeed, retry internally, or return 403. That rule is testable, which is more than gate 9 currently is. Process, and this matters more than the code:
Two things I'd want from the SDK eventually, neither blocking: Splitting out the store/CAS layerPer point 3, I'm opening a separate PR for the store and CAS layer — That PR is a straight extraction: it carries |
|
Store/CAS layer split out as #100 — eight files, byte-identical to this PR's head ( Its description lists the three known issues it carries forward unchanged rather than fixing — the two sticky-404 bricking bugs and |
|
splitting this PR into this smaller one: #100 |
|
Closing, evidence-based — not a judgment on the code quality here, which per the review history is solid. This is the enterprise-side wiring for the write-back design in #70 (now closed). #120's production telemetry (Status & Closing Plan) found the mechanism this addresses — cross-conversation Codex credential rotation — affects at most 1 of 78 observed sandboxes, and that the credential-binding machinery this depends on already auto-activates and works for the case that actually occurs (sandbox reuse with an already-persisted local credential) via Building the external HTTP activation endpoint / CAS-backed write-back path this PR adds isn't justified by what production actually shows. Re-open if new evidence changes that — the design itself isn't wrong, it's just not what the data says to build right now. |
OHE-3025 Codex auth.json: production evidence says re-scope OHE-2794 (config + data loss, not credential sync)
Telemetry-first investigation of the Codex Full writeup: enterprise#120. This issue is the OHE-side summary of what the logs actually show and what to do next. HeadlineOf 73 sandboxes that started Limits on all numbers below
New information from the logsPopulation — small feature
Two distinct failure modesA.
B.
Zero-hit signatures, with a control that makes the zeros meaningfulAll of: Control: False leads ruled out
Live cluster findings (read-only kubectl)
Code findings
Recommendations
Correction issued after further checkingPosted at enterprise#120 comment. Two claims revised, one in each direction:
Unchanged: #101's Codex allowlist and #70's rotation premise. sdk#4171 states its own status — "No production incident has been attributed to this race yet" — which the window confirms. AskRe-scope OHE-2794 from a credential-synchronisation problem to a configuration + data-loss problem, and sequence recommendation 1 ahead of the #70 design work. |
2 participants
HUMAN:
Re-targeted from OpenHands/OpenHands#15394 after the Agent Canvas repo transition moved this code into OpenHands/enterprise. Identical commits, no code changes — the branch fast-forwards onto
mainwith zero conflicts.AGENT:
Original PR: OpenHands/OpenHands#15394 (now unmergeable — its base branch became Agent Canvas).
Design: OpenHands/OpenHands#15393. Supersedes OpenHands/OpenHands#15287.
Why
Codex can rotate a conversation-local ChatGPT
auth.jsonfrom R0 to R1 while the canonical savedCODEX_AUTH_JSONremains R0. A later isolated runtime then starts from an invalidated refresh token.This is the narrow Phase 1 slice defined in #15393, which replaced #15287 (83 files, >10k added lines — effectively unreviewable). The pinned SDK/Agent Server 1.37.1 owns the private
CODEX_HOME, monitoring, masking, CAS flush, final scrub, and cold activation guard; OpenHands supplies only a versioned canonical store, scoped activation, and lifecycle ordering.Summary
CODEX_AUTH_JSONin SaaS SQL and supported OSS stores; preserve a runtime rotation when an unrelated stale whole-secret document is saved.Issue Number
OpenHands/OpenHands#15393
How to Test
Unit tests (both pass on a clean checkout):
End-to-end validation was run on the original PR against the unmodified pinned SDK/Agent Server 1.37.1 release (
99342c4), built intoopenhands-agent-server-e2e:99342c4-1.37.1-python-source-minimal, using a copied local ChatGPT credential without printing it:Type
Notes
Complexity boundary. 19 production files and roughly 1.5k added production lines, versus 83 files and >10k in #15287. No reservation/lease rows, migrations, schedulers, sibling scans, resume-task claiming, generic credential broker, pending-message changes, or duplicated SDK monitor/merge logic. OSS process sandboxes remain legacy. Docker retains its existing single-user container session key and receives a fresh scoped JWE on activation; Phase 1 does not claim pre-pause JWE rejection for Docker. SaaS remote rotates the runtime session key before reactivation.
Review history carried over from #15394. Two independent review passes (5 and 15 findings) were triaged against the #15393 design; 2 findings were withdrawn as describing intended behaviour, and a further review pass corrected three of the resulting fixes. The last four commits are that convergence:
60afce766d24d48b40useWebSocketd5e880415resume_sandbox; DockerAPIErrorhandling restored; SaaS Codex row lock order aligned153494549Two deliberate non-fixes, both design-mandated:
_prepare_for_pauseis not gated oncodex_credential_sync_enabled(). #15393 §Rollout requires reactivation to ignore the flag for already-marked conversations so a rollback preserves availability. The residual cost with the flag off is one indexed conversation scan per pause and no HTTP. Please do not optimise this away with a cache, an index, or a marker column on the sandbox row.Known follow-up (not Phase 1).
RemoteSandboxService.get_sandboxmaps every runtime-API failure toMISSING, so a transient 503 is indistinguishable from a gone runtime. Managed conversation delete therefore fails closed onMISSING, which means a managed conversation whose runtime is genuinely gone cannot be deleted. Teachingget_sandboxto distinguish a definitive 404 from a transient error touches every caller and belongs in its own PR.CI. The last three commits never got a CI signal on #15394: once its base branch became Agent Canvas the PR went
CONFLICTING, so GitHub could not buildrefs/pull/N/mergeand nopull_requestworkflow could run. This PR is the first real signal for them. Theenterprise/storage/saas_secrets_store.pylock-order change is the one edit with no local test coverage — the enterprise suite could not be run locally.Enterprise server image for this PR: