alkblobs/tasks/window-family-smells.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
window-family-smells	The window batch — N-2 transactional staging, N-3 bounded take, N-4 forward-only deadlines (sqlite + postgres + pg_lo)	completed		moderate	medium	component	implementation	wave-9 review-001 correctness window-family

Description

The window-family smell batch from review-001, kept at decomposition (the plan's §Wave 9 left this keep-or-defer here; the call: all three are small, correctness-adjacent, thematically unified, and no later wave owns them — wave 10 is security, 11 fuzzing, 12 release). All three engines host the window family and get all three fixes: src/sqlite.rs:513/546, src/postgres/windows.rs:38/65, src/pg_lo/windows.rs:86/113.

• N-2 — window_stage is not transactional in any engine: per-candidate autocommit; a mid-loop failure leaves a partially staged window (self-healing by re-mark, but the slow shape and the partial-state window are real). Make staging transactional: one transaction per window_stage call per engine (all candidates commit or none do).
• N-3 — window_take is unbounded (DELETE … RETURNING): a huge overdue backlog materializes entirely in memory in one step. Bound the take (a LIMITed delete; the batch size is a recorded design point — a named const, not a knob) and loop the sweep executor's consumption (src/store/sweep.rs:632 — the single window_take(SystemTime::now()) call) until the take returns empty/short, preserving the one-sweep-executes-all-overdue semantics the sweep report's counts assume.
• N-4 — a re-stage at an earlier deadline moves visible_after backwards (unconditional upsert on conflict): potentially accelerates deletion beyond the embedder's staging intent; the recorded semantics only admit forward moves. The conflict update keeps the later deadline (forward-only; GREATEST semantics or the equivalent conditional update per engine's SQL dialect).

Test reconciliation: the existing per-engine window tests pin the implemented behavior — e.g. window_stage_is_idempotent_and_restage_updates_the_deadline (sqlite.rs:1127, postgres/windows.rs:156, pg_lo/windows.rs:773) and the take tests. N-4 corrects the semantics: restage-at-a-later- deadline still moves forward; restage-at-an-earlier-deadline no longer moves backwards. Update the tests to pin both directions (and rename where the name now overstates). New tests: the transactional stage (a mid-loop failure leaves zero staged rows — injectable per engine's harness), the bounded take + executor loop (a backlog larger than the batch drains across loop iterations with correct report counts).

The gc.rs seam doc's staging contract text is checked against the landed behavior while here (review-001 N-10 flagged a drift batch; the staging-idempotency clause was corrected already — verify the transactional/bounded changes don't reopen it).

Live harness

The postgres and pg-lo window shapes run against the local dockerized postgres container (pglo-poc, postgres:16-alpine on :15432, running for dev/testing):

export ALKBLOBS_PG_HOST=localhost
export ALKBLOBS_PG_PORT=15432
export ALKBLOBS_PG_USER=postgres
export ALKBLOBS_PG_PASSWORD=poc
export ALKBLOBS_PG_DB=blobs

Server-less, the pg tests skip cleanly; acceptance requires green against the live harness. The sqlite column runs server-less.

Acceptance Criteria

• window_stage transactional in all three engines (mid-loop failure stages nothing; tested per engine)
• window_take bounded (the batch const recorded); the sweep executor loops until drained with correct report counts (backlog-larger-than-batch tested)
• Forward-only deadlines: restage at an earlier deadline does not move visible_after backwards (tested per engine); the existing restage tests reconciled
• The gc.rs seam doc's staging clauses still tell the truth
• Gates green: cargo test (default), cargo test --all-features server-less, cargo test --all-features + live harness, clippy --all-targets -- -D warnings across the feature variants, cargo fmt --check

References

• docs/reviews/001-storage-surface-general-review.md (N-2, N-3, N-4; N-10's staging-doc clause)
• docs/plans/implementation.md (§Wave 9 — "the window batch … kept or deferred at decomposition")
• src/sqlite.rs, src/postgres/windows.rs, src/pg_lo/windows.rs (the three hosts)
• src/store/sweep.rs (the executor's window_take consumption)
• src/gc.rs (the window-family seam contract)
• tasks/engine-sqlite-window.md, tasks/engine-pg-lo-window.md (the window tests' existing shapes)

Notes

Implementation decisions taken while here:

• The batch bound lives as WINDOW_TAKE_BATCH: usize = 100 in src/gc.rs beside the EngineWindowState trait — engine-neutral, consumed by all three engines and the sweep executor; recorded as the named design point (review-001 N-3), not a knob. Sized equal to the sweep body's SWEEP_DELETE_BATCH so one take feeds one execution batch; the executor's old per-take chunk loop collapsed into the drain loop.
• The bounded take stays a single atomic statement per engine: sqlite's DELETE … WHERE key IN (SELECT … ORDER BY visible_after LIMIT ?2) RETURNING key keeps the one-statement select+remove property without needing sqlite's compile-time-gated DELETE … LIMIT; the pg engines' twin form binds the LIMIT as i64 (the server infers bigint for LIMIT's parameter). Take order is oldest-deadline-first.
• Transactional staging's failure-path discipline mirrors the engines' existing one-tx bodies: sqlite rides guard.transaction() (rollback on drop covers the injected mid-loop failure); the pg engines ride the session-level BEGIN/COMMIT with an explicit ROLLBACK on every error arm (pin_commit's shape — deadpool's Fast recycle never resets tx state, so no path may strand the open tx). The injected failure fires on the last candidate (candidates.last() compare — no an enumerate index that vanishes under non-test builds) via a #[cfg(test)] fail_window_stage knob on all three backends, mirroring the fail_commit knob family.
• The pg upsert's conflict update references the target through an insert alias (INSERT INTO {t} AS w … DO UPDATE SET visible_after = GREATEST(w.visible_after, excluded.visible_after)) — a bare visible_after is ambiguous in the DO-UPDATE scope (verified live); sqlite carries the same semantics with its max(a, b) scalar. A nullable visible_after row (the schema tolerates it) never occurs — every staging write is explicit.
• Test reconciliation: the three engines' restage tests renamed to window_stage_is_idempotent_and_restage_moves_the_deadline_forward_only with the earlier-deadline pin added; new per-engine tests for the transactional stage (window_stage_mid_loop_failure_stages_nothing) and the bounded take (window_take_is_bounded_across_the_overdue_backlog); two store-level drains (sqlite durable + postgres fleet: *_backlog_larger_than_the_take_batch_drains_in_one_sweep) pin the executor loop's all-drained report counts over a batch+13 backlog.
• Harness plumbing touched: Harness gained DerefMut (pg engine + pg-lo engine testing) so the window tests can flip the injection knob; the per-backend fail_window_stage field threaded through the existing cfg(test) struct literals.
• gc.rs seam doc checked against the landed behavior: the window_stage clause now states the transactional + forward-only contract, the window_take clause the bounded-take + drain-loop contract (review-001 N-10's drift does not reopen).

Summary

The window batch (review-001 N-2/N-3/N-4) is landed across sqlite, postgres, and pg-lo: staging is one transaction per window_stage call (mid-loop failure stages nothing, tested per engine with an injectable failure); window_take is bounded to the recorded WINDOW_TAKE_BATCH const (oldest-deadline-first) and the sweep executor loops the take until drained, with store-level backlog-larger-than-batch drain tests on both the sqlite durable form and the postgres fleet form pinning exact report counts; a re-stage at an earlier deadline no longer moves visible_after backwards (forward-only GREATEST/max semantics, tested per engine, existing restage tests renamed and reconciled). The gc.rs seam doc's staging/take clauses were updated to state the landed contracts. All gates green: cargo test (137+54+4), cargo test --all-features both server-less (300+125+4) and against the live dockerized harness at default parallelism, clippy -D warnings across the default and all-features variants, cargo fmt --check.