Widen compaction service counts to full kNumOfReasons

[hx235]

Summary:
Widen the serialized "counts" array in CompactionServiceResult from kNumOfReasons - 1 back to the full kNumOfReasons, so the count for the newest CompactionReason is included in the remote compaction result.

The width was previously reduced by one to match readers that expected the narrower array; restoring it changes what is written. This is safe only because deserialization of "counts" is now size-tolerant (a reader expecting fewer elements drops the extra, one expecting more zero-fills the missing). It must not land until that size-tolerant read has been deployed to every primary, including rollback targets so should be 1-2 releases ahead; otherwise a primary can still or fallback to running the strict reader would reject the wider result and fail deserialization of this field.

== Test plan ==
Legend

CompactionReason is an enum whose last entry is kNumOfReasons (currently 21). CompactionStats::counts is an int[kNumOfReasons] holding one count per reason; it is serialized into the remote compaction result and read back by the primary.

• "narrow" = kNumOfReasons - 1 counts (omits the newest reason); what current code writes.
• "wide" = kNumOfReasons counts (includes the newest reason).
• strict reader: accepts only its exact expected element count; any other count fails deserialization (the plain OptionTypeInfo::Array behavior).
• tolerant reader: accepts any count -- more elements than expected are dropped, fewer are zero-filled; never fails on a count mismatch (this change, SizeTolerantDeserializeArray).

Unit test

CompactionServiceTest.CompatCheckCountsWidthTolerance -- verifies that deserializing "counts" tolerates a serialized element count equal to, one greater than, or one less than the reader's expected width (extras dropped, missing zero-filled), and that the binary serializes exactly the expected number of elements.

Manual cross-version verification (ldb remote_compaction)

Test 1: expect-narrow tolerant-read primary reads wide counts a worker write (extra reason dropped)

$ tolerant_wide remote_compaction_worker  --db=$DB --job_dir=$JOB &
$ tolerant_narrow remote_compaction_primary --db=$DB --job_dir=$JOB
remote_compaction_worker: OK (4915 bytes result)
remote_compaction_primary: OK
$ grep 'remote compaction result' $DB/LOG
[default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst

=> PASS: the primary parsed and installed the remote result.

Test 2: expect-wide tolerant-read primary reads narrow counts a worker writes (missing reason zero-filled)

$ strict_narrow remote_compaction_worker  --db=$DB --job_dir=$JOB &
$ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB
remote_compaction_worker: OK (4911 bytes result)
remote_compaction_primary: OK
$ grep 'remote compaction result' $DB/LOG
[default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst

=> PASS: the primary parsed and installed the remote result.

Test 3: expect-wide tolerant-read primary reads wide counts a worker writes

$ tolerant_wide remote_compaction_worker  --db=$DB --job_dir=$JOB &
$ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB
remote_compaction_worker: OK (4917 bytes result)
remote_compaction_primary: OK
$ grep 'remote compaction result' $DB/LOG
[default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst

=> PASS: the primary parsed and installed the remote result.

Differential Revision: D111068809

[meta-codesync]

@hx235 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D111068809.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 309.7s.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI reached the early-review threshold — reviewing commit bbb1d1e

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI reached the early-review threshold — reviewing commit bbb1d1e

---

Summary

This PR correctly addresses a real serialization compatibility problem in remote compaction by introducing SizeTolerantDeserializeArray, which replaces the strict ParseArray with a tolerant parse that drops extras and zero-fills missing elements. The approach is sound: it overrides only the parse callback while inheriting serialization and comparison from the existing Array<T, kSize> infrastructure. The new unit test is well-structured and covers the key scenarios.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Unnecessary #include <memory> in test file -- compaction_service_test.cc:6

• Issue: The diff adds #include <memory> to the test file, but the new test code does not use std::unique_ptr, std::make_unique, std::make_shared, or any other facility from <memory> that isn't already transitively available. The existing std::shared_ptr usages in the file were already working without this include.
• Root cause: Likely a leftover from an earlier draft.
• Suggested fix: Remove the #include <memory> addition unless it fixes a specific compilation issue on a supported platform (e.g., libc++ which has fewer transitive includes). If kept for libc++ portability, add a comment explaining why.

M2. return Status(std::move(s)) is redundant -- compaction_service_job.cc (new SizeTolerantDeserializeArray lambda)

• Issue: The parse lambda contains return Status(std::move(s)); which explicitly constructs a new Status from a moved Status. This is functionally correct but needlessly verbose. NRVO / move semantics already apply to return s;. The explicit Status(std::move(s)) creates an extra move-construct that a good compiler will elide, but it's not idiomatic with the rest of the codebase.
• Root cause: Overly cautious coding.
• Suggested fix: Replace with return s;.

🟢 LOW / NIT

L1. Comment in old code removed but replacement comment could be clearer -- compaction_service_job.cc

• Issue: The old code had a lengthy comment explaining the kNumOfReasons - 1 workaround and a TODO. The new code removes this entirely (which is correct since the workaround is being replaced), but the SizeTolerantDeserializeArray function comment could benefit from a brief note about why size tolerance is needed (cross-version compat in remote compaction) in addition to what it does.
• Suggested fix: Optional -- the existing comment on the function is adequate.

L2. Test helper AppendToCountsFields uses assert instead of test assertions -- compaction_service_test.cc

• Issue: The test helper functions use assert(semi != std::string::npos) etc. In release-mode test builds, these assertions would be compiled out. While test binaries are typically built in debug mode, using ASSERT_NE or at minimum a check that doesn't compile away would be more robust.
• Suggested fix: This is consistent with other test helpers in the codebase that also use assert, so this is a minor style note rather than an actionable issue.

L3. CountFirstCountsFieldElements returns 0 for empty field but doesn't test that path -- compaction_service_test.cc

• Issue: The helper has a if (semi == val_begin) return 0; path that is never exercised by the test. This is fine since the path would only be hit with a malformed counts=; input which isn't a realistic scenario.
• Suggested fix: No action needed.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
Remote compaction (primary reads)	YES	YES -- tolerant parse handles width mismatch	None
Remote compaction (worker writes)	YES	YES -- serializes full kNumOfReasons	None
Old primary + new worker	YES	Deserialization may fail with strict reader, BUT fallback to local compaction handles this gracefully (line 192-209)	Deployment ordering matters (see PR description)
New primary + old worker	YES	Tolerant reader zero-fills the missing element	None
TEST_Equals comparison	YES	Comparison uses kSize=kNumOfReasons for both sides; if one side was deserialized from narrow data, missing entries are zero-filled, so comparison is consistent	None
Local compaction	NO	counts[] serialization is only used in remote compaction path	None
ReadOnly DB	NO	No compaction	None

Deployment safety: The PR description correctly identifies that this change should land 1-2 releases after the tolerant reader, so that rollback targets always have the tolerant reader. The fallback-to-local logic provides a safety net even if deployment ordering is violated -- the primary will fall back to local compaction if deserialization fails.

Positive Observations

1. Clean design: Overriding only the parse callback while inheriting serialize/compare from Array<T, kSize> is elegant and minimally invasive. It avoids duplicating serialization logic.

2. Good test coverage: The test covers three key scenarios (same width, wider peer, narrower peer) and verifies both the format (element count) and the semantics (correct values read back).

3. Graceful degradation: Even without the tolerant reader deployed everywhere, the existing fallback-to-local-compaction logic (lines 192-209 in compaction_service_job.cc) ensures that a deserialization failure doesn't cause data loss or corruption -- it just triggers a local re-compaction.

4. Correct use of ParseVector: Using the existing ParseVector infrastructure rather than writing custom parsing logic reduces the chance of parsing bugs and maintains consistency with the codebase.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit bbb1d1e

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit bbb1d1e

---

Summary

Clean, well-scoped change that correctly solves the cross-version serialization compatibility problem for the counts field in remote compaction results. The SizeTolerantDeserializeArray approach is sound: it reuses the existing Array serialization/comparison while only overriding the parse function to be size-tolerant. The counts field is purely for metrics and does not affect compaction correctness or data integrity.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. return Status(std::move(s)) is unnecessary — compaction_service_job.cc (diff line +800)

• Issue: The parse lambda contains return Status(std::move(s)); which explicitly constructs a new Status from the moved original. A plain return s; would achieve the same result (NRVO or implicit move), and is the idiomatic pattern throughout the codebase.
• Root cause: Likely copy-paste or over-cautious coding.
• Suggested fix: Replace with return s;.

M2. Test helpers use assert() instead of gtest macros — compaction_service_test.cc (diff lines +28, +30, +46, +48, +56, +57)

• Issue: AppendToCountsFields, RemoveLastFromCountsFields, and CountFirstCountsFieldElements use C assert() for internal precondition checks. In release test builds (NDEBUG), these asserts are compiled out, meaning malformed test input would silently produce wrong results rather than failing the test.
• Suggested fix: Either convert to ASSERT_* by restructuring, or accept the risk since these are simple helpers called only from this one test.

M3. #include <memory> addition appears unnecessary — compaction_service_test.cc (diff line +3)

• Issue: The new code in this PR does not use any types from <memory>. Existing std::shared_ptr usage is already satisfied via transitive includes from db_test_util.h.
• Suggested fix: Remove unless required by IWYU policy.

🟢 LOW / NIT

L1. Consider making SizeTolerantDeserializeArray a general utility

• Issue: This tolerance pattern could be useful for other cross-version serialization arrays. Currently local to compaction_service_job.cc.
• Suggested fix: Move to a shared header when a second use case arises.

L2. Test only covers +1/-1 element mismatch

• Issue: Does not test +2 or more, or zero-element edge cases. Unlikely in practice but worth a brief comment explaining the intended coverage scope.

Cross-Component Analysis

Context	Affected?	Safe?	Notes
Old primary (strict reader) + New worker (wider write)	YES	YES	Deserialization fails; fallback to local compaction (line 192-210)
New primary (tolerant reader) + Old worker (narrow write)	YES	YES	Zero-fills missing count
ReadOnly DB	NO	N/A	No compaction
WritePreparedTxnDB	YES (if remote compaction)	YES	counts is stats-only
TEST_Equals comparison	YES	YES	Both sides use same binary's kSize

Type safety of C-array ↔ std::array cast: The existing OptionTypeInfo::Array<T, kSize> (options_type.h:392) already casts void* to std::array<T, kSize>* for C-array fields. The new code follows the exact same pattern. No new risk.

Serialization symmetry: Write uses inherited SerializeArray<int, kNumOfReasons> (21 elements). Read uses ParseVector<int> + copy loop. Round-trip is correct for same-version peers.

Positive Observations

• Good incremental approach: Deploying tolerant readers first, then widening the write, is the correct strategy for cross-version compatibility.
• Test design: Well-structured, testing same/wider/narrower scenarios and pinning serialized width.
• Minimal blast radius: Only the parse function is overridden; serialize and compare are inherited unchanged.
• Clean TODO removal: The old workaround comment is properly resolved.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase