CLI keeps the api_content sidecar when a turn was persisted early — prompt cache survives (#102194, salvage #102411)

[kshitijk4poor]

The CLI no longer loses the exact bytes it sent to the provider when a user turn is persisted before the prologue composes them — the next turn replays the same request prefix and the prompt cache stays warm.

Salvage of #102411 by @JoaoMarcos44 (cherry-picked, authorship preserved), for #102194. Sibling carriers #103565 (@itsflownium) and #103721 (@salch-cred) implement the same row-addressed design; the persist-lock commit here is the same shape as @salch-cred's follow-up on #103721 and is co-credited.

Root cause

_stamp_api_content_sidecar stamps the wire bytes (<memory-context>, plugin context) onto the live user dict as api_content; the turn-start persist normally writes them with the row. When another writer already materialised that row — a close/early flush on the CLI path, or in-place preflight compaction — the persist marker-skips the message and the row keeps api_content = NULL. Replay then sends clean content and the prefix diverges at that message.

Changes

• hermes_state_messages.py: SessionDB.set_message_api_content(session_id, row_id, content, api_content) — row-addressed UPDATE guarded on role='user', active=1, content IS ?. The positional set_latest_user_api_content gets a docstring warning: repeated identical turns ("ok", "y") make an older row compare equal, so it is only safe when the caller knows the newest row is its own.
• agent/turn_context.py: backfill by _row_id when the live dict carries one (both early writers stamp it); positional fallback only for in-place compaction; the API-only pre-flushed turn (voice prefix / model-switch note, no injection) now stamps the live bytes too.
• Commit 2 (ours): the _row_id read and the backfill run under _session_persist_lock, re-checking after acquiring — a close flush holding that lock could commit the row and only afterwards write _row_id back, so an unlocked read saw no id and skipped forever.
• Commit 3 (ours): the durable-row rule (persist override = clean transcript, live content = wire bytes) is one helper, durable_user_row_content, used by both _db_flush_row and the stamp; the lock uses the existing _persist_lock; tests trimmed from 18 to 7 invariants.

Validation

Check	Result
tests/agent/test_api_content_row_addressed_backfill.py + test_api_content_sidecar.py + test_turn_context.py	54 passed
Real-import probe: older identical "ok" row + pre-flushed current "ok" row + plugin injection	main: row api_content=None; branch: row carries the sent bytes, older row untouched
Mutation: swap agent/turn_context.py for main's copy	3 red / 4 green in the new file
Lock-wrap: _session_persist_lock is an RLock; nothing on the stamp path holds it	deadlock-free by construction
ruff, windows-footguns, compat pointers, git diff --check	clean

Closes #102194. Supersedes #102411, #103565, #103721 (credited above).

[Enough1122]

AI code review — automated review for reference; please use your judgment.

PR 105842 — CLI keeps the api_content sidecar when a turn was persisted early

Well-reasoned race fix (#102194): when another writer materializes the turn's user row before the sidecar exists (in-place preflight compaction or a close/early flush racing the prologue), the stamp is now backfilled onto the exact durable row instead of being lost to the crash-persist marker-skip. The docstrings show the failure modes were thought through, not just patched.

Strengths (verified in diff)

• Row-addressed backfill (set_message_api_content in hermes_state_messages.py) with triple guards: explicit row_id, active = 1, and content match. Cannot drift onto a neighbour row.
• The identical-turns hazard ("ok"/"y"/"continue" matching an older row) is explicitly documented, and the positional set_latest_user_api_content path is deliberately narrowed to the compaction case where the caller knows the newest row IS the stamped message. Good.
• row_id validation rejects bools (isinstance(row_id, bool)) and non-positive ints — the True == 1 footgun is covered.
• Shared durable_user_row_content helper (agent/session_persistence.py) guarantees the flush and the stamp agree on override-vs-live bytes; the previous inline duplication is removed, not copied.
• The TOCTOU on _row_id (stamp reads outside the lock while the flush commits + marks under it) is fixed by reading under _persist_lock. The comment explains the interleaving precisely.

Findings (all Non-blocking)

1. Non-blocking — _stamp_api_content_sidecar now acquires _persist_lock(agent) while performing a DB write. If any caller in the prologue path already holds that lock and it is a non-reentrant Lock, this deadlocks. Please confirm the lock is reentrant (RLock) or that no caller holds it at this point; a brief assertion or comment at the call site would lock the invariant in.
2. Non-blocking — cross-module import of a private name (from agent.session_persistence import _persist_lock). Works, but consider exporting a small public helper (e.g. persist_locked(agent)) so the underscore boundary doesn't rot as the two modules evolve.
3. Non-blocking — on the early-return path (_db is None or no row id and not compacted) the live dict keeps api_content while the DB row has NULL. That in-memory/DB skew is pre-existing behavior, but a debug-level log on the skip would make future divergence hunts easier.

No blocking issues.

[andrexibiza]

Reviewed 98ed9e58a7275eaaf2cbb8c3e3303b2e4f1f3aa4, including the four-file diff, discussion, flush/marker synchronization, compaction writer, and sidecar replay. The comparison for the existing compaction backfill is main b1f003e18633298d549668b8e186af84cca45b76.

One P2 correctness finding remains, attached inline: the clean-content projection is correct for _db_flush_row, but not for the in-place compaction writer. With a surviving API-only user variant plus injected context, this change makes the content guard reject the very row compaction just inserted. The live request contains the injection, SQLite retains api_content = NULL, and reload loses those bytes. Keep the row-addressed update and the persist-lock fix; correct the writer/guard representation mismatch rather than weakening the guard or falling back to a neighbouring row.

Verification

I ran an isolated real-SQLite/threading probe using the fetched stamp/helper logic and UPDATE statements, with minimal storage scaffolding and ASCII inputs. This was not a repository checkout or a full SessionDB/pytest run; GitHub DNS resolution failed in the execution environment. I am not claiming the PR body's 54-test result as independently reproduced.

Observed controls and counterexample:

• Early flush with plugin injection, API-only override without injection, and override plus injection all retained the sidecar. An unpersisted repeated ok did not change its predecessor; eight invalid-ID/session/content/archive guard cases refused the write.
• A deterministic pause after the close-flush INSERT but before marker/row-ID publication retained the sidecar with the lock. Removing the lock in that probe reproduced the lost sidecar.
• Compaction without an override retained the injection. Compaction with live [voice] hello, clean override hello, and PLUGIN-CTX updated zero rows on this head. The same isolated case retained the sidecar with main's previous stamp logic. The no-row-ID compaction fallback also rejected the mismatched content.

Repository regression to add

This is the corresponding production-import test to add/run in the suite; that full-import execution was not available here:

def test_compacted_api_only_override_keeps_injection(tmp_path):
    import threading
    from types import SimpleNamespace
    from agent.context_compressor import stamp_db_persisted_markers
    from agent.turn_context import _stamp_api_content_sidecar
    from hermes_state import SessionDB

    db = SessionDB(db_path=tmp_path / "state.db")
    try:
        db.create_session("s1", source="cli")
        messages = [{"role": "user", "content": "[voice] hello"}]
        db.archive_and_compact("s1", messages)
        stamp_db_persisted_markers(messages)
        agent = SimpleNamespace(
            session_id="s1", _session_db=db,
            _session_persist_lock=threading.RLock(),
            _persist_user_message_override="hello",
            _last_compaction_in_place=True,
        )
        _stamp_api_content_sidecar(
            agent, messages, 0, "", "PLUGIN-CTX",
            preflight_compressed=True,
        )
        expected = "[voice] hello\n\nPLUGIN-CTX"
        assert messages[0]["api_content"] == expected
        assert db.get_messages("s1")[-1]["api_content"] == expected
    finally:
        db.close()

Also retain a deterministic overlapping-flush regression using a real RLock: the current new tests finish flushing before stamping, and their _FakeAgent base does not install _session_persist_lock, so they do not exercise the new synchronization boundary.

Landing state

At the final read, the PR is open/non-draft and GitHub reports mergeable=false. On the exact reviewed head, CI run 34246417134 is failure, while Docker 34246417251 and Nix 34246417247 are success. The failed CI run returns zero jobs; its underlying failure cause is not established by the available job data. This is not an all-green or merge-ready head.

The preserved JoaoMarcos44 implementation from #102411 and the kshitijk4poor/salch-cred lock work should remain intact while closing this combined-path regression. Recheck the corrected implementation and focused/adjacent tests after resolving the main integration, with fresh CI evidence bound to the resulting head.

[andrexibiza]

[P2] Match the representation written by compaction, not always the flush override

durable_user_row_content() computes the content that _db_flush_row would write, but an in-place preflight compaction does not go through that writer. _commit_compaction passes compressed directly to archive_and_compact; _message_row_params inserts msg['content'] without the agent's clean override.

For a surviving current user message with live content [voice] hello, _persist_user_message_override='hello', and plugin context PLUGIN-CTX, compaction therefore inserts [voice] hello and stamps its _row_id. This helper selects hello as durable_content, so the subsequent row-addressed UPDATE ... AND content IS ? affects 0 rows. api_content exists on the live dict, but remains NULL in SQLite; the compaction persistence marker then makes the ordinary flush skip it. A later reload drops the injection and breaks the request-prefix invariant this PR is repairing. The previous compaction backfill matched the live content and did not have this failure. The positional compaction fallback receives the same wrong guard value here as well.

The isolated SQLite counterexample and a production-import regression test are in the review body. Preserve row addressing and all SQL guards, but distinguish the compaction writer's committed representation from the ordinary flush projection, or make those writers genuinely share the same projection. Cover compaction + differing persist override + injection together, while retaining the passing clean early-flush cases. Do not repair the zero-row result by removing the content guard or trying an unrelated newest row.