A late vehicle stalls its own tick, not everyone else's flight

[yalexx]

What

Interactive mode paced the world off the wall clock and evicted whatever could not keep up. That is the wrong thing to trade away.

A vehicle that misses its slot gets its physics advanced without it — and the autopilot on the other side of the JSON link derives its entire scheduler rate from the timestamp delta we send (SIM_JSON.cpp calls adjust_frame_time(1.0 / deltat)). So a straggler was not merely late. It was flying a different simulation from the one it was being graded on.

Now a late vehicle stalls its own tick and holds the world while it catches up. Lateness costs wall-clock pacing, which is recoverable and shows up in the metrics; it no longer costs flight fidelity, which is neither.

--hold-ticks becomes --straggler-timeout, in seconds — a bound on how long to wait is a duration, not a count of frames whose length is itself the thing under discussion.

Also: the MJPEG stream was 59% duplicates

The endpoint re-sent whatever sat in the frame store on a fixed cadence, regardless of whether the renderer had produced anything new. Frames now carry their sim time and go out only when it changes.

Measured on a local SITL:

	before	after
frame rate	24.7 fps	9.8 fps
bandwidth	62 KB/s	22.7 KB/s

Nothing was lost — the surplus was all frames the client already had.

Note for whoever merges

The red check on this repo's PRs is Build not triggered: Pull request approval required for starting a build — a CodeBuild policy gate, not a code failure. Unit tests (GCC and Clang), simulator happy paths, full coverage and performance budgets are all green.

🤖 Generated with Claude Code

https://claude.ai/code/session_017cgM68QE3FDz7S2pQAfTaZ

Summary by CodeRabbit

• New Features
  • Added environment-variable configuration for simulation timing, time mode, straggler timeout, and optional MJPEG camera streaming.
  • Camera streams now provide distinct, timestamped frames.
  • Frozen vehicles can resume when input frames return.
• Bug Fixes
  • Late vehicles no longer advance using stale controls; they stall, freeze, and may despawn according to timeout settings.
  • Improved interactive-mode pacing and snapshot updates during stalls.
• Documentation
  • Expanded Docker and design documentation covering configuration, timing, straggler handling, camera setup, and updated vehicle status fields.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 76762425-9dec-44dc-8fe3-5b17fa9c8a3f

📥 Commits

Reviewing files that changed from the base of the PR and between 094526d and bc9314c.

📒 Files selected for processing (4)

• README.md
• docs/DESIGN.md
• src/core/clock.h
• src/main.cpp

🚧 Files skipped from review as they are similar to previous changes (3)

• src/core/clock.h
• README.md
• src/main.cpp

---

📝 Walkthrough

Walkthrough

The change adds environment-driven runtime flags and camera configuration, returns timestamped camera frames, deduplicates MJPEG output, and replaces stale-PWM stepping with barrier-based interactive timing and straggler handling. Vehicle telemetry, documentation, tests, and harness settings are updated.

Changes

Runtime and control flow

Layer / File(s)	Summary
Runtime configuration and documentation docker-entrypoint.sh, README.md, tools/harness/params/skysim.parm	Docker variables now map to optional simulator flags, including time mode and camera settings. Documentation covers the mappings, camera endpoint, and straggler timeout. The harness uses the X-frame configuration.
Interactive barrier and straggler lifecycle src/core/clock.h, src/main.cpp, docs/DESIGN.md, tools/harness/straggler.py	Interactive mode waits for current input frames. Silent vehicles freeze after the seconds-based timeout and are excluded from physics until frames resume. Housekeeping time is included in tick metrics.
Frame metadata and MJPEG streaming src/api/control_server.h, src/api/control_server.cpp, src/render/render_service.h, tests/test_api.cpp, tests/test_render.cpp	Camera APIs now return FrameStore::Frame values with simulation-time metadata. MJPEG endpoints emit each simulation frame once and poll every 5 ms.
Vehicle silence telemetry src/main.cpp, src/api/control_server.h, src/api/control_server.cpp, tests/test_api.cpp, README.md	Vehicle status fields and JSON responses now use silent_ticks instead of held_ticks.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant InteractiveLoop
  participant VehicleFrames
  participant PhysicsStep
  participant SnapshotPublisher
  InteractiveLoop->>VehicleFrames: wait for current input frames
  VehicleFrames-->>InteractiveLoop: provide fresh or missing frames
  InteractiveLoop->>PhysicsStep: step only vehicles with frames
  InteractiveLoop->>SnapshotPublisher: publish snapshots during stalls

