Feat/dynamic dedup tcp

[praagyajain]

Related Issue

• Info about Issue or bug

Closes: #[issue number that will be closed through this PR]

Describe the changes you've made

A clear and concise description of what you have done to successfully close your assigned issue. Any new files? or anything you feel to let us know!

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Code style update (formatting, local variables)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How did you test your code changes?

Please describe the tests(if any). Provide instructions how its affecting the coverage.

Describe if there is any unusual behaviour of your code(Write NA if there isn't)

A clear and concise description of it.

Checklist:

• My code follows the style guidelines of this project.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas and used java doc.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.

Screenshots (if any)

Original	Updated
original screenshot	updated screenshot

[Copilot]

Pull request overview

This PR updates the Java dynamic-dedup agent to support a TCP transport mode (intended for k8s / non-shared-/tmp setups) and changes how coverage fingerprints are produced/sent during replay.

Changes:

• Add dynamic transport selection: unix-socket CommandServer (local/docker) vs TCP client CoverageTcpClient (k8s) based on KEPLOY_COVERAGE_ENDPOINT.
• Add one-time “warmup” of indexed application classes on first START to reduce <clinit> noise in the first test window.
• Change coverage capture to emit per-class executed JaCoCo probe indices (instead of source-file executed line numbers).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sneha0099]

Reviewed the SDK-side dynamic-dedup changes end to end (the JaCoCo probe capture, the new TCP transport, the bytecode + manifest export, and the offline CoverageReporter). Findings below were verified against the branch code, not from memory. Line numbers refer to files on this branch.

Executive summary

• Overall assessment: REQUEST CHANGES
• Risk level: MEDIUM to HIGH

The core design is sound: probe-set fingerprints for branch-aware dedup, a warmup pass for determinism, and offline coverage reconstruction from a probe union plus a per-class manifest. The self-test proving reconstruction equals JaCoCo's own analysis is a good instinct. But the PR ships a large block of dead code (including a trust-all TLS manager), re-transmits the full bytecode blob once per newly discovered class, and changes an existing wire payload's meaning while keeping its old field name. Several correctness edges (load-time-transformed classes, warmup side effects, COV-on-exception) are unguarded, and there are no automated tests.

Findings

[HIGH] Entire HTTP bytecode-upload path is dead code, and it carries a trust-all TLS manager. KeployDedupAgent.java:272-393. bytecodeAlreadyStored (272), postBytecode (295), readStream (332), writeAscii (345), relaxTlsIfHttps (349), trustAllFactory (353), and urlEncode (387) are all new in this PR and none of them is called from anywhere. The bytecode now ships via pollBytecodeFrame over the TCP collector channel instead. That leaves roughly 130 lines of unreachable code, including an X509TrustManager that accepts every certificate and a hostname verifier that returns true. Even though it is currently unreachable, committing a disabled-cert-validation path is a loaded gun: a future edit that re-wires postBytecode silently ships with TLS verification off. Remove the whole HTTP cluster; if the HEAD-then-POST idempotency is still wanted as a fallback, wire it in behind an explicit path and do not disable certificate validation.

[HIGH] The full class zip is rebuilt and re-sent every time the manifest grows. KeployDedupAgent.java:952-995. pollBytecodeFrame gates on size <= lastUploadedManifestSize (957), but the manifest grows one class at a time as new app classes are hit across tests, and zipIndexedClasses (982) always zips every indexed class regardless of manifest size. So the build-constant bytecode blob (potentially megabytes) is re-zipped and re-sent as a single base64 line on the TCP channel on every manifest-size increase, up to O(distinct classes hit) times over a replay. On the collector side each of these arrives as one giant readLine, forcing a multi-megabyte line buffer. The bytecode never changes; send the zip once (guard with a sent-once flag) and let only the manifest grow, or send the final manifest at stop.

