[tracking202]

Backlog: 4J3V

The defect

MysqlCompanyRepository::linkUnlinkedCustomers() took up to $limit of an account's customers that have a company string and no company_id. The select had no ORDER BY and no cursor. It skipped, in PHP, every row whose string canonicalName() reduces to '' (a tab, a newline, a no-break space, an ideographic space, NUL). A skip wrote nothing, so the next run's select took the same rows first. The cron asks for 2,000 per account, so an account with more than 2,000 such rows never had anyone else linked, and every run reported company linking: 0 customers attached.

Reproduced both ways:

• On a 50,000-customer account with 3,000 blank strings ahead of 1,880 linkable ones, the old sweep returned 0 on every call and 4,880 rows stayed pending.
• On a live instance, the real 202-cronjobs/ltv_maintenance.php printed 0 customers attached on every run against the old code.

What changed

• The sweep walks with a cursor. The select is customer_id > ? ORDER BY customer_id. A call walks batches of $limit until $limit customers are linked or none are left, so a return below $limit means the account has nothing left to link. That is the contract 1.9.79: the cron job sweeps lapsed LTV subscriptions hourly and runs the rest of LTV maintenance daily #285's do { … } while ($linked === COMPANY_BATCH) loop reads; the old code broke it whenever a batch held a blank.
• Rows that can never link are stored as '', not skipped in SQL. Each batch does this in one guarded UPDATE (company_id IS NULL AND customer_id IN (…) AND company IN (<the strings as read>), updated_at bumped), so they leave the selection for good. Why '' rather than NULL or an SQL filter:
  • SQL filter. Skipping in SQL would need a second reading of canonicalName(). PHP's /\s+/u (with UCP) plus trim() (which also strips NUL), MariaDB's PCRE2 and MySQL 8's ICU REGEXP each disagree somewhere (NEL, NUL, U+200B). If SQL read a row as linkable where PHP reads blank, we are back to the bug; in the other direction, a real name would never link ([Update] Handle missing case parameter #29: one stored value, one parser).
  • NULL or ''? Taking [Setup] Remove undefined variable from template calls #25's readers of the column one by one:
    • company IS NULL (domain auto-attach eligibility, and the CRM detach's IF) is the only reader that tells NULL from ''. It already read a blank string as "not NULL", so not eligible. '' keeps its answer; NULL would make these customers newly eligible for domain auto-attach.
    • company IS NOT NULL AND company <> '' (the sweep, the cron probe, the ABM report, deleteRefusal()) reads both as none.
    • '' is also exactly what clearing an unlinked string on the customer page writes.
  • Already handled by SQL. Pure ASCII spaces were never taken: utf8mb4_general_ci is PAD SPACE, so ' ' <> '' is false (measured).
  • What becomes visible:
    • Such customers no longer appear as blank-named groups in the LTV report's Company (ABM) view and GET /ltv/abm. Measured live: four such groups (tab, newline, NBSP, ideographic space) before the sweep, none after.
    • GET /ltv/customers/{id} reads their company as "".
  • Who writes these rows today. No current writer stores such strings. Verified: POST /ltv/customers with "company": " " stores NULL. These are legacy rows.
• The select names its index. I checked rather than assumed. Left alone, MariaDB 10.11 served the cursor query from merged_into: merged_into_customer_id IS NULL holds for nearly every customer of every account, and that index also yields id order. With 100,000 other-account customers below this account's ids, it read 102,000 rows to return 2,000. FORCE INDEX (user_company) uses a range on user_id+company_id+customer_id (InnoDB's implied primary key suffix) and reads 2,000. No new index or upgrade step is needed. user_company comes from the 1.9.70 rung and the table definition, added together with company_id in one ALTER.
• A company id of 0 is an error, not a skip. findByName() can return a row with company id 0, and resolveOrCreate() can return no insert id. The sweep skipped either case the same way, so the customer would be taken again on every run (Fix newline at end of index #11). attachCustomer() returned silently on it, so a CRM save answered as done with the company unattached (Replace all <? as <?php. #1, Upgrade to PHP 8 #5 sibling). Both now throw a QueryException naming the company and the account:
  • At ingest, stampCompanyOnCreate() already catches it and logs it.
  • A CRM save is refused, and its transaction rolls back.
  • resolveOrCreate() itself is unchanged: the Companies page's "add" ignores the id, and its INSERT did land ([Core] Update PHP version check for PHP 8 #13).
• CLAUDE.md Remove Intercom integration #37 records the shape (a row a batch skips is the first row of the next batch) and the index finding. If another 1.9.79 PR lands a Remove Intercom integration #37 first, this needs renumbering.

The cron's probe

The probe in ltv_maintenance.php, and the one in #285's LtvMaintenance::companies(), asks the select's predicate without the cursor. Before this change it found blank strings forever, so the cron called the sweep for them on every run. After the first walk stores them as '', the probe stops finding them, so the probe and the select now ask the same question. ltv_maintenance.php is not changed, and this PR is not stacked on #285.

Interaction with #285 to check: its deadline is asked between calls. A single call links at most 2,000 but walks past any number of blank rows (one bulk UPDATE per 2,000). That is a one-time cleanup per row, but the first call on an account with a very large blank backlog runs past the deadline until it is through them.

Measurements (MariaDB 10.11, local)

	rows read	time
cursor select, unforced (plan: merged_into)	102,000	141 ms
cursor select, FORCE INDEX (user_company), first batch	2,000	2.7 ms
same, second batch (2,000 matches among company-less rows)	24,728	33.5 ms
old select (index_merge, unordered)	2,000	11.5 ms
new sweep, whole account (50k customers, 4,880 pending), one call	49,528 index reads, 11 selects, 1,882 updates	1.2–6.7 s (per-row link UPDATE commits, as before)
new sweep, same account, next call (nothing pending)	45,770	58–112 ms
cron probe, steady state, unchanged (index_merge)	45,770	97 ms

The probe's steady-state cost (reading every company-less customer of the account) is the same before and after this change.

Tests and plants

• tests/Ltv/CompanyLinkingIntegrationTest.php (@group integration, picked up by tests/integration-suites.php):
  • 2,100 unlinkable names (seven spellings) ahead of ' Acme Corp ', run with the cron's 2,000.
  • The limit contract in id order across interleaved blanks.
  • Account and merge scope.
• tests/Ltv/CompanyTest.php: the cleared statement and its binds, the cursor binds, and the three refusals. testCrmUpsertResolvesCompanyEntityForCompanyName now expects the refusal, because the shared fake cannot report an insert id. The success path is covered by the integration suites.
• Planted, one at a time, from a mktemp backup checked with cmp, and restored the same way:

plant	caught by
the whole old repository	testMoreThanABatchOfUnlinkableNamesAhead… (0 linked, expected 1), testACallLinksUpToItsLimit…
sweep skips an id of 0 instead of refusing	testLinkSweepRefusesAnInsertThatAnswersNoCompanyId, …AnEntityRowWithNoCompanyId
blanks not stored as none	testLinkSweepStampsTheEntityAndStoresBlankNamesAsNone + 2 integration tests
attachCustomer() returns on an id of 0	testCrmUpsertResolves…, testAttachCustomerRefuses…
a call links past its limit	testACallLinksUpToItsLimitInIdOrderAndTheNextGoesOn
no ORDER BY	testLinkSweepStampsTheEntityAndStoresBlankNamesAsNone
cursor removed (customer_id > ? * 0)	missed

The missed plant is expected. With every taken row leaving the selection, the cursor is not needed for correctness: it keeps one call from re-reading rows it has walked, and it bounds the loop if a guarded write ever misses. I did not add an engine-counter assertion (Handler_read_next), because I cannot run it on CI's MySQL 8.0 here.

Verification

.claude/skills/p202-verify/scripts/verify.sh --since origin/1.9.79 --changed (PHP 8.3.6, full vendor/):

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

(unit: 6107 tests, 0 failures, 4 skipped: the documented "User class requires DB singleton" skips)

Integration, each against a database created for the run (MariaDB 10.11):

• CompanyLinkingIntegrationTest: OK (3 tests).
• LtvDatabaseIntegrationTest: OK (43 tests).

Live, end to end (without the licence):

• Install. The licence stub is down, and install.php itself checks the key ("We couldn't confirm that license key"). The instance was installed by a scratch script that runs install.php's own steps after that check: INSTALL::install_databases(), InstallAccount::create(), and a 202_api_keys row. It was served with php -S through the CI router.
• Before the sweep:
  • POST /ltv/companies created Acme Corp (acme.com).
  • POST /ltv/customers with an ideographic-space company stored NULL.
  • Legacy-shaped rows were seeded by SQL (the only way such rows exist): 2,100 blanks, plus 'Legacy Co ' and 'acme corp'.
  • GET /ltv/abm listed four blank-named companies.
• Old code: php 202-cronjobs/ltv_maintenance.php printed company linking: 0 customers attached on two runs.
• New code: it printed 2 customers attached, then 0 on the next run.
• Database after: 2,101 customers with company ''; Legacy Co created and linked; acme corp linked to Acme Corp; the API's NULL customer still NULL.
• API after:
  • GET /ltv/abm lists only Acme Corp and Legacy Co.
  • GET /ltv/companies shows one contact each.
  • PATCH /ltv/customers/{id} with pat@acme.com and no company answered company: "" and did not attach, as before for the blank string.

Not run:

• MySQL 8.0, where CI runs the integration suite: the FORCE INDEX plan and PAD SPACE behaviour there are read from the docs, not measured.
• The Go CLI (402 without a licence; it is not on this path).
• The LTV page's Company (ABM) view in a browser: the same abmBreakdown() was read through GET /ltv/abm.
• tests/live/upgrade-equals-install.sh: there is no schema change.

For a reviewer to check

• '' vs NULL for the stored-as-none rows (above).
• The clear bumps updated_at. The link UPDATE beside it does not, and never did. I left that as it was.
• The thrown QueryException in attachCustomer() changes a CRM save with an unresolvable company id from a silent 200 to a 500. That needs a driver answering no insert id, or a 202_companies row with id 0.

---

Generated by Claude Code

[Copilot]

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

[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-28T09:35:24.938236Z We couldn’t complete this review. Comment @codex review to try again.	27e092a	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.

[github-actions]

Code review: CLEAN

For 27e092a: 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 e1307f0: 5 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.