b8d17d29c4215bc9e7776173cbaa75d88f3ee9bd
hiram
  Wed Aug 12 15:34:01 2026 -0700
fixups in response to code review refs #38001

diff --git src/hg/utils/automation/doNcbiGene.pl src/hg/utils/automation/doNcbiGene.pl
index cbeda8e064e..9a527a04d04 100755
--- src/hg/utils/automation/doNcbiGene.pl
+++ src/hg/utils/automation/doNcbiGene.pl
@@ -67,34 +67,37 @@
                           gatewayPage step), e.g.
                           \$buildDir/../../html/\$db.names.tab -- used
                           for the archived-version description page.
     -liftFile path        Optional lift file translating NCBI names to
                           UCSC names, e.g.
                           \$buildDir/../../sequence/\$asmId.ncbiToUcsc.lift
 _EOF_
   ;
   print STDERR &HgAutomate::getCommonOptionHelp('dbHost' => $dbHost,
 					'workhorse' => $defaultWorkhorse,
 					'fileServer' => $defaultFileServer);
   print STDERR "
 Automates construction of the 'ncbiGene' track from an assembly's own
 NCBI GFF3 gene predictions, for assembly hub (GenArk) builds. Steps:
     ncbiGene: if a previous \$db.ncbiGene.bb exists and the source gff
