Merge pull request 'fix: CI flakes in render coordinator readiness and quicnettest event order' (#16) from fix/ci-race-and-proxy-event-order into main
2 files · +20 -17
M internal/adapters/quicnettest/proxy.go +11 -8
| @@ -468,6 +468,17 @@ func (p *Proxy) ingest(toServer bool, raw []byte, dst *net.UDPAddr) { | |||
| 468 | 468 | counters.reordered.Add(1) | |
| 469 | 469 | } | |
| 470 | 470 | due := now.Add(link.BaseLatency + decision.ExtraDelay) | |
| 471 | + | // Emit before scheduling: once enqueued, a zero-latency copy can be | |
| 472 | + | // delivered and answered before this call returns, and the reply's event | |
| 473 | + | // must not be observed ahead of the packet that caused it. | |
| 474 | + | p.emit(Event{ | |
| 475 | + | Direction: directionOf(toServer), | |
| 476 | + | Sequence: seq, | |
| 477 | + | Bytes: len(raw), | |
| 478 | + | At: now, | |
| 479 | + | Due: due, | |
| 480 | + | Decision: decision, | |
| 481 | + | }) | |
| 471 | 482 | for copyIndex := 0; copyIndex <= decision.Duplicates; copyIndex++ { | |
| 472 | 483 | copyDue := due.Add(time.Duration(copyIndex) * link.DuplicateDelay) | |
| 473 | 484 | if !p.enqueue(packet{ | |
| @@ -480,14 +491,6 @@ func (p *Proxy) ingest(toServer bool, raw []byte, dst *net.UDPAddr) { | |||
| 480 | 491 | counters.overflowDrops.Add(1) | |
| 481 | 492 | } | |
| 482 | 493 | } | |
| 483 | - | p.emit(Event{ | |
| 484 | - | Direction: directionOf(toServer), | |
| 485 | - | Sequence: seq, | |
| 486 | - | Bytes: len(raw), | |
| 487 | - | At: now, | |
| 488 | - | Due: due, | |
| 489 | - | Decision: decision, | |
| 490 | - | }) | |
| 491 | 494 | } | |
| 492 | 495 | ||
| 493 | 496 | func (p *Proxy) enqueue(pkt packet) bool { | |
| @@ -468,6 +468,17 @@ func (p *Proxy) ingest(toServer bool, raw []byte, dst *net.UDPAddr) { | |||||
| 468 | counters.reordered.Add(1) | 468 | counters.reordered.Add(1) | ||
| 469 | } | 469 | } | ||
| 470 | due := now.Add(link.BaseLatency + decision.ExtraDelay) | 470 | due := now.Add(link.BaseLatency + decision.ExtraDelay) | ||
| 471 | + | // Emit before scheduling: once enqueued, a zero-latency copy can be | |||
| 472 | + | // delivered and answered before this call returns, and the reply's event | |||
| 473 | + | // must not be observed ahead of the packet that caused it. | |||
| 474 | + | p.emit(Event{ | |||
| 475 | + | Direction: directionOf(toServer), | |||
| 476 | + | Sequence: seq, | |||
| 477 | + | Bytes: len(raw), | |||
| 478 | + | At: now, | |||
| 479 | + | Due: due, | |||
| 480 | + | Decision: decision, | |||
| 481 | + | }) | |||
| 471 | for copyIndex := 0; copyIndex <= decision.Duplicates; copyIndex++ { | 482 | for copyIndex := 0; copyIndex <= decision.Duplicates; copyIndex++ { | ||
| 472 | copyDue := due.Add(time.Duration(copyIndex) * link.DuplicateDelay) | 483 | copyDue := due.Add(time.Duration(copyIndex) * link.DuplicateDelay) | ||
| 473 | if !p.enqueue(packet{ | 484 | if !p.enqueue(packet{ | ||
| @@ -480,14 +491,6 @@ func (p *Proxy) ingest(toServer bool, raw []byte, dst *net.UDPAddr) { | |||||
| 480 | counters.overflowDrops.Add(1) | 491 | counters.overflowDrops.Add(1) | ||
| 481 | } | 492 | } | ||
| 482 | } | 493 | } | ||
| 483 | - | p.emit(Event{ | |||
| 484 | - | Direction: directionOf(toServer), | |||
| 485 | - | Sequence: seq, | |||
| 486 | - | Bytes: len(raw), | |||
| 487 | - | At: now, | |||
| 488 | - | Due: due, | |||
| 489 | - | Decision: decision, | |||
| 490 | - | }) | |||
| 491 | } | 494 | } | ||
| 492 | 495 | ||||
| 493 | func (p *Proxy) enqueue(pkt packet) bool { | 496 | func (p *Proxy) enqueue(pkt packet) bool { | ||
M internal/usecase/daemon/render_coordinator.go +9 -9
| @@ -1005,21 +1005,21 @@ func (c *renderCoordinator) fireWithTimerTokenAndLease(token *timerToken, gen ui | |||
| 1005 | 1005 | if c.pendingGeneration == 0 { | |
| 1006 | 1006 | c.pendingGeneration = 1 | |
| 1007 | 1007 | } | |
| 1008 | - | leases := make(map[*attachedClient]*attachmentLease, len(c.leases)) | |
| 1008 | + | // lease.ready is guarded by c.mu (markAttachmentReady writes it), so only | |
| 1009 | + | // leases already ready under the lock are probed after unlocking. | |
| 1010 | + | probe := make(map[*attachedClient]*attachmentLease, len(c.leases)) | |
| 1009 | 1011 | for ac, lease := range c.leases { | |
| 1010 | - | if lease != nil && lease.active { | |
| 1011 | - | leases[ac] = lease | |
| 1012 | + | if lease != nil && lease.active && lease.ready { | |
| 1013 | + | probe[ac] = lease | |
| 1012 | 1014 | } | |
| 1013 | 1015 | } | |
| 1014 | 1016 | c.mu.Unlock() | |
| 1015 | 1017 | ||
| 1016 | - | ready := make(map[*attachmentLease]bool, len(leases)) | |
| 1017 | - | for ac, lease := range leases { | |
| 1018 | + | ready := make(map[*attachmentLease]bool, len(probe)) | |
| 1019 | + | for ac, lease := range probe { | |
| 1018 | 1020 | // Welcome holds attachment.sendMu while its transport Send is in | |
| 1019 | 1021 | // flight. Do not probe an incarnation before it is ready. | |
| 1020 | - | if lease.ready { | |
| 1021 | - | ready[lease] = c.attachmentAckReady(ac) | |
| 1022 | - | } | |
| 1022 | + | ready[lease] = c.attachmentAckReady(ac) | |
| 1023 | 1023 | } | |
| 1024 | 1024 | ||
| 1025 | 1025 | // The readiness callback can detach, replace, or publish a sync batch. | |
| @@ -1041,7 +1041,7 @@ func (c *renderCoordinator) fireWithTimerTokenAndLease(token *timerToken, gen ui | |||
| 1041 | 1041 | ||
| 1042 | 1042 | // Re-snapshot after every external readiness probe. A replacement may have | |
| 1043 | 1043 | // retired one of the leases we probed while the callback was unlocked. | |
| 1044 | - | leases = make(map[*attachedClient]*attachmentLease, len(c.leases)) | |
| 1044 | + | leases := make(map[*attachedClient]*attachmentLease, len(c.leases)) | |
| 1045 | 1045 | for ac, lease := range c.leases { | |
| 1046 | 1046 | if lease != nil && lease.active { | |
| 1047 | 1047 | leases[ac] = lease | |
| @@ -1005,21 +1005,21 @@ func (c *renderCoordinator) fireWithTimerTokenAndLease(token *timerToken, gen ui | |||||
| 1005 | if c.pendingGeneration == 0 { | 1005 | if c.pendingGeneration == 0 { | ||
| 1006 | c.pendingGeneration = 1 | 1006 | c.pendingGeneration = 1 | ||
| 1007 | } | 1007 | } | ||
| 1008 | - | leases := make(map[*attachedClient]*attachmentLease, len(c.leases)) | 1008 | + | // lease.ready is guarded by c.mu (markAttachmentReady writes it), so only |
| 1009 | + | // leases already ready under the lock are probed after unlocking. | |||
| 1010 | + | probe := make(map[*attachedClient]*attachmentLease, len(c.leases)) | |||
| 1009 | for ac, lease := range c.leases { | 1011 | for ac, lease := range c.leases { | ||
| 1010 | - | if lease != nil && lease.active { | 1012 | + | if lease != nil && lease.active && lease.ready { |
| 1011 | - | leases[ac] = lease | 1013 | + | probe[ac] = lease |
| 1012 | } | 1014 | } | ||
| 1013 | } | 1015 | } | ||
| 1014 | c.mu.Unlock() | 1016 | c.mu.Unlock() | ||
| 1015 | 1017 | ||||
| 1016 | - | ready := make(map[*attachmentLease]bool, len(leases)) | 1018 | + | ready := make(map[*attachmentLease]bool, len(probe)) |
| 1017 | - | for ac, lease := range leases { | 1019 | + | for ac, lease := range probe { |
| 1018 | // Welcome holds attachment.sendMu while its transport Send is in | 1020 | // Welcome holds attachment.sendMu while its transport Send is in | ||
| 1019 | // flight. Do not probe an incarnation before it is ready. | 1021 | // flight. Do not probe an incarnation before it is ready. | ||
| 1020 | - | if lease.ready { | 1022 | + | ready[lease] = c.attachmentAckReady(ac) |
| 1021 | - | ready[lease] = c.attachmentAckReady(ac) | |||
| 1022 | - | } | |||
| 1023 | } | 1023 | } | ||
| 1024 | 1024 | ||||
| 1025 | // The readiness callback can detach, replace, or publish a sync batch. | 1025 | // The readiness callback can detach, replace, or publish a sync batch. | ||
| @@ -1041,7 +1041,7 @@ func (c *renderCoordinator) fireWithTimerTokenAndLease(token *timerToken, gen ui | |||||
| 1041 | 1041 | ||||
| 1042 | // Re-snapshot after every external readiness probe. A replacement may have | 1042 | // Re-snapshot after every external readiness probe. A replacement may have | ||
| 1043 | // retired one of the leases we probed while the callback was unlocked. | 1043 | // retired one of the leases we probed while the callback was unlocked. | ||
| 1044 | - | leases = make(map[*attachedClient]*attachmentLease, len(c.leases)) | 1044 | + | leases := make(map[*attachedClient]*attachmentLease, len(c.leases)) |
| 1045 | for ac, lease := range c.leases { | 1045 | for ac, lease := range c.leases { | ||
| 1046 | if lease != nil && lease.active { | 1046 | if lease != nil && lease.active { | ||
| 1047 | leases[ac] = lease | 1047 | leases[ac] = lease | ||