trunk-merge/pr-107025/56b2482b-7496-4095-9dd8-0337999f217a - #107624
trunk-io[bot] wants to merge 6 commits into
Conversation
The scrub lane deduped only inline refs, so every repeat of a URL ref was scrubbed and then refused by the write-once S3 put. URL refs now join the pod's seen-ref cache when their image is staged, and leave it when that write is discarded. Copies inside one batch stay planned, so a later valid copy still stores when an earlier one fails validation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sing month key Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dropped later copy of a URL ref cleared the seen mark that an earlier, stored copy set, so the pod scrubbed the ref again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…b write Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… owns it The owner set recorded that a copy once marked a ref. If the cache evicted and re-marked the ref before the older copy was dropped, the older copy cleared the newer mark. A map of unwritten owners keeps only the current owner, and a written hand-off removes its entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughImageBatcher now tracks ownership of inline deduplication marks and updates them based on successful writes and discarded images. It skips URL references already seen by the pod. Deduplication metrics distinguish batch and pod scopes, and inline and URL sources. Tests cover repeated URL references, unavailable month keys, discarded copies, and replay after failure for inline and URL images. 🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
Full details: Description checkExplanation The description only documents Trunk Merge metadata and the tested pull request. It does not include the required Problem, Changes, testing, Release status, Docs update, or Agent context sections, and it does not explain the image-scrubbing changes. Resolution Add the required template sections. Describe the user or system problem, the URL and inline deduplication changes, the regression tests and their expected behavior, the selected release-status option, changelog and docs decisions, and the required agent details if an agent authored or assisted with the work. Retain the Trunk Merge metadata as supplementary information.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: bcb9ca35-d55c-42c8-a646-1f83a1f3a9be
📒 Files selected for processing (3)
nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.test.tsnodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.tsnodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/metrics.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
| if (this.seenRefs.has(ref)) { | ||
| ImageScrubConsumerMetrics.incDeduped('pod', 'url') | ||
| continue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- changed-file diff ---'
git diff --unified=35 69a4765903776a5e70d4633590673395b1eaec2f e10294b97e4aadf25b7b541ed6e3808241c38642 -- nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.ts nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.test.ts
printf '%s\n' '--- reviewed source around URL planning and hand-off ---'
sed -n '560,735p' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.ts
sed -n '820,925p' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.ts
printf '%s\n' '--- direct lifecycle references ---'
rg -n -C 8 'seenRefs|forgetUnwritten|handleBatch|write|month.?key|missing|available' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.ts nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.test.tsRepository: PostHog/posthog
Length of output: 41880
🤖 get_repo_knowledge executed:
get_repo_knowledge PostHog/posthog /tmp/coderabbit-repo-knowledge/posthog-posthog-1c883bd0/conventions
Length of output: 40997
🏁 Script executed:
#!/bin/bash
set -eu
file=nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.ts
test=nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.test.ts
printf '%s\n' '--- image-batcher outline ---'
ast-grep outline "$file"
printf '%s\n' '--- exact planning and batch lifecycle ---'
nl -ba "$file" | sed -n '300,560p'
nl -ba "$file" | sed -n '620,735p'
nl -ba "$file" | sed -n '820,930p'
printf '%s\n' '--- exact tests for missing keys and overlapping hand-offs ---'
nl -ba "$test" | sed -n '330,455p'
nl -ba "$test" | sed -n '1325,1380p'
printf '%s\n' '--- key reader and month-key consumers ---'
rg -n -C 12 'class .*Reader|read\\(|strongly consistent|month key|tableKeyString|sessionMonth|readKeys' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrubRepository: PostHog/posthog
Length of output: 38091
🤖 get_repo_knowledge executed:
get_repo_knowledge PostHog/posthog /tmp/coderabbit-repo-knowledge/posthog-posthog-1c883bd0
Length of output: 3910
🏁 Script executed:
#!/bin/bash
set -eu
file=nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub/image-batcher.ts
printf '%s\n' '--- handleBatch and hand-off wait path ---'
nl -ba "$file" | sed -n '230,315p'
nl -ba "$file" | sed -n '536,620p'
nl -ba "$file" | sed -n '930,1015p'
printf '%s\n' '--- key reader declarations and implementation files ---'
rg -n -F 'class MlKeyReader' nodejs/src
rg -n -F 'class KeyReader' nodejs/src
rg -n -F 'imageKeyId' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub
rg -n -F 'delete' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrubRepository: PostHog/posthog
Length of output: 13700
🤖 get_repo_knowledge executed:
get_repo_knowledge PostHog/posthog /tmp/coderabbit-repo-knowledge/posthog-posthog-1c883bd0
Length of output: 88
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- MlKeyReader source ---'
nl -ba nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys/reader.ts
printf '%s\n' '--- key schema and key-manager consumers ---'
rg -n -C 8 'imageKeyId|tableKeyString|reader\.read|MlKeyReader|sessionMonth|month key' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub || true
printf '%s\n' '--- repository month-key writes/deletes in the ML mirror area ---'
rg -n -C 8 'delete.*key|key.*delete|write.*key|put.*key|tableKeyString|imageKeyId' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror-image-scrub || trueRepository: PostHog/posthog
Length of output: 45667
🤖 get_repo_knowledge executed:
get_repo_knowledge PostHog/posthog /tmp/coderabbit-repo-knowledge/posthog-posthog-1c883bd0/conventions
Length of output: 40997
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- MlSessionKeyStore implementation ---'
rg -n -F 'class MlSessionKeyStore' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys
file=$(rg -l -F 'class MlSessionKeyStore' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys | head -n 1)
if [ -n "$file" ]; then
nl -ba "$file" | sed -n '1,260p'
fi
printf '%s\n' '--- month-key creation and repair evidence ---'
rg -n -C 12 'imageKeyId|month.*key|key.*month|repair|conditional|deleted' nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys/key-store.ts nodejs/src/ingestion/pipelines/sessionreplay/ml-mirror/keys/key-store.test.ts 2>/dev/null || trueRepository: PostHog/posthog
Length of output: 42351
🤖 get_repo_knowledge executed:
get_repo_knowledge PostHog/posthog /tmp/coderabbit-repo-knowledge/posthog-posthog-1c883bd0
Length of output: 3891
Do not deduplicate URL refs while their write is pending.
handleBatch() can return while the first hand-off awaits MlKeyReader.read(). A later batch can then hit seenRefs.has(ref), skip the copy, and record its offset. If the first read finds no month key, write() removes the first mark and stores its offsets. Both offsets can advance without storing either URL image.
MlKeyBatch can provision a missing month key, and its overlapping-batch path allows a later read before the earlier write. Keep pending URL copies retryable until the owning hand-off succeeds, or wait for the owner outcome before skipping them. Add a regression test for overlapping hand-offs.
1 participant
This pull request was created and is being managed by Trunk Merge.
This pull request is based on the master branch at SHA 69a4765903776a5e70d4633590673395b1eaec2f.
See more details here.
When CI completes, this pull request will be closed automatically.
Pull Requests Being Tested
This pull request is testing the changes from pull request 107025.