fix(monitor): hard-gate collector health on the upload delivery contract - #800
Merged
Merged
Conversation
Replace the process-state-centric breach set with exactly four hard gates on the data delivery loop, demoting everything else to warnings: 1. upload-status.json must exist and parse on the mandated lanes (binance-lob spot/usdm, binance-fee); LOB lanes no longer pass while the status file is absent. 2. last_success_at must be present and fresh per upload lane, with thresholds just above each lane's upload cadence (LOB 7200s, fee 600s, usdm-reference 1200s, polymarket 1800s, bybit 5400s). A missing last_success_at is a breach; the fee uploader gains the field in a parallel change, so the fee lane breaches by design until that deploys. 3. Pending upload backlog is bounded in count and oldest-artifact age, discovered with each collector's own pending definition (LOB *.manifest.json, fee/usdm-reference lake/raw/**/batch=*, polymarket rotated market-updates.*.ndjson tapes, bybit marked .ndjson without .uploaded.json). 4. failure_count must not grow and last_error must be empty, uniformly across all lanes (the fee-only initial-count breach is now uniform). polymarket-raw-ops-gate stays a breach on any non-disabled is-enabled state (including 'static'), and state-persistence failures stay breaches because gate 4 delta detection depends on the persisted state. Unit/timer state, restart deltas, health.json freshness/gaps, delay-gate journal trips, fee snapshot journal failures, disk, and mount are now warnings: emitted in the JSON warnings array and as warning: lines, never blocking ok:true. The monitor workflow includes the warnings in the triage issue body. Refs #738
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe collector health monitor now enforces four blocking upload gates, classifies infrastructure conditions as warnings, validates lane-specific freshness and backlogs, and reports warnings in JSON output and breach issues. ChangesCollector health monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CollectorHealth as monday-collector-health.sh
participant UploadStatus as upload status files
participant Spools as collector spools
participant Workflow as monitor-collector-host.yml
participant Issue as breach issue
CollectorHealth->>UploadStatus: validate status and freshness
CollectorHealth->>Spools: scan backlog and failure state
CollectorHealth->>Workflow: write health snapshot
Workflow->>Issue: append warnings when present
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deployment/aliyun/monday-collector-health.sh (1)
553-556: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
last_error_atandlast_errorserialize as the JSON string"null"instead ofnull.Lines 508-509 use
jq -r, so an absent field yields the four-character stringnull. Line 554 passes those values with--arg, which always produces a JSON string. The snapshot therefore reports"last_error_at": "null". A consumer cannot distinguish an absent error from an uploader that literally wrote"null". Line 462 already emits realnullfor the missing-file path, so the two paths disagree.Note that
deployment/aliyun/test-monday-collector-health.shline 746 assertslast_error_at == "null". Update that assertion together with this change.♻️ Proposed fix: keep the raw JSON values
- err_at=$(printf '%s' "$upload_json" | jq -r '(.last_error_at // null)') - err_msg=$(printf '%s' "$upload_json" | jq -r '(.last_error // null)') + err_at_json=$(printf '%s' "$upload_json" | jq -c '(.last_error_at // null)') + err_msg_json=$(printf '%s' "$upload_json" | jq -c '(.last_error // null)') + err_at=$(printf '%s' "$upload_json" | jq -r '(.last_error_at // null)') + err_msg=$(printf '%s' "$upload_json" | jq -r '(.last_error // null)')uobj=$(jq -n --argjson s "$success_raw" --argjson sa "$success_age_json" \ - --arg e "$err_at" --arg m "$err_msg" --argjson f "$failure_count" \ + --argjson e "$err_at_json" --argjson m "$err_msg_json" --argjson f "$failure_count" \ --argjson d "$failure_delta" --argjson pc "$pending_count" --argjson pa "$pending_age" \🤖 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 `@deployment/aliyun/monday-collector-health.sh` around lines 553 - 556, Update the snapshot construction around the jq invocation assigning uobj so last_error_at and last_error preserve raw JSON null values instead of passing jq -r output through --arg; use JSON arguments consistent with the existing success fields while retaining actual error strings. Update the corresponding assertion in test-monday-collector-health.sh to expect null rather than the string "null".
🤖 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 `@deployment/aliyun/monday-collector-health.sh`:
- Around line 484-496: The last_success_at parsing in the health monitor rejects
valid RFC3339 fractional-second and UTC-offset timestamps. In
deployment/aliyun/monday-collector-health.sh:484-496, normalize fractional
seconds and UTC offsets before fromdateiso8601 while preserving
epoch-millisecond handling; in
deployment/aliyun/test-monday-collector-health.sh:151-160, add a mandated-lane
fixture using a fractional-second timestamp such as 2026-08-07T01:00:00.123456Z
and assert the health check exits 0.
---
Nitpick comments:
In `@deployment/aliyun/monday-collector-health.sh`:
- Around line 553-556: Update the snapshot construction around the jq invocation
assigning uobj so last_error_at and last_error preserve raw JSON null values
instead of passing jq -r output through --arg; use JSON arguments consistent
with the existing success fields while retaining actual error strings. Update
the corresponding assertion in test-monday-collector-health.sh to expect null
rather than the string "null".
🪄 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: 579accd8-83da-44d4-bae2-84647ea4f79b
📒 Files selected for processing (5)
.github/workflows/monitor-collector-host.ymlagent-worktree.ymldeployment/aliyun/README.mddeployment/aliyun/monday-collector-health.shdeployment/aliyun/test-monday-collector-health.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 675b614c16
ℹ️ 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".
- Gate 2 parser now accepts the timestamps the uploaders actually emit: six-fractional-digit Z (polymarket_upload::utc_now, reused by fee and usdm-reference) and Chrono to_rfc3339() +00:00 with optional fractions (LOB), normalizing both before jq fromdateiso8601; non-UTC offsets are refused. Fixtures emit the real six-digit form and new green cases pin the offset/fractional variants. - Gate 3 now runs before the status-file checks, so a non-mandated lane with a missing upload-status.json can no longer hide an on-disk backlog (gate 1 absence stays a warning; gates 3 fail closed on the artifacts). - A pending-backlog scan that cannot inspect the spool (unreadable spool or a failed find traversal) is a breach instead of a silent zero. - Polymarket freshness bound raised to 7200s: both lanes rotate tapes hourly (record_market_updates_rotate_seconds = 3600; reference writer rotates at UTC-hour boundaries) and last_success_at only advances on an actual upload, so the 5-minute timer is not a heartbeat.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change contract
Collector host health is now gated on the data delivery loop instead of process state.
deployment/aliyun/monday-collector-health.shkeeps exactly four hard gates (breaches that fail closed into themonitor-collector-hostissue), and demotes every other check to warnings that are reported but never blockok:true:upload-status.jsonmust exist and parse on the mandated lanes (binance-lobspot/usdm,binance-fee). This closes the audited hole where a LOB lane reported healthy with no status file at all.last_success_atmust be present and younger than a per-lane bound set just above that lane's upload cadence (LOB 7200s: hourlySEGMENT_SECONDSrotation + 300s upload loop; fee 600s: 60s snapshot + 60s upload timer, mirroringFEE_FAILURE_WINDOW; usdm-reference 1200s and polymarket 1800s: 5-minute upload timers; bybit 5400s: hourly finalize + :23 sweep). A missing/unparseablelast_success_atis a breach: delivery is unproven.*.manifest.json= the archiver'spending_upload_segments; fee/usdm-referencelake/raw/**/batch=*directories, which the uploaders delete after verified upload; polymarket rotatedmarket-updates.<stamp>[...].ndjsontapes; bybit.ndjsonwith manifest+_SUCCESSand no.uploaded.jsonreadback marker).failure_countmust not grow between polls andlast_errormust be empty, uniformly across all lanes (the fee-only initial-count breach is now uniform).Kept as breaches beyond the four gates:
polymarket-raw-ops-gate@.serviceon any non-disabledis-enabledstate (staticincluded — it proves an uncleaned host installation), and state-persistence failures (gate 4 delta detection depends on that state).Demoted to warnings (JSON
warningsarray +warning:lines, never blockingok:true): unit/timer active+enabled+Result, restart-rate deltas,health.jsonfreshness/gaps, journald delay-gate trips, fee snapshot journal failures, disk space,/datamount. The workflow's breach issue body now includes the warnings section for triage context.Issue relationship
Refs #738
Out of scope
last_success_atfor the fee uploader lands in a parallel PR; this script intentionally treats its absence as a breach (gate 2) rather than carrying a compatibility path.Dependencies and merge order
Merge after (or together with) the parallel PR that adds
last_success_atto the fee uploader'supload-status.json. Until that deploys on the host, the fee lane breaches gate 2 by design — that is the intended fail-closed behavior, but merging this first would open a monitor issue for the fee lane.Focused validation
bash deployment/aliyun/test-monday-collector-health.sh— 89 passed, 0 failed. Red/green coverage per gate: missing/malformed status file on mandated lanes (red) vs non-mandated lane (warning only), stale and missinglast_success_atincl. the bybit epoch-ms format (red), pending count and oldest-age over bounds for LOB/reference/fee/polymarket/bybit (red), uploaded bybit segment with readback marker not counted as backlog (green),failure_countgrowth and first-observation-nonzero (red), healthy baseline with activemarket-updates.ndjsontapes excluded from backlog (green), and every demoted check (disk, units, timers, restarts, health.json, delay-gate, journald failure, fee snapshot failure, mount) asserting warning-but-ok:true. JSON shape tests pin the newwarningsarray and per-lane upload fields.bash .github/scripts/test-monitor-collector-host.sh— collector host monitor contract: ok.bash .github/scripts/agent-worktree-preflight.sh— ok.git diff --check— clean.Rollout and rollback
No runtime mutation from this PR: the workflow runs the script installed at
/opt/monday/bin/monday-collector-health.sh, so the new contract takes effect only when the host copy is updated under a named-controller cutover. Until then the old script keeps running. Rollback is reverting this commit and (if already deployed) reinstalling the previous script. Note the new script'sfailure_countpriors are compatible with the existing/var/lib/monday-collector-healthstate file, so the first post-deploy poll does not raise spurious initial-count breaches.Scope exception
None
Summary by CodeRabbit
New Features
Documentation
Tests