Skip to content

Release object tree lock before awaiting dispatch - #53

Merged
senamakel merged 2 commits into
mainfrom
release-dispatch-lock-before-await
Oct 11, 2026
Merged

senamakel merged 2 commits into
mainfrom
release-dispatch-lock-before-await

Conversation

@senamakel

@senamakel senamakel commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Async method dispatch kept the connection's object-tree read guard alive while awaiting the service method. A method that served or removed an object on that same connection then waited forever for the write lock. Dispatch now resolves and retains the selected interface under the read lock, releases the guard, and performs member/confidentiality checks and the call afterward.

The regression uses a real MemoryBus proxy to serve a second object from a method, call it, remove it from another method, and verify the removed path returns the existing wire-level UnknownObject error.

Validation: TinyBus library tests pass (274/274); the focused mutation regression passes. The same regression failed before the fix with a 250 ms timeout at Serve. cargo fmt --all -- --check and git diff --check pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved service-call handling when available objects change during request processing. Newly exported objects can respond to calls, while calls to removed objects return an unknown-object response.
    • Object updates are less likely to interfere with requests already being processed.

Co-authored-by: Medulla <medulla@tinyhumans.ai>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b41cd5d9-9def-41f3-bfd3-bea70875538e

📥 Commits

Reviewing files that changed from the base of the PR and between 6b54201 and a8851e1.


📒 Files selected for processing (3)
  • crates/tinybus/src/connection.rs
  • crates/tinybus/src/connection_tests.rs
  • crates/tinybus/src/service/tree.rs

 _______________________________________________
< Edge cases are just center cases in disguise. >
 -----------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

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

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

…se-bus

# Conflicts:
#	crates/tinybus/src/connection.rs
#	crates/tinybus/src/service/tree.rs
@senamakel
senamakel merged commit 9062dd8 into main Oct 11, 2026
7 checks passed
@tinysweeper

tinysweeper Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 5 lane(s) and found 0 active actionable finding(s). The pull request scopes the object-tree read guard in the dispatch path so it is released before `ObjectTree::invoke` is awaited, letting method callbacks mutate the tree (serve or unserve paths) while another call is in flight; a new in-crate regression test covers serving and unserving during a call. All lanes judged the change safe to merge, with no unanswered items.

State: Incomplete
Priority: none
Reviewed head: 12fdcd86bc9e
Updated: 2026-10-12T18:01:35Z

Review snapshot

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

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

Features

  • Modified — Release object tree read guard before invoking method handlers: Dispatch no longer holds the object-tree read guard across the `ObjectTree::invoke` await. The same admission snapshot is taken under the lock, so dispatch behaviour is otherwise unchanged, but method callbacks can now serve or unserve interfaces on the same connection while another call is parked. No new input-dependent behaviour or sink was introduced, and no repository rule was broken; the security lane judged this safe to merge. (crates/tinybus/src/connection.rs#async fn dispatch(inner: &Inner, header: &Header, body: Value, brokered: bool) -, crates/tinybus/src/connection.rs)

Tests

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

Findings

No active actionable findings.

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

Before merge

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

How this fits together

flowchart LR
  n0["brokered_pair<br/>changed"]:::changed
  n1["clone"]:::impacted
  n2["call"]:::impacted
  n3["path"]:::impacted
  n4["serve_at"]:::impacted
  n5["pair"]:::impacted
  n0 -->|calls| n1
  n0 -->|calls| n3
  n0 -->|calls| n4
  n2 -->|calls| n1
  n2 -->|calls| n3
  n2 -->|calls| n4
  n5 -->|calls| n3
  n5 -->|calls| n4
  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: Neutral
  • Scope reviewed: incomplete; unanswered: crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs
  • Lane summary: Reviewed 0 files; 0 findings. 2 files could not be reviewed: crates/tinybus/src/connection.rs, crates/tinybus/src/connection_tests.rs.

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The edit preserves the same admission snapshot as before and introduces nothing input-dependent, no new sink, and no repository rule violation.
  • Lane summary: The connection.rs edit only makes the object-tree read guard's scope explicit, dropping it before `ObjectTree::invoke` runs — the same admission snapshot the previous one-liner took — so no dispatch behaviour changes. The new in-crate test exercises serving and unserving a path from inside a method call on the same connection, uses the in-memory bus as the repository requires, and its assertions match the existing error-reply handling (a named `MethodFailed`). Nothing reachable-input-dependent, no new sink, and no repository rule broken; safe to merge.

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The new test follows the repository's in-crate `connection_tests.rs` convention, uses the in-memory transport as the repository requires, and bounds the call with a 250 ms deadline so a tree lock held across user code would surface as a failure rather than a hang.
  • Lane summary: The production edit scopes the object-tree read guard in `dispatch` so it is dropped before `ObjectTree::invoke` is awaited, and the new test drives `serve_at`/`unserve` from inside a method call: it asserts the freshly exported object answers at the extra path and that the removed object then fails with `ai.tinyhumans.tinybus.Error.UnknownObject`. Both assertions are real observable behaviour, the call is bounded by a 250 ms deadline so a tree lock held across user code surfaces as a failure rather than a hang, and the test follows the repository's in-crate `connection_tests.rs` convention on the in-memory transport. Nothing here looks unsafe to merge.

commits

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

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The fix does what the description says: the old `let target = inner.objects.read().await.lookup(path, interface)?;` held the read guard for the whole statement scope (lifetime-extended through the `?`), so it stayed alive across the `await` in `ObjectTree::invoke`; the new block drops the guard after `lookup` returns the `Arc<dyn Interface>`, and the member/confidentiality checks in `invoke` now run without the lock. The regression test uses the in-memory transport in `connection_tests.rs`, matching the repository's test rules, and its assertions (serve during a call, call the extra object, unserve, expect the wire-level UnknownObject error) match the described behaviour. No defects found.
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash, glm-5.3-flash
  • Spend: $0.003800
  • Tokens: 217304 input · 53260 output · 77184 cached · 165 embedding
Head State Pass summary
12fdcd86bc9e ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-12T17:48:19Z)
12fdcd86bc9e incomplete 0 active finding(s), 0 resolved finding(s) (at 2026-10-12T18:01:35Z)

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. Approving.

             $0.0053 · 290,789 in / 55,927 out · 21,568 cached (7%) · ladder/vectors-oai3, deepseek-v4.1-flash, glm-5.3-flash · 165 embedded
critique:    $0.0016 · 132,860 in / 11,971 out · 12,608 cached (9%) · deepseek-v4.1-flash, glm-5.3-flash
security:    $0.0010 · 82,338 in  / 5,555 out  · 1,792 cached (2%)  · glm-5.3-flash, deepseek-v4.1-flash
tests:       $0.0013 · 48,507 in  / 19,477 out · 7,168 cached (15%) · deepseek-v4.1-flash
description: $0.0012 · 24,313 in  / 15,516 out · 0 cached (0%)      · 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

@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.

             $0.0038 · 217,304 in / 53,260 out · 77,184 cached (36%) · ladder/vectors-oai3, deepseek-v4.1-flash, glm-5.3-flash · 165 embedded
critique:    $0.0008 · 42,584 in  / 11,908 out · 10,240 cached (24%) · deepseek-v4.1-flash
security:    $0.0012 · 106,892 in / 14,244 out · 53,632 cached (50%) · deepseek-v4.1-flash
tests:       $0.0014 · 50,974 in  / 21,595 out · 13,312 cached (26%) · deepseek-v4.1-flash
description: $0.0003 · 13,978 in  / 4,291 out  · 0 cached (0%)       · glm-5.3-flash, deepseek-v4.1-flash

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