Stop prod skysim aborting every twelve minutes on a mission path check - #11
Conversation
… check
skyhub-prod-skysim-service has been restarting in a loop, each task ending
exitCode 139. Reproduced against the prod image with its real tile set: it dies
on the 187th /mission/check, deterministically, in the container and locally.
The backtrace is not a race or a use-after-free:
JPH::QuadTree::AllocateNode "QuadTree: Out of nodes!" -> std::abort()
JPH::QuadTree::AddBodiesFinalize
JPH::BodyInterface::CreateAndAddBody
skysim::core::create_tile_body world.cpp:256
skysim::core::World::add_static_tile world.cpp:285
App::drain_commands main.cpp:608
Jolt's broadphase quadtree allocates nodes from a fixed pool and does not return
them when a body is removed — they are reclaimed only when the tree is rebuilt,
inside PhysicsSystem::Update. These services run --vehicles 0, so the strict
barrier is never satisfied, step_world never runs, and Update never happens.
Every tile a path check loads therefore leaks nodes until the pool is dry.
Two things made it worse, both fixed here:
stream_tiles() recomputed residency from fleet positions, found an empty fleet,
and evicted every tile the path check had just loaded — which the next path check
then reloaded. That is the +0 -53 line repeating in the logs with no matching
+53: the two call sites are not symmetric and only one of them prints. With no
vehicles there is nothing to stream around, so it now leaves residency alone;
the resident set is already bounded by max_resident.
The throttle guarding that work was `tick_index() % 100`, and tick_index only
advances when the world steps. In a world that never steps it is permanently 0,
so "every ~0.125 s" silently meant "every iteration of the loop".
Both call sites now go through apply_tile_plan(), which reclaims the broadphase
after a batch. Sharing the path is the point: the bookkeeping is not optional and
the next caller should not have to remember it.
Verified end to end, not inferred. Prod image, real 842-tile Plovdiv set:
1000 path checks with the fix versus death at 187 without. The eviction fix alone
is not enough — it still dies at 195 — so both halves are load-bearing.
The regression test is sized by measurement: 4000 rounds passes in 0.26 s with
the fix and aborts after 1.5 s without it. Note it aborts rather than failing,
because Jolt calls std::abort() directly, so a regression shows up as a crashed
test rather than a failed assertion.
Coverage gates hold: lines 85.9%, functions 95.7%, branches 71.7% (up from 71.3%).
Fixes #10
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe change adds a public broadphase optimization API, centralizes tile-plan application for streaming and path checks, skips empty-fleet streaming updates, and adds a regression test for repeated static-tile churn without physics stepping. ChangesTile streaming broadphase management
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main.cpp`:
- Around line 506-510: Update the throttling logic around stream_tiles() to
track the last tick index that was evaluated and return when the current tick
has already been processed, including repeated calls while the world is stalled
at tick 0 or another multiple of 100. Preserve evaluation on the first call for
each tick and retain the existing 100-tick cadence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 185e6dbd-8087-4fe4-93e4-132ee3c509b9
📒 Files selected for processing (4)
src/core/world.cppsrc/core/world.hsrc/main.cpptests/test_streamer.cpp
…umber Review caught that the fleet.empty() guard only covers half the problem. A non-empty fleet whose barrier is stalled — vehicles spawned but their SITL has not connected yet, which is the normal state for the first few seconds — also leaves tick_index() frozen. If it happens to be parked on a multiple of 100, the throttle is off entirely and the strict loop rescans residency every ~50 us, at roughly 20 kHz. `tick_index() % 100` reads as "every 100 ticks" but only advances when the world steps, and this is called whether it stepped or not. Recording which tick was last evaluated is what turns it into a throttle rather than a coincidence. Verified unchanged end to end: 1000 path checks against the prod tile set, all 16 tests green, coverage gates hold (lines 85.8%, functions 95.7%, branches 71.4%). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Fixes #10.
skyhub-prod-skysim-servicehas been restarting in a loop, each task endingexitCode 139. Reproduced against the prod image with its real 842-tile set: it dies on the 187th/mission/check, deterministically, both in the container and locally.Not what it looked like
The backtrace is not a race or a use-after-free:
Jolt's broadphase quadtree allocates nodes from a fixed pool and does not return them when a body is removed — they are reclaimed only when the tree is rebuilt, which happens inside
PhysicsSystem::Update. These services run--vehicles 0, so the strict barrier is never satisfied,step_worldnever runs, andUpdatenever happens. Every tile a path check loads leaks nodes until the pool is dry.Two things made it worse
The eviction was pure churn.
stream_tiles()recomputed residency from fleet positions, found an empty fleet, and evicted every tile the path check had just loaded — which the next path check reloaded. That is the+0 -53line repeating in the logs with no matching+53: there are two call sites and only one of them prints. With no vehicles there is nothing to stream around, so it now leaves residency alone; the set is already bounded bymax_resident.The throttle wasn't throttling. It guarded on
tick_index() % 100, andtick_indexonly advances when the world steps. In a world that never steps it is permanently0, so "every ~0.125 s" silently meant "every iteration of the loop".Both call sites now go through
apply_tile_plan(), which reclaims the broadphase after a batch. Sharing the path is deliberate — the bookkeeping is not optional and the next caller should not have to remember it.Verified, not inferred
So both halves are load-bearing — stopping the churn alone is not enough.
I initially concluded the opposite from a synthetic probe that drained the tile set to empty each round; optimizing a tree that has just been emptied reclaims nothing, so the probe said
OptimizeBroadPhasewas useless. It isn't — the probe simply wasn't representative. The numbers above come from the real binary and the real tiles.The test
Sized by measurement rather than guesswork: 4000 rounds passes in 0.26 s with the fix and aborts after 1.5 s without it. It uses a rolling resident set rather than draining to empty, for the reason above — a drain-to-zero loop exhausts the pool either way and would test nothing.
Worth knowing: it aborts rather than reporting a failure, because Jolt calls
std::abort()directly. A regression here looks like a crashed test, not a failed assertion.Coverage gates hold: lines 85.9%, functions 95.7%, branches 71.7% (up from 71.3%).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests