Skip to content

🐛 Fix: a read after a commit is not uncommitted work - #202

Draft
medazizktata25 wants to merge 3 commits into
Guepard-Corp:mainfrom
medazizktata25:fix/checkout-read-is-not-uncommitted-work
Draft

medazizktata25 wants to merge 3 commits into
Guepard-Corp:mainfrom
medazizktata25:fix/checkout-read-is-not-uncommitted-work

Conversation

@medazizktata25

@medazizktata25 medazizktata25 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Related Issue

None filed.

What

After a commit, reading the database is enough to make the next checkout refuse:

the workspace has uncommitted changes that checkout would overwrite (base/5/1249)

nothing having written to it. A user hits it like this: insert rows, commit, run a SELECT count(*), switch to another branch and back. The switch back is refused on the table's file. postgres_checkout_lifecycle fails at step 06 on main (d5a0e83) for the same reason.

Cause. The first read of a row after its inserting transaction commits sets PostgreSQL hint bits in the row header and dirties the page, which reaches disk at the next checkpoint or shutdown. The dirty check compares size and mtime against the commit's file list, so it reports the page. base/5/1249 is pg_attribute. Measured when the fix was written (see the first commit message): the bytes that change are all the HEAP_XMIN_COMMITTED bit of t_infomask, on page 16; the page LSN and checksum are unchanged.

How

Three commits:

  1. fix(checkout): a read after a commit is not uncommitted work.
    • A provider can now say whether a changed file holds the same data as the commit's snapshot copy of it (DatabaseProvider::holds_same_data; the default keeps today's answer).
    • PostgreSQL compares relation main forks page by page. On heap pages only, it disregards exactly what PostgreSQL itself masks as hints when it checks a replayed page (bufmask.c / heap_mask): pd_lsn, pd_checksum, pd_prune_xid, the PD_* hint flags, and the four xmin/xmax hint bits.
    • Index pages, other forks, other files and any other byte are still compared exactly, and an unreadable file still counts as changed.
  2. fix(test): assert on what gfs checkout printed, not on what libtest printed.
    • cli_runner::run_gfs captures stdout by redirecting the whole process's stdout, so libtest's "has been running for over 60 seconds" notice could become a checkout's output.
    • The four checkouts in postgres_checkout_lifecycle now run gfs as a subprocess, and their assertions are unconditional.
    • The new run_gfs_subprocess_checked counts only exit 0 as success.
  3. fix(branch): drop the unreachable guard #180 left in branch -d, and say why.
    • delete_branch refuses the reserved name detached on its first line, so the guard A branch named 'detached' destroyed every detached working copy #180 added after it is dead code; this removes it.
    • A regression test recreates a repository whose detached branch was made by an older gfs, and asserts the deletion is refused and the detached working copy survives.

Review Guide

  1. crates/adapters/db-providers/src/pg_page.rs: the page comparison and its masks, against PostgreSQL's heap_mask.
  2. crates/domain/src/usecases/repository/checkout_repo_usecase.rs: where the dirty check asks the provider.
  3. crates/domain/src/ports/database_provider.rs and postgresql.rs: the new provider method.
  4. Tests: cli/tests/e2e_checkout_after_read.rs (new), cli/tests/common/cli_runner.rs, e2e_checkout_postgres.rs, e2e_checkout.rs.

Testing

On Rust 1.93.1, macOS with Colima for Docker, using gfs-postgres:16/:17 images built from this tree.

  • postgres_checkout_lifecycle: all 6 steps pass, 59.6 s. On main it fails at step 06 with the refusal above.
  • e2e_checkout_after_read (new): passes.
    • It reads after a commit, then switches branches.
    • It also asserts that a real same-length UPDATE is still refused.
  • Calibration: with holds_same_data forced to false, both e2e tests fail. The lifecycle test fails with exactly the original uncommitted changes … (base/5/1249). Restored, both pass.
  • pg_page unit tests 6/6, domain checkout tests 10/10, e2e_checkout 11/11 (including the new detached regression test).
  • Full cargo test --locked: 888 passed, 1 failed, 3 ignored, across 60 test binaries. The failure is clone_range_and_temporal_hydration_elide, which fails identically on main; its fix is on its own branch.
  • cargo fmt --check clean. cargo clippy --locked --all-targets --all-features -- -D warnings clean, run on Linux (rust:1.93.1-bookworm).
  • Not verified in this run:
    • The libtest-notice failure that commit 2 fixes needs the test to cross 60 s, and this run took 59.6 s, so the fix wasn't exercised here.
    • The detached regression test was not checked against a deliberately loosened validation.
  • Not tested:
    • Docker Desktop, podman, a Linux Docker host, Windows, Kubernetes;
    • providers other than Postgres (they keep the old answer);
    • pages autovacuum changes on its own (pruning, visibility map), which are not hints and are still refused.

