CivicTechTO / CivicTechTO/toronto-bids

ariba: a re-capture can silently overwrite a fuller bundle — _MIN_CAPTURE_RATIO does not protect it

Open
#199 1 comment 0 reactions 0 assignees View on GitHub
bug ready-for-agent
Dominant language
Python
Stars
7
Forks
2
PR merge metrics
No merged PRs in 30d

Description

Found while testing a fix for #183 against production. This is a **safety-mechanism gap**, not a capture bug — it applies to every event, not just the one that exposed it.

## What happened

Re-capturing `Doc5713434353` (which already held a good 37-of-39 bundle) produced only 20 documents. `capture_files` accepted it and wrote a **347 MB** bundle over the existing **826 MB** one. 18 documents lost.

`_MIN_CAPTURE_RATIO` (#182) did not fire: **20/39 = 51%**, just above the 50% floor.

It was caught only because the original had been *moved aside* rather than deleted before the test. On an unattended nightly there would have been no signal at all — `Doc.zip` existing is the whole of what `capture_attachments` reads as "archived", and Respond dies when the posting closes, so no later run could ever repair it.

## Why the existing guard can't catch this

`_MIN_CAPTURE_RATIO` compares **this run's downloads against this run's own traversal listing**. That is the right check for what it was built for (#182: a broken run that captured 1 of 39), and it should stay.

But it is blind to the one thing that matters when a bundle already exists: **is this capture better or worse than what it is about to replace?** Nothing in `capture_files` reads the existing `Doc.zip` before overwriting it. A run that legitimately traverses 39 files and legitimately downloads 20 of them looks healthy by every check present.

## Why a re-capture happens at all

Normally it does not — `capture_attachments` skips any event whose `Doc.zip` exists, which is why this went unnoticed. It becomes reachable whenever the bundle is removed deliberately: recovering documents from a known gap record (exactly what #183's test was doing), a manual re-run after a parser fix, or any future retry-the-gaps mechanism. The moment such a mechanism exists, this becomes a live hazard rather than a latent one.

## Options

1. **Refuse to shrink a bundle.** Before `build_bundle` overwrites an existing `Doc.zip`, read its entry count (cheap — central directory only, `index_zip` already does this) and refuse if the new capture holds fewer, keeping partials and leaving the event pending. Strictly additive to the existing floor.
2. **Merge rather than replace.** Take the union of what is on disk and what this run captured, since both are real bytes that may be unrepeatable. More faithful to the archive's "never lose bytes" ethos, but changes bundle identity in a way that needs thought.
3. **Never overwrite; write beside and promote.** Heavier, but makes the replace step explicitly reversible.

Option 1 is the smallest change that closes the hole and is consistent with how every other guard here is framed (refuse, keep partials, stay pending, say so).

## Note

Whatever is chosen should be verified against a large event, not a small one — that is precisely the generalisation error that produced the regression in the first place (see the #183 re-scope).

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with capture_files and its callers, then trace how capture_attachments invokes build_bundle and how index_zip reads an existing archive's central directory. Reproduce the 37-of-39 versus 20-of-39 scenario on a large event, and verify that a recapture cannot replace a fuller bundle while partials remain available and the event stays pending.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.