Repository navigation
1.9.79: one identity merge that fails no longer stops the attribution worker - #315
Merged
Merged
Conversation
… worker AttributionWorker::requeueMerges() took the merges with requeued_at NULL in merge_id order and ran each in transaction() with nothing around it, so a merge whose transaction threw ended the loop and, the merges being step 1 of run(), the whole run: the model fan-outs, the outbox and the rollup did not run either, and the next run took the same merge first again. Measured on an instance from origin/1.9.79: a merged person of 65,534 visitor keys ahead of two ordinary merges failed every run with "too many placeholders" (errno 1390), and neither the merges after it nor a new conversion were ever processed. Each merge now takes its turn (202_identity_merges.requeue_retry_at, the merges never tried first). A failure is caught outside the transaction, which transaction() has rolled back whole (CLAUDE.md #34), recorded on the row (requeue_failures, requeue_error) and passed; it is retried on the next run, then after a minute, doubling to an hour. Only a failure the merge's own data determines ends its retries: a person with more visitor keys than one prepared statement can name (65,533, the statement naming the account twice besides; 65,536 values are refused). That is checked before the statement rather than read from the error, which once came back with no errno at all, and the merge is retired (requeue_retired_at, with why) in its own transaction. A lock wait, deadlock, lost connection or missing table is retried for as long as it lasts and never retires a merge. A rollback that failed still ends the run. GET /attribution/queue (p202 attribution queue) adds merges_failing, merges_retired and merge_rows; merges_awaiting_requeue no longer counts a retired merge. The Attribution page warns when merges are failing or retired, the worker's summary line counts them, and attribution-worker.php --retry-now makes failed and retired merges due again. The 1.9.78 -> 1.9.79 rung reconciles 202_identity_merges for the four columns and requeue_due. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Contributor
Brings in 1.9.79's new commits. The conflicts were in 202-config/changelogs.json, 202-config/functions-upgrade.php, changelogs.txt, where both sides added lines: the incoming lines are kept as they are and this branch's follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRoS2SViuxaucFaKWsFnLr
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.
Backlog: K0NM
What was wrong (measured)
AttributionWorker::requeueMerges()took202_identity_merges WHERE requeued_at IS NULL ORDER BY merge_id LIMIT 100and ran each merge intransaction()with nothing around it. A merge whose transaction threw ended the loop and, because the merges are step 1 ofrun(), the whole run: the model fan-outs, the outbox and the rollup did not run either. The next run took the same merge first and threw again.Measured on a live instance served from
origin/1.9.79's tree: one merged person with 65,534 visitor keys (merge 1) ahead of two ordinary merges made by the real identity graph, and a conversion recorded after them.attribution-worker.php, twice:QueryException: Failed to prepare MySQL statement: Prepared statement contains too many placeholders [errno 1390], exit 1.index.php: loggedAttribution worker failed: … [errno 1390], exit 0.requeued_atNULL, their conversions' journeys were still one touch, and the new conversion was still in202_attribution_pending.What can fail inside a merge, and whose failure it is
I read the three calls and decided from the code:
keysOfPerson(): two plain reads of202_identity_visitors. It answers[]for a key with no row, never a throw of its own. Its failures are the server's (a lost connection, a missing table).conversionsOnKeys(): oneIN (…)withcount($keys) + 2placeholders. A prepared statement carries at most 65,535 values: 65,536 or more is refused at prepare, measured on MariaDB 10.11. That is the one failure the row determines. A person's keys only grow (IdentityGraph::merge()only adds aliases; only the account purge deletes them, and it deletes the merges too), so every run would meet it again. Everything else here is the server's.enqueue(): an upsert into202_attribution_pending, which the ledger writes inside every conversion's transaction. A lock wait or deadlock there is the server's (CLAUDE.md Fix undefined mysql array keys #34). Its values (conv_idint unsigned from202_conversion_logs, a 14-character reason) cannot be refused by a row.UPDATEofrequeued_at: the server's.The row-determined case is checked before the statement (
count($keys) > MAX_PERSON_KEYS), not read from the error. On one connection, a 65,536-placeholder prepare right after a 65,535 one came back false with errno 0, so an errno test would have read it as the server's.What changed
202_identity_mergesgainsrequeue_failures,requeue_retry_at(0 until a failure),requeue_error,requeue_retired_atandKEY requeue_due (requeued_at, requeue_retired_at, requeue_retry_at).requeued_at IS NULL AND requeue_retired_at IS NULL AND requeue_retry_at <= now ORDER BY requeue_retry_at, merge_id. Merges never tried come first, then the one whose turn came first.requeue_duewith no sort (ANALYZEon MariaDB 10.11, 50,300 merges: range onrequeue_due, 100 rows read forLIMIT 100).transaction(), never inside its callback.transaction()has already rolled the merge back whole.requeue_failures + 1,requeue_error,requeue_retry_at) and the loop goes on.mergeBackoff(), as 1.9.79: the pending-click settler takes installs in turns, so installs that cannot settle no longer stop the rest #309's).WorkerHalted): the handle may still hold writes, so nothing more runs on it.MAX_PERSON_KEYS(65,533) is retired inside the merge's own transaction:requeue_retired_atand the reason inrequeue_error. It is logged, not tried again, and nothing of it is queued.202-cronjobs/attribution-worker.php --retry-nownow also makes failed and retired merges due again. It is the existing recovery switch, and the way back once a later version can read such a person.GET /attribution/queue(p202 attribution queue,attribution:queue) addsmerges_failing,merges_retiredandmerge_rows(merge_id, merged_at, failures, last_error, retry_at, retired_at; retired first, up tolimit).merges_awaiting_requeueno longer counts a retired merge, since nothing will re-queue it.merges re-queued N (F failed, R retired)when F or R is non-zero.index.phpnow prints the line when a merge failed or was retired, even if nothing was processed.GET /system/infoandp202 systemcarry no attribution health today, so nothing was added there.IdentityTables::identityMerges(). The 1.9.78 → 1.9.79 rung adds that one definition to its existing_upgrade_measurement_tables([...])list, with a one-line comment, as 1.9.79: the pending-click settler takes installs in turns, so installs that cannot settle no longer stop the rest #309 does forAppTables::appInstalls(). Whichever of the two merges second has a one-line conflict there.AttributionQueue, the path's description),documentation/api/13-attribution.md,features/advanced-attribution-engine.md(failure handling), the troubleshooting guide (a row and the summary line),docs/cli-agent.md. Changelog lines in both files.AttributionWorker.php(checked everyorigin/1.9.79-*against its merge base), so this is not stacked.Tests
tests/Attribution/MergeRequeueFailuresIntegrationTest.php(new,@group integration, on the real schema, the real identity graph and the real ledger):The next run does not take it, and the queue counts
0 awaiting, 0 failing, 1 retiredand lists it.requeued_atwrite, after its conversions were enqueued), ahead of a good merge:innodb_lock_wait_timeout = 1): the merge after it runs, and the failure is recorded with[errno 1205], not retired. The next run re-queues it.Plants: each was backed up to a fresh
mktemp, compared withcmpbefore planting, run against this suite, and restored and compared again. Each counts only when a named test fails, not on a lint error.WHERE requeued_at IS NULL ORDER BY merge_id)MAX_PERSON_KEYSone below (65,532)MAX_PERSON_KEYSone above (65,534)merge_rowsmisses retired mergesLive pass
The instance was served from this worktree with
php -Son the router. It was installed withinstall.php's own steps but not its licence check, because the stub is down, with every proxy variable at 127.0.0.1:9.MysqlConversionRepository::record()) and the merges through the real identity graph (IdentityGraph::attachClick()on clicks carrying both visitors' cookies). The click rows and the 65,534-key person were written by SQL.origin/1.9.79):attribution-worker.phptwice, thenindex.php. The results are in "What was wrong".202_versionwas wound back to 1.9.78, and the upgrade ran through the upgrade page (GET the form, POST its token). It saidSuccess!, logged noProsper202 upgradeline, and added the four columns andrequeue_due. The upgradedSHOW CREATE TABLE 202_identity_mergesis identical to a fresh install's.attribution-worker.phpprintedprocessed 5 (credited=5); merges re-queued 2 (0 failed, 1 retired); …, exit 0.requeue_error, and logged once.merges re-queued 0).mysqlsession held an uncommitted outbox row of a conversion of the first of two new merges, for 70 s, under the default 50-second lock wait.merges re-queued 0 (1 failed, 0 retired). The merge holdsrequeue_failures 1and…Lock wait timeout exceeded… [errno 1205], due on the next run. The wait used up the run's 50-second budget, so the second merge waited for the next run.requeued_at9 s earlier), then the retried one once the lock was released:merges re-queued 2, both journeys rebuilt.GET /api/v3/attribution/queueansweredmerges_failing,merges_retiredandmerge_rowswith the reason. Signed in, Account › Attribution showed the strip: "1 merge failed to credit its conversions again; each is tried again, less often the longer it fails; 1 merge cannot credit its conversions again and is not tried again (p202 attribution queue shows why)." The strip is absent when none are failing. The page was read again on the final commit.attribution-worker.php --retry-nowprinted0 failed row(s) and 3 failed identity merge(s) made due again. The run re-queued the two and retired the wide one again.index.phpprintedAttribution: processed 0 (none); merges re-queued 0 (0 failed, 1 retired); ….Verification
.claude/skills/p202-verify/scripts/verify.sh --since origin/1.9.79 --changed(schema tier on a scratch MariaDB 10.11 database), on the committed branch:The unit tier ran 6104 tests with 4 skipped (the documented "User class requires DB singleton" skips). Beyond the script:
@group integrationsuite (tests/run-integration-suites.sh, each on a fresh scratch database): 146 suites and 1370 tests passed, and 1 suite failed. The failure wastests/Repository/MysqlLookupRepositoriesIntegrationTest.php::testTheFactoryCachesThroughTheBootstrapHelpers, withClass "Memcached" not found, because this sandbox's PHP has no memcached extension. The run includes the new suite and every Attribution, Identity, Ltv, Upgrade and Schema suite.tests/live/upgrade-equals-install.sh(1.9.55 on PHP 7.4, upgraded through the upgrade page, compared with a fresh install): 25 passed, 0 failed. Its notable lines:The 1.9.55 upgrade creates
202_identity_mergesfrom today's definition in an earlier rung, so this run does not exercise the new rung entry. The live pass above does: a table in 1.9.78's shape, upgraded through the page, then compared.ANALYZEof the due-merge selection (above).What could not run, and what ran without the licence
install-instance.shand the Go CLI could not run against the live instance: the licence stub is down, and an instance without the licence answers the CLI 402. The live pass drove the API with curl and the cron jobs on the CLI. The queue fields render throughrender()in the Go CLI and$this->render()in the PHP CLI, generic JSON, and neither CLI was run here.tests/live/upgrade-equals-install.shran through a copy that differs in two ways:install.php's steps without the licence check, the same scratch installer as the live pass, instead ofinstall-instance.sh;Steps 1 (1.9.55 installed on PHP 7.4), 3 (the upgrade page) and 4–5 (schema and rows) are the script's own.
WorkerHaltedpath for a rollback that failed (no way found to makerollback()fail on a live handle);SIGNAL,innodb_lock_wait_timeout).GET /attribution/queueand has no new agent task.For the reviewer
processPending(), aQueryExceptionstill ends the run with every row untouched. For merges, the brief's rule applies: record, back off, walk past. The reason is that a row-determined failure can be aQueryException(this one is), and the outbox's rule turns that into a head-of-line block. The same is true of the outbox itself (see "Found outside this item").--retry-nowbrings it back.requeue_failuresis kept after a merge finally succeeds, as history. It is counted as failing only whilerequeued_atis NULL.index.php(50 s forattribution-worker.php) is no longer than InnoDB's default 50-second lock wait. In the live pass, one lock wait used the whole run. The next run then took the fresh merge before the retry, which is the turn order working. The budget itself is unchanged.Found outside this item
AttributionWorker::processPending():JourneyBuilder::build()names every key of the person in one statement (count + 6placeholders). A conversion of a person with more than 65,529 keys fails at prepare. The outbox reads that as the engine's failure (WorkerHalted), andclaim()takes that conversion first on every run, so the whole outbox stops for good. This is the same head-of-line shape, on the outbox.AttributionWorker::fanOutModelRecomputes(): the same loop as the oldrequeueMerges(). A model whose fan-out transaction throws ends the run before the outbox, and is first again on the next run (ORDER BY recompute_requested_at, model_id). I found nothing row-determined in it, so it is the server-failure half only.Generated by Claude Code