Repository navigation
A corrupt transcript line aborts import_session_to_store mid-write, leaving a partial session #1334
Description
What happens. import_session_to_store() reads a session transcript with an unguarded json.loads(line), and it flushes to the store in batches as it goes. A single unparseable line — the normal state of a transcript whose writer was killed mid-append — aborts the import with a bare json.JSONDecodeError after earlier batches are already committed.
Reproduction. Six complete entries followed by a seventh cut off mid-object, written to a file, then imported with batch_size=2:
[truncated tail, batch_size=2] raised JSONDecodeError: Unterminated string starting at: line 1 column 33 (char 32)
[truncated tail, batch_size=2] store now holds 6 entries: ['u0', 'u1', 'u2', 'u3', 'u4', 'u5']
The store is now a session that query(resume=...) will happily open, whose transcript is missing its last message, with nothing recording that it is a partial import. The exception names neither the file nor the line number within it, and the docstring's Raises: section does not mention it.
With the default batch_size (500) the same file stores nothing, because the first flush has not happened yet — so whether a partial session survives depends on a batching parameter the caller may not be thinking about.
Why I think this is a gap rather than a choice. Three other JSONL readers in this package skip corrupt lines, and the fourth raises on purpose:
sessions.py:917-920—except (json.JSONDecodeError, ValueError): continue, documented as "Skips corrupt lines".session_mutations.py:605-608— the samecontinueinfork_session.subprocess_cli.py:204-206— raisesCLIJSONDecodeErrorrather than "silently dropping a message", i.e. a deliberate domain error, not a barejsonone.session_import.py:138—batch.append(json.loads(line)), unguarded.
The strongest hint is inside the same function: the sidecar read a few lines below is explicitly hardened, with a comment saying "A missing, corrupt, or non-object sidecar is treated as absent (the transcript is still imported)" and a test named test_unusable_meta_json_sidecar_is_treated_as_absent whose docstring reads "A corrupt or non-object sidecar must not abort the import midway". That is the same hazard, the same import, and the author already decided how to treat it — for the sidecar. The transcript lines were left on the old behaviour.
What I am not claiming. I have not measured a real CLI leaving a truncated transcript; I constructed one. And the function's docstring already tells adapters to treat entry["uuid"] as an idempotency key "so re-import is duplicate-safe", which means a caller that retries is fine — the exposure is the caller that does not, or that gives up and leaves the half-session behind.
Options, if you want this changed. Either seems defensible and I do not have a strong preference:
- Skip unparseable lines the way the other three readers do, and count them so the caller can be told how many were dropped.
- Keep failing, but wrap it in a domain error naming the file and line, and make the append atomic so a failed import leaves nothing behind — the shape PR fix: atomically write forked session transcripts #960 took for
fork_sessionwriting a transcript.
Whichever you pick, I would rather not have the four readers disagree with each other.
I have not implemented either. Say which you want and I will open a PR.
Activity