feat: resolve observability, cluster, and migration issues - #6
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e2128a11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| requireAllowedClassName(className); | ||
| return super.resolveClass(descriptor); |
There was a problem hiding this comment.
When JDK migration processes Redis data that an attacker can modify, this whitelist still permits every class under the application's allowed package prefixes, and ObjectInputStream.readObject() executes those classes' readObject/readResolve callbacks before requireAllowedValue runs. A whitelisted business class with a dangerous callback therefore provides code execution despite the CLI being documented as a safe restricted decoder; use an object-input filter or a migration representation that does not instantiate arbitrary application classes.
Useful? React with 👍 / 👎.
| byte[] current = connection.stringCommands().get(sourceKey); | ||
| if (current == null) { | ||
| Boolean restored = connection.stringCommands().set( | ||
| sourceKey, legacyBytes, expirationOf(connection, backupKey), | ||
| SetOption.SET_IF_ABSENT); |
There was a problem hiding this comment.
If the application evicts a migrated source key during the rollback window, this branch treats the absence as permission to recreate it from the backup. That reverses a valid post-cutover mutation and can resurrect stale cache data until the original TTL; rollback should only replace the exact envelope produced by cutover and should fail safely when the source is absent.
Useful? React with 👍 / 👎.
| report.selected++; | ||
| report.decodedLegacy++; | ||
| record("decoded"); |
There was a problem hiding this comment.
When values fail legacy decoding or serialization, execution jumps to the catch block before selected is incremented. A pattern containing many malformed or disallowed values can therefore scan, deserialize, and log every matching key even with max-keys=1000, defeating the CLI's bounded-work safety guarantee; attempted/rejected actionable keys must also consume the invocation quota.
Useful? React with 👍 / 👎.
| switch (migration.getPhase()) { | ||
| case SHADOW_READ -> { } | ||
| case DUAL_WRITE -> writeSidecar( |
There was a problem hiding this comment.
When the matching legacy dataset exceeds max-keys, SHADOW_READ stops after the first bounded subset but writes no completion marker or cursor. Every rerun starts a fresh Redis SCAN and can select the same keys again, so the documented resumable workflow cannot validate the remainder or establish complete shadow coverage; persist scan progress or otherwise distinguish already validated keys.
Useful? React with 👍 / 👎.
| legacyMapper.activateDefaultTypingAsProperty( | ||
| validator, ObjectMapper.DefaultTyping.EVERYTHING, "@class"); | ||
| GenericJackson2JsonRedisSerializer.registerNullValueSerializer(legacyMapper, "@class"); | ||
| this.genericJackson = new GenericJackson2JsonRedisSerializer(legacyMapper); |
There was a problem hiding this comment.
When legacy values were written by GenericJackson2JsonRedisSerializer with a custom type-hint property such as _type, validation recognizes the configured property but this mapper still requires @class. DTO values from that supported serializer configuration consequently fail with a missing type-id error instead of migrating; configure the legacy mapper and null serializer with the selected legacy type property rather than hard-coding @class.
Useful? React with 👍 / 👎.
| TimerKey key = new TimerKey( | ||
| handler.getClass().getSimpleName(), | ||
| result.decision().name(), | ||
| context.getCacheName()); | ||
| Timer timer = timers.computeIfAbsent(key, this::registerTimer); |
There was a problem hiding this comment.
When callers create caches dynamically—for example, cacheManager.getCache(tenantId)—RedisProCacheManager#getMissingCache accepts each new name, and this map plus Micrometer retain a distinct timer for every handler/decision/name tuple forever. Thus a user-derived cache name becomes the unbounded dynamic tag that the change intends to avoid, causing registry and heap growth; restrict or normalize cache names before using them as timer keys.
Useful? React with 👍 / 👎.
The composite action ./.github/actions/setup-jdk-21 unconditionally forwarded server-id / server-username / server-password / gpg-private-key / gpg-passphrase inputs to actions/setup-java@v5. Even when those inputs were the empty-string defaults, setup-java@v5 would write a <server> block to ~/.m2/settings.xml with <id></id>, and Maven 3.9+ aborts at startup with `servers.server[0].id is missing`. This was the root cause of PR Quality Checks failing on PR #6 (run 30210117026, job 89814796395) and CI failing on every push to main/master since 2026-07-03 (ci.yml had not passed on either branch since the composite was introduced; only release.yml worked because it passed a non-empty server-id). Fix: - Drop the five deploy inputs from the composite action entirely. ci.yml, pr-checks.yml, dependency-check, docs-link-check, checkstyle, and build-package now get a clean setup-java call that writes no <server> block. - release.yml calls actions/setup-java@v5 directly with the same deploy inputs it used to pass to the composite. The single deploy caller is preserved with no behaviour change on tag push. Verified locally: - python3 yaml.safe_load passes for action.yml, ci.yml, pr-checks.yml, release.yml. - grep for server-* in ci.yml and pr-checks.yml returns nothing. - git diff --check clean. - release.yml deploy inputs (server-id=central, OSSRH_*, GPG_PRIVATE_KEY, GPG_PASSPHRASE) preserved verbatim.
The properties files in src/test/resources hard-coded DOCKER_API_VERSION=1.54 (testcontainers.properties) and api.version=1.54 (docker-java.properties). docker-java 3.4.0 (which testcontainers-bom 1.20.4 transitively pulls in) accepts that value and tries to negotiate it with the Docker daemon. The GitHub-hosted ubuntu-latest runner image reports "Maximum supported API version 1.48" and rejects the connection with "client version 1.54 is too new", which cascades into Testcontainers "IllegalState Previous attempts to find a Docker environment failed. Will not retry." for every @testcontainers integration test. This failure was previously hidden by an earlier CI configuration bug (composite action wrote <server><id></id> into ~/.m2/settings.xml and crashed Maven before any test ran). Once that was fixed, the api.version override surfaced. Fix: remove the hard-coded override from both files. docker-java 3.4.0 auto-negotiates the highest mutually-supported API version with the daemon, which works on both the GitHub runner (max 1.48) and any modern local Docker (we use 1.55 locally). Comments in both files now explain why we deliberately do not pin a version. Verified locally: docker-java dependency tree unchanged (testcontainers 1.20.4 -> docker-java 3.4.0); only the two properties files are touched. Refs: PR #6 PR Quality Checks run 30212533645, job 89821091011 (UnixSocketClientProviderStrategy: BadRequestException 400)
AbstractRedisIntegrationTest shells out to the `socat` binary to forward the testcontainers container bridge IP back to 127.0.0.1, which the Spring context then connects to. The Ubuntu GitHub-hosted runner does not ship socat, so every @testcontainers IT threw "IllegalState: Cannot start socat for container 172.17.0.2" before Spring could even start. Install socat via apt-get in the build / pr-checks jobs. The other ci.yml jobs (checkstyle, dependency-check, qodana, docs-link-check, build-package) do not run integration tests and are left untouched. Verified locally: - python3 yaml.safe_load passes for both ci.yml and pr-checks.yml - git diff --check clean - the new step lives between Setup JDK and the Maven build, so the JDK is in place before apt runs
77d7fa2 to
2886297
Compare
DavidHLP
added
blocked
ChatGPT Codex Connector
|
This PR is currently blocked on the unresolved review findings despite the successful CI run. The three P1 findings must be resolved before merge:
The three P2 findings also need either fixes or an explicit, evidence-backed disposition:
After updating the branch, please add focused regression tests for the affected behavior and request another review. I have added |
1 participant
Summary
handler,decision, andcacheNamedimensions toresicache.chain.executewithout unbounded Redis-key tags.ai/issues/Validation
./mvnw clean verify -Bredis:7-alpineCluster reachedcluster_state:okRedisClusterSlotIntegrationTestverified equal server-sideCLUSTER KEYSLOTvalues and executed a two-key command withoutCROSSSLOTtestcontainers-bom:1.20.4compatibility override preservedgit diff --checkpassedReview
Independent reviewer jobs did not yield within the bounded review windows, so the same Standards + Spec gates were completed in the maintainer process against the full diffs, callers, tests, configuration, and documentation.
The migration review found and fixed:
typePropertywhitelist bypassNo critical, high, or behavior-related medium findings remain.
Fixes #2
Fixes #4
Fixes #5
Issue #3 is intentionally excluded because an external contributor explicitly volunteered on 2026-07-21 and no abandonment signal has been posted.