[HIGH] The per-test payload changed meaning but kept the name executedLinesByFile. KeployDedupAgent.java:840-908 (capture now returns {vmClassName -> [probeIdx]}), 515, 663-672, 1488-1497. Both transports serialize through DedupPayload, whose field is executedLinesByFile, so the wire JSON still uses that key while the value is now probe indices keyed by VM class name, not source lines keyed by file. The local variable at 515 is named executedLinesByFile for the same probe data. This works only because the enterprise consumer (#2188) treats the map opaquely as key:int sets, so the naming is now actively misleading and the coupling is implicit. Rename the field and variables to reflect probes-by-class, and document the wire contract change explicitly since any other consumer of this format breaks silently.

[MEDIUM to HIGH] Warmup eagerly runs every app class's static initializer, opt-out and on the START path. KeployDedupAgent.java:787-803, invoked at 498 and 646 inside testCaseLock. Class.forName(name, true, loader) executes <clinit> for all indexed classes on the first START. Static initializers can open connections, spawn threads, register shutdown hooks, or fail; per-class throwables are swallowed (832) but the side effects already happened, and this runs in replay where fidelity matters. It also runs synchronously under the lock before the first START is ACKed, so on a large app the first START ACK can be delayed by seconds and may exceed the harness's START timeout. Make warmup opt-in rather than opt-out, or scope it, and move it off the ACK-blocking path.

[MEDIUM] On a capture exception, COV is not emitted before ACK, breaking the stated one-COV-per-END invariant. KeployDedupAgent.java:662-686. The comment at 667 says COV is always emitted before ACK, but if collector.capture() throws, control jumps to the catch (no COV, no CLASSES) and the finally writes only ACK. The TCP collector reads COV and ACK off the same stream, so a missing COV shifts the protocol and the collector may associate the next test's frames incorrectly. Emit an explicit empty COV in the failure path (or a distinct error token the collector understands) so the stream stays in lockstep.

[MEDIUM] Offline coverage undercounts classes transformed at load time. CoverageReporter.java:105-159 (analyzeAll at 141), manifest id captured at KeployDedupAgent.java:884-889. The reconstruction matches store entries to analyzed classes by JaCoCo's CRC64 class id. That id is computed from the bytes JaCoCo saw at instrumentation time; if another agent or a CGLIB/load-time-weaving path transformed the class, the live id will not match the on-disk .class bytes the Analyzer hashes, so the class is treated as never-hit and its covered probes are dropped from the numerator while it still counts toward the denominator. That silently deflates the reported coverage for Spring-style apps. Dedup itself is unaffected (probe sets are compared like-for-like), but the reported numbers can be wrong. Document this limitation and ideally detect and log id mismatches.

[MEDIUM] No automated tests; the self-test is a manual harness that skips the real serialization path. CoverageReporterSelfTest.java is a main() with System.exit, needs an external .exec and classes dir, and is not wired into CI. Its round-trip at lines 72-73 does Long.parseUnsignedLong(Long.toHexString(id), 16), which is an identity on the id and never exercises the Gson manifest/union serialization or the file reads the production compute path uses. There are no unit tests for CoverageReporter.compute, CoverageCollector.capture, or pollBytecodeFrame. Add a JUnit test with a small checked-in fixture (a tiny classes dir plus a synthetic manifest and union JSON) that goes through the real serialize and read path, and a protocol test for the TCP dispatch sequence.

[MEDIUM] All app class bytes are held in memory for the JVM lifetime, and the zip is built fully in memory. KeployDedupAgent.java:1392-1431, 1499-1510, 982-995. CoverageIndex reads every .class into ClassEntry.bytes and caches the list forever, and zipIndexedClasses accumulates the entire zip in a ByteArrayOutputStream. For a large app this is a persistent footprint inside the app JVM plus a transient full-zip allocation on each send. Since the bytes are only needed to build the zip once, read them lazily at zip time and drop them after, or stream the zip.

[LOW] Dead field. KeployDedupAgent.java:758. uploadInFlight is declared and never read or written. Remove it.

[LOW] Endpoint parsing does not handle IPv6. KeployDedupAgent.java:123-137 uses lastIndexOf(':') to split host and port, which mis-splits a bracketed or bare IPv6 literal. Fine if endpoints are always IPv4 or DNS names, but worth a guard or a comment stating the assumption.

Missing test cases

• CoverageReporter.compute over a fixture classes dir with a manifest and union JSON, asserting the exact line and branch counts (through the real Gson and file path, not the id no-op).
• A hit class present in the union but absent from the classes blob, and a class in the blob never hit, to lock the denominator behavior.
• A class whose live id does not match the on-disk bytecode (load-time transform), asserting the intended behavior and that it is logged rather than silently dropped.
• TCP dispatch protocol: START then END happy path, mismatched END id, and a capture failure, asserting the exact COV/ACK line sequence the collector expects.
• pollBytecodeFrame sends at most once for a build-constant class set, and no frame when KEPLOY_BUILD_TAG is unset.
• Warmup: a class whose <clinit> throws is skipped without aborting warmup, and warmup runs exactly once.

Breaking change and operational impact

• Wire contract: the per-test coverage payload changed from source-file/line to VM-class/probe under the same executedLinesByFile key. This must land in lockstep with the enterprise consumer (#2188) and k8s-proxy (#574); any other reader of this payload breaks. Call this out in the PR.
• New protocol surface: the TCP transport and the CLASSES <b64tag> <b64manifest> <b64zip> frame are a new contract shared with k8s-proxy #574; both sides must agree on framing and size limits (see the full-zip resend finding).
• New configuration: KEPLOY_COVERAGE_ENDPOINT, KEPLOY_BUILD_TAG, KEPLOY_JAVA_DEDUP_WARMUP_DISABLED, KEPLOY_JAVA_DEDUP_DISABLED, KEPLOY_JAVA_DEDUP_DIAGNOSTICS. These should be documented (README only bumped a class-name reference).
• Deployment risk: warmup runs arbitrary app static initializers in replay by default; that is the highest operational risk here for real applications.

Final verdict

Not ready to merge. The approach is right and the reconstruction idea is validated in principle, but the dead HTTP path (with trust-all TLS), the full-zip resend, and the renamed-but-not-renamed wire payload should be fixed before merge, and the warmup default and COV-on-exception edges need to be made safe. Add at least one automated test through the real serialization path. Once the dead code is gone, the zip is sent once, the payload is renamed and documented, and warmup is opt-in, this is close.