fix: /status off the spool walk, and trim drained queue files as they are processed - #101
Merged
Merged
Conversation
… are processed (1) /status was calling spool_stats() inline, which scanned the entire queue directory — stat() per file plus open+read of every .log's .offset. On a ~5000-file SMB share this caused thousands of round trips per request, with only a 5s cache TTL to spare subsequent callers. Measured today: /version returns in 0s, /status times out at 180s on the same replica. Fix: spool_stats() is now synchronous cache-only (never touches filesystem, never raises). New refresh_spool_stats() performs the scan; new _spool_stats_refresher background task runs it on its own cadence (default 60.0s), following the create_task/cancel-in-finally pattern established in PR #99. A failed refresh is logged; the loop continues. /status's spool block now includes as_of_seconds to state staleness explicitly. Result: /status is O(1) regardless of spool size. (2) commit() writes only .offset; .log files were never trimmed. Only delete_drained() (called solely from _finalize_session) reclaimed space. A session never finalized cleanly kept files forever; every boot re-walked them. The drain loop's idle branch now also calls delete_drained() — the same safe, idempotent, file_lock-guarded call finalization already makes, just earlier and repeatedly. Logic is conservative: refuses while uncommitted bytes remain, keeps .dead.jsonl, anticipates log recreation. Dead-letter files are deliberately NOT auto-purged (they are the failure record). Version: 6.7.4 -> 6.7.5 Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Add two complementary reclamation paths to recover unbounded spool state: 1. QueueManager.reclaim_drained_orphans() — called from _startup_recovery_body right after the respawn loop. Iterates every worker key and delegates to delete_drained (idempotent, safe, refuses while uncommitted bytes remain, keeps .dead.jsonl, unlinks stale .offset). A key whose drainer is mid-drain is skipped by delete_drained itself — no exclusion list, no race risk. O(keys), not O(bytes). Wrapped in try/except so disk reclaim never fails boot. 2. Raise spool_trimmed from DEBUG to INFO. Per-session trim already ran but was invisible at INFO level on live servers. Live-deploy evidence: BEFORE 25 .log files totaling 1.43 GiB (including a 691 MB and 600 MB file). Boot logged 'startup_recovery: reclaimed 25 fully-drained session file(s)'. AFTER 1 .log file / 796 bytes. recovery_complete True, recovery_error None, zero ERROR/Traceback/dead_letter/flush_chunk_failed lines. 1.43 GiB reclaimed in one boot. Verification: - pytest -m 'not neo4j': 2008 passed, 7 skipped - pytest -m neo4j: 92 passed - 9 new tests, ALL NINE verified RED on prior commit - ruff format --check: clean - Version 6.7.5 (unchanged from prior commit) Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Diego Colombo (colombod)
added a commit
that referenced
this pull request
Sep 10, 2026
PR #101 claimed '/status is now O(1) regardless of spool size', but the claim was false. The fix moved spool_stats() off the request path but missed a second unbounded scan reached via Registry.pipeline_metrics() -> QueueManager.derive_all_stats(). This path walked every worker key inline, opening and reading each .offset file and .dead.jsonl tail on each request, gated only by a 1-second READ-SIDE TTL that spared the second caller but never the first. On a healthy replica with 5000+ worker keys, /status timed out (60s/120s/150s+) while other endpoints responded normally. This fix mirrors the split already applied to spool_stats: - refresh_all_stats() — async, performs the full scan, populates cache, no TTL gate, the only operation that touches disk for these metrics - derive_all_stats() — now SYNCHRONOUS and cache-only; never touches filesystem, never raises - all_stats_age_seconds() — reports snapshot age (None before first refresh) - _spool_stats_refresher now drives BOTH refreshes each cycle Cold-window correctness: before the first refresh, in_queue and dead stats are UNKNOWN (not zero), so naive reads would fabricate loss. The snapshot carries stats_available: False, and pipeline_metrics suppresses rather than invents: residual is None when unavailable, degraded is only computed when stats are available, and _residual_positive_since is not latched on unknown data. /status now reports metrics.stats_available and metrics.as_of_seconds for full visibility. SECONDARY: test_serve_still_fails_loud_for_a_real_misconfiguration flaked on main (15s TimeoutExpired). Not a guard regression — the test passed twice on branch and fires locally in 0.74s. On the runner, the subprocess stalled during gunicorn startup, never reaching worker boot. Subprocess budget raised to 90s (zero cost on healthy runs) and timeout now raises AssertionError carrying partial stdout/stderr and naming it as wall-clock timeout, not proof of guard failure. Version: 6.7.5 -> 6.7.6 Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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.
Two problems, one root: unbounded spool work on a hot path
PR #99 stopped crash recovery blocking the HTTP surface at startup. This finishes the job for
the request path, and stops the spool growing in the first place.
1.
/statuswalks the entire queue directory, per requestMeasured on team-shared today, on a healthy replica running #99:
/statuscallsspool_stats(), which scanned inline (queue_manager.py):~5000
stat()calls plus ~1700 file opens over Azure Files SMB, on the request. The existing_spool_cache_ttl = 5.0only helped the second caller — the first caller after every expiry paidthe full walk.
So the health endpoint's cost scaled with the backlog, and became unusable at exactly the moment you
needed to look at it. Same inversion as the outage itself, one layer up.
2. Queue files are never trimmed as they're processed
commit()writes only the.offset; the.logis untouched. The only reclaim isdelete_drained(), called from exactly one place —_finalize_session. So a session that neverfinalizes cleanly (crash, orphan, indefinite idle) keeps its
.log/.offsetforever, and everysubsequent boot's recovery re-walks them.
That is the ratchet behind today's incident: the more the server failed to start, the more files it
left behind, the slower the next start. Local evidence on a developer box:
spool_bytes_total1.69 GiB with
pending_sessions: 1— nearly all of it drained and never reclaimed.Fix
/statusoff the spool walkspool_stats()is now synchronous and cache-only — it never touches the filesystem and neverraises. Returns the last snapshot, or the existing
{-1,-1}"temporarily unavailable" sentinel.refresh_spool_stats()performs the scan (the previous body, verbatim, including itsfault-isolation) and updates the cache.
_spool_stats_refresher(app)background task inmain.pyruns the refresh on its owncadence, following the exact
create_task/ cancel-in-finallypattern fix: crash recovery must not block the HTTP surface at startup #99 established for_startup_recovery. It is fail-loud but self-healing: a failed refresh is logged vialogger.exceptionand the loop continues, so the snapshot goes stale rather than the refresherdying silently.
spool_stats_refresh_interval_seconds(default60.0, validated> 0)./status'sspoolblock gainsas_of_seconds— how old the snapshot is,Nonebefore thefirst refresh. Staleness is stated, never presented as live.
_spool_cache_ttlread-gate is removed; the refresher owns the cadence./statusis now O(1) regardless of spool size.Trim as processing proceeds
In the drain loop's existing idle branch (the
idle_elapsed >= flush_timeoutblock that alreadyholds the stale-session reap check), after that check and without altering it, attempt reclaim:
This is the same call
_finalize_sessionalready makes — just triggered earlier andrepeatedly. It is safe by construction:
delete_drainedis idempotent, takes the key'sfile_lock, refuses cheaply (returnsFalse) while any uncommitted bytes remain, keeps.dead.jsonl, and explicitly anticipates recreation — it unlinks a stale.offsetso a logrecreated by a later append cannot read past its own end.
Conservation accounting is preserved. I checked the arithmetic in
recovery_seed_counts: for atrimmed key whose
.dead.jsonlsurvives,committed→0,before→0,pending→0,dead→D, sowritten_seed = max(0, 0-D) = 0,accepted += D,written += 0→ residualD - 0 - 0 - D = 0.Clean.
queue_manager.pyalready documents this case ("a.logdeleted bydelete_drainedhas nolog to reconcile").
Tests
test_spool_stats_is_synchronous_read_only_and_never_scansspool_stats()performs no filesystem access even withiterdirpatched to raisetest_spool_stats_age_seconds_none_before_any_refresh/..._reflects_time_since_refreshtest_status_spool_as_of_seconds_none_before_any_refresh/statusbefore the first refresh returns the sentinel, not a scantest_status_spool_block_reflects_real_backlog_after_refreshtest_spool_stats_refresher_survives_a_failing_refreshtest_spool_stats_refresher_task_is_cancelled_on_shutdownTestDurableSpoolTrim::test_idle_drained_session_trims_log_and_offset_keeps_dead_letters.dead.jsonlsurvivesTestDurableSpoolTrim::test_idle_uncommitted_bytes_are_never_trimmedtest_recovery_seed_counts_residual_zero_after_trimming_dead_letter_sessionPlus the pre-existing
spool_statstests updated to callrefresh_spool_stats()where theyexercise the scan.
24 tests verified RED against
main(source reverted, tests kept).Results:
pytest -m "not neo4j"→ 1999 passed, 7 skipped, 92 deselected.pytest -m neo4j(live containers) → 92 passed.ruff format --checkclean on all 7 files.Version bumped 6.7.4 → 6.7.5.
Deliberately NOT in this PR
Dead-letter files are not auto-purged. The request mentioned trimming dead letters too, and I
have not done it. They are the record of what failed — and after the
derive_labelsincident(#100) they are currently the only trace of the events that were destroyed. Auto-deletion would
destroy evidence on a timer.
purge_dead_letters()stays operator-invoked. If retention is wanted,it should be age-based and off by default — a design decision, not a patch.
Boot-time recounting is untouched.
recovery_seed_countsstill streams every byte from0tocommitted, per key, on every boot — the real cost behind the 91-second phase-2 stall. Persistingthe committed line count beside the offset would turn that into a small file read, but it changes
the on-disk offset format on the hottest write path and deserves its own PR and its own review.
The batch amplifier is untouched. A per-row failure inside
_write_batchstill fails the wholechunk, and
_handle_exhausted_batchstill dead-letters healthy neighbours and commits past them.Quarantining the offending row is the durable fix and touches queue-commit semantics.
Addendum — two additions after running it on a real spool
The first version of this PR was tested but never run. Deploying it to a live server with a
2.12 GiB spool found a real limitation, which these two additions close.
What running it revealed
The per-session trim fires in the drain loop's idle branch — so it only ever reaches a session
that currently has a live drain worker. Crash recovery respawns a worker only for a session
with undrained data. A session that fully drained and then went away therefore gets no worker,
never reaches the idle branch, and keeps its files forever — and every later boot's recovery
re-walks them.
Measured on the local box, after the per-session trim had already run and reclaimed everything it
could reach:
The largest files on disk were precisely the ones the trim could not see.
Addition 1 —
QueueManager.reclaim_drained_orphans()Called from
_startup_recovery_body, immediately after the respawn loop. Iterates every workerkey and delegates to
delete_drained, which is what makes it safe: idempotent, takes the key'sfile_lock, refuses cheaply while any uncommitted byte remains, keeps.dead.jsonl, andunlinks a stale
.offsetso a log recreated later cannot read past its own end. A key whosedrainer is mid-drain is skipped by
delete_draineditself — no exclusion list, and no way to racea drainer into losing data.
Ordering is deliberate: it runs after
recovery_seed_counts, so the conservation baseline isread from disk before anything is removed, and after the respawn loop, so a session with
pending data already has its drainer. Cost is O(keys), never O(bytes) — one
statfor thereclaimed size plus
delete_drained's own stat and offset read. Wrapped in try/except: a diskreclaim must never fail a boot.
Addition 2 —
spool_trimmedraised from DEBUG to INFOOn the live box, 16 files were trimmed and the journal showed nothing — the log sat at DEBUG
while the server ran at INFO. A reclaim an operator cannot confirm is happening may as well not be
instrumented.
Evidence from the live deploy
Installed on the local server (systemd unit, real 1.43 GiB spool):
1.43 GiB → 796 bytes, with
recovery_complete: True,recovery_error: None, and zeroTraceback/dead_letter/flush_chunk_failed/level=ERRORlines.The
/statussnapshot converging afterwards is itself a demonstration ofas_of_secondsworkingas intended — a stale snapshot is labelled stale rather than presented as live:
Tests for the additions
Nine new tests, all nine verified RED against the previous commit (
e58bbcd):test_reclaim_drained_orphans_removes_fully_drained_key(1, log_size)returntest_reclaim_drained_orphans_keeps_dead_letters.dead.jsonlsurvives and stays readabletest_reclaim_drained_orphans_skips_key_with_uncommitted_bytestest_reclaim_drained_orphans_ignores_dead_letter_only_keytest_reclaim_drained_orphans_mixed_directory(2, sum), pending untouchedtest_reclaim_drained_orphans_per_key_failure_does_not_abort_passOSErrorkey doesn't stop the passtest_startup_recovery_logs_reclaimed_orphanstest_startup_recovery_orphan_reclaim_failure_does_not_fail_boottest_idle_drained_session_trim_logs_at_infolevelno == INFO, not merely presentThe last one went red for exactly the right reason on the old source: no INFO
spool_trimmedrecord existed, only
drain_worker_cancelled— confirming the trim really was invisible at INFO.Final:
pytest -m "not neo4j"→ 2008 passed, 7 skipped, 92 deselected.pytest -m neo4j(live containers) → 92 passed. Version stays 6.7.5.