Skip to content

Expose non-owning connections for served-object callbacks - #50

Merged
senamakel merged 4 commits into
mainfrom
live-weak-connection-7414
Oct 11, 2026
Merged

senamakel merged 4 commits into
mainfrom
live-weak-connection-7414

Conversation

@senamakel

Copy link
Copy Markdown
Member

A served object that stores its own Connection creates a cycle: the connection owns the object tree, and the object keeps the connection alive. Dropping the caller’s last handle therefore leaves the object and its bus name alive. Live module callback adapters need non-owning access to avoid this cycle.

Add Connection::downgrade() and WeakConnection::upgrade(). Upgrading first acquires the public lifetime guard, so background dispatch state cannot resurrect a connection after its last owning handle disappears. Existing calls, admission, transport close, ABI and wire behavior remain unchanged. Document callback ownership beside the connection API.

Validation:

  • Actual in-memory served-object/name regression failed against a strong stored connection, then passed with the weak reference; the fixture still performs a real callback before drop.
  • The second regression verifies temporary owning upgrades and refusal to revive background dispatch state.
  • Connection suite: 27 passed. Full all-feature suite: 478 kernel tests plus workspace and documentation tests passed (existing ignored tests retained).
  • Strict all-target/all-feature Clippy, formatting, no-default-feature workspace check and diff whitespace check passed.

This is an independent TinyBus prerequisite for the TinyVoice live module migration tracked by tinyhumansai/openhuman#7414. No OpenHuman adapter or release pin is changed here.

senamakel and others added 4 commits October 11, 2026 21:16
Add a test that serves an object holding a clone of its own connection and
verifies that dropping the service connection releases both the served object
and its requested bus name, guarding against reference cycles between a
connection and the objects it serves.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
… tests

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Served objects need their connection to issue callbacks, but holding a strong
handle creates an ownership cycle that keeps the service, transport and
well-known name alive. WeakConnection borrows the connection only for the
duration of a callback and cannot resurrect it once the last strong owner
drops, even while dispatch state still holds an internal reference.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The callback connection test now stores a downgraded connection so the served object no longer keeps its own connection alive, matching the ownership model documented for callback objects. The connection module README gains a section explaining why served objects should hold weak references and how upgrade behaves across the connection lifetime.

Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 4 billable files and costs up to $1.00.

Or wait 14 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d1ed4f9b-bf89-437e-aa26-522c06564941

📥 Commits

Reviewing files that changed from the base of the PR and between f4b8f59 and 10579d4.


📒 Files selected for processing (4)
  • crates/tinybus/src/connection.rs
  • crates/tinybus/src/connection_tests.rs
  • crates/tinybus/src/lib.rs
  • docs/modules/connection/README.md

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-11T18:27:08.122049Z 10579d4 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@senamakel
senamakel merged commit 0cbc48b into main Oct 11, 2026
8 checks passed
@tinysweeper

tinysweeper Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper completed its review; deterministic results follow.

State: Incomplete
Priority: none
Reviewed head: 10579d486f27
Updated: 2026-10-12T03:12:55Z

Review snapshot

Change surface Files Review signal Count
Production 2 Active findings 0
Tests 1 Noted findings 0
Documentation 1 Resolved findings 0
Configuration 0 Pending checks/questions 4

Completeness: Incomplete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

`WeakConnection` is introduced in `crates/tinybus/src/connection.rs` as a cloneable struct holding a `std::sync::Weak` to `Inner` and a `std::sync::Weak` to the `CloseOnDrop` lifetime guard, with a `Debug` implementation reporting `has_strong_owner` based on the guard's strong count. `Connection::downgrade()` downgrades both the inner state and the lifetime guard. `WeakConnection::upgrade()` upgrades the `CloseOnDrop` guard first and returns `None` if that fails, then upgrades `Inner`; because the returned `Connection` carries the same guard, a weak reference cannot revive a connection after its last strong owner drops, even while dispatch tasks still hold internal state. An explicitly closed connection that still has an owner remains upgradable, and calls on it return the normal closed-transport errors. The type is re-exported alongside `Connection` from `crates/tinybus/src/lib.rs`, and `docs/modules/connection/README.md` gains a 'Callback ownership' section describing how to store and upgrade the reference for the duration of a callback.

Features

  • Added — WeakConnection non-owning connection handle: Provides a cloneable reference to a connection that does not keep it alive, allowing served objects to issue callbacks without creating an ownership cycle that would keep the object, transport and well-known name alive after the external owner drops. (crates/tinybus/src/connection.rs)
  • Added — Connection::downgrade(): Lets an owning handle produce a WeakConnection by weakly referencing both the inner state and the CloseOnDrop lifetime guard, so the reference carries no ownership of either. (crates/tinybus/src/connection.rs)
  • Added — WeakConnection::upgrade(): Returns an owning Connection only while the lifetime guard still has a strong owner; acquiring the guard before the inner state prevents residual dispatch state from resurrecting a connection whose last owning handle is gone, while a still-owned but explicitly closed connection remains upgradable and behaves like its owning handles. (crates/tinybus/src/connection.rs)
  • Modified — Crate-root re-export of WeakConnection: Makes the new non-owning handle reachable from the crate root alongside Connection for downstream users. (crates/tinybus/src/lib.rs)
  • Added — Callback ownership documentation: Documents that a served object should store a downgraded reference and upgrade it only for the duration of each callback, explaining the cycle created by storing a strong clone and stating that upgrade returns None after the last strong owner drops and does not recreate a dropped lifetime guard. (docs/modules/connection/README.md)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

  • Unreviewed: tinysweeper/tests

