fix(server): review-fix sweep - #175
Conversation
… real queue wait - The termination-reason classifier checked the interpolated platform substring before the anchored starvation prefix; a crafted runs-on label could flip its own classification. The anchored prefix now wins. - The unreachable NdjsonEvent::JobCompleted counter arm is gone — the variant has no constructor anywhere, so the counter is single-sourced on terminal JobStatus. - reason.detail (free-form prose interpolating workflow input) is bounded to 512 bytes on a character boundary. - Store backend label mirrors open_store precedence: the environment is consulted only when no explicit URL was supplied. - cache/artifacts directory sizes are no longer reported as inode size; the database file is the only component until the recursive walk lands. - The scheduler heartbeat now proves scan progress: the scan tasks beat per workflow file and deregister on completion, so a hung or panicked scan goes stale and /readyz reports 503. - Session-delete transitions record only when a session was actually removed. - One STALENESS_THRESHOLD constant feeds /readyz and /api/v1/status. - Claim path records true queue latency from the enqueue timestamp instead of a hardcoded 1s placeholder. - InstrumentedStore collapsed into one record() helper so a new Store method cannot silently skip instrumentation. Entire-Checkpoint: 01M0GZJX756GZ2SZ14BS9M4RPW
preloop-vm and preloop-orchestrator share the fleet registry through preloop-observability instead of owning their own, avoiding a circular dependency. The host sampler itself is a stub until the cgroup/process parser lands. Entire-Checkpoint: 01M0GZN3CAJJM3NXHQ4Q9QHKC4
Entire-Checkpoint: 01M0GZPVH9X85W3FJMQR7512W8
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| let now = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|d| d.as_nanos() as i64) | ||
| .unwrap_or(0); | ||
| let elapsed = std::time::Duration::from_nanos((now - queued.enqueued_at_unix_nanos) as u64); |
There was a problem hiding this comment.
🟡 Medium src/broker.rs:410
A backward wall-clock adjustment makes record_claim_queue_wait panic in debug builds or record a huge queue wait in release builds, corrupting the queue-wait histogram. now - queued.enqueued_at_unix_nanos can be negative when the clock rolls back or a persisted timestamp is ahead of now; use checked subtraction and skip negative results.
.duration_since(std::time::UNIX_EPOCH)
- .map(|d| d.as_nanos() as i64)
- .unwrap_or(0);
- let elapsed = std::time::Duration::from_nanos((now - queued.enqueued_at_unix_nanos) as u64);
+ .map(|d| d.as_nanos() as i64)
+ .unwrap_or(0);
+ let elapsed_nanos = match now.checked_sub(queued.enqueued_at_unix_nanos) {
+ Some(value) if value >= 0 => value,
+ _ => return,
+ };
+ let elapsed = std::time::Duration::from_nanos(elapsed_nanos as u64);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/preloop-runner-server/src/broker.rs around lines 410-414:
A backward wall-clock adjustment makes `record_claim_queue_wait` panic in debug builds or record a huge queue wait in release builds, corrupting the queue-wait histogram. `now - queued.enqueued_at_unix_nanos` can be negative when the clock rolls back or a persisted timestamp is ahead of `now`; use checked subtraction and skip negative results.
1 participant
The bot-review findings: classification can't be flipped by crafted
runs-onlabels (anchored prefix first), unreachableJobCompletedcounter arm deleted,reason.detailbounded, store-backend label honors URL precedence, directory sizes no longer faked, scheduler heartbeat proves scan progress (not a self-beating timer), session-delete metric only on real deletes, one staleness constant across/readyzand/api/v1/status, real queue-wait latency at claim,InstrumentedStorededuped into onerecord()helper, unmeasured VM host usage serialized as absent not zero.Top of the stacked series:
Summary by cubic
Fixes classification, metrics, and readiness semantics across the runner server so telemetry is accurate and health checks reflect real progress. Old behavior included misclassified terminations, synthetic metrics, and a timer-based heartbeat; new behavior enforces correct classification, measures real queue latency, and ties readiness to actual scheduler scans.
reason.detailto 512 chars on a character boundary.NdjsonEvent::JobCompletedcounter; terminal transitions are recorded once viaJobStatus.store_urloverridesPRELOOP_STORE_URL), matchingopen_storeand preventing mislabeling.cache/artifactsplaceholders until the recursive walker lands.InstrumentedStoretiming via a singlerecord()helper so new methods cannot skip instrumentation.preloop-vm::telemetryre-exports frompreloop-observabilityand usesbuild_fleet_snapshotduring bootstrap; unmeasured VM host usage is serialized as absent, not zero.Written for commit 652fa67. Summary will update on new commits.
Note
Fix metrics, heartbeat wiring, and snapshot construction in
runner-serverUnavailableplaceholder in bootstrap.rsHeartbeatHandlepassed intoScheduler::scan_workspaceandscan_remote, so beats fire per workflow entry and deregister on completionenqueued_at_unix_nanosinstead of a fixed 1s value in broker.rs; stops emitting adelete oksession-transition metric when no session is foundbounded_termination_reasoncheck order so starvation messages containing user labels are not misclassified asno_platform_runner; bounds exported reason detail to 512 chars and removes a redundantJobCompletedcounter that double-counted terminal transitions in state.rsstore_urlbackend detection now prefers explicitconfig.store_urloverPRELOOP_STORE_URLenv var, so deployments relying solely on the env var while also settingstore_urlwill see the config value take precedence📊 Macroscope summarized 652fa67. 7 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues