Skip to content

TC-822/TC-1491: follow-ups to #376 (bounded reads) - #383

Open
samgbafa wants to merge 12 commits into
mainfrom
tc1491/p-followups
Open

samgbafa wants to merge 12 commits into
mainfrom
tc1491/p-followups

Conversation

@samgbafa

Copy link
Copy Markdown
Contributor

Stacked on #376 (base is tc1491/p-bounded-reads, not main, because #376 has not merged yet; retarget to main once it does). No migrations, no schema change.

Fixes from the #376 review:

  1. CacheFileSink::commit removes all staged temps when a rename fails (test: non-empty directory at either destination).
  2. Boot/crash orphans: <file>.tmp.<pid>.<n> staging files beside the SQL and DuckDB cache files are swept before a hydration begins. Only that exact name pattern; files this process still has in flight are tracked and never swept (also covers DuckDB import_db). Tests: orphans removed; live, finished-but-uncommitted, malformed and other-database names kept.
  3. tc1274_a_held_operation_lock... warms the databases before the 200 ms bounds apply, so it asserts the lock wait, not disk speed; tc1274_a_hydration_past_its_own_bound_is_retired restores default bounds for the fresh hydration.
  4. Removed the dead SqlSizes seeded flag.
  5. read_chunks doc corrected for SQLite (one blob transiently; CPU O(n^2/chunk)).
  6. Nits: slice gate also checks the part's size; sink reset on Ok(None); MemorySink reserves the part size via a new size_hint; exhausted ArtifactChanged is SqlError::Unavailable (nothing written, retry safe). DuckDbError has no retryable variant and adding one touches every route mapping, so DuckDB stays Internal (commented).

Also removed two unused imports in duckdb_diagnostics_policy.rs (clippy with the duckdb feature).

Tests: full tinycloud-core suite with --features duckdb and node-server lib pass; sql::service / database_artifacts / duckdb lib tests run 3x; clippy -D warnings clean with and without duckdb.

Naomi and others added 7 commits October 19, 2026 18:44
Replace DatabaseArtifactRepository::load/load_sized with load_meta (metadata
columns only) and read_chunks (4 MiB substr slices of payload then
delta_payload, guarded by revision + checkpoint hash, hashes verified while
streaming) into an ArtifactSink. SQL and DuckDB hydration stream into
staged cache files (published only after the read verified); export and the
Forbid-growth size check read metadata only. Drops the bulk-read scope.

Adds docs/runbooks/tc-822-artifact-payload-storage.md (Postgres STORAGE
EXTERNAL operator step).
The slice statement was not on the read-only call allowlist, so a lost
connection mid-slice was not retried (blackhole_mid_hydration CI failure).
- CacheFileSink::commit removes every staged temp when a rename fails
- sweep stale <file>.tmp.<pid>.<n> staging files before a SQL/DuckDB hydration
  (live in-process staging files are registered and never swept)
- tc1274 lock-wait / hydration-bound tests no longer time a cold hydration
- drop the dead SqlSizes seeded flag
- read_chunks doc: SQLite memory bound is one blob, CPU O(n^2/chunk)
- slice-size gate also checks the part's own size; read_artifact resets the
  sink on Ok(None); MemorySink reserves the part size; exhausted
  ArtifactChanged maps to SqlError::Unavailable (DuckDbError has no such
  variant, stays Internal)
@samgbafa
samgbafa changed the base branch from tc1491/p-bounded-reads to main October 19, 2026 20:51
@samgbafa samgbafa closed this Oct 19, 2026
@samgbafa samgbafa reopened this Oct 19, 2026
@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-822 (reviewed exact head 83e3beb…; scope 5ccc305..83e3beb)

Note: the literal range 5ccc305..83e3beb also contains the merged main (#370/#374/#375/#380); the follow-up's own change is the single commit 83e3beb (5 files, +266/-45), which is what I reviewed. No data-loss or corruption path found.

(a) New tests fail without their fixes (built once, tinycloud-core --lib, then reverted each fix in a scratch worktree, removed afterwards):

  • a_failed_commit_leaves_no_staged_temp_files: passes at head; with the old commit it fails (main.db: ["main.db", "main.db-wal.tmp.<pid>.1"], the leaked WAL temp). Only the main.db case discriminates: for main.db-wal the old code already cleaned up.
  • a_hydration_sweeps_temp_files_a_crashed_one_left_behind: passes at head; fails with the sweep_stale_staging call removed from SqlService::hydrate.
  • the_sweep_removes_only_orphaned_staging_files passes; the live-file half is the only thing that exercises the registry (without it the pid-matching live temps would be swept).
  • tc1274 tests: not deterministic-failing by nature. Statically: the lock-wait test now warms a and b under default bounds, so the 200 ms bound only covers the lock wait, plus a warm b write against the in-memory SQLite repo. It still asserts Busy(LockWait) and the sql_busy: lock_wait: text, b unaffected, and rows==1 after release, so the property is intact. hydration_past_its_own_bound_is_retired keeps hydration_max=200ms for the gated first attempt (deterministic timeout) and resets to default bounds before the fresh hydration; loads()==2 and assert_no_temp_files stay.

(b) Sweep vs live hydration: safe in-process.

  • staging_path registers the path before File::create; release happens after rename, on remove, or in discard/Drop; the begin_part create-failure and sync_all-failure paths release too. A file that exists on disk is therefore always registered while live, so the sweep cannot take it. The duckdb import_db temp is registered with StagingRelease (RAII).
  • PID reuse (including container pid 1 after a crash): the registry is keyed by full path, not pid, and the sweep runs before the new sink allocates a name, so a reused <pid>.<n> orphan is removed before it could collide.
  • Pattern is exact: <file>.tmp.<digits>.<digits> for the DB file and its -wal/.wal sibling only; main.db.tmp.1, x.db.tmp.abc.def, tmpx, other databases and DuckDB's <temp>.wal do not match.
  • Two node processes on one cache dir: process B's sweep will delete A's in-flight temps. Outcome is A's rename failing with ENOENT, so A's hydration fails (Internal), never a wrong database. That topology is already unsupported (remove_*_cache_files and the actors' by-path files collide the same way; compose runs one container). Non-blocking, worth a doc line.

(c) ArtifactChanged → Unavailable: safe. In SQL the only producer is the read_artifact call in hydrate (read-only, no write sent). handle() returns the error at once; the only in-node loop (MAX_HYDRATIONS) iterates on Ready only. persist_write's save/save_delta/load_meta cannot yield ArtifactChanged, and the write_may_have_applied → OutcomeUnknown mapping is unchanged. No automatic retry loop in the node; the retry is the client's, via 503 + Retry-After. Note each request costs up to 3 full reads (ARTIFACT_READ_RETRIES) against a hot writer. DuckDB stays Internal as documented.

Non-blocking nits

  1. MemorySink::size_hint does reserve_exact(part size) from unverified row metadata before any byte is hashed. A corrupt/inconsistent *_size_bytes now becomes an allocation abort/capacity overflow panic in DuckDB export, where before it was a SizeMismatch error. Cap the hint (e.g. min(bytes, BASE_TIMEOUT_PAYLOAD_BYTES)).
  2. The lock-wait test still has one residual 200 ms-bounded step (the second b write, persisted to the in-memory repo): far less fsync-sensitive than before, but not zero. Could use default bounds for that write.
  3. commit uses staged.remove(0) (O(n), n ≤ 2): fine.

CI green; reviewed statically plus the targeted test runs above.

This branch has not been deployed

No deployments
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.

1 participant