Skip to content

Claim the re-solve slot on publication, not on admission - #294

Merged
jehanazad merged 3 commits into
feat/dark-solver-observabilityfrom
fix/resolve-slot-claim-on-publish
Sep 6, 2026
Merged

jehanazad merged 3 commits into
feat/dark-solver-observabilityfrom
fix/resolve-slot-claim-on-publish

Conversation

@jehanazad

Copy link
Copy Markdown
Contributor

Stacked on #293 (feat/dark-solver-observability). Review that first; this PR's diff is the resolve-slot change alone.

The bug

_claim_resolve_slot recorded _RECENT_SOLVES[tid] = (now, n) before the solve, under the same lock as the test, and never released it. But the statement a claim makes is "this aircraft is already on the map at this width" — and only a publish puts it there. A candidate that the gate stack then rejected, or one that was a contaminated superset trimmed into nothing, still held the slot for the whole SOLVER_RESOLVE_INTERVAL_S (12 s).

Two live consequences:

  1. A rejected candidate blacked out its own identical twin. ~24 % of dark attempts are rejected; the retry that would have published was suppressed by the failure.
  2. It suppressed other aircraft. Tracker track ids are shared across the association candidates of different aircraft — 74 of 178 ids in a 6 min live window appeared in published solves of more than one ground-truth aircraft, which is the finding that forced _supersession_match's spatial guard in Require a spatial or identical-inputs match before superseding a multinode track #290. So a contaminated superset the gates sank also took down every clean subset behind it, including a neighbour's only candidate.

Live baseline (test droplet, 30 min): solver_resolve_skips1 537 against 646 dark attempts — the mechanism refused more than twice as many candidates as the lane solved, for aircraft it had put nowhere.

The change

_claim_resolve_slot splits in two:

  • _resolve_slot_covered(s_in, now_s) -> (covered, blocking) — pure, read-only, run before the solve. Returns the blocking claims for Instrument the dark lane: per-lane history, resolve-slot skips, live contamination #293's skip record; an admitted candidate leaves no trace at all.
  • _record_resolve_slot(track_ids, n_nodes, now_s) — called only on the publish path, after the archive append and outside _MN_TRACKS_LOCK (so _RECENT_SOLVES_LOCK is never nested inside it), with the post-trim survivors: result["source_track_ids"], which _filter_s_in_to_nodes rebuilds from the surviving track_ids_by_node. All three producers in association.py emit that field, so the survivors are genuine, and there is a test asserting a trimmed node's track is left unclaimed. That is deliberate: a node dropped for a bad residual contributed nothing to the published position and its track was probably another aircraft's — claiming it would suppress that aircraft on the strength of a measurement this solve threw away.

The rule is otherwise unchanged: every track covered at no fewer nodes inside the 12 s window, widest claim wins, same _sweep_recent_solves pruning. state.bump_counter("solver_resolve_skips") and #293's skip deque stay at the check site. No negative claim for rejects — measure first, as the plan says.

Accepted cost

The check no longer claims under the same lock, so two workers can now both solve duplicates of one aircraft that arrived together. That is one extra solve, arbitrated downstream by keying and supersession (which handle exactly this case), against the starvation above. Documented in the block comment and in docs/solverflow.md §5.

Expected post-deploy change

