Skip to content

fix: a background record could be killed at exit; add flush() - #12

Merged
hugo8xx merged 1 commit into
mainfrom
fix/flush-background-records
Aug 25, 2026
Merged

fix: a background record could be killed at exit; add flush()#12
hugo8xx merged 1 commit into
mainfrom
fix/flush-background-records

Conversation

@hugo8xx

@hugo8xx hugo8xx commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

From the outside review, which described the failure exactly.

The bug

threading.Thread(target=_send, daemon=True, name="khwan-record").start()

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()

kw.record(turn, answer, background=True)
kw.flush()          # → how many were in flight

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 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 no sleep, exactly as a CLI would. Without the hook:

AssertionError: the process exited before the background write landed —
the failure flush() exists to prevent
✓ flush: returns only once the in-flight write has landed
✓ flush: a hung write is given up on, not raised and not waited out
✓ flush: nothing in flight → nothing to wait for
✓ exit: a script that ends immediately still lands its last write

All three existing suites still pass.

0.3.1

AsyncKhwan already had this — aclose() awaits pending tasks, since asyncio holds only a weak reference to a task. The sync client is now level with it.

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
@hugo8xx
hugo8xx merged commit 5662087 into main Aug 25, 2026
2 checks passed
@hugo8xx hugo8xx mentioned this pull request Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant