refactor(runtime): move the log, the change signal and store opening out of AgentRuntime #38

2026-11-11T00:36:59Z

james.maes commented

2026-11-11 00:36:59 +00:00

The first runtime step of the multi-server build. It moves three per-computer services out of the single AgentRuntime class, so the later split into a host and one connection per server is a move, not a rewrite. Printing, pairing, the pipe and every log line's wording are unchanged, with one declared exception (redaction, below).

Moved

ServiceLog: what WriteLog, WriteFailure and the session diagnostics did, with the 100-entry ring and the activity log file. ForServer(name) gives a view that prefixes a server's name; nothing uses it yet. The name and the message are redacted separately, so a name cannot stop a secret later in its line from being redacted, and a name cannot start a new line or reorder one.
ChangeSignal and ReleaseOnce: the pipeline's wake-up signal and its release-once lease, as types of their own that a connection and a printer queue can both use. The six places that wait on the signal now take it before they read state, so no change can be missed whichever lock the state sits under later.
One OpenStore(recover) in place of 22 scattered constructions of the store, each with the flag it passed before.
Download pacing runs on the injected clock.

The one visible change: the log's redaction now also covers the value of a Cookie: or Set-Cookie: header line and the code and state values of a callback address. Neither reaches a service log line today (traced in review); this is defense in depth. Ordinary lines such as "status code: 401" and "state=Paused" are untouched. Declared in CHANGELOG.md.

Evidence that nothing else changed: a diff of the seventeen runtime files with the mechanical renames normalized contains only the hand-made edits; a reference test writes lines through ServiceLog and through a transcription of the old method and compares the file, the sink and the ring; the existing runtime suite passes unchanged.

Tests: 60 new cases. The concurrency test for the signal fails within 30 seconds when the exchange is deliberately made non-atomic and passes in a tenth of a second on the real code.

Verification, local, on this head (on 06ab0a8): build/Verify.ps1 passed twice with no failing test: 766 Core, 1,109 Windows/command-line and 352 presentation tests, zero warnings; Core, command line and presentation 100%; Windows 74.7 to 74.9% (minimum stays 73).

Review: independent; two should-fix items (the name-and-redaction case; no test for the atomic exchange), both fixed as proposed, plus the nits. Recorded for later: Authorization: Basic and code_verifier= are still not redacted and nobody has traced whether a service line can carry them; a server name that looks like a credential keyword is over-redacted in the file.

🤖 Generated with Claude Code

The first runtime step of the multi-server build. It moves three per-computer services out of the single `AgentRuntime` class, so the later split into a host and one connection per server is a move, not a rewrite. Printing, pairing, the pipe and every log line's wording are unchanged, with one declared exception (redaction, below). **Moved** - `ServiceLog`: what `WriteLog`, `WriteFailure` and the session diagnostics did, with the 100-entry ring and the activity log file. `ForServer(name)` gives a view that prefixes a server's name; nothing uses it yet. The name and the message are redacted separately, so a name cannot stop a secret later in its line from being redacted, and a name cannot start a new line or reorder one. - `ChangeSignal` and `ReleaseOnce`: the pipeline's wake-up signal and its release-once lease, as types of their own that a connection and a printer queue can both use. The six places that wait on the signal now take it before they read state, so no change can be missed whichever lock the state sits under later. - One `OpenStore(recover)` in place of 22 scattered constructions of the store, each with the flag it passed before. - Download pacing runs on the injected clock. **The one visible change**: the log's redaction now also covers the value of a `Cookie:` or `Set-Cookie:` header line and the `code` and `state` values of a callback address. Neither reaches a service log line today (traced in review); this is defense in depth. Ordinary lines such as "status code: 401" and "state=Paused" are untouched. Declared in `CHANGELOG.md`. **Evidence that nothing else changed**: a diff of the seventeen runtime files with the mechanical renames normalized contains only the hand-made edits; a reference test writes lines through `ServiceLog` and through a transcription of the old method and compares the file, the sink and the ring; the existing runtime suite passes unchanged. **Tests**: 60 new cases. The concurrency test for the signal fails within 30 seconds when the exchange is deliberately made non-atomic and passes in a tenth of a second on the real code. **Verification**, local, on this head (on `06ab0a8`): `build/Verify.ps1` passed twice with no failing test: 766 Core, 1,109 Windows/command-line and 352 presentation tests, zero warnings; Core, command line and presentation 100%; Windows 74.7 to 74.9% (minimum stays 73). **Review**: independent; two should-fix items (the name-and-redaction case; no test for the atomic exchange), both fixed as proposed, plus the nits. Recorded for later: `Authorization: Basic` and `code_verifier=` are still not redacted and nobody has traced whether a service line can carry them; a server name that looks like a credential keyword is over-redacted in the file. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 12 commits 2026-11-11 00:37:00 +00:00

refactor(runtime): move the service log out of AgentRuntime into ServiceLog 977a2e1969

WriteLog, WriteFailure and SessionDiagnostics, with the in-memory ring and
the activity log they wrote to, become one type the runtime holds and
calls. The log belongs to the computer, not to one server connection, and
the later split of the runtime needs to hand each connection a log of its
own: ForServer(name) gives a view that puts the server's name in front of
every line and otherwise behaves the same (same redaction, same caps with
the name counted in, same debug routing, same ring and file). Nothing
calls it yet.

No behavior change. The 79 call sites are renamed and nothing else; every
line still goes through the same redaction, is cut at 1000 characters
(4000 for debug detail), reaches the ring (newest 100), the file with the
"[Service]" marker and the host's sink, and debug detail still goes to the
file only while debug logging is on. The ring is now guarded by the log's
own lock instead of the runtime's state lock.

ServiceLogTests holds the type to a transcription of the method it
replaces, line for line in the log file, and covers redaction, caps, the
ring, debug routing, failures, session diagnostics and the server view.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

refactor(runtime): ChangeSignal and ReleaseOnce as types of their own 271e291cea

The pipeline's change signal (NewSignal, SignalPipeline and the field
they swapped) becomes ChangeSignal, and PipelineLease becomes ReleaseOnce,
so that a server connection and a printer queue can each use them once the
runtime is split. The 17 raises, 7 waits and 3 leases are renamed and
nothing else.

No behavior change. A waiter still takes the task for the next change
inside the state lock in which it looks at the state, and a change is
still raised after the state was changed, so no change can be missed. The
swap itself is now an atomic exchange inside the signal instead of a swap
under the runtime's state lock; waiters still run asynchronously, never on
the raising thread.

Tests: every waiter is woken once per change; a change between the look
and the wait is not lost, also under concurrency; raising never runs
waiter code on the raising thread; a lease releases exactly once under
repeated and concurrent disposal.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

refactor(runtime): open the store in one place 2ad23c0fe2

The runtime constructed AgentStore at 22 sites across 12 of its partial
files. All of them now call one private OpenStore(recover), so the later
change to "a store opened for one server" is made in one place. Each site
passes the same recovery flag it passed before (10 with recovery, 12
without); nothing else changes.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

refactor(runtime): pace downloads on the injected clock 1d9872067f

ThrottleDownloadAsync read the system stopwatch and waited on real time,
the last use of real time in the claim pipeline that the runtime's clock
could not replace. It now measures its schedule on the injected
TimeProvider, in seconds since the runtime was created (a new field holds
that starting point), and waits on the same clock. The wait is rounded to
the clock's unit instead of cut short of it, so a wait that should be a
whole number of milliseconds is not delivered one millisecond early when
the arithmetic in fractions of a second comes out a hair low. With the
system clock nothing changes: the timestamps and the delay are the ones
the stopwatch and Task.Delay gave before.

The method is internal so a test can call it. DownloadThrottleTests runs a
runtime on a manual clock: two chunks share one schedule and are released
at exactly their moments, never a tick early; an idle period is not saved
up; the pacing is exact whatever the clock reads (eighty irregular
arrivals); no rate means no wait; a cancelled download stops waiting. The
existing real-time bandwidth test is unchanged and still passes.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

docs(development): download pacing now runs on the injected clock c75c6e5d65

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

fix(log): a server's name cannot defeat the redaction of its line a096be1fea

ForServer(name) put the name in front of the message and redacted the two
as one string. The named-secret pattern reads a quoted value up to the
next quote, so a server named `A token="` writing
`Sign-in failed: password="hunter2" rejected` had the text between the
name's quote and the password's opening quote redacted as the token, and
`hunter2` was left in the ring, the file and the host's sink.

The name is now redacted once, on its own, when the view is made, and the
message is redacted on its own and joined to it afterwards. The caps are
applied to the joined line as before (1000 characters, 4000 for debug
detail, with the name counted in).

The name is also made safe against characters that are not control
characters and so passed through: the line and paragraph separators
(U+2028, U+2029) and the format characters, which include the
bidirectional embeddings, overrides and isolates. Each becomes a space, as
a control character already did; it is the rule Core applies to text a
server sends. The name is trimmed after that instead of before, and a name
with nothing left to show is refused.

Nothing calls ForServer in production yet, so no log line changes.

Tests: the quoted name above, a name with an open apostrophe and a name
that carries a secret of its own, for an operational line, debug detail
and a failure (the two quoted cases fail on the previous code with
`hunter2` in the line); seventeen characters that end or reorder a line,
each of which becomes a space, and a name made only of them is refused.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

test(runtime): concurrent raisers must not strand a waiter ba7d0d7f6c

Nothing failed if the exchange in ChangeSignal.Raise stopped being atomic,
and that exchange is the one mechanism that replaced the swap under the
runtime's state lock. With a read followed by a write, two raises can read
the same task; the first puts in a fresh one, a waiter takes it, and the
second overwrites it, so nobody ever completes the task the waiter holds.

ConcurrentRaisers_NeverStrandAWaiter runs four raisers on threads of their
own, stepping one counter 50,000 times each, while eight followers use the
take, look, wait pattern. Every follower must reach the last step. Each
wait is bounded at 30 seconds, so a stranded follower is a failed test and
not a hung run.

Checked by mutation: with Raise written as read, write, complete, the new
test failed in four runs of four with a TimeoutException after 30 seconds,
and the seven tests that existed before still passed. With the atomic
exchange restored it passes in about a tenth of a second.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

test(runtime): make two assertions that tested nothing test something 96a8e8c8a9

ChangeSignalTests compared a task with itself. It now takes the task twice
before the change and asserts both are the same one, which is what lets
every waiter that arrives before a change be woken by it.

ServiceLogTests.ConcurrentWriters_LoseNothing asserted that a view of a
different log, which no writer in the test wrote to, was empty. It now
asserts what the test is for: the ring ends with 100 lines, each once, all
of them lines that were written, and the same 100 when read through a
server's view of the log that was written to.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

fix(log): stamp a ring entry inside the lock that orders the ring 1af1fe838f

When the log moved out of the runtime, the entry's timestamp was taken
before the ring's lock, so two writers could take their times in one order
and enter the ring in the other. The runtime's own method stamped inside
the lock. The stamp is taken inside the ring's lock again, so the ring's
order is the order of its timestamps.

To test that without waiting on real time, ServiceLog takes an optional
clock (TimeProvider). The runtime does not pass one, so it stamps with the
system clock exactly as before.

Tests: a counting clock holds the first writer's reading open while a
second writer writes a whole line on a thread of its own. With the stamp
inside the lock the second writer cannot finish during the reading and the
ring is first, second with stamps 1, 2. Checked by mutation: with the
stamp taken before the lock the test failed in four runs of four (the ring
was second, first). The concurrent-writers test now also asserts that the
ring holds the last 100 readings in order; it holds on every run but did
not catch the mutation in four runs, and its summary says so.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

style(log): three blank lines between ServiceLog's documented constants d20a011480

The four documented constants and the fields after them were one blank
line apart. They are members with a summary of their own, so they are now
three blank lines apart like the rest of the type. Blank lines only.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

refactor(runtime): waiters take the change task before they look at state 8f84e18c38

The six places that wait on the pipeline signal (a download slot, a
staging reservation, a print slot, the realtime handshake, a queued
command, the update pause) took the signal's task at the end of the lock
block, after looking at the state. That is safe only for state that
changes under the same lock. The signal the runtime had before ChangeSignal
was safe for any state changed before the raise, because the swap itself
was made under that lock, and ChangeSignal's own summary says to take the
task first. Each site now takes the task as the first statement of its
lock block, so a change raised after the look always completes the task
the waiter holds, whichever lock a later split puts the state under.

No behavior change, for these reasons:

- Taking the task has no effect of its own: it is an atomic read of the
  current source and of its task. On the paths that return, break or throw
  the task is simply not used.
- The state is read in the same lock block, in the same order, so every
  decision is the one made before.
- The task taken can differ from the one taken before only when another
  thread raises a change between the first statement of the block and the
  place the task used to be taken. The state change behind that raise was
  made under the state lock before this block began, so the waiter has
  seen it. It used to hold the fresh task and wait for the next change; it
  now holds the task that raise completed, so its wait returns at once and
  it makes one more pass of its loop, sees the same state under the lock,
  takes the fresh task and waits as before. A pass that cannot proceed
  changes nothing (the only write in a condition, adding the printer to
  the printing set, happens on the path that proceeds), and every loop
  already tolerates a wake-up that brings nothing.
- With a cancelled token the outcome is the same one pass later: a wait
  on a completed task returns, and the next wait, on a pending task,
  throws the cancellation.

So the only possible difference is one extra pass through a loop, never a
missed wake-up and never a different decision. The whole runtime suite
passes: 699 tests.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

fix(log): redact cookie headers and a callback's code and state 22363b399e

This one changes what a log line shows, so it is declared in CHANGELOG.md
under Unreleased. AgentActivityLog.Redact, which every line of the service
and the companion passes through, did not recognise two shapes:

- a `Cookie:` or `Set-Cookie:` header line. Its value is now replaced by
  [REDACTED] to the end of that line; the header's name stays.
- the one-time code and the state of a sign-in callback. The values of
  `code` and `state` in the query or fragment of an address (straight
  after `?`, `&` or `#`) are now replaced by [REDACTED]; the names stay.

`code` is deliberately not a keyword on its own, nor is `state`: they are
ordinary words in a log, and "status code: 401", "exit code=1" and
"state=Paused" must stay readable. Only a parameter of an address counts.

Neither value reaches a log line today: the companion completes the
browser sign-in and hands the service a session credential, and the
session client logs an address without its query and says only whether a
cookie was attached. This is defense in depth.

Tests (ServiceLogTests):
- nine lines with a cookie header or callback parameters, each held to
  its exact redacted text in the ring, the sink and the file, including a
  header in the middle of several lines, a quoted address, a fragment and
  parameters whose names only resemble code or state (`codec`,
  `stateless`, `session_state`), which are left alone;
- twelve ordinary lines about status codes, exit codes, states and
  cookies, each written exactly as given;
- seven lines with neither new pattern, each held to the redacted text
  the method gave before this change (checked by running them against
  the method as it was);
- six more cases in the existing secrets test.

WhatTheRedactionDoesNotRecognise_IsTheSameAsBefore recorded the two old
gaps. It is now WhatTheRedactionStillDoesNotRecognise_IsRecorded and
records what is true after this change: a code or state that is not a
parameter of an address (bare, or as JSON members), a PKCE verifier, a
session cookie outside a cookie header line (a bare pair, a JSON member)
and a Basic credential pass through unchanged.

The comparison with the transcription of the runtime's old log method
needed no edit: the transcription calls the shared Redact, so it follows
the two new patterns, and none of its lines holds either. Its summary now
says so.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Signed-off-by: James Maes <james@kof22.com>

james.maes merged commit c3efed46cc into release/2.0

2026-11-11 00:37:03 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QRun-IO/qqq-print-agent#38