From 5f2e509166ef8c6e71d3e98bf5dd8a8ff7afc304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deniz=20=C4=B0rgin?= Date: Sat, 17 Oct 2026 00:54:29 +0300 Subject: [PATCH] fix: contender stderr redacts a handoff ticket the tail cut or the pipe end splits (#399) The contender cut its stderr to the final 8 KiB before redacting, and matched whole tickets only, so a ticket straddling the cut kept its suffix in Stderr and in failure messages. The raw tail now keeps one ticket length of slack before the rendered 8 KiB, tickets are matched in bytes before decoding, and a ticket the pipe ended inside is redacted as the prefix it is. --- CHANGELOG.md | 3 + .../Internal/Contender/ServiceContender.cs | 97 ++++++++++++++----- .../Contender/ServiceContenderTests.cs | 81 ++++++++++++++++ 3 files changed, 158 insertions(+), 23 deletions(-) create mode 100644 tests/OpenCode.Sdk.Tests/Hosting/BackgroundService/Contender/ServiceContenderTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ecf95be..488b592f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,9 @@ Nightly builds of `master` are on ### 🐛 Fixes +- **A background-service start failure no longer carries part of the PTY handoff ticket.** The + contender's stderr tail redacts the ticket even where the 8 KiB cut or the end of the pipe splits + it, so no fragment of it reaches `OpenCodeServerException` messages. - **Caller cancellation preserves your token across public async doors:** HTTP, SSE, cursor traversal, both terminal families, and `OpenCodeServer.StartAsync`, `DiscoverAsync`, `EnsureAsync`, and `StopAsync`. `OperationCanceledException.CancellationToken` is the diff --git a/src/OpenCode.Sdk/Hosting/BackgroundService/Internal/Contender/ServiceContender.cs b/src/OpenCode.Sdk/Hosting/BackgroundService/Internal/Contender/ServiceContender.cs index 666802fb..c741a423 100644 --- a/src/OpenCode.Sdk/Hosting/BackgroundService/Internal/Contender/ServiceContender.cs +++ b/src/OpenCode.Sdk/Hosting/BackgroundService/Internal/Contender/ServiceContender.cs @@ -29,6 +29,8 @@ internal sealed class ServiceContender : IServiceContender /// What a redacted handoff ticket reads as in diagnostics. private const string RedactedMarker = "[redacted]"; + private static readonly byte[] RedactedMarkerBytes = Encoding.UTF8.GetBytes(RedactedMarker); + /// Windows' still-running sentinel for GetExitCodeProcess. private const uint StillActive = 259; @@ -47,7 +49,14 @@ internal sealed class ServiceContender : IServiceContender private readonly Lock _gate = new(); private readonly Stream _stderr; private readonly SafeProcessHandle? _process; - private readonly string? _redact; + /// The handoff ticket's UTF-8 bytes, matched in the raw tail so a cut cannot split a match. + private readonly byte[]? _redact; + + /// + /// The raw bytes retained: the rendered 8 KiB, plus one ticket length short of it when a ticket + /// is redacted, so a ticket the 8 KiB edge cuts through is still found whole. + /// + private readonly int _retainLimit; private byte[] _tail = []; private bool _released; @@ -65,7 +74,8 @@ public ServiceContender(int processId, Stream stderr, SafeProcessHandle? process ProcessId = processId; _stderr = stderr; _process = process; - _redact = string.IsNullOrEmpty(redact) ? null : redact; + _redact = string.IsNullOrEmpty(redact) ? null : Encoding.UTF8.GetBytes(redact); + _retainLimit = StderrLimit + (_redact is null ? 0 : _redact.Length - 1); // Hot until the first read suspends: the drain owns no caller, only the pipe, and every // fault it can see is folded into the error slot — nothing escapes unobserved. @@ -199,31 +209,71 @@ private string RenderStderrLocked() return string.Empty; } - var text = Encoding.UTF8.GetString(_tail); - if (_redact is { Length: > 0 } ticket) - { - text = RedactTicket(text, ticket); - } - + var start = Math.Max(0, _tail.Length - StderrLimit); + var text = _redact is null + ? Encoding.UTF8.GetString(_tail, start, _tail.Length - start) + : RedactTicket(_tail, start, _redact); return text.Trim(); } /// - /// Ordinal ticket redaction without the framework's comparison overload, which the downlevel - /// targets do not carry: split at every occurrence and rejoin over the marker. + /// Renders the bytes from with every byte a handoff ticket covers + /// replaced, each covered run by one marker, before any decoding. Every occurrence counts, + /// overlapping ones too; a ticket the 8 KiB edge cuts through is matched whole in the retained + /// bytes before the edge, and a ticket the pipe has not finished writing ends the tail as a + /// prefix and is covered as one. /// - private static string RedactTicket(string text, string ticket) + private static string RedactTicket(byte[] tail, int start, byte[] ticket) + { + var covered = new bool[tail.Length - start]; + var searchFrom = Math.Max(0, start - (ticket.Length - 1)); + while (searchFrom < tail.Length && tail.AsSpan(searchFrom).IndexOf(ticket) is >= 0 and var offset) + { + var index = searchFrom + offset; + Cover(covered, start, index, index + ticket.Length); + searchFrom = index + 1; + } + + var unfinished = UnfinishedTicketLength(tail, ticket); + Cover(covered, start, tail.Length - unfinished, tail.Length); + + using var rendered = new MemoryStream(covered.Length); + for (var position = 0; position < covered.Length; position++) + { + if (!covered[position]) + { + rendered.WriteByte(tail[start + position]); + } + else if (position == 0 || !covered[position - 1]) + { + rendered.Write(RedactedMarkerBytes, 0, RedactedMarkerBytes.Length); + } + } + + return Encoding.UTF8.GetString(rendered.GetBuffer(), 0, (int)rendered.Length); + } + + /// Marks the tail bytes [, ) that fall inside the window at . + private static void Cover(bool[] covered, int start, int from, int to) { - var redacted = new StringBuilder(text.Length); - var remainder = text; - while (remainder.IndexOf(ticket, StringComparison.Ordinal) is >= 0 and var index) + for (var index = Math.Max(from, start); index < to; index++) { - _ = redacted.Append(remainder, 0, index).Append(RedactedMarker); - remainder = remainder[(index + ticket.Length)..]; + covered[index - start] = true; + } + } + + /// The length of the longest proper ticket prefix the retained tail ends with; zero when none. + private static int UnfinishedTicketLength(byte[] tail, byte[] ticket) + { + for (var length = Math.Min(ticket.Length - 1, tail.Length); length > 0; length--) + { + if (tail.AsSpan().EndsWith(ticket.AsSpan(0, length))) + { + return length; + } } - _ = redacted.Append(remainder); - return redacted.ToString(); + return 0; } private string WithStderrLocked(string message) @@ -233,9 +283,10 @@ private string WithStderrLocked(string message) } /// - /// Keeps the final 8 KiB of the pipe the way upstream's onStderr does: the chunk's own - /// last 8 KiB, behind as much of the previous tail as still fits. A released contender still - /// drains, into nothing. + /// Keeps the end of the pipe the way upstream's onStderr does: the chunk's own last + /// bytes, behind as much of the previous tail as still fits. The limit is 8 KiB, plus the + /// ticket slack when a ticket is redacted; rendering still shows only the final 8 KiB. A + /// released contender still drains, into nothing. /// private void AppendTailLocked(byte[] buffer, int count) { @@ -244,8 +295,8 @@ private void AppendTailLocked(byte[] buffer, int count) return; } - var kept = Math.Min(count, StderrLimit); - var carried = Math.Min(_tail.Length, StderrLimit - kept); + var kept = Math.Min(count, _retainLimit); + var carried = Math.Min(_tail.Length, _retainLimit - kept); var next = new byte[carried + kept]; Buffer.BlockCopy(_tail, _tail.Length - carried, next, 0, carried); Buffer.BlockCopy(buffer, count - kept, next, carried, kept); diff --git a/tests/OpenCode.Sdk.Tests/Hosting/BackgroundService/Contender/ServiceContenderTests.cs b/tests/OpenCode.Sdk.Tests/Hosting/BackgroundService/Contender/ServiceContenderTests.cs new file mode 100644 index 00000000..b95af1d3 --- /dev/null +++ b/tests/OpenCode.Sdk.Tests/Hosting/BackgroundService/Contender/ServiceContenderTests.cs @@ -0,0 +1,81 @@ +using System.Text; +using OpenCode.Sdk.Internal; + +namespace OpenCode.Sdk.Tests.Hosting.BackgroundService.Contender; + +/// +/// The contender's retained stderr as diagnostics read it. The stderr is an in-memory stream, which +/// the drain consumes to its end inside the constructor, so every verdict is decided before the +/// first assertion. The pid is this process, never a child of it: a POSIX exit poll reads that as +/// reaped elsewhere, and a Windows poll has no handle to read. +/// +public sealed class ServiceContenderTests +{ + /// The pinned client's stderrLimit (service-contender.ts). + private const int StderrLimit = 8 * 1024; + + private const string Ticket = "{\"source\":\"sdk-test\",\"secret\":\"0123456789abcdef0123456789abcdef\"}"; + + [Test] + public async Task Stderr_Should_Redact_Every_Whole_Ticket() + { + using var contender = Contender("before " + Ticket + " middle " + Ticket + " after"); + + await Assert.That(contender.Stderr).IsEqualTo("before [redacted] middle [redacted] after"); + } + + /// + /// The 8 KiB edge falls inside the ticket: one byte, a few bytes, or all but one of its 65 + /// bytes lie before the edge. + /// + [Test] + [Arguments(1)] + [Arguments(13)] + [Arguments(64)] + public async Task Stderr_Should_Redact_A_Ticket_The_Tail_Cut_Splits(int bytesBeforeTheCut) + { + var after = new string('b', StderrLimit - (Ticket.Length - bytesBeforeTheCut)); + using var contender = Contender(new string('a', 100) + Ticket + after); + + await Assert.That(contender.Stderr).IsEqualTo("[redacted]" + after); + } + + [Test] + public async Task Stderr_Should_Redact_A_Ticket_The_Pipe_Ended_Inside() + { + using var contender = Contender("starting " + Ticket[..20]); + + await Assert.That(contender.Stderr).IsEqualTo("starting [redacted]"); + } + + /// A ticket whose end repeats its start: the occurrences overlap and cover one run. + [Test] + public async Task Stderr_Should_Redact_Overlapping_Tickets_As_One_Run() + { + using var contender = Contender("x ababab y", redact: "abab"); + + await Assert.That(contender.Stderr).IsEqualTo("x [redacted] y"); + } + + /// A ticket longer than the rendered 8 KiB that the pipe ended inside: no byte of it shows. + [Test] + public async Task Stderr_Should_Redact_An_Unfinished_Ticket_Longer_Than_The_Tail() + { + var body = new string('x', StderrLimit); + using var contender = Contender("start A" + body, redact: "A" + body + "Z"); + + await Assert.That(contender.Stderr).IsEqualTo("[redacted]"); + } + + [Test] + public async Task Stderr_Should_Keep_The_Final_8_KiB_Unchanged_Without_A_Ticket() + { + var kept = new string('k', StderrLimit); + using var contender = Contender(new string('d', 100) + kept, redact: null); + + await Assert.That(contender.Stderr).IsEqualTo(kept); + } + + private static ServiceContender Contender(string stderr, string? redact = Ticket) => + new(Environment.ProcessId, new MemoryStream(Encoding.UTF8.GetBytes(stderr)), process: null, redact); +}