Documentation

  • Comments on the masks in pg_page.rs name the PostgreSQL source they mirror.

User Impact

  • Fixed: reading a database after a commit no longer blocks the next checkout.
  • Unchanged: real writes are still refused, and --force still discards them.

Checklist

  • Self-review completed
  • Tests pass locally (cargo test). One fails, identically on main; see Testing.
  • Clippy passes (cargo clippy --all-targets --all-features -- -D warnings), on Linux.
  • Format check passes (cargo fmt --check)
  • This PR can be safely reverted.

`postgres_checkout_lifecycle` fails at step 06 on main: `gfs checkout main`
is refused with "uncommitted changes ... (base/5/1249)" although nothing wrote
to the database. The same refusal reaches users: insert rows, commit, run a
`SELECT count(*)`, switch to another branch and back, and the switch back is
refused on the table's file.

Cause, measured. base/5/1249 is pg_attribute. Replaying the test's steps with
the gfs binary and recording main/0 after each one, the file is unchanged by
the restore at step 04 and changed by the stop at step 05: same size, 7 bytes
different, all on page 16, every one the HEAP_XMIN_COMMITTED bit of t_infomask
(0x0100) on the attribute rows pgbench's CREATE TABLEs inserted (xmin 739/740).
Page LSN and checksum unchanged. In the user scenario, 1000 bytes of the
table's file changed, all of them that same bit, one per row read.

That is a PostgreSQL hint bit: the first read of a row after its inserting
transaction commits records the commit in the row header and dirties the page,
which reaches disk at the next checkpoint or shutdown. The dirty check compares
size and mtime against the commit's file list, so it reports the page.

Not a recent regression: the same replay against e51fc61 (before Guepard-Corp#191, Guepard-Corp#193,
Guepard-Corp#194, Guepard-Corp#180, Guepard-Corp#183, Guepard-Corp#184, Guepard-Corp#186, Guepard-Corp#196) refuses at the same step on the same file.

Fix. The provider now says whether a changed file holds the same data as the
commit's snapshot copy of it. PostgreSQL compares relation main forks page by
page and, on heap pages only, disregards what PostgreSQL itself masks as hints
when it checks a replayed page (bufmask.c / heap_mask): pd_lsn, pd_checksum,
pd_prune_xid, the PD_* hint flags and the four xmin/xmax hint bits. Index pages,
other forks, other files and any other byte are still compared exactly, and an
unreadable file still counts as changed. Other providers keep today's answer.

Still refused, by design or not yet handled: real writes (the new e2e test
asserts a same-length UPDATE is refused), and physical changes autovacuum makes
on its own (pruning, visibility map), which are not hints.
…rinted

cli_runner::run_gfs captures stdout by redirecting the whole process's stdout
with gag, so anything else printed while a command runs lands in that
command's output. postgres_checkout_lifecycle runs past 60 s on a loaded
machine, and libtest's "test postgres_checkout_lifecycle has been running for
over 60 seconds" notice then became the checkout's stdout:

  checkout stdout should show switched; got: test postgres_checkout_lifecycle
  has been running for over 60 seconds

The test only passed at all because each "Switched to" assertion sat behind
`if !stdout.is_empty()`, an escape hatch for in-process capture losing gfs's
own output.

The four checkouts in that test now run gfs as a subprocess, whose stdout
holds only what gfs printed, and the assertions are unconditional. The new
run_gfs_subprocess_checked counts only exit 0 as success; run_gfs_subprocess
counts 0, 1 and 2 (for schema diff), which would have let a failed checkout
pass "checkout should succeed".
…ch -d, and say why

Guepard-Corp#180 reserved the branch name `detached` and also added, in `branch -d`, a
branch that leaves `workspaces/detached` in place and prints a notice through a
bare println!. Its comment says a repository that already carries a `detached`
branch still reaches it. It cannot: `delete_branch` calls
`validate_branch_name` on its first line, which refuses `detached` (any case,
any `detached/...`) before anything else runs, and it is the only caller.

Run with the real CLI, on a repository whose `detached` branch was created by a
gfs from before Guepard-Corp#180 (e51fc61), with an uncommitted file in a detached working
copy:

    $ gfs branch -d detached     (82070fa)
    error: Invalid repository layout: branch name 'detached' is reserved ...
    rc=1                          (DETACHED, detached/x: the same)

and the file survives. So the guard and its println! are dead code; removed, and
the comment now names the check that does the protecting.

A regression test recreates that repository (the ref is the same 64-byte file
the older gfs wrote) and asserts the deletion is refused and the detached
working copy survives, so loosening the validation fails it.

Not changed: such a branch cannot be deleted at all with this gfs, which is
safe but leaves the user no way to remove the name.

This branch is waiting to be deployed

1 waiting deployment
pr-validation — 92fc5f49 Waiting Oct 27, 2026 by medazizktata25 via build-and-test (windows-latest) #297
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