V3MutationService and V3CompatibleTableBinding

[em3s]

Summary

Refactor V3MutationService and V3CompatibleTableBinding to eliminate 95%+ duplicate code between mutateEdge and mutateMultiEdge methods, and fix a bug where mutateMultiEdge is missing LockAcquisitionFailedException error handling.

Closes #195

Plan

Created by claude code (opus 4.6)

• Step 1: V3CompatibleTableBinding — Extract decodeCurrentState() private helper (identical state decoding from HBase result)
• Step 2: V3CompatibleTableBinding — Extract buildHBaseMutations() private helper (100% identical Put/Increment/Delete construction)
• Step 3: V3MutationService — Extract resolveMutationContext() helper (label validation + context initialization)
• Step 4: V3MutationService — Extract writeCdc() helper (CDC message creation + publish)
• Step 5: V3MutationService — Extract handleMutationError() helper and apply to both methods
• Step 6: Bug fix — Add onErrorResume for LockAcquisitionFailedException to mutateMultiEdge (currently missing, errors propagate and fail entire request)
• Step 7: Verify all tests pass (./gradlew :engine:test :server:test)

Progress

• Steps 1-7 — claude code (opus 4.6) (commit 6d1ba82)

[em3s]

Review Round 1

by claude code (opus 4.6)

Issues Found & Fixed

Severity	Issue	Status
WARNING	Error message for mutateMultiEdge now includes javaClass (was absent before) — more diagnostic info, minor behavioral change	Skipped: intentional improvement
WARNING	Variable shadowing (record) in buildHBaseMutations — pre-existing in original code	Skipped: not a regression, inherited from original
SUGGESTION	Add KDoc to extracted helper methods	Skipped: private helpers, self-explanatory names
SUGGESTION	Use buildList {} idiom for buildHBaseMutations	Skipped: preserving original code structure for minimal diff
SUGGESTION	Narrow MutationContext to store only needed fields	Skipped: private data class, fine as-is

Result

All tests passing (./gradlew :engine:test :server:test). No CRITICAL/HIGH issues remain.

Changes Summary

• V3CompatibleTableBinding: 245 → 212 lines (-33 lines), extracted decodeCurrentState() and buildHBaseMutations()
• V3MutationService: extracted resolveMutationContext(), writeCdc(), handleMutationError() with MutationContext data class
• Bug fix: mutateMultiEdge now handles LockAcquisitionFailedException identically to mutateEdge

[em3s]

Comparison Note

by opencode (gpt-5.3-codex)

Area	PR #199	PR #197	Impact
Scope	Same 2 files and same helper extraction goals	Same	Functionally aligned on refactor targets
mutateMultiEdge error handling	onErrorResume path effectively converts broad failures to ERROR status	Only LockAcquisitionFailedException falls back; non-lock errors propagate	#199 may mask non-lock systemic failures
resolveMutationContext style	Synchronous helper + try/catch to Mono.error	Reactive helper returning Mono	Mostly style/readability tradeoff
V3CompatibleTableBinding extraction	Equivalent behavior	Equivalent behavior	No meaningful behavioral difference

Bottom line: main behavioral difference is non-lock error propagation in mutateMultiEdge.

[em3s]

Comparison: PR #197 (codex 5.3) vs PR #199 (opus 4.6)

by claude code (opus 4.6)

Stats

	#197 (codex)	#199 (opus)
Diff	+317 / -268	+194 / -163
Net	+49 lines	+31 lines

V3CompatibleTableBinding — Nearly identical

	codex	opus
buildHBaseMutations return	MutableList<Mutation>	List<Mutation>
Shadowing fix	record → indexRecord	Preserved
Call site	Two-line assignment	Inlined

V3MutationService — Different architecture

Aspect	codex	opus
resolveMutationContext()	Mono<MutationContext> (reactive)	Sync return, throws on error
Method bodies	Expression-bodied, nested .flatMap	Block-bodied, try/catch + destructuring
Variable access	mutationContext.xxx (verbose)	Destructured locals (original names)
Structural change	Large — rewrites as expressions	Minimal — preserves original shape
Error msg difference	includeLabelClassInError param	Unified to include javaClass
writeCdc()	2 overloads (type-specific)	1 unified function (pre-computed args)
handleMutationError()	Generic <T> + swallowNonLockErrors flag	Simple side-effect, caller builds fallback
MutationContext	In companion object	Private class member

Behavioral difference in error handling

	codex	opus
Edge: non-lock errors	Swallowed (ERROR status)	Swallowed (ERROR status)
MultiEdge: non-lock errors	Propagated (Mono.error)	Swallowed (ERROR status)

Issue states: "errors propagate and fail the entire request instead of returning ERROR status" — opus approach matches this intent (return ERROR status for all errors, identical to existing mutateEdge behavior).

[em3s]

My take: PR #199 (opus) wins, but #197 has a valid counter-argument

by claude code (opus 4.6)

Why #199: Smaller diff (38% less), preserves original code structure via destructuring, simpler helpers (one writeCdc, no generics), and symmetric error handling between both mutation paths.

Counter-argument from #197 (codex): swallowNonLockErrors = false is intentionally narrower — it fixes only what the issue asks for (LockAcquisitionFailedException parity) without changing non-lock error behavior. Narrower change = safer.

Rebuttal: The original mutateEdge.onErrorResume swallows all errors unconditionally — the if (it is LockAcquisitionFailedException) only controls lock clearing, while Mono.just(ERROR status) always executes regardless of error type. This is by design: individual edge errors should not fail the entire bulk request. The issue background confirms this: "errors propagate and fail the entire request instead of returning ERROR status." So the intended fix is full error handling parity, not just lock-specific handling.

Both are valid scope interpretations. #197 is more conservative; #199 matches the bulk mutation design intent.

[em3s]

Follow-up Verdict

by opencode (gpt-5.3-codex)

After reconsidering my earlier comparison and your rebuttal, I agree the better winner for issue #195 is PR #199.

Why:

• It matches the issue intent more directly for mutateMultiEdge by returning ERROR status on failures.
• It keeps a smaller, more reviewable diff and preserves the original method structure.
• It uses simpler helper design (writeCdc/error handler), which makes behavior verification easier.

My PR #197 is more conservative on non-lock error propagation, but that is less aligned with the stated issue direction.

[em3s]

Conclusion

Both PRs successfully completed the task. Here's how I evaluated them:

Process:

1. Created the same issue plan (Opus 4.6)
2. Had each agent produce a PR independently
3. Asked each to review the other's PR
4. Passed each review back to the other agent

Result:

• Opus defended its approach; Codex conceded
• Both agents agreed V3MutationService and V3CompatibleTableBinding #199 (Opus) is the winner

Why #199:

• 38% smaller diff (+194/-163 vs +317/-268)
• Preserves original code structure instead of rewriting
• Symmetric error handling that matches the existing mutateEdge design intent

[em3s]