Findings

No active actionable findings.

Could not review: crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs, crates/tinybus/src/lib.rs, tinysweeper/tests

Before merge

  • Complete the critique review for crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs, crates/tinybus/src/lib.rs.
  • Complete the tests review for tinysweeper/tests.

How this fits together

flowchart LR
  n0["...on_but_preserves_an_already_admitted_call<br/>changed"]:::changed
  n1["clone"]:::impacted
  n2["serve_at"]:::impacted
  n3["call"]:::impacted
  n4["path"]:::impacted
  n5["...an_register_an_interface_without_deadlock"]:::impacted
  n6["call_raw"]:::impacted
  n0 -->|calls| n1
  n0 -->|tests| n1
  n0 -->|calls| n2
  n0 -->|tests| n2
  n0 -->|calls| n3
  n0 -->|tests| n3
  n0 -->|calls| n4
  n0 -->|tests| n4
  n0 -->|calls| n6
  n0 -->|tests| n6
  n3 -->|calls| n2
  n3 -->|calls| n4
  n5 -->|calls| n1
  n5 -->|tests| n1
  n5 -->|calls| n2
  n5 -->|tests| n2
  n5 -->|calls| n3
  n5 -->|tests| n3
  n5 -->|calls| n4
  n5 -->|tests| n4
  n5 -->|calls| n6
  n5 -->|tests| n6
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading
Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: incomplete; unanswered: crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs, crates/tinybus/src/lib.rs
  • Positive: The added documentation section on callback ownership is described as consistent with the code: `Connection::downgrade()` returns a `WeakConnection` held weakly on both `inner` and `_close`, and `WeakConnection::upgrade()` upgrades the lifetime guard first and returns `None` once the last strong owner drops, while still allowing an explicitly closed-but-owned connection to be upgraded.
  • Lane summary: The added documentation section on callback ownership is consistent with the code it describes: `Connection::downgrade()` returns a `WeakConnection` held weakly on both `inner` and `_close`, and `WeakConnection::upgrade()` upgrades the lifetime guard first and returns `None` once the last strong owner drops, while still allowing an explicitly closed-but-owned connection to be upgraded. The repository rules constrain Markdown line count, and the file stays short; nothing in this diff introduces a defect. 3 files could not be reviewed: crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs, crates/tinybus/src/lib.rs.

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: `upgrade` deliberately upgrades the `CloseOnDrop` guard before the `Inner`, which prevents a dispatch task's residual `Arc<Inner>` from resurrecting a connection whose last owning handle is gone; the returned handle holds the same guard, so it cannot outlive the connection or leak it.
  • Positive: No untrusted input, network, subprocess, deserialization, authorisation or dependency change is involved, and the new public items carry doc comments and sit in the existing test sibling file.
  • Lane summary: Reviewed the new `WeakConnection` handle and its two tests. `upgrade` deliberately upgrades the `CloseOnDrop` guard before the `Inner`, which is what stops a dispatch task's residual `Arc<Inner>` from resurrecting a connection whose last owning handle is gone; the returned handle holds the same guard, so it cannot outlive the connection or leak it. No untrusted input, network, subprocess, deserialization, authorisation or dependency change is involved, and the new public items carry doc comments and sit in the existing `connection_tests.rs` sibling file per the repository's test rules. I found no defect; I did not execute the tests. 1 file was not security-reviewed: docs/modules/connection/README.md (prose or tabular data).

tests

  • Conclusion: Neutral
  • Scope reviewed: incomplete; unanswered: tinysweeper/tests
  • Lane summary: No reviewer produced a usable response.

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Positive: Nothing sensitive was found in what this pull request commits.
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The description matches the diff: it adds `WeakConnection`/`Connection::downgrade()` with an upgrade that acquires the `CloseOnDrop` lifetime guard first, plus matching tests, re-export, and docs, and no defect was found in the new code.
  • Lane summary: Adds WeakConnection/Connection::downgrade with an upgrade that acquires the CloseOnDrop lifetime guard first, plus matching tests, re-export, and docs. The description matches the diff and I found no defect in the new code.
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.005357
  • Tokens: 196236 input · 48139 output · 32256 cached · 336 embedding
Head State Pass summary
10579d486f27 incomplete 0 active finding(s), 0 resolved finding(s) (at 2026-10-12T03:12:55Z)

tinysweeper 0.1.2

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking, but could not review everything, so this is not an approval: crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs, crates/tinybus/src/lib.rs, tinysweeper/tests.

             $0.0054 · 196,236 in / 48,139 out · 32,256 cached (16%) · ladder/vectors-oai3, deepseek-v4.1-flash · 336 embedded
critique:    $0.0004 · 35,263 in  / 6,622 out  · 4,224 cached (12%)  · deepseek-v4.1-flash
security:    $0.0036 · 110,998 in / 30,604 out · 23,552 cached (21%) · deepseek-v4.1-flash
tests:       $0.0009 · 15,700 in  / 5,368 out  · 0 cached (0%)       · deepseek-v4.1-flash
description: $0.0002 · 30,326 in  / 2,844 out  · 4,480 cached (15%)  · deepseek-v4.1-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant