[tracking202]

Backlog: BT5Z

Stacked on #299 (and so on #285 and #273): merge after them. This branch is cut from 1.9.79-cron-lock (#299's head, ce1f233b, still its head when this was opened), with origin/1.9.79 (#263, #266) merged in by a merge commit; its own work is 17dc3d98.

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

202-cronjobs/sync-messaging.php kept its runs apart with the lock index.php had before #299: file_exists(), then touch() of 202-cronjobs/messaging-sync.lock, deleted as stale once 600 s old and on shutdown. And nothing kept it apart from index.php's hourly DeliverPendingAnalytics().

They do the same work. Both call MessagingService::deliverPending() over the same rows: every active user with a pending event in 202_messaging_events or a dirty 202_messaging_attributes row, each user's events sent to the portal's POST /track and marked sent, or sync_attempts + 1 on a failure (five and the event is dropped), with 202_messaging_sync stamped. Two deliveries at once read the same pending rows before either marks them, so each event goes out twice, and while the portal is down each failure costs two of an event's five attempts for one outage.

Measured with tests/live/sync-messaging-lock.sh (below) run against #299's files. A stand-in portal on loopback held each /track request, so a run was held mid-delivery. 26 of 58 checks failed:

• A run eleven minutes into a held one ran beside it: the old lock deleted the backdated file and sent the same two events again (portal asked twice).
• index.php's hourly run beside a delivering sync-messaging.php sent them again (Analytics delivery: 1 delivered, portal asked twice).
• sync-messaging.php beside index.php's delivering hourly run sent them again (portal asked twice).
• A run killed with SIGKILL left messaging-sync.lock: the next run said "Messaging sync already in progress" and the events stayed pending.
• Two runs started in the same instant: the old lock turned the second away. Its race was not hit without widening the window.

What changed

The overlap is under one lock of its own, not the cron job's.

• MessagingService::deliverPendingExclusive($db, $lockPath) takes a CronLock on $lockPath and delivers while holding it. It answers null while another run holds the lock.
• deliverPending() is now private, so nothing delivers the backlog without the lock.
• sync-messaging.php and index.php's DeliverPendingAnalytics() both call it with CronLock::path(CronLock::ANALYTICS_DELIVERY_FILE), which is 202-cronjobs/analytics-delivery.lock.

CronLock's guarantees carry over unchanged (same class, a second file name):

• no age check;
• 'e' (close-on-exec) on every open;
• the file made 0644 and opened read-only when this user may not write it;
• stream_supports_lock() checked before flock();
• LOCK_EX | LOCK_NB;
• a file that cannot be made, opened or locked throws CronLockFailed, never "busy".

CronLock::path() now takes the file name (default FILE), and the class docblock names both locks.

What each job does now:

• sync-messaging.php: the presence lock is gone. A run that finds the lock held writes Analytics delivery already running: another run holds <path>; this run did nothing. to the error log, says "Analytics delivery already running" and exits 0, as index.php's held branch does. A CronLockFailed lands in its existing catch: "Analytics delivery failed: the cron job could not make its lock file …", exit 1.
• index.php: while the lock is held, DeliverPendingAnalytics() echoes Analytics delivery: another run holds the lock; skipped and the hourly tier goes on. A lock failure is logged as Analytics delivery failed: … (its existing catch) and the tier goes on with its other work.

Housekeeping: .gitignore, .dockerignore, the Dockerfile comment, the track.php docblock, 202-config/Messaging/CENTRAL-API.md (operational note), documentation/api/15-system.md (the cron lock section #299 added, in its wording), changelogs.txt and 202-config/changelogs.json ("Fixed:"), and tests/live/README.md.

Decisions a reviewer should check

• Its own lock file, which both jobs take, rather than cron-run.lock. sync-messaging.php does only the delivery; index.php does the delivery once an hour among everything else. Sharing cron-run.lock would have costs in both directions:

  • A sync-messaging.php run every 5 minutes would hold back index.php's minute tier (click rollups, LTV webhooks) and every page's ping for as long as it delivers. With the portal down that is up to three users × two tries × the client's 10 s timeout.
  • Any index.php run in progress, a page ping's included, would turn away a sync-messaging.php run that starts beside it.

  With the overlap under its own lock, neither holds the other's unrelated work. This also matches how the tree keeps the attribution worker, the Android intake and the LTV jobs apart from their index.php tiers: a per-job lock (runExclusive()) taken by both. It is an flock() and not GET_LOCK() because the item asks for a CronLock. Nesting is safe: index.php takes the delivery lock while holding cron-run.lock, and sync-messaging.php takes only the delivery lock. Both are LOCK_NB, so nothing waits and no cycle can form.

• A new file has to be openable. An install that followed 1.9.79: the cron job holds an flock() for the whole run #299's advice for a web server that cannot write 202-cronjobs/ (make an empty cron-run.lock by hand) now also needs an empty analytics-delivery.lock. Without it, the hourly delivery logs Analytics delivery failed: the cron job could not make its lock file … every hour and delivers nothing, while the rest of the cron runs. The changelog says so. The alternative, delivering unlocked, is the fail-open this item removes.

• When index.php's hourly tier skips its delivery, it still records the hour. The holder is delivering the same backlog. Events recorded after the holder listed its users wait for the next run of either job.

• deliverPending() made private rather than a structural ban on calling it. The two tests that called it now call deliverPendingExclusive() with a lock of their own in the temp dir.

• The old messaging-sync.lock is not read and not removed (as 1.9.79: the cron job holds an flock() for the whole run #299 did with cron.lock). A leftover from a killed old run can be deleted; the changelog says so.

• The integration test runs the real script on the checkout's real lock file. Its runner boots connect.php against the scratch database, as account-scope-runner.php does: the DB class reads globals the runner sets. A shutdown check fails the run with exit 2 if the script ran on any other database. It needs a 202-config.php, which tests/run-integration-suites.sh writes in CI.

  • The eleven-minute case sets every *.lock in 202-cronjobs/ 660 s back, so it reads the old lock's clock whatever file a run takes.
  • The cannot-be-made case moves the real file aside and restores it in tearDown.

• LocalHttpRecorder (shared with the messaging tests) changes in two ways. It can hold its answers (hold() / release(), at most 8 s, inside MessagingClient's 10 s timeout so a held request is never retried). It also runs 4 workers. The hold loop calls clearstatcache(); measured, is_file() in a loop keeps answering true for a removed file without it.

Tests

• tests/Cron/OneAnalyticsDeliveryAtATimeIntegrationTest.php (new, @group integration) runs 202-cronjobs/sync-messaging.php itself in child processes (tests/Cron/fixtures/sync-messaging-runner.php). It uses a scratch database with the schema installed and a stand-in portal that holds each /track request:
  • Three runs released by one go-file: one delivers and two are turned away (exit 0, the log line). The portal is asked once, and each event is marked sent once with no attempt spent.
  • A run held mid-delivery with every lock file 660 s old keeps the next run out. The next one, after it finishes, finds nothing pending.
  • A SIGKILLed run's lock is free at once, and the next run delivers what it never marked.
  • A directory where the lock file goes means exit 1 with the reason, the portal asked nothing, and the events pending with 0 attempts.
• tests/Cron/EveryCronRunTakesTheLockTest.php:
  • PRESENCE_LOCKS is gone, so the touch() scan now holds the whole tree to none.
  • New testTheAnalyticsDeliveryIsMadeOnlyUnderItsLock holds four things:
    • deliverPending() is private (reflection);
    • deliverPendingExclusive()'s body: take, null when held, deliver while holding;
    • the only files whose code names deliverPending* are MessagingService, index.php and sync-messaging.php;
    • both callers' exact statements: the same ANALYTICS_DELIVERY_FILE path, and the held branch.
  • NAMED_BY lists the two new files that name a cron lock, and the file-name regex and the rm check cover analytics-delivery.lock.
• tests/Cron/CronLockTest.php: path(ANALYTICS_DELIVERY_FILE) is beside the cron job. EveryCronJobIsScheduledByDefaultTest: sync-messaging.php's entry point is deliverPendingExclusive. MessagingServiceIntegrationTest: its two delivery calls use the locked entry point.

Plants (each backed up to a fresh mktemp, restored and cmp-checked, the suites re-run clean after):

Plant	Structural (EveryCronRunTakesTheLockTest)	Integration (OneAnalyticsDeliveryAtATime…)
The old script back verbatim (and deliverPending() public again, as it was), each case run alone	—	the eleven-minute case fails on behaviour (the second run delivered beside the first); the killed-run case (next run "already in progress", events left pending); the cannot-be-made case (it delivered); the together case only on its message (the old race not hit)
No lock in deliverPendingExclusive()	1 fails	3 of 4 fail
The lock released before the delivery (unset($lock))	1 fails	2 of 4 fail
index.php delivering under CronLock::path() (the cron's own lock: a second take() of it in the process that holds it answers null, executed, so the hourly delivery would always skip)	1 fails	passes (it does not run index.php; the live pass does)
sync-messaging.php reading a held lock as nothing to do	1 fails	2 of 4 fail

Verification

Ladder on the working tree, just before committing (--changed):

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), on the commit before origin/1.9.79 was merged in. It includes #273/#285/#299's files: their Go-tested files selected go and golangci, which ran under a toolchain already in the shared cache, and #285's LtvTables.php selected schema:

Scope report
------------
  go         PASS  ran under CI's go1.25.14 via GOTOOLCHAIN (local go is 1.24)
  golangci   PASS  ran under CI's go1.25.14 via GOTOOLCHAIN (local go is 1.24)
  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:

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

After merging origin/1.9.79 (#263, #266), the PHP tiers again (--since origin/1.9.79 --tier syntax --tier phpstan --tier phpcs --tier unit --tier patterns). Unit: 6210 tests, 6 skipped: the four documented UserTest DB-singleton skips, CronLockTest's unreadable-file case as root, and CronEntryPointsFailLoudlyTest's no-install case (the live instance's 202-config.php is in the checkout). PHP 8.3.6.

Scope report
------------
  syntax     PASS
  phpstan    PASS
  phpcs      PASS
  unit       PASS
  patterns   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".

Integration, run alone on freshly created scratch databases, after the merge (MariaDB 10.11, PHP 8.3.6):

• OneAnalyticsDeliveryAtATimeIntegrationTest: OK (4 tests).
• MessagingServiceIntegrationTest: OK (8 tests, 259 assertions).

Live, on an instance from this branch. It was installed with install.php's account steps without the licence form, because the licence stub is down. php -S ran on 8169 with every proxy variable pointed at the closed port 127.0.0.1:9. The jobs ran from the CLI as a crontab runs them; no web request is involved.

• tests/live/sync-messaging-lock.sh (new): 58/58, three runs in a row, and again after the merge. It covers the six steps above, plus index.php's hourly tier with the lock unmakeable (logs it, finishes the tier, exit 0).
• Against 1.9.79: the cron job holds an flock() for the whole run #299's files: 32 passed, 26 failed, as listed under "What was wrong".
• sync-messaging.php started twice in the same instant (go-file), ten times, against a portal that answers at once: each time one delivered, the other was turned away, and the portal was asked once.

What could not run

• The licence stub is down, so the installer's licence step, its POST, the Go CLI and the seeder did not run. Nothing here touches the licence, the API or the Go CLI.
• No agent eval: no agent-facing change. The Go tiers ran only because the --since origin/1.9.79 ladder selected them for 1.9.79: the minutely cron sends LTV webhooks, within a budget #273/1.9.79: the cron job sweeps lapsed LTV subscriptions hourly and runs the rest of LTV maintenance daily #285's files; this change has no Go.
• CI was not read before opening this.

Found outside this item

• 202-account/ajax/messaging/track.php's page delivery (MessagingService::sync(), per user, throttled by 202_messaging_sync.last_sync) is not under this lock. A page's sync and the cron's delivery for the same user can still both read that user's pending events before either marks them. The throttle stamp narrows the window but is not a lock. Not changed here; a backlog candidate.

---

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-28T11:55:14.851792Z We couldn’t complete this review. Comment @codex review to try again.	72ccf2f	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]

Adversarial review: NON-BLOCKING

For 9aec2d1: 4 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.