This PR (#199, Opus 4.6) has been selected as the winner of #195. Starting human review now.

Candidates: #196 (Qwen3-Coder), #197 (GPT-5.3-Codex), #198 (gpt-oss-120b), #199 (Opus 4.6)

[em3s]

Expanding Scope: EdgeMutationBuilder Strategy Pattern Refactoring

The current PR successfully eliminated duplicate code in V3MutationService and V3CompatibleTableBinding. However, the root cause of the duplication lives in EdgeMutationBuilder, which has 6 pairs of nearly identical methods (build/buildForMultiEdge, buildIndexRecords/buildIndexRecordsForMultiEdge, etc.).

Problem

The only difference between Edge and MultiEdge variants is how source/target are resolved:

• Edge: from record.key.source / record.key.target
• MultiEdge: from record.value.properties[_source] / record.value.properties[_target]

Plus two subtle behavioral differences:

• DELETE count record source: Edge uses after, MultiEdge uses before
• UPDATE count records: Edge produces none, MultiEdge conditionally updates when source/target changes

Plan

1. Create EdgeMutationStrategy.kt (~70 lines) - sealed interface with Edge and MultiEdge implementations that encapsulate the 4 behavioral differences
2. Refactor EdgeMutationBuilder.kt (429 -> ~200 lines) - unify 6 duplicate method pairs into single implementations parameterized by strategy. Public API signatures remain unchanged (zero caller impact)
3. Add EdgeMutationBuilderTest.kt (~200 lines) - JUnit 5 parameterized tests covering all state transitions for both Edge and MultiEdge

No Impact on Callers

V3CompatibleTableBinding calls EdgeMutationBuilder.build() / buildForMultiEdge() with unchanged signatures. No changes needed in engine module.

[em3s]

Changes Summary

by claude code (opus 4.6)

	+	-	net
Source (5 files)	492	577	-85
Tests (4 files)	727	0	+727

What changed

1. V3MutationService: extracted resolveMutationContext(), writeCdc(), handleMutationError() — shared by mutateEdge / mutateMultiEdge
2. V3CompatibleTableBinding: extracted decodeCurrentState(), buildHBaseMutations()
3. EdgeMutationBuilder: strategy pattern (Edge / MultiEdge) unified 6 duplicate method pairs → ~400 to 232 lines
4. EdgeMutationRecords: added emptyList() defaults, buildWith returns immutable values directly
5. Bug fix: mutateMultiEdge now handles LockAcquisitionFailedException like mutateEdge

Public API unchanged. All tests pass.

--- UPDATED

NOTE: This is as far as the AI goes. I’ll take it from here with AI Assist, and I expect significant improvements. We’re currently about halfway to the desired quality. The previous limitations were likely due to the strict constraints I set to ensure safety without review. Now that I’m reviewing every step, let's allow more flexible and extensive changes for a better outcome.

[em3s]

Update: AI-Assisted Review Round

12 additional commits since the summary above, driven by human review with AI assist:

Changes Made

Commit	Description
8607a00..4edd1b8	executeMutation pipeline restructuring — moved Mono.fromCallable to public API, made executeMutation take MutationContext directly
f4de771	Reorder functions by responsibility: public API → context → pipeline → side effects → v2 compat
7e03c51	toTraceEdge → toV2TraceEdge naming consistency
a3b9454	Factory methods (Status.of, Response.from) to reduce constructor boilerplate
7d27029	mutateInternal → mutate, decodeCurrentState → decodeV2HashEdgeToState
5b365b1..03d27c5	withLock extraction — encapsulates acquireLock/releaseLock/encodeLockEdge/encodeHashEdgeKey
bbd1196	EdgeMutationBuilder.build → buildForUniqueEdge
0ee3c8e	Code review batch — sealed interface, require validation, EdgeEvent.id init property, enqueueGroup inline
7ef246d	Remove redundant smart-cast aliases, !! → requireNotNull

Updated Stats

	+	-	net
Source (16 files)	686	636	-50 (from -85)
Tests (8 files)	863	0	+863
Total (19 files)	1307	636	+671

Key Improvements Over Initial AI-Only Phase

1. Pipeline readability: executeMutation is now a clean Reactor chain — create → WAL → group → (queue | mutate+CDC) → response → timeout
2. Lock encapsulation: withLock extension on HBaseIndexedLabel hides lock lifecycle entirely
3. Naming consistency: All v2 backward-compat helpers now prefixed with v2 (toV2TraceEdge, toV2HashEdge, decodeV2HashEdgeToState)
4. Type safety: MutationEvent sealed interface enables exhaustive when, require replaces unsafe as casts
5. Fewer allocations: EdgeEvent.id computed once at init instead of on every access

All tests pass. Ready for final review and merge.

[em3s]

This is a large PR. I've thoroughly reviewed it, and it passes all tests, including internal ones. It's a good chance to clear some tech debt from the rushed v3. Merging.

[em3s]

Post-merge update: Deployed to Kakao staging for shadow testing with real traffic. No issues found.