refactor(activity): qualify Valkey activity identifiers - #914
refactor(activity): qualify Valkey activity identifiers#914seonghobae wants to merge 70 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughValkey 활동 스트림에 문자열 입력 검증과 WATCH/MULTI/EXEC 기반 동시성 제어를 추가했습니다. 동기 게시 경로는 충돌을 최대 8회 재시도하고, 한도 초과 시 키와 원인을 노출하지 않는 Changes활동 스트림 동시성 변경
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR hardens activity-stream identity validation and reseeding while preserving the Valkey wire format. It is mergeable with owner awareness that external administrative or recovery scripts using the former client= or summary= keyword names may need updating. Sequence Diagram(s)sequenceDiagram
participant Caller
participant publish_activity_event_sync
participant Valkey
Caller->>publish_activity_event_sync: 활동 이벤트 게시 요청
publish_activity_event_sync->>Valkey: WATCH 및 기존 이벤트 조회
Valkey-->>publish_activity_event_sync: 이벤트 필드 반환
publish_activity_event_sync->>Valkey: MULTI/EXEC 및 XADD
Valkey-->>publish_activity_event_sync: 게시 결과 또는 WatchError
publish_activity_event_sync->>Valkey: 충돌 시 다음 시도
publish_activity_event_sync-->>Caller: 게시 완료 또는 8회 후 RuntimeError
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
seonghobae
added
bug
ChatGPT Codex Connector
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Cycle retry 2026-09-02T08:15Z: write-path probe on #914 (Valkey activity identifiers). Independent APPROVE still required before squash-merge; this is not a self-approve. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Performance/operability finding on the current head: index_legacy_activity_stream_aliases() runs synchronously in FastAPI lifespan before app.state.valkey and request service become available, and it awaits one SADD per discovered legacy UUID alias inside scan_iter. With N retained aliases, startup adds N serial Valkey write round trips on top of SCAN pagination. That makes rollout/readiness latency proportional to historical alias cardinality even though these writes are independent. Preserve the one-time compatibility evidence, but batch independent alias-index SADDs through a bounded non-transactional redis-py pipeline (with bounded in-memory batch size) and add a regression that rejects per-alias direct network writes. Do not remove alias discovery or hide historical streams to meet latency.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh current-head buyer-path performance finding: the alias-preserving compatibility branch still performs one XREVRANGE count=1 per stream and then one further awaited XREVRANGE count=1 for every returned event. asyncio.gather only overlaps the initial probes; a normal 50-event read for a post with one retained UUID alias can still incur roughly 50 sequential follow-up Valkey waits after alias admission, even though the output budget is already bounded. That leaves the exact historical customers for whom the compatibility bridge exists outside the p95<=20ms commercial path. Preserve complete retained-alias history, deterministic cross-stream ordering, and the 1..1000 output budget, but add a realistic RED that counts compatibility-path network exchanges and then batch a bounded amount of per-stream history through non-transactional pipeline reads before local merge. Keep a fixed memory ceiling and fall back/refill rather than buffering event_count * alias_count without a bound.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head performance finding: retained UUID-alias reads still pay a redundant Valkey exchange before compatibility merge. _activity_canonical_entries_if_alias_free() pipelines SSCAN + canonical XREVRANGE, but when an alias is present it discards both results; read_activity_events() then calls _activity_read_stream_keys() (another SSCAN traversal) and re-fetches the canonical stream in the compatibility pipeline. The existing compatibility-budget regression permits round_trips <= 3, so it does not fail on this duplicated admission/read work. Keep the current retained-history semantics and bounded 1,000-entry buffer, but make the complete first SSCAN page reusable: when cursor=0, carry its aliases plus the already-fetched canonical page into the compatibility merge, trim the canonical page to the per-stream prefetch budget, and fetch only retained alias pages in the second exchange. Realistic acceptance for the one-alias fixture is exactly two network exchanges, not three. Do not hide aliases, scan unrelated keys, or weaken the 1..1000 result bound.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Follow-up verification finding on the admission-reuse repair: the complete first SSCAN page now bypasses _activity_read_stream_keys(), so it also bypasses that function's <=999 aliases + canonical stream fan-out guard. A complete page containing 1,000 retained aliases would produce 1,001 stream keys and enter compatibility prefetch instead of failing closed. Preserve the existing total-stream budget on the reused-page path before issuing the alias pipeline. Add a regression where the first SSCAN returns cursor 0 with 1,000 aliases; it must raise Activity history has too many retained compatibility streams after the first two-command admission/data exchange and before any alias fan-out.
|
@codex review Please review exact current head |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Scope / owner boundary
LineageWeave-owned activity-stream read-model identity and retained-history compatibility at the Valkey boundary. PostgreSQL
source_post.post_idremains authoritative; Valkey remains a derived operational projection. Persisted activity fields and API response shape are preserved. ADR 0363 remains Proposed until an unchanged release-ready head is independently approved.Concurrent non-force deltas extending historical UUID alias compatibility, bounded alias cardinality/scan behavior, single-writer deployment prerequisites, and bounded multi-stream merge remain adopted. Predecessor workflow/approval evidence does not transfer.
Protected-main convergence
Protected
mainremains83eba56149eb802cd63642c507c324c9976ec78eat this fresh sweep. Exact current head is61ed3a3712d252e3c179a71d297c52f05e1bac20; the branch remains directly descended from that protected base without force push or destructive rebase. No open PR directly bases on this branch at the latest descendant sweep.Buyer-path read repair lineage
Earlier repair lineages preserve bounded canonical reads, one-exchange canonical UUID admission/read, bounded startup alias-index writes, and bounded retained-alias page prefetch.
Review
5122107164found a remaining deterministic extra exchange: when the first pipelined SSCAN returned a complete non-empty alias page, the code discarded that page and its already-fetched canonical XREVRANGE, then rescanned aliases and re-fetched canonical history before merging. RED15746e2be27effca16c71f445f8d77f5d55fe6b8tightens the one-alias fixture to exactly two network exchanges with pipeline batches[2, 1]. Production repair07fa552f9fecbe1cda79a9fccc7eeacd82c4fba5carries a complete first alias page plus canonical page into compatibility merge, trims the canonical page to the per-stream prefetch budget, and fetches only retained alias pages in the second pipeline exchange.Follow-up verification review
5122123572caught a regression before GREEN: reused complete SSCAN pages initially bypassed_activity_read_stream_keys()and therefore bypassed its existing<=999 aliases + canonical streamfan-out guard. REDa9c080c1f2fa030bd2b0cec0e2fb6b43e4328bbcreturns cursor 0 with 1,000 aliases and requires the request to fail closed after the first two-command admission/data exchange, before retained-alias fan-out. Production repair8ef3a42608739e5a32b5841e4a14494df5326a3freapplies the 1,000 distinct canonical-plus-alias stream ceiling on the reused-page path. ADR convergence61ed3a3712d252e3c179a71d297c52f05e1bac20records both the transport optimization and the preserved fan-out invariant.Central CodeQL handoff RCA
Required CodeQL run
33979627116is not a LineageWeave scan finding. On the exact head, language detection succeeded; python job101357239026and the javascript-typescript/actions siblings revalidated the live PR/head, found no authenticatedcodeql-dispatch/<language>terminal status, obtained the OIDC/repository-scoped app-token boundary, POSTed three nativecodeql-scanrepository_dispatch events, wroteverdict=pending, then intentionally failed to release their runners.All three central dispatches are now terminal and expose the precise owner-side root cause.
.githubruns33988646498,33988691535, and33988735145each obtained a runner, succeeded at OpenCode app-token exchange, then failed invalidate-dispatch -> Bind workflow inputs to live organization pull request metadata; their scan jobs were skipped. Exact python-shard run33988646498, job101366754283, recordsDISPATCH_ACTOR=opencode-agent[bot],DISPATCH_SENDER=opencode-agent[bot], butALLOWED_DISPATCH_ACTOR=github-actions[bot], then fails withrepository_dispatch authorization rejected ... because both must match one configured scheduler identity.The serialized matrix is valid and reaches this gate, so canonical.github#1926already fixed the earlier matrix-template defect.The workflow-side authorization representation is also already repaired:
.github#1932merged as6f8c51d7389c22ebaf294fe8fe9ef495257883c0and makesopencode-review-dispatch,codeql-scan-dispatch, andpr-review-fix-schedulerparseOPENCODE_REPOSITORY_DISPATCH_ACTORidentically as a comma-separated allowlist while still requiring actor and sender to match the same listed identity. Owner history establishes that both identities are intentional: the merge scheduler legitimately usesgithub-actions[bot], while.github#1497explicitly moved the Required OpenCode path onto GitHub OIDC + the OpenCode App (opencode-agent[bot]). Therefore the remaining owner/admin action tracked in.github#1927/#1929is to set the variable to the two-entry trusted listgithub-actions[bot],opencode-agent[bot]; replacing one identity with the other would simply break the opposite trusted dispatcher. This LineageWeave lane has no Actions-variable mutation surface and does not widen central authorization from a leaf repository.No
codeql-dispatch/python,/javascript-typescript, or/actionsterminal status was published to this head. Exact #914 evidence and the owner-intent correction have been added to.github#1927as a post-change canary.A separate recovery issue remains ordered after the identity repair:
.github#1902is Draft/unmerged and removes the required-workflowRUN_ATTEMPT != 1inference so an unchanged consumer head without a trusted terminal verdict can redispatch after an earlier failed/cancelled handoff. Manually rerunning #914 before the dispatcher allowlist value is corrected would repeat the known authorization rejection; after that correction, rerun behavior must still respect the protected central recovery contract rather than assuming attempt number is a dispatch receipt. Therefore no leaf no-op head churn or gate bypass is used.Exact live boundary
main@83eba56149eb802cd63642c507c324c9976ec78e61ed3a3712d252e3c179a71d297c52f05e1bac205122123572->a9c080c1...->8ef3a426...->61ed3a371...5122107164->15746e2b...->07fa552f...backend/app/activity_stream.py33979633195: success33979627218: success33979627205: success33979627116: failure/pending handoff33988646498,33988691535,33988735145: failure at dispatcher authorization; scan jobs skipped; no source result produced.github#1932merged; both trusted identities can be expressed safelyOPENCODE_REPOSITORY_DISPATCH_ACTORfrom one entry togithub-actions[bot],opencode-agent[bot]ContextualWisdomLab/.github#1902, still Draft/unmergedf9645d01eda570e32b0f229c0f8af1a23faee06eDo not self-approve, bypass required gates, force-push, mutate organization authorization from this leaf repository, hide retained activity for latency, churn the leaf head merely to retrigger central CodeQL, or promote predecessor results to current-head evidence.