Skip to content

[R35] Preserve outgoing lifecycle queues across callbacks - #41

Open
JetF0x wants to merge 1 commit into
Carsillas:rewritefrom
JetF0x:fix/review-r35
Open

JetF0x wants to merge 1 commit into
Carsillas:rewritefrom
JetF0x:fix/review-r35

Conversation

@JetF0x

@JetF0x JetF0x commented Sep 12, 2026

Copy link
Copy Markdown

Addresses review finding R35.

Deleting a related entity from EntityUnobserved, or transferring another entity from EntityOwnershipLost, modifies the outgoing collection while Tick() is enumerating it. The next iterator advance throws outside event exception handling, leaving the tick partially processed.

Capture deletion and ownership-transfer batches before outgoing lifecycle callbacks run. Consume each successful send before raising its event, retaining failed and unsent requests. New callback requests wait for the next tick, including transfers added during deletion callbacks. Existing requests retain their outgoing stage and transfer order. A live request identity check skips canceled or replaced transfers; replacing a request with identical entity and destination ids still creates new work for the next tick. The queues are no longer cleared after callbacks.

The deletion queue uses ReferenceEqualityComparer.Instance so its new live lookup and per-item removal remain valid when replicated values change after Delete(), including changes from an earlier deletion callback. This is the exact one-line comparer change shared with R2/#6; the other R2 identity sets are outside this change.

Validation

  • Re-ran the original baseline on 37f4e1ffb1d7fc3e7bcdea3dfbbd4484f9fe7516: 228/228 passed (155 runtime, 53 analyzer, 20 generator).
  • Before production edits, all four initial callback regressions failed on that baseline with InvalidOperationException from the outgoing HashSet/List enumerator. These include callbacks that throw after queuing more work.
  • The batching patch with the old deletion comparer exposed two missed-delete regressions: mutating the queued entity before a tick, and mutating other queued entities from a deletion callback. Both failed while the unchanged-value control passed. All three pass with the narrow reference-comparer change.
  • Final focused run: 19/19 passed. Recording transports verify packet contents, order, later-tick delivery without duplicates, reference-correct events, callback exception isolation, cancellation, same/different-destination replacement, canceled callback-created spawns, and send failures before any send or after a successful prefix.
  • Final full Release solution: 247/247 passed (174 runtime, 53 analyzer, 20 generator). git diff --check passed. Tested on Windows with SDK 10.0.400 and .NET 8 runtime; the existing MemoryRelayTransport.cs:21 CS8600 warning remains. The sandboxed restore hit NuGet TLS authentication failure; the authorized normal-TLS restore succeeded without changing dependencies or validation settings.
dotnet test Cat.Network.Test/Cat.Network.Test.csproj --configuration Release --no-restore --filter FullyQualifiedName~RelayClientOutgoing_
dotnet test Cat.Network.sln --configuration Release --no-restore

Compatibility and integration

  • No public API or wire-format changes. The transfer-request identity class is private. Callback-added or replaced deletes/transfers are intentionally deferred to the next tick; cancellation still suppresses an existing unsent transfer. A send that throws leaves its operation and the unsent remainder queued, while completed sends are not replayed by queue cleanup.
  • R4 / [R4] Reset relay client sessions on disconnect and reconnect #11 overlaps ProcessOutgoingMessages. When combining, retain its sessionVersion parameter, every TrySendSessionMessage call and false-return path, and every IsCurrentSession check. Remove a successful item only after TrySendSessionMessage returns true and before UnregisterEntity/RemoveOwnership; preserve the session check immediately after those callbacks. Keep the update, spawn, and rented-writer session guards and the session teardown queue clearing. A stale batch must never continue into a replacement session.
  • R2 / [R2] Fix relay identity sets for mutable entities and profiles #6 shares the deletion comparer line. Keep the identical reference comparer when resolving it. It is necessary here because a mutable value-based hash would make the new lookup/removal skip or retain a queued deletion. The other client/server identity sets and their mutable-hash bookkeeping still need [R2] Fix relay identity sets for mutable entities and profiles #6; this PR does not replace that fix.
  • R5 / [R5] Fix lifecycle callbacks that change subscriptions #8 remains separate: no event subscriber-list changes are included. R33 / [R33] Retain application packets during the memory relay handshake #29 still handles the memory-daemon handshake; these focused cases use recording transports. R34's Spawn validation edits the same partial class but a different entry method and should be retained when combining.

Validation covers this independent branch and synchronous relay API callbacks. Combined PR execution, hosted CI, concurrent use, and arbitrary transport reentrancy were not tested. Retry checks use a transport that throws before accepting a packet; they do not imply acknowledged delivery when a transport fails after accepting bytes.

Base: 37f4e1ffb1d7fc3e7bcdea3dfbbd4484f9fe7516. Commit: fda108b8d848c8cfce4c9af736bc59916b74420f on fix/review-r35.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant