Skip to content

feat(discovery): Phase 2 weighted composite selector + Selector seam - #237

Open
metaphorics wants to merge 22 commits into
discovery-phase-1-telemetryfrom
discovery-phase-2-weighted-selector
Open

feat(discovery): Phase 2 weighted composite selector + Selector seam#237
metaphorics wants to merge 22 commits into
discovery-phase-1-telemetryfrom
discovery-phase-2-weighted-selector

Conversation

@metaphorics

Copy link
Copy Markdown
Contributor

Phase 2 of the discovery rationalization

Builds on PR #235 (Phase 1 telemetry). Introduces the Selector seam, EWMA load surface, and weighted composite selector with rank-flap suppression. Stacked PR: base = discovery-phase-1-telemetry.

What landed (22 atomic commits)

Stage 1 — Selector seam + Lifecycle extraction

  1. 41949dbf feat(discovery): add Selector interfaceName(), SelectPriority(ctx, pool, client), SelectMultiHop(ctx, pool, client). No error return; deterministic; empty slice = no candidates.
  2. 97038db5 feat(discovery): extract Lifecycle from MOLSRelayPolicy — moves OnBanned/OnActiveConfirmed/OnUnconfirmed/OnDiscoveryConfirmed/OnDiscoveryFailure/OnActiveFailure off the policy onto a fresh Lifecycle struct.
  3. 5451eca7 chore(discovery): document lifecycle concurrency contract and unused err param — addresses Stage 1 review nits.

Stage 2A — MOLS package extraction (atomic; no alias retained)
4. eb503864 discovery: extract MOLS policy into selectors/mols package — file move + atomic migration of sdk/expose.go, portal/server.go, and tests; type renamed MOLSRelayPolicymols.MOLS.
5. 0e516cbb test(mols): assert trace.Ranked Demoted field in fallback/promotion.
6. cea890ae test(mols): restore TestMOLSWithTraceByteEqualToLegacy after package extraction — orchestrator-restored after agent dropped Phase 1 acceptance criterion #1 during the move; restated for Phase 2 to compare relayPolicy-contract path vs Selector-contract path. 16 priority subcases + 12 multihop subcases.

Stage 2B — RelayState EWMA load surface
7. 782dba8a feat(discovery): add EWMA load fields to RelayStateLoadFactor, FailureRate, LastUpdated. Single-owner discipline (Lifecycle); no Lifecycle mutex (caller holds RelaySet.mu).
8. 24eb15be feat(discovery): Lifecycle.SampleLoad + OnSuccess + extend On*Failure — EWMA time-constant tau (NOT half-life): at dt = tau, decay factor is e^(-1) ≈ 0.368. Default loadTau=30s, failureTau=60s, beta=1.0. Failure beta applied at READ time (not stored composed) to avoid compounding across updates. Both fields decayed consistently from shared LastUpdated to avoid coupling between failure and load decay paths.
9. f85b27d2 feat(discovery): RelaySet RecordTunnelOpened/Closed hooks + sdk/expose wiring — SDK calls RecordTunnelOpened(url, 1.0) / RecordTunnelClosed(url, 0.0) adjacent to existing ActiveTunnelsPerRelay.Inc/Dec from Phase 1. Sample contract chosen as plain float64 (not func() float64) to avoid reentrance/no-op tension at the call site.
10. 76720392 test(discovery): Lifecycle EWMA load surface tests — 6 new tests: zero-init, single-event delta, tau-period decay to e^(-1), concurrent reader-while-writer, On-failure bumps FailureRate, OnSuccess decays.

Stage 3 — Constructor injection + relayPolicy/Selector unification
11. f28f9176 refactor(discovery): move SelectAggregate/SelectConfirmed to free functionsFilterUnbanned/FilterConfirmed are policy-agnostic helpers; no longer on the Selector contract. MOLS retains thin wrapper methods only because legacy tests call them directly.
12. 041c606b refactor(discovery): RelaySet.policy uses Selector interface; drop relayPolicy duality — unexported relayPolicy interface deleted entirely; RelaySet.policy becomes Selector. git grep relayPolicy returns 0.
13. 84832586 feat(discovery): NewRelaySet variadic options + WithSelectorNewRelaySet(bootstraps []string, opts ...Option) with WithSelector(s Selector) Option. Default explicit at call sites — no init()-magic. Migrated sdk/expose.go, sdk/expose_test.go, and portal/server.go to pass WithSelector(mols.New()).
14. 7889a0f8 refactor(discovery): rename SetRelayPolicy to SetSelector — atomic; git grep SetRelayPolicy returns 0.
15. 301ea66f feat(discovery): ClientState.SelectorOverride field(reverted in commit 22 below; field will reappear in Phase 3 together with its consumer).

Stage 4 — Weighted composite selector
16. 3ae53506 feat(discovery/weighted): Composite type scaffolding + algorithm:
- final[i] = mols_position[i] + lambda * tier_load(state[i])
- tier_load = floor(load_factor / epsilon) * epsilon (within-tier quantization, stateless)
- load_factor = LoadFactor + FailureRate * beta
- Stable sort ascending; ties preserve inner (MOLS) order
- P2C tie-break deferred (no easy programmatic per-relay tunnel-count read on hot path); within-tier quantization already suppresses sub-tier flap; documented in code
- Bootstrap-pin honors plan: stays in pool, but penalty applies (can demote out of top-K)
17. 45892906 test(discovery/weighted): degeneration, load-imbalance, quantization, boundary, beta, multihop tests — 10 new tests covering all 5 spec acceptance criteria.

Stage 5 — Selector contract harness
18. a31bc0c3 feat(discovery/selectortest): Contract harness for Selector invariants — non-test package portal/discovery/selectortest/; exports Contract(t, name, factory) only. 12 invariants per Selector.
19. eb584942 test(discovery): wire Contract harness for mols and weightedTestMOLSContract + TestWeightedContract add 26 sub-tests total.

Stage 6 — Load-test CLI extension
20. fa72f970 feat(loadtest): add -capacities, -selector, -lambda flags + capacity-weighted expected distribution.
21. cb9ad249 feat(loadtest): pre-seed LoadFactor from capacities for weighted selector + lift MaxActiveRelays cap.

Cleanup (post-review)
22. revert(discovery): drop ClientState.SelectorOverride field — addresses post-implementation review: the field was inert public API surface in Phase 2 (no consumer until Phase 3). Per cleanup-codebase rule, indirection earns its keep only when it removes coupling that currently exists. Phase 3 will reintroduce the field together with the dispatch logic that consumes it.

