Revert "fix(ipc): bound PUB lifecycle, loss semantics and safe bind defaults" (#74) - #75
kiro-agent[bot] wants to merge 1 commit into
Conversation
…efaults …" This reverts commit 8fe78be.
|
Skipping PR review because a bot author is detected. If you want to trigger CodeAnt AI, comment |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -41 |
| Duplication | -12 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
There was a problem hiding this comment.
(2 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| backend.rs | 8.41 → 8.96 | Large Assertion Blocks |
| daemon.rs | 5.22 → 6.04 | Low Cohesion, Code Duplication |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Question: Answer: The changes consistently restore the pre-#74 behavior:
The main behavioral regressions—unbounded/default PUB lifecycle, all-interface binding, and loss of empty-batch suppression/accounting—are intentional consequences of this revert and match the stated goal of restoring the pre-#74 tree. |
![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: 01515b57b5
ℹ️ 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".
| pub_socket | ||
| .bind(&pub_endpoint) | ||
| .map_err(|e| anyhow::anyhow!("failed to bind ZMQ PUB on {pub_endpoint}: {e}"))?; | ||
| .bind(&format!("tcp://*:{}", cfg.spine_pub_port)) |
There was a problem hiding this comment.
When the binary is built with corpus-ipc, tcp://* exposes spike egress on every network interface. Configurations created for the parent version can still specify spine_pub_bind_host = "127.0.0.1", but that now-unknown TOML key is silently ignored, so an operator who explicitly requested loopback unexpectedly gets a remotely reachable socket. Preserve the configurable loopback default and require an explicit wildcard host for broad exposure.
Useful? React with 👍 / 👎.
| pub_socket | ||
| .bind(&pub_endpoint) | ||
| .map_err(|e| anyhow::anyhow!("failed to bind ZMQ PUB on {pub_endpoint}: {e}"))?; | ||
| .bind(&format!("tcp://*:{}", cfg.spine_pub_port)) |
There was a problem hiding this comment.
For the corpus-ipc binary, this binds the PUB socket without overriding ZeroMQ's default infinite LINGER. If a slow or disconnected subscriber leaves outbound frames pending when SIGTERM or another shutdown path returns, destruction of the socket/context can wait indefinitely rather than letting the daemon exit. Set a finite LINGER, preferably zero by default, before binding the socket.
Useful? React with 👍 / 👎.
| pub_socket | ||
| .bind(&pub_endpoint) | ||
| .map_err(|e| anyhow::anyhow!("failed to bind ZMQ PUB on {pub_endpoint}: {e}"))?; | ||
| .bind(&format!("tcp://*:{}", cfg.spine_pub_port)) |
There was a problem hiding this comment.
When a corpus-ipc deployment carries a custom spine_pub_sndhwm, that now-unknown TOML key is silently accepted but ignored and the socket falls back to libzmq's default. In particular, a low limit chosen to constrain memory under a slow subscriber can become a queue of up to 1000 messages per peer, while larger tuned limits can unexpectedly cause earlier drops. Restore the validated setting and apply it before binding.
Useful? React with 👍 / 👎.
| pub_socket, | ||
| cfg.spine_pub_send_empty_batches, | ||
| )), | ||
| sink: Box::new(brainstem_daemon::backend::ZmqSpikeSink::new(pub_socket)), |
There was a problem hiding this comment.
For configurations that set spine_pub_send_empty_batches = false, the removed field is silently ignored and constructing the sink with new restores unconditional sends. Because the tick loop calls emit even when no neurons fire, a default 1 kHz daemon now publishes roughly 1000 unwanted empty JSON frames per second, increasing egress and subscriber work despite the explicit suppression setting. Pass the configured policy into the sink.
Useful? React with 👍 / 👎.
|
Closing without merging. The intent was to fix the CodeScene code-health failure forward — extract the PUB config/validation out of |
1 participant
Reverts #74 (merge commit
8fe78be).Reason
PR #74 was merged into
mainwhile the CodeScene code-health gate was failing ("Prevent hotspot decline" — Low Cohesion onsrc/daemon.rs). Merging over a failing quality gate is not an acceptable landing, so this backs the change out ofmain.What this does
mainto its pre-fix(ipc): bound PUB lifecycle, loss semantics and safe bind defaults #74 state. The revert commit01515b5produces a tree identical to the pre-merge commit1dab1cd(verified: empty diff between them) — a clean, complete revert with no partial leftovers.main.Follow-up
Issue #68 / LIM-1320 (v0.3.0 qualification blocker) remains open and unaddressed on
mainafter this revert. Re-landing requires a version of the change that also satisfies the CodeScene cohesion gate onsrc/daemon.rs(e.g. extracting the PUB config/validation into a submodule to reduce the module's responsibility count), then re-merging only once all gates — including CodeScene — pass.Base:
main. Head:revert-74-fix/ipc-pub-lifecycle-68.Summary by cubic
Reverts the bounded PUB lifecycle, loss semantics, and safe bind defaults change (#74) from
main. The original merge landed while the CodeScene code-health gate was failing, so this backs it out cleanly and restoresmainto its pre-#74 state.spine_pub_bind_host,spine_pub_sndhwm,spine_pub_linger_ms, andspine_pub_send_empty_batchesconfig, plusSinkSendStatsand empty-batch suppression inZmqSpikeSink; PUB egress bindstcp://*:<port>again.src/daemon.rs.Written for commit 01515b5. Summary will update on new commits.