-              is newer, archive the previous version under archive/<date>/
-              (keyed by the previous build's own gff-derived mtime), then
-              translate the current gff3 into a bigGenePred track and swap
-              it into place atomically.
+              is newer, translate the current gff3 into a bigGenePred track,
+              building it entirely under *.new file names so the existing
+              live track is never touched while the build might still fail.
+              Only once that build has fully succeeded is the previous
+              version archived under archive/<date>/ (keyed by the previous
+              build's own gff-derived mtime) and the new *.new files
+              promoted into place.
     cleanup:  compress intermediate files
 ";
   print "\n";
   exit $status;
 }
 
 # Globals:
 my ($asmId, $db);
 my ($buildDir, $assemblySource, $chromSizes, $namesFile, $liftFile);
 my ($secondsStart, $secondsEnd);
 
 sub checkOptions {
   my $ok = GetOptions(@HgStepManager::optionSpec,
 		      'buildDir=s',
 		      'assemblySource=s',
@@ -146,30 +149,48 @@
   foreach my $ext (qw( bb gtf.gz ix ixx stats.txt )) {
     my $f = "$buildDir/$db.ncbiGene.$ext";
     rename($f, "$archiveDir/$db.ncbiGene.$ext") if (-e "$f");
   }
   my $geneAttrs = "$buildDir/$db.geneAttrs.ncbi.txt.gz";
   rename($geneAttrs, "$archiveDir/$db.geneAttrs.ncbi.txt.gz") if (-e "$geneAttrs");
   my $fb = "$buildDir/fb.$db.ncbiGene.txt";
   rename($fb, "$archiveDir/fb.$db.ncbiGene.txt") if (-e "$fb");
 
   writeArchiveHub($archiveDir, $priorVersion);
 
   &HgAutomate::verbose(1,
     "# ncbiGene: archived previous version to archive/$priorVersion/\n");
 } # archivePriorVersion
 
+#########################################################################
+# promote a successful *.new build into the live $db.ncbiGene.* names.
+# Only ever called after archivePriorVersion() and only after the boss
+# script below has exited 0 -- i.e. after the new track has already been
+# fully built and validated under *.new names, so every rename() here is
+# just swapping a already-good file into place, never risking the live
+# track on a build that might still fail.
+sub promoteNewBuild {
+  foreach my $ext (qw( bb gtf.gz ix ixx stats.txt )) {
+    my $new = "$buildDir/$db.ncbiGene.$ext.new";
+    rename($new, "$buildDir/$db.ncbiGene.$ext") if (-e "$new");
+  }
+  my $geneAttrsNew = "$buildDir/$db.geneAttrs.ncbi.txt.new";
+  rename($geneAttrsNew, "$buildDir/$db.geneAttrs.ncbi.txt") if (-e "$geneAttrsNew");
+  my $fbNew = "$buildDir/fb.$db.ncbiGene.txt.new";
+  rename($fbNew, "$buildDir/fb.$db.ncbiGene.txt") if (-e "$fbNew");
+} # promoteNewBuild
+
 #########################################################################
 # a single hub.txt (useOneFile on) that puts just the archived ncbiGene
 # track up for viewing.  No twoBitPath/organism/defaultPos are needed in
 # the genome stanza: the true NCBI accession is already a published
 # GenArk genome, so 'genome $accession' with no twoBitPath just attaches
 # this one extra track to that already-known assembly (see
 # trackHubGenomeReadRa() in hg/lib/trackHub.c -- twoBitPath is only
 # required when *introducing* a new genome).
 sub writeArchiveHub {
   my ($archiveDir, $priorVersion) = @_;
   my $haveIx = ( -s "$archiveDir/$db.ncbiGene.ix" );
 
   writeArchiveHtml($archiveDir, $priorVersion);
 
   # 'genome' wants the bare accession (GCF_937001465.1), not the full
@@ -231,144 +252,156 @@
 
 #########################################################################
 # * step: ncbiGene [workhorse]
 sub doNcbiGene {
   my $gffFile = "$assemblySource/${asmId}_genomic.gff.gz";
   if ( ! -s "${gffFile}" ) {
     &HgAutomate::verbose(1, "# step ncbiGene: no gff file found at:\n#  $gffFile\n");
     return;
   }
   if ( ! needsUpdate($gffFile, "$buildDir/$db.ncbiGene.bb") ) {
     &HgAutomate::verbose(1, "# ncbiGene step previously completed\n");
     return;
   }
   &HgAutomate::mustMkdir($buildDir);
 
-  archivePriorVersion();
+  # NOTE: the previous live version is deliberately *not* archived here.
+  # Everything below builds under *.new file names, leaving the existing
+  # live track completely untouched; archivePriorVersion() only runs
+  # after $bossScript->execute() returns below, i.e. only once the new
+  # build has actually succeeded (execute() dies on any remote failure,
+  # via 'set -e' in the boss script, so a failed build never reaches
+  # that line and the live track is left exactly as it was).
 
   my $whatItDoes = "translate NCBI GFF3 gene definitions into a track";
   # NOTE: must not be named doNcbiGene.bash -- doAssemblyHub.pl's own
   # wrapper script that invokes this program is *already* named
   # $buildDir/doNcbiGene.bash and is still running (mid-ssh-exec) at the
   # moment this line runs.  Reusing that filename here truncates and
   # rewrites the file the outer shell is still reading, corrupting its
   # read position and producing a bogus "syntax error" once the outer
   # shell resumes -- after the real work below has already succeeded.
   my $bossScript = newBash HgRemoteScript("$buildDir/buildNcbiGene.bash",
                     $workhorse, $buildDir, $whatItDoes);
 
   my $dupList = "";
   # this list is curated by hand into the build's own download/ area,
   # not something NCBI supplies in $assemblySource
   if ( -s "${buildDir}/../../download/${asmId}.remove.dups.list" ) {
     $dupList = " | (grep -v -f \"${buildDir}/../../download/${asmId}.remove.dups.list\"  || true)";
   }
 
   $bossScript->add(<<_EOF_
 export asmId=$db
 export gffFile=$gffFile
 export chromSizes=$chromSizes
 
 function cleanUp() {
   rm -f \$asmId.ncbiGene.genePred.gz \$asmId.ncbiGene.genePred
-  rm -f \$asmId.geneAttrs.ncbi.txt
+  rm -f \$asmId.geneAttrs.ncbi.txt.new
 }
 
 if [ \$gffFile -nt \$asmId.ncbiGene.bb ]; then
   ln -sf \$gffFile ./
   (gff3ToGenePred -warnAndContinue -useName \\
-    -attrsOut=\$asmId.geneAttrs.ncbi.txt \$gffFile stdout \\
+    -attrsOut=\$asmId.geneAttrs.ncbi.txt.new \$gffFile stdout \\
       2>> \$asmId.ncbiGene.log.txt || true) | genePredFilter \\
          -chromSizes=\$chromSizes stdin stdout \\
         $dupList | gzip -c > \$asmId.ncbiGene.genePred.gz
   genePredCheck \$asmId.ncbiGene.genePred.gz
   zcat \$asmId.ncbiGene.genePred.gz > ncbiGene.\$asmId.gp
-  genePredToGtf -utr file ncbiGene.\$asmId.gp stdout | gzip -c > \$asmId.ncbiGene.gtf.gz
+  genePredToGtf -utr file ncbiGene.\$asmId.gp stdout | gzip -c > \$asmId.ncbiGene.gtf.gz.new
   rm -f ncbiGene.\$asmId.gp
   export howMany=`genePredCheck \$asmId.ncbiGene.genePred.gz 2>&1 | grep "^checked" | awk '{print \$2}'`
   if [ "\${howMany}" -eq 0 ]; then
      printf "# ncbiGene: no gene definitions found in \$gffFile\\n"
      cleanUp
      exit 0
   fi
   export ncbiGenePred="\$asmId.ncbiGene.genePred.gz"
 _EOF_
   );
   if ( -s "$liftFile" ) {
     $bossScript->add(<<_EOF_
   liftUp -extGenePred -type=.gp stdout \\
     $liftFile warn \\
       \$asmId.ncbiGene.genePred.gz | gzip -c \\
         > \$asmId.ncbiGene.ucsc.genePred.gz
   ncbiGenePred="\$asmId.ncbiGene.ucsc.genePred.gz"
 _EOF_
     );
   }
   $bossScript->add(<<_EOF_
   ~/kent/src/hg/utils/automation/gpToIx.pl \$ncbiGenePred \\
     > \$asmId.gpToIx.txt
-  ~/kent/src/hg/utils/automation/gffAttrsToIx.py \$asmId.geneAttrs.ncbi.txt \\
+  ~/kent/src/hg/utils/automation/gffAttrsToIx.py \$asmId.geneAttrs.ncbi.txt.new \\
      \$ncbiGenePred > \$asmId.attrsToIx.txt
   sort -u \$asmId.gpToIx.txt \$asmId.attrsToIx.txt > \$asmId.ncbiGene.ix.txt
   if [ -s \$asmId.ncbiGene.ix.txt ]; then
     ixIxx \$asmId.ncbiGene.ix.txt \$asmId.ncbiGene.ix.new \$asmId.ncbiGene.ixx.new
   fi
   rm -f \$asmId.ncbiGene.ix.txt \$asmId.gpToIx.txt \$asmId.attrsToIx.txt
   genePredToBigGenePred \$ncbiGenePred stdout \\
       | sort -k1,1 -k2,2n > \$asmId.ncbiGene.bed
   (bedToBigBed -type=bed12+8 -tab -as=\$HOME/kent/src/hg/lib/bigGenePred.as \\
       -extraIndex=name \$asmId.ncbiGene.bed \\
         \$chromSizes \$asmId.ncbiGene.bb.new || true)
   if [ ! -s "\$asmId.ncbiGene.bb.new" ]; then
     printf "# ncbiGene: failing bedToBigBed\\n" 1>&2
     exit 255
   fi
   touch -r\$gffFile \$asmId.ncbiGene.bb.new
   bigBedInfo \$asmId.ncbiGene.bb.new | egrep "^itemCount:|^basesCovered:" \\
-    | sed -e 's/,//g' > \$asmId.ncbiGene.stats.txt
-  LC_NUMERIC=en_US /usr/bin/printf "# ncbiGene %s %'d %s %'d\\n" `cat \$asmId.ncbiGene.stats.txt` | xargs echo
+    | sed -e 's/,//g' > \$asmId.ncbiGene.stats.txt.new
+  LC_NUMERIC=en_US /usr/bin/printf "# ncbiGene %s %'d %s %'d\\n" `cat \$asmId.ncbiGene.stats.txt.new` | xargs echo
 
   # basesCovered comes straight out of the bigBedInfo call above --
   # no need to separately rebuild exons via bedToExons/bedSingleCover.pl
   export totalBases=`ave -col=2 \$chromSizes | grep total | awk '{printf "%d", \$NF}'`
-  export basesCovered=`grep basesCovered \$asmId.ncbiGene.stats.txt | awk '{printf "%s", \$NF}'`
+  export basesCovered=`grep basesCovered \$asmId.ncbiGene.stats.txt.new | awk '{printf "%s", \$NF}'`
   export percentCovered=`echo \$basesCovered \$totalBases | awk '{printf "%.3f", 100.0*\$1/\$2}'`
-  printf "%d bases of %d (%s%%) in intersection\\n" "\$basesCovered" "\$totalBases" "\$percentCovered" > fb.\$asmId.ncbiGene.txt
-
-  # atomic swap -- a client mid-read of the .bb/.ix/.ixx over http never
-  # sees a half-written file, unlike the old build-in-place behavior
-  mv -f \$asmId.ncbiGene.bb.new \$asmId.ncbiGene.bb
-  if [ -s \$asmId.ncbiGene.ix.new ]; then
-    mv -f \$asmId.ncbiGene.ix.new \$asmId.ncbiGene.ix
-    mv -f \$asmId.ncbiGene.ixx.new \$asmId.ncbiGene.ixx
-  fi
+  printf "%d bases of %d (%s%%) in intersection\\n" "\$basesCovered" "\$totalBases" "\$percentCovered" > fb.\$asmId.ncbiGene.txt.new
+
+  # Everything above is built entirely under *.new (or otherwise
+  # non-live) file names -- nothing under the live \$asmId.ncbiGene.*
+  # names has been touched.  The calling doNcbiGene.pl only archives the
+  # previous live version and promotes these *.new files into place
+  # once this script has exited 0, so a failure at any point above
+  # (this script runs under 'set -e') leaves the existing live track
+  # completely untouched.
 else
   printf "# ncbiGene step previously completed\\n" 1>&2
 fi
 _EOF_
   );
   $bossScript->execute();
+
+  # reached only if the boss script above exited 0 -- the new build is
+  # complete and validated under *.new file names, so it is now safe to
+  # archive the previous live version and promote the new one into place
+  archivePriorVersion();
+  promoteNewBuild();
 } # doNcbiGene
 
 #########################################################################
 # * step: cleanup [fileServer]
 sub doCleanup {
   my $whatItDoes = "compress intermediate files";
   my $bossScript = new HgRemoteScript("$buildDir/doCleanup.csh", $fileServer,
 				      $buildDir, $whatItDoes);
   $bossScript->add(<<_EOF_
-gzip -f $db.geneAttrs.ncbi.txt $db.ncbiGene.log.txt
+gzip -f $db.geneAttrs.ncbi.txt $db.ncbiGene.log.txt $db.ncbiGene.bed
 _EOF_
   );
   $bossScript->execute();
 } # doCleanup
 
 #########################################################################
 # main
 
 &HgAutomate::closeStdin();
 
 &checkOptions();
 &usage(1) if (scalar(@ARGV) != 2);
 
 $secondsStart = `date "+%s"`;
 chomp $secondsStart;