A +98/-16 Hotfix Restored removedOn Instead of Wrapping a Transaction

The reconcile only ever retries removed rows. That’s the whole bug in one sentence, and it took a half-written domain to find it.

The ticket came in as a report. Domain_SetForCustomer can fail after the code has already cleared the domain and updated the row, and nothing rolls back. The reconciliation sweep that’s supposed to catch broken domains only looks at rows where removedOn IS NOT NULL. Once this bug fires, the row no longer matches, so the retry can’t see it. The account sits broken, invisible to the one process built to fix it.

The three writes

The function is DomainReconnectRemoved() in an internal admin reconcile page. It does three separate writes with no transaction around them:

1. UPDATE app.customers SET sitemode = ...
2. Un-retire the domain row: UPDATE app.domains SET removedOn = NULL WHERE domainID = ...
3. Domain_SetForCustomer(uname, dom, byLabel, 'reconnect'), which calls the SetCustomerWebDomain stored proc and sets customers.webdomain

If step 3 fails, the code returns early:

if(domErr) { res.ok = false; res.action = 'reconnect-failed'; ... return res; }

Steps 1 and 2 are already committed by then. The account ends up with an active domains row, a flipped sitemode, and no webdomain. The reconcile’s retry query filters on removedOn IS NOT NULL, and the un-retire in step 2 is exactly what drops the row out of that query. The failure erases its own trail back to the fix.

Why reordering doesn’t work

The obvious fix is to run the risky call first and commit the safe writes only after it succeeds. Two things in the same proc rule that out. SetCustomerWebDomain reads customers.sitemode to decide whether the account is client-managed, so step 1 has to happen before step 3 runs. And the un-retire in step 2 is what makes the row active, which is what makes step 3’s UPDATE hit the existing row instead of inserting a duplicate. Both orderings are deliberate. Swapping them trades a resumable failure for a data-integrity one.

The legacy application runs on a SQL Server backend with no ORM, so there’s no ambient unit of work to lean on. Wrapping all three writes in one SQL transaction was the other option on the table. Step 3 goes through a stored proc that other paths also call, and putting a transaction around a proc call we didn’t fully control felt like the wrong risk to add in a hotfix.

What shipped: compensating undo

Before touching anything, the fix captures the row’s original removedOn and the account’s original sitemode. If the SetCustomerWebDomain call fails, both get put back before the function returns:

• removedOn is restored to its original value, not GETDATE(). A fresh timestamp would restart the 30-day reconnect window and make an old removal look recent.
• sitemode is restored so the account isn’t left flagged as something it isn’t.

That’s a rollback done as application-level compensation instead of a database transaction. The goal was to make the retry query true again. Put the row back exactly where the reconcile expects to find broken rows, and the existing sweep does the rest. No new detection path, no new query.

There’s one more layer. If the undo itself fails, that goes into res.error explicitly and gets carried into the domains@ notice instead of being swallowed. A failed undo leaves the same half-written row this ticket is about, so it shouldn’t fail silently a second time.

The neighboring bug we left alone

While in that file, we found that when Cloudflare or relay registration fails partway through a reconnect, the failure messages are collected into cfMsgs and reported in res.error, yet the function still returns action: 'reconnected'. Partial success is reported as full success, one line away from the code we were changing. It’s the same category of bug, and it stayed out of this hotfix. It’s a different behavior change with a different blast radius, and this ticket already held two defects. We filed it as a follow-up. A hotfix that starts absorbing adjacent bugs is how a one-file, +98/-16 change becomes something nobody wants to review at 5pm.

The second defect that did ship alongside it: the domains@ notification emails build their HTML by string concatenation through a Clean() function that, despite the name, doesn’t escape anything. It only turns nulls into empty strings. A real HTMLEscape sits in the same file and nothing was using it for these values. The field is staff-writable and the reader is staff-only, so it wasn’t urgent on its own. It touches the same lines as the write-path fix, so it went out together with it.

We shipped one hotfix branch touching one file, and verified it against production by reading the deployed file on the box after release. The reconcile sweep didn’t need a single changed line. It was already correct, and it only needed the data handed to it to stop misreporting its own state.

What to check on any multi-step write

For any operation that can fail partway, look at the state it leaves behind, then ask whether the process meant to repair that state can still see it. Here the recovery sweep only retried removed rows, so a half-finished change hid the account from it. Restoring the original values on failure was enough to bring the account back into the sweep’s view.