fix: a background record could be killed at exit; add flush() - #12
Merged
Conversation
record(background=True) sends on a daemon thread, and the interpreter does not
wait for daemons. A CLI, a serverless handler, or any script that ends soon after
its last turn could have that write killed mid-flight — no exception anywhere,
the turn simply never learned. It reads as unreliable memory rather than as a
missing call, which is the worst way for it to fail.
flush(timeout=None) waits for what is in flight and returns how many there were.
The timeout is a total budget, and running out returns rather than raises: a
record that did not land costs one turn of learning, and turning that into an
exception at exit would be worse than the thing it reports.
An atexit hook calls it anyway, bounded to five seconds, so a developer who never
read this docstring loses latency instead of the turn. Threads stay daemons on
purpose — a hung request must not be able to wedge the process shut.
Reported in an outside review of the SDK, which described the failure exactly.
The test that matters runs a real subprocess that exits immediately after a
background record and checks the write still landed. Without the hook:
AssertionError: the process exited before the background write landed
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
From the outside review, which described the failure exactly.
The bug
record(background=True)sends on a daemon thread, and the interpreter does not wait for daemons. A CLI, a serverless handler, or any script that ends soon after its last turn has that write killed mid-flight — no exception anywhere, the turn simply never learned.It reads as unreliable memory rather than as a missing call, which is the worst way for it to fail.
flush()The timeout is a total budget, and running out returns rather than raises: a record that did not land costs one turn of learning, and turning that into an exception at exit would be worse than the thing it reports.
An
atexithook calls it anyway, bounded to five seconds, so a developer who never read the docstring loses latency instead of the turn. Threads stay daemons on purpose — a hung request must not be able to wedge the process shut.The test that matters
A real subprocess that exits immediately after a background record, with no
flush()and nosleep, exactly as a CLI would. Without the hook:All three existing suites still pass.
0.3.1
AsyncKhwanalready had this —aclose()awaits pending tasks, since asyncio holds only a weak reference to a task. The sync client is now level with it.