Skip to content
Snippets Groups Projects

Sync changesets in the background instead of in ApplyCampaign

Merged Administrator requested to merge mrn/sync-background into main

Created by: mrnugget

This changes the ApplyCampaign mutation so that it doesn't sync imported changesets in the request path. Instead it flags the changesets as unsynced and enqueues them to be picked up the reconciler. The reconciler then syncs them.

Instead of a enum I simply used a boolean flag, which gives us a handy default value and makes the migration easy.

Since changesets can now exist in the DB without having any external data, I had to loosen the constraints in the API a bit: Title and Body are now nullable (because we don't know what the title/body are until we've synced the changeset, of course).

The downside is that a freshly added but unsynced changeset looks like this in the UI:

screenshot_2020-08-25_11 14 50@2x

It's cool that the processing works already, but the parentheses around the external ID look a bit weird...

@eseliger should we expose "Unsynced" in the GraphQL API so we can check for that? Or something like "SyncStatus"? Although, again, an enum might be overblown.

Merge request reports

Merged by avatar (Jul 6, 2025 2:36pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Created by: codecov[bot]

    Codecov Report

    Merging #13288 into main will decrease coverage by 3.26%. The diff coverage is 73.25%.

    @@            Coverage Diff             @@
    ##             main   #13288      +/-   ##
    ==========================================
    - Coverage   51.92%   48.65%   -3.27%     
    ==========================================
      Files        1492     1472      -20     
      Lines       83359    82637     -722     
      Branches     6876     6558     -318     
    ==========================================
    - Hits        43283    40210    -3073     
    - Misses      36454    38820    +2366     
    + Partials     3622     3607      -15     
    Flag Coverage Δ
    #go 52.72% <73.25%> (-0.03%) :arrow_down:
    #integration 29.16% <ø> (+0.03%) :arrow_up:
    #storybook 14.43% <ø> (ø)
    #typescript 37.23% <ø> (-12.44%) :arrow_down:
    #unit 25.08% <ø> (-8.88%) :arrow_down:
    Impacted Files Coverage Δ
    cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)
    enterprise/internal/campaigns/spec_migration.go 1.65% <0.00%> (ø)
    internal/campaigns/types.go 27.23% <0.00%> (-0.12%) :arrow_down:
    enterprise/internal/campaigns/service.go 76.04% <50.00%> (-0.59%) :arrow_down:
    enterprise/internal/campaigns/syncer.go 77.38% <53.84%> (-0.14%) :arrow_down:
    ...terprise/internal/campaigns/resolvers/changeset.go 73.68% <79.31%> (-0.15%) :arrow_down:
    enterprise/internal/campaigns/reconciler.go 59.70% <84.61%> (+0.51%) :arrow_up:
    enterprise/internal/campaigns/store_changesets.go 82.20% <89.47%> (-0.43%) :arrow_down:
    ...prise/internal/campaigns/service_apply_campaign.go 84.46% <100.00%> (+1.60%) :arrow_up:
    shared/src/util/jsonc.ts 0.00% <0.00%> (-100.00%) :arrow_down:
    ... and 129 more
  • Created by: mrnugget

    I don't think we need it, or did you have a specific use-case in mind?

    Just to be safer when checking for title/body on the client side and maybe display a message: "Syncing..." or something. But I think if we just make the external ID look good standalone then we're fine.

  • Created by: mrnugget

    I was first confused by it being published before being synced, but I guess it makes sense, at least wording-wise. That sadly brings us to the point of introducing another boolean flag in the db, but I didn't find too much of a way around that, unless we attach the changeset spec (instead of just 0), detect it's importing and see that reconciler state is processing or queued.

    I'd love to get @LawnGnome and @chrispine's thought on that. I also couldn't see a way around it. And: I think there's another boolean coming, because we also need to close changesets in the background.

    I don't think the booleans are bad, though. Since our changesets basically act like "jobs" that can be executed multiple times, we need switches to say which job should run. And those switches are really hard to fit into a single field, especially since some booleans/state fields reflect code host state, others reflect sourcegraph state, others reflect desired sourcegraph state, etc.

  • Created by: chrispine

    I don't have enough background into what this code is doing to have an opinion, and I have a bunch of writing tasks to do today. I'd love to chat with someone about it tomorrow, though!

  • Created by: mrnugget

    I do worry a bit about the potential combinatorial explosion of the various state fields we have, but it's not like trying to stuff them into one field is going to make that magically better. Complex things are complex.

    Generally speaking, I think my reaction is pretty much the same as Erik's: this feels simpler, even with the extra field and boilerplate in some of the GraphQL resolver methods, so therefore it feels like the right path.

    You managed to perfectly articulate my thinking here.

    We're all now fully aware of what we're dealing with here and I think we should all keep an eye out to see if we can maybe find a better abstraction to manage these different state transitions of a changeset.

    Sometimes I do wonder whether it was a mistake to merge "Changesets, a thing that can exist on a code host" and "Changeset, a thing that we 'run' in the background" into the same table, but I can't think of another solution that wouldn't be more complex.

    What we should also be fully aware of is that we don't leak these abstractions through all the layers. One idea is that we push more logic from the resolver layer down into the "model" layer and I might give this a shot.


    @chrispine I sent you an invite to walk you through this PR (which includes the whole call stack from request to reconciler).


    I'm going to merge this PR now, because 24 hours later I'm still happy with it and none of you spotted obvious mistakes.

    As always: nothing's set in stone and we can come up with a better way to do this, we should do it.

Please register or sign in to reply
Loading