fix(api): carry the full project id in cache and lock keys

[WhoamiI00]

Summary

Closes #6166.

_pack in api/oss/src/utils/caching.py 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 — 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 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 _pack output, 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_id opt-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 through invalidate_cache. Readers, writers, the pattern branch of invalidate_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 main any 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.py is the only caller of _pack outside caching.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 three legacy_key branches, and I'm happy to strip it.

Coordination

#6192 is open against the same id-normalization block in _pack (wildcards for an omitted user_id under pattern=True). The two are orthogonal — I kept this strictly to the truncation and will rebase if that one lands first.

Testing

Verified locally

pytest oss/tests/pytest/unit   ->  2685 passed, 73 skipped
pytest ee/tests/pytest/unit    ->   347 passed
ruff format --check / ruff check on the three changed files -> clean

(The four test_web_entrypoint_email_env.py failures on my machine are a CRLF checkout of web/entrypoint.sh, unrelated to this branch and green in CI.)

I also printed real _pack output 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:

• colliding project ids and colliding user ids no longer share a cache key; the whole id is present
• an invalidation pattern is scoped to one project
• absent / short / exactly-12-character ids keep their historical segment
• the opt-in legacy shape still collides — that is what makes it transitional
• locks: ordinary projects don't block each other; the same project still excludes itself; a holder on the previous release blocks this one; a blocked acquire doesn't strand the legacy key; release clears both generations; renew keeps both alive; a short id takes only one key (a double SET NX on one key would make every such acquire look blocked)

fakeredis only runs Lua with the optional lupa backend, 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_lock themselves run as written. No new dependency.

QA follow-up

The deploy itself is the thing to watch, and it is a one-time event:

• Every cache namespace goes cold at once when this ships (5-minute TTLs, so it refills quickly). Worth being aware of as a brief load bump on the DB rather than a correctness concern.
• During the rolling deploy, confirm eval-runtime jobs are not double-claimed. That is exactly what the legacy-key cover is there to prevent, and it is the behaviour I could only test against fakeredis.
• The transitional cover should be removed once no pod predating this change is running. It is marked REMOVE in locking.py.

Demo

N/A — backend only. The image above shows the key shape before and after.

Checklist

• I have included a video or screen recording for UI changes, or marked Demo as N/A
• Relevant tests pass locally
• Relevant linting and formatting pass locally
• I have signed the CLA, or I will sign it when the bot prompts me

[vercel]

@WhoamiI00 is attempting to deploy a commit to the agenta projects Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions]

✅ Thanks @WhoamiI00! This PR now meets the contribution requirements and has been reopened. A maintainer will review it soon.

[coderabbitai]

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Improved cache isolation by preserving complete project and user identifiers, preventing collisions between similarly named resources.
  • Maintained compatibility with existing locks during rolling deployments, reducing interruptions and preventing conflicting operations.
  • Improved lock renewal, release, and cleanup behavior across deployment transitions, including failed or cancelled acquisition attempts.
  • Preserved established behavior for short or unavailable identifiers.
  • Ensured cache invalidation remains limited to the relevant project scope.

Walkthrough

Cache keys now preserve full project and user IDs by default. Opt-in legacy truncation remains available. Locks use current and legacy keys during rolling deployments, with ownership-aware cleanup, renewal, and release. Tests cover cache isolation and lock lifecycle behavior.

Changes

Cache and lock compatibility

Layer / File(s)	Summary
Full cache scope formatting api/oss/src/utils/caching.py, api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py	_scope preserves full identifiers by default and retains padding for short or absent values. _pack forwards the legacy truncation option. Tests cover key formatting and tenant isolation.
Dual-key lock operations api/oss/src/utils/locking.py	Lock acquisition claims the legacy key before the current key when needed. Cancellation and failed acquisition release claimed legacy keys. Renewal and release operate on both keys with ownership checks.
Lock compatibility validation api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py	Tests cover rolling-deployment blocking, project isolation, cleanup, cancellation, ownership safety, renewal failures, release, and short scopes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🔵 Low · up to 2178a

The change improves cache tenant isolation and rolling lock compatibility, but a narrowly timed cancellation can leave a current-format lock blocking work until its TTL expires. Add primary-claim cleanup and regression coverage before merge.

Sequence Diagram(s)

