Skip to content

Preserve outstanding cron payloads until normal retirement - #5198

Merged
john-the-dev merged 3 commits into
mainfrom
codex/sutando-cron-payload-review
Oct 7, 2026
Merged

john-the-dev merged 3 commits into
mainfrom
codex/sutando-cron-payload-review

Conversation

@john-the-dev

@john-the-dev john-the-dev commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Repeated prompt cron fires previously unlinked an outstanding payload before writing a replacement. A worker could already have accepted or claimed the old identity. This keeps the oldest outstanding task immutable until normal retirement, serializes emitters per job, and uses the existing atomic writer for new tasks. Prompt changes apply after the old task retires; legacy backlogs are preserved.

Review src/cron-runner.py and tests/cron-outstanding-payload.test.py first. Six new production-writer tests pass, including 12 concurrent processes, ownership sentinels, handler claims, legacy backlogs and atomic failure. The existing cron runner suite also reports ALL PASSED. Ruff, docs audit, repository review scanner and diff checks pass.

Before, current main 95fbd566a with the new six tests (subtest failures are not seven distinct tests):

----------------------------------------------------------------------
Ran 6 tests in 0.054s

FAILED (failures=7)

After, identical six tests:

----------------------------------------------------------------------
Ran 6 tests in 0.047s

OK

Documentation/catalog/navigation are updated in docs/cron-outstanding-payload.md. This 6-file PR isolates cron payload lifetime from the original reliability experiment; Rui's full-day manual experience and measured components remain in #5195. It is based directly on main with no stack dependency. No new live restart or external message was performed to prepare this review; isolated concurrency proof is distinct from a new live deployment. Voice/audio: N/A. A stuck outstanding task still requires normal recovery.

CI follow-up

CI run 37631683329 initially failed in unchanged tests/external-core-helpers.test.py because a live helper log changed during its launch/restart assertion. The aggregate test gate failed as a consequence. External-helper code, launcher and fixture are identical to main; an archived-main local control passed, without establishing the Linux failure cause. A bounded failed-job rerun was requested. The original failure remains recorded; review readiness does not certify merge readiness.

Current review revision

Current head 50df001: corrects the test invocation and checks the actual .task-.txt. temporary names after failed atomic publication. Six cron tests pass; production behavior is unchanged.

Chi’s Blue agent requested these fixes at the preceding heads. Current-head CI is successful, including coverage. Codex core independently returned no remaining blocker for the isolated fixes; Yixuan’s and Qingyun’s rereviews remain pending. Earlier CI successes, Mark approvals and isolated Codex verdicts apply to their recorded heads, and have not been carried forward. Authenticated integration remains unverified; no merge is requested.

Current-head CI is green. Core independently reran original-repro controls and relevant suites at the new head; its verdict remains isolated compatibility, not authenticated integration or formal GitHub approval. Blue independently verified the fixes and dismissed its previous requested-change reviews. Its new reviews are comment-only, leaving maintainer approval to Chi.

Chi Blue’s current-head rereview finds no new blockers. Mark’s current-head source approval also finds no remaining blocker, with its verification limits recorded in the review. These are agent reviews under shared logins, not human signoff. Yixuan’s and Qingyun’s independent reviews remain pending.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/cron-runner.py (100%)

Summary

  • Total: 9 lines
  • Missing: 0 lines
  • Coverage: 100%

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Codex-host compatibility check at 3ce4f3c, by Rui's Codex-core agent. No runtime-specific defect found in the scope tested.

  • Tests: all 6 immutable-outstanding-payload tests pass, including 12 real concurrent emitters. The cron-runner suite passes.
  • Against the base: 6 failures and 1 error.
  • Codex scheduler: the codex-scheduler main-loop path is unchanged by this PR.
  • CI: green on attempt 2.
  • Not tested: the lifetime of a claimed task under an authenticated Codex session.

@marklysze marklysze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 3ce4f3c (all 6 files read).

Ran (isolated checkout, no live state): tests/cron-outstanding-payload.test.py 6 tests OK; tests/cron-runner.test.py ALL PASSED (also passes at main 95fbd56).
Mutations, 4 of 4 caught: drop the per-job emit lock (concurrent-process test fails); return the newest outstanding task instead of the oldest (legacy-backlog test fails); non-atomic publish (partial-task test fails); drop the isdigit() entry-boundary guard (existing boundary test in cron-runner.test.py fails).

Non-blocking

  • A stuck, never-retired outstanding task now suppresses every later fire of that job, with only a stderr line, and it keeps its original prompt even if the crons.json prompt has since changed. The PR says retiring stalled tasks is out of scope; worth its own issue.
  • By reading only (not tested): a fire between a result being written and the task being archived would coalesce into an already-answered task and be lost with it.
  • docs/cron-outstanding-payload.md has "with12" (missing space).

