DEV Community

Ender Yentar
Ender Yentar

Posted on Originally published at mailflat.net

Your backfill is a photograph

We were getting ready to make a column NOT NULL. Standard preparation: count the rows that would violate it. The count was not zero.

Seven API keys had no owning organization. That looked like a key problem, so we went looking at how keys are created.

It was not a key problem.

The keys were innocent

All seven belonged to the same account. And that account had no organization either. Widening the query, two accounts were in that state. Both had signed up recently, within the same three week window.

That reframed the question. Keys read organization_id off the user who owns them. If the user has none, every key that user creates is born without one. The keys were downstream of something else.

The gap

Two weeks earlier we had run a migration that created an organization for every existing user and linked them up. It worked. Every row we had at that moment was correct.

Then we shipped it and moved on.

Nobody wired the signup path.

The proof took one command:

grep -rn "Organization(" backend/app/
Enter fullscreen mode Exit fullscreen mode

Zero hits in application code. The only places that constructed an Organization were the migration itself and the tests. So from the moment the migration finished, every new account was born outside the ownership chain, and the data drifted a little further from correct with each signup.

Why nobody noticed for two weeks

Because nothing read the column yet.

The organization was groundwork for permission checks that had not shipped. No request failed. No error was logged. No user saw anything wrong. The system behaved exactly as it had before, because the broken part was not load bearing.

This is the uncomfortable shape of the bug: it produced no symptom, and the absence of a symptom is what let it grow. It only surfaced because we went looking for something else, and it had been quietly blocking that something else the whole time.

We had already written the lesson down

While reading the code, we found this in a helper module, in a comment above a completely different function:

# A backfill is a photograph. A dual write is keeping the photograph current.
Enter fullscreen mode Exit fullscreen mode

Someone on this project learned that lesson, wrote it down, applied it to inboxes, and then did not apply it to users. The knowledge was in the repository. It just was not attached to the thing that needed it.

A backfill answers "what is true right now". It cannot answer "what stays true tomorrow". Those are two different jobs and shipping the first one feels like finishing.

The fix we did not make

The obvious fix is to create the organization at signup. We counted the paths that create a user:

  1. Normal email signup
  2. Google sign-in
  3. Two separate billing paths
  4. Admin seeding
  5. The demo system user

Six call sites. Adding the same three lines to six places would have made today's bug six times more likely, not less, because the seventh path gets written next month by someone who never reads this post.

So we took the guarantee off the caller entirely. It is a session level hook that runs before every flush:

@event.listens_for(Session, "before_flush")
def _attach_personal_organization(session, flush_context, instances):
    for obj in session.new:
        if isinstance(obj, User) and obj.organization_id is None:
            # Assign the relationship, not the foreign key. See below.
            obj.organization = Organization()
Enter fullscreen mode Exit fullscreen mode

It only looks at session.new, so flushing an existing user a second time does not open a second organization. If organization_id is already set, it leaves it alone, which is what lets the backfill migration and the tests do their own thing.

Now it does not matter which path creates the user, or whether that path knows organizations exist.

There is a real cost to this and it is worth saying out loud: grep -rn "Organization(" still finds nothing useful in the application code. We traded an explicit call for an invisible one. The mitigation is signposting: the model module says in its docstring that organizations are born in a before_flush event and that grepping will not find the call. If you use this pattern, write that sign, because the next person will grep first.

The test says the same thing out loud. It does not go through the signup endpoint, because that would only prove the endpoint works:

def test_guarantee_does_not_depend_on_the_caller(client):
    s = SessionLocal()
    u = User(email="raw-insert@test.com", username="rawinsert", password="x", plan="free")
    s.add(u)
    s.commit()
    assert u.organization_id is not None
Enter fullscreen mode Exit fullscreen mode

If someone later moves the guarantee back into the signup handler, this test goes red. That is its entire job.

Two things that bit us on the way

You cannot flush inside before_flush. The first version added the organization, flushed to get its id, and assigned it to the foreign key. SQLAlchemy raises "Session is already flushing" and all four tests fail at once. The fix was to stop thinking in ids: assign the relationship, and let SQLAlchemy work out the insert order and the key.

The backfill migration passed locally and failed on Postgres. It used sa.table(), the lightweight construct, which has no primary key definition, so inserted_primary_key comes back empty and indexing it raises. sa.Table() with an explicit primary key column fixes it.

We never would have seen this locally, because our test suite runs on SQLite and Alembic does not run there at all. A small staging box with the production schema caught it. That is a separate story, and it is the same story.

What we do now

After any backfill, three questions before it counts as shipped:

  1. What creates new rows of this kind, and does that path set the field?
  2. If the answer is "nothing reads it yet", what will read it, and when?
  3. Is the guarantee attached to a caller, or to the data?

The first question is the one we skipped. The third is the one that actually fixed it.

Production is clean now: no accounts without an organization, no keys without an owner, no broken links between the two. The proof was taken inside a transaction that was rolled back, so verifying the fix did not create a row to explain later.

We hit this while building MailFlat, where every API key belongs to an inbox, which belongs to an account, which belongs to an organization. A chain is only as good as the day someone forgets to attach the next link.

Top comments (0)