Loading

Possibly related PRs

• ID-Robots/skysim#2: Both changes update SKYSIM_DT handling in docker-entrypoint.sh.
• ID-Robots/skysim#16: Both changes include Docker camera configuration and environment-to-CLI mapping updates.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title is misleading because the implementation stalls the shared simulation tick while a late vehicle catches up, rather than stalling only that vehicle's tick.	Revise the title to state that late vehicles stall the shared simulation tick before freezing or despawning under the configured timeout.
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/straggler-stalls-its-own-tick

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (3)

README.md (1)

193-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the two-stage timeout lifecycle.

--straggler-timeout freezes a silent vehicle after 2 seconds. --grace despawns it after an additional 30 seconds by default. Update the README to distinguish these timeouts.

🤖 Prompt for 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.

In `@README.md` around lines 193 - 197, Update the README section describing
interactive mode and --straggler-timeout to document the two-stage lifecycle: a
silent vehicle freezes after the straggler timeout, then is despawned after the
additional --grace period, defaulting to 30 seconds. Clearly distinguish both
timeout settings and retain the existing default of 2 seconds for
--straggler-timeout.

tests/test_api.cpp (1)

270-275: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test duplicate-frame suppression with a stable timestamp.

The callback changes sim_time_s on every poll. Every poll therefore appears to be a new frame. The test only verifies that one multipart frame arrives, so a duplicate-per-poll implementation would still pass.

Return a stable frame during several polls and assert one multipart boundary. Then advance sim_time_s and assert that a second boundary arrives.

🤖 Prompt for 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.

In `@tests/test_api.cpp` around lines 270 - 275, Update the MJPEG callback test
around the frame-producing callback to keep the frame timestamp stable across
several polls, then assert that only one multipart boundary is emitted. Advance
the simulated timestamp afterward and assert that a second boundary is received,
ensuring duplicate-frame suppression and new-frame delivery are both tested.

tests/test_render.cpp (1)

276-288: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert simulation-time propagation.

This test checks only FrameStore::Frame::jpeg. It can pass if RenderService::frame loses sim_time_s. The MJPEG route in src/api/control_server.cpp, Lines 341-363, uses sim_time_s to suppress duplicate frames. Poll into a complete FrameStore::Frame and assert that its timestamp equals pose.sim_time_s.

As per the PR objectives and the downstream MJPEG consumer contract, this test should cover timestamp propagation.

Proposed test update

