fix(db): agent_sync_state ints are bounded at their own column's ceiling — git_dir_bytes BIGINT (#2800), int4 coercion (#2827)

[dolho]

Fixes #2800
Fixes #2827

Problem

agent_sync_state.git_dir_bytes was declared INTEGER for PostgreSQL (tables.py Integer + Alembic 0019), i.e. int4 with a 2 GiB ceiling. The column exists to observe workspace-repo bloat (#1596), so the values it records are exactly the ones that overflow: any agent whose .git passes 2,147,483,647 bytes made every SyncHealthService upsert raise psycopg2.errors.NumericValueOutOfRange: integer out of range. SQLite is 64-bit, so the default backend never showed it.

Reproduced pre-fix against a disposable postgres:16-alpine:

FAILED tests/unit/test_1596_git_sync_observability.py::TestGitDirBytesRoundTrip::test_upsert_and_read_git_dir_bytes[postgres]
E   psycopg2.errors.NumericValueOutOfRange: integer out of range

Fix (dual-track, Invariant #9)

Track	Change
db/schema.py	git_dir_bytes BIGINT — single source of truth; init_schema_postgres translates the same string, so fresh PG gets int8 via 0001_baseline
db/tables.py	BigInteger
Alembic 0063_agent_sync_state_git_dir_bytes_bigint (off 0062_execution_fan_out_task_id, dev's head)	ALTER TABLE agent_sync_state ALTER COLUMN git_dir_bytes TYPE BIGINT; downgrade narrows honestly (fails on a >2 GiB row rather than truncating)
SQLite	No migration, on purpose — a comment beside _migrate_agent_sync_state_git_dir_bytes in db/migrations.py says why: INTEGER and BIGINT are one 64-bit affinity there, so an upgraded file declaring INTEGER and a fresh one declaring BIGINT store identical values, and the schema-parity suite cannot observe this column's declared type (both fixtures build from empty). The first version shipped a #1160 rename-swap rebuild on the claim that the guard would otherwise go red; a one-line negative control disproved it (2627928).

Regression seam (AC #4)

TestGitDirBytesRoundTrip is now @pytest.mark.requires_postgres, so the schema-parity PostgreSQL tier (#2434) runs its [postgres] leg. Added:

• test_git_dir_bytes_is_64_bit_on_postgres — asserts information_schema reports bigint (names the type, not a stack)
• TestGitDirBytesNeedsNoSqliteMigration — pins that no SQLite entry is registered for the widening, and proves a pre-bug: agent_sync_state.git_dir_bytes is a 32-bit INTEGER on PostgreSQL — .git over 2 GiB breaks the sync-state upsert #2800 file declaring INTEGER stores a 44 GiB value

Byte-column audit

agent_shared_files.size_bytes stays Integer: bounded by MAX_FILE_SIZE_BYTES (50 MB) at its only writer, cannot reach int4's ceiling by construction. No other byte-count columns in tables.py.

Boundary ceilings (#2827, folded in)

Widening git_dir_bytes fixed one column, but the boundary that feeds all eight was still wrong in the other direction: _coerce_nonneg_int admitted anything up to INT8_MAX for every column, while pack_count / loose_objects / maintenance_failures / ahead_main / behind_main / ahead_working / behind_working are int4 — so a value the boundary admitted but the column could not hold made the whole upsert raise NumericValueOutOfRange and the agent's sync health went dark (a raised pack_count also drops the git_dir_bytes reading beside it). The four ahead/behind counters went in uncoerced.

• Default ceiling is now INT4_MAX; git_dir_bytes and the lock-report ints opt into INT8_MAX explicitly.
• _coerce_counter bounds the four ahead/behind counters (and the legacy ahead/behind keys) the same way — rejected → 0, the column default.
• Tests drive INT4_MAX + 1 through _sync_agent per column (int4 → NULL/0, BIGINT admitted), pin that INT4_MAX itself lands, and round-trip each column's admitted maximum through SyncStateOperations on both backends (requires_postgres leg is the proof boundary and column agree).
• Mutation: default ceiling back to INT8_MAX + ahead_main left raw → 3 red (test_every_column_is_bounded_by_its_own_postgres_type, test_the_legacy_ahead_key_is_coerced_too, test_values).

Verification

• pytest unit/test_1595_sync_health_signals.py unit/test_1596_git_sync_observability.py unit/test_2742_sync_health_leader_lock.py unit/test_sync_health_service.py unit/test_73_sync_health_bulk.py → 94 passed, 1 skipped
• pytest tests/unit/test_1596_git_sync_observability.py tests/unit/test_schema_parity.py tests/unit/test_alembic_parity_guard.py tests/unit/test_alembic_revision_id_length.py with TEST_POSTGRES_URL → 104 passed, 1 skipped (skip = sqlite leg of the PG-only type assertion)
• pytest tests/unit -m requires_postgres with TEST_POSTGRES_URL → 32 passed, 2 skipped
• pytest tests/unit -k "migration or schema or sync_state or sync_health or 1596 or 389 or alembic" → 362 passed, 5 skipped
• Real Alembic path on postgres:16: upgrade 0062 → force column to INTEGER → upgrade head → information_schema.data_type = bigint, INSERT … 47244640256 lands; alembic current = 0063 (head)
• scripts/ci/check_alembic_heads.py → 64 revisions, 1 head
• scripts/ci/check_alembic_parity.py origin/dev HEAD → PASS

🤖 Generated with Claude Code

[dolho]

/review Report

Branch: fix/2800-git-dir-bytes-bigint → dev · 7 files (+245/−5)
Scope: CLEAN — diff is exactly #2800's four ACs plus the byte-column audit the issue asked for
Plan Completion: 4/4 AC done (BIGINT on PG · #1596 test green on both backends · 1 Alembic head · PG leg now requires_postgres-marked)

Non-breaking verification (both tracks, real artifacts — not the test harness)

SQLite — upgrade of a legacy file

1. Pre-fix code (dev @ 8d9af18be) built a DB: git_dir_bytes INTEGER, 116 migrations recorded; seeded 3 agent_sync_state rows (44 GiB value / small / all-NULL).
2. New code init_database() booted over it twice:
   • declared BIGINT, siblings still INTEGER; migration recorded; no orphan _new table; PRAGMA integrity_check = ok; foreign_key_check empty
   • .dump rows before vs after → byte-identical; idx_sync_state_status re-created
   • boot 2 = no-op (idempotent)
3. SyncStateOperations on the upgraded file: 1 TiB upsert, partial update preserves prior value, list_all OK.

PostgreSQL — pg_dump of a live instance into disposable postgres:16

• Restored at 0061_execution_open_canvas, git_dir_bytes = integer, 6 agents / 122 executions
• New code init_database() twice: alembic_version → 0062, column bigint, pack_count/loose_objects/consecutive_failures still int4 (only the byte column moved), index present, all rows intact, 44 GiB upsert lands.

Critical Findings

None.

Informational Findings

[I1] Migration data safety — rename-swap drops columns it doesn't name (Confidence 8/10) — fixed in 077ba3e43
The SQLite rebuild copied a hardcoded 17-column list and DROPped the old table; an unknown column would vanish silently. No path produces one today (every agent_sync_state writer sits earlier in MIGRATIONS; enterprise never ALTERs OSS tables), but the failure mode of that shape is data loss, so it now compares live PRAGMA table_info against _AGENT_SYNC_STATE_REBUILD_COLUMNS and raises before touching anything — surfaces as first_pending in the /health 503 (#1160). Pinned by test_refuses_to_drop_an_unknown_column.

[I2] Sibling int4 columns behind a 2⁶³ boundary guard (Confidence 5/10 — noted, not changed)
pack_count / loose_objects pass _coerce_nonneg_int (< 2**63) into PG INTEGER. Same class in principle, but 2.1 B packs / loose objects is unreachable (each is ≥ one file). Left alone.

Clean Categories

• SQL safety: SQLAlchemy Core only; migration DDL is static strings, no interpolation
• Concurrency: SQLite runner under the migration_lock flock, PG under the Alembic multi-worker deadlock on PG first boot (migration_lock did not serialize) #1425 advisory lock; runs at boot before SyncHealthService starts
• Auth / credentials: no endpoints, no secrets touched
• §4.14 fix completeness: all git_dir_bytes writers checked — one (sync_health_service.py:156), its guard was already int8-sized; no int32 clamp anywhere in the path
• Tests: PG-only leg now a required gate; information_schema bigint assertion; rebuild rows/index/idempotent/no-op-pre-#1596/refuse-unknown
• Docs: architecture/database.md + feature-flows/git-sync-health.md updated
• Alembic: check_alembic_heads.py → 1 head; check_alembic_parity.py origin/dev HEAD → PASS; downgrade() honestly narrows (fails on >2 GiB rows rather than a truncating CAST)

Learnings

docs/memory/learnings.md gained an entry (69f31efcf): second int4-on-PG occurrence after #2434 → durable class — growth columns (bytes / ms / counters) are BigInteger from day one; a db_backend test is not a PG gate without requires_postgres; widening a type on the dual track is not a SQLite no-op because schema-parity compares declared types.

Summary

• Critical: 0
• Informational: 2 (1 fixed in-PR, 1 noted)
• Scope: clean

🤖 Generated with Claude Code

[dolho]

/review — head 69f31efc vs merge-base eb41896f5 (second pass, after the 10:00 self-review)

Files: 8 (+288/−5) · Scope: CLEAN — the four #2800 artefacts (SQLite rebuild, Alembic 0062, schema.py, tables.py) plus tests and the three doc lines.
Merge state: CONFLICTING, and git merge-tree against dev names exactly one file — docs/memory/learnings.md (the routine keep-both class from the train's Phase 4). No code conflict. Alembic 0062 still chains off 0061_execution_open_canvas, which is dev's head today, so no second-head hazard from the drift so far — re-check scripts/ci/check_alembic_heads.py on the merged tree if anything lands under versions/ before this does.

Critical

None.

Informational

[I1] The SQLite rebuild's column list is a second copy of the table (Confidence 5/10)
db/migrations.py _AGENT_SYNC_STATE_REBUILD_COLUMNS + the inline CREATE TABLE agent_sync_state_new are the table declared a third time (after schema.py and tables.py). The refuse-if-unknown guard (test_refuses_to_drop_an_unknown_column) makes the drop direction safe — a later ADD COLUMN that this list does not know raises before touching anything rather than silently losing data — which is the right failure. It cannot catch a DEFAULT/NOT NULL divergence between the inline DDL and schema.py, but the schema-parity job compares declared types on a fresh vs upgraded file and would. Nothing to change; recording that the guard's coverage is "drops", not "drift".

Clean

• The PG fix is one statement — ALTER COLUMN … TYPE BIGINT, no USING, int4→int8 lossless; downgrade is the honest narrowing that fails on a >2 GiB row rather than truncating. 0001_baseline reuses schema.py, so fresh PG is BIGINT and the ALTER is a no-op there.
• SQLite is a declared-type-only change by construction (INTEGER affinity is 64-bit either way); the rebuild exists for the parity guard, and the docstring says so.
• Rebuild is _atomic_rebuild (fix(db): migration runner — DROP-rebuild data-loss window and no cross-process serialization #1160 rename-swap inside BEGIN/COMMIT), index re-created, FK preserved, idempotent (PRAGMA table_info reads BIGINT after; second call returns early — test_rebuild_redeclares_bigint_and_preserves_rows calls it twice).
• Tests execute the migration, not its source: three SQLite cases drive _migrate_agent_sync_state_git_dir_bytes_bigint against an in-memory table (rebuild + preservation, refusal, pre-column no-op), and test_git_dir_bytes_is_64_bit_on_postgres is requires_postgres-marked for the pg-migrations job.
• Inline -- #2800: … ; … comment inside the schema.py DDL — an existing pattern (schema.py:224, :534 already carry ; inside -- comments and translate to PG), so not a new hazard.
• No auth / credential / concurrency surface.

Verdict: READY — needs the learnings.md keep-both merge from dev and nothing else.

[github-actions]

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

[github-actions]

🚧 Alembic head check could not run — this PR conflicts with dev.

git merge-tree reported conflicts, so there is no merged tree to check. GitHub cannot compute refs/pull/N/merge in this state either, which is why a conflicting PR shows no checks at all.

Merge dev into this branch and push. The head check re-runs automatically on the next push to dev touching src/backend/migrations/versions/**.

Advisory — this check does not block merge. · head_sha: 69f31efcffc5abdf288516cc5c5b47a077216b94 · run

[vybe]

merge-train: not on this train — rides the next one

The diagnosis is right and both migration tracks are genuinely present and tested. Two things to fix first.

1. Live #2068 Alembic fork — upgrade head would apply zero revisions

0062_agent_sync_state_git_dir_bytes_bigint declares down_revision = "0061_execution_open_canvas". dev's head moved to 0062_execution_fan_out_task_id at 14:58 today (7b3cf20a8, #2532), which declares the same parent. Both files are even named 0062_.

Against the merged tree (origin/dev + this PR):

alembic-heads: FAIL — resolves to 2 heads across 64 revision(s); exactly 1 is required.
  • 0062_agent_sync_state_git_dir_bytes_bigint
  • 0062_execution_fan_out_task_id
They fork at: 0061_execution_open_canvas

This isn't your error — #2532 landed after you branched, and on the PR branch alone the check correctly reports 63 revision(s), 1 head, which is what you ran. It's the #2533 staleness window: all 23 checks here are green because they predate the 14:58 push, schema-parity's unconditional single-head step included.

Worth knowing: the backstop built for exactly this, alembic-head-watch, did fire at 14:58 and posted PR #2805 conflicts with dev — head check not evaluated. It was blocked by the unrelated learnings.md conflict, so nothing in CI is currently reporting the fork. Resolving that conflict is what un-blocks the guard that catches this.

Fix: rebase on dev, re-chain to down_revision = "0062_execution_fan_out_task_id", rename to 0063_agent_sync_state_git_dir_bytes_bigint.

2. The SQLite rebuild's stated justification is false under a negative control

The PR body, the docstring at db/migrations.py:3204-3211, and the new learnings.md entry all assert that a bare SQLite no-op "leaves upgraded files at INTEGER against a fresh BIGINT and turns that guard red forever."

Negative control: with the MIGRATIONS registration removed, tests/unit/test_schema_parity.py → 4 passed. Root cause is that both parity fixtures build from empty, and run_all_migrations on an empty DB never creates agent_sync_state at all (table_exists=False after pass 1 — the migration self-records as applied via its own early return, so pass 2 skips it), leaving init_schema to create the table at BIGINT in both snapshots. The guard is structurally blind to this column's declared type.

So as written, a DROP TABLE + rename-swap of a live production table runs at boot on every existing SQLite install for a CI benefit that doesn't exist. The mechanics are sound — _atomic_rebuild is transactional with rollback, PRAGMA foreign_keys is never ON, nothing FK-references the table, idx_sync_state_status is recreated, the unknown-column guard raises before touching anything, and it's idempotent — so this is about the rationale, not safety. But learnings.md is a durable file /autoplan reads as fact, so it shouldn't land carrying a claim a one-line control disproves. Either drop the rebuild or state the real reason.

Not blocking, worth a follow-up issue

services/sync_health_service.py:156-161 runs four fields through one _coerce_nonneg_int bounded at < 2**63, but only git_dir_bytes was widened — pack_count, loose_objects, maintenance_failures stay Integer. And :170-173 passes ahead_main/behind_main/ahead_working/behind_working uncoerced from the agent payload into int4 columns. _coerce_nonneg_int's own docstring names a "compromised/prompt-injected agent" as the threat model, so the identical NumericValueOutOfRange stays reachable through seven other columns in the same upsert. Pre-existing, and your own rule ("if the coercion allows < 2**63 the column must too") is the one that catches it.

Also: body says Related to #2800 — needs Fixes #2800 or the issue strands in status-in-progress.

🤖 Generated with Claude Code

[dolho]

Both addressed in 26279283f (after ce91353ac, the dev merge — learnings.md keep-both, the only conflict).

1. Alembic fork — re-chained: 0063_agent_sync_state_git_dir_bytes_bigint with down_revision = "0062_execution_fan_out_task_id". check_alembic_heads.py on this head: 64 revision(s), 1 head — PASS. Resolving the conflict also unblocks alembic-head-watch, so CI now reports the graph rather than "not evaluated".

2. The SQLite rebuild — dropped, not re-justified. You were right that the parity claim was false, and once it is false there is no reason left for a boot-time DROP TABLE of a live table: SQLite's INTEGER affinity already holds the 44 GiB value (the new test_a_pre_2800_sqlite_file_stores_a_64_bit_value_unchanged pins that), so the SQLite track deliberately carries no migration. A note beside _migrate_agent_sync_state_git_dir_bytes says why, test_no_sqlite_migration_is_registered_for_the_widening keeps it that way for a reason rather than by omission, and the Alembic docstring's "mirrors the SQLite migration" sentence now says the opposite. The learnings.md lesson (3) is rewritten to the thing actually learned — run the negative control before writing a guard's behaviour into a durable file — with the rebuild's unknown-column rule kept as the conditional for a case that genuinely needs one.

Follow-up filed: #2827 — the seven sibling columns of the same upsert (pack_count/loose_objects/maintenance_failures coerced to < 2**63 into int4; the four ahead/behind counts uncoerced), with your rule as the AC.

Body now says Fixes #2800.

[dolho]

Pushed 2d711a4 — folds #2827 into this PR (body + title updated, Fixes #2827 added).

Widening git_dir_bytes closed the overflow on one column; #2827 is the same class from the other side — the boundary admitted INT8_MAX for every column while seven of them are int4, and the four ahead/behind counters were not coerced at all. Now every column is bounded at its own ceiling; details in the new Boundary ceilings section.

Mutation evidence: default ceiling back to INT8_MAX + ahead_main written raw → test_every_column_is_bounded_by_its_own_postgres_type, test_the_legacy_ahead_key_is_coerced_too, test_values all red. Suite: 94 passed, 1 skipped.

Also corrected the stale 0062 (head) in Verification — after the rechain the head is 0063, 64 revisions.

[dolho]

/review Report

Branch: fix/2800-git-dir-bytes-bigint → dev (merge-base 0bddbc959)
Files Changed: 10 (+251/−13)
Scope: CLEAN — #2800 (column width) + #2827 (boundary ceilings), both named in the body; docs/learnings edits are the tiered-doc obligation for a schema change.
Plan Completion: #2827 3/3 DONE · #2800 3 DONE / 1 CHANGED

AC	Status	Evidence
#2827 one bound per column	DONE	sync_health_service.py:110 ceiling=INT4_MAX default; :433 git_dir_bytes opts into INT8_MAX; :448-451 _coerce_counter on the four ahead/behind
#2827 requires_postgres max round-trip ×8	DONE	test_1596_git_sync_observability.py::test_every_int_column_round_trips_its_own_maximum (inside the @pytest.mark.requires_postgres class, line 38)
#2827 out-of-bound → None, never driver raise	DONE	test_1595::test_every_column_is_bounded_by_its_own_postgres_type
#2800 BigInteger + Alembic ALTER; SQLite "recorded no-op"	CHANGED	tables.py:928, 0063_*.py. SQLite track carries a comment beside _migrate_agent_sync_state_git_dir_bytes (migrations.py:3211), not a registered no-op entry — deliberate (2627928): a registered entry would write a schema_migrations row that changes nothing. Deviation is visible in the diff and the revision docstring.
#2800 test_1596 both backends	DONE	see suite run below
#2800 single head	DONE	check_alembic_heads.py → 64 revisions, 1 head (0063)
#2800 PG tier runs it	DONE	test_1596 class marked requires_postgres

Critical Findings

None.

Informational Findings

[I1] Silent rejection at the boundary (Confidence: 7/10)
File: src/backend/services/sync_health_service.py:130-140
Issue: _coerce_nonneg_int / _coerce_counter reject without any log, and on an existing row the db layer's _merged (db/sync_state.py:101-104) keeps the prior value for a None — so an agent reporting pack_count: 2**40 now shows as a stale-but-plausible number with no operator signal (before this PR it raised, which was worse but at least loud on PG). Verified live: an out-of-range pack_count on a populated row left 20 in place. Pre-existing posture for strings/negatives; not a regression.
Suggestion: one throttled WARNING per (agent, field) at rejection — field name only, never the value (agent-authored). Follow-up-sized; not blocking.

[I2] Body wording (Confidence: 9/10)
"rejected → NULL/0" is exact for a fresh row; for an existing row the counters go to 0 (they pass through or 0) but pack_count/loose_objects/git_dir_bytes retain the prior value via _merged. The tests assert on fresh rows, so they are correct; the prose slightly overstates.

[I3] The parity guard is blind in this direction (Confidence: 9/10)
check_alembic_parity.py origin/dev HEAD → "no net-new MIGRATIONS entry — nothing to guard (pass)". It guards SQLite-change-without-Alembic, not Alembic-change-without-SQLite, so the dual-track claim here is proven by the local legacy-boot run below, not by CI. Already recorded in the learnings entry (item 3).

Clean Categories

• SQL safety: op.execute carries a literal DDL string, no interpolation; ALTER under the Alembic multi-worker deadlock on PG first boot (migration_lock did not serialize) #1425 advisory lock.
• Concurrency: int4→int8 rewrite of a one-row-per-agent table; second boot is a no-op (verified).
• Auth / credentials: none touched; diff --check clean, secret-pattern grep empty.
• Producer compatibility: the agent emits ahead+ahead_main as equal ints (agent_server/routers/git.py:1048-1053), so _coerce_counter's first-present-key rule vs the old or-chain is not observable.
• Sibling audit: size_bytes bounded at the writer (50 MB); the five *_ms Integer columns are bounded by construction (max execution timeout 7200 s, loop max_duration ≤ 7 d ≪ int4's 24.8 d).
• Docs: architecture/database.md + feature-flows/git-sync-health.md say BIGINT.

Backward-compatibility — tested locally, real boot path both tracks

Method: a worktree at the merge-base (pre-fix code) builds the DB through the real init_database() and seeds agent_sync_state; the PR head then boots twice over that same DB via init_database(); then a live SyncHealthService._sync_agent run with legacy and modern payloads.

SQLite (TRINITY_DB_PATH)

• Legacy file: 130 migrations, git_dir_bytes declared INTEGER, a 3 GiB value already stored (64-bit affinity).
• New code boot ×2: still 130 migrations (no SQLite migration, as designed), column still INTEGER, .dump before/after byte-identical (diff rc 0), integrity_check ok, idx_sync_state_status present.
• Live: pre-P6 payload (ahead/behind only) → (5, 2); 44 GiB → lands; INT8_MAX → lands; pack_count = INT4_MAX+1 → no raise, prior retained; ahead_main = INT4_MAX+1 → 0.

PostgreSQL (postgres:16, throwaway DBs)

• Legacy code boot: alembic_version = 0062, all nine int columns integer.
• New code boot: alembic_version = 0063, git_dir_bytes = bigint, the other eight still integer; pg_dump --data-only before/after identical; second boot idempotent.
• Negative control: the pre-fix code on that legacy DB, 44 GiB upsert → psycopg2.errors.NumericValueOutOfRange: integer out of range (the bug: agent_sync_state.git_dir_bytes is a 32-bit INTEGER on PostgreSQL — .git over 2 GiB breaks the sync-state upsert #2800 symptom, reproduced, then gone on the PR head).
• Fresh DB, new code from empty: 0063, bigint (baseline reuses schema.py DDL; the ALTER is a no-op there).
• Downgrade honesty: downgrade 0062 with an INT8_MAX row → fails integer out of range, transaction rolled back (alembic_version stays 0063, column stays bigint); with in-range rows → 0062/integer; upgrade head → 0063/bigint.

Suites with TEST_POSTGRES_URL set (both legs): test_1595, test_1596, test_sync_health_service, test_73_sync_health_bulk, test_2742, test_schema_parity, test_alembic_parity_guard, test_alembic_revision_id_length, test_2533 → 285 passed, 1 skipped (the sqlite leg of the PG-only type assertion).

Summary

• Critical: 0
• Informational: 3 — I1 is a small follow-up (a boundary WARNING), I2/I3 are wording/awareness.
• Scope: clean. Backward compatibility retained on both tracks; verdict from my side: mergeable.

[vybe]

merge-train: merged dev into this branch to resolve the docs/memory/learnings.md append collision left by the previous train member (keep both entries, theirs first). Mechanical; no other file touched.

[vybe]

merge-train: batch validated on train/20260916-0919 (train PR #2839)