[tracking202]

Backlog: Q36S

Stacked on #285 (and so #273): merge after them. This branch carries #285 (1.9.79-ltv-maintenance-scheduled, which carries #273) by a merge commit; its own work is the two commits after that merge.

What was wrong (measured on a live instance before changing anything)

202-cronjobs/index.php kept two runs apart by a file's presence: file_exists(), then touch(), the file deleted as stale once it was 600 s old.

• A run longer than ten minutes had a second one started beside it. A run held in its first tier (another session holding 202_cronjobs in LOCK TABLES … WRITE), with cron.lock backdated eleven minutes: the next run deleted the file and ran; both then ran the minute and the hourly tiers, the named locks of LTV maintenance and the Android intake each turning one of them away, and the first run's unlink() warned because the second had removed its file.
• Two runs could both find the file absent. With the window between the check and the touch() widened to 100 ms (scratch copy of the lines), two runs started together both ran.
• A run that died left the file, and the next run (and every page ping for ten minutes) said "Cron job already running".

1.9.79 adds work to the tiers (#273's LTV webhooks, #285's LTV maintenance, #277's GeoIP refresh), which makes a run over ten minutes likelier.

What changed

• Prosper202\Cron\CronLock: flock(LOCK_EX | LOCK_NB) on 202-cronjobs/cron-run.lock, held while the object is, released by its destructor or by the kernel when the process dies. No age, nothing removes the file.
  • take() answers the lock, or null when another run holds it. Anything else throws CronLockFailed: a file that cannot be made, one this user can neither write nor read, a stream without lock support (refused before flock(), whose would-block flag reads a stale errno there — 1.9.79: show the GeoIP database's age, and refresh it with the operator's MaxMind account #277's GeoIpRefresh::tryLock() lesson), or a lock error other than would-block.
  • The file is made with 'x' and then chmod 0644, whatever the umask, and opened read-only when this user may not write it (flock() needs only a read handle). Every open uses 'e' (close-on-exec): measured, without it a process the run started kept the lock after the run had exited.
  • isHeld() (for health.php): an instant's LOCK_SH | LOCK_NB; null when it cannot tell.
• 202-cronjobs/index.php: the old lock lines are replaced by $cronLock = CronLock::take(CronLock::path()) right after the bootstrap, in the top-level try every run goes through, followed by the held branch: error_log('Cron job already running: another run holds …; this run did nothing.') and exit("Cron job already running\n") (exit 0, no run recorded). Both unlink()s are gone. A CronLockFailed lands in the existing catch: cronjob.log, exit 1 with the reason on stderr, 500 over HTTP. The page ping (202-cronjobs/?page_ping=1), the scheduler line (curl or php), the Docker cron services and AutoCron all request or run this one file, so all take the lock.
• 202-cronjobs/health.php: isLocked asks the lock instead of the file's existence (the file now stays between runs, so existence would read "locked" for ever). lockAge, lockAgeFormatted and the "Lock file is stale" recommendation are gone: an flock has no start time, and the stale case cannot happen. A new recommendation when isLocked is null (the web server cannot open or lock the file, so its own runs cannot either).
• .gitignore, .dockerignore, the Dockerfile comment; tests/live/cron-needs-upgrade.sh and reclick-backfill-cron.sh stop deleting cron.lock (the upgrade pass now asserts the lock is free); docs (documentation/api/15-system.md, Cron Status), changelogs.txt, 202-config/changelogs.json.

Decisions a reviewer should check

• Where the file lives: beside the script, as before (202-cronjobs/), under a new name. Every default scheduler (the installer's printed line and install.sh: curl …/202-cronjobs/index.php; both Docker compose files: wget http://web/…; AutoCron) runs as the web server's user, and the Docker image gives www-data the tree. A crontab's php …/index.php as root is the mixed case: root's file is made 0644 even under umask 027 (measured: without the chmod, nobody could not open a root-made file and every page ping would have failed), and the web user locks it read-only (measured across users with runuser -u nobody, both directions). Not a shared temp dir: different users and a PrivateTmp web server would lock different files, and anyone could pre-create one there (CLAUDE.md [Maintenance] Remove deprecated PHP features #16).
• A directory the web server cannot write, with no file there, now refuses the run loudly ("make 202-cronjobs/ writable … or make the file there, readable by every user that runs the cron job"), where the old touch() warned and ran without a lock. That stops the cron on such installs until fixed; the changelog says so and what to do. The alternative, running unlocked, is the fail-open this item removes.
• The old cron.lock: not read and not removed. A new name means an earlier version's leftover file neither blocks nor is unlinked from under a holder (an in-flight old run at deploy would have unlink()ed a same-named file a new run had locked, letting a third run lock a new inode). The live pass runs beside a fresh cron.lock: it runs.
• The named MySQL locks (GET_LOCK(?, 0) in the attribution worker, Android intake, LTV webhooks, LTV maintenance): every one is non-blocking and the outer lock is LOCK_NB, so no process ever waits on either kind and no cycle can form. Their meaning is unchanged: they keep a tier of index.php apart from the same job's standalone entry point; two index.php runs no longer meet there at all. The standalone entry points do not take the file lock.
• Upgrade window: unchanged. connect.php's upgrade stop still runs before the lock (the lock's class comes with its autoloader anyway); tests/live/cron-needs-upgrade.sh passes (77/77) and asserts the lock free after every job refused.
• A run that hangs now holds the cron until its process ends, where after ten minutes a second run started beside it. Each turned-away run logs a line, and health.php's isLocked says a run holds it; but nothing reports how long (no start time is kept: a holder that opened the file read-only could not record one, and a stale one would read as an answer). The tiers each have budgets; a query waiting on a metadata lock or a request without a timeout is the case. Stated in the changelog.
• health.php's probe takes a shared lock for an instant; a run starting in that instant is turned away and the next minute's run makes it good (in isHeld()'s docblock).
• Multi-server installs behind a load balancer: an flock is per filesystem, as the old file was, so each server still runs its own cron. Unchanged; GET_LOCK would cover that and was not what the item asked for.

Tests

• tests/Cron/CronLockTest.php (child processes through tests/Cron/fixtures/cron-lock-runner.php): four runs released by one go-file in the same instant: exactly one runs; a run whose lock file's time is set eleven minutes back (the old lock's only clock; the flock reads none, so no wait or fake clock stands in for ten minutes) keeps the next run out, which runs once it ends; a SIGKILLed run's lock is free at once (the file stays); a process the run starts does not keep it; released when the object goes; the file is 0644 under umask 077; a read-only file is locked read-only; a file that cannot be made, one that cannot be opened (non-root only: skipped as root, run here as nobody, 12/12), a stream without lock support and the same after a stale EAGAIN each throw, never "busy". A runner that waits for the lock instead of being turned away fails within ten seconds rather than hanging the suite.
• tests/Cron/EveryCronRunTakesTheLockTest.php (structural): index.php takes the lock once, directly after its bootstrap in the top-level try, before every tier and the run's record; the held branch is exactly the log line and the exit; $cronLock is named twice (assigned, tested) and the file calls no unlink, unset, extract, $$ or $GLOBALS; nothing else includes index.php, and index.php is the directory's only index; every file outside tests/ that names the lock is listed; no script in tests/live, build or scripts removes it by name; and no other job keeps runs apart by touch()ing a file, with 202-cronjobs/sync-messaging.php listed as the one left (the same lock; a backlog item). The touch() scan was planted in six spellings (touch, \touch, TOUCH, namespace\touch, @touch, in a condition): each caught; $o->touch() and Foo::touch() not.
• tests/Cron/CronEntryPointsFailLoudlyTest.php: the lock-after-bootstrap check reads the new statement.

Plants (each restored and cmp-checked):

• the old index.php lock lines back: 5 tests of EveryCronRunTakesTheLockTest and CronEntryPointsFailLoudlyTest::testTheMinutelyCronTakesItsLockOnlyAfterItHasBootstrapped fail, and the live pass fails 17 of 40, among them "a run eleven minutes into the held one … is still turned away" (it ran the minute tier beside the held run: two minute rows) and, after a SIGKILL, "it was not turned away" (the next run said "already running");
• in CronLock (run as root and as nobody): no close-on-exec, no chmod, no stream_supports_lock() check, every failure read as busy, no read-only fallback, a shared lock taken, a blocking lock, and isHeld() answering false when unsure: each fails a named test (the read-only fallback and the isHeld() one only as non-root, as CI runs).

Verification

Ladder since the stacked base (--since origin/1.9.79-ltv-maintenance-scheduled --changed, run on the first commit; the second changes only tests/live/cron-lock.sh, for which --tier syntax reports PASS):

Scope report
------------
  patterns   PASS
  phpcs      PASS
  phpstan    PASS
  syntax     PASS
  unit       PASS

Not covered by this script:
  live      stand up an instance, drive the path a user would take
  agent     add and run a case under tests/fixtures/agent-eval/cases/
  ci        a workflow still in_progress has not passed; read the run

Report the lines above verbatim. Do not write "tests pass".

Ladder as the brief asks (--since origin/1.9.79 --changed, which includes #273/#285's files):

Scope report
------------
  openapi    PASS
  patterns   PASS
  phpcs      PASS
  phpstan    PASS
  schema     SKIP  set P202_TEST_DB_HOST and P202_TEST_DB_NAME to a SCRATCH database (this tier drops and recreates tables)
  syntax     PASS
  unit       PASS

Not covered by this script:
  live      stand up an instance, drive the path a user would take
  agent     add and run a case under tests/fixtures/agent-eval/cases/
  ci        a workflow still in_progress has not passed; read the run

Report the lines above verbatim. Do not write "tests pass".

The schema tier was then run alone against a scratch database made for it (selected by #285's LtvTables.php, not by this change):

Scope report
------------
  schema     PASS

Unit: 6138 tests, 6 skipped (the four documented UserTest DB-singleton skips; CronLockTest's unreadable-file case as root, run separately as nobody; CronEntryPointsFailLoudlyTest's no-install case, because the live instance's 202-config.php is in the checkout). PHP 8.3.6, MariaDB 10.11.

Live, against an instance on this branch (installed without the licence form, see below):

• tests/live/cron-lock.sh (new): 40/40, three runs in a row. A real run held in its first tier turns away a second command-line run (exit 0, "already running", the log line on stderr), a signed-in page's ping (POST 202-cronjobs/?page_ping=1) and a scheduler's/AutoCron's GET 202-cronjobs/index.php, none recorded, health.php isLocked: true; with the lock file (and a cron.lock) set eleven minutes back, still turned away; the held run then finishes as the one minute run and the one recorded; a SIGKILLed run's lock is free and the next run takes it at once; a run beside a fresh cron.lock runs; a directory where the lock file goes ends the run with exit 1, the reason on stderr, nothing run or recorded.
• index.php started twice in the same instant (go-file), ten times: each time exactly one ran the minute tier (9 times the other was turned away; once it started after the first had finished and found the minute done).
• Web SAPI: a lock taken in a php -S request is free for the next request in the same server process after a normal end, an uncaught Error, a fatal error, exit and max_execution_time.
• tests/live/cron-needs-upgrade.sh: 77/77 (the upgrade window: every job refused, the lock free).
• tests/live/reclick-backfill-cron.sh (only its rm -f cron.lock line changed), after one campaign was created through the API for it: 11/12; the failure is pre-existing and not this change: it greps the log for "stepped over after failing in 10 runs" while ReClickBackfill writes "stepped over after failing with its own error in 10 runs" on origin/1.9.79 as well.

#277: a local merge of this branch with origin/1.9.79-geoip-refresh conflicts in 202-config/changelogs.json (as every 1.9.79 PR), and in 202-cronjobs/index.php and tests/Validation/OutboundUrlGuardTest.php between #285/#273 and #277, not on this change's lines (index.php's lock lines and health.php merge cleanly). With those resolved by keeping both sides: tests/GeoIp/RefreshLockTest.php OK (4 tests, including the hourly-tier ordering test testTheCronJobRecordsItsHourBeforeTheRefresh), tests/GeoIp/ OK (104), tests/Cron/ OK (49, 2 skipped as above), OutboundUrlGuardTest OK (29). #277's comments that the cron reads its lock as stale after ten minutes (GeoIpRefresh's docblock, RefreshLockTest's, the hourly-tier comment) stop being true once both merge; the ordering they protect is still harmless.

What could not run

• The licence stub is down, so the instance was installed by install.php's own account steps without the licence form (the installer's POST, the Go CLI and the seeder were not run). Nothing here touches the licence or the Go CLI; the agent eval and the Go tiers did not run (no Go or agent-facing change).
• CI: not read before opening this.

---

Generated by Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-28T08:58:00.740541Z We couldn’t complete this review. Comment @codex review to try again.	ce1f233	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[github-actions]

Code review: CLEAN

For ce1f233: 0 issue(s). The review is encrypted for the maintainers in the review-findings artifact of this run.

[github-actions]

Adversarial review: NON-BLOCKING

For 8b0b097: 6 finding(s). The report is encrypted for the maintainers in the review-findings artifact of this run.

Rewritten on every push; the check fails on BLOCKING.