Gregory Szorc [:gps] Assignee
	Description • 8 years ago

If you query json-pushes?version=2&full=1 and the pushlog contains hidden changesets (changesets that are obsolete), you get an error:

 "filtered revision 'ea75f4c834c9c3aa24c5cc036297db796ea14d19' (not in 'served' subset)"

This error comes from Mercurial when you attempt to access a hidden changeset on a filtered repository.

I'm actually not sure what we want to do here. On one hand, we could expose the changeset info for the hidden changeset. On the other hand, exposing that info isn't very valuable because clients can't `hg pull` it or even get more metadata from e.g. json-rev because the changeset is hidden.

I'm leaning towards a compromise: we acknowledge the changeset exists but instead of emitting all the changeset metadata we say the changeset is obsolete and include the replacement changeset (if there is one).

	Gregory Szorc [:gps] Assignee
	Comment 1 • 8 years ago

Because it runs on 3.7 in production.

Review commit: https://reviewboard.mozilla.org/r/64344/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64344/

	Gregory Szorc [:gps] Assignee
	Comment 2 • 8 years ago

I'm pretty sure we needed the fallback when we were running an ancient
Python that didn't have sqlite3.

Review commit: https://reviewboard.mozilla.org/r/64346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64346/

	Gregory Szorc [:gps] Assignee
	Comment 3 • 8 years ago

This is safer than the alternative, which may enable if we were
disabled going in.

Review commit: https://reviewboard.mozilla.org/r/64348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64348/

	Gregory Szorc [:gps] Assignee
	Comment 4 • 8 years ago

Still not comforming to Mercurial standards. But close enough.

Review commit: https://reviewboard.mozilla.org/r/64350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64350/

	Gregory Szorc [:gps] Assignee
	Comment 5 • 8 years ago

Dropped some unused variables. Fixed some whitespace.

The entire file is now clean.

Review commit: https://reviewboard.mozilla.org/r/64352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64352/

	Gregory Szorc [:gps] Assignee
	Comment 6 • 8 years ago

It is the same as repo[x].

Review commit: https://reviewboard.mozilla.org/r/64354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64354/

	Gregory Szorc [:gps] Assignee
	Comment 7 • 8 years ago

The old behavior of relying on web being defined to populate full
metadata was a bit wonky. Change to use explicit "repo" and "full"
arguments.

Passing "repo" is also necessary so a future commit can ignore filtered
changesets.

Review commit: https://reviewboard.mozilla.org/r/64356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64356/

	Gregory Szorc [:gps] Assignee
	Comment 8 • 8 years ago

We're going to change behavior in an upcoming commit.

Review commit: https://reviewboard.mozilla.org/r/64358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64358/

	Gregory Szorc [:gps] Assignee
	Comment 9 • 8 years ago

Use sane indentation. Use single quotes for consistency.

Review commit: https://reviewboard.mozilla.org/r/64360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64360/

	Gregory Szorc [:gps] Assignee
	Comment 10 • 8 years ago

The pushlog doesn't currently behave well when encountering hidden
changesets. We prepare for improving matters by introducing tests
that perform all kinds of queries involving obsolete changesets.

Behavior will be fixed in subsequent patches.

Review commit: https://reviewboard.mozilla.org/r/64362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64362/

	Gregory Szorc [:gps] Assignee
	Comment 11 • 8 years ago

By querying against an unfiltered repo, we are able to find hidden
changesets referenced by the pushlog.

Test changes demonstrate that queries against hidden changesets now
yield values.

We still have failures for "full=1" queries because we get a
filtering error when attempting to retrieve the changectx for a
hidden changeset. And, hidden changesets are still being referenced in
JSON because we don't check for hidden state in the presentation layer.
This will be addressed in subsequent commits.

Review commit: https://reviewboard.mozilla.org/r/64364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64364/

	Gregory Szorc [:gps] Assignee
	Comment 12 • 8 years ago

An upcoming commit will change behavior to discard filtered nodes
but retain the push entry. To make the code simpler to read, create
the pushes entry first.

Review commit: https://reviewboard.mozilla.org/r/64366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64366/

	Gregory Szorc [:gps] Assignee
	Comment 13 • 8 years ago

If a changeset in the pushlog results is filtered, we detect this and
discard the changeset.

Future commits will continue to refine this behavior. For now, we gain
test coverage to make it easier to see how things change (or don't change)
going forward.

Review commit: https://reviewboard.mozilla.org/r/64368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64368/

	Gregory Szorc [:gps] Assignee
	Comment 14 • 8 years ago

The next step towards handling obsolescence data in pushlog is to expose
it to consumers so they can do intelligent things.

This commit changes behavior in a number of ways.

First, "successors" and "precursors" nodes lists are added to
changeset entries if the repository has obsolescence data.

Second, a "obsoletechangesets" key is now present on each push entry.
It operates like "changesets" except it only exposes changesets
that are obsolete.

I decided to create a new container for obsolete changesets because
adding obsolete changesets to the existing container is too risky:
existing consumers of the pushlog wouldn't know to e.g. look for
a key to identify a changeset as obsolete. By introducing a new
container, existing consumers have to explicitly opt in to looking
at obsolete changesets. And, since key additions are backwards
compatible, we don't need to introduce a v3 of the API to expose
this new data!

There should be enough metadata in the pushlog now for consumers to
determine whether a changeset was rewritten and what the
previous or new changeset is. We may also want to update Pulse
notifications to include similar metadata so Pulse consumers don't
need to consult the pushlog.

Review commit: https://reviewboard.mozilla.org/r/64370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64370/

	Gregory Szorc [:gps] Assignee
	Comment 15 • 8 years ago

The test demonstrates sub-optimal behavior when pulling normally. This is
because the remote server operates against the "visible" filter, which
doesn't expose obsoleted changesets.

We have a hack in the replication extension to allow replication access
to the unfiltered repository on the server, thus ensuring obsoleted
changesets are transferred to clients during regular `hg pull`
operations. Tests for this will be added in a different commit.

Review commit: https://reviewboard.mozilla.org/r/64466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64466/

	Gregory Szorc [:gps] Assignee
	Comment 16 • 8 years ago

This demonstrates that pushlog entries for obsolete changesets are
replicated in our fully configured server environment. This behavior
is different from what pushlog tests because of the special behavior
of the vcsreplicator extension which allows clients to `hg pull`
obsolete changesets.

Review commit: https://reviewboard.mozilla.org/r/64468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64468/

	Mike Hommey [:glandium]
	Comment 17 • 8 years ago

Comment on attachment 8771080 [details]
pushlog: mark as compatible with 3.7;

https://reviewboard.mozilla.org/r/64344/#review61484

	Mike Hommey [:glandium]
	Comment 18 • 8 years ago

Comment on attachment 8771081 [details]
pushlog: only import sqlite3;

https://reviewboard.mozilla.org/r/64346/#review61486

Considering that was added in 2008, I wouldn't be surprised if it was added to support python < 2.5. Ted might remember, but except for the archeology purpose, there's not much value in knowing for sure.

	Mike Hommey [:glandium]
	Comment 19 • 8 years ago

Comment on attachment 8771082 [details]
pushlog: use modern mechanism for disabling demand importing;

https://reviewboard.mozilla.org/r/64348/#review61490

It's worth mentioning in the commit message that the deactivated() contextmanager was added in mercurial 3.5.

	Mike Hommey [:glandium]
	Comment 20 • 8 years ago

Comment on attachment 8771083 [details]
pushlog: use more preferred import convention;

https://reviewboard.mozilla.org/r/64350/#review61492

	Mike Hommey [:glandium]
	Comment 21 • 8 years ago

Comment on attachment 8771084 [details]
pushlog: fix static analysis warnings;

https://reviewboard.mozilla.org/r/64352/#review61494

	Mike Hommey [:glandium]
	Comment 22 • 8 years ago

Comment on attachment 8771085 [details]
pushlog: stop using repo.changectx();

https://reviewboard.mozilla.org/r/64354/#review61496

	Mike Hommey [:glandium]
	Comment 23 • 8 years ago

Comment on attachment 8771086 [details]
pushlog: change arguments to pushes_worker (bug 1286426);

https://reviewboard.mozilla.org/r/64356/#review61498

	Mike Hommey [:glandium]
	Comment 24 • 8 years ago

Comment on attachment 8771087 [details]
pushlog: alias repo as local variable (bug 1286426);

https://reviewboard.mozilla.org/r/64358/#review61500

	Mike Hommey [:glandium]
	Comment 25 • 8 years ago

Comment on attachment 8771088 [details]
pushlog: fix formatting and getting hex value (bug 1286426);

https://reviewboard.mozilla.org/r/64360/#review61502

::: hgext/pushlog-legacy/pushlog-feed.py:475
(Diff revision 1)
>      for id, user, date, node in query.entries:
>          id = str(id)
>          if full:
>              ctx = repo[node]
> -            n = ctx.node()
> -            node = {"node": hex(n),
> +            node = {
> +                'node': ctx.hex(),

not that the change is wrong, but it's more than the advertised formatting changes.

	Mike Hommey [:glandium]
	Comment 26 • 8 years ago

Comment on attachment 8771089 [details]
pushlog: add tests for obsolete changesets (bug 1286426);

https://reviewboard.mozilla.org/r/64362/#review61508

::: hgext/pushlog-legacy/tests/test-obsolescence.t:110
(Diff revision 1)
> +
> +  $ httpjson "http://localhost:$HGPORT/json-pushes?version=1&full=1"
> +  404
> +  "hidden revision 'd313a202a85e114000f669c2fcb49ad42376ac04'"
> +
> +Hidden changesets should not be exposed to version 2

Maybe add a FIXME here and other places below, since AIUI, you're describing the desired behavior, but it's not happening yet.

	Mike Hommey [:glandium]
	Comment 27 • 8 years ago

Comment on attachment 8771090 [details]
pushlog: query against unfiltered repo (bug 1286426);

https://reviewboard.mozilla.org/r/64364/#review61510

::: hgext/pushlog-legacy/pushlog-feed.py:84
(Diff revision 1)
>          """Figure out what the query parameters are, and query the database
>          using those parameters."""
> -        repo = self.repo
> +        # Use an unfiltered repo because query parameters may reference hidden
> +        # changesets. Hidden changesets are still in the pushlog. We'll
> +        # treat them appropriately at the filter layer.
> +        repo = self.repo.unfiltered()

Considering it's a one-liner, I'd suggest folding with the de04b00bec23 if possible (picking that one based on title, I haven't actually read the patch yet). Anyways, this intermediate is uninteresting.

	Mike Hommey [:glandium]
	Comment 28 • 8 years ago

Comment on attachment 8771091 [details]
pushlog: create pushes entry before adding node (bug 1286426);

https://reviewboard.mozilla.org/r/64366/#review61512

	Mike Hommey [:glandium]
	Comment 29 • 8 years ago

Comment on attachment 8771092 [details]
pushlog: filter hidden changesets (bug 1286426);

https://reviewboard.mozilla.org/r/64368/#review61518

::: hgext/pushlog-legacy/tests/test-obsolescence.t:82
(Diff revision 1)
>            "date": \d+, (re)
>            "user": "user@example.com"

Do we actually want to keep that exposed?

::: hgext/pushlog-legacy/tests/test-obsolescence.t:348
(Diff revision 1)
> -   <link rel="self" href="http://*:$HGPORT/atom-log"/> (glob)
> -   <link rel="alternate" href="http://*:$HGPORT/"/> (glob)
> -   <title>Error</title>
> -   <updated>1970-01-01T00:00:00+00:00</updated>
> +   <link rel="alternate" href="http://*:$HGPORT/pushloghtml"/> (glob)
> +   <title>server Pushlog</title>
> +   <updated>*Z</updated> (glob)
> +   <entry>
> +    <title>Changeset d129109168f0ed985e51b0f86df256acdcfcfe45</title>
> +    <id>http://www.selenic.com/mercurial/#changeset-d129109168f0ed985e51b0f86df256acdcfcfe45</id>

www.selenic.com/mercurial/? really?

	Mike Hommey [:glandium]
	Comment 30 • 8 years ago

https://reviewboard.mozilla.org/r/64370/#review61520

::: docs/hgmo/pushlog.rst:319
(Diff revision 1)
> +
> +   The precursor changesets are hidden and not available to normal Mercurial
> +   operations. However, querying the pushlog for their info *may* return
> +   results.
> +
> +successors

I'm not all that convinced "successors" is interesting to add. I'd expect most consumers to get the json for recently pushed things, and getting empty successors for a given changeset because there wouldn't have been a changeset obsoleting them _yet_.

::: hgext/pushlog-legacy/pushlog-feed.py:498
(Diff revision 1)
>                  'user': user,
>                  'date': date,
>                  'changesets': [],
>              }
> +            if haveobs:
> +                pushes[id]['obsoletechangesets'] = []

contrary to changesets, I think it would be better if it wasn't always there.

::: hgext/pushlog-legacy/pushlog-feed.py:526
(Diff revision 1)
>                  'files': ctx.files()
>              }
>  
> +            # Only expose obsolescence metadata if the repo has some.
> +            if haveobs:
> +                precursors = list(obsolete.precursormarkers(ctx))

You don't need to assign a list here. Just assign the iterator.

::: hgext/pushlog-legacy/pushlog-feed.py:527
(Diff revision 1)
>              }
>  
> +            # Only expose obsolescence metadata if the repo has some.
> +            if haveobs:
> +                precursors = list(obsolete.precursormarkers(ctx))
> +                node['precursors'] = [hex(m.precnode()) for m in precursors]

I think it would be better if this key wasn't added if there aren't precursors (same for successors).

::: hgext/pushlog-legacy/pushlog-feed.py:529
(Diff revision 1)
> +            # Only expose obsolescence metadata if the repo has some.
> +            if haveobs:
> +                precursors = list(obsolete.precursormarkers(ctx))
> +                node['precursors'] = [hex(m.precnode()) for m in precursors]
> +
> +                successors = list(obsolete.allsuccessors(repo.obsstore, [ctx.node()]))

likewise

::: hgext/pushlog-legacy/pushlog-feed.py:530
(Diff revision 1)
> +            if haveobs:
> +                precursors = list(obsolete.precursormarkers(ctx))
> +                node['precursors'] = [hex(m.precnode()) for m in precursors]
> +
> +                successors = list(obsolete.allsuccessors(repo.obsstore, [ctx.node()]))
> +                successors = [n for n in successors if n != ctx.node()]

likewise (use parentheses)

	Mike Hommey [:glandium]
	Comment 31 • 8 years ago

Comment on attachment 8771093 [details]
pushlog: expose obsolescence metadata (bug 1286426);

https://reviewboard.mozilla.org/r/64370/#review61524

	Mike Hommey [:glandium]
	Comment 32 • 8 years ago

Comment on attachment 8771199 [details]
pushlog: add tests for pulling entries with obsolete changesets (bug 1286426);

https://reviewboard.mozilla.org/r/64466/#review61526

Why would you want `hg pull` to always pull obsolete changesets?

	Mike Hommey [:glandium]
	Comment 33 • 8 years ago

https://reviewboard.mozilla.org/r/64466/#review61528

Ah,  I get it.

::: hgext/pushlog/tests/test-pull-obsolete.t:83
(Diff revision 1)
> +  received pushlog entry for unknown changeset; ignoring
> +  added 2 pushes
> +
> +  $ dumppushlog clone-obsolete1
> +  ID: 1; user: hguser; Date: \d+; Rev: 0; Node: 96ee1d7354c4ad7372047672c36a1f561e3a6a4c (re)
> +  ID: 2; user: hguser; Date: \d+; Rev: 1; Node: ae13d9da6966307c98b60987fb4fedc2e2f29736 (re)

This would probably be clearer if you added some hg glog output like mercurial tests do sometimes. But it seems to me this is wrong. If I'm following it right you rebased 80c2c663cb8364f6898662a8379cb25df3ebe719 on top of ae13d9da6966307c98b60987fb4fedc2e2f29736, creating a129f82339bb933c4d72353c44bb29eb685f3d1e. But you don't have the latter in your dump.

	Mike Hommey [:glandium]
	Comment 34 • 8 years ago

Comment on attachment 8771200 [details]
hgserver: add tests for pushlog replication with obsolescence (bug 1286426);

https://reviewboard.mozilla.org/r/64468/#review61530

	Gregory Szorc [:gps] Assignee
	Comment 35 • 8 years ago

https://reviewboard.mozilla.org/r/64364/#review61510

> Considering it's a one-liner, I'd suggest folding with the de04b00bec23 if possible (picking that one based on title, I haven't actually read the patch yet). Anyways, this intermediate is uninteresting.

I think it is interesting because it makes the test diffs slightly easier to read. Since it is already reviewed, I'll keep it as is.

	Gregory Szorc [:gps] Assignee
	Comment 36 • 8 years ago

https://reviewboard.mozilla.org/r/64368/#review61518

> Do we actually want to keep that exposed?

I'm... not sure.

My thought process here is existing consumers may not like pushes disappearing on them. I should definitely run this series by the TaskCluster and Treeherder teams for their opinions.

> www.selenic.com/mercurial/? really?

That's in the template at hgtemplates/atom/pushlogentry.tmpl.

My recollection of ATOM is <id> is effectively a GUID. But, yeah, it is a bad value. I'll file a follow-up bug.

	Gregory Szorc [:gps] Assignee
	Comment 37 • 8 years ago

https://reviewboard.mozilla.org/r/64370/#review61520

> I'm not all that convinced "successors" is interesting to add. I'd expect most consumers to get the json for recently pushed things, and getting empty successors for a given changeset because there wouldn't have been a changeset obsoleting them _yet_.

I can't argue too strongly with this reasoning.

We can always add the metadata later if someone wants it.

	Gregory Szorc [:gps] Assignee
	Comment 38 • 8 years ago

Comment on attachment 8771080 [details]
pushlog: mark as compatible with 3.7;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64344/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 39 • 8 years ago

Comment on attachment 8771081 [details]
pushlog: only import sqlite3;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64346/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 40 • 8 years ago

Comment on attachment 8771082 [details]
pushlog: use modern mechanism for disabling demand importing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64348/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 41 • 8 years ago

Comment on attachment 8771083 [details]
pushlog: use more preferred import convention;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64350/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 42 • 8 years ago

Comment on attachment 8771084 [details]
pushlog: fix static analysis warnings;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64352/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 43 • 8 years ago

Comment on attachment 8771085 [details]
pushlog: stop using repo.changectx();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64354/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 44 • 8 years ago

Comment on attachment 8771086 [details]
pushlog: change arguments to pushes_worker (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64356/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 45 • 8 years ago

Comment on attachment 8771087 [details]
pushlog: alias repo as local variable (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64358/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 46 • 8 years ago

Comment on attachment 8771088 [details]
pushlog: fix formatting and getting hex value (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64360/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 47 • 8 years ago

Comment on attachment 8771089 [details]
pushlog: add tests for obsolete changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64362/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 48 • 8 years ago

Comment on attachment 8771090 [details]
pushlog: query against unfiltered repo (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64364/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 49 • 8 years ago

Comment on attachment 8771091 [details]
pushlog: create pushes entry before adding node (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64366/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 50 • 8 years ago

Comment on attachment 8771092 [details]
pushlog: filter hidden changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64368/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 51 • 8 years ago

Comment on attachment 8771093 [details]
pushlog: expose obsolescence metadata (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64370/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 52 • 8 years ago

Comment on attachment 8771199 [details]
pushlog: add tests for pulling entries with obsolete changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64466/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 53 • 8 years ago

Comment on attachment 8771200 [details]
hgserver: add tests for pushlog replication with obsolescence (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64468/diff/1-2/

	Gregory Szorc [:gps] Assignee
	Comment 54 • 8 years ago

Comment on attachment 8771092 [details]
pushlog: filter hidden changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64368/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 55 • 8 years ago

Comment on attachment 8771093 [details]
pushlog: expose obsolescence metadata (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64370/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 56 • 8 years ago

Comment on attachment 8771199 [details]
pushlog: add tests for pulling entries with obsolete changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64466/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 57 • 8 years ago

Comment on attachment 8771200 [details]
hgserver: add tests for pushlog replication with obsolescence (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64468/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 58 • 8 years ago

pushlog consumers:

This commit series changes pushlog behavior.

The JSON "schema" is backwards compatible (we only introduce new keys in a few places). However, some behavior may change. Notably, the "changesets" array may be empty.

https://reviewboard.mozilla.org/r/64342/diff/4#0 documents the changes.

I'd appreciate if you could look things over and audit your pushlog consumers to see if they'll behave well.

Scenarios that may occur when this is deployed:

* "changesets" pushlog array may be empty
* The tip-most pushlog entry may be empty
* Changesets will "disappear" from repos (you won't be able to perform json-rev queries or pull them explicitly)

Today, obsolescence can only be enabled on user repos (which likely don't have pushlog automation hooked up to them). In a few days, I'd like to enable obsolescence on the autoland repo. However, only sheriffs will have the ability to make things obsolete and we'll be able to disable this feature if it causes issues with automation.

If you don't want to audit things, the worst thing that happens is we cause a fire drill the first time we use this feature on the autoland repo. We clean things up from {buildbot, taskcluster, treeherder} and disable the feature on the autoland repo until we fix {buildbot, taskcluster, treeherder} to cope with things.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 59 • 8 years ago

Under what circumstances would a recent push have an empty changesets?

mozilla-taskcluster only ever looks at the most recent changeset in a push.  The taskgraph automation is similar, but uses json-automationrelevant to get data on all of the changesets in the push.  What those tools should do in these scenarios depends on what might cause those scenarios.  Probably "bomb out with an error" is appropriate?

	Jordan Lund (:jlund)
	Comment 60 • 8 years ago

disclaimer: first time looking at bbot pushlog consumer.

I suspect bbot will fall over if we have a recent push and the changesets key is empty.

this is based on the fact that the following code suggests we assume there to be at least one changeset when processing most recent push: https://dxr.mozilla.org/build-central/source/buildbotcustom/changes/hgpoller.py#394

maybe what we need is to tell bbot to ignore (but warn) recent pushes that have empty csets. maybe something like:

`if len(pushes) == 0 or not pushes[-1].get("changesets"):`

here:
https://dxr.mozilla.org/build-central/source/buildbotcustom/changes/hgpoller.py#259

catlee: what do you think?

	Gregory Szorc [:gps] Assignee
	Comment 61 • 8 years ago

(In reply to Dustin J. Mitchell [:dustin] from comment #59)
> Under what circumstances would a recent push have an empty changesets?

In the near future, this can happen on the autoland repo when a backout is performed. The tip-most push may even have no listed changesets!

> mozilla-taskcluster only ever looks at the most recent changeset in a push. 
> The taskgraph automation is similar, but uses json-automationrelevant to get
> data on all of the changesets in the push.  What those tools should do in
> these scenarios depends on what might cause those scenarios.  Probably "bomb
> out with an error" is appropriate?

json-automationrelevance is an interesting use case. Looking at its code, it appears it will currently 500 (or possibly 404) if an obsolete changeset is encountered. I'll need to fix that. I say it is interesting because when automation queries that endpoint, it is effectively saying "I'm building this - tell me about it." So, you can make the argument the endpoint should emit info about hidden changesets, just like what we're doing with the pushlog API.

I may have to look at the code for mozilla-taskcluster to see how it handles the empty push case. Can you please point me at it?

	Gregory Szorc [:gps] Assignee
	Comment 62 • 8 years ago

It looks like hgpoller.py will need changed to handle empty pushes better.

IIRC it isn't following the best practices detailed at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmo/pushlog.html#writing-agents-that-consume-pushlog-data (it is querying by last pushed changeset). In fear of scope bloating, now might be a good time to change that.

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 63 • 8 years ago

https://github.com/taskcluster/mozilla-taskcluster/blob/master/src/jobs/taskcluster_graph.js#L83
https://github.com/taskcluster/mozilla-taskcluster/blob/master/src/pushlog/client.js#L126

I agree regarding json-automationrelevance.  On the other hand, maybe that's a good time to say "this build is no longer worth doing"?  Will the subsequent builds be able to pull the obsolete revisions?  I feel like the ideal here would be that any taskcluster operations that touch obsolete commits would "melt away" in a relatively smooth fashion.

	Gregory Szorc [:gps] Assignee
	Comment 64 • 8 years ago

Somewhere under the bug 1286022 umbrella we talked about what to do about jobs impacted by backouts. There was a suggestion we have some process somewhere listening for "impacted by backout" pulse messages that would cancel jobs via buildbot and tc apis.

Having the jobs themselves fail if they encounter obsolete revisions is an interesting idea. I'm not sure the sheriffs would approve having to star those jobs. Perhaps we could make jobs self-abort gracefully (or in a way where auto-starring works)?

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 65 • 8 years ago

Since you're doing parallel work on replacing tc-vcs, perhaps that could produce a unique log message that would make starring easier (or automatable?).

	Chris AtLee [:catlee]
	Comment 66 • 8 years ago

(In reply to Jordan Lund (:jlund) from comment #60)
> disclaimer: first time looking at bbot pushlog consumer.
> 
> I suspect bbot will fall over if we have a recent push and the changesets
> key is empty.
> 
> this is based on the fact that the following code suggests we assume there
> to be at least one changeset when processing most recent push:
> https://dxr.mozilla.org/build-central/source/buildbotcustom/changes/hgpoller.
> py#394
> 
> maybe what we need is to tell bbot to ignore (but warn) recent pushes that
> have empty csets. maybe something like:
> 
> `if len(pushes) == 0 or not pushes[-1].get("changesets"):`
> 
> here:
> https://dxr.mozilla.org/build-central/source/buildbotcustom/changes/hgpoller.
> py#259
> 
> catlee: what do you think?

A simple guard around line 394 could protect us. We do have some tests for this code [1], so it would be relatively easy to add a test to see how the code behaves in the case of a push without changes.

If we're open to scope creep, then ripping the poller out completely and using a push-based model would be my preference. That would solve this issue, as well as other (e.g. bugs 1086961, and having to reconfig have a repo has been reset).

[1] https://dxr.mozilla.org/build-central/source/buildbotcustom/test/test_hgpoller.py

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 67 • 8 years ago

AIUI, the end for Buildbot *schedulers* (and thus changesources, of which hgpoller is one) is nearer than that for buildbot builders.  Which is to say, once we are doing in-tree scheduling with non-TC jobs run via BBB, we won't be running hgpoller.py anymore.  So maybe this isn't the best scope to creep :)

	Gregory Szorc [:gps] Assignee
	Comment 68 • 8 years ago

Looks like TC will need to be taught about empty pushes too:

let lastChangeset = push.changesets[push.changesets.length - 1];

That returns undefined in JS. Unfortunately, we try to access a property of lastChangeset a few lines down, which will throw.

	Gregory Szorc [:gps] Assignee
	Comment 69 • 8 years ago

Come to think about it, if automation is told to check out a hidden changeset, unless it has that changeset cached locally, it will fail to `hg pull -r <rev>` that changeset because the server won't expose hidden changesets. That's how hidden changesets work.

Since automation may not start immediately after a push is performed, there are all kinds of race conditions here. A changeset could become obsolete at any time, including:

1) Before the scheduler has queried the pushlog
2) After the scheduler has queried the pushlog but before a source checkout runs in the job
3) After the source checkout runs before the job accesses hg.mozilla.org (in the case of Gecko decision task)

So, scheduling, source checkout, and anything accessing hg.mozilla.org in jobs (probably just Gecko decision task) need to deal with hidden changesets and react accordingly. And I think "accordingly" has to mean "abort" because continuing after you know a changeset has disappeared is just rolling dice. For example, in bug 1286900 we're going to experiment with running WPT tests from a source checkout. What's the use in continuing a build job if WPT is going to fail the VCS interaction? Fail fast.

This also touches on an unintended side-effect of obsoleting changesets: changeset availability is no longer guaranteed. Today, if you can access a changeset on hg.mozilla.org, you can access it forever. Once we start obsoleting things, the guarantee is only that *public* changesets can be accessed forever. That's a big change. (And one of the reasons we're not going to enable users to push obsolescence markers to repos.)

	Gregory Szorc [:gps] Assignee
	Comment 70 • 8 years ago

KWierso: We want to abort jobs if we know they'll never succeed. Is there a way to do this such that Sheriffs aren't inundated with the need to star jobs? Could we potentially change auto-starring to recognize a new abort phrase?

	Ed Morley [:emorley]
	Comment 71 • 8 years ago

The jobs should be cancelled. Cancelled jobs have their own result state, which are already special-cased in the UI (to a certain extent, more can be done as needed).

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 72 • 8 years ago

(In reply to Gregory Szorc [:gps] from comment #70)
> KWierso: We want to abort jobs if we know they'll never succeed. Is there a
> way to do this such that Sheriffs aren't inundated with the need to star
> jobs? Could we potentially change auto-starring to recognize a new abort
> phrase?

This would be more of a question for the Treeherder team.

At the moment, if a job returns with a result type "retry" (result code '5', I believe), the failure does not show up in only-unstarred mode, and the build system (excluding the issues in bug 1280570) gets automatically retried. 

I imagine we could add a new result type "aborted", which could be treated like "retry" in Treeherder, just not automatically retrying after the job finishes.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 73 • 8 years ago

Or just re-use "canceled", yeah.

	Gregory Szorc [:gps] Assignee
	Comment 74 • 8 years ago

You're going to have to spoon feed this to me: what's the magic incantation of bytes we need to output to mark a job as cancelled from within the job? (I don't see the string "cancel" in testing/mozharness, which seems to indicate no existing in-tree jobs self-cancel and this has me worried...)

	Mike Hommey [:glandium]
	Comment 75 • 8 years ago

Comment on attachment 8771093 [details]
pushlog: expose obsolescence metadata (bug 1286426);

https://reviewboard.mozilla.org/r/64370/#review62114

	Mike Hommey [:glandium]
	Comment 76 • 8 years ago

Comment on attachment 8771199 [details]
pushlog: add tests for pulling entries with obsolete changesets (bug 1286426);

https://reviewboard.mozilla.org/r/64466/#review62164

::: hgext/pushlog/tests/test-pull-obsolete.t:109
(Diff revision 3)
> +  searching for changes
> +  no changes found
> +  received pushlog entry for unknown changeset; ignoring
> +  added 2 pushes
> +
> +Pushlog stops at 80c2c663cb83 because it isn't in local repo

Can you explain why this is the desired behavior?

	Dustin J. Mitchell [:dustin] (he/him)
	Comment 77 • 8 years ago

Finding and tracking every access to hg will, I suspect, be tricky and result in a lot of duplicated code:
 * two or three bits of special handling in taskgraph/**.py,
 * a few in taskcluster scripts like build-linux.sh,
 * a few in mozharness -- all unique of course,
 * a few in mozilla-taskcluster or its successor,
 * docker-image build tasks (which will eventually use subdir checkouts)
 * dozens of places where some script curls /raw-file/<rev>, not least of which the build.sh scripts in the docker images (to fetch build-linux.sh).

And these are all race conditions, so it will be difficult to test and ensure we've gotten everything.  It feels like all of those are the wrong place to implement cancel-on-obsolete.

Is there an alternative?  Perhaps when marking commits as obsolete, autoland can perform an explicit cancel operation via treeherder?  Even in that case, I suspect there's still a race, but perhaps treeherder will cover that up for us by marking cancelled jobs that actually failed as cancelled?

	Gregory Szorc [:gps] Assignee
	Comment 78 • 8 years ago

https://reviewboard.mozilla.org/r/64368/#review61518

> I'm... not sure.
> 
> My thought process here is existing consumers may not like pushes disappearing on them. I should definitely run this series by the TaskCluster and Treeherder teams for their opinions.

I think this data might be useful. If nothing else, it helps establish the forensics trail.

	Gregory Szorc [:gps] Assignee
	Comment 79 • 8 years ago

Comment on attachment 8771080 [details]
pushlog: mark as compatible with 3.7;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64344/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 80 • 8 years ago

Comment on attachment 8771081 [details]
pushlog: only import sqlite3;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64346/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 81 • 8 years ago

Comment on attachment 8771082 [details]
pushlog: use modern mechanism for disabling demand importing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64348/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 82 • 8 years ago

Comment on attachment 8771083 [details]
pushlog: use more preferred import convention;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64350/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 83 • 8 years ago

Comment on attachment 8771084 [details]
pushlog: fix static analysis warnings;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64352/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 84 • 8 years ago

Comment on attachment 8771085 [details]
pushlog: stop using repo.changectx();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64354/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 85 • 8 years ago

Comment on attachment 8771086 [details]
pushlog: change arguments to pushes_worker (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64356/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 86 • 8 years ago

Comment on attachment 8771087 [details]
pushlog: alias repo as local variable (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64358/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 87 • 8 years ago

Comment on attachment 8771088 [details]
pushlog: fix formatting and getting hex value (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64360/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 88 • 8 years ago

Comment on attachment 8771089 [details]
pushlog: add tests for obsolete changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64362/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 89 • 8 years ago

Comment on attachment 8771090 [details]
pushlog: query against unfiltered repo (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64364/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 90 • 8 years ago

Comment on attachment 8771091 [details]
pushlog: create pushes entry before adding node (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64366/diff/2-3/

	Gregory Szorc [:gps] Assignee
	Comment 91 • 8 years ago

Comment on attachment 8771092 [details]
pushlog: filter hidden changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64368/diff/3-4/

	Gregory Szorc [:gps] Assignee
	Comment 92 • 8 years ago

Comment on attachment 8771093 [details]
pushlog: expose obsolescence metadata (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64370/diff/3-4/

	Gregory Szorc [:gps] Assignee
	Comment 93 • 8 years ago

Comment on attachment 8771199 [details]
pushlog: add tests for pulling entries with obsolete changesets (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64466/diff/3-4/

	Gregory Szorc [:gps] Assignee
	Comment 94 • 8 years ago

Comment on attachment 8771200 [details]
hgserver: add tests for pushlog replication with obsolescence (bug 1286426);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64468/diff/3-4/

	Mike Hommey [:glandium]
	Comment 95 • 8 years ago

Comment on attachment 8771199 [details]
pushlog: add tests for pulling entries with obsolete changesets (bug 1286426);

https://reviewboard.mozilla.org/r/64466/#review62372

	Gregory Szorc [:gps] Assignee
	Comment 96 • 8 years ago

I'm going to land and push this series to prod. Since we don't have obsolescence marker creating enabled on any "tier 1" repos, the worst it can do is bust pushlog on user repos. And - surprise - pushlog is already busted on user repos using obsolescence. Nobody knew :)

	Pulsebot
	Comment 97 • 8 years ago

Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6b1312dc6c67
pushlog: change arguments to pushes_worker ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/8db703bd97e8
pushlog: alias repo as local variable ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/86f3b4051c18
pushlog: fix formatting and getting hex value ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4bbdf0516df1
pushlog: add tests for obsolete changesets ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b03458db3e8a
pushlog: query against unfiltered repo ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/0bc74c039485
pushlog: create pushes entry before adding node ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/cc3360b720c6
pushlog: filter hidden changesets ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6900f22f81a2
pushlog: expose obsolescence metadata ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3367357851c9
pushlog: add tests for pulling entries with obsolete changesets ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/9bf139a2cdbe
hgserver: add tests for pushlog replication with obsolescence ; r=glandium

	Cameron Dawson [:camd]
	Comment 98 • 8 years ago

Well, I'm late on chiming in here, but "usercancel" is the right incantation for a job to show as canceled by Treeherder.

Also, if a job has no changesets, we will fail to ingest that set of pushes.  I have created bug 1288181 to modify that so that we just ingest the push with no changesets.  However, Treeherder will not display a resultset/push that has no changesets.  So it'll just not appear.