fix(api): carry the full project id in cache and lock keys - #6284
fix(api): carry the full project id in cache and lock keys#6284WhoamiI00 wants to merge 2 commits into
Conversation
`_pack` cut every scope segment down to the last 12 characters of the id, so two projects whose ids share that suffix shared every cache entry in every namespace that did not opt out — including `check_permissions` and `check_action_access`, which decide authorization. Project ids are server-generated UUID4s, so a caller cannot steer a collision and the odds of one arising are remote, but one project reading another's cached permission result is not a risk worth carrying by default. Ids are carried whole now. Readers, writers, the pattern branch of `invalidate_cache` and the lock keys all derive from this one function, so they move together and no namespace is left unable to clear its own entries. The dash padding stays, so an absent or short id produces the same fixed-width segment it always did. The lock namespace is the one place the key shape cannot simply change: during a rolling deploy, pods still on the previous release take the truncated key, and a lock held only under the new key would not exclude them. Lock operations therefore cover both keys for one release, claiming the legacy key first — a pod on the previous release sets only that one, so taking it is what makes the two generations exclude each other. That cover keeps colliding projects serializing against each other on locks until it is removed, which is deliberate: letting two pods into the same critical section is worse than two unrelated tenants queueing. Cache keys, where the permission caches live, separate immediately. Closes Agenta-AI#6166
|
@WhoamiI00 is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
✅ Thanks @WhoamiI00! This PR now meets the contribution requirements and has been reopened. A maintainer will review it soon. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change preserves full project and user IDs in cache keys by default. It adds opt-in legacy truncation and dual-key lock operations for rolling deployments. Unit tests cover cache isolation, lock compatibility, ownership, cleanup, renewal, release, and short scopes. ChangesCache and lock compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant acquire_lock
participant Redis
participant lock_helpers
acquire_lock->>Redis: Claim legacy lock key
acquire_lock->>Redis: Claim current lock key
Redis-->>acquire_lock: Return acquisition result
acquire_lock->>lock_helpers: Renew or release owner keys
lock_helpers->>Redis: Update both lock keys
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Summary
Closes #6166.
_packinapi/oss/src/utils/caching.pycut every scope segment down to the last 12 characters of the id, so two projects whose ids share that suffix shared every cache entry in every namespace — includingcheck_permissionsandcheck_action_access, which decide authorization. Project ids are server-generated UUID4s, so a caller cannot steer a collision and the odds are remote, but one project reading another's cached permission result is not a risk worth carrying by default. Ids are carried whole now.The strings in the image are real
_packoutput, not illustrations.Two of the three listed blockers had already dissolved
The issue's "What a fix needs" list was written against a
full_project_idopt-out that no longer exists — it went away with the vault list cache in #6164 (grep -r full_project_id api/returns nothing today). That removes the first bullet entirely: with no flag, there is nothing to plumb throughinvalidate_cache. Readers, writers, the pattern branch ofinvalidate_cache, and the lock keys all derive from the same_pack, so they move together by construction.cache:p:is built in exactly one place and nothing parses a key back apart.The tenancy test named in the third bullet also isn't on
mainany more, so it's a new file rather than an update.The dash padding stays, so an absent or short id produces the same fixed-width segment it always did — only ids that were actually being cut change shape.
The lock namespace, and the one trade-off
This is the part that isn't just a key change. During a rolling deploy, pods still on the previous release take the truncated key, so a lock held only under the new key would not exclude them — mutual exclusion would be lost for the length of the deploy.
Taking the second of the two options in the issue, lock operations cover both keys for one release.
locking.pyis the only caller of_packoutsidecaching.py, and all of the lock call sites (eval runtime, attachment sweep, account creation, and the EE spans/sessions/events/billing routers) funnel through its three functions, so the transition lives in one module and is marked for deletion next release.The ordering is what makes it correct: the legacy key is claimed first. A pod on the previous release sets only that one, so taking it is what makes the two generations exclude each other; claiming it second would let both hold their own key and enter together. A caller that then loses the race on the primary key releases the legacy key it just took, so a failed acquire doesn't block the section for a full TTL.
The trade-off worth your explicit sign-off: while that cover is in place, two projects with colliding ids keep serializing against each other on locks. That is deliberate — letting two pods into the same critical section is worse than two unrelated tenants queueing — and it ends when the cover is removed. Cache keys, where the permission caches live and where the security consequence is, separate immediately. There's a test pinning this so it's a decision on record rather than a surprise.
If you'd rather drain than dual-cover, the whole transition is
_lock_keys' second return value plus threelegacy_keybranches, and I'm happy to strip it.Coordination
#6192 is open against the same id-normalization block in
_pack(wildcards for an omitteduser_idunderpattern=True). The two are orthogonal — I kept this strictly to the truncation and will rebase if that one lands first.Testing
Verified locally
(The four
test_web_entrypoint_email_env.pyfailures on my machine are a CRLF checkout ofweb/entrypoint.sh, unrelated to this branch and green in CI.)I also printed real
_packoutput for two colliding ids to confirm the before/after keys in the image are genuine rather than hand-written.Added or updated tests
New
api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py— 14 tests, the file the issue asks for:SET NXon one key would make every such acquire look blocked)fakeredis only runs Lua with the optional
lupabackend, which isn't a dependency here, so the two ownership scripts are supplied as an equivalent shim in the fixture —acquire_lock/renew_lock/release_lockthemselves run as written. No new dependency.QA follow-up
The deploy itself is the thing to watch, and it is a one-time event:
REMOVEinlocking.py.Demo
N/A — backend only. The image above shows the key shape before and after.
Checklist