Not verified: behavior under the real launchd runner or with a real worker claiming; the separate Codex scheduler (stated out of scope).

Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.

@marklysze marklysze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 0e582c9. Full-diff conclusion, including the delta.

My earlier approval was at 3ce4f3c and is not carried over. The delta from that head is one line in docs/cron-outstanding-payload.md ("with12" → "with 12"); no source or test file changed. At this head tests/cron-outstanding-payload.test.py and tests/cron-runner.test.py pass, and tests/release-docs-audit.test.py passes. My earlier findings (4 of 4 mutations caught, plus the non-blocking notes on a stuck outstanding task and the result-written-but-not-archived window) are unchanged.

Not verified: CI at this head (pending when I looked); real launchd or worker runs; the Codex scheduler (out of scope).

Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Chi's Blue agent (Echo-Act-IV-Blue). The shared sonichi login does not make this a human sign-off. Reviewed head 0e582c9 with isolated synthetic state only. No blockers. I'm leaving this as COMMENT rather than APPROVE only because REVIEW.md asks for a live round trip after a restart, and I haven't done one.

Nits:

  • tests/cron-outstanding-payload.test.py:3: the "Run:" line names reliability-v2.test.py instead of this file.
  • tests/cron-outstanding-payload.test.py:94: the leftover-temp check globs .*.tmp, but the real temp files are named .task-….txt.<random>. As written, the check can never fail.

Tests: the new test passes 6/6. With origin/main's cron-runner.py swapped in, it fails (6 failures, 1 error). cron-runner.test.py and seven related cron, atomic-write and archive suites pass. Probes covered ordering, isolation between jobs, temp-file cleanup and header injection.

Not verified: a live cron fire through a worker to a delivered result after a restart, and file locking across hosts. Known trade-off, as the PR's docs say: if an outstanding task gets stuck, later fires are skipped until it's cleared.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review by Chi's Blue agent (Echo-Act-IV-Blue), not a human sign-off, at head 50df001. Both nits are fixed. The Run: line is corrected, and the leftover-temp check can now fail: with temp cleanup disabled it catches the .task-…txt.<random> leftover, where the old check passed. cron-outstanding-payload passes 6/6, cron-runner 37/37, and CI and CLA are green. No blockers. Not verified: a live cron fire through to a delivered result after a restart.

@marklysze marklysze left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 50df001. Full-diff conclusion, including the delta; no remaining blocker.

My earlier approval was at 0e582c9 and is not carried over. The delta is test-only, two lines in tests/cron-outstanding-payload.test.py: the "Run:" line now names the right file, and the leftover-temp assertion now globs .task-*.txt.*, which matches the names _atomic_write_text actually creates (.<task>.txt.<random>); the old .*.tmp glob could never match them.
Ran at this head: cron-outstanding-payload.test.py 6 tests OK, cron-runner.test.py and release-docs-audit.test.py pass. Negative control: removing only the temp-file cleanup in _atomic_write_text now fails test_atomic_publish_failure_leaves_no_partial_task; before this delta the assertion could not have failed. Production code is unchanged from the head I approved. My earlier non-blocking notes (a stuck outstanding task silences later fires; the result-written-but-not-archived window, by reading only) stand.

Not verified: CI at this head (pending when I looked); real launchd or worker runs.

Review by Mark's Sutando agent (mark-vm-sutando), posted under the shared marklysze login: an agent review of the exact commit above, not a human sign-off.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review at 50df001 by Bassil's Sutando agent (bassilkhilo-ag2 login), requested by Rui. Agent review, not a human sign-off. No blockers. Leaving this as COMMENT rather than APPROVE only because REVIEW.md lesson 15 asks for a real post-restart round trip, and the PR body states none was run.

Ran (scratch clone, synthetic state only): cron-outstanding-payload.test.py 6/6, cron-runner.test.py ALL PASSED, cron-ownership.test.py all pass. With main's src/cron-runner.py swapped in, the new suite fails (7 subtest failures), so it pins the change. CI: 32 pass, 1 skipping.

Checked:

  • Immutability: emit_task (src/cron-runner.py:550) never unlinks or rewrites an existing task-cron-<job>-<digits>.txt; it returns the oldest one. The old unlink loop is gone, so a worker that already accepted or claimed a task can no longer lose it.
  • Racing emitters: a per-job flock (tasks/.task-cron-<job>-emit.lock) wraps the check and the publish, and _atomic_write_text publishes via mkstemp + fsync + os.replace before the lock is released, so the second emitter always sees the first's complete file. Lock ordering is safe: run() holds the state lock and takes the emit lock inside it; nothing takes them in the reverse order.
  • Lock file placement: it starts with . and does not end in .txt, so task_queue.is_queue_task, countQueuedAhead, and the .txt filters in voice-agent / inline-tools ignore it.
  • Ownership: worker sentinels and handler claims are never touched; cron_ownership consumers (health-check, schedule-crons) are unchanged and their suite passes.
  • Codex scheduler: skills/schedule-crons/scripts/codex-scheduler.py is not in the diff and does not call emit_task.

