Skip to content

Commit 6ca0ef8

Browse files
committed
fix: make PagingLogger IDisposable to prevent partial log loss on step fault
Steps that throw before Complete() never reach End(), silently dropping their partial page and results block. Make IPagingLogger : IDisposable and implement Dispose() (canonical pattern, idempotent, never throws) to flush+queue partial content and release the FileStream on any exit.
1 parent b06c585 commit 6ca0ef8

2 files changed

Lines changed: 489 additions & 1 deletion

File tree

src/Runner.Common/Logging.cs

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
namespace GitHub.Runner.Common
55
{
66
[ServiceLocator(Default = typeof(PagingLogger))]
7-
public interface IPagingLogger : IRunnerService
7+
public interface IPagingLogger : IRunnerService, IDisposable
88
{
99
long TotalLines { get; }
1010
void Setup(Guid timelineId, Guid timelineRecordId);
@@ -45,6 +45,8 @@ public class PagingLogger : RunnerService, IPagingLogger
4545
private int _blockByteCount;
4646
private int _blockCount;
4747

48+
private bool _disposed;
49+
4850
public long TotalLines => _totalLines;
4951

5052
public override void Initialize(IHostContext hostContext)
@@ -164,5 +166,75 @@ private void EndBlock(bool finalize)
164166
_jobServerQueue.QueueResultsUpload(_timelineRecordId, "ResultsLog", _resultsDataFileName, "Results.Core.Log", deleteSource: true, finalize, firstBlock: _resultsDataFileName.EndsWith(".1"), totalLines: _totalLines);
165167
}
166168
}
169+
170+
// Close any page/block writers that were opened by Write() but never
171+
// finalized by End() - for example when the owning step throws between
172+
// Setup() and End(). Prior to this being IDisposable, such a throw left
173+
// the FileStream handles open until GC finalization eventually ran, and
174+
// - more importantly - silently dropped the partial page's content:
175+
// QueueFileUpload/QueueResultsUpload are only invoked from EndPage /
176+
// EndBlock, so any partial page on a faulted step was never shipped to
177+
// the server-side log. On long-running self-hosted runners this also
178+
// adds pressure on the per-process file descriptor quota (the runner's
179+
// systemd template inherits systemd's DefaultLimitNOFILE, which ranges
180+
// from 1024 on older distros to 524288+ on modern systemd >= 240).
181+
//
182+
// Dispose is best-effort and must not throw, because it also runs from
183+
// service-shutdown paths. We still try to flush+upload partial content
184+
// so no log data is silently dropped; failures there are swallowed.
185+
public void Dispose()
186+
{
187+
Dispose(true);
188+
GC.SuppressFinalize(this);
189+
}
190+
191+
protected virtual void Dispose(bool disposing)
192+
{
193+
if (_disposed)
194+
{
195+
return;
196+
}
197+
198+
if (disposing)
199+
{
200+
// Try the normal flush+queue path first. If it throws (e.g. the
201+
// JobServerQueue is already shutting down) we fall through to the
202+
// safety-net cleanup below.
203+
try { EndPage(); } catch { }
204+
205+
// Safety net: EndPage early-returns when _pageWriter is null, so
206+
// it does not cover the case where the StreamWriter ctor in
207+
// NewPage threw after the FileStream was opened (leaving
208+
// _pageData non-null with _pageWriter null). Always release any
209+
// remaining handle so the fd is returned to the OS.
210+
if (_pageWriter != null)
211+
{
212+
try { _pageWriter.Dispose(); } catch { }
213+
_pageWriter = null;
214+
_pageData = null;
215+
}
216+
else if (_pageData != null)
217+
{
218+
try { _pageData.Dispose(); } catch { }
219+
_pageData = null;
220+
}
221+
222+
try { EndBlock(finalize: true); } catch { }
223+
224+
if (_resultsBlockWriter != null)
225+
{
226+
try { _resultsBlockWriter.Dispose(); } catch { }
227+
_resultsBlockWriter = null;
228+
_resultsBlockData = null;
229+
}
230+
else if (_resultsBlockData != null)
231+
{
232+
try { _resultsBlockData.Dispose(); } catch { }
233+
_resultsBlockData = null;
234+
}
235+
}
236+
237+
_disposed = true;
238+
}
167239
}
168240
}

0 commit comments

Comments
 (0)