You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A refusal Codex wrote could be thrown away — recorded nowhere, not merely missing from a turn's result. The read end of the worker's output is released when the session stops waiting for it, and closing it discards whatever the worker wrote that nobody has parsed. Writing a refusal to the ledger is allowed ten seconds and ran on the goroutine doing that parsing, so a refusal arriving while an earlier one was being written went with the pipe.
Every bound fails while the goroutine that reads the pipe also does slow work:
A clock runs out inside a ten-second ledger write, and the reader comes back to find its own pipe closed with the worker's next refusal still in it.
Waiting for the pipe to fall idle never runs out against a descendant outside the worker's group that keeps writing.
A count of bytes is both at once: too permissive on time, because a descendant can drip them out forever, and too strict on volume, because a worker inherits the pipe descriptor and may enlarge it beyond any size assumed for it. I tried this one on the way here and it was wrong in both directions.
Waiting and following are different questions, and neither could be answered while the reader was doing something other than reading.
What changes
The reader only reads. Two things came off it. The ledger write, which is slow and now belongs to a scribe. And a turn's ending, which is not parsing but waiting — for the policy check's verdict and for the worker to be gone so its stderr is whole. Those happen once, at the end of a turn, and for this driver the event that ends a turn is the last one a process sends.
What is left on the reader is parsing, appending and handing over. That is what makes the bounds honest: a drain budget that is a clock over waiting, and a queue cap that counts what it says.
The pipe belongs to the reader while it is reading. Termination asks it to stop — and asks before it waits on it, or a descendant holding the output would keep the descriptor open for as long as it lived. The deadline on the pipe has one owner. The descriptor is released when the reader says it is through. A cancel asks too.
What the split costs, and what it does not
Named in driver.go rather than left to be discovered.
A refusal is in memory between being read and being written, and a connector that crashes in that window loses it. That is the trade.
It survives everything else the contract names. The queue is drained before the session's updates close — where the dispatcher settles what the ledger would not take — so a worker that exits before its result, a turn cut short by a deadline, and a session closed under a turn all still find it written. The update for a refusal is emitted by the scribe, after its write lands, so the contract's own order holds exactly rather than being narrowed. And the recorder is still never called concurrently with itself: every write takes one lock, wherever it is made from.
The queue, and what it does when it fills
It blocks the reader before the stop, and never drops. That is backpressure onto a live worker — what a slow ledger already did to this driver when both ran on one goroutine.
After the stop it never blocks, because the drain's clock would run while the reader sat waiting for room. It grows to a cap, and at the cap the reader asks the worker to stop reading and goes on parsing what it already holds. It does not break out of the scan loop: the scanner holds a bufferful the pipe no longer does, and abandoning that would drop lines nobody would see again.
Two bounds that were missing, pre-existing and not caused by this change
A tool call id is the agent's, and the redactor takes things out of a string without making it shorter — so a refusal could keep a megabyte, on the turn, in the queue, and in what the session remembers having refused. And what it remembers had no bound at all, where the acp driver has maxRecorded for exactly this. Both are bounded now, and reaching the second ends the session rather than leave it unable to tell a repeat from a first.
What I tried and abandoned
A byte budget on the drain, described above. And an earlier form of this split that emitted the update at hand-off rather than after the write, which broke the contract's ordering; the scribe emits now, and the order is kept exactly.
Two claims I made in review were wrong and are corrected on the threads: that the cap "loses nothing" when it lost the scanner's buffer, and that a queued refusal keeps "a few hundred bytes" when the id was unbounded. I checked the second only after being challenged.
Evidence
The end-to-end case is the defect itself: a ledger held past every clock in this path, with a refusal sitting in the pipe the whole time. The second refusal is said only once the ledger is holding the first, so it is in the pipe and nowhere else. The fake fails loudly if that trigger never comes.
on main
here
a ledger held past the grace and the drain budget together
10 failures in 10, the refusal reaching neither the ledger nor the result
0 in 5
Every other property is proven by deleting the thing it exists for:
delete this
and this goes red
the absolute bound on following
a descendant dripping a byte per window cannot hold the reader — 3 of 3
the stop's non-destructiveness
a stop does not discard what is in the pipe — 5 of 5
the same
a stop does not touch the read deadline — 5 of 5
the update ordering
the update comes after the write — 5 of 5
the late path
a late refusal is still written — 5 of 5
the backpressure
a full queue holds a live reader back — 5 of 5
the no-waiting bypass
a stopped reader is never held back — 3 of 3
the drain on close
closing writes everything handed over — 5 of 5
the late-write barrier
close waits for a late write already begun — 5 of 5
the drain cap
a full drain tells the reader to stop — 5 of 5
the write lock
the recorder is never called twice at once — 5 of 5
the id bound
a refusal keeps a bounded id — 5 of 5
the remembered bound
a session remembers so many and no more — 5 of 5
the monotonic clock
the stop's clock survives the wall clock moving — 5 of 5
go build ./..., go vet ./..., gofmt -l ., golangci-lint run --build-tags dev ./... and BASECAMP_NO_KEYRING=1 go test -tags dev ./... are all clean, as are GOOS=windows and GOOS=darwin builds. go test -tags dev -race ./internal/connector/... ran 5 times with 0 failures.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The asynchronous handoff weakens refusal durability and does not reliably bound shutdown.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Separates Codex output reading from ledger writes to prevent refusals being discarded during shutdown.
Changes:
Adds reader-owned pipe draining and shutdown signaling.
Introduces a bounded refusal queue and scribe goroutine.
Adds regression coverage for slow ledger writes.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
File summaries
File
Description
internal/connector/driver/worker.go
Adds reader-controlled pipe lifetime and drain deadlines.
internal/connector/driver/worker_other.go
Adds platform stub methods for the reader lifecycle.
internal/connector/driver/codex/codex.go
Moves refusal recording to an asynchronous scribe.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The byte-based drain can block shutdown for hours and may truncate output from enlarged pipes.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
internal/connector/driver/codex/codex.go:338
The PR description still presents a reader/scribe split, bounded queue, and clock-based drain, but this revision intentionally keeps RecordRefusal synchronous on the reader and implements a byte budget with no scribe or queue. Please update the description and evidence so the architecture being reviewed matches this implementation.
// The worker's output is released when this session has read it, not on
// a clock: what the reader does between reads — writing a refusal to the
// ledger — is allowed to take longer than any such clock.
worker.ReadingDone(s.readerEnd)
A refusal Codex wrote could be thrown away: recorded nowhere, not merely
missing from a turn's result. The read end of the worker's output is
released when the session stops waiting for it, and closing it discards
whatever the worker wrote that nobody has parsed. Writing a refusal to
the ledger is allowed ten seconds and ran on the goroutine doing that
parsing, so there was no bound that could be put on the reading. A clock
runs out inside a write and takes the pipe away with the worker's next
refusal still in it. A bound that waits for the pipe to fall idle never
runs out at all against a descendant that keeps writing. A count of
bytes is both at once: too permissive on time, because a descendant can
drip them out forever, and too strict on volume, because a worker
inherits the pipe descriptor and may enlarge it beyond any size assumed
for it. I tried the byte count on the way here and it was wrong in both
directions.
They were never one bound. Waiting and following are different
questions, and the reason neither could be answered was that the reader
did the slow work itself.
So the reader only reads, and a scribe writes. A reader with nothing
arriving calls the pipe empty after a window of its own, which is how a
stop ordinarily ends. A reader that keeps being handed output follows it
only so long, which is the one case that has no other ending. The second
is absolute from the asking, and that is honest only because the reader
now does nothing slow between reads.
What the split costs is named in driver.go rather than left to be
found. A refusal is in memory between being read and being written, and
a connector that crashes there loses it. It survives everything else:
the queue is drained before the session's updates close, which is where
the dispatcher settles what the ledger would not take, and the update
for a refusal is emitted by the scribe after its write lands, so the
contract's own order — recorded before the update — holds exactly rather
than being narrowed.
The queue blocks the reader and never drops. Before the reader has been
asked to stop that is backpressure onto a live worker, which is what a
slow ledger already did to this driver when both ran on one goroutine.
After the asking it would be fatal, because the clock would run while
the reader sat on a full queue, so after the asking the reader never
waits and the queue grows instead — a burst at the end of a session,
bounded by what one drain can produce.
Held past every clock in this path, with a refusal sitting in the pipe
the whole time, this fails 10 runs of 10 on main, losing it from both
the ledger and the result. Eight more properties are held by tests that
each go red when the thing they exist for is deleted.
…ision
Four things the scribe had wrong, and the first is this pull request's own
defect one layer up.
A refusal handed over after the scribe had drained was written by whoever
read it, and counted with a WaitGroup beside the lifecycle rather than
inside it. So a close could Wait on a zero counter while another
goroutine was entering that branch and adding to it — a positive Add
racing a zero-count Wait, which the language forbids, and a close
deciding the ledger was whole while a write was starting. Admission and
completion are one decision under one lock now, and the barrier says what
it promises: every refusal handed over so far is written when close
returns, including the ones a caller wrote itself. It promises nothing
about one handed over after it returns; that is still written, by
whoever read it, and it is not waited for. Neither case is dropped.
The queue was bounded before the reader is asked to stop and not after
it. A drain bounded only by its clock is not bounded in memory: two
seconds of reading from a worker refusing as fast as it can write is as
much as the machine will take. There is a cap now, and at the cap the
reader stops reading — which loses nothing that was read and abandons
what was not, exactly as the clock was about to.
The drain's clock started when a read next noticed the flag, not when
the stop was asked for. A reader sitting in an idle window would not
notice for up to that whole window, and the budget would then run from
there — longer than the comment beside it promised. The time is taken at
the asking now, before the flag that publishes it, and a read's window
is never longer than what is left of the budget.
And the recorder's own documentation still said RecordRefusal is called
on the goroutine reading the agent's stream. It is not, in this driver,
which is the whole point. Both places that said so now say which
goroutine is the driver's choice and what an implementation may assume.
I swept the claim rather than the code this time; the dispatcher's
timeout comment said it too.
Each is held by a test that goes red when the thing it exists for is
deleted, including the clock, which is asked as the question it is —
whether the time is taken at the asking — rather than timed, because the
gap it guards only opens on a knife-edge and a test on one proves
nothing.
Raw IDs make the capped queue consume unbounded memory
internal/connector/driver/codex/codex.go:1076
The item cap does not bound queue memory because pending.update retains the raw, agent-controlled id here until emission. bufio.Scanner accepts a token up to 64 MiB, and Redactor.Sanitize does not impose a length limit, so a slow ledger can retain very large IDs across hundreds or thousands of entries. Bound the display ID before enqueueing and use that bounded value in both the refusal and update, while keeping the raw key only for deduplication if needed.
The new contract still says a worker that exits before its result has already had each refusal recorded, but Codex can call s.finish and return the prompt result before the deferred s.scribe.close drains a blocked ledger write. The slow-ledger test also waits for Close before inspecting the ledger, so it does not establish this result-time ordering. Either drain before settling the turn or remove the result-time guarantee and document the updates/session-close barrier consistently.
The reader was never the thin pump the bounds assumed. A turn ends by
waiting — for the policy check's verdict, and for the worker to be gone
so its stderr is whole — and both waits ran on the goroutine reading the
worker's output. So the drain's clock could run out entirely inside one,
and the reader would come back to a pipe it was no longer allowed to
read, with the worker's output still in it. That is this branch's own
defect, moved from the ledger write to the turn's ending.
Both waits are now taken off the read path. They happen once, at the end
of a turn, and for this driver the event that ends a turn is the last one
a process sends, so nothing is left unread by moving them. The reader's
own ending waits for them before it closes the scribe, because they read
the worker's last word too. What is left on the reader is parsing,
appending and handing over — so the drain budget is a clock over
waiting, and the queue's cap counts what it says.
Four more, all real, and the first two are guarantees this branch itself
wrote and then broke.
A late refusal is written on its reader's goroutine rather than the
scribe's, and two endings can make one each — so the recorder could be
called twice at once, against the promise in driver.go that it never is.
Every write takes the same lock now, wherever it is made from.
The queue's cap stopped the reader by breaking out of the scan loop,
which skips whatever the scanner had already taken from the pipe: lines
nobody would see again. It asks the worker to stop reading instead, and
goes on parsing what it holds. I said that cap "loses nothing" when it
lost things; it loses nothing now.
Terminate waited on a registered reader without asking it to stop, so a
descendant holding the output kept the descriptor — and everything
waiting on the reading — open for as long as it lived. It asks first.
And the stop's clock was kept as Unix nanoseconds and rebuilt with
time.Unix, which throws away Go's monotonic reading: a wall clock moving
forward ends a drain early with output still buffered, and moving back
holds a session past its budget. It is a time.Time now, kept whole.
Two bounds that were missing rather than broken, both pre-existing and
neither caused by this change. A tool call id is the agent's, and the
redactor takes things out of a string without making it shorter — so a
refusal could keep a megabyte, on the turn, in the queue and in what the
session remembers. And what it remembers had no bound at all, where the
acp driver has one for exactly this. Both are bounded, and reaching the
second ends the session rather than leave it unable to tell a repeat
from a first.
A canceled turn came back without the refusal Codex logged on its way
out — `TestACanceledTurnCarriesALateRefusalInItsResult`, on CI under the
race detector. It is not the test being slow: a turn can now be ended by
two goroutines, and both read the worker's last word, and only one of
them can be the first to take the turn out of the session. Whichever
read that stderr second recorded the refusal to the ledger and then
looked for a turn to put it on and found none, so the result the caller
got had nothing on it. Slowing everything down only widened the window.
A refusal read for a turn now goes on that turn. The endings say which
one they are settling, and only a refusal read outside one — from the
stream, mid-turn — looks the session up.
Two more from the same review, both mine and both introduced by this
branch.
Ending the worker asks its reader to stop, which starts the drain's
clock, and the reader could at that moment be waiting for room at the
scribe's queue — spending the clock without reading, and abandoning the
pipe with the worker's output still in it. Every ending of the worker
now frees the reader from waiting before it ends anything.
And the scribe was drained only when the reader tore down, so a turn
could hand its result back with its refusals still queued in memory —
the caller would ask the ledger and find nothing, which is the ordering
callers had when the write was on the reader. Every ending drains before
it settles. Endings run away from the reader, so that waits for nobody
who is reading.
The stop's flag was also sampled before the read deadline was installed,
so a stop landing between the two was hidden behind a whole idle window.
The read looks again rather than installing a window it already knows is
wrong.
… memory is recorded
Bounding what a session remembers a refusal by, I cut the key — which
makes a prefix the identity. Two tool call ids alike for longer than the
cut would collapse to one entry, and every later one would be taken for
a repeat: not recorded, not on the turn, not on the stream. A bound on
memory that loses refusals is the defect this branch exists to close,
written by the fix for a different one.
The key is a digest past that length now. Bounded, because the agent
writes it and a session's memory is not the agent's to grow, and still
distinct for ids that only begin alike.
And the check for that memory being full ran before the refusal that
filled it was taken, so that one was dropped on its way to ending the
session. It is recorded like any other now; the session ends after
taking it rather than instead of it, and records nothing after, because
past the bound a repeat cannot be told from a first and recording one
twice would be worse.
Both were found in review, and both are held by tests that go red when
the thing they exist for is deleted: identity by prefix loses the second
of two ids alike, 5 runs of 5, and checking the bound first loses the
refusal that reached it, 5 of 5.
The queued update still retains the raw agent-supplied id. Because a scanner token may be up to 64 MiB, a blocked recorder can leave many large IDs in the queue even though refusal.ToolCallID is capped, so the item-count limit does not bound memory. Queue the already sanitized, bounded refusal fields instead.
Retained prefixes keep oversized ID allocations alive
internal/connector/driver/codex/codex.go:1255
Returning s[:keep] limits the visible length but keeps the full backing allocation reachable. A near-64 MiB agent ID can therefore remain live through each supposedly bounded refusal or map key. Clone the retained prefix so the discarded suffix can be collected.
The reason will be displayed to describe this comment to others. Learn more.
🤖 Real, and not fixed — I am stopping this branch here rather than push a fix I cannot stand behind, so this is an honest open finding rather than an answered one.
You are right on the code: settled() went onto every ending except this one. The prompt's writer, on a write that failed without a cancel, still calls finish directly, so a result can go back with a refusal the scribe has not written — the ordering every other ending now keeps.
I wrote the one-line fix for it together with the cap one below, and the package deadlocked: the codex tests ran past ten minutes instead of thirty seconds. I have reverted both rather than leave a broken tree at a handoff. The fix is almost certainly right and the interaction is almost certainly in the other change, but "almost certainly" is not something to push.
Left for whoever picks this up, with that warning attached.
The reason will be displayed to describe this comment to others. Learn more.
🤖 Real, and not fixed. Same reason as the thread above: my attempt at it deadlocked the package and I reverted rather than push it.
You are right that it is not a hard cap. The item is appended before heard is set, every later handoff is appended too, and StopReading only starts the drain — so for up to a whole drain budget a fast descendant keeps being read while nowait holds the backpressure off. The cap marks a moment rather than bounding a size.
What I tried, so it is on the record: past the cap, stop appending and have the reader write what it reads itself. That is lossless — nothing is dropped, the reader is slowed to the ledger's pace, and its own clock then ends the reading sooner. It is the shape I would try again. Something in it deadlocks against the drain that every ending now does, and I did not have the room to find out which way round.
Worth saying plainly: with the cap as it stands the memory bound is a soft one, and the honest description of it is "a mark where the reader is told to stop", not a cap. I have not rewritten the comment to say that, because the branch is stopping on the previous commit.
maxToolCallID does not bound the payload retained by the scribe: this pending update still stores the original agent-supplied id. A JSON token can approach the scanner's 64 MiB limit, and emit sanitizes the ID without shortening it, so a slow ledger can retain large strings in every queued refusal despite the 256/4096 item limits. Build the queued update from the already bounded refusal fields instead.
The reason will be displayed to describe this comment to others. Learn more.
🤖 Real, and open. endWorker turns backpressure off before the worker is actually gone, so between those two the reader is free to pile refusals up while the worker is still writing — the no-waiting bypass exists for a drain and this lets it start before one.
Not fixed. I stopped this branch at the previous commit after a fix for two other findings deadlocked the package, and this belongs with them rather than alone: the reader stopping, the queue growing and the drain waiting are one lifecycle, and the review has now found the same seam from four sides in two rounds. Patching one door at a time is what has kept this open.
The reason will be displayed to describe this comment to others. Learn more.
🤖 This is the one that explains the deadlock I hit, and it is the most useful finding on the pull request. Real, and open.
drain waits for the queue to be globally empty rather than for the handoffs the caller is settling. While a reader goes on handing refusals in, it never is — so an ending that drains can wait for as long as the worker keeps refusing. That is what hung the codex package past ten minutes when I added the drain to the prompt writer's ending: the writer waited for a queue the reader was still filling.
So settled() as it stands is only safe on an ending where the reader has stopped, and every ending is not that. The fix is to make it wait for a bounded set — a sequence number taken at the call, or a count of what was handed over by then — rather than for the queue to fall empty. I did not write that; the branch stops on the previous commit.
Recording it here because it is the reason the other two findings are open rather than fixed, and because whoever picks this up should know the drain is the thing to change first.
jorgemanrubia
changed the title
A refusal could be thrown away while an earlier one was being written
WIP: A refusal could be thrown away while an earlier one was being written
Sep 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A refusal Codex wrote could be thrown away — recorded nowhere, not merely missing from a turn's result. The read end of the worker's output is released when the session stops waiting for it, and closing it discards whatever the worker wrote that nobody has parsed. Writing a refusal to the ledger is allowed ten seconds and ran on the goroutine doing that parsing, so a refusal arriving while an earlier one was being written went with the pipe.
Carded as The worker's output pipe is closed on a clock, and a refusal can be lost with it, after being found and taken back out of An unsafe session could report fewer refusals than it made.
Why there was no bound to add
Every bound fails while the goroutine that reads the pipe also does slow work:
Waiting and following are different questions, and neither could be answered while the reader was doing something other than reading.
What changes
The reader only reads. Two things came off it. The ledger write, which is slow and now belongs to a scribe. And a turn's ending, which is not parsing but waiting — for the policy check's verdict and for the worker to be gone so its stderr is whole. Those happen once, at the end of a turn, and for this driver the event that ends a turn is the last one a process sends.
What is left on the reader is parsing, appending and handing over. That is what makes the bounds honest: a drain budget that is a clock over waiting, and a queue cap that counts what it says.
The pipe belongs to the reader while it is reading. Termination asks it to stop — and asks before it waits on it, or a descendant holding the output would keep the descriptor open for as long as it lived. The deadline on the pipe has one owner. The descriptor is released when the reader says it is through. A cancel asks too.
What the split costs, and what it does not
Named in
driver.gorather than left to be discovered.A refusal is in memory between being read and being written, and a connector that crashes in that window loses it. That is the trade.
It survives everything else the contract names. The queue is drained before the session's updates close — where the dispatcher settles what the ledger would not take — so a worker that exits before its result, a turn cut short by a deadline, and a session closed under a turn all still find it written. The update for a refusal is emitted by the scribe, after its write lands, so the contract's own order holds exactly rather than being narrowed. And the recorder is still never called concurrently with itself: every write takes one lock, wherever it is made from.
The queue, and what it does when it fills
It blocks the reader before the stop, and never drops. That is backpressure onto a live worker — what a slow ledger already did to this driver when both ran on one goroutine.
After the stop it never blocks, because the drain's clock would run while the reader sat waiting for room. It grows to a cap, and at the cap the reader asks the worker to stop reading and goes on parsing what it already holds. It does not break out of the scan loop: the scanner holds a bufferful the pipe no longer does, and abandoning that would drop lines nobody would see again.
Two bounds that were missing, pre-existing and not caused by this change
A tool call id is the agent's, and the redactor takes things out of a string without making it shorter — so a refusal could keep a megabyte, on the turn, in the queue, and in what the session remembers having refused. And what it remembers had no bound at all, where the acp driver has
maxRecordedfor exactly this. Both are bounded now, and reaching the second ends the session rather than leave it unable to tell a repeat from a first.What I tried and abandoned
A byte budget on the drain, described above. And an earlier form of this split that emitted the update at hand-off rather than after the write, which broke the contract's ordering; the scribe emits now, and the order is kept exactly.
Two claims I made in review were wrong and are corrected on the threads: that the cap "loses nothing" when it lost the scanner's buffer, and that a queued refusal keeps "a few hundred bytes" when the id was unbounded. I checked the second only after being challenged.
Evidence
The end-to-end case is the defect itself: a ledger held past every clock in this path, with a refusal sitting in the pipe the whole time. The second refusal is said only once the ledger is holding the first, so it is in the pipe and nowhere else. The fake fails loudly if that trigger never comes.
mainEvery other property is proven by deleting the thing it exists for:
go build ./...,go vet ./...,gofmt -l .,golangci-lint run --build-tags dev ./...andBASECAMP_NO_KEYRING=1 go test -tags dev ./...are all clean, as areGOOS=windowsandGOOS=darwinbuilds.go test -tags dev -race ./internal/connector/...ran 5 times with 0 failures.