sequenceDiagram
  participant acquire_lock
  participant Redis
  participant renew_lock
  participant release_lock
  acquire_lock->>Redis: Claim legacy and current lock keys
  Redis-->>acquire_lock: Return acquisition result
  renew_lock->>Redis: Renew current and legacy keys
  release_lock->>Redis: Release current and legacy keys

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the primary change: cache and lock keys now retain the full project ID.
Description check	✅ Passed	The description directly explains the cache-key collision fix, rolling-deployment lock compatibility, tests, and validation results.
Linked Issues check	✅ Passed	The PR satisfies issue [#6166] by using full project IDs for cache key generation, preserving invalidation scoping, and providing temporary dual-key lock coverage during rolling deployments. The added…
Out of Scope Changes check	✅ Passed	The cache changes, lock transition logic, cancellation cleanup, and related tests all support the requirements in issue [#6166]. No unrelated code changes are identified.
Docstring Coverage	✅ Passed	Docstring coverage is 60.61% which is sufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Labeeb2339]

I reviewed the cache-key and rolling-lock changes together. The shared _scope helper keeps the historical padding for short/absent identifiers while preserving full UUIDs, so readers, writers, and scoped invalidation derive the same new key. The dual-key lock path claims the legacy key first, cleans it up when the primary claim loses, and renews/releases both generations; the short-scope branch correctly avoids claiming the same key twice. The tests cover collision isolation, pattern scoping, same-project exclusion, previous-release holders, failed-acquire cleanup, renewal, release, and short IDs. I found no additional correctness issue in the Python diff.

[mmabrouk]

thanks for the careful rolling-deploy coverage. i found one lock-renewal gap that needs a change before qa.

[mmabrouk]

The legacy-key renewal result is discarded. If the primary key still exists but the legacy key expired or was lost, renewed stays true and this function reports success even though a pod on the previous release can now enter the same critical section. Require both renewals to succeed, and add a regression test where the legacy key is missing before renew_lock runs.

[mmabrouk]

thanks for the pr. one lock-renewal case needs a change before qa.

[mmabrouk]

if the primary key still exists but the transitional legacy key has expired or been deleted, this call returns false but its result is discarded. renew_lock then returns true from the primary renewal below, even though a pod on the previous release can acquire the missing legacy key and enter the same critical section. require both renewals to succeed, and add a regression test where the primary key exists but the legacy key is missing.

[WhoamiI00]

Correction to my earlier reply — I said this was ready from my side before properly reading your review. It wasn't: this finding was outstanding, and it was right. Sorry for the noise.

Fixed in b672a4a.

The legacy renewal's result was discarded, so renew_lock reported the primary key's outcome alone:

renewed = await _renew_if_owner(lock_key, owner, ttl)

if legacy_key is not None:
    await _renew_if_owner(legacy_key, owner, ttl)   # result dropped

If the primary survived while the legacy key expired or was evicted, the call returned True and the holder carried on inside the critical section — while a pod on the previous release could acquire the now-missing legacy key and enter it too. That is exactly the cross-release mutual exclusion the dual-key path exists to preserve, so the bug undercut the point of the change.

Both renewals now have to succeed:

renewed = await _renew_if_owner(legacy_key, owner, ttl) and renewed

The legacy call is deliberately on the left so it is always attempted rather than short-circuited — that way the primary is still extended when the legacy key is the one that failed, and the caller can finish its work and release cleanly instead of having the section pulled out from under it twice over.

Two tests added:

• test_renew_fails_when_the_legacy_key_is_gone — the case you asked for: primary held, legacy key deleted, renew_lock must return False. It also asserts the primary's TTL was still extended. Fails on the previous commit.
• test_renew_fails_when_the_primary_key_is_gone — the mirror, so a live legacy key can never mask a lapsed primary. Passes either way; it pins the boundary.

16 tests green in test_cache_key_tenancy.py, ruff format --check clean.

[WhoamiI00]

One more thing, since "require both" raises the obvious question about the other two dual-key sites: I checked them and left them alone deliberately.

• release_lock — deleted still reflects the primary alone, and the legacy release stays best-effort. Both keys are cleared either way, so the section really is free; requiring both to succeed would make release_lock report failure for a lock that was genuinely released (the legacy key having already expired is the normal case, not an error).
• acquire_lock, blocked path — the legacy key is released because this caller is not entering the section, and the function returns None regardless of how that goes.

So the asymmetry is intentional: holding requires both keys to be held, which is why renewal now needs both, whereas releasing only needs both to end up gone. Happy to add that as a comment in the code if you'd like it recorded there rather than just here.

[mmabrouk]

hi @WhoamiI00, do you still want to work on this? a quick update is enough.

[WhoamiI00]

Yes — still on it, and it's ready from my side.

fix/full-project-id-in-cache-keys is rebased on current main, MERGEABLE, and every test job is green (run-api-unit-tests, run-services-unit-tests, run-web-unit-tests, run-sdk-unit-tests). The only red checks are the build-image jobs, which fail on denied: installation not allowed to Write organization package — that's the fork-PR registry permission, not something in this diff.

@Labeeb2339 reviewed the cache-key and rolling-lock changes together and found no correctness issue in the Python diff.

Happy to rebase again or split anything out if that helps it land — just say which. Same goes for #6227, which is also green and waiting.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Team

Run ID: f33ed2ef-fdbd-49c7-8620-9d06aac0953c

📥 Commits

Reviewing files that changed from the base of the PR and between a09a0f0 and b672a4a.

⛔ Files ignored due to path filters (1)

• .github/pr-assets/6166-full-project-id-cache-keys.png is excluded by !**/*.png

📒 Files selected for processing (3)

• api/oss/src/utils/caching.py
• api/oss/src/utils/locking.py
• api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

[coderabbitai]

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.

[WhoamiI00]

Rebased onto current main (508b551 / v0.114.8) — the branch had gone conflicting.

One conflict worth flagging, because resolving it naively would have quietly dropped a feature that landed on main after this branch was cut. _pack gained a wildcard for an absent user scope:

else:
    user_id = "*" if pattern else "-" * 12

My side had replaced that whole block with _scope(user_id, legacy_truncated_scope), which returns a fixed-width padded segment and cannot express *. Taking my side wholesale would have made pattern-based invalidation stop matching across users. Resolved to keep both:

project_id = _scope(project_id, legacy_truncated_scope)
user_id = (
    "*" if pattern and not user_id else _scope(user_id, legacy_truncated_scope)
)

Added test_an_invalidation_pattern_still_wildcards_an_absent_user to pin it, asserting u:* is present for a pattern with no user and absent once a user is named — so a future rebase can't lose it silently.

60 tests pass across oss/tests/pytest/unit/utils/, which includes main's own test_caching.py, and ruff format --check is clean on all three files.

[mmabrouk]

thanks for the rebase. i found one cancellation window that can still strand the transitional lock key.

[mmabrouk]

legacy_claimed becomes true only after the awaited SET NX returns. If cancellation arrives after Redis applies the set but before the client receives its response, this assignment never runs, so the finally block leaves the legacy key held until its TTL and both old and new pods report a false block. I reproduced this by making the set write the key, suspend before returning, and then cancelling acquire_lock. Mark the cleanup obligation before awaiting the legacy set, clear it after a confirmed failed claim or ownership transfer, and add that cancellation regression.

[WhoamiI00]

Confirmed and fixed in 2178a0b — and thank you for actually reproducing it, that's a narrow window and the repro made it unambiguous.

You're right that the flag was on the wrong side of the await. legacy_claimed = True only ran once the reply came back, so a cancellation landing after Redis applied the SET but before delivery skipped it, the finally saw no obligation, and the key sat for its full TTL with the section handed to nobody.

The obligation is now raised before the await and lowered on a confirmed refusal:

if legacy_key is not None:
    legacy_claimed = True
    if not await _lock_engine.set(legacy_key, lock_owner, nx=True, ex=ttl):
        # A confirmed refusal — the key belongs to someone else.
        legacy_claimed = False
        return None

Claiming an obligation that may not exist is the safe direction here, because _release_if_owner compares the owner token — so if Redis never applied our SET and another pod holds the key, the cleanup is a no-op rather than a wrongful delete. I've added test_a_refused_legacy_claim_leaves_the_holders_key_alone to pin that specifically, since the wider cleanup window is only sound while that ownership check holds.

The cancellation regression is test_acquire_releases_the_legacy_key_when_cancelled_mid_claim, built the way you described it: the patched set writes the key, signals, then suspends before returning, and the task is cancelled at that point. It asserts the key is present while suspended, gone after cancellation, and that the next caller acquires immediately rather than waiting out the TTL. It fails on the previous commit.

20 tests green in test_cache_key_tenancy.py, ruff format --check clean.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Team

Run ID: f2c8548d-ea3e-49d5-94a8-03248038109c

📥 Commits

Reviewing files that changed from the base of the PR and between 6b42d56 and 2178a0b.

📒 Files selected for processing (2)

• api/oss/src/utils/locking.py
• api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🤖 get_repo_knowledge executed:

get_repo_knowledge Agenta-AI/agenta /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/conventions /tmp/coderabbit-repo-knowledge/agenta-ai-agenta-4b53879a/learnings

Length of output: 14349

---

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- changed file context ---'
sed -n '150,255p' api/oss/src/utils/locking.py
printf '%s\n' '--- relevant tests ---'
sed -n '330,440p' api/oss/tests/pytest/unit/utils/test_cache_key_tenancy.py
printf '%s\n' '--- lock symbol references ---'
rg -n "_release_if_owner|acquire_lock|legacy_claimed|primary_claimed|lock_key" api/oss/src/utils/locking.py api/oss/tests/pytest/unit/utils

Repository: Agenta-AI/agenta

Length of output: 17434

---

🏁 Script executed:

#!/bin/bash
set -eu
sed -n '245,282p' api/oss/src/utils/locking.py
sed -n '40,112p' api/oss/src/utils/locking.py
git diff -- api/oss/src/utils/locking.py | sed -n '1,180p'

Repository: Agenta-AI/agenta

Length of output: 4012

---

Track cleanup for the primary claim.

If cancellation occurs after Redis writes lock_key but before _lock_engine.set() returns, finally releases only legacy_key. The primary lock remains until its TTL expires, so new callers can receive a blocked result while no caller owns the critical section.

Set a primary cleanup obligation before the primary SET. Clear it after a confirmed refusal or successful ownership transfer. Release lock_key with _release_if_owner in finally, using the existing best-effort cleanup pattern. Add a regression test that suspends after writing lock_key, then cancels the task.