-        std::vector<uint8_t> frame;
-        for (int i = 0; i < 200 && frame.empty(); ++i) {
+        skysim::render::FrameStore::Frame frame;
+        for (int i = 0; i < 200 && frame.jpeg.empty(); ++i) {
             std::this_thread::sleep_for(std::chrono::milliseconds(10));
-            frame = service.frame(1).jpeg;
+            frame = service.frame(1);
         }
-        CHECK(!frame.empty());
-        CHECK(frame.size() > 2 && frame[0] == 0xFF && frame[1] == 0xD8);
+        CHECK(!frame.jpeg.empty());
+        CHECK(frame.sim_time_s == pose.sim_time_s);
+        CHECK(frame.jpeg.size() > 2 && frame.jpeg[0] == 0xFF && frame.jpeg[1] == 0xD8);

🤖 Prompt for 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.

In `@tests/test_render.cpp` around lines 276 - 288, Update the frame polling in
the render-service test to retrieve the complete FrameStore::Frame rather than
only its jpeg field, and assert that the returned frame’s sim_time_s equals the
published pose’s sim_time_s. Preserve the existing JPEG validity and
despawn-clearing assertions while covering timestamp propagation through
RenderService::frame.

🤖 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 `@README.md`:
- Line 236: Update the --straggler-timeout entry in the README options table to
show that it requires a seconds argument, using the syntax --straggler-timeout
<seconds> rather than presenting it as a valueless switch.

In `@src/core/clock.h`:
- Around line 23-26: The TickGate documentation in src/core/clock.h lines 23-26
must define the barrier over connected vehicles rather than all vehicles, and
explicitly state that interactive freeze removes a vehicle from the barrier.
Update docs/DESIGN.md lines 38-42 to clarify that timeout, freeze, and blocking
apply only after a vehicle has connected.

In `@src/main.cpp`:
- Around line 2-3: Update run_strict to track any_connected independently from
any_fresh, preserving any_fresh solely for tick readiness. Base the
strict_timeout_s expiration check on any_connected so the run aborts when all
previously connected vehicles stop sending frames, while retaining existing
behavior for fresh-frame processing.
- Around line 926-950: Update wait_for_frames to exclude vehicles whose frozen
flag is already true from the pre-deadline barrier, while continuing to poll all
vehicles in the subsequent readiness loop so frozen slots can thaw when frames
resume. Keep the existing connected-slot waiting behavior for non-frozen
vehicles.
- Around line 173-174: Update the --straggler-timeout branch in parse_args to
validate the parsed value before assigning straggler_timeout_s: reject
non-finite values, negatives, and values whose later tick conversion cannot fit
in int, reporting the argument as invalid. Only assign accepted values so line
893’s conversion receives a finite, non-negative, representable timeout.

---

Nitpick comments:
In `@README.md`:
- Around line 193-197: Update the README section describing interactive mode and
--straggler-timeout to document the two-stage lifecycle: a silent vehicle
freezes after the straggler timeout, then is despawned after the additional
--grace period, defaulting to 30 seconds. Clearly distinguish both timeout
settings and retain the existing default of 2 seconds for --straggler-timeout.

In `@tests/test_api.cpp`:
- Around line 270-275: Update the MJPEG callback test around the frame-producing
callback to keep the frame timestamp stable across several polls, then assert
that only one multipart boundary is emitted. Advance the simulated timestamp
afterward and assert that a second boundary is received, ensuring
duplicate-frame suppression and new-frame delivery are both tested.

In `@tests/test_render.cpp`:
- Around line 276-288: Update the frame polling in the render-service test to
retrieve the complete FrameStore::Frame rather than only its jpeg field, and
assert that the returned frame’s sim_time_s equals the published pose’s
sim_time_s. Preserve the existing JPEG validity and despawn-clearing assertions
while covering timestamp propagation through RenderService::frame.

🪄 Autofix

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 Plus

Run ID: 498cc8ce-284b-47c3-922f-e7898c100bfb

📥 Commits

Reviewing files that changed from the base of the PR and between 70b0c01 and 094526d.

📒 Files selected for processing (12)

• README.md
• docker-entrypoint.sh
• docs/DESIGN.md
• src/api/control_server.cpp
• src/api/control_server.h
• src/core/clock.h
• src/main.cpp
• src/render/render_service.h
• tests/test_api.cpp
• tests/test_render.cpp
• tools/harness/params/skysim.parm
• tools/harness/straggler.py

[yalexx]

All five applied in bc9314c.

Exclude frozen slots from the pre-deadline wait — confirmed and embarrassing: the barrier below excludes a frozen vehicle, which is the entire meaning of freezing it, but wait_for_frames did not. So the fleet spent the whole frame grace every tick waiting on a vehicle it had already agreed to step without. Freezing a dead SITL was supposed to end that stall, not relocate it earlier in the tick.

Abort when all connected vehicles are silent — confirmed. any_fresh meant the abort sat quietly through the one scenario it exists for, every SITL going silent at once. Now gated on any_connected, which still lets an empty world wait for its first arrival.

Validate --straggler-timeout — taken. atof gives nan for "abc" and inf for "1e400", and both go through an int conversion to become a tick count, so the freeze threshold was undefined behaviour on a flag about how long to tolerate silence. Rejected outside [0, 86400].

Both doc findings — taken. clock.h and DESIGN.md now say the barrier is over connected vehicles and that freezing removes one from it; the README shows <seconds>.

Verified: 17/17 ctest pass, clean build.

Note for whoever merges: the red check here is Build not triggered: Pull request approval required for starting a build — a CodeBuild policy gate, not a failure. Every other check is green.