DanielLeens commented on PR #10879:
URL: https://github.com/apache/seatunnel/pull/10879#issuecomment-4550420535
Thanks for the follow-up. I re-reviewed the latest head, including the
latest delta after my previously approved head, and I do not see a reopened
source-level blocker on the current Phase 2 path.
## What this PR is trying to solve
- User pain point: the standalone Edge Agent needs to collect local files
and deliver them over EdgeSocket reliably; if connect/auth failure handling or
stream/resource cleanup is loose, long-running edge nodes accumulate avoidable
instability.
- Fix approach: the latest delta hardens a few boundary details on top of
the existing Phase 2 path: constant-time token comparison, explicit socket
close on connect/auth failure, explicit `Inflater.end()` on close, and
retry-budget reset after a successful ingress bind.
- One-line summary: this latest delta is a focused hardening pass rather
than a protocol rewrite, and from the source-level review side I do not see a
new blocker.
## Runtime path I re-traced
```text
edge input
-> FileCollectReader.poll()
-> scheduler flush/send loop
-> EdgeTransportClient.ensureAuthenticatedSession()
-> socket connect
-> auth
-> send batch / poll commit
server side
-> EdgeSocketIngressServer.start()
-> openServerSocketWithRetry()
-> reset retry budget after a successful bind
-> IngressProtocolHandler.authenticate()
-> constantTimeEquals()
```
## Findings
- `IngressProtocolHandler.java:71-152`
- token comparison now uses `MessageDigest.isEqual(...)`, which is a
sensible hardening step for the auth boundary.
- `EdgeTransportClient.java:178-186`
- connect/auth failure now closes the socket explicitly instead of leaving
cleanup entirely to later GC/finalization timing.
- `EdgeSocketCompressionPayloadDeserializer.java:133-140`
- `Inflater.end()` is now guaranteed on close, which is the right
direction for native resource cleanup.
- `EdgeSocketIngressServer.java:103-120`
- retry budget is reset after a successful bind, so a later reopen cycle
does not inherit stale retry state.
- `FileCollectReader.java:144-159`
- the delta here is only dead local-variable removal; it does not change
the read semantics.
## Code quality / tests / docs
- I did not find a reopened flaky-test pattern on the current head.
- Stability note:
- the shared EdgeSocket reader tests still use `Awaitility` for readiness
rather than hard sleeps, which is the right test shape for socket startup.
- This latest delta does not introduce a new user-visible behavior change
that would require extra docs on top of the existing Phase 2 documentation.
## Conclusion
### Conclusion: can merge
1. Blockers
- No source-level blocker from this review.
2. Suggested follow-ups
- The current `Build` check is still `in_progress`, so the merge gate still
depends on CI settling green. I am treating that as a gate status, not as a new
code blocker on this delta.
Overall, this is a focused hardening delta on top of an already-reviewed
main path, and I do not see a reopened correctness or compatibility issue in
the latest code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