resolve_skips / attempts (the resolve_skips.attempts_ratio #293 adds) should fall from ~0.96–2.4 toward ≤ 0.5, with attempts rising and dark published rising with it, and no growth in the solver_fail_rms_delay share of rejects. Solver load rises — pool headroom on the test droplet is fine (queue depth ≈ 0).

Tests

The existing _claim_resolve_slot cases become check/record cases (_resolve_slot_covered / _record_resolve_slot), plus:

  • test_the_check_alone_claims_nothing — the whole point of the split.
  • test_a_rejected_candidate_does_not_block_an_identical_twin — end-to-end through _process_solver_item: reject on rms_delay, then the twin solves and publishes, solver_resolve_skips == 0.
  • test_a_subset_for_another_aircraft_survives_a_rejected_superset.
  • test_only_the_published_width_is_claimed.
  • TestTrimmedTracksAreNotClaimed (in test_solver_trimming.py) — _RECENT_SOLVES holds exactly the survivors, and a neighbour built on the dropped track is still admitted.

Checked for other callers: none outside tests (known_lane sets source_track_ids = [] and does not go through _process_solver_item); test_feed_multinode.py is unaffected and passes.

How verified

  • tests/test_solver_worker.py tests/test_solver_trimming.py tests/test_mlat_history.py tests/test_solver_stats.py tests/test_feed_multinode.py: 252 passed.
  • Full backend suite: 2813 passed, 2 skipped in 206.51s (-n 2 -m "not external").
  • pre-commit run --all-files: all five hooks Passed.

Not deployed.

🤖 Generated with Claude Code

The suppression rule reads "this aircraft is already on the map at this
width, at every track it is built from". _claim_resolve_slot recorded the
claim BEFORE the solve, so a candidate that never reached the map still made
that statement — and never released it.

Two consequences, both live:

* A rejected candidate blacked out its own identical twin for the full 12 s.
  24% of dark attempts are rejected, and the retry that would have published
  was suppressed by the failure.
* Tracker track ids are shared across the association candidates of DIFFERENT
  aircraft (74 of 178 ids in a 6 min window appeared in solves of more than
  one ground-truth aircraft — the finding that forced _supersession_match's
  spatial guard in #290). So a contaminated superset that the gates sank also
  suppressed the clean subsets behind it, including its neighbour's only
  candidate.

Measured on the test droplet: ~1,537 skips against 646 dark attempts per
30 min. The mechanism refused more than twice as many candidates as it
solved, for aircraft it had put nowhere.

_claim_resolve_slot splits into a pure _resolve_slot_covered (read-only,
run before the solve, also returning the blocking claims for the skip record)
and _record_resolve_slot, called only on the publish path with the POST-TRIM
survivors — result["source_track_ids"], which _filter_s_in_to_nodes rebuilds
from the surviving track_ids_by_node. A trimmed node's track is deliberately
left unclaimed: it contributed nothing to the published position and was
probably another aircraft's, so claiming it would suppress that aircraft on
the strength of a measurement this solve threw away.

The rule itself is unchanged: every track covered at no fewer nodes within
_SOLVER_RESOLVE_INTERVAL_S, widest claim wins, same pruning sweep. No
negative claim for rejects — measure first.

The cost is that the check no longer claims under the same lock, so two
workers can both solve duplicates that arrived together. That is one extra
solve, arbitrated downstream by keying and supersession, against the
starvation above.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@claude

This comment has been minimized.

jehanazad pushed a commit that referenced this pull request Sep 6, 2026
…urement-epochs

state.py / routes/test.py / solver.py / test_solver_stats.py: adjacent
additions from both sides kept (epoch-align flag + counters next to the
free-altitude flags, resolve_skips_dark and altitude_mode stamps).
libs/retina-analytics: analytics main.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Claude finished @jehanazad's task in 1m 37s —— View job


Code review: #294 — Claim the re-solve slot on publication, not on admission

Todo list:

  • Gather context (PR body, changed files, diff against feat/dark-solver-observability)
  • Understand the request (re-review triggered by branch synchronize)
  • Diff against previous review point to see what changed
  • Trace the split (_resolve_slot_covered / _record_resolve_slot) through _process_solver_item
  • Verify lock scoping, callers, and test coverage
  • Post review findings

Summary

This synchronize event was triggered by merge commits (fcccb1f, d09dd12) that pull the stacked base branch (feat/dark-solver-observability) forward and re-merge it into this branch — git diff origin/feat/dark-solver-observability...HEAD is unchanged from the prior review (still 306/-77 across the same 7 files; the substantive commit 3a69d2d "Claim a re-solve slot on publication, not on admission" predates it). I re-traced the change independently rather than trusting that, and confirmed the same conclusions:

  • Single publish site, correctly ordered. _record_resolve_slot (solver.py:2565) runs once, after state.multinode_tracks[key] = result (line 2551) and the with _MN_TRACKS_LOCK: block has closed (line 2438 opens it, dedents by line 2554) — so _RECENT_SOLVES_LOCK is genuinely never nested inside _MN_TRACKS_LOCK, as claimed.
  • Post-trim survivors, verified end to end. result["source_track_ids"] (solver.py:2492) is built from s_in.get("track_ids") — and s_in itself is reassigned to the trimmed input at solver.py:2017 (result, s_in, trim_meta = _trim_and_resolve(...)), whose _filter_s_in_to_nodes (solver.py:442-466) rebuilds track_ids from the surviving track_ids_by_node. So a dropped node's track genuinely never reaches _record_resolve_slot. result.get("n_nodes") passed alongside it is the same post-trim count (set at solver.py:460), so the claimed width and the claimed track set are consistent with each other.
  • known_lane.py confirmed clean. It sets source_track_ids = [] (known_lane.py:323) and has no call to _record_resolve_slot anywhere in the file — matches the PR's "checked other callers: none" claim.
  • No stale references_claim_resolve_slot / _resolve_slot_blockers are fully removed, no leftover callers in backend/.
  • Test coverage (test_solver_worker.py, test_solver_trimming.py::TestTrimmedTracksAreNotClaimed, test_mlat_history.py) exercises exactly the scenarios above, including the trim-survivor case via the real _process_solver_item path rather than mocks.

No correctness issues found. The same minor, non-blocking observations from the prior pass still stand and remain optional:

  1. _sweep_recent_solves now only runs from the publish path, so _RECENT_SOLVES is pruned somewhat less often (publishes are a subset of admissions) — unlikely to matter given the 12s window, just worth knowing if that dict's size is ever monitored.
  2. _record_resolve_slot(track_ids, n_nodes: int, ...) types n_nodes as int but the caller passes result.get("n_nodes") (Optional) — handled correctly at runtime (int(n_nodes or 0)), cosmetic only.
  3. state.py:592-594's counter comment ("while the claim was taken on ADMISSION...") reads a little ambiguous out of context (historical justification vs. current behavior) but is consistent with this file's existing narrative-comment style elsewhere and not misleading once read in full.

I was not able to execute the test suite in this sandbox (no backend .venv), so this is a static re-trace of the diff plus targeted greps to verify the specific claims in the PR body; nothing here contradicts the reported 252/2813 passing results.

Overall: unchanged from the prior review — looks good to merge on its own merits.

@jehanazad
jehanazad marked this pull request as ready for review September 6, 2026 02:38
@jehanazad
jehanazad merged commit 784d855 into feat/dark-solver-observability Sep 6, 2026
15 checks passed
@jehanazad

Copy link
Copy Markdown
Contributor Author

Note for history readers: this PR's base was feat/dark-solver-observability (#293), and GitHub kept that base after #293 merged, so gh pr merge landed this merge commit (784d855) on that feature branch rather than on main. The content is on main all the same: #295's branch was chained on this head (d09dd12), which is an ancestor of main since c94d1d4.

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.

2 participants