Johan Corveleyn wrote on Thu, Oct 13, 2016 at 13:14:30 +0200: > On Thu, Oct 13, 2016 at 1:05 PM, Branko Čibej <br...@apache.org> wrote: > > On 13.10.2016 13:01, Branko Čibej wrote: > >> On 13.10.2016 11:39, Ivan Zhakov wrote: > >>> On 13 October 2016 at 10:59, <jcor...@apache.org> wrote: > >>>> Author: jcorvel > >>>> Date: Thu Oct 13 08:59:07 2016 > >>>> New Revision: 1764631 > >>>> > >>>> URL: http://svn.apache.org/viewvc?rev=1764631&view=rev > >>>> Log: > >>>> Resurrect the '1.9.x-r1721488' branch, to give backport.pl another > >>>> chance to execute the correct backport commands, after backport mess. > >>>> > >>> I'm wondering if we really need backport.pl running as cronjob to > >>> merge backports automatically to the stable branches? It's not a first > >>> time when this automatic job performs invalid merges. And as far I > >>> understand we still spend some time to babysit this tool, fix bugs > >>> etc. What is wrong with old proven process by merging revisions > >>> manually? > >> It doesn't happen all that often that backport.pl makes a mistake. I bet > >> manual merging would be just as error-prone. > >> > >> Backporting is a well-defined process. The best possible way to document > >> a process is to automate it. Errors will happen but that's no reason to > >> revert to PEBKAC; bugs can be fixed. > > > > > > In fact, now that I've read Johan's mail ... it /was/ a PEBKAC and > > nothing was wrong with backport.pl. > > Indeed, what we experienced last night was not a script bug (unless if > you state that it should have protected against that race condition > :-)).
I think the issue is different. backport.pl was never expected to run concurrently with itself. However, tonight's bug would also have occurred if a human made a commit that met all the following conditions: - Committed between 04:00:02 and (roughly) 04:00:12 UTC, - on a night when there are 2+ approved groups, - removes a group (other than the last) from the "Approved changes" section. That's a pretty rare combination: we have few commits at around 4am UTC, and we have few cases of vetoing an approved group. That's why this bug has never been encountered in 5+ years of using the cron job (and even tonight, it was encountered due to the migration, not due to a nocturnal committer). Patch attached. Cheers, Daniel [[[ backport.py: Fix a race condition involving concurrent commits to STATUS (see r1764633). The fix is to run 'update' at the top of the outermost loop, rather than immediately before calling 'svn merge'. * tools/dist/backport/merger.py (merge): Don't run revert+update; instead, expect the caller to have done so. * tools/dist/merge-approved-backports.py: Run 'update' and re-parse STATUS before each merge. * tools/dist/detect-conflicting-backports.py: Track API change of merge(). ]]] [[[ Index: backport/merger.py =================================================================== --- backport/merger.py (revision 1762016) +++ backport/merger.py (working copy) @@ -195,11 +195,8 @@ def merge(entry, expected_stderr=None, *, commit=F mergeargs.extend(['--', sf.trunk_url()]) logmsg += entry.raw - # TODO(interactive mode): exclude STATUS from reverts - # TODO(interactive mode): save local mods to disk, as backport.pl does - run_revert() + no_local_mods('.') - run_svn_quiet(['update']) # TODO: use select() to restore interweaving of stdout/stderr _, stdout, stderr = run_svn_quiet(['merge'] + mergeargs, expected_stderr) sys.stdout.write(stdout) Index: detect-conflicting-backports.py =================================================================== --- detect-conflicting-backports.py (revision 1762016) +++ detect-conflicting-backports.py (working copy) @@ -77,6 +77,7 @@ ERRORS = collections.defaultdict(list) for entry_para in sf.entries_paras(): entry = entry_para.entry() # SVN_ERR_WC_FOUND_CONFLICT = 155015 + backport.merger.run_svn_quiet(['update']) # TODO: what to do if this pulls in a STATUS mod? backport.merger.merge(entry, 'svn: E155015' if entry.depends else None) _, output, _ = backport.merger.run_svn(['status']) Index: merge-approved-backports.py =================================================================== --- merge-approved-backports.py (revision 1762016) +++ merge-approved-backports.py (working copy) @@ -40,11 +40,14 @@ if sys.argv[1:]: sys.exit(0) backport.merger.no_local_mods('./STATUS') -sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8")) -# Duplicate sf.paragraphs, since merge() will be removing elements from it. -entries_paras = list(sf.entries_paras()) -for entry_para in entries_paras: - if entry_para.approved(): - entry = entry_para.entry() - backport.merger.merge(entry, commit=True) +while True: + backport.merger.run_svn_quiet(['update']) + sf = backport.status.StatusFile(open('./STATUS', encoding="UTF-8")) + for entry_para in sf.entries_paras(): + if entry_para.approved(): + entry = entry_para.entry() + backport.merger.merge(entry, commit=True) + break # 'continue' the outer loop + else: + break ]]]