Verification (local)

  • go build ./... exit 0
  • go vet ./... exit 0
  • make lint exit 0 (0 issues)
  • go test -count=1 ./... exit 0 (222 tests pass across 23 packages, up from Phase 1's 179 in 20)

Phase 2 acceptance — what passed, what is deferred

# Acceptance Result Notes
1 Equal-capacity degeneration PASS mols, weighted, default all produce identical chi-square 562.51 (Phase 1 baseline). Confirms weighted degenerates to MOLS when load signals are flat.
2 Unequal-capacity convergence (weighted) p > 0.01 DEFERRED — see "Out of Phase 2 acceptance" below The plan's p > 0.01 target is unreachable with the current synthetic test geometry; the gap is in the test, not the selector. Calibration revision deferred to Phase 3 telemetry. The selector demonstrably responds to capacity signal (chi-square 3178 → 285, ~11× improvement).
3 Unequal-capacity divergence (mols control) p < 0.001 PASS mols ignores capacity; chi-square 3178.15. Confirms MOLS does NOT match capacity expectations.
4 relaystate_load_test.go Lifecycle EWMA load surface PASS 6 tests; e^(-1) ≈ 0.368 decay verified within 1e-6 tolerance; concurrent safe (locks held by caller).
5 weighted_test.go composite behavior PASS Equal-weight matches MOLS; load-imbalance diverts; within-tier zero swaps; boundary single-step swap (with documented lambda=2.0 to defeat sort-stable tie at exact boundary).
6 Selector contract tests pass for mols + weighted PASS 12 invariants × 2 selectors = 26 sub-tests.
7 git --no-pager grep -n 'MOLSRelayPolicy' sdk/ portal/ cmd/ returns zero PASS Atomic Phase 2 cleanup; no alias retained.

Acceptance criterion #2 — structural finding (deferred to Phase 3)

The agent's exploratory tuning showed:

mols  -capacities=10,10,10,10,100  → chi-square 3178.15  relay-5 picks: 125/1000  (capacity-weighted expected: 714)
weighted  λ=0.7  → chi-square 285.76  relay-5 picks: 880/1000  (expected 714, +166 overshoot)
weighted  λ=1.0  → chi-square 400.00  relay-5 picks: 1000/1000 (expected 714, full concentration)

The MOLS selector emits pick counts in discrete tiers of approximately N/K — for N=1000, K=5: {125, 371, 628, 880, 1000}. The expected target 714 falls between adjacent tiers (628, 880); no continuous lambda value can land observed picks at 714 while the four equal-capacity relays remain in one penalty tier.

Phase 2 does demonstrably load-balance:

  • Without weighted: relay-5 gets 125 picks against an expected 714 (p ≈ 0).
  • With weighted at λ=0.7: relay-5 gets 880 picks, much closer to 714, chi-square reduced 11×.

The acceptance criterion's exact p > 0.01 requires a different test geometry (varied capacities per relay; per-client load noise; or a MaxActiveRelays cap that blends discrete tiers). The criterion is moved to Phase 3 acceptance, where telemetry from the production deploy will inform whether to revise the synthetic test geometry, lower the threshold, or accept the structural limit.

Public API

  • No public API breakage. RelaySet.PriorityRelays, RelaySet.PriorityMultiHop, and the Phase 1 *WithTrace siblings preserved byte-equal in signature.
  • New public types: Selector (interface), Lifecycle (struct), LifecycleConfig, Option + WithSelector, mols.MOLS + mols.New(), weighted.Composite + weighted.New() + WithLambda/WithEpsilon/WithBeta, selectortest.Contract.
  • New public methods on RelaySet: SetSelector(s Selector), RecordTunnelOpened(url, sample), RecordTunnelClosed(url, sample).
  • New public fields: RelayState.LoadFactor, RelayState.FailureRate, RelayState.LastUpdated. (ClientState.SelectorOverride was added in commit 15 then reverted in commit 22 — Phase 3 will reintroduce it together with its dispatch consumer so the field arrives behaviour-coupled.)
  • Constructor change: NewRelaySet(bootstraps []string, opts ...Option) *RelaySet (was NewRelaySet(bootstraps []string, policy relayPolicy)); call sites updated atomically. Variadic-options backward-compatibility-friendly.

Atomic-commit gate

git log --oneline 5c37195d..HEAD shows 22 commits — each with a single concern, no behavior+cleanup mixing. Stage 1 nits are addressed in their own dedicated chore: commit; Stage 2A's accidental keyless_tls/ submodule pointer was already cleaned up in Phase 1; Stage 6's structural concern is reported truthfully rather than papered over with weakened tests; commit 22 reverts the inert SelectorOverride field per cleanup-codebase rules.

Phase 3 entry

Phase 3 (multi-hop role + opt-in /16 diversity) follows on stacked branch discovery-phase-3-multihop-diversity with base = this PR's branch. Phase 4 (reservation voucher) follows similarly.

Out of Phase 2 acceptance — moved to Phase 3

  • Acceptance Proper server shutdown process in goroutine wg countings. (Fully fixes infinite hang issues when the control-c interrupt is called) #2 calibration revision (weighted p > 0.01 against capacity-weighted expected). Structural test-geometry limit; selector responds correctly. Phase 3 will revise the synthetic load-test geometry (e.g., per-relay unique capacities, per-client load noise, or a MaxActiveRelays cap that blends adjacent MOLS pick tiers) before re-running this criterion.
  • ClientState.SelectorOverride field + per-client dispatch — reintroduced in Phase 3 with its consumer.
  • P2C tie-break in weighted composite — deferred; within-tier quantization handles flap suppression for now.

…err param

- Lifecycle: not internally synchronized; callers must hold RelaySet.mu.
  Stage 2B EWMA fields follow the same discipline (no internal mutex on
  Lifecycle); single owner avoids nested-lock hazard.
- OnDiscoveryFailure / OnActiveFailure: document why the err parameter is
  accepted but not inspected (interface compatibility / future use).

Addresses two non-blocking warnings from Phase 2 Stage 1 review.
- Add portal/discovery/selectors/mols with type MOLS (renamed from
  MOLSRelayPolicy), constructor New(), and compile-time assertion
  var _ discovery.Selector = (*MOLS)(nil).

- Define local unexported relayPolicy interface in portal/discovery to
  break the import cycle; RelaySet.policy is now relayPolicy, and
  NewRelaySet accepts a relayPolicy parameter (callers pass mols.New()).
  isNilableAndNil helper guards against typed-nil injection.

- Rename RelayState.hasObservedDescriptor() -> HasObservedDescriptor()
  and add IsSuppressedActive(now time.Time) bool for cross-package access.

- Migrate all call sites (portal/server.go, sdk/expose.go,
  sdk/expose_test.go, cmd/portal-loadtest/main.go) to mols.New().

- Add package-discovery stubRelayPolicy in testhelpers_test.go for
  white-box tests that cannot import selectors/mols (cycle).

- Move math and policy tests to selectors/mols/mols_test.go (package mols,
  white-box); keep suppress/backoff tests in portal/discovery/mols_test.go
  (require unexported RelayState fields); keep lifecycle tests in
  portal/discovery/policy_test.go.

- Delete portal/discovery/mols.go. Trim portal/discovery/mols_test.go to
  the two tests that access unexported RelayState fields. Remove policy
  tests from policy_test.go that are now covered in selectors/mols.

go build ./... && go vet ./... && go test ./... all pass (149 tests).
git grep MOLSRelayPolicy -- ':!docs' ':!*.md' returns zero matches.
…extraction

Phase 1 acceptance criterion #1 (golden no-behavior-change) was lost during
the discovery -> selectors/mols extraction. Restored as a parameterized test
that asserts the relayPolicy-contract path (SelectPriorityWithTrace /
SelectMultiHopWithTrace) and the Selector-contract path (SelectPriority /
SelectMultiHop with ctx) produce byte-identical OutputURLs.

Backoff cases use Lifecycle.OnActiveFailure / OnDiscoveryFailure to drive
state into suppressActiveUntil / nextDiscoveryRefreshAt without touching
unexported discovery fields; noDescRelay is constructed via direct struct
literal (LastSeenAt zero -> HasObservedDescriptor() false).

16 priority subcases + 12 multihop subcases. Test count: 179 -> 180.
Parameterized test harness that runs the same set of safety invariants
against any discovery.Selector. Each invariant is a t.Run subtest under
the caller-supplied name.

Invariants covered:
  - explicit_relay_precedence
  - max_active_relays_cap
  - banned_excluded
  - suppression_respected (driven via Lifecycle.OnActiveFailure)
  - bootstrap_pin_survives_aggregate
  - freshness_gate_skips_expired
  - determinism_fixed_input
  - empty_pool_returns_nil
  - multihop_depth_zero_returns_nil
  - multihop_depth_one_returns_nil
  - trace_pool_total_matches_input
  - every_excluded_has_reason

Internal helpers (mustRelayDescriptor / mustConfirmedRelayState /
mustBootstrapRelayState / mustOverlayRelayState) mirror the existing
mols_test.go pattern; they remain unexported so the package's only
exported surface is Contract(...).

Stage 5 of Phase 2.
TestMOLSContract and TestWeightedContract invoke selectortest.Contract
with their respective Selector factories. Each adds 13 sub-tests
covering all 12 invariants plus the parent dispatcher.

Test count: 196 -> 222 (+26 across both selectors).

Stage 5 of Phase 2.
Reverts the inert public field added by 301ea66. Per the cleanup-codebase
rule "indirection earns its keep only when it removes real coupling that
*currently* exists," introducing API surface that has no consumer is
forbidden — even with a comment promising future use.

Phase 3 will reintroduce the field together with the dispatch logic that
consumes it, so the addition arrives behaviour-coupled in the same commit.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 993606d63d

ℹ️ 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".

Comment on lines +163 to +164
innerURLs, innerTrace := c.inner.SelectPriority(ctx, pool, client)
out := c.applyPenalty(innerURLs, pool)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Apply weighted scoring before inner-selector truncation

SelectPriority and SelectMultiHop call the inner selector first and only then run applyPenalty, but the inner MOLS selector already truncates candidates by MaxActiveRelays / MultiHopDepth. That means the weighted selector can only reshuffle an already-capped subset and cannot promote lower-ranked but less-loaded relays into the output, which breaks load-balancing whenever the initial MOLS top-K is overloaded. In practice this makes weighted behavior depend on callers manually expanding caps (as done in the loadtest), instead of correctly computing weighted rank over the full eligible pool and then applying the final cap.

Useful? React with 👍 / 👎.

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