Add window-function variant (option 4) for list-named-blobs SQL - #3260
Merged
zichengl merged 4 commits intoMay 13, 2026
Merged
Conversation
Adds MySqlNamedBlobDbConfig.listNamedBlobsSQLOption=4: a window-function form of the LIST_WITH_PREFIX SQL that computes per-blob latest version via MAX(version) OVER (PARTITION BY blob_name) in a single PK range scan, instead of the INNER JOIN (option 2) or correlated subquery (option 3). Why --- Option 3 produces a StreamAgg + IndexJoin plan on TiDB: every outer candidate row drives a per-row inner PK probe to fetch MAX(version). Under TiKV region degradation, even zero-result probes inherit the full coprocessor RPC deadline, amplifying transient TiKV stalls into the LIST tail. The window form runs as a single PK range scan with streaming max — no per-row inner probe. Correctness invariant --------------------- Same as options 2 and 3: MAX(version) is computed over all blob_state=READY rows INCLUDING rows with a non-null deleted_ts; the deleted_ts predicate is applied only on the outer SELECT after the window operator. If the latest READY version of a blob has expired, the blob is hidden entirely — we never surface a stale older version. Putting deleted_ts in the inner scan would silently violate this. Compatibility ------------- Default (DEFAULT_LIST_NAMED_BLOBS_SQL_OPTION=2) is unchanged. Operators must opt in per fabric. Window functions require MySQL 8.0+ or TiDB. Testing Done ------------ - ./gradlew :ambry-named-mysql:test — all 11 unit tests pass - ./gradlew :ambry-api:compileJava :ambry-named-mysql:compileJava :ambry-named-mysql:compileTestJava :ambry-named-mysql:compileIntTestJava — clean build - Extended MySqlNamedBlobDbListOperationIntegrationTest @parameterized matrix to cover option 4 (× hard-delete on/off). Existing options 2 and 3 retained explicitly so the matrix stays stable as MAX moves to 4. - Local integration tests not run (require MySQL); CI will exercise option 4 against the same scenarios already covering options 2 and 3: TTL-expired latest version, soft-deleted records, prefix pagination, hard-delete on/off. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds testListHidesBlobWhenLatestVersionIsExpired to the parametrized LIST integration test. Constructs a blob with v1 (not expired) and v2 (latest, expired), then asserts LIST returns zero rows. This is the targeted test for the deleted_ts-placement invariant that options 2, 3, and 4 all rely on: when the latest version of a blob is expired or soft-deleted, the blob is hidden entirely; we never surface an older non-expired version. An optimization that pushes the deleted_ts predicate inside the windowed MAX(version) scan would silently leak the older version — empirically observed as 4 stale rows on a production prefix during the option-4 vetting comparison. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3260 +/- ##
=============================================
- Coverage 64.24% 51.21% -13.03%
+ Complexity 10398 8675 -1723
=============================================
Files 840 931 +91
Lines 71755 79540 +7785
Branches 8611 9525 +914
=============================================
- Hits 46099 40739 -5360
- Misses 23004 35416 +12412
- Partials 2652 3385 +733 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previous int-test run failed on testPutWithDigest (PUT-then-GET race unrelated to this PR's LIST SQL changes). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
beijxu
approved these changes
May 13, 2026
zichengl
added a commit
to zichengl/ambry
that referenced
this pull request
May 29, 2026
Two related changes: 1. NamedBlobPath.parseS3: collapse empty-string `prefix` to null. S3 clients can send `prefix=` with no value, which is semantically equivalent to omitting the prefix entirely. Pre-linkedin#3260 with option 3, `prefix=""` still completed under MAX_EXECUTION_TIME because the correlated-subquery plan streamed per-row probes. Under linkedin#3260 option 4, the same request executes LIST_WITH_PREFIX_SQL with `blob_name LIKE '%'`, which TiDB plans as Window-over-full-table- scan and can exceed the server kill on large containers. Routing the empty case to LIST_ALL_QUERY restores the bounded plan. 2. MySqlNamedBlobDb: gate LIST_ALL_QUERY behind the same listNamedBlobsSQLOption knob as LIST_WITH_PREFIX_SQL. Options 2/3 keep the existing INNER-JOIN + MAX-grouped subquery shape; option 4 adds a window-function variant matching the option-4 LIST_WITH_PREFIX_SQL plan shape and semantic (deleted_ts on OUTER select, hide-blob-if-latest-deleted). Operators flipping to option 4 get the same single-PK-scan-plus-streaming-window plan for both with-prefix and no-prefix LISTs, and a unified deleted-latest semantic across the two paths. Why this gap existed -------------------- linkedin#3260's integration matrix exercised non-empty prefixes only; the unbounded `LIKE '%'` case under option 4 was not validated. Reported via a downstream S3 correctness test failing testSuccessListObjectsEmptyPrefix after an option-4 canary deploy against a container with a substantial number of named blobs. Semantics --------- - Options 2/3 LIST_ALL_QUERY: unchanged. Inner deleted_ts filter preserved -> if latest version is soft-deleted, second-latest non-deleted surfaces. (Different from LIST_WITH_PREFIX_SQL options 2/3, but preserves pre-existing behavior.) - Option 4 LIST_ALL: window function with deleted_ts on OUTER select -> if latest version is soft-deleted, the blob is hidden entirely, matching LIST_WITH_PREFIX_SQL option 4. - Option 4 deployments inherit a behavior change on no-prefix LIST: latest-deleted blobs now hide instead of surfacing the second-latest. This unifies the semantic between with-prefix and no-prefix; option 2/3 deployments are unaffected. Compatibility ------------- - Option 4's window function requires MySQL 8.0+ or TiDB. Already enforced by config range and by the option-4 LIST_WITH_PREFIX_SQL shipped in linkedin#3260; no new compatibility constraint. - Default listNamedBlobsSQLOption remains 2. No behavior change for any deployment that has not opted into option 4. - The empty-prefix normalization is unconditional but only affects S3 callers, and only when they explicitly send `prefix=` empty. Non-S3 paths and S3 callers that omit prefix entirely are unchanged. Testing Done ------------ - ./gradlew :ambry-api:compileJava :ambry-named-mysql:compileJava :ambry-named-mysql:compileTestJava :ambry-named-mysql:compileIntTestJava -> BUILD SUCCESSFUL - ./gradlew :ambry-api:test --tests NamedBlobPathTest -> 10/10 pass including 3 new tests (empty-prefix-collapses-to-null, omitted-prefix-is-null, non-empty-prefix-preserved). - ./gradlew :ambry-named-mysql:test -> all unit tests pass. - Integration tests (intTest) not run locally; CI will exercise LIST_ALL via the existing MySqlNamedBlobDbListOperationIntegrationTest matrix for options 2/3/4. Durability risk: read-only LIST routing change. No write path, no schema, no atomicity, no callback, no resource cleanup. No durability concern. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zichengl
added a commit
that referenced
this pull request
May 30, 2026
…LL (#3265) * Fix option-4 empty-prefix LIST regression; add window-function LIST_ALL Two related changes: 1. NamedBlobPath.parseS3: collapse empty-string `prefix` to null. S3 clients can send `prefix=` with no value, which is semantically equivalent to omitting the prefix entirely. Pre-#3260 with option 3, `prefix=""` still completed under MAX_EXECUTION_TIME because the correlated-subquery plan streamed per-row probes. Under #3260 option 4, the same request executes LIST_WITH_PREFIX_SQL with `blob_name LIKE '%'`, which TiDB plans as Window-over-full-table- scan and can exceed the server kill on large containers. Routing the empty case to LIST_ALL_QUERY restores the bounded plan. 2. MySqlNamedBlobDb: gate LIST_ALL_QUERY behind the same listNamedBlobsSQLOption knob as LIST_WITH_PREFIX_SQL. Options 2/3 keep the existing INNER-JOIN + MAX-grouped subquery shape; option 4 adds a window-function variant matching the option-4 LIST_WITH_PREFIX_SQL plan shape and semantic (deleted_ts on OUTER select, hide-blob-if-latest-deleted). Operators flipping to option 4 get the same single-PK-scan-plus-streaming-window plan for both with-prefix and no-prefix LISTs, and a unified deleted-latest semantic across the two paths.
zichengl
added a commit
to zichengl/ambry
that referenced
this pull request
Jun 24, 2026
…ge gaps After linkedin#3265 fixed the empty-prefix LIST regression on option 4, two test-coverage gaps remained that linkedin#3265 didn't close: 1. MySqlNamedBlobDbListOperationIntegrationTest exercised list() only with non-null prefixes, so LIST_ALL_QUERY (and the new option-4 LIST_ALL_SQL window-function variant) had no parametrized integration coverage. The empty-prefix LIST failure that motivated linkedin#3265 specifically traversed this path (S3 SDK sends prefix= empty -> parseS3 collapses to null -> list() with null prefix -> LIST_ALL under option 4), and the int-test matrix would not have caught a regression in that branch. 2. S3IntegrationTest had no end-to-end test that explicitly emits `?prefix=` (parameter present, value empty) -- the exact request shape the AWS S3 SDK uses when ListObjectsRequest is built with an empty or unset prefix. Without it, a future refactor of S3ListHandler or NamedBlobPath.parseS3 that drops the empty->null collapse would slip past CI. This commit adds both: (a) ambry-named-mysql/src/integration-test: - testListNamedBlobsWithNullPrefix -- option-agnostic basic test that PUTs 5 blobs and asserts list(account, container, null, null, null) returns all 5 in blob_name order. Runs against options 2/3/4 via the existing @parameterized matrix. - testListAllNullPrefixHidesDeletedLatestUnderOption4 -- option-4 only via Assume.assumeTrue; mirrors the invariant testListHidesBlobWhenLatestVersionIsExpired added in linkedin#3260 but for the null-prefix path. Verifies that under option 4 a blob whose latest version is TTL-expired is hidden entirely from LIST_ALL_SQL (the deleted_ts-on-outer-SELECT contract). Options 2/3 have the opposite legacy semantic by design and are excluded. (b) ambry-frontend/src/integration-test: - s3ListEmptyPrefixTest -- exercises GET /s3/{account}/{container}?prefix= end-to-end through HTTP -> Netty -> S3ListHandler -> NamedBlobPath.parseS3 -> NamedBlobListHandler -> NamedBlobDb#list. Asserts 200 OK for both v1 (no list-type) and v2 (list-type=2) shapes. The named-blob DB in this integration test is InMemNamedBlobDbFactory, so this catches regressions in the S3-handler routing layer (empty-prefix collapse, parseS3 logic) but not SQL-side regressions -- those are covered by (a) against a real MySQL backend in :ambry-named-mysql:intTest. Full stitched coverage (S3 handler -> real MySQL with option 4 on a real container) is left to a follow-up that refactors S3IntegrationTest's buildFrontendVProps to accept a configurable NamedBlobDbFactory, allowing a sibling test class to swap in the MySQL-backed factory. That refactor wasn't bundled here to keep this diff small and review-focused on the specific gaps. Testing Done ------------ - ./gradlew :ambry-named-mysql:compileIntTestJava -> BUILD SUCCESSFUL. - ./gradlew :ambry-frontend:compileIntTestJava -> BUILD SUCCESSFUL. - ./gradlew :ambry-named-mysql:intTest --tests '*MySqlNamedBlobDbListOperationIntegrationTest.testListNamedBlobs WithNullPrefix*' --tests '*MySqlNamedBlobDbListOperationIntegrationTest.testListAllNullPrefix HidesDeletedLatestUnderOption4*' -> 12 runs (6 per method x 2 methods), 8 passed, 4 skipped. The 4 skipped are the option-4-only invariant on options 2/3 via Assume.assumeTrue, as intended. - ./gradlew :ambry-frontend:intTest --tests '*S3IntegrationTest.s3ListEmptyPrefixTest' -> 1 passed. - Full :ambry-named-mysql:intTest matrix runs against a local MySQL 8.0 container; passes 30/30 baseline + 12 new = 42 total with 8 added passes + 4 added Assume-skips. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zichengl
added a commit
that referenced
this pull request
Jul 15, 2026
…rge containers (#3272) * Close LIST_ALL_QUERY (null-prefix) and S3-handler empty-prefix coverage gaps After #3265 fixed the empty-prefix LIST regression on option 4, two test-coverage gaps remained that #3265 didn't close: 1. MySqlNamedBlobDbListOperationIntegrationTest exercised list() only with non-null prefixes, so LIST_ALL_QUERY (and the new option-4 LIST_ALL_SQL window-function variant) had no parametrized integration coverage. The empty-prefix LIST failure that motivated #3265 specifically traversed this path (S3 SDK sends prefix= empty -> parseS3 collapses to null -> list() with null prefix -> LIST_ALL under option 4), and the int-test matrix would not have caught a regression in that branch. 2. S3IntegrationTest had no end-to-end test that explicitly emits `?prefix=` (parameter present, value empty) -- the exact request shape the AWS S3 SDK uses when ListObjectsRequest is built with an empty or unset prefix. Without it, a future refactor of S3ListHandler or NamedBlobPath.parseS3 that drops the empty->null collapse would slip past CI. This commit adds both: (a) ambry-named-mysql/src/integration-test: - testListNamedBlobsWithNullPrefix -- option-agnostic basic test that PUTs 5 blobs and asserts list(account, container, null, null, null) returns all 5 in blob_name order. Runs against options 2/3/4 via the existing @parameterized matrix. - testListAllNullPrefixHidesDeletedLatestUnderOption4 -- option-4 only via Assume.assumeTrue; mirrors the invariant testListHidesBlobWhenLatestVersionIsExpired added in #3260 but for the null-prefix path. Verifies that under option 4 a blob whose latest version is TTL-expired is hidden entirely from LIST_ALL_SQL (the deleted_ts-on-outer-SELECT contract). Options 2/3 have the opposite legacy semantic by design and are excluded. (b) ambry-frontend/src/integration-test: - s3ListEmptyPrefixTest -- exercises GET /s3/{account}/{container}?prefix= end-to-end through HTTP -> Netty -> S3ListHandler -> NamedBlobPath.parseS3 -> NamedBlobListHandler -> NamedBlobDb#list. Asserts 200 OK for both v1 (no list-type) and v2 (list-type=2) shapes. The named-blob DB in this integration test is InMemNamedBlobDbFactory, so this catches regressions in the S3-handler routing layer (empty-prefix collapse, parseS3 logic) but not SQL-side regressions -- those are covered by (a) against a real MySQL backend in :ambry-named-mysql:intTest. Full stitched coverage (S3 handler -> real MySQL with option 4 on a real container) is left to a follow-up that refactors S3IntegrationTest's buildFrontendVProps to accept a configurable NamedBlobDbFactory, allowing a sibling test class to swap in the MySQL-backed factory. That refactor wasn't bundled here to keep this diff small and review-focused on the specific gaps. Testing Done ------------ - ./gradlew :ambry-named-mysql:compileIntTestJava -> BUILD SUCCESSFUL. - ./gradlew :ambry-frontend:compileIntTestJava -> BUILD SUCCESSFUL. - ./gradlew :ambry-named-mysql:intTest --tests '*MySqlNamedBlobDbListOperationIntegrationTest.testListNamedBlobs WithNullPrefix*' --tests '*MySqlNamedBlobDbListOperationIntegrationTest.testListAllNullPrefix HidesDeletedLatestUnderOption4*' -> 12 runs (6 per method x 2 methods), 8 passed, 4 skipped. The 4 skipped are the option-4-only invariant on options 2/3 via Assume.assumeTrue, as intended. - ./gradlew :ambry-frontend:intTest --tests '*S3IntegrationTest.s3ListEmptyPrefixTest' -> 1 passed. - Full :ambry-named-mysql:intTest matrix runs against a local MySQL 8.0 container; passes 30/30 baseline + 12 new = 42 total with 8 added passes + 4 added Assume-skips. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix option-4 LIST_ALL (empty-prefix S3 LIST) timeout/HTTP 500 on large containers An empty-prefix S3 ListObjects collapses to a null prefix (#3265) and routes to LIST_ALL. Under listNamedBlobsSqlOption=4, LIST_ALL used a window-function query (MAX(version) OVER (PARTITION BY blob_name)) whose derived table is materialized in full before the outer LIMIT -- it scans the entire container and cannot early-terminate. On large containers it ran past a short socket timeout, threw "Communications link failure", and returned HTTP 500. Because the empty-prefix path is the only way an S3 client reaches LIST_ALL, this blocks the option-4 rollout: any LIST with an empty prefix on a large container would 500. Fix: - Replace the option-4 LIST_ALL window query with a correlated-subquery form (the LIST_WITH_PREFIX option-3 shape, minus the prefix predicate). The outer scans candidates in PRIMARY KEY (blob_name) order and keeps only version = MAX(version); deleted_ts sits on the outer candidate, so a soft-deleted latest version hides the blob entirely (no older-version leak) -- preserving option-4 semantics. With the PK already blob_name-ordered, ORDER BY blob_name LIMIT N early-terminates after N matches instead of materializing the whole container. Options 2/3 LIST_ALL are unchanged. - run_list_v2 now switches the null-prefix binder by option (2/3 -> 5 params, 4 -> new constructListAllQueryV4 -> 7 params) to match the statement. - Add a default-off list query-timeout knob (mysql.named.blob.list.query.timeout.seconds) applied via Statement.setQueryTimeout, so a residual slow LIST surfaces a clean SQLException instead of tearing the connection (500). No-op for existing fabrics. Testing Done: - ./gradlew :ambry-named-mysql:intTest --tests "*MySqlNamedBlobDbListOperationIntegrationTest" against MySQL 8.0: 54 tests, 46 passed, 0 failed, 8 skipped (option-4-only invariants Assume-skip on options 2/3). Includes new testListAllNullPrefixPagination (options 2/3/4) and testListAllNullPrefixReturnsLatestVersionUnderOption4, plus the existing testListAllNullPrefixHidesDeletedLatestUnderOption4 regression guard (green under the new option-4 LIST_ALL). - ./gradlew :ambry-frontend:intTest --tests "*S3MySqlNamedBlobListIntegrationTest" --tests "*S3IntegrationTest" against MySQL 8.0: 4 passed, 0 failed. The new S3MySqlNamedBlobListIntegrationTest exercises the empty-prefix LIST end-to-end (S3 SDK -> Netty -> S3ListHandler -> parseS3 -> NamedBlobListHandler -> MySqlNamedBlobDb.list null-prefix -> option-4 LIST_ALL) against real MySQL and asserts 200 -- the stitched coverage previously deferred; existing S3IntegrationTest still passes after the buildFrontendVProps refactor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Zicheng Liu <8845012+zichengl@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mysql.named.blob.list.named.blobs.sql.option=4: a window-function variant ofLIST_WITH_PREFIX_SQLthat replaces option 3's correlated-subqueryMAX(version)lookup withMAX(version) OVER (PARTITION BY blob_name)over a single PK range scan.DEFAULT_LIST_NAMED_BLOBS_SQL_OPTION = 2). Opt-in per fabric — no behavior change for existing deployments until an operator flips the knob.deleted_ts-placement invariant shared by every LIST option.Why
TiDB's plan for option 3 on
named_blobs_v2isProjection → IndexJoin (outer: StreamAgg over IndexLookup | inner: PointGet by (account_id, container_id, blob_name, MAX(version))). Every outer candidate row drives a per-row PK probe against TiKV to fetchMAX(version)— even when the probe returns zero rows, it inherits the full coprocessor RPC deadline. Under transient single-region degradation in TiKV this fans out: hundreds of zero-result probes stall on the same degraded region and the LIST tail latency rises from sub-second to multi-second (and worse under sustained degradation).Option 4 runs a single PK range scan with a streaming window operator. No per-row inner probe; no deadline inheritance to amplify.
Empirical evidence
EXPLAIN ANALYZEon a production-replica TiDB against thecheckpoints/%prefix onaccount_id=1437, container_id=4540withLIMIT 10001:The plan tree for option 4 contains no
IndexJoin. The operator chain isProjection → Limit → Selection → Window → TableReader → Selection (cop[tikv]) → TableRangeScan (cop[tikv]).Correctness invariant — preserved and empirically verified
The
(deleted_ts IS NULL OR deleted_ts > UTC_TIMESTAMP(6))predicate lives on the outerWHERE, after the windowedMAX(version). This preserves option 3's exact semantics: when the latest READY version of a blob is expired or soft-deleted, the blob is hidden entirely; an older non-expired version never resurfaces.During vetting we also tested an alternate form with the
deleted_tspredicate pushed inside the windowed scan (0.26 s wall clock — faster, because TiKV gets to filter before shipping rows to TiDB). A set-difference query against the same production prefix showed 4 rows that the alternate form would have surfaced and option 4 correctly hides — older non-expired versions for blobs whose latest version has expired. Option 4 ships the correct-but-slightly-slower form deliberately; the alternate would leak stale blob versions to clients.Testing Done
Local build & unit tests
./gradlew :ambry-api:compileJava :ambry-named-mysql:compileJava :ambry-named-mysql:compileTestJava :ambry-named-mysql:compileIntTestJava— BUILD SUCCESSFUL../gradlew :ambry-named-mysql:test— 11/11 unit tests pass.New invariant test
testListHidesBlobWhenLatestVersionIsExpiredinMySqlNamedBlobDbListOperationIntegrationTest:namedBlobDb.list(...)returns zero entries.enableHardDeletetoggled on and off.deleted_tspredicate inside the windowed/groupedMAX(version)scan.Integration test matrix extended
@Parameterized.ParametersinMySqlNamedBlobDbListOperationIntegrationTestnow runs every existing LIST scenario against options 2, 3, and 4 (× hard-delete on/off). The matrix is pinned to literals{2, 3, 4}rather than{MIN, MAX}so future range bumps cannot silently drop coverage of an option. Existing scenarios covered:IN_PROGRESSblob while an older READY exists — LIST returns the older READY.LIMITboundary respected.Integration tests are deferred to CI (require live MySQL).
Production-replica verification
The
EXPLAIN ANALYZEnumbers above were captured against a production-replica TiDB on the actualcheckpoints/%workload. The 4-row leak set-difference was also computed there.Durability risk analysis
Page<NamedBlobRecord>return shape and content.option=4at config-validation time (getIntInRange) and falls back to its own configured option. New code on a DB written by older code is unaffected. No deployment-ordering constraint.Rollout plan
2. PR is a no-op for every existing deployment.mysql.named.blob.list.named.blobs.sql.option=4onprod-lor1first: smallest blast radius and the fabric where the original incident was observed.statements_summarydigest for option 3 should be replaced by a new digest for option 4; the new digest's p50/p99 latency must drop or stay flat.namedBlobListRate.<dc>flat ±2 %. Zero 10-second coprocessor-deadline timeouts on the new digest. No new error class in frontend logs.=3. Single line, no code revert needed.Out of scope (deliberate)
LIST_ALL_QUERY(the no-prefix LIST path) is not rewritten in this PR. It uses an INNER-JOIN+MAX(version) shape that may carry similar plan-risk and possibly a relateddeleted_ts-placement question of its own — tracking as a follow-up so this PR stays surgical and easy to review.🤖 Generated with Claude Code