Skip to content

fix(queue): corruption-free durable ingest under non-atomic shared storage (#473) - #92

Merged
Salil Das (sadlilas) merged 3 commits into
mainfrom
feat/durable-queue-hardening
Sep 1, 2026
Merged

fix(queue): corruption-free durable ingest under non-atomic shared storage (#473)#92
Salil Das (sadlilas) merged 3 commits into
mainfrom
feat/durable-queue-hardening

Conversation

@colombod

@colombod Diego Colombo (colombod) commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Fixes the silent tail-drop where large events were dropped from a session's ingest log under concurrent writes. Ships as 6.7.1 so the fix is visible on /version.

Problem

Events are appended to per-session on-disk logs on a mounted volume (Azure Files in cloud, local mount in dev), then drained into Neo4j. The append was not serialized: open(path, "ab").write(line) relied on O_APPEND atomicity, and a large record is written in several write() calls. Two concurrent appends to the same session could therefore interleave, producing one line that is a fragment of one record concatenated with another. That line fails to parse, and the parse failure killed the session's unsupervised drain task silently — so every event after it was never persisted. POST /events returns 202 before any of this, so the client saw 100% success with nothing logged anywhere.

Fix

  • Serialized append — one threading.Lock per worker key, held on the writing thread for the whole record, so two writes to a key's file can never interleave. O_APPEND is retained as defence in depth only; correctness no longer depends on filesystem write atomicity.
  • Complete writes_write_all loops over short writes. A record lands as exactly one newline-terminated byte range, or the fragment is terminated and dead-lettered. Bytes are never removed from a queue file.
  • Supervised drain — a failed dead-letter or commit can no longer kill a drainer silently. _on_drain_done logs, closes and deregisters so a respawn or boot recover() picks the session back up.
  • Queue-owned offsetsread_batch returns Records carrying their own start/end, so the registry never computes byte positions itself.

Behaviour change worth knowing

session:end is now dispatched twice per session. The terminal record is deliberately left uncommitted so that "ended but not finalized" survives a respawn; _finalize_session then re-reads and re-dispatches it. All three handlers on the terminal path are read-then-MERGE and tolerate this — any new terminal handler must be idempotent. Documented at SessionRegistry._process_batch and SessionHandler._handle_end.

Relatedly, SessionHandler._handle_end no longer flushes on its own: the drainer's _flush_barrier is the single write boundary and the only thing holding the Neo4j write semaphore.

Validation

  • Corruption proof: tests/test_durable_append_framing.py models a non-atomic filesystem by splitting each os.write into short writes, and includes an explicit non-vacuity control (test_smb_shim_tears_WITHOUT_the_gate_control) proving that the same shim does merge records once the guard is removed.
  • Full -m "not neo4j": 1930 passed, 8 skipped.
  • -m neo4j (live container): 83 passed.

Deliberately not in this PR

  • Boot-time torn-tail truncation. It truncated every queue file at startup, which is safe only if exactly one process writes the volume — and Container Apps starts a new revision before stopping the old one, so it could remove records the outgoing revision was still writing. The per-key lock plus _discard_partial plus dead-lettering already handle torn fragments without removing bytes.
  • A substitute workspace for unrecoverable sessions. workspace is the graph partition key (MERGE on {node_id, workspace}), so dispatching under a placeholder would write a whole session into a partition it does not belong to and drop its contributor. Recovery now skips such a session and leaves the data durable on disk for an operator.
  • A dual-format .offset reader. No deployed build writes that format.
  • Lock-serialized .offset writes. Not needed: one drainer owns a key, and commit is a tmp-write plus os.replace.

🤖 Generated with Amplifier

@colombod
Diego Colombo (colombod) changed the base branch from fix/neo4j-bounded-shared-driver to main August 27, 2026 09:14
@colombod Diego Colombo (colombod) changed the title feat(queue): corruption-free, self-bounding durable ingest queue fix(queue): corruption-free, self-bounding durable ingest queue Aug 27, 2026
@colombod Diego Colombo (colombod) changed the title fix(queue): corruption-free, self-bounding durable ingest queue fix(queue): corruption-free durable ingest under non-atomic shared storage (#473) Aug 27, 2026
…orage (#473)

Large-payload events were silently dropped because concurrent,
non-atomic writes on cloud/SMB storage could corrupt a session's
on-disk queue (.log/.offset/.dead.jsonl). This fixes it with:

- A per-session lock (_KeyGuard) serializing every write to a session's
  queue files, so a record lands whole-or-not-at-all regardless of
  filesystem write atomicity.
- Whole-record framing (Record/Batch.records) with queue-produced,
  opaque start/end cursors -- callers no longer recompute byte offsets
  from raw payload length, closing a class of off-by-one corruption.
- heal_torn_tails(): a boot-time pass that truncates any session's
  queue files back to their last complete record, quarantining the
  torn bytes to a sidecar rather than losing or corrupting them.
- A dual-format offset reader (_read_committed_offset) so a session
  written by a prior/older version still drains correctly.
- delete_drained() now refuses to remove a session's queue files while
  bytes remain uncommitted (a late-arriving append raced with
  finalize), instead of unconditionally deleting; the caller
  (_finalize_session) retries a bounded number of times, re-draining
  each attempt, and gives up cleanly (leaving a recoverable log) if the
  race persists.
- Crash recovery gains a byte-0 fallback and last-resort sentinel
  workspace so a torn/corrupted head at the committed offset never
  strands the still-durable data behind it.

Scope note: this PR is intentionally limited to the #473 corruption
fix. Several adjacent hardening features that were bundled with it in
earlier iterations -- log compaction, boot-time reclaim/GC of
unresumable sessions, dead-letter retention/expiry, exactly-once
ingest idempotency, and drain-supervision/boot-phase orchestration --
have been parked (preserved in full on branch
parking/queue-hardening-full) for separate review as their own
follow-up PRs.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Salil Das (sadlilas) added a commit that referenced this pull request Sep 1, 2026
Remove three unsafe or unjustified additions from PR #92:

1. Removed heal_torn_tails / _heal_one (queue_manager.py) and lifespan call
   (main.py). Truncating queue files at boot is only safe with a single
   writer; Azure Container Apps overlaps revisions, creating a race where
   the outgoing revision loses records still being written. The per-session
   lock + _discard_partial + dead-lettering already handle torn fragments
   safely.

2. Removed _RECOVERY_FALLBACK_WORKSPACE sentinel and byte-0 fallback in
   crash recovery (_recover_one_session, _crash_recovery_topup in main.py).
   workspace is the Neo4j partition key; dispatching under a substitute
   workspace writes to the wrong partition and drops the contributor.
   Recovery now skips unparseable sessions, leaving data durable on disk.
   Kept _parse_workspace_and_creator (genuine robustness improvement).

3. Removed dual-format legacy JSON .offset reader (no deployment ever
   wrote this; exists only on an unmerged branch).

Added documentation: session:end is dispatched twice per session
(consequence of terminal-commit redesign: terminal record uncommitted,
re-read during finalization). Restored load-bearing note in _flush_barrier
on commit-after-flush and neo4j_store._flush_body buffer restoration.

Fixed 2 unused imports (ruff F401) in test_large_event_tail_drop.py.

Tests: baseline 1935 passed → 1930 passed (removed 5 tests for deleted
features). ruff check and format clean on all touched files.

Fixes: #92

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Remove three unsafe or unjustified additions from PR #92:

1. Removed heal_torn_tails / _heal_one (queue_manager.py) and lifespan call
   (main.py). Truncating queue files at boot is only safe with a single
   writer; Azure Container Apps overlaps revisions, creating a race where
   the outgoing revision loses records still being written. The per-session
   lock + _discard_partial + dead-lettering already handle torn fragments
   safely.

2. Removed _RECOVERY_FALLBACK_WORKSPACE sentinel and byte-0 fallback in
   crash recovery (_recover_one_session, _crash_recovery_topup in main.py).
   workspace is the Neo4j partition key; dispatching under a substitute
   workspace writes to the wrong partition and drops the contributor.
   Recovery now skips unparseable sessions, leaving data durable on disk.
   Kept _parse_workspace_and_creator (genuine robustness improvement).

3. Removed dual-format legacy JSON .offset reader (no deployment ever
   wrote this; exists only on an unmerged branch).

Added documentation: session:end is dispatched twice per session
(consequence of terminal-commit redesign: terminal record uncommitted,
re-read during finalization). Restored load-bearing note in _flush_barrier
on commit-after-flush and neo4j_store._flush_body buffer restoration.

Fixed 2 unused imports (ruff F401) in test_large_event_tail_drop.py.

Tests: baseline 1935 passed → 1930 passed (removed 5 tests for deleted
features). ruff check and format clean on all touched files.

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Two docstrings in tests became stale after a previous commit removed:
- The legacy-offset reader in _read_committed_offset
- The recovery-under-fallback fallback path

Updated docstring in test_queue_manager.py to accurately describe
_read_committed_offset behavior (bare-int only). Updated docstring in
test_lifespan_skips_recovery_for_empty_workspace to match its assertion
(session skipped, not dispatched under fallback).

Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@sadlilas
Salil Das (sadlilas) merged commit 23d013f into main Sep 1, 2026
3 checks passed
@sadlilas
Salil Das (sadlilas) deleted the feat/durable-queue-hardening branch September 1, 2026 13:43
Salil Das (sadlilas) added a commit that referenced this pull request Sep 1, 2026
…ibution

PR #92 rewrote the drain loop to return Records with their own start/end cursors,
moving the terminal-check logic into _process_batch returning (safe_count, terminal_at)
and dispatching session:end twice deliberately.

This branch threads session working directory attribution through that same rewritten
code. Conflict resolution strategy: accepted main's entire drain/terminal logic, then
re-applied working_dir threading on top by:

- _parse_line now returns working_dir as a fourth element
- _process_batch and _handle_exhausted_batch forward it to _process_one via rec.raw
- Updated five test files that stub process_event with fixed positional signatures
  to accept the new keyword argument
- Updated one direct _parse_line call site to unpack four values

All 2037 tests pass (7 skipped), ruff clean, mutation checks correct.
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.

2 participants