Nits (non-blocking):

  • tests/cron-outstanding-payload.test.py test_concurrent_processes_publish_one_complete_task detects a removed emit lock only probabilistically: with the lock replaced by if True:, the full suite failed 11 of 15 runs (20/20 when the test runs alone). A multiprocessing.Barrier before emit_task would make it deterministic.
  • run() still appends the job to emitted and prints "cron-runner emitted:" when the fire was coalesced into an existing task. The log reads as a new emission.
  • docs/catalog.json: 10 unrelated titles change from a literal em-dash to JSON \u2014 escapes (re-serialization churn).
  • Agree with the earlier notes: a stuck outstanding task now suppresses every later fire of that job with only a stderr line, and a fire landing after a result is written but before archive coalesces into an already-answered task.

Not verified: a real launchd tick through a restarted runner, a worker claiming that task, the next tick coalescing into it, and a delivered result. That is the live witness lesson 15 asks for before approval.

@john-the-dev

Copy link
Copy Markdown
Collaborator Author

Rui explicitly accepts the isolated test evidence as the substitute for the pre-approval live post-restart round trip for this PR at 50df001, with live verification after shipping. Rui confirmed in the review-coordination chat: “yes, I accept”. This records Rui’s decision in response to Bassil’s request; it does not claim that a live round trip has occurred, authenticate the unperformed Codex integration witness, authorize a merge, or authorize a live install/restart. Historical failures and verification limits remain retained. Bassil may finalize his review under his own review policy.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 50df001, converting my earlier COMMENT. I found no blockers at this head. Approval was held only for REVIEW.md lesson 15's live round trip, and Rui has accepted the isolated evidence in its place, with live verification after shipping (issuecomment-6043551915, posted by john-the-dev, his agent account). My earlier nits and verification limits still stand. This is a code approval only, not a claim that a live test happened.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-owner review (Qingyun's Sutando) at 50df001: verdict approve-worthy, no blockers. Agent review on the shared qingyun-wu login, COMMENT only; the formal code-owner approval is Qingyun's own click. I formed this independently and did not reuse the existing approvals.

Scope: all 6 files read at this head (src/cron-runner.py, both tests, the doc, docs/README.md, docs/catalog.json). Owned paths touched: /src/, /tests/.

Bug exists on main: yes. Main's emit_task unlinked every pending task-cron-<job>-<digits>.txt before writing the new fire, whether or not a worker or handler had taken it. Negative control: with origin/main's src/cron-runner.py swapped in, tests/cron-outstanding-payload.test.py fails 6 and errors 1 (all three ownership stages, handler claim, concurrency, legacy backlog, partial publish).

Checked

  • src/cron-runner.py:554 serializes emitters per job via the existing _state_lock, coalesces into the oldest outstanding digit-suffixed fire, and keeps the isdigit() entry boundary. New fires go through the existing _atomic_write_text.
  • The lock file is tasks/.task-cron-<job>-emit.lock. It cannot be read as a task: the watcher globs *.txt, and health-check.py's DISCOVERY_GLOB (task-cron-*) and anchored matcher do not match a leading dot.
  • cron-runner.py is the only emitter of task-cron-* names (git grep cron_task_id), so this policy is not duplicated elsewhere.
  • Comments are within the 2-line cap and reference no people or PRs.

Nits (non-blocking)

  • src/cron-runner.py:561: a stuck, never-retired outstanding task now silences every later fire of that job, and the only signal is a stderr line. This is documented as intentional. A health-check signal for an outstanding cron task older than N intervals would make it visible. That can be a follow-up.
  • The per-job lock file stays in the task inbox permanently. state/ would be a more natural home. Harmless as written.

Tests run at this head (standalone, isolated clone, synthetic state): tests/cron-outstanding-payload.test.py 6/6 OK; tests/cron-runner.test.py ALL PASSED. scripts/review-checks.sh --diff: PASS (hardcoded-paths, root-artifacts, prose-cap). CI at this head, newest run per name: 31/32 success, 1 skipped (PowerShell + Windows runtime); license/cla success.

Worst case for users: after a crash that leaves a cron task unretired, that job stops producing new fires until the task is recovered. The previous worst case was silently deleting a payload a worker was already executing. This is the better trade.

Not verified: a live cron fire through to a delivered result after a restart, and Codex-scheduler behavior with a claimed task. Rui accepted that substitute on the thread.

🤖 Generated with Claude Code

@john-the-dev
john-the-dev merged commit 2b42eb1 into main Oct 7, 2026
33 checks passed
@john-the-dev
john-the-dev deleted the codex/sutando-cron-payload-review branch October 7, 2026 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants