Repository navigation
fix(cron): keep a run's real outcome when an unknown-fenced delivery meets a claim miss - #136066
Draft
liuhao1024 wants to merge 2 commits into
Draft
liuhao1024 wants to merge 2 commits into
liuhao1024 wants to merge 2 commits into
Conversation
…meets a claim miss NousResearch#132078: a restart-safe worker's 300s wait budget fences a mid-send delivery "unknown" (cron/delivery_queue._terminalize_wait_timeout) even though the gateway's at-most-once send may still land — the reporter's fleet saw the receipt land 9 minutes before the runner tail died. The post-delivery fire-claim sample then missed, and the NousResearch#105861 fall-through only exempted a delivery with NO error, so the delivered run was closed "failed: Interrupted by shutdown before terminal completion." for a shutdown that never happened, polluting error streaks and incident ledgers. The queue branch of _deliver_result now stamps job["_delivery_outcome_unknown"] when the queue row fenced the send unknown (same pattern as _notification_all_targets_suppressed, re-derived per attempt), and the post-delivery ownership check extends the existing fall-through to that case: the run keeps its real outcome (ok run records delivery_failed with the streak untouched; failed run keeps its real error), while the queue row remains where the uncertainty is recorded. An explicit transport cancel stays fail-closed; the synthetic message wording is left to NousResearch#116463.
Offset the file-size ratchet for the delivery-outcome marker added in this PR: scheduler_delivery.py is over its cap and may only go down, so the self-contained payload redaction helper moves to a topical sibling and is re-exported from the original module (callers and monkeypatching of the old name are unchanged).
This branch has not been deployed
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
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.
What does this PR do?
A cron execution whose delivery the durable queue fenced
unknown(the restart-safe worker's 300s wait budget ran out while the gateway was still mid-send) could still be closed asfailed: Interrupted by shutdown before terminal completion.when the runner tail died afterwards and the post-delivery fire-claim sample missed. In the reporter's fleet the delivery receipt demonstrably landed ~9 minutes before the tail died and the gateway process was up the whole time — no shutdown ever occurred.The gap was in the #105861 fall-through: it exempts a claim miss only when the delivery returned no error. An
unknownoutcome returns an error string while the gateway's at-most-once send may still land (by design the queue never retries it), so a delivered run fell into_record_fire_ownership_lostand got overwritten with the synthetic shutdown string, pollutinglast_status, error streaks and incident ledgers.This PR:
job["_delivery_outcome_unknown"]in the queue branch of_deliver_resultwhen the queue row fenced the sendunknown(same in-band pattern as the existing_notification_all_targets_suppressedmarker, re-derived per attempt and cleared at the top of every_deliver_resultcall), and_finish_completed_run). An ok run recordslast_status="delivery_failed"withlast_error=Noneand the streak untouched whilelast_delivery_errorcarries the honest "outcome is unknown" text; a failed run keeps its real error. The queue row remains where the uncertainty is recorded. An explicit transport cancel stays fail-closed, exactly as before.The synthetic
_OWNERSHIP_LOST_INTERRUPTEDwording itself is left untouched — #116463 already reworks that message and would conflict.Related Issue
Fixes #132078
Type of Change
Changes Made
cron/scheduler_delivery.py: queue branch stamps_delivery_outcome_unknownwhenget_statusreportsunknown;_deliver_resultpops a stale marker up front.cron/scheduler.py: the post-delivery ownership-lost check (_run_one_job_body) extends the Cron agent jobs: successful delivery overwritten with "Interrupted by shutdown before terminal completion." when fire-claim heartbeat lapses mid-run #105861 fall-through to an outcome-unknown delivery; the warning distinguishes "a completed" from "an outcome-unknown" delivery.cron/scheduler_delivery_redact.py:_redact_cron_payloadsplit out unchanged (re-exported fromscheduler_delivery) purely to offset the file-size ratchet, sincescheduler_delivery.pyis over its cap and may only go down.tests/cron/test_fire_claim_lost_after_delivery.py: two regression tests — the full_driveharness with an unknown-fenced delivery and a post-delivery claim-sample miss (ok and failure-notice variants), and the queue-branch stamping/clearing behaviour.How to Test
python -m pytest tests/cron/test_fire_claim_lost_after_delivery.py -q— 9 passed. On main without the fix, the three new tests fail: the run recordslast_status="error"/last_error="Interrupted by shutdown before terminal completion."(and the queue branch stamps nothing).python -m pytest tests/cron/ -q— 1476 passed, 11 skipped; the only failure,test_atomic_paused_creation.py::test_invalid_creation_is_rejected_without_writes, reproduces identically on a clean checkout of upstream/main on this machine (hermes_bootstrap._settle_interrupted_updateprobes the checkout's.gitat import time and trips the home-I/O guard) and is unrelated to these files.python scripts/check --base upstream/main— 11 checks, ok.Observed result: after the fix, the unknown-fenced run's on-disk record is
last_status="delivery_failed"/last_error=None/failure_streak=0with the "outcome is unknown" text preserved inlast_delivery_error— the run's real outcome survives the claim miss instead of the synthetic shutdown failure.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passpython scripts/checkpasses (the blocking lint and code-health checks CI runs)Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A — no skill added.
Screenshots / Logs