@@ -86,8 +86,12 @@ private synchronized void open() throws IOException {
8686 os = new FileOutputStream (log , true );
8787 if (index .isFile ()) {
8888 try (BufferedReader r = Files .newBufferedReader (index .toPath (), StandardCharsets .UTF_8 )) {
89+ // TODO would be faster to scan the file backwards for the penultimate \n, then convert the byte sequence from there to EOF to UTF-8 and set lastId accordingly
8990 String lastLine = null ;
9091 while (true ) {
92+ // Note that BufferedReader tolerates final lines without a line separator, so if for some reason the last write has been truncated this result could be incorrect.
93+ // In practice this seems unlikely since we explicitly flush after the newline, so we should be sending a single small block to the filesystem to persist.
94+ // Anyway at worst the result would be a (perhaps temporarily) incorrect line → step mapping, which is tolerable for one step of one build, and barely affects the overall build log.
9195 String line = r .readLine ();
9296 if (line == null ) {
9397 break ;
@@ -122,6 +126,9 @@ private void checkId(String id) throws IOException {
122126 } else {
123127 indexOs .write (pos + " " + id + "\n " );
124128 }
129+ // Could call FileChannel.force(true) like hudson.util.FileChannelWriter does for AtomicFileWriter,
130+ // though making index-log writes slower is likely a poor tradeoff for slightly more reliable log display,
131+ // since logs are often never read and this is transient data rather than configuration or valuable state.
125132 indexOs .flush ();
126133 lastId = id ;
127134 }
@@ -180,13 +187,23 @@ private final class IndexOutputStream extends OutputStream {
180187 try (BufferedReader indexBR = index .isFile () ? Files .newBufferedReader (index .toPath (), StandardCharsets .UTF_8 ) : new BufferedReader (new NullReader (0 ))) {
181188 ConsoleAnnotationOutputStream <FlowExecutionOwner .Executable > caos = new ConsoleAnnotationOutputStream <>(w , ConsoleAnnotators .createAnnotator (build ), build , StandardCharsets .UTF_8 );
182189 long r = this .writeRawLogTo (start , new FilterOutputStream (caos ) {
190+ // To insert startStep/endStep annotations into the overall log, we need to simultaneously read index-log.
191+ // We use the standard LargeText.FileSession to get the raw log text (we need not think about ConsoleNote here), having seeked to the start position.
192+ // Then we read index-log in order, looking for transitions from one step to the next (or to or from non-step overall output).
193+ // Whenever we are about to write a byte which is at a boundary, or if there is a boundary at EOF, the HTML annotations are injected into the output;
194+ // the read of index-log is advanced lazily (it is not necessary to have the whole mapping in memory).
183195 long lastTransition = -1 ;
196+ boolean eof ; // NullReader is strict and throws IOException (not EOFException) if you read() again after having already gotten -1
184197 String lastId ;
185198 long pos = start ;
186199 boolean hadLastId ;
187200 @ Override public void write (int b ) throws IOException {
188- String line ;
189- while (lastTransition < pos && (line = indexBR .readLine ()) != null ) {
201+ while (lastTransition < pos && !eof ) {
202+ String line = indexBR .readLine ();
203+ if (line == null ) {
204+ eof = true ;
205+ break ;
206+ }
190207 int space = line .indexOf (' ' );
191208 try {
192209 lastTransition = Long .parseLong (space == -1 ? line : line .substring (0 , space ));
@@ -226,6 +243,12 @@ private final class IndexOutputStream extends OutputStream {
226243 try (ByteBuffer buf = new ByteBuffer ();
227244 RandomAccessFile raf = new RandomAccessFile (log , "r" );
228245 BufferedReader indexBR = index .isFile () ? Files .newBufferedReader (index .toPath (), StandardCharsets .UTF_8 ) : new BufferedReader (new NullReader (0 ))) {
246+ // Check this _before_ reading index-log to reduce the chance of a race condition resulting in recent content being associated with the wrong step:
247+ long end = raf .length ();
248+ // To produce just the output for a single step (again we do not need to pay attention to ConsoleNote here since AnnotatedLargeText handles it),
249+ // index-log is read looking for transitions that pertain to this step: beginning or ending its content, including at EOF if applicable.
250+ // (Other transitions, such as to or from unrelated steps, are irrelevant).
251+ // Once a start and end position have been identified, that block is copied to a memory buffer.
229252 String line ;
230253 long pos = -1 ; // -1 if not currently in this node, start position if we are
231254 while ((line = indexBR .readLine ()) != null ) {
@@ -234,27 +257,42 @@ private final class IndexOutputStream extends OutputStream {
234257 try {
235258 lastTransition = Long .parseLong (space == -1 ? line : line .substring (0 , space ));
236259 } catch (NumberFormatException x ) {
260+ // If index-log is corrupt for whatever reason, we given up on this step in this build;
261+ // there is no way we would be able to produce accurate output anyway.
262+ // Note that NumberFormatException is already logged separately for the overall build log,
263+ // which is in that case nonfatal: the whole-build HTML output always includes exactly what is in the main log file,
264+ // at worst with some missing or inaccurate startStep/endStep annotations.
237265 break ; // corrupt index file; forget it
238266 }
239267 if (pos == -1 ) {
240268 if (space != -1 && line .substring (space + 1 ).equals (id )) {
241269 pos = lastTransition ;
242270 }
243- } else {
271+ } else if ( lastTransition > pos ) {
244272 raf .seek (pos );
245273 if (lastTransition > pos + Integer .MAX_VALUE ) {
246274 throw new IOException ("Cannot read more than 2Gib at a time" ); // ByteBuffer does not support it anyway
247275 }
248- // TODO can probably be done a bit more efficiently with FileChannel methods
276+ // Could perhaps be done a bit more efficiently with FileChannel methods,
277+ // at least if org.kohsuke.stapler.framework.io.ByteBuffer were replaced by java.nio.[Heap]ByteBuffer.
278+ // The overall bottleneck here is however the need to use a memory buffer to begin with:
279+ // LargeText.Source/Session are not public so, pending improvements to Stapler,
280+ // we cannot lazily stream per-step content the way we do for the overall log.
281+ // (Except perhaps by extending ByteBuffer and then overriding every public method!)
282+ // LargeText also needs to be improved to support opaque (non-long) cursors
283+ // (and callers such as progressiveText.jelly and Blue Ocean updated accordingly),
284+ // which is a hard requirement for efficient rendering of cloud-backed logs,
285+ // though for this implementation we do not need it since we can work with byte offsets.
249286 byte [] data = new byte [(int ) (lastTransition - pos )];
250287 raf .readFully (data );
251288 buf .write (data );
252289 pos = -1 ;
253- }
290+ } // else some sort of mismatch
254291 }
255- if (pos != -1 ) {
292+ if (pos != -1 && /* otherwise race condition? */ end > pos ) {
293+ // In case the build is ongoing and we are still actively writing content for this step,
294+ // we will hit EOF before any other transition. Otherwise identical to normal case above.
256295 raf .seek (pos );
257- long end = raf .length ();
258296 if (end > pos + Integer .MAX_VALUE ) {
259297 throw new IOException ("Cannot read more than 2Gib at a time" );
260298 }
0 commit comments