[494] redis failover outbox 도입 및 redis key를 hash 구조로 변경#234
Merged
Conversation
📝 WalkthroughWalkthroughAdds a DB-backed Redis Failover Outbox: schema, mapper, domain payloads, services, retry scheduler and strategy registry, versioned Redis CAS Lua scripts, idempotent refill support, and integration into refill and policy write-through/hydration flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Application (DB tx)
participant DB as Database
participant Outbox as TRAFFIC_REDIS_OUTBOX
participant Redis as Redis (CAS scripts)
rect rgba(100,200,100,0.5)
Client->>DB: Business update (e.g., refill / policy change)
DB->>Outbox: INSERT PENDING outbox record (uuid,payload)
Outbox-->>DB: outboxRecordId
DB-->>Client: return result w/ outbox metadata
Client->>Redis: Execute versioned CAS Lua or applyRefillWithIdempotency
alt Redis SUCCESS
Redis-->>Client: success
Client->>Outbox: markSuccess(outboxRecordId)
else Redis STALE
Redis-->>Client: stale
Client->>Outbox: markSuccess or markFail (per mapping)
else Redis ERROR/TIMEOUT
Redis-->>Client: error
Client->>Outbox: leave PENDING or markFail (per flow)
end
end
sequenceDiagram
participant Scheduler as Outbox Retry Scheduler
participant DB as TRAFFIC_REDIS_OUTBOX
participant Registry as Strategy Registry
participant Strategy as Outbox Strategy
participant Redis as Redis
rect rgba(100,150,200,0.5)
Scheduler->>DB: lockRetryCandidatesAndMarkProcessing(limit,...)
DB-->>Scheduler: records
loop for each record
Scheduler->>Registry: get(strategy for eventType)
Registry-->>Scheduler: Strategy
Scheduler->>Strategy: execute(record)
Strategy->>Redis: run CAS or applyRefillWithIdempotency
alt Strategy SUCCESS
Redis-->>Strategy: success
Strategy-->>Scheduler: SUCCESS
Scheduler->>DB: markSuccess(id)
else Strategy FAIL
Strategy-->>Scheduler: FAIL
Scheduler->>DB: markFailWithRetryIncrement(id) or terminal handling
alt REFILL terminal exceeded
Scheduler->>DB: markRevertIfCompensable(id)
Scheduler->>DB: invoke restore via support service
end
end
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java (1)
169-190:⚠️ Potential issue | 🔴 CriticalUse the same CAS Lua script path as
TrafficPolicyVersionedRedisServiceinstead of pipelined raw HSET operations.The two separate
stringOperations.opsForHash().put()calls for "value" and "version" are not atomic—pipelining batches commands but executes them independently. A concurrent write-through update can land between the two calls, causing bootstrap to overwrite a newer version or leave an inconsistent (value, version) pair. Replace this with a Lua script that atomically compares the version before updating, matching the pattern used bysyncVersionedValue()inTrafficPolicyVersionedRedisService.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java` around lines 169 - 190, The current pipelined loop in TrafficPolicyBootstrapService (inside cacheStringRedisTemplate.executePipelined SessionCallback) does two separate opsForHash.put calls on policyKey for "value" and "version", which is non-atomic; replace that per-policy pair of HSETs with an invocation of the same CAS Lua script used by TrafficPolicyVersionedRedisService.syncVersionedValue so the update compares the existing version and atomically sets both "value" and "version" only when allowed. Concretely: remove the two stringOperations.opsForHash().put(...) calls for each policyKey, build the same Lua script key/arg invocation (use the same script path/sha/mechanism as syncVersionedValue), pass policyKey and expectedVersion/bootstrapVersionEpochMillis and desired value ("1"/"0") as args, and call the script via the RedisTemplate (or the helper method used by TrafficPolicyVersionedRedisService) so each policy update is performed with the CAS Lua logic and not two separate HSETs.
🧹 Nitpick comments (4)
src/main/resources/db/migration/V2603150500__create_table_traffic_redis_outbox.sql (1)
8-8: Consider adding an index onuuidif lookups by idempotency key are frequent.The
uuidcolumn is used for idempotency checks (e.g., REFILL events). If the application performs lookups byuuidto check for duplicate processing, an index would improve query performance.💡 Optional index for uuid lookups
KEY idx_traffic_redis_outbox_status_created_at (status, created_at), - KEY idx_traffic_redis_outbox_status_updated_at (status, status_updated_at) + KEY idx_traffic_redis_outbox_status_updated_at (status, status_updated_at), + KEY idx_traffic_redis_outbox_uuid (uuid) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V2603150500__create_table_traffic_redis_outbox.sql` at line 8, Add a non-unique index on the uuid column to speed idempotency lookups: update the migration that creates the outbox table to include an index on the uuid column (e.g., INDEX idx_traffic_redis_outbox_uuid (uuid)) or add a separate CREATE INDEX statement for the table; ensure the index name is descriptive (idx_traffic_redis_outbox_uuid) and use a normal B-tree index unless uniqueness is required.src/test/java/com/pooli/traffic/service/decision/TrafficDefaultQuotaSourceAdapterTest.java (1)
47-48: Assert the zero-actual branches never enqueue outbox work.The new mock is only exercised on successful claims. Please add
never()checks to the non-positive-request and failed-UPDATE tests; otherwise a regression that creates refill outbox rows withactual=0will still pass here.verify(redisOutboxRecordService, never()).createPending(Mockito.any(), Mockito.any(), Mockito.any());Also applies to: 89-90, 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/pooli/traffic/service/decision/TrafficDefaultQuotaSourceAdapterTest.java` around lines 47 - 48, Add assertions that redisOutboxRecordService never enqueues outbox work in the tests covering the non-positive-request branch and the failed-UPDATE branch: after invoking the code under test, call verify(redisOutboxRecordService, never()).createPending(Mockito.any(), Mockito.any(), Mockito.any()) to ensure createPending is not called; apply the same never() verification to the other indicated test cases so regressions that create pending outbox rows with actual=0 will fail the tests.src/main/resources/mapper/traffic/TrafficRefillSourceMapper.xml (1)
41-47: Consider adding a safeguard against negative or excessive restore amounts.The deduct operations include a
remaining_data >= #{deductAmount}guard, but the restore operations have no validation. A negativerestoreAmountwould act as a deduction, and there's no upper-bound check.If input validation is guaranteed by the caller, this is acceptable. Otherwise, consider adding
AND #{restoreAmount} > 0to the WHERE clause for defense-in-depth.🛡️ Optional safeguard
<update id="restoreIndividualRemaining"> UPDATE LINE SET remaining_data = remaining_data + #{restoreAmount}, updated_at = NOW() WHERE line_id = #{lineId} AND deleted_at IS NULL + AND #{restoreAmount} > 0 </update>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/mapper/traffic/TrafficRefillSourceMapper.xml` around lines 41 - 47, The update id "restoreIndividualRemaining" currently adds #{restoreAmount} to remaining_data without validation; modify this mapper to guard against negative or excessively large restores by adding a WHERE clause condition such as AND #{restoreAmount} > 0 to prevent negative values from acting as deductions, and optionally enforce an upper bound (e.g., cap with a max capacity or use a DB-side cap when updating remaining_data) referencing remaining_data, #{restoreAmount}, and line_id in the change.src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.java (1)
59-62: Consider logging when skipping invalid candidates.Silently skipping candidates with null
idoreventTypecould mask data integrity issues in the outbox table. A debug or warn log would help diagnose such anomalies.📝 Suggested improvement
for (RedisOutboxRecord candidate : candidates) { if (candidate.getId() == null || candidate.getEventType() == null) { + log.warn("traffic_outbox_invalid_candidate id={} eventType={}", candidate.getId(), candidate.getEventType()); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.java` around lines 59 - 62, In RedisOutboxRetryScheduler's loop that iterates over candidates (the block checking candidate.getId() == null || candidate.getEventType() == null), add a concise log statement (preferably logger.warn or logger.debug depending on severity) when skipping an invalid candidate that includes which field is null and key identifying info (e.g., candidate.getId(), candidate.getEventType(), or the candidate object) so you can diagnose data integrity issues; update the skip branch to log the reason before continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/traffic-outbox-part-c-plan.md`:
- Around line 285-291: Update Section 10.1 to remove or reword the claim that
Outbox classes "전혀 존재하지 않습니다" and reflect that this PR introduces the listed
artifacts; specifically, mention that the package
com.pooli.traffic.domain.outbox now contains RedisOutboxRecord, OutboxEventType,
OutboxStatus, OutboxRetryResult, add RedisOutboxMapper (MyBatis),
RedisOutboxRecordService and RedisOutboxRetryScheduler, and the
com.pooli.traffic.service.outbox.strategy package with its strategy
interface/registry and the nine implementations; ensure the paragraph no longer
contradicts the PR by either deleting the stale sentence or replacing it with a
note about the new implementations and their locations (use the class and
package names above to locate the changes).
- Line 38: Several fenced code blocks in docs/traffic-outbox-part-c-plan.md lack
language identifiers, triggering MD040; update each triple-backtick block to
include an appropriate language token (e.g., change ``` to ```text for
human-readable snippets like the block starting with "[요청 인입]" and to ```lua for
Redis script blocks beginning with comments such as "-- KEYS[1]: 대상 Redis Key").
Locate the plain ``` fences around those examples and add the correct language
marker (text or lua) so all code fences (including the blocks that contain the
Lua script using KEYS[1]) are annotated for lint compliance and readability.
In
`@src/main/java/com/pooli/traffic/service/outbox/strategy/AbstractPolicyOutboxRetryStrategy.java`:
- Around line 11-18: The mapPolicySyncResult method in
AbstractPolicyOutboxRetryStrategy incorrectly maps
PolicySyncResult.CONNECTION_FAILURE to OutboxRetryResult.SUCCESS causing
transient Redis/network errors to be treated as completed; update
mapPolicySyncResult so that PolicySyncResult.CONNECTION_FAILURE returns
OutboxRetryResult.FAIL (like other transient failures) to allow the retry
scheduler to retry the outbox record, referencing the method name
mapPolicySyncResult and the enums PolicySyncResult and OutboxRetryResult; ensure
no other cleanup is assumed (unlike RefillOutboxStrategy.restoreClaimedAmount),
just change the mapping to FAIL.
In
`@src/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.java`:
- Around line 44-47: The duplicate-idempotency short-circuit is unsafe: when
tryRegisterIdempotency(payload.getUuid()) returns false the code immediately
returns OutboxRetryResult.SUCCESS while the actual refill logic (the refill step
that runs later for the same payload UUID) may not have executed, risking lost
writes; fix this by making idempotency registration and the refill atomic (e.g.
merge tryRegisterIdempotency + the refill update into a single Redis LUA script
or Redis transaction) or, if atomic change is not feasible, when
tryRegisterIdempotency returns false perform a verification/repair step that
checks whether the refill for payload.getUuid has already been applied and only
return SUCCESS if the verification confirms the refill; update the code paths
around tryRegisterIdempotency and the refill logic so they use the combined
atomic operation or include the verification before returning
OutboxRetryResult.SUCCESS.
In
`@src/main/java/com/pooli/traffic/service/outbox/strategy/SyncPolicyActivationOutboxStrategy.java`:
- Around line 33-38: The current code in SyncPolicyActivationOutboxStrategy
coerces a missing active flag into false via
Boolean.TRUE.equals(payload.getActive()), which can replay an unintended
deactivation; instead validate that payload.getActive() is non-null (treat
missing as an error), surface/throw or log a validation error from
readPayload/SyncPolicyActivationOutboxStrategy when
PolicyActivationOutboxPayload.getActive() is null, and then pass the Boolean
value returned by getActive() directly into
trafficPolicyWriteThroughService.syncPolicyActivationUntracked (via
mapPolicySyncResult) without forcing a true/false coercion so the original
intent is preserved.
In
`@src/main/java/com/pooli/traffic/service/outbox/TrafficPolicyVersionedRedisService.java`:
- Around line 198-205: The loadScriptText method leaks the InputStream from
ClassPathResource.getInputStream(); update loadScriptText to open the
InputStream in a try-with-resources (or explicitly close it) so the stream is
always closed after reading; locate the loadScriptText method and wrap
resource.getInputStream() in a try (InputStream in = resource.getInputStream())
{ return new String(in.readAllBytes(), StandardCharsets.UTF_8); } and keep the
existing IOException handling to rethrow IllegalStateException with the
resourcePath and cause.
In
`@src/main/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportService.java`:
- Around line 33-53: REFILL_IDEMPOTENCY_TTL_SECONDS is too short and allows
duplicate refills; update tryRegisterIdempotency in
TrafficRefillOutboxSupportService to derive the TTL from the outbox replay
window (or remove the fixed TTL and only delete the key when the outbox reaches
terminal state). Replace the static REFILL_IDEMPOTENCY_TTL_SECONDS usage with a
dynamic Duration computed from the configured retry horizon exposed by
TrafficRedisRuntimePolicy (or inject a dedicated OutboxReplayWindow config), and
use that Duration in cacheStringRedisTemplate.opsForValue().setIfAbsent(...).
Ensure the key cleanup strategy is updated accordingly if you choose to remove
the TTL (i.e., delete the idempotency key when the outbox entry becomes
terminal).
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficLinePolicyHydrationService.java`:
- Around line 117-128: The current hydration uses a local wall-clock timestamp
(variable `version` set from System.currentTimeMillis()) in
TrafficLinePolicyHydrationService and then calls syncLineLimitUntracked,
syncImmediateBlockEndUntracked, syncRepeatBlockUntracked, and
syncAppPolicySnapshotUntracked with that value, which can cause stale snapshots
to outrank newer DB writes; replace the wall-clock version with a
source-of-truth monotonic version (for example the snapshot’s version field,
e.g., snapshot.getVersion(), or the shared monotonic generator used by
write-through paths) and use that single derived version for all sync* calls so
ordering is consistent with the canonical data source.
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java`:
- Around line 184-187: TrafficPolicyBootstrapService is writing policy state
with stringOperations.opsForHash().put(policyKey, "value", ...) and
put(policyKey, "version", String.valueOf(bootstrapVersionEpochMillis)) without
CAS, which can let an older snapshot overwrite newer writes; change the
bootstrap write to use the same CAS Lua path used by
TrafficPolicyWriteThroughService (policy_value_cas.lua) so the update only
succeeds if the stored version is older than bootstrapVersionEpochMillis:
construct the same Redis key via trafficRedisKeyFactory.policyKey(policyId),
pass the computed policyValue and bootstrapVersionEpochMillis, and invoke the
existing Lua script execution helper (the same code path used by
TrafficPolicyWriteThroughService) instead of calling
stringOperations.opsForHash().put directly so version semantics are enforced
against snapshot.getIsActive() updates.
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java`:
- Around line 537-540: The method isSuccessEquivalent currently treats
PolicySyncResult.CONNECTION_FAILURE as a success-equivalent, which can cause the
outbox to be marked completed on transient Redis connection drops; update
isSuccessEquivalent (used by TrafficPolicyWriteThroughService) to remove
PolicySyncResult.CONNECTION_FAILURE from the success set so only
PolicySyncResult.SUCCESS and PolicySyncResult.STALE_REJECTED are considered
success-equivalent, and add a brief comment noting that CONNECTION_FAILURE must
remain replayable so executeTrackedAfterCommit() does not permanently drop the
record.
- Around line 386-397: The method executeAfterCommit currently uses
TransactionSynchronizationManager.isSynchronizationActive(), which allows silent
fallback to immediate Redis writes; change it to use
TransactionSynchronizationManager.isActualTransactionActive() and fail fast when
no actual transaction is present (e.g., throw an IllegalStateException with a
clear message) instead of calling redisWriteOperation.run() immediately; keep
the registerSynchronization(new TransactionSynchronization() { public void
afterCommit() { redisWriteOperation.run(); } }) behavior when an actual
transaction is active so Redis writes still occur after commit.
- Around line 543-548: The current nowEpochMillis() wall-clock version is racy
and should be replaced by the DB-monotonic outbox id: after calling
redisOutboxRecordService.createPending(...) read the generated key
(useGeneratedKeys via MyBatis) and use that numeric outbox id as the version
value passed to whatever logic uses nowEpochMillis() (e.g., the CAS/Lua incoming
version compare and any policy version fields). Remove calls to nowEpochMillis()
in TrafficPolicyWriteThroughService and ensure the code writes the outbox record
first, captures the generated id, and then uses that id (or its transformed form
required by your CAS protocol) as the authoritative version; also ensure the
version format matches the design spec (convert if you must to epoch seconds
Asia/Seoul per traffic-outbox-part-c-plan.md).
In `@src/main/resources/lua/traffic/app_policy_snapshot_cas.lua`:
- Around line 14-29: Move the three cjson.decode calls for ARGV[2], ARGV[3], and
ARGV[4] so they run before any mutation (the redis.call('DEL', KEYS[1], KEYS[2],
KEYS[3]) line); parse dataPayload, speedPayload, and whitelistPayload first
using cjson.decode and only after successful decoding perform the DEL and
subsequent HSET/SADD operations (references: cjson.decode, ARGV[2], ARGV[3],
ARGV[4], redis.call('DEL', ...), redis.call('HSET', KEYS[1], ...),
redis.call('HSET', KEYS[2], ...), redis.call('SADD', KEYS[3], ...)).
In `@src/main/resources/lua/traffic/policy_value_cas.lua`:
- Around line 6-14: Add a Redis type check before calling HGET: call
redis.call('TYPE', KEYS[1]) and only perform redis.call('HGET', KEYS[1],
'version') and the version compare when that TYPE result equals 'hash'; if the
TYPE is not 'hash' (e.g. 'string' or 'none') skip the HGET/version logic so the
script falls through to the existing redis.call('HSET', KEYS[1], 'value',
ARGV[2], 'version', ARGV[1]) and thus will not raise WRONGTYPE for legacy String
keys. Ensure you reference the existing symbols currentRaw, redis.call('HGET',
KEYS[1], 'version') and the final redis.call('HSET', KEYS[1], 'value', ARGV[2],
'version', ARGV[1]) when adding the branch.
In `@src/main/resources/lua/traffic/repeat_block_snapshot_cas.lua`:
- Around line 14-19: The script currently calls redis.call('DEL', KEYS[1])
before decoding the incoming payload, risking data loss if cjson.decode(ARGV[2])
fails; change the order in repeat_block_snapshot_cas.lua so you first call
cjson.decode(ARGV[2]) into a local payload, validate it, then perform
redis.call('DEL', KEYS[1]) and the subsequent HSET loop and '__version' HSET;
ensure the code references the same KEYS[1] and ARGV indices as before and only
deletes after successful decode/validation.
---
Outside diff comments:
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java`:
- Around line 169-190: The current pipelined loop in
TrafficPolicyBootstrapService (inside cacheStringRedisTemplate.executePipelined
SessionCallback) does two separate opsForHash.put calls on policyKey for "value"
and "version", which is non-atomic; replace that per-policy pair of HSETs with
an invocation of the same CAS Lua script used by
TrafficPolicyVersionedRedisService.syncVersionedValue so the update compares the
existing version and atomically sets both "value" and "version" only when
allowed. Concretely: remove the two stringOperations.opsForHash().put(...) calls
for each policyKey, build the same Lua script key/arg invocation (use the same
script path/sha/mechanism as syncVersionedValue), pass policyKey and
expectedVersion/bootstrapVersionEpochMillis and desired value ("1"/"0") as args,
and call the script via the RedisTemplate (or the helper method used by
TrafficPolicyVersionedRedisService) so each policy update is performed with the
CAS Lua logic and not two separate HSETs.
---
Nitpick comments:
In
`@src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.java`:
- Around line 59-62: In RedisOutboxRetryScheduler's loop that iterates over
candidates (the block checking candidate.getId() == null ||
candidate.getEventType() == null), add a concise log statement (preferably
logger.warn or logger.debug depending on severity) when skipping an invalid
candidate that includes which field is null and key identifying info (e.g.,
candidate.getId(), candidate.getEventType(), or the candidate object) so you can
diagnose data integrity issues; update the skip branch to log the reason before
continuing.
In
`@src/main/resources/db/migration/V2603150500__create_table_traffic_redis_outbox.sql`:
- Line 8: Add a non-unique index on the uuid column to speed idempotency
lookups: update the migration that creates the outbox table to include an index
on the uuid column (e.g., INDEX idx_traffic_redis_outbox_uuid (uuid)) or add a
separate CREATE INDEX statement for the table; ensure the index name is
descriptive (idx_traffic_redis_outbox_uuid) and use a normal B-tree index unless
uniqueness is required.
In `@src/main/resources/mapper/traffic/TrafficRefillSourceMapper.xml`:
- Around line 41-47: The update id "restoreIndividualRemaining" currently adds
#{restoreAmount} to remaining_data without validation; modify this mapper to
guard against negative or excessively large restores by adding a WHERE clause
condition such as AND #{restoreAmount} > 0 to prevent negative values from
acting as deductions, and optionally enforce an upper bound (e.g., cap with a
max capacity or use a DB-side cap when updating remaining_data) referencing
remaining_data, #{restoreAmount}, and line_id in the change.
In
`@src/test/java/com/pooli/traffic/service/decision/TrafficDefaultQuotaSourceAdapterTest.java`:
- Around line 47-48: Add assertions that redisOutboxRecordService never enqueues
outbox work in the tests covering the non-positive-request branch and the
failed-UPDATE branch: after invoking the code under test, call
verify(redisOutboxRecordService, never()).createPending(Mockito.any(),
Mockito.any(), Mockito.any()) to ensure createPending is not called; apply the
same never() verification to the other indicated test cases so regressions that
create pending outbox rows with actual=0 will fail the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b1b7f0f-0de8-45ef-b00e-1ff121ad48a1
📒 Files selected for processing (50)
docs/traffic-outbox-part-c-plan.mdsrc/main/java/com/pooli/traffic/domain/TrafficDbRefillClaimResult.javasrc/main/java/com/pooli/traffic/domain/outbox/OutboxEventType.javasrc/main/java/com/pooli/traffic/domain/outbox/OutboxRetryResult.javasrc/main/java/com/pooli/traffic/domain/outbox/OutboxStatus.javasrc/main/java/com/pooli/traffic/domain/outbox/RedisOutboxRecord.javasrc/main/java/com/pooli/traffic/domain/outbox/payload/AppPolicyOutboxPayload.javasrc/main/java/com/pooli/traffic/domain/outbox/payload/ImmediateBlockOutboxPayload.javasrc/main/java/com/pooli/traffic/domain/outbox/payload/LineLimitOutboxPayload.javasrc/main/java/com/pooli/traffic/domain/outbox/payload/LineScopedOutboxPayload.javasrc/main/java/com/pooli/traffic/domain/outbox/payload/PolicyActivationOutboxPayload.javasrc/main/java/com/pooli/traffic/domain/outbox/payload/RefillOutboxPayload.javasrc/main/java/com/pooli/traffic/mapper/RedisOutboxMapper.javasrc/main/java/com/pooli/traffic/mapper/TrafficRefillSourceMapper.javasrc/main/java/com/pooli/traffic/service/decision/TrafficDefaultQuotaSourceAdapter.javasrc/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.javasrc/main/java/com/pooli/traffic/service/decision/TrafficQuotaSourcePort.javasrc/main/java/com/pooli/traffic/service/outbox/PolicySyncResult.javasrc/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.javasrc/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.javasrc/main/java/com/pooli/traffic/service/outbox/TrafficPolicyVersionedRedisService.javasrc/main/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportService.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/AbstractPolicyOutboxRetryStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/OutboxEventRetryStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/OutboxRetryStrategyRegistry.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/SyncAppPolicyOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/SyncAppPolicySnapshotOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/SyncImmediateBlockOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/SyncLineLimitOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/SyncPolicyActivationOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/SyncRepeatBlockOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/policy/TrafficLinePolicyHydrationService.javasrc/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.javasrc/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.javasrc/main/java/com/pooli/traffic/service/runtime/TrafficRedisKeyFactory.javasrc/main/resources/db/migration/V2603150500__create_table_traffic_redis_outbox.sqlsrc/main/resources/lua/traffic/app_policy_single_cas.luasrc/main/resources/lua/traffic/app_policy_snapshot_cas.luasrc/main/resources/lua/traffic/deduct_indiv.luasrc/main/resources/lua/traffic/deduct_shared.luasrc/main/resources/lua/traffic/policy_value_cas.luasrc/main/resources/lua/traffic/repeat_block_snapshot_cas.luasrc/main/resources/mapper/traffic/RedisOutboxMapper.xmlsrc/main/resources/mapper/traffic/TrafficRefillSourceMapper.xmlsrc/test/java/com/pooli/traffic/service/TrafficProfileBootTest.javasrc/test/java/com/pooli/traffic/service/decision/TrafficDefaultQuotaSourceAdapterTest.javasrc/test/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterServiceTest.javasrc/test/java/com/pooli/traffic/service/policy/TrafficLinePolicyHydrationServiceTest.javasrc/test/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughServiceTest.java
Comment on lines
+11
to
+18
| protected OutboxRetryResult mapPolicySyncResult(PolicySyncResult syncResult) { | ||
| if (syncResult == PolicySyncResult.SUCCESS | ||
| || syncResult == PolicySyncResult.STALE_REJECTED | ||
| || syncResult == PolicySyncResult.CONNECTION_FAILURE) { | ||
| return OutboxRetryResult.SUCCESS; | ||
| } | ||
| return OutboxRetryResult.FAIL; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how OutboxRetryResult values are used to transition outbox states
rg -n "OutboxRetryResult" --type java -C 3Repository: pooli-dev/pooli-be
Length of output: 25628
🏁 Script executed:
#!/bin/bash
# Find PolicySyncResult enum definition
rg -n "enum PolicySyncResult\|class PolicySyncResult" --type java -A 10Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for PolicySyncResult definition more broadly
rg -n "PolicySyncResult" --type java | head -20Repository: pooli-dev/pooli-be
Length of output: 3192
🏁 Script executed:
#!/bin/bash
# Find PolicySyncResult file
fd -n "PolicySyncResult.java" --type fRepository: pooli-dev/pooli-be
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Find PolicySyncResult file
fd "PolicySyncResult.java" --type fRepository: pooli-dev/pooli-be
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Read PolicySyncResult enum
cat -n src/main/java/com/pooli/traffic/service/outbox/PolicySyncResult.javaRepository: pooli-dev/pooli-be
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Look at TrafficPolicyVersionedRedisService to understand policy sync operations
cat -n src/main/java/com/pooli/traffic/service/outbox/TrafficPolicyVersionedRedisService.javaRepository: pooli-dev/pooli-be
Length of output: 9927
🏁 Script executed:
#!/bin/bash
# Look at full RedisOutboxRetryScheduler logic to understand FAIL handling
cat -n src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.javaRepository: pooli-dev/pooli-be
Length of output: 5548
CONNECTION_FAILURE should be retried, not marked as completed.
The issue is correct. PolicySyncResult.CONNECTION_FAILURE indicates the Redis write never executed due to a network error—it's a transient failure that should be retried. Mapping it to OutboxRetryResult.SUCCESS marks the outbox record as completed permanently, preventing the scheduler from retrying when Redis recovers.
For comparison, RefillOutboxStrategy handles connection failures by returning SUCCESS but only after cleanup (line 52: restoreClaimedAmount). The policy sync strategies lack this recovery logic and map CONNECTION_FAILURE directly to success with no action.
This violates the outbox pattern: unwritten data becomes permanent gaps. CONNECTION_FAILURE should map to FAIL instead, allowing the retry scheduler to attempt the write again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/pooli/traffic/service/outbox/strategy/AbstractPolicyOutboxRetryStrategy.java`
around lines 11 - 18, The mapPolicySyncResult method in
AbstractPolicyOutboxRetryStrategy incorrectly maps
PolicySyncResult.CONNECTION_FAILURE to OutboxRetryResult.SUCCESS causing
transient Redis/network errors to be treated as completed; update
mapPolicySyncResult so that PolicySyncResult.CONNECTION_FAILURE returns
OutboxRetryResult.FAIL (like other transient failures) to allow the retry
scheduler to retry the outbox record, referencing the method name
mapPolicySyncResult and the enums PolicySyncResult and OutboxRetryResult; ensure
no other cleanup is assumed (unlike RefillOutboxStrategy.restoreClaimedAmount),
just change the mapping to FAIL.
✅ Confirmed as addressed by @Iwantcod
Member
Author
There was a problem hiding this comment.
hydrate 로직이 동작하고 있으니, redis와 connection_failure가 된 상황에서는 write-through 동기화를 할 필요가 없다고 판단했습니다. redis 서버가 재가동되면 hydrate 로직에 따라 동기화될 것입니다.
There was a problem hiding this comment.
@Iwantcod, 설명 감사합니다. hydrate 로직이 Redis 재가동 시 전체 상태를 DB 기준으로 재동기화하므로, CONNECTION_FAILURE 시 outbox 재시도 없이 SUCCESS로 처리하는 설계가 타당합니다. 정책 동기화는 최신 snapshot 기반이라 중복 적용 위험도 없고, outbox 큐가 Redis 장애 중 불필요하게 쌓이는 것도 방지됩니다. 이전 리뷰 의견을 철회합니다.
✏️ Learnings added
Learnt from: Iwantcod
Repo: pooli-dev/pooli-be PR: 234
File: src/main/java/com/pooli/traffic/service/outbox/strategy/AbstractPolicyOutboxRetryStrategy.java:11-18
Timestamp: 2026-03-15T05:33:38.845Z
Learning: In the pooli-be project (Java/Spring), when a Redis CONNECTION_FAILURE occurs during policy outbox retry strategies (AbstractPolicyOutboxRetryStrategy and its subclasses), the result is intentionally mapped to OutboxRetryResult.SUCCESS (not FAIL/retry). The rationale is that a separate hydrate process (TrafficLinePolicyHydrationService) will re-sync all policy state from DB to Redis when the Redis server restarts, making outbox-level retries for connection failures unnecessary and redundant for policy sync events.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Iwantcod
Repo: pooli-dev/pooli-be PR: 234
File: src/main/java/com/pooli/traffic/service/outbox/strategy/SyncAppPolicySnapshotOutboxStrategy.java:38-45
Timestamp: 2026-03-15T05:24:20.077Z
Learning: In the pooli-be project (Java/Spring), policies are always treated as the "latest snapshot". Outbox retry strategies for policy sync (e.g., SyncAppPolicySnapshotOutboxStrategy) intentionally re-fetch the current state from the DB rather than storing the snapshot in the outbox payload. The outbox record's version is used only for CAS ordering in Redis Lua scripts, not to pin the payload data.
Comment on lines
+543
to
+548
| /** | ||
| * 정책 버전 비교에 사용하는 Epoch Millis를 생성합니다. | ||
| */ | ||
| private long nowEpochMillis() { | ||
| return System.currentTimeMillis(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "TrafficPolicyWriteThroughService.java" -type fRepository: pooli-dev/pooli-be
Length of output: 148
🏁 Script executed:
# Get an overview of the file structure
git ls-files | grep -E "(traffic|policy)" | head -20Repository: pooli-dev/pooli-be
Length of output: 1429
🏁 Script executed:
# Look for the nowEpochMillis method and its usage
rg "nowEpochMillis" --type java -B 3 -A 5Repository: pooli-dev/pooli-be
Length of output: 8927
🏁 Script executed:
# Read the TrafficPolicyWriteThroughService file
wc -l src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.javaRepository: pooli-dev/pooli-be
Length of output: 150
🏁 Script executed:
# Get the full content of the file
cat -n src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.javaRepository: pooli-dev/pooli-be
Length of output: 24683
🏁 Script executed:
# Search for any CAS, compare-and-swap, or version comparison logic
rg "CAS|compare.*swap|version.*compar|optimistic" --type java -iRepository: pooli-dev/pooli-be
Length of output: 14918
🏁 Script executed:
# Look for DB schema or monotonic version sources
rg "version|sequence|sequence_id|revision" --type java -i | grep -E "(database|schema|column|@Column|id)" | head -20Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Look at the TrafficPolicyVersionedRedisService to understand the CAS logic
find . -name "TrafficPolicyVersionedRedisService.java" -type fRepository: pooli-dev/pooli-be
Length of output: 150
🏁 Script executed:
# Check the CAS Lua scripts to understand version comparison
find . -path "*/lua/traffic/*cas*" -type fRepository: pooli-dev/pooli-be
Length of output: 298
🏁 Script executed:
# Search for any database-level version or sequence generation
rg "version|sequence|@GeneratedValue|@SequenceGenerator" --type java | grep -i "policy\|traffic" | head -30Repository: pooli-dev/pooli-be
Length of output: 4319
🏁 Script executed:
# Examine one of the CAS Lua scripts to understand version comparison
cat -n ./src/main/resources/lua/traffic/policy_value_cas.luaRepository: pooli-dev/pooli-be
Length of output: 496
🏁 Script executed:
# Look at the TrafficPolicyVersionedRedisService implementation
cat -n ./src/main/java/com/pooli/traffic/service/outbox/TrafficPolicyVersionedRedisService.javaRepository: pooli-dev/pooli-be
Length of output: 9927
🏁 Script executed:
# Check the test to see what the issue with versions might be
cat -n ./src/test/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughServiceTest.java | head -150Repository: pooli-dev/pooli-be
Length of output: 7910
🏁 Script executed:
# Check if there's any existing transaction versioning or sequence ID mechanism
rg "transaction|commit|@Version|OptimisticLocking|update.*timestamp" --type java | grep -i "policy\|traffic" | head -20Repository: pooli-dev/pooli-be
Length of output: 1880
🏁 Script executed:
# Look for any database schema or entity definitions for policy
find . -name "*Policy*.java" -type java | grep entityRepository: pooli-dev/pooli-be
Length of output: 96
🏁 Script executed:
# Check the outbox event/payload structure to understand the full flow
cat -n ./src/main/java/com/pooli/traffic/domain/outbox/OutboxEventType.javaRepository: pooli-dev/pooli-be
Length of output: 481
🏁 Script executed:
# Check if there's a database version column or auto-increment on policy tables
find . -name "*.java" -type f | xargs grep -l "class.*Policy.*Entity\|@Entity.*Policy" | head -5Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Look at the policy entity definitions
find . -path "*/domain/entity/*Policy*.java" -type fRepository: pooli-dev/pooli-be
Length of output: 380
🏁 Script executed:
# Check one of the policy entities to see if there's an existing version field
cat -n src/main/java/com/pooli/policy/domain/entity/Policy.java 2>/dev/null | head -100Repository: pooli-dev/pooli-be
Length of output: 794
🏁 Script executed:
# Search for any persistence layer audit columns or version tracking
rg "@Version|@CreatedDate|@LastModifiedDate|@UpdateTimestamp" --type java | head -20Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Check if there's any transaction timestamp or sequence mechanism in the database layer
rg "Sequence|AUTO_INCREMENT|GENERATED.*ALWAYS|transaction.*id" --type java | head -20Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Look at how other parts of the codebase handle versioning
find . -name "*.java" -type f | xargs grep -l "version.*sequence\|sequence.*version" | head -5Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Check if there's any discussion or documentation about version strategy
find . -name "*.md" -type f | xargs grep -l "version\|CAS\|optimistic" 2>/dev/nullRepository: pooli-dev/pooli-be
Length of output: 139
🏁 Script executed:
# Examine the test more to understand what behavior is expected
cat -n ./src/test/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughServiceTest.java | tail -50Repository: pooli-dev/pooli-be
Length of output: 2918
🏁 Script executed:
# Check the outbox design document
cat -n ./docs/traffic-outbox-part-c-plan.md | head -100Repository: pooli-dev/pooli-be
Length of output: 3548
🏁 Script executed:
# Check the collaboration guide
cat -n ./docs/traffic-collaboration-guide-v1.md | grep -i "version\|sequence\|monotonic" -A 5 -B 5Repository: pooli-dev/pooli-be
Length of output: 869
🏁 Script executed:
# Look for any database outbox table schema to see if there's a sequence
find . -name "*Outbox*.java" -type f | xargs grep -l "Entity\|@Table" | head -3Repository: pooli-dev/pooli-be
Length of output: 160
🏁 Script executed:
# Look for database schema files or migration scripts
find . -name "*.sql" -o -name "*migration*" -o -name "*schema*" | head -20Repository: pooli-dev/pooli-be
Length of output: 1590
🏁 Script executed:
# Check if there's a sequence or auto-increment mechanism in the outbox table
rg "CREATE TABLE.*outbox\|AUTO_INCREMENT\|IDENTITY" --type sql -iRepository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Look at RedisOutboxRecordService to see what data is available
cat -n ./src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.java | head -100Repository: pooli-dev/pooli-be
Length of output: 3731
🏁 Script executed:
# Check the outbox table schema to see the AUTO_INCREMENT structure
cat -n ./src/main/resources/db/migration/V1__init.sql | grep -A 20 "redis_outbox\|outbox"Repository: pooli-dev/pooli-be
Length of output: 44
🏁 Script executed:
# Check RedisOutboxRecord to see if ID is captured after insert
cat -n ./src/main/java/com/pooli/traffic/domain/outbox/RedisOutboxRecord.javaRepository: pooli-dev/pooli-be
Length of output: 950
🏁 Script executed:
# Look at how createPending returns the ID and could be used as version
cat -n ./src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.java | grep -A 20 "createPending"Repository: pooli-dev/pooli-be
Length of output: 937
🏁 Script executed:
# Check the mapper to see how the ID is populated after insert
find . -name "RedisOutboxMapper.java" -o -name "*RedisOutboxMapper.xml" | head -5Repository: pooli-dev/pooli-be
Length of output: 183
🏁 Script executed:
# Look at the actual flow - when nowEpochMillis is called relative to outbox insert
grep -n "createPending\|nowEpochMillis\|executeTrackedAfterCommit" ./src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java | head -20Repository: pooli-dev/pooli-be
Length of output: 827
🏁 Script executed:
# Check the migration file for the actual outbox table structure
find . -name "*.sql" -exec grep -l "redis_outbox\|REDIS_OUTBOX" {} \;Repository: pooli-dev/pooli-be
Length of output: 146
🏁 Script executed:
# Check the outbox table schema
cat -n ./src/main/resources/db/migration/V2603150500__create_table_traffic_redis_outbox.sqlRepository: pooli-dev/pooli-be
Length of output: 937
🏁 Script executed:
# Check the mapper implementation
cat -n ./src/main/java/com/pooli/traffic/mapper/RedisOutboxMapper.javaRepository: pooli-dev/pooli-be
Length of output: 1751
🏁 Script executed:
# Check the mapper XML for the insert statement
cat -n ./src/main/resources/mapper/traffic/RedisOutboxMapper.xmlRepository: pooli-dev/pooli-be
Length of output: 3403
Use the outbox record's monotonic ID as the version source instead of wall-clock millis.
nowEpochMillis() captures wall-clock time before the outbox record is persisted. This has critical race condition issues: multiple transactions in the same millisecond receive identical versions, causing the later-committed state to be rejected as "stale" by the CAS layer (Lua script line 9: incoming <= current). Additionally, clock skew across nodes or backward clock adjustment breaks version ordering.
The outbox table's AUTO_INCREMENT id is immediately available after redisOutboxRecordService.createPending() via MyBatis useGeneratedKeys. This provides a database-monotonic, write-ordered version that reflects actual commit sequence for each key, eliminating both concurrency collisions and clock-related vulnerabilities.
Also note: Design spec (traffic-outbox-part-c-plan.md) specifies epoch seconds in Asia/Seoul timezone; current implementation uses milliseconds in JVM default timezone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java`
around lines 543 - 548, The current nowEpochMillis() wall-clock version is racy
and should be replaced by the DB-monotonic outbox id: after calling
redisOutboxRecordService.createPending(...) read the generated key
(useGeneratedKeys via MyBatis) and use that numeric outbox id as the version
value passed to whatever logic uses nowEpochMillis() (e.g., the CAS/Lua incoming
version compare and any policy version fields). Remove calls to nowEpochMillis()
in TrafficPolicyWriteThroughService and ensure the code writes the outbox record
first, captures the generated id, and then uses that id (or its transformed form
required by your CAS protocol) as the authoritative version; also ensure the
version format matches the design spec (convert if you must to epoch seconds
Asia/Seoul per traffic-outbox-part-c-plan.md).
Comment on lines
+14
to
+29
| redis.call('DEL', KEYS[1], KEYS[2], KEYS[3]) | ||
|
|
||
| local dataPayload = cjson.decode(ARGV[2]) | ||
| for field, value in pairs(dataPayload) do | ||
| redis.call('HSET', KEYS[1], field, tostring(value)) | ||
| end | ||
| redis.call('HSET', KEYS[1], '__version', ARGV[1]) | ||
|
|
||
| local speedPayload = cjson.decode(ARGV[3]) | ||
| for field, value in pairs(speedPayload) do | ||
| redis.call('HSET', KEYS[2], field, tostring(value)) | ||
| end | ||
|
|
||
| local whitelistPayload = cjson.decode(ARGV[4]) | ||
| for _, member in ipairs(whitelistPayload) do | ||
| redis.call('SADD', KEYS[3], tostring(member)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Redis Lua scripts, if cjson.decode raises after a DEL/HSET sequence, are the earlier writes rolled back or can partial writes remain?
💡 Result:
Partial writes can remain.
Redis Lua scripts execute atomically only in the sense that no other client command interleaves while the script runs, but Redis does not roll back writes that already happened if the script later errors (e.g., cjson.decode raising). So if your script does DEL / HSET and then cjson.decode fails, the earlier DEL/HSET effects are still committed/visible. [1][2]
Redis’s own docs also allude to this by noting that once a script has performed writes, it can’t be safely killed because that could leave “half-written” information. [3]
Sources: [1] [2] [3]
Parse all payloads before clearing the live snapshot.
Lines 14-29 delete KEYS[1], KEYS[2], KEYS[3] before decoding ARGV[2], ARGV[3], ARGV[4]. In Redis Lua scripts, writes are not rolled back on error—if cjson.decode raises after the DEL executes, the prior snapshot is gone but the new data was never written, resulting in data loss. Move all three cjson.decode calls before the DEL so that any parse failure prevents state mutation.
🛠️ Safer ordering
local currentRaw = redis.call('HGET', KEYS[1], '__version')
if currentRaw then
local current = tonumber(currentRaw)
if current and incoming <= current then
return 0
end
end
-redis.call('DEL', KEYS[1], KEYS[2], KEYS[3])
-
local dataPayload = cjson.decode(ARGV[2])
+local speedPayload = cjson.decode(ARGV[3])
+local whitelistPayload = cjson.decode(ARGV[4])
+
+redis.call('DEL', KEYS[1], KEYS[2], KEYS[3])
for field, value in pairs(dataPayload) do
redis.call('HSET', KEYS[1], field, tostring(value))
end
redis.call('HSET', KEYS[1], '__version', ARGV[1])
-local speedPayload = cjson.decode(ARGV[3])
for field, value in pairs(speedPayload) do
redis.call('HSET', KEYS[2], field, tostring(value))
end
-local whitelistPayload = cjson.decode(ARGV[4])
for _, member in ipairs(whitelistPayload) do
redis.call('SADD', KEYS[3], tostring(member))
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/lua/traffic/app_policy_snapshot_cas.lua` around lines 14 -
29, Move the three cjson.decode calls for ARGV[2], ARGV[3], and ARGV[4] so they
run before any mutation (the redis.call('DEL', KEYS[1], KEYS[2], KEYS[3]) line);
parse dataPayload, speedPayload, and whitelistPayload first using cjson.decode
and only after successful decoding perform the DEL and subsequent HSET/SADD
operations (references: cjson.decode, ARGV[2], ARGV[3], ARGV[4],
redis.call('DEL', ...), redis.call('HSET', KEYS[1], ...), redis.call('HSET',
KEYS[2], ...), redis.call('SADD', KEYS[3], ...)).
Comment on lines
+6
to
+14
| local currentRaw = redis.call('HGET', KEYS[1], 'version') | ||
| if currentRaw then | ||
| local current = tonumber(currentRaw) | ||
| if current and incoming <= current then | ||
| return 0 | ||
| end | ||
| end | ||
|
|
||
| redis.call('HSET', KEYS[1], 'value', ARGV[2], 'version', ARGV[1]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Redis, what happens if a Lua script calls HGET on a key that currently stores a String value instead of a Hash?
💡 Result:
If a Lua script executes redis.call("HGET", key, field) and key currently holds a String (not a Hash), Redis returns a WRONGTYPE error (“Operation against a key holding the wrong kind of value”). The error raised by redis.call() aborts the script and is returned to the client as an EVAL/script execution error. [1], [2]
If you use redis.pcall("HGET", key, field) instead, the script won’t abort; it will receive a Lua table like {err="WRONGTYPE ..."} that you can handle in the script. [1]
Sources: [1] [2]
🏁 Script executed:
cat -n src/main/resources/lua/traffic/policy_value_cas.luaRepository: pooli-dev/pooli-be
Length of output: 496
Add type checking before HGET to handle legacy String keys.
Line 6 calls HGET on KEYS[1] without confirming it's a Hash. If the key still holds a String value from the old schema, the script will error with WRONGTYPE and abort before the migration can complete, blocking outbox replay and leaving the system in an inconsistent state during rollout.
Migration approach
local incoming = tonumber(ARGV[1])
if not incoming then
return -1
end
+local keyType = redis.call('TYPE', KEYS[1]).ok
+if keyType == 'string' then
+ redis.call('DEL', KEYS[1])
+end
+
local currentRaw = redis.call('HGET', KEYS[1], 'version')
if currentRaw then
local current = tonumber(currentRaw)
if current and incoming <= current then
return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/lua/traffic/policy_value_cas.lua` around lines 6 - 14, Add
a Redis type check before calling HGET: call redis.call('TYPE', KEYS[1]) and
only perform redis.call('HGET', KEYS[1], 'version') and the version compare when
that TYPE result equals 'hash'; if the TYPE is not 'hash' (e.g. 'string' or
'none') skip the HGET/version logic so the script falls through to the existing
redis.call('HSET', KEYS[1], 'value', ARGV[2], 'version', ARGV[1]) and thus will
not raise WRONGTYPE for legacy String keys. Ensure you reference the existing
symbols currentRaw, redis.call('HGET', KEYS[1], 'version') and the final
redis.call('HSET', KEYS[1], 'value', ARGV[2], 'version', ARGV[1]) when adding
the branch.
Comment on lines
+14
to
+19
| redis.call('DEL', KEYS[1]) | ||
| local payload = cjson.decode(ARGV[2]) | ||
| for field, value in pairs(payload) do | ||
| redis.call('HSET', KEYS[1], field, tostring(value)) | ||
| end | ||
| redis.call('HSET', KEYS[1], '__version', ARGV[1]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Redis Lua scripts, if cjson.decode raises after a DEL command, are the earlier writes rolled back?
💡 Result:
No. In Redis Lua (EVAL/EVALSHA), scripts run atomically w.r.t. other clients (no interleaving), but there is no rollback if the script errors partway through—any commands already executed (e.g., a DEL) remain applied even if cjson.decode() (or any other Lua/runtime error) aborts the script afterward. (stackoverflow.com)
The “rolled back” behavior you may see is specific to the Lua debugger session, not normal script execution. (redis.io)
Citations:
- 1: https://stackoverflow.com/questions/61389040/is-redis-eval-really-atomic-and-crash-safe
- 2: https://redis.io/docs/latest/develop/programmability/lua-debugging/?utm_source=openai
Reorder to decode the payload before deleting the current snapshot.
In Redis Lua scripts, if cjson.decode(ARGV[2]) fails after the DEL command executes, there is no rollback—the deletion remains applied. A single malformed outbox payload can permanently erase the last good repeat-block schedule.
Decode first, then delete:
Safer ordering
local currentRaw = redis.call('HGET', KEYS[1], '__version')
if currentRaw then
local current = tonumber(currentRaw)
if current and incoming <= current then
return 0
end
end
-redis.call('DEL', KEYS[1])
local payload = cjson.decode(ARGV[2])
+redis.call('DEL', KEYS[1])
for field, value in pairs(payload) do
redis.call('HSET', KEYS[1], field, tostring(value))
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| redis.call('DEL', KEYS[1]) | |
| local payload = cjson.decode(ARGV[2]) | |
| for field, value in pairs(payload) do | |
| redis.call('HSET', KEYS[1], field, tostring(value)) | |
| end | |
| redis.call('HSET', KEYS[1], '__version', ARGV[1]) | |
| local payload = cjson.decode(ARGV[2]) | |
| redis.call('DEL', KEYS[1]) | |
| for field, value in pairs(payload) do | |
| redis.call('HSET', KEYS[1], field, tostring(value)) | |
| end | |
| redis.call('HSET', KEYS[1], '__version', ARGV[1]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/lua/traffic/repeat_block_snapshot_cas.lua` around lines 14
- 19, The script currently calls redis.call('DEL', KEYS[1]) before decoding the
incoming payload, risking data loss if cjson.decode(ARGV[2]) fails; change the
order in repeat_block_snapshot_cas.lua so you first call cjson.decode(ARGV[2])
into a local payload, validate it, then perform redis.call('DEL', KEYS[1]) and
the subsequent HSET loop and '__version' HSET; ensure the code references the
same KEYS[1] and ARGV indices as before and only deletes after successful
decode/validation.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java (2)
386-397:⚠️ Potential issue | 🟠 MajorRequire an actual transaction for tracked write-through.
Lines 387-397 fall back to immediate Redis execution when no transaction is active. That breaks the outbox guarantee that Redis only observes the change after the DB commit; non-transactional callers already have the
Untrackedmethods. Fail fast unless an actual transaction is active.Verify Spring's contract here against the javadocs.
Spring TransactionSynchronizationManager isSynchronizationActive vs isActualTransactionActive afterCommit callback behavior🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java` around lines 386 - 397, The helper executeAfterCommit should not fall back to immediate Redis writes; require an actual DB transaction and fail fast for tracked write-through callers: check TransactionSynchronizationManager.isActualTransactionActive() (not just isSynchronizationActive()), and if it returns false throw an IllegalStateException with a clear message that tracked methods require an active transaction (caller should use Untracked variants); if true, register the TransactionSynchronization and call redisWriteOperation.run() in afterCommit as before. Ensure you reference executeAfterCommit, TransactionSynchronizationManager, and the Untracked methods in the error message or javadoc for clarity.
545-550:⚠️ Potential issue | 🔴 CriticalUse the outbox id, not wall-clock millis, as the CAS version.
System.currentTimeMillis()is not monotonic enough for anincomingVersion > currentVersionCAS rule: two writes in the same millisecond—or skewed clocks across nodes—can make the later commit look stale. The generated outbox id is already a monotonic per-write value and should be the version source instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java` around lines 545 - 550, The CAS version currently produced by nowEpochMillis() is unsafe; replace use of System.currentTimeMillis() as the incomingVersion/currentVersion comparator with the monotonic per-write outbox id generated for each write (use the generated outbox id value available in the write path—e.g., the variable/field representing the new outbox id such as outboxId or outbox.getId()) so that incomingVersion is set from that outbox id instead of nowEpochMillis(); update any callers of nowEpochMillis()/the version assignment in TrafficPolicyWriteThroughService to use the outbox id, adjust types/signatures if necessary, and remove or deprecate nowEpochMillis() after migrating all uses.
🧹 Nitpick comments (3)
src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.java (1)
56-76: Consider filtering out records that failed to transition to PROCESSING.The method returns all selected candidates, including those where
markProcessingreturned 0 (e.g., due to concurrent REVERT). While the retry strategy should handle this gracefully, filtering these records here would prevent unnecessary processing attempts.♻️ Optional: Return only successfully transitioned records
`@Transactional` public List<RedisOutboxRecord> lockRetryCandidatesAndMarkProcessing( int limit, int pendingDelaySeconds, int processingStuckSeconds ) { List<RedisOutboxRecord> candidates = redisOutboxMapper.selectRetryCandidatesForUpdate( limit, pendingDelaySeconds, processingStuckSeconds ); + List<RedisOutboxRecord> processed = new ArrayList<>(); for (RedisOutboxRecord candidate : candidates) { if (candidate.getId() == null) { continue; } - redisOutboxMapper.markProcessing(candidate.getId()); + int updated = redisOutboxMapper.markProcessing(candidate.getId()); + if (updated > 0) { + processed.add(candidate); + } } - return candidates; + return processed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.java` around lines 56 - 76, lockRetryCandidatesAndMarkProcessing currently returns all candidates even if redisOutboxMapper.markProcessing(...) failed (returned 0); change the method to collect and return only records that were successfully transitioned to PROCESSING by calling markProcessing(candidate.getId()) and checking its return value, e.g., call selectRetryCandidatesForUpdate(...), iterate candidates, call int updated = redisOutboxMapper.markProcessing(candidate.getId()), and add the candidate to the result list only when updated > 0 (skip otherwise) so the returned List<RedisOutboxRecord> contains only successfully marked records.src/main/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportService.java (1)
190-234: Exception classification relies on heuristics.The string-based detection (
className.contains("connection"),message.contains("timeout")) is inherently fragile but provides reasonable coverage for edge cases not captured by specific exception types. Consider logging unclassified exceptions during development to refine patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportService.java` around lines 190 - 234, Exception classification in isConnectionFailure and isTimeoutFailure relies on fragile string heuristics; improve reliability by adding structured logging for unclassified exceptions and expanding checks. Update isConnectionFailure and isTimeoutFailure to log (at debug or warn) the exception class and message when no classifier matches, include known concrete exception types beyond RedisConnectionFailureException if available, and centralize the string checks into helper methods (e.g., containsConnectionKeyword, containsTimeoutKeyword) to make patterns easier to refine; keep walking the cause chain (current variable) but ensure logs include the top-level and root causes to aid pattern tuning during development.src/test/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterServiceTest.java (1)
183-238: Add regression coverage for the new refill edge paths.These updates cover the happy path, but there is still no direct test for lock loss after
writeDbEmptyFlag()or forclearIdempotency()throwing after a successful apply. Both branches now affect user-visible result/state consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterServiceTest.java` around lines 183 - 238, Add two unit tests to TrafficHydrateRefillAdapterServiceTest that exercise the new edge paths: (1) “lock loss after writeDbEmptyFlag” — reuse the setup from refillGateOkThenRetrySuccess but mock trafficLuaScriptInfraService.executeLockRelease (or executeLockHeartbeat) to simulate losing the lock after trafficQuotaCacheService.writeDbEmptyFlag(...) (e.g., return false or false then false) and assert executeIndividualWithRecovery(payload, ...) still returns the expected TrafficLuaExecutionResult and that writeDbEmptyFlag was called and lock-release behavior is handled (verify executeLockRelease/heartbeat interactions); (2) “clearIdempotency throws after successful apply” — reuse the happy-path setup but after mocking applyRefillWithIdempotency to succeed, stub trafficQuotaCacheService.clearIdempotency(...) to throw an exception and assert executeIndividualWithRecovery(...) still returns the correct status/answer and that applyRefillWithIdempotency was called (verify exception from clearIdempotency is swallowed/handled and lock release is invoked). Use the existing method names executeIndividualWithRecovery, writeDbEmptyFlag, applyRefillWithIdempotency, clearIdempotency, executeLockRelease, and executeLockHeartbeat to locate and modify the mocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.java`:
- Around line 294-297: The cached "is_empty" flag is being written via
trafficQuotaCacheService.writeDbEmptyFlag using the post-claim state before the
compensation (lock-loss) branch, which can leave Redis stale if the code later
compensates the DB balance (e.g., the branch that calls
markOutboxSuccessIfPresent and restores actualRefillAmount). To fix, move the
trafficQuotaCacheService.writeDbEmptyFlag call so it executes after the
compensation/rollback branch completes (or explicitly call
trafficQuotaCacheService.writeDbEmptyFlag(...) again to reset the flag if
compensation occurs), ensuring the cached empty flag reflects the final DB
balance; update the code paths around the compensation logic (the branch that
restores balance/outbox handling) to reset the cache accordingly.
- Around line 354-355: The current try/catch scopes both the successful-apply
path and post-success cleanup so that an exception from
trafficRefillOutboxSupportService.clearIdempotency(outboxRefillUuid) can revert
a successful refill (marked by markOutboxSuccessIfPresent(outboxRecordId)) back
to retriedResult/NO_BALANCE; refactor by narrowing the try/catch to only cover
the apply/pre-success logic (where retriedResult should be returned on failure)
and move clearIdempotency(outboxRefillUuid) into a separate post-success block
that catches and logs any exceptions (referencing markOutboxSuccessIfPresent,
clearIdempotency, outboxRecordId, outboxRefillUuid, retriedResult) so cleanup
failures are logged and swallowed and do not change the already-determined
success result or rethrow into the original catch path.
---
Duplicate comments:
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.java`:
- Around line 386-397: The helper executeAfterCommit should not fall back to
immediate Redis writes; require an actual DB transaction and fail fast for
tracked write-through callers: check
TransactionSynchronizationManager.isActualTransactionActive() (not just
isSynchronizationActive()), and if it returns false throw an
IllegalStateException with a clear message that tracked methods require an
active transaction (caller should use Untracked variants); if true, register the
TransactionSynchronization and call redisWriteOperation.run() in afterCommit as
before. Ensure you reference executeAfterCommit,
TransactionSynchronizationManager, and the Untracked methods in the error
message or javadoc for clarity.
- Around line 545-550: The CAS version currently produced by nowEpochMillis() is
unsafe; replace use of System.currentTimeMillis() as the
incomingVersion/currentVersion comparator with the monotonic per-write outbox id
generated for each write (use the generated outbox id value available in the
write path—e.g., the variable/field representing the new outbox id such as
outboxId or outbox.getId()) so that incomingVersion is set from that outbox id
instead of nowEpochMillis(); update any callers of nowEpochMillis()/the version
assignment in TrafficPolicyWriteThroughService to use the outbox id, adjust
types/signatures if necessary, and remove or deprecate nowEpochMillis() after
migrating all uses.
---
Nitpick comments:
In
`@src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.java`:
- Around line 56-76: lockRetryCandidatesAndMarkProcessing currently returns all
candidates even if redisOutboxMapper.markProcessing(...) failed (returned 0);
change the method to collect and return only records that were successfully
transitioned to PROCESSING by calling markProcessing(candidate.getId()) and
checking its return value, e.g., call selectRetryCandidatesForUpdate(...),
iterate candidates, call int updated =
redisOutboxMapper.markProcessing(candidate.getId()), and add the candidate to
the result list only when updated > 0 (skip otherwise) so the returned
List<RedisOutboxRecord> contains only successfully marked records.
In
`@src/main/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportService.java`:
- Around line 190-234: Exception classification in isConnectionFailure and
isTimeoutFailure relies on fragile string heuristics; improve reliability by
adding structured logging for unclassified exceptions and expanding checks.
Update isConnectionFailure and isTimeoutFailure to log (at debug or warn) the
exception class and message when no classifier matches, include known concrete
exception types beyond RedisConnectionFailureException if available, and
centralize the string checks into helper methods (e.g.,
containsConnectionKeyword, containsTimeoutKeyword) to make patterns easier to
refine; keep walking the cause chain (current variable) but ensure logs include
the top-level and root causes to aid pattern tuning during development.
In
`@src/test/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterServiceTest.java`:
- Around line 183-238: Add two unit tests to
TrafficHydrateRefillAdapterServiceTest that exercise the new edge paths: (1)
“lock loss after writeDbEmptyFlag” — reuse the setup from
refillGateOkThenRetrySuccess but mock
trafficLuaScriptInfraService.executeLockRelease (or executeLockHeartbeat) to
simulate losing the lock after trafficQuotaCacheService.writeDbEmptyFlag(...)
(e.g., return false or false then false) and assert
executeIndividualWithRecovery(payload, ...) still returns the expected
TrafficLuaExecutionResult and that writeDbEmptyFlag was called and lock-release
behavior is handled (verify executeLockRelease/heartbeat interactions); (2)
“clearIdempotency throws after successful apply” — reuse the happy-path setup
but after mocking applyRefillWithIdempotency to succeed, stub
trafficQuotaCacheService.clearIdempotency(...) to throw an exception and assert
executeIndividualWithRecovery(...) still returns the correct status/answer and
that applyRefillWithIdempotency was called (verify exception from
clearIdempotency is swallowed/handled and lock release is invoked). Use the
existing method names executeIndividualWithRecovery, writeDbEmptyFlag,
applyRefillWithIdempotency, clearIdempotency, executeLockRelease, and
executeLockHeartbeat to locate and modify the mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d59a0c83-a32c-4faa-9409-9f4b88362aeb
📒 Files selected for processing (18)
src/main/java/com/pooli/traffic/domain/outbox/OutboxStatus.javasrc/main/java/com/pooli/traffic/mapper/RedisOutboxMapper.javasrc/main/java/com/pooli/traffic/service/decision/TrafficDefaultQuotaSourceAdapter.javasrc/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.javasrc/main/java/com/pooli/traffic/service/decision/TrafficQuotaSourcePort.javasrc/main/java/com/pooli/traffic/service/outbox/RedisOutboxRecordService.javasrc/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.javasrc/main/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportService.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/AbstractPolicyOutboxRetryStrategy.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughService.javasrc/main/java/com/pooli/traffic/service/runtime/TrafficQuotaCacheService.javasrc/main/resources/lua/traffic/refill_apply_with_idempotency.luasrc/main/resources/mapper/traffic/RedisOutboxMapper.xmlsrc/test/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterServiceTest.javasrc/test/java/com/pooli/traffic/service/outbox/TrafficRefillOutboxSupportServiceTest.javasrc/test/java/com/pooli/traffic/service/policy/TrafficPolicyWriteThroughServiceTest.javasrc/test/java/com/pooli/traffic/service/runtime/TrafficQuotaCacheServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.java
- src/main/java/com/pooli/traffic/service/outbox/RedisOutboxRetryScheduler.java
- src/main/java/com/pooli/traffic/service/outbox/strategy/AbstractPolicyOutboxRetryStrategy.java
Comment on lines
+354
to
+355
| markOutboxSuccessIfPresent(outboxRecordId); | ||
| trafficRefillOutboxSupportService.clearIdempotency(outboxRefillUuid); |
There was a problem hiding this comment.
Don't let cleanup failures turn a successful refill back into NO_BALANCE.
Once Line 373 has marked the outbox SUCCESS, a later clearIdempotency() failure on Line 374 still falls into the catch block and returns the original retriedResult. That makes the current request fail even though the refill already landed in Redis, and it can also try to push the outbox back to FAIL. Keep the fail path scoped to the actual apply / pre-success section, and handle post-success cleanup separately.
Also applies to: 373-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.java`
around lines 354 - 355, The current try/catch scopes both the successful-apply
path and post-success cleanup so that an exception from
trafficRefillOutboxSupportService.clearIdempotency(outboxRefillUuid) can revert
a successful refill (marked by markOutboxSuccessIfPresent(outboxRecordId)) back
to retriedResult/NO_BALANCE; refactor by narrowing the try/catch to only cover
the apply/pre-success logic (where retriedResult should be returned on failure)
and move clearIdempotency(outboxRefillUuid) into a separate post-success block
that catches and logs any exceptions (referencing markOutboxSuccessIfPresent,
clearIdempotency, outboxRecordId, outboxRefillUuid, retriedResult) so cleanup
failures are logged and swallowed and do not change the already-determined
success result or rethrow into the original catch path.
pjh813
previously approved these changes
Mar 15, 2026
hyeonRS
previously approved these changes
Mar 15, 2026
…를 false로 수정하는 로직 추가
… 활성화 상태로 전환하는 로직 추가
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.java (1)
412-440:⚠️ Potential issue | 🟠 MajorDon't let idempotency cleanup downgrade an already-applied refill.
clearIdempotency(...)still runs inside the same failure scope as the Redis apply. If cleanup throws aftermarkOutboxSuccessIfPresent(...), this request falls back toretriedResulteven though the refill already landed in Redis.Suggested shape
if (!applied) { log.info( "traffic_refill_idempotent_skip poolType={} balanceKey={} outboxId={} uuid={}", poolType, balanceKey, outboxRecordId, outboxRefillUuid ); markOutboxSuccessIfPresent(outboxRecordId); - trafficRefillOutboxSupportService.clearIdempotency(outboxRefillUuid); + clearIdempotencyBestEffort(outboxRefillUuid, outboxRecordId, poolType); trafficRefillMetrics.increment(poolType.name(), "idempotent_skip"); TrafficLuaExecutionResult refillRetryResult = executeDeduct( poolType, payload, balanceKey, @@ // refill + idempotency key 등록을 단일 Lua로 수행했으므로 // 여기서는 성공 상태 전이만 마무리한다. markOutboxSuccessIfPresent(outboxRecordId); - trafficRefillOutboxSupportService.clearIdempotency(outboxRefillUuid); + clearIdempotencyBestEffort(outboxRefillUuid, outboxRecordId, poolType); } catch (RuntimeException redisApplyException) { markOutboxFailIfPresent(outboxRecordId); RuntimeException unwrapped = trafficRefillOutboxSupportService.unwrapRuntimeException(redisApplyException); trafficRefillMetrics.increment( poolType.name(),private void clearIdempotencyBestEffort(String refillUuid, Long outboxRecordId, TrafficPoolType poolType) { try { trafficRefillOutboxSupportService.clearIdempotency(refillUuid); } catch (RuntimeException e) { log.warn( "traffic_refill_idempotency_clear_failed poolType={} outboxId={} uuid={}", poolType, outboxRecordId, refillUuid, e ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.java` around lines 412 - 440, The idempotency cleanup (trafficRefillOutboxSupportService.clearIdempotency) must be made best-effort so it cannot convert a successful Redis apply into a failure path; create a helper like clearIdempotencyBestEffort(String refillUuid, Long outboxRecordId, TrafficPoolType poolType) in TrafficHydrateRefillAdapterService that calls trafficRefillOutboxSupportService.clearIdempotency(refillUuid) inside a try/catch(RuntimeException) and only logs a warn with poolType/outboxRecordId/refillUuid on exception, then replace direct clearIdempotency(...) calls after markOutboxSuccessIfPresent(...) with this new best-effort helper so exceptions won’t cause the method to return retriedResult or be treated as a Redis failure.src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java (1)
193-196:⚠️ Potential issue | 🟠 MajorBootstrap still bypasses the versioned CAS path.
Unlike the write-through flow, these direct hash writes ignore the stored policy version, so a stale snapshot can still overwrite a newer Redis state during bootstrap/reconcile. Please route this through the same CAS helper used by the policy sync path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java` around lines 193 - 196, The bootstrap code in TrafficPolicyBootstrapService currently writes policy state directly with stringOperations.opsForHash().put using trafficRedisKeyFactory.policyKey(policyId) and bootstrapVersionEpochMillis, which can overwrite newer versions; instead call the same CAS helper used by the policy sync path to perform an atomic version-checked update. Replace the two direct puts (value and version) with the versioned CAS API from the policy sync code so it compares the stored version and only writes the new "value" and "version" (using bootstrapVersionEpochMillis) when the CAS check passes; ensure you pass the policyKey and the new version value into that helper so bootstrap follows the identical versioned write path.
🧹 Nitpick comments (2)
docs/traffic_policy_acceptance_test_guide.md (1)
11-11: Consider adding blank lines around tables for markdown compliance.Markdown best practices recommend surrounding tables with blank lines for better parsing and rendering consistency.
♻️ Proposed formatting improvements
For the table at line 11:
### 2.1 정책 ID 매핑 + | ID | 상수명 | 설명 |For the table at line 104:
## 4. 테스트 요약 + | 카테고리 | 테스트 수 | 주요 검증 포인트 |Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/traffic_policy_acceptance_test_guide.md` at line 11, Add blank lines before and after the markdown tables that start with the header row "| ID | 상수명 | 설명 |" (the table at the earlier occurrence and the one around line 104) so each table is separated by an empty line above and below for proper Markdown parsing and consistent rendering.src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java (1)
742-746:MAX(repeat_block_id)대신 생성 키를 직접 받는 방식이 안전합니다.Line 742-746은 동시 삽입 상황에서 다른 레코드를 집을 수 있어 플래키 테스트 원인이 됩니다. Insert 시 generated key를 바로 받아 쓰는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java` around lines 742 - 746, The current use of jdbcTemplate.queryForObject to select MAX(repeat_block_id) is racy; change the insert flow in TrafficFlowLocalAcceptanceTest so you capture the generated repeat_block_id from the insert rather than querying MAX. Replace the SELECT/MAX usage around the repeatBlockId variable with a jdbcTemplate.update call that uses a PreparedStatementCreator/KeyHolder (or SimpleJdbcInsert) to execute the INSERT into REPEAT_BLOCK and retrieve the generated key (repeat_block_id) atomically, then use that returned key wherever repeatBlockId was used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/traffic_policy_acceptance_test_guide.md`:
- Line 83: Replace the duplicate phrase "공유풀 차단 차단" in the expected result text
with the corrected wording; choose either "공유풀 차감 차단" if intent is to indicate
deduction-plus-blocking, or "공유풀 차단" if only blocking was intended, and ensure
the updated sentence matches the surrounding Korean phrasing and context.
In `@docs/traffic-local-acceptance-scenarios.md`:
- Line 7: The docs claim Redis `flushall` clears both cache and streams, but
this conflicts with the test contract in TrafficFlowLocalAcceptanceTest which
explicitly avoids flushing the streams Redis; update the doc line to reflect the
test behavior by changing the instruction to only flush the cache Redis (not
streams) or add a clear note that streams must not be flushed to prevent
NOGROUP; reference the TrafficFlowLocalAcceptanceTest and the “streams Redis”
behavior when making the doc change so the documentation matches the test
implementation.
- Line 157: The documented test file path
`src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`
in docs/traffic-local-acceptance-scenarios.md is incorrect; find the actual test
class file named TrafficFlowLocalAcceptanceTest.java in the repository (search
for the class name) and update the markdown entry to the correct relative path
so readers can jump to the real implementation and any repo links resolve.
In
`@src/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.java`:
- Around line 52-63: The call to
trafficQuotaCacheService.applyRefillWithIdempotency currently passes a
hard-coded false for the DB-empty flag which causes replay to overwrite true
DB-empty state; update the outbox replay path so it preserves and uses the
original dbRemainingAfter/isDbEmpty from the outbox payload (or stores that flag
into the payload when creating the outbox) instead of unconditionally passing
false to applyRefillWithIdempotency; ensure payload.getUuid() and
trafficRefillOutboxSupportService.refillIdempotencyTtlSeconds() are still used
and modify the logic that invokes refill_apply_with_idempotency.lua (or the
parameters passed to trafficQuotaCacheService.applyRefillWithIdempotency) to
pass the persisted isDbEmpty value or a sentinel that lets the Lua script
preserve existing DB-empty state when final balance is unknown.
In
`@src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`:
- Line 152: The UPDATE in TrafficFlowLocalAcceptanceTest currently resets the
entire POLICY table via jdbcTemplate.update("UPDATE POLICY SET ..."), so change
it to only target the policies created/used by this test: modify the SQL in the
jdbcTemplate.update call to include a WHERE clause that restricts by the test
policy identifiers (e.g., policy_id, name, or a list of IDs returned when
creating fixtures) and pass those identifier parameters to the jdbcTemplate
call; locate the test setup/fixture code in TrafficFlowLocalAcceptanceTest to
use the same identifiers (or capture the created policy IDs) so the WHERE filter
references the exact test records only.
- Around line 929-933: The test helper flushAll(StringRedisTemplate
redisTemplate) uses connection.serverCommands().flushAll(), which clears all
Redis databases and is unsafe for shared instances; change it to call
connection.serverCommands().flushDb() to only clear the current DB, or replace
the implementation with a prefix-scoped deletion using the test key prefix (scan
and delete matching keys via RedisConnection key commands or RedisTemplate ops)
to ensure safer, finer-grained test isolation while keeping the method name
(flushAll) or renaming it to reflect the safer behavior.
---
Duplicate comments:
In
`@src/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.java`:
- Around line 412-440: The idempotency cleanup
(trafficRefillOutboxSupportService.clearIdempotency) must be made best-effort so
it cannot convert a successful Redis apply into a failure path; create a helper
like clearIdempotencyBestEffort(String refillUuid, Long outboxRecordId,
TrafficPoolType poolType) in TrafficHydrateRefillAdapterService that calls
trafficRefillOutboxSupportService.clearIdempotency(refillUuid) inside a
try/catch(RuntimeException) and only logs a warn with
poolType/outboxRecordId/refillUuid on exception, then replace direct
clearIdempotency(...) calls after markOutboxSuccessIfPresent(...) with this new
best-effort helper so exceptions won’t cause the method to return retriedResult
or be treated as a Redis failure.
In
`@src/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.java`:
- Around line 193-196: The bootstrap code in TrafficPolicyBootstrapService
currently writes policy state directly with stringOperations.opsForHash().put
using trafficRedisKeyFactory.policyKey(policyId) and
bootstrapVersionEpochMillis, which can overwrite newer versions; instead call
the same CAS helper used by the policy sync path to perform an atomic
version-checked update. Replace the two direct puts (value and version) with the
versioned CAS API from the policy sync code so it compares the stored version
and only writes the new "value" and "version" (using
bootstrapVersionEpochMillis) when the CAS check passes; ensure you pass the
policyKey and the new version value into that helper so bootstrap follows the
identical versioned write path.
---
Nitpick comments:
In `@docs/traffic_policy_acceptance_test_guide.md`:
- Line 11: Add blank lines before and after the markdown tables that start with
the header row "| ID | 상수명 | 설명 |" (the table at the earlier occurrence and the
one around line 104) so each table is separated by an empty line above and below
for proper Markdown parsing and consistent rendering.
In
`@src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`:
- Around line 742-746: The current use of jdbcTemplate.queryForObject to select
MAX(repeat_block_id) is racy; change the insert flow in
TrafficFlowLocalAcceptanceTest so you capture the generated repeat_block_id from
the insert rather than querying MAX. Replace the SELECT/MAX usage around the
repeatBlockId variable with a jdbcTemplate.update call that uses a
PreparedStatementCreator/KeyHolder (or SimpleJdbcInsert) to execute the INSERT
into REPEAT_BLOCK and retrieve the generated key (repeat_block_id) atomically,
then use that returned key wherever repeatBlockId was used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ef5171-0f78-402b-8605-c486282d512c
📒 Files selected for processing (17)
docs/feat-traffic-generator-guide.mddocs/traffic-local-acceptance-scenarios.mddocs/traffic_policy_acceptance_test_guide.mdsrc/main/java/com/pooli/traffic/domain/enums/TrafficLuaStatus.javasrc/main/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterService.javasrc/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.javasrc/main/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapService.javasrc/main/java/com/pooli/traffic/service/runtime/TrafficQuotaCacheService.javasrc/main/resources/lua/traffic/deduct_indiv.luasrc/main/resources/lua/traffic/deduct_shared.luasrc/main/resources/lua/traffic/refill_apply_with_idempotency.luasrc/main/resources/mapper/policy/AdminPolicyMapper.xmlsrc/main/resources/mapper/policy/PolicyBackOfficeMapper.xmlsrc/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.javasrc/test/java/com/pooli/traffic/service/decision/TrafficHydrateRefillAdapterServiceTest.javasrc/test/java/com/pooli/traffic/service/policy/TrafficPolicyBootstrapServiceTest.javasrc/test/java/com/pooli/traffic/service/runtime/TrafficLuaPolicyContractTest.java
💤 Files with no reviewable changes (1)
- src/main/resources/mapper/policy/PolicyBackOfficeMapper.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/lua/traffic/refill_apply_with_idempotency.lua
| | 23 | `shouldReturnPartialWhenBothPoolsExhausted` | 개인풀 10, 공유풀 10 상태에서 50 요청 | 총 20 차감, `apiRemainingData=30`, `PARTIAL_SUCCESS` | | ||
| | 24 | `shouldApplyDailyLimitOnSharedPool` | 개인풀 0, 공유풀 전환 시 일일 한도 20 적용 중 50 요청 | 공유풀 20만 차감, `HIT_DAILY_LIMIT` | | ||
| | 25 | `shouldBlockSharedPoolByRepeatBlock` | 개인풀 0, 반복 차단 활성 상태에서 공유풀 차감 시도 | 공유풀 차단 차단, `BLOCKED_REPEAT` | | ||
|
|
There was a problem hiding this comment.
Fix duplicate word in expected result.
The text "공유풀 차단 차단" appears to have a duplicate word. This should likely be "공유풀 차감 차단" or simply "공유풀 차단".
📝 Proposed fix
-| 25 | `shouldBlockSharedPoolByRepeatBlock` | 개인풀 0, 반복 차단 활성 상태에서 공유풀 차감 시도 | 공유풀 차단 차단, `BLOCKED_REPEAT` |
+| 25 | `shouldBlockSharedPoolByRepeatBlock` | 개인풀 0, 반복 차단 활성 상태에서 공유풀 차감 시도 | 공유풀 차감 차단, `BLOCKED_REPEAT` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | 25 | `shouldBlockSharedPoolByRepeatBlock` | 개인풀 0, 반복 차단 활성 상태에서 공유풀 차감 시도 | 공유풀 차감 차단, `BLOCKED_REPEAT` | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/traffic_policy_acceptance_test_guide.md` at line 83, Replace the
duplicate phrase "공유풀 차단 차단" in the expected result text with the corrected
wording; choose either "공유풀 차감 차단" if intent is to indicate
deduction-plus-blocking, or "공유풀 차단" if only blocking was intended, and ensure
the updated sentence matches the surrounding Korean phrasing and context.
| - 대상: `family_id=1`, `line_id=1~4` | ||
| - 방식: 각 시나리오마다 트래픽 처리 API(`/api/traffic/requests`) 호출 | ||
| - 공통 사전조건: | ||
| - Redis `flushall` (cache, streams 둘 다) |
There was a problem hiding this comment.
streams까지 flushall 하는 전제는 실제 테스트 코드와 충돌합니다.
Line 7 지침은 현재 TrafficFlowLocalAcceptanceTest의 계약과 다릅니다. 실제 코드는 streams Redis를 flush 하지 않도록 명시하고 있어, 문서대로 수행하면 NOGROUP 상태를 유발할 수 있습니다.
✏️ Suggested doc fix
-- - Redis `flushall` (cache, streams 둘 다)
+- - Redis flush: cache Redis만 `flushall` 수행 (streams Redis는 consumer group/PEL 보호를 위해 제외)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/traffic-local-acceptance-scenarios.md` at line 7, The docs claim Redis
`flushall` clears both cache and streams, but this conflicts with the test
contract in TrafficFlowLocalAcceptanceTest which explicitly avoids flushing the
streams Redis; update the doc line to reflect the test behavior by changing the
instruction to only flush the cache Redis (not streams) or add a clear note that
streams must not be flushed to prevent NOGROUP; reference the
TrafficFlowLocalAcceptanceTest and the “streams Redis” behavior when making the
doc change so the documentation matches the test implementation.
| --- | ||
|
|
||
| ## 6) 현재 코드 반영 상태 | ||
| - 구현됨: `src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java` |
There was a problem hiding this comment.
구현 파일 경로가 실제 테스트 클래스 위치와 다릅니다.
Line 157의 경로가 잘못되어 문서에서 바로 코드로 이동하기 어렵습니다.
✏️ Suggested doc fix
-- 구현됨: `src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`
+- 구현됨: `src/test/java/com/pooli/traffic/service/TrafficFlowLocalAcceptanceTest.java`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - 구현됨: `src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java` | |
| - 구현됨: `src/test/java/com/pooli/traffic/service/TrafficFlowLocalAcceptanceTest.java` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/traffic-local-acceptance-scenarios.md` at line 157, The documented test
file path
`src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`
in docs/traffic-local-acceptance-scenarios.md is incorrect; find the actual test
class file named TrafficFlowLocalAcceptanceTest.java in the repository (search
for the class name) and update the markdown entry to the correct relative path
so readers can jump to the real implementation and any repo links resolve.
Comment on lines
+52
to
+63
| trafficQuotaCacheService.applyRefillWithIdempotency( | ||
| balanceKey, | ||
| idempotencyKey, | ||
| payload.getUuid(), | ||
| refillAmount, | ||
| expireAtEpochSeconds, | ||
| trafficRefillOutboxSupportService.refillIdempotencyTtlSeconds(), | ||
| // [Option A] Outbox 재처리 시점에는 DB 잔량 상태를 정확히 알 수 없으므로 | ||
| // is_empty를 false로 전달한다. 잔량 복구가 목적이며, | ||
| // is_empty 상태는 다음 정상 리필 사이클에서 재기록된다. | ||
| false | ||
| ); |
There was a problem hiding this comment.
Preserve the DB-empty flag when replaying REFILL outbox events.
Passing false here is not replay-neutral. If this retry is the first successful Redis apply for a refill that exhausted the DB source, refill_apply_with_idempotency.lua will write is_empty=0 and lose that state. Persist dbRemainingAfter/isDbEmpty in the outbox payload, or make the replay path preserve the existing flag when it cannot know the final DB balance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/pooli/traffic/service/outbox/strategy/RefillOutboxStrategy.java`
around lines 52 - 63, The call to
trafficQuotaCacheService.applyRefillWithIdempotency currently passes a
hard-coded false for the DB-empty flag which causes replay to overwrite true
DB-empty state; update the outbox replay path so it preserves and uses the
original dbRemainingAfter/isDbEmpty from the outbox payload (or stores that flag
into the payload when creating the outbox) instead of unconditionally passing
false to applyRefillWithIdempotency; ensure payload.getUuid() and
trafficRefillOutboxSupportService.refillIdempotencyTtlSeconds() are still used
and modify the logic that invokes refill_apply_with_idempotency.lua (or the
parameters passed to trafficQuotaCacheService.applyRefillWithIdempotency) to
pass the persisted isDbEmpty value or a sentinel that lets the Lua script
preserve existing DB-empty state when final balance is unknown.
| jdbcTemplate.update("DELETE FROM LINE_LIMIT WHERE line_id IN (" + TARGET_LINE_IDS + ")"); | ||
|
|
||
| // 전역 정책 레코드는 soft-delete 흔적을 제거하고 기본 활성 상태로 맞춘다. | ||
| jdbcTemplate.update("UPDATE POLICY SET deleted_at = NULL, is_active = true, updated_at = NOW(6)"); |
There was a problem hiding this comment.
정책 초기화 SQL 범위를 테스트 대상 정책으로 제한하세요.
Line 152는 POLICY 전체를 복구/활성화해서, 테스트와 무관한 정책까지 상태를 바꿉니다. 로컬 회귀 신뢰성을 떨어뜨릴 수 있습니다.
🔧 Proposed fix
- jdbcTemplate.update("UPDATE POLICY SET deleted_at = NULL, is_active = true, updated_at = NOW(6)");
+ jdbcTemplate.update(
+ "UPDATE POLICY SET deleted_at = NULL, is_active = true, updated_at = NOW(6) " +
+ "WHERE policy_id BETWEEN 1 AND 7"
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`
at line 152, The UPDATE in TrafficFlowLocalAcceptanceTest currently resets the
entire POLICY table via jdbcTemplate.update("UPDATE POLICY SET ..."), so change
it to only target the policies created/used by this test: modify the SQL in the
jdbcTemplate.update call to include a WHERE clause that restricts by the test
policy identifiers (e.g., policy_id, name, or a list of IDs returned when
creating fixtures) and pass those identifier parameters to the jdbcTemplate
call; locate the test setup/fixture code in TrafficFlowLocalAcceptanceTest to
use the same identifiers (or capture the created policy IDs) so the WHERE filter
references the exact test records only.
Comment on lines
+929
to
+933
| private void flushAll(StringRedisTemplate redisTemplate) { | ||
| redisTemplate.execute((RedisCallback<String>) connection -> { | ||
| try { | ||
| connection.serverCommands().flushAll(); | ||
| } catch (DataAccessException e) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether local profile points to isolated Redis and whether FLUSHALL is necessary.
# Expected: local Redis is isolated/dedicated; otherwise replace FLUSHALL with safer cleanup.
fd -a 'application*.yml' src/main/resources src/test/resources
fd -a 'application*.yaml' src/main/resources src/test/resources
fd -a 'application*.properties' src/main/resources src/test/resources
rg -n 'spring\.data\.redis|redis|cacheStringRedisTemplate|stream|flushAll\(|flushDb\(' src/main src/test
# If custom Redis config exists, inspect factory/template wiring
rg -n 'RedisConnectionFactory|LettuceConnectionFactory|StringRedisTemplate|cacheStringRedisTemplate' src/main src/testRepository: pooli-dev/pooli-be
Length of output: 50587
🏁 Script executed:
# Read the test file around the flushAll() method
head -n 950 src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java | tail -n 40Repository: pooli-dev/pooli-be
Length of output: 1550
🏁 Script executed:
# Check the application-local.yml to understand Redis configuration
cat src/main/resources/application-local.yml | head -n 100Repository: pooli-dev/pooli-be
Length of output: 2945
🏁 Script executed:
# Search for other flushAll or flushDb usage in test files
rg -n 'flushAll|flushDb' src/test/javaRepository: pooli-dev/pooli-be
Length of output: 473
🏁 Script executed:
# Read CacheRedisConfig to understand cacheStringRedisTemplate setup
cat -n src/main/java/com/pooli/common/config/CacheRedisConfig.javaRepository: pooli-dev/pooli-be
Length of output: 5095
🏁 Script executed:
# Check if there's test setup showing Redis configuration or isolation
rg -n 'setUp|@Before|@BeforeEach|Testcontainers|Redis' src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java | head -20Repository: pooli-dev/pooli-be
Length of output: 1365
🏁 Script executed:
# Look at the complete test class header and setup to understand Redis usage
head -n 150 src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.javaRepository: pooli-dev/pooli-be
Length of output: 6177
Replace flushAll() with flushDb() or key-prefix deletion for safer test isolation.
Line 932's flushAll() clears all databases on the Redis instance, creating risk if local Redis configuration points to a shared instance. Use flushDb() to flush only the current database, or implement prefix-based key deletion for finer-grained cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/test/java/com/pooli/traffic/acceptance/TrafficFlowLocalAcceptanceTest.java`
around lines 929 - 933, The test helper flushAll(StringRedisTemplate
redisTemplate) uses connection.serverCommands().flushAll(), which clears all
Redis databases and is unsafe for shared instances; change it to call
connection.serverCommands().flushDb() to only clear the current DB, or replace
the implementation with a prefix-scoped deletion using the test key prefix (scan
and delete matching keys via RedisConnection key commands or RedisTemplate ops)
to ensure safer, finer-grained test isolation while keeping the method name
(flushAll) or renaming it to reflect the safer behavior.
pjh813
approved these changes
Mar 15, 2026
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
3 participants
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.
개요
Redis failover 대응을 위해 Traffic 정책/리필 write-through를 Outbox 패턴으로 확장하고, 정책 키 저장 구조를 String에서 Hash(
value,version)로 전환했습니다.핵심 목표는 다음 3가지입니다.
Hash(value, version)형태로 개편했습니다.incomingVersion)이 현재 저장된 버전(currentVersion)보다 클 때만 업데이트를 수행하여 역전 업데이트 현상을 원천 차단했습니다.Outbox 패턴을 통한 Redis 동기화 보장
도입 이유: Redis Write-Through 동기화 작업 유실 방지를 위함입니다.
원자적 기록: DB 업데이트와 Redis에 반영할 이벤트 정보를 하나의 DB 트랜잭션으로 묶어 Outbox 테이블에 함께 저장함으로써, Redis 쓰기 시점의 장애와 무관하게 동기화 대상을 안전하게 보존합니다.
장애 내성 확보: Redis 서버 다운이나 네트워크 오류로 인해 실시간 Write-through가 실패하더라도, DB에 기록된 Outbox 레코드를 통해 언제든 실패 지점부터 재시도를 수행할 수 있습니다.
최종 정합성 보장: 별도의 스케줄러(Relay)가 Outbox 테이블의 미처리 데이터를 주기적으로 확인하여 Redis로 재전송함으로써, 결과적으로 DB와 Redis 간의 데이터 상태를 일치시킵니다.
관련 BackLog
Resolves: 494
PR 유형
PR Checklist
개요 시퀀스 다이어그램
1. Redis 장애 시 데이터 유실 방지 및 자동 복구 (Outbox 패턴 도입)
sequenceDiagram participant App as Application participant Redis as Redis participant DB as DB (TRAFFIC_REDIS_OUTBOX) participant Sch as Retry Scheduler App->>DB: 1) Outbox 생성 (status=PENDING, retry_count=0) App->>Redis: 2) 즉시 반영 시도 alt 즉시 반영 성공 Redis-->>App: OK App->>DB: 3) status=SUCCESS else 즉시 반영 실패 Redis--xApp: connection/timeout/error App->>DB: 3) status=FAIL end loop fixedDelay(5s) Sch->>DB: 4) 재시도 대상 조회 + PROCESSING 선점\n(FAIL, 지연된 PENDING, 정체 PROCESSING) Sch->>Redis: 5) 재처리 실행 alt 재처리 성공 Redis-->>Sch: OK Sch->>DB: 6) status=SUCCESS else 재처리 실패 Redis--xSch: FAIL Sch->>DB: 6) status=FAIL, retry_count+1 end alt retry_count >= 10 alt event_type == REFILL Sch->>DB: 7) CAS 전이 (FAIL/PENDING/PROCESSING -> REVERT) Sch->>DB: 8) DB 원천 잔량 1회 반납 (보상) else non-REFILL Sch->>DB: 7) FAIL + retry_count=22 (terminal marker) end end end2. 동시성 환경에서 과거 데이터의 최신 덮어쓰기 방지 (CAS 적용)
redis 정책 동기화
sequenceDiagram participant App as Application participant WR as Policy WriteThrough/Retry participant Redis as Redis (CAS Lua) App->>WR: 정책 변경 이벤트 처리 (version 포함) WR->>Redis: CAS Lua 실행 (incomingVersion, value) note right of Redis: Hash: { value, version } Redis->>Redis: currentVersion 비교 alt incomingVersion > currentVersion Redis->>Redis: value/version 갱신 Redis-->>WR: 1 (SUCCESS) WR-->>App: 성공 처리 else incomingVersion <= currentVersion Redis-->>WR: 0 (STALE_REJECTED) note right of WR: 역전 업데이트 방지\n(무시되지만 정상 결과로 취급) WR-->>App: 성공 처리(무해한 skip) else Redis 연결/실행 실패 Redis--xWR: 예외/실패 WR-->>App: 실패 처리 (Outbox FAIL, 재시도 대상) endredis 데이터 잔량 REFILL 동기화
sequenceDiagram participant App as Application participant Redis as Redis participant DB as DB (Outbox/Refill Source) App->>DB: 1) DB 리필 차감 + Outbox(PENDING) 생성 App->>Redis: 2) 원자 Lua 실행\n(amount 증가 + idempotency key set NX) alt idempotency key 없음 (최초 처리) Redis->>Redis: amount 증가, TTL 설정, idempotency key 생성 Redis-->>App: 1 (APPLIED) App->>DB: 3) Outbox SUCCESS App->>Redis: 4) idempotency key 즉시 삭제 else idempotency key 이미 존재 (중복 시도) Redis-->>App: 0 (SKIPPED) App->>DB: 3) Outbox SUCCESS App->>Redis: 4) idempotency key 즉시 삭제 else Redis 실패 Redis--xApp: 예외/실패 App->>DB: 3) Outbox FAIL (재시도 대상) end변경 파일/클래스 정리
1) Outbox 스키마/도메인
Outbox 테이블 생성 스크립트와 이벤트 처리를 위한 도메인 및 페이로드 정의 파일입니다.
db/migrationTRAFFIC_REDIS_OUTBOX테이블 및 인덱스 생성traffic.domain.outboxtraffic.domain.outboxtraffic.domain.outboxtraffic.domain.outboxtraffic.domain.outbox.payloadtraffic.domain.outbox.payloadtraffic.domain.outbox.payloadtraffic.domain.outbox.payloadtraffic.domain.outbox.payloadtraffic.domain.outbox.payload2) Mapper 계층
Outbox 데이터 접근 및 REFILL DB 처리를 위한 Mapper 구성입니다.
traffic.mappermapper/trafficFOR UPDATE SKIP LOCKED포함 SQL 정의traffic.mappermapper/trafficrestoreIndividualRemaining등) 추가3) Outbox 서비스/전략
이벤트 재시도 스케줄링 및 타입별 처리 전략 로직입니다.
traffic.service.outboxtraffic.service.outboxtraffic.service.outbox.strategytraffic.service.outbox.strategytraffic.service.outbox.strategy4) 기존 트래픽 서비스/런타임 수정
Outbox 패턴 적용에 따른 기존 비즈니스 로직의 변경 사항입니다.
traffic.service.policytraffic.service.decisiontraffic.service.runtime5) Lua 스크립트
Redis 내 원자적 처리를 위한 스크립트 변경 및 추가 사항입니다.
lua/trafficvalue/version) 읽기 방식으로 변경lua/trafficlua/traffic6) 테스트/문서
traffic.service.policydocs클래스 간 상호관계
graph TD A["TrafficPolicyWriteThroughService"] --> B["RedisOutboxRecordService"] A --> C["TrafficPolicyVersionedRedisService"] C --> L1["policy_value_cas.lua"] C --> L2["repeat_block_snapshot_cas.lua"] C --> L3["app_policy_single_cas.lua"] C --> L4["app_policy_snapshot_cas.lua"] B --> M1["RedisOutboxMapper"] M1 --> DB1[("TRAFFIC_REDIS_OUTBOX")] H["TrafficLinePolicyHydrationService"] --> A S["RedisOutboxRetryScheduler"] --> B S --> R["OutboxRetryStrategyRegistry"] R --> ST1["Sync*OutboxStrategy"] R --> ST2["RefillOutboxStrategy"] ST1 --> A ST2 --> U["TrafficRefillOutboxSupportService"] U --> M2["TrafficRefillSourceMapper"] D["TrafficHydrateRefillAdapterService"] --> Q["TrafficQuotaSourcePort"] Q --> QI["TrafficDefaultQuotaSourceAdapter"] QI --> M2 QI --> B D --> U D --> B변경사항(PR 리뷰 반영)
Outbox 보상 멱등성 강화 (REVERT 도입)
OutboxStatus에REVERT상태 추가markRevertIfCompensable(CAS update)로 상태 전이를 먼저 수행한 뒤, 영향 행이 1일 때만 DB 반납 실행status IN ('FAIL','PENDING','PROCESSING')조건으로SUCCESS/REVERT오염 방지REVERT상태는 재시도 조회 대상에서 제외되도록 처리REFILL 보상 경로 단일화
compensateRefillOnce(...)로 통합REFILL Redis 반영 원자성 개선
amount 증가 + 멱등키 등록을 단일 Lua(refill_apply_with_idempotency.lua)로 원자 처리멱등키 선등록 후 refill순서 제거SUCCESS마킹이 완료되면 멱등키 즉시 삭제멱등 TTL/재시도 파라미터 조정
600s로 상향pendingDelaySeconds=60,processingStuckSeconds=150,maxRetryCount=10retry_count 0~9로 동작하도록 상한 로직 정리정책 동기화 CONNECTION_FAILURE 처리 수정
CONNECTION_FAILURE를 더 이상 성공 동등 처리하지 않음FAIL경로로 전환하여 재시도/복구 가능하게 수정Hydrate/Refill 실패 시 정합성 보강
compensateRefillOnce) 수행ERROR로 강등하지 않고HYDRATE/NO_BALANCE상태 유지테스트 보강
REVERT CAS 기반 compensateRefillOnce멱등성 테스트 추가./gradlew test) 통과 확인Summary by CodeRabbit
New Features
Database
Documentation
Tests