feat(secrets): add versioned compare-and-swap to the secrets stores - #100
feat(secrets): add versioned compare-and-swap to the secrets stores#100simonrosenberg wants to merge 2 commits into
Conversation
Split out of #77 so the store layer can be reviewed and landed on its own. The files are byte-identical to that PR's head (77a7fd9); no behaviour is added or changed here. - SecretsStore gains load_versioned/replace_versioned and CredentialVersionConflict, both defaulting to NotImplementedError so unsupported stores keep the legacy path. - FileStore gains supports_locked_update/locked_update; LocalFileStore implements it with flock (msvcrt on Windows), InMemoryFileStore with an RLock. - FileSecretsStore stores an opaque random generation alongside the secret document and does read/compare/write under that lock. - SaasSecretsStore locks all matching Codex rows, treats the newest duplicate as canonical, checks its row generation, and rewrites duplicates together. The two new endpoints have no callers until #77's callback router lands, so this is inert on merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Follow-up for note 3 is up as #101, stacked on this branch: it replaces the remembered-baseline guard with a structural one, which deletes Worth knowing while reviewing this PR: I proved both failure modes against this branch's head, as regression tests on #101 — (b) is Also: the enterprise suite does run locally, contrary to what this PR's description says. Only test-only deps were missing from the root |
|
@OpenHands /codereview-roasted
Review #100 first, then #101 as a delta. #101's base is Grounding:
Merged SDK PRs these pin against (Agent Server 1.37.1 — do not assume it can change):
Predecessor: #77 — these two were split Two known-deferred items are documented in #101's description — Highest-value focus: the |
|
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.
🟡 Acceptable — The CAS implementation and real concurrency coverage are solid, but an existing read path now requires write access.
[CRITICAL ISSUES]
- One blocking read-only regression is documented inline. This is distinct from the known-deferred
load_versionedbootstrap write: ordinaryload()now creates a lock sidecar.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This touches canonical secret persistence and breaks an existing read-only deployment mode on a request-reachable path, despite the PR's inertness claim. Recommendation: Do not auto-merge; have a human reviewer validate the read-only persistence behavior after the fix.
VERDICT:
❌ Needs rework: Restore read-only ordinary loads before merging.
KEY INSIGHT:
Serialize read/compare/write mutations, but do not make an ordinary atomic read depend on creating a writable sidecar.
This review was created by an AI agent (OpenHands) on behalf of the PR author.
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 (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
| self._loaded_codex_auth = _LoadedCredential(value) | ||
| return loaded | ||
|
|
||
| return await call_sync_from_async( |
There was a problem hiding this comment.
🔴 Critical: load() was previously read-only, but this call enters LocalFileStore.locked_update(), which opens or creates <path>.lock. A readable secrets.json on a read-only persistence volume now raises PermissionError before the file is read; I reproduced this with the file at 0444 and its directory at 0555. This affects ordinary settings/secrets GETs immediately, not only the known-deferred load_versioned() bootstrap. Keep ordinary load() on _read_data() without requiring the sidecar lock (the baseline can still be recorded from that snapshot), or otherwise use a read strategy that does not require write permission, and add a read-only regression test.
|
Reviewed both stacked PRs and posted AI-disclosed inline feedback:
Validation completed:
The reviews appear as |
Addresses both review findings, each reproduced first. load() took the update lock, which opens <path>.lock with O_CREAT. A readable secrets.json on a read-only volume raised PermissionError where a plain read succeeded, and the exclusive lock serialised every read: four concurrent loads over a 0.4s critical section took 1.62s. #100 needed that lock to snapshot the preserve baseline; this branch deleted the baseline, so it was dead weight. load() is a plain read again, as it was before #100. The /secrets dispatch called the per-key writers for CODEX_AUTH_JSON on any store, but SecretsStore is a documented extension point and third-party implementations only had to provide load/store/get_instance. They inherit NotImplementedError, so a legacy custom store returned 500 for that one name while ordinary secrets returned 201. Gate the dispatch on a supports_protected_credentials capability, mirroring FileStore.supports_locked_update: False on the base class, True on both first-party stores. Withholding protected names from store() and writing them per-key are two halves of one feature, so a store that lacks the second does not get the first and keeps the whole-document path. Both regressions fail against 979b8d5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Not closing this one — decoupling it from the Codex write-back track instead, per the correction in #120's thread. This PR was split out of #77 so the store layer could be reviewed independently, and #77 is now closed on Codex-specific evidence (see its closing comment). That evidence doesn't apply here: this PR's actual subject — every secrets write in production being an unguarded read-modify-write across 16 Recommend evaluating this purely on those merits — general secrets-store concurrency safety — rather than as scaffolding for #70/#101 (both closed). #101 stacked on this one specifically to add Codex-scoped hardening on top; with that closed, this PR's own value stands or falls independent of it. |
|
Closing this as superseded after rechecking the underlying race, rather than applying #121's one-shot-migration reasoning. The lost-update bug is real, ongoing, and worth fixing. It has now been fixed directly by #133: that PR reproduced lost writes through the live secrets router, serialized all five ongoing Why #100 should not land on top of that fix:
I reran the implementation's claims on the current head (
Operational evidence supports the risk but cannot prove a historical collision. Over the last 15 days, Datadog showed 432 deduplicated successful direct secrets writes (321 POST, 45 PUT, 66 DELETE). I found 39 sub-second, cross-pod pairs sharing an internal source IP among successful versioned-image requests, but those addresses are proxy/internal addresses and the endpoint logs contain no user, org, session, trace, or request identifier. Application-error and store-name searches found no consistency signal; a silent lost update is not recoverable from these logs after the fact. The current production deployment has 15 ready replicas (not 16) and is still on enterprise-server 1.49.1. Multiple replicas increase collision probability, but two are already sufficient; the remaining operational action is rolling out the released 1.50.0 fix, not merging this unused CAS layer. Tracking/index context: #120. |
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. |
1 participant
HUMAN:
Split out of #77 so the store layer can be reviewed on its own. See the triage comment on that PR for why: #77 (comment)
AGENT:
Parent PR: #77. Design: OpenHands/OpenHands#15393 §1 ("Add only a versioned canonical-store contract").
Why
#77 has been through four review passes without converging. The triage linked above breaks down where its bugs actually live, and the split is clean:
secrets_store,file_secrets_store,saas_secrets_store,file_store/*) has real non-mocked concurrency tests — including a cross-process CAS test that spawns four processes and asserts exactly one winner — and has produced zero escaped bugs across all four passes.pause_old_sandboxesself-selection. All cross-transaction ordering bugs that mocks cannot represent.Landing this separately shrinks #77 to the part that actually needs transaction-level test treatment, and gets the well-tested half out of a review loop it isn't causing.
This is safe to land first because it is inert.
load_versionedandreplace_versionedhave no callers until #77's callback router lands. Nothing in this diff is reachable from a request path.Summary
SecretsStoregainsload_versioned(name, organization_id) -> (value, opaque_version)andreplace_versioned(name, expected_version, value, organization_id) -> successor_version, plus aCredentialVersionConflictexception. Both default toNotImplementedErrorso unsupported stores keep the legacy path (the callback turns that into a 501).FileStoregainssupports_locked_update/locked_update.LocalFileStoreimplements it withflock(msvcrton Windows),InMemoryFileStorewith anRLock. Other stores report unsupported.FileSecretsStorepersists an opaque random generation alongside the secret document and does read/compare/write inside that lock.SaasSecretsStorelocks all matching Codex rows, treats the newest duplicate as canonical, checks its opaque row generation, and rewrites duplicates together.The eight files are byte-identical to #77's head (
77a7fd92f). No behaviour is added, removed, or changed in the split — verified file by file. Review this as the extraction it is.Issue Number
OpenHands/OpenHands#15393
How to Test
OH_PERSISTENCE_DIR=$(mktemp -d) uv run pytest tests/unit/app_server/test_file_secrets_store_versioning.py -q11 passed. Full suite:
1502 passed, 4 failedintests/unit/app_server/; the 4 are pre-existing macOS-only environment assertions (TMPDIR vs/tmp, and a missingservermodule) — confirmed by running them on a cleanmaincheckout with these changes stashed, where they fail identically.Lint: both pre-commit configs pass on all eight files — root (
./dev_config/python/.pre-commit-config.yaml) and enterprise (enterprise/dev_config/python/.pre-commit-config.yaml, run fromenterprise/). The enterprise files are formatted to the enterprise config, so a bare rootruff formatwill want to reformat them; that is expected and matches4d391c2d7on #77.The enterprise suite does run locally — #77 says it can't, but the only blockers are test-only deps missing from the root
uvvenv (the enterprise poetry venv is the dead end, since it resolvesopenhands-sdk1.29.0 against a tree needing newer):enterprise/tests/unit/test_saas_secrets_store.py: 15 passed. Whole suite: 2633 passed, 17 failed — the 17 are pre-existing (slack-sdk, posthog,saas_serverroute order), confirmed identical onmain. Seven files still needgoogle-cloud-recaptcha-enterpriseto collect; none touch secrets. CI covers those.Type
Notes
Three known issues are carried forward unchanged rather than fixed here, because fixing them means changing the version-derivation contract or reaching into #77's layer, and those are maintainer calls rather than something to slip into a PR labelled as a split. All three are in the triage comment with locations. Issue 3 now has a fix up as #101, stacked on this branch.
file_secrets_store.py:233—load_versionedwrites the file during a GET to mint a missing generation. On a read-only volume that is an uncaughtOSError→ 500, which SDK 1.37.1 latches as a permanentCredentialSyncError(acp_file_credentials.py:196, cleared only forCredentialAuthorizationRejectedat:435). Conversation-bricking once the callback is wired. Fixing it properly needs a decision on how to version a pre-existing secret that has no stored generation — #15393 explicitly rejected a plaintext digest, so there is no free answer.saas_secrets_store.py::_require_organization_idraisesKeyErrorfor three distinct conditions, includingOrgMemberStore.get_org_member(...) is None. The callback mapsKeyError→ 404 → the same permanent latch. An authz revocation should be a 403, which the SDK can clear on reactivation. The status mapping lives insandbox_router.py(in feat(app-server): sync managed Codex credentials (Phase 1) #77, not here), so the store needs a distinct exception type and the router needs to map it — a two-PR change.preserve_codex/_loaded_codex_authis a CAS emulated on top of a store whose contract is "delete every row, insert what I sent," using per-request in-memory state. Two paths defeat it, and I verified both against this branch's head:(b) is
settings_router.py:390—invalidate_legacy_secrets_storeruns insideGET /settings, establishes a baseline, then savescustom_secrets={}, sopreserve_codexisFalseand the row is deleted with nothing re-inserted. Pre-existing, not introduced by feat(app-server): sync managed Codex credentials (Phase 1) #77, but it means the invariant the feature needs is false today. Fixed in fix(secrets): make the managed-credential write guard structural #101:storedrops protected names unconditionally, which deletes the baseline machinery and the Codex row lock. Worth correcting one thing I wrote earlier — that fix needs no schema change and no migration; the credential stays aCustomSecretin the same table with the same encryption and the sameSecretStrmodel, and only who may write it changes.None of the three is reachable while this PR is inert, which is the argument for landing it and fixing them against a smaller surface.
Enterprise server image for this PR: